PCI: histb: add power control GPIO for PCIe slot

Message ID 1516669477-20151-1-git-send-email-shawn.guo@linaro.org
State Superseded
Headers show
Series
  • PCI: histb: add power control GPIO for PCIe slot
Related show

Commit Message

Shawn Guo Jan. 23, 2018, 1:04 a.m.
From: Jianguo Sun <sunjianguo1@huawei.com>

Besides the GPIO for controlling reset, there is also possibly another
GPIO for turning on/off the power of PCIe slot.  Let's add the support
for that with another optional device tree property 'power-gpios'.

Signed-off-by: Jianguo Sun <sunjianguo1@huawei.com>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../bindings/pci/hisilicon-histb-pcie.txt          |  1 +
 drivers/pci/dwc/pcie-histb.c                       | 28 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

Comments

Fabio Estevam Jan. 23, 2018, 1:12 a.m. | #1
Hi Shawn,

On Mon, Jan 22, 2018 at 11:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> From: Jianguo Sun <sunjianguo1@huawei.com>
>
> Besides the GPIO for controlling reset, there is also possibly another
> GPIO for turning on/off the power of PCIe slot.  Let's add the support
> for that with another optional device tree property 'power-gpios'.

It seems that a better approach would be to add regulator support
instead as it is more general.

We do this in drivers/pci/dwc/pci-imx6.c.
Daniel Thompson Jan. 23, 2018, 9:39 a.m. | #2
On Tue, Jan 23, 2018 at 09:04:37AM +0800, Shawn Guo wrote:
> From: Jianguo Sun <sunjianguo1@huawei.com>
> 
> Besides the GPIO for controlling reset, there is also possibly another
> GPIO for turning on/off the power of PCIe slot.  Let's add the support
> for that with another optional device tree property 'power-gpios'.
> 
> Signed-off-by: Jianguo Sun <sunjianguo1@huawei.com>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../bindings/pci/hisilicon-histb-pcie.txt          |  1 +
>  drivers/pci/dwc/pcie-histb.c                       | 28 +++++++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> index c84bc027930b..597397a048f8 100644
> --- a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> @@ -33,6 +33,7 @@ Required properties
>    "bus": bus reset.
>  
>  Optional properties:
> +- power-gpios: The gpio to control the power of PCIe slot.
>  - reset-gpios: The gpio to generate PCIe PERST# assert and deassert signal.
>  - phys: List of phandle and phy mode specifier, should be 0.
>  - phy-names: Must be "phy".
> diff --git a/drivers/pci/dwc/pcie-histb.c b/drivers/pci/dwc/pcie-histb.c
> index 33b01b734d7d..2a6b18619b25 100644
> --- a/drivers/pci/dwc/pcie-histb.c
> +++ b/drivers/pci/dwc/pcie-histb.c
> @@ -63,6 +63,7 @@ struct histb_pcie {
>  	struct reset_control *sys_reset;
>  	struct reset_control *bus_reset;
>  	void __iomem *ctrl;
> +	int power_gpio;
>  	int reset_gpio;
>  };
>  
> @@ -230,6 +231,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
>  
>  	if (gpio_is_valid(hipcie->reset_gpio))
>  		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
> +	if (gpio_is_valid(hipcie->power_gpio))
> +		gpio_set_value_cansleep(hipcie->power_gpio, 0);
>  }
>  
>  static int histb_pcie_host_enable(struct pcie_port *pp)
> @@ -240,8 +243,14 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
>  	int ret;
>  
>  	/* power on PCIe device if have */
> -	if (gpio_is_valid(hipcie->reset_gpio))
> +	if (gpio_is_valid(hipcie->power_gpio))
> +		gpio_set_value_cansleep(hipcie->power_gpio, 1);
> +
> +	if (gpio_is_valid(hipcie->reset_gpio)) {
> +		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
> +		mdelay(10);
>  		gpio_set_value_cansleep(hipcie->reset_gpio, 1);
> +	}
>  
>  	ret = clk_prepare_enable(hipcie->bus_clk);
>  	if (ret) {
> @@ -335,15 +344,28 @@ static int histb_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pci->dbi_base);
>  	}
>  
> +	hipcie->power_gpio = of_get_named_gpio_flags(np,
> +				"power-gpios", 0, &of_flags);
> +	if (of_flags & OF_GPIO_ACTIVE_LOW)
> +		flag |= GPIOF_ACTIVE_LOW;

Why isn't this inside the if statement?


> +	if (gpio_is_valid(hipcie->power_gpio)) {
> +		ret = devm_gpio_request_one(dev, hipcie->power_gpio,
> +				flag, "PCIe device power control");
> +		if (ret) {
> +			dev_err(dev, "unable to request power gpio\n");
> +			return ret;
> +		}
> +	}
> +
>  	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");
> +				flag, "PCIe device reset control");
>  		if (ret) {
> -			dev_err(dev, "unable to request gpio\n");
> +			dev_err(dev, "unable to request reset gpio\n");
>  			return ret;
>  		}
>  	}
> -- 
> 1.9.1
>
Shawn Guo Jan. 23, 2018, 10:52 a.m. | #3
Hi Daniel,

On Tue, Jan 23, 2018 at 09:39:44AM +0000, Daniel Thompson wrote:
> > @@ -335,15 +344,28 @@ static int histb_pcie_probe(struct platform_device *pdev)
> >  		return PTR_ERR(pci->dbi_base);
> >  	}
> >  
> > +	hipcie->power_gpio = of_get_named_gpio_flags(np,
> > +				"power-gpios", 0, &of_flags);
> > +	if (of_flags & OF_GPIO_ACTIVE_LOW)
> > +		flag |= GPIOF_ACTIVE_LOW;
> 
> Why isn't this inside the if statement?

Are you asking why the flag manipulation is not in the gpio_is_valid()
if-clause below?

I guess this is a copy of how reset_gpio is handled.  It might be a bit
more sensible to check the validity of the GPIO before handling the
flag, but practically the current code doesn't really hurt too much.

I will take Fabio's suggestion to reimplement it with a fixed regulator.
But thanks for the comment anyway.

Shawn

> > +	if (gpio_is_valid(hipcie->power_gpio)) {
> > +		ret = devm_gpio_request_one(dev, hipcie->power_gpio,
> > +				flag, "PCIe device power control");
> > +		if (ret) {
> > +			dev_err(dev, "unable to request power gpio\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	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");
> > +				flag, "PCIe device reset control");
> >  		if (ret) {
> > -			dev_err(dev, "unable to request gpio\n");
> > +			dev_err(dev, "unable to request reset gpio\n");
> >  			return ret;
> >  		}
> >  	}
> > -- 
> > 1.9.1
> >
Shawn Guo Jan. 23, 2018, 10:54 a.m. | #4
On Mon, Jan 22, 2018 at 11:12:44PM -0200, Fabio Estevam wrote:
> Hi Shawn,
> 
> On Mon, Jan 22, 2018 at 11:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > From: Jianguo Sun <sunjianguo1@huawei.com>
> >
> > Besides the GPIO for controlling reset, there is also possibly another
> > GPIO for turning on/off the power of PCIe slot.  Let's add the support
> > for that with another optional device tree property 'power-gpios'.
> 
> It seems that a better approach would be to add regulator support
> instead as it is more general.

It's a sensible suggestion.  I will take it in v2.  Thanks, Fabio.

Shawn
Lorenzo Pieralisi March 2, 2018, 12:10 p.m. | #5
Hi Shawn,

On Tue, Jan 23, 2018 at 09:04:37AM +0800, Shawn Guo wrote:
> From: Jianguo Sun <sunjianguo1@huawei.com>
> 
> Besides the GPIO for controlling reset, there is also possibly another
> GPIO for turning on/off the power of PCIe slot.  Let's add the support
> for that with another optional device tree property 'power-gpios'.
> 
> Signed-off-by: Jianguo Sun <sunjianguo1@huawei.com>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../bindings/pci/hisilicon-histb-pcie.txt          |  1 +
>  drivers/pci/dwc/pcie-histb.c                       | 28 +++++++++++++++++++---

Is there a dependency between this patch and this series:

https://patchwork.ozlabs.org/project/linux-pci/list/?series=31410

It does not look like, I'd apply the series but I wanted to ask
first.

This patch misses Rob's ACK.

Thanks,
Lorenzo

>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> index c84bc027930b..597397a048f8 100644
> --- a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> @@ -33,6 +33,7 @@ Required properties
>    "bus": bus reset.
>  
>  Optional properties:
> +- power-gpios: The gpio to control the power of PCIe slot.
>  - reset-gpios: The gpio to generate PCIe PERST# assert and deassert signal.
>  - phys: List of phandle and phy mode specifier, should be 0.
>  - phy-names: Must be "phy".
> diff --git a/drivers/pci/dwc/pcie-histb.c b/drivers/pci/dwc/pcie-histb.c
> index 33b01b734d7d..2a6b18619b25 100644
> --- a/drivers/pci/dwc/pcie-histb.c
> +++ b/drivers/pci/dwc/pcie-histb.c
> @@ -63,6 +63,7 @@ struct histb_pcie {
>  	struct reset_control *sys_reset;
>  	struct reset_control *bus_reset;
>  	void __iomem *ctrl;
> +	int power_gpio;
>  	int reset_gpio;
>  };
>  
> @@ -230,6 +231,8 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
>  
>  	if (gpio_is_valid(hipcie->reset_gpio))
>  		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
> +	if (gpio_is_valid(hipcie->power_gpio))
> +		gpio_set_value_cansleep(hipcie->power_gpio, 0);
>  }
>  
>  static int histb_pcie_host_enable(struct pcie_port *pp)
> @@ -240,8 +243,14 @@ static int histb_pcie_host_enable(struct pcie_port *pp)
>  	int ret;
>  
>  	/* power on PCIe device if have */
> -	if (gpio_is_valid(hipcie->reset_gpio))
> +	if (gpio_is_valid(hipcie->power_gpio))
> +		gpio_set_value_cansleep(hipcie->power_gpio, 1);
> +
> +	if (gpio_is_valid(hipcie->reset_gpio)) {
> +		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
> +		mdelay(10);
>  		gpio_set_value_cansleep(hipcie->reset_gpio, 1);
> +	}
>  
>  	ret = clk_prepare_enable(hipcie->bus_clk);
>  	if (ret) {
> @@ -335,15 +344,28 @@ static int histb_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pci->dbi_base);
>  	}
>  
> +	hipcie->power_gpio = of_get_named_gpio_flags(np,
> +				"power-gpios", 0, &of_flags);
> +	if (of_flags & OF_GPIO_ACTIVE_LOW)
> +		flag |= GPIOF_ACTIVE_LOW;
> +	if (gpio_is_valid(hipcie->power_gpio)) {
> +		ret = devm_gpio_request_one(dev, hipcie->power_gpio,
> +				flag, "PCIe device power control");
> +		if (ret) {
> +			dev_err(dev, "unable to request power gpio\n");
> +			return ret;
> +		}
> +	}
> +
>  	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");
> +				flag, "PCIe device reset control");
>  		if (ret) {
> -			dev_err(dev, "unable to request gpio\n");
> +			dev_err(dev, "unable to request reset gpio\n");
>  			return ret;
>  		}
>  	}
> -- 
> 1.9.1
>
Shawn Guo March 2, 2018, 12:36 p.m. | #6
Hi Lorenzo,

On Fri, Mar 02, 2018 at 12:10:00PM +0000, Lorenzo Pieralisi wrote:
> Hi Shawn,
> 
> On Tue, Jan 23, 2018 at 09:04:37AM +0800, Shawn Guo wrote:
> > From: Jianguo Sun <sunjianguo1@huawei.com>
> > 
> > Besides the GPIO for controlling reset, there is also possibly another
> > GPIO for turning on/off the power of PCIe slot.  Let's add the support
> > for that with another optional device tree property 'power-gpios'.
> > 
> > Signed-off-by: Jianguo Sun <sunjianguo1@huawei.com>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  .../bindings/pci/hisilicon-histb-pcie.txt          |  1 +
> >  drivers/pci/dwc/pcie-histb.c                       | 28 +++++++++++++++++++---
> 
> Is there a dependency between this patch and this series:
> 
> https://patchwork.ozlabs.org/project/linux-pci/list/?series=31410
> 
> It does not look like, I'd apply the series but I wanted to ask
> first.

This is the v1 of the same series.  Please ignore this one, and only
review the latest v3.  Sorry for the confusion.

Shawn

Patch

diff --git a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
index c84bc027930b..597397a048f8 100644
--- a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
@@ -33,6 +33,7 @@  Required properties
   "bus": bus reset.
 
 Optional properties:
+- power-gpios: The gpio to control the power of PCIe slot.
 - reset-gpios: The gpio to generate PCIe PERST# assert and deassert signal.
 - phys: List of phandle and phy mode specifier, should be 0.
 - phy-names: Must be "phy".
diff --git a/drivers/pci/dwc/pcie-histb.c b/drivers/pci/dwc/pcie-histb.c
index 33b01b734d7d..2a6b18619b25 100644
--- a/drivers/pci/dwc/pcie-histb.c
+++ b/drivers/pci/dwc/pcie-histb.c
@@ -63,6 +63,7 @@  struct histb_pcie {
 	struct reset_control *sys_reset;
 	struct reset_control *bus_reset;
 	void __iomem *ctrl;
+	int power_gpio;
 	int reset_gpio;
 };
 
@@ -230,6 +231,8 @@  static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 
 	if (gpio_is_valid(hipcie->reset_gpio))
 		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
+	if (gpio_is_valid(hipcie->power_gpio))
+		gpio_set_value_cansleep(hipcie->power_gpio, 0);
 }
 
 static int histb_pcie_host_enable(struct pcie_port *pp)
@@ -240,8 +243,14 @@  static int histb_pcie_host_enable(struct pcie_port *pp)
 	int ret;
 
 	/* power on PCIe device if have */
-	if (gpio_is_valid(hipcie->reset_gpio))
+	if (gpio_is_valid(hipcie->power_gpio))
+		gpio_set_value_cansleep(hipcie->power_gpio, 1);
+
+	if (gpio_is_valid(hipcie->reset_gpio)) {
+		gpio_set_value_cansleep(hipcie->reset_gpio, 0);
+		mdelay(10);
 		gpio_set_value_cansleep(hipcie->reset_gpio, 1);
+	}
 
 	ret = clk_prepare_enable(hipcie->bus_clk);
 	if (ret) {
@@ -335,15 +344,28 @@  static int histb_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(pci->dbi_base);
 	}
 
+	hipcie->power_gpio = of_get_named_gpio_flags(np,
+				"power-gpios", 0, &of_flags);
+	if (of_flags & OF_GPIO_ACTIVE_LOW)
+		flag |= GPIOF_ACTIVE_LOW;
+	if (gpio_is_valid(hipcie->power_gpio)) {
+		ret = devm_gpio_request_one(dev, hipcie->power_gpio,
+				flag, "PCIe device power control");
+		if (ret) {
+			dev_err(dev, "unable to request power gpio\n");
+			return ret;
+		}
+	}
+
 	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");
+				flag, "PCIe device reset control");
 		if (ret) {
-			dev_err(dev, "unable to request gpio\n");
+			dev_err(dev, "unable to request reset gpio\n");
 			return ret;
 		}
 	}