From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ACBD8C433F5 for ; Thu, 28 Oct 2021 21:09:40 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B1E61610CA for ; Thu, 28 Oct 2021 21:09:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B1E61610CA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 2657E834F1; Thu, 28 Oct 2021 23:09:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id AD64883513; Thu, 28 Oct 2021 23:09:34 +0200 (CEST) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 271CF801B2 for ; Thu, 28 Oct 2021 23:09:30 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=maz@kernel.org Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 52E24610CB; Thu, 28 Oct 2021 21:09:28 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mgCeM-002IZh-0c; Thu, 28 Oct 2021 22:09:26 +0100 Date: Thu, 28 Oct 2021 22:09:25 +0100 Message-ID: <87k0hwua9m.wl-maz@kernel.org> From: Marc Zyngier To: Michael Walle Cc: u-boot@lists.denx.de, Vladimir Oltean , Hou Zhiqiang , Bharat Gooty , Rayagonda Kokatanur , Simon Glass , Priyanka Jain , Tom Rini Subject: Re: [PATCH 2/2] Revert "arch: arm: use dt and UCLASS_SYSCON to get gic lpi details" In-Reply-To: <20211027165454.1501398-3-michael@walle.cc> References: <20211027165454.1501398-1-michael@walle.cc> <20211027165454.1501398-3-michael@walle.cc> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: michael@walle.cc, u-boot@lists.denx.de, vladimir.oltean@nxp.com, Zhiqiang.Hou@nxp.com, bharat.gooty@broadcom.com, rayagonda.kokatanur@broadcom.com, sjg@chromium.org, priyanka.jain@nxp.com, trini@konsulko.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Wed, 27 Oct 2021 17:54:54 +0100, Michael Walle wrote: > > Stop using the device tree as a source for ad-hoc information. > > This reverts commit 2ae7adc659f7fca9ea65df4318e5bca2b8274310. > > Signed-off-by: Michael Walle > --- > 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 > #include > #include > -#include > -#include > #include > #include > #include > @@ -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.