On Thu, 2026-07-30 at 15:09 -0500, Mikail Sadic wrote:
> Prepare a common CFAM parent class for CFAM-S implementation.
>
> Add an abstract TYPE_FSI_CFAM_COMMON with an FSICFAMCommonClass holding
> what varies between types: the config table, the offsets of the FSI
> responder and the local bus within the register slot, and a
> realize_engines() hook for the engines on the local bus. Its realize is
> driven by those fields, so one realize serves every type. TYPE_FSI_CFAM
> keeps its name and becomes the first derived type, supplying the P9
> config table, its slot offsets and the scratchpad engine. The config
> table becomes data, still written with the same CFAM_CONFIG_REG()
> expressions, which move to the header so a derived type can describe its
> own table the same way.
>
> No functional change: same memory map, same config words, same type and
> child names. aspeed_fsi-test verified.
>
> Signed-off-by: Mikail Sadic <[email protected]>
> ---
> include/hw/fsi/cfam.h | 58 ++++++++++++++++++--
> hw/fsi/cfam.c | 123 ++++++++++++++++++++++++------------------
> hw/fsi/fsi-master.c | 2 +-
> 3 files changed, 127 insertions(+), 56 deletions(-)
>
> diff --git a/include/hw/fsi/cfam.h b/include/hw/fsi/cfam.h
> index cceb4bd6f1..0f5464b4a0 100644
> --- a/include/hw/fsi/cfam.h
> +++ b/include/hw/fsi/cfam.h
> @@ -7,18 +7,44 @@
> #ifndef FSI_CFAM_H
> #define FSI_CFAM_H
>
> +#include "qemu/units.h"
> #include "system/memory.h"
>
> #include "hw/fsi/fsi.h"
> #include "hw/fsi/lbus.h"
>
> +/*
> + * All CFAM flavors present a register slot holding a config table, an FSI
> + * responder and a local bus carrying the engines the config table describes.
> + * That is the common model; each flavor supplies its own table, slot layout
> + * and engines.
> + */
> +#define TYPE_FSI_CFAM_COMMON "cfam-common"
> +OBJECT_DECLARE_TYPE(FSICFAMCommonState, FSICFAMCommonClass, FSI_CFAM_COMMON)
I would prefer it if you could drop the `State` suffix from the name.
We probably don't need the `FSI` prefix either since I believe there
are only FSI-based CFAMs, but it's already there from before so it's
fine for now.
> +
> #define TYPE_FSI_CFAM "cfam"
> -#define FSI_CFAM(obj) OBJECT_CHECK(FSICFAMState, (obj), TYPE_FSI_CFAM)
> +OBJECT_DECLARE_SIMPLE_TYPE(FSICFAMState, FSI_CFAM)
>
> /* P9-ism */
> #define CFAM_CONFIG_NR_REGS 0x28
>
> -typedef struct FSICFAMState {
> +#define FSI_CFAM_SLOT_SIZE (2 * MiB)
> +#define FSI_CFAM_CONFIG_SIZE 0x400
> +
> +#define ENGINE_CONFIG_NEXT BIT(31)
> +#define ENGINE_CONFIG_TYPE_PEEK (0x02 << 4)
> +#define ENGINE_CONFIG_TYPE_FSI (0x03 << 4)
> +#define ENGINE_CONFIG_TYPE_SCRATCHPAD (0x06 << 4)
> +
> +/* Valid, slots, version, type, crc */
> +#define CFAM_CONFIG_REG(__VER, __TYPE, __CRC) \
> + (ENGINE_CONFIG_NEXT | \
> + 0x00010000 | \
> + (__VER) | \
> + (__TYPE) | \
> + (__CRC))
> +
> +struct FSICFAMCommonState {
> /* < private > */
> FSISlaveState parent;
>
> @@ -28,7 +54,33 @@ typedef struct FSICFAMState {
> MemoryRegion mr;
>
> FSILBus lbus;
> +};
> +
> +struct FSICFAMCommonClass {
> + /* < private > */
> + DeviceClass parent_class;
> +
> + /* < public > */
> + /* Config table served by the common ops, one word per 4-byte offset */
> + const uint32_t *config;
> + unsigned config_nr;
> +
> + /* Layout of the register slot */
> + hwaddr responder_offset;
> + hwaddr lbus_offset;
> +
> + /* Realize and map this flavor's local bus engines */
> + bool (*realize_engines)(FSICFAMCommonState *cfam, Error **errp);
> +};
> +
> +struct FSICFAMState {
> + /* < private > */
> + FSICFAMCommonState parent;
> +
> FSIScratchPad scratchpad;
> -} FSICFAMState;
> +};
> +
> +bool fsi_cfam_add_engine(FSICFAMCommonState *cfam, DeviceState *engine,
> + hwaddr offset, Error **errp);
>
> #endif /* FSI_CFAM_H */
> diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
> index 54c0b05769..361cb34808 100644
> --- a/hw/fsi/cfam.c
> +++ b/hw/fsi/cfam.c
> @@ -16,19 +16,6 @@
>
> #include "hw/core/qdev-properties.h"
>
> -#define ENGINE_CONFIG_NEXT BIT(31)
> -#define ENGINE_CONFIG_TYPE_PEEK (0x02 << 4)
> -#define ENGINE_CONFIG_TYPE_FSI (0x03 << 4)
> -#define ENGINE_CONFIG_TYPE_SCRATCHPAD (0x06 << 4)
> -
> -/* Valid, slots, version, type, crc */
> -#define CFAM_CONFIG_REG(__VER, __TYPE, __CRC) \
> - (ENGINE_CONFIG_NEXT | \
> - 0x00010000 | \
> - (__VER) | \
> - (__TYPE) | \
> - (__CRC))
> -
> #define TO_REG(x) ((x) >> 2)
>
> #define CFAM_CONFIG_CHIP_ID TO_REG(0x00)
> @@ -36,34 +23,33 @@
> #define CFAM_CONFIG_CHIP_ID_P9 0xc0022d15
> #define CFAM_CONFIG_CHIP_ID_BREAK 0xc0de0000
>
> +/*
> + * Config table of the P9 CFAM: the chip ID followed by one entry per engine,
> + * entry n describing the engine at address n * 4. We need to add future
> + * engines from address 0x10 onwards.
> + */
> +static const uint32_t cfam_p9_config[] = {
> + CFAM_CONFIG_CHIP_ID_P9,
> + CFAM_CONFIG_REG(0x1000, ENGINE_CONFIG_TYPE_PEEK, 0xc),
> + CFAM_CONFIG_REG(0x5000, ENGINE_CONFIG_TYPE_FSI, 0xa),
> + CFAM_CONFIG_REG(0x1000, ENGINE_CONFIG_TYPE_SCRATCHPAD, 0x7),
> +};
> +
> static uint64_t fsi_cfam_config_read(void *opaque, hwaddr addr, unsigned
> size)
> {
> + FSICFAMCommonClass *cc = FSI_CFAM_COMMON_GET_CLASS(opaque);
> + unsigned int reg = TO_REG(addr);
> +
> trace_fsi_cfam_config_read(addr, size);
>
> - switch (addr) {
> - case 0x00:
> - return CFAM_CONFIG_CHIP_ID_P9;
> - case 0x04:
> - return CFAM_CONFIG_REG(0x1000, ENGINE_CONFIG_TYPE_PEEK, 0xc);
> - case 0x08:
> - return CFAM_CONFIG_REG(0x5000, ENGINE_CONFIG_TYPE_FSI, 0xa);
> - case 0xc:
> - return CFAM_CONFIG_REG(0x1000, ENGINE_CONFIG_TYPE_SCRATCHPAD, 0x7);
> - default:
> - /*
> - * The config table contains different engines from 0xc onwards.
> - * The scratch pad is already added at address 0xc. We need to add
> - * future engines from address 0x10 onwards. Returning 0 as engine
> - * is not implemented.
> - */
> - return 0;
> - }
> + /* Engines past the end of the table are not implemented */
> + return reg < cc->config_nr ? cc->config[reg] : 0;
> }
>
> static void fsi_cfam_config_write(void *opaque, hwaddr addr, uint64_t data,
> unsigned size)
> {
> - FSICFAMState *cfam = FSI_CFAM(opaque);
> + FSICFAMCommonState *cfam = FSI_CFAM_COMMON(opaque);
>
> trace_fsi_cfam_config_write(addr, size, data);
>
> @@ -109,59 +95,92 @@ static const struct MemoryRegionOps
> fsi_cfam_unimplemented_ops = {
> .endianness = DEVICE_BIG_ENDIAN,
> };
>
> -static void fsi_cfam_instance_init(Object *obj)
> +bool fsi_cfam_add_engine(FSICFAMCommonState *cfam, DeviceState *engine,
> + hwaddr offset, Error **errp)
> {
> - FSICFAMState *s = FSI_CFAM(obj);
> + if (!qdev_realize(engine, BUS(&cfam->lbus), errp)) {
> + return false;
> + }
>
> - object_initialize_child(obj, "scratchpad", &s->scratchpad,
> - TYPE_FSI_SCRATCHPAD);
> + memory_region_add_subregion(&cfam->lbus.mr, offset,
> + &FSI_LBUS_DEVICE(engine)->iomem);
> + return true;
> }
>
> -static void fsi_cfam_realize(DeviceState *dev, Error **errp)
> +static void fsi_cfam_common_realize(DeviceState *dev, Error **errp)
> {
> - FSICFAMState *cfam = FSI_CFAM(dev);
> + FSICFAMCommonState *cfam = FSI_CFAM_COMMON(dev);
> + FSICFAMCommonClass *cc = FSI_CFAM_COMMON_GET_CLASS(dev);
> FSISlaveState *slave = FSI_SLAVE(dev);
> + const char *type = object_get_typename(OBJECT(dev));
> + g_autofree char *config_name = g_strdup_printf("%s.config", type);
>
> /* Each slave has a 2MiB address space */
> memory_region_init_io(&cfam->mr, OBJECT(cfam),
> &fsi_cfam_unimplemented_ops,
> - cfam, TYPE_FSI_CFAM, 2 * MiB);
> + cfam, type, FSI_CFAM_SLOT_SIZE);
There is now an extended addressing mode that allows a single CFAM to
have 8MB instead of having 4 cascaded CFAM's sharing the 8MB space, but
I understand if you don't want to implement this now.
>
> qbus_init(&cfam->lbus, sizeof(cfam->lbus), TYPE_FSI_LBUS, DEVICE(cfam),
> NULL);
>
> memory_region_init_io(&cfam->config_iomem, OBJECT(cfam),
> &cfam_config_ops,
> - cfam, TYPE_FSI_CFAM ".config", 0x400);
> + cfam, config_name, FSI_CFAM_CONFIG_SIZE);
>
> 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);
> + memory_region_add_subregion(&cfam->mr, cc->responder_offset,
> &slave->iomem);
> + memory_region_add_subregion(&cfam->mr, cc->lbus_offset, &cfam->lbus.mr);
> +
> + cc->realize_engines(cfam, errp);
> +}
> +
> +static void fsi_cfam_common_class_init(ObjectClass *klass, const void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->bus_type = TYPE_FSI_BUS;
> + dc->realize = fsi_cfam_common_realize;
> +}
> +
> +static bool fsi_cfam_realize_engines(FSICFAMCommonState *cfam, Error **errp)
> +{
> + FSICFAMState *s = FSI_CFAM(cfam);
>
> /* Add scratchpad engine */
> - if (!qdev_realize(DEVICE(&cfam->scratchpad), BUS(&cfam->lbus), errp)) {
> - return;
> - }
> + object_initialize_child(OBJECT(s), "scratchpad", &s->scratchpad,
> + TYPE_FSI_SCRATCHPAD);
>
> - FSILBusDevice *fsi_dev = FSI_LBUS_DEVICE(&cfam->scratchpad);
> - memory_region_add_subregion(&cfam->lbus.mr, 0, &fsi_dev->iomem);
> + return fsi_cfam_add_engine(cfam, DEVICE(&s->scratchpad), 0, errp);
> }
>
> static void fsi_cfam_class_init(ObjectClass *klass, const void *data)
> {
> - DeviceClass *dc = DEVICE_CLASS(klass);
> - dc->bus_type = TYPE_FSI_BUS;
> - dc->realize = fsi_cfam_realize;
> + FSICFAMCommonClass *cc = FSI_CFAM_COMMON_CLASS(klass);
> +
> + cc->config = cfam_p9_config;
> + cc->config_nr = ARRAY_SIZE(cfam_p9_config);
> + cc->responder_offset = 0x800;
> + cc->lbus_offset = 0xc00;
> + cc->realize_engines = fsi_cfam_realize_engines;
> }
>
> +static const TypeInfo fsi_cfam_common_info = {
> + .name = TYPE_FSI_CFAM_COMMON,
> + .parent = TYPE_FSI_SLAVE,
> + .instance_size = sizeof(FSICFAMCommonState),
> + .class_size = sizeof(FSICFAMCommonClass),
> + .class_init = fsi_cfam_common_class_init,
> + .abstract = true,
> +};
> +
> static const TypeInfo fsi_cfam_info = {
> .name = TYPE_FSI_CFAM,
> - .parent = TYPE_FSI_SLAVE,
> - .instance_init = fsi_cfam_instance_init,
> + .parent = TYPE_FSI_CFAM_COMMON,
> .instance_size = sizeof(FSICFAMState),
> .class_init = fsi_cfam_class_init,
> };
>
> static void fsi_cfam_register_types(void)
> {
> + type_register_static(&fsi_cfam_common_info);
> type_register_static(&fsi_cfam_info);
> }
>
> diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c
> index 083a5507ab..d82df1c094 100644
> --- a/hw/fsi/fsi-master.c
> +++ b/hw/fsi/fsi-master.c
> @@ -130,7 +130,7 @@ static void fsi_master_realize(DeviceState *dev, Error
> **errp)
> }
>
> /* address ? */
> - memory_region_add_subregion(&s->opb2fsi, 0, &s->cfam.mr);
> + memory_region_add_subregion(&s->opb2fsi, 0, &s->cfam.parent.mr);
> }
>
> static void fsi_master_reset(DeviceState *dev)
