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;
+} 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'))
--
2.39.2On 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'))
>
© 2016 - 2024 Red Hat, Inc.