| Message ID | 20231026164741.1184058-6-ninad@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce model for IBM's FSI | expand |
On 10/26/23 18:47, Ninad Palsule wrote: > This is a part of patchset where IBM's Flexible Service Interface is > introduced. > > The On-Chip Peripheral Bus (OPB): A low-speed bus typically found in > POWER processors. This now makes an appearance in the ASPEED SoC due > to tight integration of the FSI master IP with the OPB, mainly the > existence of an MMIO-mapping of the CFAM address straight onto a > sub-region of the OPB address space. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> > Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > v2: > - Incorporated review comment by Joel. > v5: > - Incorporated review comments by Cedric. > v6: > - Incorporated review comments by Cedric & Daniel > v7: > - Incorporated review comments by Cedric. > --- > include/hw/fsi/opb.h | 33 ++++++++++++++++++++ > hw/fsi/fsi-master.c | 3 +- > hw/fsi/opb.c | 74 ++++++++++++++++++++++++++++++++++++++++++++ > hw/fsi/Kconfig | 4 +++ > hw/fsi/meson.build | 1 + > 5 files changed, 113 insertions(+), 2 deletions(-) > create mode 100644 include/hw/fsi/opb.h > create mode 100644 hw/fsi/opb.c > > diff --git a/include/hw/fsi/opb.h b/include/hw/fsi/opb.h > new file mode 100644 > index 0000000000..8b71bb55c2 > --- /dev/null > +++ b/include/hw/fsi/opb.h > @@ -0,0 +1,33 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * Copyright (C) 2023 IBM Corp. > + * > + * IBM On-Chip Peripheral Bus > + */ > +#ifndef FSI_OPB_H > +#define FSI_OPB_H > + > +#include "exec/memory.h" > +#include "hw/fsi/fsi-master.h" > + > +#define TYPE_OP_BUS "opb" > +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS) > + > +typedef struct OPBus { > + /*< private >*/ > + BusState bus; > + > + /*< public >*/ > + MemoryRegion mr; > + AddressSpace as; > + > + /* Model OPB as dumb enough just to provide an address-space */ > + /* TODO: Maybe don't store device state in the bus? */ > + FSIMasterState fsi; Please remove. FSIMasterState should be introduced later. > +} OPBus; > + > +typedef struct OPBusClass { > + BusClass parent_class; > +} OPBusClass; > + > +#endif /* FSI_OPB_H */ > diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c > index bb7a893003..ec092b42ea 100644 > --- a/hw/fsi/fsi-master.c > +++ b/hw/fsi/fsi-master.c > @@ -11,8 +11,7 @@ > #include "trace.h" > > #include "hw/fsi/fsi-master.h" > - > -#define TYPE_OP_BUS "opb" > +#include "hw/fsi/opb.h" ouch. This is an ugly hack because the modeling is broken. OPB should be introduced before tFSIMasterState. > #define TO_REG(x) ((x) >> 2) > > diff --git a/hw/fsi/opb.c b/hw/fsi/opb.c > new file mode 100644 > index 0000000000..04771b4b27 > --- /dev/null > +++ b/hw/fsi/opb.c > @@ -0,0 +1,74 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * Copyright (C) 2023 IBM Corp. > + * > + * IBM On-chip Peripheral Bus > + */ > + > +#include "qemu/osdep.h" > + > +#include "qapi/error.h" > +#include "qemu/log.h" > + > +#include "hw/fsi/opb.h" > + > +static void fsi_opb_realize(BusState *bus, Error **errp) > +{ > + OPBus *opb = OP_BUS(bus); > + > + memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb, > + NULL, UINT32_MAX); > + address_space_init(&opb->as, &opb->mr, "opb"); Please keep the above and put it in a instance_init handler and remove the rest. It should go under AspeedAPB2OPBState. > + > + if (!object_property_set_bool(OBJECT(&opb->fsi), "realized", true, errp)) { > + return; > + } > + > + memory_region_add_subregion(&opb->mr, 0x80000000, &opb->fsi.iomem); > + > + /* OPB2FSI region */ > + /* > + * Avoid endianness issues by mapping each slave's memory region directly. > + * Manually bridging multiple address-spaces causes endian swapping > + * headaches as memory_region_dispatch_read() and > + * memory_region_dispatch_write() correct the endianness based on the > + * target machine endianness and not relative to the device endianness on > + * either side of the bridge. > + */ > + /* > + * XXX: This is a bit hairy and will need to be fixed when I sort out the > + * bus/slave relationship and any changes to the CFAM modelling (multiple > + * slaves, LBUS) > + */ > + memory_region_add_subregion(&opb->mr, 0xa0000000, &opb->fsi.opb2fsi); > +} > + > +static void fsi_opb_init(Object *o) > +{ > + OPBus *opb = OP_BUS(o); > + > + object_initialize_child(o, "fsi-master", &opb->fsi, TYPE_FSI_MASTER); > + qdev_set_parent_bus(DEVICE(&opb->fsi), BUS(o), &error_abort); > +} Drop all of the above. Thanks, C. > + > +static void fsi_opb_class_init(ObjectClass *klass, void *data) > +{ > + BusClass *bc = BUS_CLASS(klass); > + bc->realize = fsi_opb_realize; > +} > + > +static const TypeInfo opb_info = { > + .name = TYPE_OP_BUS, > + .parent = TYPE_BUS, > + .instance_init = fsi_opb_init, > + .instance_size = sizeof(OPBus), > + .class_init = fsi_opb_class_init, > + .class_size = sizeof(OPBusClass), > +}; > + > +static void fsi_opb_register_types(void) > +{ > + type_register_static(&opb_info); > +} > + > +type_init(fsi_opb_register_types); > diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig > index 8d712e77ed..0f6e6d331a 100644 > --- a/hw/fsi/Kconfig > +++ b/hw/fsi/Kconfig > @@ -1,3 +1,7 @@ > +config FSI_OPB > + bool > + select FSI_CFAM > + > config FSI_CFAM > bool > select FSI > diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build > index f617943b4a..407b8c2775 100644 > --- a/hw/fsi/meson.build > +++ b/hw/fsi/meson.build > @@ -2,3 +2,4 @@ system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c')) > system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c')) > system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c')) > system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi.c','fsi-master.c','fsi-slave.c')) > +system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))
Hello Cedric, On 11/27/23 10:23, Cédric Le Goater wrote: > On 10/26/23 18:47, Ninad Palsule wrote: >> This is a part of patchset where IBM's Flexible Service Interface is >> introduced. >> >> The On-Chip Peripheral Bus (OPB): A low-speed bus typically found in >> POWER processors. This now makes an appearance in the ASPEED SoC due >> to tight integration of the FSI master IP with the OPB, mainly the >> existence of an MMIO-mapping of the CFAM address straight onto a >> sub-region of the OPB address space. >> >> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com> >> Reviewed-by: Joel Stanley <joel@jms.id.au> >> --- >> v2: >> - Incorporated review comment by Joel. >> v5: >> - Incorporated review comments by Cedric. >> v6: >> - Incorporated review comments by Cedric & Daniel >> v7: >> - Incorporated review comments by Cedric. >> --- >> include/hw/fsi/opb.h | 33 ++++++++++++++++++++ >> hw/fsi/fsi-master.c | 3 +- >> hw/fsi/opb.c | 74 ++++++++++++++++++++++++++++++++++++++++++++ >> hw/fsi/Kconfig | 4 +++ >> hw/fsi/meson.build | 1 + >> 5 files changed, 113 insertions(+), 2 deletions(-) >> create mode 100644 include/hw/fsi/opb.h >> create mode 100644 hw/fsi/opb.c >> >> diff --git a/include/hw/fsi/opb.h b/include/hw/fsi/opb.h >> new file mode 100644 >> index 0000000000..8b71bb55c2 >> --- /dev/null >> +++ b/include/hw/fsi/opb.h >> @@ -0,0 +1,33 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * Copyright (C) 2023 IBM Corp. >> + * >> + * IBM On-Chip Peripheral Bus >> + */ >> +#ifndef FSI_OPB_H >> +#define FSI_OPB_H >> + >> +#include "exec/memory.h" >> +#include "hw/fsi/fsi-master.h" >> + >> +#define TYPE_OP_BUS "opb" >> +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS) >> + >> +typedef struct OPBus { >> + /*< private >*/ >> + BusState bus; >> + >> + /*< public >*/ >> + MemoryRegion mr; >> + AddressSpace as; >> + >> + /* Model OPB as dumb enough just to provide an address-space */ >> + /* TODO: Maybe don't store device state in the bus? */ >> + FSIMasterState fsi; > > Please remove. FSIMasterState should be introduced later. ok, Thanks for the change. Looks like you moved it from OPBus to AspeedAPB2OPBState. I am fine with the change if it is making model cleaner. > >> +} OPBus; >> + >> +typedef struct OPBusClass { >> + BusClass parent_class; >> +} OPBusClass; >> + >> +#endif /* FSI_OPB_H */ >> diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c >> index bb7a893003..ec092b42ea 100644 >> --- a/hw/fsi/fsi-master.c >> +++ b/hw/fsi/fsi-master.c >> @@ -11,8 +11,7 @@ >> #include "trace.h" >> #include "hw/fsi/fsi-master.h" >> - >> -#define TYPE_OP_BUS "opb" >> +#include "hw/fsi/opb.h" > > ouch. > > This is an ugly hack because the modeling is broken. OPB should be > introduced > before tFSIMasterState. OK, Thanks for the change. > > >> #define TO_REG(x) ((x) >> 2) >> diff --git a/hw/fsi/opb.c b/hw/fsi/opb.c >> new file mode 100644 >> index 0000000000..04771b4b27 >> --- /dev/null >> +++ b/hw/fsi/opb.c >> @@ -0,0 +1,74 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * Copyright (C) 2023 IBM Corp. >> + * >> + * IBM On-chip Peripheral Bus >> + */ >> + >> +#include "qemu/osdep.h" >> + >> +#include "qapi/error.h" >> +#include "qemu/log.h" >> + >> +#include "hw/fsi/opb.h" >> + >> +static void fsi_opb_realize(BusState *bus, Error **errp) >> +{ >> + OPBus *opb = OP_BUS(bus); >> + >> + memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb, >> + NULL, UINT32_MAX); >> + address_space_init(&opb->as, &opb->mr, "opb"); > > Please keep the above and put it in a instance_init handler and remove > the rest. It should go under AspeedAPB2OPBState. OK, Thanks for the change. > >> + >> + if (!object_property_set_bool(OBJECT(&opb->fsi), "realized", >> true, errp)) { >> + return; >> + } >> + >> + memory_region_add_subregion(&opb->mr, 0x80000000, &opb->fsi.iomem); >> + >> + /* OPB2FSI region */ >> + /* >> + * Avoid endianness issues by mapping each slave's memory region >> directly. >> + * Manually bridging multiple address-spaces causes endian swapping >> + * headaches as memory_region_dispatch_read() and >> + * memory_region_dispatch_write() correct the endianness based >> on the >> + * target machine endianness and not relative to the device >> endianness on >> + * either side of the bridge. >> + */ >> + /* >> + * XXX: This is a bit hairy and will need to be fixed when I >> sort out the >> + * bus/slave relationship and any changes to the CFAM modelling >> (multiple >> + * slaves, LBUS) >> + */ >> + memory_region_add_subregion(&opb->mr, 0xa0000000, >> &opb->fsi.opb2fsi); >> +} >> + >> +static void fsi_opb_init(Object *o) >> +{ >> + OPBus *opb = OP_BUS(o); >> + >> + object_initialize_child(o, "fsi-master", &opb->fsi, >> TYPE_FSI_MASTER); >> + qdev_set_parent_bus(DEVICE(&opb->fsi), BUS(o), &error_abort); >> +} > > Drop all of the above. OK, Thanks for the review and change. Regards, Ninad > > > Thanks, > > C. > > > >> + >> +static void fsi_opb_class_init(ObjectClass *klass, void *data) >> +{ >> + BusClass *bc = BUS_CLASS(klass); >> + bc->realize = fsi_opb_realize; >> +} >> + >> +static const TypeInfo opb_info = { >> + .name = TYPE_OP_BUS, >> + .parent = TYPE_BUS, >> + .instance_init = fsi_opb_init, >> + .instance_size = sizeof(OPBus), >> + .class_init = fsi_opb_class_init, >> + .class_size = sizeof(OPBusClass), >> +}; >> + >> +static void fsi_opb_register_types(void) >> +{ >> + type_register_static(&opb_info); >> +} >> + >> +type_init(fsi_opb_register_types); >> diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig >> index 8d712e77ed..0f6e6d331a 100644 >> --- a/hw/fsi/Kconfig >> +++ b/hw/fsi/Kconfig >> @@ -1,3 +1,7 @@ >> +config FSI_OPB >> + bool >> + select FSI_CFAM >> + >> config FSI_CFAM >> bool >> select FSI >> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build >> index f617943b4a..407b8c2775 100644 >> --- a/hw/fsi/meson.build >> +++ b/hw/fsi/meson.build >> @@ -2,3 +2,4 @@ system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: >> files('lbus.c')) >> system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: >> files('engine-scratchpad.c')) >> system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c')) >> system_ss.add(when: 'CONFIG_FSI', if_true: >> files('fsi.c','fsi-master.c','fsi-slave.c')) >> +system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c')) >
diff --git a/include/hw/fsi/opb.h b/include/hw/fsi/opb.h new file mode 100644 index 0000000000..8b71bb55c2 --- /dev/null +++ b/include/hw/fsi/opb.h @@ -0,0 +1,33 @@ +/* + * SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (C) 2023 IBM Corp. + * + * IBM On-Chip Peripheral Bus + */ +#ifndef FSI_OPB_H +#define FSI_OPB_H + +#include "exec/memory.h" +#include "hw/fsi/fsi-master.h" + +#define TYPE_OP_BUS "opb" +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS) + +typedef struct OPBus { + /*< private >*/ + BusState bus; + + /*< public >*/ + MemoryRegion mr; + AddressSpace as; + + /* Model OPB as dumb enough just to provide an address-space */ + /* TODO: Maybe don't store device state in the bus? */ + FSIMasterState fsi; +} OPBus; + +typedef struct OPBusClass { + BusClass parent_class; +} OPBusClass; + +#endif /* FSI_OPB_H */ diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c index bb7a893003..ec092b42ea 100644 --- a/hw/fsi/fsi-master.c +++ b/hw/fsi/fsi-master.c @@ -11,8 +11,7 @@ #include "trace.h" #include "hw/fsi/fsi-master.h" - -#define TYPE_OP_BUS "opb" +#include "hw/fsi/opb.h" #define TO_REG(x) ((x) >> 2) diff --git a/hw/fsi/opb.c b/hw/fsi/opb.c new file mode 100644 index 0000000000..04771b4b27 --- /dev/null +++ b/hw/fsi/opb.c @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (C) 2023 IBM Corp. + * + * IBM On-chip Peripheral Bus + */ + +#include "qemu/osdep.h" + +#include "qapi/error.h" +#include "qemu/log.h" + +#include "hw/fsi/opb.h" + +static void fsi_opb_realize(BusState *bus, Error **errp) +{ + OPBus *opb = OP_BUS(bus); + + memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb, + NULL, UINT32_MAX); + address_space_init(&opb->as, &opb->mr, "opb"); + + if (!object_property_set_bool(OBJECT(&opb->fsi), "realized", true, errp)) { + return; + } + + memory_region_add_subregion(&opb->mr, 0x80000000, &opb->fsi.iomem); + + /* OPB2FSI region */ + /* + * Avoid endianness issues by mapping each slave's memory region directly. + * Manually bridging multiple address-spaces causes endian swapping + * headaches as memory_region_dispatch_read() and + * memory_region_dispatch_write() correct the endianness based on the + * target machine endianness and not relative to the device endianness on + * either side of the bridge. + */ + /* + * XXX: This is a bit hairy and will need to be fixed when I sort out the + * bus/slave relationship and any changes to the CFAM modelling (multiple + * slaves, LBUS) + */ + memory_region_add_subregion(&opb->mr, 0xa0000000, &opb->fsi.opb2fsi); +} + +static void fsi_opb_init(Object *o) +{ + OPBus *opb = OP_BUS(o); + + object_initialize_child(o, "fsi-master", &opb->fsi, TYPE_FSI_MASTER); + qdev_set_parent_bus(DEVICE(&opb->fsi), BUS(o), &error_abort); +} + +static void fsi_opb_class_init(ObjectClass *klass, void *data) +{ + BusClass *bc = BUS_CLASS(klass); + bc->realize = fsi_opb_realize; +} + +static const TypeInfo opb_info = { + .name = TYPE_OP_BUS, + .parent = TYPE_BUS, + .instance_init = fsi_opb_init, + .instance_size = sizeof(OPBus), + .class_init = fsi_opb_class_init, + .class_size = sizeof(OPBusClass), +}; + +static void fsi_opb_register_types(void) +{ + type_register_static(&opb_info); +} + +type_init(fsi_opb_register_types); diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig index 8d712e77ed..0f6e6d331a 100644 --- a/hw/fsi/Kconfig +++ b/hw/fsi/Kconfig @@ -1,3 +1,7 @@ +config FSI_OPB + bool + select FSI_CFAM + config FSI_CFAM bool select FSI diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build index f617943b4a..407b8c2775 100644 --- a/hw/fsi/meson.build +++ b/hw/fsi/meson.build @@ -2,3 +2,4 @@ system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c')) system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c')) system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c')) system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi.c','fsi-master.c','fsi-slave.c')) +system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))