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 5463EC433EF for ; Mon, 1 Nov 2021 09:39:01 +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 ADBD860C40 for ; Mon, 1 Nov 2021 09:39:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org ADBD860C40 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 EA21583395; Mon, 1 Nov 2021 10:38:58 +0100 (CET) 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 7099082EA5; Mon, 1 Nov 2021 10:38:56 +0100 (CET) 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 63C8C8352D for ; Mon, 1 Nov 2021 10:38:45 +0100 (CET) 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 D7E67610A2; Mon, 1 Nov 2021 09:38:43 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.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 1mhTm5-002pFC-NA; Mon, 01 Nov 2021 09:38:41 +0000 Date: Mon, 01 Nov 2021 09:38:41 +0000 Message-ID: <87cznk9pwe.wl-maz@kernel.org> From: Marc Zyngier To: "Z.Q. Hou" Cc: Michael Walle , "u-boot@lists.denx.de" , Vladimir Oltean , 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: References: <20211027165454.1501398-1-michael@walle.cc> <20211027165454.1501398-3-michael@walle.cc> <87k0hwua9m.wl-maz@kernel.org> 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=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: zhiqiang.hou@nxp.com, michael@walle.cc, u-boot@lists.denx.de, vladimir.oltean@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 Sun, 31 Oct 2021 16:45:41 +0000, "Z.Q. Hou" wrote: >=20 >=20 >=20 > > -----Original Message----- > > From: Marc Zyngier [mailto:maz@kernel.org] > > Sent: 2021=E5=B9=B410=E6=9C=8829=E6=97=A5 5:09 > > To: Michael Walle > > Cc: u-boot@lists.denx.de; Vladimir Oltean ; Z.= Q. Hou > > ; 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" > >=20 > > 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 > >=20 > > Why is this FSL specific? > >=20 > > > @@ -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) > >=20 > > This looks completely wrong. > >=20 > > The pending table needs one bit per LPI, and the property table one byt= e per LPI. > > Here, you have it the other way around. >=20 > 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. >=20 > > Also, the property table alignment requirement is 4kB, not 64kB, > > and its size is defined as the maximum number of LPIs - 8192. >=20 > 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? >=20 > >=20 > > 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. > > >=20 > 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. >=20 > > 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). > >=20 > > > +#define GIC_LPI_SIZE ALIGN(cpu_numcores() * PENDTABLE_MAX_SZ + \ > > > + PROPTABLE_MAX_SZ, SZ_1M) > >=20 > > Why the 1MB alignment? There is no such requirement in the > > architecture (64kB for the pending tables, 4kB for the property > > table). >=20 > 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? >=20 > >=20 > > > +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 =3D base; > > > + gic_rd_tables.end =3D base + size - 1; > > > + err =3D fdtdec_add_reserved_memory(blob, "gic-rd-tables", &gic_rd_t= ables, > > > + 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 =3D gic_lpi_tables_init(); > > > + gic_lpi_base =3D ALIGN(gd->arch.resv_ram - GIC_LPI_SIZE, SZ_64K); > > > + ret =3D fdt_add_resv_mem_gic_rd_tables(blob, gic_lpi_base, GIC_LPI_= SIZE); > > > + if (ret) > > > + return ret; > > > + > > > + ret =3D gic_lpi_tables_init(gic_lpi_base, cpu_numcores()); > >=20 > > This really should fetch the number of CPUs from the DT rather > > then some SoC specific black magic... >=20 > 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. --=20 Without deviation from the norm, progress is not possible.