[PATCH v4 0/9] A bunch of changes to refine i.MX PCIe driver

[PATCH v4 0/9] A bunch of changes to refine i.MX PCIe driver

[Richard Zhu]

A bunch of changes to refine i.MX PCIe driver.
- Add ref clock gate for i.MX95 PCIe by #1, #2 and #9 patches.
  The changes of clock part are here [1].
  [1] https://lkml.org/lkml/2024/10/15/390
- #3 and #4 patches clean i.MX PCIe driver by removing useless codes.
  Patch #3 depends on dts changes. And the dts changes had been applied
  by Shawn, there is no dependecy now.
- Make core reset and enable_ref_clk symmetric for i.MX PCIe driver by
  #5 and #6 patches.
- Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q and
  i.MX95 PCIe PM supports by #7 and #8 patches.

v4 changes:
It's my fault that I missing Manivanna in the reviewer list.
I'm sorry about that.
- Rebase to v6.12-rc3, and resolve the dtsi conflictions.
  Add Manivanna into reviewer list.

v3 changes:
- Update EP binding refer to comments provided by Krzysztof Kozlowski.
  Thanks.

v2 changes:
- Add the reasons why one more clock is added for i.MX95 PCIe in patch #1.
- Add the "Reviewed-by: Frank Li <Frank.Li@nxp.com>" into patch #2, #4, #5,
  #6, #8 and #9.

[PATCH v4 1/9] dt-bindings: imx6q-pcie: Add ref clock for i.MX95 PCIe
[PATCH v4 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe
[PATCH v4 3/9] PCI: imx6: Fetch dbi2 and iATU base addesses from DT
[PATCH v4 4/9] PCI: imx6: Correct controller_id generation logic for
[PATCH v4 5/9] PCI: imx6: Make core reset assertion deassertion
[PATCH v4 6/9] PCI: imx6: Make *_enable_ref_clk() function symmetric
[PATCH v4 7/9] PCI: imx6: Use dwc common suspend resume method
[PATCH v4 8/9] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 PCIe PM
[PATCH v4 9/9] arm64: dts: imx95: Add ref clock for i.MX95 PCIe

Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml |   4 +-
Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml     |   1 +
Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml        |  25 ++++++++++--
arch/arm64/boot/dts/freescale/imx95.dtsi                         |  18 +++++++--
drivers/pci/controller/dwc/pci-imx6.c                            | 166 +++++++++++++++++++++++++++-------------------------------------------------
5 files changed, 97 insertions(+), 117 deletions(-)

[PATCH v4 1/9] dt-bindings: imx6q-pcie: Add ref clock for i.MX95 PCIe RC

[Richard Zhu]

Previous reference clock of i.MX95 PCIe RC is on when system boot to
kernel. But boot firmware change the behavor, it is off when boot. So it
needs be turn on when it is used. Also it needs be turn off/on when suspend
and resume.

Add one ref clock for i.MX95 PCIe RC. Increase clocks' maxItems to 5 and keep
the same restriction with other compatible string.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/pci/fsl,imx6q-pcie-common.yaml   |  4 +--
 .../bindings/pci/fsl,imx6q-pcie-ep.yaml       |  1 +
 .../bindings/pci/fsl,imx6q-pcie.yaml          | 25 ++++++++++++++++---
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
index a8b34f58f8f4..cddbe21f99f2 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml
@@ -17,11 +17,11 @@ description:
 properties:
   clocks:
     minItems: 3
-    maxItems: 4
+    maxItems: 5
 
   clock-names:
     minItems: 3
-    maxItems: 4
+    maxItems: 5
 
   num-lanes:
     const: 1
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
index 84ca12e8b25b..f41f704c6729 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
@@ -103,6 +103,7 @@ allOf:
       properties:
         clocks:
           minItems: 4
+          maxItems: 4
         clock-names:
           items:
             - const: pcie
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index 1e05c560d797..4c76cd3f98a9 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -40,10 +40,11 @@ properties:
       - description: PCIe PHY clock.
       - description: Additional required clock entry for imx6sx-pcie,
            imx6sx-pcie-ep, imx8mq-pcie, imx8mq-pcie-ep.
+      - description: PCIe reference clock.
 
   clock-names:
     minItems: 3
-    maxItems: 4
+    maxItems: 5
 
   interrupts:
     items:
@@ -127,7 +128,7 @@ allOf:
     then:
       properties:
         clocks:
-          minItems: 4
+          maxItems: 4
         clock-names:
           items:
             - const: pcie
@@ -140,11 +141,10 @@ allOf:
         compatible:
           enum:
             - fsl,imx8mq-pcie
-            - fsl,imx95-pcie
     then:
       properties:
         clocks:
-          minItems: 4
+          maxItems: 4
         clock-names:
           items:
             - const: pcie
@@ -200,6 +200,23 @@ allOf:
             - const: mstr
             - const: slv
 
+  - if:
+      properties:
+        compatible:
+          enum:
+            - fsl,imx95-pcie
+    then:
+      properties:
+        clocks:
+          maxItems: 5
+        clock-names:
+          items:
+            - const: pcie
+            - const: pcie_bus
+            - const: pcie_phy
+            - const: pcie_aux
+            - const: ref
+
 unevaluatedProperties: false
 
 examples:
-- 
2.37.1

[PATCH v4 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe

[Richard Zhu]

Add "ref" clock to enable reference clock.

If use external clock, ref clock should point to external reference.

If use internal clock, CREF_EN in LAST_TO_REG controls reference output,
which implement in drivers/clk/imx/clk-imx95-blk-ctl.c.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 808d1f105417..52a8b2dc828a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1480,6 +1480,7 @@ static const char * const imx8mm_clks[] = {"pcie_bus", "pcie", "pcie_aux"};
 static const char * const imx8mq_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"};
 static const char * const imx6sx_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"};
 static const char * const imx8q_clks[] = {"mstr", "slv", "dbi"};
+static const char * const imx95_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux", "ref"};
 
 static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX6Q] = {
@@ -1593,8 +1594,8 @@ static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX95] = {
 		.variant = IMX95,
 		.flags = IMX_PCIE_FLAG_HAS_SERDES,
-		.clk_names = imx8mq_clks,
-		.clks_cnt = ARRAY_SIZE(imx8mq_clks),
+		.clk_names = imx95_clks,
+		.clks_cnt = ARRAY_SIZE(imx95_clks),
 		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
 		.ltssm_mask = IMX95_PCIE_LTSSM_EN,
 		.mode_off[0]  = IMX95_PE0_GEN_CTRL_1,
-- 
2.37.1

[PATCH v4 3/9] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Richard Zhu]

Since dbi2 and atu regs are added for i.MX8M PCIes. Fetch the dbi2 and iATU
base addresses from DT directly, and remove the useless codes.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 52a8b2dc828a..2ae6fa4b5d32 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1113,7 +1113,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
 			   struct platform_device *pdev)
 {
 	int ret;
-	unsigned int pcie_dbi2_offset;
 	struct dw_pcie_ep *ep;
 	struct dw_pcie *pci = imx_pcie->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
@@ -1123,25 +1122,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
 	ep = &pci->ep;
 	ep->ops = &pcie_ep_ops;
 
-	switch (imx_pcie->drvdata->variant) {
-	case IMX8MQ_EP:
-	case IMX8MM_EP:
-	case IMX8MP_EP:
-		pcie_dbi2_offset = SZ_1M;
-		break;
-	default:
-		pcie_dbi2_offset = SZ_4K;
-		break;
-	}
-
-	pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
-
-	/*
-	 * FIXME: Ideally, dbi2 base address should come from DT. But since only IMX95 is defining
-	 * "dbi2" in DT, "dbi_base2" is set to NULL here for that platform alone so that the DWC
-	 * core code can fetch that from DT. But once all platform DTs were fixed, this and the
-	 * above "dbi_base2" setting should be removed.
-	 */
 	if (device_property_match_string(dev, "reg-names", "dbi2") >= 0)
 		pci->dbi_base2 = NULL;
 
-- 
2.37.1

[PATCH v4 4/9] PCI: imx6: Correct controller_id generation logic for i.MX7D

[Richard Zhu]

i.MX7D only has one PCIe controller, so controller_id should always be 0.
The previous code is incorrect although yielding the correct result. Fix by
removing IMX7D from the switch case branch.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 2ae6fa4b5d32..ca8714c625fe 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1338,7 +1338,6 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	switch (imx_pcie->drvdata->variant) {
 	case IMX8MQ:
 	case IMX8MQ_EP:
-	case IMX7D:
 		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
 			imx_pcie->controller_id = 1;
 		break;
-- 
2.37.1

[PATCH v4 5/9] PCI: imx6: Make core reset assertion deassertion symmetric

[Richard Zhu]

Add apps_reset deassertion in the imx_pcie_deassert_core_reset(). Let it be
symmetric with imx_pcie_assert_core_reset().

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index ca8714c625fe..93e2bcf9aa0a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -770,6 +770,7 @@ static void imx_pcie_assert_core_reset(struct imx_pcie *imx_pcie)
 static int imx_pcie_deassert_core_reset(struct imx_pcie *imx_pcie)
 {
 	reset_control_deassert(imx_pcie->pciephy_reset);
+	reset_control_deassert(imx_pcie->apps_reset);
 
 	if (imx_pcie->drvdata->core_reset)
 		imx_pcie->drvdata->core_reset(imx_pcie, false);
-- 
2.37.1

[PATCH v4 6/9] PCI: imx6: Make *_enable_ref_clk() function symmetric

[Richard Zhu]

Ensure the *_enable_ref_clk() function is symmetric by addressing missing
disable parts on some platforms. Also, remove the duplicate
imx7d_pcie_init_phy() function as it is the same as
imx7d_pcie_enable_ref_clk().

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 33 +++++++++++----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 93e2bcf9aa0a..161daad34a94 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -388,13 +388,6 @@ static int imx8mq_pcie_init_phy(struct imx_pcie *imx_pcie)
 	return 0;
 }
 
-static int imx7d_pcie_init_phy(struct imx_pcie *imx_pcie)
-{
-	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
-
-	return 0;
-}
-
 static int imx_pcie_init_phy(struct imx_pcie *imx_pcie)
 {
 	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
@@ -593,13 +586,13 @@ static int imx_pcie_attach_pd(struct device *dev)
 
 static int imx6sx_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
 {
-	if (enable)
-		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				  IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
-
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
+			   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
+			   enable ? 0 : IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
 	return 0;
 }
 
+
 static int imx6q_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
 {
 	if (enable) {
@@ -625,19 +618,20 @@ static int imx8mm_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
 {
 	int offset = imx_pcie_grp_offset(imx_pcie);
 
-	if (enable) {
-		regmap_clear_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE);
-		regmap_set_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
-	}
-
+	regmap_update_bits(imx_pcie->iomuxc_gpr, offset,
+			   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
+			   enable ? 0 : IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE);
+	regmap_update_bits(imx_pcie->iomuxc_gpr, offset,
+			   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
+			   enable ? IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN : 0);
 	return 0;
 }
 
 static int imx7d_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
 {
-	if (!enable)
-		regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
+			   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+			   enable ? 0 : IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
 	return 0;
 }
 
@@ -1522,7 +1516,6 @@ static const struct imx_pcie_drvdata drvdata[] = {
 		.clks_cnt = ARRAY_SIZE(imx6q_clks),
 		.mode_off[0] = IOMUXC_GPR12,
 		.mode_mask[0] = IMX6Q_GPR12_DEVICE_TYPE,
-		.init_phy = imx7d_pcie_init_phy,
 		.enable_ref_clk = imx7d_pcie_enable_ref_clk,
 		.core_reset = imx7d_pcie_core_reset,
 	},
-- 
2.37.1

[PATCH v4 7/9] PCI: imx6: Use dwc common suspend resume method

[Richard Zhu]

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.

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.
+ */
+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,
-- 
2.37.1

[PATCH v4 8/9] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 PCIe PM support

[Richard Zhu]

Add iMX8MQ i.MX8Q and i.MX95 PCIe suspend/resume support.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index baa853d84b4d..1f873ac6eed2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1497,7 +1497,8 @@ static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX8MQ] = {
 		.variant = IMX8MQ,
 		.flags = IMX_PCIE_FLAG_HAS_APP_RESET |
-			 IMX_PCIE_FLAG_HAS_PHY_RESET,
+			 IMX_PCIE_FLAG_HAS_PHY_RESET |
+			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
 		.gpr = "fsl,imx8mq-iomuxc-gpr",
 		.clk_names = imx8mq_clks,
 		.clks_cnt = ARRAY_SIZE(imx8mq_clks),
@@ -1535,13 +1536,15 @@ static const struct imx_pcie_drvdata drvdata[] = {
 	[IMX8Q] = {
 		.variant = IMX8Q,
 		.flags = IMX_PCIE_FLAG_HAS_PHYDRV |
-			 IMX_PCIE_FLAG_CPU_ADDR_FIXUP,
+			 IMX_PCIE_FLAG_CPU_ADDR_FIXUP |
+			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
 		.clk_names = imx8q_clks,
 		.clks_cnt = ARRAY_SIZE(imx8q_clks),
 	},
 	[IMX95] = {
 		.variant = IMX95,
-		.flags = IMX_PCIE_FLAG_HAS_SERDES,
+		.flags = IMX_PCIE_FLAG_HAS_SERDES |
+			 IMX_PCIE_FLAG_SUPPORTS_SUSPEND,
 		.clk_names = imx95_clks,
 		.clks_cnt = ARRAY_SIZE(imx95_clks),
 		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
-- 
2.37.1

[PATCH v4 9/9] arm64: dts: imx95: Add ref clock for i.MX95 PCIe

[Richard Zhu]

Add ref clock for i.MX95 PCIe.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx95.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
index 03661e76550f..5cb504b5f851 100644
--- a/arch/arm64/boot/dts/freescale/imx95.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
@@ -1473,6 +1473,14 @@ smmu: iommu@490d0000 {
 			};
 		};
 
+		hsio_blk_ctl: syscon@4c0100c0 {
+			compatible = "nxp,imx95-hsio-blk-ctl", "syscon";
+			reg = <0x0 0x4c0100c0 0x0 0x4>;
+			#clock-cells = <1>;
+			clocks = <&dummy>;
+			power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
+		};
+
 		pcie0: pcie@4c300000 {
 			compatible = "fsl,imx95-pcie";
 			reg = <0 0x4c300000 0 0x10000>,
@@ -1500,8 +1508,9 @@ pcie0: pcie@4c300000 {
 			clocks = <&scmi_clk IMX95_CLK_HSIO>,
 				 <&scmi_clk IMX95_CLK_HSIOPLL>,
 				 <&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
-				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
-			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux";
+				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>,
+				 <&hsio_blk_ctl 0>;
+			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux", "ref";
 			assigned-clocks =<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
 					 <&scmi_clk IMX95_CLK_HSIOPLL>,
 					 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
@@ -1528,8 +1537,9 @@ pcie0_ep: pcie-ep@4c300000 {
 			clocks = <&scmi_clk IMX95_CLK_HSIO>,
 				 <&scmi_clk IMX95_CLK_HSIOPLL>,
 				 <&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
-				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
-			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux";
+				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>,
+				 <&hsio_blk_ctl 0>;
+			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux", "ref";
 			assigned-clocks =<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
 					 <&scmi_clk IMX95_CLK_HSIOPLL>,
 					 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
-- 
2.37.1

RE: [PATCH v4 0/9] A bunch of changes to refine i.MX PCIe driver

[Hongxing Zhu]

Hi Manivannan:
Can you help to review this series patches?
Thanks in advanced.

Best Regards
Richard Zhu

> -----Original Message-----
> From: Richard Zhu <hongxing.zhu@nxp.com>
> Sent: 2024年10月15日 16:33
> To: kw@linux.com; manivannan.sadhasivam@linaro.org;
> 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
> Cc: Hongxing Zhu <hongxing.zhu@nxp.com>; 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: [PATCH v4 0/9] A bunch of changes to refine i.MX PCIe driver
>
> A bunch of changes to refine i.MX PCIe driver.
> - Add ref clock gate for i.MX95 PCIe by #1, #2 and #9 patches.
>   The changes of clock part are here [1].
>   [1]
> https://lkml.or/
> g%2Flkml%2F2024%2F10%2F15%2F390&data=05%7C02%7Chongxing.zhu%4
> 0nxp.com%7Cc7634ee6fb8640ab102808dcecf76207%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638645794454239228%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=wAIxX5kGS3OZS33GfDVdo%2FNOf
> AxWxsA6Sc%2FAOlXkuzM%3D&reserved=0
> - #3 and #4 patches clean i.MX PCIe driver by removing useless codes.
>   Patch #3 depends on dts changes. And the dts changes had been applied
>   by Shawn, there is no dependecy now.
> - Make core reset and enable_ref_clk symmetric for i.MX PCIe driver by
>   #5 and #6 patches.
> - Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q
> and
>   i.MX95 PCIe PM supports by #7 and #8 patches.
>
> v4 changes:
> It's my fault that I missing Manivanna in the reviewer list.
> I'm sorry about that.
> - Rebase to v6.12-rc3, and resolve the dtsi conflictions.
>   Add Manivanna into reviewer list.
>
> v3 changes:
> - Update EP binding refer to comments provided by Krzysztof Kozlowski.
>   Thanks.
>
> v2 changes:
> - Add the reasons why one more clock is added for i.MX95 PCIe in patch #1.
> - Add the "Reviewed-by: Frank Li <Frank.Li@nxp.com>" into patch #2, #4, #5,
>   #6, #8 and #9.
>
> [PATCH v4 1/9] dt-bindings: imx6q-pcie: Add ref clock for i.MX95 PCIe [PATCH
> v4 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe [PATCH v4 3/9] PCI: imx6:
> Fetch dbi2 and iATU base addesses from DT [PATCH v4 4/9] PCI: imx6: Correct
> controller_id generation logic for [PATCH v4 5/9] PCI: imx6: Make core reset
> assertion deassertion [PATCH v4 6/9] PCI: imx6: Make *_enable_ref_clk()
> function symmetric [PATCH v4 7/9] PCI: imx6: Use dwc common suspend
> resume method [PATCH v4 8/9] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95
> PCIe PM [PATCH v4 9/9] arm64: dts: imx95: Add ref clock for i.MX95 PCIe
>
> Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-common.yaml |   4
> +-
> Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml     |   1 +
> Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml        |  25
> ++++++++++--
> arch/arm64/boot/dts/freescale/imx95.dtsi                         |
> 18 +++++++--
> drivers/pci/controller/dwc/pci-imx6.c                            |
> 166
> +++++++++++++++++++++++++++-------------------------------------------------
> 5 files changed, 97 insertions(+), 117 deletions(-)

Re: [PATCH v4 1/9] dt-bindings: imx6q-pcie: Add ref clock for i.MX95 PCIe RC

[Bjorn Helgaas]

On Tue, Oct 15, 2024 at 04:33:25PM +0800, Richard Zhu wrote:
> Previous reference clock of i.MX95 PCIe RC is on when system boot to
> kernel. But boot firmware change the behavor, it is off when boot. So it
> needs be turn on when it is used. Also it needs be turn off/on when suspend
> and resume.

I think this background would make more sense in patch 2.  IIUC,
that's where the driver behavior changes to do something with the
"ref" clock.

I'm not sure how to interpret "Previous reference clock of i.MX95 PCIe
RC is on when system boot to kernel. But boot firmware change the
behavor, it is off when boot."

Does that mean a previous version of the boot firmware left the ref
clock on at handoff to the OS, and newer firmware turns it off?  If
so, I think it would be useful to include information about the
relevant firmware versions.

> Add one ref clock for i.MX95 PCIe RC. Increase clocks' maxItems to 5 and keep
> the same restriction with other compatible string.

Re: [PATCH v4 1/9] dt-bindings: imx6q-pcie: Add ref clock for i.MX95 PCIe RC

[Frank Li]

On Fri, Oct 18, 2024 at 06:13:05PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 15, 2024 at 04:33:25PM +0800, Richard Zhu wrote:
> > Previous reference clock of i.MX95 PCIe RC is on when system boot to
> > kernel. But boot firmware change the behavor, it is off when boot. So it
> > needs be turn on when it is used. Also it needs be turn off/on when suspend
> > and resume.
>
> I think this background would make more sense in patch 2.  IIUC,
> that's where the driver behavior changes to do something with the
> "ref" clock.

Yes, use "ref" clock are more reasonable because we have not consider
external osc clock case at beggining.

>
> I'm not sure how to interpret "Previous reference clock of i.MX95 PCIe
> RC is on when system boot to kernel. But boot firmware change the
> behavor, it is off when boot."
>
> Does that mean a previous version of the boot firmware left the ref
> clock on at handoff to the OS, and newer firmware turns it off?  If
> so, I think it would be useful to include information about the
> relevant firmware versions.

i.MX95 is quite new. previous version should be 'preview' boot firmware.
The production version turn ref clock off. Can we simple said "preview"
version. Most people should use production version, which is general
avaible for public.

Frank
>
> > Add one ref clock for i.MX95 PCIe RC. Increase clocks' maxItems to 5 and keep
> > the same restriction with other compatible string.

Re: [PATCH v4 1/9] dt-bindings: imx6q-pcie: Add ref clock for i.MX95 PCIe RC

[Manivannan Sadhasivam]

On Fri, Oct 18, 2024 at 06:13:05PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 15, 2024 at 04:33:25PM +0800, Richard Zhu wrote:
> > Previous reference clock of i.MX95 PCIe RC is on when system boot to
> > kernel. But boot firmware change the behavor, it is off when boot. So it
> > needs be turn on when it is used. Also it needs be turn off/on when suspend
> > and resume.
> 
> I think this background would make more sense in patch 2.  IIUC,
> that's where the driver behavior changes to do something with the
> "ref" clock.
> 
> I'm not sure how to interpret "Previous reference clock of i.MX95 PCIe
> RC is on when system boot to kernel. But boot firmware change the
> behavor, it is off when boot."
> 
> Does that mean a previous version of the boot firmware left the ref
> clock on at handoff to the OS, and newer firmware turns it off?  If
> so, I think it would be useful to include information about the
> relevant firmware versions.
> 

Most likely that the bootloader *used to* turn on the reference clock and now it
decides not to do so (for whatever reason). But ideally, the clock should be
voted/enabled by the kernel irrespective of that. So the reference of 'boot
firmware' is not relevant here.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 04:33:26PM +0800, Richard Zhu wrote:
> Add "ref" clock to enable reference clock.
> 
> If use external clock, ref clock should point to external reference.
> 
> If use internal clock, CREF_EN in LAST_TO_REG controls reference output,
> which implement in drivers/clk/imx/clk-imx95-blk-ctl.c.
> 

So this means the driver won't work with old devicetrees. Am I right? Then you
are breaking the DT compatibility.

You should make the clock optional in the driver.

- Mani

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 808d1f105417..52a8b2dc828a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1480,6 +1480,7 @@ static const char * const imx8mm_clks[] = {"pcie_bus", "pcie", "pcie_aux"};
>  static const char * const imx8mq_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"};
>  static const char * const imx6sx_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"};
>  static const char * const imx8q_clks[] = {"mstr", "slv", "dbi"};
> +static const char * const imx95_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux", "ref"};
>  
>  static const struct imx_pcie_drvdata drvdata[] = {
>  	[IMX6Q] = {
> @@ -1593,8 +1594,8 @@ static const struct imx_pcie_drvdata drvdata[] = {
>  	[IMX95] = {
>  		.variant = IMX95,
>  		.flags = IMX_PCIE_FLAG_HAS_SERDES,
> -		.clk_names = imx8mq_clks,
> -		.clks_cnt = ARRAY_SIZE(imx8mq_clks),
> +		.clk_names = imx95_clks,
> +		.clks_cnt = ARRAY_SIZE(imx95_clks),
>  		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
>  		.ltssm_mask = IMX95_PCIE_LTSSM_EN,
>  		.mode_off[0]  = IMX95_PE0_GEN_CTRL_1,
> -- 
> 2.37.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/9] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 04:33:27PM +0800, Richard Zhu wrote:
> Since dbi2 and atu regs are added for i.MX8M PCIes. Fetch the dbi2 and iATU
> base addresses from DT directly, and remove the useless codes.
> 

Again, what will happen to old dts that don't define these regions?

- Mani

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 20 --------------------
>  1 file changed, 20 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 52a8b2dc828a..2ae6fa4b5d32 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1113,7 +1113,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
>  			   struct platform_device *pdev)
>  {
>  	int ret;
> -	unsigned int pcie_dbi2_offset;
>  	struct dw_pcie_ep *ep;
>  	struct dw_pcie *pci = imx_pcie->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
> @@ -1123,25 +1122,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
>  	ep = &pci->ep;
>  	ep->ops = &pcie_ep_ops;
>  
> -	switch (imx_pcie->drvdata->variant) {
> -	case IMX8MQ_EP:
> -	case IMX8MM_EP:
> -	case IMX8MP_EP:
> -		pcie_dbi2_offset = SZ_1M;
> -		break;
> -	default:
> -		pcie_dbi2_offset = SZ_4K;
> -		break;
> -	}
> -
> -	pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
> -
> -	/*
> -	 * FIXME: Ideally, dbi2 base address should come from DT. But since only IMX95 is defining
> -	 * "dbi2" in DT, "dbi_base2" is set to NULL here for that platform alone so that the DWC
> -	 * core code can fetch that from DT. But once all platform DTs were fixed, this and the
> -	 * above "dbi_base2" setting should be removed.
> -	 */
>  	if (device_property_match_string(dev, "reg-names", "dbi2") >= 0)
>  		pci->dbi_base2 = NULL;
>  
> -- 
> 2.37.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 4/9] PCI: imx6: Correct controller_id generation logic for i.MX7D

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 04:33:28PM +0800, Richard Zhu wrote:
> i.MX7D only has one PCIe controller, so controller_id should always be 0.
> The previous code is incorrect although yielding the correct result. Fix by
> removing IMX7D from the switch case branch.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 2ae6fa4b5d32..ca8714c625fe 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1338,7 +1338,6 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	switch (imx_pcie->drvdata->variant) {
>  	case IMX8MQ:
>  	case IMX8MQ_EP:
> -	case IMX7D:
>  		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)

This is just *wrong*. You cannot hardcode the MMIO address in the driver. Even
though this code is old, you should fix it instead of building on top of it.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 5/9] PCI: imx6: Make core reset assertion deassertion symmetric

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 04:33:29PM +0800, Richard Zhu wrote:
> Add apps_reset deassertion in the imx_pcie_deassert_core_reset(). Let it be
> symmetric with imx_pcie_assert_core_reset().
> 

Am I correctly interpreting that this patch adds the *missing* reset deassert
for 'apps reset' line?

If so, then there should be a relevant Fixes tag. Also, there should be some
info about the implications of not deasserting it.

- Mani

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index ca8714c625fe..93e2bcf9aa0a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -770,6 +770,7 @@ static void imx_pcie_assert_core_reset(struct imx_pcie *imx_pcie)
>  static int imx_pcie_deassert_core_reset(struct imx_pcie *imx_pcie)
>  {
>  	reset_control_deassert(imx_pcie->pciephy_reset);
> +	reset_control_deassert(imx_pcie->apps_reset);
>  
>  	if (imx_pcie->drvdata->core_reset)
>  		imx_pcie->drvdata->core_reset(imx_pcie, false);
> -- 
> 2.37.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 6/9] PCI: imx6: Make *_enable_ref_clk() function symmetric

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 04:33:30PM +0800, Richard Zhu wrote:
> Ensure the *_enable_ref_clk() function is symmetric by addressing missing
> disable parts on some platforms.

This warrants a Fixes tag. And the patch subject should be something like,

"PCI: imx6: Fix the missing reference clock disable logic"

> Also, remove the duplicate
> imx7d_pcie_init_phy() function as it is the same as
> imx7d_pcie_enable_ref_clk().
> 

This is a cleanup, so should be a separate patch.

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 33 +++++++++++----------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 93e2bcf9aa0a..161daad34a94 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -388,13 +388,6 @@ static int imx8mq_pcie_init_phy(struct imx_pcie *imx_pcie)
>  	return 0;
>  }
>  
> -static int imx7d_pcie_init_phy(struct imx_pcie *imx_pcie)
> -{
> -	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
> -
> -	return 0;
> -}
> -
>  static int imx_pcie_init_phy(struct imx_pcie *imx_pcie)
>  {
>  	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
> @@ -593,13 +586,13 @@ static int imx_pcie_attach_pd(struct device *dev)
>  
>  static int imx6sx_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
>  {
> -	if (enable)
> -		regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -				  IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
> -
> +	regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
> +			   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> +			   enable ? 0 : IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
>  	return 0;
>  }
>  
> +

Spurious change.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 7/9] PCI: imx6: Use dwc common suspend resume method

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 9/9] arm64: dts: imx95: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Tue, Oct 15, 2024 at 04:33:33PM +0800, Richard Zhu wrote:
> Add ref clock for i.MX95 PCIe.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx95.dtsi | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
> index 03661e76550f..5cb504b5f851 100644
> --- a/arch/arm64/boot/dts/freescale/imx95.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
> @@ -1473,6 +1473,14 @@ smmu: iommu@490d0000 {
>  			};
>  		};
>  
> +		hsio_blk_ctl: syscon@4c0100c0 {
> +			compatible = "nxp,imx95-hsio-blk-ctl", "syscon";
> +			reg = <0x0 0x4c0100c0 0x0 0x4>;
> +			#clock-cells = <1>;
> +			clocks = <&dummy>;
> +			power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
> +		};

This is an internal reference clock, right? Please mention it in patch
description since the controller supports external reference clock also.

- Mani

> +
>  		pcie0: pcie@4c300000 {
>  			compatible = "fsl,imx95-pcie";
>  			reg = <0 0x4c300000 0 0x10000>,
> @@ -1500,8 +1508,9 @@ pcie0: pcie@4c300000 {
>  			clocks = <&scmi_clk IMX95_CLK_HSIO>,
>  				 <&scmi_clk IMX95_CLK_HSIOPLL>,
>  				 <&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
> -				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
> -			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux";
> +				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>,
> +				 <&hsio_blk_ctl 0>;
> +			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux", "ref";
>  			assigned-clocks =<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
>  					 <&scmi_clk IMX95_CLK_HSIOPLL>,
>  					 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
> @@ -1528,8 +1537,9 @@ pcie0_ep: pcie-ep@4c300000 {
>  			clocks = <&scmi_clk IMX95_CLK_HSIO>,
>  				 <&scmi_clk IMX95_CLK_HSIOPLL>,
>  				 <&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
> -				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
> -			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux";
> +				 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>,
> +				 <&hsio_blk_ctl 0>;
> +			clock-names = "pcie", "pcie_bus", "pcie_phy", "pcie_aux", "ref";
>  			assigned-clocks =<&scmi_clk IMX95_CLK_HSIOPLL_VCO>,
>  					 <&scmi_clk IMX95_CLK_HSIOPLL>,
>  					 <&scmi_clk IMX95_CLK_HSIOPCIEAUX>;
> -- 
> 2.37.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 7/9] PCI: imx6: Use dwc common suspend resume method

[Frank Li]

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
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/9] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Frank Li]

On Tue, Oct 22, 2024 at 10:18:01PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Oct 15, 2024 at 04:33:27PM +0800, Richard Zhu wrote:
> > Since dbi2 and atu regs are added for i.MX8M PCIes. Fetch the dbi2 and iATU
> > base addresses from DT directly, and remove the useless codes.
> >
>
> Again, what will happen to old dts that don't define these regions?

Upsteam dts's have not enabled EP function. So not function broken for old
upsteam's dtb.

Frank

>
> - Mani
>
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 20 --------------------
> >  1 file changed, 20 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 52a8b2dc828a..2ae6fa4b5d32 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1113,7 +1113,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
> >  			   struct platform_device *pdev)
> >  {
> >  	int ret;
> > -	unsigned int pcie_dbi2_offset;
> >  	struct dw_pcie_ep *ep;
> >  	struct dw_pcie *pci = imx_pcie->pci;
> >  	struct dw_pcie_rp *pp = &pci->pp;
> > @@ -1123,25 +1122,6 @@ static int imx_add_pcie_ep(struct imx_pcie *imx_pcie,
> >  	ep = &pci->ep;
> >  	ep->ops = &pcie_ep_ops;
> >
> > -	switch (imx_pcie->drvdata->variant) {
> > -	case IMX8MQ_EP:
> > -	case IMX8MM_EP:
> > -	case IMX8MP_EP:
> > -		pcie_dbi2_offset = SZ_1M;
> > -		break;
> > -	default:
> > -		pcie_dbi2_offset = SZ_4K;
> > -		break;
> > -	}
> > -
> > -	pci->dbi_base2 = pci->dbi_base + pcie_dbi2_offset;
> > -
> > -	/*
> > -	 * FIXME: Ideally, dbi2 base address should come from DT. But since only IMX95 is defining
> > -	 * "dbi2" in DT, "dbi_base2" is set to NULL here for that platform alone so that the DWC
> > -	 * core code can fetch that from DT. But once all platform DTs were fixed, this and the
> > -	 * above "dbi_base2" setting should be removed.
> > -	 */
> >  	if (device_property_match_string(dev, "reg-names", "dbi2") >= 0)
> >  		pci->dbi_base2 = NULL;
> >
> > --
> > 2.37.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

RE: [PATCH v4 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年10月23日 0:46
> 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 2/9] PCI: imx6: Add ref clock for i.MX95 PCIe
> 
> On Tue, Oct 15, 2024 at 04:33:26PM +0800, Richard Zhu wrote:
> > Add "ref" clock to enable reference clock.
> >
> > If use external clock, ref clock should point to external reference.
> >
> > If use internal clock, CREF_EN in LAST_TO_REG controls reference
> > output, which implement in drivers/clk/imx/clk-imx95-blk-ctl.c.
> >
> 
> So this means the driver won't work with old devicetrees. Am I right? Then
> you are breaking the DT compatibility.
> 
> You should make the clock optional in the driver.
Anyway, old DTBs can't work even probe is complete successfully. Since this
 bit would be gated off by released boot firmware.

i.MX95 is a pretty new chip, it's my fault that I didn't figure out this bit
can gate the clock out when internal PLL is used as reference clock in the
initial i.MX95 PCIe support upstream.
 
So, there is no different results whatever this commit is applied or not, when
old DTBs are used.
 
How about just keep this commit?
Since the ref clock is not optional for i.MX95 PCIe from HW view actually.

At end, the commit of this patch is updated as below.
"
PCI: imx6: Add ref clock support for i.MX95 PCIe
 
Add "ref" clock to enable the PCIe reference clock on i.MX95, despite breaking
DT compatibility. This change addresses issues with older DTBs, which would not 
work even if probing was successful, as the reference clock bit is gated off by 
the production boot firmware.
 
For systems using an external clock, the ref clock should point to the external 
reference. For systems using the internal clock, the CREF_EN bit in LAST_TO_REG 
will control the reference output, which is implemented in the 
drivers/clk/imx/clk-imx95-blk-ctl.c driver.
"
Best Regards
Richard Zhu
> 
> - Mani
> 
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 808d1f105417..52a8b2dc828a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1480,6 +1480,7 @@ static const char * const imx8mm_clks[] =
> > {"pcie_bus", "pcie", "pcie_aux"};  static const char * const
> > imx8mq_clks[] = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"};  static
> > const char * const imx6sx_clks[] = {"pcie_bus", "pcie", "pcie_phy",
> > "pcie_inbound_axi"};  static const char * const imx8q_clks[] =
> > {"mstr", "slv", "dbi"};
> > +static const char * const imx95_clks[] = {"pcie_bus", "pcie",
> > +"pcie_phy", "pcie_aux", "ref"};
> >
> >  static const struct imx_pcie_drvdata drvdata[] = {
> >  	[IMX6Q] = {
> > @@ -1593,8 +1594,8 @@ static const struct imx_pcie_drvdata drvdata[] = {
> >  	[IMX95] = {
> >  		.variant = IMX95,
> >  		.flags = IMX_PCIE_FLAG_HAS_SERDES,
> > -		.clk_names = imx8mq_clks,
> > -		.clks_cnt = ARRAY_SIZE(imx8mq_clks),
> > +		.clk_names = imx95_clks,
> > +		.clks_cnt = ARRAY_SIZE(imx95_clks),
> >  		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
> >  		.ltssm_mask = IMX95_PCIE_LTSSM_EN,
> >  		.mode_off[0]  = IMX95_PE0_GEN_CTRL_1,
> > --
> > 2.37.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

RE: [PATCH v4 4/9] PCI: imx6: Correct controller_id generation logic for i.MX7D

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年10月23日 0:56
> 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 4/9] PCI: imx6: Correct controller_id generation logic
> for i.MX7D
> 
> On Tue, Oct 15, 2024 at 04:33:28PM +0800, Richard Zhu wrote:
> > i.MX7D only has one PCIe controller, so controller_id should always be 0.
> > The previous code is incorrect although yielding the correct result.
> > Fix by removing IMX7D from the switch case branch.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 2ae6fa4b5d32..ca8714c625fe 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1338,7 +1338,6 @@ static int imx_pcie_probe(struct platform_device
> *pdev)
> >  	switch (imx_pcie->drvdata->variant) {
> >  	case IMX8MQ:
> >  	case IMX8MQ_EP:
> > -	case IMX7D:
> >  		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> 
> This is just *wrong*. You cannot hardcode the MMIO address in the driver.
> Even though this code is old, you should fix it instead of building on top of it.
Hi Manivannan:
Thanks for your comments.
To avoid break DT compatibility, a proper method should be figured out to fix
 the hardcode here.
But it should be another story, how about to add this to the TO-DO list, and
issue the patch later?

Best Regards
Richard Zhu
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்

RE: [PATCH v4 7/9] PCI: imx6: Use dwc common suspend resume method

[Hongxing Zhu]

> -----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
> 
> --
> மணிவண்ணன் சதாசிவம்