[PATCH v2 0/2] Add Rockchip RK3588 GIC ITS support

[PATCH v2 0/2] Add Rockchip RK3588 GIC ITS support

[Sebastian Reichel]

Hi,

We got an Errata # from Rockchip for the GIC600 integration issue in
RK3588, so here is another try adding upstream ITS support for that
platform. I noticed
https://lore.kernel.org/lkml/DU0PR04MB9417388F9BDD73080294FA8E88889@DU0PR04MB9417.eurprd04.prod.outlook.com/
so I kept the flag name itself generic.

P.S.: I'm taking over from Lucas Tanure, who no longer works for
Collabora.

List of previous attempts / changelog:
 * PATCHv1
  - https://lore.kernel.org/lkml/20230227151847.207922-1-lucas.tanure@collabora.com/
  - uses of_dma_is_coherent() instead of providing errata info from kernel
 * RFCv1
  - https://lore.kernel.org/lkml/20230310080518.78054-1-lucas.tanure@collabora.com/
  - uses 0x0201743b IIDR for quirk detection and misses errata #

Greetings,

-- Sebastian

Sebastian Reichel (2):
  irqchip/gic-v3: Add Rockchip 3588001 errata workaround
  arm64: dts: rockchip: rk3588: add MSI support

 Documentation/arm64/silicon-errata.rst    |  3 +++
 arch/arm64/Kconfig                        | 10 +++++++++
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 17 +++++++++++++++
 drivers/irqchip/irq-gic-v3-its.c          | 25 +++++++++++++++++++++++
 4 files changed, 55 insertions(+)

-- 
2.39.2

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

[Sebastian Reichel]

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.

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    |
++----------------+-----------------+-----------------+-----------------------------+
 
 +----------------+-----------------+-----------------+-----------------------------+
 | 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"
+	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.
+
+	  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));
+	}
+
 	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;
+
 	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
 }
 
 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;
+
 	if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) {
 		if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) {
 			/*
-- 
2.39.2

[PATCH v2 2/2] arm64: dts: rockchip: rk3588: add MSI support

[Sebastian Reichel]

Add the two Interrupt Translation Service (ITS) IPs that are part of the
GIC-600, which are required for message signalled interrupts (MSI). This
is required for PCIe, which fully relies on MSI for interrupts.

Enabling the ITS nodes requires an operating system, that has a
workaround for Rockchip errata #3588001 (GIC600 can not support
shareable attribute).

Co-developed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 2124c654f665..62204b96b0b4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1741,7 +1741,24 @@ gic: interrupt-controller@fe600000 {
 		mbi-alias = <0x0 0xfe610000>;
 		mbi-ranges = <424 56>;
 		msi-controller;
+		ranges;
+		#address-cells = <2>;
 		#interrupt-cells = <4>;
+		#size-cells = <2>;
+
+		its0: msi-controller@fe640000 {
+			compatible = "arm,gic-v3-its";
+			msi-controller;
+			#msi-cells = <1>;
+			reg = <0x0 0xfe640000 0x0 0x20000>;
+		};
+
+		its1: msi-controller@fe660000 {
+			compatible = "arm,gic-v3-its";
+			msi-controller;
+			#msi-cells = <1>;
+			reg = <0x0 0xfe660000 0x0 0x20000>;
+		};
 
 		ppi-partitions {
 			ppi_partition0: interrupt-partition-0 {
-- 
2.39.2

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

[Marc Zyngier]

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.

Re: [PATCH v2 2/2] arm64: dts: rockchip: rk3588: add MSI support

[Marc Zyngier]

On Mon, 17 Apr 2023 16:00:38 +0100,
Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> 
> Add the two Interrupt Translation Service (ITS) IPs that are part of the
> GIC-600, which are required for message signalled interrupts (MSI). This
> is required for PCIe, which fully relies on MSI for interrupts.
> 
> Enabling the ITS nodes requires an operating system, that has a
> workaround for Rockchip errata #3588001 (GIC600 can not support
> shareable attribute).

$SUBJECT is inaccurate. This adds the descriptions of the ITSs, but
doesn't wire any MSI support at all (nobody even refers these
nodes, so the whole thing is pretty pointless).

	M.

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

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

Hi,

On Mon, Apr 17, 2023 at 05:20:08PM +0100, Marc Zyngier wrote:
>> [...]
> Please see below for an untested diff against your patch, addressing
> most of the issues mentioned here.

Thanks, looks good. I integrated the changes into v3.

> 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?

It got fixed in RK3588.

> [...]

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marc Zyngier]

On Mon, 17 Apr 2023 22:40:33 +0100,
Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> 
> Hi,
> 
> On Mon, Apr 17, 2023 at 05:20:08PM +0100, Marc Zyngier wrote:
> >> [...]
> > Please see below for an untested diff against your patch, addressing
> > most of the issues mentioned here.
> 
> Thanks, looks good. I integrated the changes into v3.
> 
> > 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?
> 
> It got fixed in RK3588.

So what's the plan for the affected SoCs? It might be a good idea to
address it too, unless there are none of them in the wild?

	M.

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

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

Hi,

On Tue, Apr 18, 2023 at 09:58:17AM +0100, Marc Zyngier wrote:
> On Mon, 17 Apr 2023 22:40:33 +0100,
> Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> > On Mon, Apr 17, 2023 at 05:20:08PM +0100, Marc Zyngier wrote:
> > > 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?
> > 
> > It got fixed in RK3588.
> 
> So what's the plan for the affected SoCs? It might be a good idea to
> address it too, unless there are none of them in the wild?

The previous generation (RK3566 and RK3568) is affected by the 32
bit issue as well as the shareability issue. I'm currently focusing
on the RK3588 and that's the only Rockchip platform I have on my
desk. Missing PCIe support is a lot more critical on RK3588, because
multiple board designers decided to use a 2.5G PCIe network card
instead of using the 1G network MAC from the SoC.

Anyways, I will try to help out with RK356x once I find some time.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]