Re: [RFT PATCH v3 13/27] arm64: Add Apple vendor-specific system registers

[Will Deacon]

On Wed, Mar 24, 2021 at 06:59:21PM +0000, Mark Rutland wrote:
> On Wed, Mar 24, 2021 at 06:38:18PM +0000, Will Deacon wrote:
> > On Fri, Mar 05, 2021 at 06:38:48AM +0900, Hector Martin wrote:
> > > Apple ARM64 SoCs have a ton of vendor-specific registers we're going to
> > > have to deal with, and those don't really belong in sysreg.h with all
> > > the architectural registers. Make a new home for them, and add some
> > > registers which are useful for early bring-up.
> > > 
> > > Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
> > > ---
> > >  MAINTAINERS                           |  1 +
> > >  arch/arm64/include/asm/sysreg_apple.h | 69 +++++++++++++++++++++++++++
> > >  2 files changed, 70 insertions(+)
> > >  create mode 100644 arch/arm64/include/asm/sysreg_apple.h
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index aec14fbd61b8..3a352c687d4b 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -1646,6 +1646,7 @@ B:	https://github.com/AsahiLinux/linux/issues
> > >  C:	irc://chat.freenode.net/asahi-dev
> > >  T:	git https://github.com/AsahiLinux/linux.git
> > >  F:	Documentation/devicetree/bindings/arm/apple.yaml
> > > +F:	arch/arm64/include/asm/sysreg_apple.h
> > 
> > (this isn't needed with my suggestion below).
> > 
> > >  ARM/ARTPEC MACHINE SUPPORT
> > >  M:	Jesper Nilsson <jesper.nilsson@xxxxxxxx>
> > > diff --git a/arch/arm64/include/asm/sysreg_apple.h b/arch/arm64/include/asm/sysreg_apple.h
> > > new file mode 100644
> > > index 000000000000..48347a51d564
> > > --- /dev/null
> > > +++ b/arch/arm64/include/asm/sysreg_apple.h
> > 
> > I doubt apple are the only folks doing this, so can we instead have
> > sysreg-impdef.h please, and then have an Apple section in there for these
> > registers? That way, we could also have an imp_sys_reg() macro to limit
> > CRn to 11 or 15, which is the reserved encoding space for these registers.
> > 
> > We'll cc you for any patches touching the Apple parts, as we don't have
> > the first clue about what's hiding in there.
> 
> For existing IMP-DEF sysregs (e.g. the Kryo L2 control registers), we've
> put the definitions in the drivers, rather than collating
> non-architectural bits under arch/arm64/.

Yeah, but we could include those here as well.

> So far we've kept arch/arm64/ largely devoid of IMP-DEF bits, and it
> seems a shame to add something with the sole purpose of collating that,
> especially given arch code shouldn't need to touch these if FW and
> bootloader have done their jobs right.
> 
> Can we put the definitions in the relevant drivers? That would sidestep
> any pain with MAINTAINERS, too.

If we can genuinely ignore these in arch code, then sure. I just don't know
how long that is going to be the case, and ending up in a situation where
these are scattered randomly throughout the tree sounds horrible to me.

Will