Re: [PATCH] pinctrl: aspeed: Log error if SCU protection is active
From: Andrew Jeffery <andrew@codeconstruct.com.au>
Date: 2025-06-13 06:02:01
Also in:
linux-aspeed, linux-gpio, lkml, openbmc
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" :/
quoted hunk ↗ jump to hunk
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 <redacted> Signed-off-by: Tan Siewert <redacted> --- 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 ...
quoted hunk ↗ jump to hunk
+ /* * 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