* [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 14:53 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 2/9] hw/fsi: Introduce IBM's FSI Bus and FSI slave Ninad Palsule
` (8 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm, Andrew Jeffery
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
The LBUS is modelled to maintain mapped memory for the devices. The
memory is mapped after CFAM config, peek table and FSI slave registers.
The scratchpad provides a set of non-functional registers. The firmware
is free to use them, hardware does not support any special management
support. The scratchpad registers can be read or written from LBUS
slave. The scratch pad is managed under FSI CFAM state.
[ clg: - removed lbus_add_device() bc unused
- removed lbus_create_device() bc used only once
- removed "address" property
- updated meson.build to build fsi dir
- included an empty hw/fsi/trace-events ]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v9:
- Changed LBUS memory region to 1MB.
---
meson.build | 1 +
hw/fsi/trace.h | 1 +
include/hw/fsi/lbus.h | 52 ++++++++++++++++++
hw/fsi/lbus.c | 121 ++++++++++++++++++++++++++++++++++++++++++
hw/Kconfig | 1 +
hw/fsi/Kconfig | 2 +
hw/fsi/meson.build | 1 +
hw/fsi/trace-events | 2 +
hw/meson.build | 1 +
9 files changed, 182 insertions(+)
create mode 100644 hw/fsi/trace.h
create mode 100644 include/hw/fsi/lbus.h
create mode 100644 hw/fsi/lbus.c
create mode 100644 hw/fsi/Kconfig
create mode 100644 hw/fsi/meson.build
create mode 100644 hw/fsi/trace-events
diff --git a/meson.build b/meson.build
index 371edafae6..498d08b866 100644
--- a/meson.build
+++ b/meson.build
@@ -3273,6 +3273,7 @@ if have_system
'hw/char',
'hw/display',
'hw/dma',
+ 'hw/fsi',
'hw/hyperv',
'hw/i2c',
'hw/i386',
diff --git a/hw/fsi/trace.h b/hw/fsi/trace.h
new file mode 100644
index 0000000000..ee67c7fb04
--- /dev/null
+++ b/hw/fsi/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-hw_fsi.h"
diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h
new file mode 100644
index 0000000000..8bacdded7f
--- /dev/null
+++ b/include/hw/fsi/lbus.h
@@ -0,0 +1,52 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * IBM Local bus and connected device structures.
+ */
+#ifndef FSI_LBUS_H
+#define FSI_LBUS_H
+
+#include "hw/qdev-core.h"
+#include "qemu/units.h"
+#include "exec/memory.h"
+
+#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
+OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
+
+#define FSI_LBUS_MEM_REGION_SIZE (1 * MiB)
+#define FSI_LBUSDEV_IOMEM_START 0xc00 /* 3K used by CFAM config etc */
+
+typedef struct FSILBusDevice {
+ DeviceState parent;
+
+ MemoryRegion iomem;
+} FSILBusDevice;
+
+typedef struct FSILBusDeviceClass {
+ DeviceClass parent;
+
+ uint32_t config;
+} FSILBusDeviceClass;
+
+#define TYPE_FSI_LBUS "fsi.lbus"
+OBJECT_DECLARE_SIMPLE_TYPE(FSILBus, FSI_LBUS)
+
+typedef struct FSILBus {
+ BusState bus;
+
+ MemoryRegion mr;
+} FSILBus;
+
+#define TYPE_FSI_SCRATCHPAD "fsi.scratchpad"
+#define SCRATCHPAD(obj) OBJECT_CHECK(FSIScratchPad, (obj), TYPE_FSI_SCRATCHPAD)
+
+#define FSI_SCRATCHPAD_NR_REGS 4
+
+typedef struct FSIScratchPad {
+ FSILBusDevice parent;
+
+ uint32_t reg[FSI_SCRATCHPAD_NR_REGS];
+} FSIScratchPad;
+
+#endif /* FSI_LBUS_H */
diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c
new file mode 100644
index 0000000000..34c450cc68
--- /dev/null
+++ b/hw/fsi/lbus.c
@@ -0,0 +1,121 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * IBM Local bus where FSI slaves are connected
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/fsi/lbus.h"
+
+#include "hw/qdev-properties.h"
+
+#include "trace.h"
+
+static void lbus_init(Object *o)
+{
+ FSILBus *lbus = FSI_LBUS(o);
+
+ memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS,
+ FSI_LBUS_MEM_REGION_SIZE - FSI_LBUSDEV_IOMEM_START);
+}
+
+static const TypeInfo lbus_info = {
+ .name = TYPE_FSI_LBUS,
+ .parent = TYPE_BUS,
+ .instance_init = lbus_init,
+ .instance_size = sizeof(FSILBus),
+};
+
+static void lbus_device_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->bus_type = TYPE_FSI_LBUS;
+}
+
+static const TypeInfo lbus_device_type_info = {
+ .name = TYPE_FSI_LBUS_DEVICE,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(FSILBusDevice),
+ .abstract = true,
+ .class_init = lbus_device_class_init,
+ .class_size = sizeof(FSILBusDeviceClass),
+};
+
+static uint64_t fsi_scratchpad_read(void *opaque, hwaddr addr, unsigned size)
+{
+ FSIScratchPad *s = SCRATCHPAD(opaque);
+
+ trace_fsi_scratchpad_read(addr, size);
+
+ if (addr & ~(FSI_SCRATCHPAD_NR_REGS - 1)) {
+ return 0;
+ }
+
+ return s->reg[addr];
+}
+
+static void fsi_scratchpad_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ FSIScratchPad *s = SCRATCHPAD(opaque);
+
+ trace_fsi_scratchpad_write(addr, size, data);
+
+ if (addr & ~(FSI_SCRATCHPAD_NR_REGS - 1)) {
+ return;
+ }
+
+ s->reg[addr] = data;
+}
+
+static const struct MemoryRegionOps scratchpad_ops = {
+ .read = fsi_scratchpad_read,
+ .write = fsi_scratchpad_write,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void fsi_scratchpad_realize(DeviceState *dev, Error **errp)
+{
+ FSILBusDevice *ldev = FSI_LBUS_DEVICE(dev);
+
+ memory_region_init_io(&ldev->iomem, OBJECT(ldev), &scratchpad_ops,
+ ldev, TYPE_FSI_SCRATCHPAD, 0x400);
+}
+
+static void fsi_scratchpad_reset(DeviceState *dev)
+{
+ FSIScratchPad *s = SCRATCHPAD(dev);
+ int i;
+
+ for (i = 0; i < FSI_SCRATCHPAD_NR_REGS; i++) {
+ s->reg[i] = 0;
+ }
+}
+
+static void fsi_scratchpad_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = fsi_scratchpad_realize;
+ dc->reset = fsi_scratchpad_reset;
+}
+
+static const TypeInfo fsi_scratchpad_info = {
+ .name = TYPE_FSI_SCRATCHPAD,
+ .parent = TYPE_FSI_LBUS_DEVICE,
+ .instance_size = sizeof(FSIScratchPad),
+ .class_init = fsi_scratchpad_class_init,
+ .class_size = sizeof(FSILBusDeviceClass),
+};
+
+static void lbus_register_types(void)
+{
+ type_register_static(&lbus_info);
+ type_register_static(&lbus_device_type_info);
+ type_register_static(&fsi_scratchpad_info);
+}
+
+type_init(lbus_register_types);
diff --git a/hw/Kconfig b/hw/Kconfig
index 9ca7b38c31..2c00936c28 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -9,6 +9,7 @@ source core/Kconfig
source cxl/Kconfig
source display/Kconfig
source dma/Kconfig
+source fsi/Kconfig
source gpio/Kconfig
source hyperv/Kconfig
source i2c/Kconfig
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
new file mode 100644
index 0000000000..9c34a418d7
--- /dev/null
+++ b/hw/fsi/Kconfig
@@ -0,0 +1,2 @@
+config FSI
+ bool
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
new file mode 100644
index 0000000000..93ba19dd04
--- /dev/null
+++ b/hw/fsi/meson.build
@@ -0,0 +1 @@
+system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
new file mode 100644
index 0000000000..c5753e2791
--- /dev/null
+++ b/hw/fsi/trace-events
@@ -0,0 +1,2 @@
+fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
diff --git a/hw/meson.build b/hw/meson.build
index f01fac4617..463d702683 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -44,6 +44,7 @@ subdir('virtio')
subdir('watchdog')
subdir('xen')
subdir('xenpv')
+subdir('fsi')
subdir('alpha')
subdir('arm')
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad
2024-01-10 23:15 ` [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad Ninad Palsule
@ 2024-01-12 14:53 ` Cédric Le Goater
2024-01-25 23:29 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 14:53 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
On 1/11/24 00:15, Ninad Palsule wrote:
> This is a part of patchset where IBM's Flexible Service Interface is
> introduced.
>
> The LBUS is modelled to maintain mapped memory for the devices. The
> memory is mapped after CFAM config, peek table and FSI slave registers.
>
> The scratchpad provides a set of non-functional registers. The firmware
> is free to use them, hardware does not support any special management
> support. The scratchpad registers can be read or written from LBUS
> slave. The scratch pad is managed under FSI CFAM state.
This scratchpad mode should be in its own patch and thanks for the
extending the number of registers
> [ clg: - removed lbus_add_device() bc unused
> - removed lbus_create_device() bc used only once
> - removed "address" property
> - updated meson.build to build fsi dir
> - included an empty hw/fsi/trace-events ]
this list of modifications should be before my S-o-b.
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v9:
> - Changed LBUS memory region to 1MB.
> ---
> meson.build | 1 +
> hw/fsi/trace.h | 1 +
> include/hw/fsi/lbus.h | 52 ++++++++++++++++++
> hw/fsi/lbus.c | 121 ++++++++++++++++++++++++++++++++++++++++++
> hw/Kconfig | 1 +
> hw/fsi/Kconfig | 2 +
> hw/fsi/meson.build | 1 +
> hw/fsi/trace-events | 2 +
> hw/meson.build | 1 +
> 9 files changed, 182 insertions(+)
> create mode 100644 hw/fsi/trace.h
> create mode 100644 include/hw/fsi/lbus.h
> create mode 100644 hw/fsi/lbus.c
> create mode 100644 hw/fsi/Kconfig
> create mode 100644 hw/fsi/meson.build
> create mode 100644 hw/fsi/trace-events
>
> diff --git a/meson.build b/meson.build
> index 371edafae6..498d08b866 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3273,6 +3273,7 @@ if have_system
> 'hw/char',
> 'hw/display',
> 'hw/dma',
> + 'hw/fsi',
> 'hw/hyperv',
> 'hw/i2c',
> 'hw/i386',
> diff --git a/hw/fsi/trace.h b/hw/fsi/trace.h
> new file mode 100644
> index 0000000000..ee67c7fb04
> --- /dev/null
> +++ b/hw/fsi/trace.h
> @@ -0,0 +1 @@
> +#include "trace/trace-hw_fsi.h"
> diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h
> new file mode 100644
> index 0000000000..8bacdded7f
> --- /dev/null
> +++ b/include/hw/fsi/lbus.h
> @@ -0,0 +1,52 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2024 IBM Corp.
> + *
> + * IBM Local bus and connected device structures.
> + */
> +#ifndef FSI_LBUS_H
> +#define FSI_LBUS_H
> +
> +#include "hw/qdev-core.h"
> +#include "qemu/units.h"
> +#include "exec/memory.h"
> +
> +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
> +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
> +
> +#define FSI_LBUS_MEM_REGION_SIZE (1 * MiB)
> +#define FSI_LBUSDEV_IOMEM_START 0xc00 /* 3K used by CFAM config etc */
These define are not very useful. Please remove (see comments in
lbus.c)
> +
> +typedef struct FSILBusDevice {
> + DeviceState parent;
> +
> + MemoryRegion iomem;
> +} FSILBusDevice;
> +
> +typedef struct FSILBusDeviceClass {
> + DeviceClass parent;
> +
> + uint32_t config;
> +} FSILBusDeviceClass;
This class is unused now.
> +
> +#define TYPE_FSI_LBUS "fsi.lbus"
> +OBJECT_DECLARE_SIMPLE_TYPE(FSILBus, FSI_LBUS)
> +
> +typedef struct FSILBus {
> + BusState bus;
> +
> + MemoryRegion mr;
> +} FSILBus;
> +
> +#define TYPE_FSI_SCRATCHPAD "fsi.scratchpad"
> +#define SCRATCHPAD(obj) OBJECT_CHECK(FSIScratchPad, (obj), TYPE_FSI_SCRATCHPAD)
> +
> +#define FSI_SCRATCHPAD_NR_REGS 4
> +
> +typedef struct FSIScratchPad {
> + FSILBusDevice parent;
> +
> + uint32_t reg[FSI_SCRATCHPAD_NR_REGS];
> +} FSIScratchPad;
> +
> +#endif /* FSI_LBUS_H */
> diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c
> new file mode 100644
> index 0000000000..34c450cc68
> --- /dev/null
> +++ b/hw/fsi/lbus.c
> @@ -0,0 +1,121 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2024 IBM Corp.
> + *
> + * IBM Local bus where FSI slaves are connected
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/fsi/lbus.h"
> +
> +#include "hw/qdev-properties.h"
> +
> +#include "trace.h"
> +
> +static void lbus_init(Object *o)
I would prefix the lbus routine and struct with fsi_lbus to be
consistent with the types and macro.
> +{
> + FSILBus *lbus = FSI_LBUS(o);
> +
> + memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS,
> + FSI_LBUS_MEM_REGION_SIZE - FSI_LBUSDEV_IOMEM_START);
This is enough :
memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS, 1 * MiB);
> +}
> +
> +static const TypeInfo lbus_info = {
> + .name = TYPE_FSI_LBUS,
> + .parent = TYPE_BUS,
> + .instance_init = lbus_init,
> + .instance_size = sizeof(FSILBus),
> +};
> +
> +static void lbus_device_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->bus_type = TYPE_FSI_LBUS;
> +}
> +
> +static const TypeInfo lbus_device_type_info = {
> + .name = TYPE_FSI_LBUS_DEVICE,
> + .parent = TYPE_DEVICE,
> + .instance_size = sizeof(FSILBusDevice),
> + .abstract = true,
> + .class_init = lbus_device_class_init,
> + .class_size = sizeof(FSILBusDeviceClass),
> +};
> +
> +static uint64_t fsi_scratchpad_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + FSIScratchPad *s = SCRATCHPAD(opaque);
> +
> + trace_fsi_scratchpad_read(addr, size);
> +
> + if (addr & ~(FSI_SCRATCHPAD_NR_REGS - 1)) {
> + return 0;
> + }
> +
> + return s->reg[addr];
> +}
> +
> +static void fsi_scratchpad_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + FSIScratchPad *s = SCRATCHPAD(opaque);
> +
> + trace_fsi_scratchpad_write(addr, size, data);
> +
> + if (addr & ~(FSI_SCRATCHPAD_NR_REGS - 1)) {
There is a type confusion. addr is an offset in a memory region and
FSI_SCRATCHPAD_NR_REGS is an index an array.
> + return;
> + }
> +
> + s->reg[addr] = data;
same here.
> +}
> +
> +static const struct MemoryRegionOps scratchpad_ops = {
> + .read = fsi_scratchpad_read,
> + .write = fsi_scratchpad_write,
> + .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void fsi_scratchpad_realize(DeviceState *dev, Error **errp)
> +{
> + FSILBusDevice *ldev = FSI_LBUS_DEVICE(dev);
> +
> + memory_region_init_io(&ldev->iomem, OBJECT(ldev), &scratchpad_ops,
> + ldev, TYPE_FSI_SCRATCHPAD, 0x400);
> +}
> +
> +static void fsi_scratchpad_reset(DeviceState *dev)
> +{
> + FSIScratchPad *s = SCRATCHPAD(dev);
> + int i;
> +
> + for (i = 0; i < FSI_SCRATCHPAD_NR_REGS; i++) {
> + s->reg[i] = 0;
memset(s->regs, 0, sizeof(s->regs));
Thanks,
C.
> + }
> +}
> +
> +static void fsi_scratchpad_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = fsi_scratchpad_realize;
> + dc->reset = fsi_scratchpad_reset;
> +}
> +
> +static const TypeInfo fsi_scratchpad_info = {
> + .name = TYPE_FSI_SCRATCHPAD,
> + .parent = TYPE_FSI_LBUS_DEVICE,
> + .instance_size = sizeof(FSIScratchPad),
> + .class_init = fsi_scratchpad_class_init,
> + .class_size = sizeof(FSILBusDeviceClass),
> +};
> +
> +static void lbus_register_types(void)
> +{
> + type_register_static(&lbus_info);
> + type_register_static(&lbus_device_type_info);
> + type_register_static(&fsi_scratchpad_info);
> +}
> +
> +type_init(lbus_register_types);
> diff --git a/hw/Kconfig b/hw/Kconfig
> index 9ca7b38c31..2c00936c28 100644
> --- a/hw/Kconfig
> +++ b/hw/Kconfig
> @@ -9,6 +9,7 @@ source core/Kconfig
> source cxl/Kconfig
> source display/Kconfig
> source dma/Kconfig
> +source fsi/Kconfig
> source gpio/Kconfig
> source hyperv/Kconfig
> source i2c/Kconfig
> diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
> new file mode 100644
> index 0000000000..9c34a418d7
> --- /dev/null
> +++ b/hw/fsi/Kconfig
> @@ -0,0 +1,2 @@
> +config FSI
> + bool
> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> new file mode 100644
> index 0000000000..93ba19dd04
> --- /dev/null
> +++ b/hw/fsi/meson.build
> @@ -0,0 +1 @@
> +system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c'))
> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
> new file mode 100644
> index 0000000000..c5753e2791
> --- /dev/null
> +++ b/hw/fsi/trace-events
> @@ -0,0 +1,2 @@
> +fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> diff --git a/hw/meson.build b/hw/meson.build
> index f01fac4617..463d702683 100644
> --- a/hw/meson.build
> +++ b/hw/meson.build
> @@ -44,6 +44,7 @@ subdir('virtio')
> subdir('watchdog')
> subdir('xen')
> subdir('xenpv')
> +subdir('fsi')
>
> subdir('alpha')
> subdir('arm')
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad
2024-01-12 14:53 ` Cédric Le Goater
@ 2024-01-25 23:29 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-25 23:29 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Cedric,
>
>
>> [ clg: - removed lbus_add_device() bc unused
>> - removed lbus_create_device() bc used only once
>> - removed "address" property
>> - updated meson.build to build fsi dir
>> - included an empty hw/fsi/trace-events ]
>
> this list of modifications should be before my S-o-b.
Moved it to correct place.
>> +
>> +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
>> +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
>> +
>> +#define FSI_LBUS_MEM_REGION_SIZE (1 * MiB)
>> +#define FSI_LBUSDEV_IOMEM_START 0xc00 /* 3K used by CFAM config
>> etc */
>
> These define are not very useful. Please remove (see comments in
> lbus.c)
Removed.
>
>>
>> +typedef struct FSILBusDeviceClass {
>> + DeviceClass parent;
>> +
>> + uint32_t config;
>> +} FSILBusDeviceClass;
>
> This class is unused now.
Removed the class. We will add it once we add more devices.
>
>>
>> +#include "trace.h"
>> +
>> +static void lbus_init(Object *o)
>
> I would prefix the lbus routine and struct with fsi_lbus to be
> consistent with the types and macro.
Added prefix fsi_ for all routines.
>
>> +{
>> + FSILBus *lbus = FSI_LBUS(o);
>> +
>> + memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS,
>> + FSI_LBUS_MEM_REGION_SIZE -
>> FSI_LBUSDEV_IOMEM_START);
>
> This is enough :
>
> memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS, 1 * MiB);
Changed as per your suggestion.
>
>>
>> +
>> +static void fsi_scratchpad_write(void *opaque, hwaddr addr, uint64_t
>> data,
>> + unsigned size)
>> +{
>> + FSIScratchPad *s = SCRATCHPAD(opaque);
>> +
>> + trace_fsi_scratchpad_write(addr, size, data);
>> +
>> + if (addr & ~(FSI_SCRATCHPAD_NR_REGS - 1)) {
>
> There is a type confusion. addr is an offset in a memory region and
> FSI_SCRATCHPAD_NR_REGS is an index an array.
Fixed.
>
>
>> + return;
>> + }
>> +
>> + s->reg[addr] = data;
>
> same here.
Fixed.
>
>>
>> +static void fsi_scratchpad_reset(DeviceState *dev)
>> +{
>> + FSIScratchPad *s = SCRATCHPAD(dev);
>> + int i;
>> +
>> + for (i = 0; i < FSI_SCRATCHPAD_NR_REGS; i++) {
>> + s->reg[i] = 0;
>
> memset(s->regs, 0, sizeof(s->regs));
Fixed.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 2/9] hw/fsi: Introduce IBM's FSI Bus and FSI slave
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
2024-01-10 23:15 ` [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 15:03 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam Ninad Palsule
` (7 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm, Andrew Jeffery
This is a part of patchset where FSI bus is introduced.
The FSI bus is a simple bus where FSI master is attached.
The FSI slave: The slave is the terminal point of the FSI bus for
FSI symbols addressed to it. Slaves can be cascaded off of one
another. The slave's configuration registers appear in address space
of the CFAM to which it is attached.
[ clg: - removed include/hw/fsi/engine-scratchpad.h and
hw/fsi/engine-scratchpad.c
- dropped FSI_SCRATCHPAD
- included FSIBus definition
- dropped hw/fsi/trace-events changes ]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
include/hw/fsi/fsi.h | 38 +++++++++++++++
hw/fsi/fsi.c | 111 +++++++++++++++++++++++++++++++++++++++++++
hw/fsi/meson.build | 2 +-
hw/fsi/trace-events | 2 +
4 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 include/hw/fsi/fsi.h
create mode 100644 hw/fsi/fsi.c
diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
new file mode 100644
index 0000000000..6e11747dd5
--- /dev/null
+++ b/include/hw/fsi/fsi.h
@@ -0,0 +1,38 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * IBM Flexible Service Interface
+ */
+#ifndef FSI_FSI_H
+#define FSI_FSI_H
+
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
+#include "hw/fsi/lbus.h"
+#include "qemu/bitops.h"
+
+/* Bitwise operations at the word level. */
+#define BE_BIT(x) BIT(31 - (x))
+#define BE_GENMASK(hb, lb) MAKE_64BIT_MASK((lb), ((hb) - (lb) + 1))
+
+#define TYPE_FSI_BUS "fsi.bus"
+OBJECT_DECLARE_SIMPLE_TYPE(FSIBus, FSI_BUS)
+
+typedef struct FSIBus {
+ BusState bus;
+} FSIBus;
+
+#define TYPE_FSI_SLAVE "fsi.slave"
+OBJECT_DECLARE_SIMPLE_TYPE(FSISlaveState, FSI_SLAVE)
+
+#define FSI_SLAVE_CONTROL_NR_REGS ((0x40 >> 2) + 1)
+
+typedef struct FSISlaveState {
+ DeviceState parent;
+
+ MemoryRegion iomem;
+ uint32_t regs[FSI_SLAVE_CONTROL_NR_REGS];
+} FSISlaveState;
+
+#endif /* FSI_FSI_H */
diff --git a/hw/fsi/fsi.c b/hw/fsi/fsi.c
new file mode 100644
index 0000000000..0c73ca14ad
--- /dev/null
+++ b/hw/fsi/fsi.c
@@ -0,0 +1,111 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * IBM Flexible Service Interface
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/fsi.h"
+
+static const TypeInfo fsi_bus_info = {
+ .name = TYPE_FSI_BUS,
+ .parent = TYPE_BUS,
+ .instance_size = sizeof(FSIBus),
+};
+
+static void fsi_bus_register_types(void)
+{
+ type_register_static(&fsi_bus_info);
+}
+
+type_init(fsi_bus_register_types);
+
+#define TO_REG(x) ((x) >> 2)
+
+static uint64_t fsi_slave_read(void *opaque, hwaddr addr, unsigned size)
+{
+ FSISlaveState *s = FSI_SLAVE(opaque);
+ int reg = TO_REG(addr);
+
+ trace_fsi_slave_read(addr, size);
+
+ if (reg >= FSI_SLAVE_CONTROL_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds read: 0x%"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return 0;
+ }
+
+ return s->regs[reg];
+}
+
+static void fsi_slave_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ FSISlaveState *s = FSI_SLAVE(opaque);
+ int reg = TO_REG(addr);
+
+ trace_fsi_slave_write(addr, size, data);
+
+ if (reg >= FSI_SLAVE_CONTROL_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds write: 0x%"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ s->regs[reg] = data;
+}
+
+static const struct MemoryRegionOps fsi_slave_ops = {
+ .read = fsi_slave_read,
+ .write = fsi_slave_write,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void fsi_slave_reset(DeviceState *dev)
+{
+ FSISlaveState *s = FSI_SLAVE(dev);
+ int i;
+
+ /* Initialize registers */
+ for (i = 0; i < FSI_SLAVE_CONTROL_NR_REGS; i++) {
+ s->regs[i] = 0;
+ }
+}
+
+static void fsi_slave_init(Object *o)
+{
+ FSISlaveState *s = FSI_SLAVE(o);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &fsi_slave_ops,
+ s, TYPE_FSI_SLAVE, 0x400);
+}
+
+static void fsi_slave_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->bus_type = TYPE_FSI_BUS;
+ dc->desc = "FSI Slave";
+ dc->reset = fsi_slave_reset;
+}
+
+static const TypeInfo fsi_slave_info = {
+ .name = TYPE_FSI_SLAVE,
+ .parent = TYPE_DEVICE,
+ .instance_init = fsi_slave_init,
+ .instance_size = sizeof(FSISlaveState),
+ .class_init = fsi_slave_class_init,
+};
+
+static void fsi_slave_register_types(void)
+{
+ type_register_static(&fsi_slave_info);
+}
+
+type_init(fsi_slave_register_types);
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index 93ba19dd04..574f5f9289 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -1 +1 @@
-system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c'))
+system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index c5753e2791..8f29adb7df 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -1,2 +1,4 @@
fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+fsi_slave_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_slave_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 2/9] hw/fsi: Introduce IBM's FSI Bus and FSI slave
2024-01-10 23:15 ` [PATCH v10 2/9] hw/fsi: Introduce IBM's FSI Bus and FSI slave Ninad Palsule
@ 2024-01-12 15:03 ` Cédric Le Goater
2024-01-25 23:59 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 15:03 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
On 1/11/24 00:15, Ninad Palsule wrote:
> This is a part of patchset where FSI bus is introduced.
>
> The FSI bus is a simple bus where FSI master is attached.
>
> The FSI slave: The slave is the terminal point of the FSI bus for
> FSI symbols addressed to it. Slaves can be cascaded off of one
> another. The slave's configuration registers appear in address space
> of the CFAM to which it is attached.
Please add another patch.
> [ clg: - removed include/hw/fsi/engine-scratchpad.h and
> hw/fsi/engine-scratchpad.c
> - dropped FSI_SCRATCHPAD
> - included FSIBus definition
> - dropped hw/fsi/trace-events changes ]
Move the list down before my S-o-b.
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> include/hw/fsi/fsi.h | 38 +++++++++++++++
> hw/fsi/fsi.c | 111 +++++++++++++++++++++++++++++++++++++++++++
> hw/fsi/meson.build | 2 +-
> hw/fsi/trace-events | 2 +
> 4 files changed, 152 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/fsi/fsi.h
> create mode 100644 hw/fsi/fsi.c
>
> diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
> new file mode 100644
> index 0000000000..6e11747dd5
> --- /dev/null
> +++ b/include/hw/fsi/fsi.h
> @@ -0,0 +1,38 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2024 IBM Corp.
> + *
> + * IBM Flexible Service Interface
> + */
> +#ifndef FSI_FSI_H
> +#define FSI_FSI_H
> +
> +#include "exec/memory.h"
> +#include "hw/qdev-core.h"
> +#include "hw/fsi/lbus.h"
> +#include "qemu/bitops.h"
> +
> +/* Bitwise operations at the word level. */
> +#define BE_BIT(x) BIT(31 - (x))
> +#define BE_GENMASK(hb, lb) MAKE_64BIT_MASK((lb), ((hb) - (lb) + 1))
> +
> +#define TYPE_FSI_BUS "fsi.bus"
> +OBJECT_DECLARE_SIMPLE_TYPE(FSIBus, FSI_BUS)
> +
> +typedef struct FSIBus {
> + BusState bus;
> +} FSIBus;
> +
> +#define TYPE_FSI_SLAVE "fsi.slave"
> +OBJECT_DECLARE_SIMPLE_TYPE(FSISlaveState, FSI_SLAVE)
> +
> +#define FSI_SLAVE_CONTROL_NR_REGS ((0x40 >> 2) + 1)
> +
> +typedef struct FSISlaveState {
> + DeviceState parent;
> +
> + MemoryRegion iomem;
> + uint32_t regs[FSI_SLAVE_CONTROL_NR_REGS];
> +} FSISlaveState;
> +
> +#endif /* FSI_FSI_H */
> diff --git a/hw/fsi/fsi.c b/hw/fsi/fsi.c
> new file mode 100644
> index 0000000000..0c73ca14ad
> --- /dev/null
> +++ b/hw/fsi/fsi.c
> @@ -0,0 +1,111 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2024 IBM Corp.
> + *
> + * IBM Flexible Service Interface
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/log.h"
> +#include "trace.h"
> +
> +#include "hw/fsi/fsi.h"
> +
> +static const TypeInfo fsi_bus_info = {
> + .name = TYPE_FSI_BUS,
> + .parent = TYPE_BUS,
> + .instance_size = sizeof(FSIBus),
> +};
> +
> +static void fsi_bus_register_types(void)
> +{
> + type_register_static(&fsi_bus_info);
> +}
> +
> +type_init(fsi_bus_register_types);
> +
> +#define TO_REG(x) ((x) >> 2)
> +
> +static uint64_t fsi_slave_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + FSISlaveState *s = FSI_SLAVE(opaque);
> + int reg = TO_REG(addr);
> +
> + trace_fsi_slave_read(addr, size);
> +
> + if (reg >= FSI_SLAVE_CONTROL_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds read: 0x%"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return 0;
> + }
> +
> + return s->regs[reg];
> +}
> +
> +static void fsi_slave_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + FSISlaveState *s = FSI_SLAVE(opaque);
> + int reg = TO_REG(addr);
> +
> + trace_fsi_slave_write(addr, size, data);
> +
> + if (reg >= FSI_SLAVE_CONTROL_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds write: 0x%"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return;
> + }
> +
> + s->regs[reg] = data;
> +}
> +
> +static const struct MemoryRegionOps fsi_slave_ops = {
> + .read = fsi_slave_read,
> + .write = fsi_slave_write,
> + .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void fsi_slave_reset(DeviceState *dev)
> +{
> + FSISlaveState *s = FSI_SLAVE(dev);
> + int i;
> +
> + /* Initialize registers */
> + for (i = 0; i < FSI_SLAVE_CONTROL_NR_REGS; i++) {
> + s->regs[i] = 0;
> + }
memset(s->regs, 0, sizeof(s->regs));
> +}
> +
> +static void fsi_slave_init(Object *o)
> +{
> + FSISlaveState *s = FSI_SLAVE(o);
> +
> + memory_region_init_io(&s->iomem, OBJECT(s), &fsi_slave_ops,
> + s, TYPE_FSI_SLAVE, 0x400);
> +}
> +
> +static void fsi_slave_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->bus_type = TYPE_FSI_BUS;
> + dc->desc = "FSI Slave";
> + dc->reset = fsi_slave_reset;
> +}
> +
> +static const TypeInfo fsi_slave_info = {
> + .name = TYPE_FSI_SLAVE,
> + .parent = TYPE_DEVICE,
> + .instance_init = fsi_slave_init,
> + .instance_size = sizeof(FSISlaveState),
> + .class_init = fsi_slave_class_init,
> +};
> +
> +static void fsi_slave_register_types(void)
> +{
> + type_register_static(&fsi_slave_info);
> +}
> +
> +type_init(fsi_slave_register_types);
> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> index 93ba19dd04..574f5f9289 100644
> --- a/hw/fsi/meson.build
> +++ b/hw/fsi/meson.build
> @@ -1 +1 @@
> -system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c'))
> +system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c'))
> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
> index c5753e2791..8f29adb7df 100644
> --- a/hw/fsi/trace-events
> +++ b/hw/fsi/trace-events
> @@ -1,2 +1,4 @@
> fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +fsi_slave_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +fsi_slave_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v10 2/9] hw/fsi: Introduce IBM's FSI Bus and FSI slave
2024-01-12 15:03 ` Cédric Le Goater
@ 2024-01-25 23:59 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-25 23:59 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Cedric,
>>
>> The FSI slave: The slave is the terminal point of the FSI bus for
>> FSI symbols addressed to it. Slaves can be cascaded off of one
>> another. The slave's configuration registers appear in address space
>> of the CFAM to which it is attached.
>
> Please add another patch.
Added new patch for FSI-slave.
>
>> [ clg: - removed include/hw/fsi/engine-scratchpad.h and
>> hw/fsi/engine-scratchpad.c
>> - dropped FSI_SCRATCHPAD
>> - included FSIBus definition
>> - dropped hw/fsi/trace-events changes ]
>
> Move the list down before my S-o-b.
Moved the list.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
2024-01-10 23:15 ` [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad Ninad Palsule
2024-01-10 23:15 ` [PATCH v10 2/9] hw/fsi: Introduce IBM's FSI Bus and FSI slave Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 15:02 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 4/9] hw/fsi: Introduce IBM's FSI master Ninad Palsule
` (6 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm, Andrew Jeffery
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
The Common FRU Access Macro (CFAM), an address space containing
various "engines" that drive accesses on busses internal and external
to the POWER chip. Examples include the SBEFIFO and I2C masters. The
engines hang off of an internal Local Bus (LBUS) which is described
by the CFAM configuration block.
[ clg: - moved object FSIScratchPad under FSICFAMState
- moved FSIScratchPad code under cfam.c
- introduced fsi_cfam_instance_init()
- reworked fsi_cfam_realize() ]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v9:
- Added more registers to scratchpad
- Removed unnecessary address space
- Removed unnecessary header file
- Defined macros for config values.
- Cleaned up cfam config read.
---
include/hw/fsi/cfam.h | 34 ++++++++
hw/fsi/cfam.c | 182 ++++++++++++++++++++++++++++++++++++++++++
hw/fsi/meson.build | 2 +-
hw/fsi/trace-events | 5 ++
4 files changed, 222 insertions(+), 1 deletion(-)
create mode 100644 include/hw/fsi/cfam.h
create mode 100644 hw/fsi/cfam.c
diff --git a/include/hw/fsi/cfam.h b/include/hw/fsi/cfam.h
new file mode 100644
index 0000000000..bba5e3323a
--- /dev/null
+++ b/include/hw/fsi/cfam.h
@@ -0,0 +1,34 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Common FRU Access Macro
+ */
+#ifndef FSI_CFAM_H
+#define FSI_CFAM_H
+
+#include "exec/memory.h"
+
+#include "hw/fsi/fsi.h"
+#include "hw/fsi/lbus.h"
+
+#define TYPE_FSI_CFAM "cfam"
+#define FSI_CFAM(obj) OBJECT_CHECK(FSICFAMState, (obj), TYPE_FSI_CFAM)
+
+/* P9-ism */
+#define CFAM_CONFIG_NR_REGS 0x28
+
+typedef struct FSICFAMState {
+ /* < private > */
+ FSISlaveState parent;
+
+ /* CFAM config address space */
+ MemoryRegion config_iomem;
+
+ MemoryRegion mr;
+
+ FSILBus lbus;
+ FSIScratchPad scratchpad;
+} FSICFAMState;
+
+#endif /* FSI_CFAM_H */
diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
new file mode 100644
index 0000000000..d9ed1b532a
--- /dev/null
+++ b/hw/fsi/cfam.c
@@ -0,0 +1,182 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Common FRU Access Macro
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/fsi/cfam.h"
+#include "hw/fsi/fsi.h"
+
+#include "hw/qdev-properties.h"
+
+#define ENGINE_CONFIG_NEXT BE_BIT(0)
+#define ENGINE_CONFIG_TYPE_PEEK (0x02 << 4)
+#define ENGINE_CONFIG_TYPE_FSI (0x03 << 4)
+#define ENGINE_CONFIG_TYPE_SCRATCHPAD (0x06 << 4)
+
+/* Valid, slots, version, type, crc */
+#define CFAM_CONFIG_REG_PEEK (ENGINE_CONFIG_NEXT | \
+ 0x00010000 | \
+ 0x00001000 | \
+ ENGINE_CONFIG_TYPE_PEEK | \
+ 0x0000000c)
+
+/* Valid, slots, version, type, crc */
+#define CFAM_CONFIG_REG_FSI_SLAVE (ENGINE_CONFIG_NEXT | \
+ 0x00010000 | \
+ 0x00005000 | \
+ ENGINE_CONFIG_TYPE_FSI | \
+ 0x0000000a)
+
+/* Valid, slots, version, type, crc */
+#define CFAM_CONFIG_REG_SCRATCHPAD (ENGINE_CONFIG_NEXT | \
+ 0x00010000 | \
+ 0x00001000 | \
+ ENGINE_CONFIG_TYPE_SCRATCHPAD | \
+ 0x00000007)
+
+#define TO_REG(x) ((x) >> 2)
+
+#define CFAM_CONFIG_CHIP_ID TO_REG(0x00)
+#define CFAM_CONFIG_PEEK_STATUS TO_REG(0x04)
+#define CFAM_CONFIG_CHIP_ID_P9 0xc0022d15
+#define CFAM_CONFIG_CHIP_ID_BREAK 0xc0de0000
+
+static uint64_t fsi_cfam_config_read(void *opaque, hwaddr addr, unsigned size)
+{
+ trace_fsi_cfam_config_read(addr, size);
+
+ switch (addr) {
+ case 0x00:
+ return CFAM_CONFIG_CHIP_ID_P9;
+ case 0x04:
+ return CFAM_CONFIG_REG_PEEK;
+ case 0x08:
+ return CFAM_CONFIG_REG_FSI_SLAVE;
+ case 0xc:
+ return CFAM_CONFIG_REG_SCRATCHPAD;
+ default:
+ /*
+ * The config table contains different engines from 0xc onwards.
+ * The scratch pad is already added at address 0xc. We need to add
+ * future engines from address 0x10 onwards. Returning 0 as engine
+ * is not implemented.
+ */
+ return 0;
+ }
+}
+
+static void fsi_cfam_config_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ FSICFAMState *cfam = FSI_CFAM(opaque);
+
+ trace_fsi_cfam_config_write(addr, size, data);
+
+ switch (TO_REG(addr)) {
+ case CFAM_CONFIG_CHIP_ID:
+ case CFAM_CONFIG_PEEK_STATUS:
+ if (data == CFAM_CONFIG_CHIP_ID_BREAK) {
+ bus_cold_reset(BUS(&cfam->lbus));
+ }
+ break;
+ default:
+ trace_fsi_cfam_config_write_noaddr(addr, size, data);
+ }
+}
+
+static const struct MemoryRegionOps cfam_config_ops = {
+ .read = fsi_cfam_config_read,
+ .write = fsi_cfam_config_write,
+ .valid.max_access_size = 4,
+ .valid.min_access_size = 4,
+ .impl.max_access_size = 4,
+ .impl.min_access_size = 4,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static uint64_t fsi_cfam_unimplemented_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ trace_fsi_cfam_unimplemented_read(addr, size);
+
+ return 0;
+}
+
+static void fsi_cfam_unimplemented_write(void *opaque, hwaddr addr,
+ uint64_t data, unsigned size)
+{
+ trace_fsi_cfam_unimplemented_write(addr, size, data);
+}
+
+static const struct MemoryRegionOps fsi_cfam_unimplemented_ops = {
+ .read = fsi_cfam_unimplemented_read,
+ .write = fsi_cfam_unimplemented_write,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void fsi_cfam_instance_init(Object *obj)
+{
+ FSICFAMState *s = FSI_CFAM(obj);
+
+ object_initialize_child(obj, "scratchpad", &s->scratchpad,
+ TYPE_FSI_SCRATCHPAD);
+}
+
+static void fsi_cfam_realize(DeviceState *dev, Error **errp)
+{
+ FSICFAMState *cfam = FSI_CFAM(dev);
+ FSISlaveState *slave = FSI_SLAVE(dev);
+
+ /* Each slave has a 2MiB address space */
+ memory_region_init_io(&cfam->mr, OBJECT(cfam), &fsi_cfam_unimplemented_ops,
+ cfam, TYPE_FSI_CFAM, 2 * MiB);
+
+ qbus_init(&cfam->lbus, sizeof(cfam->lbus), TYPE_FSI_LBUS, DEVICE(cfam),
+ NULL);
+
+ memory_region_init_io(&cfam->config_iomem, OBJECT(cfam), &cfam_config_ops,
+ cfam, TYPE_FSI_CFAM ".config", 0x400);
+
+ memory_region_add_subregion(&cfam->mr, 0, &cfam->config_iomem);
+ memory_region_add_subregion(&cfam->mr, 0x800, &slave->iomem);
+ memory_region_add_subregion(&cfam->mr, 0xc00, &cfam->lbus.mr);
+
+ /* Add scratchpad engine */
+ if (!qdev_realize(DEVICE(&cfam->scratchpad), BUS(&cfam->lbus),
+ errp)) {
+ return;
+ }
+
+ FSILBusDevice *fsi_dev = FSI_LBUS_DEVICE(&cfam->scratchpad);
+ memory_region_add_subregion(&cfam->lbus.mr, 0, &fsi_dev->iomem);
+}
+
+static void fsi_cfam_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ dc->bus_type = TYPE_FSI_BUS;
+ dc->realize = fsi_cfam_realize;
+}
+
+static const TypeInfo fsi_cfam_info = {
+ .name = TYPE_FSI_CFAM,
+ .parent = TYPE_FSI_SLAVE,
+ .instance_init = fsi_cfam_instance_init,
+ .instance_size = sizeof(FSICFAMState),
+ .class_init = fsi_cfam_class_init,
+};
+
+static void fsi_cfam_register_types(void)
+{
+ type_register_static(&fsi_cfam_info);
+}
+
+type_init(fsi_cfam_register_types);
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index 574f5f9289..96403d4efc 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -1 +1 @@
-system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c'))
+system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c','cfam.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index 8f29adb7df..b542956fb3 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -2,3 +2,8 @@ fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
fsi_slave_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_slave_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+fsi_cfam_config_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_cfam_config_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+fsi_cfam_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_cfam_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+fsi_cfam_config_write_noaddr(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam
2024-01-10 23:15 ` [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam Ninad Palsule
@ 2024-01-12 15:02 ` Cédric Le Goater
2024-01-26 0:09 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 15:02 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
On 1/11/24 00:15, Ninad Palsule wrote:
> This is a part of patchset where IBM's Flexible Service Interface is
> introduced.
>
> The Common FRU Access Macro (CFAM), an address space containing
> various "engines" that drive accesses on busses internal and external
> to the POWER chip. Examples include the SBEFIFO and I2C masters. The
> engines hang off of an internal Local Bus (LBUS) which is described
> by the CFAM configuration block.
>
> [ clg: - moved object FSIScratchPad under FSICFAMState
> - moved FSIScratchPad code under cfam.c
> - introduced fsi_cfam_instance_init()
> - reworked fsi_cfam_realize() ]
Move the list down before my S-o-b.
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v9:
> - Added more registers to scratchpad
> - Removed unnecessary address space
> - Removed unnecessary header file
> - Defined macros for config values.
> - Cleaned up cfam config read.
> ---
> include/hw/fsi/cfam.h | 34 ++++++++
> hw/fsi/cfam.c | 182 ++++++++++++++++++++++++++++++++++++++++++
> hw/fsi/meson.build | 2 +-
> hw/fsi/trace-events | 5 ++
> 4 files changed, 222 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/fsi/cfam.h
> create mode 100644 hw/fsi/cfam.c
>
> diff --git a/include/hw/fsi/cfam.h b/include/hw/fsi/cfam.h
> new file mode 100644
> index 0000000000..bba5e3323a
> --- /dev/null
> +++ b/include/hw/fsi/cfam.h
> @@ -0,0 +1,34 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * IBM Common FRU Access Macro
> + */
> +#ifndef FSI_CFAM_H
> +#define FSI_CFAM_H
> +
> +#include "exec/memory.h"
> +
> +#include "hw/fsi/fsi.h"
> +#include "hw/fsi/lbus.h"
> +
> +#define TYPE_FSI_CFAM "cfam"
> +#define FSI_CFAM(obj) OBJECT_CHECK(FSICFAMState, (obj), TYPE_FSI_CFAM)
> +
> +/* P9-ism */
> +#define CFAM_CONFIG_NR_REGS 0x28
> +
> +typedef struct FSICFAMState {
> + /* < private > */
> + FSISlaveState parent;
> +
> + /* CFAM config address space */
> + MemoryRegion config_iomem;
> +
> + MemoryRegion mr;
> +
> + FSILBus lbus;
> + FSIScratchPad scratchpad;
> +} FSICFAMState;
> +
> +#endif /* FSI_CFAM_H */
> diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
> new file mode 100644
> index 0000000000..d9ed1b532a
> --- /dev/null
> +++ b/hw/fsi/cfam.c
> @@ -0,0 +1,182 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * IBM Common FRU Access Macro
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +
> +#include "qapi/error.h"
> +#include "trace.h"
> +
> +#include "hw/fsi/cfam.h"
> +#include "hw/fsi/fsi.h"
> +
> +#include "hw/qdev-properties.h"
> +
> +#define ENGINE_CONFIG_NEXT BE_BIT(0)
> +#define ENGINE_CONFIG_TYPE_PEEK (0x02 << 4)
> +#define ENGINE_CONFIG_TYPE_FSI (0x03 << 4)
> +#define ENGINE_CONFIG_TYPE_SCRATCHPAD (0x06 << 4)
> +
> +/* Valid, slots, version, type, crc */
> +#define CFAM_CONFIG_REG_PEEK (ENGINE_CONFIG_NEXT | \
> + 0x00010000 | \
> + 0x00001000 | \
> + ENGINE_CONFIG_TYPE_PEEK | \
> + 0x0000000c)
> +
> +/* Valid, slots, version, type, crc */
> +#define CFAM_CONFIG_REG_FSI_SLAVE (ENGINE_CONFIG_NEXT | \
> + 0x00010000 | \
> + 0x00005000 | \
> + ENGINE_CONFIG_TYPE_FSI | \
> + 0x0000000a)
> +
> +/* Valid, slots, version, type, crc */
> +#define CFAM_CONFIG_REG_SCRATCHPAD (ENGINE_CONFIG_NEXT | \
> + 0x00010000 | \
> + 0x00001000 | \
> + ENGINE_CONFIG_TYPE_SCRATCHPAD | \
> + 0x00000007)
I was expecting a macro taking argument to build the config reg value
of each sub engine but that's fine also.
> +#define TO_REG(x) ((x) >> 2)
> +
> +#define CFAM_CONFIG_CHIP_ID TO_REG(0x00)
> +#define CFAM_CONFIG_PEEK_STATUS TO_REG(0x04)
> +#define CFAM_CONFIG_CHIP_ID_P9 0xc0022d15
> +#define CFAM_CONFIG_CHIP_ID_BREAK 0xc0de0000
> +
> +static uint64_t fsi_cfam_config_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + trace_fsi_cfam_config_read(addr, size);
> +
> + switch (addr) {
> + case 0x00:
> + return CFAM_CONFIG_CHIP_ID_P9;
> + case 0x04:
> + return CFAM_CONFIG_REG_PEEK;
> + case 0x08:
> + return CFAM_CONFIG_REG_FSI_SLAVE;
> + case 0xc:
> + return CFAM_CONFIG_REG_SCRATCHPAD;
> + default:
> + /*
> + * The config table contains different engines from 0xc onwards.
> + * The scratch pad is already added at address 0xc. We need to add
> + * future engines from address 0x10 onwards. Returning 0 as engine
> + * is not implemented.
> + */
> + return 0;
> + }
> +}
> +
> +static void fsi_cfam_config_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + FSICFAMState *cfam = FSI_CFAM(opaque);
> +
> + trace_fsi_cfam_config_write(addr, size, data);
> +
> + switch (TO_REG(addr)) {
> + case CFAM_CONFIG_CHIP_ID:
> + case CFAM_CONFIG_PEEK_STATUS:
> + if (data == CFAM_CONFIG_CHIP_ID_BREAK) {
> + bus_cold_reset(BUS(&cfam->lbus));
> + }
> + break;
> + default:
> + trace_fsi_cfam_config_write_noaddr(addr, size, data);
> + }
> +}
> +
> +static const struct MemoryRegionOps cfam_config_ops = {
> + .read = fsi_cfam_config_read,
> + .write = fsi_cfam_config_write,
> + .valid.max_access_size = 4,
> + .valid.min_access_size = 4,
> + .impl.max_access_size = 4,
> + .impl.min_access_size = 4,
> + .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static uint64_t fsi_cfam_unimplemented_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + trace_fsi_cfam_unimplemented_read(addr, size);
> +
> + return 0;
> +}
> +
> +static void fsi_cfam_unimplemented_write(void *opaque, hwaddr addr,
> + uint64_t data, unsigned size)
> +{
> + trace_fsi_cfam_unimplemented_write(addr, size, data);
> +}
> +
> +static const struct MemoryRegionOps fsi_cfam_unimplemented_ops = {
> + .read = fsi_cfam_unimplemented_read,
> + .write = fsi_cfam_unimplemented_write,
> + .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void fsi_cfam_instance_init(Object *obj)
> +{
> + FSICFAMState *s = FSI_CFAM(obj);
> +
> + object_initialize_child(obj, "scratchpad", &s->scratchpad,
> + TYPE_FSI_SCRATCHPAD);
> +}
> +
> +static void fsi_cfam_realize(DeviceState *dev, Error **errp)
> +{
> + FSICFAMState *cfam = FSI_CFAM(dev);
> + FSISlaveState *slave = FSI_SLAVE(dev);
> +
> + /* Each slave has a 2MiB address space */
> + memory_region_init_io(&cfam->mr, OBJECT(cfam), &fsi_cfam_unimplemented_ops,
> + cfam, TYPE_FSI_CFAM, 2 * MiB);
> +
> + qbus_init(&cfam->lbus, sizeof(cfam->lbus), TYPE_FSI_LBUS, DEVICE(cfam),
> + NULL);
> +
> + memory_region_init_io(&cfam->config_iomem, OBJECT(cfam), &cfam_config_ops,
> + cfam, TYPE_FSI_CFAM ".config", 0x400);
> +
> + memory_region_add_subregion(&cfam->mr, 0, &cfam->config_iomem);
> + memory_region_add_subregion(&cfam->mr, 0x800, &slave->iomem);
> + memory_region_add_subregion(&cfam->mr, 0xc00, &cfam->lbus.mr);
> +
> + /* Add scratchpad engine */
> + if (!qdev_realize(DEVICE(&cfam->scratchpad), BUS(&cfam->lbus),
> + errp)) {
could be a single line.
Thanks,
C.
> + return;
> + }
> +
> + FSILBusDevice *fsi_dev = FSI_LBUS_DEVICE(&cfam->scratchpad);
> + memory_region_add_subregion(&cfam->lbus.mr, 0, &fsi_dev->iomem);
> +}
> +
> +static void fsi_cfam_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + dc->bus_type = TYPE_FSI_BUS;
> + dc->realize = fsi_cfam_realize;
> +}
> +
> +static const TypeInfo fsi_cfam_info = {
> + .name = TYPE_FSI_CFAM,
> + .parent = TYPE_FSI_SLAVE,
> + .instance_init = fsi_cfam_instance_init,
> + .instance_size = sizeof(FSICFAMState),
> + .class_init = fsi_cfam_class_init,
> +};
> +
> +static void fsi_cfam_register_types(void)
> +{
> + type_register_static(&fsi_cfam_info);
> +}
> +
> +type_init(fsi_cfam_register_types);
> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> index 574f5f9289..96403d4efc 100644
> --- a/hw/fsi/meson.build
> +++ b/hw/fsi/meson.build
> @@ -1 +1 @@
> -system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c'))
> +system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c','cfam.c'))
> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
> index 8f29adb7df..b542956fb3 100644
> --- a/hw/fsi/trace-events
> +++ b/hw/fsi/trace-events
> @@ -2,3 +2,8 @@ fsi_scratchpad_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_scratchpad_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> fsi_slave_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_slave_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +fsi_cfam_config_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +fsi_cfam_config_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +fsi_cfam_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +fsi_cfam_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +fsi_cfam_config_write_noaddr(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam
2024-01-12 15:02 ` Cédric Le Goater
@ 2024-01-26 0:09 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-26 0:09 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Cedric,
>>
>> [ clg: - moved object FSIScratchPad under FSICFAMState
>> - moved FSIScratchPad code under cfam.c
>> - introduced fsi_cfam_instance_init()
>> - reworked fsi_cfam_realize() ]
>
> Move the list down before my S-o-b.
Done.
>
>>
>> +
>> +/* Valid, slots, version, type, crc */
>> +#define CFAM_CONFIG_REG_FSI_SLAVE (ENGINE_CONFIG_NEXT | \
>> + 0x00010000 | \
>> + 0x00005000 | \
>> + ENGINE_CONFIG_TYPE_FSI | \
>> + 0x0000000a)
>> +
>> +/* Valid, slots, version, type, crc */
>> +#define CFAM_CONFIG_REG_SCRATCHPAD (ENGINE_CONFIG_NEXT | \
>> + 0x00010000 | \
>> + 0x00001000 | \
>> + ENGINE_CONFIG_TYPE_SCRATCHPAD | \
>> + 0x00000007)
>
> I was expecting a macro taking argument to build the config reg value
> of each sub engine but that's fine also.
Added single macro.
>
>>
>> +
>> + memory_region_add_subregion(&cfam->mr, 0, &cfam->config_iomem);
>> + memory_region_add_subregion(&cfam->mr, 0x800, &slave->iomem);
>> + memory_region_add_subregion(&cfam->mr, 0xc00, &cfam->lbus.mr);
>> +
>> + /* Add scratchpad engine */
>> + if (!qdev_realize(DEVICE(&cfam->scratchpad), BUS(&cfam->lbus),
>> + errp)) {
>
> could be a single line.
Yep, Made it a single line.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 4/9] hw/fsi: Introduce IBM's FSI master
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (2 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 3/9] hw/fsi: Introduce IBM's cfam Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 15:05 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus Ninad Palsule
` (5 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm, Andrew Jeffery
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
This commit models the FSI master. CFAM is hanging out of FSI master which is a bus controller.
The FSI master: A controller in the platform service processor (e.g.
BMC) driving CFAM engine accesses into the POWER chip. At the
hardware level FSI is a bit-based protocol supporting synchronous and
DMA-driven accesses of engines in a CFAM.
[ clg: - move FSICFAMState object under FSIMasterState
- introduced fsi_master_init()
- reworked fsi_master_realize()
- dropped FSIBus definition ]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v9:
- Initialized registers.
- Fixed the address check.
---
include/hw/fsi/fsi-master.h | 32 +++++++
hw/fsi/fsi-master.c | 173 ++++++++++++++++++++++++++++++++++++
hw/fsi/meson.build | 2 +-
hw/fsi/trace-events | 2 +
4 files changed, 208 insertions(+), 1 deletion(-)
create mode 100644 include/hw/fsi/fsi-master.h
create mode 100644 hw/fsi/fsi-master.c
diff --git a/include/hw/fsi/fsi-master.h b/include/hw/fsi/fsi-master.h
new file mode 100644
index 0000000000..3830869877
--- /dev/null
+++ b/include/hw/fsi/fsi-master.h
@@ -0,0 +1,32 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2019 IBM Corp.
+ *
+ * IBM Flexible Service Interface Master
+ */
+#ifndef FSI_FSI_MASTER_H
+#define FSI_FSI_MASTER_H
+
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
+#include "hw/fsi/fsi.h"
+#include "hw/fsi/cfam.h"
+
+#define TYPE_FSI_MASTER "fsi.master"
+OBJECT_DECLARE_SIMPLE_TYPE(FSIMasterState, FSI_MASTER)
+
+#define FSI_MASTER_NR_REGS ((0x2e0 >> 2) + 1)
+
+typedef struct FSIMasterState {
+ DeviceState parent;
+ MemoryRegion iomem;
+ MemoryRegion opb2fsi;
+
+ FSIBus bus;
+
+ uint32_t regs[FSI_MASTER_NR_REGS];
+ FSICFAMState cfam;
+} FSIMasterState;
+
+
+#endif /* FSI_FSI_H */
diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c
new file mode 100644
index 0000000000..939de5927f
--- /dev/null
+++ b/hw/fsi/fsi-master.c
@@ -0,0 +1,173 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Flexible Service Interface master
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/fsi-master.h"
+
+#define TYPE_OP_BUS "opb"
+
+#define TO_REG(x) ((x) >> 2)
+
+#define FSI_MENP0 TO_REG(0x010)
+#define FSI_MENP32 TO_REG(0x014)
+#define FSI_MSENP0 TO_REG(0x018)
+#define FSI_MLEVP0 TO_REG(0x018)
+#define FSI_MSENP32 TO_REG(0x01c)
+#define FSI_MLEVP32 TO_REG(0x01c)
+#define FSI_MCENP0 TO_REG(0x020)
+#define FSI_MREFP0 TO_REG(0x020)
+#define FSI_MCENP32 TO_REG(0x024)
+#define FSI_MREFP32 TO_REG(0x024)
+
+#define FSI_MVER TO_REG(0x074)
+#define FSI_MRESP0 TO_REG(0x0d0)
+
+#define FSI_MRESB0 TO_REG(0x1d0)
+#define FSI_MRESB0_RESET_GENERAL BE_BIT(0)
+#define FSI_MRESB0_RESET_ERROR BE_BIT(1)
+
+static uint64_t fsi_master_read(void *opaque, hwaddr addr, unsigned size)
+{
+ FSIMasterState *s = FSI_MASTER(opaque);
+ int reg = TO_REG(addr);
+
+ trace_fsi_master_read(addr, size);
+
+ if (reg >= FSI_MASTER_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds read: 0x%"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return 0;
+ }
+
+ return s->regs[reg];
+}
+
+static void fsi_master_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ FSIMasterState *s = FSI_MASTER(opaque);
+ int reg = TO_REG(addr);
+
+ trace_fsi_master_write(addr, size, data);
+
+ if (reg >= FSI_MASTER_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ switch (reg) {
+ case FSI_MENP0:
+ s->regs[FSI_MENP0] = data;
+ break;
+ case FSI_MENP32:
+ s->regs[FSI_MENP32] = data;
+ break;
+ case FSI_MSENP0:
+ s->regs[FSI_MENP0] |= data;
+ break;
+ case FSI_MSENP32:
+ s->regs[FSI_MENP32] |= data;
+ break;
+ case FSI_MCENP0:
+ s->regs[FSI_MENP0] &= ~data;
+ break;
+ case FSI_MCENP32:
+ s->regs[FSI_MENP32] &= ~data;
+ break;
+ case FSI_MRESP0:
+ /* Perform necessary resets leave register 0 to indicate no errors */
+ break;
+ case FSI_MRESB0:
+ if (data & FSI_MRESB0_RESET_GENERAL) {
+ device_cold_reset(DEVICE(opaque));
+ }
+ if (data & FSI_MRESB0_RESET_ERROR) {
+ /* FIXME: this seems dubious */
+ device_cold_reset(DEVICE(opaque));
+ }
+ break;
+ default:
+ s->regs[reg] = data;
+ }
+}
+
+static const struct MemoryRegionOps fsi_master_ops = {
+ .read = fsi_master_read,
+ .write = fsi_master_write,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void fsi_master_init(Object *o)
+{
+ FSIMasterState *s = FSI_MASTER(o);
+
+ object_initialize_child(o, "cfam", &s->cfam, TYPE_FSI_CFAM);
+
+ qbus_init(&s->bus, sizeof(s->bus), TYPE_FSI_BUS, DEVICE(s), NULL);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &fsi_master_ops, s,
+ TYPE_FSI_MASTER, 0x10000000);
+ memory_region_init(&s->opb2fsi, OBJECT(s), "fsi.opb2fsi", 0x10000000);
+}
+
+static void fsi_master_realize(DeviceState *dev, Error **errp)
+{
+ FSIMasterState *s = FSI_MASTER(dev);
+
+ if (!qdev_realize(DEVICE(&s->cfam), BUS(&s->bus), errp)) {
+ return;
+ }
+
+ /* address ? */
+ memory_region_add_subregion(&s->opb2fsi, 0, &s->cfam.mr);
+}
+
+static void fsi_master_reset(DeviceState *dev)
+{
+ FSIMasterState *s = FSI_MASTER(dev);
+ int i;
+
+ /* Initialize registers */
+ for (i = 0; i < FSI_MASTER_NR_REGS; i++) {
+ s->regs[i] = 0;
+ }
+
+ /* ASPEED default */
+ s->regs[FSI_MVER] = 0xe0050101;
+}
+
+static void fsi_master_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->bus_type = TYPE_OP_BUS;
+ dc->desc = "FSI Master";
+ dc->realize = fsi_master_realize;
+ dc->reset = fsi_master_reset;
+}
+
+static const TypeInfo fsi_master_info = {
+ .name = TYPE_FSI_MASTER,
+ .parent = TYPE_DEVICE,
+ .instance_init = fsi_master_init,
+ .instance_size = sizeof(FSIMasterState),
+ .class_init = fsi_master_class_init,
+};
+
+static void fsi_register_types(void)
+{
+ type_register_static(&fsi_master_info);
+}
+
+type_init(fsi_register_types);
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index 96403d4efc..7803b3afd1 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -1 +1 @@
-system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c','cfam.c'))
+system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c','cfam.c','fsi-master.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index b542956fb3..bf417b6dc3 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -7,3 +7,5 @@ fsi_cfam_config_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64
fsi_cfam_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_cfam_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
fsi_cfam_config_write_noaddr(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+fsi_master_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_master_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 4/9] hw/fsi: Introduce IBM's FSI master
2024-01-10 23:15 ` [PATCH v10 4/9] hw/fsi: Introduce IBM's FSI master Ninad Palsule
@ 2024-01-12 15:05 ` Cédric Le Goater
2024-01-16 19:43 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 15:05 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
On 1/11/24 00:15, Ninad Palsule wrote:
> This is a part of patchset where IBM's Flexible Service Interface is
> introduced.
>
> This commit models the FSI master. CFAM is hanging out of FSI master which is a bus controller.
>
> The FSI master: A controller in the platform service processor (e.g.
> BMC) driving CFAM engine accesses into the POWER chip. At the
> hardware level FSI is a bit-based protocol supporting synchronous and
> DMA-driven accesses of engines in a CFAM.
>
> [ clg: - move FSICFAMState object under FSIMasterState
> - introduced fsi_master_init()
> - reworked fsi_master_realize()
> - dropped FSIBus definition ]
Move the list down before my S-o-b.
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
I think you can drop Joel's reviews, the code has changed a lot.
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v9:
> - Initialized registers.
> - Fixed the address check.
> ---
> include/hw/fsi/fsi-master.h | 32 +++++++
> hw/fsi/fsi-master.c | 173 ++++++++++++++++++++++++++++++++++++
> hw/fsi/meson.build | 2 +-
> hw/fsi/trace-events | 2 +
> 4 files changed, 208 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/fsi/fsi-master.h
> create mode 100644 hw/fsi/fsi-master.c
>
> diff --git a/include/hw/fsi/fsi-master.h b/include/hw/fsi/fsi-master.h
> new file mode 100644
> index 0000000000..3830869877
> --- /dev/null
> +++ b/include/hw/fsi/fsi-master.h
> @@ -0,0 +1,32 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2019 IBM Corp.
> + *
> + * IBM Flexible Service Interface Master
> + */
> +#ifndef FSI_FSI_MASTER_H
> +#define FSI_FSI_MASTER_H
> +
> +#include "exec/memory.h"
> +#include "hw/qdev-core.h"
> +#include "hw/fsi/fsi.h"
> +#include "hw/fsi/cfam.h"
> +
> +#define TYPE_FSI_MASTER "fsi.master"
> +OBJECT_DECLARE_SIMPLE_TYPE(FSIMasterState, FSI_MASTER)
> +
> +#define FSI_MASTER_NR_REGS ((0x2e0 >> 2) + 1)
> +
> +typedef struct FSIMasterState {
> + DeviceState parent;
> + MemoryRegion iomem;
> + MemoryRegion opb2fsi;
> +
> + FSIBus bus;
> +
> + uint32_t regs[FSI_MASTER_NR_REGS];
> + FSICFAMState cfam;
> +} FSIMasterState;
> +
> +
> +#endif /* FSI_FSI_H */
> diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c
> new file mode 100644
> index 0000000000..939de5927f
> --- /dev/null
> +++ b/hw/fsi/fsi-master.c
> @@ -0,0 +1,173 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * IBM Flexible Service Interface master
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/log.h"
> +#include "trace.h"
> +
> +#include "hw/fsi/fsi-master.h"
> +
> +#define TYPE_OP_BUS "opb"
> +
> +#define TO_REG(x) ((x) >> 2)
> +
> +#define FSI_MENP0 TO_REG(0x010)
> +#define FSI_MENP32 TO_REG(0x014)
> +#define FSI_MSENP0 TO_REG(0x018)
> +#define FSI_MLEVP0 TO_REG(0x018)
> +#define FSI_MSENP32 TO_REG(0x01c)
> +#define FSI_MLEVP32 TO_REG(0x01c)
> +#define FSI_MCENP0 TO_REG(0x020)
> +#define FSI_MREFP0 TO_REG(0x020)
> +#define FSI_MCENP32 TO_REG(0x024)
> +#define FSI_MREFP32 TO_REG(0x024)
> +
> +#define FSI_MVER TO_REG(0x074)
> +#define FSI_MRESP0 TO_REG(0x0d0)
> +
> +#define FSI_MRESB0 TO_REG(0x1d0)
> +#define FSI_MRESB0_RESET_GENERAL BE_BIT(0)
> +#define FSI_MRESB0_RESET_ERROR BE_BIT(1)
> +
> +static uint64_t fsi_master_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + FSIMasterState *s = FSI_MASTER(opaque);
> + int reg = TO_REG(addr);
> +
> + trace_fsi_master_read(addr, size);
> +
> + if (reg >= FSI_MASTER_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds read: 0x%"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return 0;
> + }
> +
> + return s->regs[reg];
> +}
> +
> +static void fsi_master_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + FSIMasterState *s = FSI_MASTER(opaque);
> + int reg = TO_REG(addr);
> +
> + trace_fsi_master_write(addr, size, data);
> +
> + if (reg >= FSI_MASTER_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return;
> + }
> +
> + switch (reg) {
> + case FSI_MENP0:
> + s->regs[FSI_MENP0] = data;
> + break;
> + case FSI_MENP32:
> + s->regs[FSI_MENP32] = data;
> + break;
> + case FSI_MSENP0:
> + s->regs[FSI_MENP0] |= data;
> + break;
> + case FSI_MSENP32:
> + s->regs[FSI_MENP32] |= data;
> + break;
> + case FSI_MCENP0:
> + s->regs[FSI_MENP0] &= ~data;
> + break;
> + case FSI_MCENP32:
> + s->regs[FSI_MENP32] &= ~data;
> + break;
> + case FSI_MRESP0:
> + /* Perform necessary resets leave register 0 to indicate no errors */
> + break;
> + case FSI_MRESB0:
> + if (data & FSI_MRESB0_RESET_GENERAL) {
> + device_cold_reset(DEVICE(opaque));
> + }
> + if (data & FSI_MRESB0_RESET_ERROR) {
> + /* FIXME: this seems dubious */
> + device_cold_reset(DEVICE(opaque));
> + }
> + break;
> + default:
> + s->regs[reg] = data;
> + }
> +}
> +
> +static const struct MemoryRegionOps fsi_master_ops = {
> + .read = fsi_master_read,
> + .write = fsi_master_write,
> + .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void fsi_master_init(Object *o)
> +{
> + FSIMasterState *s = FSI_MASTER(o);
> +
> + object_initialize_child(o, "cfam", &s->cfam, TYPE_FSI_CFAM);
> +
> + qbus_init(&s->bus, sizeof(s->bus), TYPE_FSI_BUS, DEVICE(s), NULL);
> +
> + memory_region_init_io(&s->iomem, OBJECT(s), &fsi_master_ops, s,
> + TYPE_FSI_MASTER, 0x10000000);
> + memory_region_init(&s->opb2fsi, OBJECT(s), "fsi.opb2fsi", 0x10000000);
> +}
> +
> +static void fsi_master_realize(DeviceState *dev, Error **errp)
> +{
> + FSIMasterState *s = FSI_MASTER(dev);
> +
> + if (!qdev_realize(DEVICE(&s->cfam), BUS(&s->bus), errp)) {
> + return;
> + }
> +
> + /* address ? */
> + memory_region_add_subregion(&s->opb2fsi, 0, &s->cfam.mr);
> +}
> +
> +static void fsi_master_reset(DeviceState *dev)
> +{
> + FSIMasterState *s = FSI_MASTER(dev);
> + int i;
> +
> + /* Initialize registers */
> + for (i = 0; i < FSI_MASTER_NR_REGS; i++) {
> + s->regs[i] = 0;
> + }
memset(s->regs, 0, sizeof(s->regs));
Thanks,
C.
> + /* ASPEED default */
> + s->regs[FSI_MVER] = 0xe0050101;
> +}
> +
> +static void fsi_master_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->bus_type = TYPE_OP_BUS;
> + dc->desc = "FSI Master";
> + dc->realize = fsi_master_realize;
> + dc->reset = fsi_master_reset;
> +}
> +
> +static const TypeInfo fsi_master_info = {
> + .name = TYPE_FSI_MASTER,
> + .parent = TYPE_DEVICE,
> + .instance_init = fsi_master_init,
> + .instance_size = sizeof(FSIMasterState),
> + .class_init = fsi_master_class_init,
> +};
> +
> +static void fsi_register_types(void)
> +{
> + type_register_static(&fsi_master_info);
> +}
> +
> +type_init(fsi_register_types);
> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> index 96403d4efc..7803b3afd1 100644
> --- a/hw/fsi/meson.build
> +++ b/hw/fsi/meson.build
> @@ -1 +1 @@
> -system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c','cfam.c'))
> +system_ss.add(when: 'CONFIG_FSI', if_true: files('lbus.c','fsi.c','cfam.c','fsi-master.c'))
> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
> index b542956fb3..bf417b6dc3 100644
> --- a/hw/fsi/trace-events
> +++ b/hw/fsi/trace-events
> @@ -7,3 +7,5 @@ fsi_cfam_config_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64
> fsi_cfam_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_cfam_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> fsi_cfam_config_write_noaddr(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +fsi_master_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +fsi_master_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v10 4/9] hw/fsi: Introduce IBM's FSI master
2024-01-12 15:05 ` Cédric Le Goater
@ 2024-01-16 19:43 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-16 19:43 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Cedric,
>>
>> [ clg: - move FSICFAMState object under FSIMasterState
>> - introduced fsi_master_init()
>> - reworked fsi_master_realize()
>> - dropped FSIBus definition ]
>
> Move the list down before my S-o-b.
Done.
>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> I think you can drop Joel's reviews, the code has changed a lot.
Removed Joel's review tag.
>
>> +static void fsi_master_reset(DeviceState *dev)
>> +{
>> + FSIMasterState *s = FSI_MASTER(dev);
>> + int i;
>> +
>> + /* Initialize registers */
>> + for (i = 0; i < FSI_MASTER_NR_REGS; i++) {
>> + s->regs[i] = 0;
>> + }
>
> memset(s->regs, 0, sizeof(s->regs));
Replaced for loop with memset.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (3 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 4/9] hw/fsi: Introduce IBM's FSI master Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 16:00 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 6/9] hw/arm: Hook up FSI module in AST2600 Ninad Palsule
` (4 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm, Andrew Jeffery
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
An APB-to-OPB bridge enabling access to the OPB from the ARM core in
the AST2600. Hardware limitations prevent the OPB from being directly
mapped into APB, so all accesses are indirect through the bridge.
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.
[ clg: - moved FSIMasterState under AspeedAPB2OPBState
- modified fsi_opb_fsi_master_address() and
fsi_opb_opb2fsi_address()
- instroduced fsi_aspeed_apb2opb_init()
- reworked fsi_aspeed_apb2opb_realize()
- removed FSIMasterState object and fsi_opb_realize()
- simplified OPBus ]
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
v9:
- Removed unused parameters from function.
- Used qdev_realize() instead of qdev_realize_and_undef
- Given a name to the opb memory region.
v10:
- Combine Aspeed APB2OPB and on-chip pheripheral bus
---
include/hw/misc/aspeed-apb2opb.h | 50 +++++
hw/misc/aspeed-apb2opb.c | 338 +++++++++++++++++++++++++++++++
hw/arm/Kconfig | 1 +
hw/misc/Kconfig | 5 +
hw/misc/meson.build | 1 +
hw/misc/trace-events | 4 +
6 files changed, 399 insertions(+)
create mode 100644 include/hw/misc/aspeed-apb2opb.h
create mode 100644 hw/misc/aspeed-apb2opb.c
diff --git a/include/hw/misc/aspeed-apb2opb.h b/include/hw/misc/aspeed-apb2opb.h
new file mode 100644
index 0000000000..fcd76631a9
--- /dev/null
+++ b/include/hw/misc/aspeed-apb2opb.h
@@ -0,0 +1,50 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * ASPEED APB2OPB Bridge
+ * IBM On-Chip Peripheral Bus
+ */
+#ifndef FSI_ASPEED_APB2OPB_H
+#define FSI_ASPEED_APB2OPB_H
+
+#include "exec/memory.h"
+#include "hw/fsi/fsi-master.h"
+#include "hw/sysbus.h"
+
+#define TYPE_FSI_OPB "fsi.opb"
+
+#define TYPE_OP_BUS "opb"
+OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
+
+typedef struct OPBus {
+ /*< private >*/
+ BusState bus;
+
+ /*< public >*/
+ MemoryRegion mr;
+ AddressSpace as;
+} OPBus;
+
+#define TYPE_ASPEED_APB2OPB "aspeed.apb2opb"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedAPB2OPBState, ASPEED_APB2OPB)
+
+#define ASPEED_APB2OPB_NR_REGS ((0xe8 >> 2) + 1)
+
+#define ASPEED_FSI_NUM 2
+
+typedef struct AspeedAPB2OPBState {
+ /*< private >*/
+ SysBusDevice parent_obj;
+
+ /*< public >*/
+ MemoryRegion iomem;
+
+ uint32_t regs[ASPEED_APB2OPB_NR_REGS];
+ qemu_irq irq;
+
+ OPBus opb[ASPEED_FSI_NUM];
+ FSIMasterState fsi[ASPEED_FSI_NUM];
+} AspeedAPB2OPBState;
+
+#endif /* FSI_ASPEED_APB2OPB_H */
diff --git a/hw/misc/aspeed-apb2opb.c b/hw/misc/aspeed-apb2opb.c
new file mode 100644
index 0000000000..19545c780f
--- /dev/null
+++ b/hw/misc/aspeed-apb2opb.c
@@ -0,0 +1,338 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * ASPEED APB-OPB FSI interface
+ * IBM On-chip Peripheral Bus
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/misc/aspeed-apb2opb.h"
+#include "hw/qdev-core.h"
+
+#define TO_REG(x) (x >> 2)
+
+#define APB2OPB_VERSION TO_REG(0x00)
+#define APB2OPB_TRIGGER TO_REG(0x04)
+
+#define APB2OPB_CONTROL TO_REG(0x08)
+#define APB2OPB_CONTROL_OFF BE_GENMASK(31, 13)
+
+#define APB2OPB_OPB2FSI TO_REG(0x0c)
+#define APB2OPB_OPB2FSI_OFF BE_GENMASK(31, 22)
+
+#define APB2OPB_OPB0_SEL TO_REG(0x10)
+#define APB2OPB_OPB1_SEL TO_REG(0x28)
+#define APB2OPB_OPB_SEL_EN BIT(0)
+
+#define APB2OPB_OPB0_MODE TO_REG(0x14)
+#define APB2OPB_OPB1_MODE TO_REG(0x2c)
+#define APB2OPB_OPB_MODE_RD BIT(0)
+
+#define APB2OPB_OPB0_XFER TO_REG(0x18)
+#define APB2OPB_OPB1_XFER TO_REG(0x30)
+#define APB2OPB_OPB_XFER_FULL BIT(1)
+#define APB2OPB_OPB_XFER_HALF BIT(0)
+
+#define APB2OPB_OPB0_ADDR TO_REG(0x1c)
+#define APB2OPB_OPB0_WRITE_DATA TO_REG(0x20)
+
+#define APB2OPB_OPB1_ADDR TO_REG(0x34)
+#define APB2OPB_OPB1_WRITE_DATA TO_REG(0x38)
+
+#define APB2OPB_IRQ_STS TO_REG(0x48)
+#define APB2OPB_IRQ_STS_OPB1_TX_ACK BIT(17)
+#define APB2OPB_IRQ_STS_OPB0_TX_ACK BIT(16)
+
+#define APB2OPB_OPB0_WRITE_WORD_ENDIAN TO_REG(0x4c)
+#define APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE 0x0011101b
+#define APB2OPB_OPB0_WRITE_BYTE_ENDIAN TO_REG(0x50)
+#define APB2OPB_OPB0_WRITE_BYTE_ENDIAN_BE 0x0c330f3f
+#define APB2OPB_OPB1_WRITE_WORD_ENDIAN TO_REG(0x54)
+#define APB2OPB_OPB1_WRITE_BYTE_ENDIAN TO_REG(0x58)
+#define APB2OPB_OPB0_READ_BYTE_ENDIAN TO_REG(0x5c)
+#define APB2OPB_OPB1_READ_BYTE_ENDIAN TO_REG(0x60)
+#define APB2OPB_OPB0_READ_WORD_ENDIAN_BE 0x00030b1b
+
+#define APB2OPB_OPB0_READ_DATA TO_REG(0x84)
+#define APB2OPB_OPB1_READ_DATA TO_REG(0x90)
+
+/*
+ * The following magic values came from AST2600 data sheet
+ * The register values are defined under section "FSI controller"
+ * as initial values.
+ */
+static const uint32_t aspeed_apb2opb_reset[ASPEED_APB2OPB_NR_REGS] = {
+ [APB2OPB_VERSION] = 0x000000a1,
+ [APB2OPB_OPB0_WRITE_WORD_ENDIAN] = 0x0044eee4,
+ [APB2OPB_OPB0_WRITE_BYTE_ENDIAN] = 0x0055aaff,
+ [APB2OPB_OPB1_WRITE_WORD_ENDIAN] = 0x00117717,
+ [APB2OPB_OPB1_WRITE_BYTE_ENDIAN] = 0xffaa5500,
+ [APB2OPB_OPB0_READ_BYTE_ENDIAN] = 0x0044eee4,
+ [APB2OPB_OPB1_READ_BYTE_ENDIAN] = 0x00117717
+};
+
+static void fsi_opb_fsi_master_address(FSIMasterState *fsi, hwaddr addr)
+{
+ memory_region_transaction_begin();
+ memory_region_set_address(&fsi->iomem, addr);
+ memory_region_transaction_commit();
+}
+
+static void fsi_opb_opb2fsi_address(FSIMasterState *fsi, hwaddr addr)
+{
+ memory_region_transaction_begin();
+ memory_region_set_address(&fsi->opb2fsi, addr);
+ memory_region_transaction_commit();
+}
+
+static uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
+ unsigned int reg = TO_REG(addr);
+
+ trace_fsi_aspeed_apb2opb_read(addr, size);
+
+ if (reg >= ASPEED_APB2OPB_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds read: 0x%"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return 0;
+ }
+
+ return s->regs[reg];
+}
+
+static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
+ unsigned int reg = TO_REG(addr);
+
+ trace_fsi_aspeed_apb2opb_write(addr, size, data);
+
+ if (reg >= ASPEED_APB2OPB_NR_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ switch (reg) {
+ case APB2OPB_CONTROL:
+ fsi_opb_fsi_master_address(&s->fsi[0],
+ data & APB2OPB_CONTROL_OFF);
+ break;
+ case APB2OPB_OPB2FSI:
+ fsi_opb_opb2fsi_address(&s->fsi[0],
+ data & APB2OPB_OPB2FSI_OFF);
+ break;
+ case APB2OPB_OPB0_WRITE_WORD_ENDIAN:
+ if (data != APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bridge needs to be driven as BE (0x%x)\n",
+ __func__, APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE);
+ }
+ break;
+ case APB2OPB_OPB0_WRITE_BYTE_ENDIAN:
+ if (data != APB2OPB_OPB0_WRITE_BYTE_ENDIAN_BE) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bridge needs to be driven as BE (0x%x)\n",
+ __func__, APB2OPB_OPB0_WRITE_BYTE_ENDIAN_BE);
+ }
+ break;
+ case APB2OPB_OPB0_READ_BYTE_ENDIAN:
+ if (data != APB2OPB_OPB0_READ_WORD_ENDIAN_BE) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Bridge needs to be driven as BE (0x%x)\n",
+ __func__, APB2OPB_OPB0_READ_WORD_ENDIAN_BE);
+ }
+ break;
+ case APB2OPB_TRIGGER:
+ {
+ uint32_t opb, op_mode, op_size, op_addr, op_data;
+ MemTxResult result;
+ bool is_write;
+ int index;
+ AddressSpace *as;
+
+ assert((s->regs[APB2OPB_OPB0_SEL] & APB2OPB_OPB_SEL_EN) ^
+ (s->regs[APB2OPB_OPB1_SEL] & APB2OPB_OPB_SEL_EN));
+
+ if (s->regs[APB2OPB_OPB0_SEL] & APB2OPB_OPB_SEL_EN) {
+ opb = 0;
+ op_mode = s->regs[APB2OPB_OPB0_MODE];
+ op_size = s->regs[APB2OPB_OPB0_XFER];
+ op_addr = s->regs[APB2OPB_OPB0_ADDR];
+ op_data = s->regs[APB2OPB_OPB0_WRITE_DATA];
+ } else if (s->regs[APB2OPB_OPB1_SEL] & APB2OPB_OPB_SEL_EN) {
+ opb = 1;
+ op_mode = s->regs[APB2OPB_OPB1_MODE];
+ op_size = s->regs[APB2OPB_OPB1_XFER];
+ op_addr = s->regs[APB2OPB_OPB1_ADDR];
+ op_data = s->regs[APB2OPB_OPB1_WRITE_DATA];
+ } else {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Invalid operation: 0x%"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ if (op_size & ~(APB2OPB_OPB_XFER_HALF | APB2OPB_OPB_XFER_FULL)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "OPB transaction failed: Unrecognized access width: %d\n",
+ op_size);
+ return;
+ }
+
+ op_size += 1;
+ is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
+ index = opb ? APB2OPB_OPB1_READ_DATA : APB2OPB_OPB0_READ_DATA;
+ as = &s->opb[opb].as;
+
+ result = address_space_rw(as, op_addr, MEMTXATTRS_UNSPECIFIED,
+ &op_data, op_size, is_write);
+ if (result != MEMTX_OK) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: OPB %s failed @%08x\n",
+ __func__, is_write ? "write" : "read", op_addr);
+ return;
+ }
+
+ if (!is_write) {
+ s->regs[index] = op_data;
+ }
+
+ s->regs[APB2OPB_IRQ_STS] |= opb ? APB2OPB_IRQ_STS_OPB1_TX_ACK
+ : APB2OPB_IRQ_STS_OPB0_TX_ACK;
+ break;
+ }
+ }
+
+ s->regs[reg] = data;
+}
+
+static const struct MemoryRegionOps aspeed_apb2opb_ops = {
+ .read = fsi_aspeed_apb2opb_read,
+ .write = fsi_aspeed_apb2opb_write,
+ .valid.max_access_size = 4,
+ .valid.min_access_size = 4,
+ .impl.max_access_size = 4,
+ .impl.min_access_size = 4,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void fsi_aspeed_apb2opb_init(Object *o)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(o);
+ int i;
+
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, DEVICE(s),
+ NULL);
+ }
+
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ object_initialize_child(o, "fsi-master[*]", &s->fsi[i],
+ TYPE_FSI_MASTER);
+ }
+}
+
+static void fsi_aspeed_apb2opb_realize(DeviceState *dev, Error **errp)
+{
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
+ int i;
+
+ sysbus_init_irq(sbd, &s->irq);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_apb2opb_ops, s,
+ TYPE_ASPEED_APB2OPB, 0x1000);
+ sysbus_init_mmio(sbd, &s->iomem);
+
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ if (!qdev_realize(DEVICE(&s->fsi[i]), BUS(&s->opb[i]),
+ errp)) {
+ return;
+ }
+
+ memory_region_add_subregion(&s->opb[i].mr, 0x80000000,
+ &s->fsi[i].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(&s->opb[i].mr, 0xa0000000,
+ &s->fsi[i].opb2fsi);
+ }
+}
+
+static void fsi_aspeed_apb2opb_reset(DeviceState *dev)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
+
+ memcpy(s->regs, aspeed_apb2opb_reset, ASPEED_APB2OPB_NR_REGS);
+}
+
+static void fsi_aspeed_apb2opb_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->desc = "ASPEED APB2OPB Bridge";
+ dc->realize = fsi_aspeed_apb2opb_realize;
+ dc->reset = fsi_aspeed_apb2opb_reset;
+}
+
+static const TypeInfo aspeed_apb2opb_info = {
+ .name = TYPE_ASPEED_APB2OPB,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_init = fsi_aspeed_apb2opb_init,
+ .instance_size = sizeof(AspeedAPB2OPBState),
+ .class_init = fsi_aspeed_apb2opb_class_init,
+};
+
+static void aspeed_apb2opb_register_types(void)
+{
+ type_register_static(&aspeed_apb2opb_info);
+}
+
+type_init(aspeed_apb2opb_register_types);
+
+static void fsi_opb_init(Object *o)
+{
+ OPBus *opb = OP_BUS(o);
+
+ memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb,
+ TYPE_FSI_OPB, UINT32_MAX);
+ address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
+}
+
+static const TypeInfo opb_info = {
+ .name = TYPE_OP_BUS,
+ .parent = TYPE_BUS,
+ .instance_init = fsi_opb_init,
+ .instance_size = sizeof(OPBus),
+};
+
+static void fsi_opb_register_types(void)
+{
+ type_register_static(&opb_info);
+}
+
+type_init(fsi_opb_register_types);
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 660f49db49..4ae424acdd 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -560,6 +560,7 @@ config ASPEED_SOC
select LED
select PMBUS
select MAX31785
+ select FSI_APB2OPB_ASPEED
config MPS2
bool
diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
index cc8a8c1418..56ff42c14c 100644
--- a/hw/misc/Kconfig
+++ b/hw/misc/Kconfig
@@ -200,4 +200,9 @@ config IOSB
config XLNX_VERSAL_TRNG
bool
+config FSI_APB2OPB_ASPEED
+ bool
+ depends on ASPEED_SOC
+ select FSI
+
source macio/Kconfig
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 36c20d5637..6b5cc63e17 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -127,6 +127,7 @@ system_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
system_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
system_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
+ 'aspeed-apb2opb.c',
'aspeed_hace.c',
'aspeed_i3c.c',
'aspeed_lpc.c',
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 85725506bf..7d72771db8 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -330,3 +330,7 @@ djmemc_write(int reg, uint64_t value, unsigned int size) "reg=0x%x value=0x%"PRI
# iosb.c
iosb_read(int reg, uint64_t value, unsigned int size) "reg=0x%x value=0x%"PRIx64" size=%u"
iosb_write(int reg, uint64_t value, unsigned int size) "reg=0x%x value=0x%"PRIx64" size=%u"
+
+# aspeed-apb2opb.c
+fsi_aspeed_apb2opb_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_aspeed_apb2opb_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
2024-01-10 23:15 ` [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus Ninad Palsule
@ 2024-01-12 16:00 ` Cédric Le Goater
2024-01-26 2:23 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 16:00 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
The subject is not very clear.
On 1/11/24 00:15, Ninad Palsule wrote:
> This is a part of patchset where IBM's Flexible Service Interface is
> introduced.
>
> An APB-to-OPB bridge enabling access to the OPB from the ARM core in
> the AST2600. Hardware limitations prevent the OPB from being directly
> mapped into APB, so all accesses are indirect through the bridge.
>
> 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.
>
> [ clg: - moved FSIMasterState under AspeedAPB2OPBState
> - modified fsi_opb_fsi_master_address() and
> fsi_opb_opb2fsi_address()
> - instroduced fsi_aspeed_apb2opb_init()
> - reworked fsi_aspeed_apb2opb_realize()
> - removed FSIMasterState object and fsi_opb_realize()
> - simplified OPBus ]
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
Nah. Joel you should re-review next respin :)
> ---
> v9:
> - Removed unused parameters from function.
> - Used qdev_realize() instead of qdev_realize_and_undef
> - Given a name to the opb memory region.
>
> v10:
> - Combine Aspeed APB2OPB and on-chip pheripheral bus
>
> ---
> include/hw/misc/aspeed-apb2opb.h | 50 +++++
> hw/misc/aspeed-apb2opb.c | 338 +++++++++++++++++++++++++++++++
As said in the cover letter, I think now that hw/fsi is a better place
for these files and should be compiled if CONSG_ASPEED_SOC. Sorry about
that. Also,please use 'aspeed_' for the file names.
> hw/arm/Kconfig | 1 +
> hw/misc/Kconfig | 5 +
> hw/misc/meson.build | 1 +
> hw/misc/trace-events | 4 +
> 6 files changed, 399 insertions(+)
> create mode 100644 include/hw/misc/aspeed-apb2opb.h
> create mode 100644 hw/misc/aspeed-apb2opb.c
>
> diff --git a/include/hw/misc/aspeed-apb2opb.h b/include/hw/misc/aspeed-apb2opb.h
> new file mode 100644
> index 0000000000..fcd76631a9
> --- /dev/null
> +++ b/include/hw/misc/aspeed-apb2opb.h
> @@ -0,0 +1,50 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2024 IBM Corp.
> + *
> + * ASPEED APB2OPB Bridge
> + * IBM On-Chip Peripheral Bus
> + */
> +#ifndef FSI_ASPEED_APB2OPB_H
> +#define FSI_ASPEED_APB2OPB_H
> +
> +#include "exec/memory.h"
> +#include "hw/fsi/fsi-master.h"
> +#include "hw/sysbus.h"
> +
> +#define TYPE_FSI_OPB "fsi.opb"
> +
> +#define TYPE_OP_BUS "opb"
> +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
> +
> +typedef struct OPBus {
> + /*< private >*/
please remove the private and public comment.
> + BusState bus;
> +
> + /*< public >*/
> + MemoryRegion mr;
> + AddressSpace as;
indent is wrong.
> +} OPBus;
> +
> +#define TYPE_ASPEED_APB2OPB "aspeed.apb2opb"
> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedAPB2OPBState, ASPEED_APB2OPB)
> +
> +#define ASPEED_APB2OPB_NR_REGS ((0xe8 >> 2) + 1)
> +
> +#define ASPEED_FSI_NUM 2
> +
> +typedef struct AspeedAPB2OPBState {
> + /*< private >*/
> + SysBusDevice parent_obj;
> +
> + /*< public >*/
> + MemoryRegion iomem;
> +
> + uint32_t regs[ASPEED_APB2OPB_NR_REGS];
> + qemu_irq irq;
> +
> + OPBus opb[ASPEED_FSI_NUM];
> + FSIMasterState fsi[ASPEED_FSI_NUM];
> +} AspeedAPB2OPBState;
> +
> +#endif /* FSI_ASPEED_APB2OPB_H */
> diff --git a/hw/misc/aspeed-apb2opb.c b/hw/misc/aspeed-apb2opb.c
> new file mode 100644
> index 0000000000..19545c780f
> --- /dev/null
> +++ b/hw/misc/aspeed-apb2opb.c
> @@ -0,0 +1,338 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2024 IBM Corp.
> + *
> + * ASPEED APB-OPB FSI interface
> + * IBM On-chip Peripheral Bus
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qom/object.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +
> +#include "hw/misc/aspeed-apb2opb.h"
> +#include "hw/qdev-core.h"
> +
> +#define TO_REG(x) (x >> 2)
> +
> +#define APB2OPB_VERSION TO_REG(0x00)
> +#define APB2OPB_TRIGGER TO_REG(0x04)
> +
> +#define APB2OPB_CONTROL TO_REG(0x08)
> +#define APB2OPB_CONTROL_OFF BE_GENMASK(31, 13)
> +
> +#define APB2OPB_OPB2FSI TO_REG(0x0c)
> +#define APB2OPB_OPB2FSI_OFF BE_GENMASK(31, 22)
> +
> +#define APB2OPB_OPB0_SEL TO_REG(0x10)
> +#define APB2OPB_OPB1_SEL TO_REG(0x28)
> +#define APB2OPB_OPB_SEL_EN BIT(0)
> +
> +#define APB2OPB_OPB0_MODE TO_REG(0x14)
> +#define APB2OPB_OPB1_MODE TO_REG(0x2c)
> +#define APB2OPB_OPB_MODE_RD BIT(0)
> +
> +#define APB2OPB_OPB0_XFER TO_REG(0x18)
> +#define APB2OPB_OPB1_XFER TO_REG(0x30)
> +#define APB2OPB_OPB_XFER_FULL BIT(1)
> +#define APB2OPB_OPB_XFER_HALF BIT(0)
> +
> +#define APB2OPB_OPB0_ADDR TO_REG(0x1c)
> +#define APB2OPB_OPB0_WRITE_DATA TO_REG(0x20)
> +
> +#define APB2OPB_OPB1_ADDR TO_REG(0x34)
> +#define APB2OPB_OPB1_WRITE_DATA TO_REG(0x38)
> +
> +#define APB2OPB_IRQ_STS TO_REG(0x48)
> +#define APB2OPB_IRQ_STS_OPB1_TX_ACK BIT(17)
> +#define APB2OPB_IRQ_STS_OPB0_TX_ACK BIT(16)
> +
> +#define APB2OPB_OPB0_WRITE_WORD_ENDIAN TO_REG(0x4c)
> +#define APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE 0x0011101b
> +#define APB2OPB_OPB0_WRITE_BYTE_ENDIAN TO_REG(0x50)
> +#define APB2OPB_OPB0_WRITE_BYTE_ENDIAN_BE 0x0c330f3f
> +#define APB2OPB_OPB1_WRITE_WORD_ENDIAN TO_REG(0x54)
> +#define APB2OPB_OPB1_WRITE_BYTE_ENDIAN TO_REG(0x58)
> +#define APB2OPB_OPB0_READ_BYTE_ENDIAN TO_REG(0x5c)
> +#define APB2OPB_OPB1_READ_BYTE_ENDIAN TO_REG(0x60)
> +#define APB2OPB_OPB0_READ_WORD_ENDIAN_BE 0x00030b1b
> +
> +#define APB2OPB_OPB0_READ_DATA TO_REG(0x84)
> +#define APB2OPB_OPB1_READ_DATA TO_REG(0x90)
> +
> +/*
> + * The following magic values came from AST2600 data sheet
> + * The register values are defined under section "FSI controller"
> + * as initial values.
> + */
> +static const uint32_t aspeed_apb2opb_reset[ASPEED_APB2OPB_NR_REGS] = {
> + [APB2OPB_VERSION] = 0x000000a1,
> + [APB2OPB_OPB0_WRITE_WORD_ENDIAN] = 0x0044eee4,
> + [APB2OPB_OPB0_WRITE_BYTE_ENDIAN] = 0x0055aaff,
> + [APB2OPB_OPB1_WRITE_WORD_ENDIAN] = 0x00117717,
> + [APB2OPB_OPB1_WRITE_BYTE_ENDIAN] = 0xffaa5500,
> + [APB2OPB_OPB0_READ_BYTE_ENDIAN] = 0x0044eee4,
> + [APB2OPB_OPB1_READ_BYTE_ENDIAN] = 0x00117717
> +};
> +
> +static void fsi_opb_fsi_master_address(FSIMasterState *fsi, hwaddr addr)
> +{
> + memory_region_transaction_begin();
> + memory_region_set_address(&fsi->iomem, addr);
> + memory_region_transaction_commit();
> +}
> +
> +static void fsi_opb_opb2fsi_address(FSIMasterState *fsi, hwaddr addr)
> +{
> + memory_region_transaction_begin();
> + memory_region_set_address(&fsi->opb2fsi, addr);
> + memory_region_transaction_commit();
> +}
> +
> +static uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
> + unsigned int reg = TO_REG(addr);
> +
> + trace_fsi_aspeed_apb2opb_read(addr, size);
> +
> + if (reg >= ASPEED_APB2OPB_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds read: 0x%"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return 0;
> + }
> +
> + return s->regs[reg];
> +}
> +
> +static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
> + unsigned int reg = TO_REG(addr);
> +
> + trace_fsi_aspeed_apb2opb_write(addr, size, data);
> +
> + if (reg >= ASPEED_APB2OPB_NR_REGS) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return;
> + }
> +
> + switch (reg) {
> + case APB2OPB_CONTROL:
> + fsi_opb_fsi_master_address(&s->fsi[0],
> + data & APB2OPB_CONTROL_OFF);
> + break;
> + case APB2OPB_OPB2FSI:
> + fsi_opb_opb2fsi_address(&s->fsi[0],
> + data & APB2OPB_OPB2FSI_OFF);
> + break;
> + case APB2OPB_OPB0_WRITE_WORD_ENDIAN:
> + if (data != APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Bridge needs to be driven as BE (0x%x)\n",
> + __func__, APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE);
> + }
> + break;
> + case APB2OPB_OPB0_WRITE_BYTE_ENDIAN:
> + if (data != APB2OPB_OPB0_WRITE_BYTE_ENDIAN_BE) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Bridge needs to be driven as BE (0x%x)\n",
> + __func__, APB2OPB_OPB0_WRITE_BYTE_ENDIAN_BE);
> + }
> + break;
> + case APB2OPB_OPB0_READ_BYTE_ENDIAN:
> + if (data != APB2OPB_OPB0_READ_WORD_ENDIAN_BE) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Bridge needs to be driven as BE (0x%x)\n",
> + __func__, APB2OPB_OPB0_READ_WORD_ENDIAN_BE);
> + }
> + break;
> + case APB2OPB_TRIGGER:
> + {
> + uint32_t opb, op_mode, op_size, op_addr, op_data;
> + MemTxResult result;
> + bool is_write;
> + int index;
> + AddressSpace *as;
> +
> + assert((s->regs[APB2OPB_OPB0_SEL] & APB2OPB_OPB_SEL_EN) ^
> + (s->regs[APB2OPB_OPB1_SEL] & APB2OPB_OPB_SEL_EN));
> +
> + if (s->regs[APB2OPB_OPB0_SEL] & APB2OPB_OPB_SEL_EN) {
> + opb = 0;
> + op_mode = s->regs[APB2OPB_OPB0_MODE];
> + op_size = s->regs[APB2OPB_OPB0_XFER];
> + op_addr = s->regs[APB2OPB_OPB0_ADDR];
> + op_data = s->regs[APB2OPB_OPB0_WRITE_DATA];
> + } else if (s->regs[APB2OPB_OPB1_SEL] & APB2OPB_OPB_SEL_EN) {
> + opb = 1;
> + op_mode = s->regs[APB2OPB_OPB1_MODE];
> + op_size = s->regs[APB2OPB_OPB1_XFER];
> + op_addr = s->regs[APB2OPB_OPB1_ADDR];
> + op_data = s->regs[APB2OPB_OPB1_WRITE_DATA];
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Invalid operation: 0x%"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return;
> + }
> +
> + if (op_size & ~(APB2OPB_OPB_XFER_HALF | APB2OPB_OPB_XFER_FULL)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "OPB transaction failed: Unrecognized access width: %d\n",
> + op_size);
> + return;
> + }
> +
> + op_size += 1;
> + is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
> + index = opb ? APB2OPB_OPB1_READ_DATA : APB2OPB_OPB0_READ_DATA;
> + as = &s->opb[opb].as;
> +
> + result = address_space_rw(as, op_addr, MEMTXATTRS_UNSPECIFIED,
> + &op_data, op_size, is_write);
> + if (result != MEMTX_OK) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: OPB %s failed @%08x\n",
> + __func__, is_write ? "write" : "read", op_addr);
> + return;
> + }
> +
> + if (!is_write) {
> + s->regs[index] = op_data;
> + }
> +
> + s->regs[APB2OPB_IRQ_STS] |= opb ? APB2OPB_IRQ_STS_OPB1_TX_ACK
> + : APB2OPB_IRQ_STS_OPB0_TX_ACK;
> + break;
> + }
> + }
> +
> + s->regs[reg] = data;
> +}
> +
> +static const struct MemoryRegionOps aspeed_apb2opb_ops = {
> + .read = fsi_aspeed_apb2opb_read,
> + .write = fsi_aspeed_apb2opb_write,
> + .valid.max_access_size = 4,
> + .valid.min_access_size = 4,
> + .impl.max_access_size = 4,
> + .impl.min_access_size = 4,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void fsi_aspeed_apb2opb_init(Object *o)
> +{
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(o);
> + int i;
> +
> + for (i = 0; i < ASPEED_FSI_NUM; i++) {
> + qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, DEVICE(s),
> + NULL);
See comment in fsi_opb_init()
> + }
> +
> + for (i = 0; i < ASPEED_FSI_NUM; i++) {
> + object_initialize_child(o, "fsi-master[*]", &s->fsi[i],
> + TYPE_FSI_MASTER);
> + }
> +}
> +
> +static void fsi_aspeed_apb2opb_realize(DeviceState *dev, Error **errp)
> +{
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
> + int i;
> +
> + sysbus_init_irq(sbd, &s->irq);
> +
> + memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_apb2opb_ops, s,
> + TYPE_ASPEED_APB2OPB, 0x1000);
> + sysbus_init_mmio(sbd, &s->iomem);
> +
> + for (i = 0; i < ASPEED_FSI_NUM; i++) {
> + if (!qdev_realize(DEVICE(&s->fsi[i]), BUS(&s->opb[i]),
> + errp)) {
this could be a single line.
> + return;
> + }
> +
> + memory_region_add_subregion(&s->opb[i].mr, 0x80000000,
> + &s->fsi[i].iomem);
> +
> + /* OPB2FSI region */
Please remove the comments below, I am not sure they are valid
anymore.
> + /*
> + * 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(&s->opb[i].mr, 0xa0000000,
> + &s->fsi[i].opb2fsi);
> + }
> +}
> +
> +static void fsi_aspeed_apb2opb_reset(DeviceState *dev)
> +{
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
> +
> + memcpy(s->regs, aspeed_apb2opb_reset, ASPEED_APB2OPB_NR_REGS);
> +}
> +
> +static void fsi_aspeed_apb2opb_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->desc = "ASPEED APB2OPB Bridge";
> + dc->realize = fsi_aspeed_apb2opb_realize;
> + dc->reset = fsi_aspeed_apb2opb_reset;
> +}
> +
> +static const TypeInfo aspeed_apb2opb_info = {
> + .name = TYPE_ASPEED_APB2OPB,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_init = fsi_aspeed_apb2opb_init,
> + .instance_size = sizeof(AspeedAPB2OPBState),
> + .class_init = fsi_aspeed_apb2opb_class_init,
> +};
> +
> +static void aspeed_apb2opb_register_types(void)
> +{
> + type_register_static(&aspeed_apb2opb_info);
> +}
> +
> +type_init(aspeed_apb2opb_register_types);
> +
> +static void fsi_opb_init(Object *o)
> +{
> + OPBus *opb = OP_BUS(o);
> +
> + memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb,
> + TYPE_FSI_OPB, UINT32_MAX);
This is better :
memory_region_init(&opb->mr, o, TYPE_OP_BUS, UINT32_MAX);
> + address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
This routine is problematic. If you run 'make check', you should see
test tests/qtest/device-introspect-test crash in weird way because of
a memory corruption. I didn't dig into the details but I suppose this
a use after free problem.
To solve, we should move qbus_init() done in fsi_aspeed_apb2opb_init()
under fsi_aspeed_apb2opb_realize(), or improve the model a litle more.
It seems we are lacking the OPB/FSI bridge :
typedef struct OPBFSIBridge {
DeviceState parent;
OPBus opb;
FSIMasterState fsi;
MemoryRegion mr;
AddressSpace as;
} OPBFSIBridge;
Something like that. It is difficult to understand the design from
the OpenFSI specs. The OPB bus seems overkill. It you could clarify
this aspect, it would be nice.
Thanks,
C.
> +}
> +
> +static const TypeInfo opb_info = {
> + .name = TYPE_OP_BUS,
> + .parent = TYPE_BUS,
> + .instance_init = fsi_opb_init,
> + .instance_size = sizeof(OPBus),
> +};
> +
> +static void fsi_opb_register_types(void)
> +{
> + type_register_static(&opb_info);
> +}
> +
> +type_init(fsi_opb_register_types);
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 660f49db49..4ae424acdd 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -560,6 +560,7 @@ config ASPEED_SOC
> select LED
> select PMBUS
> select MAX31785
> + select FSI_APB2OPB_ASPEED
>
> config MPS2
> bool
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index cc8a8c1418..56ff42c14c 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -200,4 +200,9 @@ config IOSB
> config XLNX_VERSAL_TRNG
> bool
>
> +config FSI_APB2OPB_ASPEED
> + bool
> + depends on ASPEED_SOC
> + select FSI
> +
> source macio/Kconfig
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 36c20d5637..6b5cc63e17 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -127,6 +127,7 @@ system_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
> system_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
> system_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
> system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> + 'aspeed-apb2opb.c',
> 'aspeed_hace.c',
> 'aspeed_i3c.c',
> 'aspeed_lpc.c',
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 85725506bf..7d72771db8 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -330,3 +330,7 @@ djmemc_write(int reg, uint64_t value, unsigned int size) "reg=0x%x value=0x%"PRI
> # iosb.c
> iosb_read(int reg, uint64_t value, unsigned int size) "reg=0x%x value=0x%"PRIx64" size=%u"
> iosb_write(int reg, uint64_t value, unsigned int size) "reg=0x%x value=0x%"PRIx64" size=%u"
> +
> +# aspeed-apb2opb.c
> +fsi_aspeed_apb2opb_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> +fsi_aspeed_apb2opb_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
2024-01-12 16:00 ` Cédric Le Goater
@ 2024-01-26 2:23 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-26 2:23 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Cedric,
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> Nah. Joel you should re-review next respin :)
Removed Joel's review tag.
>
>>
>> ---
>> include/hw/misc/aspeed-apb2opb.h | 50 +++++
>> hw/misc/aspeed-apb2opb.c | 338 +++++++++++++++++++++++++++++++
>
> As said in the cover letter, I think now that hw/fsi is a better place
> for these files and should be compiled if CONSG_ASPEED_SOC. Sorry about
> that. Also,please use 'aspeed_' for the file names.
Transferred to the old location and rename with "aspeed_" prefix.
>
>>
>> +
>> +#define TYPE_OP_BUS "opb"
>> +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
>> +
>> +typedef struct OPBus {
>> + /*< private >*/
>
> please remove the private and public comment.
Removed.
>
>> + BusState bus;
>> +
>> + /*< public >*/
>> + MemoryRegion mr;
>> + AddressSpace as;
>
> indent is wrong.
Fixed indent.
>
>>
>> +static void fsi_aspeed_apb2opb_init(Object *o)
>> +{
>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(o);
>> + int i;
>> +
>> + for (i = 0; i < ASPEED_FSI_NUM; i++) {
>> + qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS,
>> DEVICE(s),
>> + NULL);
>
> See comment in fsi_opb_init()
Moved it to si_aspeed_apb2opb_realize(),
>
>> + for (i = 0; i < ASPEED_FSI_NUM; i++) {
>> + if (!qdev_realize(DEVICE(&s->fsi[i]), BUS(&s->opb[i]),
>> + errp)) {
>
> this could be a single line.
Removed extra line.
>
> Please remove the comments below, I am not sure they are valid
> anymore.
Removed the comment.
>
>> + /*
>> + * 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(&s->opb[i].mr, 0xa0000000,
>> + &s->fsi[i].opb2fsi);
>> + }
>> +}
>>
>> +
>> +static void fsi_opb_init(Object *o)
>> +{
>> + OPBus *opb = OP_BUS(o);
>> +
>> + memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb,
>> + TYPE_FSI_OPB, UINT32_MAX);
>
> This is better :
>
> memory_region_init(&opb->mr, o, TYPE_OP_BUS, UINT32_MAX);
Changed it as per your suggestion.
>
>
>> + address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
>
> This routine is problematic. If you run 'make check', you should see
> test tests/qtest/device-introspect-test crash in weird way because of
> a memory corruption. I didn't dig into the details but I suppose this
> a use after free problem.
>
> To solve, we should move qbus_init() done in fsi_aspeed_apb2opb_init()
> under fsi_aspeed_apb2opb_realize(), or improve the model a litle more.
>
> It seems we are lacking the OPB/FSI bridge :
>
> typedef struct OPBFSIBridge {
> DeviceState parent;
>
> OPBus opb;
> FSIMasterState fsi;
> MemoryRegion mr;
> AddressSpace as;
> } OPBFSIBridge;
>
> Something like that. It is difficult to understand the design from
> the OpenFSI specs. The OPB bus seems overkill. It you could clarify
> this aspect, it would be nice.
For now moved qbus_init() to fsi_aspeed_apb2opb_realize(). I will run
make check.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 6/9] hw/arm: Hook up FSI module in AST2600
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (4 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-10 23:15 ` [PATCH v10 7/9] hw/fsi: Added qtest Ninad Palsule
` (3 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm, Andrew Jeffery
This patchset introduces IBM's Flexible Service Interface(FSI).
Time for some fun with inter-processor buses. FSI allows a service
processor access to the internal buses of a host POWER processor to
perform configuration or debugging.
FSI has long existed in POWER processes and so comes with some baggage,
including how it has been integrated into the ASPEED SoC.
Working backwards from the POWER processor, the fundamental pieces of
interest for the implementation are:
1. The Common FRU Access Macro (CFAM), an address space containing
various "engines" that drive accesses on buses internal and external
to the POWER chip. Examples include the SBEFIFO and I2C masters. The
engines hang off of an internal Local Bus (LBUS) which is described
by the CFAM configuration block.
2. The FSI slave: The slave is the terminal point of the FSI bus for
FSI symbols addressed to it. Slaves can be cascaded off of one
another. The slave's configuration registers appear in address space
of the CFAM to which it is attached.
3. The FSI master: A controller in the platform service processor (e.g.
BMC) driving CFAM engine accesses into the POWER chip. At the
hardware level FSI is a bit-based protocol supporting synchronous and
DMA-driven accesses of engines in a CFAM.
4. 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.
5. An APB-to-OPB bridge enabling access to the OPB from the ARM core in
the AST2600. Hardware limitations prevent the OPB from being directly
mapped into APB, so all accesses are indirect through the bridge.
The implementation appears as following in the qemu device tree:
(qemu) info qtree
bus: main-system-bus
type System
...
dev: aspeed.apb2opb, id ""
gpio-out "sysbus-irq" 1
mmio 000000001e79b000/0000000000001000
bus: opb.1
type opb
dev: fsi.master, id ""
bus: fsi.bus.1
type fsi.bus
dev: cfam.config, id ""
dev: cfam, id ""
bus: fsi.lbus.1
type lbus
dev: scratchpad, id ""
address = 0 (0x0)
bus: opb.0
type opb
dev: fsi.master, id ""
bus: fsi.bus.0
type fsi.bus
dev: cfam.config, id ""
dev: cfam, id ""
bus: fsi.lbus.0
type lbus
dev: scratchpad, id ""
address = 0 (0x0)
The LBUS is modelled to maintain the qdev bus hierarchy and to take
advantage of the object model to automatically generate the CFAM
configuration block. The configuration block presents engines in the
order they are attached to the CFAM's LBUS. Engine implementations
should subclass the LBusDevice and set the 'config' member of
LBusDeviceClass to match the engine's type.
CFAM designs offer a lot of flexibility, for instance it is possible for
a CFAM to be simultaneously driven from multiple FSI links. The modeling
is not so complete; it's assumed that each CFAM is attached to a single
FSI slave (as a consequence the CFAM subclasses the FSI slave).
As for FSI, its symbols and wire-protocol are not modelled at all. This
is not necessary to get FSI off the ground thanks to the mapping of the
CFAM address space onto the OPB address space - the models follow this
directly and map the CFAM memory region into the OPB's memory region.
Future work includes supporting more advanced accesses that drive the
FSI master directly rather than indirectly via the CFAM mapping, which
will require implementing the FSI state machine and methods for each of
the FSI symbols on the slave. Further down the track we can also look at
supporting the bitbanged SoftFSI drivers in Linux by extending the FSI
slave model to resolve sequences of GPIO IRQs into FSI symbols, and
calling the associated symbol method on the slave to map the access onto
the CFAM.
Testing:
Tested by reading cfam config address 0 on rainier machine type.
root@p10bmc:~# pdbg -a getcfam 0x0
p0: 0x0 = 0xc0022d15
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
include/hw/arm/aspeed_soc.h | 4 ++++
hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index cb832bc1ee..9bc5c7a5ad 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -36,6 +36,7 @@
#include "hw/misc/aspeed_lpc.h"
#include "hw/misc/unimp.h"
#include "hw/misc/aspeed_peci.h"
+#include "hw/misc/aspeed-apb2opb.h"
#include "hw/char/serial.h"
#define ASPEED_SPIS_NUM 2
@@ -90,6 +91,7 @@ struct AspeedSoCState {
UnimplementedDeviceState udc;
UnimplementedDeviceState sgpiom;
UnimplementedDeviceState jtag[ASPEED_JTAG_NUM];
+ AspeedAPB2OPBState fsi[2];
};
#define TYPE_ASPEED_SOC "aspeed-soc"
@@ -214,6 +216,8 @@ enum {
ASPEED_DEV_SGPIOM,
ASPEED_DEV_JTAG0,
ASPEED_DEV_JTAG1,
+ ASPEED_DEV_FSI1,
+ ASPEED_DEV_FSI2,
};
#define ASPEED_SOC_SPI_BOOT_ADDR 0x0
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 3a9a303ab8..30da88361b 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -75,6 +75,8 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
[ASPEED_DEV_UART12] = 0x1E790600,
[ASPEED_DEV_UART13] = 0x1E790700,
[ASPEED_DEV_VUART] = 0x1E787000,
+ [ASPEED_DEV_FSI1] = 0x1E79B000,
+ [ASPEED_DEV_FSI2] = 0x1E79B100,
[ASPEED_DEV_I3C] = 0x1E7A0000,
[ASPEED_DEV_SDRAM] = 0x80000000,
};
@@ -132,6 +134,8 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_ETH4] = 33,
[ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
[ASPEED_DEV_DP] = 62,
+ [ASPEED_DEV_FSI1] = 100,
+ [ASPEED_DEV_FSI2] = 101,
[ASPEED_DEV_I3C] = 102, /* 102 -> 107 */
};
@@ -264,6 +268,10 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "emmc-boot-controller",
&s->emmc_boot_controller,
TYPE_UNIMPLEMENTED_DEVICE);
+
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ object_initialize_child(obj, "fsi[*]", &s->fsi[i], TYPE_ASPEED_APB2OPB);
+ }
}
/*
@@ -623,6 +631,17 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
return;
}
aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sbc), 0, sc->memmap[ASPEED_DEV_SBC]);
+
+ /* FSI */
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->fsi[i]), errp)) {
+ return;
+ }
+ aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->fsi[i]), 0,
+ sc->memmap[ASPEED_DEV_FSI1 + i]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->fsi[i]), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_FSI1 + i));
+ }
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH v10 7/9] hw/fsi: Added qtest
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (5 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 6/9] hw/arm: Hook up FSI module in AST2600 Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 16:02 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 8/9] hw/fsi: Added FSI documentation Ninad Palsule
` (2 subsequent siblings)
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm
Added basic qtests for FSI model.
Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
tests/qtest/aspeed-fsi-test.c | 205 ++++++++++++++++++++++++++++++++++
tests/qtest/meson.build | 1 +
2 files changed, 206 insertions(+)
create mode 100644 tests/qtest/aspeed-fsi-test.c
diff --git a/tests/qtest/aspeed-fsi-test.c b/tests/qtest/aspeed-fsi-test.c
new file mode 100644
index 0000000000..b3020dd821
--- /dev/null
+++ b/tests/qtest/aspeed-fsi-test.c
@@ -0,0 +1,205 @@
+/*
+ * QTest testcases for IBM's Flexible Service Interface (FSI)
+ *
+ * Copyright (c) 2023 IBM Corporation
+ *
+ * Authors:
+ * Ninad Palsule <ninad@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include <glib/gstdio.h>
+
+#include "qemu/module.h"
+#include "libqtest-single.h"
+
+/* Registers from ast2600 specifications */
+#define ASPEED_FSI_ENGINER_TRIGGER 0x04
+#define ASPEED_FSI_OPB0_BUS_SELECT 0x10
+#define ASPEED_FSI_OPB1_BUS_SELECT 0x28
+#define ASPEED_FSI_OPB0_RW_DIRECTION 0x14
+#define ASPEED_FSI_OPB1_RW_DIRECTION 0x2c
+#define ASPEED_FSI_OPB0_XFER_SIZE 0x18
+#define ASPEED_FSI_OPB1_XFER_SIZE 0x30
+#define ASPEED_FSI_OPB0_BUS_ADDR 0x1c
+#define ASPEED_FSI_OPB1_BUS_ADDR 0x34
+#define ASPEED_FSI_INTRRUPT_CLEAR 0x40
+#define ASPEED_FSI_INTRRUPT_STATUS 0x48
+#define ASPEED_FSI_OPB0_BUS_STATUS 0x80
+#define ASPEED_FSI_OPB1_BUS_STATUS 0x8c
+#define ASPEED_FSI_OPB0_READ_DATA 0x84
+#define ASPEED_FSI_OPB1_READ_DATA 0x90
+
+/*
+ * FSI Base addresses from the ast2600 specifications.
+ */
+#define AST2600_OPB_FSI0_BASE_ADDR 0x1e79b000
+#define AST2600_OPB_FSI1_BASE_ADDR 0x1e79b100
+
+static uint32_t aspeed_fsi_base_addr;
+
+static uint32_t aspeed_fsi_readl(QTestState *s, uint32_t reg)
+{
+ return qtest_readl(s, aspeed_fsi_base_addr + reg);
+}
+
+static void aspeed_fsi_writel(QTestState *s, uint32_t reg, uint32_t val)
+{
+ qtest_writel(s, aspeed_fsi_base_addr + reg, val);
+}
+
+/* Setup base address and select register */
+static void test_fsi_setup(QTestState *s, uint32_t base_addr)
+{
+ uint32_t curval;
+
+ aspeed_fsi_base_addr = base_addr;
+
+ /* Set the base select register */
+ if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
+ /* Unselect FSI1 */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_SELECT, 0x0);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_SELECT);
+ g_assert_cmpuint(curval, ==, 0x0);
+
+ /* Select FSI0 */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_SELECT, 0x1);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_SELECT);
+ g_assert_cmpuint(curval, ==, 0x1);
+ } else if (base_addr == AST2600_OPB_FSI1_BASE_ADDR) {
+ /* Unselect FSI0 */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_SELECT, 0x0);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_SELECT);
+ g_assert_cmpuint(curval, ==, 0x0);
+
+ /* Select FSI1 */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_SELECT, 0x1);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_SELECT);
+ g_assert_cmpuint(curval, ==, 0x1);
+ } else {
+ g_assert_not_reached();
+ }
+}
+
+static void test_fsi_reg_change(QTestState *s, uint32_t reg, uint32_t newval)
+{
+ uint32_t base;
+ uint32_t curval;
+
+ base = aspeed_fsi_readl(s, reg);
+ aspeed_fsi_writel(s, reg, newval);
+ curval = aspeed_fsi_readl(s, reg);
+ g_assert_cmpuint(curval, ==, newval);
+ aspeed_fsi_writel(s, reg, base);
+ curval = aspeed_fsi_readl(s, reg);
+ g_assert_cmpuint(curval, ==, base);
+}
+
+static void test_fsi0_master_regs(const void *data)
+{
+ QTestState *s = (QTestState *)data;
+
+ test_fsi_setup(s, AST2600_OPB_FSI0_BASE_ADDR);
+
+ test_fsi_reg_change(s, ASPEED_FSI_OPB0_RW_DIRECTION, 0xF3F4F514);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB0_XFER_SIZE, 0xF3F4F518);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB0_BUS_ADDR, 0xF3F4F51c);
+ test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_CLEAR, 0xF3F4F540);
+ test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_STATUS, 0xF3F4F548);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB0_BUS_STATUS, 0xF3F4F580);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB0_READ_DATA, 0xF3F4F584);
+}
+
+static void test_fsi1_master_regs(const void *data)
+{
+ QTestState *s = (QTestState *)data;
+
+ test_fsi_setup(s, AST2600_OPB_FSI1_BASE_ADDR);
+
+ test_fsi_reg_change(s, ASPEED_FSI_OPB1_RW_DIRECTION, 0xF3F4F514);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB1_XFER_SIZE, 0xF3F4F518);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB1_BUS_ADDR, 0xF3F4F51c);
+ test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_CLEAR, 0xF3F4F540);
+ test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_STATUS, 0xF3F4F548);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB1_BUS_STATUS, 0xF3F4F580);
+ test_fsi_reg_change(s, ASPEED_FSI_OPB1_READ_DATA, 0xF3F4F584);
+}
+
+static void test_fsi0_getcfam_addr0(const void *data)
+{
+ QTestState *s = (QTestState *)data;
+ uint32_t curval;
+
+ test_fsi_setup(s, AST2600_OPB_FSI0_BASE_ADDR);
+
+ /* Master access direction read */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB0_RW_DIRECTION, 0x1);
+ /* word */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB0_XFER_SIZE, 0x3);
+ /* Address */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_ADDR, 0xa0000000);
+ aspeed_fsi_writel(s, ASPEED_FSI_INTRRUPT_CLEAR, 0x1);
+ aspeed_fsi_writel(s, ASPEED_FSI_ENGINER_TRIGGER, 0x1);
+
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_INTRRUPT_STATUS);
+ g_assert_cmpuint(curval, ==, 0x10000);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_STATUS);
+ g_assert_cmpuint(curval, ==, 0x0);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_READ_DATA);
+ g_assert_cmpuint(curval, ==, 0x152d02c0);
+}
+
+static void test_fsi1_getcfam_addr0(const void *data)
+{
+ QTestState *s = (QTestState *)data;
+ uint32_t curval;
+
+ test_fsi_setup(s, AST2600_OPB_FSI1_BASE_ADDR);
+
+ /* Master access direction read */
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB1_RW_DIRECTION, 0x1);
+
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB1_XFER_SIZE, 0x3);
+ aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_ADDR, 0xa0000000);
+ aspeed_fsi_writel(s, ASPEED_FSI_INTRRUPT_CLEAR, 0x1);
+ aspeed_fsi_writel(s, ASPEED_FSI_ENGINER_TRIGGER, 0x1);
+
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_INTRRUPT_STATUS);
+ g_assert_cmpuint(curval, ==, 0x20000);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_STATUS);
+ g_assert_cmpuint(curval, ==, 0x0);
+ curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_READ_DATA);
+ g_assert_cmpuint(curval, ==, 0x152d02c0);
+}
+
+int main(int argc, char **argv)
+{
+ int ret = -1;
+ QTestState *s;
+
+ g_test_init(&argc, &argv, NULL);
+
+ s = qtest_init("-machine ast2600-evb ");
+
+ /* Tests for OPB/FSI0 */
+ qtest_add_data_func("/aspeed-fsi-test/test_fsi0_master_regs", s,
+ test_fsi0_master_regs);
+
+ qtest_add_data_func("/aspeed-fsi-test/test_fsi0_getcfam_addr0", s,
+ test_fsi0_getcfam_addr0);
+
+ /* Tests for OPB/FSI1 */
+ qtest_add_data_func("/aspeed-fsi-test/test_fsi1_master_regs", s,
+ test_fsi1_master_regs);
+
+ qtest_add_data_func("/aspeed-fsi-test/test_fsi1_getcfam_addr0", s,
+ test_fsi1_getcfam_addr0);
+
+ ret = g_test_run();
+ qtest_quit(s);
+
+ return ret;
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index f25bffcc20..8c09159702 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -207,6 +207,7 @@ qtests_arm = \
(config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
(config_all_devices.has_key('CONFIG_VEXPRESS') ? ['test-arm-mptimer'] : []) + \
(config_all_devices.has_key('CONFIG_MICROBIT') ? ['microbit-test'] : []) + \
+ (config_all_devices.has_key('CONFIG_FSI_APB2OPB_ASPEED') ? ['aspeed-fsi-test'] : []) + \
['arm-cpu-features',
'boot-serial-test']
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 7/9] hw/fsi: Added qtest
2024-01-10 23:15 ` [PATCH v10 7/9] hw/fsi: Added qtest Ninad Palsule
@ 2024-01-12 16:02 ` Cédric Le Goater
2024-01-16 19:37 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 16:02 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
On 1/11/24 00:15, Ninad Palsule wrote:
> Added basic qtests for FSI model.
>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Please remove mu S-o-b.
Thanks,
C.
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> tests/qtest/aspeed-fsi-test.c | 205 ++++++++++++++++++++++++++++++++++
> tests/qtest/meson.build | 1 +
> 2 files changed, 206 insertions(+)
> create mode 100644 tests/qtest/aspeed-fsi-test.c
>
> diff --git a/tests/qtest/aspeed-fsi-test.c b/tests/qtest/aspeed-fsi-test.c
> new file mode 100644
> index 0000000000..b3020dd821
> --- /dev/null
> +++ b/tests/qtest/aspeed-fsi-test.c
> @@ -0,0 +1,205 @@
> +/*
> + * QTest testcases for IBM's Flexible Service Interface (FSI)
> + *
> + * Copyright (c) 2023 IBM Corporation
> + *
> + * Authors:
> + * Ninad Palsule <ninad@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include <glib/gstdio.h>
> +
> +#include "qemu/module.h"
> +#include "libqtest-single.h"
> +
> +/* Registers from ast2600 specifications */
> +#define ASPEED_FSI_ENGINER_TRIGGER 0x04
> +#define ASPEED_FSI_OPB0_BUS_SELECT 0x10
> +#define ASPEED_FSI_OPB1_BUS_SELECT 0x28
> +#define ASPEED_FSI_OPB0_RW_DIRECTION 0x14
> +#define ASPEED_FSI_OPB1_RW_DIRECTION 0x2c
> +#define ASPEED_FSI_OPB0_XFER_SIZE 0x18
> +#define ASPEED_FSI_OPB1_XFER_SIZE 0x30
> +#define ASPEED_FSI_OPB0_BUS_ADDR 0x1c
> +#define ASPEED_FSI_OPB1_BUS_ADDR 0x34
> +#define ASPEED_FSI_INTRRUPT_CLEAR 0x40
> +#define ASPEED_FSI_INTRRUPT_STATUS 0x48
> +#define ASPEED_FSI_OPB0_BUS_STATUS 0x80
> +#define ASPEED_FSI_OPB1_BUS_STATUS 0x8c
> +#define ASPEED_FSI_OPB0_READ_DATA 0x84
> +#define ASPEED_FSI_OPB1_READ_DATA 0x90
> +
> +/*
> + * FSI Base addresses from the ast2600 specifications.
> + */
> +#define AST2600_OPB_FSI0_BASE_ADDR 0x1e79b000
> +#define AST2600_OPB_FSI1_BASE_ADDR 0x1e79b100
> +
> +static uint32_t aspeed_fsi_base_addr;
> +
> +static uint32_t aspeed_fsi_readl(QTestState *s, uint32_t reg)
> +{
> + return qtest_readl(s, aspeed_fsi_base_addr + reg);
> +}
> +
> +static void aspeed_fsi_writel(QTestState *s, uint32_t reg, uint32_t val)
> +{
> + qtest_writel(s, aspeed_fsi_base_addr + reg, val);
> +}
> +
> +/* Setup base address and select register */
> +static void test_fsi_setup(QTestState *s, uint32_t base_addr)
> +{
> + uint32_t curval;
> +
> + aspeed_fsi_base_addr = base_addr;
> +
> + /* Set the base select register */
> + if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
> + /* Unselect FSI1 */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_SELECT, 0x0);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_SELECT);
> + g_assert_cmpuint(curval, ==, 0x0);
> +
> + /* Select FSI0 */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_SELECT, 0x1);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_SELECT);
> + g_assert_cmpuint(curval, ==, 0x1);
> + } else if (base_addr == AST2600_OPB_FSI1_BASE_ADDR) {
> + /* Unselect FSI0 */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_SELECT, 0x0);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_SELECT);
> + g_assert_cmpuint(curval, ==, 0x0);
> +
> + /* Select FSI1 */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_SELECT, 0x1);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_SELECT);
> + g_assert_cmpuint(curval, ==, 0x1);
> + } else {
> + g_assert_not_reached();
> + }
> +}
> +
> +static void test_fsi_reg_change(QTestState *s, uint32_t reg, uint32_t newval)
> +{
> + uint32_t base;
> + uint32_t curval;
> +
> + base = aspeed_fsi_readl(s, reg);
> + aspeed_fsi_writel(s, reg, newval);
> + curval = aspeed_fsi_readl(s, reg);
> + g_assert_cmpuint(curval, ==, newval);
> + aspeed_fsi_writel(s, reg, base);
> + curval = aspeed_fsi_readl(s, reg);
> + g_assert_cmpuint(curval, ==, base);
> +}
> +
> +static void test_fsi0_master_regs(const void *data)
> +{
> + QTestState *s = (QTestState *)data;
> +
> + test_fsi_setup(s, AST2600_OPB_FSI0_BASE_ADDR);
> +
> + test_fsi_reg_change(s, ASPEED_FSI_OPB0_RW_DIRECTION, 0xF3F4F514);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB0_XFER_SIZE, 0xF3F4F518);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB0_BUS_ADDR, 0xF3F4F51c);
> + test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_CLEAR, 0xF3F4F540);
> + test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_STATUS, 0xF3F4F548);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB0_BUS_STATUS, 0xF3F4F580);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB0_READ_DATA, 0xF3F4F584);
> +}
> +
> +static void test_fsi1_master_regs(const void *data)
> +{
> + QTestState *s = (QTestState *)data;
> +
> + test_fsi_setup(s, AST2600_OPB_FSI1_BASE_ADDR);
> +
> + test_fsi_reg_change(s, ASPEED_FSI_OPB1_RW_DIRECTION, 0xF3F4F514);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB1_XFER_SIZE, 0xF3F4F518);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB1_BUS_ADDR, 0xF3F4F51c);
> + test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_CLEAR, 0xF3F4F540);
> + test_fsi_reg_change(s, ASPEED_FSI_INTRRUPT_STATUS, 0xF3F4F548);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB1_BUS_STATUS, 0xF3F4F580);
> + test_fsi_reg_change(s, ASPEED_FSI_OPB1_READ_DATA, 0xF3F4F584);
> +}
> +
> +static void test_fsi0_getcfam_addr0(const void *data)
> +{
> + QTestState *s = (QTestState *)data;
> + uint32_t curval;
> +
> + test_fsi_setup(s, AST2600_OPB_FSI0_BASE_ADDR);
> +
> + /* Master access direction read */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB0_RW_DIRECTION, 0x1);
> + /* word */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB0_XFER_SIZE, 0x3);
> + /* Address */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB0_BUS_ADDR, 0xa0000000);
> + aspeed_fsi_writel(s, ASPEED_FSI_INTRRUPT_CLEAR, 0x1);
> + aspeed_fsi_writel(s, ASPEED_FSI_ENGINER_TRIGGER, 0x1);
> +
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_INTRRUPT_STATUS);
> + g_assert_cmpuint(curval, ==, 0x10000);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_BUS_STATUS);
> + g_assert_cmpuint(curval, ==, 0x0);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB0_READ_DATA);
> + g_assert_cmpuint(curval, ==, 0x152d02c0);
> +}
> +
> +static void test_fsi1_getcfam_addr0(const void *data)
> +{
> + QTestState *s = (QTestState *)data;
> + uint32_t curval;
> +
> + test_fsi_setup(s, AST2600_OPB_FSI1_BASE_ADDR);
> +
> + /* Master access direction read */
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB1_RW_DIRECTION, 0x1);
> +
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB1_XFER_SIZE, 0x3);
> + aspeed_fsi_writel(s, ASPEED_FSI_OPB1_BUS_ADDR, 0xa0000000);
> + aspeed_fsi_writel(s, ASPEED_FSI_INTRRUPT_CLEAR, 0x1);
> + aspeed_fsi_writel(s, ASPEED_FSI_ENGINER_TRIGGER, 0x1);
> +
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_INTRRUPT_STATUS);
> + g_assert_cmpuint(curval, ==, 0x20000);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_BUS_STATUS);
> + g_assert_cmpuint(curval, ==, 0x0);
> + curval = aspeed_fsi_readl(s, ASPEED_FSI_OPB1_READ_DATA);
> + g_assert_cmpuint(curval, ==, 0x152d02c0);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int ret = -1;
> + QTestState *s;
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + s = qtest_init("-machine ast2600-evb ");
> +
> + /* Tests for OPB/FSI0 */
> + qtest_add_data_func("/aspeed-fsi-test/test_fsi0_master_regs", s,
> + test_fsi0_master_regs);
> +
> + qtest_add_data_func("/aspeed-fsi-test/test_fsi0_getcfam_addr0", s,
> + test_fsi0_getcfam_addr0);
> +
> + /* Tests for OPB/FSI1 */
> + qtest_add_data_func("/aspeed-fsi-test/test_fsi1_master_regs", s,
> + test_fsi1_master_regs);
> +
> + qtest_add_data_func("/aspeed-fsi-test/test_fsi1_getcfam_addr0", s,
> + test_fsi1_getcfam_addr0);
> +
> + ret = g_test_run();
> + qtest_quit(s);
> +
> + return ret;
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index f25bffcc20..8c09159702 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -207,6 +207,7 @@ qtests_arm = \
> (config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
> (config_all_devices.has_key('CONFIG_VEXPRESS') ? ['test-arm-mptimer'] : []) + \
> (config_all_devices.has_key('CONFIG_MICROBIT') ? ['microbit-test'] : []) + \
> + (config_all_devices.has_key('CONFIG_FSI_APB2OPB_ASPEED') ? ['aspeed-fsi-test'] : []) + \
> ['arm-cpu-features',
> 'boot-serial-test']
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v10 7/9] hw/fsi: Added qtest
2024-01-12 16:02 ` Cédric Le Goater
@ 2024-01-16 19:37 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-16 19:37 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
Hi Cedric,
On 1/12/24 10:02, Cédric Le Goater wrote:
> On 1/11/24 00:15, Ninad Palsule wrote:
>> Added basic qtests for FSI model.
>>
>> Acked-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Please remove mu S-o-b.
Removed.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 8/9] hw/fsi: Added FSI documentation
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (6 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 7/9] hw/fsi: Added qtest Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 16:02 ` Cédric Le Goater
2024-01-10 23:15 ` [PATCH v10 9/9] hw/fsi: Update MAINTAINER list Ninad Palsule
2024-01-12 14:42 ` [PATCH v10 0/9] Introduce model for IBM's FSI Cédric Le Goater
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm
Documentation for IBM FSI model.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
docs/specs/fsi.rst | 138 +++++++++++++++++++++++++++++++++++++++++++
docs/specs/index.rst | 1 +
2 files changed, 139 insertions(+)
create mode 100644 docs/specs/fsi.rst
diff --git a/docs/specs/fsi.rst b/docs/specs/fsi.rst
new file mode 100644
index 0000000000..05a6b6347a
--- /dev/null
+++ b/docs/specs/fsi.rst
@@ -0,0 +1,138 @@
+======================================
+IBM's Flexible Service Interface (FSI)
+======================================
+
+The QEMU FSI emulation implements hardware interfaces between ASPEED SOC, FSI
+master/slave and the end engine.
+
+FSI is a point-to-point two wire interface which is capable of supporting
+distances of up to 4 meters. FSI interfaces have been used successfully for
+many years in IBM servers to attach IBM Flexible Support Processors(FSP) to
+CPUs and IBM ASICs.
+
+FSI allows a service processor access to the internal buses of a host POWER
+processor to perform configuration or debugging. FSI has long existed in POWER
+processes and so comes with some baggage, including how it has been integrated
+into the ASPEED SoC.
+
+Working backwards from the POWER processor, the fundamental pieces of interest
+for the implementation are: (see the `FSI specification`_ for more details)
+
+1. The Common FRU Access Macro (CFAM), an address space containing various
+ "engines" that drive accesses on buses internal and external to the POWER
+ chip. Examples include the SBEFIFO and I2C masters. The engines hang off of
+ an internal Local Bus (LBUS) which is described by the CFAM configuration
+ block.
+
+2. The FSI slave: The slave is the terminal point of the FSI bus for FSI
+ symbols addressed to it. Slaves can be cascaded off of one another. The
+ slave's configuration registers appear in address space of the CFAM to
+ which it is attached.
+
+3. The FSI master: A controller in the platform service processor (e.g. BMC)
+ driving CFAM engine accesses into the POWER chip. At the hardware level
+ FSI is a bit-based protocol supporting synchronous and DMA-driven accesses
+ of engines in a CFAM.
+
+4. 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.
+
+5. An APB-to-OPB bridge enabling access to the OPB from the ARM core in the
+ AST2600. Hardware limitations prevent the OPB from being directly mapped
+ into APB, so all accesses are indirect through the bridge.
+
+The LBUS is modelled to maintain the qdev bus hierarchy and to take advantages
+of the object model to automatically generate the CFAM configuration block.
+The configuration block presents engines in the order they are attached to the
+CFAM's LBUS. Engine implementations should subclass the LBusDevice and set the
+'config' member of LBusDeviceClass to match the engine's type.
+
+CFAM designs offer a lot of flexibility, for instance it is possible for a
+CFAM to be simultaneously driven from multiple FSI links. The modeling is not
+so complete; it's assumed that each CFAM is attached to a single FSI slave (as
+a consequence the CFAM subclasses the FSI slave).
+
+As for FSI, its symbols and wire-protocol are not modelled at all. This is not
+necessary to get FSI off the ground thanks to the mapping of the CFAM address
+space onto the OPB address space - the models follow this directly and map the
+CFAM memory region into the OPB's memory region.
+
+QEMU files related to FSI interface:
+ - ``hw/fsi/aspeed-apb2opb.c``
+ - ``include/hw/fsi/aspeed-apb2opb.h``
+ - ``hw/fsi/opb.c``
+ - ``include/hw/fsi/opb.h``
+ - ``hw/fsi/fsi.c``
+ - ``include/hw/fsi/fsi.h``
+ - ``hw/fsi/fsi-master.c``
+ - ``include/hw/fsi/fsi-master.h``
+ - ``hw/fsi/fsi-slave.c``
+ - ``include/hw/fsi/fsi-slave.h``
+ - ``hw/fsi/cfam.c``
+ - ``include/hw/fsi/cfam.h``
+ - ``hw/fsi/engine-scratchpad.c``
+ - ``include/hw/fsi/engine-scratchpad.h``
+ - ``include/hw/fsi/lbus.h``
+
+The following commands start the rainier machine with built-in FSI model.
+There are no model specific arguments.
+
+.. code-block:: console
+
+ qemu-system-arm -M rainier-bmc -nographic \
+ -kernel fitImage-linux.bin \
+ -dtb aspeed-bmc-ibm-rainier.dtb \
+ -initrd obmc-phosphor-initramfs.rootfs.cpio.xz \
+ -drive file=obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
+ -append "rootwait console=ttyS4,115200n8 root=PARTLABEL=rofs-a"
+
+The implementation appears as following in the qemu device tree:
+
+.. code-block:: console
+
+ (qemu) info qtree
+ bus: main-system-bus
+ type System
+ ...
+ dev: aspeed.apb2opb, id ""
+ gpio-out "sysbus-irq" 1
+ mmio 000000001e79b000/0000000000001000
+ bus: opb.1
+ type opb
+ dev: fsi.master, id ""
+ bus: fsi.bus.1
+ type fsi.bus
+ dev: cfam.config, id ""
+ dev: cfam, id ""
+ bus: lbus.1
+ type lbus
+ dev: scratchpad, id ""
+ address = 0 (0x0)
+ bus: opb.0
+ type opb
+ dev: fsi.master, id ""
+ bus: fsi.bus.0
+ type fsi.bus
+ dev: cfam.config, id ""
+ dev: cfam, id ""
+ bus: lbus.0
+ type lbus
+ dev: scratchpad, id ""
+ address = 0 (0x0)
+
+pdbg is a simple application to allow debugging of the host POWER processors
+from the BMC. (see the `pdbg source repository`_ for more details)
+
+.. code-block:: console
+
+ root@p10bmc:~# pdbg -a getcfam 0x0
+ p0: 0x0 = 0xc0022d15
+
+.. _FSI specification:
+ https://openpowerfoundation.org/specifications/fsi/
+
+.. _pdbg source repository:
+ https://github.com/open-power/pdbg
diff --git a/docs/specs/index.rst b/docs/specs/index.rst
index b3f482b0aa..1484e3e760 100644
--- a/docs/specs/index.rst
+++ b/docs/specs/index.rst
@@ -24,6 +24,7 @@ guest hardware that is specific to QEMU.
acpi_erst
sev-guest-firmware
fw_cfg
+ fsi
vmw_pvscsi-spec
edu
ivshmem-spec
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 8/9] hw/fsi: Added FSI documentation
2024-01-10 23:15 ` [PATCH v10 8/9] hw/fsi: Added FSI documentation Ninad Palsule
@ 2024-01-12 16:02 ` Cédric Le Goater
2024-01-16 19:33 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 16:02 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
On 1/11/24 00:15, Ninad Palsule wrote:
> Documentation for IBM FSI model.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Please remove mu S-o-b.
Thanks,
C.
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> docs/specs/fsi.rst | 138 +++++++++++++++++++++++++++++++++++++++++++
> docs/specs/index.rst | 1 +
> 2 files changed, 139 insertions(+)
> create mode 100644 docs/specs/fsi.rst
>
> diff --git a/docs/specs/fsi.rst b/docs/specs/fsi.rst
> new file mode 100644
> index 0000000000..05a6b6347a
> --- /dev/null
> +++ b/docs/specs/fsi.rst
> @@ -0,0 +1,138 @@
> +======================================
> +IBM's Flexible Service Interface (FSI)
> +======================================
> +
> +The QEMU FSI emulation implements hardware interfaces between ASPEED SOC, FSI
> +master/slave and the end engine.
> +
> +FSI is a point-to-point two wire interface which is capable of supporting
> +distances of up to 4 meters. FSI interfaces have been used successfully for
> +many years in IBM servers to attach IBM Flexible Support Processors(FSP) to
> +CPUs and IBM ASICs.
> +
> +FSI allows a service processor access to the internal buses of a host POWER
> +processor to perform configuration or debugging. FSI has long existed in POWER
> +processes and so comes with some baggage, including how it has been integrated
> +into the ASPEED SoC.
> +
> +Working backwards from the POWER processor, the fundamental pieces of interest
> +for the implementation are: (see the `FSI specification`_ for more details)
> +
> +1. The Common FRU Access Macro (CFAM), an address space containing various
> + "engines" that drive accesses on buses internal and external to the POWER
> + chip. Examples include the SBEFIFO and I2C masters. The engines hang off of
> + an internal Local Bus (LBUS) which is described by the CFAM configuration
> + block.
> +
> +2. The FSI slave: The slave is the terminal point of the FSI bus for FSI
> + symbols addressed to it. Slaves can be cascaded off of one another. The
> + slave's configuration registers appear in address space of the CFAM to
> + which it is attached.
> +
> +3. The FSI master: A controller in the platform service processor (e.g. BMC)
> + driving CFAM engine accesses into the POWER chip. At the hardware level
> + FSI is a bit-based protocol supporting synchronous and DMA-driven accesses
> + of engines in a CFAM.
> +
> +4. 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.
> +
> +5. An APB-to-OPB bridge enabling access to the OPB from the ARM core in the
> + AST2600. Hardware limitations prevent the OPB from being directly mapped
> + into APB, so all accesses are indirect through the bridge.
> +
> +The LBUS is modelled to maintain the qdev bus hierarchy and to take advantages
> +of the object model to automatically generate the CFAM configuration block.
> +The configuration block presents engines in the order they are attached to the
> +CFAM's LBUS. Engine implementations should subclass the LBusDevice and set the
> +'config' member of LBusDeviceClass to match the engine's type.
> +
> +CFAM designs offer a lot of flexibility, for instance it is possible for a
> +CFAM to be simultaneously driven from multiple FSI links. The modeling is not
> +so complete; it's assumed that each CFAM is attached to a single FSI slave (as
> +a consequence the CFAM subclasses the FSI slave).
> +
> +As for FSI, its symbols and wire-protocol are not modelled at all. This is not
> +necessary to get FSI off the ground thanks to the mapping of the CFAM address
> +space onto the OPB address space - the models follow this directly and map the
> +CFAM memory region into the OPB's memory region.
> +
> +QEMU files related to FSI interface:
> + - ``hw/fsi/aspeed-apb2opb.c``
> + - ``include/hw/fsi/aspeed-apb2opb.h``
> + - ``hw/fsi/opb.c``
> + - ``include/hw/fsi/opb.h``
> + - ``hw/fsi/fsi.c``
> + - ``include/hw/fsi/fsi.h``
> + - ``hw/fsi/fsi-master.c``
> + - ``include/hw/fsi/fsi-master.h``
> + - ``hw/fsi/fsi-slave.c``
> + - ``include/hw/fsi/fsi-slave.h``
> + - ``hw/fsi/cfam.c``
> + - ``include/hw/fsi/cfam.h``
> + - ``hw/fsi/engine-scratchpad.c``
> + - ``include/hw/fsi/engine-scratchpad.h``
> + - ``include/hw/fsi/lbus.h``
> +
> +The following commands start the rainier machine with built-in FSI model.
> +There are no model specific arguments.
> +
> +.. code-block:: console
> +
> + qemu-system-arm -M rainier-bmc -nographic \
> + -kernel fitImage-linux.bin \
> + -dtb aspeed-bmc-ibm-rainier.dtb \
> + -initrd obmc-phosphor-initramfs.rootfs.cpio.xz \
> + -drive file=obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
> + -append "rootwait console=ttyS4,115200n8 root=PARTLABEL=rofs-a"
> +
> +The implementation appears as following in the qemu device tree:
> +
> +.. code-block:: console
> +
> + (qemu) info qtree
> + bus: main-system-bus
> + type System
> + ...
> + dev: aspeed.apb2opb, id ""
> + gpio-out "sysbus-irq" 1
> + mmio 000000001e79b000/0000000000001000
> + bus: opb.1
> + type opb
> + dev: fsi.master, id ""
> + bus: fsi.bus.1
> + type fsi.bus
> + dev: cfam.config, id ""
> + dev: cfam, id ""
> + bus: lbus.1
> + type lbus
> + dev: scratchpad, id ""
> + address = 0 (0x0)
> + bus: opb.0
> + type opb
> + dev: fsi.master, id ""
> + bus: fsi.bus.0
> + type fsi.bus
> + dev: cfam.config, id ""
> + dev: cfam, id ""
> + bus: lbus.0
> + type lbus
> + dev: scratchpad, id ""
> + address = 0 (0x0)
> +
> +pdbg is a simple application to allow debugging of the host POWER processors
> +from the BMC. (see the `pdbg source repository`_ for more details)
> +
> +.. code-block:: console
> +
> + root@p10bmc:~# pdbg -a getcfam 0x0
> + p0: 0x0 = 0xc0022d15
> +
> +.. _FSI specification:
> + https://openpowerfoundation.org/specifications/fsi/
> +
> +.. _pdbg source repository:
> + https://github.com/open-power/pdbg
> diff --git a/docs/specs/index.rst b/docs/specs/index.rst
> index b3f482b0aa..1484e3e760 100644
> --- a/docs/specs/index.rst
> +++ b/docs/specs/index.rst
> @@ -24,6 +24,7 @@ guest hardware that is specific to QEMU.
> acpi_erst
> sev-guest-firmware
> fw_cfg
> + fsi
> vmw_pvscsi-spec
> edu
> ivshmem-spec
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v10 8/9] hw/fsi: Added FSI documentation
2024-01-12 16:02 ` Cédric Le Goater
@ 2024-01-16 19:33 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-16 19:33 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
Hello Cedric,
On 1/12/24 10:02, Cédric Le Goater wrote:
> On 1/11/24 00:15, Ninad Palsule wrote:
>> Documentation for IBM FSI model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Please remove mu S-o-b.
Removed.
Thanks for the review.
Regards,
Ninad
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v10 9/9] hw/fsi: Update MAINTAINER list
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (7 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 8/9] hw/fsi: Added FSI documentation Ninad Palsule
@ 2024-01-10 23:15 ` Ninad Palsule
2024-01-12 16:03 ` Cédric Le Goater
2024-01-12 14:42 ` [PATCH v10 0/9] Introduce model for IBM's FSI Cédric Le Goater
9 siblings, 1 reply; 27+ messages in thread
From: Ninad Palsule @ 2024-01-10 23:15 UTC (permalink / raw)
To: qemu-devel, clg, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: Ninad Palsule, qemu-arm
Added maintainer for IBM FSI model
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 00ec1f7eca..79f97a3fb9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3569,6 +3569,14 @@ F: tests/qtest/adm1272-test.c
F: tests/qtest/max34451-test.c
F: tests/qtest/isl_pmbus_vr-test.c
+FSI
+M: Ninad Palsule <ninad@linux.ibm.com>
+S: Maintained
+F: hw/fsi/*
+F: include/hw/fsi/*
+F: docs/specs/fsi.rst
+F: tests/qtest/fsi-test.c
+
Firmware schema specifications
M: Philippe Mathieu-Daudé <philmd@linaro.org>
R: Daniel P. Berrange <berrange@redhat.com>
--
2.39.2
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH v10 9/9] hw/fsi: Update MAINTAINER list
2024-01-10 23:15 ` [PATCH v10 9/9] hw/fsi: Update MAINTAINER list Ninad Palsule
@ 2024-01-12 16:03 ` Cédric Le Goater
2024-01-16 19:30 ` Ninad Palsule
0 siblings, 1 reply; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 16:03 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
On 1/11/24 00:15, Ninad Palsule wrote:
> Added maintainer for IBM FSI model
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
Please remove my S-o-b.
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> MAINTAINERS | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 00ec1f7eca..79f97a3fb9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3569,6 +3569,14 @@ F: tests/qtest/adm1272-test.c
> F: tests/qtest/max34451-test.c
> F: tests/qtest/isl_pmbus_vr-test.c
>
> +FSI
> +M: Ninad Palsule <ninad@linux.ibm.com>
and add me as Reviewer.
Thanks,
C.
> +S: Maintained
> +F: hw/fsi/*
> +F: include/hw/fsi/*
> +F: docs/specs/fsi.rst
> +F: tests/qtest/fsi-test.c
> +
> Firmware schema specifications
> M: Philippe Mathieu-Daudé <philmd@linaro.org>
> R: Daniel P. Berrange <berrange@redhat.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v10 9/9] hw/fsi: Update MAINTAINER list
2024-01-12 16:03 ` Cédric Le Goater
@ 2024-01-16 19:30 ` Ninad Palsule
0 siblings, 0 replies; 27+ messages in thread
From: Ninad Palsule @ 2024-01-16 19:30 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
Hello Cedric,
On 1/12/24 10:03, Cédric Le Goater wrote:
> On 1/11/24 00:15, Ninad Palsule wrote:
>> Added maintainer for IBM FSI model
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Please remove my S-o-b.
Removed.
>
>>
>> F: tests/qtest/isl_pmbus_vr-test.c
>> +FSI
>> +M: Ninad Palsule <ninad@linux.ibm.com>
>
> and add me as Reviewer.
Added you as a reviewer.
Thanks for the review.
Regards,
Ninad
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v10 0/9] Introduce model for IBM's FSI
2024-01-10 23:15 [PATCH v10 0/9] Introduce model for IBM's FSI Ninad Palsule
` (8 preceding siblings ...)
2024-01-10 23:15 ` [PATCH v10 9/9] hw/fsi: Update MAINTAINER list Ninad Palsule
@ 2024-01-12 14:42 ` Cédric Le Goater
9 siblings, 0 replies; 27+ messages in thread
From: Cédric Le Goater @ 2024-01-12 14:42 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
Hello Ninad,
This is looking much better. I have a couple more comments.
Please wait a little before sending a respin ! :) and please
run make check and script/checkpatch.pl.
On 1/11/24 00:15, Ninad Palsule wrote:
> Hello,
>
> Please review the patch-set version 10.
> I have incorporated review comments from Cedric.
> v10:
> - Moved aspeed-apb2opb to hw/misc directory
So, it looked better before ... can you please move it back to
the fsi dir ? Sorry about that.
Thanks,
C.
> - Moved scratchpad to lbus files.
> - Moved fsi-slave to fsi files.
> - Merged opb changes in the aspeed-apb2opb files
> - Reduced number of config option to 2
>
> Ninad Palsule (9):
> hw/fsi: Introduce IBM's Local bus and scratchpad
> hw/fsi: Introduce IBM's FSI Bus and FSI slave
> hw/fsi: Introduce IBM's cfam
> hw/fsi: Introduce IBM's FSI master
> hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
> hw/arm: Hook up FSI module in AST2600
> hw/fsi: Added qtest
> hw/fsi: Added FSI documentation
> hw/fsi: Update MAINTAINER list
>
> MAINTAINERS | 8 +
> docs/specs/fsi.rst | 138 +++++++++++++
> docs/specs/index.rst | 1 +
> meson.build | 1 +
> hw/fsi/trace.h | 1 +
> include/hw/arm/aspeed_soc.h | 4 +
> include/hw/fsi/cfam.h | 34 ++++
> include/hw/fsi/fsi-master.h | 32 +++
> include/hw/fsi/fsi.h | 38 ++++
> include/hw/fsi/lbus.h | 52 +++++
> include/hw/misc/aspeed-apb2opb.h | 50 +++++
> hw/arm/aspeed_ast2600.c | 19 ++
> hw/fsi/cfam.c | 182 +++++++++++++++++
> hw/fsi/fsi-master.c | 173 ++++++++++++++++
> hw/fsi/fsi.c | 111 ++++++++++
> hw/fsi/lbus.c | 121 +++++++++++
> hw/misc/aspeed-apb2opb.c | 338 +++++++++++++++++++++++++++++++
> tests/qtest/aspeed-fsi-test.c | 205 +++++++++++++++++++
> hw/Kconfig | 1 +
> hw/arm/Kconfig | 1 +
> hw/fsi/Kconfig | 2 +
> hw/fsi/meson.build | 1 +
> hw/fsi/trace-events | 11 +
> hw/meson.build | 1 +
> hw/misc/Kconfig | 5 +
> hw/misc/meson.build | 1 +
> hw/misc/trace-events | 4 +
> tests/qtest/meson.build | 1 +
> 28 files changed, 1536 insertions(+)
> create mode 100644 docs/specs/fsi.rst
> create mode 100644 hw/fsi/trace.h
> create mode 100644 include/hw/fsi/cfam.h
> create mode 100644 include/hw/fsi/fsi-master.h
> create mode 100644 include/hw/fsi/fsi.h
> create mode 100644 include/hw/fsi/lbus.h
> create mode 100644 include/hw/misc/aspeed-apb2opb.h
> create mode 100644 hw/fsi/cfam.c
> create mode 100644 hw/fsi/fsi-master.c
> create mode 100644 hw/fsi/fsi.c
> create mode 100644 hw/fsi/lbus.c
> create mode 100644 hw/misc/aspeed-apb2opb.c
> create mode 100644 tests/qtest/aspeed-fsi-test.c
> create mode 100644 hw/fsi/Kconfig
> create mode 100644 hw/fsi/meson.build
> create mode 100644 hw/fsi/trace-events
>
^ permalink raw reply [flat|nested] 27+ messages in thread