[PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details"
Z.Q. Hou
zhiqiang.hou at nxp.com
Sun Oct 31 17:45:41 CET 2021
> -----Original Message-----
> From: Marc Zyngier [mailto:maz at kernel.org]
> Sent: 2021年10月29日 5:09
> To: Michael Walle <michael at walle.cc>
> Cc: u-boot at lists.denx.de; Vladimir Oltean <vladimir.oltean at nxp.com>; Z.Q. Hou
> <zhiqiang.hou at nxp.com>; Bharat Gooty <bharat.gooty at broadcom.com>;
> Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>; Simon Glass
> <sjg at chromium.org>; Priyanka Jain <priyanka.jain at nxp.com>; Tom Rini
> <trini at 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 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.
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.
More information about the U-Boot
mailing list