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

[Ninad Palsule]

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 <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?
Added comment for the magic number 0xc0de0000
> 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