Re: [PATCH v3 08/22] ppc/ppc4xx: Introduce a DCR device model

[BALATON Zoltan]

On Mon, 8 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 a MMIO regions and/or IRQs. Being a SysBusDevice makes things

Drop "a", just "MMIO regions" due to plural. Also "makes it easier to 
install" or "makes things easier".

> 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    | 46 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> index 021376c2d260..bb373db0ba10 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"
>
> /* PowerPC 4xx core initialization */
> void ppc4xx_reset(void *opaque);
> @@ -50,4 +51,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"

Should it be named ppc4xx-dcr-device for clarity? This probably won't 
appear anywhere where users have to type it.

> +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 f20098cf417c..e07bdba0f912 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -696,3 +696,49 @@ 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)
> +{
> +    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);
> +
> +    dc->user_creatable = false;

Should this be .abstract instead? We expect this to not be used directly 
but only via SoC devices which is what abstract is for AFAIK.

> +    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,

Oh, it's abstract already. So does it also need user_creatable for an 
abstract class then? Maybe you can drop the user_creatable.

Regards,
BALATON Zoltan

> +    }
> +};
> +
> +DEFINE_TYPES(ppc4xx_types)