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

[Ninad Palsule]

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 <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.
>
> 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