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

[Ninad Palsule <ninad@linux.ibm.com>]

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