[PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Michael Walle]

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@broadcom.com/
[3] https://lore.kernel.org/u-boot/20200610104120.30668-10-rayagonda.kokatanur@broadcom.com/
[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(-)

-- 
2.30.2

[PATCH 1/2] Revert "arm64: Layerscape: Survive LPI one-way reset workaround"

[Michael Walle]

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 */
-- 
2.30.2

Re: [PATCH 1/2] Revert "arm64: Layerscape: Survive LPI one-way reset workaround"

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] Revert "arm64: Layerscape: Survive LPI one-way reset workaround"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Michael Walle]

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,
-};
-- 
2.30.2

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[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>
> ---
>  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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Michael Walle]

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

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

RE: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[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.

> 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.

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Michael Walle]

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

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Marc Zyngier]

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.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Bharat Gooty]

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

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Bharat Gooty]

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.
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Michael Walle]

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

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[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?

> But all of this doesn't need any new device tree node.
>
> -michael
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Michael Walle]

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

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

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!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Simon Glass]

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

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Michael Walle]

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

Re: [PATCH 0/2] arch: arm: gic-v3-its: stop abusing the device tree

[Simon Glass]

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