Re: [PATCH v3 12/22] ppc/ppc405: QOM'ify GPIO

[Cédric Le Goater]

On 8/8/22 16:32, BALATON Zoltan wrote:
> On Mon, 8 Aug 2022, Cédric Le Goater wrote:
> > The GPIO controller is currently modeled as a simple SysBus device
> > with a unique memory region.
> >
> > Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> > hw/ppc/ppc405.h    | 21 +++++++++++++++++++
> > hw/ppc/ppc405_uc.c | 50 +++++++++++++++++++++-------------------------
> > 2 files changed, 44 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> > index a5b493d3e7bf..21f6cb358501 100644
> > --- a/hw/ppc/ppc405.h
> > +++ b/hw/ppc/ppc405.h
> > @@ -63,6 +63,26 @@ struct ppc4xx_bd_info_t {
> >     uint32_t bi_iic_fast[2];
> > };
> >
> > +/* GPIO */
> > +#define TYPE_PPC405_GPIO "ppc405-gpio"
> > +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405GpioState, PPC405_GPIO);
> > +struct Ppc405GpioState {
> > +    SysBusDevice parent_obj;
> > +
> > +    MemoryRegion io;
> > +    uint32_t or;
> > +    uint32_t tcr;
> > +    uint32_t osrh;
> > +    uint32_t osrl;
> > +    uint32_t tsrh;
> > +    uint32_t tsrl;
> > +    uint32_t odr;
> > +    uint32_t ir;
> > +    uint32_t rr1;
> > +    uint32_t isr1h;
> > +    uint32_t isr1l;
> > +};
> > +
> > /* On Chip Memory */
> > #define TYPE_PPC405_OCM "ppc405-ocm"
> > OBJECT_DECLARE_SIMPLE_TYPE(Ppc405OcmState, PPC405_OCM);
> > @@ -152,6 +172,7 @@ struct Ppc405SoCState {
> >     Ppc405CpcState cpc;
> >     Ppc405GptState gpt;
> >     Ppc405OcmState ocm;
> > +    Ppc405GpioState gpio;
> > };
> >
> > /* PowerPC 405 core */
> > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> > index 961a7c851d4a..768e6909a831 100644
> > --- a/hw/ppc/ppc405_uc.c
> > +++ b/hw/ppc/ppc405_uc.c
> > @@ -713,23 +713,6 @@ static void ppc405_dma_init(CPUPPCState *env, qemu_irq 
> > irqs[4])
> > }
> >
> > /*****************************************************************************/
> > -/* GPIO */
> > -typedef struct ppc405_gpio_t ppc405_gpio_t;
> > -struct ppc405_gpio_t {
> > -    MemoryRegion io;
> > -    uint32_t or;
> > -    uint32_t tcr;
> > -    uint32_t osrh;
> > -    uint32_t osrl;
> > -    uint32_t tsrh;
> > -    uint32_t tsrl;
> > -    uint32_t odr;
> > -    uint32_t ir;
> > -    uint32_t rr1;
> > -    uint32_t isr1h;
> > -    uint32_t isr1l;
> > -};
> > -
> > static uint64_t ppc405_gpio_read(void *opaque, hwaddr addr, unsigned size)
> > {
> >     trace_ppc405_gpio_read(addr, size);
> > @@ -748,20 +731,22 @@ static const MemoryRegionOps ppc405_gpio_ops = {
> >     .endianness = DEVICE_NATIVE_ENDIAN,
> > };
> >
> > -static void ppc405_gpio_reset (void *opaque)
> > +static void ppc405_gpio_realize(DeviceState *dev, Error **errp)
> > {
> > +    Ppc405GpioState *s = PPC405_GPIO(dev);
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +
> > +    memory_region_init_io(&s->io, OBJECT(s), &ppc405_gpio_ops, s, "gpio",
> > +                          0x038);
> > +    sysbus_init_mmio(sbd, &s->io);
> > }
> >
> > -static void ppc405_gpio_init(hwaddr base)
> > +static void ppc405_gpio_class_init(ObjectClass *oc, void *data)
> > {
> > -    ppc405_gpio_t *gpio;
> > -
> > -    trace_ppc405_gpio_init(base);
>
> Trace gone, update trace-events too.

Yes. I have checked the others.

Thanks,

C.

> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> >
> > -    gpio = g_new0(ppc405_gpio_t, 1);
> > -    memory_region_init_io(&gpio->io, NULL, &ppc405_gpio_ops, gpio, "pgio", 
> > 0x038);
> > -    memory_region_add_subregion(get_system_memory(), base, &gpio->io);
> > -    qemu_register_reset(&ppc405_gpio_reset, gpio);
> > +    dc->realize = ppc405_gpio_realize;
> > +    dc->user_creatable = false;
> > }
> >
> > /*****************************************************************************/
> > @@ -1405,6 +1390,8 @@ static void ppc405_soc_instance_init(Object *obj)
> >     object_initialize_child(obj, "gpt", &s->gpt, TYPE_PPC405_GPT);
> >
> >     object_initialize_child(obj, "ocm", &s->ocm, TYPE_PPC405_OCM);
> > +
> > +    object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
> > }
> >
> > static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> > @@ -1477,8 +1464,12 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
> > **errp)
> >     /* I2C controller */
> >     sysbus_create_simple(TYPE_PPC4xx_I2C, 0xef600500,
> >                          qdev_get_gpio_in(s->uic, 2));
> > +
> >     /* GPIO */
> > -    ppc405_gpio_init(0xef600700);
> > +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpio), errp)) {
> > +        return;
> > +    }
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0, 0xef600700);
> >
> >     /* Serial ports */
> >     if (serial_hd(0) != NULL) {
> > @@ -1540,6 +1531,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void 
> > *data)
> >
> > static const TypeInfo ppc405_types[] = {
> >     {
> > +        .name           = TYPE_PPC405_GPIO,
> > +        .parent         = TYPE_SYS_BUS_DEVICE,
> > +        .instance_size  = sizeof(Ppc405GpioState),
> > +        .class_init     = ppc405_gpio_class_init,
> > +    }, {
> >         .name           = TYPE_PPC405_OCM,
> >         .parent         = TYPE_PPC4xx_DCR_DEVICE,
> >         .instance_size  = sizeof(Ppc405OcmState),