[PATCH v2 0/2] Improve pcie-histb GPIO reset implementation

[PATCH v2 0/2] Improve pcie-histb GPIO reset implementation

[Shawn Guo]

It switches pcie-histb PCIe reset code use gpio_desc for GPIO
configuration, and corrects the GPIO reset operation as per PCI EXPRESS
CARD ELECTROMECHANICAL SPECIFICATION.

Changes for v2:
 - Split into two patches
 - Capitalize the subject
 - Add some comments for PCIe reset operation

Shawn Guo (2):
  PCI: histb: Use gpio_desc for PCIe GPIO reset
  PCI: histb: Correct PCIe reset operation

 drivers/pci/controller/dwc/pcie-histb.c | 41 ++++++++++++-------------
 1 file changed, 20 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH v2 0/2] Improve pcie-histb GPIO reset implementation

[Shawn Guo]

It switches pcie-histb PCIe reset code use gpio_desc for GPIO
configuration, and corrects the GPIO reset operation as per PCI EXPRESS
CARD ELECTROMECHANICAL SPECIFICATION.

Changes for v2:
 - Split into two patches
 - Capitalize the subject
 - Add some comments for PCIe reset operation

Shawn Guo (2):
  PCI: histb: Use gpio_desc for PCIe GPIO reset
  PCI: histb: Correct PCIe reset operation

 drivers/pci/controller/dwc/pcie-histb.c | 41 ++++++++++++-------------
 1 file changed, 20 insertions(+), 21 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] PCI: histb: Use gpio_desc for PCIe GPIO reset

[Shawn Guo]

It switches GPIO reset code to use gpio_desc, so that the code becomes
simpler and cleaner.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pci/controller/dwc/pcie-histb.c | 29 +++++++++----------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 811b5c6d62ea..112254619ed0 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -60,7 +60,7 @@ struct histb_pcie {
 	struct reset_control *sys_reset;
 	struct reset_control *bus_reset;
 	void __iomem *ctrl;
-	int reset_gpio;
+	struct gpio_desc *reset_gpio;
 	struct regulator *vpcie;
 };
 
@@ -219,8 +219,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 	clk_disable_unprepare(hipcie->sys_clk);
 	clk_disable_unprepare(hipcie->bus_clk);
 
-	if (gpio_is_valid(hipcie->reset_gpio))
-		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
+	if (hipcie->reset_gpio)
+		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
 
 	if (hipcie->vpcie)
 		regulator_disable(hipcie->vpcie);
@@ -242,8 +242,8 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
 		}
 	}
 
-	if (gpio_is_valid(hipcie->reset_gpio))
-		gpio_set_value_cansleep(hipcie->reset_gpio, 1);
+	if (hipcie->reset_gpio)
+		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
 
 	ret = clk_prepare_enable(hipcie->bus_clk);
 	if (ret) {
@@ -305,10 +305,7 @@ static int histb_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	struct pcie_port *pp;
 	struct resource *res;
-	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
-	enum of_gpio_flags of_flags;
-	unsigned long flag = GPIOF_DIR_OUT;
 	int ret;
 
 	hipcie = devm_kzalloc(dev, sizeof(*hipcie), GFP_KERNEL);
@@ -345,17 +342,11 @@ static int histb_pcie_probe(struct platform_device *pdev)
 		hipcie->vpcie = NULL;
 	}
 
-	hipcie->reset_gpio = of_get_named_gpio_flags(np,
-				"reset-gpios", 0, &of_flags);
-	if (of_flags & OF_GPIO_ACTIVE_LOW)
-		flag |= GPIOF_ACTIVE_LOW;
-	if (gpio_is_valid(hipcie->reset_gpio)) {
-		ret = devm_gpio_request_one(dev, hipcie->reset_gpio,
-				flag, "PCIe device power control");
-		if (ret) {
-			dev_err(dev, "unable to request gpio\n");
-			return ret;
-		}
+	hipcie->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(hipcie->reset_gpio)) {
+		ret = PTR_ERR(hipcie->reset_gpio);
+		return ret;
 	}
 
 	hipcie->aux_clk = devm_clk_get(dev, "aux");
-- 
2.17.1

[PATCH v2 1/2] PCI: histb: Use gpio_desc for PCIe GPIO reset

[Shawn Guo]

It switches GPIO reset code to use gpio_desc, so that the code becomes
simpler and cleaner.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pci/controller/dwc/pcie-histb.c | 29 +++++++++----------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 811b5c6d62ea..112254619ed0 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -60,7 +60,7 @@ struct histb_pcie {
 	struct reset_control *sys_reset;
 	struct reset_control *bus_reset;
 	void __iomem *ctrl;
-	int reset_gpio;
+	struct gpio_desc *reset_gpio;
 	struct regulator *vpcie;
 };
 
@@ -219,8 +219,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 	clk_disable_unprepare(hipcie->sys_clk);
 	clk_disable_unprepare(hipcie->bus_clk);
 
-	if (gpio_is_valid(hipcie->reset_gpio))
-		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
+	if (hipcie->reset_gpio)
+		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
 
 	if (hipcie->vpcie)
 		regulator_disable(hipcie->vpcie);
@@ -242,8 +242,8 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
 		}
 	}
 
-	if (gpio_is_valid(hipcie->reset_gpio))
-		gpio_set_value_cansleep(hipcie->reset_gpio, 1);
+	if (hipcie->reset_gpio)
+		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
 
 	ret = clk_prepare_enable(hipcie->bus_clk);
 	if (ret) {
@@ -305,10 +305,7 @@ static int histb_pcie_probe(struct platform_device *pdev)
 	struct dw_pcie *pci;
 	struct pcie_port *pp;
 	struct resource *res;
-	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
-	enum of_gpio_flags of_flags;
-	unsigned long flag = GPIOF_DIR_OUT;
 	int ret;
 
 	hipcie = devm_kzalloc(dev, sizeof(*hipcie), GFP_KERNEL);
@@ -345,17 +342,11 @@ static int histb_pcie_probe(struct platform_device *pdev)
 		hipcie->vpcie = NULL;
 	}
 
-	hipcie->reset_gpio = of_get_named_gpio_flags(np,
-				"reset-gpios", 0, &of_flags);
-	if (of_flags & OF_GPIO_ACTIVE_LOW)
-		flag |= GPIOF_ACTIVE_LOW;
-	if (gpio_is_valid(hipcie->reset_gpio)) {
-		ret = devm_gpio_request_one(dev, hipcie->reset_gpio,
-				flag, "PCIe device power control");
-		if (ret) {
-			dev_err(dev, "unable to request gpio\n");
-			return ret;
-		}
+	hipcie->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(hipcie->reset_gpio)) {
+		ret = PTR_ERR(hipcie->reset_gpio);
+		return ret;
 	}
 
 	hipcie->aux_clk = devm_clk_get(dev, "aux");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] PCI: histb: Correct PCIe reset operation

[Shawn Guo]

The PCIe reset via GPIO in the driver never worked as expected.  Per
"Power Sequencing and Reset Signal Timings" table in
PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, the PERST# should be
deasserted after minimum of 100us once REFCLK is stable.

The assertion has been done when the GPIO is being requested, and
deassertion should be done in host enabling rather than disabling. Also
a bit wait is added to ensure device get ready after reset.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pci/controller/dwc/pcie-histb.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 112254619ed0..67c27a8036c7 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -219,9 +219,6 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 	clk_disable_unprepare(hipcie->sys_clk);
 	clk_disable_unprepare(hipcie->bus_clk);
 
-	if (hipcie->reset_gpio)
-		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
-
 	if (hipcie->vpcie)
 		regulator_disable(hipcie->vpcie);
 }
@@ -242,9 +239,6 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
 		}
 	}
 
-	if (hipcie->reset_gpio)
-		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
-
 	ret = clk_prepare_enable(hipcie->bus_clk);
 	if (ret) {
 		dev_err(dev, "cannot prepare/enable bus clk\n");
@@ -278,6 +272,20 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
 	reset_control_assert(hipcie->bus_reset);
 	reset_control_deassert(hipcie->bus_reset);
 
+	if (hipcie->reset_gpio) {
+		/*
+		 * "Power Sequencing and Reset Signal Timings" table in
+		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, indicates
+		 * PERST# should be deasserted after minimum of 100us
+		 * once REFCLK is stable.
+		 */
+		usleep_range(100, 200);
+		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
+
+		/* wait 1ms for device to be ready */
+		usleep_range(1000, 2000);
+	}
+
 	return 0;
 
 err_aux_clk:
-- 
2.17.1

[PATCH v2 2/2] PCI: histb: Correct PCIe reset operation

[Shawn Guo]

The PCIe reset via GPIO in the driver never worked as expected.  Per
"Power Sequencing and Reset Signal Timings" table in
PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, the PERST# should be
deasserted after minimum of 100us once REFCLK is stable.

The assertion has been done when the GPIO is being requested, and
deassertion should be done in host enabling rather than disabling. Also
a bit wait is added to ensure device get ready after reset.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pci/controller/dwc/pcie-histb.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 112254619ed0..67c27a8036c7 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -219,9 +219,6 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 	clk_disable_unprepare(hipcie->sys_clk);
 	clk_disable_unprepare(hipcie->bus_clk);
 
-	if (hipcie->reset_gpio)
-		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
-
 	if (hipcie->vpcie)
 		regulator_disable(hipcie->vpcie);
 }
@@ -242,9 +239,6 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
 		}
 	}
 
-	if (hipcie->reset_gpio)
-		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
-
 	ret = clk_prepare_enable(hipcie->bus_clk);
 	if (ret) {
 		dev_err(dev, "cannot prepare/enable bus clk\n");
@@ -278,6 +272,20 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
 	reset_control_assert(hipcie->bus_reset);
 	reset_control_deassert(hipcie->bus_reset);
 
+	if (hipcie->reset_gpio) {
+		/*
+		 * "Power Sequencing and Reset Signal Timings" table in
+		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, indicates
+		 * PERST# should be deasserted after minimum of 100us
+		 * once REFCLK is stable.
+		 */
+		usleep_range(100, 200);
+		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
+
+		/* wait 1ms for device to be ready */
+		usleep_range(1000, 2000);
+	}
+
 	return 0;
 
 err_aux_clk:
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] PCI: histb: Correct PCIe reset operation

[Lorenzo Pieralisi]

On Thu, Jan 09, 2020 at 11:28:51AM +0800, Shawn Guo wrote:
> The PCIe reset via GPIO in the driver never worked as expected.  Per
> "Power Sequencing and Reset Signal Timings" table in
> PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, the PERST# should be
> deasserted after minimum of 100us once REFCLK is stable.
> 
> The assertion has been done when the GPIO is being requested, and
> deassertion should be done in host enabling rather than disabling. Also
> a bit wait is added to ensure device get ready after reset.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-histb.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)

Shawn,

this looks like a fix, please tag it as such and let me know if
it has to be backported, in which case also the previous patch
should I assume.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index 112254619ed0..67c27a8036c7 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -219,9 +219,6 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
>  	clk_disable_unprepare(hipcie->sys_clk);
>  	clk_disable_unprepare(hipcie->bus_clk);
>  
> -	if (hipcie->reset_gpio)
> -		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
> -
>  	if (hipcie->vpcie)
>  		regulator_disable(hipcie->vpcie);
>  }
> @@ -242,9 +239,6 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
>  		}
>  	}
>  
> -	if (hipcie->reset_gpio)
> -		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
> -
>  	ret = clk_prepare_enable(hipcie->bus_clk);
>  	if (ret) {
>  		dev_err(dev, "cannot prepare/enable bus clk\n");
> @@ -278,6 +272,20 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
>  	reset_control_assert(hipcie->bus_reset);
>  	reset_control_deassert(hipcie->bus_reset);
>  
> +	if (hipcie->reset_gpio) {
> +		/*
> +		 * "Power Sequencing and Reset Signal Timings" table in
> +		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, indicates
> +		 * PERST# should be deasserted after minimum of 100us
> +		 * once REFCLK is stable.
> +		 */
> +		usleep_range(100, 200);
> +		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
> +
> +		/* wait 1ms for device to be ready */
> +		usleep_range(1000, 2000);
> +	}
> +
>  	return 0;
>  
>  err_aux_clk:
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/2] PCI: histb: Correct PCIe reset operation

[Lorenzo Pieralisi]

On Thu, Jan 09, 2020 at 11:28:51AM +0800, Shawn Guo wrote:
> The PCIe reset via GPIO in the driver never worked as expected.  Per
> "Power Sequencing and Reset Signal Timings" table in
> PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, the PERST# should be
> deasserted after minimum of 100us once REFCLK is stable.
> 
> The assertion has been done when the GPIO is being requested, and
> deassertion should be done in host enabling rather than disabling. Also
> a bit wait is added to ensure device get ready after reset.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-histb.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)

Shawn,

this looks like a fix, please tag it as such and let me know if
it has to be backported, in which case also the previous patch
should I assume.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index 112254619ed0..67c27a8036c7 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -219,9 +219,6 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
>  	clk_disable_unprepare(hipcie->sys_clk);
>  	clk_disable_unprepare(hipcie->bus_clk);
>  
> -	if (hipcie->reset_gpio)
> -		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
> -
>  	if (hipcie->vpcie)
>  		regulator_disable(hipcie->vpcie);
>  }
> @@ -242,9 +239,6 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
>  		}
>  	}
>  
> -	if (hipcie->reset_gpio)
> -		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
> -
>  	ret = clk_prepare_enable(hipcie->bus_clk);
>  	if (ret) {
>  		dev_err(dev, "cannot prepare/enable bus clk\n");
> @@ -278,6 +272,20 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
>  	reset_control_assert(hipcie->bus_reset);
>  	reset_control_deassert(hipcie->bus_reset);
>  
> +	if (hipcie->reset_gpio) {
> +		/*
> +		 * "Power Sequencing and Reset Signal Timings" table in
> +		 * PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, indicates
> +		 * PERST# should be deasserted after minimum of 100us
> +		 * once REFCLK is stable.
> +		 */
> +		usleep_range(100, 200);
> +		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
> +
> +		/* wait 1ms for device to be ready */
> +		usleep_range(1000, 2000);
> +	}
> +
>  	return 0;
>  
>  err_aux_clk:
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] PCI: histb: Correct PCIe reset operation

[Shawn Guo]

On Wed, Feb 26, 2020 at 7:31 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Jan 09, 2020 at 11:28:51AM +0800, Shawn Guo wrote:
> > The PCIe reset via GPIO in the driver never worked as expected.  Per
> > "Power Sequencing and Reset Signal Timings" table in
> > PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, the PERST# should be
> > deasserted after minimum of 100us once REFCLK is stable.
> >
> > The assertion has been done when the GPIO is being requested, and
> > deassertion should be done in host enabling rather than disabling. Also
> > a bit wait is added to ensure device get ready after reset.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-histb.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
>
> Shawn,
>
> this looks like a fix, please tag it as such and let me know if
> it has to be backported, in which case also the previous patch
> should I assume.

Hi Lorenzo,

It is a fix, but we recently realized that the problem needs to be
fixed in a way that does not break existing DTB.  So please ignore the
series for now, and we will try to work out a better one.  Sorry that
we did not update the thread in time.

Shawn

Re: [PATCH v2 2/2] PCI: histb: Correct PCIe reset operation

[Shawn Guo]

On Wed, Feb 26, 2020 at 7:31 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Jan 09, 2020 at 11:28:51AM +0800, Shawn Guo wrote:
> > The PCIe reset via GPIO in the driver never worked as expected.  Per
> > "Power Sequencing and Reset Signal Timings" table in
> > PCI EXPRESS CARD ELECTROMECHANICAL SPECIFICATION, the PERST# should be
> > deasserted after minimum of 100us once REFCLK is stable.
> >
> > The assertion has been done when the GPIO is being requested, and
> > deassertion should be done in host enabling rather than disabling. Also
> > a bit wait is added to ensure device get ready after reset.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-histb.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
>
> Shawn,
>
> this looks like a fix, please tag it as such and let me know if
> it has to be backported, in which case also the previous patch
> should I assume.

Hi Lorenzo,

It is a fix, but we recently realized that the problem needs to be
fixed in a way that does not break existing DTB.  So please ignore the
series for now, and we will try to work out a better one.  Sorry that
we did not update the thread in time.

Shawn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel