Re: [PATCH v4 08/24] ppc/ppc4xx: Introduce a DCR device model

[Cédric Le Goater]

On 8/10/22 15:28, BALATON Zoltan wrote:
> On Wed, 10 Aug 2022, Cédric Le Goater wrote:
> > On 8/9/22 19:21, BALATON Zoltan wrote:
> > > On Tue, 9 Aug 2022, Cédric Le Goater wrote:
> > > > The Device Control Registers (DCR) of on-SoC devices are accessed by
> > > > software through the use of the mtdcr and mfdcr instructions. These
> > > > are converted in transactions on a side band bus, the DCR bus, which
> > > > connects the on-SoC devices to the CPU.
> > > >
> > > > Ideally, we should model these accesses with a DCR namespace and DCR
> > > > memory regions but today the DCR handlers are installed in a DCR table
> > > > under the CPU. Instead introduce a little device model wrapper to hold
> > > > a CPU link and handle registration of DCR handlers.
> > > >
> > > > The DCR device inherits from SysBus because most of these devices also
> > > > have MMIO regions and/or IRQs. Being a SysBusDevice makes things easier
> > > > to install the device model in the overall SoC.
> > > >
> > > > The "cpu" link should be considered as modeling the piece of HW logic
> > > > connecting the device to the DCR bus.
> > > >
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > > include/hw/ppc/ppc4xx.h | 17 ++++++++++++++++
> > > > hw/ppc/ppc4xx_devs.c    | 44 +++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 61 insertions(+)
> > > >
> > > > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> > > > index 591e2421a343..82e60b0e0742 100644
> > > > --- a/include/hw/ppc/ppc4xx.h
> > > > +++ b/include/hw/ppc/ppc4xx.h
> > > > @@ -27,6 +27,7 @@
> > > >
> > > > #include "hw/ppc/ppc.h"
> > > > #include "exec/memory.h"
> > > > +#include "hw/sysbus.h"
> > > >
> > > > void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
> > > >                         MemoryRegion ram_memories[],
> > > > @@ -44,4 +45,20 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, 
> > > > uint8_t rxcnum,
> > > >
> > > > #define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
> > > >
> > > > +/*
> > > > + * Generic DCR device
> > > > + */
> > > > +#define TYPE_PPC4xx_DCR_DEVICE "ppc4xx-dcr-device"
> > > > +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);
> > > > +bool ppc4xx_dcr_realize(Ppc4xxDcrDeviceState *dev, PowerPCCPU *cpu,
> > > > +                        Error **errp);
> > > > +
> > > > #endif /* PPC4XX_H */
> > > > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > > > index 069b51195160..bce7ef461346 100644
> > > > --- a/hw/ppc/ppc4xx_devs.c
> > > > +++ b/hw/ppc/ppc4xx_devs.c
> > > > @@ -664,3 +664,47 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, 
> > > > uint8_t rxcnum,
> > > >                          mal, &dcr_read_mal, &dcr_write_mal);
> > > >     }
> > > > }
> > > > +
> > > > +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn,
> > > > +                         dcr_read_cb dcr_read, dcr_write_cb dcr_write)
> > >
> > > I still think this should have a separate void *opaque parameter for the callbacks and 
> > > not pass dev for that as the callbacks could use anything they wish for that parameter. 
> > > (Additionally this allows dropping a lot of QOM casts. If you want to see how often these 
> > > are accessed, you can try -trace enable="ppc_dcr*"; on the machines and OS I've 
> > > tested some are read/written frequently so I'd not add unnecessary overhead without a 
> > > good reason.)
> >
> > This machine has been abandoned for 15 years and broken for maybe 10.
> > I think it is fine for now. We will see if further needs arise.
>
> It will arise as I'd like to keep at least the devices used by sam460ex somewhat sane 

What do you mean by somewhat sane ? If it is the QOM casts, I don't
understand why you worry so much about it because QOM cast debugging
is not enabled by default. So it really should not impact performance
as you think it would.

C.

> so if you don't change this now I'd likely want to change it back. I undetstand 
> it's a hassle to do in a rebase now but keeping the opaque parameter means just 
> a search replace from the old ppc_dcr_register to ppc4xx_dcr_register so maybe 
> not that hard to do at this point. Once you apply this patch it will be more 
> difficult to revert it.
>
> Regards,
> BALATON Zoltan
>
> > Thanks,
> >
> > C.
> >
> > > Otherwise:
> > >
> > > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> > >
> > > Regards,
> > > BALATOn Zoltan
> > >
> > > > +{
> > > > +    CPUPPCState *env;
> > > > +
> > > > +    assert(dev->cpu);
> > > > +
> > > > +    env = &dev->cpu->env;
> > > > +
> > > > +    ppc_dcr_register(env, dcrn, dev, dcr_read, dcr_write);
> > > > +}
> > > > +
> > > > +bool ppc4xx_dcr_realize(Ppc4xxDcrDeviceState *dev, PowerPCCPU *cpu,
> > > > +                        Error **errp)
> > > > +{
> > > > +    object_property_set_link(OBJECT(dev), "cpu", OBJECT(cpu), &error_abort);
> > > > +    return sysbus_realize(SYS_BUS_DEVICE(dev), errp);
> > > > +}
> > > > +
> > > > +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);
> > > > +
> > > > +    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,
> > > > +    }
> > > > +};
> > > > +
> > > > +DEFINE_TYPES(ppc4xx_types)