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

[Ninad Palsule]

Hello Cedric,

On 11/27/23 10:30, Cédric Le Goater wrote:
> On 10/26/23 18:47, 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: Ninad Palsule <ninad@linux.ibm.com>
> > ---
> > v2:
> > - Incorporated Joel's review comments.
> > v5:
> > - Incorporated review comments by Cedric.
> > v6:
> > - Incorporated review comments by Cedric & Daniel.
> > v7:
> > - Incorporated review comments by Philippe.
> > ---
> >   include/hw/fsi/lbus.h | 43 +++++++++++++++++++++++++
> >   hw/fsi/lbus.c         | 74 +++++++++++++++++++++++++++++++++++++++++++
> >   hw/Kconfig            |  1 +
> >   hw/fsi/Kconfig        |  2 ++
> >   hw/fsi/meson.build    |  1 +
> >   hw/meson.build        |  1 +
> >   6 files changed, 122 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..4fa696bbdb
> > --- /dev/null
> > +++ b/include/hw/fsi/lbus.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * 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;
> > +
> > +#define TYPE_FSI_LBUS "fsi.lbus"
> > +OBJECT_DECLARE_SIMPLE_TYPE(FSILBus, FSI_LBUS)
> > +
> > +typedef struct FSILBus {
> > +    BusState bus;
> > +
> > +    MemoryRegion mr;
> > +} 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/hw/fsi/lbus.c b/hw/fsi/lbus.c
> > new file mode 100644
> > index 0000000000..3a7335dde5
> > --- /dev/null
> > +++ b/hw/fsi/lbus.c
> > @@ -0,0 +1,74 @@
> > +/*
> > + * 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_init(Object *o)
> > +{
> > +    FSILBus *lbus = FSI_LBUS(o);
> > +
> > +    memory_region_init(&lbus->mr, OBJECT(lbus), TYPE_FSI_LBUS,
> > +                       FSI_LBUS_MEM_REGION_SIZE - 
> > FSI_LBUSDEV_IOMEM_SIZE);
>
> That's a bit difficult to understand. What are these two regions ?
> CAn you explain more the mappings ?

Please see the attached file for memory mapping. I realized the 
FSI_LBUSDEV_IOMEM_SIZE is not a correct subtraction. It should 0xc00 as 
first 3 pages are used by CFAM so devices get less space.

Thanks for the review and making the changes.

Regards,

Ninad

> Thanks,
>
> C.
>
>
>
>
> > +}
> > +
> > +static const TypeInfo lbus_info = {
> > +    .name = TYPE_FSI_LBUS,
> > +    .parent = TYPE_BUS,
> > +    .instance_init = lbus_init,
> > +    .instance_size = sizeof(FSILBus),
> > +};
> > +
> > +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 *ds;
> > +    BusState *state = BUS(bus);
> > +    FSILBusDevice *dev;
> > +
> > +    ds = qdev_new(type);
> > +    qdev_prop_set_uint32(ds, "address", addr);
> > +    qdev_realize_and_unref(ds, state, &error_fatal);
> > +
> > +    dev = FSI_LBUS_DEVICE(ds);
> > +    memory_region_add_subregion(&bus->mr, dev->address,
> > +                                &dev->iomem);
> > +
> > +    return ds;
> > +}
> > +
> > +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 9ca7b38c31..2c00936c28 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 f01fac4617..463d702683 100644
> > --- a/hw/meson.build
> > +++ b/hw/meson.build
> > @@ -44,6 +44,7 @@ subdir('virtio')
> >   subdir('watchdog')
> >   subdir('xen')
> >   subdir('xenpv')
> > +subdir('fsi')
> >     subdir('alpha')
> >   subdir('arm')

MemoryMap.odt
Description: application/vnd.oasis.opendocument.text