diff mbox

PCI: imx6: Add support for MX6SX LDO PCIE domain regulator

Message ID 1465344472-15703-1-git-send-email-festevam@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Fabio Estevam June 8, 2016, 12:07 a.m. UTC 
From: Fabio Estevam <fabio.estevam@nxp.com>

MX6SX has an internal LDO regulator for the PCIE domain, which needs
to be turned on for PCIE functionality.

Add support for it.

Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  2 ++
 drivers/pci/host/pci-imx6.c                        | 29 +++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

Comments

Christoph Fritz June 8, 2016, 9:35 a.m. UTC | #1 
Hi Fabio

On Tue, 2016-06-07 at 21:07 -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@nxp.com>
> 
> MX6SX has an internal LDO regulator for the PCIE domain, which needs
> to be turned on for PCIE functionality.
> 
> Add support for it.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  2 ++
>  drivers/pci/host/pci-imx6.c                        | 29 +++++++++++++++++++++-

There was a discussion doing the handling of the regulator inside the PM
backend:

> On Mon, 2016-02-15 at 07:24 +0000, Richard Zhu wrote:
> > As Lucas discussed with me before,  the GPC regulator operations should[n't] be touched in
> > Imx pcie driver at all.  These bits operations should be encapsulate into the PM system,
> >  for example, the regulator driver.

On Thu, 2016-02-18 at 12:59 +0100, Christoph Fritz wrote:
> Ok, for an initial version, I'll just leave the regulator in my
> devicetree enabled. For further enhancements I suppose
> arch/arm/mach-imx/gpc.c needs to get touched?
> 
@Richard and @Lucas: Any further hints on this?

Thanks
 -- Christoph

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam June 12, 2016, 1:51 p.m. UTC | #2 
Hi Christoph,

On Wed, Jun 8, 2016 at 6:35 AM, Christoph Fritz
<chf.fritz@googlemail.com> wrote:

> There was a discussion doing the handling of the regulator inside the PM
> backend:

Thanks for pointing me to this discussion.

>
>> On Mon, 2016-02-15 at 07:24 +0000, Richard Zhu wrote:
>> > As Lucas discussed with me before,  the GPC regulator operations should[n't] be touched in
>> > Imx pcie driver at all.  These bits operations should be encapsulate into the PM system,
>> >  for example, the regulator driver.
>
> On Thu, 2016-02-18 at 12:59 +0100, Christoph Fritz wrote:
>> Ok, for an initial version, I'll just leave the regulator in my
>> devicetree enabled. For further enhancements I suppose
>> arch/arm/mach-imx/gpc.c needs to get touched?
>>
> @Richard and @Lucas: Any further hints on this?

Yes, would appreciate some hints or examples as to how to properly
handle the PCI LDO regulator on mx6sx.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach June 13, 2016, 10:41 a.m. UTC | #3 
Hi Fabio,

Am Sonntag, den 12.06.2016, 10:51 -0300 schrieb Fabio Estevam:
> Hi Christoph,
> 
> On Wed, Jun 8, 2016 at 6:35 AM, Christoph Fritz
> <chf.fritz@googlemail.com> wrote:
> 
> > There was a discussion doing the handling of the regulator inside the PM
> > backend:
> 
> Thanks for pointing me to this discussion.
> 
> >
> >> On Mon, 2016-02-15 at 07:24 +0000, Richard Zhu wrote:
> >> > As Lucas discussed with me before,  the GPC regulator operations should[n't] be touched in
> >> > Imx pcie driver at all.  These bits operations should be encapsulate into the PM system,
> >> >  for example, the regulator driver.
> >
> > On Thu, 2016-02-18 at 12:59 +0100, Christoph Fritz wrote:
> >> Ok, for an initial version, I'll just leave the regulator in my
> >> devicetree enabled. For further enhancements I suppose
> >> arch/arm/mach-imx/gpc.c needs to get touched?
> >>
> > @Richard and @Lucas: Any further hints on this?
> 
> Yes, would appreciate some hints or examples as to how to properly
> handle the PCI LDO regulator on mx6sx.
> 
It should be handled the same way as the PU domain regulator on mx6q.
This means the regulator is a supply of the PCIe PHY power domain and
should be en-/disabled through the GPC driver.

I already posted a series to rework the GPC driver to allow adding new
power domains easily, but it was rejected by Shawn on formal grounds, as
it's mostly a single big patch to do the rework. I don't have time to
split this up further at the moment, as this is really non-trivial, but
maybe Shawn is willing to take it if someone does a proper review of the
patch.

With this series applied it should be easy to add the required power
domains and regulator handling for mx6sx.

Regards,
Lucas

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam June 14, 2016, 3:18 a.m. UTC | #4 
Hi Lucas,

On Mon, Jun 13, 2016 at 7:41 AM, Lucas Stach <l.stach@pengutronix.de> wrote:

> It should be handled the same way as the PU domain regulator on mx6q.
> This means the regulator is a supply of the PCIe PHY power domain and
> should be en-/disabled through the GPC driver.
>
> I already posted a series to rework the GPC driver to allow adding new
> power domains easily, but it was rejected by Shawn on formal grounds, as

Could you please point me to this series?

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach June 14, 2016, 8:37 a.m. UTC | #5 
Hi Fabio,

Am Dienstag, den 14.06.2016, 00:18 -0300 schrieb Fabio Estevam:
> Hi Lucas,
> 
> On Mon, Jun 13, 2016 at 7:41 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> 
> > It should be handled the same way as the PU domain regulator on mx6q.
> > This means the regulator is a supply of the PCIe PHY power domain and
> > should be en-/disabled through the GPC driver.
> >
> > I already posted a series to rework the GPC driver to allow adding new
> > power domains easily, but it was rejected by Shawn on formal grounds, as
> 
> Could you please point me to this series?
> 
Here you go:

https://patchwork.kernel.org/patch/8599181/
https://patchwork.kernel.org/patch/8599251/
https://patchwork.kernel.org/patch/8599241/

Regards,
Lucas

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

Patch

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index f3d26f47..cfb0e86 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -33,6 +33,8 @@  Optional properties:
 Additional required properties for imx6sx-pcie:
 - clock names: Must include the following additional entry:
 	- "pcie_inbound_axi"
+- pcie-phy-supply: Must point to the internal PCIE power domain regulator:
+		   <&reg_pcie>;
 
 Example:
 
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index b741a36..ad9cc8b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -27,6 +27,7 @@ 
 #include <linux/signal.h>
 #include <linux/types.h>
 #include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
 
 #include "pcie-designware.h"
 
@@ -55,6 +56,7 @@  struct imx6_pcie {
 	u32			tx_swing_full;
 	u32			tx_swing_low;
 	int			link_gen;
+	struct regulator	*phy_regulator;
 };
 
 /* PCIe Root Complex registers (memory-mapped) */
@@ -96,6 +98,8 @@  struct imx6_pcie {
 #define PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5)
 #define PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
 
+#define MX6SX_PCIE_LDO				1100000
+
 static int pcie_phy_poll_ack(void __iomem *dbi_base, int exp_val)
 {
 	u32 val;
@@ -407,11 +411,26 @@  err_pcie_phy:
 static void imx6_pcie_init_phy(struct pcie_port *pp)
 {
 	struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+	int ret;
+
+	if (imx6_pcie->variant == IMX6SX) {
+		ret = regulator_set_voltage(imx6_pcie->phy_regulator,
+					    MX6SX_PCIE_LDO, MX6SX_PCIE_LDO);
+		if (ret) {
+			dev_err(pp->dev, "failed to set pcie phy voltage.\n");
+			return ret;
+		}
+
+		ret = regulator_enable(imx6_pcie->phy_regulator);
+		if (ret) {
+			dev_err(pp->dev, "failed to enable pcie regulator.\n");
+			return;
+		}
 
-	if (imx6_pcie->variant == IMX6SX)
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX6SX_GPR12_PCIE_RX_EQ_MASK,
 				   IMX6SX_GPR12_PCIE_RX_EQ_2);
+	}
 
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 			IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
@@ -680,6 +699,14 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 				"pcie_incbound_axi clock missing or invalid\n");
 			return PTR_ERR(imx6_pcie->pcie_inbound_axi);
 		}
+
+		imx6_pcie->phy_regulator = devm_regulator_get(pp->dev,
+							      "pcie-phy");
+		if (IS_ERR(imx6_pcie->phy_regulator)) {
+			dev_err(&pdev->dev,
+				"failed to get pcie-phy regulator\n");
+			return PTR_ERR(imx6_pcie->phy_regulator);
+		}
 	}
 
 	/* Grab GPR config register range */