Re: [PATCH v4 23/24] ppc/ppc405: QOM'ify SDRAM

[Cédric Le Goater]

On 8/9/22 19:53, BALATON Zoltan wrote:
> On Tue, 9 Aug 2022, Cédric Le Goater wrote:
> > This is an initial change of the SDRAM controller preserving the
> > compatibility with the current modeling. Further cleanup will be
> > possible after conversion of the ppc4xx_sdram_banks() and
> > ppc4xx_sdram_init() routines of the sam460ex and bamboo machines.
> >
> > The size and base address of the RAM banks are now set using QOM
> > property arrays. RAM is equally distributed on each bank at the SoC
> > level depending on the number of banks we want to initialize (default
> > is 2). Each RAM memory region representing a RAM bank is initialized
> > in the realize routine of the SDRAM model after a minimal check on the
> > RAM size value with the sdram_bcr() routine. This has the benefit of
> > reporting an error to the user if the requested RAM size is invalid
> > for the SDRAM controller.
>
> Haven't looked at it in detail yet but I think we have two versions of this 
> (one in ppc4xx_devs.c and another in ppc440_uc.c) that are slightly different 
> due to the differences of the memory controllers of later SoCs. I'm not sure 
> how to clean this up and forgot most of the details about this.

AFAICT We need a Ppc4xxSdramClass to abstract the differences.

Thanks,

C.

> Regards,
> BALATON Zoltan
>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> > hw/ppc/ppc405.h         |   6 +--
> > include/hw/ppc/ppc4xx.h |  32 ++++++++++++
> > hw/ppc/ppc405_uc.c      |  34 ++++++++----
> > hw/ppc/ppc4xx_devs.c    | 113 ++++++++++++++++++++++++++++------------
> > 4 files changed, 140 insertions(+), 45 deletions(-)
> >
> > diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> > index 56881b181ba1..8c19d167391c 100644
> > --- a/hw/ppc/ppc405.h
> > +++ b/hw/ppc/ppc405.h
> > @@ -228,12 +228,9 @@ struct Ppc405SoCState {
> >     DeviceState parent_obj;
> >
> >     /* Public */
> > -    MemoryRegion ram_banks[2];
> > -    hwaddr ram_bases[2], ram_sizes[2];
> > -    bool do_dram_init;
> > -
> >     MemoryRegion *dram_mr;
> >     hwaddr ram_size;
> > +    uint32_t nr_banks;
> >
> >     PowerPCCPU cpu;
> >     PPCUIC uic;
> > @@ -241,6 +238,7 @@ struct Ppc405SoCState {
> >     Ppc405GptState gpt;
> >     Ppc405OcmState ocm;
> >     Ppc405GpioState gpio;
> > +    Ppc4xxSdramState sdram;
> >     Ppc405DmaState dma;
> >     PPC4xxI2CState i2c;
> >     Ppc405EbcState ebc;
> > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> > index acd096cb2394..b841f6600b55 100644
> > --- a/include/hw/ppc/ppc4xx.h
> > +++ b/include/hw/ppc/ppc4xx.h
> > @@ -87,4 +87,36 @@ struct Ppc4xxMalState {
> > void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum,
> >                      qemu_irq irqs[4]);
> >
> > +/* SDRAM controller */
> > +#define TYPE_PPC4xx_SDRAM "ppc4xx-sdram"
> > +OBJECT_DECLARE_SIMPLE_TYPE(Ppc4xxSdramState, PPC4xx_SDRAM);
> > +struct Ppc4xxSdramState {
> > +    Ppc4xxDcrDeviceState parent_obj;
> > +
> > +    MemoryRegion *dram_mr;
> > +    bool dram_init;
> > +
> > +    MemoryRegion containers[4]; /* used for clipping */
> > +    MemoryRegion *ram_memories;
> > +    hwaddr *ram_bases;
> > +    hwaddr *ram_sizes;
> > +    uint32_t nb_ram_bases;
> > +    uint32_t nb_ram_sizes;
> > +    uint32_t nbanks; /* Redundant */
> > +
> > +    uint32_t addr;
> > +    uint32_t besr0;
> > +    uint32_t besr1;
> > +    uint32_t bear;
> > +    uint32_t cfg;
> > +    uint32_t status;
> > +    uint32_t rtr;
> > +    uint32_t pmit;
> > +    uint32_t bcr[4];
> > +    uint32_t tr;
> > +    uint32_t ecccfg;
> > +    uint32_t eccesr;
> > +    qemu_irq irq;
> > +};
> > +
> > #endif /* PPC4XX_H */
> > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> > index 1045f5f13e6c..fe0c92ba0d54 100644
> > --- a/hw/ppc/ppc405_uc.c
> > +++ b/hw/ppc/ppc405_uc.c
> > @@ -1361,6 +1361,9 @@ static void ppc405_soc_instance_init(Object *obj)
> >
> >     object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
> >
> > +    object_initialize_child(obj, "sdram", &s->sdram, TYPE_PPC4xx_SDRAM);
> > +    object_property_add_alias(obj, "dram-init", OBJECT(&s->sdram), 
> > "dram-init");
> > +
> >     object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
> >
> >     object_initialize_child(obj, "i2c", &s->i2c, TYPE_PPC4xx_I2C);
> > @@ -1432,15 +1435,28 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
> > **errp)
> >
> >     /* SDRAM controller */
> >         /* XXX 405EP has no ECC interrupt */
> > -    s->ram_bases[0] = 0;
> > -    s->ram_sizes[0] = s->ram_size;
> > -    memory_region_init_alias(&s->ram_banks[0], OBJECT(s),
> > -                             "ppc405.sdram0", s->dram_mr,
> > -                             s->ram_bases[0], s->ram_sizes[0]);
> > +    object_property_set_link(OBJECT(&s->sdram), "dram", OBJECT(s->dram_mr),
> > +                             &error_abort);
> > +
> > +    qdev_prop_set_uint32(DEVICE(&s->sdram), "len-ram-sizes", s->nr_banks);
> > +    qdev_prop_set_uint32(DEVICE(&s->sdram), "len-ram-bases", s->nr_banks);
> >
> > -    ppc4xx_sdram_init(env, qdev_get_gpio_in(DEVICE(&s->uic), 17), 1,
> > -                      s->ram_banks, s->ram_bases, s->ram_sizes,
> > -                      s->do_dram_init);
> > +    for (i = 0; i < s->nr_banks; i++) {
> > +        char name[32];
> > +        snprintf(name, sizeof(name), "ram-bases[%d]", i);
> > +        qdev_prop_set_uint32(DEVICE(&s->sdram), name,
> > +                             i * s->ram_size / s->nr_banks);
> > +
> > +        snprintf(name, sizeof(name), "ram-sizes[%d]", i);
> > +        qdev_prop_set_uint32(DEVICE(&s->sdram), name,
> > +                             s->ram_size / s->nr_banks);
> > +    }
> > +
> > +    if (!ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(&s->sdram), &s->cpu, errp)) {
> > +        return;
> > +    }
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdram), 0,
> > +                       qdev_get_gpio_in(DEVICE(&s->uic), 17));
> >
> >     /* External bus controller */
> >     if (!ppc4xx_dcr_realize(PPC4xx_DCR_DEVICE(&s->ebc), &s->cpu, errp)) {
> > @@ -1520,7 +1536,7 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
> > **errp)
> > static Property ppc405_soc_properties[] = {
> >     DEFINE_PROP_LINK("dram", Ppc405SoCState, dram_mr, TYPE_MEMORY_REGION,
> >                      MemoryRegion *),
> > -    DEFINE_PROP_BOOL("dram-init", Ppc405SoCState, do_dram_init, 0),
> > +    DEFINE_PROP_UINT32("nr-banks", Ppc405SoCState, nr_banks, 2),
> >     DEFINE_PROP_UINT64("ram-size", Ppc405SoCState, ram_size, 0),
> >     DEFINE_PROP_END_OF_LIST(),
> > };
> > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > index c06c20b195cd..a9ceea13f218 100644
> > --- a/hw/ppc/ppc4xx_devs.c
> > +++ b/hw/ppc/ppc4xx_devs.c
> > @@ -39,28 +39,6 @@
> >
> > /*****************************************************************************/
> > /* SDRAM controller */
> > -typedef struct ppc4xx_sdram_t ppc4xx_sdram_t;
> > -struct ppc4xx_sdram_t {
> > -    uint32_t addr;
> > -    int nbanks;
> > -    MemoryRegion containers[4]; /* used for clipping */
> > -    MemoryRegion *ram_memories;
> > -    hwaddr ram_bases[4];
> > -    hwaddr ram_sizes[4];
> > -    uint32_t besr0;
> > -    uint32_t besr1;
> > -    uint32_t bear;
> > -    uint32_t cfg;
> > -    uint32_t status;
> > -    uint32_t rtr;
> > -    uint32_t pmit;
> > -    uint32_t bcr[4];
> > -    uint32_t tr;
> > -    uint32_t ecccfg;
> > -    uint32_t eccesr;
> > -    qemu_irq irq;
> > -};
> > -
> > enum {
> >     SDRAM0_CFGADDR = 0x010,
> >     SDRAM0_CFGDATA = 0x011,
> > @@ -128,7 +106,7 @@ static target_ulong sdram_size (uint32_t bcr)
> >     return size;
> > }
> >
> > -static void sdram_set_bcr(ppc4xx_sdram_t *sdram, int i,
> > +static void sdram_set_bcr(Ppc4xxSdramState *sdram, int i,
> >                           uint32_t bcr, int enabled)
> > {
> >     if (sdram->bcr[i] & 0x00000001) {
> > @@ -154,7 +132,7 @@ static void sdram_set_bcr(ppc4xx_sdram_t *sdram, int i,
> >     }
> > }
> >
> > -static void sdram_map_bcr (ppc4xx_sdram_t *sdram)
> > +static void sdram_map_bcr(Ppc4xxSdramState *sdram)
> > {
> >     int i;
> >
> > @@ -168,7 +146,7 @@ static void sdram_map_bcr (ppc4xx_sdram_t *sdram)
> >     }
> > }
> >
> > -static void sdram_unmap_bcr (ppc4xx_sdram_t *sdram)
> > +static void sdram_unmap_bcr(Ppc4xxSdramState *sdram)
> > {
> >     int i;
> >
> > @@ -182,7 +160,7 @@ static void sdram_unmap_bcr (ppc4xx_sdram_t *sdram)
> >
> > static uint32_t dcr_read_sdram (void *opaque, int dcrn)
> > {
> > -    ppc4xx_sdram_t *sdram;
> > +    Ppc4xxSdramState *sdram;
> >     uint32_t ret;
> >
> >     sdram = opaque;
> > @@ -250,7 +228,7 @@ static uint32_t dcr_read_sdram (void *opaque, int dcrn)
> >
> > static void dcr_write_sdram (void *opaque, int dcrn, uint32_t val)
> > {
> > -    ppc4xx_sdram_t *sdram;
> > +    Ppc4xxSdramState *sdram;
> >
> >     sdram = opaque;
> >     switch (dcrn) {
> > @@ -329,11 +307,10 @@ static void dcr_write_sdram (void *opaque, int dcrn, 
> > uint32_t val)
> >     }
> > }
> >
> > -static void sdram_reset (void *opaque)
> > +static void ppc4xx_sdram_reset(DeviceState *dev)
> > {
> > -    ppc4xx_sdram_t *sdram;
> > +    Ppc4xxSdramState *sdram = (Ppc4xxSdramState *) dev;
> >
> > -    sdram = opaque;
> >     sdram->addr = 0x00000000;
> >     sdram->bear = 0x00000000;
> >     sdram->besr0 = 0x00000000; /* No error */
> > @@ -349,21 +326,88 @@ static void sdram_reset (void *opaque)
> >     sdram->cfg = 0x00800000;
> > }
> >
> > +static void sdram_reset(void *opaque)
> > +{
> > +    ppc4xx_sdram_reset(opaque);
> > +}
> > +
> > +static void ppc4xx_sdram_realize(DeviceState *dev, Error **errp)
> > +{
> > +    Ppc4xxSdramState *s = PPC4xx_SDRAM(dev);
> > +    Ppc4xxDcrDeviceState *dcr = PPC4xx_DCR_DEVICE(dev);
> > +    int i;
> > +
> > +    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> > +
> > +    ppc4xx_dcr_register(dcr, SDRAM0_CFGADDR, &dcr_read_sdram, 
> > &dcr_write_sdram);
> > +    ppc4xx_dcr_register(dcr, SDRAM0_CFGDATA, &dcr_read_sdram, 
> > &dcr_write_sdram);
> > +
> > +    if (!s->nb_ram_bases || s->nb_ram_bases != s->nb_ram_sizes) {
> > +        error_setg(errp, "Invalid number of RAM banks");
> > +        return;
> > +    }
> > +
> > +    s->ram_memories = g_new0(MemoryRegion, s->nb_ram_bases);
> > +    for (i = 0; i < s->nb_ram_bases; i++) {
> > +        g_autofree char *name = g_strdup_printf(TYPE_PPC4xx_SDRAM "%d", i);
> > +
> > +        if (!sdram_bcr(s->ram_bases[i], s->ram_sizes[i])) {
> > +            error_setg(errp, "Invalid RAM size 0x%" HWADDR_PRIx,
> > +                       s->ram_sizes[i]);
> > +            return;
> > +        }
> > +
> > +        memory_region_init_alias(&s->ram_memories[i], OBJECT(s), name,
> > +                                 s->dram_mr, s->ram_bases[i], s->ram_sizes[i]);
> > +    }
> > +
> > +    s->nbanks = s->nb_ram_sizes;
> > +    if (s->dram_init) {
> > +        sdram_map_bcr(s);
> > +    }
> > +}
> > +
> > +static Property ppc4xx_sdram_properties[] = {
> > +    DEFINE_PROP_LINK("dram", Ppc4xxSdramState, dram_mr, TYPE_MEMORY_REGION,
> > +                     MemoryRegion *),
> > +    DEFINE_PROP_BOOL("dram-init", Ppc4xxSdramState, dram_init, false),
> > +    DEFINE_PROP_ARRAY("ram-sizes", Ppc4xxSdramState, nb_ram_sizes,
> > +                      ram_sizes, qdev_prop_uint64, uint64_t),
> > +    DEFINE_PROP_ARRAY("ram-bases", Ppc4xxSdramState, nb_ram_bases,
> > +                      ram_bases, qdev_prop_uint64, uint64_t),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void ppc4xx_sdram_class_init(ObjectClass *oc, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(oc);
> > +
> > +    dc->realize = ppc4xx_sdram_realize;
> > +    dc->user_creatable = false;
> > +    dc->reset = ppc4xx_sdram_reset;
> > +    device_class_set_props(dc, ppc4xx_sdram_properties);
> > +}
> > +
> > void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks,
> >                         MemoryRegion *ram_memories,
> >                         hwaddr *ram_bases,
> >                         hwaddr *ram_sizes,
> >                         int do_init)
> > {
> > -    ppc4xx_sdram_t *sdram;
> > +    Ppc4xxSdramState *sdram;
> >
> > -    sdram = g_new0(ppc4xx_sdram_t, 1);
> > +    sdram = g_new0(Ppc4xxSdramState, 1);
> >     sdram->irq = irq;
> >     sdram->nbanks = nbanks;
> >     sdram->ram_memories = ram_memories;
> > +
> > +    sdram->ram_bases = g_new0(hwaddr, 4);
> > +
> >     memset(sdram->ram_bases, 0, 4 * sizeof(hwaddr));
> >     memcpy(sdram->ram_bases, ram_bases,
> >            nbanks * sizeof(hwaddr));
> > +
> > +    sdram->ram_sizes = g_new0(hwaddr, 4);
> >     memset(sdram->ram_sizes, 0, 4 * sizeof(hwaddr));
> >     memcpy(sdram->ram_sizes, ram_sizes,
> >            nbanks * sizeof(hwaddr));
> > @@ -683,6 +727,11 @@ static void ppc4xx_dcr_class_init(ObjectClass *oc, void 
> > *data)
> >
> > static const TypeInfo ppc4xx_types[] = {
> >     {
> > +        .name           = TYPE_PPC4xx_SDRAM,
> > +        .parent         = TYPE_PPC4xx_DCR_DEVICE,
> > +        .instance_size  = sizeof(Ppc4xxSdramState),
> > +        .class_init     = ppc4xx_sdram_class_init,
> > +    }, {
> >         .name           = TYPE_PPC4xx_MAL,
> >         .parent         = TYPE_PPC4xx_DCR_DEVICE,
> >         .instance_size  = sizeof(Ppc4xxMalState),