[PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used in linux, there is this nice piece:
gic_lpi_base: syscon@0x80000000 { compatible = "gic-lpi-base"; reg = <0x0 0x80000000 0x0 0x100000>; max-gic-redistributors = <2>; };
There is no offical binding for it. Also, the chances that there will be one are virtually zero. We need to get rid of it. In fact, most information there are already known or can be deduced via the offical binding.
More than a month ago NXP [1] said they will look into it and try to get the bindings together. I don't think this will happen. Actually, I don't think the culprit is that commit, but rather the one which introduced that broken binding in the first place [2]. Therefore, revert it, too.
The funny thing is, I don't even know why this is needed at all. Linux will happily setup the LPI for us. At least for the LS1028A (but I guess also for all other layerscape SoC) the u-boot LPI setup code is only called right before we jump to linux. So u-boot doesn't even seem to use the interrupts. Now I can't speak of the Broadcom NS3 SoC where this 'feature' was introduced in the first place [3]. Unfortunately, it's not mentioned in the commit log *why* it was introduced. But this also seem to have crept into the layerscape SoCs [4]. All layerscape boards have CONFIG_GIC_V3_ITS enabled, well except for one; mine, the kontron_sl28 (which has a LS1028A). I haven't noticed anything out of the ordinary, though. Why is CONFIG_GIC_V3_ITS needed?
I briefly tested this on my board with CONFIG_GIC_V3_ITS enabled, at least linux prints: [ 0.000000] GICv3: Using preallocated redistributor tables
and there will be a reserved memory area: reserved-memory { #address-cells = <0x02>; #size-cells = <0x02>; ranges;
gic-rd-tables@20,fff00000 { reg = <0x20 0xfff00000 0x00 0x100000>; }; };
Also please note, that the reverts needed some manual adjustments because there were changes in the meantime.
While this will hopefully not break the layerscape boards, it will definetly break the Broadcom NS3, because the gic_lpi_tables_init() is called without arguments. But, these information are also not available in u-boot't device tree for the ns3. Soooo..
[1] https://lore.kernel.org/all/20210825210510.24766-1-trini@konsulko.com/ [2] https://lore.kernel.org/u-boot/20200726170733.30214-1-rayagonda.kokatanur@br... [3] https://lore.kernel.org/u-boot/20200610104120.30668-10-rayagonda.kokatanur@b... [4] https://lore.kernel.org/u-boot/20200428021935.27659-1-Zhiqiang.Hou@nxp.com/
Michael Walle (1): Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Tom Rini (1): Revert "arm64: Layerscape: Survive LPI one-way reset workaround"
arch/arm/Kconfig | 2 - arch/arm/cpu/armv8/fsl-layerscape/soc.c | 41 +++++++++------ arch/arm/dts/fsl-ls1028a.dtsi | 6 --- arch/arm/dts/fsl-ls1088a.dtsi | 6 --- arch/arm/dts/fsl-ls2080a.dtsi | 6 --- arch/arm/dts/fsl-lx2160a.dtsi | 6 --- arch/arm/include/asm/gic-v3.h | 4 +- arch/arm/lib/gic-v3-its.c | 66 +++---------------------- 8 files changed, 35 insertions(+), 102 deletions(-)
From: Tom Rini trini@konsulko.com
Ad-hoc bindings that are not part of the upstream device tree / bindings are not allowed in-tree. Only bindings that are in-progress with upstream and then re-synced once agreed upon are.
This reverts commit af288cb291da3abef6be0875527729296f7de7a0.
Cc: Hou Zhiqiang Zhiqiang.Hou@nxp.com Cc: Priyanka Jain priyanka.jain@nxp.com Reported-by: Michael Walle michael@walle.cc Signed-off-by: Tom Rini trini@konsulko.com --- arch/arm/cpu/armv8/fsl-layerscape/soc.c | 18 +----------------- arch/arm/dts/fsl-ls1028a.dtsi | 6 ------ arch/arm/dts/fsl-ls1088a.dtsi | 6 ------ arch/arm/dts/fsl-ls2080a.dtsi | 6 ------ arch/arm/dts/fsl-lx2160a.dtsi | 6 ------ 5 files changed, 1 insertion(+), 41 deletions(-)
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c b/arch/arm/cpu/armv8/fsl-layerscape/soc.c index 9820d3290e..c0e100d21c 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c @@ -43,23 +43,7 @@ DECLARE_GLOBAL_DATA_PTR; #ifdef CONFIG_GIC_V3_ITS int ls_gic_rd_tables_init(void *blob) { - struct fdt_memory lpi_base; - fdt_addr_t addr; - fdt_size_t size; - int offset, ret; - - offset = fdt_path_offset(gd->fdt_blob, "/syscon@0x80000000"); - addr = fdtdec_get_addr_size_auto_noparent(gd->fdt_blob, offset, "reg", - 0, &size, false); - - lpi_base.start = addr; - lpi_base.end = addr + size - 1; - ret = fdtdec_add_reserved_memory(blob, "lpi_rd_table", &lpi_base, NULL, - 0, NULL, 0); - if (ret) { - debug("%s: failed to add reserved memory\n", __func__); - return ret; - } + int ret;
ret = gic_lpi_tables_init(); if (ret) diff --git a/arch/arm/dts/fsl-ls1028a.dtsi b/arch/arm/dts/fsl-ls1028a.dtsi index 50f9b527cd..53b052ed32 100644 --- a/arch/arm/dts/fsl-ls1028a.dtsi +++ b/arch/arm/dts/fsl-ls1028a.dtsi @@ -44,12 +44,6 @@ IRQ_TYPE_LEVEL_LOW)>; };
- gic_lpi_base: syscon@0x80000000 { - compatible = "gic-lpi-base"; - reg = <0x0 0x80000000 0x0 0x100000>; - max-gic-redistributors = <2>; - }; - timer { compatible = "arm,armv8-timer"; interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | diff --git a/arch/arm/dts/fsl-ls1088a.dtsi b/arch/arm/dts/fsl-ls1088a.dtsi index 64caa600ad..3a5a50fb83 100644 --- a/arch/arm/dts/fsl-ls1088a.dtsi +++ b/arch/arm/dts/fsl-ls1088a.dtsi @@ -27,12 +27,6 @@ interrupts = <1 9 0x4>; };
- gic_lpi_base: syscon@0x80000000 { - compatible = "gic-lpi-base"; - reg = <0x0 0x80000000 0x0 0x100000>; - max-gic-redistributors = <8>; - }; - timer { compatible = "arm,armv8-timer"; interrupts = <1 13 0x8>, /* Physical Secure PPI, active-low */ diff --git a/arch/arm/dts/fsl-ls2080a.dtsi b/arch/arm/dts/fsl-ls2080a.dtsi index 7374d580e0..278daeeb6e 100644 --- a/arch/arm/dts/fsl-ls2080a.dtsi +++ b/arch/arm/dts/fsl-ls2080a.dtsi @@ -27,12 +27,6 @@ interrupts = <1 9 0x4>; };
- gic_lpi_base: syscon@0x80000000 { - compatible = "gic-lpi-base"; - reg = <0x0 0x80000000 0x0 0x100000>; - max-gic-redistributors = <8>; - }; - timer { compatible = "arm,armv8-timer"; interrupts = <1 13 0x8>, /* Physical Secure PPI, active-low */ diff --git a/arch/arm/dts/fsl-lx2160a.dtsi b/arch/arm/dts/fsl-lx2160a.dtsi index a6f0e9bc56..3b5f0d119e 100644 --- a/arch/arm/dts/fsl-lx2160a.dtsi +++ b/arch/arm/dts/fsl-lx2160a.dtsi @@ -43,12 +43,6 @@ interrupts = <1 9 0x4>; };
- gic_lpi_base: syscon@0x80000000 { - compatible = "gic-lpi-base"; - reg = <0x0 0x80000000 0x0 0x200000>; - max-gic-redistributors = <16>; - }; - timer { compatible = "arm,armv8-timer"; interrupts = <1 13 0x8>, /* Physical Secure PPI, active-low */
On Wed, 27 Oct 2021 17:54:53 +0100, Michael Walle michael@walle.cc wrote:
From: Tom Rini trini@konsulko.com
Ad-hoc bindings that are not part of the upstream device tree / bindings are not allowed in-tree. Only bindings that are in-progress with upstream and then re-synced once agreed upon are.
This reverts commit af288cb291da3abef6be0875527729296f7de7a0.
Cc: Hou Zhiqiang Zhiqiang.Hou@nxp.com Cc: Priyanka Jain priyanka.jain@nxp.com Reported-by: Michael Walle michael@walle.cc Signed-off-by: Tom Rini trini@konsulko.com
arch/arm/cpu/armv8/fsl-layerscape/soc.c | 18 +----------------- arch/arm/dts/fsl-ls1028a.dtsi | 6 ------ arch/arm/dts/fsl-ls1088a.dtsi | 6 ------ arch/arm/dts/fsl-ls2080a.dtsi | 6 ------ arch/arm/dts/fsl-lx2160a.dtsi | 6 ------ 5 files changed, 1 insertion(+), 41 deletions(-)
Acked-by: Marc Zyngier maz@kernel.org
I have a number of comments on the second patch, but getting rid of this gunk is a good start.
Thanks,
M.
On Wed, Oct 27, 2021 at 06:54:53PM +0200, Michael Walle wrote:
From: Tom Rini trini@konsulko.com
Ad-hoc bindings that are not part of the upstream device tree / bindings are not allowed in-tree. Only bindings that are in-progress with upstream and then re-synced once agreed upon are.
This reverts commit af288cb291da3abef6be0875527729296f7de7a0.
Cc: Hou Zhiqiang Zhiqiang.Hou@nxp.com Cc: Priyanka Jain priyanka.jain@nxp.com Reported-by: Michael Walle michael@walle.cc Signed-off-by: Tom Rini trini@konsulko.com Acked-by: Marc Zyngier maz@kernel.org
Applied to u-boot/master, thanks!
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 @@ -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) +#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + \ + PROPTABLE_MAX_SZ, SZ_1M) +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()); if (ret) debug("%s: failed to init gic-lpi-tables\n", __func__);
diff --git a/arch/arm/include/asm/gic-v3.h b/arch/arm/include/asm/gic-v3.h index 35efec78c3..5131fabec4 100644 --- a/arch/arm/include/asm/gic-v3.h +++ b/arch/arm/include/asm/gic-v3.h @@ -127,9 +127,9 @@ #define GIC_REDISTRIBUTOR_OFFSET 0x20000
#ifdef CONFIG_GIC_V3_ITS -int gic_lpi_tables_init(void); +int gic_lpi_tables_init(u64 base, u32 max_redist); #else -int gic_lpi_tables_init(void) +int gic_lpi_tables_init(u64 base, u32 max_redist) { return 0; } diff --git a/arch/arm/lib/gic-v3-its.c b/arch/arm/lib/gic-v3-its.c index 2d3fdb600e..f6211a2d92 100644 --- a/arch/arm/lib/gic-v3-its.c +++ b/arch/arm/lib/gic-v3-its.c @@ -5,8 +5,6 @@ #include <common.h> #include <cpu_func.h> #include <dm.h> -#include <regmap.h> -#include <syscon.h> #include <asm/gic.h> #include <asm/gic-v3.h> #include <asm/io.h> @@ -19,22 +17,15 @@ static u32 lpi_id_bits; #define LPI_PROPBASE_SZ ALIGN(BIT(LPI_NRBITS), SZ_64K) #define LPI_PENDBASE_SZ ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
-/* Number of GIC re-distributors */ -#define MAX_GIC_REDISTRIBUTORS 8 - /* * gic_v3_its_priv - gic details * * @gicd_base: gicd base address * @gicr_base: gicr base address - * @lpi_base: gic lpi base address - * @num_redist: number of gic re-distributors */ struct gic_v3_its_priv { ulong gicd_base; ulong gicr_base; - ulong lpi_base; - u32 num_redist; };
static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv *priv) @@ -68,39 +59,13 @@ static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv *priv) return 0; }
-static int gic_v3_its_get_gic_lpi_addr(struct gic_v3_its_priv *priv) -{ - struct regmap *regmap; - struct udevice *dev; - int ret; - - ret = uclass_get_device_by_driver(UCLASS_SYSCON, - DM_DRIVER_GET(gic_lpi_syscon), &dev); - if (ret) { - pr_err("%s: failed to get %s syscon device\n", __func__, - DM_DRIVER_GET(gic_lpi_syscon)->name); - return ret; - } - - regmap = syscon_get_regmap(dev); - if (!regmap) { - pr_err("%s: failed to regmap for %s syscon device\n", __func__, - DM_DRIVER_GET(gic_lpi_syscon)->name); - return -ENODEV; - } - priv->lpi_base = regmap->ranges[0].start; - - priv->num_redist = dev_read_u32_default(dev, "max-gic-redistributors", - MAX_GIC_REDISTRIBUTORS); - - return 0; -} - /* * Program the GIC LPI configuration tables for all * the re-distributors and enable the LPI table + * base: Configuration table address + * num_redist: number of redistributors */ -int gic_lpi_tables_init(void) +int gic_lpi_tables_init(u64 base, u32 num_redist) { struct gic_v3_its_priv priv; u32 gicd_typer; @@ -109,15 +74,12 @@ int gic_lpi_tables_init(void) int i; u64 redist_lpi_base; u64 pend_base; - ulong pend_tab_total_sz; + ulong pend_tab_total_sz = num_redist * LPI_PENDBASE_SZ; void *pend_tab_va;
if (gic_v3_its_get_gic_addr(&priv)) return -EINVAL;
- if (gic_v3_its_get_gic_lpi_addr(&priv)) - return -EINVAL; - gicd_typer = readl((uintptr_t)(priv.gicd_base + GICD_TYPER)); /* GIC support for Locality specific peripheral interrupts (LPI's) */ if (!(gicd_typer & GICD_TYPER_LPIS)) { @@ -130,7 +92,7 @@ int gic_lpi_tables_init(void) * Once the LPI table is enabled, can not program the * LPI configuration tables again, unless the GIC is reset. */ - for (i = 0; i < priv.num_redist; i++) { + for (i = 0; i < num_redist; i++) { u32 offset = i * GIC_REDISTRIBUTOR_OFFSET;
if ((readl((uintptr_t)(priv.gicr_base + offset))) & @@ -146,7 +108,7 @@ int gic_lpi_tables_init(void) ITS_MAX_LPI_NRBITS);
/* Set PropBase */ - val = (priv.lpi_base | + val = (base | GICR_PROPBASER_INNERSHAREABLE | GICR_PROPBASER_RAWAWB | ((LPI_NRBITS - 1) & GICR_PROPBASER_IDBITS_MASK)); @@ -163,8 +125,7 @@ int gic_lpi_tables_init(void) } }
- redist_lpi_base = priv.lpi_base + LPI_PROPBASE_SZ; - pend_tab_total_sz = priv.num_redist * LPI_PENDBASE_SZ; + redist_lpi_base = base + LPI_PROPBASE_SZ; pend_tab_va = map_physmem(redist_lpi_base, pend_tab_total_sz, MAP_NOCACHE); memset(pend_tab_va, 0, pend_tab_total_sz); @@ -172,7 +133,7 @@ int gic_lpi_tables_init(void) unmap_physmem(pend_tab_va, MAP_NOCACHE);
pend_base = priv.gicr_base + GICR_PENDBASER; - for (i = 0; i < priv.num_redist; i++) { + for (i = 0; i < num_redist; i++) { u32 offset = i * GIC_REDISTRIBUTOR_OFFSET;
val = ((redist_lpi_base + (i * LPI_PENDBASE_SZ)) | @@ -207,14 +168,3 @@ U_BOOT_DRIVER(arm_gic_v3_its) = { .id = UCLASS_IRQ, .of_match = gic_v3_its_ids, }; - -static const struct udevice_id gic_lpi_syscon_ids[] = { - { .compatible = "gic-lpi-base" }, - {} -}; - -U_BOOT_DRIVER(gic_lpi_syscon) = { - .name = "gic-lpi-base", - .id = UCLASS_SYSCON, - .of_match = gic_lpi_syscon_ids, -};
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. Also, the property table alignment requirement is 4kB, not 64kB, and its size is defined as the maximum number of LPIs - 8192.
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.
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).
+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...
if (ret) debug("%s: failed to init gic-lpi-tables\n", __func__);
diff --git a/arch/arm/include/asm/gic-v3.h b/arch/arm/include/asm/gic-v3.h index 35efec78c3..5131fabec4 100644 --- a/arch/arm/include/asm/gic-v3.h +++ b/arch/arm/include/asm/gic-v3.h @@ -127,9 +127,9 @@ #define GIC_REDISTRIBUTOR_OFFSET 0x20000
#ifdef CONFIG_GIC_V3_ITS -int gic_lpi_tables_init(void); +int gic_lpi_tables_init(u64 base, u32 max_redist); #else -int gic_lpi_tables_init(void) +int gic_lpi_tables_init(u64 base, u32 max_redist) { return 0; } diff --git a/arch/arm/lib/gic-v3-its.c b/arch/arm/lib/gic-v3-its.c index 2d3fdb600e..f6211a2d92 100644 --- a/arch/arm/lib/gic-v3-its.c +++ b/arch/arm/lib/gic-v3-its.c @@ -5,8 +5,6 @@ #include <common.h> #include <cpu_func.h> #include <dm.h> -#include <regmap.h> -#include <syscon.h> #include <asm/gic.h> #include <asm/gic-v3.h> #include <asm/io.h> @@ -19,22 +17,15 @@ static u32 lpi_id_bits; #define LPI_PROPBASE_SZ ALIGN(BIT(LPI_NRBITS), SZ_64K) #define LPI_PENDBASE_SZ ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
This is marginally more correct, but you have to wonder why you have the same thing defined twice...
-/* Number of GIC re-distributors */ -#define MAX_GIC_REDISTRIBUTORS 8
/*
- gic_v3_its_priv - gic details
- @gicd_base: gicd base address
- @gicr_base: gicr base address
- @lpi_base: gic lpi base address
*/
- @num_redist: number of gic re-distributors
struct gic_v3_its_priv { ulong gicd_base; ulong gicr_base;
- ulong lpi_base;
- u32 num_redist;
};
static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv *priv) @@ -68,39 +59,13 @@ static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv *priv) return 0; }
-static int gic_v3_its_get_gic_lpi_addr(struct gic_v3_its_priv *priv) -{
- struct regmap *regmap;
- struct udevice *dev;
- int ret;
- ret = uclass_get_device_by_driver(UCLASS_SYSCON,
DM_DRIVER_GET(gic_lpi_syscon), &dev);- if (ret) {
pr_err("%s: failed to get %s syscon device\n", __func__,DM_DRIVER_GET(gic_lpi_syscon)->name);return ret;- }
- regmap = syscon_get_regmap(dev);
- if (!regmap) {
pr_err("%s: failed to regmap for %s syscon device\n", __func__,DM_DRIVER_GET(gic_lpi_syscon)->name);return -ENODEV;- }
- priv->lpi_base = regmap->ranges[0].start;
- priv->num_redist = dev_read_u32_default(dev, "max-gic-redistributors",
MAX_GIC_REDISTRIBUTORS);- return 0;
-}
/*
- Program the GIC LPI configuration tables for all
- the re-distributors and enable the LPI table
... enable LPI forwarding, not the tables...
- base: Configuration table address
*/
- num_redist: number of redistributors
-int gic_lpi_tables_init(void) +int gic_lpi_tables_init(u64 base, u32 num_redist) { struct gic_v3_its_priv priv; u32 gicd_typer; @@ -109,15 +74,12 @@ int gic_lpi_tables_init(void) int i; u64 redist_lpi_base; u64 pend_base;
- ulong pend_tab_total_sz;
ulong pend_tab_total_sz = num_redist * LPI_PENDBASE_SZ; void *pend_tab_va;
if (gic_v3_its_get_gic_addr(&priv)) return -EINVAL;
- if (gic_v3_its_get_gic_lpi_addr(&priv))
return -EINVAL;- gicd_typer = readl((uintptr_t)(priv.gicd_base + GICD_TYPER)); /* GIC support for Locality specific peripheral interrupts (LPI's) */ if (!(gicd_typer & GICD_TYPER_LPIS)) {
@@ -130,7 +92,7 @@ int gic_lpi_tables_init(void) * Once the LPI table is enabled, can not program the * LPI configuration tables again, unless the GIC is reset. */
- for (i = 0; i < priv.num_redist; i++) {
for (i = 0; i < num_redist; i++) { u32 offset = i * GIC_REDISTRIBUTOR_OFFSET;
if ((readl((uintptr_t)(priv.gicr_base + offset))) &
@@ -146,7 +108,7 @@ int gic_lpi_tables_init(void) ITS_MAX_LPI_NRBITS);
/* Set PropBase */
- val = (priv.lpi_base |
- val = (base | GICR_PROPBASER_INNERSHAREABLE | GICR_PROPBASER_RAWAWB | ((LPI_NRBITS - 1) & GICR_PROPBASER_IDBITS_MASK));
@@ -163,8 +125,7 @@ int gic_lpi_tables_init(void) } }
- redist_lpi_base = priv.lpi_base + LPI_PROPBASE_SZ;
- pend_tab_total_sz = priv.num_redist * LPI_PENDBASE_SZ;
- redist_lpi_base = base + LPI_PROPBASE_SZ; pend_tab_va = map_physmem(redist_lpi_base, pend_tab_total_sz, MAP_NOCACHE); memset(pend_tab_va, 0, pend_tab_total_sz);
@@ -172,7 +133,7 @@ int gic_lpi_tables_init(void) unmap_physmem(pend_tab_va, MAP_NOCACHE);
pend_base = priv.gicr_base + GICR_PENDBASER;
- for (i = 0; i < priv.num_redist; i++) {
for (i = 0; i < num_redist; i++) { u32 offset = i * GIC_REDISTRIBUTOR_OFFSET;
val = ((redist_lpi_base + (i * LPI_PENDBASE_SZ)) |
@@ -207,14 +168,3 @@ U_BOOT_DRIVER(arm_gic_v3_its) = { .id = UCLASS_IRQ, .of_match = gic_v3_its_ids, };
-static const struct udevice_id gic_lpi_syscon_ids[] = {
- { .compatible = "gic-lpi-base" },
- {}
-};
-U_BOOT_DRIVER(gic_lpi_syscon) = {
- .name = "gic-lpi-base",
- .id = UCLASS_SYSCON,
- .of_match = gic_lpi_syscon_ids,
-};
Thanks,
M.
On Thu, Oct 28, 2021 at 10:09:25PM +0100, Marc Zyngier wrote:
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?
Note that this is a fairly direct revert. Based on your comments a follow-up patch to correct things is in order.
Hi,
thanks for the review, as Tom already mentioned this is just a revert. I'm still unsure wether I should care or not. Actually, NXP or Broadcom should. OTOH it would be nice if the kontron_sl28 can do kexec also with booti (and not just bootefi). Anyway, I still have some remarks.
Am 2021-10-28 23:09, schrieb Marc Zyngier:
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
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...
Remember that this is the bootloader. There might be a chicken egg problem. I guess, usually the bootloader will fixup the number of cores in the DT. Therefore, if we rely on the DT here, it has to be fixed up before this code is run.
Conceptually, I'm unsure if this should actually be a driver (UCLASS_IRQ). It's just setting up the interrupt controller, it doesn't provide any ops. So it might well be called from somewhere else instead of binding as a driver to the gic.
If it will be a driver, then the call to gic_lpi_tables_init() should go away. Instead the driver should just set the tables up.
If its not a driver, then gic_lpi_tables_init() (maybe renamed to gic_v3_lpi_tables_init()) should work without anything else and it should scan the device tree for the compatible node and fetch all needed information to set up the tables.
In any case, all this code should be guarded by a Kconfig option which clearly states *why* this is needed.
Thoughts?
Michael,
On Fri, 29 Oct 2021 12:49:21 +0100, Michael Walle michael@walle.cc wrote:
Hi,
thanks for the review, as Tom already mentioned this is just a revert. I'm still unsure wether I should care or not. Actually, NXP or Broadcom should. OTOH it would be nice if the kontron_sl28 can do kexec also with booti (and not just bootefi). Anyway, I still have some remarks.
Am 2021-10-28 23:09, schrieb Marc Zyngier:
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
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...
Remember that this is the bootloader. There might be a chicken egg problem. I guess, usually the bootloader will fixup the number of cores in the DT. Therefore, if we rely on the DT here, it has to be fixed up before this code is run.
I appreciate that. However...
Conceptually, I'm unsure if this should actually be a driver (UCLASS_IRQ). It's just setting up the interrupt controller, it doesn't provide any ops. So it might well be called from somewhere else instead of binding as a driver to the gic.
If it will be a driver, then the call to gic_lpi_tables_init() should go away. Instead the driver should just set the tables up.
If its not a driver, then gic_lpi_tables_init() (maybe renamed to gic_v3_lpi_tables_init()) should work without anything else and it should scan the device tree for the compatible node and fetch all needed information to set up the tables.
Exactly. This thing doesn't provide *any* service to u-boot itself. It just grabs some memory, point a couple of registers at it, and flip a bit. There is no actual ITS driver, so nothing makes use of it.
So this can be run as late as you want, long after you have worked out how many CPUs you really have. Which means this can be done in a SoC-agnostic way.
In any case, all this code should be guarded by a Kconfig option which clearly states *why* this is needed.
Even more: if it is selected, it should be possible to disable this at runtime (environment variable?) and leave the OS in charge of it. This really should an opt-in, instead of being forced on the users.
Thanks,
M.
-----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.
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.
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.
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.
+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.
Thanks, Zhiqiang
if (ret) debug("%s: failed to init gic-lpi-tables\n", __func__);
diff --git a/arch/arm/include/asm/gic-v3.h b/arch/arm/include/asm/gic-v3.h index 35efec78c3..5131fabec4 100644 --- a/arch/arm/include/asm/gic-v3.h +++ b/arch/arm/include/asm/gic-v3.h @@ -127,9 +127,9 @@ #define GIC_REDISTRIBUTOR_OFFSET 0x20000
#ifdef CONFIG_GIC_V3_ITS -int gic_lpi_tables_init(void); +int gic_lpi_tables_init(u64 base, u32 max_redist); #else -int gic_lpi_tables_init(void) +int gic_lpi_tables_init(u64 base, u32 max_redist) { return 0; } diff --git a/arch/arm/lib/gic-v3-its.c b/arch/arm/lib/gic-v3-its.c index 2d3fdb600e..f6211a2d92 100644 --- a/arch/arm/lib/gic-v3-its.c +++ b/arch/arm/lib/gic-v3-its.c @@ -5,8 +5,6 @@ #include <common.h> #include <cpu_func.h> #include <dm.h> -#include <regmap.h> -#include <syscon.h> #include <asm/gic.h> #include <asm/gic-v3.h> #include <asm/io.h> @@ -19,22 +17,15 @@ static u32 lpi_id_bits; #define LPI_PROPBASE_SZ ALIGN(BIT(LPI_NRBITS), SZ_64K) #define LPI_PENDBASE_SZ ALIGN(BIT(LPI_NRBITS) / 8, SZ_64K)
This is marginally more correct, but you have to wonder why you have the same thing defined twice...
-/* Number of GIC re-distributors */ -#define MAX_GIC_REDISTRIBUTORS 8
/*
- gic_v3_its_priv - gic details
- @gicd_base: gicd base address
- @gicr_base: gicr base address
- @lpi_base: gic lpi base address
*/
- @num_redist: number of gic re-distributors
struct gic_v3_its_priv { ulong gicd_base; ulong gicr_base;
- ulong lpi_base;
- u32 num_redist;
};
static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv *priv) @@ -68,39 +59,13 @@ static int gic_v3_its_get_gic_addr(struct gic_v3_its_priv
*priv)
return 0; }
-static int gic_v3_its_get_gic_lpi_addr(struct gic_v3_its_priv *priv) -{
- struct regmap *regmap;
- struct udevice *dev;
- int ret;
- ret = uclass_get_device_by_driver(UCLASS_SYSCON,
DM_DRIVER_GET(gic_lpi_syscon), &dev);- if (ret) {
pr_err("%s: failed to get %s syscon device\n", __func__,DM_DRIVER_GET(gic_lpi_syscon)->name);return ret;- }
- regmap = syscon_get_regmap(dev);
- if (!regmap) {
pr_err("%s: failed to regmap for %s syscon device\n", __func__,DM_DRIVER_GET(gic_lpi_syscon)->name);return -ENODEV;- }
- priv->lpi_base = regmap->ranges[0].start;
- priv->num_redist = dev_read_u32_default(dev, "max-gic-redistributors",
MAX_GIC_REDISTRIBUTORS);- return 0;
-}
/*
- Program the GIC LPI configuration tables for all
- the re-distributors and enable the LPI table
... enable LPI forwarding, not the tables...
- base: Configuration table address
*/
- num_redist: number of redistributors
-int gic_lpi_tables_init(void) +int gic_lpi_tables_init(u64 base, u32 num_redist) { struct gic_v3_its_priv priv; u32 gicd_typer; @@ -109,15 +74,12 @@ int gic_lpi_tables_init(void) int i; u64 redist_lpi_base; u64 pend_base;
- ulong pend_tab_total_sz;
ulong pend_tab_total_sz = num_redist * LPI_PENDBASE_SZ; void *pend_tab_va;
if (gic_v3_its_get_gic_addr(&priv)) return -EINVAL;
- if (gic_v3_its_get_gic_lpi_addr(&priv))
return -EINVAL;- gicd_typer = readl((uintptr_t)(priv.gicd_base + GICD_TYPER)); /* GIC support for Locality specific peripheral interrupts (LPI's) */ if (!(gicd_typer & GICD_TYPER_LPIS)) { @@ -130,7 +92,7 @@ int
gic_lpi_tables_init(void) * Once the LPI table is enabled, can not program the * LPI configuration tables again, unless the GIC is reset. */
- for (i = 0; i < priv.num_redist; i++) {
for (i = 0; i < num_redist; i++) { u32 offset = i * GIC_REDISTRIBUTOR_OFFSET;
if ((readl((uintptr_t)(priv.gicr_base + offset))) & @@ -146,7
+108,7 @@ int gic_lpi_tables_init(void) ITS_MAX_LPI_NRBITS);
/* Set PropBase */
- val = (priv.lpi_base |
- val = (base | GICR_PROPBASER_INNERSHAREABLE | GICR_PROPBASER_RAWAWB | ((LPI_NRBITS - 1) & GICR_PROPBASER_IDBITS_MASK)); @@ -163,8
+125,7 @@ int gic_lpi_tables_init(void) } }
- redist_lpi_base = priv.lpi_base + LPI_PROPBASE_SZ;
- pend_tab_total_sz = priv.num_redist * LPI_PENDBASE_SZ;
- redist_lpi_base = base + LPI_PROPBASE_SZ; pend_tab_va = map_physmem(redist_lpi_base, pend_tab_total_sz, MAP_NOCACHE); memset(pend_tab_va, 0, pend_tab_total_sz); @@ -172,7 +133,7 @@ int
gic_lpi_tables_init(void) unmap_physmem(pend_tab_va, MAP_NOCACHE);
pend_base = priv.gicr_base + GICR_PENDBASER;
- for (i = 0; i < priv.num_redist; i++) {
for (i = 0; i < num_redist; i++) { u32 offset = i * GIC_REDISTRIBUTOR_OFFSET;
val = ((redist_lpi_base + (i * LPI_PENDBASE_SZ)) | @@ -207,14
+168,3 @@ U_BOOT_DRIVER(arm_gic_v3_its) = { .id = UCLASS_IRQ, .of_match = gic_v3_its_ids, };
-static const struct udevice_id gic_lpi_syscon_ids[] = {
- { .compatible = "gic-lpi-base" },
- {}
-};
-U_BOOT_DRIVER(gic_lpi_syscon) = {
- .name = "gic-lpi-base",
- .id = UCLASS_SYSCON,
- .of_match = gic_lpi_syscon_ids,
-};
Thanks,
M.
-- Without deviation from the norm, progress is not possible.
Hi,
Am 2021-10-31 17:45, schrieb Z.Q. Hou:
-----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.
Please keep me on CC.
..
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.
But why is this layerscape only? Can't we make it so it works on any platform. If I understand Marc correctly, it should be possible.
..
- 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.
So this would be the perfect time to actually sync the device trees with linux.
-michael
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.
On Wed, Oct 27, 2021 at 06:54:54PM +0200, Michael Walle 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
With a quick change to make ns3 build but note the problem at runtime, applied to u-boot/master, thanks. And note that I expect a follow-up from interested parties to implement the required changes here correctly.
On Wed, 27 Oct 2021 17:54:52 +0100, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used in linux, there is this nice piece:
gic_lpi_base: syscon@0x80000000 { compatible = "gic-lpi-base"; reg = <0x0 0x80000000 0x0 0x100000>; max-gic-redistributors = <2>; };
There is no offical binding for it. Also, the chances that there will be one are virtually zero. We need to get rid of it. In fact, most information there are already known or can be deduced via the offical binding.
It is not "virtually zero". It is *exactly* zero. This node only shows that the author didn't understand the nature of the problem, nor were they aware of the existing solution which has been around since July 2018. This solution doesn't require any update to the binding, only to reserve the memory.
I really wish people would stop piling crap in u-boot, and that the u-boot maintainers would reach out to people familiar with the architecture before merging this sort of changes.
More than a month ago NXP [1] said they will look into it and try to get the bindings together. I don't think this will happen. Actually, I don't think the culprit is that commit, but rather the one which introduced that broken binding in the first place [2]. Therefore, revert it, too.
The funny thing is, I don't even know why this is needed at all. Linux will happily setup the LPI for us. At least for the LS1028A (but I guess also for all other layerscape SoC) the u-boot LPI setup code is only called right before we jump to linux. So u-boot doesn't even seem to use the interrupts. Now I can't speak of the Broadcom NS3 SoC where this 'feature' was introduced in the first place [3]. Unfortunately, it's not mentioned in the commit log *why* it was introduced. But this also seem to have crept into the layerscape SoCs [4]. All layerscape boards have CONFIG_GIC_V3_ITS enabled, well except for one; mine, the kontron_sl28 (which has a LS1028A). I haven't noticed anything out of the ordinary, though. Why is CONFIG_GIC_V3_ITS needed?
Now, that's a very interesting question: u-boot doesn't know how to drive an ITS (there is no support code for it, despite what arch/arm/lib/gic-v3-its.c suggest). Only the LPI allocation code is there. For what purpose? This is a pretty useless piece of code as far as I can see, unless the author had some unspecified ulterior motives (in which case a bit of documentation and a renaming of this file wouldn't go amiss).
Thanks,
M.
On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier maz@kernel.org wrote:
On Wed, 27 Oct 2021 17:54:52 +0100, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used
in
linux, there is this nice piece:
gic_lpi_base: syscon@0x80000000 { compatible = "gic-lpi-base"; reg = <0x0 0x80000000 0x0 0x100000>; max-gic-redistributors = <2>; };There is no offical binding for it. Also, the chances that there will be one are virtually zero. We need to get rid of it. In fact, most
information
there are already known or can be deduced via the offical binding.
It is not "virtually zero". It is *exactly* zero. This node only shows that the author didn't understand the nature of the problem, nor were they aware of the existing solution which has been around since July 2018. This solution doesn't require any update to the binding, only to reserve the memory.
I really wish people would stop piling crap in u-boot, and that the u-boot maintainers would reach out to people familiar with the architecture before merging this sort of changes.
More than a month ago NXP [1] said they will look into it and try to get the bindings together. I don't think this will happen. Actually, I don't think the culprit is that commit, but rather the one which introduced
that
broken binding in the first place [2]. Therefore, revert it, too.
The funny thing is, I don't even know why this is needed at all. Linux
will
happily setup the LPI for us. At least for the LS1028A (but I guess also for all other layerscape SoC) the u-boot LPI setup code is only called right before we jump to linux. So u-boot doesn't even seem to use the interrupts. Now I can't speak of the Broadcom NS3 SoC where this
'feature'
was introduced in the first place [3]. Unfortunately, it's not mentioned
in
the commit log *why* it was introduced. But this also seem to have crept into the layerscape SoCs [4]. All layerscape boards have
CONFIG_GIC_V3_ITS
enabled, well except for one; mine, the kontron_sl28 (which has a
LS1028A).
I haven't noticed anything out of the ordinary, though. Why is CONFIG_GIC_V3_ITS needed?
Now, that's a very interesting question: u-boot doesn't know how to drive an ITS (there is no support code for it, despite what arch/arm/lib/gic-v3-its.c suggest). Only the LPI allocation code is there. For what purpose? This is a pretty useless piece of code as far as I can see, unless the author had some unspecified ulterior motives (in which case a bit of documentation and a renaming of this file wouldn't go amiss).
Thanks,
M.-- Without deviation from the norm, progress is not possible.
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset. For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated. For these reasons, We allocate the LPI table memory in u-boot and reserve that memory. Sorry, I have not followed the code. Initially the GIC_LPI_BASE memory is defined in the platform file.
Thanks, -Bharat
On Thu, 28 Oct 2021 10:20:53 +0100, Bharat Gooty bharat.gooty@broadcom.com wrote:
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset.
Please tell me something I don't know...
For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated.
Since July 2018, the Linux kernel is perfectly able to deal with this without any extra support. It will communicate the reservation to the secondary kernel, and the secondary kernel will happily use this.
For these reasons, We allocate the LPI table memory in u-boot and reserve that memory.
What sort of antiquated kernel are you using? And even if you are running something that predates the kernel changes, reserving the memory in the DT serves the same purpose. Why the custom node declaring the allocation?
And since your kernel is able to kexec, it obviously knows about the reservation/pre-programming trick. This hardly makes any sense.
M.
On Thu, Oct 28, 2021 at 3:19 PM Marc Zyngier maz@kernel.org wrote:
On Thu, 28 Oct 2021 10:20:53 +0100, Bharat Gooty bharat.gooty@broadcom.com wrote:
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset.
Please tell me something I don't know...
For GIC V3 , once the Locality specific peripheral interrupts (LPI) table
is programmed and enabled, unless the GIC is reset, we can not re-program the LPI tables. For these reasons, reserve the memory and program the GIC redistributor PROPBASER and LPI_PENDBASE registers. If we do not program the LPI table in the bootloader, during the Linux boot, Linux will allocate the LPI table memory. Assume you want to boot a new kernel and do the kexec kernel. For the kexec kernel boot, Linux will again allocate the LPI table memory. But writing to the GIC registers will fail. As the LPI for the redistributors is already enabled by the previous Linux kernel, unless we do GIC reset, we can not update the LPI tables.
For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be
updated.
Since July 2018, the Linux kernel is perfectly able to deal with this without any extra support. It will communicate the reservation to the secondary kernel, and the secondary kernel will happily use this.
Can you specify the kernel version and the GIC versions which were used?
For these reasons, We allocate the LPI table memory in u-boot and reserve that memory.
What sort of antiquated kernel are you using? And even if you are running something that predates the kernel changes, reserving the memory in the DT serves the same purpose. Why the custom node declaring the allocation?
Tried using LTS 5.4 and 5.9 Linux kernels. Problem is updating the GIC V3
registers with the new LPI table memory after enabling the LPI for the redistributors.
And since your kernel is able to kexec, it obviously knows about the
reservation/pre-programming trick. This hardly makes any sense.
M.-- Without deviation from the norm, progress is not possible.
On Thu, 28 Oct 2021 11:27:44 +0100, Bharat Gooty bharat.gooty@broadcom.com wrote:
[1 <text/plain; UTF-8 (7bit)>] On Thu, Oct 28, 2021 at 3:19 PM Marc Zyngier maz@kernel.org wrote:
On Thu, 28 Oct 2021 10:20:53 +0100, Bharat Gooty bharat.gooty@broadcom.com wrote:
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset.
Please tell me something I don't know...
For GIC V3 , once the Locality specific peripheral interrupts (LPI) table
is programmed and enabled, unless the GIC is reset, we can not re-program the LPI tables. For these reasons, reserve the memory and program the GIC redistributor PROPBASER and LPI_PENDBASE registers. If we do not program the LPI table in the bootloader, during the Linux boot, Linux will allocate the LPI table memory. Assume you want to boot a new kernel and do the kexec kernel. For the kexec kernel boot, Linux will again allocate the LPI table memory. But writing to the GIC registers will fail. As the LPI for the redistributors is already enabled by the previous Linux kernel, unless we do GIC reset, we can not update the LPI tables.
This really was a rhetorical question. I am painfully aware of most of the GIC's "features", having dealt with it in Linux and at the architecture level for the past 10+ years.
For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be
updated.
Since July 2018, the Linux kernel is perfectly able to deal with this without any extra support. It will communicate the reservation to the secondary kernel, and the secondary kernel will happily use this.
Can you specify the kernel version and the GIC versions which were used?
Any kernel containing this commit:
commit 5e2c9f9a627772672accd80fa15359c0de6aa894 Author: Marc Zyngier maz@kernel.org Date: Fri Jul 27 16:23:18 2018 +0100
irqchip/gic-v3-its: Allow use of LPI tables in reserved memory
If the LPI tables have been reserved with the EFI reservation mechanism, we assume that these tables are safe to use even when we find the redistributors to have LPIs enabled at boot time, meaning that kexec can now work with GICv3.
You're welcome.
Tested-by: Jeremy Linton jeremy.linton@arm.com Tested-by: Bhupesh Sharma bhsharma@redhat.com Tested-by: Lei Zhang zhang.lei@jp.fujitsu.com Signed-off-by: Marc Zyngier marc.zyngier@arm.com
And the version of the IP doesn't matter at all.
For these reasons, We allocate the LPI table memory in u-boot and reserve that memory.
What sort of antiquated kernel are you using? And even if you are running something that predates the kernel changes, reserving the memory in the DT serves the same purpose. Why the custom node declaring the allocation?
Tried using LTS 5.4 and 5.9 Linux kernels. Problem is updating the GIC V3
registers with the new LPI table memory after enabling the LPI for the redistributors.
Then you are doing something wrong. There are two cases that work:
(1) You are using EFI: nothing to do. The kernel will reserve the memory, configure the RDs, and the secondary kernel will happily use the same memory, as the address is conveyed in an EFI-specific table, without attempting to reprogram the RDs
(2) You are not using EFI, and you need to reserve the memory *using the appropriate DT reservation mechanism* as well as program the RDs. The kernel will detect the reservation, and will not attempt to reprogram the RDs.
If you invent your own binding, use another reservation mechanism or otherwise, this will not work. u-boot shouldn't expose this syscon node, because it makes no sense at all, and no upstream software knows about it. This code must die.
M.
Am 2021-10-28 11:20, schrieb Bharat Gooty:
On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier maz@kernel.org wrote:
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset. For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated.
kexec.. this should have really gone into both the commit message _and_ the kconfig menu. In fact, it is really just a workaround for the kexec case. If I understand it correctly, the kernel is able to communicate the reserved memory area, but only if you have EFI support. So, as a workaround, the bootloader can pre-allocate the memory and put it in the device tree, which is then passed from the old to the new kernel and the reservation is preserved. Correct, Marc?
But all of this doesn't need any new device tree node.
-michael
On Thu, Oct 28, 2021 at 4:52 PM Michael Walle michael@walle.cc wrote:
Am 2021-10-28 11:20, schrieb Bharat Gooty:
On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier maz@kernel.org wrote:
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset. For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated.
kexec.. this should have really gone into both the commit message _and_ the kconfig menu. In fact, it is really just a workaround for the kexec case. If I understand it correctly, the kernel is able to communicate the reserved memory area, but only if you have EFI support. So, as a workaround, the bootloader can pre-allocate the memory and put it in the device tree, which is then passed from the old to the new kernel and the reservation is preserved. Correct, Marc?
If EFI support is enabled, that's true, Pre-allocate the memory and Kernel
can get that memory using EFI. What if EFI support is not enabled, like in a Broadcom NS3 or NXP platform? What is your suggestion for solving the kexec problem?
But all of this doesn't need any new device tree node.
-michael
Am 2021-10-28 13:35, schrieb Bharat Gooty:
On Thu, Oct 28, 2021 at 4:52 PM Michael Walle michael@walle.cc wrote:
Am 2021-10-28 11:20, schrieb Bharat Gooty:
On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier maz@kernel.org
wrote:
For GIC V3, once the LPI tables are programmed, we can not update
it,
unless we do a reset. For the kexec kernel, where the reboot does not happen, in this
case,
during the new kernel boot, the new LPI tables address will not be updated.
kexec.. this should have really gone into both the commit message _and_ the kconfig menu. In fact, it is really just a workaround for the kexec case. If I understand it correctly, the kernel is able to communicate the reserved memory area, but only if you have EFI support. So, as a workaround, the bootloader can pre-allocate the memory and put it in the device tree, which is then passed from the old to the new kernel and the reservation is preserved. Correct, Marc?
If EFI support is enabled, that's true, Pre-allocate the memory and Kernel can get that memory using EFI. What if EFI support is not enabled, like in a Broadcom NS3 or NXP platform? What is your suggestion for solving the kexec problem?
Iff that is correct what I've said above, then (1) rename the config symbol (I'm not sure, Tom?) and provide a better help text (2) drop the device tree node. after all you only have to allocate the node (3) keep most of the current code, but instead of reading the address from the device tree. Just allocate memory (within the alignment restrictions or whatever) and mark it as reserve it in the device tree. If I understood Marc correct, you can read the number of redistributors from the current gic-v3 binding.
Now, how and if this will fit into the u-boot device model, that's up to you.
In the meantime, it would be great if you can have a look at these two patches and trying to get them work for the ns3, so I can move forward with the device tree sync.
-michael
On Thu, 28 Oct 2021 12:21:59 +0100, Michael Walle michael@walle.cc wrote:
Am 2021-10-28 11:20, schrieb Bharat Gooty:
On Thu, Oct 28, 2021 at 2:33 PM Marc Zyngier maz@kernel.org wrote:
For GIC V3, once the LPI tables are programmed, we can not update it, unless we do a reset. For the kexec kernel, where the reboot does not happen, in this case, during the new kernel boot, the new LPI tables address will not be updated.
kexec.. this should have really gone into both the commit message _and_ the kconfig menu. In fact, it is really just a workaround for the kexec case. If I understand it correctly, the kernel is able to communicate the reserved memory area, but only if you have EFI support. So, as a workaround, the bootloader can pre-allocate the memory and put it in the device tree, which is then passed from the old to the new kernel and the reservation is preserved. Correct, Marc?
See my reply to Bharat. Either you use EFI, or you reserve the memory and program the RDs. In either of these two cases, kexec will just work.
But all of this doesn't need any new device tree node.
Exactly. The whole syscon stuff is a point hack that makes no sense, and which isn't parsed by any upstream SW.
M.
On Thu, Oct 28, 2021 at 10:01:34AM +0100, Marc Zyngier wrote:
On Wed, 27 Oct 2021 17:54:52 +0100, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used in linux, there is this nice piece:
gic_lpi_base: syscon@0x80000000 { compatible = "gic-lpi-base"; reg = <0x0 0x80000000 0x0 0x100000>; max-gic-redistributors = <2>; };
There is no offical binding for it. Also, the chances that there will be one are virtually zero. We need to get rid of it. In fact, most information there are already known or can be deduced via the offical binding.
It is not "virtually zero". It is *exactly* zero. This node only shows that the author didn't understand the nature of the problem, nor were they aware of the existing solution which has been around since July 2018. This solution doesn't require any update to the binding, only to reserve the memory.
I really wish people would stop piling crap in u-boot, and that the u-boot maintainers would reach out to people familiar with the architecture before merging this sort of changes.
I'd be happy to reach out to people if I knew who would be receptive to spending some of their already I assume overload spare time looking in to things. If you're volunteering for "GIC related things" I'd be happy to CC you when patches come up. Thanks!
On Thu, 28 Oct 2021 13:31:13 +0100, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 28, 2021 at 10:01:34AM +0100, Marc Zyngier wrote:
On Wed, 27 Oct 2021 17:54:52 +0100, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used in linux, there is this nice piece:
gic_lpi_base: syscon@0x80000000 { compatible = "gic-lpi-base"; reg = <0x0 0x80000000 0x0 0x100000>; max-gic-redistributors = <2>; };
There is no offical binding for it. Also, the chances that there will be one are virtually zero. We need to get rid of it. In fact, most information there are already known or can be deduced via the offical binding.
It is not "virtually zero". It is *exactly* zero. This node only shows that the author didn't understand the nature of the problem, nor were they aware of the existing solution which has been around since July 2018. This solution doesn't require any update to the binding, only to reserve the memory.
I really wish people would stop piling crap in u-boot, and that the u-boot maintainers would reach out to people familiar with the architecture before merging this sort of changes.
I'd be happy to reach out to people if I knew who would be receptive to spending some of their already I assume overload spare time looking in to things. If you're volunteering for "GIC related things" I'd be happy to CC you when patches come up. Thanks!
Absolutely. It is far less painful for me to quickly eyeball a change and ask pointed questions on the spot, rather than having to reverse engineer a set of dubious changes months after they have been merged.
I already provide similar "services" for EDK2, for example, so getting the odd u-boot patch in my k.org inbox isn't a big deal.
Thanks,
M.
On Thu, Oct 28, 2021 at 02:38:26PM +0100, Marc Zyngier wrote:
On Thu, 28 Oct 2021 13:31:13 +0100, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 28, 2021 at 10:01:34AM +0100, Marc Zyngier wrote:
On Wed, 27 Oct 2021 17:54:52 +0100, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
On the quest of syncing the device tree used in u-boot with the one used in linux, there is this nice piece:
gic_lpi_base: syscon@0x80000000 { compatible = "gic-lpi-base"; reg = <0x0 0x80000000 0x0 0x100000>; max-gic-redistributors = <2>; };
There is no offical binding for it. Also, the chances that there will be one are virtually zero. We need to get rid of it. In fact, most information there are already known or can be deduced via the offical binding.
It is not "virtually zero". It is *exactly* zero. This node only shows that the author didn't understand the nature of the problem, nor were they aware of the existing solution which has been around since July 2018. This solution doesn't require any update to the binding, only to reserve the memory.
I really wish people would stop piling crap in u-boot, and that the u-boot maintainers would reach out to people familiar with the architecture before merging this sort of changes.
I'd be happy to reach out to people if I knew who would be receptive to spending some of their already I assume overload spare time looking in to things. If you're volunteering for "GIC related things" I'd be happy to CC you when patches come up. Thanks!
Absolutely. It is far less painful for me to quickly eyeball a change and ask pointed questions on the spot, rather than having to reverse engineer a set of dubious changes months after they have been merged.
I already provide similar "services" for EDK2, for example, so getting the odd u-boot patch in my k.org inbox isn't a big deal.
Will do, thanks!
Hi Michael,
On Wed, 27 Oct 2021 at 10:55, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
Can I suggest that your commit subject be a little more specific? Perhaps 'Drop use of unwanted binding' ? It seems quite vague to me, like 'Fix a horrible bug' or 'Sort out all the problems' :-)
Regards, Simon
Am 2021-10-29 00:36, schrieb Simon Glass:
On Wed, 27 Oct 2021 at 10:55, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
Can I suggest that your commit subject be a little more specific?
Sure. Actually, it should have be a bit provocant; and the cover letter subject doesn't really matter, does it?
Perhaps 'Drop use of unwanted binding' ? It seems quite vague to me, like 'Fix a horrible bug' or 'Sort out all the problems' :-)
-michael
Hi Michael,
On Fri, 29 Oct 2021 at 05:54, Michael Walle michael@walle.cc wrote:
Am 2021-10-29 00:36, schrieb Simon Glass:
On Wed, 27 Oct 2021 at 10:55, Michael Walle michael@walle.cc wrote:
Please stop throwing every ad-hoc information in the device tree. Use the official bindings (or maybe some bindings which will get approved soon).
Can I suggest that your commit subject be a little more specific?
Sure. Actually, it should have be a bit provocant; and the cover letter subject doesn't really matter, does it?
I suppose not and it is somewhat entertaining.
Perhaps 'Drop use of unwanted binding' ? It seems quite vague to me, like 'Fix a horrible bug' or 'Sort out all the problems' :-)
Regards, SImon
participants (6)
-
Bharat Gooty -
Marc Zyngier -
Michael Walle -
Simon Glass -
Tom Rini -
Z.Q. Hou