Re: [PATCH] pinctrl: aspeed: Fix GPIO requests on pass-through banks
On Thu, Nov 26, 2020 at 7:34 AM Andrew Jeffery wrote:
> Commit 6726fbff19bf ("pinctrl: aspeed: Fix GPI only function problem.")
> fixes access to GPIO banks T and U on the AST2600. Both banks contain
> input-only pins and the GPIO pin function is named GPITx and GPIUx
> respectively. Unfortunately the fix had a negative impact on GPIO banks
> D and E for the AST2400 and AST2500 where the GPIO pass-through
> functions take similar "GPI"-style names. The net effect on the older
> SoCs was that when the GPIO subsystem requested a pin in banks D or E be
> muxed for GPIO, they were instead muxed for pass-through mode.
> Mistakenly muxing pass-through mode e.g. breaks booting the host on
> IBM's Witherspoon (AC922) platform where GPIOE0 is used for FSI.
>
> Further exploit the names in the provided expression structure to
> differentiate pass-through from pin-specific GPIO modes.
>
> This follow-up fix gives the expected behaviour for the following tests:
>
> Witherspoon BMC (AST2500):
>
> 1. Power-on the Witherspoon host
> 2. Request GPIOD1 be muxed via /sys/class/gpio/export
> 3. Request GPIOE1 be muxed via /sys/class/gpio/export
> 4. Request the balls for GPIOs E2 and E3 be muxed as GPIO pass-through
>("GPIE2" mode) via a pinctrl hog in the devicetree
>
> Rainier BMC (AST2600):
>
> 5. Request GPIT0 be muxed via /sys/class/gpio/export
> 6. Request GPIU0 be muxed via /sys/class/gpio/export
>
> Together the tests demonstrate that all three pieces of functionality
> (general GPIOs via 1, 2 and 3, input-only GPIOs via 5 and 6, pass-through
> mode via 4) operate as desired across old and new SoCs.
>
> Fixes: 6726fbff19bf ("pinctrl: aspeed: Fix GPI only function problem.")
> Cc: Billy Tsai
> Cc: Joel Stanley
> Signed-off-by: Andrew Jeffery
Patch applied for fixes.
Yours,
Linus Walleij
Re: [PATCH] pinctrl: aspeed: Fix GPIO requests on pass-through banks
On Thu, 26 Nov 2020 at 06:34, Andrew Jeffery wrote:
>
> Commit 6726fbff19bf ("pinctrl: aspeed: Fix GPI only function problem.")
> fixes access to GPIO banks T and U on the AST2600.
...but caused a regression when muxing GPIOs.
> Fixes: 6726fbff19bf ("pinctrl: aspeed: Fix GPI only function problem.")
> Cc: Billy Tsai
> Cc: Joel Stanley
> Signed-off-by: Andrew Jeffery
I didn't read all of the text, but the code change looks good. This
should go to stable as the offending commit was also added to stable.
Reviewed-by: Joel Stanley
Tested-by: Joel Stanley
Cc: sta...@vger.kernel.org
> ---
> drivers/pinctrl/aspeed/pinctrl-aspeed.c | 74 +++--
> drivers/pinctrl/aspeed/pinmux-aspeed.h | 7 ++-
> 2 files changed, 72 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 1d603732903f..9c44ef11b567 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -286,14 +286,76 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev,
> unsigned int function,
> static bool aspeed_expr_is_gpio(const struct aspeed_sig_expr *expr)
> {
> /*
> -* The signal type is GPIO if the signal name has "GPI" as a prefix.
> -* strncmp (rather than strcmp) is used to implement the prefix
> -* requirement.
> +* We need to differentiate between GPIO and non-GPIO signals to
> +* implement the gpio_request_enable() interface. For better or worse
> +* the ASPEED pinctrl driver uses the expression names to determine
> +* whether an expression will mux a pin for GPIO.
> *
> -* expr->signal might look like "GPIOB1" in the GPIO case.
> -* expr->signal might look like "GPIT0" in the GPI case.
> +* Generally we have the following - A GPIO such as B1 has:
> +*
> +*- expr->signal set to "GPIOB1"
> +*- expr->function set to "GPIOB1"
> +*
> +* Using this fact we can determine whether the provided expression is
> +* a GPIO expression by testing the signal name for the string prefix
> +* "GPIO".
> +*
> +* However, some GPIOs are input-only, and the ASPEED datasheets name
> +* them differently. An input-only GPIO such as T0 has:
> +*
> +*- expr->signal set to "GPIT0"
> +*- expr->function set to "GPIT0"
> +*
> +* It's tempting to generalise the prefix test from "GPIO" to "GPI" to
> +* account for both GPIOs and GPIs, but in doing so we run aground on
> +* another feature:
> +*
> +* Some pins in the ASPEED BMC SoCs have a "pass-through" GPIO
> +* function where the input state of one pin is replicated as the
> +* output state of another (as if they were shorted together - a mux
> +* configuration that is typically enabled by hardware strapping).
> +* This feature allows the BMC to pass e.g. power button state through
> +* to the host while the BMC is yet to boot, but take control of the
> +* button state once the BMC has booted by muxing each pin as a
> +* separate, pin-specific GPIO.
> +*
> +* Conceptually this pass-through mode is a form of GPIO and is named
> +* as such in the datasheets, e.g. "GPID0". This naming similarity
> +* trips us up with the simple GPI-prefixed-signal-name scheme
> +* discussed above, as the pass-through configuration is not what we
> +* want when muxing a pin as GPIO for the GPIO subsystem.
> +*
> +* On e.g. the AST2400, a pass-through function "GPID0" is grouped on
> +* balls A18 and D16, where we have:
> +*
> +*For ball A18:
> +*- expr->signal set to "GPID0IN"
> +*- expr->function set to "GPID0"
> +*
> +*For ball D16:
> +*- expr->signal set to "GPID0OUT"
> +*- expr->function set to "GPID0"
> +*
> +* By contrast, the pin-specific GPIO expressions for the same pins
> are
> +* as follows:
> +*
> +*For ball A18:
> +*- expr->signal looks like "GPIOD0"
> +*- expr->function looks like "GPIOD0"
> +*
> +*For ball D16:
> +*- expr->signal looks like "GPIOD1"
> +*- expr->function looks like "GPIOD1"
> +*
> +* Testing both the signal _and_ function names gives us the means
> +* differentiate the pass-through GPIO pinmux configuration from the
> +* pin-specific configuration that the GPIO subsystem is after: An
> +* expression is a pin-specific (non-pass-through) GPIO configuration
> +* if the signal prefix is "GPI" and the signal name matches the
> +* function name.
> */
> - return strncmp(expr->signal, "GPI", 3) == 0;
[PATCH] pinctrl: aspeed: Fix GPIO requests on pass-through banks
Commit 6726fbff19bf ("pinctrl: aspeed: Fix GPI only function problem.")
fixes access to GPIO banks T and U on the AST2600. Both banks contain
input-only pins and the GPIO pin function is named GPITx and GPIUx
respectively. Unfortunately the fix had a negative impact on GPIO banks
D and E for the AST2400 and AST2500 where the GPIO pass-through
functions take similar "GPI"-style names. The net effect on the older
SoCs was that when the GPIO subsystem requested a pin in banks D or E be
muxed for GPIO, they were instead muxed for pass-through mode.
Mistakenly muxing pass-through mode e.g. breaks booting the host on
IBM's Witherspoon (AC922) platform where GPIOE0 is used for FSI.
Further exploit the names in the provided expression structure to
differentiate pass-through from pin-specific GPIO modes.
This follow-up fix gives the expected behaviour for the following tests:
Witherspoon BMC (AST2500):
1. Power-on the Witherspoon host
2. Request GPIOD1 be muxed via /sys/class/gpio/export
3. Request GPIOE1 be muxed via /sys/class/gpio/export
4. Request the balls for GPIOs E2 and E3 be muxed as GPIO pass-through
("GPIE2" mode) via a pinctrl hog in the devicetree
Rainier BMC (AST2600):
5. Request GPIT0 be muxed via /sys/class/gpio/export
6. Request GPIU0 be muxed via /sys/class/gpio/export
Together the tests demonstrate that all three pieces of functionality
(general GPIOs via 1, 2 and 3, input-only GPIOs via 5 and 6, pass-through
mode via 4) operate as desired across old and new SoCs.
Fixes: 6726fbff19bf ("pinctrl: aspeed: Fix GPI only function problem.")
Cc: Billy Tsai
Cc: Joel Stanley
Signed-off-by: Andrew Jeffery
---
drivers/pinctrl/aspeed/pinctrl-aspeed.c | 74 +++--
drivers/pinctrl/aspeed/pinmux-aspeed.h | 7 ++-
2 files changed, 72 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index 1d603732903f..9c44ef11b567 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -286,14 +286,76 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev,
unsigned int function,
static bool aspeed_expr_is_gpio(const struct aspeed_sig_expr *expr)
{
/*
-* The signal type is GPIO if the signal name has "GPI" as a prefix.
-* strncmp (rather than strcmp) is used to implement the prefix
-* requirement.
+* We need to differentiate between GPIO and non-GPIO signals to
+* implement the gpio_request_enable() interface. For better or worse
+* the ASPEED pinctrl driver uses the expression names to determine
+* whether an expression will mux a pin for GPIO.
*
-* expr->signal might look like "GPIOB1" in the GPIO case.
-* expr->signal might look like "GPIT0" in the GPI case.
+* Generally we have the following - A GPIO such as B1 has:
+*
+*- expr->signal set to "GPIOB1"
+*- expr->function set to "GPIOB1"
+*
+* Using this fact we can determine whether the provided expression is
+* a GPIO expression by testing the signal name for the string prefix
+* "GPIO".
+*
+* However, some GPIOs are input-only, and the ASPEED datasheets name
+* them differently. An input-only GPIO such as T0 has:
+*
+*- expr->signal set to "GPIT0"
+*- expr->function set to "GPIT0"
+*
+* It's tempting to generalise the prefix test from "GPIO" to "GPI" to
+* account for both GPIOs and GPIs, but in doing so we run aground on
+* another feature:
+*
+* Some pins in the ASPEED BMC SoCs have a "pass-through" GPIO
+* function where the input state of one pin is replicated as the
+* output state of another (as if they were shorted together - a mux
+* configuration that is typically enabled by hardware strapping).
+* This feature allows the BMC to pass e.g. power button state through
+* to the host while the BMC is yet to boot, but take control of the
+* button state once the BMC has booted by muxing each pin as a
+* separate, pin-specific GPIO.
+*
+* Conceptually this pass-through mode is a form of GPIO and is named
+* as such in the datasheets, e.g. "GPID0". This naming similarity
+* trips us up with the simple GPI-prefixed-signal-name scheme
+* discussed above, as the pass-through configuration is not what we
+* want when muxing a pin as GPIO for the GPIO subsystem.
+*
+* On e.g. the AST2400, a pass-through function "GPID0" is grouped on
+* balls A18 and D16, where we have:
+*
+*For ball A18:
+*- expr->signal set to "GPID0IN"
+*- expr->function set to "GPID0"
+*
+*For ball D16:
+*- expr->signal set to "GPID0OUT"
+*- expr->function set to "GPID0"
+
