[16/25] hw/arm/stellaris: Convert SSYS to QOM device

Convert the SSYS code in the Stellaris boards (which encapsulates the
system registers) to a proper QOM device.  This will provide us with
somewhere to put the output Clock whose frequency depends on the
setting of the PLL configuration registers.

This is a migration compatibility break for lm3s811evb, lm3s6965evb.

We use 3-phase reset here because the Clock will need to propagate
its value in the hold phase.

For the moment we reset the device during the board creation so that
the system_clock_scale global gets set; this will be removed in a
subsequent commit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/stellaris.c | 132 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 107 insertions(+), 25 deletions(-)

Hi Peter,

On 1/21/21 8:06 PM, Peter Maydell wrote:
> Convert the SSYS code in the Stellaris boards (which encapsulates the
> system registers) to a proper QOM device.  This will provide us with
> somewhere to put the output Clock whose frequency depends on the
> setting of the PLL configuration registers.
> 
> This is a migration compatibility break for lm3s811evb, lm3s6965evb.
> 
> We use 3-phase reset here because the Clock will need to propagate
> its value in the hold phase.
> 
> For the moment we reset the device during the board creation so that
> the system_clock_scale global gets set; this will be removed in a
> subsequent commit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/stellaris.c | 132 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 107 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 652823195b1..0194ede2fe0 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -357,7 +357,12 @@ static void stellaris_gptm_realize(DeviceState *dev, Error **errp)
>  
>  /* System controller.  */
>  
> -typedef struct {
> +#define TYPE_STELLARIS_SYS "stellaris-sys"
> +OBJECT_DECLARE_SIMPLE_TYPE(ssys_state, STELLARIS_SYS)
> +
> +struct ssys_state {
> +    SysBusDevice parent_obj;
> +
>      MemoryRegion iomem;
>      uint32_t pborctl;
>      uint32_t ldopctl;
> @@ -371,11 +376,18 @@ typedef struct {
>      uint32_t dcgc[3];
>      uint32_t clkvclr;
>      uint32_t ldoarst;
> +    qemu_irq irq;
> +    /* Properties (all read-only registers) */
>      uint32_t user0;
>      uint32_t user1;
> -    qemu_irq irq;
> -    stellaris_board_info *board;
> -} ssys_state;
> +    uint32_t did0;
> +    uint32_t did1;
> +    uint32_t dc0;
> +    uint32_t dc1;
> +    uint32_t dc2;
> +    uint32_t dc3;
> +    uint32_t dc4;

Shouldn't these be class properties?

> +};

On 19:06 Thu 21 Jan     , Peter Maydell wrote:
> Convert the SSYS code in the Stellaris boards (which encapsulates the
> system registers) to a proper QOM device.  This will provide us with
> somewhere to put the output Clock whose frequency depends on the
> setting of the PLL configuration registers.
> 
> This is a migration compatibility break for lm3s811evb, lm3s6965evb.
> 
> We use 3-phase reset here because the Clock will need to propagate
> its value in the hold phase.
> 
> For the moment we reset the device during the board creation so that
> the system_clock_scale global gets set; this will be removed in a
> subsequent commit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
>  hw/arm/stellaris.c | 132 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 107 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 652823195b1..0194ede2fe0 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -357,7 +357,12 @@ static void stellaris_gptm_realize(DeviceState *dev, Error **errp)
>  
>  /* System controller.  */
>  
> -typedef struct {
> +#define TYPE_STELLARIS_SYS "stellaris-sys"
> +OBJECT_DECLARE_SIMPLE_TYPE(ssys_state, STELLARIS_SYS)
> +
> +struct ssys_state {
> +    SysBusDevice parent_obj;
> +
>      MemoryRegion iomem;
>      uint32_t pborctl;
>      uint32_t ldopctl;
> @@ -371,11 +376,18 @@ typedef struct {
>      uint32_t dcgc[3];
>      uint32_t clkvclr;
>      uint32_t ldoarst;
> +    qemu_irq irq;
> +    /* Properties (all read-only registers) */
>      uint32_t user0;
>      uint32_t user1;
> -    qemu_irq irq;
> -    stellaris_board_info *board;
> -} ssys_state;
> +    uint32_t did0;
> +    uint32_t did1;
> +    uint32_t dc0;
> +    uint32_t dc1;
> +    uint32_t dc2;
> +    uint32_t dc3;
> +    uint32_t dc4;
> +};
>  
>  static void ssys_update(ssys_state *s)
>  {
> @@ -430,7 +442,7 @@ static uint32_t pllcfg_fury[16] = {
>  
>  static int ssys_board_class(const ssys_state *s)
>  {
> -    uint32_t did0 = s->board->did0;
> +    uint32_t did0 = s->did0;
>      switch (did0 & DID0_VER_MASK) {
>      case DID0_VER_0:
>          return DID0_CLASS_SANDSTORM;
> @@ -456,19 +468,19 @@ static uint64_t ssys_read(void *opaque, hwaddr offset,
>  
>      switch (offset) {
>      case 0x000: /* DID0 */
> -        return s->board->did0;
> +        return s->did0;
>      case 0x004: /* DID1 */
> -        return s->board->did1;
> +        return s->did1;
>      case 0x008: /* DC0 */
> -        return s->board->dc0;
> +        return s->dc0;
>      case 0x010: /* DC1 */
> -        return s->board->dc1;
> +        return s->dc1;
>      case 0x014: /* DC2 */
> -        return s->board->dc2;
> +        return s->dc2;
>      case 0x018: /* DC3 */
> -        return s->board->dc3;
> +        return s->dc3;
>      case 0x01c: /* DC4 */
> -        return s->board->dc4;
> +        return s->dc4;
>      case 0x030: /* PBORCTL */
>          return s->pborctl;
>      case 0x034: /* LDOPCTL */
> @@ -646,9 +658,9 @@ static const MemoryRegionOps ssys_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static void ssys_reset(void *opaque)
> +static void stellaris_sys_reset_enter(Object *obj, ResetType type)
>  {
> -    ssys_state *s = (ssys_state *)opaque;
> +    ssys_state *s = STELLARIS_SYS(obj);
>  
>      s->pborctl = 0x7ffd;
>      s->rcc = 0x078e3ac0;
> @@ -661,9 +673,19 @@ static void ssys_reset(void *opaque)
>      s->rcgc[0] = 1;
>      s->scgc[0] = 1;
>      s->dcgc[0] = 1;
> +}
> +
> +static void stellaris_sys_reset_hold(Object *obj)
> +{
> +    ssys_state *s = STELLARIS_SYS(obj);
> +
>      ssys_calculate_system_clock(s);
>  }
>  
> +static void stellaris_sys_reset_exit(Object *obj)
> +{
> +}
> +
>  static int stellaris_sys_post_load(void *opaque, int version_id)
>  {
>      ssys_state *s = opaque;
> @@ -695,27 +717,66 @@ static const VMStateDescription vmstate_stellaris_sys = {
>      }
>  };
>  
> +static Property stellaris_sys_properties[] = {
> +    DEFINE_PROP_UINT32("user0", ssys_state, user0, 0),
> +    DEFINE_PROP_UINT32("user1", ssys_state, user1, 0),
> +    DEFINE_PROP_UINT32("did0", ssys_state, did0, 0),
> +    DEFINE_PROP_UINT32("did1", ssys_state, did1, 0),
> +    DEFINE_PROP_UINT32("dc0", ssys_state, dc0, 0),
> +    DEFINE_PROP_UINT32("dc1", ssys_state, dc1, 0),
> +    DEFINE_PROP_UINT32("dc2", ssys_state, dc2, 0),
> +    DEFINE_PROP_UINT32("dc3", ssys_state, dc3, 0),
> +    DEFINE_PROP_UINT32("dc4", ssys_state, dc4, 0),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void stellaris_sys_instance_init(Object *obj)
> +{
> +    ssys_state *s = STELLARIS_SYS(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
> +
> +    memory_region_init_io(&s->iomem, obj, &ssys_ops, s, "ssys", 0x00001000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
>  static int stellaris_sys_init(uint32_t base, qemu_irq irq,
>                                stellaris_board_info * board,
>                                uint8_t *macaddr)
>  {
> -    ssys_state *s;
> +    DeviceState *dev = qdev_new(TYPE_STELLARIS_SYS);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> -    s = g_new0(ssys_state, 1);
> -    s->irq = irq;
> -    s->board = board;
>      /* Most devices come preprogrammed with a MAC address in the user data. */
> -    s->user0 = macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16);
> -    s->user1 = macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16);
> +    qdev_prop_set_uint32(dev, "user0",
> +                         macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16));
> +    qdev_prop_set_uint32(dev, "user1",
> +                         macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16));
> +    qdev_prop_set_uint32(dev, "did0", board->did0);
> +    qdev_prop_set_uint32(dev, "did1", board->did1);
> +    qdev_prop_set_uint32(dev, "dc0", board->dc0);
> +    qdev_prop_set_uint32(dev, "dc1", board->dc1);
> +    qdev_prop_set_uint32(dev, "dc2", board->dc2);
> +    qdev_prop_set_uint32(dev, "dc3", board->dc3);
> +    qdev_prop_set_uint32(dev, "dc4", board->dc4);
> +
> +    sysbus_realize_and_unref(sbd, &error_fatal);
> +    sysbus_mmio_map(sbd, 0, base);
> +    sysbus_connect_irq(sbd, 0, irq);
> +
> +    /*
> +     * Normally we should not be resetting devices like this during
> +     * board creation. For the moment we need to do so, because
> +     * system_clock_scale will only get set when the STELLARIS_SYS
> +     * device is reset, and we need its initial value to pass to
> +     * the watchdog device. This hack can be removed once the
> +     * watchdog has been converted to use a Clock input instead.
> +     */
> +    device_cold_reset(dev);
>  
> -    memory_region_init_io(&s->iomem, NULL, &ssys_ops, s, "ssys", 0x00001000);
> -    memory_region_add_subregion(get_system_memory(), base, &s->iomem);
> -    ssys_reset(s);
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_stellaris_sys, s);
>      return 0;
>  }
>  
> -
>  /* I2C controller.  */
>  
>  #define TYPE_STELLARIS_I2C "stellaris-i2c"
> @@ -1553,11 +1614,32 @@ static const TypeInfo stellaris_adc_info = {
>      .class_init    = stellaris_adc_class_init,
>  };
>  
> +static void stellaris_sys_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_stellaris_sys;
> +    rc->phases.enter = stellaris_sys_reset_enter;
> +    rc->phases.hold = stellaris_sys_reset_hold;
> +    rc->phases.exit = stellaris_sys_reset_exit;
> +    device_class_set_props(dc, stellaris_sys_properties);
> +}
> +
> +static const TypeInfo stellaris_sys_info = {
> +    .name = TYPE_STELLARIS_SYS,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(ssys_state),
> +    .instance_init = stellaris_sys_instance_init,
> +    .class_init = stellaris_sys_class_init,
> +};
> +
>  static void stellaris_register_types(void)
>  {
>      type_register_static(&stellaris_i2c_info);
>      type_register_static(&stellaris_gptm_info);
>      type_register_static(&stellaris_adc_info);
> +    type_register_static(&stellaris_sys_info);
>  }
>  
>  type_init(stellaris_register_types)
> -- 
> 2.20.1
> 

--

On Thu, 21 Jan 2021 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 1/21/21 8:06 PM, Peter Maydell wrote:
> > Convert the SSYS code in the Stellaris boards (which encapsulates the
> > system registers) to a proper QOM device.  This will provide us with
> > somewhere to put the output Clock whose frequency depends on the
> > setting of the PLL configuration registers.
> >
> > This is a migration compatibility break for lm3s811evb, lm3s6965evb.
> >
> > We use 3-phase reset here because the Clock will need to propagate
> > its value in the hold phase.
> >
> > For the moment we reset the device during the board creation so that
> > the system_clock_scale global gets set; this will be removed in a
> > subsequent commit.

> > +
> > +struct ssys_state {
> > +    SysBusDevice parent_obj;
> > +
> >      MemoryRegion iomem;
> >      uint32_t pborctl;
> >      uint32_t ldopctl;
> > @@ -371,11 +376,18 @@ typedef struct {
> >      uint32_t dcgc[3];
> >      uint32_t clkvclr;
> >      uint32_t ldoarst;
> > +    qemu_irq irq;
> > +    /* Properties (all read-only registers) */
> >      uint32_t user0;
> >      uint32_t user1;
> > -    qemu_irq irq;
> > -    stellaris_board_info *board;
> > -} ssys_state;
> > +    uint32_t did0;
> > +    uint32_t did1;
> > +    uint32_t dc0;
> > +    uint32_t dc1;
> > +    uint32_t dc2;
> > +    uint32_t dc3;
> > +    uint32_t dc4;
>
> Shouldn't these be class properties?

Could you elaborate on what you think the code ought to look like?
I just used the usual thing of defining uint32 qdev properties so we
can set these values when we create the device, as a replacement
for the existing code which either reaches directly into the
state struct to set the user0/user1 values or sets the
stellaris_board_info pointer in the state struct.

thanks
-- PMM

On 1/25/21 12:48 PM, Peter Maydell wrote:
> On Thu, 21 Jan 2021 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 1/21/21 8:06 PM, Peter Maydell wrote:
>>> Convert the SSYS code in the Stellaris boards (which encapsulates the
>>> system registers) to a proper QOM device.  This will provide us with
>>> somewhere to put the output Clock whose frequency depends on the
>>> setting of the PLL configuration registers.
>>>
>>> This is a migration compatibility break for lm3s811evb, lm3s6965evb.
>>>
>>> We use 3-phase reset here because the Clock will need to propagate
>>> its value in the hold phase.
>>>
>>> For the moment we reset the device during the board creation so that
>>> the system_clock_scale global gets set; this will be removed in a
>>> subsequent commit.
> 
>>> +
>>> +struct ssys_state {
>>> +    SysBusDevice parent_obj;
>>> +
>>>      MemoryRegion iomem;
>>>      uint32_t pborctl;
>>>      uint32_t ldopctl;
>>> @@ -371,11 +376,18 @@ typedef struct {
>>>      uint32_t dcgc[3];
>>>      uint32_t clkvclr;
>>>      uint32_t ldoarst;
>>> +    qemu_irq irq;
>>> +    /* Properties (all read-only registers) */
>>>      uint32_t user0;
>>>      uint32_t user1;
>>> -    qemu_irq irq;
>>> -    stellaris_board_info *board;
>>> -} ssys_state;
>>> +    uint32_t did0;
>>> +    uint32_t did1;
>>> +    uint32_t dc0;
>>> +    uint32_t dc1;
>>> +    uint32_t dc2;
>>> +    uint32_t dc3;
>>> +    uint32_t dc4;
>>
>> Shouldn't these be class properties?
> 
> Could you elaborate on what you think the code ought to look like?

I am thinking something similar how Igor asked me to implement
RaspiMachineClass::board_rev in hw/arm/raspi.c, as the did/dc registers
are read-only. Anyhow this is 1/ probably not necessary and 2/ out of
the scope of this series, this patch is already complex enough, and
the work is done.

> I just used the usual thing of defining uint32 qdev properties so we
> can set these values when we create the device, as a replacement
> for the existing code which either reaches directly into the
> state struct to set the user0/user1 values or sets the
> stellaris_board_info pointer in the state struct.

No problem.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>