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

[Sebastian Reichel]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>]

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: signature.asc
Description: PGP signature