[PATCH v1 1/1] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3588S

[PATCH v1 1/1] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3588S

[Sebastian Reichel]

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(-)

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;
-- 
2.40.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1 1/1] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3588S

[Marc Zyngier]

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.

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1 1/1] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3588S

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 3526 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v1 1/1] irqchip/gic-v3: Enable Rockchip 3588001 erratum workaround for RK3588S

[Marc Zyngier]

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.

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip