| Message ID | 50e79b2c5f2c17e2b6b7920dd6526b5c091ac8bb.1660402839.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QOMify PPC4xx devices and minor clean ups | expand |
On 8/13/22 17:34, BALATON Zoltan wrote: > From: Cédric Le Goater <clg@kaod.org> > > 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. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> When re-sending a patch, it is a good practice to list the changes before the Sob of the person doing the resend. I think you only changed the ppc4xx_dcr_register prototype. Correct ? Thanks, C. > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/ppc/ppc4xx_devs.c | 41 +++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/ppc4xx.h | 17 +++++++++++++++++ > 2 files changed, 58 insertions(+) > > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c > index 069b511951..f4d7ae9567 100644 > --- a/hw/ppc/ppc4xx_devs.c > +++ b/hw/ppc/ppc4xx_devs.c > @@ -664,3 +664,44 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum, > mal, &dcr_read_mal, &dcr_write_mal); > } > } > + > +/* PPC4xx_DCR_DEVICE */ > + > +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn, void *opaque, > + dcr_read_cb dcr_read, dcr_write_cb dcr_write) > +{ > + assert(dev->cpu); > + ppc_dcr_register(&dev->cpu->env, dcrn, opaque, 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) > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h > index 591e2421a3..a537a5567b 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, void *opaque, > + dcr_read_cb dcr_read, dcr_write_cb dcr_write); > +bool ppc4xx_dcr_realize(Ppc4xxDcrDeviceState *dev, PowerPCCPU *cpu, > + Error **errp); > + > #endif /* PPC4XX_H */
On Tue, 16 Aug 2022, Cédric Le Goater wrote: > On 8/13/22 17:34, BALATON Zoltan wrote: >> From: Cédric Le Goater <clg@kaod.org> >> >> 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. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > When re-sending a patch, it is a good practice to list the changes before > the Sob of the person doing the resend. > > I think you only changed the ppc4xx_dcr_register prototype. Correct ? Mostly, and the resulting rebase but maybe some small changes here and there but I think those are also just code style fixes. I did not know what you prefer with the from line so if you're OK with keeping it I can go through it again and mark changes before signed-off if you think that's better. I've also started cleaning up the sdram model, I need a bit more time for that, I'll probably send it as a separate series. > Thanks, > > C. > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/ppc/ppc4xx_devs.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/ppc4xx.h | 17 +++++++++++++++++ >> 2 files changed, 58 insertions(+) >> >> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c >> index 069b511951..f4d7ae9567 100644 >> --- a/hw/ppc/ppc4xx_devs.c >> +++ b/hw/ppc/ppc4xx_devs.c >> @@ -664,3 +664,44 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, >> uint8_t rxcnum, >> mal, &dcr_read_mal, &dcr_write_mal); >> } >> } >> + >> +/* PPC4xx_DCR_DEVICE */ >> + >> +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn, void >> *opaque, >> + dcr_read_cb dcr_read, dcr_write_cb dcr_write) >> +{ >> + assert(dev->cpu); >> + ppc_dcr_register(&dev->cpu->env, dcrn, opaque, 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) >> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h >> index 591e2421a3..a537a5567b 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, void >> *opaque, >> + dcr_read_cb dcr_read, dcr_write_cb dcr_write); >> +bool ppc4xx_dcr_realize(Ppc4xxDcrDeviceState *dev, PowerPCCPU *cpu, >> + Error **errp); >> + >> #endif /* PPC4XX_H */ > > >
On 8/16/22 11:33, BALATON Zoltan wrote: > On Tue, 16 Aug 2022, Cédric Le Goater wrote: >> On 8/13/22 17:34, BALATON Zoltan wrote: >>> From: Cédric Le Goater <clg@kaod.org> >>> >>> 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. >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> >> When re-sending a patch, it is a good practice to list the changes before >> the Sob of the person doing the resend. >> >> I think you only changed the ppc4xx_dcr_register prototype. Correct ? > > Mostly, and the resulting rebase but maybe some small changes here and there but I think those are also just code style fixes. I did not know what you prefer with the from line See commit 6a54ac2a9737 for next time. > so if you're OK with keeping it I can go through it again and mark changes before signed-off if you think that's better. Don't bother. I did a quick scan and didn't notice anything important. > I've also started cleaning up the sdram model, I need a bit more time for that, I'll probably send it as a separate series. OK. My initial patches were good enough for 405 and bamboo. May be keep the same basic idea. Thanks, C. >> Thanks, >> >> C. >> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/ppc/ppc4xx_devs.c | 41 +++++++++++++++++++++++++++++++++++++++++ >>> include/hw/ppc/ppc4xx.h | 17 +++++++++++++++++ >>> 2 files changed, 58 insertions(+) >>> >>> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c >>> index 069b511951..f4d7ae9567 100644 >>> --- a/hw/ppc/ppc4xx_devs.c >>> +++ b/hw/ppc/ppc4xx_devs.c >>> @@ -664,3 +664,44 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum, >>> mal, &dcr_read_mal, &dcr_write_mal); >>> } >>> } >>> + >>> +/* PPC4xx_DCR_DEVICE */ >>> + >>> +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn, void *opaque, >>> + dcr_read_cb dcr_read, dcr_write_cb dcr_write) >>> +{ >>> + assert(dev->cpu); >>> + ppc_dcr_register(&dev->cpu->env, dcrn, opaque, 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) >>> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h >>> index 591e2421a3..a537a5567b 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, void *opaque, >>> + 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 069b511951..f4d7ae9567 100644 --- a/hw/ppc/ppc4xx_devs.c +++ b/hw/ppc/ppc4xx_devs.c @@ -664,3 +664,44 @@ void ppc4xx_mal_init(CPUPPCState *env, uint8_t txcnum, uint8_t rxcnum, mal, &dcr_read_mal, &dcr_write_mal); } } + +/* PPC4xx_DCR_DEVICE */ + +void ppc4xx_dcr_register(Ppc4xxDcrDeviceState *dev, int dcrn, void *opaque, + dcr_read_cb dcr_read, dcr_write_cb dcr_write) +{ + assert(dev->cpu); + ppc_dcr_register(&dev->cpu->env, dcrn, opaque, 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) diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h index 591e2421a3..a537a5567b 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, void *opaque, + dcr_read_cb dcr_read, dcr_write_cb dcr_write); +bool ppc4xx_dcr_realize(Ppc4xxDcrDeviceState *dev, PowerPCCPU *cpu, + Error **errp); + #endif /* PPC4XX_H */