[1/2] PCI: imx6: Add power-supply support

Some PCIe ports gate power to the slot.  In order to prevent system lockup,
these boards must enable power to the slot before attempting communication
over the PCI bus.

Signed-off-by: Sean Cross <xobs@kosagi.com>
---
 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt       |  3 +++
 drivers/pci/host/pci-imx6.c                          | 20 +++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

Am Freitag, den 19.12.2014, 16:10 +0800 schrieb Sean Cross:
> Some PCIe ports gate power to the slot.  In order to prevent system lockup,
> these boards must enable power to the slot before attempting communication
> over the PCI bus.
> 
> Signed-off-by: Sean Cross <xobs@kosagi.com>
> ---
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt       |  3 +++
>  drivers/pci/host/pci-imx6.c                          | 20 +++++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 6fbba53..fe912bd 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -13,6 +13,9 @@ Required properties:
>  - clock-names: Must include the following additional entries:
>  	- "pcie_phy"
>  
> +Optional properties:
> +- power-supply: A regulator that controls power to the port

This is a way too generic name. In order to provide the functionality
you are intending, this regulator needs to supply the whole bus
(remember not every board has just a single slot). It should be named
accordingly.

So I would expect this to look something like the following:

bus-supply: A regulator that controls main power to every device on the
bus.

> +
>  Example:
>  
>  	pcie@0x01000000 {
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 69202d1..9c2140e 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -22,6 +22,7 @@
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/resource.h>
>  #include <linux/signal.h>
>  #include <linux/types.h>
> @@ -39,6 +40,7 @@ struct imx6_pcie {
>  	struct pcie_port	pp;
>  	struct regmap		*iomuxc_gpr;
>  	void __iomem		*mem_base;
> +	struct regulator	*power_reg;
>  };
>  
>  /* PCIe Root Complex registers (memory-mapped) */
> @@ -588,6 +590,19 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* Fetch power supply */
> +	imx6_pcie->power_reg = devm_regulator_get(&pdev->dev, "power");

Use devm_regulator_get_optional instead.

> +	if (IS_ERR(imx6_pcie->power_reg))
> +		imx6_pcie->power_reg = 0;

This can go away if you use the above, but really? 0 to a pointer? I
know it isn't invalid, but it's also bad style.

You must handle -EPROBE_DEFER here instead.

> +
> +	if (imx6_pcie->power_reg) {
> +		ret = regulator_enable(imx6_pcie->power_reg);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Unable to enable power regulator");
> +			return ret;
> +		}
> +	}
> +
I'm not really comfortable with enabling directly at probe time, but
given our failure paths are horrible enough as they are right now I
agree that this is the sanest solution for now.

But please move both enabling and disabling at failure into
imx6_add_pcie_port().

>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
>  	if (IS_ERR(imx6_pcie->pcie_phy)) {
> @@ -619,8 +634,11 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  	}
>  
>  	ret = imx6_add_pcie_port(pp, pdev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		if (imx6_pcie->power_reg)
> +			regulator_disable(imx6_pcie->power_reg);
>  		return ret;
> +	}
>  
>  	platform_set_drvdata(pdev, imx6_pcie);
>  	return 0;

Regards,
Lucas