| Message ID | 1516669477-20151-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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.
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 >
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 > >
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
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 >
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
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; } }