| Message ID | 20230417150038.51698-2-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Rockchip RK3588 GIC ITS support | expand |
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)
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 <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.
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
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)) { /*