| Message ID | 20220808102734.133084-9-clg@kaod.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ppc: QOM'ify 405 board | expand |
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) >
On 8/8/22 15:29, BALATON Zoltan wrote: > 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. ok. > >> +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. Indeed. Thanks, C. > Regards, > BALATON Zoltan > >> + } >> +}; >> + >> +DEFINE_TYPES(ppc4xx_types) >>
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" +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; + 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)
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 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(+)