Re: [PATCH v7 05/10] hw/fsi: IBM's On-chip Peripheral Bus

[Ninad Palsule]

Hello Cedric,

On 11/27/23 10:23, 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 On-Chip Peripheral Bus (OPB): A low-speed bus typically found in
> > POWER processors. This now makes an appearance in the ASPEED SoC due
> > to tight integration of the FSI master IP with the OPB, mainly the
> > existence of an MMIO-mapping of the CFAM address straight onto a
> > sub-region of the OPB address space.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > ---
> > v2:
> > - Incorporated review comment by Joel.
> > v5:
> > - Incorporated review comments by Cedric.
> > v6:
> > - Incorporated review comments by Cedric & Daniel
> > v7:
> > - Incorporated review comments by Cedric.
> > ---
> >   include/hw/fsi/opb.h | 33 ++++++++++++++++++++
> >   hw/fsi/fsi-master.c  |  3 +-
> >   hw/fsi/opb.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++
> >   hw/fsi/Kconfig       |  4 +++
> >   hw/fsi/meson.build   |  1 +
> >   5 files changed, 113 insertions(+), 2 deletions(-)
> >   create mode 100644 include/hw/fsi/opb.h
> >   create mode 100644 hw/fsi/opb.c
> >
> > diff --git a/include/hw/fsi/opb.h b/include/hw/fsi/opb.h
> > new file mode 100644
> > index 0000000000..8b71bb55c2
> > --- /dev/null
> > +++ b/include/hw/fsi/opb.h
> > @@ -0,0 +1,33 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright (C) 2023 IBM Corp.
> > + *
> > + * IBM On-Chip Peripheral Bus
> > + */
> > +#ifndef FSI_OPB_H
> > +#define FSI_OPB_H
> > +
> > +#include "exec/memory.h"
> > +#include "hw/fsi/fsi-master.h"
> > +
> > +#define TYPE_OP_BUS "opb"
> > +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
> > +
> > +typedef struct OPBus {
> > +        /*< private >*/
> > +        BusState bus;
> > +
> > +        /*< public >*/
> > +        MemoryRegion mr;
> > +        AddressSpace as;
> > +
> > +        /* Model OPB as dumb enough just to provide an address-space */
> > +        /* TODO: Maybe don't store device state in the bus? */
> > +        FSIMasterState fsi;
>
> Please remove. FSIMasterState should be introduced later.
ok, Thanks for the change. Looks like you moved it from OPBus to 
AspeedAPB2OPBState. I am fine with the change if it is making model cleaner.
> > +} OPBus;
> > +
> > +typedef struct OPBusClass {
> > +        BusClass parent_class;
> > +} OPBusClass;
> > +
> > +#endif /* FSI_OPB_H */
> > diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c
> > index bb7a893003..ec092b42ea 100644
> > --- a/hw/fsi/fsi-master.c
> > +++ b/hw/fsi/fsi-master.c
> > @@ -11,8 +11,7 @@
> >   #include "trace.h"
> >     #include "hw/fsi/fsi-master.h"
> > -
> > -#define TYPE_OP_BUS "opb"
> > +#include "hw/fsi/opb.h"
>
> ouch.
>
> This is an ugly hack because the modeling is broken. OPB should be 
> introduced
> before tFSIMasterState.
OK, Thanks for the change.
> >   #define TO_REG(x)                               ((x) >> 2)
> >   diff --git a/hw/fsi/opb.c b/hw/fsi/opb.c
> > new file mode 100644
> > index 0000000000..04771b4b27
> > --- /dev/null
> > +++ b/hw/fsi/opb.c
> > @@ -0,0 +1,74 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * Copyright (C) 2023 IBM Corp.
> > + *
> > + * IBM On-chip Peripheral Bus
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "qapi/error.h"
> > +#include "qemu/log.h"
> > +
> > +#include "hw/fsi/opb.h"
> > +
> > +static void fsi_opb_realize(BusState *bus, Error **errp)
> > +{
> > +    OPBus *opb = OP_BUS(bus);
> > +
> > +    memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb,
> > +                          NULL, UINT32_MAX);
> > +    address_space_init(&opb->as, &opb->mr, "opb");
>
> Please keep the above and put it in a instance_init handler and remove
> the rest. It should go under AspeedAPB2OPBState.
OK, Thanks for the change.
> > +
> > +    if (!object_property_set_bool(OBJECT(&opb->fsi), "realized", 
> > true, errp)) {
> > +        return;
> > +    }
> > +
> > +    memory_region_add_subregion(&opb->mr, 0x80000000, &opb->fsi.iomem);
> > +
> > +    /* OPB2FSI region */
> > +    /*
> > +     * Avoid endianness issues by mapping each slave's memory region 
> > directly.
> > +     * Manually bridging multiple address-spaces causes endian swapping
> > +     * headaches as memory_region_dispatch_read() and
> > +     * memory_region_dispatch_write() correct the endianness based 
> > on the
> > +     * target machine endianness and not relative to the device 
> > endianness on
> > +     * either side of the bridge.
> > +     */
> > +    /*
> > +     * XXX: This is a bit hairy and will need to be fixed when I 
> > sort out the
> > +     * bus/slave relationship and any changes to the CFAM modelling 
> > (multiple
> > +     * slaves, LBUS)
> > +     */
> > +    memory_region_add_subregion(&opb->mr, 0xa0000000, 
> > &opb->fsi.opb2fsi);
> > +}
> > +
> > +static void fsi_opb_init(Object *o)
> > +{
> > +    OPBus *opb = OP_BUS(o);
> > +
> > +    object_initialize_child(o, "fsi-master", &opb->fsi, 
> > TYPE_FSI_MASTER);
> > +    qdev_set_parent_bus(DEVICE(&opb->fsi), BUS(o), &error_abort);
> > +}
>
> Drop all of the above.

OK, Thanks for the review and change.

Regards,

Ninad

> Thanks,
>
> C.
>
>
>
> > +
> > +static void fsi_opb_class_init(ObjectClass *klass, void *data)
> > +{
> > +    BusClass *bc = BUS_CLASS(klass);
> > +    bc->realize = fsi_opb_realize;
> > +}
> > +
> > +static const TypeInfo opb_info = {
> > +    .name = TYPE_OP_BUS,
> > +    .parent = TYPE_BUS,
> > +    .instance_init = fsi_opb_init,
> > +    .instance_size = sizeof(OPBus),
> > +    .class_init = fsi_opb_class_init,
> > +    .class_size = sizeof(OPBusClass),
> > +};
> > +
> > +static void fsi_opb_register_types(void)
> > +{
> > +    type_register_static(&opb_info);
> > +}
> > +
> > +type_init(fsi_opb_register_types);
> > diff --git a/hw/fsi/Kconfig b/hw/fsi/Kconfig
> > index 8d712e77ed..0f6e6d331a 100644
> > --- a/hw/fsi/Kconfig
> > +++ b/hw/fsi/Kconfig
> > @@ -1,3 +1,7 @@
> > +config FSI_OPB
> > +    bool
> > +    select FSI_CFAM
> > +
> >   config FSI_CFAM
> >       bool
> >       select FSI
> > diff --git a/hw/fsi/meson.build b/hw/fsi/meson.build
> > index f617943b4a..407b8c2775 100644
> > --- a/hw/fsi/meson.build
> > +++ b/hw/fsi/meson.build
> > @@ -2,3 +2,4 @@ system_ss.add(when: 'CONFIG_FSI_LBUS', if_true: 
> > files('lbus.c'))
> >   system_ss.add(when: 'CONFIG_FSI_SCRATCHPAD', if_true: 
> > files('engine-scratchpad.c'))
> >   system_ss.add(when: 'CONFIG_FSI_CFAM', if_true: files('cfam.c'))
> >   system_ss.add(when: 'CONFIG_FSI', if_true: 
> > files('fsi.c','fsi-master.c','fsi-slave.c'))
> > +system_ss.add(when: 'CONFIG_FSI_OPB', if_true: files('opb.c'))