Re: [RFC PATCH 11/13] hw/timer/allwinner: Introduce TYPE_AW_COMMON_PIT abstract device

[Niek Linnenbank <nieklinnenbank@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5026 bytes --]

Hi Philippe,

On Thu, Dec 19, 2019 at 7:51 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Extract the common code from the TYPE_AW_A10_PIT device into a new
> abstract device: TYPE_AW_COMMON_PIT, then use it as parent, so we
> inherit the same functionalities.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> At this point, the only fields we can modify are the timer_count
> and the region_size. Not enough to implement the H3 timer, since
> we need to move the WDOG register. Still some progress, so Niek
> can continue ;)
> ---
>  include/hw/timer/allwinner-a10-pit.h |  1 +
>  hw/timer/allwinner-a10-pit.c         | 50 +++++++++++++++++++++++-----
>  2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/timer/allwinner-a10-pit.h
> b/include/hw/timer/allwinner-a10-pit.h
> index 9e28c6697a..8453a62706 100644
> --- a/include/hw/timer/allwinner-a10-pit.h
> +++ b/include/hw/timer/allwinner-a10-pit.h
> @@ -4,6 +4,7 @@
>  #include "hw/ptimer.h"
>  #include "hw/sysbus.h"
>
> +#define TYPE_AW_COMMON_PIT "allwinner-timer-controller"
>  #define TYPE_AW_A10_PIT "allwinner-A10-timer"
>

So for the Allwinner H3, that means we'll need another TYPE_AW_H3_PIT
definition?


>
>  #define AW_PIT_TIMER_MAX        6
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index f2ac271e80..ad409b96a1 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
>

Perhaps we can rename the hw/timer/allwinner-a10-pit.c to a generic name,
for example hw/timer/allwinner-pit.c ?


> @@ -54,6 +54,20 @@
>  #define AW_A10_PIT(obj) \
>      OBJECT_CHECK(AllwinnerTmrCtrlState, (obj), TYPE_AW_A10_PIT)
>
> +typedef struct AllwinnerTmrCtrlClass {
> +    /*< private >*/
> +    SysBusDeviceClass parent_class;
> +    /*< public >*/
> +
> +    size_t timer_count;
> +    size_t region_size;
> +} AllwinnerTmrCtrlClass;
> +
> +#define AW_TIMER_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(AllwinnerTmrCtrlClass, (klass),
> TYPE_AW_COMMON_PIT)
> +#define AW_TIMER_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(AllwinnerTmrCtrlClass, (obj), TYPE_AW_COMMON_PIT)
> +
>  static void a10_pit_update_irq(AllwinnerTmrCtrlState *s)
>  {
>      int i;
> @@ -303,19 +317,20 @@ static void a10_pit_timer_cb(void *opaque)
>      }
>  }
>
> -static void a10_pit_init(Object *obj)
> +static void aw_pit_instance_init(Object *obj)
>  {
>      AllwinnerTmrCtrlState *s = AW_A10_PIT(obj);
> +    AllwinnerTmrCtrlClass *c = AW_TIMER_GET_CLASS(s);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      uint8_t i;
>
> -    s->timer_count = AW_A10_PIT_TIMER_NR;
> +    s->timer_count = c->timer_count;
>
>      for (i = 0; i < s->timer_count; i++) {
>          sysbus_init_irq(sbd, &s->timer[i].irq);
>      }
>      memory_region_init_io(&s->iomem, OBJECT(s), &a10_pit_ops, s,
>

I am curious how to support the different WDOG0 registers for the Allwinner
H3 while keeping
the A10 functionality also working :-) Will you give the TYPE_AW_H3_PIT
its own MemoryRegionOps with read/write?


> -                          TYPE_AW_A10_PIT, 0x400);
> +                          TYPE_AW_A10_PIT, c->region_size);
>      sysbus_init_mmio(sbd, &s->iomem);
>
>      for (i = 0; i < s->timer_count; i++) {
> @@ -328,26 +343,45 @@ static void a10_pit_init(Object *obj)
>      }
>  }
>
> -static void a10_pit_class_init(ObjectClass *klass, void *data)
> +static void aw_timer_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->reset = a10_pit_reset;
>      dc->props = a10_pit_properties;
> -    dc->desc = "allwinner a10 timer";
> +    dc->desc = "Allwinner Timer Controller";
>      dc->vmsd = &vmstate_a10_pit;
>  }
>
> +static const TypeInfo allwinner_pit_info = {
> +    .name = TYPE_AW_COMMON_PIT,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_init = aw_pit_instance_init,
> +    .instance_size = sizeof(AllwinnerTmrCtrlState),
> +    .class_init = aw_timer_class_init,
> +    .class_size = sizeof(AllwinnerTmrCtrlClass),
> +    .abstract   = true,
> +};
> +
> +static void a10_pit_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AllwinnerTmrCtrlClass *atc = AW_TIMER_CLASS(klass);
> +
> +    dc->desc = "Allwinner A10 Timer Controller";
> +    atc->timer_count = AW_A10_PIT_TIMER_NR;
> +    atc->region_size = 0x400;
> +}
> +
>  static const TypeInfo a10_pit_info = {
>      .name = TYPE_AW_A10_PIT,
> -    .parent = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(AllwinnerTmrCtrlState),
> -    .instance_init = a10_pit_init,
> +    .parent = TYPE_AW_COMMON_PIT,
>      .class_init = a10_pit_class_init,
>  };
>
>  static void a10_register_types(void)
>  {
> +    type_register_static(&allwinner_pit_info);
>      type_register_static(&a10_pit_info);
>  }
>
> --
> 2.21.0
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 6596 bytes --]