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

[Cédric Le Goater]

On 8/6/22 11:38, BALATON Zoltan wrote:
> On Fri, 5 Aug 2022, BALATON Zoltan wrote:
> > On Fri, 5 Aug 2022, Peter Maydell wrote:
> > > On Fri, 5 Aug 2022 at 17:50, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > > > This also
> > > > allows to get the cpu without a link with something like:
> > > >
> > > > PPC4XX_MACHINE(current_machine /* or qdev_get_machine() */)->soc.cpu
> > >
> > > ...and now you have device code that's making assumptions about
> > > the machine and SoC it's in.
> >
> > Since these devices are strictly part of this SoC making this asumption should 
> > be OK. Making assumption about the machine may be a valid argument
>
> I think this is my really last comment on this topic but I can't stop thinking about this. If we can't user parent or get the cpu any other way and we have to use a link property then we probably need a PPC4xxSocDevice abstract class that has the cpu link and all these devices derive from that. 

When experimenting, I added a PPC4xx_DCR_DEVICE implementing what
you are proposing. I didn't keep it because the benefits are limited
and adding a DCR address space would be a better change. If you think
it would help QOMifying the other PPC4xx parts, I can include it in v3.

See below,

> That way the boilerplate for the link property is confined to this SocDevice 
> class and does not have to be repeated in every other SoC device. Unless QOM 
> properties don't work that way and they would need to be aliased in which case 
> it does not bring anything.

You still have to set the object link before "realizing" each object.

Thanks,

C.


#define TYPE_PPC4xx_DCR_DEVICE "ppc4xx-dcr"
OBJECT_DECLARE_SIMPLE_TYPE(Ppc4xxDcrDeviceState, PPC4xx_DCR_DEVICE);
struct Ppc4xxDcrDeviceState {
    SysBusDevice parent_obj;

    PowerPCCPU *cpu;
};

void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn,
                         dcr_read_cb dcr_read, dcr_write_cb dcr_write)
{
    CPUPPCState *env;

    assert(dev->cpu);

    env = &dev->cpu->env;

    ppc_dcr_register(env, dcrn, dev, dcr_read, dcr_write);
}

static Property ppc4xx_dcr_properties[] = {
    DEFINE_PROP_LINK("cpu", Ppc4xxDcrDeviceState, cpu, TYPE_POWERPC_CPU,
                     PowerPCCPU *),
    DEFINE_PROP_END_OF_LIST(),
};

static void ppc4xx_dcr_class_init(ObjectClass *oc, void *data)
{
    DeviceClass *dc = DEVICE_CLASS(oc);

    dc->user_creatable = false;
    device_class_set_props(dc, ppc4xx_dcr_properties);
}

static const TypeInfo ppc4xx_types[] = {
   {
        .name           = TYPE_PPC4xx_DCR_DEVICE,
        .parent         = TYPE_SYS_BUS_DEVICE,
        .instance_size  = sizeof(Ppc4xxDcrDeviceState),
        .class_init     = ppc4xx_dcr_class_init,
        .abstract       = true,
   }
};