| Message ID | 20240607090400.1816612-1-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 277a0363120276645ae598d8d5fea7265e076ae9 |
| Headers | show |
| Series | MIPS: pci: lantiq: restore reset gpio polarity | expand |
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. > > In order to remain compatible with all these existing device trees, we > should therefore keep the logic as it was before the commit. > > Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API") > Cc: stable@vger.kernel.org > Signed-off-by: Martin Schiller <ms@dev.tdt.de> > --- > arch/mips/pci/pci-lantiq.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) applied to mips-fixes Thomas.
Hi Thomas, Martin, On Tue, Jun 11, 2024 at 04:12:18PM +0200, Thomas Bogendoerfer wrote: > 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. > > > > In order to remain compatible with all these existing device trees, we > > should therefore keep the logic as it was before the commit. > > > > Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API") > > Cc: stable@vger.kernel.org > > Signed-off-by: Martin Schiller <ms@dev.tdt.de> > > --- > > arch/mips/pci/pci-lantiq.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > applied to mips-fixes The patch is unfortunately also wrong as it will break any board that actually has correct polarity annotation. I will prepare a quirk for drivers/gpio/gpiolib-of.c to force the polarity to low for this GPIO. Thanks.
Hi Marton, 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. > > 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). This still leaves the question about boards that require inversion. Are you saying that they have real signal inverter on the line or that their device trees correctly describe the signal as GPIO_ACTIVE_LOW? BTW, please consider getting DTS trees for your devices into mainline. Why do you keep them separate? > > Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API") > Cc: stable@vger.kernel.org > Signed-off-by: Martin Schiller <ms@dev.tdt.de> > --- > arch/mips/pci/pci-lantiq.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c > index 68a8cefed420..0844db34022e 100644 > --- a/arch/mips/pci/pci-lantiq.c > +++ b/arch/mips/pci/pci-lantiq.c > @@ -124,14 +124,14 @@ static int ltq_pci_startup(struct platform_device *pdev) > clk_disable(clk_external); > > /* setup reset gpio used by pci */ > - reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", > - GPIOD_OUT_LOW); > + reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_ASIS); > error = PTR_ERR_OR_ZERO(reset_gpio); > if (error) { > dev_err(&pdev->dev, "failed to request gpio: %d\n", error); > return error; > } > gpiod_set_consumer_name(reset_gpio, "pci_reset"); > + gpiod_direction_output(reset_gpio, 1); > > /* enable auto-switching between PCI and EBU */ > ltq_pci_w32(0xa, PCI_CR_CLK_CTRL); > @@ -194,10 +194,10 @@ static int ltq_pci_startup(struct platform_device *pdev) > > /* toggle reset pin */ > if (reset_gpio) { > - gpiod_set_value_cansleep(reset_gpio, 1); > + gpiod_set_value_cansleep(reset_gpio, 0); > wmb(); > mdelay(1); > - gpiod_set_value_cansleep(reset_gpio, 0); > + gpiod_set_value_cansleep(reset_gpio, 1); > } > return 0; > } > -- > 2.39.2 > Thanks.
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). > > This still leaves the question about boards that require inversion. Are > you saying that they have real signal inverter on the line or that > their > device trees correctly describe the signal as GPIO_ACTIVE_LOW? > > BTW, please consider getting DTS trees for your devices into mainline. > Why do you keep them separate? Unfortunately, these are not "my" devices and I can't even test them. I've got feedback from some users when I updated the lantiq target to linux 6.1 in openwrt. Let's assume that all boards physically expect an active-low signal. If the GPIO_ACTIVE_LOW flag were now set in the device tree, the original (old) driver would have an incorrect initial level (LOW instead of HIGH) due to the gpio_direction_output(reset_gpio, 1); This is probably the reason why the flag GPIO_ACTIVE_HIGH is set in almost all dts files in openwrt. But with commit 90c2d2eb7ab5 the initial level (LOW) is guaranteed to be wrong because of the "GPIOD_OUT_LOW" and cannot be changed by "wrong" device tree settings. The signal curve is LOW -> LOW -> HIGH instead of HIGH -> LOW -> HIGH. > >> >> Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API") >> Cc: stable@vger.kernel.org >> Signed-off-by: Martin Schiller <ms@dev.tdt.de> >> --- >> arch/mips/pci/pci-lantiq.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c >> index 68a8cefed420..0844db34022e 100644 >> --- a/arch/mips/pci/pci-lantiq.c >> +++ b/arch/mips/pci/pci-lantiq.c >> @@ -124,14 +124,14 @@ static int ltq_pci_startup(struct >> platform_device *pdev) >> clk_disable(clk_external); >> >> /* setup reset gpio used by pci */ >> - reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", >> - GPIOD_OUT_LOW); >> + reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", >> GPIOD_ASIS); >> error = PTR_ERR_OR_ZERO(reset_gpio); >> if (error) { >> dev_err(&pdev->dev, "failed to request gpio: %d\n", error); >> return error; >> } >> gpiod_set_consumer_name(reset_gpio, "pci_reset"); >> + gpiod_direction_output(reset_gpio, 1); >> >> /* enable auto-switching between PCI and EBU */ >> ltq_pci_w32(0xa, PCI_CR_CLK_CTRL); >> @@ -194,10 +194,10 @@ static int ltq_pci_startup(struct >> platform_device *pdev) >> >> /* toggle reset pin */ >> if (reset_gpio) { >> - gpiod_set_value_cansleep(reset_gpio, 1); >> + gpiod_set_value_cansleep(reset_gpio, 0); >> wmb(); >> mdelay(1); >> - gpiod_set_value_cansleep(reset_gpio, 0); >> + gpiod_set_value_cansleep(reset_gpio, 1); >> } >> return 0; >> } >> -- >> 2.39.2 >> > > Thanks. [1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=tree;f=target/linux/lantiq/files/arch/mips/boot/dts/lantiq
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. For the lantiq targets in openWrt, the devicetree blob is appended to the kernel image and therefore also updated when doing a firmware upgrade. So, maybe it would also be an option to fix the driver (using GPIO_ACTIVE_* flag for the initial level and set it to 0 -> 1 -> 0) and rework all the dts files to use GPIO_ACTIVE_LOW. Then we won't need any quirks. >> >> This still leaves the question about boards that require inversion. >> Are >> you saying that they have real signal inverter on the line or that >> their >> device trees correctly describe the signal as GPIO_ACTIVE_LOW? >> >> BTW, please consider getting DTS trees for your devices into mainline. >> Why do you keep them separate? > > Unfortunately, these are not "my" devices and I can't even test them. > I've got feedback from some users when I updated the lantiq target to > linux 6.1 in openwrt. > > > Let's assume that all boards physically expect an active-low signal. > > If the GPIO_ACTIVE_LOW flag were now set in the device tree, the > original (old) driver would have an incorrect initial level (LOW > instead > of HIGH) due to the > > gpio_direction_output(reset_gpio, 1); > > This is probably the reason why the flag GPIO_ACTIVE_HIGH is set in > almost all dts files in openwrt. > > But with commit 90c2d2eb7ab5 the initial level (LOW) is guaranteed to > be > wrong because of the "GPIOD_OUT_LOW" and cannot be changed by "wrong" > device tree settings. > > The signal curve is LOW -> LOW -> HIGH instead of HIGH -> LOW -> HIGH. > >> >>> >>> Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Martin Schiller <ms@dev.tdt.de> >>> --- >>> arch/mips/pci/pci-lantiq.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c >>> index 68a8cefed420..0844db34022e 100644 >>> --- a/arch/mips/pci/pci-lantiq.c >>> +++ b/arch/mips/pci/pci-lantiq.c >>> @@ -124,14 +124,14 @@ static int ltq_pci_startup(struct >>> platform_device *pdev) >>> clk_disable(clk_external); >>> >>> /* setup reset gpio used by pci */ >>> - reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", >>> - GPIOD_OUT_LOW); >>> + reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", >>> GPIOD_ASIS); >>> error = PTR_ERR_OR_ZERO(reset_gpio); >>> if (error) { >>> dev_err(&pdev->dev, "failed to request gpio: %d\n", error); >>> return error; >>> } >>> gpiod_set_consumer_name(reset_gpio, "pci_reset"); >>> + gpiod_direction_output(reset_gpio, 1); >>> >>> /* enable auto-switching between PCI and EBU */ >>> ltq_pci_w32(0xa, PCI_CR_CLK_CTRL); >>> @@ -194,10 +194,10 @@ static int ltq_pci_startup(struct >>> platform_device *pdev) >>> >>> /* toggle reset pin */ >>> if (reset_gpio) { >>> - gpiod_set_value_cansleep(reset_gpio, 1); >>> + gpiod_set_value_cansleep(reset_gpio, 0); >>> wmb(); >>> mdelay(1); >>> - gpiod_set_value_cansleep(reset_gpio, 0); >>> + gpiod_set_value_cansleep(reset_gpio, 1); >>> } >>> return 0; >>> } >>> -- >>> 2.39.2 >>> >> >> Thanks. > > [1] > https://git.openwrt.org/?p=openwrt/openwrt.git;a=tree;f=target/linux/lantiq/files/arch/mips/boot/dts/lantiq
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.
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 /*
On 2024-06-13 01:32, Dmitry Torokhov wrote: > 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 > > /* I wonder, when this renaming did not work so far, why did we not see the error message "failed to request gpio" in the log? @Aleksander Jan Bajkowski: You had problems with the PCI connection when you switched to linux 6.1 and also have corresponding devices to test. Could you please remove my patch in the latest openWrt and try out Dmitry's change?
On June 12, 2024 11:01:42 PM PDT, Martin Schiller <ms@dev.tdt.de> wrote: >On 2024-06-13 01:32, Dmitry Torokhov wrote: >> 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 >> >> /* > >I wonder, when this renaming did not work so far, why did we not see the >error message "failed to request gpio" in the log? The GPIO is requested as optional, so if it's not found there's no error. Interestingly the only user in the mainline kernel uses "gpios-reset" in is device tree which never worked.
On Wed, Jun 12, 2024 at 08:39:59PM +0200, Martin Schiller wrote: > @Thomas Bogendoerfer: > Would it be possible to stop the merging of this patch? > I think We have to do do some further/other changes. I'll revert the patch in mips-fixes. Thomas.
On 6/12/24 21:47, 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. > > For the lantiq targets in openWrt, the devicetree blob is appended to > the kernel image and therefore also updated when doing a firmware > upgrade. So, maybe it would also be an option to fix the driver (using > GPIO_ACTIVE_* flag for the initial level and set it to 0 -> 1 -> 0) and > rework all the dts files to use GPIO_ACTIVE_LOW. > > Then we won't need any quirks. I am not aware that anyone is using a recent kernel on the VRX200 outside of OpenWrt. I am also not aware that anyone is *not* appending the DTB to the kernel. The SoC is pretty old now, the successor of this SoC was released about 10 years ago. For me it would be fine if you fix the broken device device trees shipped with the upstream kernel and with OpenWrt to make them work with the PCI driver instead of investing too much time into handling old DTBs. The PCI reset is inverted on some boards to handle a dying gasp. If the power breaks down the reset should get triggered and the PCIe device can send a dying gasp signal to the other side. This is done on the reference designs of some Lantiq PCIe DSL card for the VRX318 and probably also some other components. Hauke
On 2024-06-13 22:06, Hauke Mehrtens wrote: > On 6/12/24 21:47, 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. >> >> For the lantiq targets in openWrt, the devicetree blob is appended to >> the kernel image and therefore also updated when doing a firmware >> upgrade. So, maybe it would also be an option to fix the driver (using >> GPIO_ACTIVE_* flag for the initial level and set it to 0 -> 1 -> 0) >> and >> rework all the dts files to use GPIO_ACTIVE_LOW. >> >> Then we won't need any quirks. > > I am not aware that anyone is using a recent kernel on the VRX200 > outside of OpenWrt. I am also not aware that anyone is *not* appending > the DTB to the kernel. The SoC is pretty old now, the successor of > this SoC was released about 10 years ago. > We're not just talking about VRX200 (VR9) here, but even older devices such as AR9 and Danube. > For me it would be fine if you fix the broken device device trees > shipped with the upstream kernel and with OpenWrt to make them work > with the PCI driver instead of investing too much time into handling > old DTBs. > > The PCI reset is inverted on some boards to handle a dying gasp. If > the power breaks down the reset should get triggered and the PCIe > device can send a dying gasp signal to the other side. This is done on > the reference designs of some Lantiq PCIe DSL card for the VRX318 and > probably also some other components. > > Hauke What I missed so far is the fact that the driver used '__gpio_set_value' before Dmitry's commit and thus used raw access to the GPIO. This effectively means that every device that has worked with the driver so far must have an ACTIVE_LOW reset, no matter what was configured in the device tree. So renaming the property in the dts from "gpio-reset" to "reset-gpios" and setting the FLAGS to "GPIO_ACTIVE_LOW" should actually solve the problem. What still bothers me about the driver itself are 2 things: 1. the initial value of GPIOD_OUT_LOW. This means that there is no real defined HIGH -> LOW -> HIGH on reset. 2. if we change 1., then I think "mdelay(1)" is too short and we would have to change it to at least "mdelay(100)". Martin
On Fri, Jun 14, 2024 at 10:43:29AM +0200, Martin Schiller wrote: > On 2024-06-13 22:06, Hauke Mehrtens wrote: > > On 6/12/24 21:47, 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. > > > > > > For the lantiq targets in openWrt, the devicetree blob is appended to > > > the kernel image and therefore also updated when doing a firmware > > > upgrade. So, maybe it would also be an option to fix the driver (using > > > GPIO_ACTIVE_* flag for the initial level and set it to 0 -> 1 -> 0) > > > and > > > rework all the dts files to use GPIO_ACTIVE_LOW. Yes, cleaning up DTS files when it is possible is nice. > > > > > > Then we won't need any quirks. Quirks are fairly cheap and we are not in a hot path here. > > > > I am not aware that anyone is using a recent kernel on the VRX200 > > outside of OpenWrt. I am also not aware that anyone is *not* appending > > the DTB to the kernel. The SoC is pretty old now, the successor of > > this SoC was released about 10 years ago. > > > > We're not just talking about VRX200 (VR9) here, but even older devices > such as AR9 and Danube. > > > For me it would be fine if you fix the broken device device trees > > shipped with the upstream kernel and with OpenWrt to make them work > > with the PCI driver instead of investing too much time into handling > > old DTBs. > > > > The PCI reset is inverted on some boards to handle a dying gasp. If > > the power breaks down the reset should get triggered and the PCIe > > device can send a dying gasp signal to the other side. This is done on > > the reference designs of some Lantiq PCIe DSL card for the VRX318 and > > probably also some other components. > > > > Hauke > > What I missed so far is the fact that the driver used '__gpio_set_value' > before Dmitry's commit and thus used raw access to the GPIO. > > This effectively means that every device that has worked with the driver > so far must have an ACTIVE_LOW reset, no matter what was configured in > the device tree. > > > So renaming the property in the dts from "gpio-reset" to "reset-gpios" > and setting the FLAGS to "GPIO_ACTIVE_LOW" should actually solve the > problem. Right, luckily (to a definition of luckily) the driver and DTB used "wrong" syntax for the gpio property, so we can quirk it and make force ACTIVE_LOW polarity on old DTBs, and new DTBs with "reset-gpios" property will follow polarity specified in DTB. > > What still bothers me about the driver itself are 2 things: > 1. the initial value of GPIOD_OUT_LOW. This means that there is no real > defined HIGH -> LOW -> HIGH on reset. Is this actually needed? Typically a card requires certain time in reset state (with reset line active) before it can be released, however there usually no restrictions on line being inactive beforehand. But typically it will be pulled up to avoid leakage... Thanks.
On 2024-06-20 02:54, Dmitry Torokhov wrote: > On Fri, Jun 14, 2024 at 10:43:29AM +0200, Martin Schiller wrote: >> On 2024-06-13 22:06, Hauke Mehrtens wrote: >> > On 6/12/24 21:47, 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. >> > > >> > > For the lantiq targets in openWrt, the devicetree blob is appended to >> > > the kernel image and therefore also updated when doing a firmware >> > > upgrade. So, maybe it would also be an option to fix the driver (using >> > > GPIO_ACTIVE_* flag for the initial level and set it to 0 -> 1 -> 0) >> > > and >> > > rework all the dts files to use GPIO_ACTIVE_LOW. > > Yes, cleaning up DTS files when it is possible is nice. > >> > > >> > > Then we won't need any quirks. > > Quirks are fairly cheap and we are not in a hot path here. > >> > >> > I am not aware that anyone is using a recent kernel on the VRX200 >> > outside of OpenWrt. I am also not aware that anyone is *not* appending >> > the DTB to the kernel. The SoC is pretty old now, the successor of >> > this SoC was released about 10 years ago. >> > >> >> We're not just talking about VRX200 (VR9) here, but even older devices >> such as AR9 and Danube. >> >> > For me it would be fine if you fix the broken device device trees >> > shipped with the upstream kernel and with OpenWrt to make them work >> > with the PCI driver instead of investing too much time into handling >> > old DTBs. >> > >> > The PCI reset is inverted on some boards to handle a dying gasp. If >> > the power breaks down the reset should get triggered and the PCIe >> > device can send a dying gasp signal to the other side. This is done on >> > the reference designs of some Lantiq PCIe DSL card for the VRX318 and >> > probably also some other components. >> > >> > Hauke >> >> What I missed so far is the fact that the driver used >> '__gpio_set_value' >> before Dmitry's commit and thus used raw access to the GPIO. >> >> This effectively means that every device that has worked with the >> driver >> so far must have an ACTIVE_LOW reset, no matter what was configured in >> the device tree. >> >> >> So renaming the property in the dts from "gpio-reset" to "reset-gpios" >> and setting the FLAGS to "GPIO_ACTIVE_LOW" should actually solve the >> problem. > > Right, luckily (to a definition of luckily) the driver and DTB used > "wrong" syntax for the gpio property, so we can quirk it and make > force ACTIVE_LOW polarity on old DTBs, and new DTBs with "reset-gpios" > property will follow polarity specified in DTB. We have already adapted the device trees in openWrt. [1] Will you create a quirk patch for the old DTBs or should I create one? > >> >> What still bothers me about the driver itself are 2 things: >> 1. the initial value of GPIOD_OUT_LOW. This means that there is no >> real >> defined HIGH -> LOW -> HIGH on reset. > > Is this actually needed? Typically a card requires certain time in > reset > state (with reset line active) before it can be released, however there > usually no restrictions on line being inactive beforehand. But > typically > it will be pulled up to avoid leakage... > No, I don't think that's absolutely necessary. Several tests have so far shown that it works as it is at the moment. [1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=f6fe19ed6dfaf0444cd80f530bf89f6878fd5936
diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c index 68a8cefed420..0844db34022e 100644 --- a/arch/mips/pci/pci-lantiq.c +++ b/arch/mips/pci/pci-lantiq.c @@ -124,14 +124,14 @@ static int ltq_pci_startup(struct platform_device *pdev) clk_disable(clk_external); /* setup reset gpio used by pci */ - reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", - GPIOD_OUT_LOW); + reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_ASIS); error = PTR_ERR_OR_ZERO(reset_gpio); if (error) { dev_err(&pdev->dev, "failed to request gpio: %d\n", error); return error; } gpiod_set_consumer_name(reset_gpio, "pci_reset"); + gpiod_direction_output(reset_gpio, 1); /* enable auto-switching between PCI and EBU */ ltq_pci_w32(0xa, PCI_CR_CLK_CTRL); @@ -194,10 +194,10 @@ static int ltq_pci_startup(struct platform_device *pdev) /* toggle reset pin */ if (reset_gpio) { - gpiod_set_value_cansleep(reset_gpio, 1); + gpiod_set_value_cansleep(reset_gpio, 0); wmb(); mdelay(1); - gpiod_set_value_cansleep(reset_gpio, 0); + gpiod_set_value_cansleep(reset_gpio, 1); } return 0; }
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. In order to remain compatible with all these existing device trees, we should therefore keep the logic as it was before the commit. Fixes: 90c2d2eb7ab5 ("MIPS: pci: lantiq: switch to using gpiod API") Cc: stable@vger.kernel.org Signed-off-by: Martin Schiller <ms@dev.tdt.de> --- arch/mips/pci/pci-lantiq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)