[PATCH] ARM: imx6: Fix PCIe reset gpio polarity on Toradex Apalis

[PATCH] ARM: imx6: Fix PCIe reset gpio polarity on Toradex Apalis

[Petr Štetiar]

In commit 5c5fb40de8f14 "PCI: imx6: Add support for active-low reset GPIO"
I've made GPIO reset polarity aware, which made the PCIe working on Apalis SoM
(reset active-high), but it has broken PCIe on several other i.MX6 boards
(reset active-low) due to wrong logic level. It has made the default reset
logic active-high, but it was active-low since beginning due to the bug in the
reset code:

	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
	msleep(100);
	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);

This breakage couldn't be fixed correctly by fixing the reset code snippet
mentioned above and fixing the broken DTBs, as we can't touch the DTBs, so the
5c5fb40de8f14 commit is going to be reverted, making PCIe reset broken on
Apalis modules again.

As suggested by Fabio, to fix this situation in backward compatible manner, we
should introduce new boolean DT property reset-gpio-active-high, which would
set PCIe reset GPIO polarity as needed. This patch series tries to do so.

[PATCH 1/2] ARM: dts: imx6: Fix PCIe reset gpio polarity on Toradex Apalis Ixora

[Petr Štetiar]

Adding reset-gpio-active-high boolean DT binding property, which we need
to make PCIe working on Apalis SoMs and not break old DTBs. While at it,
I've fixed comment and GPIO polarity.

On Apalis SoMs the GPIO1_IO28 used to PCIe reset is not connected
directly to PERST# PCIe signal, but it's ORed with RESETBMCU coming off
the PMIC, and thus is inverted, active-high.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 arch/arm/boot/dts/imx6q-apalis-ixora.dts | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-apalis-ixora.dts b/arch/arm/boot/dts/imx6q-apalis-ixora.dts
index 2cba82d..4b533cb 100644
--- a/arch/arm/boot/dts/imx6q-apalis-ixora.dts
+++ b/arch/arm/boot/dts/imx6q-apalis-ixora.dts
@@ -174,8 +174,9 @@
 };
 
 &pcie {
-	/* active-low meaning opposite of regular PERST# active-low polarity */
-	reset-gpio = <&gpio1 28 GPIO_ACTIVE_LOW>;
+	/* active-high meaning opposite of regular PERST# active-low polarity */
+	reset-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	reset-gpio-active-high;
 	status = "okay";
 };
 
-- 
1.9.1

[PATCH 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Petr Štetiar]

We need that property in order to make the Toradex Apalis SoMs working
without breaking old DTBs. On Apalis SoMs the GPIO1_IO28 used to PCIe
reset is not connected directly to PERST# PCIe signal, but it's ORed
with RESETBMCU coming off the PMIC, and thus is inverted, active-high.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 drivers/pci/host/pci-imx6.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 2f817fa..1b3bcfe 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -33,6 +33,7 @@
 
 struct imx6_pcie {
 	int			reset_gpio;
+	bool			gpio_active_high;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -310,9 +311,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 
 	/* 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);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					!!imx6_pcie->gpio_active_high);
 		msleep(100);
-		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					!imx6_pcie->gpio_active_high);
 	}
 	return 0;
 
@@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 
 	/* Fetch GPIOs */
 	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+	imx6_pcie->gpio_active_high = of_property_read_bool(np,
+						"reset-gpio-active-high");
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
 		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
-					    GPIOF_OUT_INIT_LOW, "PCIe reset");
+				imx6_pcie->gpio_active_high ?
+					GPIOF_OUT_INIT_HIGH :
+					GPIOF_OUT_INIT_LOW,
+				"PCIe reset");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get reset gpio\n");
 			return ret;
-- 
1.9.1

Re: [PATCH 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Fabio Estevam]

On Fri, Apr 1, 2016 at 9:41 AM, Petr Štetiar <ynezz@true.cz> wrote:

>         /* 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);
> +               gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +                                       !!imx6_pcie->gpio_active_high);

gpio_set_value_cansleep(imx6_pcie->reset_gpio,
                                      imx6_pcie->gpio_active_high); is enough.

> @@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>
>         /* Fetch GPIOs */
>         imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +       imx6_pcie->gpio_active_high = of_property_read_bool(np,
> +                                               "reset-gpio-active-high");

You need to document reset-gpio-active-high property in
Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt.

Re: [PATCH 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Petr Štetiar]

Fabio Estevam <festevam@gmail.com> [2016-04-05 12:46:53]:

> > @@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
> >
> >         /* Fetch GPIOs */
> >         imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> > +       imx6_pcie->gpio_active_high = of_property_read_bool(np,
> > +                                               "reset-gpio-active-high");
> 
> You need to document reset-gpio-active-high property in
> Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt.

Is something like this fine?

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 3be80c6..23ecb47 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -19,6 +19,11 @@ Optional properties:
 - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
 - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127
 - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127
+- reset-gpio: Should specify the GPIO for PHY reset. Its not polarity aware
+  and defaults to active-low reset sequence (L=reset state, H=operation state).
+- reset-gpio-active-high: If present then the reset sequence using the GPIO
+  specified in the "reset-gpio" property is reversed (H=reset state,
+  L=operation state).

-- ynezz

[PATCH v2 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Petr Štetiar]

We need that property in order to make the Toradex Apalis SoMs working
without breaking old DTBs. On Apalis SoMs the GPIO1_IO28 used to PCIe
reset is not connected directly to PERST# PCIe signal, but it's ORed
with RESETBMCU coming off the PMIC, and thus is inverted, active-high.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 Changes since v1:
  
  * Added documentation of reset-gpio and reset-gpio-active-high DT properties
  * Removed unnecessary double negation of GPIO value

 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt |  5 +++++
 drivers/pci/host/pci-imx6.c                              | 14 +++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 3be80c6..23ecb47 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -19,6 +19,11 @@ Optional properties:
 - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
 - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127
 - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127
+- reset-gpio: Should specify the GPIO for PHY reset. Its not polarity aware
+  and defaults to active-low reset sequence (L=reset state, H=operation state).
+- reset-gpio-active-high: If present then the reset sequence using the GPIO
+  specified in the "reset-gpio" property is reversed (H=reset state,
+  L=operation state).
 
 Example:
 
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 2f817fa..17f4cc3 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -33,6 +33,7 @@
 
 struct imx6_pcie {
 	int			reset_gpio;
+	bool			gpio_active_high;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -310,9 +311,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 
 	/* 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);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					imx6_pcie->gpio_active_high);
 		msleep(100);
-		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					!imx6_pcie->gpio_active_high);
 	}
 	return 0;
 
@@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 
 	/* Fetch GPIOs */
 	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+	imx6_pcie->gpio_active_high = of_property_read_bool(np,
+						"reset-gpio-active-high");
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
 		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
-					    GPIOF_OUT_INIT_LOW, "PCIe reset");
+				imx6_pcie->gpio_active_high ?
+					GPIOF_OUT_INIT_HIGH :
+					GPIOF_OUT_INIT_LOW,
+				"PCIe reset");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get reset gpio\n");
 			return ret;
-- 
1.9.1

Re: [PATCH v2 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Lucas Stach]

Am Mittwoch, den 06.04.2016, 11:34 +0200 schrieb Petr Štetiar:
> We need that property in order to make the Toradex Apalis SoMs working
> without breaking old DTBs. On Apalis SoMs the GPIO1_IO28 used to PCIe
> reset is not connected directly to PERST# PCIe signal, but it's ORed
> with RESETBMCU coming off the PMIC, and thus is inverted, active-high.
> 
I don't think the commit message should contain references to the
specific board, as the board implementation is unrelated to the PCIe
change.

Please just explain why the added property is necessary and why it is
done this way.

> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  Changes since v1:
>   
>   * Added documentation of reset-gpio and reset-gpio-active-high DT properties
>   * Removed unnecessary double negation of GPIO value
> 
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt |  5 +++++
>  drivers/pci/host/pci-imx6.c                              | 14 +++++++++++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 3be80c6..23ecb47 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -19,6 +19,11 @@ Optional properties:
>  - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
>  - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127
>  - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127
> +- reset-gpio: Should specify the GPIO for PHY reset. Its not polarity aware
> +  and defaults to active-low reset sequence (L=reset state, H=operation state).

This is not a PHY reset GPIO. It's a GPIO controlling the PCI bus device
reset signal.

> +- reset-gpio-active-high: If present then the reset sequence using the GPIO
> +  specified in the "reset-gpio" property is reversed (H=reset state,
> +  L=operation state).
>  
>  Example:
>  
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 2f817fa..17f4cc3 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -33,6 +33,7 @@
>  
>  struct imx6_pcie {
>  	int			reset_gpio;
> +	bool			gpio_active_high;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -310,9 +311,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  
>  	/* 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);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					imx6_pcie->gpio_active_high);
>  		msleep(100);
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					!imx6_pcie->gpio_active_high);
>  	}
>  	return 0;
>  
> @@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  
>  	/* Fetch GPIOs */
>  	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +	imx6_pcie->gpio_active_high = of_property_read_bool(np,
> +						"reset-gpio-active-high");
>  	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
>  		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> -					    GPIOF_OUT_INIT_LOW, "PCIe reset");
> +				imx6_pcie->gpio_active_high ?
> +					GPIOF_OUT_INIT_HIGH :
> +					GPIOF_OUT_INIT_LOW,
> +				"PCIe reset");
>  		if (ret) {
>  			dev_err(&pdev->dev, "unable to get reset gpio\n");
>  			return ret;

[PATCH v3 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Petr Štetiar]

Currently the reset-gpio DT property which controls the PCI bus device
reset signal defaults to active-low reset sequence (L=reset state,
H=operation state) plus the code in reset function isn't GPIO polarity
aware - it doesn't matter if the defined reset-gpio is active-low or
active-high, it will always result into active-low reset sequence.

I've tried to fix it properly and changed the reset-gpio reset sequence
to be polarity aware, but this patch has been accepted and then reverted
as it has introduced few backward incompatible issues:

1. Some of the DTBs as for example imx6qdl-sabresd, doesn't define
reset-gpio polarity correctly:

  reset-gpio = <&gpio7 12 0>;

which means, that it's defined as active-high, but in reality it's
active-low, thus it wouldn't work without DTS fix.

2. The logic in reset function is inverted:

	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0)
	msleep(100);
	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);

so even if some of the i.MX6 boards had reset-gpio polarity defined
correctly in their DTSes, they would stop working.

As we can't break old DTBs, we can't fix them and that's why we need to
introduce this new DT reset-gpio-active-high boolean property, so we can
support boards with active-high reset sequence.

This active-high reset sequence is for example needed on Apalis SoMs,
where GPIO1_IO28, used to PCIe reset is not connected directly to PERST#
PCIe signal, but it's ORed with RESETBMCU coming off the PMIC, and thus
is inverted, active-high.

Signed-off-by: Petr Štetiar <ynezz@true.cz>
---
 Changes since v1:

  * Added documentation of reset-gpio and reset-gpio-active-high DT properties
  * Removed unnecessary double negation of GPIO value

 Changes since v2:

  * Changed commit message so it explains in more detail why we need new DT
    property
  * Changed PHY to 'bus device' in binding's documentation

 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt |  6 ++++++
 drivers/pci/host/pci-imx6.c                              | 14 +++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 3be80c6..072efbf 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -19,6 +19,12 @@ Optional properties:
 - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
 - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127
 - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127
+- reset-gpio: Should specify the GPIO for controlling the PCI bus device reset
+  signal. Its not polarity aware and defaults to active-low reset sequence
+  (L=reset state, H=operation state).
+- reset-gpio-active-high: If present then the reset sequence using the GPIO
+  specified in the "reset-gpio" property is reversed (H=reset state,
+  L=operation state).
 
 Example:
 
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 2f817fa..17f4cc3 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -33,6 +33,7 @@
 
 struct imx6_pcie {
 	int			reset_gpio;
+	bool			gpio_active_high;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -310,9 +311,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 
 	/* 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);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					imx6_pcie->gpio_active_high);
 		msleep(100);
-		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
+					!imx6_pcie->gpio_active_high);
 	}
 	return 0;
 
@@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 
 	/* Fetch GPIOs */
 	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+	imx6_pcie->gpio_active_high = of_property_read_bool(np,
+						"reset-gpio-active-high");
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
 		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
-					    GPIOF_OUT_INIT_LOW, "PCIe reset");
+				imx6_pcie->gpio_active_high ?
+					GPIOF_OUT_INIT_HIGH :
+					GPIOF_OUT_INIT_LOW,
+				"PCIe reset");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get reset gpio\n");
 			return ret;
-- 
1.9.1

Re: [PATCH v3 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Lucas Stach]

Am Mittwoch, den 06.04.2016, 14:36 +0200 schrieb Petr Štetiar:
> Currently the reset-gpio DT property which controls the PCI bus device
> reset signal defaults to active-low reset sequence (L=reset state,
> H=operation state) plus the code in reset function isn't GPIO polarity
> aware - it doesn't matter if the defined reset-gpio is active-low or
> active-high, it will always result into active-low reset sequence.
> 
> I've tried to fix it properly and changed the reset-gpio reset sequence
> to be polarity aware, but this patch has been accepted and then reverted
> as it has introduced few backward incompatible issues:
> 
> 1. Some of the DTBs as for example imx6qdl-sabresd, doesn't define
> reset-gpio polarity correctly:
> 
>   reset-gpio = <&gpio7 12 0>;
> 
> which means, that it's defined as active-high, but in reality it's
> active-low, thus it wouldn't work without DTS fix.
> 
> 2. The logic in reset function is inverted:
> 
> 	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0)
> 	msleep(100);
> 	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> 
> so even if some of the i.MX6 boards had reset-gpio polarity defined
> correctly in their DTSes, they would stop working.
> 
> As we can't break old DTBs, we can't fix them and that's why we need to
> introduce this new DT reset-gpio-active-high boolean property, so we can
> support boards with active-high reset sequence.
> 
> This active-high reset sequence is for example needed on Apalis SoMs,
> where GPIO1_IO28, used to PCIe reset is not connected directly to PERST#
> PCIe signal, but it's ORed with RESETBMCU coming off the PMIC, and thus
> is inverted, active-high.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  Changes since v1:
> 
>   * Added documentation of reset-gpio and reset-gpio-active-high DT properties
>   * Removed unnecessary double negation of GPIO value
> 
>  Changes since v2:
> 
>   * Changed commit message so it explains in more detail why we need new DT
>     property
>   * Changed PHY to 'bus device' in binding's documentation
> 
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt |  6 ++++++
>  drivers/pci/host/pci-imx6.c                              | 14 +++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 3be80c6..072efbf 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -19,6 +19,12 @@ Optional properties:
>  - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
>  - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127
>  - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127
> +- reset-gpio: Should specify the GPIO for controlling the PCI bus device reset
> +  signal. Its not polarity aware and defaults to active-low reset sequence
> +  (L=reset state, H=operation state).
> +- reset-gpio-active-high: If present then the reset sequence using the GPIO
> +  specified in the "reset-gpio" property is reversed (H=reset state,
> +  L=operation state).
>  
>  Example:
>  
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 2f817fa..17f4cc3 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -33,6 +33,7 @@
>  
>  struct imx6_pcie {
>  	int			reset_gpio;
> +	bool			gpio_active_high;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -310,9 +311,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>  
>  	/* 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);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					imx6_pcie->gpio_active_high);
>  		msleep(100);
> -		gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +		gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +					!imx6_pcie->gpio_active_high);
>  	}
>  	return 0;
>  
> @@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  
>  	/* Fetch GPIOs */
>  	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +	imx6_pcie->gpio_active_high = of_property_read_bool(np,
> +						"reset-gpio-active-high");
>  	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
>  		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> -					    GPIOF_OUT_INIT_LOW, "PCIe reset");
> +				imx6_pcie->gpio_active_high ?
> +					GPIOF_OUT_INIT_HIGH :
> +					GPIOF_OUT_INIT_LOW,
> +				"PCIe reset");
>  		if (ret) {
>  			dev_err(&pdev->dev, "unable to get reset gpio\n");
>  			return ret;

Re: [PATCH v3 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Rob Herring]

On Wed, Apr 06, 2016 at 02:36:47PM +0200, Petr Štetiar wrote:
> Currently the reset-gpio DT property which controls the PCI bus device
> reset signal defaults to active-low reset sequence (L=reset state,
> H=operation state) plus the code in reset function isn't GPIO polarity
> aware - it doesn't matter if the defined reset-gpio is active-low or
> active-high, it will always result into active-low reset sequence.
> 
> I've tried to fix it properly and changed the reset-gpio reset sequence
> to be polarity aware, but this patch has been accepted and then reverted
> as it has introduced few backward incompatible issues:
> 
> 1. Some of the DTBs as for example imx6qdl-sabresd, doesn't define
> reset-gpio polarity correctly:
> 
>   reset-gpio = <&gpio7 12 0>;
> 
> which means, that it's defined as active-high, but in reality it's
> active-low, thus it wouldn't work without DTS fix.
> 
> 2. The logic in reset function is inverted:
> 
> 	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0)
> 	msleep(100);
> 	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> 
> so even if some of the i.MX6 boards had reset-gpio polarity defined
> correctly in their DTSes, they would stop working.
> 
> As we can't break old DTBs, we can't fix them and that's why we need to
> introduce this new DT reset-gpio-active-high boolean property, so we can
> support boards with active-high reset sequence.
> 
> This active-high reset sequence is for example needed on Apalis SoMs,
> where GPIO1_IO28, used to PCIe reset is not connected directly to PERST#
> PCIe signal, but it's ORed with RESETBMCU coming off the PMIC, and thus
> is inverted, active-high.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  Changes since v1:
> 
>   * Added documentation of reset-gpio and reset-gpio-active-high DT properties
>   * Removed unnecessary double negation of GPIO value
> 
>  Changes since v2:
> 
>   * Changed commit message so it explains in more detail why we need new DT
>     property
>   * Changed PHY to 'bus device' in binding's documentation
> 
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt |  6 ++++++
>  drivers/pci/host/pci-imx6.c                              | 14 +++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/2] PCI: imx6: Add reset-gpio-active-high boolean property to DT

[Tim Harvey]

On Wed, Apr 6, 2016 at 5:36 AM, Petr Štetiar <ynezz@true.cz> wrote:
> Currently the reset-gpio DT property which controls the PCI bus device
> reset signal defaults to active-low reset sequence (L=reset state,
> H=operation state) plus the code in reset function isn't GPIO polarity
> aware - it doesn't matter if the defined reset-gpio is active-low or
> active-high, it will always result into active-low reset sequence.
>
> I've tried to fix it properly and changed the reset-gpio reset sequence
> to be polarity aware, but this patch has been accepted and then reverted
> as it has introduced few backward incompatible issues:
>
> 1. Some of the DTBs as for example imx6qdl-sabresd, doesn't define
> reset-gpio polarity correctly:
>
>   reset-gpio = <&gpio7 12 0>;
>
> which means, that it's defined as active-high, but in reality it's
> active-low, thus it wouldn't work without DTS fix.
>
> 2. The logic in reset function is inverted:
>
>         gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0)
>         msleep(100);
>         gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
>
> so even if some of the i.MX6 boards had reset-gpio polarity defined
> correctly in their DTSes, they would stop working.
>
> As we can't break old DTBs, we can't fix them and that's why we need to
> introduce this new DT reset-gpio-active-high boolean property, so we can
> support boards with active-high reset sequence.
>
> This active-high reset sequence is for example needed on Apalis SoMs,
> where GPIO1_IO28, used to PCIe reset is not connected directly to PERST#
> PCIe signal, but it's ORed with RESETBMCU coming off the PMIC, and thus
> is inverted, active-high.
>
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  Changes since v1:
>
>   * Added documentation of reset-gpio and reset-gpio-active-high DT properties
>   * Removed unnecessary double negation of GPIO value
>
>  Changes since v2:
>
>   * Changed commit message so it explains in more detail why we need new DT
>     property
>   * Changed PHY to 'bus device' in binding's documentation
>
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt |  6 ++++++
>  drivers/pci/host/pci-imx6.c                              | 14 +++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 3be80c6..072efbf 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -19,6 +19,12 @@ Optional properties:
>  - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20
>  - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127
>  - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127
> +- reset-gpio: Should specify the GPIO for controlling the PCI bus device reset
> +  signal. Its not polarity aware and defaults to active-low reset sequence
> +  (L=reset state, H=operation state).
> +- reset-gpio-active-high: If present then the reset sequence using the GPIO
> +  specified in the "reset-gpio" property is reversed (H=reset state,
> +  L=operation state).
>
>  Example:
>
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 2f817fa..17f4cc3 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -33,6 +33,7 @@
>
>  struct imx6_pcie {
>         int                     reset_gpio;
> +       bool                    gpio_active_high;
>         struct clk              *pcie_bus;
>         struct clk              *pcie_phy;
>         struct clk              *pcie;
> @@ -310,9 +311,11 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>
>         /* 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);
> +               gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +                                       imx6_pcie->gpio_active_high);
>                 msleep(100);
> -               gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +               gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> +                                       !imx6_pcie->gpio_active_high);
>         }
>         return 0;
>
> @@ -546,9 +549,14 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>
>         /* Fetch GPIOs */
>         imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +       imx6_pcie->gpio_active_high = of_property_read_bool(np,
> +                                               "reset-gpio-active-high");
>         if (gpio_is_valid(imx6_pcie->reset_gpio)) {
>                 ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> -                                           GPIOF_OUT_INIT_LOW, "PCIe reset");
> +                               imx6_pcie->gpio_active_high ?
> +                                       GPIOF_OUT_INIT_HIGH :
> +                                       GPIOF_OUT_INIT_LOW,
> +                               "PCIe reset");
>                 if (ret) {
>                         dev_err(&pdev->dev, "unable to get reset gpio\n");
>                         return ret;
> --
> 1.9.1
>

Tested on Gateworks Ventana boards (which have active-low PERST#)

Tested-by: Tim Harvey <tharvey@gateworks.com>

Re: [PATCH] ARM: imx6: Fix PCIe reset gpio polarity on Toradex Apalis

[Bjorn Helgaas]

On Fri, Apr 01, 2016 at 02:41:46PM +0200, Petr Štetiar wrote:
> In commit 5c5fb40de8f14 "PCI: imx6: Add support for active-low reset GPIO"
> I've made GPIO reset polarity aware, which made the PCIe working on Apalis SoM
> (reset active-high), but it has broken PCIe on several other i.MX6 boards
> (reset active-low) due to wrong logic level. It has made the default reset
> logic active-high, but it was active-low since beginning due to the bug in the
> reset code:
> 
> 	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> 	msleep(100);
> 	gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> 
> This breakage couldn't be fixed correctly by fixing the reset code snippet
> mentioned above and fixing the broken DTBs, as we can't touch the DTBs, so the
> 5c5fb40de8f14 commit is going to be reverted, making PCIe reset broken on
> Apalis modules again.
> 
> As suggested by Fabio, to fix this situation in backward compatible manner, we
> should introduce new boolean DT property reset-gpio-active-high, which would
> set PCIe reset GPIO polarity as needed. This patch series tries to do so.

I applied both of these (the original Toradex Apalis Ixora DTS fix and
the v3 "Add reset-gpio-active-high" change) to pci/host-imx6 for v4.7,
with Lucas' Reviewed-by, Rob's Acked-by, and Tim's Tested-by.  Thanks,
Petr!