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/[email protected]/
- uses of_dma_is_coherent() instead of providing errata info from kernel
* RFCv1
- https://lore.kernel.org/lkml/[email protected]/
- 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
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 <[email protected]>
Signed-off-by: XiaoDong Huang <[email protected]>
Co-developed-by: Kever Yang <[email protected]>
Signed-off-by: Kever Yang <[email protected]>
Co-developed-by: Lucas Tanure <[email protected]>
Signed-off-by: Lucas Tanure <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
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
On Mon, 17 Apr 2023 16:00:37 +0100,
Sebastian Reichel <[email protected]> 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 <[email protected]>
> Signed-off-by: XiaoDong Huang <[email protected]>
> Co-developed-by: Kever Yang <[email protected]>
> Signed-off-by: Kever Yang <[email protected]>
> Co-developed-by: Lucas Tanure <[email protected]>
> Signed-off-by: Lucas Tanure <[email protected]>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> 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.
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
On Mon, 17 Apr 2023 22:40:33 +0100,
Sebastian Reichel <[email protected]> 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.
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 <[email protected]> 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