[PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Marc Zyngier
maz at kernel.org
Thu Oct 28 23:09:25 CEST 2021
On Wed, 27 Oct 2021 17:54:54 +0100,
Michael Walle <michael at 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 at 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.
More information about the U-Boot
mailing list