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
/*
