diff mbox series

pinctrl: aspeed: Log error if SCU protection is active

Message ID 20250612151900.32874-1-tan@siewert.io
State New
Headers show
Series pinctrl: aspeed: Log error if SCU protection is active | expand

Commit Message

Tan Siewert June 12, 2025, 3:18 p.m. UTC
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, 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(-)

Comments

Tan Siewert June 13, 2025, 11:32 a.m. UTC | #1
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
diff mbox series

Patch

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
+
 /*
  * 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) {
+			dev_err(ctx->dev,
+				"SCU protection is active, cannot continue\n");
+			return -EPERM;
+		}
+
 		ret = regmap_update_bits(ctx->maps[desc->ip], desc->reg,
 					 desc->mask, val);
 
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
index 5bb8fd0d1e41..7b3f887edda5 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.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
+
 /*
  * The "Multi-function Pins Mapping and Control" table in the SoC datasheet
  * references registers by the device/offset mnemonic. The register macros
@@ -37,6 +39,7 @@ 
  * reset control and MAC clock configuration registers. The AST2500 goes a step
  * further and references registers in the graphics IP block.
  */
+#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 */
@@ -2763,7 +2766,26 @@  static int aspeed_g5_sig_expr_set(struct aspeed_pinmux_data *ctx,
 
 		/* On AST2500, Set bits in SCU70 are cleared from SCU7C */
 		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1) {
-			u32 value = ~val & desc->mask;
+			/*
+			 * 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) {
+				dev_err(ctx->dev,
+					"SCU protection is active, cannot continue\n");
+				return -EPERM;
+			}
+
+			value = ~val & desc->mask;
 
 			if (value) {
 				ret = regmap_write(ctx->maps[desc->ip],
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index 5a7cd0a88687..68e40f2c015b 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -17,6 +17,10 @@ 
 #include "../pinctrl-utils.h"
 #include "pinctrl-aspeed.h"
 
+#define SCU_UNLOCKED_VALUE 0x00000001
+
+#define SCU000		0x000 /* Protection Key Register */
+#define SCU010		0x010 /* Protection Key Register 2 */
 #define SCU400		0x400 /* Multi-function Pin Control #1  */
 #define SCU404		0x404 /* Multi-function Pin Control #2  */
 #define SCU40C		0x40C /* Multi-function Pin Control #3  */
@@ -2668,6 +2672,28 @@  static int aspeed_g6_sig_expr_set(struct aspeed_pinmux_data *ctx,
 		WARN_ON(desc->ip != ASPEED_IP_SCU);
 		is_strap = desc->reg == SCU500 || desc->reg == SCU510;
 
+		/*
+		 * The SCU should be unlocked, with SCU000 and SCU010
+		 * both returning 0x01. However, it may have been locked,
+		 * e.g. by a userspace script using /dev/mem.
+		 */
+		u32 scuprt_val, scuprt2_val;
+
+		ret = regmap_read(ctx->maps[desc->ip], SCU000, &scuprt_val);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_read(ctx->maps[desc->ip], SCU010, &scuprt2_val);
+		if (ret < 0)
+			return ret;
+
+		if (scuprt_val != SCU_UNLOCKED_VALUE ||
+		    scuprt2_val != SCU_UNLOCKED_VALUE) {
+			dev_err(ctx->dev,
+				"SCU protection is active, cannot continue\n");
+			return -EPERM;
+		}
+
 		if (is_strap) {
 			/*
 			 * The AST2600 has write protection mask registers for