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

[Tan Siewert <tan@siewert.io>]

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