Received-SPF: pass (google.com: domain of linux-kernel+bounces-198402-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 2 Jun 2024 21:15:09 +0100
From: Andre Przywara <andre.przywara@arm.com>
To: "harry.yu185" <harry.yu185@gmail.com>
Cc: corbet@lwn.net, catalin.marinas@arm.com, will@kernel.org,
 tglx@linutronix.de, maz@kernel.org, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-sunxi <linux-sunxi@lists.linux.dev>
Subject: Re: [PATCH] irqchip/gic-v3: Add Allwinner sunxi001 erratum
 workaround
Message-ID: <20240602211509.67df243e@minigeek.lan>
In-Reply-To: <20240602071058.6405-1-harry.yu185@gmail.com>
References: <20240602071058.6405-1-harry.yu185@gmail.com>
Organization: Arm Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun,  2 Jun 2024 15:10:58 +0800
"harry.yu185" <harry.yu185@gmail.com> wrote:

Hi,

(please make sure to CC: the linux-sunxi mailing list on Allwinner
related patches)

I do hope that this whole patch is unnecessary, as Marc pointed out, but
just for the records some comments for future reference, since this
patch seems either premature or out of place.

> Allwinner A523 GIC600 integration does not support the
> sharability feature. So assigned Erratum ID #sunxi001 for this
> issue.
> 
> That the 0x0201643b ID is not Allwinner specific and thus
> there is an extra of_machine_is_compatible() check.
> 
> Note, because more than one soc may have this problem, the 'sunxi'
> name is used instead of a fixed soc name like A523.
> 
> Signed-off-by: harry.yu185 <harry.yu185@gmail.com>
> ---
>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>  arch/arm64/Kconfig                          | 10 ++++++++++
>  drivers/irqchip/irq-gic-v3-its.c            | 21 +++++++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
> index eb8af8032c31..351dd6094a6c 100644
> --- a/Documentation/arch/arm64/silicon-errata.rst
> +++ b/Documentation/arch/arm64/silicon-errata.rst
> @@ -242,6 +242,8 @@ stable kernels.
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Rockchip       | RK3588          | #3588001        | ROCKCHIP_ERRATUM_3588001    |
>  +----------------+-----------------+-----------------+-----------------------------+
> +| Allwinner      | SUN55IW3        | #sunxi001       | ALLWINNER_ERRATUM_SUNXI001  |

"sun55iw3" is a CPU die identifier only used in Allwinner's BSP
kernels, mainline uses a different naming scheme.

Also the erratum name looks odd, at the very least I'd expect it to
read "ALLWINNER_ERRATUM_00001" or something. But there is already some
Allwinner timer erratum, which we boldly gave ID 1 (short of an
official erratum number from Allwinner), so using the same number again
will surely lead to confusion.

> ++----------------+-----------------+-----------------+-----------------------------+
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Fujitsu        | A64FX           | E#010001        | FUJITSU_ERRATUM_010001      |
>  +----------------+-----------------+-----------------+-----------------------------+
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5d91259ee7b5..5a71227d119a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1279,6 +1279,16 @@ config ROCKCHIP_ERRATUM_3588001
>  
>  	  If unsure, say Y.
>  
> +config ALLWINNER_ERRATUM_SUNXI001
> +	bool "Allwinner sunxi001: GIC600 can not support shareability attributes"
> +	default y

"default ARCH_SUNXI" would be better suited here, I think.

> +	help
> +	  The Allwinner GIC600 SoC integration does not support ACE/ACE-lite.
> +	  This means, that its sharability feature may not be used, even though it
> +	  is supported by the IP itself.
> +
> +	  If unsure, say Y.
> +
>  config SOCIONEXT_SYNQUACER_PREITS
>  	bool "Socionext Synquacer: Workaround for GICv3 pre-ITS"
>  	default y
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 40ebf1726393..d93348947353 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -4775,6 +4775,19 @@ static bool __maybe_unused its_enable_rk3588001(void *data)
>  	return true;
>  }
>  
> +static bool __maybe_unused its_enable_sunxi001(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	if (!of_machine_is_compatible("arm,sun55iw3p1"))

You cannot reference a compatible name here that is not documented in
the bindings. Which brings us to the elephant in the room: there is no
upstream support for this SoC (family) yet. I have some
work-in-progress series [1], but it's far from finished, mostly blocked
by the lack of hackable hardware (hopefully fixed soon).

So what kernel is this patch supposed to be applied against? You would
need at least a pinctrl and clock driver for even basic operation, none
of that I have seen posted.

Cheers,
Andre

[1] https://github.com/apritzel/linux/commits/a523-EARLY/

> +		return false;
> +
> +	its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
> +	gic_rdists->flags |= RDIST_FLAGS_FORCE_NON_SHAREABLE;
> +
> +	return true;
> +}
> +
>  static bool its_set_non_coherent(void *data)
>  {
>  	struct its_node *its = data;
> @@ -4836,6 +4849,14 @@ static const struct gic_quirk its_quirks[] = {
>  		.mask   = 0xffffffff,
>  		.init   = its_enable_rk3588001,
>  	},
> +#endif
> +#ifdef CONFIG_ALLWINNER_ERRATUM_SUNXI001
> +	{
> +		.desc   = "ITS: Allwinner erratum sunxi001",
> +		.iidr   = 0x0201643b,
> +		.mask   = 0xffffffff,
> +		.init   = its_enable_sunxi001,
> +	},
>  #endif
>  	{
>  		.desc   = "ITS: non-coherent attribute",