diff mbox series

[1/2] hw: timer: Add ASPEED RTC device

Message ID 20190328062222.8409-2-joel@jms.id.au
State Superseded
Headers show
Series arm: aspeed: Add RTC Model | expand

Commit Message

Joel Stanley March 28, 2019, 6:22 a.m. UTC
Signed-off-by: Joel Stanley <joel@jms.id.au>

Signed-off-by: Cédric Le Goater <clg@kaod.org>

---
 hw/timer/Makefile.objs        |   2 +-
 hw/timer/aspeed_rtc.c         | 157 ++++++++++++++++++++++++++++++++++
 hw/timer/trace-events         |   4 +
 include/hw/timer/aspeed_rtc.h |  31 +++++++
 4 files changed, 193 insertions(+), 1 deletion(-)
 create mode 100644 hw/timer/aspeed_rtc.c
 create mode 100644 include/hw/timer/aspeed_rtc.h

-- 
2.20.1

Comments

Peter Maydell April 11, 2019, 3:25 p.m. UTC | #1
On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote:
>

> Signed-off-by: Joel Stanley <joel@jms.id.au>

> Signed-off-by: Cédric Le Goater <clg@kaod.org>

> ---

>  hw/timer/Makefile.objs        |   2 +-

>  hw/timer/aspeed_rtc.c         | 157 ++++++++++++++++++++++++++++++++++

>  hw/timer/trace-events         |   4 +

>  include/hw/timer/aspeed_rtc.h |  31 +++++++

>  4 files changed, 193 insertions(+), 1 deletion(-)

>  create mode 100644 hw/timer/aspeed_rtc.c

>  create mode 100644 include/hw/timer/aspeed_rtc.h

>

> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs

> index 0e9a4530f848..123d92c9692c 100644

> --- a/hw/timer/Makefile.objs

> +++ b/hw/timer/Makefile.objs

> @@ -41,7 +41,7 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o

>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o

>

>  common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o

> -common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o

> +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o

>

>  common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o

>  common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o

> diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c

> new file mode 100644

> index 000000000000..daccf00eccdc

> --- /dev/null

> +++ b/hw/timer/aspeed_rtc.c

> @@ -0,0 +1,157 @@

> +/*

> + * ASPEED Real Time Clock

> + * Joel Stanley <joel@jms.id.au>

> + *

> + * Copyright 2019 IBM Corp

> + * SPDX-License-Identifier: GPL-2.0-or-later

> + */

> +

> +#include "qemu/osdep.h"

> +#include "qemu-common.h"

> +#include "hw/timer/aspeed_rtc.h"

> +#include "qemu/log.h"

> +#include "qemu/timer.h"

> +

> +#include "trace.h"

> +

> +#define COUNTER1        (0x00 / 4)

> +#define COUNTER2        (0x04 / 4)

> +#define ALARM           (0x08 / 4)

> +#define CONTROL         (0x10 / 4)

> +#define ALARM_STATUS    (0x14 / 4)


Not mandatory, but you might like the REG32() macro in
hw/registerfields.h which defines A_FOO and R_FOO
constants for you for the addresses and the indexes.

> +

> +#define RTC_UNLOCKED    BIT(1)

> +#define RTC_ENABLED     BIT(0)

> +

> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

> +{

> +    struct tm tm;

> +    uint32_t year, cent;

> +    uint32_t reg1 = rtc->reg[COUNTER1];

> +    uint32_t reg2 = rtc->reg[COUNTER2];

> +

> +    tm.tm_mday = (reg1 >> 24) & 0x1f;

> +    tm.tm_hour = (reg1 >> 16) & 0x1f;

> +    tm.tm_min = (reg1 >> 8) & 0x3f;

> +    tm.tm_sec = (reg1 >> 0) & 0x3f;


Shift by zero ?

Consider using extract32() rather than by-hand shift and mask.

> +

> +    cent = (reg2 >> 16) & 0x1f;

> +    year = (reg2 >> 8) & 0x7f;

> +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;

> +    tm.tm_year = year + (cent * 100) - 1900;

> +

> +    rtc->offset = qemu_timedate_diff(&tm);

> +}

> +

> +static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)

> +{

> +    uint32_t year, cent;

> +    struct tm now;

> +

> +    qemu_get_timedate(&now, rtc->offset);

> +

> +    switch (r) {

> +    case COUNTER1:

> +        return (now.tm_mday << 24) | (now.tm_hour << 16) |

> +            (now.tm_min << 8) | now.tm_sec;

> +    case COUNTER2:

> +        cent = (now.tm_year + 1900) / 100;

> +        year = now.tm_year % 100;

> +        return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |

> +            ((now.tm_mon + 1) & 0xf);

> +    default:

> +        abort();


More usually written g_assert_not_reached().

> +    }

> +}

> +

> +static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,

> +                                unsigned size)

> +{

> +    AspeedRtcState *rtc = opaque;

> +    uint64_t val;

> +    uint32_t r = addr >> 2;

> +

> +    switch (r) {

> +    case COUNTER1:

> +    case COUNTER2:

> +        if (rtc->reg[CONTROL] & RTC_ENABLED) {

> +            rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);

> +        }


If this is deliberately going to fall through to the next
case then it should have a /* fall through */ comment
(ditto in the write fn).

> +    case ALARM:

> +    case CONTROL:

> +    case ALARM_STATUS:

> +        val = rtc->reg[r];

> +        break;

> +    default:

> +        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);

> +        return 0;

> +    }

> +

> +    trace_aspeed_rtc_read(addr, val);

> +

> +    return val;

> +}


> +static void aspeed_rtc_class_init(ObjectClass *klass, void *data)

> +{

> +    DeviceClass *dc = DEVICE_CLASS(klass);

> +

> +    dc->realize = aspeed_rtc_realize;


This is missing a reset function and vmstate.

thanks
-- PMM
Cédric Le Goater April 11, 2019, 3:30 p.m. UTC | #2
On 4/11/19 5:25 PM, Peter Maydell wrote:
> On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote:

>>

>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>> Signed-off-by: Cédric Le Goater <clg@kaod.org>

>> ---

>>  hw/timer/Makefile.objs        |   2 +-

>>  hw/timer/aspeed_rtc.c         | 157 ++++++++++++++++++++++++++++++++++

>>  hw/timer/trace-events         |   4 +

>>  include/hw/timer/aspeed_rtc.h |  31 +++++++

>>  4 files changed, 193 insertions(+), 1 deletion(-)

>>  create mode 100644 hw/timer/aspeed_rtc.c

>>  create mode 100644 include/hw/timer/aspeed_rtc.h

>>

>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs

>> index 0e9a4530f848..123d92c9692c 100644

>> --- a/hw/timer/Makefile.objs

>> +++ b/hw/timer/Makefile.objs

>> @@ -41,7 +41,7 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o

>>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o

>>

>>  common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o

>> -common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o

>> +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o

>>

>>  common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o

>>  common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o

>> diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c

>> new file mode 100644

>> index 000000000000..daccf00eccdc

>> --- /dev/null

>> +++ b/hw/timer/aspeed_rtc.c

>> @@ -0,0 +1,157 @@

>> +/*

>> + * ASPEED Real Time Clock

>> + * Joel Stanley <joel@jms.id.au>

>> + *

>> + * Copyright 2019 IBM Corp

>> + * SPDX-License-Identifier: GPL-2.0-or-later

>> + */

>> +

>> +#include "qemu/osdep.h"

>> +#include "qemu-common.h"

>> +#include "hw/timer/aspeed_rtc.h"

>> +#include "qemu/log.h"

>> +#include "qemu/timer.h"

>> +

>> +#include "trace.h"

>> +

>> +#define COUNTER1        (0x00 / 4)

>> +#define COUNTER2        (0x04 / 4)

>> +#define ALARM           (0x08 / 4)

>> +#define CONTROL         (0x10 / 4)

>> +#define ALARM_STATUS    (0x14 / 4)

> 

> Not mandatory, but you might like the REG32() macro in

> hw/registerfields.h which defines A_FOO and R_FOO

> constants for you for the addresses and the indexes.


Yes. May be we should start using these macros in all our models.

> 

>> +

>> +#define RTC_UNLOCKED    BIT(1)

>> +#define RTC_ENABLED     BIT(0)

>> +

>> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

>> +{

>> +    struct tm tm;

>> +    uint32_t year, cent;

>> +    uint32_t reg1 = rtc->reg[COUNTER1];

>> +    uint32_t reg2 = rtc->reg[COUNTER2];

>> +

>> +    tm.tm_mday = (reg1 >> 24) & 0x1f;

>> +    tm.tm_hour = (reg1 >> 16) & 0x1f;

>> +    tm.tm_min = (reg1 >> 8) & 0x3f;

>> +    tm.tm_sec = (reg1 >> 0) & 0x3f;

> 

> Shift by zero ?

> 

> Consider using extract32() rather than by-hand shift and mask.


What about the FIELD*() macros in hw/registerfields.h ?

Thanks,

C.

> 

>> +

>> +    cent = (reg2 >> 16) & 0x1f;

>> +    year = (reg2 >> 8) & 0x7f;

>> +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;

>> +    tm.tm_year = year + (cent * 100) - 1900;

>> +

>> +    rtc->offset = qemu_timedate_diff(&tm);

>> +}

>> +

>> +static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)

>> +{

>> +    uint32_t year, cent;

>> +    struct tm now;

>> +

>> +    qemu_get_timedate(&now, rtc->offset);

>> +

>> +    switch (r) {

>> +    case COUNTER1:

>> +        return (now.tm_mday << 24) | (now.tm_hour << 16) |

>> +            (now.tm_min << 8) | now.tm_sec;

>> +    case COUNTER2:

>> +        cent = (now.tm_year + 1900) / 100;

>> +        year = now.tm_year % 100;

>> +        return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |

>> +            ((now.tm_mon + 1) & 0xf);

>> +    default:

>> +        abort();

> 

> More usually written g_assert_not_reached().

> 

>> +    }

>> +}

>> +

>> +static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,

>> +                                unsigned size)

>> +{

>> +    AspeedRtcState *rtc = opaque;

>> +    uint64_t val;

>> +    uint32_t r = addr >> 2;

>> +

>> +    switch (r) {

>> +    case COUNTER1:

>> +    case COUNTER2:

>> +        if (rtc->reg[CONTROL] & RTC_ENABLED) {

>> +            rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);

>> +        }

> 

> If this is deliberately going to fall through to the next

> case then it should have a /* fall through */ comment

> (ditto in the write fn).

> 

>> +    case ALARM:

>> +    case CONTROL:

>> +    case ALARM_STATUS:

>> +        val = rtc->reg[r];

>> +        break;

>> +    default:

>> +        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);

>> +        return 0;

>> +    }

>> +

>> +    trace_aspeed_rtc_read(addr, val);

>> +

>> +    return val;

>> +}

> 

>> +static void aspeed_rtc_class_init(ObjectClass *klass, void *data)

>> +{

>> +    DeviceClass *dc = DEVICE_CLASS(klass);

>> +

>> +    dc->realize = aspeed_rtc_realize;

> 

> This is missing a reset function and vmstate.

> 

> thanks

> -- PMM

>
Philippe Mathieu-Daudé April 11, 2019, 4:36 p.m. UTC | #3
Hi Peter,

On 4/11/19 5:30 PM, Cédric Le Goater wrote:
> On 4/11/19 5:25 PM, Peter Maydell wrote:

>> On Thu, 28 Mar 2019 at 06:22, Joel Stanley <joel@jms.id.au> wrote:

>>>

>>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>

>>> ---

>>>  hw/timer/Makefile.objs        |   2 +-

>>>  hw/timer/aspeed_rtc.c         | 157 ++++++++++++++++++++++++++++++++++

>>>  hw/timer/trace-events         |   4 +

>>>  include/hw/timer/aspeed_rtc.h |  31 +++++++

>>>  4 files changed, 193 insertions(+), 1 deletion(-)

>>>  create mode 100644 hw/timer/aspeed_rtc.c

>>>  create mode 100644 include/hw/timer/aspeed_rtc.h

>>>

>>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs

>>> index 0e9a4530f848..123d92c9692c 100644

>>> --- a/hw/timer/Makefile.objs

>>> +++ b/hw/timer/Makefile.objs

>>> @@ -41,7 +41,7 @@ obj-$(CONFIG_MC146818RTC) += mc146818rtc.o

>>>  obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o

>>>

>>>  common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o

>>> -common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o

>>> +common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o

>>>

>>>  common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o

>>>  common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o

>>> diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c

>>> new file mode 100644

>>> index 000000000000..daccf00eccdc

>>> --- /dev/null

>>> +++ b/hw/timer/aspeed_rtc.c

>>> @@ -0,0 +1,157 @@

>>> +/*

>>> + * ASPEED Real Time Clock

>>> + * Joel Stanley <joel@jms.id.au>

>>> + *

>>> + * Copyright 2019 IBM Corp

>>> + * SPDX-License-Identifier: GPL-2.0-or-later

>>> + */

>>> +

>>> +#include "qemu/osdep.h"

>>> +#include "qemu-common.h"

>>> +#include "hw/timer/aspeed_rtc.h"

>>> +#include "qemu/log.h"

>>> +#include "qemu/timer.h"

>>> +

>>> +#include "trace.h"

>>> +

>>> +#define COUNTER1        (0x00 / 4)

>>> +#define COUNTER2        (0x04 / 4)

>>> +#define ALARM           (0x08 / 4)

>>> +#define CONTROL         (0x10 / 4)

>>> +#define ALARM_STATUS    (0x14 / 4)

>>

>> Not mandatory, but you might like the REG32() macro in

>> hw/registerfields.h which defines A_FOO and R_FOO

>> constants for you for the addresses and the indexes.

> 

> Yes. May be we should start using these macros in all our models.

> 

>>

>>> +

>>> +#define RTC_UNLOCKED    BIT(1)

>>> +#define RTC_ENABLED     BIT(0)

>>> +

>>> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

>>> +{

>>> +    struct tm tm;

>>> +    uint32_t year, cent;

>>> +    uint32_t reg1 = rtc->reg[COUNTER1];

>>> +    uint32_t reg2 = rtc->reg[COUNTER2];

>>> +

>>> +    tm.tm_mday = (reg1 >> 24) & 0x1f;

>>> +    tm.tm_hour = (reg1 >> 16) & 0x1f;

>>> +    tm.tm_min = (reg1 >> 8) & 0x3f;

>>> +    tm.tm_sec = (reg1 >> 0) & 0x3f;

>>

>> Shift by zero ?

>>

>> Consider using extract32() rather than by-hand shift and mask.

> 

> What about the FIELD*() macros in hw/registerfields.h ?

> 

> Thanks,

> 

> C.

> 

>>

>>> +

>>> +    cent = (reg2 >> 16) & 0x1f;

>>> +    year = (reg2 >> 8) & 0x7f;

>>> +    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;

>>> +    tm.tm_year = year + (cent * 100) - 1900;

>>> +

>>> +    rtc->offset = qemu_timedate_diff(&tm);

>>> +}

>>> +

>>> +static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)

>>> +{

>>> +    uint32_t year, cent;

>>> +    struct tm now;

>>> +

>>> +    qemu_get_timedate(&now, rtc->offset);

>>> +

>>> +    switch (r) {

>>> +    case COUNTER1:

>>> +        return (now.tm_mday << 24) | (now.tm_hour << 16) |

>>> +            (now.tm_min << 8) | now.tm_sec;

>>> +    case COUNTER2:

>>> +        cent = (now.tm_year + 1900) / 100;

>>> +        year = now.tm_year % 100;

>>> +        return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |

>>> +            ((now.tm_mon + 1) & 0xf);

>>> +    default:

>>> +        abort();

>>

>> More usually written g_assert_not_reached().

>>

>>> +    }

>>> +}

>>> +

>>> +static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,

>>> +                                unsigned size)

>>> +{

>>> +    AspeedRtcState *rtc = opaque;

>>> +    uint64_t val;

>>> +    uint32_t r = addr >> 2;

>>> +

>>> +    switch (r) {

>>> +    case COUNTER1:

>>> +    case COUNTER2:

>>> +        if (rtc->reg[CONTROL] & RTC_ENABLED) {

>>> +            rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);

>>> +        }

>>

>> If this is deliberately going to fall through to the next

>> case then it should have a /* fall through */ comment

>> (ditto in the write fn).

>>

>>> +    case ALARM:

>>> +    case CONTROL:

>>> +    case ALARM_STATUS:

>>> +        val = rtc->reg[r];

>>> +        break;

>>> +    default:

>>> +        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);

>>> +        return 0;

>>> +    }

>>> +

>>> +    trace_aspeed_rtc_read(addr, val);

>>> +

>>> +    return val;

>>> +}

>>

>>> +static void aspeed_rtc_class_init(ObjectClass *klass, void *data)

>>> +{

>>> +    DeviceClass *dc = DEVICE_CLASS(klass);

>>> +

>>> +    dc->realize = aspeed_rtc_realize;

>>

>> This is missing a reset function and vmstate.


Is vmstate mandatory for new devices?

>>

>> thanks

>> -- PMM

>>

> 

>
Peter Maydell April 11, 2019, 4:55 p.m. UTC | #4
On Thu, 11 Apr 2019 at 17:37, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>

> Hi Peter,

>

> On 4/11/19 5:30 PM, Cédric Le Goater wrote:

> > On 4/11/19 5:25 PM, Peter Maydell wrote:

> >> This is missing a reset function and vmstate.

>

> Is vmstate mandatory for new devices.


Yes, I think so. You can have vmstate that says
"this can't be migrated" if you absolutely must, but
it's generally easy enough to make it properly migratable.
If the device generally has no state to migrate it
should at least provide a comment saying so.
(I think we should add a way to say this in a vmstate,
so that we can then assert that every device sets its
vmsd pointer to something.)

Failing to provide a vmstate means you have a machine
which will silently break if the user tries to do
savevm/loadvm, which is not very nice.

thanks
-- PMM
Joel Stanley April 12, 2019, 3:17 a.m. UTC | #5
Thanks for the review Peter and Cedric.

On Thu, 11 Apr 2019 at 15:30, Cédric Le Goater <clg@kaod.org> wrote:
>

> On 4/11/19 5:25 PM, Peter Maydell wrote:

> >> +

> >> +#define COUNTER1        (0x00 / 4)

> >> +#define COUNTER2        (0x04 / 4)

> >> +#define ALARM           (0x08 / 4)

> >> +#define CONTROL         (0x10 / 4)

> >> +#define ALARM_STATUS    (0x14 / 4)

> >

> > Not mandatory, but you might like the REG32() macro in

> > hw/registerfields.h which defines A_FOO and R_FOO

> > constants for you for the addresses and the indexes.

>

> Yes. May be we should start using these macros in all our models.


I don't like them as you can no longer jump between the definition and
the use of the defines.

>

> >

> >> +

> >> +#define RTC_UNLOCKED    BIT(1)

> >> +#define RTC_ENABLED     BIT(0)

> >> +

> >> +static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)

> >> +{

> >> +    struct tm tm;

> >> +    uint32_t year, cent;

> >> +    uint32_t reg1 = rtc->reg[COUNTER1];

> >> +    uint32_t reg2 = rtc->reg[COUNTER2];

> >> +

> >> +    tm.tm_mday = (reg1 >> 24) & 0x1f;

> >> +    tm.tm_hour = (reg1 >> 16) & 0x1f;

> >> +    tm.tm_min = (reg1 >> 8) & 0x3f;

> >> +    tm.tm_sec = (reg1 >> 0) & 0x3f;

> >

> > Shift by zero ?

> >

> > Consider using extract32() rather than by-hand shift and mask.


I looked at these and found them more confusing than writing what was
actually happening.

This code came from the Linux kernel driver, which I wrote, so I know
it's correct. The shift by zero is there to follow the pattern of the
code proceeding it, again to stop mistakes.

If we require using these helper macros then I can make the change. If
it's optional then I would prefer to leave it as is.

>

> What about the FIELD*() macros in hw/registerfields.h ?

>


> >> +static void aspeed_rtc_class_init(ObjectClass *klass, void *data)

> >> +{

> >> +    DeviceClass *dc = DEVICE_CLASS(klass);

> >> +

> >> +    dc->realize = aspeed_rtc_realize;

> >

> > This is missing a reset function and vmstate.


Ok, I can add those.

vmstate and migration is a foreign concept for the small ARM machines.
As well as being a developer, I am an end user of Qemu for them (the
aspeeds, microbit), and the use case is to boot up a firmware and
check that it does correct thing. I've never considered saving the
state and resming it, and can't think of a situation where that would
be done.

If we're adding it for consistency then I understand. I don't think it
sees any testing though.

Cheers,

Joel
Peter Maydell April 12, 2019, 11:55 a.m. UTC | #6
On Fri, 12 Apr 2019 at 04:17, Joel Stanley <joel@jms.id.au> wrote:
> vmstate and migration is a foreign concept for the small ARM machines.

> As well as being a developer, I am an end user of Qemu for them (the

> aspeeds, microbit), and the use case is to boot up a firmware and

> check that it does correct thing. I've never considered saving the

> state and resming it, and can't think of a situation where that would

> be done.


It's a really useful debugging tool:
 https://translatedcode.wordpress.com/2015/07/06/tricks-for-debugging-qemu-savevm-snapshots/

thanks
-- PMM
Cédric Le Goater April 15, 2019, 6:13 a.m. UTC | #7
On 4/12/19 1:55 PM, Peter Maydell wrote:
> On Fri, 12 Apr 2019 at 04:17, Joel Stanley <joel@jms.id.au> wrote:

>> vmstate and migration is a foreign concept for the small ARM machines.

>> As well as being a developer, I am an end user of Qemu for them (the

>> aspeeds, microbit), and the use case is to boot up a firmware and

>> check that it does correct thing. I've never considered saving the

>> state and resming it, and can't think of a situation where that would

>> be done.

> 

> It's a really useful debugging tool:

>  https://translatedcode.wordpress.com/2015/07/06/tricks-for-debugging-qemu-savevm-snapshots/


yes.  Our Aspeed machines migrate quite well today and, who knows,
we might want one day to run Aspeed VMs on the "cloud". I doubt it 
but, anyway, it's really not complex to do either.

Thanks,

C.
diff mbox series

Patch

diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 0e9a4530f848..123d92c9692c 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -41,7 +41,7 @@  obj-$(CONFIG_MC146818RTC) += mc146818rtc.o
 obj-$(CONFIG_ALLWINNER_A10_PIT) += allwinner-a10-pit.o
 
 common-obj-$(CONFIG_STM32F2XX_TIMER) += stm32f2xx_timer.o
-common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o
+common-obj-$(CONFIG_ASPEED_SOC) += aspeed_timer.o aspeed_rtc.o
 
 common-obj-$(CONFIG_SUN4V_RTC) += sun4v-rtc.o
 common-obj-$(CONFIG_CMSDK_APB_TIMER) += cmsdk-apb-timer.o
diff --git a/hw/timer/aspeed_rtc.c b/hw/timer/aspeed_rtc.c
new file mode 100644
index 000000000000..daccf00eccdc
--- /dev/null
+++ b/hw/timer/aspeed_rtc.c
@@ -0,0 +1,157 @@ 
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/timer/aspeed_rtc.h"
+#include "qemu/log.h"
+#include "qemu/timer.h"
+
+#include "trace.h"
+
+#define COUNTER1        (0x00 / 4)
+#define COUNTER2        (0x04 / 4)
+#define ALARM           (0x08 / 4)
+#define CONTROL         (0x10 / 4)
+#define ALARM_STATUS    (0x14 / 4)
+
+#define RTC_UNLOCKED    BIT(1)
+#define RTC_ENABLED     BIT(0)
+
+static void aspeed_rtc_calc_offset(AspeedRtcState *rtc)
+{
+    struct tm tm;
+    uint32_t year, cent;
+    uint32_t reg1 = rtc->reg[COUNTER1];
+    uint32_t reg2 = rtc->reg[COUNTER2];
+
+    tm.tm_mday = (reg1 >> 24) & 0x1f;
+    tm.tm_hour = (reg1 >> 16) & 0x1f;
+    tm.tm_min = (reg1 >> 8) & 0x3f;
+    tm.tm_sec = (reg1 >> 0) & 0x3f;
+
+    cent = (reg2 >> 16) & 0x1f;
+    year = (reg2 >> 8) & 0x7f;
+    tm.tm_mon = ((reg2 >>  0) & 0x0f) - 1;
+    tm.tm_year = year + (cent * 100) - 1900;
+
+    rtc->offset = qemu_timedate_diff(&tm);
+}
+
+static uint32_t aspeed_rtc_get_counter(AspeedRtcState *rtc, int r)
+{
+    uint32_t year, cent;
+    struct tm now;
+
+    qemu_get_timedate(&now, rtc->offset);
+
+    switch (r) {
+    case COUNTER1:
+        return (now.tm_mday << 24) | (now.tm_hour << 16) |
+            (now.tm_min << 8) | now.tm_sec;
+    case COUNTER2:
+        cent = (now.tm_year + 1900) / 100;
+        year = now.tm_year % 100;
+        return ((cent & 0x1f) << 16) | ((year & 0x7f) << 8) |
+            ((now.tm_mon + 1) & 0xf);
+    default:
+        abort();
+    }
+}
+
+static uint64_t aspeed_rtc_read(void *opaque, hwaddr addr,
+                                unsigned size)
+{
+    AspeedRtcState *rtc = opaque;
+    uint64_t val;
+    uint32_t r = addr >> 2;
+
+    switch (r) {
+    case COUNTER1:
+    case COUNTER2:
+        if (rtc->reg[CONTROL] & RTC_ENABLED) {
+            rtc->reg[r] = aspeed_rtc_get_counter(rtc, r);
+        }
+    case ALARM:
+    case CONTROL:
+    case ALARM_STATUS:
+        val = rtc->reg[r];
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+        return 0;
+    }
+
+    trace_aspeed_rtc_read(addr, val);
+
+    return val;
+}
+
+static void aspeed_rtc_write(void *opaque, hwaddr addr,
+                             uint64_t val, unsigned size)
+{
+    AspeedRtcState *rtc = opaque;
+    uint32_t r = addr >> 2;
+
+    switch (r) {
+    case COUNTER1:
+    case COUNTER2:
+        if (!(rtc->reg[CONTROL] & RTC_UNLOCKED)) {
+            break;
+        }
+    case CONTROL:
+        rtc->reg[r] = val;
+        aspeed_rtc_calc_offset(rtc);
+        break;
+    case ALARM:
+    case ALARM_STATUS:
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx "\n", __func__, addr);
+        break;
+    }
+    trace_aspeed_rtc_write(addr, val);
+}
+
+static const MemoryRegionOps aspeed_rtc_ops = {
+    .read = aspeed_rtc_read,
+    .write = aspeed_rtc_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void aspeed_rtc_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedRtcState *s = ASPEED_RTC(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_rtc_ops, s,
+                          "aspeed-rtc", 0x18ULL);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void aspeed_rtc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aspeed_rtc_realize;
+}
+
+static const TypeInfo aspeed_rtc_info = {
+    .name          = TYPE_ASPEED_RTC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedRtcState),
+    .class_init    = aspeed_rtc_class_init,
+};
+
+static void aspeed_rtc_register_types(void)
+{
+    type_register_static(&aspeed_rtc_info);
+}
+
+type_init(aspeed_rtc_register_types)
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index dcaf3d6da6c8..db02a9142cda 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -66,6 +66,10 @@  cmsdk_apb_dualtimer_read(uint64_t offset, uint64_t data, unsigned size) "CMSDK A
 cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK APB dualtimer write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
 cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
 
+# hw/timer/aspeed-rtc.c
+aspeed_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
+aspeed_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 0x%08" PRIx64
+
 # sun4v-rtc.c
 sun4v_rtc_read(uint64_t addr, uint64_t value) "read: addr 0x%" PRIx64 " value 0x%" PRIx64
 sun4v_rtc_write(uint64_t addr, uint64_t value) "write: addr 0x%" PRIx64 " value 0x%" PRIx64
diff --git a/include/hw/timer/aspeed_rtc.h b/include/hw/timer/aspeed_rtc.h
new file mode 100644
index 000000000000..1f1155a676c1
--- /dev/null
+++ b/include/hw/timer/aspeed_rtc.h
@@ -0,0 +1,31 @@ 
+/*
+ * ASPEED Real Time Clock
+ * Joel Stanley <joel@jms.id.au>
+ *
+ * Copyright 2019 IBM Corp
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef ASPEED_RTC_H
+#define ASPEED_RTC_H
+
+#include <stdint.h>
+
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "hw/sysbus.h"
+
+typedef struct AspeedRtcState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t reg[0x18];
+    int offset;
+
+} AspeedRtcState;
+
+#define TYPE_ASPEED_RTC "aspeed.rtc"
+#define ASPEED_RTC(obj) OBJECT_CHECK(AspeedRtcState, (obj), TYPE_ASPEED_RTC)
+
+#endif /* ASPEED_RTC_H */