| Message ID | 20201126063337.489927-1-andrew@aj.id.au |
|---|---|
| State | Accepted |
| Commit | 7aeb353802611a8e655e019f09a370ff682af1a6 |
| Headers | show |
| Series | pinctrl: aspeed: Fix GPIO requests on pass-through banks | expand |
On Thu, 26 Nov 2020 at 06:34, Andrew Jeffery <andrew@aj.id.au> 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 <billy_tsai@aspeedtech.com> > Cc: Joel Stanley <joel@jms.id.au> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> 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 <joel@jms.id.au> Tested-by: Joel Stanley <joel@jms.id.au> Cc: stable@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; > + 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 <andrew@aj.id.au> 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 <billy_tsai@aspeedtech.com> > Cc: Joel Stanley <joel@jms.id.au> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Patch applied for fixes. Yours, Linus Walleij
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
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 <billy_tsai@aspeedtech.com> Cc: Joel Stanley <joel@jms.id.au> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/pinctrl/aspeed/pinctrl-aspeed.c | 74 +++++++++++++++++++++++-- drivers/pinctrl/aspeed/pinmux-aspeed.h | 7 ++- 2 files changed, 72 insertions(+), 9 deletions(-)