Re: [PATCH v2 16/20] ppc/ppc405: QOM'ify MAL

[Daniel Henrique Barboza <danielhb413@gmail.com>]

This patch really broke sam460ex boot, but not because of the
QOMification. I managed to get it work by doing the following:

On 8/3/22 10:28, Cédric Le Goater wrote:
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/ppc/ppc405.h         |   1 +
>   include/hw/ppc/ppc4xx.h |  28 ++++++++++
>   hw/ppc/ppc405_uc.c      |  20 +++++--
>   hw/ppc/ppc4xx_devs.c    | 120 +++++++++++++++++++++++++---------------
>   4 files changed, 118 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 8ca32f35ce67..7d585a244d18 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -259,6 +259,7 @@ struct Ppc405SoCState {
>       Ppc405OpbaState opba;
>       Ppc405PobState pob;
>       Ppc405PlbState plb;
> +    Ppc4xxMalState mal;
>   };
>   
>   /* PowerPC 405 core */
> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> index 021376c2d260..c31219265273 100644
> --- a/include/hw/ppc/ppc4xx.h
> +++ b/include/hw/ppc/ppc4xx.h
> @@ -26,6 +26,7 @@
>   #define PPC4XX_H
>   
>   #include "hw/ppc/ppc.h"
> +#include "hw/sysbus.h"
>   #include "exec/memory.h"
>   
>   /* PowerPC 4xx core initialization */
> @@ -45,6 +46,33 @@ void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks,
>                           hwaddr *ram_sizes,
>                           int do_init);
>   
> +/* Memory Access Layer (MAL) */
> +#define TYPE_PPC4xx_MAL "ppc4xx-mal"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc4xxMalState, PPC4xx_MAL);
> +struct Ppc4xxMalState {
> +    SysBusDevice parent_obj;
> +
> +    PowerPCCPU *cpu;
> +
> +    qemu_irq irqs[4];
> +    uint32_t cfg;
> +    uint32_t esr;
> +    uint32_t ier;
> +    uint32_t txcasr;
> +    uint32_t txcarr;
> +    uint32_t txeobisr;
> +    uint32_t txdeir;
> +    uint32_t rxcasr;
> +    uint32_t rxcarr;
> +    uint32_t rxeobisr;
> +    uint32_t rxdeir;
> +    uint32_t *txctpr;
> +    uint32_t *rxctpr;
> +    uint32_t *rcbs;
> +    uint8_t  txcnum;
> +    uint8_t  rxcnum;
> +};
> +
>   void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum,
>                        qemu_irq irqs[4]);
>   
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 9bbd524ad5ea..f39e0b44f9cc 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -1466,12 +1466,13 @@ static void ppc405_soc_instance_init(Object *obj)
>       object_initialize_child(obj, "pob", &s->pob, TYPE_PPC405_POB);
>   
>       object_initialize_child(obj, "plb", &s->plb, TYPE_PPC405_PLB);
> +
> +    object_initialize_child(obj, "mal", &s->mal, TYPE_PPC4xx_MAL);
>   }
>   
>   static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>   {
>       Ppc405SoCState *s = PPC405_SOC(dev);
> -    qemu_irq mal_irqs[4];
>       CPUPPCState *env;
>       Error *err = NULL;
>       int i;
> @@ -1610,11 +1611,18 @@ static void ppc405_soc_realize(DeviceState *dev, Error **errp)
>       }
>   
>       /* MAL */
> -    mal_irqs[0] = qdev_get_gpio_in(s->uic, 11);
> -    mal_irqs[1] = qdev_get_gpio_in(s->uic, 12);
> -    mal_irqs[2] = qdev_get_gpio_in(s->uic, 13);
> -    mal_irqs[3] = qdev_get_gpio_in(s->uic, 14);
> -    ppc4xx_mal_init(env, 4, 2, mal_irqs);
> +    object_property_set_int(OBJECT(&s->mal), "txc-num", 4, &error_abort);
> +    object_property_set_int(OBJECT(&s->mal), "rxc-num", 2, &error_abort);
> +    object_property_set_link(OBJECT(&s->mal), "cpu", OBJECT(&s->cpu),
> +                             &error_abort);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->mal), errp)) {
> +        return;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(s->mal.irqs); i++) {
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->mal), i,
> +                           qdev_get_gpio_in(s->uic, 11 + i));
> +    }
>   
>       /* Ethernet */
>       /* Uses UIC IRQs 9, 15, 17 */
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index f20098cf417c..0e97347e2839 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -491,32 +491,10 @@ enum {
>       MAL0_RCBS1    = 0x1E1,
>   };
>   
> -typedef struct ppc4xx_mal_t ppc4xx_mal_t;
> -struct ppc4xx_mal_t {
> -    qemu_irq irqs[4];
> -    uint32_t cfg;
> -    uint32_t esr;
> -    uint32_t ier;
> -    uint32_t txcasr;
> -    uint32_t txcarr;
> -    uint32_t txeobisr;
> -    uint32_t txdeir;
> -    uint32_t rxcasr;
> -    uint32_t rxcarr;
> -    uint32_t rxeobisr;
> -    uint32_t rxdeir;
> -    uint32_t *txctpr;
> -    uint32_t *rxctpr;
> -    uint32_t *rcbs;
> -    uint8_t  txcnum;
> -    uint8_t  rxcnum;
> -};
> -
> -static void ppc4xx_mal_reset(void *opaque)
> +static void ppc4xx_mal_reset(DeviceState *dev)
>   {
> -    ppc4xx_mal_t *mal;
> +    Ppc4xxMalState *mal = PPC4xx_MAL(dev);
>   
> -    mal = opaque;
>       mal->cfg = 0x0007C000;
>       mal->esr = 0x00000000;
>       mal->ier = 0x00000000;
> @@ -530,10 +508,9 @@ static void ppc4xx_mal_reset(void *opaque)
>   
>   static uint32_t dcr_read_mal(void *opaque, int dcrn)
>   {
> -    ppc4xx_mal_t *mal;
> +    Ppc4xxMalState *mal = PPC4xx_MAL(opaque);
>       uint32_t ret;
>   
> -    mal = opaque;
>       switch (dcrn) {
>       case MAL0_CFG:
>           ret = mal->cfg;
> @@ -587,13 +564,12 @@ static uint32_t dcr_read_mal(void *opaque, int dcrn)
>   
>   static void dcr_write_mal(void *opaque, int dcrn, uint32_t val)
>   {
> -    ppc4xx_mal_t *mal;
> +    Ppc4xxMalState *mal = PPC4xx_MAL(opaque);
>   
> -    mal = opaque;
>       switch (dcrn) {
>       case MAL0_CFG:
>           if (val & 0x80000000) {
> -            ppc4xx_mal_reset(mal);
> +            ppc4xx_mal_reset(DEVICE(mal));
>           }
>           mal->cfg = val & 0x00FFC087;
>           break;
> @@ -644,23 +620,30 @@ static void dcr_write_mal(void *opaque, int dcrn, uint32_t val)
>       }
>   }
>   
> -void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum,
> -                     qemu_irq irqs[4])
> +static void ppc4xx_mal_realize(DeviceState *dev, Error **errp)
>   {
> -    ppc4xx_mal_t *mal;
> +    Ppc4xxMalState *mal = PPC4xx_MAL(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    CPUPPCState *env;
>       int i;
>   
> -    assert(txcnum <= 32 && rxcnum <= 32);
> -    mal = g_malloc0(sizeof(*mal));
> -    mal->txcnum = txcnum;
> -    mal->rxcnum = rxcnum;
> -    mal->txctpr = g_new0(uint32_t, txcnum);
> -    mal->rxctpr = g_new0(uint32_t, rxcnum);
> -    mal->rcbs = g_new0(uint32_t, rxcnum);
> -    for (i = 0; i < 4; i++) {
> -        mal->irqs[i] = irqs[i];
> +    assert(mal->cpu);
> +
> +    env = &mal->cpu->env;
> +
> +    if (mal->txcnum > 32 || mal->rxcnum > 32) {
> +        error_setg(errp, "invalid TXC/RXC number");
> +        return;
>       }
> -    qemu_register_reset(&ppc4xx_mal_reset, mal);
> +
> +    mal->txctpr = g_new0(uint32_t, mal->txcnum);
> +    mal->rxctpr = g_new0(uint32_t, mal->rxcnum);
> +    mal->rcbs = g_new0(uint32_t, mal->rxcnum);
> +
> +    for (i = 0; i < ARRAY_SIZE(mal->irqs); i++) {
> +        sysbus_init_irq(sbd, &mal->irqs[i]);
> +    }
> +
>       ppc_dcr_register(env, MAL0_CFG,
>                        mal, &dcr_read_mal, &dcr_write_mal);
>       ppc_dcr_register(env, MAL0_ESR,
> @@ -683,16 +666,63 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum,
>                        mal, &dcr_read_mal, &dcr_write_mal);
>       ppc_dcr_register(env, MAL0_RXDEIR,
>                        mal, &dcr_read_mal, &dcr_write_mal);
> -    for (i = 0; i < txcnum; i++) {
> +    for (i = 0; i < mal->txcnum; i++) {
>           ppc_dcr_register(env, MAL0_TXCTP0R + i,
>                            mal, &dcr_read_mal, &dcr_write_mal);
>       }
> -    for (i = 0; i < rxcnum; i++) {
> +    for (i = 0; i < mal->rxcnum; i++) {
>           ppc_dcr_register(env, MAL0_RXCTP0R + i,
>                            mal, &dcr_read_mal, &dcr_write_mal);
>       }
> -    for (i = 0; i < rxcnum; i++) {
> +    for (i = 0; i < mal->rxcnum; i++) {
>           ppc_dcr_register(env, MAL0_RCBS0 + i,
>                            mal, &dcr_read_mal, &dcr_write_mal);
>       }
>   }
> +
> +static Property ppc4xx_mal_properties[] = {
> +    DEFINE_PROP_UINT8("txc-num", Ppc4xxMalState, txcnum, 0),
> +    DEFINE_PROP_UINT8("rxc-num", Ppc4xxMalState, rxcnum, 0),
> +    DEFINE_PROP_LINK("cpu", Ppc4xxMalState, cpu, TYPE_POWERPC_CPU,
> +                     PowerPCCPU *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc4xx_mal_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc4xx_mal_realize;
> +    dc->user_creatable = false;
> +    dc->reset = ppc4xx_mal_reset;
> +    device_class_set_props(dc, ppc4xx_mal_properties);
> +}
> +
> +void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum,
> +                     qemu_irq irqs[4])
> +{
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    DeviceState *dev = qdev_new(TYPE_PPC4xx_MAL);
> +    Ppc4xxMalState *mal = PPC4xx_MAL(dev);
> +    int i;
> +
> +    qdev_prop_set_uint32(dev, "txc-num", txcnum);
> +    qdev_prop_set_uint32(dev, "rxc-num", rxcnum);
> +    object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);

Changed this to:

      object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);


> +    qdev_realize_and_unref(dev, NULL, &error_fatal);

Changed this to:

     sysbus_realize(SYS_BUS_DEVICE(dev), &error_fatal);

Because qdev complained that MAL is a sysbus device.


> +
> +    for (i = 0; i < ARRAY_SIZE(mal->irqs); i++) {
> +        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, irqs[i]);
> +    }

And this line broke sam460ex because, back in sam460ex.c, we're not storing
all the GPIO lines in sam460ex_init():


      for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
         mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
      }
      ppc4xx_mal_init(env, 4, 16, mal_irqs);

We're just storing the last line in mal_irqs[0]. sysbus_connect_irq() gets really
upset about it.


Since this is a separated bug (which is eligible for the freeze) I sent a patch
to fix it standalone. Your series can then be rebased on top of that fix.



Daniel

> +}
> +
> +static const TypeInfo ppc4xx_types[] = {
> +    {
> +        .name           = TYPE_PPC4xx_MAL,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(Ppc4xxMalState),
> +        .class_init     = ppc4xx_mal_class_init,
> +    }
> +};
> +
> +DEFINE_TYPES(ppc4xx_types)