All of lore.kernel.org
 help / color / mirror / Atom feed
From: khalasa@piap.pl (Krzysztof Hałasa)
To: "Petr Štetiar" <ynezz@true.cz>,
	"Richard Zhu" <Richard.Zhu@freescale.com>,
	"Lucas Stach" <l.stach@pengutronix.de>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Date: Fri, 25 Mar 2016 14:32:35 +0100	[thread overview]
Message-ID: <m3h9fuk7ik.fsf@t19.piap.pl> (raw)

A recent commit 5c5fb40de8f14391a1238db05cef88754faf9229 stated:
Follows: linus/v4.4-rc2
Precedes: linus/v4.5-rc1

    PCI: imx6: Add support for active-low reset GPIO

    We previously used of_get_named_gpio(), which ignores the OF flags cell, so
    the reset GPIO defaulted to "active high." This doesn't work on the Toradex
    Apalis SoM with Ixora base board, which has an active-low reset GPIO.

    Use devm_gpiod_get_optional() instead so we pay attention to the active
    high/low flag.  This also adds support for GPIOs described via ACPI.

The (now replaced) code doesn't support the above:
@@ -287,10 +287,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
     usleep_range(200, 500);
 
     /* Some boards don't have PCIe reset GPIO. */
-    if (gpio_is_valid(imx6_pcie->reset_gpio)) {
-        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
+    if (imx6_pcie->reset_gpio) {
+        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
         msleep(100);
-        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
     }
     return 0;

If the reset_gpio setup code had ignored the flags (haven't checked
that), then clearly the resets were active-low (most reset lines are,
because they can be then driven with open-drain/collector output).
The gpiod_set_value*(0) activates reset, gpiod_set_value(1) -
deactivates.

Now we're told the setup code is now level-aware, but the above sequence
thus _deactivates_ reset for 100 ms, then _activates_ it again. It has
no chance to work, unless a board has a broken DTS file. A quick grep
shows that about half the IMX6 boards specify an active-low PCIe reset,
4 ask for active-high, and another 4 don't bother.


I wonder if all boards (except maybe that Toradex set) use an active-low
PCIe reset and are now broken. Perhaps Toradex uses active-high and thus
works.

I'm not fixing individual DTS files because I don't really know, though
perhaps we should change them all to "active-low", since it would work
the same as before the 5c5fb40de8f14391a1238db05cef88754faf9229 change.

Confirmed to fix Gateworks Laguna GW54xx.
Without the patch, the following happens (as expected):

PCI host bridge /soc/pcie@0x01000000 ranges:
  No bus range found for /soc/pcie@0x01000000, using [bus 00-ff]
   IO 0x01f80000..0x01f8ffff -> 0x00000000
  MEM 0x01000000..0x01efffff -> 0x01000000
imx6q-pcie 1ffc000.pcie: phy link never came up

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fe60096..f17fb02 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -288,9 +288,9 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 
 	/* Some boards don't have PCIe reset GPIO. */
 	if (imx6_pcie->reset_gpio) {
-		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
-		msleep(100);
 		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		msleep(100);
+		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
 	}
 	return 0;
 

WARNING: multiple messages have this Message-ID (diff)
From: khalasa@piap.pl (Krzysztof Hałasa)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity
Date: Fri, 25 Mar 2016 14:32:35 +0100	[thread overview]
Message-ID: <m3h9fuk7ik.fsf@t19.piap.pl> (raw)

A recent commit 5c5fb40de8f14391a1238db05cef88754faf9229 stated:
Follows: linus/v4.4-rc2
Precedes: linus/v4.5-rc1

    PCI: imx6: Add support for active-low reset GPIO

    We previously used of_get_named_gpio(), which ignores the OF flags cell, so
    the reset GPIO defaulted to "active high." This doesn't work on the Toradex
    Apalis SoM with Ixora base board, which has an active-low reset GPIO.

    Use devm_gpiod_get_optional() instead so we pay attention to the active
    high/low flag.  This also adds support for GPIOs described via ACPI.

The (now replaced) code doesn't support the above:
@@ -287,10 +287,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
     usleep_range(200, 500);
 
     /* Some boards don't have PCIe reset GPIO. */
-    if (gpio_is_valid(imx6_pcie->reset_gpio)) {
-        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
+    if (imx6_pcie->reset_gpio) {
+        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
         msleep(100);
-        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
     }
     return 0;

If the reset_gpio setup code had ignored the flags (haven't checked
that), then clearly the resets were active-low (most reset lines are,
because they can be then driven with open-drain/collector output).
The gpiod_set_value*(0) activates reset, gpiod_set_value(1) -
deactivates.

Now we're told the setup code is now level-aware, but the above sequence
thus _deactivates_ reset for 100 ms, then _activates_ it again. It has
no chance to work, unless a board has a broken DTS file. A quick grep
shows that about half the IMX6 boards specify an active-low PCIe reset,
4 ask for active-high, and another 4 don't bother.


I wonder if all boards (except maybe that Toradex set) use an active-low
PCIe reset and are now broken. Perhaps Toradex uses active-high and thus
works.

I'm not fixing individual DTS files because I don't really know, though
perhaps we should change them all to "active-low", since it would work
the same as before the 5c5fb40de8f14391a1238db05cef88754faf9229 change.

Confirmed to fix Gateworks Laguna GW54xx.
Without the patch, the following happens (as expected):

PCI host bridge /soc/pcie at 0x01000000 ranges:
  No bus range found for /soc/pcie at 0x01000000, using [bus 00-ff]
   IO 0x01f80000..0x01f8ffff -> 0x00000000
  MEM 0x01000000..0x01efffff -> 0x01000000
imx6q-pcie 1ffc000.pcie: phy link never came up

Signed-off-by: Krzysztof Ha?asa <khalasa@piap.pl>

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fe60096..f17fb02 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -288,9 +288,9 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 
 	/* Some boards don't have PCIe reset GPIO. */
 	if (imx6_pcie->reset_gpio) {
-		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
-		msleep(100);
 		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		msleep(100);
+		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
 	}
 	return 0;
 

             reply	other threads:[~2016-03-25 13:32 UTC|newest]

Thread overview: 85+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-25 13:32 Krzysztof Hałasa [this message]
2016-03-25 13:32 ` [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Krzysztof Hałasa
2016-03-27 14:44 ` Fabio Estevam
2016-03-27 14:44   ` Fabio Estevam
2016-03-28  0:26   ` Fabio Estevam
2016-03-28  0:26     ` Fabio Estevam
2016-03-28 19:59     ` Tim Harvey
2016-03-28 19:59       ` Tim Harvey
2016-03-28 20:13       ` Fabio Estevam
2016-03-28 20:13         ` Fabio Estevam
2016-03-28 20:42         ` Tim Harvey
2016-03-28 20:42           ` Tim Harvey
2016-03-28 21:30           ` Fabio Estevam
2016-03-28 21:30             ` Fabio Estevam
2016-03-28 22:06             ` Tim Harvey
2016-03-28 22:06               ` Tim Harvey
2016-03-28 22:13               ` Fabio Estevam
2016-03-28 22:13                 ` Fabio Estevam
2016-03-29  5:40                 ` Krzysztof Hałasa
2016-03-29  5:40                   ` Krzysztof Hałasa
2016-03-29  5:43                   ` Krzysztof Hałasa
2016-03-29  5:43                     ` Krzysztof Hałasa
2016-03-29  5:29               ` Krzysztof Hałasa
2016-03-29  5:29                 ` Krzysztof Hałasa
2016-03-29  8:55               ` Lucas Stach
2016-03-29  8:55                 ` Lucas Stach
2016-03-29 10:39                 ` Krzysztof Hałasa
2016-03-29 10:39                   ` Krzysztof Hałasa
2016-03-29 10:55                   ` Lucas Stach
2016-03-29 10:55                     ` Lucas Stach
2016-03-29 13:12                     ` Arnd Bergmann
2016-03-29 13:12                       ` Arnd Bergmann
2016-03-29 13:32                     ` Tim Harvey
2016-03-29 13:32                       ` Tim Harvey
2016-03-29 13:52                       ` Arnd Bergmann
2016-03-29 13:52                         ` Arnd Bergmann
2016-03-29 14:29                         ` Tim Harvey
2016-03-29 14:29                           ` Tim Harvey
2016-03-29 14:50                           ` Arnd Bergmann
2016-03-29 14:50                             ` Arnd Bergmann
2016-03-29 15:10                             ` Tim Harvey
2016-03-29 15:10                               ` Tim Harvey
2016-03-29 15:24                               ` Arnd Bergmann
2016-03-29 15:24                                 ` Arnd Bergmann
2016-03-29 17:38                                 ` Tim Harvey
2016-03-29 17:38                                   ` Tim Harvey
2016-03-29 19:39                                   ` Arnd Bergmann
2016-03-29 19:39                                     ` Arnd Bergmann
2016-03-29 17:56                                 ` Marc Zyngier
2016-03-29 17:56                                   ` Marc Zyngier
2016-03-29 16:13                               ` Roberto Fichera
2016-03-29 16:13                                 ` Roberto Fichera
2016-03-29 16:40                                 ` Tim Harvey
2016-03-29 16:40                                   ` Tim Harvey
2016-03-29 16:44                                   ` Roberto Fichera
2016-03-29 16:44                                     ` Roberto Fichera
2016-03-29 17:31                                     ` Tim Harvey
2016-03-29 17:31                                       ` Tim Harvey
2016-03-30  8:00                                       ` Roberto Fichera
2016-03-30  8:00                                         ` Roberto Fichera
2016-03-30 10:10                                         ` Arnd Bergmann
2016-03-30 10:10                                           ` Arnd Bergmann
2016-03-30 12:50                                           ` Roberto Fichera
2016-03-30 12:50                                             ` Roberto Fichera
2016-03-30 13:38                                             ` Tim Harvey
2016-03-30 13:38                                               ` Tim Harvey
2016-03-30 15:20                                               ` Roberto Fichera
2016-03-30 15:20                                                 ` Roberto Fichera
2016-03-30  8:10                     ` Krzysztof Hałasa
2016-03-30  8:10                       ` Krzysztof Hałasa
2016-03-31 16:19                       ` Tim Harvey
2016-03-31 16:19                         ` Tim Harvey
2016-04-04 10:37                         ` Krzysztof Hałasa
2016-04-04 10:37                           ` Krzysztof Hałasa
2016-03-29 14:14                 ` Fabio Estevam
2016-03-29 14:14                   ` Fabio Estevam
2016-03-29  5:21           ` Krzysztof Hałasa
2016-03-29  5:21             ` Krzysztof Hałasa
2016-03-30 12:06 ` Petr Štetiar
2016-03-30 12:06   ` Petr Štetiar
2016-03-30 12:45   ` Fabio Estevam
2016-03-30 12:45     ` Fabio Estevam
2016-03-30 14:38   ` Marcel Ziswiler
2016-03-30 14:38     ` Marcel Ziswiler
2016-03-30 14:38     ` Marcel Ziswiler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m3h9fuk7ik.fsf@t19.piap.pl \
    --to=khalasa@piap.pl \
    --cc=Richard.Zhu@freescale.com \
    --cc=bhelgaas@google.com \
    --cc=l.stach@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=ynezz@true.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.