[PATCH] PCI: imx6: Fix reset-gpio to work with active low GPIO

[PATCH] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Petr Štetiar]

It's currently not possible to use reset-gpio on board where the GPIO is
active low as the code incorrectly assumes, that reset-gpio will be
always active high.

In my case the broken behavior was observed on Toradex Apalis SoM with
Ixora base board.

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

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fdb9536..cd7ed71 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -293,9 +293,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(imx6_pcie->reset_gpio, 0);
+		struct gpio_desc *gd = gpio_to_desc(imx6_pcie->reset_gpio);
+
+		gpiod_set_value(gd, 0);
 		msleep(100);
-		gpio_set_value(imx6_pcie->reset_gpio, 1);
+		gpiod_set_value(gd, 1);
 	}
 	return 0;
 
@@ -559,6 +561,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 	struct pcie_port *pp;
 	struct device_node *np = pdev->dev.of_node;
 	struct resource *dbi_base;
+	enum of_gpio_flags of_flags;
+	int flags = GPIOF_OUT_INIT_LOW;
 	int ret;
 
 	imx6_pcie = devm_kzalloc(&pdev->dev, sizeof(*imx6_pcie), GFP_KERNEL);
@@ -578,10 +582,13 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pp->dbi_base);
 
 	/* Fetch GPIOs */
-	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
+	imx6_pcie->reset_gpio = of_get_named_gpio_flags(np, "reset-gpio", 0,
+							&of_flags);
 	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
+		if (of_flags & OF_GPIO_ACTIVE_LOW)
+			flags |= GPIOF_ACTIVE_LOW;
 		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
-					    GPIOF_OUT_INIT_LOW, "PCIe reset");
+					    flags, "PCIe reset");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get reset gpio\n");
 			return ret;
-- 
1.7.9.5

Re: [PATCH] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Lucas Stach]

Hi Petr,

sorry for the delay in reviewing this.

Am Montag, den 16.11.2015, 16:19 +0100 schrieb Petr Štetiar:
> It's currently not possible to use reset-gpio on board where the GPIO is
> active low as the code incorrectly assumes, that reset-gpio will be
> always active high.
> 
I see the need for this change, but I'm not completely happy with the
implementation.

> In my case the broken behavior was observed on Toradex Apalis SoM with
> Ixora base board.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>  drivers/pci/host/pci-imx6.c |   15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index fdb9536..cd7ed71 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -293,9 +293,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(imx6_pcie->reset_gpio, 0);
> +		struct gpio_desc *gd = gpio_to_desc(imx6_pcie->reset_gpio);
> +
> +		gpiod_set_value(gd, 0);
>  		msleep(100);
> -		gpio_set_value(imx6_pcie->reset_gpio, 1);
> +		gpiod_set_value(gd, 1);
>  	}
>  	return 0;
>  
> @@ -559,6 +561,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  	struct pcie_port *pp;
>  	struct device_node *np = pdev->dev.of_node;
>  	struct resource *dbi_base;
> +	enum of_gpio_flags of_flags;
> +	int flags = GPIOF_OUT_INIT_LOW;
>  	int ret;
>  
>  	imx6_pcie = devm_kzalloc(&pdev->dev, sizeof(*imx6_pcie), GFP_KERNEL);
> @@ -578,10 +582,13 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> +	imx6_pcie->reset_gpio = of_get_named_gpio_flags(np, "reset-gpio", 0,
> +	

Rather than adding all this complexity to this function, this should
really be switched over to the descriptor based GPIO interface as a
whole. So the above should become a single call to
"devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW)" and the dependent
code changed accordingly.

Regards,
Lucas

> 						&of_flags);
>  	if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> +		if (of_flags & OF_GPIO_ACTIVE_LOW)
> +			flags |= GPIOF_ACTIVE_LOW;
>  		ret = devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> -					    GPIOF_OUT_INIT_LOW, "PCIe reset");
> +					    flags, "PCIe reset");
>  		if (ret) {
>  			dev_err(&pdev->dev, "unable to get reset gpio\n");
>  			return ret;

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH v2] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Petr Štetiar]

It's currently not possible to use reset-gpio on board where the GPIO is
active low as the code incorrectly assumes, that reset-gpio will be
always active high.

In my case the broken behavior was observed on Toradex Apalis SoM with
Ixora base board.

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

Changes since v1:
 * reworked to use descriptor based GPIO interface

 drivers/pci/host/pci-imx6.c |   20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 22e8224..c0e121d 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -32,7 +32,7 @@
 #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
 
 struct imx6_pcie {
-	int			reset_gpio;
+	struct gpio_desc	*reset_gpio;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -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(imx6_pcie->reset_gpio, 0);
+	if (imx6_pcie->reset_gpio) {
+		gpiod_set_value(imx6_pcie->reset_gpio, 0);
 		msleep(100);
-		gpio_set_value(imx6_pcie->reset_gpio, 1);
+		gpiod_set_value(imx6_pcie->reset_gpio, 1);
 	}
 	return 0;
 
@@ -560,7 +560,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct imx6_pcie *imx6_pcie;
 	struct pcie_port *pp;
-	struct device_node *np = pdev->dev.of_node;
 	struct resource *dbi_base;
 	int ret;
 
@@ -581,15 +580,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pp->dbi_base);
 
 	/* Fetch GPIOs */
-	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
-	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");
-		if (ret) {
-			dev_err(&pdev->dev, "unable to get reset gpio\n");
-			return ret;
-		}
-	}
+	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
+							GPIOD_OUT_LOW);
 
 	/* Fetch clocks */
 	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
-- 
1.7.9.5

Re: [PATCH v2] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Lucas Stach]

Am Freitag, den 27.11.2015, 11:01 +0100 schrieb Petr Štetiar:
> It's currently not possible to use reset-gpio on board where the GPIO is
> active low as the code incorrectly assumes, that reset-gpio will be
> always active high.
> 
> In my case the broken behavior was observed on Toradex Apalis SoM with
> Ixora base board.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
> 
> Changes since v1:
>  * reworked to use descriptor based GPIO interface
> 
>  drivers/pci/host/pci-imx6.c |   20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 22e8224..c0e121d 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -32,7 +32,7 @@
>  #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
>  
>  struct imx6_pcie {
> -	int			reset_gpio;
> +	struct gpio_desc	*reset_gpio;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -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(imx6_pcie->reset_gpio, 0);
> +	if (imx6_pcie->reset_gpio) {
> +		gpiod_set_value(imx6_pcie->reset_gpio, 0);

This now collides with a fix from Fabio that allows this function to
sleep. So this should use gpiod_set_value_cansleep() and be rebased to
pci/host-imx6, or if Bjorn prefers to pull this in quickly, replace
Fabios commit.

Regards,
Lucas

>  		msleep(100);
> -		gpio_set_value(imx6_pcie->reset_gpio, 1);
> +		gpiod_set_value(imx6_pcie->reset_gpio, 1);
>  	}
>  	return 0;
>  
> @@ -560,7 +560,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie;
>  	struct pcie_port *pp;
> -	struct device_node *np = pdev->dev.of_node;
>  	struct resource *dbi_base;
>  	int ret;
>  
> @@ -581,15 +580,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> -	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");
> -		if (ret) {
> -			dev_err(&pdev->dev, "unable to get reset gpio\n");
> -			return ret;
> -		}
> -	}
> +	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> +							GPIOD_OUT_LOW);
>  
>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH v3] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Petr Štetiar]

It's currently not possible to use reset-gpio on board where the GPIO is
active low as the code incorrectly assumes, that reset-gpio will be
always active high.

In my case the broken behavior was observed on Toradex Apalis SoM with
Ixora base board.

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

Changes since v1:                                                                                                                                                        
 * reworked to use descriptor based GPIO interface

Changes since v2:
 * rebased on pci/host-imx6 which now uses gpio_set_value_cansleep()

 drivers/pci/host/pci-imx6.c |   20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 3c3b37f..8e9afa5 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -32,7 +32,7 @@
 #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
 
 struct imx6_pcie {
-	int			reset_gpio;
+	struct gpio_desc	*reset_gpio;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -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;
 
@@ -560,7 +560,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct imx6_pcie *imx6_pcie;
 	struct pcie_port *pp;
-	struct device_node *np = pdev->dev.of_node;
 	struct resource *dbi_base;
 	int ret;
 
@@ -581,15 +580,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pp->dbi_base);
 
 	/* Fetch GPIOs */
-	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
-	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");
-		if (ret) {
-			dev_err(&pdev->dev, "unable to get reset gpio\n");
-			return ret;
-		}
-	}
+	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
+							GPIOD_OUT_LOW);
 
 	/* Fetch clocks */
 	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
-- 
1.7.9.5

Re: [PATCH v3] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Lucas Stach]

Am Freitag, den 27.11.2015, 11:56 +0100 schrieb Petr Štetiar:
> It's currently not possible to use reset-gpio on board where the GPIO is
> active low as the code incorrectly assumes, that reset-gpio will be
> always active high.
> 
> In my case the broken behavior was observed on Toradex Apalis SoM with
> Ixora base board.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>

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

> ---
> 
> Changes since v1:                                                                                                                                                        
>  * reworked to use descriptor based GPIO interface
> 
> Changes since v2:
>  * rebased on pci/host-imx6 which now uses gpio_set_value_cansleep()
> 
>  drivers/pci/host/pci-imx6.c |   20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 3c3b37f..8e9afa5 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -32,7 +32,7 @@
>  #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
>  
>  struct imx6_pcie {
> -	int			reset_gpio;
> +	struct gpio_desc	*reset_gpio;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -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;
>  
> @@ -560,7 +560,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie;
>  	struct pcie_port *pp;
> -	struct device_node *np = pdev->dev.of_node;
>  	struct resource *dbi_base;
>  	int ret;
>  
> @@ -581,15 +580,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> -	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");
> -		if (ret) {
> -			dev_err(&pdev->dev, "unable to get reset gpio\n");
> -			return ret;
> -		}
> -	}
> +	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> +							GPIOD_OUT_LOW);
>  
>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH v3] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Bjorn Helgaas]

On Fri, Nov 27, 2015 at 11:56:34AM +0100, Petr Štetiar wrote:
> It's currently not possible to use reset-gpio on board where the GPIO is
> active low as the code incorrectly assumes, that reset-gpio will be
> always active high.
> 
> In my case the broken behavior was observed on Toradex Apalis SoM with
> Ixora base board.
> 
> Signed-off-by: Petr Štetiar <ynezz@true.cz>

Applied as follows with Lucas' reviewed-by to pci/host-imx6 for v4.5,
thanks!


commit 5c5fb40de8f14391a1238db05cef88754faf9229
Author: Petr Štetiar <ynezz@true.cz>
Date:   Fri Nov 27 11:56:34 2015 +0100

    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.
    
    [bhelgaas: changelog]
    Signed-off-by: Petr Štetiar <ynezz@true.cz>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 3c3b37f..8e9afa5 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -32,7 +32,7 @@
 #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
 
 struct imx6_pcie {
-	int			reset_gpio;
+	struct gpio_desc	*reset_gpio;
 	struct clk		*pcie_bus;
 	struct clk		*pcie_phy;
 	struct clk		*pcie;
@@ -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;
 
@@ -560,7 +560,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 {
 	struct imx6_pcie *imx6_pcie;
 	struct pcie_port *pp;
-	struct device_node *np = pdev->dev.of_node;
 	struct resource *dbi_base;
 	int ret;
 
@@ -581,15 +580,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pp->dbi_base);
 
 	/* Fetch GPIOs */
-	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
-	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");
-		if (ret) {
-			dev_err(&pdev->dev, "unable to get reset gpio\n");
-			return ret;
-		}
-	}
+	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
+							GPIOD_OUT_LOW);
 
 	/* Fetch clocks */
 	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");

Re: [PATCH v3] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Bjorn Helgaas]

[+cc Jingoo]

On Fri, Dec 04, 2015 at 02:59:06PM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 27, 2015 at 11:56:34AM +0100, Petr Štetiar wrote:
> > It's currently not possible to use reset-gpio on board where the GPIO is
> > active low as the code incorrectly assumes, that reset-gpio will be
> > always active high.
> > 
> > In my case the broken behavior was observed on Toradex Apalis SoM with
> > Ixora base board.
> > 
> > Signed-off-by: Petr Štetiar <ynezz@true.cz>
> 
> Applied as follows with Lucas' reviewed-by to pci/host-imx6 for v4.5,
> thanks!

Jingoo, I noticed pci-exynos.c also uses of_get_named_gpio().  Do you
care about this issue, too, or are you content to ignore the GPIO
flags for your "reset-gpio"?

Bjorn

> commit 5c5fb40de8f14391a1238db05cef88754faf9229
> Author: Petr Štetiar <ynezz@true.cz>
> Date:   Fri Nov 27 11:56:34 2015 +0100
> 
>     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.
>     
>     [bhelgaas: changelog]
>     Signed-off-by: Petr Štetiar <ynezz@true.cz>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 3c3b37f..8e9afa5 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -32,7 +32,7 @@
>  #define to_imx6_pcie(x)	container_of(x, struct imx6_pcie, pp)
>  
>  struct imx6_pcie {
> -	int			reset_gpio;
> +	struct gpio_desc	*reset_gpio;
>  	struct clk		*pcie_bus;
>  	struct clk		*pcie_phy;
>  	struct clk		*pcie;
> @@ -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;
>  
> @@ -560,7 +560,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  {
>  	struct imx6_pcie *imx6_pcie;
>  	struct pcie_port *pp;
> -	struct device_node *np = pdev->dev.of_node;
>  	struct resource *dbi_base;
>  	int ret;
>  
> @@ -581,15 +580,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
> -	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> -	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");
> -		if (ret) {
> -			dev_err(&pdev->dev, "unable to get reset gpio\n");
> -			return ret;
> -		}
> -	}
> +	imx6_pcie->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
> +							GPIOD_OUT_LOW);
>  
>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");

Re: [PATCH v3] PCI: imx6: Fix reset-gpio to work with active low GPIO

[Bjorn Helgaas]

[Jingoo's response didn't make it to the list because it contained
HTML, but I incorporated it below manually]

On Tue, Dec 8, 2015 at 11:28 PM, Han Jingoo wrote:
> On Fri, Dec 04, 2015 at 03:02:34PM -0600, Bjorn Helgaas wrote:
> > [+cc Jingoo]
> > 
> > On Fri, Dec 04, 2015 at 02:59:06PM -0600, Bjorn Helgaas wrote:
> > > On Fri, Nov 27, 2015 at 11:56:34AM +0100, Petr Štetiar wrote:
> > > > It's currently not possible to use reset-gpio on board where the GPIO is
> > > > active low as the code incorrectly assumes, that reset-gpio will be
> > > > always active high.
> > > > 
> > > > In my case the broken behavior was observed on Toradex Apalis SoM with
> > > > Ixora base board.
> > > > 
> > > > Signed-off-by: Petr Štetiar <ynezz@true.cz>
> > > 
> > > Applied as follows with Lucas' reviewed-by to pci/host-imx6 for v4.5,
> > > thanks!
> > 
> > Jingoo, I noticed pci-exynos.c also uses of_get_named_gpio().  Do you
> > care about this issue, too, or are you content to ignore the GPIO
> > flags for your "reset-gpio"?
>
> Sorry, I don't care about it.
> Also, there is no issue with Exynos platform.

OK, thanks for checking!

Bjorn