Discussion:
[PATCH v2 2/5] PCI: imx6: wait the clocks to stabilize after ref_en
Richard Zhu
2014-09-23 04:11:35 UTC
Permalink
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the
"clk_prepare_enable" is return. So I think it's ok to move the
usleep delay after the pcie_ref_en is set.

Signed-off-by: Richard Zhu <***@freescale.com>
---
drivers/pci/host/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 233fe8a..bc4222b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}

- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* power up core phy and enable ref clock */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);

+ /* allow the clocks to stabilize */
+ usleep_range(200, 500);
+
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value(imx6_pcie->reset_gpio, 0);
--
1.9.1
Richard Zhu
2014-09-23 04:11:34 UTC
Permalink
- enable pcie on imx6qdl sabreauto boards.

Signed-off-by: Richard Zhu <***@freescale.com>
---
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 009abd6..d6040a5 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -410,6 +410,10 @@
};
};

+&pcie {
+ status = "okay";
+};
+
&pwm3 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm3>;
--
1.9.1
Lucas Stach
2014-09-23 09:19:41 UTC
Permalink
Post by Richard Zhu
- enable pcie on imx6qdl sabreauto boards.
---
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 009abd6..d6040a5 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -410,6 +410,10 @@
};
};
+&pcie {
+ status = "okay";
+};
+
&pwm3 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm3>;
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Fabio Estevam
2014-09-23 12:40:33 UTC
Permalink
Hi Richard,
Post by Richard Zhu
- enable pcie on imx6qdl sabreauto boards.
---
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 009abd6..d6040a5 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -410,6 +410,10 @@
};
};
+&pcie {
+ status = "okay";
+};
It would be better if you could pass the PCI reset pin that comes from
the GPIO expander.
H***@freescale.com
2014-09-24 02:54:07 UTC
Permalink
Hi Fabio
-----Original Message-----
Sent: Tuesday, September 23, 2014 8:41 PM
To: Zhu Richard-R65037
R65073; Lucas Stach; Tim Harvey
Subject: Re: [PATCH v2 1/5] PCI: imx6: enable pcie on imx6qdl sabreauto
Hi Richard,
Post by Richard Zhu
- enable pcie on imx6qdl sabreauto boards.
---
arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 009abd6..d6040a5 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -410,6 +410,10 @@
};
};
+&pcie {
+ status = "okay";
+};
It would be better if you could pass the PCI reset pin that comes from the
GPIO expander.
[Richard] 6qdl sabreauto boards don't have the pcie reset gpio in the b
Fabio Estevam
2014-09-24 21:04:03 UTC
Permalink
Hi Richard,
[Richard] 6qdl sabreauto boards don't have the pcie reset gpio in the board design at all.
I have just downloaded the mx6 sabreauto board schematics from
freescale.com and it matches the one I have seen before.

You can search for the CPU_PER_RST_B signal. It is connected via R785
0 ohm resistor to PCIE_RST_B.

CPU_PER_RST_B can be controlled via MAX7310 pin IO/2.
H***@freescale.com
2014-09-25 01:21:12 UTC
Permalink
-----Original Message-----
Sent: Thursday, September 25, 2014 5:04 AM
To: Zhu Richard-R65037
R65073; Lucas Stach; Tim Harvey
Subject: Re: [PATCH v2 1/5] PCI: imx6: enable pcie on imx6qdl sabreauto
Hi Richard,
[Richard] 6qdl sabreauto boards don't have the pcie reset gpio in the board
design at all.
I have just downloaded the mx6 sabreauto board schematics from freescale.com
and it matches the one I have seen before.
You can search for the CPU_PER_RST_B signal. It is connected via R785
0 ohm resistor to PCIE_RST_B.
CPU_PER_RST_B can be controlled via MAX7310 pin IO/2.
[Richard] Yes it is. On ARD board, the PCIE_RST_B is connected to CPU_PER_RST_B.
But this is not one signal that can be controlled by PCIE module itself.
It is kicked once at the moment when the
Fabio Estevam
2014-09-25 01:39:30 UTC
Permalink
Post by H***@freescale.com
[Richard] Yes it is. On ARD board, the PCIE_RST_B is connected to CPU_PER_RST_B.
But this is not one signal that can be controlled by PCIE module itself.
Let's take imx6qdl-sabresd.dtsi for example:

&pcie {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pcie>;
reset-gpio = <&gpio7 12 0>;
status = "okay";
};

It uses GPIO7_12 for PCI reset.

For sabreauto we just need to change to something like this format:

reset-gpio = <&max7310_b 2 0>;
Post by H***@freescale.com
It is kicked once at the moment when the board is powered up.
Yes, the signal is connected to power-on and it can also be
independently controlled via MAX7310.

Anyway, no need to change this if you don't want. I can send a patch
adding the reset later :-)
H***@freescale.com
2014-09-25 02:02:11 UTC
Permalink
-----Original Message-----
Sent: Thursday, September 25, 2014 9:40 AM
To: Zhu Richard-R65037
R65073; Lucas Stach; Tim Harvey
Subject: Re: [PATCH v2 1/5] PCI: imx6: enable pcie on imx6qdl sabreauto
Post by H***@freescale.com
[Richard] Yes it is. On ARD board, the PCIE_RST_B is connected to
CPU_PER_RST_B.
Post by H***@freescale.com
But this is not one signal that can be controlled by PCIE module itself.
&pcie {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pcie>;
reset-gpio = <&gpio7 12 0>;
status = "okay";
};
It uses GPIO7_12 for PCI reset.
reset-gpio = <&max7310_b 2 0>;
Post by H***@freescale.com
It is kicked once at the moment when the board is powered up.
Yes, the signal is connected to power-on and it can also be independently
controlled via MAX7310.
Anyway, no need to change this if you don't want. I can send a patch adding
the reset later :-)
[Richard] One more dependency, this signal would be share-used by multi-modules.
I'm afraid the operations of the pcie-reset-b would bring un-except
Richard Zhu
2014-09-23 04:11:38 UTC
Permalink
- imx6sx pcie has its own standalone pcie power supply.
In order to turn on the imx6sx pcie power during
initialization. Add the pcie regulator and the gpc regmap
into the imx6sx pcie structure.
- imx6sx pcie has the new added reset mechanism, add the
reset operations into the initialization.
- Register one PM call-back, enter/exit L2 state of the ASPM
during system suspend/resume.

Signed-off-by: Richard Zhu <***@freescale.com>
---
drivers/pci/host/pci-imx6.c | 164 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 146 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index bc4222b..99ecb5d 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -18,12 +18,16 @@
#include <linux/mfd/syscon.h>
#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
#include <linux/of_gpio.h>
#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/syscore_ops.h>
#include <linux/types.h>
#include <linux/interrupt.h>

@@ -31,15 +35,30 @@

#define to_imx6_pcie(x) container_of(x, struct imx6_pcie, pp)

+/* The pcie who have standalone power domain */
+#define PCIE_PHY_HAS_PWR_DOMAIN BIT(0)
+
+struct imx_pcie_data {
+ unsigned int flags;
+};
+
+static const struct imx_pcie_data imx6sx_pcie_data = {
+ .flags = PCIE_PHY_HAS_PWR_DOMAIN,
+};
+
struct imx6_pcie {
int reset_gpio;
+ const struct imx_pcie_data *data;
struct clk *pcie_bus;
struct clk *pcie_phy;
struct clk *pcie;
struct pcie_port pp;
struct regmap *iomuxc_gpr;
+ struct regmap *gpc_ips_reg;
+ struct regulator *pcie_regulator;
void __iomem *mem_base;
};
+static struct imx6_pcie *imx6_pcie;

/* PCIe Root Complex registers (memory-mapped) */
#define PCIE_RC_LCR 0x7c
@@ -77,6 +96,11 @@ 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)

+static inline bool is_imx6sx_pcie(struct imx6_pcie *imx6_pcie)
+{
+ return imx6_pcie->data == &imx6sx_pcie_data;
+}
+
static int pcie_phy_poll_ack(void __iomem *dbi_base, int exp_val)
{
u32 val;
@@ -275,11 +299,17 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}

- /* power up core phy and enable ref clock */
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
- IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
- IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_PCIE_TEST_PD,
+ IMX6SX_GPR12_PCIE_TEST_PD_CLR);
+ } else {
+ /* power up core phy and enable ref clock */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ }

/* allow the clocks to stabilize */
usleep_range(200, 500);
@@ -290,6 +320,18 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
msleep(100);
gpio_set_value(imx6_pcie->reset_gpio, 1);
}
+
+ /*
+ * iMX6SX PCIe has the stand-alone power domain.
+ * refer to the initialization for iMX6SX PCIe,
+ * release the PCIe PHY reset here,
+ * before LTSSM enable is set.
+ */
+ if (is_imx6sx_pcie(imx6_pcie))
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST_CLR);
+
return 0;

err_pcie:
@@ -304,15 +346,38 @@ err_pcie_phy:
static void imx6_pcie_init_phy(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ int ret;
+
+ /*
+ * iMX6SX PCIe has the stand-alone power domain
+ * add the initialization here for iMX6SX PCIe.
+ */
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* Force PCIe PHY reset */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST);
+
+ regmap_update_bits(imx6_pcie->gpc_ips_reg, 0, 1 << 7, 1 << 7);
+ /* Power up PCIe PHY, ANATOP_REG_CORE offset 0x140, bit13-9 */
+ regulator_set_voltage(imx6_pcie->pcie_regulator,
+ 1100000, 1100000);
+ ret = regulator_enable(imx6_pcie->pcie_regulator);
+ if (ret)
+ dev_info(pp->dev, "failed to enable pcie regulator.\n");
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_RX_EQ_MASK, IMX6SX_GPR12_RX_EQ_2);
+ }

regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2_CLR);

/* configure constant input signal to the pcie ctrl and phy */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
+ IMX6Q_GPR12_LOS_LEVEL, IMX6Q_GPR12_LOS_LEVEL_9);

regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
IMX6Q_GPR8_TX_DEEMPH_GEN1, 0 << 0);
@@ -370,7 +435,8 @@ static int imx6_pcie_start_link(struct pcie_port *pp)

/* Start LTSSM. */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2);

ret = imx6_pcie_wait_for_link(pp);
if (ret)
@@ -546,10 +612,64 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
return 0;
}

+static const struct of_device_id imx6_pcie_of_match[] = {
+ { .compatible = "fsl,imx6q-pcie", },
+ { .compatible = "fsl,imx6sx-pcie", .data = &imx6sx_pcie_data},
+ {},
+};
+MODULE_DEVICE_TABLE(of, imx6_pcie_of_match);
+
+#ifdef CONFIG_PM_SLEEP
+static int pci_imx_suspend(void)
+{
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* PM_TURN_OFF */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ BIT(16), 1 << 16);
+ udelay(10);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ BIT(16), 0 << 16);
+ }
+
+ return 0;
+}
+
+static void pci_imx_resume(void)
+{
+ struct pcie_port *pp = &imx6_pcie->pp;
+
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* reset iMX6SX PCIe */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 1 << 18);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 0 << 18);
+
+ /*
+ * controller maybe turn off, re-configure again
+ * Set the CLASS_REV of RC CFG header to
+ * PCI_CLASS_BRIDGE_PCI
+ */
+ writel(readl(pp->dbi_base + PCI_CLASS_REVISION)
+ | (PCI_CLASS_BRIDGE_PCI << 16),
+ pp->dbi_base + PCI_CLASS_REVISION);
+
+ dw_pcie_setup_rc(pp);
+ }
+}
+
+static struct syscore_ops pci_imx_syscore_ops = {
+ .suspend = pci_imx_suspend,
+ .resume = pci_imx_resume,
+};
+#endif
+
static int __init imx6_pcie_probe(struct platform_device *pdev)
{
- struct imx6_pcie *imx6_pcie;
struct pcie_port *pp;
+ const struct of_device_id *of_id =
+ of_match_device(imx6_pcie_of_match, &pdev->dev);
struct device_node *np = pdev->dev.of_node;
struct resource *dbi_base;
int ret;
@@ -560,6 +680,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)

pp = &imx6_pcie->pp;
pp->dev = &pdev->dev;
+ imx6_pcie->data = of_id->data;

/* Added for PCI abort handling */
hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
@@ -603,9 +724,19 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return PTR_ERR(imx6_pcie->pcie);
}

- /* Grab GPR config register range */
- imx6_pcie->iomuxc_gpr =
- syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ imx6_pcie->pcie_regulator = devm_regulator_get(pp->dev, "pcie");
+
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible
+ ("fsl,imx6sx-iomuxc-gpr");
+ imx6_pcie->gpc_ips_reg =
+ syscon_regmap_lookup_by_compatible("fsl,imx6sx-gpc");
+ } else {
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible
+ ("fsl,imx6q-iomuxc-gpr");
+ }
if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
dev_err(&pdev->dev, "unable to find iomuxc registers\n");
return PTR_ERR(imx6_pcie->iomuxc_gpr);
@@ -616,6 +747,9 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return ret;

platform_set_drvdata(pdev, imx6_pcie);
+#ifdef CONFIG_PM_SLEEP
+ register_syscore_ops(&pci_imx_syscore_ops);
+#endif
return 0;
}

@@ -627,12 +761,6 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
imx6_pcie_assert_core_reset(&imx6_pcie->pp);
}

-static const struct of_device_id imx6_pcie_of_match[] = {
- { .compatible = "fsl,imx6q-pcie", },
- {},
-};
-MODULE_DEVICE_TABLE(of, imx6_pcie_of_match);
-
static struct platform_driver imx6_pcie_driver = {
.driver = {
.name = "imx6q-pcie",
--
1.9.1
Lucas Stach
2014-09-23 11:00:06 UTC
Permalink
Post by Richard Zhu
- imx6sx pcie has its own standalone pcie power supply.
In order to turn on the imx6sx pcie power during
initialization. Add the pcie regulator and the gpc regmap
into the imx6sx pcie structure.
- imx6sx pcie has the new added reset mechanism, add the
reset operations into the initialization.
- Register one PM call-back, enter/exit L2 state of the ASPM
during system suspend/resume.
---
drivers/pci/host/pci-imx6.c | 164 +++++++++++++++++++++++++++++++++++++++-----
1 file changed, 146 insertions(+), 18 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index bc4222b..99ecb5d 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -18,12 +18,16 @@
#include <linux/mfd/syscon.h>
#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
#include <linux/of_gpio.h>
#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/syscore_ops.h>
#include <linux/types.h>
#include <linux/interrupt.h>
@@ -31,15 +35,30 @@
#define to_imx6_pcie(x) container_of(x, struct imx6_pcie, pp)
+/* The pcie who have standalone power domain */
+#define PCIE_PHY_HAS_PWR_DOMAIN BIT(0)
+
+struct imx_pcie_data {
+ unsigned int flags;
+};
+
+static const struct imx_pcie_data imx6sx_pcie_data = {
+ .flags = PCIE_PHY_HAS_PWR_DOMAIN,
+};
+
You don't use this flag anywhere else so all the above is not needed if
you rewrite the below...
Post by Richard Zhu
struct imx6_pcie {
int reset_gpio;
+ const struct imx_pcie_data *data;
struct clk *pcie_bus;
struct clk *pcie_phy;
struct clk *pcie;
struct pcie_port pp;
struct regmap *iomuxc_gpr;
+ struct regmap *gpc_ips_reg;
+ struct regulator *pcie_regulator;
void __iomem *mem_base;
};
+static struct imx6_pcie *imx6_pcie;
/* PCIe Root Complex registers (memory-mapped) */
#define PCIE_RC_LCR 0x7c
@@ -77,6 +96,11 @@ 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)
+static inline bool is_imx6sx_pcie(struct imx6_pcie *imx6_pcie)
+{
+ return imx6_pcie->data == &imx6sx_pcie_data;
... to return of_device_is_compatible(np, "fsl,imx6sx-pcie");
Post by Richard Zhu
+}
+
static int pcie_phy_poll_ack(void __iomem *dbi_base, int exp_val)
{
u32 val;
@@ -275,11 +299,17 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}
- /* power up core phy and enable ref clock */
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
- IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
- IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_PCIE_TEST_PD,
+ IMX6SX_GPR12_PCIE_TEST_PD_CLR);
+ } else {
+ /* power up core phy and enable ref clock */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ }
/* allow the clocks to stabilize */
usleep_range(200, 500);
@@ -290,6 +320,18 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
msleep(100);
gpio_set_value(imx6_pcie->reset_gpio, 1);
}
+
+ /*
+ * iMX6SX PCIe has the stand-alone power domain.
+ * refer to the initialization for iMX6SX PCIe,
+ * release the PCIe PHY reset here,
+ * before LTSSM enable is set.
+ */
This comment is confusing. I don't see how this has something to do with
the power-domain. It should read something like "Release the PHY reset,
that we have set in imx6_pcie_init_phy() now."
Post by Richard Zhu
+ if (is_imx6sx_pcie(imx6_pcie))
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST_CLR);
+
return 0;
static void imx6_pcie_init_phy(struct pcie_port *pp)
{
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ int ret;
+
+ /*
+ * iMX6SX PCIe has the stand-alone power domain
+ * add the initialization here for iMX6SX PCIe.
+ */
Again this could be phrased better: "Power up the separate domain
available on i.MX6SX"
Post by Richard Zhu
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* Force PCIe PHY reset */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST);
+
+ regmap_update_bits(imx6_pcie->gpc_ips_reg, 0, 1 << 7, 1 << 7);
Magic values here. Also this is the only time we need to access
gpc_ips_reg. So if this is a prerequisite to enabling the ANATOP
regulator, I would argue it should be done in the regulator driver.
Post by Richard Zhu
+ /* Power up PCIe PHY, ANATOP_REG_CORE offset 0x140, bit13-9 */
Oh so this is actually a PHY regulator, not feeding the whole core, but
just the PHY? You could remove the comment it is clear what you are
doing from the code and the offsets are of no interest in the PCIe
driver.
Post by Richard Zhu
+ regulator_set_voltage(imx6_pcie->pcie_regulator,
+ 1100000, 1100000);
+ ret = regulator_enable(imx6_pcie->pcie_regulator);
+ if (ret)
+ dev_info(pp->dev, "failed to enable pcie regulator.\n");
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_RX_EQ_MASK, IMX6SX_GPR12_RX_EQ_2);
+ }
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2_CLR);
/* configure constant input signal to the pcie ctrl and phy */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
+ IMX6Q_GPR12_LOS_LEVEL, IMX6Q_GPR12_LOS_LEVEL_9);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
IMX6Q_GPR8_TX_DEEMPH_GEN1, 0 << 0);
@@ -370,7 +435,8 @@ static int imx6_pcie_start_link(struct pcie_port *pp)
/* Start LTSSM. */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2);
ret = imx6_pcie_wait_for_link(pp);
if (ret)
@@ -546,10 +612,64 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
return 0;
}
+static const struct of_device_id imx6_pcie_of_match[] = {
+ { .compatible = "fsl,imx6q-pcie", },
+ { .compatible = "fsl,imx6sx-pcie", .data = &imx6sx_pcie_data},
+ {},
+};
+MODULE_DEVICE_TABLE(of, imx6_pcie_of_match);
+
Why are you moving the match table? This seems like unnecessary churn to
me.
Post by Richard Zhu
+#ifdef CONFIG_PM_SLEEP
+static int pci_imx_suspend(void)
+{
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* PM_TURN_OFF */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ BIT(16), 1 << 16);
+ udelay(10);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ BIT(16), 0 << 16);
Magic numbers here. Please add defines for those.
Post by Richard Zhu
+ }
+
+ return 0;
+}
+
+static void pci_imx_resume(void)
+{
+ struct pcie_port *pp = &imx6_pcie->pp;
+
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* reset iMX6SX PCIe */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 1 << 18);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 0 << 18);
+
Again magic numbers here. Please add defines for those.
Post by Richard Zhu
+ /*
+ * controller maybe turn off, re-configure again
+ * Set the CLASS_REV of RC CFG header to
+ * PCI_CLASS_BRIDGE_PCI
+ */
+ writel(readl(pp->dbi_base + PCI_CLASS_REVISION)
+ | (PCI_CLASS_BRIDGE_PCI << 16),
+ pp->dbi_base + PCI_CLASS_REVISION);
+
Can't we just move the call to set this from dw_pcie_host_init() to
dw_pcie_setup_rc() so we don't need to do this ourselves? It seems to be
the more logical change.
Post by Richard Zhu
+ dw_pcie_setup_rc(pp);
+ }
+}
+
+static struct syscore_ops pci_imx_syscore_ops = {
+ .suspend = pci_imx_suspend,
+ .resume = pci_imx_resume,
+};
+#endif
+
Why does this need to be syscore_ops instead of dev_pm_ops?
Post by Richard Zhu
static int __init imx6_pcie_probe(struct platform_device *pdev)
{
- struct imx6_pcie *imx6_pcie;
struct pcie_port *pp;
+ const struct of_device_id *of_id =
+ of_match_device(imx6_pcie_of_match, &pdev->dev);
struct device_node *np = pdev->dev.of_node;
struct resource *dbi_base;
int ret;
@@ -560,6 +680,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
pp = &imx6_pcie->pp;
pp->dev = &pdev->dev;
+ imx6_pcie->data = of_id->data;
/* Added for PCI abort handling */
hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
@@ -603,9 +724,19 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return PTR_ERR(imx6_pcie->pcie);
}
- /* Grab GPR config register range */
- imx6_pcie->iomuxc_gpr =
- syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ imx6_pcie->pcie_regulator = devm_regulator_get(pp->dev, "pcie");
+
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible
+ ("fsl,imx6sx-iomuxc-gpr");
+ imx6_pcie->gpc_ips_reg =
+ syscon_regmap_lookup_by_compatible("fsl,imx6sx-gpc");
+ } else {
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible
+ ("fsl,imx6q-iomuxc-gpr");
+ }
if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
dev_err(&pdev->dev, "unable to find iomuxc registers\n");
return PTR_ERR(imx6_pcie->iomuxc_gpr);
@@ -616,6 +747,9 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
return ret;
platform_set_drvdata(pdev, imx6_pcie);
+#ifdef CONFIG_PM_SLEEP
+ register_syscore_ops(&pci_imx_syscore_ops);
+#endif
return 0;
}
@@ -627,12 +761,6 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
imx6_pcie_assert_core_reset(&imx6_pcie->pp);
}
-static const struct of_device_id imx6_pcie_of_match[] = {
- { .compatible = "fsl,imx6q-pcie", },
- {},
-};
-MODULE_DEVICE_TABLE(of, imx6_pcie_of_match);
-
static struct platform_driver imx6_pcie_driver = {
.driver = {
.name = "imx6q-pcie",
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
H***@freescale.com
2014-09-24 07:09:33 UTC
Permalink
-----Original Message-----
Sent: Tuesday, September 23, 2014 7:00 PM
To: Zhu Richard-R65037
Subject: Re: [PATCH v2 5/5] PCI: imx6: add imx6sx pcie support
Post by Richard Zhu
- imx6sx pcie has its own standalone pcie power supply.
In order to turn on the imx6sx pcie power during initialization. Add
the pcie regulator and the gpc regmap into the imx6sx pcie structure.
- imx6sx pcie has the new added reset mechanism, add the reset
operations into the initialization.
- Register one PM call-back, enter/exit L2 state of the ASPM during
system suspend/resume.
---
drivers/pci/host/pci-imx6.c | 164
+++++++++++++++++++++++++++++++++++++++-----
1 file changed, 146 insertions(+), 18 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index bc4222b..99ecb5d 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -18,12 +18,16 @@
#include <linux/mfd/syscon.h>
#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
#include <linux/of_gpio.h>
#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/syscore_ops.h>
#include <linux/types.h>
#include <linux/interrupt.h>
@@ -31,15 +35,30 @@
#define to_imx6_pcie(x) container_of(x, struct imx6_pcie, pp)
+/* The pcie who have standalone power domain */
+#define PCIE_PHY_HAS_PWR_DOMAIN BIT(0)
+
+struct imx_pcie_data {
+ unsigned int flags;
+};
+
+static const struct imx_pcie_data imx6sx_pcie_data = {
+ .flags = PCIE_PHY_HAS_PWR_DOMAIN,
+};
+
You don't use this flag anywhere else so all the above is not needed if you
rewrite the below...
[Richard] Your suggest is great, thanks.
Post by Richard Zhu
struct imx6_pcie {
int reset_gpio;
+ const struct imx_pcie_data *data;
struct clk *pcie_bus;
struct clk *pcie_phy;
struct clk *pcie;
struct pcie_port pp;
struct regmap *iomuxc_gpr;
+ struct regmap *gpc_ips_reg;
+ struct regulator *pcie_regulator;
void __iomem *mem_base;
};
+static struct imx6_pcie *imx6_pcie;
/* PCIe Root Complex registers (memory-mapped) */
#define PCIE_RC_LCR 0x7c
@@ -77,6 +96,11 @@ 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)
+static inline bool is_imx6sx_pcie(struct imx6_pcie *imx6_pcie) {
+ return imx6_pcie->data == &imx6sx_pcie_data;
... to return of_device_is_compatible(np, "fsl,imx6sx-pcie");
[Richard] Exactly, thanks.
Post by Richard Zhu
+}
+
static int pcie_phy_poll_ack(void __iomem *dbi_base, int exp_val) {
u32 val;
@@ -275,11 +299,17 @@ static int imx6_pcie_deassert_core_reset(struct
pcie_port *pp)
Post by Richard Zhu
goto err_pcie;
}
- /* power up core phy and enable ref clock */
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
- IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
- regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
- IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_PCIE_TEST_PD,
+ IMX6SX_GPR12_PCIE_TEST_PD_CLR);
+ } else {
+ /* power up core phy and enable ref clock */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
+ IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ }
/* allow the clocks to stabilize */
usleep_range(200, 500);
@@ -290,6 +320,18 @@ static int imx6_pcie_deassert_core_reset(struct
pcie_port *pp)
Post by Richard Zhu
msleep(100);
gpio_set_value(imx6_pcie->reset_gpio, 1);
}
+
+ /*
+ * iMX6SX PCIe has the stand-alone power domain.
+ * refer to the initialization for iMX6SX PCIe,
+ * release the PCIe PHY reset here,
+ * before LTSSM enable is set.
+ */
This comment is confusing. I don't see how this has something to do with the
power-domain. It should read something like "Release the PHY reset, that we
have set in imx6_pcie_init_phy() now."
[Richard]Ok.
Post by Richard Zhu
+ if (is_imx6sx_pcie(imx6_pcie))
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST_CLR);
+
return 0;
static void imx6_pcie_init_phy(struct pcie_port *pp) {
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+ int ret;
+
+ /*
+ * iMX6SX PCIe has the stand-alone power domain
+ * add the initialization here for iMX6SX PCIe.
+ */
Again this could be phrased better: "Power up the separate domain available on
i.MX6SX"
[Richard] Ok.
Post by Richard Zhu
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* Force PCIe PHY reset */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST);
+
+ regmap_update_bits(imx6_pcie->gpc_ips_reg, 0, 1 << 7, 1 << 7);
Magic values here. Also this is the only time we need to access gpc_ips_reg.
So if this is a prerequisite to enabling the ANATOP regulator, I would argue
it should be done in the regulator driver.
[Richard]Magic values would be replaced.
Yes, this is the only time we need to access gpc_ips_reg.
It's a little complex to add the GPC manipulations in
ANATOP/regulator framework/driver codes.
Since ANATOP regulator is common framework and driver, it's hard to manipulate
GPC bits in ANATOP/regulator driver.
In order to be easier, I add the GPC bits manipulation here directly.
How do you think about that?
Post by Richard Zhu
+ /* Power up PCIe PHY, ANATOP_REG_CORE offset 0x140, bit13-9 */
Oh so this is actually a PHY regulator, not feeding the whole core, but just
the PHY? You could remove the comment it is clear what you are doing from the
code and the offsets are of no interest in the PCIe driver.
[Richard] yes, it is a PHY regulator, not used to feed the whole core.
Ok, the comment would be removed.
Post by Richard Zhu
+ regulator_set_voltage(imx6_pcie->pcie_regulator,
+ 1100000, 1100000);
+ ret = regulator_enable(imx6_pcie->pcie_regulator);
+ if (ret)
+ dev_info(pp->dev, "failed to enable pcie regulator.\n");
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_RX_EQ_MASK, IMX6SX_GPR12_RX_EQ_2);
+ }
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2_CLR);
/* configure constant input signal to the pcie ctrl and phy */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
+ IMX6Q_GPR12_LOS_LEVEL, IMX6Q_GPR12_LOS_LEVEL_9);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
int
Post by Richard Zhu
imx6_pcie_start_link(struct pcie_port *pp)
/* Start LTSSM. */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 1 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2);
ret = imx6_pcie_wait_for_link(pp);
if (ret)
@@ -546,10 +612,64 @@ static int __init imx6_add_pcie_port(struct pcie_port
*pp,
Post by Richard Zhu
return 0;
}
+static const struct of_device_id imx6_pcie_of_match[] = {
+ { .compatible = "fsl,imx6q-pcie", },
+ { .compatible = "fsl,imx6sx-pcie", .data = &imx6sx_pcie_data},
+ {},
+};
+MODULE_DEVICE_TABLE(of, imx6_pcie_of_match);
+
Why are you moving the match table? This seems like unnecessary churn to me.
[Richard] it is used by _probe in v2 patch-set. Changes would be discarded in next version patch-set.
Post by Richard Zhu
+#ifdef CONFIG_PM_SLEEP
+static int pci_imx_suspend(void)
+{
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* PM_TURN_OFF */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ BIT(16), 1 << 16);
+ udelay(10);
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ BIT(16), 0 << 16);
Magic numbers here. Please add defines for those.
[Richard] Ok.
Post by Richard Zhu
+ }
+
+ return 0;
+}
+
+static void pci_imx_resume(void)
+{
+ struct pcie_port *pp = &imx6_pcie->pp;
+
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* reset iMX6SX PCIe */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 1 << 18);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 0 << 18);
+
Again magic numbers here. Please add defines for those.
[Richard] Ok.
Post by Richard Zhu
+ /*
+ * controller maybe turn off, re-configure again
+ * Set the CLASS_REV of RC CFG header to
+ * PCI_CLASS_BRIDGE_PCI
+ */
+ writel(readl(pp->dbi_base + PCI_CLASS_REVISION)
+ | (PCI_CLASS_BRIDGE_PCI << 16),
+ pp->dbi_base + PCI_CLASS_REVISION);
+
Can't we just move the call to set this from dw_pcie_host_init() to
dw_pcie_setup_rc() so we don't need to do this ourselves? It seems to be the
more logical change.
[Richard]dw_pcie_host_init contains the whole re-initialization and re-link-up
again of the pcie module. It's not proper to re-call dw_pcie_host_init().
I find that the msi init should be re-configured again after resume back.
So, the definitions of the "PCIE_MSI_ADDR_LO" and "PCIE_MSI_ADDR_HI" would be used
here.

BTW, do you know why the "/* Synopsis specific PCIE configuration registers */"
is not defined in pcie-designware.h, but in pcie-designware.c?
Post by Richard Zhu
+ dw_pcie_setup_rc(pp);
+ }
+}
+
+static struct syscore_ops pci_imx_syscore_ops = {
+ .suspend = pci_imx_suspend,
+ .resume = pci_imx_resume,
+};
+#endif
+
Why does this need to be syscore_ops instead of dev_pm_ops?
[Richard] PM_TURN_OFF msg should be sent out at the end of the suspend of pcie subsystem.
Resume and re-configure of rc controller should be done before the resume of pcie subsystem.
So, syscore_ops is used here.
Post by Richard Zhu
static int __init imx6_pcie_probe(struct platform_device *pdev) {
- struct imx6_pcie *imx6_pcie;
struct pcie_port *pp;
+ const struct of_device_id *of_id =
+ of_match_device(imx6_pcie_of_match, &pdev->dev);
struct device_node *np = pdev->dev.of_node;
struct resource *dbi_base;
int ret;
@@ -560,6 +680,7 @@ static int __init imx6_pcie_probe(struct
platform_device *pdev)
pp = &imx6_pcie->pp;
pp->dev = &pdev->dev;
+ imx6_pcie->data = of_id->data;
/* Added for PCI abort handling */
*pdev)
Post by Richard Zhu
return PTR_ERR(imx6_pcie->pcie);
}
- /* Grab GPR config register range */
- imx6_pcie->iomuxc_gpr =
- syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ imx6_pcie->pcie_regulator = devm_regulator_get(pp->dev, "pcie");
+
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible
+ ("fsl,imx6sx-iomuxc-gpr");
+ imx6_pcie->gpc_ips_reg =
+ syscon_regmap_lookup_by_compatible("fsl,imx6sx-gpc");
+ } else {
+ imx6_pcie->iomuxc_gpr =
+ syscon_regmap_lookup_by_compatible
+ ("fsl,imx6q-iomuxc-gpr");
+ }
if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
dev_err(&pdev->dev, "unable to find iomuxc registers\n");
int __init imx6_pcie_probe(struct platform_device *pdev)
return ret;
platform_set_drvdata(pdev, imx6_pcie);
+#ifdef CONFIG_PM_SLEEP
+ register_syscore_ops(&pci_imx_syscore_ops);
+#endif
return 0;
}
@@ -627,12 +761,6 @@ static void imx6_pcie_shutdown(struct platform_device
*pdev)
Post by Richard Zhu
imx6_pcie_assert_core_reset(&imx6_pcie->pp);
}
-static const struct of_device_id imx6_pcie_of_match[] = {
- { .compatible = "fsl,imx6q-pcie", },
- {},
-};
-MODULE_DEVICE_TABLE(of, imx6_pcie_of_match);
-
static struct platform_driver imx6_pcie_driver = {
.driver = {
.name = "imx6q-pcie",
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Best Regards
Richard Zhu
Lucas Stach
2014-09-24 09:46:02 UTC
Permalink
Am Mittwoch, den 24.09.2014, 07:09 +0000 schrieb
Hong-***@freescale.com:

[...]
Post by H***@freescale.com
Post by Richard Zhu
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* Force PCIe PHY reset */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
+ IMX6SX_GPR5_PCIE_BTNRST,
+ IMX6SX_GPR5_PCIE_BTNRST);
+
+ regmap_update_bits(imx6_pcie->gpc_ips_reg, 0, 1 << 7, 1 << 7);
Magic values here. Also this is the only time we need to access gpc_ips_reg.
So if this is a prerequisite to enabling the ANATOP regulator, I would argue
it should be done in the regulator driver.
[Richard]Magic values would be replaced.
Yes, this is the only time we need to access gpc_ips_reg.
It's a little complex to add the GPC manipulations in
ANATOP/regulator framework/driver codes.
Since ANATOP regulator is common framework and driver, it's hard to manipulate
GPC bits in ANATOP/regulator driver.
In order to be easier, I add the GPC bits manipulation here directly.
How do you think about that?
I still think it would be better to handle this in the regulator driver.
But as I don't yet have a reference manual for the imx6sx: can you
please describe what this bit does exactly? Maybe this helps me to
understand where the call should be placed.
Post by H***@freescale.com
Post by Richard Zhu
+ /* Power up PCIe PHY, ANATOP_REG_CORE offset 0x140, bit13-9 */
Oh so this is actually a PHY regulator, not feeding the whole core, but just
the PHY? You could remove the comment it is clear what you are doing from the
code and the offsets are of no interest in the PCIe driver.
[Richard] yes, it is a PHY regulator, not used to feed the whole core.
Ok, so I expect this to be called "pcie-phy-supply" in the binding.
Post by H***@freescale.com
Post by Richard Zhu
+ regulator_set_voltage(imx6_pcie->pcie_regulator,
+ 1100000, 1100000);
+ ret = regulator_enable(imx6_pcie->pcie_regulator);
+ if (ret)
+ dev_info(pp->dev, "failed to enable pcie regulator.\n");
+ regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_RX_EQ_MASK, IMX6SX_GPR12_RX_EQ_2);
+ }
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
+ IMX6Q_GPR12_PCIE_CTL_2,
+ IMX6Q_GPR12_PCIE_CTL_2_CLR);
/* configure constant input signal to the pcie ctrl and phy */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
+ IMX6Q_GPR12_LOS_LEVEL, IMX6Q_GPR12_LOS_LEVEL_9);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
[...]
Post by H***@freescale.com
Post by Richard Zhu
+static void pci_imx_resume(void)
+{
+ struct pcie_port *pp = &imx6_pcie->pp;
+
+ if (is_imx6sx_pcie(imx6_pcie)) {
+ /* reset iMX6SX PCIe */
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 1 << 18);
+
+ regmap_update_bits(imx6_pcie->iomuxc_gpr,
+ IOMUXC_GPR5, BIT(18), 0 << 18);
+
Again magic numbers here. Please add defines for those.
[Richard] Ok.
Post by Richard Zhu
+ /*
+ * controller maybe turn off, re-configure again
+ * Set the CLASS_REV of RC CFG header to
+ * PCI_CLASS_BRIDGE_PCI
+ */
+ writel(readl(pp->dbi_base + PCI_CLASS_REVISION)
+ | (PCI_CLASS_BRIDGE_PCI << 16),
+ pp->dbi_base + PCI_CLASS_REVISION);
+
Can't we just move the call to set this from dw_pcie_host_init() to
dw_pcie_setup_rc() so we don't need to do this ourselves? It seems to be the
more logical change.
[Richard]dw_pcie_host_init contains the whole re-initialization and re-link-up
again of the pcie module. It's not proper to re-call dw_pcie_host_init().
I find that the msi init should be re-configured again after resume back.
So, the definitions of the "PCIE_MSI_ADDR_LO" and "PCIE_MSI_ADDR_HI" would be used
here.
I seems you misunderstood my comment here. I'm not saying we should call
dw_pcie_host_init() here, which would be clearly wrong.

What I'm saying is that the call to set up the CLASS_REV register is
currently done in dw_pcie_host_init(), but from a quick look at the code
I think it is safe to move this call to dw_pcie_setup_rc().
If you move it to this function there would no need to do it explicitly
from this resume hook again.
Post by H***@freescale.com
BTW, do you know why the "/* Synopsis specific PCIE configuration registers */"
is not defined in pcie-designware.h, but in pcie-designware.c?
Most probably because the setup for the DW PCIe core should be handled
through pcie-designware.c and not through the individual SoC drivers.
Post by H***@freescale.com
Post by Richard Zhu
+ dw_pcie_setup_rc(pp);
+ }
+}
+
+static struct syscore_ops pci_imx_syscore_ops = {
+ .suspend = pci_imx_suspend,
+ .resume = pci_imx_resume,
+};
+#endif
+
Why does this need to be syscore_ops instead of dev_pm_ops?
[Richard] PM_TURN_OFF msg should be sent out at the end of the suspend of pcie subsystem.
Resume and re-configure of rc controller should be done before the resume of pcie subsystem.
So, syscore_ops is used here.
Ok, makes sense.

Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
H***@freescale.com
2014-09-24 10:15:10 UTC
Permalink
This post might be inappropriate. Click to display it.
Richard Zhu
2014-09-23 04:11:37 UTC
Permalink
Signed-off-by: Richard Zhu <***@freescale.com>
---
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index ff44374..f02875e 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -113,10 +113,12 @@
#define IMX6Q_GPR1_MIPI_IPU1_MUX_GASKET 0x0
#define IMX6Q_GPR1_MIPI_IPU1_MUX_IOMUX BIT(19)
#define IMX6Q_GPR1_PCIE_TEST_PD BIT(18)
+#define IMX6Q_GPR1_PCIE_TEST_PD_CLR 0x0
#define IMX6Q_GPR1_IPU_VPU_MUX_MASK BIT(17)
#define IMX6Q_GPR1_IPU_VPU_MUX_IPU1 0x0
#define IMX6Q_GPR1_IPU_VPU_MUX_IPU2 BIT(17)
#define IMX6Q_GPR1_PCIE_REF_CLK_EN BIT(16)
+#define IMX6Q_GPR1_PCIE_REF_CLK_CLR 0x0
#define IMX6Q_GPR1_USB_EXP_MODE BIT(15)
#define IMX6Q_GPR1_PCIE_INT BIT(14)
#define IMX6Q_GPR1_USB_OTG_ID_SEL_MASK BIT(13)
@@ -300,7 +302,9 @@
#define IMX6Q_GPR12_ARMP_APB_CLK_EN BIT(24)
#define IMX6Q_GPR12_DEVICE_TYPE (0xf << 12)
#define IMX6Q_GPR12_PCIE_CTL_2 BIT(10)
+#define IMX6Q_GPR12_PCIE_CTL_2_CLR 0x0
#define IMX6Q_GPR12_LOS_LEVEL (0x1f << 4)
+#define IMX6Q_GPR12_LOS_LEVEL_9 (0x9 << 4)

#define IMX6Q_GPR13_SDMA_STOP_REQ BIT(30)
#define IMX6Q_GPR13_CAN2_STOP_REQ BIT(29)
@@ -395,4 +399,14 @@
#define IMX6SL_GPR1_FEC_CLOCK_MUX1_SEL_MASK (0x3 << 17)
#define IMX6SL_GPR1_FEC_CLOCK_MUX2_SEL_MASK (0x1 << 14)

+/* For imx6sx iomux gpr register field define */
+#define IMX6SX_GPR5_PCIE_BTNRST BIT(19)
+#define IMX6SX_GPR5_PCIE_BTNRST_CLR 0x0
+#define IMX6SX_GPR5_PCIE_PERST BIT(18)
+#define IMX6SX_GPR5_PCIE_PERST_CLR 0x0
+
+#define IMX6SX_GPR12_PCIE_TEST_PD BIT(30)
+#define IMX6SX_GPR12_PCIE_TEST_PD_CLR 0x0
+#define IMX6SX_GPR12_RX_EQ_MASK (0x7 << 0)
+#define IMX6SX_GPR12_RX_EQ_2 (0x2 << 0)
#endif /* __LINUX_IMX6Q_IOMUXC_GPR_H */
--
1.9.1
Lucas Stach
2014-09-23 10:21:03 UTC
Permalink
I don't think those _CLR defines make any sense. Can we just use the
mask and a value of 0 in the regmap updates? I don't see how those
defines add any value.
Post by Richard Zhu
---
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index ff44374..f02875e 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -113,10 +113,12 @@
#define IMX6Q_GPR1_MIPI_IPU1_MUX_GASKET 0x0
#define IMX6Q_GPR1_MIPI_IPU1_MUX_IOMUX BIT(19)
#define IMX6Q_GPR1_PCIE_TEST_PD BIT(18)
+#define IMX6Q_GPR1_PCIE_TEST_PD_CLR 0x0
#define IMX6Q_GPR1_IPU_VPU_MUX_MASK BIT(17)
#define IMX6Q_GPR1_IPU_VPU_MUX_IPU1 0x0
#define IMX6Q_GPR1_IPU_VPU_MUX_IPU2 BIT(17)
#define IMX6Q_GPR1_PCIE_REF_CLK_EN BIT(16)
+#define IMX6Q_GPR1_PCIE_REF_CLK_CLR 0x0
#define IMX6Q_GPR1_USB_EXP_MODE BIT(15)
#define IMX6Q_GPR1_PCIE_INT BIT(14)
#define IMX6Q_GPR1_USB_OTG_ID_SEL_MASK BIT(13)
@@ -300,7 +302,9 @@
#define IMX6Q_GPR12_ARMP_APB_CLK_EN BIT(24)
#define IMX6Q_GPR12_DEVICE_TYPE (0xf << 12)
#define IMX6Q_GPR12_PCIE_CTL_2 BIT(10)
+#define IMX6Q_GPR12_PCIE_CTL_2_CLR 0x0
#define IMX6Q_GPR12_LOS_LEVEL (0x1f << 4)
+#define IMX6Q_GPR12_LOS_LEVEL_9 (0x9 << 4)
#define IMX6Q_GPR13_SDMA_STOP_REQ BIT(30)
#define IMX6Q_GPR13_CAN2_STOP_REQ BIT(29)
@@ -395,4 +399,14 @@
#define IMX6SL_GPR1_FEC_CLOCK_MUX1_SEL_MASK (0x3 << 17)
#define IMX6SL_GPR1_FEC_CLOCK_MUX2_SEL_MASK (0x1 << 14)
+/* For imx6sx iomux gpr register field define */
+#define IMX6SX_GPR5_PCIE_BTNRST BIT(19)
+#define IMX6SX_GPR5_PCIE_BTNRST_CLR 0x0
+#define IMX6SX_GPR5_PCIE_PERST BIT(18)
+#define IMX6SX_GPR5_PCIE_PERST_CLR 0x0
+
+#define IMX6SX_GPR12_PCIE_TEST_PD BIT(30)
+#define IMX6SX_GPR12_PCIE_TEST_PD_CLR 0x0
+#define IMX6SX_GPR12_RX_EQ_MASK (0x7 << 0)
+#define IMX6SX_GPR12_RX_EQ_2 (0x2 << 0)
#endif /* __LINUX_IMX6Q_IOMUXC_GPR_H */
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
H***@freescale.com
2014-09-24 04:45:06 UTC
Permalink
-----Original Message-----
Sent: Tuesday, September 23, 2014 6:21 PM
To: Zhu Richard-R65037
Subject: Re: [PATCH v2 4/5] PCI: imx6: add imx6sx pcie related gpr bits
definitions
I don't think those _CLR defines make any sense. Can we just use the mask and
a value of 0 in the regmap updates? I don't see how those defines add any
value.
[Richard] Ok.
Best Regards
Richard Zhu
Post by Richard Zhu
---
include/linux/mfd/syscon/imx6q-iomuxc-gpr.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index ff44374..f02875e 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -113,10 +113,12 @@
#define IMX6Q_GPR1_MIPI_IPU1_MUX_GASKET 0x0
#define IMX6Q_GPR1_MIPI_IPU1_MUX_IOMUX BIT(19)
#define IMX6Q_GPR1_PCIE_TEST_PD BIT(18)
+#define IMX6Q_GPR1_PCIE_TEST_PD_CLR 0x0
#define IMX6Q_GPR1_IPU_VPU_MUX_MASK BIT(17)
#define IMX6Q_GPR1_IPU_VPU_MUX_IPU1 0x0
#define IMX6Q_GPR1_IPU_VPU_MUX_IPU2 BIT(17)
#define IMX6Q_GPR1_PCIE_REF_CLK_EN BIT(16)
+#define IMX6Q_GPR1_PCIE_REF_CLK_CLR 0x0
#define IMX6Q_GPR1_USB_EXP_MODE BIT(15)
#define IMX6Q_GPR1_PCIE_INT BIT(14)
#define IMX6Q_GPR1_USB_OTG_ID_SEL_MASK BIT(13)
@@ -300,7 +302,9 @@
#define IMX6Q_GPR12_ARMP_APB_CLK_EN BIT(24)
#define IMX6Q_GPR12_DEVICE_TYPE (0xf << 12)
#define IMX6Q_GPR12_PCIE_CTL_2 BIT(10)
+#define IMX6Q_GPR12_PCIE_CTL_2_CLR 0x0
#define IMX6Q_GPR12_LOS_LEVEL (0x1f << 4)
+#define IMX6Q_GPR12_LOS_LEVEL_9 (0x9 << 4)
#define IMX6Q_GPR13_SDMA_STOP_REQ BIT(30)
#define IMX6Q_GPR13_CAN2_STOP_REQ BIT(29)
@@ -395,4 +399,14 @@
#define IMX6SL_GPR1_FEC_CLOCK_MUX1_SEL_MASK (0x3 << 17)
#define IMX6SL_GPR1_FEC_CLOCK_MUX2_SEL_MASK (0x1 << 14)
+/* For imx6sx iomux gpr register field define */
+#define IMX6SX_GPR5_PCIE_BTNRST BIT(19)
+#define IMX6SX_GPR5_PCIE_BTNRST_CLR 0x0
+#define IMX6SX_GPR5_PCIE_PERST BIT(18)
+#define IMX6SX_GPR5_PCIE_PERST_CLR 0x0
+
+#define IMX6SX_GPR12_PCIE_TEST_PD BIT(30)
+#define IMX6SX_GPR12_PCIE_TEST_PD_CLR 0x0
+#define IMX6SX_GPR12_RX_EQ_MASK (0x7 << 0)
+#define IMX6SX_GPR12_RX_EQ_2 (0x2 << 0)
#endif /* __LINUX_IMX6Q_IOMUXC_GPR_H */
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions |
Richard Zhu
2014-09-23 04:11:36 UTC
Permalink
- imx6sx pcie has its own power regulator.
add the pcie power suppy into dts and binding.
- enable pcie on imx6sx soc.

Signed-off-by: Richard Zhu <***@freescale.com>
---
.../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 4 ++-
arch/arm/boot/dts/imx6sx-sdb.dts | 13 +++++++++
arch/arm/boot/dts/imx6sx.dtsi | 33 +++++++++++++---------
arch/arm/mach-imx/Kconfig | 1 +
4 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 9455fd0..d3b5704 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -4,7 +4,7 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
and thus inherits all the common properties defined in designware-pcie.txt.

Required properties:
-- compatible: "fsl,imx6q-pcie"
+- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie"
- reg: base addresse and length of the pcie controller
- interrupts: A list of interrupt outputs of the controller. Must contain an
entry for each entry in the interrupt-names property.
@@ -12,6 +12,7 @@ Required properties:
- "msi": The interrupt that is asserted when an MSI is received
- clock-names: Must include the following additional entries:
- "pcie_phy"
+- regulator: regulator used by imx6sx pcie module.

Example:

@@ -35,4 +36,5 @@ Example:
<0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clks 144>, <&clks 206>, <&clks 189>;
clock-names = "pcie", "pcie_bus", "pcie_phy";
+ pcie-supply = <&reg_pcie>;
};
diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts
index a3980d9..2976913 100644
--- a/arch/arm/boot/dts/imx6sx-sdb.dts
+++ b/arch/arm/boot/dts/imx6sx-sdb.dts
@@ -251,6 +251,13 @@
};
};

+&pcie {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pcie>;
+ reset-gpio = <&gpio2 0 0>;
+ status = "okay";
+};
+
&ssi2 {
status = "okay";
};
@@ -365,6 +372,12 @@
;
};

+ pinctrl_pcie: pciegrp {
+ fsl,pins = <
+ MX6SX_PAD_ENET1_COL__GPIO2_IO_0 0x17059
+ >;
+ };
+
pinctrl_vcc_sd3: vccsd3grp {
fsl,pins = <
MX6SX_PAD_KEY_COL1__GPIO2_IO_11 0x17059
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index f4b9da6..4911160 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -689,9 +689,11 @@
};

gpc: ***@020dc000 {
- compatible = "fsl,imx6sx-gpc", "fsl,imx6q-gpc";
+ compatible = "fsl,imx6sx-gpc",
+ "fsl,imx6q-gpc", "syscon";
reg = <0x020dc000 0x4000>;
interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+ pcie-supply = <&reg_pcie>;
};

iomuxc: ***@020e0000 {
@@ -1188,20 +1190,23 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- /* configuration space */
- ranges = <0x00000800 0 0x08f00000 0x08f00000 0 0x00080000
- /* downstream I/O */
- 0x81000000 0 0 0x08f80000 0 0x00010000
- /* non-prefetchable memory */
- 0x82000000 0 0x08000000 0x08000000 0 0x00f00000>;
+ ranges = <0x00000800 0 0x01f00000 0x08f00000 0 0x00080000 /* configuration space */
+ 0x81000000 0 0 0x08f80000 0 0x00010000 /* downstream I/O */
+ 0x82000000 0 0x01000000 0x08000000 0 0x00f00000>; /* non-prefetchable memory */
num-lanes = <1>;
- interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX6SX_CLK_PCIE_REF_125M>,
- <&clks IMX6SX_CLK_PCIE_AXI>,
- <&clks IMX6SX_CLK_LVDS1_OUT>,
- <&clks IMX6SX_CLK_DISPLAY_AXI>;
- clock-names = "pcie_ref_125m", "pcie_axi",
- "lvds_gate", "display_axi";
+ interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 2 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 3 &intc GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX6SX_CLK_PCIE_AXI>,
+ <&clks IMX6SX_CLK_DISPLAY_AXI>,
+ <&clks IMX6SX_CLK_LVDS1_OUT>;
+ clock-names = "pcie", "pcie_phy", "pcie_bus";
+ pcie-supply = <&reg_pcie>;
status = "disabled";
};
};
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index be9a51a..0a055f0 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -718,6 +718,7 @@ config SOC_IMX6SL

config SOC_IMX6SX
bool "i.MX6 SoloX support"
+ select PCI_DOMAINS if PCI
select PINCTRL_IMX6SX
select SOC_IMX6
--
1.9.1
Lucas Stach
2014-09-23 10:19:21 UTC
Permalink
Post by Richard Zhu
- imx6sx pcie has its own power regulator.
add the pcie power suppy into dts and binding.
- enable pcie on imx6sx soc.
---
.../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 4 ++-
arch/arm/boot/dts/imx6sx-sdb.dts | 13 +++++++++
arch/arm/boot/dts/imx6sx.dtsi | 33 +++++++++++++---------
arch/arm/mach-imx/Kconfig | 1 +
4 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 9455fd0..d3b5704 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -4,7 +4,7 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
and thus inherits all the common properties defined in designware-pcie.txt.
-- compatible: "fsl,imx6q-pcie"
+- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie"
- reg: base addresse and length of the pcie controller
- interrupts: A list of interrupt outputs of the controller. Must contain an
entry for each entry in the interrupt-names property.
- "msi": The interrupt that is asserted when an MSI is received
- "pcie_phy"
+- regulator: regulator used by imx6sx pcie module.
There are multiple issues with this line:
It should move into it's own section that clearly states that this is a
required property only for compatible fsl,imx6sx-pcie.
It doesn't mention the actual name of the supply.
The name you are using in the example below is too broad: what is this
supply used for? Is it feeding the whole PCIe partition, or just the
PHY? In either case it should be named something like pcie-core-supply
or pcie-phy-supply. We may later add regulators that can be clearly
differentiated by their name.
Post by Richard Zhu
<0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clks 144>, <&clks 206>, <&clks 189>;
clock-names = "pcie", "pcie_bus", "pcie_phy";
+ pcie-supply = <&reg_pcie>;
};
diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts
index a3980d9..2976913 100644
--- a/arch/arm/boot/dts/imx6sx-sdb.dts
+++ b/arch/arm/boot/dts/imx6sx-sdb.dts
@@ -251,6 +251,13 @@
};
};
+&pcie {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pcie>;
+ reset-gpio = <&gpio2 0 0>;
+ status = "okay";
+};
+
This is adding PCIe support to a single board and has nothing to do with
the binding. Split out into another patch.
Post by Richard Zhu
&ssi2 {
status = "okay";
};
@@ -365,6 +372,12 @@
;
};
+ pinctrl_pcie: pciegrp {
+ fsl,pins = <
+ MX6SX_PAD_ENET1_COL__GPIO2_IO_0 0x17059
+ >;
+ };
+
pinctrl_vcc_sd3: vccsd3grp {
fsl,pins = <
MX6SX_PAD_KEY_COL1__GPIO2_IO_11 0x17059
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index f4b9da6..4911160 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -689,9 +689,11 @@
};
- compatible = "fsl,imx6sx-gpc", "fsl,imx6q-gpc";
+ compatible = "fsl,imx6sx-gpc",
+ "fsl,imx6q-gpc", "syscon";
This has nothing to do with the imx6sx binding change. Split out into
another patch with own justification.
Post by Richard Zhu
reg = <0x020dc000 0x4000>;
interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+ pcie-supply = <&reg_pcie>;
This shouldn't be here.
Post by Richard Zhu
};
@@ -1188,20 +1190,23 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- /* configuration space */
- ranges = <0x00000800 0 0x08f00000 0x08f00000 0 0x00080000
- /* downstream I/O */
- 0x81000000 0 0 0x08f80000 0 0x00010000
- /* non-prefetchable memory */
- 0x82000000 0 0x08000000 0x08000000 0 0x00f00000>;
+ ranges = <0x00000800 0 0x01f00000 0x08f00000 0 0x00080000 /* configuration space */
+ 0x81000000 0 0 0x08f80000 0 0x00010000 /* downstream I/O */
+ 0x82000000 0 0x01000000 0x08000000 0 0x00f00000>; /* non-prefetchable memory */
You are changing the configuration space here. Was it wrong before? If
so this needs to be mentioned in the commit message. Also config space
assigned in ranges is deprecated. Please add it to the regs property as
done on imx6q.
Post by Richard Zhu
num-lanes = <1>;
- interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX6SX_CLK_PCIE_REF_125M>,
- <&clks IMX6SX_CLK_PCIE_AXI>,
- <&clks IMX6SX_CLK_LVDS1_OUT>,
- <&clks IMX6SX_CLK_DISPLAY_AXI>;
- clock-names = "pcie_ref_125m", "pcie_axi",
- "lvds_gate", "display_axi";
+ interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
Again, changing something without mentioning if it was wrong before.
Post by Richard Zhu
+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 2 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 3 &intc GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX6SX_CLK_PCIE_AXI>,
+ <&clks IMX6SX_CLK_DISPLAY_AXI>,
+ <&clks IMX6SX_CLK_LVDS1_OUT>;
+ clock-names = "pcie", "pcie_phy", "pcie_bus";
Is this display_axi clock really feeding the PHY, or is it just a parent
of pcie_axi that needs to be enabled for pcie_axi to work? In that case
we need to make pcie_phy clock optional for imx6sx and model the
relationship between pcie_axi and display_axi in the clock driver.

I will not allow the enabling of clocks not directly related to the PCIe
core to creep back into this driver. It has cost me quite some time and
a binding change to correct this for imx6q.
Post by Richard Zhu
+ pcie-supply = <&reg_pcie>;
status = "disabled";
};
};
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index be9a51a..0a055f0 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -718,6 +718,7 @@ config SOC_IMX6SL
config SOC_IMX6SX
bool "i.MX6 SoloX support"
+ select PCI_DOMAINS if PCI
select PINCTRL_IMX6SX
select SOC_IMX6
This change is completely unrelated. Also I don't see why you need this.
If you need this for imx6sx please look at the linux-pci ML, Phil
Edworthy posted a patch to enable this for all ARM devices and I would
like to see your option there.

regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
H***@freescale.com
2014-09-24 09:43:39 UTC
Permalink
-----Original Message-----
Sent: Tuesday, September 23, 2014 6:19 PM
To: Zhu Richard-R65037
Subject: Re: [PATCH v2 3/5] PCI: imx6: update dts and binding for imx6sx pcie
Post by Richard Zhu
- imx6sx pcie has its own power regulator.
add the pcie power suppy into dts and binding.
- enable pcie on imx6sx soc.
---
.../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 4 ++-
arch/arm/boot/dts/imx6sx-sdb.dts | 13 +++++++++
arch/arm/boot/dts/imx6sx.dtsi | 33 +++++++++++++------
---
Post by Richard Zhu
arch/arm/mach-imx/Kconfig | 1 +
4 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 9455fd0..d3b5704 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -4,7 +4,7 @@ This PCIe host controller is based on the Synopsis
Designware PCIe IP and thus inherits all the common properties defined in
designware-pcie.txt.
Post by Richard Zhu
-- compatible: "fsl,imx6q-pcie"
+- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie"
- reg: base addresse and length of the pcie controller
- interrupts: A list of interrupt outputs of the controller. Must contain
an
Post by Richard Zhu
entry for each entry in the interrupt-names property.
- "msi": The interrupt that is asserted when an MSI is received
- "pcie_phy"
+- regulator: regulator used by imx6sx pcie module.
It should move into it's own section that clearly states that this is a
required property only for compatible fsl,imx6sx-pcie.
It doesn't mention the actual name of the supply.
The name you are using in the example below is too broad: what is this supply
used for? Is it feeding the whole PCIe partition, or just the PHY? In either
case it should be named something like pcie-core-supply or pcie-phy-supply. We
may later add regulators that can be clearly differentiated by their name.
[Richard]pcie-phy-supply property is moved into "Power supplies for imx6sx:" section.
This regulator is just feeding only the PHY. Would be renamed to pcie_phy-supply later.
Post by Richard Zhu
<0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clks 144>, <&clks 206>, <&clks 189>;
clock-names = "pcie", "pcie_bus", "pcie_phy";
+ pcie-supply = <&reg_pcie>;
};
diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts
b/arch/arm/boot/dts/imx6sx-sdb.dts
index a3980d9..2976913 100644
--- a/arch/arm/boot/dts/imx6sx-sdb.dts
+++ b/arch/arm/boot/dts/imx6sx-sdb.dts
@@ -251,6 +251,13 @@
};
};
+&pcie {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pcie>;
+ reset-gpio = <&gpio2 0 0>;
+ status = "okay";
+};
+
This is adding PCIe support to a single board and has nothing to do with the
binding. Split out into another patch.
[Richard] Ok.
Post by Richard Zhu
&ssi2 {
status = "okay";
};
@@ -365,6 +372,12 @@
;
};
+ pinctrl_pcie: pciegrp {
+ fsl,pins = <
+ MX6SX_PAD_ENET1_COL__GPIO2_IO_0 0x17059
+ >;
+ };
+
pinctrl_vcc_sd3: vccsd3grp {
fsl,pins = <
MX6SX_PAD_KEY_COL1__GPIO2_IO_11 0x17059
diff --git a/arch/arm/boot/dts/imx6sx.dtsi
b/arch/arm/boot/dts/imx6sx.dtsi index f4b9da6..4911160 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -689,9 +689,11 @@
};
- compatible = "fsl,imx6sx-gpc", "fsl,imx6q-gpc";
+ compatible = "fsl,imx6sx-gpc",
+ "fsl,imx6q-gpc", "syscon";
This has nothing to do with the imx6sx binding change. Split out into another
patch with own justification.
[Richard] Ok.
Post by Richard Zhu
reg = <0x020dc000 0x4000>;
interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+ pcie-supply = <&reg_pcie>;
This shouldn't be here.
[Richard] would be removed.
Post by Richard Zhu
};
@@ -1188,20 +1190,23 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- /* configuration space */
- ranges = <0x00000800 0 0x08f00000 0x08f00000 0 0x00080000
- /* downstream I/O */
- 0x81000000 0 0 0x08f80000 0 0x00010000
- /* non-prefetchable memory */
- 0x82000000 0 0x08000000 0x08000000 0 0x00f00000>;
+ ranges = <0x00000800 0 0x01f00000 0x08f00000 0 0x00080000 /*
configuration space */
Post by Richard Zhu
+ 0x81000000 0 0 0x08f80000 0 0x00010000 /*
downstream I/O */
Post by Richard Zhu
+ 0x82000000 0 0x01000000 0x08000000 0 0x00f00000>; /*
+non-prefetchable memory */
You are changing the configuration space here. Was it wrong before? If so this
needs to be mentioned in the commit message. Also config space assigned in
ranges is deprecated. Please add it to the regs property as done on imx6q.
[Richard] No, it isn't wrong before. My spell-mistakes here.
Would be changed later.
There are differences of the memory ranges
of pcie cfg/io/mem between imx6sx and imx6qdl.
On imx6q, the offset is 0x01xx_xxxx, on imx6sx, the offset is 0x08xx_0000.
The others are same to each other.
Post by Richard Zhu
num-lanes = <1>;
- interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
- clocks = <&clks IMX6SX_CLK_PCIE_REF_125M>,
- <&clks IMX6SX_CLK_PCIE_AXI>,
- <&clks IMX6SX_CLK_LVDS1_OUT>,
- <&clks IMX6SX_CLK_DISPLAY_AXI>;
- clock-names = "pcie_ref_125m", "pcie_axi",
- "lvds_gate", "display_axi";
+ interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
Again, changing something without mentioning if it was wrong before.
[Richard]It's not wrong before, just align with imx6qdl in 3.17-rc2 kernel.
Post by Richard Zhu
+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 2 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 3 &intc GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+ <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks IMX6SX_CLK_PCIE_AXI>,
+ <&clks IMX6SX_CLK_DISPLAY_AXI>,
+ <&clks IMX6SX_CLK_LVDS1_OUT>;
+ clock-names = "pcie", "pcie_phy", "pcie_bus";
Is this display_axi clock really feeding the PHY, or is it just a parent of
pcie_axi that needs to be enabled for pcie_axi to work? In that case we need
to make pcie_phy clock optional for imx6sx and model the relationship between
pcie_axi and display_axi in the clock driver.
I will not allow the enabling of clocks not directly related to the PCIe core
to creep back into this driver. It has cost me quite some time and a binding
change to correct this for imx6q.
[Richard]Confirmed that the display_axi is not related to pcie clk.
Only the dispay_podf is required to be opened for pcie_axi.
And it is constructed already in arch/arm/mach-imx/clk-imx6sx.c.
I'm glad that you consist to make comments on the wrong-usage of display_axi clock.
Thanks.
Post by Richard Zhu
+ pcie-supply = <&reg_pcie>;
status = "disabled";
};
};
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index be9a51a..0a055f0 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -718,6 +718,7 @@ config SOC_IMX6SL
config SOC_IMX6SX
bool "i.MX6 SoloX support"
+ select PCI_DOMAINS if PCI
select PINCTRL_IMX6SX
select SOC_IMX6
This change is completely unrelated. Also I don't see why you need this.
If you need this for imx6sx please look at the linux-pci ML, Phil Edworthy
posted a patch to enable this for all ARM devices and I would like to see your
option there.
[Richard] Would be removed.
regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutro
Lucas Stach
2014-09-23 09:18:36 UTC
Permalink
Hi Richard,
After I changed the wait clocks stabilize delay after pcie_ref_en is set
in this patch-set.
Can you help to make a double check whether it's ok or not at your side?
Thanks in advanced.
I'll go through this series today. Please give me some time to properly
comment on every patch before posting a new version of the series.
Thanks.
1. Regarding to Lucas' comments, seperated the enalbe pcie on
imx6qdl sabreauto patch.
2. Add the description why the wait clock stabilize delay should
be run after pcie_ref_en is set.
3. Return 0 directly in suspend call back.
Thanks for quick review from Lucas.
1. seperate the smashed patch-set.
2. remove the "power-on-gpio".
3. add/update the pcie-supply of the dts and binding.
4.
[PATCH v2 1/5] PCI: imx6: enable pcie on imx6qdl sabreauto
[PATCH v2 2/5] PCI: imx6: wait the clocks to stabilize after ref_en
[PATCH v2 3/5] PCI: imx6: update dts and binding for imx6sx pcie
[PATCH v2 4/5] PCI: imx6: add imx6sx pcie related gpr bits
[PATCH v2 5/5] PCI: imx6: add imx6sx pcie support
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
H***@freescale.com
2014-09-23 09:29:43 UTC
Permalink
-----Original Message-----
Sent: Tuesday, September 23, 2014 5:19 PM
To: Zhu Richard-R65037
Subject: Re: [PATCH v2]PCI: imx6: enable pcie on imx6sx sdb and imx6qdl
sabreauto.
Hi Richard,
After I changed the wait clocks stabilize delay after pcie_ref_en is
set in this patch-set.
Can you help to make a double check whether it's ok or not at your side?
Thanks in advanced.
I'll go through this series today. Please give me some time to properly
comment on every patch before posting a new version of the series.
Thanks.
Ok no problem, thanks a lot for your kindly help.

Best Regards
Richard Zhu
1. Regarding to Lucas' comments, seperated the enalbe pcie on imx6qdl
sabreauto patch.
2. Add the description why the wait clock stabilize delay should be
run after pcie_ref_en is set.
3. Return 0 directly in suspend call back.
Thanks for quick review from Lucas.
1. seperate the smashed patch-set.
2. remove the "power-on-gpio".
3. add/update the pcie-supply of the dts and binding.
4.
[PATCH v2 1/5] PCI: imx6: enable pcie on imx6qdl sabreauto [PATCH v2
2/5] PCI: imx6: wait the clocks to stabilize after ref_en [PATCH v2
3/5] PCI: imx6: update dts and binding for imx6sx pcie [PATCH v2 4/5]
add imx6sx pcie support
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | ht
Lucas Stach
2014-09-23 09:56:30 UTC
Permalink
Post by Richard Zhu
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the
"clk_prepare_enable" is return. So I think it's ok to move the
usleep delay after the pcie_ref_en is set.
You are describing a lot of the conditions around the issue, but not the
issue itself, which makes it hard to follow your commit message. After
looking at the code I think the problem is this (and should be described
accordingly):

For boards without a reset gpio we skip the delay between enabling the
pcie_ref_clk and touching the RC registers for configuration. Apparently
this hangs when the clocks are not yet settled in the DW PCIe core. So
we need to make sure that there is always an appropriate delay between
those two actions.

I have not found this constraint anywhere in the i.MX6 Reference Manual,
nor in the DW PCIe documents I have access to, which makes me a bit feel
a bit unhappy about this. Richard, do you have better info on why this
delay is needed and how long it needs to be? Or is this just empirical?

In general I'm ok with this patch, but still want a confirmation from
Tim that this doesn't break anything.
Post by Richard Zhu
---
drivers/pci/host/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 233fe8a..bc4222b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* power up core phy and enable ref clock */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ /* allow the clocks to stabilize */
+ usleep_range(200, 500);
+
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value(imx6_pcie->reset_gpio, 0);
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Tim Harvey
2014-09-23 12:28:49 UTC
Permalink
Post by Lucas Stach
Post by Richard Zhu
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the
"clk_prepare_enable" is return. So I think it's ok to move the
usleep delay after the pcie_ref_en is set.
You are describing a lot of the conditions around the issue, but not the
issue itself, which makes it hard to follow your commit message. After
looking at the code I think the problem is this (and should be described
For boards without a reset gpio we skip the delay between enabling the
pcie_ref_clk and touching the RC registers for configuration. Apparently
this hangs when the clocks are not yet settled in the DW PCIe core. So
we need to make sure that there is always an appropriate delay between
those two actions.
I have not found this constraint anywhere in the i.MX6 Reference Manual,
nor in the DW PCIe documents I have access to, which makes me a bit feel
a bit unhappy about this. Richard, do you have better info on why this
delay is needed and how long it needs to be? Or is this just empirical?
In general I'm ok with this patch, but still want a confirmation from
Tim that this doesn't break anything.
I agree with Lucas' comments and also agree that this can use some
testing. Based on my previous findings PCI link is very fragile. It
will take me a few days to get a proper test setup in a thermal
chamber with a host of boards but I will report back when I have
findings.

Tim
Post by Lucas Stach
Post by Richard Zhu
---
drivers/pci/host/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 233fe8a..bc4222b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* power up core phy and enable ref clock */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ /* allow the clocks to stabilize */
+ usleep_range(200, 500);
+
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value(imx6_pcie->reset_gpio, 0);
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
H***@freescale.com
2014-09-25 05:21:54 UTC
Permalink
-----Original Message-----
Sent: Tuesday, September 23, 2014 8:29 PM
To: Lucas Stach; Zhu Richard-R65037
R65073; Fabio Estevam
Subject: Re: [PATCH v2 2/5] PCI: imx6: wait the clocks to stabilize after
ref_en
Post by Lucas Stach
- a while delay is mandatory required after pcie_ref_clk_en is set.
Otherwise, the system would be hang on imx6qdl ard boards, because
that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the "clk_prepare_enable"
is return. So I think it's ok to move the usleep delay after the
pcie_ref_en is set.
You are describing a lot of the conditions around the issue, but not
the issue itself, which makes it hard to follow your commit message.
After looking at the code I think the problem is this (and should be
described
For boards without a reset gpio we skip the delay between enabling the
pcie_ref_clk and touching the RC registers for configuration.
Apparently this hangs when the clocks are not yet settled in the DW
PCIe core. So we need to make sure that there is always an appropriate
delay between those two actions.
[Richard] Thanks.
Post by Lucas Stach
I have not found this constraint anywhere in the i.MX6 Reference
Manual, nor in the DW PCIe documents I have access to, which makes me
a bit feel a bit unhappy about this. Richard, do you have better info
on why this delay is needed and how long it needs to be? Or is this just
empirical?
[Richard] I used to fix one less than 0.3% percentage randomly link down issue
during the warm-reset loop stress tests.
I used get some info from design team that " the async reset input need ref clock to sync internally, when the ref clock comes after reset, internal synced reset time is too short , cannot meet the requirement." "at least 4us delay is required after ref clock stable and before ssp_en is assert"
I would add about 10us delay just between clocks are stable and ssp_en is assert later.
Post by Lucas Stach
In general I'm ok with this patch, but still want a confirmation from
Tim that this doesn't break anything.
I agree with Lucas' comments and also agree that this can use some testing.
Based on my previous findings PCI link is very fragile. It will take me a few
days to get a proper test setup in a thermal chamber with a host of boards but
I will report back when I have findings.
Tim
Best Regards
Richard Zhu
Post by Lucas Stach
---
drivers/pci/host/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c
b/drivers/pci/host/pci-imx6.c index 233fe8a..bc4222b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct
pcie_port *pp)
Post by Lucas Stach
goto err_pcie;
}
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* power up core phy and enable ref clock */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ /* allow the clocks to stabilize */
+ usleep_range(200, 500);
+
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value(imx6_pcie->reset_gpio, 0);
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-pci"
info at http://vger.kernel.o
Tim Harvey
2014-10-01 18:00:46 UTC
Permalink
Post by Tim Harvey
Post by Lucas Stach
Post by Richard Zhu
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the
"clk_prepare_enable" is return. So I think it's ok to move the
usleep delay after the pcie_ref_en is set.
You are describing a lot of the conditions around the issue, but not the
issue itself, which makes it hard to follow your commit message. After
looking at the code I think the problem is this (and should be described
For boards without a reset gpio we skip the delay between enabling the
pcie_ref_clk and touching the RC registers for configuration. Apparently
this hangs when the clocks are not yet settled in the DW PCIe core. So
we need to make sure that there is always an appropriate delay between
those two actions.
I have not found this constraint anywhere in the i.MX6 Reference Manual,
nor in the DW PCIe documents I have access to, which makes me a bit feel
a bit unhappy about this. Richard, do you have better info on why this
delay is needed and how long it needs to be? Or is this just empirical?
In general I'm ok with this patch, but still want a confirmation from
Tim that this doesn't break anything.
I agree with Lucas' comments and also agree that this can use some
testing. Based on my previous findings PCI link is very fragile. It
will take me a few days to get a proper test setup in a thermal
chamber with a host of boards but I will report back when I have
findings.
Tim
Post by Lucas Stach
Post by Richard Zhu
---
drivers/pci/host/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 233fe8a..bc4222b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* power up core phy and enable ref clock */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ /* allow the clocks to stabilize */
+ usleep_range(200, 500);
+
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value(imx6_pcie->reset_gpio, 0);
I tested this across temperature over 300+ boots each on several IMX6
based boards with switches and did not encounter any link failures.

Tested-by: Tim Harvey <***@gateworks.com>
H***@freescale.com
2014-10-02 02:26:16 UTC
Permalink
Thanks Tim.

Best regards
Richard
Post by Tim Harvey
Post by Tim Harvey
Post by Lucas Stach
Post by Richard Zhu
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the
"clk_prepare_enable" is return. So I think it's ok to move the
usleep delay after the pcie_ref_en is set.
You are describing a lot of the conditions around the issue, but not the
issue itself, which makes it hard to follow your commit message. After
looking at the code I think the problem is this (and should be described
For boards without a reset gpio we skip the delay between enabling the
pcie_ref_clk and touching the RC registers for configuration. Apparently
this hangs when the clocks are not yet settled in the DW PCIe core. So
we need to make sure that there is always an appropriate delay between
those two actions.
I have not found this constraint anywhere in the i.MX6 Reference Manual,
nor in the DW PCIe documents I have access to, which makes me a bit feel
a bit unhappy about this. Richard, do you have better info on why this
delay is needed and how long it needs to be? Or is this just empirical?
In general I'm ok with this patch, but still want a confirmation from
Tim that this doesn't break anything.
I agree with Lucas' comments and also agree that this can use some
testing. Based on my previous findings PCI link is very fragile. It
will take me a few days to get a proper test setup in a thermal
chamber with a host of boards but I will report back when I have
findings.
Tim
Post by Lucas Stach
Post by Richard Zhu
---
drivers/pci/host/pci-imx6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 233fe8a..bc4222b 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -275,15 +275,15 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
goto err_pcie;
}
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* power up core phy and enable ref clock */
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18);
regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1,
IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16);
+ /* allow the clocks to stabilize */
+ usleep_range(200, 500);
+
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value(imx6_pcie->reset_gpio, 0);
I tested this across temperature over 300+ boots each on several IMX6
based boards with switches and did not encounter any link failures.
Tes
Fabio Estevam
2014-09-23 12:45:52 UTC
Permalink
Post by Richard Zhu
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
The mx6qdl sabreauto boards do have PCI reset pins. They come from the
I2C MAX7310 expander.

Yes, there are boards that do not have PCI reset GPIO, but this commit
log need to be rewritten.
Fabio Estevam
2014-10-24 01:51:14 UTC
Permalink
Hi Richard,
Post by Richard Zhu
- a while delay is mandatory required after pcie_ref_clk_en
is set. Otherwise, the system would be hang on imx6qdl ard
boards, because that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the
"clk_prepare_enable" is return. So I think it's ok to move the
usleep delay after the pcie_ref_en is set.
Tested-by: Fabio Estevam <***@freescale.com>

Without this patch we notice that the kernel does not boot anymore
since commit 3fce0e882f61 (PCI: imx6: Delay enabling reference clock
for SS until it stabilizes) on a system that does not pass the PCI
gpio reset in the dtb. This causes a regression on mx6 nitrogen
boards.

I would suggest that you resend this patch only so that it could be
applied into 3.18 as a bug fix.
R***@freescale.com
2014-10-24 02:46:11 UTC
Permalink
-----Original Message-----
Sent: Friday, October 24, 2014 9:51 AM
To: Zhu Richard-R65037
R65073; Lucas Stach; Tim Harvey; Bjorn Helgaas
Subject: Re: [PATCH v2 2/5] PCI: imx6: wait the clocks to stabilize after
ref_en
Hi Richard,
- a while delay is mandatory required after pcie_ref_clk_en is set.
Otherwise, the system would be hang on imx6qdl ard boards, because
that imx6qdl boards don't have the reset_gpio.
- the clocks should be stable already after the "clk_prepare_enable"
is return. So I think it's ok to move the usleep delay after the
pcie_ref_en is set.
Without this patch we notice that the kernel does not boot anymore since
commit 3fce0e882f61 (PCI: imx6: Delay enabling reference clock for SS until
it stabilizes) on a system that does not pass the PCI gpio reset in the dtb.
This causes a regression on mx6 nitrogen boards.
I would suggest that you resend this patch only so that it could be applied
into 3.18 as a bug fix.
Okay, I would send out the patch today.

Best Regards
Richard

Loading...