Re: [PATCH v4 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave
Hello Cedric,
Thanks for the review.
On 9/11/23 07:19, Cédric Le Goater wrote:
On 9/9/23 00:28, Ninad Palsule wrote:
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
The Common FRU Access Macro (CFAM), an address space containing
various "engines" that drive accesses on busses internal and external
to the POWER chip. Examples include the SBEFIFO and I2C masters. The
engines hang off of an internal Local Bus (LBUS) which is described
by the CFAM configuration block.
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
Signed-off-by: Cédric Le Goater
Signed-off-by: Ninad Palsule
---
v2:
- Incorporated Joel's review comments.
v3:
- Incorporated Thomas Huth's review comments.
---
hw/fsi/Kconfig | 9 ++
hw/fsi/cfam.c | 238 +
hw/fsi/fsi-slave.c | 109 +
hw/fsi/meson.build | 2 +
hw/fsi/trace-events | 5 +
include/hw/fsi/cfam.h | 61 ++
include/hw/fsi/fsi-slave.h | 29 +
7 files changed, 453 insertions(+)
create mode 100644 hw/fsi/cfam.c
create mode 100644 hw/fsi/fsi-slave.c
create mode 100644 hw/fsi/trace-events
create mode 100644 include/hw/fsi/cfam.h
create mode 100644 include/hw/fsi/fsi-slave.h
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index 2a9c49f2c9..087980be22 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,3 +1,12 @@
+config CFAM
+ bool
+ select FSI
+ select SCRATCHPAD
+ select LBUS
+
+config FSI
+ bool
+
config SCRATCHPAD
bool
select LBUS
diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
new file mode 100644
index 00..9a9e65d33f
--- /dev/null
+++ b/hw/fsi/cfam.c
@@ -0,0 +1,238 @@
+/*
+ * 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 "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/bits.h"
+#include "hw/fsi/cfam.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 0xc0de
+
+static uint64_t cfam_config_read(void *opaque, hwaddr addr, unsigned
size)
+{
+ CFAMConfig *config;
+ CFAMState *cfam;
+ LBusNode *node;
+ int i;
+
+ config = CFAM_CONFIG(opaque);
+ cfam = container_of(config, CFAMState, config);
+
+ trace_cfam_config_read(addr, size);
+
+ assert(size == 4);
+ assert(!(addr & 3));
These checks are useless if the MemoryRegionOps is defined accordingly.
Good suggestion. Updated MemoryRegionOps registration and removed these
asserts.
+
+ switch (addr) {
+ case 0x00:
+ return CFAM_CONFIG_CHIP_ID_P9;
+ case 0x04:
+ return ENGINE_CONFIG_NEXT
+ | 0x0001 /* slots */
+ | 0x1000 /* version */
+ | ENGINE_CONFIG_TYPE_PEEK /* type */
+ | 0x000c; /* crc */
+ case 0x08:
+ return ENGINE_CONFIG_NEXT
+ | 0x0001 /* slots */
+ | 0x5000 /* version */
+ | ENGINE_CONFIG_TYPE_FSI /* type */
+ | 0x000a; /* crc */
+ break;
+ default:
+ /* FIXME: Improve this */
This looks hacky. What is it suppose to do ?
Yes, This is kind of work in progress. Looks like it is expecting
address 0xc onwards devices and if we find any device then it will
return config data for that device otherwise its a bad things and sends
a break to reset.
+ i = 0xc;
+ QLIST_FOREACH(node, >lbus.devices, next) {
+ if (i == addr) {
+ return LBUS_DEVICE_GET_CLASS(node->ldev)->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 0xc0de;
we could use a definition.
Fixed
+ }
+}
+
+static void cfam_config_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+ CFAMConfig *s = CFAM_CONFIG(opaque);
+
+ trace_cfam_config_write(addr, size, data);
+
+ assert(size == 4);
+ assert(!(addr & 3));
These checks are useless if the MemoryRegionOps is defined accordingly.
Removed.
Re: [PATCH v4 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave
On 9/9/23 00:28, Ninad Palsule wrote:
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
The Common FRU Access Macro (CFAM), an address space containing
various "engines" that drive accesses on busses internal and external
to the POWER chip. Examples include the SBEFIFO and I2C masters. The
engines hang off of an internal Local Bus (LBUS) which is described
by the CFAM configuration block.
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
Signed-off-by: Cédric Le Goater
Signed-off-by: Ninad Palsule
---
v2:
- Incorporated Joel's review comments.
v3:
- Incorporated Thomas Huth's review comments.
---
hw/fsi/Kconfig | 9 ++
hw/fsi/cfam.c | 238 +
hw/fsi/fsi-slave.c | 109 +
hw/fsi/meson.build | 2 +
hw/fsi/trace-events| 5 +
include/hw/fsi/cfam.h | 61 ++
include/hw/fsi/fsi-slave.h | 29 +
7 files changed, 453 insertions(+)
create mode 100644 hw/fsi/cfam.c
create mode 100644 hw/fsi/fsi-slave.c
create mode 100644 hw/fsi/trace-events
create mode 100644 include/hw/fsi/cfam.h
create mode 100644 include/hw/fsi/fsi-slave.h
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index 2a9c49f2c9..087980be22 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,3 +1,12 @@
+config CFAM
+bool
+select FSI
+select SCRATCHPAD
+select LBUS
+
+config FSI
+bool
+
config SCRATCHPAD
bool
select LBUS
diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
new file mode 100644
index 00..9a9e65d33f
--- /dev/null
+++ b/hw/fsi/cfam.c
@@ -0,0 +1,238 @@
+/*
+ * 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 "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/bits.h"
+#include "hw/fsi/cfam.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_IDTO_REG(0x00)
+#define CFAM_CONFIG_CHIP_ID_P9 0xc0022d15
+#define CFAM_CONFIG_CHIP_ID_BREAK0xc0de
+
+static uint64_t cfam_config_read(void *opaque, hwaddr addr, unsigned size)
+{
+CFAMConfig *config;
+CFAMState *cfam;
+LBusNode *node;
+int i;
+
+config = CFAM_CONFIG(opaque);
+cfam = container_of(config, CFAMState, config);
+
+trace_cfam_config_read(addr, size);
+
+assert(size == 4);
+assert(!(addr & 3));
These checks are useless if the MemoryRegionOps is defined accordingly.
+
+switch (addr) {
+case 0x00:
+return CFAM_CONFIG_CHIP_ID_P9;
+case 0x04:
+return ENGINE_CONFIG_NEXT
+| 0x0001/* slots */
+| 0x1000/* version */
+| ENGINE_CONFIG_TYPE_PEEK /* type */
+| 0x000c; /* crc */
+case 0x08:
+return ENGINE_CONFIG_NEXT
+| 0x0001/* slots */
+| 0x5000/* version */
+| ENGINE_CONFIG_TYPE_FSI/* type */
+| 0x000a; /* crc */
+break;
+default:
+/* FIXME: Improve this */
This looks hacky. What is it suppose to do ?
+i = 0xc;
+QLIST_FOREACH(node, >lbus.devices, next) {
+if (i == addr) {
+return LBUS_DEVICE_GET_CLASS(node->ldev)->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 0xc0de;
we could use a definition.
+}
+}
+
+static void cfam_config_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+CFAMConfig *s = CFAM_CONFIG(opaque);
+
+trace_cfam_config_write(addr, size, data);
+
+assert(size == 4);
+assert(!(addr & 3));
These checks are useless if the MemoryRegionOps is defined accordingly.
+
+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(qdev_get_parent_bus(DEVICE(s)));
+}
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Not implemented: 0x%"
+ HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+}
+}
[PATCH v4 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave
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
Signed-off-by: Cédric Le Goater
Signed-off-by: Ninad Palsule
---
v2:
- Incorporated Joel's review comments.
v3:
- Incorporated Thomas Huth's review comments.
---
hw/fsi/Kconfig | 9 ++
hw/fsi/cfam.c | 238 +
hw/fsi/fsi-slave.c | 109 +
hw/fsi/meson.build | 2 +
hw/fsi/trace-events| 5 +
include/hw/fsi/cfam.h | 61 ++
include/hw/fsi/fsi-slave.h | 29 +
7 files changed, 453 insertions(+)
create mode 100644 hw/fsi/cfam.c
create mode 100644 hw/fsi/fsi-slave.c
create mode 100644 hw/fsi/trace-events
create mode 100644 include/hw/fsi/cfam.h
create mode 100644 include/hw/fsi/fsi-slave.h
diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
index 2a9c49f2c9..087980be22 100644
--- a/hw/fsi/Kconfig
+++ b/hw/fsi/Kconfig
@@ -1,3 +1,12 @@
+config CFAM
+bool
+select FSI
+select SCRATCHPAD
+select LBUS
+
+config FSI
+bool
+
config SCRATCHPAD
bool
select LBUS
diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
new file mode 100644
index 00..9a9e65d33f
--- /dev/null
+++ b/hw/fsi/cfam.c
@@ -0,0 +1,238 @@
+/*
+ * 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 "qemu/log.h"
+#include "trace.h"
+
+#include "hw/fsi/bits.h"
+#include "hw/fsi/cfam.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_IDTO_REG(0x00)
+#define CFAM_CONFIG_CHIP_ID_P9 0xc0022d15
+#define CFAM_CONFIG_CHIP_ID_BREAK0xc0de
+
+static uint64_t cfam_config_read(void *opaque, hwaddr addr, unsigned size)
+{
+CFAMConfig *config;
+CFAMState *cfam;
+LBusNode *node;
+int i;
+
+config = CFAM_CONFIG(opaque);
+cfam = container_of(config, CFAMState, config);
+
+trace_cfam_config_read(addr, size);
+
+assert(size == 4);
+assert(!(addr & 3));
+
+switch (addr) {
+case 0x00:
+return CFAM_CONFIG_CHIP_ID_P9;
+case 0x04:
+return ENGINE_CONFIG_NEXT
+| 0x0001/* slots */
+| 0x1000/* version */
+| ENGINE_CONFIG_TYPE_PEEK /* type */
+| 0x000c; /* crc */
+case 0x08:
+return ENGINE_CONFIG_NEXT
+| 0x0001/* slots */
+| 0x5000/* version */
+| ENGINE_CONFIG_TYPE_FSI/* type */
+| 0x000a; /* crc */
+break;
+default:
+/* FIXME: Improve this */
+i = 0xc;
+QLIST_FOREACH(node, >lbus.devices, next) {
+if (i == addr) {
+return LBUS_DEVICE_GET_CLASS(node->ldev)->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 0xc0de;
+}
+}
+
+static void cfam_config_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned size)
+{
+CFAMConfig *s = CFAM_CONFIG(opaque);
+
+trace_cfam_config_write(addr, size, data);
+
+assert(size == 4);
+assert(!(addr & 3));
+
+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(qdev_get_parent_bus(DEVICE(s)));
+}
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Not implemented: 0x%"
+ HWADDR_PRIx" for %u\n",
+ __func__, addr, size);
+}
+}
+
+static const struct MemoryRegionOps cfam_config_ops = {
+.read = cfam_config_read,
+.write = cfam_config_write,
+.endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void cfam_config_realize(DeviceState *dev, Error **errp)
+{
+CFAMConfig *s = CFAM_CONFIG(dev);
+
+
