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 <and...@aj.id.au> > Signed-off-by: Cédric Le Goater <c...@kaod.org> > Signed-off-by: Ninad Palsule <ni...@linux.ibm.com> > Reviewed-by: Joel Stanley <j...@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 > + > +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 > +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 > + 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 With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|