Re: [PATCH v1 1/4] irqchip/gic-v3: Add Rockchip 3568002 erratum workaround

[Marc Zyngier <maz@kernel.org>]

On Sat, 15 Feb 2025 23:54:28 +0000,
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> 
> Rockchip RK3566/RK3568 GIC600 integration has DDR addressing
> limited to first 4GB of DRAM. Rockchip assigned Erratum ID #3568002
> for this issue. Add driver quirk for this Rockchip GIC Erratum.

Thanks for taking the time to submit this. It only took 5 years for
this erratum to be published...

However, my understanding of this issue is that the integration is
limited to the first 32bit of physical address space, not the first
32bit of RAM. If the memory is placed as physical address 0, then they
represent the same space. But this is still an important distinction.

> 
> Note, that the 0x0201743b ID is not Rockchip 356x specific and thus
> there is an extra of_machine_is_compatible() check. Rockchip 3588 uses
> same ID and it is not affected by this errata.

This ID is that of ARM's GIC600, which is a very common GICv3
implementation, and is not Rockchip-specific. Please capture this in
the commit message.

> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>  arch/arm64/Kconfig                          |  9 ++++++++
>  drivers/irqchip/irq-gic-v3-its.c            | 23 ++++++++++++++++++++-
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
> index f074f6219f5c..f968c13b46a7 100644
> --- a/Documentation/arch/arm64/silicon-errata.rst
> +++ b/Documentation/arch/arm64/silicon-errata.rst
> @@ -284,6 +284,8 @@ stable kernels.
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Rockchip       | RK3588          | #3588001        | ROCKCHIP_ERRATUM_3588001    |
>  +----------------+-----------------+-----------------+-----------------------------+
> +| Rockchip       | RK3568          | #3568002        | ROCKCHIP_ERRATUM_3568002    |
> ++----------------+-----------------+-----------------+-----------------------------+
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Fujitsu        | A64FX           | E#010001        | FUJITSU_ERRATUM_010001      |
>  +----------------+-----------------+-----------------+-----------------------------+
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index c997b27b7da1..0428ad8f324d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1302,6 +1302,15 @@ config NVIDIA_CARMEL_CNP_ERRATUM
>  
>  	  If unsure, say Y.
>  
> +config ROCKCHIP_ERRATUM_3568002
> +	bool "Rockchip 3568002: can not support DDR addresses higher than 4G"
> +	default y
> +	help
> +	  The Rockchip RK3566 and RK3568 GIC600 SoC integrations have DDR
> +	  addressing limited to first 4GB.
> +
> +	  If unsure, say Y.
> +

s/DDR addresses/physical addresses/

>  config ROCKCHIP_ERRATUM_3588001
>  	bool "Rockchip 3588001: GIC600 can not support shareability attributes"
>  	default y
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 8c3ec5734f1e..f30ed281882f 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -205,13 +205,15 @@ static DEFINE_IDA(its_vpeid_ida);
>  #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
>  #define gic_data_rdist_vlpi_base()	(gic_data_rdist_rd_base() + SZ_128K)
>  
> +static gfp_t gfp_flags_quirk;
> +
>  static struct page *its_alloc_pages_node(int node, gfp_t gfp,
>  					 unsigned int order)
>  {
>  	struct page *page;
>  	int ret = 0;
>  
> -	page = alloc_pages_node(node, gfp, order);
> +	page = alloc_pages_node(node, gfp | gfp_flags_quirk, order);
>
>  	if (!page)
>  		return NULL;
> @@ -4887,6 +4889,17 @@ static bool __maybe_unused its_enable_quirk_hip09_162100801(void *data)
>  	return true;
>  }
>  
> +static bool __maybe_unused its_enable_rk3568002(void *data)
> +{
> +	if (!of_machine_is_compatible("rockchip,rk3566") &&
> +	    !of_machine_is_compatible("rockchip,rk3568"))
> +		return false;
> +
> +	gfp_flags_quirk |= GFP_DMA32;
> +
> +	return true;
> +}
> +
>  static const struct gic_quirk its_quirks[] = {
>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>  	{
> @@ -4954,6 +4967,14 @@ static const struct gic_quirk its_quirks[] = {
>  		.property = "dma-noncoherent",
>  		.init   = its_set_non_coherent,
>  	},
> +#ifdef CONFIG_ROCKCHIP_ERRATUM_3568002
> +	{
> +		.desc   = "ITS: Rockchip erratum RK3568002",
> +		.iidr   = 0x0201743b,
> +		.mask   = 0xffffffff,
> +		.init   = its_enable_rk3568002,
> +	},
> +#endif
>  	{
>  	}
>  };

Another thing is that this patch conflates ITS and redistributors. As
it turns out, we use the same allocator for both, but they are
distinct architectural concepts, even if GIC600 is a monolithic
implementation. It is OK for now, but it will have to be revisited if
we ever move the redistributor management outside of the ITS driver.

With the other comments addressed:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.