Re: [PATCH v2 1/2] irqchip/gic-v3: Add Rockchip 3588001 errata workaround

[Marc Zyngier <maz@kernel.org>]

On Mon, 17 Apr 2023 16:00:37 +0100,
Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> 
> Rockchip RK3588/RK3588s GIC600 integration is not cache coherent. Thus
> even though the GIC600 itself supports the sharability feature, it may
> not be used. Rockchip assigned Errata ID #3588001 for this issue.
> 
> The RK3588's GIC600 IP is using ARM's ID in the IIDR register
> (0x0201743b), so it cannot be used to apply the quirk. Thus I used the
> machine compatible instead.

These are not incompatible requirements, see below.

> 
> I named the flag ITS_FLAGS_BROKEN_SHAREABILITY to be vendor agnostic,
> since apparently similar integration design errors exist in other
> platforms and they can reuse the same flag.
> 
> Co-developed-by: XiaoDong Huang <derrick.huang@rock-chips.com>
> Signed-off-by: XiaoDong Huang <derrick.huang@rock-chips.com>
> Co-developed-by: Kever Yang <kever.yang@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Co-developed-by: Lucas Tanure <lucas.tanure@collabora.com>
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  Documentation/arm64/silicon-errata.rst |  3 +++
>  arch/arm64/Kconfig                     | 10 ++++++++++
>  drivers/irqchip/irq-gic-v3-its.c       | 25 +++++++++++++++++++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
> index ec5f889d7681..46d06ed3e4f4 100644
> --- a/Documentation/arm64/silicon-errata.rst
> +++ b/Documentation/arm64/silicon-errata.rst
> @@ -205,6 +205,9 @@ stable kernels.
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Qualcomm Tech. | Kryo4xx Gold    | N/A             | ARM64_ERRATUM_1286807       |
>  +----------------+-----------------+-----------------+-----------------------------+
> ++----------------+-----------------+-----------------+-----------------------------+
> +| Rockchip       | RK3588          | #3588001        | ROCKCHIP_ERRATUM_3588001    |
> ++----------------+-----------------+-----------------+-----------------------------+

Finally. It only took two years... :-/

>
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Fujitsu        | A64FX           | E#010001        | FUJITSU_ERRATUM_010001      |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1023e896d46b..640619459648 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1150,6 +1150,16 @@ config NVIDIA_CARMEL_CNP_ERRATUM
>  
>  	  If unsure, say Y.
>  
> +config ROCKCHIP_ERRATUM_3588001
> +	bool "Rockchip 3588001: GIC600 can not support shareable attribute"

s/shareable attributes/shareability attributes/

> +	default y
> +	help
> +	  The Rockchip RK3588 GIC600 SoC integration does not support ACE/ACE-lite
> +	  and thus is not cache coherent. This means, that the GIC600 may not use
> +	  the sharability feature even though it is supported by the IP itself.

Shareability and cache coherence are not the same thing. The latter
derive from the former, but not the other way around. So please drop
any reference to cache coherency, because that's not the problem at
hand.

> +
> +	  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 586271b8aa39..0701d0b67690 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -42,6 +42,7 @@
>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
> +#define ITS_FLAGS_BROKEN_SHAREABILITY		(1ULL << 3)
>  
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  #define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> @@ -2359,6 +2360,13 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
>  	its_write_baser(its, baser, val);
>  	tmp = baser->val;
>  
> +	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY) {
> +		if (tmp & GITS_BASER_SHAREABILITY_MASK)
> +			tmp &= ~GITS_BASER_SHAREABILITY_MASK;
> +		else
> +			gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
> +	}

Hmmm. This doesn't really make much sense: why only clear the
shareability if it is set? You know, by definition, that it is not
working anyway.

Also, why the clean to PoC? We already have it a few lines below...

> +
>  	if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) {
>  		/*
>  		 * Shareability didn't stick. Just use
> @@ -3055,6 +3063,7 @@ static u64 its_clear_vpend_valid(void __iomem *vlpi_base, u64 clr, u64 set)
>  
>  static void its_cpu_init_lpis(void)
>  {
> +	struct its_node *its = list_first_entry(&its_nodes, struct its_node, entry);
>  	void __iomem *rbase = gic_data_rdist_rd_base();
>  	struct page *pend_page;
>  	phys_addr_t paddr;
> @@ -3096,6 +3105,9 @@ static void its_cpu_init_lpis(void)
>  	gicr_write_propbaser(val, rbase + GICR_PROPBASER);
>  	tmp = gicr_read_propbaser(rbase + GICR_PROPBASER);
>  
> +	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY)
> +		tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK;
> +

Why treat ITS and redistributor the same way? We have redistributor
flags already.

>  	if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) {
>  		if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) {
>  			/*
> @@ -3120,6 +3132,9 @@ static void its_cpu_init_lpis(void)
>  	gicr_write_pendbaser(val, rbase + GICR_PENDBASER);
>  	tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER);
>  
> +	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY)
> +		tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK;
> +
>  	if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) {
>  		/*
>  		 * The HW reports non-shareable, we must remove the
> @@ -4765,6 +4780,13 @@ static void its_enable_quirks(struct its_node *its)
>  	u32 iidr = readl_relaxed(its->base + GITS_IIDR);
>  
>  	gic_enable_quirks(iidr, its_quirks, its);
> +
> +#ifdef CONFIG_ROCKCHIP_ERRATUM_3588001
> +	if (of_machine_is_compatible("rockchip,rk3588")) {
> +		its->flags |= ITS_FLAGS_BROKEN_SHAREABILITY;
> +		pr_info("GIC: enabling workaround for Rockchip #3588001\n");
> +	}
> +#endif

What prevents you from implementing this as a standard quirk and apply
further filtering inside the callback?

>  }
>  
>  static int its_save_disable(void)
> @@ -5096,6 +5118,9 @@ static int __init its_probe_one(struct resource *res,
>  	gits_write_cbaser(baser, its->base + GITS_CBASER);
>  	tmp = gits_read_cbaser(its->base + GITS_CBASER);
>  
> +	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY)
> +		tmp &= ~GITS_CBASER_SHAREABILITY_MASK;
> +

And here, you correctly deal with the masking of the attributes. Why
the lack of consistency?

Please see below for an untested diff against your patch, addressing
most of the issues mentioned here.

Also, I don't see anything here addressing the *other* bug this
platform suffers from, which is the 32bit limit to the allocations.
Without a fix for it, this patch is pointless as the GIC may end-up
with memory it cannot reach.

What;s the plan for that?

	M.

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0701d0b67690..dcdcffb4ca86 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -46,6 +46,7 @@
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
 #define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
+#define RDIST_FLAGS_BROKEN_SHAREABILITY		(1 << 2)
 
 #define RD_LOCAL_LPI_ENABLED                    BIT(0)
 #define RD_LOCAL_PENDTABLE_PREALLOCATED         BIT(1)
@@ -2360,12 +2361,8 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
 	its_write_baser(its, baser, val);
 	tmp = baser->val;
 
-	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY) {
-		if (tmp & GITS_BASER_SHAREABILITY_MASK)
-			tmp &= ~GITS_BASER_SHAREABILITY_MASK;
-		else
-			gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
-	}
+	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY)
+		tmp &= ~GITS_BASER_SHAREABILITY_MASK;
 
 	if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) {
 		/*
@@ -3063,7 +3060,6 @@ static u64 its_clear_vpend_valid(void __iomem *vlpi_base, u64 clr, u64 set)
 
 static void its_cpu_init_lpis(void)
 {
-	struct its_node *its = list_first_entry(&its_nodes, struct its_node, entry);
 	void __iomem *rbase = gic_data_rdist_rd_base();
 	struct page *pend_page;
 	phys_addr_t paddr;
@@ -3105,7 +3101,7 @@ static void its_cpu_init_lpis(void)
 	gicr_write_propbaser(val, rbase + GICR_PROPBASER);
 	tmp = gicr_read_propbaser(rbase + GICR_PROPBASER);
 
-	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY)
+	if (gic_rdists->flags & RDIST_FLAGS_BROKEN_SHAREABILITY)
 		tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK;
 
 	if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) {
@@ -3132,7 +3128,7 @@ static void its_cpu_init_lpis(void)
 	gicr_write_pendbaser(val, rbase + GICR_PENDBASER);
 	tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER);
 
-	if (its->flags & ITS_FLAGS_BROKEN_SHAREABILITY)
+	if (gic_rdists->flags & RDIST_FLAGS_BROKEN_SHAREABILITY)
 		tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK;
 
 	if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) {
@@ -4725,6 +4721,19 @@ static bool __maybe_unused its_enable_quirk_hip07_161600802(void *data)
 	return true;
 }
 
+static bool __maybe_unused its_enable_rk3388001(void *data)
+{
+	struct its_node *its = data;
+
+	if (!of_machine_is_compatible("rockchip,rk3588"))
+		return false;
+
+	its->flags |= ITS_FLAGS_BROKEN_SHAREABILITY;
+	gic_rdists->flags |= RDIST_FLAGS_BROKEN_SHAREABILITY;
+
+	return true;
+}
+
 static const struct gic_quirk its_quirks[] = {
 #ifdef CONFIG_CAVIUM_ERRATUM_22375
 	{
@@ -4770,6 +4779,14 @@ static const struct gic_quirk its_quirks[] = {
 		.mask	= 0xffffffff,
 		.init	= its_enable_quirk_hip07_161600802,
 	},
+#endif
+#ifdef CONFIG_ROCKCHIP_ERRATUM_3588001
+	{
+		.desc	= "ITS: Rockchip erratum RK3588001",
+		.iidr	= 0x0201743b,
+		.mask	= 0xffffffff,
+		.init	= its_enable_rk3388001,
+	},
 #endif
 	{
 	}
@@ -4780,13 +4797,6 @@ static void its_enable_quirks(struct its_node *its)
 	u32 iidr = readl_relaxed(its->base + GITS_IIDR);
 
 	gic_enable_quirks(iidr, its_quirks, its);
-
-#ifdef CONFIG_ROCKCHIP_ERRATUM_3588001
-	if (of_machine_is_compatible("rockchip,rk3588")) {
-		its->flags |= ITS_FLAGS_BROKEN_SHAREABILITY;
-		pr_info("GIC: enabling workaround for Rockchip #3588001\n");
-	}
-#endif
 }
 
 static int its_save_disable(void)


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