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

[Cédric Le Goater]

[ Resending to all ]

On 8/4/22 14:09, BALATON Zoltan wrote:
> On Thu, 4 Aug 2022, Cédric Le Goater wrote:
> > On 8/4/22 01:36, Daniel Henrique Barboza wrote:
> > > Cedric,
> > >
> > > 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    | 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;
> > > >       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);
> > > >       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);
> > >
> > > This line is breaking the boot of sam460ex:
> > >
> > >
> > >   ./qemu-system-ppc64 -display none -M sam460ex
> > > Unexpected error in object_property_find_err() at ../qom/object.c:1304:
> > > qemu-system-ppc64: Property '460exb-powerpc64-cpu.cpu' not found
> > > Aborted (core dumped)
> > >
> > >
> > > I think you meant to link the cpu prop of the EBC obj to the CPU object,
> > > not the cpu prop of the CPU obj to the EBC dev.
> >
> > Yes. ppc405_ebc_init() has only one user left, the sam460ex, which I didn't
> > test :/
>
> This patch changes ppc405_ebc_init to a realize method so shouldn't the sam460ex be changed to create the new object instead of calling ppc405_ebc_init too instead? 

Sure.

First step was to make sure nothing was broken. I can add some extra
patches in v3 to convert ppc405_ebc_init(), ppc4xx_plb_init() and
ppc4xx_mal_init() in the ppc4x machines. I don't think that would be
too much work. It's a good opportunity to modernize a bit the ppc4xx
machines also.

> Is the only reason the keep ppc405_ebc_init to add the cpu link? 

Yes. That's all there is to it really: convert the routines
parameters in object properties.

> As I noted before it would be nice to get rid of this link somehow, 
> it would allow dropping this init func and a bunch of property 
> descriptors where this cpu link is the only object. 

We should introduce a DCR namespace instead and use DCR memory regions
but that's much more work.
 
> It should be possble to get from a QOM object to its parent and the 
> cpu from there but I could not find out how. Maybe somehow with 
> object_resolve_path() or object_resolve_path_type() but I don't know 
> QOM enough and did not find anything in docs.
>
> Does somebody know how to do that? 

One way, would be to introduce a base class DCRDeviceState with a "cpu"
link and a class handler to install the DCR read/write ops. But I think
it's not worth the time and effort. Adding DCR namespace and DCR memory
regions would be better.

Thanks,

C.



> Or maybe the paths are always the same so it could resolve an absolute path. Don't know how it looks buth something like /machine/soc/cpu or similar to get to the cpu to get the env. This could work as long as we assume we only have one cpu but these SoC all have. Then no cpu link is needed and 
> could get rid of a lot of boilerplate code. Does this make sense?
>
> Regards,
> BALATON Zoltan
>
> > Thanks,
> >
> > C.
> >
> > > This fixes the issue:
> > >
> > >
> > > $ git diff
> > > diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> > > index 0166f3fc36..aac3a3f761 100644
> > > --- a/hw/ppc/ppc405_uc.c
> > > +++ b/hw/ppc/ppc405_uc.c
> > > @@ -594,7 +594,7 @@ 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);
> > > +    object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);
> > >       qdev_realize_and_unref(dev, NULL, &error_fatal);
> > >   }
> > >
> > >
> > > Daniel
> > >
> > >
> > > > +    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);
> > > > +    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),