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

[Andrew Jeffery <andrew@codeconstruct.com.au>]

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.

"Don't do that" :/

> causing pinctrl operations such as disabling GPIOD passthrough
> to fail in not-so-obvious ways. For example, a GPIO request for GPID0 on
> an AST2500 fails with:
> 
>   [  428.204733] aspeed-g5-pinctrl 1e6e2080.pinctrl: request() failed for pin 24
>   [  428.204998] aspeed-g5-pinctrl 1e6e2080.pinctrl: pin-24 (1e780000.gpio:536) status -1
> 
> With dynamic_debug enabled, the SCU write failures become visible:
> 
>   [  428.204657] Disabling signal GPID0IN for GPID
>   [  428.204673] Want SCU70[0x00200000]=0x1, got 0x1 from 0xF122D206
>   [  428.204708] Want SCU70[0x00200000]=0x0, got 0x1 from 0xF122D206
> 
> Since SCU unlocking would need to be done in multiple drivers, adding
> unlock logic to each is not viable. Instead, this patch adds an
> explicit error message and early abort in `sig_expr_set()` if SCU
> protection is detected by checking the SCU Protection Key Register.
> 
> Before:
> 
>   [  428.204733] aspeed-g5-pinctrl 1e6e2080.pinctrl: request() failed for pin 24
>   [  428.204998] aspeed-g5-pinctrl 1e6e2080.pinctrl: pin-24 (1e780000.gpio:536) status -1
> 
> After:
> 
>   [   43.558353] aspeed-g5-pinctrl 1e6e2080.pinctrl: SCU protection is active, cannot continue
>   [   43.559107] aspeed-g5-pinctrl 1e6e2080.pinctrl: request() failed for pin 24
>   [   43.559434] aspeed-g5-pinctrl 1e6e2080.pinctrl: pin-24 (1e780000.gpio:536) status -1
> 
> Suggested-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Tan Siewert <tan@siewert.io>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c | 21 +++++++++++++++++
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 24 +++++++++++++++++++-
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 26 ++++++++++++++++++++++
>  3 files changed, 70 insertions(+), 1 deletion(-)
> 
> 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 ...

> +
>  /*
>   * 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.

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

Andrew