| Message ID | 20230703164129.193991-1-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3588S | expand |
On Mon, 03 Jul 2023 17:41:29 +0100, Sebastian Reichel <sebastian.reichel@collabora.com> wrote: > > Commit a8707f553884 ("irqchip/gic-v3: Add Rockchip 3588001 erratum > workaround") mentioned RK3588S (the slimmed down variant of RK3588) > being affected, but did not check for its compatible value. Thus the > quirk is not applied on RK3588S. Since the GIC ITS node got added to the > upstream DT, boards using RK3588S are no longer booting without this > quirk being applied. > > Fixes: 06cdac8e8407 ("arm64: dts: rockchip: add GIC ITS support to rk3588") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > I recently got a Rock 5A and noticed this issue. Apart from it, the Indiedroid > Nova should also be affected (I don't have that board). There are no other > upstream RK3588S boards at the moment. What about "khadas,edge2"? > --- > drivers/irqchip/irq-gic-v3-its.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index 1994541eaef8..034ece9ac47c 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -4727,7 +4727,8 @@ static bool __maybe_unused its_enable_rk3588001(void *data) > { > struct its_node *its = data; > > - if (!of_machine_is_compatible("rockchip,rk3588")) > + if (!of_machine_is_compatible("rockchip,rk3588") && > + !of_machine_is_compatible("rockchip,rk3588s")) > return false; > > its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE; I don't mind taking this, but it also mean that only a new kernel will boot. Shouldn't you *also* fix the DT so that it advertises rk3588 as a fallback to rk3588s? This would ensure that an old kernel can boot as well. M.
Hi, On Mon, Jul 03, 2023 at 05:54:36PM +0100, Marc Zyngier wrote: > On Mon, 03 Jul 2023 17:41:29 +0100, > Sebastian Reichel <sebastian.reichel@collabora.com> wrote: > > > > Commit a8707f553884 ("irqchip/gic-v3: Add Rockchip 3588001 erratum > > workaround") mentioned RK3588S (the slimmed down variant of RK3588) > > being affected, but did not check for its compatible value. Thus the > > quirk is not applied on RK3588S. Since the GIC ITS node got added to the > > upstream DT, boards using RK3588S are no longer booting without this > > quirk being applied. > > > > Fixes: 06cdac8e8407 ("arm64: dts: rockchip: add GIC ITS support to rk3588") > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > I recently got a Rock 5A and noticed this issue. Apart from it, the Indiedroid > > Nova should also be affected (I don't have that board). There are no other > > upstream RK3588S boards at the moment. > > What about "khadas,edge2"? [+cc Yixun Lan <dlan@gentoo.org>] Ah yes, that too. I should have grepped for rk3588s instead of rockchip,rk3588 and trying to sort out the false positives (I tried it that way to check if any other potential suffixes being used). > > --- > > drivers/irqchip/irq-gic-v3-its.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > index 1994541eaef8..034ece9ac47c 100644 > > --- a/drivers/irqchip/irq-gic-v3-its.c > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > @@ -4727,7 +4727,8 @@ static bool __maybe_unused its_enable_rk3588001(void *data) > > { > > struct its_node *its = data; > > > > - if (!of_machine_is_compatible("rockchip,rk3588")) > > + if (!of_machine_is_compatible("rockchip,rk3588") && > > + !of_machine_is_compatible("rockchip,rk3588s")) > > return false; > > > > its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE; > > I don't mind taking this, but it also mean that only a new kernel > will boot. Yes. My assumption is, that this is considered a fix and landing in the 6.5 cycle. The rk3588s.dtsi from v6.4 does not yet have the GIC ITS nodes. So there is not yet a tagged kernel with the boot failure. The first one will be v6.5-rc1. The quirk in the GIC driver only landed in v6.4, so anything older is broken anyways. So effectively we are talking about v6.4 booting a v6.5 DT, which is not something we guarantee to be working as far as I know. > Shouldn't you *also* fix the DT so that it advertises rk3588 as > a fallback to rk3588s? This would ensure that an old kernel can boot > as well. RK3588S is a subset of RK3588. Thus rk3588s could be a fallback for rk3588, but not the other way around. That way the quirk could only check for "rockchip,rk3588s". But this would break the following DTs, if they are not being patched in parallel: $ grep '"rockchip,rk3588"' *dts rk3588-edgeble-neu6a-io.dts: "edgeble,neural-compute-module-6a", "rockchip,rk3588"; rk3588-edgeble-neu6b-io.dts: "edgeble,neural-compute-module-6b", "rockchip,rk3588"; rk3588-evb1-v10.dts: compatible = "rockchip,rk3588-evb1-v10", "rockchip,rk3588"; rk3588-rock-5b.dts: compatible = "radxa,rock-5b", "rockchip,rk3588"; And in this case it breaks the DT backwards compatibility guarantee, because new kernel is supposed to be able to boot an old DT. I suppose adding the extra fallback to RK3588 might still be sensible for any future issues. -- Sebastian
On Mon, 03 Jul 2023 18:42:33 +0100, Sebastian Reichel <sebastian.reichel@collabora.com> wrote: > > > > --- > > > drivers/irqchip/irq-gic-v3-its.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > > index 1994541eaef8..034ece9ac47c 100644 > > > --- a/drivers/irqchip/irq-gic-v3-its.c > > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > > @@ -4727,7 +4727,8 @@ static bool __maybe_unused its_enable_rk3588001(void *data) > > > { > > > struct its_node *its = data; > > > > > > - if (!of_machine_is_compatible("rockchip,rk3588")) > > > + if (!of_machine_is_compatible("rockchip,rk3588") && > > > + !of_machine_is_compatible("rockchip,rk3588s")) > > > return false; > > > > > > its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE; > > > > I don't mind taking this, but it also mean that only a new kernel > > will boot. > > Yes. My assumption is, that this is considered a fix and landing in the > 6.5 cycle. The rk3588s.dtsi from v6.4 does not yet have the GIC ITS > nodes. So there is not yet a tagged kernel with the boot failure. The > first one will be v6.5-rc1. Ah, fair enough. In this case I'll queue this patch without any remorse. > The quirk in the GIC driver only landed in v6.4, so anything older > is broken anyways. So effectively we are talking about v6.4 booting > a v6.5 DT, which is not something we guarantee to be working as far > as I know. In general, I do make a point in making things work *in both directions*. But given that this is an erratum, there isn't much we can do, and advertising an ITS to a kernel that cannot handle it is doomed. Thanks, M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 1994541eaef8..034ece9ac47c 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -4727,7 +4727,8 @@ static bool __maybe_unused its_enable_rk3588001(void *data) { struct its_node *its = data; - if (!of_machine_is_compatible("rockchip,rk3588")) + if (!of_machine_is_compatible("rockchip,rk3588") && + !of_machine_is_compatible("rockchip,rk3588s")) return false; its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE;
Commit a8707f553884 ("irqchip/gic-v3: Add Rockchip 3588001 erratum workaround") mentioned RK3588S (the slimmed down variant of RK3588) being affected, but did not check for its compatible value. Thus the quirk is not applied on RK3588S. Since the GIC ITS node got added to the upstream DT, boards using RK3588S are no longer booting without this quirk being applied. Fixes: 06cdac8e8407 ("arm64: dts: rockchip: add GIC ITS support to rk3588") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- I recently got a Rock 5A and noticed this issue. Apart from it, the Indiedroid Nova should also be affected (I don't have that board). There are no other upstream RK3588S boards at the moment. --- drivers/irqchip/irq-gic-v3-its.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)