Messages in this thread | | | Subject | Re: [PATCH 1/2 v9] serial: mctrl_gpio: Check if GPIO property exisits before requesting it | From | Stefan Roese <> | Date | Mon, 24 Jun 2019 17:29:24 +0200 |
| |
On 24.06.19 10:42, Geert Uytterhoeven wrote: > CC gpio > > This is now commit d99482673f950817 ("serial: mctrl_gpio: Check if GPIO > property exisits before requesting it") in tty-next. > > On Thu, Jun 20, 2019 at 8:24 AM Stefan Roese <sr@denx.de> wrote: >> This patch adds a check for the GPIOs property existence, before the >> GPIO is requested. This fixes an issue seen when the 8250 mctrl_gpio >> support is added (2nd patch in this patch series) on x86 platforms using >> ACPI. >> >> Here Mika's comments from 2016-08-09: >> >> " >> I noticed that with v4.8-rc1 serial console of some of our Broxton >> systems does not work properly anymore. I'm able to see output but input >> does not work. >> >> I bisected it down to commit 4ef03d328769eddbfeca1f1c958fdb181a69c341 >> ("tty/serial/8250: use mctrl_gpio helpers"). >> >> The reason why it fails is that in ACPI we do not have names for GPIOs >> (except when _DSD is used) so we use the "idx" to index into _CRS GPIO >> resources. Now mctrl_gpio_init_noauto() goes through a list of GPIOs >> calling devm_gpiod_get_index_optional() passing "idx" of 0 for each. The >> UART device in Broxton has following (simplified) ACPI description: >> >> Device (URT4) >> { >> ... >> Name (_CRS, ResourceTemplate () { >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, >> "\\_SB.GPO0", 0x00, ResourceConsumer) >> { >> 0x003A >> } >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, >> "\\_SB.GPO0", 0x00, ResourceConsumer) >> { >> 0x003D >> } >> }) >> >> In this case it finds the first GPIO (0x003A which happens to be RX pin >> for that UART), turns it into GPIO which then breaks input for the UART >> device. This also breaks systems with bluetooth connected to UART (those >> typically have some GPIOs in their _CRS). >> >> Any ideas how to fix this? >> >> We cannot just drop the _CRS index lookup fallback because that would >> break many existing machines out there so maybe we can limit this to >> only DT enabled machines. Or alternatively probe if the property first >> exists before trying to acquire the GPIOs (using >> device_property_present()). >> " >> >> This patch implements the fix suggested by Mika in his statement above. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Tested-by: Yegor Yefremov <yegorslists@googlemail.com> >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Cc: Yegor Yefremov <yegorslists@googlemail.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Giulio Benetti <giulio.benetti@micronovasrl.com> >> --- >> v9: >> - Rebased on top of "tty-next", patch 2/3 dropped as its already applied >> >> v8: >> - Rebased on top of "tty-next" >> >> v7: >> - Include <linux/property.h> to fix compile breakage on OMAP >> >> v6: >> - No change >> >> v5: >> - Simplified the code a bit (Andy) >> - Added gpio_str == NULL handling (Andy) >> >> v4: >> - Add missing free() calls (Johan) >> - Added Mika's reviewed by tag >> - Added Johan to Cc >> >> v3: >> - No change >> >> v2: >> - Include the problem description and analysis from Mika into the commit >> text, as suggested by Greg. >> >> drivers/tty/serial/serial_mctrl_gpio.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/tty/serial/serial_mctrl_gpio.c b/drivers/tty/serial/serial_mctrl_gpio.c >> index 39ed56214cd3..2b400189be91 100644 >> --- a/drivers/tty/serial/serial_mctrl_gpio.c >> +++ b/drivers/tty/serial/serial_mctrl_gpio.c >> @@ -12,6 +12,7 @@ >> #include <linux/termios.h> >> #include <linux/serial_core.h> >> #include <linux/module.h> >> +#include <linux/property.h> >> >> #include "serial_mctrl_gpio.h" >> >> @@ -116,6 +117,19 @@ struct mctrl_gpios *mctrl_gpio_init_noauto(struct device *dev, unsigned int idx) >> >> for (i = 0; i < UART_GPIO_MAX; i++) { >> enum gpiod_flags flags; >> + char *gpio_str; >> + bool present; >> + >> + /* Check if GPIO property exists and continue if not */ >> + gpio_str = kasprintf(GFP_KERNEL, "%s-gpios", >> + mctrl_gpios_desc[i].name); > > This will silently break DTBs using "(cts|dsr|dcd|rng|rts|dtr)-gpio" instead > of "(cts|dsr|dcd|rng|rts|dtr)-gpios".
Should both options be supported ("cts-gpio" vs "cts-gpios")? Documentation/devicetree/bindings/serial/serial.txt only mentions the "-gpios" variant.
Thanks, Stefan
| |