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
