Re: [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
Hello Cedric,
Signed-off-by: Andrew Jeffery
Signed-off-by: Cédric Le Goater
Signed-off-by: Ninad Palsule
Reviewed-by: Joel Stanley
Nah. Joel you should re-review next respin :)
Removed Joel's review tag.
---
include/hw/misc/aspeed-apb2opb.h | 50 +
hw/misc/aspeed-apb2opb.c | 338 +++
As said in the cover letter, I think now that hw/fsi is a better place
for these files and should be compiled if CONSG_ASPEED_SOC. Sorry about
that. Also,please use 'aspeed_' for the file names.
Transferred to the old location and rename with "aspeed_" prefix.
+
+#define TYPE_OP_BUS "opb"
+OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
+
+typedef struct OPBus {
+ /*< private >*/
please remove the private and public comment.
Removed.
+ BusState bus;
+
+ /*< public >*/
+ MemoryRegion mr;
+ AddressSpace as;
indent is wrong.
Fixed indent.
+static void fsi_aspeed_apb2opb_init(Object *o)
+{
+ AspeedAPB2OPBState *s = ASPEED_APB2OPB(o);
+ int i;
+
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ qbus_init(>opb[i], sizeof(s->opb[i]), TYPE_OP_BUS,
DEVICE(s),
+ NULL);
See comment in fsi_opb_init()
Moved it to si_aspeed_apb2opb_realize(),
+ for (i = 0; i < ASPEED_FSI_NUM; i++) {
+ if (!qdev_realize(DEVICE(>fsi[i]), BUS(>opb[i]),
+ errp)) {
this could be a single line.
Removed extra line.
Please remove the comments below, I am not sure they are valid
anymore.
Removed the comment.
+ /*
+ * 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[i].mr, 0xa000,
+ >fsi[i].opb2fsi);
+ }
+}
+
+static void fsi_opb_init(Object *o)
+{
+ OPBus *opb = OP_BUS(o);
+
+ memory_region_init_io(>mr, OBJECT(opb), NULL, opb,
+ TYPE_FSI_OPB, UINT32_MAX);
This is better :
memory_region_init(>mr, o, TYPE_OP_BUS, UINT32_MAX);
Changed it as per your suggestion.
+ address_space_init(>as, >mr, TYPE_FSI_OPB);
This routine is problematic. If you run 'make check', you should see
test tests/qtest/device-introspect-test crash in weird way because of
a memory corruption. I didn't dig into the details but I suppose this
a use after free problem.
To solve, we should move qbus_init() done in fsi_aspeed_apb2opb_init()
under fsi_aspeed_apb2opb_realize(), or improve the model a litle more.
It seems we are lacking the OPB/FSI bridge :
typedef struct OPBFSIBridge {
DeviceState parent;
OPBus opb;
FSIMasterState fsi;
MemoryRegion mr;
AddressSpace as;
} OPBFSIBridge;
Something like that. It is difficult to understand the design from
the OpenFSI specs. The OPB bus seems overkill. It you could clarify
this aspect, it would be nice.
For now moved qbus_init() to fsi_aspeed_apb2opb_realize(). I will run
make check.
Thanks for the review.
Regards,
Ninad
Re: [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
The subject is not very clear.
On 1/11/24 00:15, Ninad Palsule wrote:
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
An APB-to-OPB bridge enabling access to the OPB from the ARM core in
the AST2600. Hardware limitations prevent the OPB from being directly
mapped into APB, so all accesses are indirect through the bridge.
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.
[ clg: - moved FSIMasterState under AspeedAPB2OPBState
- modified fsi_opb_fsi_master_address() and
fsi_opb_opb2fsi_address()
- instroduced fsi_aspeed_apb2opb_init()
- reworked fsi_aspeed_apb2opb_realize()
- removed FSIMasterState object and fsi_opb_realize()
- simplified OPBus ]
Signed-off-by: Andrew Jeffery
Signed-off-by: Cédric Le Goater
Signed-off-by: Ninad Palsule
Reviewed-by: Joel Stanley
Nah. Joel you should re-review next respin :)
---
v9:
- Removed unused parameters from function.
- Used qdev_realize() instead of qdev_realize_and_undef
- Given a name to the opb memory region.
v10:
- Combine Aspeed APB2OPB and on-chip pheripheral bus
---
include/hw/misc/aspeed-apb2opb.h | 50 +
hw/misc/aspeed-apb2opb.c | 338 +++
As said in the cover letter, I think now that hw/fsi is a better place
for these files and should be compiled if CONSG_ASPEED_SOC. Sorry about
that. Also,please use 'aspeed_' for the file names.
hw/arm/Kconfig | 1 +
hw/misc/Kconfig | 5 +
hw/misc/meson.build | 1 +
hw/misc/trace-events | 4 +
6 files changed, 399 insertions(+)
create mode 100644 include/hw/misc/aspeed-apb2opb.h
create mode 100644 hw/misc/aspeed-apb2opb.c
diff --git a/include/hw/misc/aspeed-apb2opb.h b/include/hw/misc/aspeed-apb2opb.h
new file mode 100644
index 00..fcd76631a9
--- /dev/null
+++ b/include/hw/misc/aspeed-apb2opb.h
@@ -0,0 +1,50 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * ASPEED APB2OPB Bridge
+ * IBM On-Chip Peripheral Bus
+ */
+#ifndef FSI_ASPEED_APB2OPB_H
+#define FSI_ASPEED_APB2OPB_H
+
+#include "exec/memory.h"
+#include "hw/fsi/fsi-master.h"
+#include "hw/sysbus.h"
+
+#define TYPE_FSI_OPB "fsi.opb"
+
+#define TYPE_OP_BUS "opb"
+OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
+
+typedef struct OPBus {
+/*< private >*/
please remove the private and public comment.
+BusState bus;
+
+/*< public >*/
+MemoryRegion mr;
+AddressSpace as;
indent is wrong.
+} OPBus;
+
+#define TYPE_ASPEED_APB2OPB "aspeed.apb2opb"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedAPB2OPBState, ASPEED_APB2OPB)
+
+#define ASPEED_APB2OPB_NR_REGS ((0xe8 >> 2) + 1)
+
+#define ASPEED_FSI_NUM 2
+
+typedef struct AspeedAPB2OPBState {
+/*< private >*/
+SysBusDevice parent_obj;
+
+/*< public >*/
+MemoryRegion iomem;
+
+uint32_t regs[ASPEED_APB2OPB_NR_REGS];
+qemu_irq irq;
+
+OPBus opb[ASPEED_FSI_NUM];
+FSIMasterState fsi[ASPEED_FSI_NUM];
+} AspeedAPB2OPBState;
+
+#endif /* FSI_ASPEED_APB2OPB_H */
diff --git a/hw/misc/aspeed-apb2opb.c b/hw/misc/aspeed-apb2opb.c
new file mode 100644
index 00..19545c780f
--- /dev/null
+++ b/hw/misc/aspeed-apb2opb.c
@@ -0,0 +1,338 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * ASPEED APB-OPB FSI interface
+ * IBM On-chip Peripheral Bus
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/misc/aspeed-apb2opb.h"
+#include "hw/qdev-core.h"
+
+#define TO_REG(x) (x >> 2)
+
+#define APB2OPB_VERSIONTO_REG(0x00)
+#define APB2OPB_TRIGGERTO_REG(0x04)
+
+#define APB2OPB_CONTROLTO_REG(0x08)
+#define APB2OPB_CONTROL_OFF BE_GENMASK(31, 13)
+
+#define APB2OPB_OPB2FSITO_REG(0x0c)
+#define APB2OPB_OPB2FSI_OFF BE_GENMASK(31, 22)
+
+#define APB2OPB_OPB0_SEL TO_REG(0x10)
+#define APB2OPB_OPB1_SEL TO_REG(0x28)
+#define APB2OPB_OPB_SEL_EN BIT(0)
+
+#define APB2OPB_OPB0_MODE TO_REG(0x14)
+#define APB2OPB_OPB1_MODE TO_REG(0x2c)
+#define APB2OPB_OPB_MODE_RD BIT(0)
+
+#define APB2OPB_OPB0_XFER TO_REG(0x18)
+#define APB2OPB_OPB1_XFER TO_REG(0x30)
+#define APB2OPB_OPB_XFER_FULLBIT(1)
+#define APB2OPB_OPB_XFER_HALFBIT(0)
+
+#define APB2OPB_OPB0_ADDR TO_REG(0x1c)
+#define APB2OPB_OPB0_WRITE_DATATO_REG(0x20)
+
+#define APB2OPB_OPB1_ADDR
[PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus
This is a part of patchset where IBM's Flexible Service Interface is
introduced.
An APB-to-OPB bridge enabling access to the OPB from the ARM core in
the AST2600. Hardware limitations prevent the OPB from being directly
mapped into APB, so all accesses are indirect through the bridge.
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.
[ clg: - moved FSIMasterState under AspeedAPB2OPBState
- modified fsi_opb_fsi_master_address() and
fsi_opb_opb2fsi_address()
- instroduced fsi_aspeed_apb2opb_init()
- reworked fsi_aspeed_apb2opb_realize()
- removed FSIMasterState object and fsi_opb_realize()
- simplified OPBus ]
Signed-off-by: Andrew Jeffery
Signed-off-by: Cédric Le Goater
Signed-off-by: Ninad Palsule
Reviewed-by: Joel Stanley
---
v9:
- Removed unused parameters from function.
- Used qdev_realize() instead of qdev_realize_and_undef
- Given a name to the opb memory region.
v10:
- Combine Aspeed APB2OPB and on-chip pheripheral bus
---
include/hw/misc/aspeed-apb2opb.h | 50 +
hw/misc/aspeed-apb2opb.c | 338 +++
hw/arm/Kconfig | 1 +
hw/misc/Kconfig | 5 +
hw/misc/meson.build | 1 +
hw/misc/trace-events | 4 +
6 files changed, 399 insertions(+)
create mode 100644 include/hw/misc/aspeed-apb2opb.h
create mode 100644 hw/misc/aspeed-apb2opb.c
diff --git a/include/hw/misc/aspeed-apb2opb.h b/include/hw/misc/aspeed-apb2opb.h
new file mode 100644
index 00..fcd76631a9
--- /dev/null
+++ b/include/hw/misc/aspeed-apb2opb.h
@@ -0,0 +1,50 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * ASPEED APB2OPB Bridge
+ * IBM On-Chip Peripheral Bus
+ */
+#ifndef FSI_ASPEED_APB2OPB_H
+#define FSI_ASPEED_APB2OPB_H
+
+#include "exec/memory.h"
+#include "hw/fsi/fsi-master.h"
+#include "hw/sysbus.h"
+
+#define TYPE_FSI_OPB "fsi.opb"
+
+#define TYPE_OP_BUS "opb"
+OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
+
+typedef struct OPBus {
+/*< private >*/
+BusState bus;
+
+/*< public >*/
+MemoryRegion mr;
+AddressSpace as;
+} OPBus;
+
+#define TYPE_ASPEED_APB2OPB "aspeed.apb2opb"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedAPB2OPBState, ASPEED_APB2OPB)
+
+#define ASPEED_APB2OPB_NR_REGS ((0xe8 >> 2) + 1)
+
+#define ASPEED_FSI_NUM 2
+
+typedef struct AspeedAPB2OPBState {
+/*< private >*/
+SysBusDevice parent_obj;
+
+/*< public >*/
+MemoryRegion iomem;
+
+uint32_t regs[ASPEED_APB2OPB_NR_REGS];
+qemu_irq irq;
+
+OPBus opb[ASPEED_FSI_NUM];
+FSIMasterState fsi[ASPEED_FSI_NUM];
+} AspeedAPB2OPBState;
+
+#endif /* FSI_ASPEED_APB2OPB_H */
diff --git a/hw/misc/aspeed-apb2opb.c b/hw/misc/aspeed-apb2opb.c
new file mode 100644
index 00..19545c780f
--- /dev/null
+++ b/hw/misc/aspeed-apb2opb.c
@@ -0,0 +1,338 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright (C) 2024 IBM Corp.
+ *
+ * ASPEED APB-OPB FSI interface
+ * IBM On-chip Peripheral Bus
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qom/object.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+#include "hw/misc/aspeed-apb2opb.h"
+#include "hw/qdev-core.h"
+
+#define TO_REG(x) (x >> 2)
+
+#define APB2OPB_VERSIONTO_REG(0x00)
+#define APB2OPB_TRIGGERTO_REG(0x04)
+
+#define APB2OPB_CONTROLTO_REG(0x08)
+#define APB2OPB_CONTROL_OFF BE_GENMASK(31, 13)
+
+#define APB2OPB_OPB2FSITO_REG(0x0c)
+#define APB2OPB_OPB2FSI_OFF BE_GENMASK(31, 22)
+
+#define APB2OPB_OPB0_SEL TO_REG(0x10)
+#define APB2OPB_OPB1_SEL TO_REG(0x28)
+#define APB2OPB_OPB_SEL_EN BIT(0)
+
+#define APB2OPB_OPB0_MODE TO_REG(0x14)
+#define APB2OPB_OPB1_MODE TO_REG(0x2c)
+#define APB2OPB_OPB_MODE_RD BIT(0)
+
+#define APB2OPB_OPB0_XFER TO_REG(0x18)
+#define APB2OPB_OPB1_XFER TO_REG(0x30)
+#define APB2OPB_OPB_XFER_FULLBIT(1)
+#define APB2OPB_OPB_XFER_HALFBIT(0)
+
+#define APB2OPB_OPB0_ADDR TO_REG(0x1c)
+#define APB2OPB_OPB0_WRITE_DATATO_REG(0x20)
+
+#define APB2OPB_OPB1_ADDR TO_REG(0x34)
+#define APB2OPB_OPB1_WRITE_DATA TO_REG(0x38)
+
+#define APB2OPB_IRQ_STSTO_REG(0x48)
+#define APB2OPB_IRQ_STS_OPB1_TX_ACK BIT(17)
+#define APB2OPB_IRQ_STS_OPB0_TX_ACK BIT(16)
+
+#define APB2OPB_OPB0_WRITE_WORD_ENDIAN TO_REG(0x4c)
+#define APB2OPB_OPB0_WRITE_WORD_ENDIAN_BE 0x0011101b
+#define APB2OPB_OPB0_WRITE_BYTE_ENDIAN
