Message-ID: <Zmov-8IGm-misoRs@google.com>
Date: Wed, 12 Jun 2024 16:32:11 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Martin Schiller <ms@....tdt.de>
Cc: hauke@...ke-m.de, tsbogend@...ha.franken.de, rdunlap@...radead.org,
	robh@...nel.org, bhelgaas@...gle.com, linux-mips@...r.kernel.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] MIPS: pci: lantiq: restore reset gpio polarity

On Wed, Jun 12, 2024 at 02:45:37PM -0700, Dmitry Torokhov wrote:
> On Wed, Jun 12, 2024 at 09:47:39PM +0200, Martin Schiller wrote:
> > On 2024-06-12 20:39, Martin Schiller wrote:
> > > On 2024-06-12 19:47, Dmitry Torokhov wrote:
> > > > Hi Marton,
> > > 
> > > Hi Dmitry,
> > > 
> > > > 
> > > > On Fri, Jun 07, 2024 at 11:04:00AM +0200, Martin Schiller wrote:
> > > > > Commit 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod
> > > > > API") not
> > > > > only switched to the gpiod API, but also inverted / changed the
> > > > > polarity
> > > > > of the GPIO.
> > > > > 
> > > > > According to the PCI specification, the RST# pin is an active-low
> > > > > signal. However, most of the device trees that have been widely
> > > > > used for
> > > > > a long time (mainly in the openWrt project) define this GPIO as
> > > > > active-high and the old driver code inverted the signal internally.
> > > > > 
> > > > > Apparently there are actually boards where the reset gpio must be
> > > > > operated inverted. For this reason, we cannot use the
> > > > > GPIOD_OUT_LOW/HIGH
> > > > > flag for initialization. Instead, we must explicitly set the gpio to
> > > > > value 1 in order to take into account any "GPIO_ACTIVE_LOW" flag that
> > > > > may have been set.
> > > > 
> > > > Do you have example of such boards? They could not have worked before
> > > > 90c2d2eb7ab5 because it was actively setting the reset line to
> > > > physical
> > > > high, which should leave the device in reset state if there is an
> > > > inverter between the AP and the device.
> > > 
> > > Oh, you're right. I totally missed that '__gpio_set_value' was used in
> > > the original code and that raw accesses took place without paying
> > > attention to the GPIO_ACTIVE_* flags.
> > > 
> > > You can find the device trees I am talking about in [1].
> > > 
> > > @Thomas Bogendoerfer:
> > > Would it be possible to stop the merging of this patch?
> > > I think We have to do do some further/other changes.
> > > 
> > > > 
> > > > > 
> > > > > In order to remain compatible with all these existing device
> > > > > trees, we
> > > > > should therefore keep the logic as it was before the commit.
> > > > 
> > > > With gpiod API operating with logical states there's still
> > > > difference in
> > > > logic:
> > > > 
> > > > 	gpiod_set_value_cansleep(reset_gpio, 1);
> > > > 
> > > > will leave GPIO at 1 if it is described as GPIO_ACTIVE_HIGH (which is
> > > > apparently what you want for boards with broken DTS) but for boards
> > > > that accurately describe GPIO as GPIO_ACTIVE_LOW it well drive GPIO to
> > > > 0, leaving the card in reset state.
> > > > 
> > > > You should either use gpiod_set_raw_value_calsleep() or we can try and
> > > > quirk it in gpiolib (like we do for many other cases of incorrect GPIO
> > > > polarity descriptions and which is my preference).
> > 
> > So you mean we should add an entry for "lantiq,pci-xway" to the
> > of_gpio_try_fixup_polarity()?
> > Do you know any dts / device outside the openWrt universe which is using
> > this driver.
> 
> No, I don't.
> 
> Could you please try this:
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 59c7f8a2431a..4948ecaa422c 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -203,6 +203,16 @@ static void of_gpio_try_fixup_polarity(const struct device_node *np,
>  		 */
>  		{ "qi,lb60",		"rb-gpios",	true },
>  #endif
> +#if IS_ENABLED(CONFIG_PCI_LANTIQ)
> +		/*
> +		 * According to the PCI specification, the RST# pin is an
> +		 * active-low signal. However, most of the device trees that
> +		 * have been widely used for a long time incorrectly describe
> +		 * reset GPIO as active-high, and were also using wrong name
> +		 * for the property.
> +		 */
> +		{ "lantiq,pci-xway",	"gpios-reset",	false },

Sorry, "gpios-reset" is wrong, the driver used "gpio-reset". So:

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 59c7f8a2431a..d21085830632 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -203,6 +203,16 @@ static void of_gpio_try_fixup_polarity(const struct device_node *np,
 		 */
 		{ "qi,lb60",		"rb-gpios",	true },
 #endif
+#if IS_ENABLED(CONFIG_PCI_LANTIQ)
+		/*
+		 * According to the PCI specification, the RST# pin is an
+		 * active-low signal. However, most of the device trees that
+		 * have been widely used for a long time incorrectly describe
+		 * reset GPIO as active-high, and were also using wrong name
+		 * for the property.
+		 */
+		{ "lantiq,pci-xway",	"gpio-reset",	false },
+#endif
 #if IS_ENABLED(CONFIG_TOUCHSCREEN_TSC2005)
 		/*
 		 * DTS for Nokia N900 incorrectly specified "active high"
@@ -512,9 +522,9 @@ static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
 		{ "reset",	"reset-n-io",	"marvell,nfc-uart" },
 		{ "reset",	"reset-n-io",	"mrvl,nfc-uart" },
 #endif
-#if !IS_ENABLED(CONFIG_PCI_LANTIQ)
+#if IS_ENABLED(CONFIG_PCI_LANTIQ)
 		/* MIPS Lantiq PCI */
-		{ "reset",	"gpios-reset",	"lantiq,pci-xway" },
+		{ "reset",	"gpio-reset",	"lantiq,pci-xway" },
 #endif
 
 		/*

-- 
Dmitry

Powered by blists - more mailing lists