| Message ID | 1728981213-8771-8-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A bunch of changes to refine i.MX PCIe driver | expand |
On Tue, Oct 15, 2024 at 04:33:31PM +0800, Richard Zhu wrote: > From: Frank Li <Frank.Li@nxp.com> > > Call common dwc suspend/resume function. Use dwc common iATU method to send > out PME_TURN_OFF message. Old platform such as iMX6SX and iMX6QP, iATU > CTRL2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. So can't send out MSG > without data by dummy MMIO write. Without PCIE_ATU_INHIBIT_PAYLOAD, MSGD > will be sent out instead of MSG. So keep old method to send PME_TURN_OFF > MSG. > This PME_Turn_Off implementation is the only difference between the DWC common ops and the custom one here? I don't think so. Please describe all the differences. > Signed-off-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 97 ++++++++++----------------- > 1 file changed, 36 insertions(+), 61 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 161daad34a94..baa853d84b4d 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -33,6 +33,7 @@ > #include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > > +#include "../../pci.h" > #include "pcie-designware.h" > > #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9) > @@ -82,6 +83,7 @@ enum imx_pcie_variants { > #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) > #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) > #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) > +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) > > #define imx_check_flag(pci, val) (pci->drvdata->flags & val) > > @@ -106,19 +108,18 @@ struct imx_pcie_drvdata { > int (*init_phy)(struct imx_pcie *pcie); > int (*enable_ref_clk)(struct imx_pcie *pcie, bool enable); > int (*core_reset)(struct imx_pcie *pcie, bool assert); > + const struct dw_pcie_host_ops *ops; > }; > > struct imx_pcie { > struct dw_pcie *pci; > struct gpio_desc *reset_gpiod; > - bool link_is_up; > struct clk_bulk_data clks[IMX_PCIE_MAX_CLKS]; > struct regmap *iomuxc_gpr; > u16 msi_ctrl; > u32 controller_id; > struct reset_control *pciephy_reset; > struct reset_control *apps_reset; > - struct reset_control *turnoff_reset; > u32 tx_deemph_gen1; > u32 tx_deemph_gen2_3p5db; > u32 tx_deemph_gen2_6db; > @@ -898,13 +899,11 @@ static int imx_pcie_start_link(struct dw_pcie *pci) > dev_info(dev, "Link: Only Gen1 is enabled\n"); > } > > - imx_pcie->link_is_up = true; > tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS); > return 0; > > err_reset_phy: > - imx_pcie->link_is_up = false; > dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n", > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0), > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); > @@ -1023,9 +1022,33 @@ static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr) > return cpu_addr - entry->offset; > } > > +/* > + * Old dwc iATU ctrl2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. So can't > + * send out MSG without data by dummy MMIO write. Without > + * PCIE_ATU_INHIBIT_PAYLOAD, MSGD will be sent out. So have to keep old method > + * to send PME_TURN_OFF MSG. Please reword the comments: "In Old DWC implementations, PCIE_ATU_INHIBIT_PAYLOAD bit in iATU Ctrl2 register is reserved. So the generic DWC implementation of sending the PME_Turn_Off message using a dummy MMIO write cannot be used." > + */ > +static void imx_pcie_pm_turn_off(struct dw_pcie_rp *pp) s/pm/pme > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct imx_pcie *imx_pcie = to_imx_pcie(pci); > + > + regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_PM_TURN_OFF); > + regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_PM_TURN_OFF); > + > + usleep_range(PCIE_PME_TO_L2_TIMEOUT_US/10, PCIE_PME_TO_L2_TIMEOUT_US); > +} > + > + > static const struct dw_pcie_host_ops imx_pcie_host_ops = { > .init = imx_pcie_host_init, > .deinit = imx_pcie_host_exit, > + .pme_turn_off = imx_pcie_pm_turn_off, > +}; > + > +static const struct dw_pcie_host_ops imx_pcie_host_dw_pme_ops = { > + .init = imx_pcie_host_init, > + .deinit = imx_pcie_host_exit, > }; > > static const struct dw_pcie_ops dw_pcie_ops = { > @@ -1146,43 +1169,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie, > return 0; > } > > -static void imx_pcie_pm_turnoff(struct imx_pcie *imx_pcie) > -{ > - struct device *dev = imx_pcie->pci->dev; > - > - /* Some variants have a turnoff reset in DT */ > - if (imx_pcie->turnoff_reset) { > - reset_control_assert(imx_pcie->turnoff_reset); > - reset_control_deassert(imx_pcie->turnoff_reset); > - goto pm_turnoff_sleep; > - } What about this part of the code? Don't you need it now? > - > - /* Others poke directly at IOMUXC registers */ > - switch (imx_pcie->drvdata->variant) { > - case IMX6SX: > - case IMX6QP: > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > - IMX6SX_GPR12_PCIE_PM_TURN_OFF, > - IMX6SX_GPR12_PCIE_PM_TURN_OFF); > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > - IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0); > - break; > - default: > - dev_err(dev, "PME_Turn_Off not implemented\n"); > - return; > - } > - > - /* > - * Components with an upstream port must respond to > - * PME_Turn_Off with PME_TO_Ack but we can't check. > - * > - * The standard recommends a 1-10ms timeout after which to > - * proceed anyway as if acks were received. > - */ > -pm_turnoff_sleep: > - usleep_range(1000, 10000); > -} > - > static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save) > { > u8 offset; > @@ -1206,36 +1192,26 @@ static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save) > static int imx_pcie_suspend_noirq(struct device *dev) > { > struct imx_pcie *imx_pcie = dev_get_drvdata(dev); > - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; > > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) > return 0; > > imx_pcie_msi_save_restore(imx_pcie, true); > - imx_pcie_pm_turnoff(imx_pcie); > - imx_pcie_stop_link(imx_pcie->pci); > - imx_pcie_host_exit(pp); > - > - return 0; > + return dw_pcie_suspend_noirq(imx_pcie->pci); > } > > static int imx_pcie_resume_noirq(struct device *dev) > { > int ret; > struct imx_pcie *imx_pcie = dev_get_drvdata(dev); > - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; > > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) > return 0; > > - ret = imx_pcie_host_init(pp); > + ret = dw_pcie_resume_noirq(imx_pcie->pci); > if (ret) > return ret; > imx_pcie_msi_save_restore(imx_pcie, false); > - dw_pcie_setup_rc(pp); > - > - if (imx_pcie->link_is_up) > - imx_pcie_start_link(imx_pcie->pci); So this is also not needed? Why? Please explain in the commit message. - Mani
On Tue, Oct 22, 2024 at 10:48:00PM +0530, Manivannan Sadhasivam wrote: > On Tue, Oct 15, 2024 at 04:33:31PM +0800, Richard Zhu wrote: > > From: Frank Li <Frank.Li@nxp.com> > > > > Call common dwc suspend/resume function. Use dwc common iATU method to send > > out PME_TURN_OFF message. Old platform such as iMX6SX and iMX6QP, iATU > > CTRL2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. So can't send out MSG > > without data by dummy MMIO write. Without PCIE_ATU_INHIBIT_PAYLOAD, MSGD > > will be sent out instead of MSG. So keep old method to send PME_TURN_OFF > > MSG. > > > > This PME_Turn_Off implementation is the only difference between the DWC common > ops and the custom one here? I don't think so. Please describe all the > differences. > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 97 ++++++++++----------------- > > 1 file changed, 36 insertions(+), 61 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 161daad34a94..baa853d84b4d 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -33,6 +33,7 @@ > > #include <linux/pm_domain.h> > > #include <linux/pm_runtime.h> > > > > +#include "../../pci.h" > > #include "pcie-designware.h" > > > > #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9) > > @@ -82,6 +83,7 @@ enum imx_pcie_variants { > > #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) > > #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) > > #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) > > +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) > > > > #define imx_check_flag(pci, val) (pci->drvdata->flags & val) > > > > @@ -106,19 +108,18 @@ struct imx_pcie_drvdata { > > int (*init_phy)(struct imx_pcie *pcie); > > int (*enable_ref_clk)(struct imx_pcie *pcie, bool enable); > > int (*core_reset)(struct imx_pcie *pcie, bool assert); > > + const struct dw_pcie_host_ops *ops; > > }; > > > > struct imx_pcie { > > struct dw_pcie *pci; > > struct gpio_desc *reset_gpiod; > > - bool link_is_up; > > struct clk_bulk_data clks[IMX_PCIE_MAX_CLKS]; > > struct regmap *iomuxc_gpr; > > u16 msi_ctrl; > > u32 controller_id; > > struct reset_control *pciephy_reset; > > struct reset_control *apps_reset; > > - struct reset_control *turnoff_reset; > > u32 tx_deemph_gen1; > > u32 tx_deemph_gen2_3p5db; > > u32 tx_deemph_gen2_6db; > > @@ -898,13 +899,11 @@ static int imx_pcie_start_link(struct dw_pcie *pci) > > dev_info(dev, "Link: Only Gen1 is enabled\n"); > > } > > > > - imx_pcie->link_is_up = true; > > tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > > dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS); > > return 0; > > > > err_reset_phy: > > - imx_pcie->link_is_up = false; > > dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n", > > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0), > > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); > > @@ -1023,9 +1022,33 @@ static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr) > > return cpu_addr - entry->offset; > > } > > > > +/* > > + * Old dwc iATU ctrl2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. So can't > > + * send out MSG without data by dummy MMIO write. Without > > + * PCIE_ATU_INHIBIT_PAYLOAD, MSGD will be sent out. So have to keep old method > > + * to send PME_TURN_OFF MSG. > > Please reword the comments: > > "In Old DWC implementations, PCIE_ATU_INHIBIT_PAYLOAD bit in iATU Ctrl2 register > is reserved. So the generic DWC implementation of sending the PME_Turn_Off > message using a dummy MMIO write cannot be used." > > > + */ > > +static void imx_pcie_pm_turn_off(struct dw_pcie_rp *pp) > > s/pm/pme > > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > + struct imx_pcie *imx_pcie = to_imx_pcie(pci); > > + > > + regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_PM_TURN_OFF); > > + regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_PM_TURN_OFF); > > + > > + usleep_range(PCIE_PME_TO_L2_TIMEOUT_US/10, PCIE_PME_TO_L2_TIMEOUT_US); > > +} > > + > > + > > static const struct dw_pcie_host_ops imx_pcie_host_ops = { > > .init = imx_pcie_host_init, > > .deinit = imx_pcie_host_exit, > > + .pme_turn_off = imx_pcie_pm_turn_off, > > +}; > > + > > +static const struct dw_pcie_host_ops imx_pcie_host_dw_pme_ops = { > > + .init = imx_pcie_host_init, > > + .deinit = imx_pcie_host_exit, > > }; > > > > static const struct dw_pcie_ops dw_pcie_ops = { > > @@ -1146,43 +1169,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie, > > return 0; > > } > > > > -static void imx_pcie_pm_turnoff(struct imx_pcie *imx_pcie) > > -{ > > - struct device *dev = imx_pcie->pci->dev; > > - > > - /* Some variants have a turnoff reset in DT */ > > - if (imx_pcie->turnoff_reset) { > > - reset_control_assert(imx_pcie->turnoff_reset); > > - reset_control_deassert(imx_pcie->turnoff_reset); > > - goto pm_turnoff_sleep; > > - } > > What about this part of the code? Don't you need it now? Don't need it, previous wrongly use reset interface to do send pme turnoff operate. Now dwc common driver can do it. > > > - > > - /* Others poke directly at IOMUXC registers */ > > - switch (imx_pcie->drvdata->variant) { > > - case IMX6SX: > > - case IMX6QP: > > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > - IMX6SX_GPR12_PCIE_PM_TURN_OFF, > > - IMX6SX_GPR12_PCIE_PM_TURN_OFF); > > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > - IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0); > > - break; > > - default: > > - dev_err(dev, "PME_Turn_Off not implemented\n"); > > - return; > > - } > > - > > - /* > > - * Components with an upstream port must respond to > > - * PME_Turn_Off with PME_TO_Ack but we can't check. > > - * > > - * The standard recommends a 1-10ms timeout after which to > > - * proceed anyway as if acks were received. > > - */ > > -pm_turnoff_sleep: > > - usleep_range(1000, 10000); > > -} > > - > > static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save) > > { > > u8 offset; > > @@ -1206,36 +1192,26 @@ static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save) > > static int imx_pcie_suspend_noirq(struct device *dev) > > { > > struct imx_pcie *imx_pcie = dev_get_drvdata(dev); > > - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; > > > > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) > > return 0; > > > > imx_pcie_msi_save_restore(imx_pcie, true); > > - imx_pcie_pm_turnoff(imx_pcie); > > - imx_pcie_stop_link(imx_pcie->pci); > > - imx_pcie_host_exit(pp); > > - > > - return 0; > > + return dw_pcie_suspend_noirq(imx_pcie->pci); > > } > > > > static int imx_pcie_resume_noirq(struct device *dev) > > { > > int ret; > > struct imx_pcie *imx_pcie = dev_get_drvdata(dev); > > - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; > > > > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) > > return 0; > > > > - ret = imx_pcie_host_init(pp); > > + ret = dw_pcie_resume_noirq(imx_pcie->pci); > > if (ret) > > return ret; > > imx_pcie_msi_save_restore(imx_pcie, false); > > - dw_pcie_setup_rc(pp); > > - > > - if (imx_pcie->link_is_up) > > - imx_pcie_start_link(imx_pcie->pci); > > So this is also not needed? Why? Please explain in the commit message. dw_pcie_resume_noirq() dw_pcie_start_link() imx_pcie_start_link() Will add in commit message. > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
> -----Original Message----- > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Sent: 2024年10月23日 1:18 > To: Hongxing Zhu <hongxing.zhu@nxp.com> > Cc: kw@linux.com; bhelgaas@google.com; lpieralisi@kernel.org; Frank Li > <frank.li@nxp.com>; l.stach@pengutronix.de; robh+dt@kernel.org; > conor+dt@kernel.org; shawnguo@kernel.org; > krzysztof.kozlowski+dt@linaro.org; festevam@gmail.com; > s.hauer@pengutronix.de; linux-pci@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; kernel@pengutronix.de; imx@lists.linux.dev > Subject: Re: [PATCH v4 7/9] PCI: imx6: Use dwc common suspend resume > method > > On Tue, Oct 15, 2024 at 04:33:31PM +0800, Richard Zhu wrote: > > From: Frank Li <Frank.Li@nxp.com> > > > > Call common dwc suspend/resume function. Use dwc common iATU > method to > > send out PME_TURN_OFF message. Old platform such as iMX6SX and > iMX6QP, > > iATU > > CTRL2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. So can't send > > out MSG without data by dummy MMIO write. Without > > PCIE_ATU_INHIBIT_PAYLOAD, MSGD will be sent out instead of MSG. So > > keep old method to send PME_TURN_OFF MSG. > > > > This PME_Turn_Off implementation is the only difference between the DWC > common ops and the custom one here? I don't think so. Please describe all > the differences. > Hi Manivannan: Thanks for your comments. On i.MX6SX and i.MX6QP platforms, the PME_TURN_OFF would be issued by the hooked callback pp.ops->pme_turn_off(). They are same to the previous operations. In the other words, these two platforms don't use the dwc generic PME_TURN_OFF implementation. Do you means to describe all the differences between dwc common suspend/resume and custom suspend/resume in commit message? In dwc suspend, the l1ss and link stats are checked before suspend entry. And after PME_TURN_OFF is issued, the L2 stat is polling. These two differences are not contained in the custom PM operations. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 97 > > ++++++++++----------------- > > 1 file changed, 36 insertions(+), 61 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 161daad34a94..baa853d84b4d 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -33,6 +33,7 @@ > > #include <linux/pm_domain.h> > > #include <linux/pm_runtime.h> > > > > +#include "../../pci.h" > > #include "pcie-designware.h" > > > > #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9) > > @@ -82,6 +83,7 @@ enum imx_pcie_variants { > > #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) > > #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) > > #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) > > +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) > > > > #define imx_check_flag(pci, val) (pci->drvdata->flags & val) > > > > @@ -106,19 +108,18 @@ struct imx_pcie_drvdata { > > int (*init_phy)(struct imx_pcie *pcie); > > int (*enable_ref_clk)(struct imx_pcie *pcie, bool enable); > > int (*core_reset)(struct imx_pcie *pcie, bool assert); > > + const struct dw_pcie_host_ops *ops; > > }; > > > > struct imx_pcie { > > struct dw_pcie *pci; > > struct gpio_desc *reset_gpiod; > > - bool link_is_up; > > struct clk_bulk_data clks[IMX_PCIE_MAX_CLKS]; > > struct regmap *iomuxc_gpr; > > u16 msi_ctrl; > > u32 controller_id; > > struct reset_control *pciephy_reset; > > struct reset_control *apps_reset; > > - struct reset_control *turnoff_reset; > > u32 tx_deemph_gen1; > > u32 tx_deemph_gen2_3p5db; > > u32 tx_deemph_gen2_6db; > > @@ -898,13 +899,11 @@ static int imx_pcie_start_link(struct dw_pcie > *pci) > > dev_info(dev, "Link: Only Gen1 is enabled\n"); > > } > > > > - imx_pcie->link_is_up = true; > > tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > > dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS); > > return 0; > > > > err_reset_phy: > > - imx_pcie->link_is_up = false; > > dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n", > > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0), > > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); @@ -1023,9 > +1022,33 @@ > > static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr) > > return cpu_addr - entry->offset; > > } > > > > +/* > > + * Old dwc iATU ctrl2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. > > +So can't > > + * send out MSG without data by dummy MMIO write. Without > > + * PCIE_ATU_INHIBIT_PAYLOAD, MSGD will be sent out. So have to keep > > +old method > > + * to send PME_TURN_OFF MSG. > > Please reword the comments: > > "In Old DWC implementations, PCIE_ATU_INHIBIT_PAYLOAD bit in iATU Ctrl2 > register is reserved. So the generic DWC implementation of sending the > PME_Turn_Off message using a dummy MMIO write cannot be used." > > > + */ > > +static void imx_pcie_pm_turn_off(struct dw_pcie_rp *pp) > > s/pm/pme > > > +{ > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > + struct imx_pcie *imx_pcie = to_imx_pcie(pci); > > + > > + regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > IMX6SX_GPR12_PCIE_PM_TURN_OFF); > > + regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > +IMX6SX_GPR12_PCIE_PM_TURN_OFF); > > + > > + usleep_range(PCIE_PME_TO_L2_TIMEOUT_US/10, > > +PCIE_PME_TO_L2_TIMEOUT_US); } > > + > > + > > static const struct dw_pcie_host_ops imx_pcie_host_ops = { > > .init = imx_pcie_host_init, > > .deinit = imx_pcie_host_exit, > > + .pme_turn_off = imx_pcie_pm_turn_off, }; > > + > > +static const struct dw_pcie_host_ops imx_pcie_host_dw_pme_ops = { > > + .init = imx_pcie_host_init, > > + .deinit = imx_pcie_host_exit, > > }; > > > > static const struct dw_pcie_ops dw_pcie_ops = { @@ -1146,43 +1169,6 > > @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie, > > return 0; > > } > > > > -static void imx_pcie_pm_turnoff(struct imx_pcie *imx_pcie) -{ > > - struct device *dev = imx_pcie->pci->dev; > > - > > - /* Some variants have a turnoff reset in DT */ > > - if (imx_pcie->turnoff_reset) { > > - reset_control_assert(imx_pcie->turnoff_reset); > > - reset_control_deassert(imx_pcie->turnoff_reset); > > - goto pm_turnoff_sleep; > > - } > > What about this part of the code? Don't you need it now? > > > - > > - /* Others poke directly at IOMUXC registers */ > > - switch (imx_pcie->drvdata->variant) { > > - case IMX6SX: > > - case IMX6QP: > > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > - IMX6SX_GPR12_PCIE_PM_TURN_OFF, > > - IMX6SX_GPR12_PCIE_PM_TURN_OFF); > > - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > - IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0); > > - break; > > - default: > > - dev_err(dev, "PME_Turn_Off not implemented\n"); > > - return; > > - } > > - > > - /* > > - * Components with an upstream port must respond to > > - * PME_Turn_Off with PME_TO_Ack but we can't check. > > - * > > - * The standard recommends a 1-10ms timeout after which to > > - * proceed anyway as if acks were received. > > - */ > > -pm_turnoff_sleep: > > - usleep_range(1000, 10000); > > -} > > - > > static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool > > save) { > > u8 offset; > > @@ -1206,36 +1192,26 @@ static void imx_pcie_msi_save_restore(struct > > imx_pcie *imx_pcie, bool save) static int > > imx_pcie_suspend_noirq(struct device *dev) { > > struct imx_pcie *imx_pcie = dev_get_drvdata(dev); > > - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; > > > > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) > > return 0; > > > > imx_pcie_msi_save_restore(imx_pcie, true); > > - imx_pcie_pm_turnoff(imx_pcie); > > - imx_pcie_stop_link(imx_pcie->pci); > > - imx_pcie_host_exit(pp); > > - > > - return 0; > > + return dw_pcie_suspend_noirq(imx_pcie->pci); > > } > > > > static int imx_pcie_resume_noirq(struct device *dev) { > > int ret; > > struct imx_pcie *imx_pcie = dev_get_drvdata(dev); > > - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; > > > > if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) > > return 0; > > > > - ret = imx_pcie_host_init(pp); > > + ret = dw_pcie_resume_noirq(imx_pcie->pci); > > if (ret) > > return ret; > > imx_pcie_msi_save_restore(imx_pcie, false); > > - dw_pcie_setup_rc(pp); > > - > > - if (imx_pcie->link_is_up) > > - imx_pcie_start_link(imx_pcie->pci); > > So this is also not needed? Why? Please explain in the commit message. These codes would be invoked from dw_pcie_resume_noirq() too. Frank had pointed the invoke procedure. Would add them into commit message later. Best Regards Richard > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 161daad34a94..baa853d84b4d 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -33,6 +33,7 @@ #include <linux/pm_domain.h> #include <linux/pm_runtime.h> +#include "../../pci.h" #include "pcie-designware.h" #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9) @@ -82,6 +83,7 @@ enum imx_pcie_variants { #define IMX_PCIE_FLAG_HAS_SERDES BIT(6) #define IMX_PCIE_FLAG_SUPPORT_64BIT BIT(7) #define IMX_PCIE_FLAG_CPU_ADDR_FIXUP BIT(8) +#define IMX_PCIE_FLAG_CUSTOM_PME_TURNOFF BIT(9) #define imx_check_flag(pci, val) (pci->drvdata->flags & val) @@ -106,19 +108,18 @@ struct imx_pcie_drvdata { int (*init_phy)(struct imx_pcie *pcie); int (*enable_ref_clk)(struct imx_pcie *pcie, bool enable); int (*core_reset)(struct imx_pcie *pcie, bool assert); + const struct dw_pcie_host_ops *ops; }; struct imx_pcie { struct dw_pcie *pci; struct gpio_desc *reset_gpiod; - bool link_is_up; struct clk_bulk_data clks[IMX_PCIE_MAX_CLKS]; struct regmap *iomuxc_gpr; u16 msi_ctrl; u32 controller_id; struct reset_control *pciephy_reset; struct reset_control *apps_reset; - struct reset_control *turnoff_reset; u32 tx_deemph_gen1; u32 tx_deemph_gen2_3p5db; u32 tx_deemph_gen2_6db; @@ -898,13 +899,11 @@ static int imx_pcie_start_link(struct dw_pcie *pci) dev_info(dev, "Link: Only Gen1 is enabled\n"); } - imx_pcie->link_is_up = true; tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS); return 0; err_reset_phy: - imx_pcie->link_is_up = false; dev_dbg(dev, "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n", dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0), dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); @@ -1023,9 +1022,33 @@ static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr) return cpu_addr - entry->offset; } +/* + * Old dwc iATU ctrl2 bit 22 (PCIE_ATU_INHIBIT_PAYLOAD) are reserved. So can't + * send out MSG without data by dummy MMIO write. Without + * PCIE_ATU_INHIBIT_PAYLOAD, MSGD will be sent out. So have to keep old method + * to send PME_TURN_OFF MSG. + */ +static void imx_pcie_pm_turn_off(struct dw_pcie_rp *pp) +{ + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct imx_pcie *imx_pcie = to_imx_pcie(pci); + + regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_PM_TURN_OFF); + regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX6SX_GPR12_PCIE_PM_TURN_OFF); + + usleep_range(PCIE_PME_TO_L2_TIMEOUT_US/10, PCIE_PME_TO_L2_TIMEOUT_US); +} + + static const struct dw_pcie_host_ops imx_pcie_host_ops = { .init = imx_pcie_host_init, .deinit = imx_pcie_host_exit, + .pme_turn_off = imx_pcie_pm_turn_off, +}; + +static const struct dw_pcie_host_ops imx_pcie_host_dw_pme_ops = { + .init = imx_pcie_host_init, + .deinit = imx_pcie_host_exit, }; static const struct dw_pcie_ops dw_pcie_ops = { @@ -1146,43 +1169,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie, return 0; } -static void imx_pcie_pm_turnoff(struct imx_pcie *imx_pcie) -{ - struct device *dev = imx_pcie->pci->dev; - - /* Some variants have a turnoff reset in DT */ - if (imx_pcie->turnoff_reset) { - reset_control_assert(imx_pcie->turnoff_reset); - reset_control_deassert(imx_pcie->turnoff_reset); - goto pm_turnoff_sleep; - } - - /* Others poke directly at IOMUXC registers */ - switch (imx_pcie->drvdata->variant) { - case IMX6SX: - case IMX6QP: - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, - IMX6SX_GPR12_PCIE_PM_TURN_OFF, - IMX6SX_GPR12_PCIE_PM_TURN_OFF); - regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, - IMX6SX_GPR12_PCIE_PM_TURN_OFF, 0); - break; - default: - dev_err(dev, "PME_Turn_Off not implemented\n"); - return; - } - - /* - * Components with an upstream port must respond to - * PME_Turn_Off with PME_TO_Ack but we can't check. - * - * The standard recommends a 1-10ms timeout after which to - * proceed anyway as if acks were received. - */ -pm_turnoff_sleep: - usleep_range(1000, 10000); -} - static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save) { u8 offset; @@ -1206,36 +1192,26 @@ static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save) static int imx_pcie_suspend_noirq(struct device *dev) { struct imx_pcie *imx_pcie = dev_get_drvdata(dev); - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) return 0; imx_pcie_msi_save_restore(imx_pcie, true); - imx_pcie_pm_turnoff(imx_pcie); - imx_pcie_stop_link(imx_pcie->pci); - imx_pcie_host_exit(pp); - - return 0; + return dw_pcie_suspend_noirq(imx_pcie->pci); } static int imx_pcie_resume_noirq(struct device *dev) { int ret; struct imx_pcie *imx_pcie = dev_get_drvdata(dev); - struct dw_pcie_rp *pp = &imx_pcie->pci->pp; if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_SUPPORTS_SUSPEND)) return 0; - ret = imx_pcie_host_init(pp); + ret = dw_pcie_resume_noirq(imx_pcie->pci); if (ret) return ret; imx_pcie_msi_save_restore(imx_pcie, false); - dw_pcie_setup_rc(pp); - - if (imx_pcie->link_is_up) - imx_pcie_start_link(imx_pcie->pci); return 0; } @@ -1267,11 +1243,14 @@ static int imx_pcie_probe(struct platform_device *pdev) pci->dev = dev; pci->ops = &dw_pcie_ops; - pci->pp.ops = &imx_pcie_host_ops; imx_pcie->pci = pci; imx_pcie->drvdata = of_device_get_match_data(dev); + pci->pp.ops = &imx_pcie_host_dw_pme_ops; + if (imx_pcie->drvdata->ops) + pci->pp.ops = imx_pcie->drvdata->ops; + /* Find the PHY if one is defined, only imx7d uses it */ np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0); if (np) { @@ -1340,13 +1319,6 @@ static int imx_pcie_probe(struct platform_device *pdev) break; } - /* Grab turnoff reset */ - imx_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff"); - if (IS_ERR(imx_pcie->turnoff_reset)) { - dev_err(dev, "Failed to get TURNOFF reset control\n"); - return PTR_ERR(imx_pcie->turnoff_reset); - } - if (imx_pcie->drvdata->gpr) { /* Grab GPR config register range */ imx_pcie->iomuxc_gpr = @@ -1425,6 +1397,7 @@ static int imx_pcie_probe(struct platform_device *pdev) if (ret < 0) return ret; } else { + pci->pp.use_atu_msg = true; ret = dw_pcie_host_init(&pci->pp); if (ret < 0) return ret; @@ -1488,6 +1461,7 @@ static const struct imx_pcie_drvdata drvdata[] = { .init_phy = imx6sx_pcie_init_phy, .enable_ref_clk = imx6sx_pcie_enable_ref_clk, .core_reset = imx6sx_pcie_core_reset, + .ops = &imx_pcie_host_ops, }, [IMX6QP] = { .variant = IMX6QP, @@ -1505,6 +1479,7 @@ static const struct imx_pcie_drvdata drvdata[] = { .init_phy = imx_pcie_init_phy, .enable_ref_clk = imx6q_pcie_enable_ref_clk, .core_reset = imx6qp_pcie_core_reset, + .ops = &imx_pcie_host_ops, }, [IMX7D] = { .variant = IMX7D,