PCI: histb: add power control GPIO for PCIe slot

Message ID 1516669477-20151-1-git-send-email-shawn.guo@linaro.org
State New
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(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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

> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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

> > 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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

> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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;
 		}
 	}