From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 21E07C43613 for ; Mon, 24 Jun 2019 15:29:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EA24920652 for ; Mon, 24 Jun 2019 15:29:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730970AbfFXP3g (ORCPT ); Mon, 24 Jun 2019 11:29:36 -0400 Received: from mx2.mailbox.org ([80.241.60.215]:34618 "EHLO mx2.mailbox.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726708AbfFXP3e (ORCPT ); Mon, 24 Jun 2019 11:29:34 -0400 Received: from smtp1.mailbox.org (smtp1.mailbox.org [80.241.60.240]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by mx2.mailbox.org (Postfix) with ESMTPS id 86C8DA1240; Mon, 24 Jun 2019 17:29:30 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp1.mailbox.org ([80.241.60.240]) by spamfilter04.heinlein-hosting.de (spamfilter04.heinlein-hosting.de [80.241.56.122]) (amavisd-new, port 10030) with ESMTP id 2MhzBchGb8rw; Mon, 24 Jun 2019 17:29:26 +0200 (CEST) Subject: Re: [PATCH 1/2 v9] serial: mctrl_gpio: Check if GPIO property exisits before requesting it To: Geert Uytterhoeven Cc: "open list:SERIAL DRIVERS" , Linux Kernel Mailing List , Mika Westerberg , Andy Shevchenko , Yegor Yefremov , Greg Kroah-Hartman , Giulio Benetti , Linus Walleij , Bartosz Golaszewski , "open list:GPIO SUBSYSTEM" References: <20190620062420.11650-1-sr@denx.de> From: Stefan Roese Message-ID: <24bfb52c-6f77-b7cd-7421-9e6e4b0aa7d3@denx.de> Date: Mon, 24 Jun 2019 17:29:24 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 >> Reviewed-by: Mika Westerberg >> Reviewed-by: Andy Shevchenko >> Tested-by: Yegor Yefremov >> Cc: Mika Westerberg >> Cc: Andy Shevchenko >> Cc: Yegor Yefremov >> Cc: Greg Kroah-Hartman >> Cc: Giulio Benetti >> --- >> v9: >> - Rebased on top of "tty-next", patch 2/3 dropped as its already applied >> >> v8: >> - Rebased on top of "tty-next" >> >> v7: >> - Include 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 >> #include >> #include >> +#include >> >> #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