Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave
Hello Cedric, On 8/29/23 08:43, Cédric Le Goater wrote: On 8/29/23 15:39, Ninad Palsule wrote: Hello Thomas, On 8/28/23 21:03, Thomas Huth wrote: On 25/08/2023 22.30, 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 --- ... diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c new file mode 100644 index 00..19256050bd --- /dev/null +++ b/hw/fsi/cfam.c @@ -0,0 +1,235 @@ +/* + * 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 "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); + + qemu_log_mask(LOG_UNIMP, "%s: read @0x%" HWADDR_PRIx " size=%d\n", + __func__, 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; + } + + return 0xc0de; Can you explain the magic number at least with a comment? Added comment for the magic number 0xc0de Maybe it would also make sense to add a qemu_log_mask(LOG_GUEST_ERROR, ...) or qemu_log_mask(LOG_UNIMP, ...) statement here? There is LOG_UNIMP most of the function. I added it in the reset function. I took a quick look at the series and I think that all the qemu_log_mask(LOG_UNIMP, ..) as the one above should be replaced by trace events instead. I have converted 4 logs into trace. Thank you for the review. ~Ninad Thanks, C. Thomas Thank you for the review. Ninad
Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave
On 8/29/23 15:39, Ninad Palsule wrote: Hello Thomas, On 8/28/23 21:03, Thomas Huth wrote: On 25/08/2023 22.30, 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 --- ... diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c new file mode 100644 index 00..19256050bd --- /dev/null +++ b/hw/fsi/cfam.c @@ -0,0 +1,235 @@ +/* + * 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 "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); + + qemu_log_mask(LOG_UNIMP, "%s: read @0x%" HWADDR_PRIx " size=%d\n", + __func__, 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; + } + + return 0xc0de; Can you explain the magic number at least with a comment? Added comment for the magic number 0xc0de Maybe it would also make sense to add a qemu_log_mask(LOG_GUEST_ERROR, ...) or qemu_log_mask(LOG_UNIMP, ...) statement here? There is LOG_UNIMP most of the function. I added it in the reset function. I took a quick look at the series and I think that all the qemu_log_mask(LOG_UNIMP, ..) as the one above should be replaced by trace events instead. Thanks, C. Thomas Thank you for the review. Ninad
Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave
Hello Thomas, On 8/28/23 21:03, Thomas Huth wrote: On 25/08/2023 22.30, 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 --- ... diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c new file mode 100644 index 00..19256050bd --- /dev/null +++ b/hw/fsi/cfam.c @@ -0,0 +1,235 @@ +/* + * 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 "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); + + qemu_log_mask(LOG_UNIMP, "%s: read @0x%" HWADDR_PRIx " size=%d\n", + __func__, 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; + } + + return 0xc0de; Can you explain the magic number at least with a comment? Added comment for the magic number 0xc0de Maybe it would also make sense to add a qemu_log_mask(LOG_GUEST_ERROR, ...) or qemu_log_mask(LOG_UNIMP, ...) statement here? There is LOG_UNIMP most of the function. I added it in the reset function. Thomas Thank you for the review. Ninad
Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave
On 25/08/2023 22.30, 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 --- ... diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c new file mode 100644 index 00..19256050bd --- /dev/null +++ b/hw/fsi/cfam.c @@ -0,0 +1,235 @@ +/* + * 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 "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); + +qemu_log_mask(LOG_UNIMP, "%s: read @0x%" HWADDR_PRIx " size=%d\n", + __func__, 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; +} + +return 0xc0de; Can you explain the magic number at least with a comment? Maybe it would also make sense to add a qemu_log_mask(LOG_GUEST_ERROR, ...) or qemu_log_mask(LOG_UNIMP, ...) statement here? Thomas
[PATCH v1 3/7] 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 --- hw/fsi/Kconfig | 9 ++ hw/fsi/cfam.c | 235 + hw/fsi/fsi-slave.c | 109 + hw/fsi/meson.build | 2 + include/hw/fsi/cfam.h | 61 ++ include/hw/fsi/fsi-slave.h | 29 + 6 files changed, 445 insertions(+) create mode 100644 hw/fsi/cfam.c create mode 100644 hw/fsi/fsi-slave.c 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..19256050bd --- /dev/null +++ b/hw/fsi/cfam.c @@ -0,0 +1,235 @@ +/* + * 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 "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); + +qemu_log_mask(LOG_UNIMP, "%s: read @0x%" HWADDR_PRIx " size=%d\n", + __func__, 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; +} + +return 0xc0de; +} +} + +static void cfam_config_write(void *opaque, hwaddr addr, uint64_t data, + unsigned size) +{ +CFAMConfig *s = CFAM_CONFIG(opaque); + +qemu_log_mask(LOG_UNIMP, "%s: write @0x%" HWADDR_PRIx " size=%d " + "value=%"PRIx64"\n", __func__, 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); + +memory_region_init_io(>iomem, OBJECT(s), _config_ops, s, + TYPE_CFAM_CONFIG, 0x400); +} + +static void cfam_config_reset(DeviceState *dev) +{ +/* Config is read-only */ +} + +static void cfam_config_class_init(ObjectClass