| Message ID | 1411445498-20250-6-git-send-email-r65037@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb 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. > > Signed-off-by: Richard Zhu <r65037@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, > +}; > + You don't use this flag anywhere else so all the above is not needed if you rewrite the below... > 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"); > +} > + > 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." > + 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. > + */ Again this could be phrased better: "Power up the separate domain available on i.MX6SX" > + 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. > + /* 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. > + 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. > +#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. > + } > + > + 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. > + /* > + * 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. > + 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? > 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",
> -----Original Message----- > From: Lucas Stach [mailto:l.stach@pengutronix.de] > Sent: Tuesday, September 23, 2014 7:00 PM > To: Zhu Richard-R65037 > Cc: linux-pci-owner@vger.kernel.org; linux-pci@vger.kernel.org; Guo Shawn- > R65073; festevam@gmail.com; tharvey@gateworks.com > Subject: Re: [PATCH v2 5/5] PCI: imx6: add imx6sx pcie support > > Am Dienstag, den 23.09.2014, 12:11 +0800 schrieb 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. > > > > Signed-off-by: Richard Zhu <r65037@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, > > +}; > > + > > 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. > > > 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. > > +} > > + > > 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." [Richard]Ok. > > > + 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. > > + */ > > Again this could be phrased better: "Power up the separate domain available on > i.MX6SX" [Richard] Ok. > > > + 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? > > > + /* 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. > > > + 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. [Richard] it is used by _probe in v2 patch-set. Changes would be discarded in next version patch-set. > > > +#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. > > > + } > > + > > + 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. > > > + /* > > + * 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? > > > + 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. > > > 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/ | Best Regards Richard Zhu
Am Mittwoch, den 24.09.2014, 07:09 +0000 schrieb Hong-Xing.Zhu@freescale.com: [...] > > > + 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. > > > > > + /* 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. > > > > > + 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 [...] > > > +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. > > > > > + /* > > > + * 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. > 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. > > > > > + 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
> -----Original Message----- > From: Lucas Stach [mailto:l.stach@pengutronix.de] > Sent: Wednesday, September 24, 2014 5:46 PM > To: Zhu Richard-R65037 > Cc: linux-pci-owner@vger.kernel.org; linux-pci@vger.kernel.org; Guo Shawn- > R65073; festevam@gmail.com; tharvey@gateworks.com > Subject: Re: [PATCH v2 5/5] PCI: imx6: add imx6sx pcie support > > Am Mittwoch, den 24.09.2014, 07:09 +0000 schrieb > Hong-Xing.Zhu@freescale.com: > > [...] > > > > > + 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. [Richard] Hi Lucas: Here it is: Bit 7 of GPC Interface control register (GPC_CNTR), located in General Power Controller PCIE_PHY_PUP_REQ PCIE PHY power up request. Self-clear bit. NOTE: Software may directly control display power gate and utilize hardware control for reset sequence. 0 — No Request 1 — Request power up sequence > > > > > > > > > + /* 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. [Richard] Thanks. > > > > > > > > + 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 > > [...] > > > > > +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. > > > > > > > + /* > > > > + * 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. [Richard] Understood, thanks. Then, I would move the setup of the CLASS_REV register to dw_pcie_set_rc() later. > > > 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. > [Richard] Got that, thanks. > > > > > > > + 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. [Richard]Thanks. > > Regards, > Lucas > > -- > Pengutronix e.K. | Lucas Stach | > Industrial Linux Solutions | http://www.pengutronix.de/ | Best Regards Richard Zhu
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",
- 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 <r65037@freescale.com> --- drivers/pci/host/pci-imx6.c | 164 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 146 insertions(+), 18 deletions(-)