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 <[email protected]>
Cc: Joel Stanley <[email protected]>
Signed-off-by: Andrew Jeffery <[email protected]>
---
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;
+ return !strncmp(expr->signal, "GPI", 3) &&
+ !strcmp(expr->signal, expr->function);
}
static bool aspeed_gpio_in_exprs(const struct aspeed_sig_expr **exprs)
diff --git a/drivers/pinctrl/aspeed/pinmux-aspeed.h b/drivers/pinctrl/aspeed/pinmux-aspeed.h
index f86739e800c3..dba5875ff276 100644
--- a/drivers/pinctrl/aspeed/pinmux-aspeed.h
+++ b/drivers/pinctrl/aspeed/pinmux-aspeed.h
@@ -452,10 +452,11 @@ struct aspeed_sig_desc {
* evaluation of the descriptors.
*
* @signal: The signal name for the priority level on the pin. If the signal
- * type is GPIO, then the signal name must begin with the string
- * "GPIO", e.g. GPIOA0, GPIOT4 etc.
+ * type is GPIO, then the signal name must begin with the
+ * prefix "GPI", e.g. GPIOA0, GPIT0 etc.
* @function: The name of the function the signal participates in for the
- * associated expression
+ * associated expression. For pin-specific GPIO, the function
+ * name must match the signal name.
* @ndescs: The number of signal descriptors in the expression
* @descs: Pointer to an array of signal descriptors that comprise the
* function expression
--
2.27.0
On Thu, 26 Nov 2020 at 06:34, Andrew Jeffery <[email protected]> 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 <[email protected]>
> Cc: Joel Stanley <[email protected]>
> Signed-off-by: Andrew Jeffery <[email protected]>
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 <[email protected]>
Tested-by: Joel Stanley <[email protected]>
Cc: [email protected]
> ---
> 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;
> + return !strncmp(expr->signal, "GPI", 3) &&
> + !strcmp(expr->signal, expr->function);
> }
>
> static bool aspeed_gpio_in_exprs(const struct aspeed_sig_expr **exprs)
> diff --git a/drivers/pinctrl/aspeed/pinmux-aspeed.h b/drivers/pinctrl/aspeed/pinmux-aspeed.h
> index f86739e800c3..dba5875ff276 100644
> --- a/drivers/pinctrl/aspeed/pinmux-aspeed.h
> +++ b/drivers/pinctrl/aspeed/pinmux-aspeed.h
> @@ -452,10 +452,11 @@ struct aspeed_sig_desc {
> * evaluation of the descriptors.
> *
> * @signal: The signal name for the priority level on the pin. If the signal
> - * type is GPIO, then the signal name must begin with the string
> - * "GPIO", e.g. GPIOA0, GPIOT4 etc.
> + * type is GPIO, then the signal name must begin with the
> + * prefix "GPI", e.g. GPIOA0, GPIT0 etc.
> * @function: The name of the function the signal participates in for the
> - * associated expression
> + * associated expression. For pin-specific GPIO, the function
> + * name must match the signal name.
> * @ndescs: The number of signal descriptors in the expression
> * @descs: Pointer to an array of signal descriptors that comprise the
> * function expression
> --
> 2.27.0
>
On Thu, Nov 26, 2020 at 7:34 AM Andrew Jeffery <[email protected]> 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 <[email protected]>
> Cc: Joel Stanley <[email protected]>
> Signed-off-by: Andrew Jeffery <[email protected]>
Patch applied for fixes.
Yours,
Linus Walleij