[V6,26/27] PCI: tegra: Add support for GPIO based PERST#
diff mbox series

Message ID 20190618180206.4908-27-mmaddireddy@nvidia.com
State New
Headers show
Series
  • Enable Tegra PCIe root port features
Related show

Commit Message

Manikanta Maddireddy June 18, 2019, 6:02 p.m. UTC
Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be
controlled by AFI port register. However, if a platform routes a different
GPIO to the PCIe slot, then port register cannot control it. Add support
for GPIO based PERST# signal for such platforms. GPIO number comes from per
port PCIe device tree node. PCIe driver probe doesn't fail if per port
"reset-gpios" property is not populated, make sure that DT property is not
missed for such platforms.

Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
V6: No change

V5:
* Updated reset gpio toggle logic to reflect active low usage
* Replaced kasprintf() with devm_kasprintf()
* Updated commit message with more information.

V4: Using devm_gpiod_get_from_of_node() to get reset-gpios

V3: Using helper function to get reset-gpios

V2: Using standard "reset-gpio" property

 drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

Comments

Jon Hunter July 4, 2019, 2:48 p.m. UTC | #1
On 18/06/2019 19:02, Manikanta Maddireddy wrote:
> Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be
> controlled by AFI port register. However, if a platform routes a different
> GPIO to the PCIe slot, then port register cannot control it. Add support
> for GPIO based PERST# signal for such platforms. GPIO number comes from per
> port PCIe device tree node. PCIe driver probe doesn't fail if per port
> "reset-gpios" property is not populated, make sure that DT property is not
> missed for such platforms.

Really? So how come on Jetson TK1 I see ...

[    1.073165] tegra-pcie 1003000.pcie: failed to get reset GPIO: -2


[    1.073210] tegra-pcie: probe of 1003000.pcie failed with error -2

And now ethernet is broken? Why has this not been tested on all Tegra
platforms? There is no reason why code submitted to upstream by us
(people at NVIDIA) have not tested this on our internal test farm. This
is disappointing.

> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> ---
> V6: No change
> 
> V5:
> * Updated reset gpio toggle logic to reflect active low usage
> * Replaced kasprintf() with devm_kasprintf()
> * Updated commit message with more information.
> 
> V4: Using devm_gpiod_get_from_of_node() to get reset-gpios
> 
> V3: Using helper function to get reset-gpios
> 
> V2: Using standard "reset-gpio" property
> 
>  drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index d2841532ff0e..a819bc087a05 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c
> @@ -17,6 +17,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/export.h>
> +#include <linux/gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/iopoll.h>
>  #include <linux/irq.h>
> @@ -399,6 +400,8 @@ struct tegra_pcie_port {
>  	unsigned int lanes;
>  
>  	struct phy **phys;
> +
> +	struct gpio_desc *reset_gpio;
>  };
>  
>  struct tegra_pcie_bus {
> @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port)
>  	unsigned long value;
>  
>  	/* pulse reset signal */
> -	value = afi_readl(port->pcie, ctrl);
> -	value &= ~AFI_PEX_CTRL_RST;
> -	afi_writel(port->pcie, value, ctrl);
> +	if (port->reset_gpio) {
> +		gpiod_set_value(port->reset_gpio, 1);
> +	} else {
> +		value = afi_readl(port->pcie, ctrl);
> +		value &= ~AFI_PEX_CTRL_RST;
> +		afi_writel(port->pcie, value, ctrl);
> +	}
>  
>  	usleep_range(1000, 2000);
>  
> -	value = afi_readl(port->pcie, ctrl);
> -	value |= AFI_PEX_CTRL_RST;
> -	afi_writel(port->pcie, value, ctrl);
> +	if (port->reset_gpio) {
> +		gpiod_set_value(port->reset_gpio, 0);
> +	} else {
> +		value = afi_readl(port->pcie, ctrl);
> +		value |= AFI_PEX_CTRL_RST;
> +		afi_writel(port->pcie, value, ctrl);
> +	}
>  }
>  
>  static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
> @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		struct tegra_pcie_port *rp;
>  		unsigned int index;
>  		u32 value;
> +		char *label;
>  
>  		err = of_pci_get_devfn(port);
>  		if (err < 0) {
> @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>  		if (IS_ERR(rp->base))
>  			return PTR_ERR(rp->base);
>  
> +		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
> +		if (!label) {
> +			dev_err(dev, "failed to create reset GPIO label\n");
> +			return -ENOMEM;
> +		}
> +
> +		/*
> +		 * Returns null if reset-gpios property is not populated and
> +		 * fall back to using AFI per port register to toggle PERST#
> +		 * SFIO line.
> +		 */
> +		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
> +							     "reset-gpios", 0,
> +							     GPIOD_OUT_LOW,
> +							     label);
> +		if (IS_ERR(rp->reset_gpio)) {
> +			err = PTR_ERR(rp->reset_gpio);
> +			dev_err(dev, "failed to get reset GPIO: %d\n", err);
> +			return err;
> +		}
> +

So I believe that we can just remove the above. I will give it a test
and see.

Cheers
Jon
Manikanta Maddireddy July 4, 2019, 3:29 p.m. UTC | #2
On 04-Jul-19 8:18 PM, Jon Hunter wrote:
> On 18/06/2019 19:02, Manikanta Maddireddy wrote:
>> Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be
>> controlled by AFI port register. However, if a platform routes a different
>> GPIO to the PCIe slot, then port register cannot control it. Add support
>> for GPIO based PERST# signal for such platforms. GPIO number comes from per
>> port PCIe device tree node. PCIe driver probe doesn't fail if per port
>> "reset-gpios" property is not populated, make sure that DT property is not
>> missed for such platforms.
> Really? So how come on Jetson TK1 I see ...
>
> [    1.073165] tegra-pcie 1003000.pcie: failed to get reset GPIO: -2
>
>
> [    1.073210] tegra-pcie: probe of 1003000.pcie failed with error -2
>
> And now ethernet is broken? Why has this not been tested on all Tegra
> platforms? There is no reason why code submitted to upstream by us
> (people at NVIDIA) have not tested this on our internal test farm. This
> is disappointing.

I did verified this patch on our internal TK1 test farm. I also tested
on TX1 and TX2.
This issue popped up because below fix in gpiolib driver went in
at the same time.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=025bf37725f1929542361eef2245df30badf242e

>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> ---
>> V6: No change
>>
>> V5:
>> * Updated reset gpio toggle logic to reflect active low usage
>> * Replaced kasprintf() with devm_kasprintf()
>> * Updated commit message with more information.
>>
>> V4: Using devm_gpiod_get_from_of_node() to get reset-gpios
>>
>> V3: Using helper function to get reset-gpios
>>
>> V2: Using standard "reset-gpio" property
>>
>>  drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++----
>>  1 file changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>> index d2841532ff0e..a819bc087a05 100644
>> --- a/drivers/pci/controller/pci-tegra.c
>> +++ b/drivers/pci/controller/pci-tegra.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/debugfs.h>
>>  #include <linux/delay.h>
>>  #include <linux/export.h>
>> +#include <linux/gpio.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/irq.h>
>> @@ -399,6 +400,8 @@ struct tegra_pcie_port {
>>  	unsigned int lanes;
>>  
>>  	struct phy **phys;
>> +
>> +	struct gpio_desc *reset_gpio;
>>  };
>>  
>>  struct tegra_pcie_bus {
>> @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port)
>>  	unsigned long value;
>>  
>>  	/* pulse reset signal */
>> -	value = afi_readl(port->pcie, ctrl);
>> -	value &= ~AFI_PEX_CTRL_RST;
>> -	afi_writel(port->pcie, value, ctrl);
>> +	if (port->reset_gpio) {
>> +		gpiod_set_value(port->reset_gpio, 1);
>> +	} else {
>> +		value = afi_readl(port->pcie, ctrl);
>> +		value &= ~AFI_PEX_CTRL_RST;
>> +		afi_writel(port->pcie, value, ctrl);
>> +	}
>>  
>>  	usleep_range(1000, 2000);
>>  
>> -	value = afi_readl(port->pcie, ctrl);
>> -	value |= AFI_PEX_CTRL_RST;
>> -	afi_writel(port->pcie, value, ctrl);
>> +	if (port->reset_gpio) {
>> +		gpiod_set_value(port->reset_gpio, 0);
>> +	} else {
>> +		value = afi_readl(port->pcie, ctrl);
>> +		value |= AFI_PEX_CTRL_RST;
>> +		afi_writel(port->pcie, value, ctrl);
>> +	}
>>  }
>>  
>>  static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>> @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>  		struct tegra_pcie_port *rp;
>>  		unsigned int index;
>>  		u32 value;
>> +		char *label;
>>  
>>  		err = of_pci_get_devfn(port);
>>  		if (err < 0) {
>> @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>  		if (IS_ERR(rp->base))
>>  			return PTR_ERR(rp->base);
>>  
>> +		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
>> +		if (!label) {
>> +			dev_err(dev, "failed to create reset GPIO label\n");
>> +			return -ENOMEM;
>> +		}
>> +
>> +		/*
>> +		 * Returns null if reset-gpios property is not populated and
>> +		 * fall back to using AFI per port register to toggle PERST#
>> +		 * SFIO line.
>> +		 */
>> +		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
>> +							     "reset-gpios", 0,
>> +							     GPIOD_OUT_LOW,
>> +							     label);
>> +		if (IS_ERR(rp->reset_gpio)) {
>> +			err = PTR_ERR(rp->reset_gpio);
>> +			dev_err(dev, "failed to get reset GPIO: %d\n", err);
>> +			return err;
>> +		}
>> +
> So I believe that we can just remove the above. I will give it a test
> and see.
>
> Cheers
> Jon
>
Error check should be modified as follows,

	if (PTR_ERR(rp->reset_gpio) == -ENOENT)
		rp->reset_gpio = NULL;
	else if (IS_ERR(rp->reset_gpio))
 		return PTR_ERR(rp->reset_gpio);

Manikanta
Jon Hunter July 4, 2019, 5:23 p.m. UTC | #3
On 04/07/2019 16:29, Manikanta Maddireddy wrote:
> 
> 
> On 04-Jul-19 8:18 PM, Jon Hunter wrote:
>> On 18/06/2019 19:02, Manikanta Maddireddy wrote:
>>> Tegra PCIe has fixed per port SFIO line to signal PERST#, which can be
>>> controlled by AFI port register. However, if a platform routes a different
>>> GPIO to the PCIe slot, then port register cannot control it. Add support
>>> for GPIO based PERST# signal for such platforms. GPIO number comes from per
>>> port PCIe device tree node. PCIe driver probe doesn't fail if per port
>>> "reset-gpios" property is not populated, make sure that DT property is not
>>> missed for such platforms.
>> Really? So how come on Jetson TK1 I see ...
>>
>> [    1.073165] tegra-pcie 1003000.pcie: failed to get reset GPIO: -2
>>
>>
>> [    1.073210] tegra-pcie: probe of 1003000.pcie failed with error -2
>>
>> And now ethernet is broken? Why has this not been tested on all Tegra
>> platforms? There is no reason why code submitted to upstream by us
>> (people at NVIDIA) have not tested this on our internal test farm. This
>> is disappointing.
> 
> I did verified this patch on our internal TK1 test farm. I also tested
> on TX1 and TX2.
> This issue popped up because below fix in gpiolib driver went in
> at the same time.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=025bf37725f1929542361eef2245df30badf242e

Ah! OK now that was unlucky. I guess a bisect would have told me that
but it was clear to see PCI was broken and so quicker to debug this
directly. OK, then sorry for the rant!
 
>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> V6: No change
>>>
>>> V5:
>>> * Updated reset gpio toggle logic to reflect active low usage
>>> * Replaced kasprintf() with devm_kasprintf()
>>> * Updated commit message with more information.
>>>
>>> V4: Using devm_gpiod_get_from_of_node() to get reset-gpios
>>>
>>> V3: Using helper function to get reset-gpios
>>>
>>> V2: Using standard "reset-gpio" property
>>>
>>>  drivers/pci/controller/pci-tegra.c | 45 ++++++++++++++++++++++++++----
>>>  1 file changed, 39 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>>> index d2841532ff0e..a819bc087a05 100644
>>> --- a/drivers/pci/controller/pci-tegra.c
>>> +++ b/drivers/pci/controller/pci-tegra.c
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/debugfs.h>
>>>  #include <linux/delay.h>
>>>  #include <linux/export.h>
>>> +#include <linux/gpio.h>
>>>  #include <linux/interrupt.h>
>>>  #include <linux/iopoll.h>
>>>  #include <linux/irq.h>
>>> @@ -399,6 +400,8 @@ struct tegra_pcie_port {
>>>  	unsigned int lanes;
>>>  
>>>  	struct phy **phys;
>>> +
>>> +	struct gpio_desc *reset_gpio;
>>>  };
>>>  
>>>  struct tegra_pcie_bus {
>>> @@ -544,15 +547,23 @@ static void tegra_pcie_port_reset(struct tegra_pcie_port *port)
>>>  	unsigned long value;
>>>  
>>>  	/* pulse reset signal */
>>> -	value = afi_readl(port->pcie, ctrl);
>>> -	value &= ~AFI_PEX_CTRL_RST;
>>> -	afi_writel(port->pcie, value, ctrl);
>>> +	if (port->reset_gpio) {
>>> +		gpiod_set_value(port->reset_gpio, 1);
>>> +	} else {
>>> +		value = afi_readl(port->pcie, ctrl);
>>> +		value &= ~AFI_PEX_CTRL_RST;
>>> +		afi_writel(port->pcie, value, ctrl);
>>> +	}
>>>  
>>>  	usleep_range(1000, 2000);
>>>  
>>> -	value = afi_readl(port->pcie, ctrl);
>>> -	value |= AFI_PEX_CTRL_RST;
>>> -	afi_writel(port->pcie, value, ctrl);
>>> +	if (port->reset_gpio) {
>>> +		gpiod_set_value(port->reset_gpio, 0);
>>> +	} else {
>>> +		value = afi_readl(port->pcie, ctrl);
>>> +		value |= AFI_PEX_CTRL_RST;
>>> +		afi_writel(port->pcie, value, ctrl);
>>> +	}
>>>  }
>>>  
>>>  static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>>> @@ -2199,6 +2210,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>>  		struct tegra_pcie_port *rp;
>>>  		unsigned int index;
>>>  		u32 value;
>>> +		char *label;
>>>  
>>>  		err = of_pci_get_devfn(port);
>>>  		if (err < 0) {
>>> @@ -2257,6 +2269,27 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>>  		if (IS_ERR(rp->base))
>>>  			return PTR_ERR(rp->base);
>>>  
>>> +		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
>>> +		if (!label) {
>>> +			dev_err(dev, "failed to create reset GPIO label\n");
>>> +			return -ENOMEM;
>>> +		}
>>> +
>>> +		/*
>>> +		 * Returns null if reset-gpios property is not populated and
>>> +		 * fall back to using AFI per port register to toggle PERST#
>>> +		 * SFIO line.
>>> +		 */
>>> +		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
>>> +							     "reset-gpios", 0,
>>> +							     GPIOD_OUT_LOW,
>>> +							     label);
>>> +		if (IS_ERR(rp->reset_gpio)) {
>>> +			err = PTR_ERR(rp->reset_gpio);
>>> +			dev_err(dev, "failed to get reset GPIO: %d\n", err);
>>> +			return err;
>>> +		}
>>> +
>> So I believe that we can just remove the above. I will give it a test
>> and see.
>>
>> Cheers
>> Jon
>>
> Error check should be modified as follows,
> 
> 	if (PTR_ERR(rp->reset_gpio) == -ENOENT)
> 		rp->reset_gpio = NULL;
> 	else if (IS_ERR(rp->reset_gpio))
>  		return PTR_ERR(rp->reset_gpio);

OK, but I think that this should be ...

+               if (IS_ERR(rp->reset_gpio)) {
+                       if (PTR_ERR(rp->reset_gpio) == -ENOENT) {
+                               rp->reset_gpio = NULL;
+                       } else {
+                               dev_err(dev, "failed to get reset GPIO: %d\n", err);
+                               return PTR_ERR(rp->reset_gpio);
+                       }
+               }
 
Cheers
Jon

Patch
diff mbox series

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index d2841532ff0e..a819bc087a05 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -17,6 +17,7 @@ 
 #include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/irq.h>
@@ -399,6 +400,8 @@  struct tegra_pcie_port {
 	unsigned int lanes;
 
 	struct phy **phys;
+
+	struct gpio_desc *reset_gpio;
 };
 
 struct tegra_pcie_bus {
@@ -544,15 +547,23 @@  static void tegra_pcie_port_reset(struct tegra_pcie_port *port)
 	unsigned long value;
 
 	/* pulse reset signal */
-	value = afi_readl(port->pcie, ctrl);
-	value &= ~AFI_PEX_CTRL_RST;
-	afi_writel(port->pcie, value, ctrl);
+	if (port->reset_gpio) {
+		gpiod_set_value(port->reset_gpio, 1);
+	} else {
+		value = afi_readl(port->pcie, ctrl);
+		value &= ~AFI_PEX_CTRL_RST;
+		afi_writel(port->pcie, value, ctrl);
+	}
 
 	usleep_range(1000, 2000);
 
-	value = afi_readl(port->pcie, ctrl);
-	value |= AFI_PEX_CTRL_RST;
-	afi_writel(port->pcie, value, ctrl);
+	if (port->reset_gpio) {
+		gpiod_set_value(port->reset_gpio, 0);
+	} else {
+		value = afi_readl(port->pcie, ctrl);
+		value |= AFI_PEX_CTRL_RST;
+		afi_writel(port->pcie, value, ctrl);
+	}
 }
 
 static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
@@ -2199,6 +2210,7 @@  static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		struct tegra_pcie_port *rp;
 		unsigned int index;
 		u32 value;
+		char *label;
 
 		err = of_pci_get_devfn(port);
 		if (err < 0) {
@@ -2257,6 +2269,27 @@  static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 		if (IS_ERR(rp->base))
 			return PTR_ERR(rp->base);
 
+		label = devm_kasprintf(dev, GFP_KERNEL, "pex-reset-%u", index);
+		if (!label) {
+			dev_err(dev, "failed to create reset GPIO label\n");
+			return -ENOMEM;
+		}
+
+		/*
+		 * Returns null if reset-gpios property is not populated and
+		 * fall back to using AFI per port register to toggle PERST#
+		 * SFIO line.
+		 */
+		rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port,
+							     "reset-gpios", 0,
+							     GPIOD_OUT_LOW,
+							     label);
+		if (IS_ERR(rp->reset_gpio)) {
+			err = PTR_ERR(rp->reset_gpio);
+			dev_err(dev, "failed to get reset GPIO: %d\n", err);
+			return err;
+		}
+
 		list_add_tail(&rp->list, &pcie->ports);
 	}