Re: [PATCH v8 03/10] hw/fsi: Introduce IBM's cfam,fsi-slave,scratchpad

[Ninad Palsule]

Hello Cedric,


> > +
> > +#define TYPE_FSI_SCRATCHPAD "fsi.scratchpad"
> > +#define SCRATCHPAD(obj) OBJECT_CHECK(FSIScratchPad, (obj), 
> > TYPE_FSI_SCRATCHPAD)
> > +
> > +typedef struct FSIScratchPad {
> > +        FSILBusDevice parent;
> > +
> > +        uint32_t reg;
> > +} FSIScratchPad;
>
> We could extend to 4 regs possibly.
OK, Added 4 registers.
> > +
> > +#define TYPE_FSI_CFAM "cfam"
> > +#define FSI_CFAM(obj) OBJECT_CHECK(FSICFAMState, (obj), TYPE_FSI_CFAM)
> > +
> > +/* P9-ism */
> > +#define CFAM_CONFIG_NR_REGS 0x28
> > +
> > +typedef struct FSICFAMState {
> > +    /* < private > */
> > +    FSISlaveState parent;
> > +
> > +    /* CFAM config address space */
> > +    MemoryRegion config_iomem;
> > +
> > +    MemoryRegion mr;
> > +    AddressSpace as;
>
> The address space is not used. please remove.
Removed address space.
> > +#include "exec/memory.h"
> > +#include "hw/qdev-core.h"
> > +
> > +#include "hw/fsi/lbus.h"
> > +
> > +#include <stdint.h>
>
> Not needed. Please remove.
Removed the header file.
> > +
> > +static uint64_t fsi_cfam_config_read(void *opaque, hwaddr addr, 
> > unsigned size)
> > +{
> > +    FSICFAMState *cfam = FSI_CFAM(opaque);
> > +    BusChild *kid;
> > +    int i;
> > +
> > +    trace_fsi_cfam_config_read(addr, size);
> > +
> > +    switch (addr) {
> > +    case 0x00:
> > +        return CFAM_CONFIG_CHIP_ID_P9;
> > +    case 0x04:
> > +        return ENGINE_CONFIG_NEXT       |   /* valid */
> > +               0x00010000               |   /* slots */
> > +               0x00001000               |   /* version */
> > +               ENGINE_CONFIG_TYPE_PEEK  |   /* type */
> > +               0x0000000c;                  /* crc */
> > +    case 0x08:
> > +        return ENGINE_CONFIG_NEXT       |   /* valid */
> > +               0x00010000               |   /* slots */
> > +               0x00005000               |   /* version */
> > +               ENGINE_CONFIG_TYPE_FSI   |   /* type */
> > +               0x0000000a;                  /* crc */
>
> Please introduce a macro to build these register values.
Added macros
> > +        break;
> > +    default:
> > +        /* The config table contains different engines from 0xc 
> > onwards. */
> > +        i = 0xc;
> > +        QTAILQ_FOREACH(kid, &cfam->lbus.bus.children, sibling) {
> > +            if (i == addr) {
> > +                DeviceState *ds = kid->child;
> > +                FSILBusDevice *dev = FSI_LBUS_DEVICE(ds);
> > +                return FSI_LBUS_DEVICE_GET_CLASS(dev)->config;
> > +            }
> > +            i += size;
> > +        }
> > +
> > +        if (i == addr) {
> > +            return 0;
> > +        }
>
> If I understand correctly, the register 0xC contains some static config
> value for the first device engine, the scratchpad device mapped at 0xC00,
> and following registers would do the same for other devices if they were
> modelled.
>
> This is certtainly hardwired in HW, so I would simplify to :
>
>     case 0xC:
>         return ldc->config
>     default:
>         /* log not implemented */
>
> And extend the list when more devices are modeled.
Simplified as per your suggestion.
> > +        /*
> > +         * As per FSI specification, This is a magic value at 
> > address 0 of
> > +         * given FSI port. This causes FSI master to send BREAK 
> > command for
> > +         * initialization and recovery.
> > +         */
> > +        return CFAM_CONFIG_CHIP_ID_BREAK;
>
> This looks weird. I don't understant to which offset this value belongs.
Yes, Removed it for now. We are handling break command in the config write.
> > +    }
> > +}
> > +
> > +static void fsi_cfam_config_write(void *opaque, hwaddr addr, 
> > uint64_t data,
> > +                                  unsigned size)
> > +{
> > +    FSICFAMState *cfam = FSI_CFAM(opaque);
> > +
> > +    trace_fsi_cfam_config_write(addr, size, data);
> > +
> > +    switch (TO_REG(addr)) {
> > +    case CFAM_CONFIG_CHIP_ID:
> > +    case CFAM_CONFIG_CHIP_ID + 4:
>
> Couldn't we introduce a proper define for this reg ? and can we write to
> the config space ? This break command seems to be sent to the FSI master,
> according to Linux. Why is it handled in the CFAM config space ?
Added new PEEK_STATUS register. The BREAK command is send by FSI-master 
to FSI-slave and FSI-slave is embedded into CFAM hence we are handling 
it here.
> > +        if (data == CFAM_CONFIG_CHIP_ID_BREAK) {
> > +            bus_cold_reset(BUS(&cfam->lbus));
> > +        }
> > +    break;
>
> alignment is not good.
Fixed the alignment.
> > +static void fsi_cfam_realize(DeviceState *dev, Error **errp)
> > +{
> > +    FSICFAMState *cfam = FSI_CFAM(dev);
> > +    FSISlaveState *slave = FSI_SLAVE(dev);
> > +
> > +    /* Each slave has a 2MiB address space */
> > +    memory_region_init_io(&cfam->mr, OBJECT(cfam), 
> > &fsi_cfam_unimplemented_ops,
> > +                          cfam, TYPE_FSI_CFAM, 2 * 1024 * 1024);
>
> 2 * MiB
Now using MiB.
> > +
> > +    /* Add scratchpad engine */
> > +    if (!qdev_realize_and_unref(DEVICE(&cfam->scratchpad), 
> > BUS(&cfam->lbus),
>
> cfam->scratchpad is not allocated. We should use qdev_realize instead.
Fixed it.
> > +    /* TODO: clarify scratchpad mapping */
>
> You can remove the TODO now. All Local bus devices are mapped at offset
> 0xc00.
Removed it.
> > +static void fsi_scratchpad_reset(DeviceState *dev)
> > +{
> > +    FSIScratchPad *s = SCRATCHPAD(dev);
> > +
> > +    s->reg = 0;
>
> Just one reg ! Too easy :) let's have a few more.
Now clear 4 registers.
> > +    ldc->config =
> > +          ENGINE_CONFIG_NEXT            | /* valid */
> > +          0x00010000                    | /* slots */
> > +          0x00001000                    | /* version */
> > +          ENGINE_CONFIG_TYPE_SCRATCHPAD | /* type */
> > +          0x00000007;                     /* crc */
>
> This class and attribute do not  look useful. Please use a macro
> to build the value and return it in the CFAM config read operation.
Added macro for SCARTCHPAD config value but keeping the class as new 
devices need it.
> > +
> > +static uint64_t fsi_slave_read(void *opaque, hwaddr addr, unsigned 
> > size)
> > +{
> > +    FSISlaveState *s = FSI_SLAVE(opaque);
> > +
> > +    trace_fsi_slave_read(addr, size);
> > +
> > +    if (addr + size > sizeof(s->regs)) {
>
> This test is mixing memory region offsets and memop size. These are two
> fields of different nature. So this is quite incorrect !
>
> Ideally, we should have a switch statement with handlers for implemented
> registers and a default for the rest. Please see the aspeed_scu model
> for an example.
I have fixed the limit check but registers are simply used as a memory 
region hence did not add switch statement.
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Out of bounds read: 0x%"HWADDR_PRIx" for 
> > %u\n",
> > +                      __func__, addr, size);
> > +        return 0;
> > +    }
> > +
> > +    return s->regs[TO_REG(addr)];
> > +}
> > +
> > +static void fsi_slave_write(void *opaque, hwaddr addr, uint64_t data,
> > +                                 unsigned size)
> > +{
> > +    FSISlaveState *s = FSI_SLAVE(opaque);
> > +
> > +    trace_fsi_slave_write(addr, size, data);
> > +
> > +    if (addr + size > sizeof(s->regs)) {
>
> Same here.
Fixed the limit check.
> > +
> > +static void fsi_slave_init(Object *o)
> > +{
> > +    FSISlaveState *s = FSI_SLAVE(o);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &fsi_slave_ops,
> > +                          s, TYPE_FSI_SLAVE, 0x400);
> > +}
>
>
> No reset handler ?

Added reset handler.

Thanks for the review.

Regards,

Ninad