On Sun, 31 Oct 2021 16:45:41 +0000, "Z.Q. Hou" zhiqiang.hou@nxp.com wrote:
-----Original Message----- From: Marc Zyngier [mailto:maz@kernel.org] Sent: 2021年10月29日 5:09 To: Michael Walle michael@walle.cc Cc: u-boot@lists.denx.de; Vladimir Oltean vladimir.oltean@nxp.com; Z.Q. Hou zhiqiang.hou@nxp.com; Bharat Gooty bharat.gooty@broadcom.com; Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com; Simon Glass sjg@chromium.org; Priyanka Jain priyanka.jain@nxp.com; Tom Rini trini@konsulko.com Subject: Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
On Wed, 27 Oct 2021 17:54:54 +0100, Michael Walle michael@walle.cc wrote:
Stop using the device tree as a source for ad-hoc information.
This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310.
Signed-off-by: Michael Walle michael@walle.cc
arch/arm/Kconfig | 2 - arch/arm/cpu/armv8/fsl-layerscape/soc.c | 27 +++++++++- arch/arm/include/asm/gic-v3.h | 4 +- arch/arm/lib/gic-v3-its.c | 66 +++---------------------- 4 files changed, 36 insertions(+), 63 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 02f8306f15..86c1ebde05 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -82,8 +82,6 @@ config GICV3
config GIC_V3_ITS bool "ARM GICV3 ITS"
- select REGMAP
- select SYSCON select IRQ help ARM GICV3 Interrupt translation service (ITS).
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c b/arch/arm/cpu/armv8/fsl-layerscape/soc.c index c0e100d21c..a08ed3f544 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
Why is this FSL specific?
@@ -41,11 +41,36 @@ DECLARE_GLOBAL_DATA_PTR; #endif
#ifdef CONFIG_GIC_V3_ITS +#define PENDTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS), SZ_64K) +#define PROPTABLE_MAX_SZ ALIGN(BIT(ITS_MAX_LPI_NRBITS) / 8,
SZ_64K)
This looks completely wrong.
The pending table needs one bit per LPI, and the property table one byte per LPI. Here, you have it the other way around.
It's a typo, will fix after the revert patch applied.
A typo that has the potential to corrupt to corrupt memory. This clearly was never tested.
Also, the property table alignment requirement is 4kB, not 64kB, and its size is defined as the maximum number of LPIs - 8192.
As in the accessor gic_lpi_tables_init() there isn't alignment operation for both property table and pending table, we have to pass a 64KB alignment address, even though the property table only requires 4KB alignment.
So how about fixing it?
Finally, ITS_MAX_LPI_NRBITS is hardcoded to 16, while it can actually vary from 14 to 32 (and even further limited by some hypervisors), depending on the implementation. Granted, this was broken before this patch, and in most cases, 64k is more than enough.
This is only for Layerscape platforms, so hardcoded to 16 bit works.
Let me say it again: hardcoding things for a specific platform for no good reason is utterly wasteful. There is no reason to write SoC specific code here, and allocating tables should be done in an architectural way.
However, given that this defining the number of LPIs for the lifetime of the system, it would be better to actually allocate what the HW advertises (GICD_TYPER.IDbits, capped by GICD_TYPER.num_LPIs).
+#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + \
PROPTABLE_MAX_SZ, SZ_1M)Why the 1MB alignment? There is no such requirement in the architecture (64kB for the pending tables, 4kB for the property table).
This is definition of the size instead of address, 1MB size alignment is to ensure we have enough space to do address alignment, perhaps 64KB should be enough.
Again: straying out of the architecture requirement is wasteful. The architecture states all the requirements. How about you follow them instead of picking random numbers?
+static int fdt_add_resv_mem_gic_rd_tables(void *blob, u64 base, +size_t size) {
- int err;
- struct fdt_memory gic_rd_tables;
- gic_rd_tables.start = base;
- gic_rd_tables.end = base + size - 1;
- err = fdtdec_add_reserved_memory(blob, "gic-rd-tables", &gic_rd_tables,
NULL, 0, NULL, 0);- if (err < 0)
debug("%s: failed to add reserved memory: %d\n", __func__, err);- return err;
+}
int ls_gic_rd_tables_init(void *blob) {
- u64 gic_lpi_base; int ret;
- ret = gic_lpi_tables_init();
- gic_lpi_base = ALIGN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K);
- ret = fdt_add_resv_mem_gic_rd_tables(blob, gic_lpi_base, GIC_LPI_SIZE);
- if (ret)
return ret;- ret = gic_lpi_tables_init(gic_lpi_base, cpu_numcores());
This really should fetch the number of CPUs from the DT rather then some SoC specific black magic...
Currently in most Layerscape platforms' DTS file there isn't cpu nodes. On Layerscape platforms the implemented core number can be get from GUT register.
The CPU nodes will eventually be generated, if only for the sake of being able to boot an operating system. Given that the table allocation serves no purpose for u-boot itself, such allocation can be delayed until the nodes are populated, and the number of CPUs known.
No need for anything SoC specific whatsoever. Really, you should lose the SoC-specific mindset here, because you are:
- reinventing the wheel - introducing stupid bugs - making life difficult for everyone else
All of which are the hallmarks of bad engineering.
M.