Re: [PATCH v3 10/22] ppc/ppc405: QOM'ify GPT

From: BALATON Zoltan <balaton@eik.bme.hu>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: qemu-ppc@nongnu.org,
	Daniel Henrique Barboza <danielhb413@gmail.com>,
	 qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>
Subject: Re: [PATCH v3 10/22] ppc/ppc405: QOM'ify GPT
Date: Mon, 8 Aug 2022 16:25:14 +0200 (CEST)	[thread overview]
Message-ID: <b3a0ebc-8711-2e98-439c-f9c108dc83@eik.bme.hu> (raw)
In-Reply-To: <20220808102734.133084-11-clg@kaod.org>

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

On Mon, 8 Aug 2022, Cédric Le Goater wrote:
> The GPT controller is currently modeled as a SysBus device with a
> unique memory region, a couple of IRQs and a timer.
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/ppc405.h    | 22 ++++++++++++
> hw/ppc/ppc405_uc.c | 90 +++++++++++++++++++++++-----------------------
> 2 files changed, 67 insertions(+), 45 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 2ba829988de2..bcf55e4f6b2e 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -63,6 +63,27 @@ struct ppc4xx_bd_info_t {
>     uint32_t bi_iic_fast[2];
> };
>
> +/* General purpose timers */
> +#define TYPE_PPC405_GPT "ppc405-gpt"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405GptState, PPC405_GPT);
> +struct Ppc405GptState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +
> +    int64_t tb_offset;
> +    uint32_t tb_freq;
> +    QEMUTimer *timer;
> +    qemu_irq irqs[5];
> +    uint32_t oe;
> +    uint32_t ol;
> +    uint32_t im;
> +    uint32_t is;
> +    uint32_t ie;
> +    uint32_t comp[5];
> +    uint32_t mask[5];
> +};
> +
> #define TYPE_PPC405_CPC "ppc405-cpc"
> OBJECT_DECLARE_SIMPLE_TYPE(Ppc405CpcState, PPC405_CPC);
>
> @@ -114,6 +135,7 @@ struct Ppc405SoCState {
>     PowerPCCPU cpu;
>     DeviceState *uic;
>     Ppc405CpcState cpc;
> +    Ppc405GptState gpt;
> };
>
> /* PowerPC 405 core */
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index e45535b8e52a..f64e661eef6c 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -926,34 +926,18 @@ static void ppc405_ocm_init(CPUPPCState *env)
>
> /*****************************************************************************/
> /* General purpose timers */
> -typedef struct ppc4xx_gpt_t ppc4xx_gpt_t;
> -struct ppc4xx_gpt_t {
> -    MemoryRegion iomem;
> -    int64_t tb_offset;
> -    uint32_t tb_freq;
> -    QEMUTimer *timer;
> -    qemu_irq irqs[5];
> -    uint32_t oe;
> -    uint32_t ol;
> -    uint32_t im;
> -    uint32_t is;
> -    uint32_t ie;
> -    uint32_t comp[5];
> -    uint32_t mask[5];
> -};
> -
> -static int ppc4xx_gpt_compare (ppc4xx_gpt_t *gpt, int n)
> +static int ppc4xx_gpt_compare(Ppc405GptState *gpt, int n)
> {
>     /* XXX: TODO */
>     return 0;
> }
>
> -static void ppc4xx_gpt_set_output (ppc4xx_gpt_t *gpt, int n, int level)
> +static void ppc4xx_gpt_set_output(Ppc405GptState *gpt, int n, int level)
> {
>     /* XXX: TODO */
> }
>
> -static void ppc4xx_gpt_set_outputs (ppc4xx_gpt_t *gpt)
> +static void ppc4xx_gpt_set_outputs(Ppc405GptState *gpt)
> {
>     uint32_t mask;
>     int i;
> @@ -974,7 +958,7 @@ static void ppc4xx_gpt_set_outputs (ppc4xx_gpt_t *gpt)
>     }
> }
>
> -static void ppc4xx_gpt_set_irqs (ppc4xx_gpt_t *gpt)
> +static void ppc4xx_gpt_set_irqs(Ppc405GptState *gpt)
> {
>     uint32_t mask;
>     int i;
> @@ -989,14 +973,14 @@ static void ppc4xx_gpt_set_irqs (ppc4xx_gpt_t *gpt)
>     }
> }
>
> -static void ppc4xx_gpt_compute_timer (ppc4xx_gpt_t *gpt)
> +static void ppc4xx_gpt_compute_timer(Ppc405GptState *gpt)
> {
>     /* XXX: TODO */
> }
>
> static uint64_t ppc4xx_gpt_read(void *opaque, hwaddr addr, unsigned size)
> {
> -    ppc4xx_gpt_t *gpt = opaque;
> +    Ppc405GptState *gpt = PPC405_GPT(opaque);
>     uint32_t ret;
>     int idx;
>
> @@ -1050,7 +1034,7 @@ static uint64_t ppc4xx_gpt_read(void *opaque, hwaddr addr, unsigned size)
> static void ppc4xx_gpt_write(void *opaque, hwaddr addr, uint64_t value,
>                              unsigned size)
> {
> -    ppc4xx_gpt_t *gpt = opaque;
> +    Ppc405GptState *gpt = PPC405_GPT(opaque);
>     int idx;
>
>     trace_ppc4xx_gpt_write(addr, size, value);
> @@ -1116,20 +1100,18 @@ static const MemoryRegionOps gpt_ops = {
>
> static void ppc4xx_gpt_cb (void *opaque)
> {
> -    ppc4xx_gpt_t *gpt;
> +    Ppc405GptState *gpt = PPC405_GPT(opaque);
>
> -    gpt = opaque;
>     ppc4xx_gpt_set_irqs(gpt);
>     ppc4xx_gpt_set_outputs(gpt);
>     ppc4xx_gpt_compute_timer(gpt);
> }
>
> -static void ppc4xx_gpt_reset (void *opaque)
> +static void ppc405_gpt_reset(DeviceState *dev)
> {
> -    ppc4xx_gpt_t *gpt;
> +    Ppc405GptState *gpt = PPC405_GPT(dev);
>     int i;
>
> -    gpt = opaque;
>     timer_del(gpt->timer);
>     gpt->oe = 0x00000000;
>     gpt->ol = 0x00000000;
> @@ -1142,21 +1124,28 @@ static void ppc4xx_gpt_reset (void *opaque)
>     }
> }
>
> -static void ppc4xx_gpt_init(hwaddr base, qemu_irq irqs[5])
> +static void ppc405_gpt_realize(DeviceState *dev, Error **errp)
> {
> -    ppc4xx_gpt_t *gpt;
> +    Ppc405GptState *s = PPC405_GPT(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>     int i;
>
> -    trace_ppc4xx_gpt_init(base);

Where has this trace gone? If you've dropped it then you should also drop 
it from trace-events.

> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &ppc4xx_gpt_cb, s);
> +    memory_region_init_io(&s->iomem, OBJECT(s), &gpt_ops, s, "gpt", 0x0d4);
> +    sysbus_init_mmio(sbd, &s->iomem);
>
> -    gpt = g_new0(ppc4xx_gpt_t, 1);
> -    for (i = 0; i < 5; i++) {
> -        gpt->irqs[i] = irqs[i];
> +    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> +        sysbus_init_irq(sbd, &s->irqs[i]);
>     }
> -    gpt->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &ppc4xx_gpt_cb, gpt);
> -    memory_region_init_io(&gpt->iomem, NULL, &gpt_ops, gpt, "gpt", 0x0d4);
> -    memory_region_add_subregion(get_system_memory(), base, &gpt->iomem);
> -    qemu_register_reset(ppc4xx_gpt_reset, gpt);
> +}
> +
> +static void ppc405_gpt_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc405_gpt_realize;
> +    dc->user_creatable = false;
> +    dc->reset = ppc405_gpt_reset;
> }
>
> /*****************************************************************************/
> @@ -1421,13 +1410,16 @@ static void ppc405_soc_instance_init(Object *obj)
>
>     object_initialize_child(obj, "cpc", &s->cpc, TYPE_PPC405_CPC);
>     object_property_add_alias(obj, "sys-clk", OBJECT(&s->cpc), "sys-clk");
> +
> +    object_initialize_child(obj, "gpt", &s->gpt, TYPE_PPC405_GPT);
> }
>
> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> {
>     Ppc405SoCState *s = PPC405_SOC(dev);
> -    qemu_irq dma_irqs[4], gpt_irqs[5], mal_irqs[4];
> +    qemu_irq dma_irqs[4], mal_irqs[4];
>     CPUPPCState *env;
> +    int i;
>
>     /* init CPUs */
>     if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
> @@ -1513,12 +1505,15 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>     ppc405_ocm_init(env);
>
>     /* GPT */
> -    gpt_irqs[0] = qdev_get_gpio_in(s->uic, 19);
> -    gpt_irqs[1] = qdev_get_gpio_in(s->uic, 20);
> -    gpt_irqs[2] = qdev_get_gpio_in(s->uic, 21);
> -    gpt_irqs[3] = qdev_get_gpio_in(s->uic, 22);
> -    gpt_irqs[4] = qdev_get_gpio_in(s->uic, 23);
> -    ppc4xx_gpt_init(0xef600000, gpt_irqs);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpt), errp)) {
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpt), 0, 0xef600000);
> +
> +    for (i = 0; i < ARRAY_SIZE(s->gpt.irqs); i++) {
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpt), i,
> +                           qdev_get_gpio_in(s->uic, 19 + i));

Maybe you should have a SysBusDevice *sbd local here and cast only once if 
you need it several times. It's also more readable that way.

Regards,
BALATON Zoltan

> +    }
>
>     /* MAL */
>     mal_irqs[0] = qdev_get_gpio_in(s->uic, 11);
> @@ -1550,6 +1545,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void *data)
>
> static const TypeInfo ppc405_types[] = {
>     {
> +        .name           = TYPE_PPC405_GPT,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(Ppc405GptState),
> +        .class_init     = ppc405_gpt_class_init,
> +    }, {
>         .name           = TYPE_PPC405_CPC,
>         .parent         = TYPE_PPC4xx_DCR_DEVICE,
>         .instance_size  = sizeof(Ppc405CpcState),
>