Re: [PATCH] pinctrl: aspeed: Log error if SCU protection is active

[Tan Siewert]

On 13.06.25 08:01, Andrew Jeffery wrote:
> On Thu, 2025-06-12 at 17:18 +0200, Tan Siewert wrote:
> > ASPEED pinctrl and other drivers accessing SCU registers rely on the
> > bootloader to unlock the SCU before handing over to the kernel.
> >
> > However, some userspace scripts may re-enable SCU protection via
> > /dev/mem,
>
> Hmm, if this was caused by poking /dev/mem, then I'm not sure I'm in
> favour of it. The source of your problem wasn't apparent to me in our
> off-list discussion.

This was only an example of what I've already seen on GA firmware, but 
it could also be done by some custom out-of-tree driver.

> "Don't do that" :/

I agree on that ^^

> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c
> > index 774f8d05142f..81680c032b3c 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c
> > @@ -28,6 +28,8 @@
> >   #define SIG_EXPR_LIST_DECL_SINGLE SIG_EXPR_LIST_DECL_SESG
> >   #define SIG_EXPR_LIST_DECL_DUAL SIG_EXPR_LIST_DECL_DESG
> >   
> > +#define SCU_UNLOCKED_VALUE 0x00000001
>
> Bit of a nit-pick but I'm not sure this is worthwhile, or that the
> leading zeros are necessary. I'd be tempted just to use the constant
> '1' directly inline ...

This was more of a convenience to make it obvious that the "unlocked" 
value is meant here (as per the datasheet). The leading zeros are 
unnecessary, yes.

> > +
> >   /*
> >    * The "Multi-function Pins Mapping and Control" table in the SoC datasheet
> >    * references registers by the device/offset mnemonic. The register macros
> > @@ -36,6 +38,7 @@
> >    * reference registers beyond those dedicated to pinmux, such as the system
> >    * reset control and MAC clock configuration registers.
> >    */
> > +#define SCU00           0x00 /* Protection Key Register */
> >   #define SCU2C           0x2C /* Misc. Control Register */
> >   #define SCU3C           0x3C /* System Reset Control/Status Register */
> >   #define SCU48           0x48 /* MAC Interface Clock Delay Setting */
> > @@ -2582,6 +2585,24 @@ static int aspeed_g4_sig_expr_set(struct aspeed_pinmux_data *ctx,
> >                  if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2)
> >                          continue;
> >   
> > +               /*
> > +                * The SCU should be unlocked, with SCU00 returning 0x01.
> > +                * However, it may have been locked, e.g. by a
> > +                * userspace script using /dev/mem.
> > +                */
> > +               u32 value;
> > +
> > +               ret = regmap_read(ctx->maps[desc->ip], SCU00, &value);
> > +
> > +               if (ret < 0)
> > +                       return ret;
> > +
> > +               if (value != SCU_UNLOCKED_VALUE) {
>
> ... i.e. `if (value != 1)` here
>
> > +                       dev_err(ctx->dev,
> > +                               "SCU protection is active, cannot continue\n");
> > +                       return -EPERM;
> > +               }
> > +
>
> Doing this test for each value in the signal expression seems a bit
> excessive.

Ack

> I was suggesting we only print the warning if we detect the writes
> failed to stick (this is checked towards the end of e.g.
> aspeed_g4_sig_expr_set())

Ayy, thanks for pointing this out! I overlooked the 
`aspeed_sig_expr_eval` check at the end which definitely suits this case.

Cheers,
Tan