Re: [PATCH v2 12/20] ppc/ppc405: QOM'ify EBC

[BALATON Zoltan]

On Wed, 3 Aug 2022, 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    | 16 +++++++++++
> hw/ppc/ppc405_uc.c | 71 +++++++++++++++++++++++++++++++---------------
> 2 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/hw/ppc/ppc405.h b/hw/ppc/ppc405.h
> index 1da34a7f10f3..1c7fe07b8084 100644
> --- a/hw/ppc/ppc405.h
> +++ b/hw/ppc/ppc405.h
> @@ -65,7 +65,22 @@ struct ppc4xx_bd_info_t {
>
> typedef struct Ppc405SoCState Ppc405SoCState;
>
> +/* Peripheral controller */
> +#define TYPE_PPC405_EBC "ppc405-ebc"
> +OBJECT_DECLARE_SIMPLE_TYPE(Ppc405EbcState, PPC405_EBC);
> +struct Ppc405EbcState {
> +    DeviceState parent_obj;
> +
> +    PowerPCCPU *cpu;
>
> +    uint32_t addr;
> +    uint32_t bcr[8];
> +    uint32_t bap[8];
> +    uint32_t bear;
> +    uint32_t besr0;
> +    uint32_t besr1;
> +    uint32_t cfg;
> +};
>
> /* DMA controller */
> #define TYPE_PPC405_DMA "ppc405-dma"
> @@ -203,6 +218,7 @@ struct Ppc405SoCState {
>     Ppc405OcmState ocm;
>     Ppc405GpioState gpio;
>     Ppc405DmaState dma;
> +    Ppc405EbcState ebc;
> };
>
> /* PowerPC 405 core */
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index 6bd93c1cb90c..0166f3fc36da 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -393,17 +393,6 @@ static void ppc4xx_opba_init(hwaddr base)
>
> /*****************************************************************************/
> /* Peripheral controller */
> -typedef struct ppc4xx_ebc_t ppc4xx_ebc_t;
> -struct ppc4xx_ebc_t {
> -    uint32_t addr;
> -    uint32_t bcr[8];
> -    uint32_t bap[8];
> -    uint32_t bear;
> -    uint32_t besr0;
> -    uint32_t besr1;
> -    uint32_t cfg;
> -};
> -
> enum {
>     EBC0_CFGADDR = 0x012,
>     EBC0_CFGDATA = 0x013,
> @@ -411,10 +400,9 @@ enum {
>
> static uint32_t dcr_read_ebc (void *opaque, int dcrn)
> {
> -    ppc4xx_ebc_t *ebc;
> +    Ppc405EbcState *ebc = PPC405_EBC(opaque);
>     uint32_t ret;
>
> -    ebc = opaque;

I think QOM casts are kind of expensive (maybe because we have quo-debug 
enabled by default even without --enable-debug and it does additional 
checks; I've tried to change this default once but it was thought to be 
better to have it enabled). So it's advised to use QOM casts sparingly, 
e.g. store the result in a local variable if you need it more than once 
and so. Therefore I tend to consider these read/write callbacks that the 
object itself registers with itself as the opaque pointer to be internal 
to the object and guaranteed to be passed the object pointer so no QOM 
cast is necessary and the direct assignment can be kept. This avoids 
potential overhead on every register access. Not sure if it's measurable 
but I think if an overhead can be avoided it probably should be.

>     switch (dcrn) {
>     case EBC0_CFGADDR:
>         ret = ebc->addr;
> @@ -496,9 +484,8 @@ static uint32_t dcr_read_ebc (void *opaque, int dcrn)
>
> static void dcr_write_ebc (void *opaque, int dcrn, uint32_t val)
> {
> -    ppc4xx_ebc_t *ebc;
> +    Ppc405EbcState *ebc = PPC405_EBC(opaque);
>
> -    ebc = opaque;
>     switch (dcrn) {
>     case EBC0_CFGADDR:
>         ebc->addr = val;
> @@ -554,12 +541,11 @@ static void dcr_write_ebc (void *opaque, int dcrn, 
> uint32_t val)
>     }
> }
>
> -static void ebc_reset (void *opaque)
> +static void ppc405_ebc_reset(DeviceState *dev)
> {
> -    ppc4xx_ebc_t *ebc;
> +    Ppc405EbcState *ebc = PPC405_EBC(dev);

In this case the cast is OK as it's casting a different object so it's 
needed and also it's infrequently called so should not matter.

>     int i;
>
> -    ebc = opaque;
>     ebc->addr = 0x00000000;
>     ebc->bap[0] = 0x7F8FFE80;
>     ebc->bcr[0] = 0xFFE28000;
> @@ -572,18 +558,46 @@ static void ebc_reset (void *opaque)
>     ebc->cfg = 0x80400000;
> }
>
> -void ppc405_ebc_init(CPUPPCState *env)
> +static void ppc405_ebc_realize(DeviceState *dev, Error **errp)
> {
> -    ppc4xx_ebc_t *ebc;
> +    Ppc405EbcState *ebc = PPC405_EBC(dev);
> +    CPUPPCState *env;
> +
> +    assert(ebc->cpu);
> +
> +    env = &ebc->cpu->env;
>
> -    ebc = g_new0(ppc4xx_ebc_t, 1);
> -    qemu_register_reset(&ebc_reset, ebc);
>     ppc_dcr_register(env, EBC0_CFGADDR,
>                      ebc, &dcr_read_ebc, &dcr_write_ebc);
>     ppc_dcr_register(env, EBC0_CFGDATA,
>                      ebc, &dcr_read_ebc, &dcr_write_ebc);
> }
>
> +static Property ppc405_ebc_properties[] = {
> +    DEFINE_PROP_LINK("cpu", Ppc405EbcState, cpu, TYPE_POWERPC_CPU,
> +                     PowerPCCPU *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void ppc405_ebc_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = ppc405_ebc_realize;
> +    dc->user_creatable = false;
> +    dc->reset = ppc405_ebc_reset;
> +    device_class_set_props(dc, ppc405_ebc_properties);
> +}
> +
> +void ppc405_ebc_init(CPUPPCState *env)
> +{
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    DeviceState *dev = qdev_new(TYPE_PPC405_EBC);
> +
> +    object_property_set_link(OBJECT(cpu), "cpu", OBJECT(dev), &error_abort);
> +    qdev_realize_and_unref(dev, NULL, &error_fatal);
> +}
> +
> /*****************************************************************************/
> /* DMA controller */
> enum {
> @@ -1418,6 +1432,8 @@ static void ppc405_soc_instance_init(Object *obj)
>     object_initialize_child(obj, "gpio", &s->gpio, TYPE_PPC405_GPIO);
>
>     object_initialize_child(obj, "dma", &s->dma, TYPE_PPC405_DMA);
> +
> +    object_initialize_child(obj, "ebc", &s->ebc, TYPE_PPC405_EBC);
> }
>
> static void ppc405_soc_realize(DeviceState *dev, Error **errp)
> @@ -1490,7 +1506,11 @@ static void ppc405_soc_realize(DeviceState *dev, Error 
> **errp)
>                       s->ram_bases, s->ram_sizes, s->do_dram_init);
>
>     /* External bus controller */
> -    ppc405_ebc_init(env);
> +    object_property_set_link(OBJECT(&s->ebc), "cpu", OBJECT(&s->cpu),
> +                             &error_abort);

I wonder if this link to cpu could be avoided somehow? Maybe assuming that 
this device and the cpu is part of the same SoC it could get it's parent 
and access the cpu field of the parent or if that's not possible adding a 
method to the SoC to get it could avoid this link?

Regards,
BALATON Zoltan

> +    if (!qdev_realize(DEVICE(&s->ebc), NULL, errp)) {
> +        return;
> +    }
>
>     /* DMA controller */
>     object_property_set_link(OBJECT(&s->dma), "cpu", OBJECT(&s->cpu),
> @@ -1576,6 +1596,11 @@ static void ppc405_soc_class_init(ObjectClass *oc, void 
> *data)
>
> static const TypeInfo ppc405_types[] = {
>     {
> +        .name           = TYPE_PPC405_EBC,
> +        .parent         = TYPE_DEVICE,
> +        .instance_size  = sizeof(Ppc405EbcState),
> +        .class_init     = ppc405_ebc_class_init,
> +    }, {
>         .name           = TYPE_PPC405_DMA,
>         .parent         = TYPE_SYS_BUS_DEVICE,
>         .instance_size  = sizeof(Ppc405DmaState),