Re: [PATCH v10 1/9] hw/fsi: Introduce IBM's Local bus and scratchpad

[Ninad Palsule]

Hello Cedric,

> > [ clg: - removed lbus_add_device() bc unused
> >         - removed lbus_create_device() bc used only once
> >         - removed "address" property
> >         - updated meson.build to build fsi dir
> >         - included an empty hw/fsi/trace-events ]
>
> this list of modifications should be before my S-o-b.
Moved it to correct place.

> > +
> > +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
> > +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
> > +
> > +#define FSI_LBUS_MEM_REGION_SIZE  (1 * MiB)
> > +#define FSI_LBUSDEV_IOMEM_START   0xc00 /* 3K used by CFAM config 
> > etc */
>
> These define are not very useful. Please remove (see comments in
> lbus.c)
Removed.
> > +typedef struct FSILBusDeviceClass {
> > +    DeviceClass parent;
> > +
> > +    uint32_t config;
> > +} FSILBusDeviceClass;
>
> This class is unused now.
Removed the class. We will add it once we add more devices.
> > +#include "trace.h"
> > +
> > +static void lbus_init(Object *o)
>
> I would prefix the lbus routine and struct with fsi_lbus to be
> consistent with the types and macro.
Added prefix fsi_ for all routines.
> > +{
> > +    FSILBus *lbus = FSI_LBUS(o);
> > +
> > +    memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS,
> > +                       FSI_LBUS_MEM_REGION_SIZE - 
> > FSI_LBUSDEV_IOMEM_START);
>
> This is enough :
>
>     memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS, 1 * MiB);
Changed as per your suggestion.

> > +
> > +static void fsi_scratchpad_write(void *opaque, hwaddr addr, uint64_t 
> > data,
> > +                                 unsigned size)
> > +{
> > +    FSIScratchPad *s = SCRATCHPAD(opaque);
> > +
> > +    trace_fsi_scratchpad_write(addr, size, data);
> > +
> > +    if (addr & ~(FSI_SCRATCHPAD_NR_REGS - 1)) {
>
> There is a type confusion. addr is an offset in a memory region and
> FSI_SCRATCHPAD_NR_REGS is an index an array.
Fixed.
> > +        return;
> > +    }
> > +
> > +    s->reg[addr] = data;
>
> same here.
Fixed.
> > +static void fsi_scratchpad_reset(DeviceState *dev)
> > +{
> > +    FSIScratchPad *s = SCRATCHPAD(dev);
> > +    int i;
> > +
> > +    for (i = 0; i < FSI_SCRATCHPAD_NR_REGS; i++) {
> > +        s->reg[i] = 0;
>
> memset(s->regs, 0, sizeof(s->regs));

Fixed.

Thanks for the review.

Regards,

Ninad