* [PATCH v6 01/10] hw/fsi: Introduce IBM's Local bus
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-23 14:58 ` Philippe Mathieu-Daudé
2023-10-21 21:17 ` [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad Ninad Palsule
` (8 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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 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.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v2:
- Incorporated Joel's review comments.
v5:
- Incorporated review comments by Cedric.
v6:
- Incorporated review comments by Cedric & Daniel.
---
include/hw/fsi/lbus.h | 43 ++++++++++++++++++++++++
hw/fsi/lbus.c | 76 +++++++++++++++++++++++++++++++++++++++++++
hw/Kconfig | 1 +
hw/fsi/Kconfig | 2 ++
hw/fsi/meson.build | 1 +
hw/meson.build | 1 +
6 files changed, 124 insertions(+)
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
diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h
new file mode 100644
index 0000000000..4fa696bbdb
--- /dev/null
+++ b/include/hw/fsi/lbus.h
@@ -0,0 +1,43 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Local bus and connected device structures.
+ */
+#ifndef FSI_LBUS_H
+#define FSI_LBUS_H
+
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
+
+#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
+OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
+
+#define FSI_LBUS_MEM_REGION_SIZE (2 * 1024 * 1024)
+#define FSI_LBUSDEV_IOMEM_SIZE 0x400
+
+typedef struct FSILBusDevice {
+ DeviceState parent;
+
+ MemoryRegion iomem;
+ uint32_t address;
+} 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;
+
+DeviceState *lbus_create_device(FSILBus *bus, const char *type, uint32_t addr);
+int lbus_add_device(FSILBus *bus, FSILBusDevice *dev);
+#endif /* FSI_LBUS_H */
diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c
new file mode 100644
index 0000000000..50d926dbe2
--- /dev/null
+++ b/hw/fsi/lbus.c
@@ -0,0 +1,76 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 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"
+
+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_SIZE);
+}
+
+static const TypeInfo lbus_info = {
+ .name = TYPE_FSI_LBUS,
+ .parent = TYPE_BUS,
+ .instance_init = lbus_init,
+ .instance_size = sizeof(FSILBus),
+};
+
+static Property lbus_device_props[] = {
+ DEFINE_PROP_UINT32("address", FSILBusDevice, address, 0),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+DeviceState *lbus_create_device(FSILBus *bus, const char *type, uint32_t addr)
+{
+ DeviceState *ds;
+ BusState *state = BUS(bus);
+ FSILBusDevice *dev;
+
+ ds = qdev_new(type);
+ qdev_prop_set_uint8(ds, "address", addr);
+ qdev_realize_and_unref(ds, state, &error_fatal);
+
+ dev = FSI_LBUS_DEVICE(ds);
+ memory_region_add_subregion(&bus->mr, dev->address,
+ &dev->iomem);
+
+ qdev_set_parent_bus(ds, state, &error_abort);
+
+ return ds;
+}
+
+static void lbus_device_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->bus_type = TYPE_FSI_LBUS;
+ device_class_set_props(dc, lbus_device_props);
+}
+
+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 void lbus_register_types(void)
+{
+ type_register_static(&lbus_info);
+ type_register_static(&lbus_device_type_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..e650c660f0
--- /dev/null
+++ b/hw/fsi/Kconfig
@@ -0,0 +1,2 @@
+config FSI_LBUS
+ bool
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
new file mode 100644
index 0000000000..4074d3a7d2
--- /dev/null
+++ b/hw/fsi/meson.build
@@ -0,0 +1 @@
+system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
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] 30+ messages in thread* Re: [PATCH v6 01/10] hw/fsi: Introduce IBM's Local bus
2023-10-21 21:17 ` [PATCH v6 01/10] hw/fsi: Introduce IBM's Local bus Ninad Palsule
@ 2023-10-23 14:58 ` Philippe Mathieu-Daudé
2023-10-23 17:10 ` Ninad Palsule
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-23 14:58 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, clg, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, lvivier
Cc: qemu-arm, Andrew Jeffery
Hi Ninad and Andrew,
On 21/10/23 23:17, Ninad Palsule wrote:
> This is a part of patchset where IBM's Flexible Service Interface is
> introduced.
>
> 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.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v2:
> - Incorporated Joel's review comments.
> v5:
> - Incorporated review comments by Cedric.
> v6:
> - Incorporated review comments by Cedric & Daniel.
> ---
> include/hw/fsi/lbus.h | 43 ++++++++++++++++++++++++
> hw/fsi/lbus.c | 76 +++++++++++++++++++++++++++++++++++++++++++
> hw/Kconfig | 1 +
> hw/fsi/Kconfig | 2 ++
> hw/fsi/meson.build | 1 +
> hw/meson.build | 1 +
> 6 files changed, 124 insertions(+)
> 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
> +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
> +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
> +
> +#define FSI_LBUS_MEM_REGION_SIZE (2 * 1024 * 1024)
> +#define FSI_LBUSDEV_IOMEM_SIZE 0x400
> +
> +typedef struct FSILBusDevice {
> + DeviceState parent;
> +
> + MemoryRegion iomem;
> + uint32_t address;
[1] 32-bit address,
> +} FSILBusDevice;
> +
> diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c
> new file mode 100644
> index 0000000000..50d926dbe2
> --- /dev/null
> +++ b/hw/fsi/lbus.c
> @@ -0,0 +1,76 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 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"
> +
> +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_SIZE);
> +}
> +
> +static const TypeInfo lbus_info = {
> + .name = TYPE_FSI_LBUS,
> + .parent = TYPE_BUS,
> + .instance_init = lbus_init,
> + .instance_size = sizeof(FSILBus),
> +};
> +
> +static Property lbus_device_props[] = {
> + DEFINE_PROP_UINT32("address", FSILBusDevice, address, 0),
[2] 32-bit address,
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +DeviceState *lbus_create_device(FSILBus *bus, const char *type, uint32_t addr)
> +{
> + DeviceState *ds;
> + BusState *state = BUS(bus);
> + FSILBusDevice *dev;
> +
> + ds = qdev_new(type);
> + qdev_prop_set_uint8(ds, "address", addr);
[3] set 8-bit address but [1] and [2] declare as 32-bit.
> + qdev_realize_and_unref(ds, state, &error_fatal);
If you pass the bus as argument here, ...
> +
> + dev = FSI_LBUS_DEVICE(ds);
> + memory_region_add_subregion(&bus->mr, dev->address,
> + &dev->iomem);
> +
> + qdev_set_parent_bus(ds, state, &error_abort);
... why do you need that call here?
> +
> + return ds;
> +}
Thanks,
Phil.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v6 01/10] hw/fsi: Introduce IBM's Local bus
2023-10-23 14:58 ` Philippe Mathieu-Daudé
@ 2023-10-23 17:10 ` Ninad Palsule
0 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-23 17:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, clg, peter.maydell,
andrew, joel, pbonzini, marcandre.lureau, berrange, thuth,
lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Philippe,
On 10/23/23 09:58, Philippe Mathieu-Daudé wrote:
> Hi Ninad and Andrew,
>
> On 21/10/23 23:17, Ninad Palsule wrote:
>> This is a part of patchset where IBM's Flexible Service Interface is
>> introduced.
>>
>> 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.
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v2:
>> - Incorporated Joel's review comments.
>> v5:
>> - Incorporated review comments by Cedric.
>> v6:
>> - Incorporated review comments by Cedric & Daniel.
>> ---
>> include/hw/fsi/lbus.h | 43 ++++++++++++++++++++++++
>> hw/fsi/lbus.c | 76 +++++++++++++++++++++++++++++++++++++++++++
>> hw/Kconfig | 1 +
>> hw/fsi/Kconfig | 2 ++
>> hw/fsi/meson.build | 1 +
>> hw/meson.build | 1 +
>> 6 files changed, 124 insertions(+)
>> 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
>
>
>> +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
>> +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
>> +
>> +#define FSI_LBUS_MEM_REGION_SIZE (2 * 1024 * 1024)
>> +#define FSI_LBUSDEV_IOMEM_SIZE 0x400
>> +
>> +typedef struct FSILBusDevice {
>> + DeviceState parent;
>> +
>> + MemoryRegion iomem;
>> + uint32_t address;
>
> [1] 32-bit address,
>
>> +} FSILBusDevice;
>> +
>
>
>> diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c
>> new file mode 100644
>> index 0000000000..50d926dbe2
>> --- /dev/null
>> +++ b/hw/fsi/lbus.c
>> @@ -0,0 +1,76 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 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"
>> +
>> +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_SIZE);
>> +}
>> +
>> +static const TypeInfo lbus_info = {
>> + .name = TYPE_FSI_LBUS,
>> + .parent = TYPE_BUS,
>> + .instance_init = lbus_init,
>> + .instance_size = sizeof(FSILBus),
>> +};
>> +
>> +static Property lbus_device_props[] = {
>> + DEFINE_PROP_UINT32("address", FSILBusDevice, address, 0),
>
> [2] 32-bit address,
>
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +DeviceState *lbus_create_device(FSILBus *bus, const char *type,
>> uint32_t addr)
>> +{
>> + DeviceState *ds;
>> + BusState *state = BUS(bus);
>> + FSILBusDevice *dev;
>> +
>> + ds = qdev_new(type);
>> + qdev_prop_set_uint8(ds, "address", addr);
>
> [3] set 8-bit address but [1] and [2] declare as 32-bit.
Good catch. Fixed it.
>
>> + qdev_realize_and_unref(ds, state, &error_fatal);
>
> If you pass the bus as argument here, ...
>
>> +
>> + dev = FSI_LBUS_DEVICE(ds);
>> + memory_region_add_subregion(&bus->mr, dev->address,
>> + &dev->iomem);
>> +
>> + qdev_set_parent_bus(ds, state, &error_abort);
>
> ... why do you need that call here?
You are right. qdev_realize() call qdev_set_parent_bus() so I removed
this call. I will publish the change in version 7.
Thanks for the review.
Regards,
Ninad
>
>> +
>> + return ds;
>> +}
>
> Thanks,
>
> Phil.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 01/10] hw/fsi: Introduce IBM's Local bus Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-23 15:00 ` Philippe Mathieu-Daudé
2023-10-21 21:17 ` [PATCH v6 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave Ninad Palsule
` (7 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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 scratchpad is introduced.
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.
In this model, The LBUS device is parent for the scratchpad.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v2:
- Incorporated Joel's review comments.
v5:
- Incorporated review comments by Cedric.
v6:
- Incorporated review comments by Daniel.
---
meson.build | 1 +
hw/fsi/trace.h | 1 +
include/hw/fsi/engine-scratchpad.h | 32 ++++++++++
include/hw/fsi/fsi.h | 16 +++++
hw/fsi/engine-scratchpad.c | 93 ++++++++++++++++++++++++++++++
hw/fsi/Kconfig | 4 ++
hw/fsi/meson.build | 1 +
hw/fsi/trace-events | 2 +
8 files changed, 150 insertions(+)
create mode 100644 hw/fsi/trace.h
create mode 100644 include/hw/fsi/engine-scratchpad.h
create mode 100644 include/hw/fsi/fsi.h
create mode 100644 hw/fsi/engine-scratchpad.c
create mode 100644 hw/fsi/trace-events
diff --git a/meson.build b/meson.build
index 4961c82a6b..31078f0bb3 100644
--- a/meson.build
+++ b/meson.build
@@ -3256,6 +3256,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/engine-scratchpad.h b/include/hw/fsi/engine-scratchpad.h
new file mode 100644
index 0000000000..959a8a6677
--- /dev/null
+++ b/include/hw/fsi/engine-scratchpad.h
@@ -0,0 +1,32 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM scratchpad engne
+ */
+#ifndef FSI_ENGINE_SCRATCHPAD_H
+#define FSI_ENGINE_SCRATCHPAD_H
+
+#include "hw/fsi/lbus.h"
+#include "hw/fsi/fsi.h"
+
+#define ENGINE_CONFIG_NEXT BE_BIT(0)
+#define ENGINE_CONFIG_VPD BE_BIT(1)
+#define ENGINE_CONFIG_SLOTS BE_GENMASK(8, 15)
+#define ENGINE_CONFIG_VERSION BE_GENMASK(16, 19)
+#define ENGINE_CONFIG_TYPE BE_GENMASK(20, 27)
+#define ENGINE_CONFIG_TYPE_PEEK (0x02 << 4)
+#define ENGINE_CONFIG_TYPE_FSI (0x03 << 4)
+#define ENGINE_CONFIG_TYPE_SCRATCHPAD (0x06 << 4)
+#define ENGINE_CONFIG_CRC BE_GENMASK(28, 31)
+
+#define TYPE_FSI_SCRATCHPAD "fsi.scratchpad"
+#define SCRATCHPAD(obj) OBJECT_CHECK(FSIScratchPad, (obj), TYPE_FSI_SCRATCHPAD)
+
+typedef struct FSIScratchPad {
+ FSILBusDevice parent;
+
+ uint32_t reg;
+} FSIScratchPad;
+
+#endif /* FSI_ENGINE_SCRATCHPAD_H */
diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
new file mode 100644
index 0000000000..e65f26f17b
--- /dev/null
+++ b/include/hw/fsi/fsi.h
@@ -0,0 +1,16 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Flexible Service Interface
+ */
+#ifndef FSI_FSI_H
+#define FSI_FSI_H
+
+/* Bitwise operations at the word level. */
+#define BE_BIT(x) BIT(31 - (x))
+#define GENMASK(t, b) \
+ (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
+#define BE_GENMASK(t, b) GENMASK(BE_BIT(t), BE_BIT(b))
+
+#endif
diff --git a/hw/fsi/engine-scratchpad.c b/hw/fsi/engine-scratchpad.c
new file mode 100644
index 0000000000..a8887cd613
--- /dev/null
+++ b/hw/fsi/engine-scratchpad.c
@@ -0,0 +1,93 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM scratchpad engine
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/engine-scratchpad.h"
+
+static uint64_t fsi_scratchpad_read(void *opaque, hwaddr addr, unsigned size)
+{
+ FSIScratchPad *s = SCRATCHPAD(opaque);
+
+ trace_fsi_scratchpad_read(addr, size);
+
+ if (addr) {
+ return 0;
+ }
+
+ return s->reg;
+}
+
+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) {
+ return;
+ }
+
+ s->reg = 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);
+
+ s->reg = 0;
+}
+
+static void fsi_scratchpad_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ FSILBusDeviceClass *ldc = FSI_LBUS_DEVICE_CLASS(klass);
+
+ dc->realize = fsi_scratchpad_realize;
+ dc->reset = fsi_scratchpad_reset;
+
+ ldc->config =
+ ENGINE_CONFIG_NEXT | /* valid */
+ 0x00010000 | /* slots */
+ 0x00001000 | /* version */
+ ENGINE_CONFIG_TYPE_SCRATCHPAD | /* type */
+ 0x00000007; /* crc */
+}
+
+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 fsi_scratchpad_register_types(void)
+{
+ type_register_static(&fsi_scratchpad_info);
+}
+
+type_init(fsi_scratchpad_register_types);
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index e650c660f0..f7c7fd1b28 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,2 +1,6 @@
+config FSI_SCRATCHPAD
+ bool
+ select FSI_LBUS
+
config FSI_LBUS
bool
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index 4074d3a7d2..d45a98c223 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -1 +1,2 @@
system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
+system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c'))
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
--
2.39.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad
2023-10-21 21:17 ` [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad Ninad Palsule
@ 2023-10-23 15:00 ` Philippe Mathieu-Daudé
2023-10-23 17:08 ` Ninad Palsule
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-23 15:00 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, clg, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, lvivier
Cc: qemu-arm, Andrew Jeffery
On 21/10/23 23:17, Ninad Palsule wrote:
> This is a part of patchset where scratchpad is introduced.
>
> 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.
>
> In this model, The LBUS device is parent for the scratchpad.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v2:
> - Incorporated Joel's review comments.
> v5:
> - Incorporated review comments by Cedric.
> v6:
> - Incorporated review comments by Daniel.
> ---
> meson.build | 1 +
> hw/fsi/trace.h | 1 +
> include/hw/fsi/engine-scratchpad.h | 32 ++++++++++
> include/hw/fsi/fsi.h | 16 +++++
> hw/fsi/engine-scratchpad.c | 93 ++++++++++++++++++++++++++++++
> hw/fsi/Kconfig | 4 ++
> hw/fsi/meson.build | 1 +
> hw/fsi/trace-events | 2 +
> 8 files changed, 150 insertions(+)
> create mode 100644 hw/fsi/trace.h
> create mode 100644 include/hw/fsi/engine-scratchpad.h
> create mode 100644 include/hw/fsi/fsi.h
> create mode 100644 hw/fsi/engine-scratchpad.c
> create mode 100644 hw/fsi/trace-events
> diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
> new file mode 100644
> index 0000000000..e65f26f17b
> --- /dev/null
> +++ b/include/hw/fsi/fsi.h
> @@ -0,0 +1,16 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * IBM Flexible Service Interface
> + */
> +#ifndef FSI_FSI_H
> +#define FSI_FSI_H
> +
> +/* Bitwise operations at the word level. */
> +#define BE_BIT(x) BIT(31 - (x))
> +#define GENMASK(t, b) \
> + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
Please use MAKE_64BIT_MASK() from "qemu/bitops.h".
> +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t), BE_BIT(b))
> +
> +#endif
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad
2023-10-23 15:00 ` Philippe Mathieu-Daudé
@ 2023-10-23 17:08 ` Ninad Palsule
2023-10-24 7:08 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-23 17:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, clg, peter.maydell,
andrew, joel, pbonzini, marcandre.lureau, berrange, thuth,
lvivier
Cc: qemu-arm, Andrew Jeffery
Hello Philippe,
On 10/23/23 10:00, Philippe Mathieu-Daudé wrote:
> On 21/10/23 23:17, Ninad Palsule wrote:
>> This is a part of patchset where scratchpad is introduced.
>>
>> 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.
>>
>> In this model, The LBUS device is parent for the scratchpad.
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v2:
>> - Incorporated Joel's review comments.
>> v5:
>> - Incorporated review comments by Cedric.
>> v6:
>> - Incorporated review comments by Daniel.
>> ---
>> meson.build | 1 +
>> hw/fsi/trace.h | 1 +
>> include/hw/fsi/engine-scratchpad.h | 32 ++++++++++
>> include/hw/fsi/fsi.h | 16 +++++
>> hw/fsi/engine-scratchpad.c | 93 ++++++++++++++++++++++++++++++
>> hw/fsi/Kconfig | 4 ++
>> hw/fsi/meson.build | 1 +
>> hw/fsi/trace-events | 2 +
>> 8 files changed, 150 insertions(+)
>> create mode 100644 hw/fsi/trace.h
>> create mode 100644 include/hw/fsi/engine-scratchpad.h
>> create mode 100644 include/hw/fsi/fsi.h
>> create mode 100644 hw/fsi/engine-scratchpad.c
>> create mode 100644 hw/fsi/trace-events
>
>
>> diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
>> new file mode 100644
>> index 0000000000..e65f26f17b
>> --- /dev/null
>> +++ b/include/hw/fsi/fsi.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 IBM Corp.
>> + *
>> + * IBM Flexible Service Interface
>> + */
>> +#ifndef FSI_FSI_H
>> +#define FSI_FSI_H
>> +
>> +/* Bitwise operations at the word level. */
>> +#define BE_BIT(x) BIT(31 - (x))
>> +#define GENMASK(t, b) \
>> + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
>
> Please use MAKE_64BIT_MASK() from "qemu/bitops.h".
The GENMASK and MAKE_64BIT_MASK macros are invoke differently.
GENMASK is invoked with bit t and bit b (t:b) and it provides the mask and
MAKE_64BIT_MASK uses shift and length.
Thanks for the review.
Regards,
Ninad
>> +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t),
>> BE_BIT(b))
>> +
>> +#endif
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad
2023-10-23 17:08 ` Ninad Palsule
@ 2023-10-24 7:08 ` Philippe Mathieu-Daudé
2023-10-26 15:24 ` Ninad Palsule
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-24 7:08 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, clg, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, lvivier
Cc: qemu-arm, Andrew Jeffery
On 23/10/23 19:08, Ninad Palsule wrote:
> Hello Philippe,
>
> On 10/23/23 10:00, Philippe Mathieu-Daudé wrote:
>> On 21/10/23 23:17, Ninad Palsule wrote:
>>> This is a part of patchset where scratchpad is introduced.
>>>
>>> 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.
>>>
>>> In this model, The LBUS device is parent for the scratchpad.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>> ---
>>> v2:
>>> - Incorporated Joel's review comments.
>>> v5:
>>> - Incorporated review comments by Cedric.
>>> v6:
>>> - Incorporated review comments by Daniel.
>>> ---
>>> meson.build | 1 +
>>> hw/fsi/trace.h | 1 +
>>> include/hw/fsi/engine-scratchpad.h | 32 ++++++++++
>>> include/hw/fsi/fsi.h | 16 +++++
>>> hw/fsi/engine-scratchpad.c | 93 ++++++++++++++++++++++++++++++
>>> hw/fsi/Kconfig | 4 ++
>>> hw/fsi/meson.build | 1 +
>>> hw/fsi/trace-events | 2 +
>>> 8 files changed, 150 insertions(+)
>>> create mode 100644 hw/fsi/trace.h
>>> create mode 100644 include/hw/fsi/engine-scratchpad.h
>>> create mode 100644 include/hw/fsi/fsi.h
>>> create mode 100644 hw/fsi/engine-scratchpad.c
>>> create mode 100644 hw/fsi/trace-events
>>
>>
>>> diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
>>> new file mode 100644
>>> index 0000000000..e65f26f17b
>>> --- /dev/null
>>> +++ b/include/hw/fsi/fsi.h
>>> @@ -0,0 +1,16 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + * Copyright (C) 2023 IBM Corp.
>>> + *
>>> + * IBM Flexible Service Interface
>>> + */
>>> +#ifndef FSI_FSI_H
>>> +#define FSI_FSI_H
>>> +
>>> +/* Bitwise operations at the word level. */
>>> +#define BE_BIT(x) BIT(31 - (x))
>>> +#define GENMASK(t, b) \
>>> + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
>>
>> Please use MAKE_64BIT_MASK() from "qemu/bitops.h".
>
> The GENMASK and MAKE_64BIT_MASK macros are invoke differently.
>
> GENMASK is invoked with bit t and bit b (t:b) and it provides the mask and
>
> MAKE_64BIT_MASK uses shift and length.
Don't we have:
#define GENMASK(t, b) MAKE_64BIT_MASK(t, b - t + 1)
?
> Thanks for the review.
>
> Regards,
>
> Ninad
>
>
>>> +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t),
>>> BE_BIT(b))
>>> +
>>> +#endif
>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad
2023-10-24 7:08 ` Philippe Mathieu-Daudé
@ 2023-10-26 15:24 ` Ninad Palsule
0 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-26 15:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel, clg, peter.maydell,
andrew, joel, pbonzini, marcandre.lureau, berrange, thuth,
lvivier
Cc: qemu-arm, Andrew Jeffery
On 10/24/23 02:08, Philippe Mathieu-Daudé wrote:
> On 23/10/23 19:08, Ninad Palsule wrote:
>> Hello Philippe,
>>
>> On 10/23/23 10:00, Philippe Mathieu-Daudé wrote:
>>> On 21/10/23 23:17, Ninad Palsule wrote:
>>>> This is a part of patchset where scratchpad is introduced.
>>>>
>>>> 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.
>>>>
>>>> In this model, The LBUS device is parent for the scratchpad.
>>>>
>>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>> ---
>>>> v2:
>>>> - Incorporated Joel's review comments.
>>>> v5:
>>>> - Incorporated review comments by Cedric.
>>>> v6:
>>>> - Incorporated review comments by Daniel.
>>>> ---
>>>> meson.build | 1 +
>>>> hw/fsi/trace.h | 1 +
>>>> include/hw/fsi/engine-scratchpad.h | 32 ++++++++++
>>>> include/hw/fsi/fsi.h | 16 +++++
>>>> hw/fsi/engine-scratchpad.c | 93
>>>> ++++++++++++++++++++++++++++++
>>>> hw/fsi/Kconfig | 4 ++
>>>> hw/fsi/meson.build | 1 +
>>>> hw/fsi/trace-events | 2 +
>>>> 8 files changed, 150 insertions(+)
>>>> create mode 100644 hw/fsi/trace.h
>>>> create mode 100644 include/hw/fsi/engine-scratchpad.h
>>>> create mode 100644 include/hw/fsi/fsi.h
>>>> create mode 100644 hw/fsi/engine-scratchpad.c
>>>> create mode 100644 hw/fsi/trace-events
>>>
>>>
>>>> diff --git a/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
>>>> new file mode 100644
>>>> index 0000000000..e65f26f17b
>>>> --- /dev/null
>>>> +++ b/include/hw/fsi/fsi.h
>>>> @@ -0,0 +1,16 @@
>>>> +/*
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + * Copyright (C) 2023 IBM Corp.
>>>> + *
>>>> + * IBM Flexible Service Interface
>>>> + */
>>>> +#ifndef FSI_FSI_H
>>>> +#define FSI_FSI_H
>>>> +
>>>> +/* Bitwise operations at the word level. */
>>>> +#define BE_BIT(x) BIT(31 - (x))
>>>> +#define GENMASK(t, b) \
>>>> + (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
>>>
>>> Please use MAKE_64BIT_MASK() from "qemu/bitops.h".
>>
>> The GENMASK and MAKE_64BIT_MASK macros are invoke differently.
>>
>> GENMASK is invoked with bit t and bit b (t:b) and it provides the
>> mask and
>>
>> MAKE_64BIT_MASK uses shift and length.
>
> Don't we have:
>
> #define GENMASK(t, b) MAKE_64BIT_MASK(t, b - t + 1)
>
> ?
You are right. I am able to use this macro. I have removed some unused
macros.
Thanks for the review.
Regards,
Ninad
>
>> Thanks for the review.
>>
>> Regards,
>>
>> Ninad
>>
>>
>>>> +#define BE_GENMASK(t, b) GENMASK(BE_BIT(t),
>>>> BE_BIT(b))
>>>> +
>>>> +#endif
>>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 01/10] hw/fsi: Introduce IBM's Local bus Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 02/10] hw/fsi: Introduce IBM's scratchpad Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 04/10] hw/fsi: Introduce IBM's FSI Ninad Palsule
` (6 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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.
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.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v2:
- Incorporated Joel's review comments.
v3:
- Incorporated Thomas Huth's review comments.
v5:
- Incorporated review comments by Cedric.
v6:
- Incorporated review comments by Cedric & Daniel
---
include/hw/fsi/cfam.h | 34 ++++++++
include/hw/fsi/fsi-slave.h | 29 +++++++
include/hw/fsi/fsi.h | 21 +++++
hw/fsi/cfam.c | 173 +++++++++++++++++++++++++++++++++++++
hw/fsi/fsi-slave.c | 96 ++++++++++++++++++++
hw/fsi/Kconfig | 9 ++
hw/fsi/meson.build | 2 +
hw/fsi/trace-events | 7 ++
8 files changed, 371 insertions(+)
create mode 100644 include/hw/fsi/cfam.h
create mode 100644 include/hw/fsi/fsi-slave.h
create mode 100644 hw/fsi/cfam.c
create mode 100644 hw/fsi/fsi-slave.c
diff --git a/include/hw/fsi/cfam.h b/include/hw/fsi/cfam.h
new file mode 100644
index 0000000000..842a3bad0c
--- /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-slave.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;
+ AddressSpace as;
+
+ FSILBus lbus;
+} FSICFAMState;
+
+#endif /* FSI_CFAM_H */
diff --git a/include/hw/fsi/fsi-slave.h b/include/hw/fsi/fsi-slave.h
new file mode 100644
index 0000000000..f5f23f4457
--- /dev/null
+++ b/include/hw/fsi/fsi-slave.h
@@ -0,0 +1,29 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Flexible Service Interface slave
+ */
+#ifndef FSI_FSI_SLAVE_H
+#define FSI_FSI_SLAVE_H
+
+#include "exec/memory.h"
+#include "hw/qdev-core.h"
+
+#include "hw/fsi/lbus.h"
+
+#include <stdint.h>
+
+#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/include/hw/fsi/fsi.h b/include/hw/fsi/fsi.h
index e65f26f17b..14bc19279f 100644
--- a/include/hw/fsi/fsi.h
+++ b/include/hw/fsi/fsi.h
@@ -7,10 +7,31 @@
#ifndef FSI_FSI_H
#define FSI_FSI_H
+#include "hw/qdev-core.h"
+
+/*
+ * TODO: Maybe unwind this dependency with const links? Store a
+ * pointer in FSIBus?
+ */
+#include "hw/fsi/cfam.h"
+
/* Bitwise operations at the word level. */
#define BE_BIT(x) BIT(31 - (x))
#define GENMASK(t, b) \
(((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
#define BE_GENMASK(t, b) GENMASK(BE_BIT(t), BE_BIT(b))
+#define TYPE_FSI_BUS "fsi.bus"
+OBJECT_DECLARE_SIMPLE_TYPE(FSIBus, FSI_BUS)
+
+/* TODO: Figure out what's best with a point-to-point bus */
+typedef struct FSISlaveState FSISlaveState;
+
+typedef struct FSIBus {
+ BusState bus;
+
+ /* XXX: It's point-to-point, just instantiate the slave directly for now */
+ FSICFAMState slave;
+} FSIBus;
+
#endif
diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
new file mode 100644
index 0000000000..a1c037925f
--- /dev/null
+++ b/hw/fsi/cfam.c
@@ -0,0 +1,173 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Common FRU Access Macro
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/fsi/cfam.h"
+#include "hw/fsi/fsi.h"
+#include "hw/fsi/engine-scratchpad.h"
+
+#include "hw/qdev-properties.h"
+
+#define TO_REG(x) ((x) >> 2)
+
+#define CFAM_ENGINE_CONFIG TO_REG(0x04)
+
+#define CFAM_CONFIG_CHIP_ID TO_REG(0x00)
+#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)
+{
+ FSICFAMState *cfam = FSI_CFAM(opaque);
+ BusChild *kid;
+ int i;
+
+ trace_fsi_cfam_config_read(addr, size);
+
+ switch (addr) {
+ case 0x00:
+ return CFAM_CONFIG_CHIP_ID_P9;
+ case 0x04:
+ return ENGINE_CONFIG_NEXT | /* valid */
+ 0x00010000 | /* slots */
+ 0x00001000 | /* version */
+ ENGINE_CONFIG_TYPE_PEEK | /* type */
+ 0x0000000c; /* crc */
+ case 0x08:
+ return ENGINE_CONFIG_NEXT | /* valid */
+ 0x00010000 | /* slots */
+ 0x00005000 | /* version */
+ ENGINE_CONFIG_TYPE_FSI | /* type */
+ 0x0000000a; /* crc */
+ break;
+ default:
+ /* The config table contains different engines from 0xc onwards. */
+ i = 0xc;
+ QTAILQ_FOREACH(kid, &cfam->lbus.bus.children, sibling) {
+ if (i == addr) {
+ DeviceState *ds = kid->child;
+ FSILBusDevice *dev = FSI_LBUS_DEVICE(ds);
+ return FSI_LBUS_DEVICE_GET_CLASS(dev)->config;
+ }
+ i += size;
+ }
+
+ if (i == addr) {
+ return 0;
+ }
+
+ /*
+ * As per FSI specification, This is a magic value at address 0 of
+ * given FSI port. This causes FSI master to send BREAK command for
+ * initialization and recovery.
+ */
+ return CFAM_CONFIG_CHIP_ID_BREAK;
+ }
+}
+
+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_CHIP_ID + 4:
+ 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_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 * 1024 * 1024);
+ address_space_init(&cfam->as, &cfam->mr, TYPE_FSI_CFAM);
+
+ 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, 0x400);
+
+ if (!object_property_set_bool(OBJECT(&cfam->lbus), "realized", true,
+ errp)) {
+ return;
+ }
+
+ 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 */
+ lbus_create_device(&cfam->lbus, TYPE_FSI_SCRATCHPAD, 0);
+}
+
+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_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/fsi-slave.c b/hw/fsi/fsi-slave.c
new file mode 100644
index 0000000000..d81842209e
--- /dev/null
+++ b/hw/fsi/fsi-slave.c
@@ -0,0 +1,96 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Flexible Service Interface slave
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/fsi-slave.h"
+#include "hw/fsi/fsi.h"
+
+#define TO_REG(x) ((x) >> 2)
+
+#define FSI_SMODE TO_REG(0x00)
+#define FSI_SMODE_WSTART BE_BIT(0)
+#define FSI_SMODE_AUX_EN BE_BIT(1)
+#define FSI_SMODE_SLAVE_ID BE_GENMASK(6, 7)
+#define FSI_SMODE_ECHO_DELAY BE_GENMASK(8, 11)
+#define FSI_SMODE_SEND_DELAY BE_GENMASK(12, 15)
+#define FSI_SMODE_LBUS_DIV BE_GENMASK(20, 23)
+#define FSI_SMODE_BRIEF_LEFT BE_GENMASK(24, 27)
+#define FSI_SMODE_BRIEF_RIGHT BE_GENMASK(28, 31)
+
+#define FSI_SDMA TO_REG(0x04)
+#define FSI_SISC TO_REG(0x08)
+#define FSI_SCISC TO_REG(0x08)
+#define FSI_SISM TO_REG(0x0c)
+#define FSI_SISS TO_REG(0x10)
+#define FSI_SSISM TO_REG(0x10)
+#define FSI_SCISM TO_REG(0x14)
+
+static uint64_t fsi_slave_read(void *opaque, hwaddr addr, unsigned size)
+{
+ FSISlaveState *s = FSI_SLAVE(opaque);
+
+ trace_fsi_slave_read(addr, size);
+
+ if (addr + size > sizeof(s->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[TO_REG(addr)];
+}
+
+static void fsi_slave_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ FSISlaveState *s = FSI_SLAVE(opaque);
+
+ trace_fsi_slave_write(addr, size, data);
+
+ if (addr + size > sizeof(s->regs)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds write: 0x%"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ s->regs[TO_REG(addr)] = data;
+}
+
+static const struct MemoryRegionOps fsi_slave_ops = {
+ .read = fsi_slave_read,
+ .write = fsi_slave_write,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+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 const TypeInfo fsi_slave_info = {
+ .name = TYPE_FSI_SLAVE,
+ .parent = TYPE_DEVICE,
+ .instance_init = fsi_slave_init,
+ .instance_size = sizeof(FSISlaveState),
+};
+
+static void fsi_slave_register_types(void)
+{
+ type_register_static(&fsi_slave_info);
+}
+
+type_init(fsi_slave_register_types);
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index f7c7fd1b28..8d712e77ed 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,3 +1,12 @@
+config FSI_CFAM
+ bool
+ select FSI
+ select FSI_SCRATCHPAD
+ select FSI_LBUS
+
+config FSI
+ bool
+
config FSI_SCRATCHPAD
bool
select FSI_LBUS
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index d45a98c223..a9e7cd4099 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -1,2 +1,4 @@
system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c'))
+system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
+system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi-slave.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index c5753e2791..b57b2dcc86 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -1,2 +1,9 @@
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_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
+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] 30+ messages in thread* [PATCH v6 04/10] hw/fsi: Introduce IBM's FSI
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (2 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 05/10] hw/fsi: IBM's On-chip Peripheral Bus Ninad Palsule
` (5 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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 bus. CFAM is hanging out of FSI bus. The bus
is model such a way that it is embedded inside the 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.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
v2:
- Incorporated review comments by Joel
v5:
- Incorporated review comments by Cedric.
v6:
- Incorporated review comments by Cedric & Daniel
---
include/hw/fsi/fsi-master.h | 30 ++++++
hw/fsi/fsi-master.c | 202 ++++++++++++++++++++++++++++++++++++
hw/fsi/fsi.c | 25 +++++
hw/fsi/meson.build | 2 +-
hw/fsi/trace-events | 2 +
5 files changed, 260 insertions(+), 1 deletion(-)
create mode 100644 include/hw/fsi/fsi-master.h
create mode 100644 hw/fsi/fsi-master.c
create mode 100644 hw/fsi/fsi.c
diff --git a/include/hw/fsi/fsi-master.h b/include/hw/fsi/fsi-master.h
new file mode 100644
index 0000000000..847078919c
--- /dev/null
+++ b/include/hw/fsi/fsi-master.h
@@ -0,0 +1,30 @@
+/*
+ * 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"
+
+#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];
+} 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..fa23af3de2
--- /dev/null
+++ b/hw/fsi/fsi-master.c
@@ -0,0 +1,202 @@
+/*
+ * 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_MMODE TO_REG(0x000)
+#define FSI_MMODE_IPOLL_DMA_EN BE_BIT(0)
+#define FSI_MMODE_HW_ERROR_RECOVERY_EN BE_BIT(1)
+#define FSI_MMODE_RELATIVE_ADDRESS_EN BE_BIT(2)
+#define FSI_MMODE_PARITY_CHECK_EN BE_BIT(3)
+#define FSI_MMODE_CLOCK_DIVIDER_0 BE_GENMASK(4, 13)
+#define FSI_MMODE_CLOCK_DIVIDER_1 BE_GENMASK(14, 23)
+#define FSI_MMODE_DEBUG_EN BE_BIT(24)
+
+#define FSI_MDELAY TO_REG(0x004)
+#define FSI_MDELAY_ECHO_0 BE_GENMASK(0, 3)
+#define FSI_MDELAY_SEND_0 BE_GENMASK(4, 7)
+#define FSI_MDELAY_ECHO_1 BE_GENMASK(8, 11)
+#define FSI_MDELAY_SEND_1 BE_GENMASK(12, 15)
+
+#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_MAEB TO_REG(0x070)
+#define FSI_MAEB_ANY_CPU_ERROR BE_BIT(0)
+#define FSI_MAEB_ANY_DMA_ERROR BE_GENMASK(1, 16)
+#define FSI_MAEB_ANY_PARITY_ERROR BE_BIT(17)
+
+#define FSI_MVER TO_REG(0x074)
+#define FSI_MVER_VERSION BE_GENMASK(0, 7)
+#define FSI_MVER_BRIDGES BE_GENMASK(8, 15)
+#define FSI_MVER_PORTS BE_GENMASK(16, 23)
+
+#define FSI_MRESP0 TO_REG(0x0d0)
+#define FSI_MRESP0_RESET_PORT_GENERAL BE_BIT(0)
+#define FSI_MRESP0_RESET_PORT_ERROR BE_BIT(1)
+#define FSI_MRESP0_RESET_ALL_BRIDGES_GENERAL BE_BIT(2)
+#define FSI_MRESP0_RESET_ALL_PORTS_GENERAL BE_BIT(3)
+#define FSI_MRESP0_RESET_MASTER BE_BIT(4)
+#define FSI_MRESP0_RESET_PARITY_ERROR_LATCH BE_BIT(5)
+
+#define FSI_MRESB0 TO_REG(0x1d0)
+#define FSI_MRESB0_RESET_GENERAL BE_BIT(0)
+#define FSI_MRESB0_RESET_ERROR BE_BIT(1)
+#define FSI_MRESB0_SET_DMA_SUSPEND BE_BIT(5)
+#define FSI_MRESB0_CLEAR_DMA_SUSPEND BE_BIT(6)
+#define FSI_MRESB0_SET_DELAY_MEASURE BE_BIT(7)
+
+#define FSI_MECTRL TO_REG(0x2e0)
+#define FSI_MECTRL_TEST_PULSE BE_GENMASK(0, 7)
+#define FSI_MECTRL_INHIBIT_PARITY_ERROR BE_GENMASK(8, 15)
+#define FSI_MECTRL_ENABLE_OPB_ERR_ACK BE_BIT(16)
+#define FSI_MECTRL_AUTO_TERMINATE BE_BIT(17)
+#define FSI_MECTRL_PORT_ERROR_FREEZE BE_BIT(18)
+
+static uint64_t fsi_master_read(void *opaque, hwaddr addr, unsigned size)
+{
+ FSIMasterState *s = FSI_MASTER(opaque);
+
+ trace_fsi_master_read(addr, size);
+
+ if (addr + size > sizeof(s->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[TO_REG(addr)];
+}
+
+static void fsi_master_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ FSIMasterState *s = FSI_MASTER(opaque);
+
+ trace_fsi_master_write(addr, size, data);
+
+ if (addr + size > sizeof(s->regs)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ switch (TO_REG(addr)) {
+ 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[TO_REG(addr)] = data;
+ }
+}
+
+static const struct MemoryRegionOps fsi_master_ops = {
+ .read = fsi_master_read,
+ .write = fsi_master_write,
+ .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void fsi_master_realize(DeviceState *dev, Error **errp)
+{
+ FSIMasterState *s = FSI_MASTER(dev);
+
+ 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);
+
+ if (!object_property_set_bool(OBJECT(&s->bus), "realized", true, errp)) {
+ return;
+ }
+
+ memory_region_add_subregion(&s->opb2fsi, 0, &s->bus.slave.mr);
+
+ /* Let's add first CFAM */
+ object_initialize_child(OBJECT(BUS(&s->bus)), TYPE_FSI_CFAM, &s->bus.slave,
+ TYPE_FSI_CFAM);
+ qdev_set_parent_bus(DEVICE(&s->bus.slave), BUS(&s->bus), &error_abort);
+ object_property_set_bool(OBJECT(&s->bus.slave), "realized", true, errp);
+}
+
+static void fsi_master_reset(DeviceState *dev)
+{
+ FSIMasterState *s = FSI_MASTER(dev);
+
+ /* 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_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/fsi.c b/hw/fsi/fsi.c
new file mode 100644
index 0000000000..180283746e
--- /dev/null
+++ b/hw/fsi/fsi.c
@@ -0,0 +1,25 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM Flexible Service Interface
+ */
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+
+#include "hw/fsi/fsi.h"
+#include "hw/fsi/cfam.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);
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index a9e7cd4099..f617943b4a 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -1,4 +1,4 @@
system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c'))
system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
-system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi-slave.c'))
+system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi.c','fsi-master.c','fsi-slave.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index b57b2dcc86..89d8cd62c8 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -7,3 +7,5 @@ fsi_cfam_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%"
fsi_cfam_config_write_noaddr(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_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] 30+ messages in thread* [PATCH v6 05/10] hw/fsi: IBM's On-chip Peripheral Bus
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (3 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 04/10] hw/fsi: Introduce IBM's FSI Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface Ninad Palsule
` (4 subsequent siblings)
9 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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 On-Chip Peripheral Bus (OPB): A low-speed bus typically found in
POWER processors. This now makes an appearance in the ASPEED SoC due
to tight integration of the FSI master IP with the OPB, mainly the
existence of an MMIO-mapping of the CFAM address straight onto a
sub-region of the OPB address space.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
v2:
- Incorporated review comment by Joel.
v5:
- Incorporated review comments by Cedric.
v6:
- Incorporated review comments by Cedric & Daniel
---
include/hw/fsi/opb.h | 43 ++++++++++++
hw/fsi/fsi-master.c | 3 +-
hw/fsi/opb.c | 164 +++++++++++++++++++++++++++++++++++++++++++
hw/fsi/Kconfig | 4 ++
hw/fsi/meson.build | 1 +
hw/fsi/trace-events | 8 +++
6 files changed, 221 insertions(+), 2 deletions(-)
create mode 100644 include/hw/fsi/opb.h
create mode 100644 hw/fsi/opb.c
diff --git a/include/hw/fsi/opb.h b/include/hw/fsi/opb.h
new file mode 100644
index 0000000000..9b143564ae
--- /dev/null
+++ b/include/hw/fsi/opb.h
@@ -0,0 +1,43 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM On-Chip Peripheral Bus
+ */
+#ifndef FSI_OPB_H
+#define FSI_OPB_H
+
+#include "exec/memory.h"
+#include "hw/fsi/fsi-master.h"
+
+#define TYPE_OP_BUS "opb"
+OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
+
+typedef struct OPBus {
+ /*< private >*/
+ BusState bus;
+
+ /*< public >*/
+ MemoryRegion mr;
+ AddressSpace as;
+
+ /* Model OPB as dumb enough just to provide an address-space */
+ /* TODO: Maybe don't store device state in the bus? */
+ FSIMasterState fsi;
+} OPBus;
+
+typedef struct OPBusClass {
+ BusClass parent_class;
+} OPBusClass;
+
+uint8_t fsi_opb_read8(OPBus *opb, hwaddr addr);
+uint16_t fsi_opb_read16(OPBus *opb, hwaddr addr);
+uint32_t fsi_opb_read32(OPBus *opb, hwaddr addr);
+void fsi_opb_write8(OPBus *opb, hwaddr addr, uint8_t data);
+void fsi_opb_write16(OPBus *opb, hwaddr addr, uint16_t data);
+void fsi_opb_write32(OPBus *opb, hwaddr addr, uint32_t data);
+
+void fsi_opb_fsi_master_address(OPBus *opb, hwaddr addr);
+void fsi_opb_opb2fsi_address(OPBus *opb, hwaddr addr);
+
+#endif /* FSI_OPB_H */
diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c
index fa23af3de2..91d1a57c4e 100644
--- a/hw/fsi/fsi-master.c
+++ b/hw/fsi/fsi-master.c
@@ -11,8 +11,7 @@
#include "trace.h"
#include "hw/fsi/fsi-master.h"
-
-#define TYPE_OP_BUS "opb"
+#include "hw/fsi/opb.h"
#define TO_REG(x) ((x) >> 2)
diff --git a/hw/fsi/opb.c b/hw/fsi/opb.c
new file mode 100644
index 0000000000..087e233701
--- /dev/null
+++ b/hw/fsi/opb.c
@@ -0,0 +1,164 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * IBM On-chip Peripheral Bus
+ */
+
+#include "qemu/osdep.h"
+
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/opb.h"
+
+uint8_t fsi_opb_read8(OPBus *opb, hwaddr addr)
+{
+ MemTxResult tx;
+ uint8_t data;
+
+ tx = address_space_read(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, &data,
+ sizeof(data));
+ if (tx) {
+ trace_fsi_opb_read8(addr, sizeof(data));
+ }
+
+ return data;
+}
+
+uint16_t fsi_opb_read16(OPBus *opb, hwaddr addr)
+{
+ MemTxResult tx;
+ uint16_t data;
+
+ tx = address_space_read(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, &data,
+ sizeof(data));
+ if (tx) {
+ trace_fsi_opb_read16(addr, sizeof(data));
+ }
+
+ return data;
+}
+
+uint32_t fsi_opb_read32(OPBus *opb, hwaddr addr)
+{
+ MemTxResult tx;
+ uint32_t data;
+
+ tx = address_space_read(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, &data,
+ sizeof(data));
+ if (tx) {
+ trace_fsi_opb_read32(addr, sizeof(data));
+ }
+
+ return data;
+}
+
+void fsi_opb_write8(OPBus *opb, hwaddr addr, uint8_t data)
+{
+ MemTxResult tx;
+
+ tx = address_space_write(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, &data,
+ sizeof(data));
+ if (tx) {
+ trace_fsi_opb_write8(addr, sizeof(data));
+ }
+}
+
+void fsi_opb_write16(OPBus *opb, hwaddr addr, uint16_t data)
+{
+ MemTxResult tx;
+
+ tx = address_space_write(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, &data,
+ sizeof(data));
+ if (tx) {
+ trace_fsi_opb_write16(addr, sizeof(data));
+ }
+}
+
+void fsi_opb_write32(OPBus *opb, hwaddr addr, uint32_t data)
+{
+ MemTxResult tx;
+
+ tx = address_space_write(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, &data,
+ sizeof(data));
+ if (tx) {
+ trace_fsi_opb_write32(addr, sizeof(data));
+ }
+}
+
+void fsi_opb_fsi_master_address(OPBus *opb, hwaddr addr)
+{
+ memory_region_transaction_begin();
+ memory_region_set_address(&opb->fsi.iomem, addr);
+ memory_region_transaction_commit();
+}
+
+void fsi_opb_opb2fsi_address(OPBus *opb, hwaddr addr)
+{
+ memory_region_transaction_begin();
+ memory_region_set_address(&opb->fsi.opb2fsi, addr);
+ memory_region_transaction_commit();
+}
+
+static void fsi_opb_realize(BusState *bus, Error **errp)
+{
+ OPBus *opb = OP_BUS(bus);
+
+ memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb,
+ NULL, UINT32_MAX);
+ address_space_init(&opb->as, &opb->mr, "opb");
+
+ if (!object_property_set_bool(OBJECT(&opb->fsi), "realized", true, errp)) {
+ return;
+ }
+
+ memory_region_add_subregion(&opb->mr, 0x80000000, &opb->fsi.iomem);
+
+ /* OPB2FSI region */
+ /*
+ * Avoid endianness issues by mapping each slave's memory region directly.
+ * Manually bridging multiple address-spaces causes endian swapping
+ * headaches as memory_region_dispatch_read() and
+ * memory_region_dispatch_write() correct the endianness based on the
+ * target machine endianness and not relative to the device endianness on
+ * either side of the bridge.
+ */
+ /*
+ * XXX: This is a bit hairy and will need to be fixed when I sort out the
+ * bus/slave relationship and any changes to the CFAM modelling (multiple
+ * slaves, LBUS)
+ */
+ memory_region_add_subregion(&opb->mr, 0xa0000000, &opb->fsi.opb2fsi);
+}
+
+static void fsi_opb_init(Object *o)
+{
+ OPBus *opb = OP_BUS(o);
+
+ object_initialize_child(o, "fsi-master", &opb->fsi, TYPE_FSI_MASTER);
+ qdev_set_parent_bus(DEVICE(&opb->fsi), BUS(o), &error_abort);
+}
+
+static void fsi_opb_class_init(ObjectClass *klass, void *data)
+{
+ BusClass *bc = BUS_CLASS(klass);
+ bc->realize = fsi_opb_realize;
+}
+
+static const TypeInfo opb_info = {
+ .name = TYPE_OP_BUS,
+ .parent = TYPE_BUS,
+ .instance_init = fsi_opb_init,
+ .instance_size = sizeof(OPBus),
+ .class_init = fsi_opb_class_init,
+ .class_size = sizeof(OPBusClass),
+};
+
+static void fsi_opb_register_types(void)
+{
+ type_register_static(&opb_info);
+}
+
+type_init(fsi_opb_register_types);
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index 8d712e77ed..0f6e6d331a 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,3 +1,7 @@
+config FSI_OPB
+ bool
+ select FSI_CFAM
+
config FSI_CFAM
bool
select FSI
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index f617943b4a..407b8c2775 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -2,3 +2,4 @@ system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c'))
system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi.c','fsi-master.c','fsi-slave.c'))
+system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index 89d8cd62c8..13b211a815 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -9,3 +9,11 @@ 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_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
+fsi_opb_read8(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_read16(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_read32(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_write8(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_write16(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_write32(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
+fsi_opb_unimplemented_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] 30+ messages in thread* [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (4 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 05/10] hw/fsi: IBM's On-chip Peripheral Bus Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-24 7:46 ` Cédric Le Goater
2023-10-21 21:17 ` [PATCH v6 07/10] hw/arm: Hook up FSI module in AST2600 Ninad Palsule
` (3 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v2:
- Incorporated review comments by Joel
v3:
- Incorporated review comments by Thomas Huth
v4:
- Compile FSI with ASPEED_SOC only.
v5:
- Incorporated review comments by Cedric.
v6:
- Incorporated review comments by Cedric.
---
include/hw/fsi/aspeed-apb2opb.h | 33 ++++
hw/fsi/aspeed-apb2opb.c | 280 ++++++++++++++++++++++++++++++++
hw/arm/Kconfig | 1 +
hw/fsi/Kconfig | 4 +
hw/fsi/meson.build | 1 +
hw/fsi/trace-events | 2 +
6 files changed, 321 insertions(+)
create mode 100644 include/hw/fsi/aspeed-apb2opb.h
create mode 100644 hw/fsi/aspeed-apb2opb.c
diff --git a/include/hw/fsi/aspeed-apb2opb.h b/include/hw/fsi/aspeed-apb2opb.h
new file mode 100644
index 0000000000..a81ae67023
--- /dev/null
+++ b/include/hw/fsi/aspeed-apb2opb.h
@@ -0,0 +1,33 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * ASPEED APB2OPB Bridge
+ */
+#ifndef FSI_ASPEED_APB2OPB_H
+#define FSI_ASPEED_APB2OPB_H
+
+#include "hw/sysbus.h"
+#include "hw/fsi/opb.h"
+
+#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];
+} AspeedAPB2OPBState;
+
+#endif /* FSI_ASPEED_APB2OPB_H */
diff --git a/hw/fsi/aspeed-apb2opb.c b/hw/fsi/aspeed-apb2opb.c
new file mode 100644
index 0000000000..6f97a6bc7d
--- /dev/null
+++ b/hw/fsi/aspeed-apb2opb.c
@@ -0,0 +1,280 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2023 IBM Corp.
+ *
+ * ASPEED APB-OPB FSI interface
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/fsi/aspeed-apb2opb.h"
+#include "hw/qdev-core.h"
+
+#define TO_REG(x) (x >> 2)
+#define GENMASK(t, b) (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
+
+#define APB2OPB_VERSION TO_REG(0x00)
+#define APB2OPB_VERSION_VER GENMASK(7, 0)
+
+#define APB2OPB_TRIGGER TO_REG(0x04)
+#define APB2OPB_TRIGGER_EN BIT(0)
+
+#define APB2OPB_CONTROL TO_REG(0x08)
+#define APB2OPB_CONTROL_OFF GENMASK(31, 13)
+
+#define APB2OPB_OPB2FSI TO_REG(0x0c)
+#define APB2OPB_OPB2FSI_OFF 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 uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
+
+ trace_fsi_aspeed_apb2opb_read(addr, size);
+
+ if (addr + size > sizeof(s->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[TO_REG(addr)];
+}
+
+static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
+
+ trace_fsi_aspeed_apb2opb_write(addr, size, data);
+
+ if (addr + size > sizeof(s->regs)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+ return;
+ }
+
+ switch (TO_REG(addr)) {
+ case APB2OPB_CONTROL:
+ fsi_opb_fsi_master_address(&s->opb[0], data & APB2OPB_CONTROL_OFF);
+ break;
+ case APB2OPB_OPB2FSI:
+ fsi_opb_opb2fsi_address(&s->opb[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;
+
+ 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: Unrecognised access width: %d\n",
+ op_size);
+ return;
+ }
+
+ op_size += 1;
+
+ if (op_mode & APB2OPB_OPB_MODE_RD) {
+ int index = opb ? APB2OPB_OPB1_READ_DATA
+ : APB2OPB_OPB0_READ_DATA;
+
+ switch (op_size) {
+ case 1:
+ s->regs[index] = fsi_opb_read8(&s->opb[opb], op_addr);
+ break;
+ case 2:
+ s->regs[index] = fsi_opb_read16(&s->opb[opb], op_addr);
+ break;
+ case 4:
+ s->regs[index] = fsi_opb_read32(&s->opb[opb], op_addr);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Size not supported: %u\n",
+ __func__, size);
+ return;
+ }
+ } else {
+ /* FIXME: Endian swizzling */
+ switch (op_size) {
+ case 1:
+ fsi_opb_write8(&s->opb[opb], op_addr, op_data);
+ break;
+ case 2:
+ fsi_opb_write16(&s->opb[opb], op_addr, op_data);
+ break;
+ case 4:
+ fsi_opb_write32(&s->opb[opb], op_addr, op_data);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Size not supported: %u\n",
+ __func__, op_size);
+ return;
+ }
+ }
+ s->regs[APB2OPB_IRQ_STS] |= opb ? APB2OPB_IRQ_STS_OPB1_TX_ACK
+ : APB2OPB_IRQ_STS_OPB0_TX_ACK;
+ break;
+ }
+ }
+
+ s->regs[TO_REG(addr)] = 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_realize(DeviceState *dev, Error **errp)
+{
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
+
+ qbus_init(&s->opb[0], sizeof(s->opb[0]), TYPE_OP_BUS,
+ DEVICE(s), NULL);
+ qbus_init(&s->opb[1], sizeof(s->opb[1]), TYPE_OP_BUS,
+ DEVICE(s), NULL);
+
+ 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);
+}
+
+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_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);
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 7e68348440..d963de74c9 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -555,6 +555,7 @@ config ASPEED_SOC
select LED
select PMBUS
select MAX31785
+ select FSI_APB2OPB_ASPEED
config MPS2
bool
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index 0f6e6d331a..6bbcb8f6ca 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,3 +1,7 @@
+config FSI_APB2OPB_ASPEED
+ bool
+ select FSI_OPB
+
config FSI_OPB
bool
select FSI_CFAM
diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
index 407b8c2775..1bc6bb63cc 100644
--- a/hw/fsi/meson.build
+++ b/hw/fsi/meson.build
@@ -3,3 +3,4 @@ system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c
system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi.c','fsi-master.c','fsi-slave.c'))
system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))
+system_ss.add(when: 'CONFIG_FSI_APB2OPB_ASPEED', if_true: files('aspeed-apb2opb.c'))
diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
index 13b211a815..9a45843eb6 100644
--- a/hw/fsi/trace-events
+++ b/hw/fsi/trace-events
@@ -17,3 +17,5 @@ fsi_opb_write16(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_opb_write32(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_opb_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
fsi_opb_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
+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] 30+ messages in thread* Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-21 21:17 ` [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface Ninad Palsule
@ 2023-10-24 7:46 ` Cédric Le Goater
2023-10-24 15:00 ` Ninad Palsule
0 siblings, 1 reply; 30+ messages in thread
From: Cédric Le Goater @ 2023-10-24 7:46 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 10/21/23 23:17, 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.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v2:
> - Incorporated review comments by Joel
> v3:
> - Incorporated review comments by Thomas Huth
> v4:
> - Compile FSI with ASPEED_SOC only.
> v5:
> - Incorporated review comments by Cedric.
> v6:
> - Incorporated review comments by Cedric.
> ---
> include/hw/fsi/aspeed-apb2opb.h | 33 ++++
> hw/fsi/aspeed-apb2opb.c | 280 ++++++++++++++++++++++++++++++++
> hw/arm/Kconfig | 1 +
> hw/fsi/Kconfig | 4 +
> hw/fsi/meson.build | 1 +
> hw/fsi/trace-events | 2 +
> 6 files changed, 321 insertions(+)
> create mode 100644 include/hw/fsi/aspeed-apb2opb.h
> create mode 100644 hw/fsi/aspeed-apb2opb.c
>
> diff --git a/include/hw/fsi/aspeed-apb2opb.h b/include/hw/fsi/aspeed-apb2opb.h
> new file mode 100644
> index 0000000000..a81ae67023
> --- /dev/null
> +++ b/include/hw/fsi/aspeed-apb2opb.h
> @@ -0,0 +1,33 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * ASPEED APB2OPB Bridge
> + */
> +#ifndef FSI_ASPEED_APB2OPB_H
> +#define FSI_ASPEED_APB2OPB_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/fsi/opb.h"
> +
> +#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];
> +} AspeedAPB2OPBState;
> +
> +#endif /* FSI_ASPEED_APB2OPB_H */
> diff --git a/hw/fsi/aspeed-apb2opb.c b/hw/fsi/aspeed-apb2opb.c
> new file mode 100644
> index 0000000000..6f97a6bc7d
> --- /dev/null
> +++ b/hw/fsi/aspeed-apb2opb.c
> @@ -0,0 +1,280 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * ASPEED APB-OPB FSI interface
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qom/object.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +
> +#include "hw/fsi/aspeed-apb2opb.h"
> +#include "hw/qdev-core.h"
> +
> +#define TO_REG(x) (x >> 2)
> +#define GENMASK(t, b) (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
> +
> +#define APB2OPB_VERSION TO_REG(0x00)
> +#define APB2OPB_VERSION_VER GENMASK(7, 0)
> +
> +#define APB2OPB_TRIGGER TO_REG(0x04)
> +#define APB2OPB_TRIGGER_EN BIT(0)
> +
> +#define APB2OPB_CONTROL TO_REG(0x08)
> +#define APB2OPB_CONTROL_OFF GENMASK(31, 13)
> +
> +#define APB2OPB_OPB2FSI TO_REG(0x0c)
> +#define APB2OPB_OPB2FSI_OFF 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 uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
> +
> + trace_fsi_aspeed_apb2opb_read(addr, size);
> +
> + if (addr + size > sizeof(s->regs)) {
hmm, the parameter 'size' is a memop transaction size not an address offset.
> + 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[TO_REG(addr)];
> +}
> +
> +static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr, uint64_t data,
> + unsigned size)
> +{
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
> +
> + trace_fsi_aspeed_apb2opb_write(addr, size, data);
> +
> + if (addr + size > sizeof(s->regs)) {
same comment.
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
> + __func__, addr, size);
> + return;
> + }
> +
> + switch (TO_REG(addr)) {
> + case APB2OPB_CONTROL:
> + fsi_opb_fsi_master_address(&s->opb[0], data & APB2OPB_CONTROL_OFF);
fsi_opb_fsi_master_address() should statically defined in this file
> + break;
> + case APB2OPB_OPB2FSI:
> + fsi_opb_opb2fsi_address(&s->opb[0], data & APB2OPB_OPB2FSI_OFF);
same for fsi_opb_opb2fsi_address()
> + 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;
> +
> + 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: Unrecognised access width: %d\n",
Unrecognized
> + op_size);
> + return;
> + }
> +
> + op_size += 1;
> +
> + if (op_mode & APB2OPB_OPB_MODE_RD) {
> + int index = opb ? APB2OPB_OPB1_READ_DATA
> + : APB2OPB_OPB0_READ_DATA;
> +
> + switch (op_size) {
> + case 1:
> + s->regs[index] = fsi_opb_read8(&s->opb[opb], op_addr);
> + break;
> + case 2:
> + s->regs[index] = fsi_opb_read16(&s->opb[opb], op_addr);
> + break;
> + case 4:
> + s->regs[index] = fsi_opb_read32(&s->opb[opb], op_addr);
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Size not supported: %u\n",
> + __func__, size);
this should use op_size and not size and seems redudant with
the unrecognized test above.
> + return;
> + }
> + } else {
> + /* FIXME: Endian swizzling */
> + switch (op_size) {
> + case 1:
> + fsi_opb_write8(&s->opb[opb], op_addr, op_data);
> + break;
> + case 2:
> + fsi_opb_write16(&s->opb[opb], op_addr, op_data);
> + break;
> + case 4:
> + fsi_opb_write32(&s->opb[opb], op_addr, op_data);
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Size not supported: %u\n",
> + __func__, op_size);
> + return;
> + }
> + }
The above is equivalent to :
MemTxResult result;
bool is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
int index = opb ? APB2OPB_OPB1_READ_DATA : APB2OPB_OPB0_READ_DATA;
AddressSpace *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;
}
and the fsi_opb_* routines are useless to me.
> + s->regs[APB2OPB_IRQ_STS] |= opb ? APB2OPB_IRQ_STS_OPB1_TX_ACK
> + : APB2OPB_IRQ_STS_OPB0_TX_ACK;
> + break;
> + }
> + }
> +
> + s->regs[TO_REG(addr)] = 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_realize(DeviceState *dev, Error **errp)
> +{
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
> +
> + qbus_init(&s->opb[0], sizeof(s->opb[0]), TYPE_OP_BUS,
> + DEVICE(s), NULL);
> + qbus_init(&s->opb[1], sizeof(s->opb[1]), TYPE_OP_BUS,
> + DEVICE(s), NULL);
> +
> + 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);
> +}
> +
> +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_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);
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 7e68348440..d963de74c9 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -555,6 +555,7 @@ config ASPEED_SOC
> select LED
> select PMBUS
> select MAX31785
> + select FSI_APB2OPB_ASPEED
>
> config MPS2
> bool
> diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
> index 0f6e6d331a..6bbcb8f6ca 100644
> --- a/hw/fsi/Kconfig
> +++ b/hw/fsi/Kconfig
> @@ -1,3 +1,7 @@
> +config FSI_APB2OPB_ASPEED
> + bool
> + select FSI_OPB
> +
> config FSI_OPB
> bool
> select FSI_CFAM
> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> index 407b8c2775..1bc6bb63cc 100644
> --- a/hw/fsi/meson.build
> +++ b/hw/fsi/meson.build
> @@ -3,3 +3,4 @@ system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: files('engine-scratchpad.c
> system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
> system_ss.add(when: 'CONFIG_FSI', if_true: files('fsi.c','fsi-master.c','fsi-slave.c'))
> system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))
> +system_ss.add(when: 'CONFIG_FSI_APB2OPB_ASPEED', if_true: files('aspeed-apb2opb.c'))
> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
> index 13b211a815..9a45843eb6 100644
> --- a/hw/fsi/trace-events
> +++ b/hw/fsi/trace-events
> @@ -17,3 +17,5 @@ fsi_opb_write16(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_opb_write32(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_opb_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> fsi_opb_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
> +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] 30+ messages in thread* Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-24 7:46 ` Cédric Le Goater
@ 2023-10-24 15:00 ` Ninad Palsule
2023-10-24 15:21 ` Cédric Le Goater
0 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-24 15:00 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,
On 10/24/23 02:46, Cédric Le Goater wrote:
> On 10/21/23 23:17, 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.
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v2:
>> - Incorporated review comments by Joel
>> v3:
>> - Incorporated review comments by Thomas Huth
>> v4:
>> - Compile FSI with ASPEED_SOC only.
>> v5:
>> - Incorporated review comments by Cedric.
>> v6:
>> - Incorporated review comments by Cedric.
>> ---
>> include/hw/fsi/aspeed-apb2opb.h | 33 ++++
>> hw/fsi/aspeed-apb2opb.c | 280 ++++++++++++++++++++++++++++++++
>> hw/arm/Kconfig | 1 +
>> hw/fsi/Kconfig | 4 +
>> hw/fsi/meson.build | 1 +
>> hw/fsi/trace-events | 2 +
>> 6 files changed, 321 insertions(+)
>> create mode 100644 include/hw/fsi/aspeed-apb2opb.h
>> create mode 100644 hw/fsi/aspeed-apb2opb.c
>>
>> diff --git a/include/hw/fsi/aspeed-apb2opb.h
>> b/include/hw/fsi/aspeed-apb2opb.h
>> new file mode 100644
>> index 0000000000..a81ae67023
>> --- /dev/null
>> +++ b/include/hw/fsi/aspeed-apb2opb.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 IBM Corp.
>> + *
>> + * ASPEED APB2OPB Bridge
>> + */
>> +#ifndef FSI_ASPEED_APB2OPB_H
>> +#define FSI_ASPEED_APB2OPB_H
>> +
>> +#include "hw/sysbus.h"
>> +#include "hw/fsi/opb.h"
>> +
>> +#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];
>> +} AspeedAPB2OPBState;
>> +
>> +#endif /* FSI_ASPEED_APB2OPB_H */
>> diff --git a/hw/fsi/aspeed-apb2opb.c b/hw/fsi/aspeed-apb2opb.c
>> new file mode 100644
>> index 0000000000..6f97a6bc7d
>> --- /dev/null
>> +++ b/hw/fsi/aspeed-apb2opb.c
>> @@ -0,0 +1,280 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + * Copyright (C) 2023 IBM Corp.
>> + *
>> + * ASPEED APB-OPB FSI interface
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qom/object.h"
>> +#include "qapi/error.h"
>> +#include "trace.h"
>> +
>> +#include "hw/fsi/aspeed-apb2opb.h"
>> +#include "hw/qdev-core.h"
>> +
>> +#define TO_REG(x) (x >> 2)
>> +#define GENMASK(t, b) (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) -
>> 1))
>> +
>> +#define APB2OPB_VERSION TO_REG(0x00)
>> +#define APB2OPB_VERSION_VER GENMASK(7, 0)
>> +
>> +#define APB2OPB_TRIGGER TO_REG(0x04)
>> +#define APB2OPB_TRIGGER_EN BIT(0)
>> +
>> +#define APB2OPB_CONTROL TO_REG(0x08)
>> +#define APB2OPB_CONTROL_OFF GENMASK(31, 13)
>> +
>> +#define APB2OPB_OPB2FSI TO_REG(0x0c)
>> +#define APB2OPB_OPB2FSI_OFF 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 uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
>> + unsigned size)
>> +{
>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>> +
>> + trace_fsi_aspeed_apb2opb_read(addr, size);
>> +
>> + if (addr + size > sizeof(s->regs)) {
>
>
> hmm, the parameter 'size' is a memop transaction size not an address
> offset.
OK, Changed it to validate the register (index) instead of addr + size.
>
>> + 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[TO_REG(addr)];
>> +}
>> +
>> +static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr,
>> uint64_t data,
>> + unsigned size)
>> +{
>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>> +
>> + trace_fsi_aspeed_apb2opb_write(addr, size, data);
>> +
>> + if (addr + size > sizeof(s->regs)) {
>
> same comment.
Fixed it same as above.
>
>
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Out of bounds write: %"HWADDR_PRIx" for
>> %u\n",
>> + __func__, addr, size);
>> + return;
>> + }
>> +
>> + switch (TO_REG(addr)) {
>> + case APB2OPB_CONTROL:
>> + fsi_opb_fsi_master_address(&s->opb[0], data &
>> APB2OPB_CONTROL_OFF);
>
> fsi_opb_fsi_master_address() should statically defined in this file
We have separation of OPB bus implementation and APB2OPB interface. If
we move this function here then we will be exposing OPB implementation here.
>
>> + break;
>> + case APB2OPB_OPB2FSI:
>> + fsi_opb_opb2fsi_address(&s->opb[0], data &
>> APB2OPB_OPB2FSI_OFF);
>
>
> same for fsi_opb_opb2fsi_address()
Same as above.
>
>> + 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;
>> +
>> + 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: Unrecognised
>> access width: %d\n",
>
> Unrecognized
Fixed
>
>> + op_size);
>> + return;
>> + }
>> +
>> + op_size += 1;
>> +
>> + if (op_mode & APB2OPB_OPB_MODE_RD) {
>> + int index = opb ? APB2OPB_OPB1_READ_DATA
>> + : APB2OPB_OPB0_READ_DATA;
>> +
>> + switch (op_size) {
>> + case 1:
>> + s->regs[index] = fsi_opb_read8(&s->opb[opb], op_addr);
>> + break;
>> + case 2:
>> + s->regs[index] = fsi_opb_read16(&s->opb[opb], op_addr);
>> + break;
>> + case 4:
>> + s->regs[index] = fsi_opb_read32(&s->opb[opb], op_addr);
>> + break;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Size not supported: %u\n",
>> + __func__, size);
>
> this should use op_size and not size and seems redudant with
> the unrecognized test above.
true, Keeping it in case bits meaning change in future.
>
>
>> + return;
>> + }
>> + } else {
>> + /* FIXME: Endian swizzling */
>> + switch (op_size) {
>> + case 1:
>> + fsi_opb_write8(&s->opb[opb], op_addr, op_data);
>> + break;
>> + case 2:
>> + fsi_opb_write16(&s->opb[opb], op_addr, op_data);
>> + break;
>> + case 4:
>> + fsi_opb_write32(&s->opb[opb], op_addr, op_data);
>> + break;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "%s: Size not supported: %u\n",
>> + __func__, op_size);
>> + return;
>> + }
>> + }
>
>
> The above is equivalent to :
>
> MemTxResult result;
> bool is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
> int index = opb ? APB2OPB_OPB1_READ_DATA :
> APB2OPB_OPB0_READ_DATA;
> AddressSpace *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;
> }
>
> and the fsi_opb_* routines are useless to me.
We are trying to keep the separation between OPB implementation and
interface hence we have all those fsi_opb_*. I feel that we should keep
as it is so that future extensions will be easier. Please let me know.
>
>
>
>
>> + s->regs[APB2OPB_IRQ_STS] |= opb ? APB2OPB_IRQ_STS_OPB1_TX_ACK
>> + : APB2OPB_IRQ_STS_OPB0_TX_ACK;
>> + break;
>> + }
>> + }
>> +
>> + s->regs[TO_REG(addr)] = 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_realize(DeviceState *dev, Error **errp)
>> +{
>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(dev);
>> +
>> + qbus_init(&s->opb[0], sizeof(s->opb[0]), TYPE_OP_BUS,
>> + DEVICE(s), NULL);
>> + qbus_init(&s->opb[1], sizeof(s->opb[1]), TYPE_OP_BUS,
>> + DEVICE(s), NULL);
>> +
>> + 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);
>> +}
>> +
>> +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_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);
>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>> index 7e68348440..d963de74c9 100644
>> --- a/hw/arm/Kconfig
>> +++ b/hw/arm/Kconfig
>> @@ -555,6 +555,7 @@ config ASPEED_SOC
>> select LED
>> select PMBUS
>> select MAX31785
>> + select FSI_APB2OPB_ASPEED
>> config MPS2
>> bool
>> diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
>> index 0f6e6d331a..6bbcb8f6ca 100644
>> --- a/hw/fsi/Kconfig
>> +++ b/hw/fsi/Kconfig
>> @@ -1,3 +1,7 @@
>> +config FSI_APB2OPB_ASPEED
>> + bool
>> + select FSI_OPB
>> +
>> config FSI_OPB
>> bool
>> select FSI_CFAM
>> diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
>> index 407b8c2775..1bc6bb63cc 100644
>> --- a/hw/fsi/meson.build
>> +++ b/hw/fsi/meson.build
>> @@ -3,3 +3,4 @@ system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true:
>> files('engine-scratchpad.c
>> system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
>> system_ss.add(when: 'CONFIG_FSI', if_true:
>> files('fsi.c','fsi-master.c','fsi-slave.c'))
>> system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))
>> +system_ss.add(when: 'CONFIG_FSI_APB2OPB_ASPEED', if_true:
>> files('aspeed-apb2opb.c'))
>> diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
>> index 13b211a815..9a45843eb6 100644
>> --- a/hw/fsi/trace-events
>> +++ b/hw/fsi/trace-events
>> @@ -17,3 +17,5 @@ fsi_opb_write16(uint64_t addr, uint32_t size)
>> "@0x%" PRIx64 " size=%d"
>> fsi_opb_write32(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
>> fsi_opb_unimplemented_read(uint64_t addr, uint32_t size) "@0x%"
>> PRIx64 " size=%d"
>> fsi_opb_unimplemented_write(uint64_t addr, uint32_t size, uint64_t
>> data) "@0x%" PRIx64 " size=%d value=0x%"PRIx64
>> +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] 30+ messages in thread* Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-24 15:00 ` Ninad Palsule
@ 2023-10-24 15:21 ` Cédric Le Goater
2023-10-24 18:42 ` Ninad Palsule
2023-10-26 15:27 ` Ninad Palsule
0 siblings, 2 replies; 30+ messages in thread
From: Cédric Le Goater @ 2023-10-24 15:21 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 10/24/23 17:00, Ninad Palsule wrote:
> Hello Cedric,
>
> On 10/24/23 02:46, Cédric Le Goater wrote:
>> On 10/21/23 23:17, 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.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>> ---
>>> v2:
>>> - Incorporated review comments by Joel
>>> v3:
>>> - Incorporated review comments by Thomas Huth
>>> v4:
>>> - Compile FSI with ASPEED_SOC only.
>>> v5:
>>> - Incorporated review comments by Cedric.
>>> v6:
>>> - Incorporated review comments by Cedric.
>>> ---
>>> include/hw/fsi/aspeed-apb2opb.h | 33 ++++
>>> hw/fsi/aspeed-apb2opb.c | 280 ++++++++++++++++++++++++++++++++
>>> hw/arm/Kconfig | 1 +
>>> hw/fsi/Kconfig | 4 +
>>> hw/fsi/meson.build | 1 +
>>> hw/fsi/trace-events | 2 +
>>> 6 files changed, 321 insertions(+)
>>> create mode 100644 include/hw/fsi/aspeed-apb2opb.h
>>> create mode 100644 hw/fsi/aspeed-apb2opb.c
>>>
>>> diff --git a/include/hw/fsi/aspeed-apb2opb.h b/include/hw/fsi/aspeed-apb2opb.h
>>> new file mode 100644
>>> index 0000000000..a81ae67023
>>> --- /dev/null
>>> +++ b/include/hw/fsi/aspeed-apb2opb.h
>>> @@ -0,0 +1,33 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + * Copyright (C) 2023 IBM Corp.
>>> + *
>>> + * ASPEED APB2OPB Bridge
>>> + */
>>> +#ifndef FSI_ASPEED_APB2OPB_H
>>> +#define FSI_ASPEED_APB2OPB_H
>>> +
>>> +#include "hw/sysbus.h"
>>> +#include "hw/fsi/opb.h"
>>> +
>>> +#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];
>>> +} AspeedAPB2OPBState;
>>> +
>>> +#endif /* FSI_ASPEED_APB2OPB_H */
>>> diff --git a/hw/fsi/aspeed-apb2opb.c b/hw/fsi/aspeed-apb2opb.c
>>> new file mode 100644
>>> index 0000000000..6f97a6bc7d
>>> --- /dev/null
>>> +++ b/hw/fsi/aspeed-apb2opb.c
>>> @@ -0,0 +1,280 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + * Copyright (C) 2023 IBM Corp.
>>> + *
>>> + * ASPEED APB-OPB FSI interface
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "qom/object.h"
>>> +#include "qapi/error.h"
>>> +#include "trace.h"
>>> +
>>> +#include "hw/fsi/aspeed-apb2opb.h"
>>> +#include "hw/qdev-core.h"
>>> +
>>> +#define TO_REG(x) (x >> 2)
>>> +#define GENMASK(t, b) (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
>>> +
>>> +#define APB2OPB_VERSION TO_REG(0x00)
>>> +#define APB2OPB_VERSION_VER GENMASK(7, 0)
>>> +
>>> +#define APB2OPB_TRIGGER TO_REG(0x04)
>>> +#define APB2OPB_TRIGGER_EN BIT(0)
>>> +
>>> +#define APB2OPB_CONTROL TO_REG(0x08)
>>> +#define APB2OPB_CONTROL_OFF GENMASK(31, 13)
>>> +
>>> +#define APB2OPB_OPB2FSI TO_REG(0x0c)
>>> +#define APB2OPB_OPB2FSI_OFF 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 uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
>>> + unsigned size)
>>> +{
>>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>>> +
>>> + trace_fsi_aspeed_apb2opb_read(addr, size);
>>> +
>>> + if (addr + size > sizeof(s->regs)) {
>>
>>
>> hmm, the parameter 'size' is a memop transaction size not an address offset.
> OK, Changed it to validate the register (index) instead of addr + size.
>>
>>> + 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[TO_REG(addr)];
>>> +}
>>> +
>>> +static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr, uint64_t data,
>>> + unsigned size)
>>> +{
>>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>>> +
>>> + trace_fsi_aspeed_apb2opb_write(addr, size, data);
>>> +
>>> + if (addr + size > sizeof(s->regs)) {
>>
>> same comment.
> Fixed it same as above.
>>
>>
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "%s: Out of bounds write: %"HWADDR_PRIx" for %u\n",
>>> + __func__, addr, size);
>>> + return;
>>> + }
>>> +
>>> + switch (TO_REG(addr)) {
>>> + case APB2OPB_CONTROL:
>>> + fsi_opb_fsi_master_address(&s->opb[0], data & APB2OPB_CONTROL_OFF);
>>
>> fsi_opb_fsi_master_address() should statically defined in this file
> We have separation of OPB bus implementation and APB2OPB interface. If we move this function here then we will be exposing OPB implementation here.
>>
>>> + break;
>>> + case APB2OPB_OPB2FSI:
>>> + fsi_opb_opb2fsi_address(&s->opb[0], data & APB2OPB_OPB2FSI_OFF);
>>
>>
>> same for fsi_opb_opb2fsi_address()
> Same as above.
>>
>>> + 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;
>>> +
>>> + 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: Unrecognised access width: %d\n",
>>
>> Unrecognized
> Fixed
>>
>>> + op_size);
>>> + return;
>>> + }
>>> +
>>> + op_size += 1;
>>> +
>>> + if (op_mode & APB2OPB_OPB_MODE_RD) {
>>> + int index = opb ? APB2OPB_OPB1_READ_DATA
>>> + : APB2OPB_OPB0_READ_DATA;
>>> +
>>> + switch (op_size) {
>>> + case 1:
>>> + s->regs[index] = fsi_opb_read8(&s->opb[opb], op_addr);
>>> + break;
>>> + case 2:
>>> + s->regs[index] = fsi_opb_read16(&s->opb[opb], op_addr);
>>> + break;
>>> + case 4:
>>> + s->regs[index] = fsi_opb_read32(&s->opb[opb], op_addr);
>>> + break;
>>> + default:
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "%s: Size not supported: %u\n",
>>> + __func__, size);
>>
>> this should use op_size and not size and seems redudant with
>> the unrecognized test above.
> true, Keeping it in case bits meaning change in future.
>>
>>
>>> + return;
>>> + }
>>> + } else {
>>> + /* FIXME: Endian swizzling */
>>> + switch (op_size) {
>>> + case 1:
>>> + fsi_opb_write8(&s->opb[opb], op_addr, op_data);
>>> + break;
>>> + case 2:
>>> + fsi_opb_write16(&s->opb[opb], op_addr, op_data);
>>> + break;
>>> + case 4:
>>> + fsi_opb_write32(&s->opb[opb], op_addr, op_data);
>>> + break;
>>> + default:
>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>> + "%s: Size not supported: %u\n",
>>> + __func__, op_size);
>>> + return;
>>> + }
>>> + }
>>
>>
>> The above is equivalent to :
>>
>> MemTxResult result;
>> bool is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
>> int index = opb ? APB2OPB_OPB1_READ_DATA : APB2OPB_OPB0_READ_DATA;
>> AddressSpace *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;
>> }
>>
>> and the fsi_opb_* routines are useless to me.
> We are trying to keep the separation between OPB implementation and interface hence we have all those fsi_opb_*. I feel that we should keep as it is so that future extensions will be easier. Please let me know.
Well, I can't really tell because I don't know enough about FSI :/
The models look fragile and I have spent already a lot of time trying
to untangle what they are trying to do. Please ask your teammates or
let's see in the next QEMU cycle.
Thanks,
C.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-24 15:21 ` Cédric Le Goater
@ 2023-10-24 18:42 ` Ninad Palsule
2023-10-26 15:27 ` Ninad Palsule
1 sibling, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-24 18:42 UTC (permalink / raw)
To: andrew, Cédric Le Goater
Cc: qemu-arm, Andrew Jeffery, Cédric Le Goater, qemu-devel,
peter.maydell, joel, pbonzini, marcandre.lureau, berrange, thuth,
philmd, lvivier
Hello Cedric & Andrew,
On 10/24/23 10:21, Cédric Le Goater wrote:
> On 10/24/23 17:00, Ninad Palsule wrote:
>> Hello Cedric,
>>
>> On 10/24/23 02:46, Cédric Le Goater wrote:
>>> On 10/21/23 23:17, 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.
>>>>
>>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>> ---
>>>> v2:
>>>> - Incorporated review comments by Joel
>>>> v3:
>>>> - Incorporated review comments by Thomas Huth
>>>> v4:
>>>> - Compile FSI with ASPEED_SOC only.
>>>> v5:
>>>> - Incorporated review comments by Cedric.
>>>> v6:
>>>> - Incorporated review comments by Cedric.
>>>> ---
>>>> include/hw/fsi/aspeed-apb2opb.h | 33 ++++
>>>> hw/fsi/aspeed-apb2opb.c | 280
>>>> ++++++++++++++++++++++++++++++++
>>>> hw/arm/Kconfig | 1 +
>>>> hw/fsi/Kconfig | 4 +
>>>> hw/fsi/meson.build | 1 +
>>>> hw/fsi/trace-events | 2 +
>>>> 6 files changed, 321 insertions(+)
>>>> create mode 100644 include/hw/fsi/aspeed-apb2opb.h
>>>> create mode 100644 hw/fsi/aspeed-apb2opb.c
>>>>
>>>> diff --git a/include/hw/fsi/aspeed-apb2opb.h
>>>> b/include/hw/fsi/aspeed-apb2opb.h
>>>> new file mode 100644
>>>> index 0000000000..a81ae67023
>>>> --- /dev/null
>>>> +++ b/include/hw/fsi/aspeed-apb2opb.h
>>>> @@ -0,0 +1,33 @@
>>>> +/*
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + * Copyright (C) 2023 IBM Corp.
>>>> + *
>>>> + * ASPEED APB2OPB Bridge
>>>> + */
>>>> +#ifndef FSI_ASPEED_APB2OPB_H
>>>> +#define FSI_ASPEED_APB2OPB_H
>>>> +
>>>> +#include "hw/sysbus.h"
>>>> +#include "hw/fsi/opb.h"
>>>> +
>>>> +#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];
>>>> +} AspeedAPB2OPBState;
>>>> +
>>>> +#endif /* FSI_ASPEED_APB2OPB_H */
>>>> diff --git a/hw/fsi/aspeed-apb2opb.c b/hw/fsi/aspeed-apb2opb.c
>>>> new file mode 100644
>>>> index 0000000000..6f97a6bc7d
>>>> --- /dev/null
>>>> +++ b/hw/fsi/aspeed-apb2opb.c
>>>> @@ -0,0 +1,280 @@
>>>> +/*
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + * Copyright (C) 2023 IBM Corp.
>>>> + *
>>>> + * ASPEED APB-OPB FSI interface
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qom/object.h"
>>>> +#include "qapi/error.h"
>>>> +#include "trace.h"
>>>> +
>>>> +#include "hw/fsi/aspeed-apb2opb.h"
>>>> +#include "hw/qdev-core.h"
>>>> +
>>>> +#define TO_REG(x) (x >> 2)
>>>> +#define GENMASK(t, b) (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b))
>>>> - 1))
>>>> +
>>>> +#define APB2OPB_VERSION TO_REG(0x00)
>>>> +#define APB2OPB_VERSION_VER GENMASK(7, 0)
>>>> +
>>>> +#define APB2OPB_TRIGGER TO_REG(0x04)
>>>> +#define APB2OPB_TRIGGER_EN BIT(0)
>>>> +
>>>> +#define APB2OPB_CONTROL TO_REG(0x08)
>>>> +#define APB2OPB_CONTROL_OFF GENMASK(31, 13)
>>>> +
>>>> +#define APB2OPB_OPB2FSI TO_REG(0x0c)
>>>> +#define APB2OPB_OPB2FSI_OFF 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 uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
>>>> + unsigned size)
>>>> +{
>>>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>>>> +
>>>> + trace_fsi_aspeed_apb2opb_read(addr, size);
>>>> +
>>>> + if (addr + size > sizeof(s->regs)) {
>>>
>>>
>>> hmm, the parameter 'size' is a memop transaction size not an address
>>> offset.
>> OK, Changed it to validate the register (index) instead of addr + size.
>>>
>>>> + 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[TO_REG(addr)];
>>>> +}
>>>> +
>>>> +static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr,
>>>> uint64_t data,
>>>> + unsigned size)
>>>> +{
>>>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>>>> +
>>>> + trace_fsi_aspeed_apb2opb_write(addr, size, data);
>>>> +
>>>> + if (addr + size > sizeof(s->regs)) {
>>>
>>> same comment.
>> Fixed it same as above.
>>>
>>>
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: Out of bounds write: %"HWADDR_PRIx" for
>>>> %u\n",
>>>> + __func__, addr, size);
>>>> + return;
>>>> + }
>>>> +
>>>> + switch (TO_REG(addr)) {
>>>> + case APB2OPB_CONTROL:
>>>> + fsi_opb_fsi_master_address(&s->opb[0], data &
>>>> APB2OPB_CONTROL_OFF);
>>>
>>> fsi_opb_fsi_master_address() should statically defined in this file
>> We have separation of OPB bus implementation and APB2OPB interface.
>> If we move this function here then we will be exposing OPB
>> implementation here.
>>>
>>>> + break;
>>>> + case APB2OPB_OPB2FSI:
>>>> + fsi_opb_opb2fsi_address(&s->opb[0], data &
>>>> APB2OPB_OPB2FSI_OFF);
>>>
>>>
>>> same for fsi_opb_opb2fsi_address()
>> Same as above.
>>>
>>>> + 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;
>>>> +
>>>> + 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: Unrecognised
>>>> access width: %d\n",
>>>
>>> Unrecognized
>> Fixed
>>>
>>>> + op_size);
>>>> + return;
>>>> + }
>>>> +
>>>> + op_size += 1;
>>>> +
>>>> + if (op_mode & APB2OPB_OPB_MODE_RD) {
>>>> + int index = opb ? APB2OPB_OPB1_READ_DATA
>>>> + : APB2OPB_OPB0_READ_DATA;
>>>> +
>>>> + switch (op_size) {
>>>> + case 1:
>>>> + s->regs[index] = fsi_opb_read8(&s->opb[opb],
>>>> op_addr);
>>>> + break;
>>>> + case 2:
>>>> + s->regs[index] = fsi_opb_read16(&s->opb[opb],
>>>> op_addr);
>>>> + break;
>>>> + case 4:
>>>> + s->regs[index] = fsi_opb_read32(&s->opb[opb],
>>>> op_addr);
>>>> + break;
>>>> + default:
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: Size not supported: %u\n",
>>>> + __func__, size);
>>>
>>> this should use op_size and not size and seems redudant with
>>> the unrecognized test above.
>> true, Keeping it in case bits meaning change in future.
>>>
>>>
>>>> + return;
>>>> + }
>>>> + } else {
>>>> + /* FIXME: Endian swizzling */
>>>> + switch (op_size) {
>>>> + case 1:
>>>> + fsi_opb_write8(&s->opb[opb], op_addr, op_data);
>>>> + break;
>>>> + case 2:
>>>> + fsi_opb_write16(&s->opb[opb], op_addr, op_data);
>>>> + break;
>>>> + case 4:
>>>> + fsi_opb_write32(&s->opb[opb], op_addr, op_data);
>>>> + break;
>>>> + default:
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: Size not supported: %u\n",
>>>> + __func__, op_size);
>>>> + return;
>>>> + }
>>>> + }
>>>
>>>
>>> The above is equivalent to :
>>>
>>> MemTxResult result;
>>> bool is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
>>> int index = opb ? APB2OPB_OPB1_READ_DATA :
>>> APB2OPB_OPB0_READ_DATA;
>>> AddressSpace *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;
>>> }
>>>
>>> and the fsi_opb_* routines are useless to me.
>> We are trying to keep the separation between OPB implementation and
>> interface hence we have all those fsi_opb_*. I feel that we should
>> keep as it is so that future extensions will be easier. Please let me
>> know.
>
> Well, I can't really tell because I don't know enough about FSI :/
>
> The models look fragile and I have spent already a lot of time trying
> to untangle what they are trying to do. Please ask your teammates or
> let's see in the next QEMU cycle.
Thanks for helping me to get in this patch.
On chip peripherals are connected to OPB bus for easier access. All FSI
masters (including cascaded) will be attached to OPB bus but we may
never implement it. So the current model is only instantiating bus and
having wrapper around mmio. Are you suggesting to simplify it by not
having this indirection as its not providing much? If that's the case
then I can change as per your suggestion.
Andrew, Do you have any thoughts about this?
>
> Thanks,
>
> C.
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-24 15:21 ` Cédric Le Goater
2023-10-24 18:42 ` Ninad Palsule
@ 2023-10-26 15:27 ` Ninad Palsule
2023-10-27 5:25 ` Andrew Jeffery
1 sibling, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-26 15:27 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,
On 10/24/23 10:21, Cédric Le Goater wrote:
> On 10/24/23 17:00, Ninad Palsule wrote:
>> Hello Cedric,
>>
>> On 10/24/23 02:46, Cédric Le Goater wrote:
>>> On 10/21/23 23:17, 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.
>>>>
>>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>> ---
>>>> v2:
>>>> - Incorporated review comments by Joel
>>>> v3:
>>>> - Incorporated review comments by Thomas Huth
>>>> v4:
>>>> - Compile FSI with ASPEED_SOC only.
>>>> v5:
>>>> - Incorporated review comments by Cedric.
>>>> v6:
>>>> - Incorporated review comments by Cedric.
>>>> ---
>>>> include/hw/fsi/aspeed-apb2opb.h | 33 ++++
>>>> hw/fsi/aspeed-apb2opb.c | 280
>>>> ++++++++++++++++++++++++++++++++
>>>> hw/arm/Kconfig | 1 +
>>>> hw/fsi/Kconfig | 4 +
>>>> hw/fsi/meson.build | 1 +
>>>> hw/fsi/trace-events | 2 +
>>>> 6 files changed, 321 insertions(+)
>>>> create mode 100644 include/hw/fsi/aspeed-apb2opb.h
>>>> create mode 100644 hw/fsi/aspeed-apb2opb.c
>>>>
>>>> diff --git a/include/hw/fsi/aspeed-apb2opb.h
>>>> b/include/hw/fsi/aspeed-apb2opb.h
>>>> new file mode 100644
>>>> index 0000000000..a81ae67023
>>>> --- /dev/null
>>>> +++ b/include/hw/fsi/aspeed-apb2opb.h
>>>> @@ -0,0 +1,33 @@
>>>> +/*
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + * Copyright (C) 2023 IBM Corp.
>>>> + *
>>>> + * ASPEED APB2OPB Bridge
>>>> + */
>>>> +#ifndef FSI_ASPEED_APB2OPB_H
>>>> +#define FSI_ASPEED_APB2OPB_H
>>>> +
>>>> +#include "hw/sysbus.h"
>>>> +#include "hw/fsi/opb.h"
>>>> +
>>>> +#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];
>>>> +} AspeedAPB2OPBState;
>>>> +
>>>> +#endif /* FSI_ASPEED_APB2OPB_H */
>>>> diff --git a/hw/fsi/aspeed-apb2opb.c b/hw/fsi/aspeed-apb2opb.c
>>>> new file mode 100644
>>>> index 0000000000..6f97a6bc7d
>>>> --- /dev/null
>>>> +++ b/hw/fsi/aspeed-apb2opb.c
>>>> @@ -0,0 +1,280 @@
>>>> +/*
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + * Copyright (C) 2023 IBM Corp.
>>>> + *
>>>> + * ASPEED APB-OPB FSI interface
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qom/object.h"
>>>> +#include "qapi/error.h"
>>>> +#include "trace.h"
>>>> +
>>>> +#include "hw/fsi/aspeed-apb2opb.h"
>>>> +#include "hw/qdev-core.h"
>>>> +
>>>> +#define TO_REG(x) (x >> 2)
>>>> +#define GENMASK(t, b) (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b))
>>>> - 1))
>>>> +
>>>> +#define APB2OPB_VERSION TO_REG(0x00)
>>>> +#define APB2OPB_VERSION_VER GENMASK(7, 0)
>>>> +
>>>> +#define APB2OPB_TRIGGER TO_REG(0x04)
>>>> +#define APB2OPB_TRIGGER_EN BIT(0)
>>>> +
>>>> +#define APB2OPB_CONTROL TO_REG(0x08)
>>>> +#define APB2OPB_CONTROL_OFF GENMASK(31, 13)
>>>> +
>>>> +#define APB2OPB_OPB2FSI TO_REG(0x0c)
>>>> +#define APB2OPB_OPB2FSI_OFF 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 uint64_t fsi_aspeed_apb2opb_read(void *opaque, hwaddr addr,
>>>> + unsigned size)
>>>> +{
>>>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>>>> +
>>>> + trace_fsi_aspeed_apb2opb_read(addr, size);
>>>> +
>>>> + if (addr + size > sizeof(s->regs)) {
>>>
>>>
>>> hmm, the parameter 'size' is a memop transaction size not an address
>>> offset.
>> OK, Changed it to validate the register (index) instead of addr + size.
>>>
>>>> + 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[TO_REG(addr)];
>>>> +}
>>>> +
>>>> +static void fsi_aspeed_apb2opb_write(void *opaque, hwaddr addr,
>>>> uint64_t data,
>>>> + unsigned size)
>>>> +{
>>>> + AspeedAPB2OPBState *s = ASPEED_APB2OPB(opaque);
>>>> +
>>>> + trace_fsi_aspeed_apb2opb_write(addr, size, data);
>>>> +
>>>> + if (addr + size > sizeof(s->regs)) {
>>>
>>> same comment.
>> Fixed it same as above.
>>>
>>>
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: Out of bounds write: %"HWADDR_PRIx" for
>>>> %u\n",
>>>> + __func__, addr, size);
>>>> + return;
>>>> + }
>>>> +
>>>> + switch (TO_REG(addr)) {
>>>> + case APB2OPB_CONTROL:
>>>> + fsi_opb_fsi_master_address(&s->opb[0], data &
>>>> APB2OPB_CONTROL_OFF);
>>>
>>> fsi_opb_fsi_master_address() should statically defined in this file
>> We have separation of OPB bus implementation and APB2OPB interface.
>> If we move this function here then we will be exposing OPB
>> implementation here.
Defined a static function and removed from opb.c
>>>
>>>> + break;
>>>> + case APB2OPB_OPB2FSI:
>>>> + fsi_opb_opb2fsi_address(&s->opb[0], data &
>>>> APB2OPB_OPB2FSI_OFF);
>>>
>>>
>>> same for fsi_opb_opb2fsi_address()
>> Same as above.
Same as above.
>>>
>>>> + 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;
>>>> +
>>>> + 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: Unrecognised
>>>> access width: %d\n",
>>>
>>> Unrecognized
>> Fixed
>>>
>>>> + op_size);
>>>> + return;
>>>> + }
>>>> +
>>>> + op_size += 1;
>>>> +
>>>> + if (op_mode & APB2OPB_OPB_MODE_RD) {
>>>> + int index = opb ? APB2OPB_OPB1_READ_DATA
>>>> + : APB2OPB_OPB0_READ_DATA;
>>>> +
>>>> + switch (op_size) {
>>>> + case 1:
>>>> + s->regs[index] = fsi_opb_read8(&s->opb[opb],
>>>> op_addr);
>>>> + break;
>>>> + case 2:
>>>> + s->regs[index] = fsi_opb_read16(&s->opb[opb],
>>>> op_addr);
>>>> + break;
>>>> + case 4:
>>>> + s->regs[index] = fsi_opb_read32(&s->opb[opb],
>>>> op_addr);
>>>> + break;
>>>> + default:
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: Size not supported: %u\n",
>>>> + __func__, size);
>>>
>>> this should use op_size and not size and seems redudant with
>>> the unrecognized test above.
>> true, Keeping it in case bits meaning change in future.
>>>
>>>
>>>> + return;
>>>> + }
>>>> + } else {
>>>> + /* FIXME: Endian swizzling */
>>>> + switch (op_size) {
>>>> + case 1:
>>>> + fsi_opb_write8(&s->opb[opb], op_addr, op_data);
>>>> + break;
>>>> + case 2:
>>>> + fsi_opb_write16(&s->opb[opb], op_addr, op_data);
>>>> + break;
>>>> + case 4:
>>>> + fsi_opb_write32(&s->opb[opb], op_addr, op_data);
>>>> + break;
>>>> + default:
>>>> + qemu_log_mask(LOG_GUEST_ERROR,
>>>> + "%s: Size not supported: %u\n",
>>>> + __func__, op_size);
>>>> + return;
>>>> + }
>>>> + }
>>>
>>>
>>> The above is equivalent to :
>>>
>>> MemTxResult result;
>>> bool is_write = !(op_mode & APB2OPB_OPB_MODE_RD);
>>> int index = opb ? APB2OPB_OPB1_READ_DATA :
>>> APB2OPB_OPB0_READ_DATA;
>>> AddressSpace *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;
>>> }
>>>
>>> and the fsi_opb_* routines are useless to me.
>> We are trying to keep the separation between OPB implementation and
>> interface hence we have all those fsi_opb_*. I feel that we should
>> keep as it is so that future extensions will be easier. Please let me
>> know.
>
> Well, I can't really tell because I don't know enough about FSI :/
>
> The models look fragile and I have spent already a lot of time trying
> to untangle what they are trying to do. Please ask your teammates or
> let's see in the next QEMU cycle.
I have decided to go with the approach you suggested and it looks much
better. Fixed it.
Thanks for the review.
Regards,
Ninad
>
> Thanks,
>
> C.
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
2023-10-26 15:27 ` Ninad Palsule
@ 2023-10-27 5:25 ` Andrew Jeffery
0 siblings, 0 replies; 30+ messages in thread
From: Andrew Jeffery @ 2023-10-27 5:25 UTC (permalink / raw)
To: Ninad Palsule, Cédric Le Goater, qemu-devel, peter.maydell,
joel, pbonzini, marcandre.lureau, berrange, thuth, philmd,
lvivier
Cc: qemu-arm, Andrew Jeffery
On Thu, 2023-10-26 at 10:27 -0500, Ninad Palsule wrote:
> Hello Cedric,
>
>
> On 10/24/23 10:21, Cédric Le Goater wrote:
> > On 10/24/23 17:00, Ninad Palsule wrote:
> > > Hello Cedric,
> > >
> > > On 10/24/23 02:46, Cédric Le Goater wrote:
> > > > and the fsi_opb_* routines are useless to me.
> > > We are trying to keep the separation between OPB implementation and
> > > interface hence we have all those fsi_opb_*. I feel that we should
> > > keep as it is so that future extensions will be easier. Please let me
> > > know.
> >
> > Well, I can't really tell because I don't know enough about FSI :/
> >
> > The models look fragile and I have spent already a lot of time trying
> > to untangle what they are trying to do. Please ask your teammates or
> > let's see in the next QEMU cycle.
>
> I have decided to go with the approach you suggested and it looks much
> better. Fixed it.
I intended to reply to this before Ninad sent out v7, but life
intervened.
If we can't justify it with the code we have now I think it's right to
pull it out. Add the code to support the things we're trying to do when
we need to do them. As long as we don't do anything that precludes us
from adding that code later (and I can't really imagine how we'd corner
ourselves like that).
We should bear in mind I wrote the initial models several years ago in
the space of about a week while I was trying to learn FSI (and more
deeply about the QEMU bus and address space modelling). I think I was
doing that to unblock some CI due to the introduction of the kernel
driver for the Aspeed FSI hardware. The models were pretty rough -
prior to all this review the code reflected my hazy understanding of
the problems. I didn't get time to remove the complexities introduced
by my misunderstandings, and now it's been so long that I'm not much
help with fixing them.
Andrew
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 07/10] hw/arm: Hook up FSI module in AST2600
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (5 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-23 15:03 ` Philippe Mathieu-Daudé
2023-10-21 21:17 ` [PATCH v6 08/10] hw/fsi: Added qtest Ninad Palsule
` (2 subsequent siblings)
9 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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: Ninad Palsule <ninad@linux.ibm.com>
---
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 8adff70072..db3ba3abc7 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/fsi/aspeed-apb2opb.h"
#include "hw/char/serial.h"
#define ASPEED_SPIS_NUM 2
@@ -96,6 +97,7 @@ struct AspeedSoCState {
UnimplementedDeviceState udc;
UnimplementedDeviceState sgpiom;
UnimplementedDeviceState jtag[ASPEED_JTAG_NUM];
+ AspeedAPB2OPBState fsi[2];
};
#define TYPE_ASPEED_SOC "aspeed-soc"
@@ -191,6 +193,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 e122e1c32d..2ab6260955 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 */
};
@@ -262,6 +266,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);
+ }
}
/*
@@ -622,6 +630,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] 30+ messages in thread* Re: [PATCH v6 07/10] hw/arm: Hook up FSI module in AST2600
2023-10-21 21:17 ` [PATCH v6 07/10] hw/arm: Hook up FSI module in AST2600 Ninad Palsule
@ 2023-10-23 15:03 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-23 15:03 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, clg, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, thuth, lvivier
Cc: qemu-arm, Andrew Jeffery
On 21/10/23 23:17, Ninad Palsule wrote:
> 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: Ninad Palsule <ninad@linux.ibm.com>
> ---
> include/hw/arm/aspeed_soc.h | 4 ++++
> hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
> 2 files changed, 23 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 08/10] hw/fsi: Added qtest
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (6 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 07/10] hw/arm: Hook up FSI module in AST2600 Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-23 6:51 ` Thomas Huth
2023-10-24 7:34 ` Cédric Le Goater
2023-10-21 21:17 ` [PATCH v6 09/10] hw/fsi: Added FSI documentation Ninad Palsule
2023-10-21 21:17 ` [PATCH v6 10/10] hw/fsi: Update MAINTAINER list Ninad Palsule
9 siblings, 2 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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.
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
v3:
- Added new qtest as per Cedric's comment.
V4:
- Remove MAINTAINER and documentation changes from this commit
v6:
- Incorporated review comments by Thomas Huth.
---
tests/qtest/fsi-test.c | 207 ++++++++++++++++++++++++++++++++++++++++
tests/qtest/meson.build | 1 +
2 files changed, 208 insertions(+)
create mode 100644 tests/qtest/fsi-test.c
diff --git a/tests/qtest/fsi-test.c b/tests/qtest/fsi-test.c
new file mode 100644
index 0000000000..01a0739092
--- /dev/null
+++ b/tests/qtest/fsi-test.c
@@ -0,0 +1,207 @@
+/*
+ * 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;
+
+ /* Set the base select register */
+ if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
+ aspeed_fsi_base_addr = 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) {
+ aspeed_fsi_base_addr = 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("/fsi-test/test_fsi0_master_regs", s,
+ test_fsi0_master_regs);
+
+ qtest_add_data_func("/fsi-test/test_fsi0_getcfam_addr0", s,
+ test_fsi0_getcfam_addr0);
+
+ /* Tests for OPB/FSI1 */
+ qtest_add_data_func("/fsi-test/test_fsi1_master_regs", s,
+ test_fsi1_master_regs);
+
+ qtest_add_data_func("/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 d6022ebd64..74228a4aed 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') ? ['fsi-test'] : []) + \
['arm-cpu-features',
'boot-serial-test']
--
2.39.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v6 08/10] hw/fsi: Added qtest
2023-10-21 21:17 ` [PATCH v6 08/10] hw/fsi: Added qtest Ninad Palsule
@ 2023-10-23 6:51 ` Thomas Huth
2023-10-23 15:25 ` Ninad Palsule
2023-10-24 7:34 ` Cédric Le Goater
1 sibling, 1 reply; 30+ messages in thread
From: Thomas Huth @ 2023-10-23 6:51 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, clg, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, philmd, lvivier
Cc: qemu-arm
On 21/10/2023 23.17, Ninad Palsule wrote:
> Added basic qtests for FSI model.
>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v3:
> - Added new qtest as per Cedric's comment.
> V4:
> - Remove MAINTAINER and documentation changes from this commit
> v6:
> - Incorporated review comments by Thomas Huth.
> ---
> tests/qtest/fsi-test.c | 207 ++++++++++++++++++++++++++++++++++++++++
> tests/qtest/meson.build | 1 +
> 2 files changed, 208 insertions(+)
> create mode 100644 tests/qtest/fsi-test.c
>
> diff --git a/tests/qtest/fsi-test.c b/tests/qtest/fsi-test.c
> new file mode 100644
> index 0000000000..01a0739092
> --- /dev/null
> +++ b/tests/qtest/fsi-test.c
> @@ -0,0 +1,207 @@
> +/*
> + * 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;
> +
> + /* Set the base select register */
> + if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
> + aspeed_fsi_base_addr = 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) {
> + aspeed_fsi_base_addr = base_addr;
You could move "aspeed_fsi_base_addr = base_addr" before the if-statement,
so that you don't have to repeat it in both cases.
> + /* 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();
> + }
> +}
Anyway:
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v6 08/10] hw/fsi: Added qtest
2023-10-23 6:51 ` Thomas Huth
@ 2023-10-23 15:25 ` Ninad Palsule
0 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-23 15:25 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, clg, peter.maydell, andrew, joel,
pbonzini, marcandre.lureau, berrange, philmd, lvivier
Cc: qemu-arm
Hello Thomas,
On 10/23/23 01:51, Thomas Huth wrote:
> On 21/10/2023 23.17, Ninad Palsule wrote:
>> Added basic qtests for FSI model.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v3:
>> - Added new qtest as per Cedric's comment.
>> V4:
>> - Remove MAINTAINER and documentation changes from this commit
>> v6:
>> - Incorporated review comments by Thomas Huth.
>> ---
>> tests/qtest/fsi-test.c | 207 ++++++++++++++++++++++++++++++++++++++++
>> tests/qtest/meson.build | 1 +
>> 2 files changed, 208 insertions(+)
>> create mode 100644 tests/qtest/fsi-test.c
>>
>> diff --git a/tests/qtest/fsi-test.c b/tests/qtest/fsi-test.c
>> new file mode 100644
>> index 0000000000..01a0739092
>> --- /dev/null
>> +++ b/tests/qtest/fsi-test.c
>> @@ -0,0 +1,207 @@
>> +/*
>> + * 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;
>> +
>> + /* Set the base select register */
>> + if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
>> + aspeed_fsi_base_addr = 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) {
>> + aspeed_fsi_base_addr = base_addr;
>
> You could move "aspeed_fsi_base_addr = base_addr" before the
> if-statement, so that you don't have to repeat it in both cases.
Fixed it. I will update it in version 7.
Thanks for the review.
Regards,
Ninad
>
>> + /* 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();
>> + }
>> +}
>
> Anyway:
> Acked-by: Thomas Huth <thuth@redhat.com>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 08/10] hw/fsi: Added qtest
2023-10-21 21:17 ` [PATCH v6 08/10] hw/fsi: Added qtest Ninad Palsule
2023-10-23 6:51 ` Thomas Huth
@ 2023-10-24 7:34 ` Cédric Le Goater
2023-10-26 15:30 ` Ninad Palsule
1 sibling, 1 reply; 30+ messages in thread
From: Cédric Le Goater @ 2023-10-24 7:34 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
On 10/21/23 23:17, Ninad Palsule wrote:
> Added basic qtests for FSI model.
>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v3:
> - Added new qtest as per Cedric's comment.
> V4:
> - Remove MAINTAINER and documentation changes from this commit
> v6:
> - Incorporated review comments by Thomas Huth.
> ---
> tests/qtest/fsi-test.c | 207 ++++++++++++++++++++++++++++++++++++++++
please rename the file to aspeed-fsi-test.c
Thanks,
C.
> tests/qtest/meson.build | 1 +
> 2 files changed, 208 insertions(+)
> create mode 100644 tests/qtest/fsi-test.c
>
> diff --git a/tests/qtest/fsi-test.c b/tests/qtest/fsi-test.c
> new file mode 100644
> index 0000000000..01a0739092
> --- /dev/null
> +++ b/tests/qtest/fsi-test.c
> @@ -0,0 +1,207 @@
> +/*
> + * 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;
> +
> + /* Set the base select register */
> + if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
> + aspeed_fsi_base_addr = 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) {
> + aspeed_fsi_base_addr = 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("/fsi-test/test_fsi0_master_regs", s,
> + test_fsi0_master_regs);
> +
> + qtest_add_data_func("/fsi-test/test_fsi0_getcfam_addr0", s,
> + test_fsi0_getcfam_addr0);
> +
> + /* Tests for OPB/FSI1 */
> + qtest_add_data_func("/fsi-test/test_fsi1_master_regs", s,
> + test_fsi1_master_regs);
> +
> + qtest_add_data_func("/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 d6022ebd64..74228a4aed 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') ? ['fsi-test'] : []) + \
> ['arm-cpu-features',
> 'boot-serial-test']
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v6 08/10] hw/fsi: Added qtest
2023-10-24 7:34 ` Cédric Le Goater
@ 2023-10-26 15:30 ` Ninad Palsule
0 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-26 15: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 10/24/23 02:34, Cédric Le Goater wrote:
> On 10/21/23 23:17, Ninad Palsule wrote:
>> Added basic qtests for FSI model.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v3:
>> - Added new qtest as per Cedric's comment.
>> V4:
>> - Remove MAINTAINER and documentation changes from this commit
>> v6:
>> - Incorporated review comments by Thomas Huth.
>> ---
>> tests/qtest/fsi-test.c | 207 ++++++++++++++++++++++++++++++++++++++++
>
> please rename the file to aspeed-fsi-test.c
Renamed it.
Thanks for the review.
Regards,
Ninad
>
> Thanks,
>
> C.
>
>
>> tests/qtest/meson.build | 1 +
>> 2 files changed, 208 insertions(+)
>> create mode 100644 tests/qtest/fsi-test.c
>>
>> diff --git a/tests/qtest/fsi-test.c b/tests/qtest/fsi-test.c
>> new file mode 100644
>> index 0000000000..01a0739092
>> --- /dev/null
>> +++ b/tests/qtest/fsi-test.c
>> @@ -0,0 +1,207 @@
>> +/*
>> + * 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;
>> +
>> + /* Set the base select register */
>> + if (base_addr == AST2600_OPB_FSI0_BASE_ADDR) {
>> + aspeed_fsi_base_addr = 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) {
>> + aspeed_fsi_base_addr = 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("/fsi-test/test_fsi0_master_regs", s,
>> + test_fsi0_master_regs);
>> +
>> + qtest_add_data_func("/fsi-test/test_fsi0_getcfam_addr0", s,
>> + test_fsi0_getcfam_addr0);
>> +
>> + /* Tests for OPB/FSI1 */
>> + qtest_add_data_func("/fsi-test/test_fsi1_master_regs", s,
>> + test_fsi1_master_regs);
>> +
>> + qtest_add_data_func("/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 d6022ebd64..74228a4aed 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') ?
>> ['fsi-test'] : []) + \
>> ['arm-cpu-features',
>> 'boot-serial-test']
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 09/10] hw/fsi: Added FSI documentation
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (7 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 08/10] hw/fsi: Added qtest Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
2023-10-24 7:37 ` Cédric Le Goater
2023-10-21 21:17 ` [PATCH v6 10/10] hw/fsi: Update MAINTAINER list Ninad Palsule
9 siblings, 1 reply; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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: Ninad Palsule <ninad@linux.ibm.com>
---
v4:
- Added separate commit for documentation
---
docs/specs/fsi.rst | 141 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 141 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..73b082afe1
--- /dev/null
+++ b/docs/specs/fsi.rst
@@ -0,0 +1,141 @@
+======================================
+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:
+
+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
+
+Refer following documents for more details.
+
+.. _FSI specification:
+ https://openpowerfoundation.org/specifications/fsi/
+ https://wiki.raptorcs.com/w/images/9/97/OpenFSI-spec-20161212.pdf
+
+.. _pdbg source repository:
+ https://github.com/open-power/pdbg
--
2.39.2
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v6 09/10] hw/fsi: Added FSI documentation
2023-10-21 21:17 ` [PATCH v6 09/10] hw/fsi: Added FSI documentation Ninad Palsule
@ 2023-10-24 7:37 ` Cédric Le Goater
2023-10-26 15:32 ` Ninad Palsule
0 siblings, 1 reply; 30+ messages in thread
From: Cédric Le Goater @ 2023-10-24 7:37 UTC (permalink / raw)
To: Ninad Palsule, qemu-devel, peter.maydell, andrew, joel, pbonzini,
marcandre.lureau, berrange, thuth, philmd, lvivier
Cc: qemu-arm
On 10/21/23 23:17, Ninad Palsule wrote:
> Documentation for IBM FSI model.
>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
> v4:
> - Added separate commit for documentation
> ---
> docs/specs/fsi.rst | 141 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
> create mode 100644 docs/specs/fsi.rst
Documentation build is broken.
a 'fsi" entry should be added in docs/specs/index.rst. More below.
> diff --git a/docs/specs/fsi.rst b/docs/specs/fsi.rst
> new file mode 100644
> index 0000000000..73b082afe1
> --- /dev/null
> +++ b/docs/specs/fsi.rst
> @@ -0,0 +1,141 @@
> +======================================
> +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:
> +
> +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)
+ from the BMC. (see the ``pdbg source repository`` for more details)
Please check before sending.
Thanks,
C.
> +
> +.. code-block:: console
> +
> + root@p10bmc:~# pdbg -a getcfam 0x0
> + p0: 0x0 = 0xc0022d15
> +
> +Refer following documents for more details.
> +
> +.. _FSI specification:
> + https://openpowerfoundation.org/specifications/fsi/
> + https://wiki.raptorcs.com/w/images/9/97/OpenFSI-spec-20161212.pdf
> +
> +.. _pdbg source repository:
> + https://github.com/open-power/pdbg
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 09/10] hw/fsi: Added FSI documentation
2023-10-24 7:37 ` Cédric Le Goater
@ 2023-10-26 15:32 ` Ninad Palsule
0 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-26 15:32 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 10/24/23 02:37, Cédric Le Goater wrote:
> On 10/21/23 23:17, Ninad Palsule wrote:
>> Documentation for IBM FSI model.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>> v4:
>> - Added separate commit for documentation
>> ---
>> docs/specs/fsi.rst | 141 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 141 insertions(+)
>> create mode 100644 docs/specs/fsi.rst
>
>
> Documentation build is broken.
>
> a 'fsi" entry should be added in docs/specs/index.rst. More below.
Sorry about that. Added entry in the index.rst
>
>
>
>> diff --git a/docs/specs/fsi.rst b/docs/specs/fsi.rst
>> new file mode 100644
>> index 0000000000..73b082afe1
>> --- /dev/null
>> +++ b/docs/specs/fsi.rst
>> @@ -0,0 +1,141 @@
>> +======================================
>> +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:
>> +
>> +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)
>
> + from the BMC. (see the ``pdbg source repository`` for more details)
>
> Please check before sending.
Sorry about that. Fixed it. Checked it using "rstcheck" tool and also
through web browser.
Thanks for the review.
Regards,
Ninad
>
> Thanks,
>
> C.
>
>
>> +
>> +.. code-block:: console
>> +
>> + root@p10bmc:~# pdbg -a getcfam 0x0
>> + p0: 0x0 = 0xc0022d15
>> +
>> +Refer following documents for more details.
>> +
>> +.. _FSI specification:
>> + https://openpowerfoundation.org/specifications/fsi/
>> + https://wiki.raptorcs.com/w/images/9/97/OpenFSI-spec-20161212.pdf
>> +
>> +.. _pdbg source repository:
>> + https://github.com/open-power/pdbg
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 10/10] hw/fsi: Update MAINTAINER list
2023-10-21 21:17 [PATCH v6 00/10] Introduce model for IBM's FSI Ninad Palsule
` (8 preceding siblings ...)
2023-10-21 21:17 ` [PATCH v6 09/10] hw/fsi: Added FSI documentation Ninad Palsule
@ 2023-10-21 21:17 ` Ninad Palsule
9 siblings, 0 replies; 30+ messages in thread
From: Ninad Palsule @ 2023-10-21 21:17 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: Ninad Palsule <ninad@linux.ibm.com>
---
V4:
- Added separate commit for MAINTAINER change.
V5:
- Use * instead of listing all files in dir
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f9912baa0..381923c937 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3476,6 +3476,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] 30+ messages in thread