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

[Ninad Palsule]

Hello Daniel,


On 10/19/23 03:30, Daniel P. Berrangé wrote:
> On Wed, Oct 11, 2023 at 10:13:34AM -0500, 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: Cédric Le Goater <clg@kaod.org>
> > 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.
> > ---
> >   include/hw/fsi/opb.h |  43 ++++++++++
> >   hw/fsi/fsi-master.c  |   3 +-
> >   hw/fsi/opb.c         | 185 +++++++++++++++++++++++++++++++++++++++++++
> >   hw/fsi/Kconfig       |   4 +
> >   hw/fsi/meson.build   |   1 +
> >   hw/fsi/trace-events  |   2 +
> >   6 files changed, 236 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..f8ce00383e
> > --- /dev/null
> > +++ b/include/hw/fsi/opb.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * 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;
> > +} OPBus;
> > +
> > +typedef struct OPBusClass {
> > +        BusClass parent_class;
> > +} OPBusClass;
> > +
> > +uint8_t opb_read8(OPBus *opb, hwaddr addr);
> > +uint16_t opb_read16(OPBus *opb, hwaddr addr);
> > +uint32_t opb_read32(OPBus *opb, hwaddr addr);
> > +void opb_write8(OPBus *opb, hwaddr addr, uint8_t data);
> > +void opb_write16(OPBus *opb, hwaddr addr, uint16_t data);
> > +void opb_write32(OPBus *opb, hwaddr addr, uint32_t data);
> > +
> > +void opb_fsi_master_address(OPBus *opb, hwaddr addr);
> > +void opb_opb2fsi_address(OPBus *opb, hwaddr addr);
> > +
> > +#endif /* FSI_OPB_H */
> > diff --git a/hw/fsi/fsi-master.c b/hw/fsi/fsi-master.c
> > index 8f4ae641c7..ede1a58e98 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"
> >   
> >   #define TO_REG(x)                               ((x) >> 2)
> >   
> > diff --git a/hw/fsi/opb.c b/hw/fsi/opb.c
> > new file mode 100644
> > index 0000000000..7ffd7730f7
> > --- /dev/null
> > +++ b/hw/fsi/opb.c
> > @@ -0,0 +1,185 @@
> > +/*
> > + * 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 "trace.h"
> > +
> > +#include "hw/fsi/opb.h"
> > +
> > +static MemTxResult opb_read(OPBus *opb, hwaddr addr, void *data, size_t len)
> > +{
> > +    MemTxResult tx;
> > +    tx = address_space_read(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> > +                              len);
> > +    if (tx) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: OPB read failed: 0x%"HWADDR_PRIx" for %lu\n",
> > +                      __func__, addr, len);
> > +    }
> > +    return tx;
> > +}
> Use a tracepoint rather than an always-on log
Done.
> > +
> > +uint8_t opb_read8(OPBus *opb, hwaddr addr)
> > +{
> > +    uint8_t data = -1;
> > +
> > +    (void)opb_read(opb, addr, &data, sizeof(data));
> > +
> > +    return data;
> > +}
> > +
> > +uint16_t opb_read16(OPBus *opb, hwaddr addr)
> > +{
> > +    uint16_t data = -1;
> > +
> > +    (void)opb_read(opb, addr, &data, sizeof(data));
> > +
> > +    return data;
> > +}
> > +
> > +uint32_t opb_read32(OPBus *opb, hwaddr addr)
> > +{
> > +    uint32_t data = -1;
> > +
> > +    (void)opb_read(opb, addr, &data, sizeof(data));
> > +
> > +    return data;
> > +}
> > +
> > +static void opb_write(OPBus *opb, hwaddr addr, void *data, size_t len)
> > +{
> > +    MemTxResult tx;
> > +    tx = address_space_write(&opb->as, addr, MEMTXATTRS_UNSPECIFIED, data,
> > +                               len);
> > +    if (tx) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: OPB write failed: %"HWADDR_PRIx" for %lu\n",
> > +                      __func__, addr, len);
> > +    }
> > +}
> Again tracepoint
Done
> > +static void opb_realize(BusState *bus, Error **errp)
> > +{
> > +    OPBus *opb = OP_BUS(bus);
> > +    Error *err = NULL;
> > +
> > +    memory_region_init_io(&opb->mr, OBJECT(opb), &opb_unimplemented_ops, opb,
> > +                          NULL, UINT32_MAX);
> > +    address_space_init(&opb->as, &opb->mr, "opb");
> > +
> > +    object_property_set_bool(OBJECT(&opb->fsi), "realized", true, &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> Redundant local error object
Fixed.
> > +    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);
> > +}
> > +
> > diff --git a/hw/fsi/trace-events b/hw/fsi/trace-events
> > index d7afef0460..ec9bab2fe8 100644
> > --- a/hw/fsi/trace-events
> > +++ b/hw/fsi/trace-events
> > @@ -9,3 +9,5 @@ fsi_slave_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " 
> > size=%d"
> >   fsi_slave_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " 
> > size=%d value=0x%"PRIx64
> >   fsi_master_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> >   fsi_master_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 " 
> > size=%d value=0x%"PRIx64
> > +opb_unimplemented_read(uint64_t addr, uint32_t size) "@0x%" PRIx64 " size=%d"
> > +opb_unimplemented_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" PRIx64 
> > " size=%d value=0x%"PRIx64
> Please consistently use an 'fsi_' prefix for all trace points, as it
> makes it possible to enable all tracing for this subsystem using a
> wildcard match

Good suggestion. Fixed it.

Thanks for the review.

Regards,

Ninad

> With regards,
> Daniel