Received-SPF: pass (google.com: domain of linux-kernel+bounces-212289-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 12 Jun 2024 14:45:37 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Martin Schiller <ms@dev.tdt.de>
Cc: hauke@hauke-m.de, tsbogend@alpha.franken.de, rdunlap@infradead.org,
	robh@kernel.org, bhelgaas@google.com, linux-mips@vger.kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] MIPS: pci: lantiq: restore reset gpio polarity
Message-ID: <ZmoXAR79emQBNyhj@google.com>
References: <20240607090400.1816612-1-ms@dev.tdt.de>
 <ZmnfQWFoIw5UCV-k@google.com>
 <7d34eb4017e809245daa342e3ccddf4f@dev.tdt.de>
 <b6bea9239050ed39ce3a051a5985b86d@dev.tdt.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b6bea9239050ed39ce3a051a5985b86d@dev.tdt.de>

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 },
+#endif
 #if IS_ENABLED(CONFIG_TOUCHSCREEN_TSC2005)
 		/*
 		 * DTS for Nokia N900 incorrectly specified "active high"
@@ -512,7 +522,7 @@ 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" },
 #endif

Thanks.

-- 
Dmitry