Re: [PATCH v5 01/10] hw/fsi: Introduce IBM's Local bus

[Ninad Palsule]

Hello Cedric,

On 10/19/23 03:03, Cédric Le Goater wrote:
> On 10/11/23 17:13, Ninad Palsule wrote:
> > This is a part of patchset where IBM's Flexible Service Interface is
> > introduced.
> >
> > The LBUS is modelled to maintain the qdev bus hierarchy and to take
> > advantage of the object model to automatically generate the CFAM
> > configuration block. The configuration block presents engines in the
> > order they are attached to the CFAM's LBUS. Engine implementations
> > should subclass the LBusDevice and set the 'config' member of
> > LBusDeviceClass to match the engine's type.
> >
> > 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>
> > ---
> > v2:
> > - Incorporated Joel's review comments.
> > v5:
> > - Incorporated review comments by Cedric.
> > ---
> >   include/hw/fsi/lbus.h | 51 +++++++++++++++++++++++++
> >   include/qemu/bitops.h |  6 +++
> >   hw/fsi/lbus.c         | 87 +++++++++++++++++++++++++++++++++++++++++++
> >   hw/Kconfig            |  1 +
> >   hw/fsi/Kconfig        |  2 +
> >   hw/fsi/meson.build    |  1 +
> >   hw/meson.build        |  1 +
> >   7 files changed, 149 insertions(+)
> >   create mode 100644 include/hw/fsi/lbus.h
> >   create mode 100644 hw/fsi/lbus.c
> >   create mode 100644 hw/fsi/Kconfig
> >   create mode 100644 hw/fsi/meson.build
> >
> > diff --git a/include/hw/fsi/lbus.h b/include/hw/fsi/lbus.h
> > new file mode 100644
> > index 0000000000..408fe25831
> > --- /dev/null
> > +++ b/include/hw/fsi/lbus.h
> > @@ -0,0 +1,51 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright (C) 2023 IBM Corp.
> > + *
> > + * IBM Local bus and connected device structures.
> > + */
> > +#ifndef FSI_LBUS_H
> > +#define FSI_LBUS_H
> > +
> > +#include "exec/memory.h"
> > +#include "hw/qdev-core.h"
> > +
> > +#define TYPE_FSI_LBUS_DEVICE "fsi.lbus.device"
> > +OBJECT_DECLARE_TYPE(FSILBusDevice, FSILBusDeviceClass, FSI_LBUS_DEVICE)
> > +
> > +#define FSI_LBUS_MEM_REGION_SIZE  (2 * 1024 * 1024)
> > +#define FSI_LBUSDEV_IOMEM_SIZE    0x400
> > +
> > +typedef struct FSILBusDevice {
> > +    DeviceState parent;
> > +
> > +    MemoryRegion iomem;
> > +    uint32_t address;
> > +} FSILBusDevice;
> > +
> > +typedef struct FSILBusDeviceClass {
> > +    DeviceClass parent;
> > +
> > +    uint32_t config;
> > +} FSILBusDeviceClass;
> > +
> > +typedef struct FSILBusNode {
> > +    FSILBusDevice *ldev;
> > +
> > +    QLIST_ENTRY(FSILBusNode) next;
> > +} FSILBusNode;
> > +
> > +#define TYPE_FSI_LBUS "fsi.lbus"
> > +OBJECT_DECLARE_SIMPLE_TYPE(FSILBus, FSI_LBUS)
> > +
> > +typedef struct FSILBus {
> > +    BusState bus;
> > +
> > +    MemoryRegion mr;
> > +
> > +    QLIST_HEAD(, FSILBusNode) devices;
> > +} FSILBus;
> > +
> > +DeviceState *lbus_create_device(FSILBus *bus, const char *type, 
> > uint32_t addr);
> > +int lbus_add_device(FSILBus *bus, FSILBusDevice *dev);
> > +#endif /* FSI_LBUS_H */
> > diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> > index cb3526d1f4..e12496f619 100644
> > --- a/include/qemu/bitops.h
> > +++ b/include/qemu/bitops.h
> > @@ -618,4 +618,10 @@ static inline uint64_t half_unshuffle64(uint64_t x)
> >       return x;
> >   }
> >   +/* Bitwise operations at the word level. */
> > +#define BE_BIT(x)                          BIT(31 - (x))
> > +#define GENMASK(t, b) \
> > +    (((1ULL << ((t) + 1)) - 1) & ~((1ULL << (b)) - 1))
> > +#define BE_GENMASK(t, b)                   GENMASK(BE_BIT(t), 
> > BE_BIT(b))
> > +
> >   #endif
> > diff --git a/hw/fsi/lbus.c b/hw/fsi/lbus.c
> > new file mode 100644
> > index 0000000000..d7117d1299
> > --- /dev/null
> > +++ b/hw/fsi/lbus.c
> > @@ -0,0 +1,87 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright (C) 2023 IBM Corp.
> > + *
> > + * IBM Local bus where FSI slaves are connected
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "hw/fsi/lbus.h"
> > +
> > +#include "hw/qdev-properties.h"
> > +
> > +static void lbus_realize(BusState *bus, Error **errp)
> > +{
> > +    FSILBusNode *node;
> > +    FSILBus *lbus = FSI_LBUS(bus);
> > +
> > +    memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS,
> > +                       FSI_LBUS_MEM_REGION_SIZE - 
> > FSI_LBUSDEV_IOMEM_SIZE);
>
> the region init should be done in an init handler (this to avoid the 
> realize
> of the bus)
Fixed.
> > +
> > +    QLIST_FOREACH(node, &lbus->devices, next) {
> > +        memory_region_add_subregion(&lbus->mr, node->ldev->address,
> > + &node->ldev->iomem);
>
> This is redudant with what is done in cfam_realize() and please
> remove this list, also redudant with the bus list.

Removed the list and used the children. Removed the lbus_realize() function.

Thanks for the review.

~Ninad

> Thanks,
>
> C.
>
> > +    }
> > +}
> > +
> > +static void lbus_class_init(ObjectClass *klass, void *data)
> > +{
> > +    BusClass *k = BUS_CLASS(klass);
> > +    k->realize = lbus_realize;
> > +}
> > +
> > +static const TypeInfo lbus_info = {
> > +    .name = TYPE_FSI_LBUS,
> > +    .parent = TYPE_BUS,
> > +    .instance_size = sizeof(FSILBus),
> > +    .class_init = lbus_class_init,
> > +};
> > +
> > +static Property lbus_device_props[] = {
> > +    DEFINE_PROP_UINT32("address", FSILBusDevice, address, 0),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +DeviceState *lbus_create_device(FSILBus *bus, const char *type, 
> > uint32_t addr)
> > +{
> > +    DeviceState *dev;
> > +    FSILBusNode *node;
> > +    BusState *state = BUS(bus);
> > +
> > +    dev = qdev_new(type);
> > +    qdev_prop_set_uint8(dev, "address", addr);
> > +    qdev_realize_and_unref(dev, state, &error_fatal);
> > +
> > +    /* Move to post_load */
> > +    node = g_malloc(sizeof(struct FSILBusNode));
> > +    node->ldev = FSI_LBUS_DEVICE(dev);
> > +    QLIST_INSERT_HEAD(&bus->devices, node, next);
> > +
> > +    return dev;
> > +}
> > +
> > +static void lbus_device_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->bus_type = TYPE_FSI_LBUS;
> > +    device_class_set_props(dc, lbus_device_props);
> > +}
> > +
> > +static const TypeInfo lbus_device_type_info = {
> > +    .name = TYPE_FSI_LBUS_DEVICE,
> > +    .parent = TYPE_DEVICE,
> > +    .instance_size = sizeof(FSILBusDevice),
> > +    .abstract = true,
> > +    .class_init = lbus_device_class_init,
> > +    .class_size = sizeof(FSILBusDeviceClass),
> > +};
> > +
> > +static void lbus_register_types(void)
> > +{
> > +    type_register_static(&lbus_info);
> > +    type_register_static(&lbus_device_type_info);
> > +}
> > +
> > +type_init(lbus_register_types);
> > diff --git a/hw/Kconfig b/hw/Kconfig
> > index ba62ff6417..2ccb73add5 100644
> > --- a/hw/Kconfig
> > +++ b/hw/Kconfig
> > @@ -9,6 +9,7 @@ source core/Kconfig
> >   source cxl/Kconfig
> >   source display/Kconfig
> >   source dma/Kconfig
> > +source fsi/Kconfig
> >   source gpio/Kconfig
> >   source hyperv/Kconfig
> >   source i2c/Kconfig
> > diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
> > new file mode 100644
> > index 0000000000..e650c660f0
> > --- /dev/null
> > +++ b/hw/fsi/Kconfig
> > @@ -0,0 +1,2 @@
> > +config FSI_LBUS
> > +    bool
> > diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> > new file mode 100644
> > index 0000000000..4074d3a7d2
> > --- /dev/null
> > +++ b/hw/fsi/meson.build
> > @@ -0,0 +1 @@
> > +system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: files('lbus.c'))
> > diff --git a/hw/meson.build b/hw/meson.build
> > index c7ac7d3d75..6c71ee9cfa 100644
> > --- a/hw/meson.build
> > +++ b/hw/meson.build
> > @@ -43,6 +43,7 @@ subdir('virtio')
> >   subdir('watchdog')
> >   subdir('xen')
> >   subdir('xenpv')
> > +subdir('fsi')
> >     subdir('alpha')
> >   subdir('arm')