Re: [PATCH v1 3/7] hw/fsi: Introduce IBM's cfam,fsi-slave

[Thomas Huth <thuth@redhat.com>]

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 <andrew@aj.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
...
> diff --git a/hw/fsi/cfam.c b/hw/fsi/cfam.c
> new file mode 100644
> index 0000000000..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        0xc0de0000
> +
> +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
> +            | 0x00010000                    /* slots */
> +            | 0x00001000                    /* version */
> +            | ENGINE_CONFIG_TYPE_PEEK   /* type */
> +            | 0x0000000c;                   /* crc */
> +    case 0x08:
> +        return ENGINE_CONFIG_NEXT
> +            | 0x00010000                    /* slots */
> +            | 0x00005000                    /* version */
> +            | ENGINE_CONFIG_TYPE_FSI    /* type */
> +            | 0x0000000a;                   /* crc */
> +        break;
> +    default:
> +        /* FIXME: Improve this */
> +        i = 0xc;
> +        QLIST_FOREACH(node, &cfam->lbus.devices, next) {
> +            if (i == addr) {
> +                return LBUS_DEVICE_GET_CLASS(node->ldev)->config;
> +            }
> +            i += size;
> +        }
> +
> +        if (i == addr) {
> +            return 0;
> +        }
> +
> +        return 0xc0de0000;

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