Re: [PATCH v4 2/2] pinctrl: aspeed: Hand over global GPIOP passthrough to local mux controls

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

Hi Prasanth,

On Fri, 2026-11-06 at 07:59 +0530, Prasanth Kumar Padarthi wrote:
> On B650D4U, GPIO-only host power control fails when SCU51C[9]
> enables GPIOP passthrough, even after the normal GPIO request path
> clears the individual SCU4BC selections.
> 
> Transfer globally selected GPIOP0-P5 THRU roles to SCU4BC[29:24]
> before registering the controller, then clear the active shared
> sources. Leave PWM selection and the strap-source selector unchanged.
> Clear SCU510[28] through SCU514 and verify the shared enables cleared.
> 
> If handover fails, retain an instance-local error and reject affected
> GPIO and mux requests before changing their mux state. Check every
> pin in a mux group before calling the shared helper. Retain prepared
> local selections after partial failure rather than blindly restoring
> a shared enable.
> 
> Probe-time normalization policy, concurrent SCU writers and
> post-handover registration failure remain open design questions.
> Electrical continuity, mixed-pair forwarding, protected-write paths,
> PWM coexistence and SCU510[28] hardware handling remain unvalidated.
> 
> The implementation passed remote power-on, remote graceful power-off
> and physical-button power-on tests on B650D4U with an OpenBMC Linux
> 6.18.24-based kernel. The rebased version was ARM compile-tested on
> v7.4-rc1 using aspeed_g5_defconfig with AST2600 pinctrl enabled.
> With the managed-registration prerequisite, the v7.4-rc1 version
> also completed one unbind/rebind cycle under generic KASAN on
> B650D4U hardware and in AST2600 QEMU. Host power operations have
> not been retested with the v7.4-rc1 kernel.
> 
> Signed-off-by: Prasanth Kumar Padarthi <prasanth.padarthi10@gmail.com>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 145 ++++++++++++++++++++-
>  1 file changed, 141 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> index 85d6640ff..be22a9184 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> @@ -4,7 +4,10 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/pinmux.h>
>  #include <linux/types.h>
> @@ -36,6 +39,8 @@
>  #define SCU4D8		0x4D8 /* Multi-function Pin Control #23 */
>  #define SCU500		0x500 /* Hardware Strap 1 */
>  #define SCU510		0x510 /* Hardware Strap 2 */
> +#define SCU514		0x514 /* Hardware Strap 2 Clear */
> +#define SCU51C		0x51c /* Hardware Strap 3 */
>  #define SCU610		0x610 /* Disable GPIO Internal Pull-Down #0 */
>  #define SCU614		0x614 /* Disable GPIO Internal Pull-Down #1 */
>  #define SCU618		0x618 /* Disable GPIO Internal Pull-Down #2 */
> @@ -2743,12 +2748,56 @@ static struct aspeed_pinctrl_data aspeed_g6_pinctrl_data = {
>  	.nconfmaps = ARRAY_SIZE(aspeed_g6_pin_config_map),
>  };
>  
> +struct aspeed_g6_probe_data {
> +	struct aspeed_pinctrl_data pdata;
> +	int thru_error;
> +};
> +
> +static bool aspeed_g6_is_thru_pin(unsigned int pin)
> +{
> +	return pin >= AB22 && pin <= AB23;
> +}
> +
> +static int aspeed_g6_gpio_request_enable(struct pinctrl_dev *pctldev,
> +					 struct pinctrl_gpio_range *range,
> +					 unsigned int offset)
> +{
> +	struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> +	struct aspeed_g6_probe_data *g6 =
> +		container_of(pdata, struct aspeed_g6_probe_data, pdata);
> +
> +	if (g6->thru_error && aspeed_g6_is_thru_pin(offset))
> +		return g6->thru_error;
> +
> +	return aspeed_gpio_request_enable(pctldev, range, offset);
> +}
> +
> +static int aspeed_g6_set_mux(struct pinctrl_dev *pctldev,
> +			     unsigned int function, unsigned int group)
> +{
> +	struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> +	struct aspeed_g6_probe_data *g6 =
> +		container_of(pdata, struct aspeed_g6_probe_data, pdata);
> +	const struct aspeed_pin_group *pgroup = &pdata->pinmux.groups[group];
> +	unsigned int i;
> +
> +	/* Check the entire group before the shared helper changes any pin. */
> +	if (g6->thru_error) {
> +		for (i = 0; i < pgroup->npins; i++) {
> +			if (aspeed_g6_is_thru_pin(pgroup->pins[i]))
> +				return g6->thru_error;
> +		}
> +	}
> +
> +	return aspeed_pinmux_set_mux(pctldev, function, group);
> +}
> +
>  static const struct pinmux_ops aspeed_g6_pinmux_ops = {
>  	.get_functions_count = aspeed_pinmux_get_fn_count,
>  	.get_function_name = aspeed_pinmux_get_fn_name,
>  	.get_function_groups = aspeed_pinmux_get_fn_groups,
> -	.set_mux = aspeed_pinmux_set_mux,
> -	.gpio_request_enable = aspeed_gpio_request_enable,
> +	.set_mux = aspeed_g6_set_mux,
> +	.gpio_request_enable = aspeed_g6_gpio_request_enable,
>  	.strict = true,
>  };
>  
> @@ -2778,15 +2827,103 @@ static const struct pinctrl_desc aspeed_g6_pinctrl_desc = {
>  	.confops = &aspeed_g6_conf_ops,
>  };
>  
> +/*
> + * Experimental: transfer the global GPIOP0-P5 THRU selection to the
> + * individual mux controls before registering any pinctrl consumers.
> + * PWM selection is higher priority and is deliberately left unchanged.
> + *
> + * Do not undo the local selections on error: a shared enable may already
> + * have cleared. Keeping the equivalent local selections preserves routing
> + * while rejecting affected requests, rather than granting an unsafe GPIO.
> + * Signal continuity during this transfer still needs hardware validation.
> + */
> +static int aspeed_g6_thru_handover(struct regmap *scu)
> +{
> +	const u32 local_mask = GENMASK(29, 24);

Defining a variable with the value (even though that variable is
qualified const) means it's not an integer constant expression, which
may impede some optimisations.

Further, local_mask is not a descriptive name for what's occurring
here. Please name the mask in terms of its behaviour. Perhaps:

#define GPIOP_UNGROUPED_PASSTHROUGH_MASK GENMASK(29, 24)

> +	unsigned int strap2, strap3, local;

Please lets improve on the name 'local'. Perhaps: ungrouped_passthrough

> +	int ret;
> +
> +	ret = regmap_read(scu, SCU510, &strap2);
> +	if (ret)
> +		return ret;

Consider this error-handling strategy:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/pinctrl-gemini.c?h=v7.4-rc1#n2556

If the first read doesn't work, then we can't expect any of the further
accesses by the core aspeed pinctrl implementation to work either, so
we can just bail out entirely in the caller.

> +	ret = regmap_read(scu, SCU51C, &strap3);
> +	if (ret)
> +		return ret;
> +
> +	if (!(strap2 & BIT(28)) &&
> +	    ((strap2 & BIT(30)) || !(strap3 & BIT(9))))

Please give these bits named macros. This is not easy to comprehend as
it is.

Also, consider FIELD_GET() from include/linux/bitfield.h. Something
like:

   if (!FIELD_GET(OTPSTRAP_ENABLE_GPIO_PASSTHROUGH, strap2) &&
       (FIELD_GET(OTPSTRAP_ENABLE_DEDICATED_STRAP_PINS, strap2) ||
           !(FIELD_GET(STRAP_ENABLE_GPIO_PASSTHROUGH, strap3)))


> +		return 0;
> +
> +	/* Preserve every globally selected role, including under PWM. */

This comment feels like a classic LLM statement that somehow manages to
be simultaneously both too abstract and too detailed, while also devoid
of context, all in 8 words.

Can we rather say that the PWM functions on these pins are higher
priority in the mux than the pass-through GPIO function, so if PWM is
already selected, then the following write does not affect the PWM
configuration?

> +	ret = regmap_update_bits(scu, SCU4BC, local_mask, local_mask);
> +	if (ret)
> +		return ret;
> +	ret = regmap_read(scu, SCU4BC, &local);
> +	if (ret)
> +		return ret;
> +	if ((local & local_mask) != local_mask)
> +		return -EPERM;

Please add a comment here about the write-protection in SCUF30.

> +
> +	/* Leave the inactive GPIOZ6 source alone in dedicated-strap mode. */

'dedicated strap mode' is a confusing statement because ASPEED's
documentation is confusing. I think we can drop this comment with
regard to making other improvements.

> +	if (!(strap2 & BIT(30)) && (strap3 & BIT(9))) {

We've already inspected these bit values previously. Please consider
extracting them to bool variables for re-use and to improve everyone's
ability to comprehend the expression.

> +		ret = regmap_update_bits(scu, SCU51C, BIT(9), 0);

Essentially, having configured the ungrouped passthrough functions
above, we can now de-configure the grouped passthrough function in
strap3 (SCU51C).

It's probably worth noting in a comment that SCU51C cannot be write-
protected.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (strap2 & BIT(28)) {
> +		/* SCU510 is write-one-set; do not use an ordinary RW clear. */
> +		ret = regmap_write(scu, SCU514, BIT(28));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(scu, SCU510, &strap2);
> +	if (ret)
> +		return ret;
> +	ret = regmap_read(scu, SCU51C, &strap3);
> +	if (ret)
> +		return ret;
> +	if ((strap2 & BIT(28)) ||
> +	    (!(strap2 & BIT(30)) && (strap3 & BIT(9))))
> +		return -EPERM;

This appears to be the negated expression of the earlier one, and given
we're only returning a fixed value in either case, you could rather use
the same expression again. Given it's repeated, you could extract it to
a macro or static inline function.

> +
> +	return 0;
> +}
> +
>  static int aspeed_g6_pinctrl_probe(struct platform_device *pdev)
>  {
> -	int i;
> +	struct aspeed_g6_probe_data *g6;
> +	struct regmap *scu;
> +	int i, ret;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +	scu = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(scu))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(scu),
> +				     "No SCU regmap for THRU handover\n");
> +
> +	g6 = devm_kzalloc(&pdev->dev, sizeof(*g6), GFP_KERNEL);
> +	if (!g6)
> +		return -ENOMEM;
> +	memcpy(&g6->pdata, &aspeed_g6_pinctrl_data, sizeof(g6->pdata));

This seems unfortunate.

Have you considered other approaches?

> +
> +	ret = aspeed_g6_thru_handover(scu);

Must this logic be separate to the usual set_mux path? If not, I expect
we don't need to retain thru_error.

> +	if (ret == -EPROBE_DEFER)

Do you have an example of how ret could be -EPROBE_DEFER?

> +		return ret;
> +	if (ret) {

If this handover logic is to be integrated here, can you please test
for specific return values to separate the regmap access failure from
the write-protection failure?

Related, I've heard opinions from Tan that this patch is not necessary
to get the platform booting. Given the current complexity, I'm
interested to hear whether you can do without it.

Andrew