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

[PATCH v7 0/10] 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.
  The changes of clock part are here [1], and had been applied by Abel.
  [1] https://lkml.org/lkml/2024/10/15/390
- 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.
- Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q and
  i.MX95 PCIe PM supports.

v7 changes:
Thanks a lot for Manivannan's kindly review.
- Rebase to latest pcie/next with "tag: pci-v6.13-changes", and with Frank's v8
"PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()" patch-set applied.
https://patchwork.kernel.org/project/linux-pci/cover/20241119-pci_fixup_addr-v8-0-c4bfa5193288@nxp.com/
- #2 patch.
  - Update the commit message
  - Use devm_clk_get_optional(dev, "ref"); to get the optional clock directly.
- #3 patch: Update the commit message.
- #4 patch: Add one Fixes tag.
- #5&#6&9 patches: Update commit message.
- #7 patch: Refine the subject, and the commit message.
- #10 patch: Replace the dummp_clk by one fixed clock.
- Add Manivannan's reviewed-by tag into #3, #4, #5, #6, #7, and #9 patches. 

v6 changes:
Thanks for Frank's comments.
- Add optional clk fetch, without losting safty check.
- Update commit message in #3 and #8 patch of v6
- Add previous discussion as annotation into #4 patch.

v5 changes:
Thanks for Manivannan's review.
- To avoid the DT compatibility on i.MX95, let to fetch i.MX95 PCIe clocks be
  optinal in driver.
- Add Fixes tags into #5 and #6 patches.
- Split the clean up codes into #7 in v5.
- Update the commit message in #10, and #8
  "PCI: imx6: Use dwc common suspend resume method" 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 v7 01/10] dt-bindings: imx6q-pcie: Add ref clock for i.MX95
[PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
[PATCH v7 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT
[PATCH v7 04/10] PCI: imx6: Correct controller_id generation logic
[PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
[PATCH v7 06/10] PCI: imx6: Fix the missing reference clock disable
[PATCH v7 07/10] PCI: imx6: Remove imx7d_pcie_init_phy() function
[PATCH v7 08/10] PCI: imx6: Use dwc common suspend resume method
[PATCH v7 09/10] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 PM support
[PATCH v7 10/10] 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                         |  25 +++++++++--
drivers/pci/controller/dwc/pci-imx6.c                            | 178 ++++++++++++++++++++++++++++------------------------------------------------
5 files changed, 110 insertions(+), 123 deletions(-)

[PATCH v7 01/10] 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 7bd00faa1f2c..0b3526de1d62 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-ep.yaml
@@ -118,6 +118,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 v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Richard Zhu]

Add "ref" clock to enable reference clock. To avoid breaking DT
backwards compatibility, i.MX95 REF clock might be optional. Use
devm_clk_get_optional() to fetch i.MX95 PCIe optional clocks in driver.

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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 385f6323e3ca..f7e928e0a018 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -103,6 +103,7 @@ struct imx_pcie_drvdata {
 	const char *gpr;
 	const char * const *clk_names;
 	const u32 clks_cnt;
+	const u32 clks_optional_cnt;
 	const u32 ltssm_off;
 	const u32 ltssm_mask;
 	const u32 mode_off[IMX_PCIE_MAX_INSTANCES];
@@ -1306,9 +1307,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	struct device_node *np;
 	struct resource *dbi_base;
 	struct device_node *node = dev->of_node;
-	int ret;
+	int i, ret, req_cnt;
 	u16 val;
-	int i;
 
 	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
 	if (!imx_pcie)
@@ -1358,9 +1358,13 @@ static int imx_pcie_probe(struct platform_device *pdev)
 		imx_pcie->clks[i].id = imx_pcie->drvdata->clk_names[i];
 
 	/* Fetch clocks */
-	ret = devm_clk_bulk_get(dev, imx_pcie->drvdata->clks_cnt, imx_pcie->clks);
+	req_cnt = imx_pcie->drvdata->clks_cnt - imx_pcie->drvdata->clks_optional_cnt;
+	ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks);
 	if (ret)
 		return ret;
+	imx_pcie->clks[req_cnt].clk = devm_clk_get_optional(dev, "ref");
+	if (IS_ERR(imx_pcie->clks[req_cnt].clk))
+		return PTR_ERR(imx_pcie->clks[req_cnt].clk);
 
 	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_PHYDRV)) {
 		imx_pcie->phy = devm_phy_get(dev, "pcie-phy");
@@ -1509,6 +1513,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] = {
@@ -1623,8 +1628,9 @@ 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),
+		.clks_optional_cnt = 1,
 		.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 v7 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Richard Zhu]

Since dw_pcie_get_resources() gets the dbi2 and iATU base addresses from
DT, remove the code from imx6 driver that does the same.

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

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pci-imx6.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f7e928e0a018..81f1f68ccc14 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1114,7 +1114,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;
@@ -1124,28 +1123,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;
-
 	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_SUPPORT_64BIT))
 		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
 
-- 
2.37.1

[PATCH v7 04/10] 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.

Fixes: 2d8ed461dbc9 ("PCI: imx6: Add support for i.MX8MQ")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
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 81f1f68ccc14..3538440601a7 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1367,7 +1367,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 v7 05/10] PCI: imx6: Deassert apps_reset in imx_pcie_deassert_core_reset()

[Richard Zhu]

Since the apps_reset is asserted in imx_pcie_assert_core_reset(), it should
be deasserted in imx_pcie_deassert_core_reset().

Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
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 3538440601a7..413db182ce9f 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -776,6 +776,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 v7 06/10] PCI: imx6: Fix the missing reference clock disable logic

[Richard Zhu]

Ensure the *_enable_ref_clk() function is symmetric by addressing missing
disable parts on some platforms.

Fixes: d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 413db182ce9f..ab2c97a8c327 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -599,10 +599,9 @@ 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;
 }
 
@@ -631,19 +630,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;
 }
 
-- 
2.37.1

[PATCH v7 07/10] PCI: imx6: Remove imx7d_pcie_init_phy() function

[Richard Zhu]

This function essentially duplicates imx7d_pcie_enable_ref_clk(). So remove
it.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index ab2c97a8c327..743a71789d17 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -394,13 +394,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,
@@ -1554,7 +1547,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 v7 08/10] 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. 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. Use previouse method to kick off
PME_TURN_OFF MSG for these platforms.

SRC(System Reset Control) interface is used to toggle 'turnoff_reset' to
send PME_TURN_OFF and since the DWC implementation is used, it is not
needed now.

Replace the imx_pcie_stop_link() and imx_pcie_host_exit() by
dw_pcie_suspend_noirq() in imx_pcie_suspend_noirq().

Since dw_pcie_suspend_noirq() already does these, see below call stack:
dw_pcie_suspend_noirq()
  dw_pcie_stop_link();
    imx_pcie_stop_link();
  pci->pp.ops->deinit();
    imx_pcie_host_exit();

Replace the imx_pcie_host_init(), dw_pcie_setup_rc() and
imx_pcie_start_link() by dw_pcie_resume_noirq() in
imx_pcie_resume_noirq().

Since dw_pcie_resume_noirq() already does these, see below call stack:
dw_pcie_resume_noirq()
  pci->pp.ops->init();
    imx_pcie_host_init();
  dw_pcie_setup_rc();
  dw_pcie_start_link();
    imx_pcie_start_link();

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 | 96 ++++++++++-----------------
 1 file changed, 35 insertions(+), 61 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 743a71789d17..87dac4ac9d10 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)
@@ -112,19 +113,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;
@@ -903,13 +903,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));
@@ -1014,9 +1012,31 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
 		regulator_disable(imx_pcie->vpcie);
 }
 
+/*
+ * 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_pme_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_pme_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 = {
@@ -1143,43 +1163,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;
@@ -1203,7 +1186,6 @@ 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;
@@ -1218,9 +1200,7 @@ static int imx_pcie_suspend_noirq(struct device *dev)
 		imx_pcie_assert_core_reset(imx_pcie);
 		imx_pcie->drvdata->enable_ref_clk(imx_pcie, false);
 	} else {
-		imx_pcie_pm_turnoff(imx_pcie);
-		imx_pcie_stop_link(imx_pcie->pci);
-		imx_pcie_host_exit(pp);
+		return dw_pcie_suspend_noirq(imx_pcie->pci);
 	}
 
 	return 0;
@@ -1230,7 +1210,6 @@ 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;
@@ -1250,17 +1229,12 @@ static int imx_pcie_resume_noirq(struct device *dev)
 		ret = dw_pcie_setup_rc(&imx_pcie->pci->pp);
 		if (ret)
 			return ret;
-		imx_pcie_msi_save_restore(imx_pcie, false);
 	} else {
-		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);
 	}
+	imx_pcie_msi_save_restore(imx_pcie, false);
 
 	return 0;
 }
@@ -1291,11 +1265,15 @@ 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);
 
+	if (imx_pcie->drvdata->ops)
+		pci->pp.ops = imx_pcie->drvdata->ops;
+	else
+		pci->pp.ops = &imx_pcie_host_dw_pme_ops;
+
 	/* Find the PHY if one is defined, only imx7d uses it */
 	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
 	if (np) {
@@ -1368,13 +1346,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 =
@@ -1454,6 +1425,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;
@@ -1519,6 +1491,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,
@@ -1536,6 +1509,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 v7 09/10] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 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: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
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 87dac4ac9d10..852b34572bb8 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1527,7 +1527,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),
@@ -1564,13 +1565,15 @@ static const struct imx_pcie_drvdata drvdata[] = {
 	},
 	[IMX8Q] = {
 		.variant = IMX8Q,
-		.flags = IMX_PCIE_FLAG_HAS_PHYDRV,
+		.flags = IMX_PCIE_FLAG_HAS_PHYDRV |
+			 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),
 		.clks_optional_cnt = 1,
-- 
2.37.1

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

[Richard Zhu]

Add ref clock for i.MX95 PCIe here, when the internal PLL is used as
PCIe reference clock.

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

diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi b/arch/arm64/boot/dts/freescale/imx95.dtsi
index 03661e76550f..9951d2c84799 100644
--- a/arch/arm64/boot/dts/freescale/imx95.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
@@ -236,6 +236,13 @@ clk_ext1: clock-ext1 {
 		clock-output-names = "clk_ext1";
 	};
 
+	clk_sys100m: clock-sys100m {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <1000000>;
+		clock-output-names = "clk_sys100m";
+	};
+
 	sai1_mclk: clock-sai-mclk1 {
 		compatible = "fixed-clock";
 		#clock-cells = <0>;
@@ -1473,6 +1480,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 = <&clk_sys100m>;
+			power-domains = <&scmi_devpd IMX95_PD_HSIO_TOP>;
+		};
+
 		pcie0: pcie@4c300000 {
 			compatible = "fsl,imx95-pcie";
 			reg = <0 0x4c300000 0 0x10000>,
@@ -1500,8 +1515,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>;
@@ -1567,8 +1583,9 @@ pcie1: pcie@4c380000 {
 			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 v7 0/10] A bunch of changes to refine i.MX PCIe driver

[Frank Li]

On Tue, Nov 26, 2024 at 03:56:52PM +0800, Richard Zhu wrote:
> A bunch of changes to refine i.MX PCIe driver.
> - Add ref clock gate for i.MX95 PCIe.
>   The changes of clock part are here [1], and had been applied by Abel.
>   [1] https://lkml.org/lkml/2024/10/15/390
> - 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.
> - Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q and
>   i.MX95 PCIe PM supports.
>
> v7 changes:
> Thanks a lot for Manivannan's kindly review.
> - Rebase to latest pcie/next with "tag: pci-v6.13-changes", and with Frank's v8
> "PCI: dwc: opitimaze RC Host/EP pci_fixup_addr()" patch-set applied.
> https://patchwork.kernel.org/project/linux-pci/cover/20241119-pci_fixup_addr-v8-0-c4bfa5193288@nxp.com/
> - #2 patch.
>   - Update the commit message
>   - Use devm_clk_get_optional(dev, "ref"); to get the optional clock directly.
> - #3 patch: Update the commit message.
> - #4 patch: Add one Fixes tag.
> - #5&#6&9 patches: Update commit message.
> - #7 patch: Refine the subject, and the commit message.
> - #10 patch: Replace the dummp_clk by one fixed clock.
> - Add Manivannan's reviewed-by tag into #3, #4, #5, #6, #7, and #9 patches.

Mani and Krzysztof Wilczyński:

	Could you please help review these patches, it was posted at
Nov 26.

Frank

>
> v6 changes:
> Thanks for Frank's comments.
> - Add optional clk fetch, without losting safty check.
> - Update commit message in #3 and #8 patch of v6
> - Add previous discussion as annotation into #4 patch.
>
> v5 changes:
> Thanks for Manivannan's review.
> - To avoid the DT compatibility on i.MX95, let to fetch i.MX95 PCIe clocks be
>   optinal in driver.
> - Add Fixes tags into #5 and #6 patches.
> - Split the clean up codes into #7 in v5.
> - Update the commit message in #10, and #8
>   "PCI: imx6: Use dwc common suspend resume method" 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 v7 01/10] dt-bindings: imx6q-pcie: Add ref clock for i.MX95
> [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> [PATCH v7 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT
> [PATCH v7 04/10] PCI: imx6: Correct controller_id generation logic
> [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> [PATCH v7 06/10] PCI: imx6: Fix the missing reference clock disable
> [PATCH v7 07/10] PCI: imx6: Remove imx7d_pcie_init_phy() function
> [PATCH v7 08/10] PCI: imx6: Use dwc common suspend resume method
> [PATCH v7 09/10] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 PM support
> [PATCH v7 10/10] 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                         |  25 +++++++++--
> drivers/pci/controller/dwc/pci-imx6.c                            | 178 ++++++++++++++++++++++++++++------------------------------------------------
> 5 files changed, 110 insertions(+), 123 deletions(-)
>

Re: [PATCH v7 0/10] A bunch of changes to refine i.MX PCIe driver

[Krzysztof Wilczyński]

Hello,

> A bunch of changes to refine i.MX PCIe driver.
> - Add ref clock gate for i.MX95 PCIe.
>   The changes of clock part are here [1], and had been applied by Abel.
>   [1] https://lkml.org/lkml/2024/10/15/390
> - 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.
> - Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q and
>   i.MX95 PCIe PM supports.

Applied to controller/imx6 for v6.14, thank you!

	Krzysztof

Re: [PATCH v7 0/10] A bunch of changes to refine i.MX PCIe driver

[Krzysztof Wilczyński]

Hello,

> > A bunch of changes to refine i.MX PCIe driver.
> > - Add ref clock gate for i.MX95 PCIe.
> >   The changes of clock part are here [1], and had been applied by Abel.
> >   [1] https://lkml.org/lkml/2024/10/15/390
> > - 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.
> > - Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q and
> >   i.MX95 PCIe PM supports.
> 
> Applied to controller/imx6 for v6.14, thank you!

Richard and Frank, please have a look at the code to make sure that
everything looks fine to you.  There were some conflicts while I applied
the series, and I want to make sure that nothing is broken.

Thank you!

	Krzysztof

Re: [PATCH v7 0/10] A bunch of changes to refine i.MX PCIe driver

[Krzysztof Wilczyński]

Hello,

> > > A bunch of changes to refine i.MX PCIe driver.
> > > - Add ref clock gate for i.MX95 PCIe.
> > >   The changes of clock part are here [1], and had been applied by Abel.
> > >   [1] https://lkml.org/lkml/2024/10/15/390
> > > - 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.
> > > - Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q and
> > >   i.MX95 PCIe PM supports.
> > 
> > Applied to controller/imx6 for v6.14, thank you!
> 
> Richard and Frank, please have a look at the code to make sure that
> everything looks fine to you.  There were some conflicts while I applied
> the series, and I want to make sure that nothing is broken.
> 
> Thank you!

I moved this series to the controller/dwc branch as we have changes there
on which this series depends.  Hopefully, this will solve the build failure
we've seen on our next.

	Krzysztof

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

[Hongxing Zhu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 2382 bytes --]

> -----Original Message-----
> From: Krzysztof Wilczy¨½ski <kw@linux.com>
> Sent: 2025Äê1ÔÂ15ÈÕ 21:06
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; shawnguo@kernel.org; Frank Li <frank.li@nxp.com>;
> s.hauer@pengutronix.de; festevam@gmail.com; imx@lists.linux.dev;
> kernel@pengutronix.de; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 0/10] A bunch of changes to refine i.MX PCIe driver
> 
> Hello,
> 
> > > A bunch of changes to refine i.MX PCIe driver.
> > > - Add ref clock gate for i.MX95 PCIe.
> > >   The changes of clock part are here [1], and had been applied by Abel.
> > >   [1]
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flk
> > >
> ml.org%2Flkml%2F2024%2F10%2F15%2F390&data=05%7C02%7Chongxing.zh
> u%40n
> > >
> xp.com%7Cbf8b9025e8fd4da26c9a08dd35656424%7C686ea1d3bc2b4c6fa92c
> d99c
> > >
> 5c301635%7C0%7C0%7C638725431787342214%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbX
> > >
> B0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpb
> C
> > >
> IsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=6jYiGJUjQyM9OA8jgn%2B6OG
> %2FvR%
> > > 2FQT5uvlKnfO3vYjEb8%3D&reserved=0
> > > - 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.
> > > - Use dwc common suspend resume method, and enable i.MX8MQ, i.MX8Q
> and
> > >   i.MX95 PCIe PM supports.
> >
> > Applied to controller/imx6 for v6.14, thank you!
> 
> Richard and Frank, please have a look at the code to make sure that everything
> looks fine to you.  There were some conflicts while I applied the series, and I
> want to make sure that nothing is broken.
Hi Krzysztof:
Thanks for your kindly help to pick up this series.
I checked the patches, they are fine to me.
BTW, I saw the dts patch had been queued into pcie/next too.
It's better let Shawn merge the dts changes after the driver patches are merged.

Best Regards
Richard Zhu
> 
> Thank you!
> 
> 	Krzysztof

Re: [PATCH v7 0/10] A bunch of changes to refine i.MX PCIe driver

[Krzysztof Wilczy��ski]

Hello,

[...]
> Thanks for your kindly help to pick up this series.

No problem.  Thank you for persevering.

I apologise for letting this fall through the cracks.

> I checked the patches, they are fine to me.

Thank you for double-checking things for me.  Much appreciated.

> BTW, I saw the dts patch had been queued into pcie/next too.
> It's better let Shawn merge the dts changes after the driver patches are merged.

No worries.  I will drop the patch.

	Krzysztof

Re: [PATCH v7 06/10] PCI: imx6: Fix the missing reference clock disable logic

[Bjorn Helgaas]

On Tue, Nov 26, 2024 at 03:56:58PM +0800, Richard Zhu wrote:
> Ensure the *_enable_ref_clk() function is symmetric by addressing missing
> disable parts on some platforms.
> 
> Fixes: d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable")

The patch below looks fine to me, and I guess it's more than just
making the code prettier; it also actually *fixes* something, right?

It looks like a functional change because imx_pcie_clk_enable() will
now enable the IMX7D refclk when it didn't before, and
imx_pcie_clk_disable() will disable the IMX6SX and IMX8M* refclk when
it didn't before?

But I don't think the Fixes: tag is correct.  I looked at uses of
these symbols:

  IMX6SX_GPR12_PCIE_TEST_POWERDOWN
    enabled by imx6_pcie_enable_ref_clk()
    disabled by imx6_pcie_assert_core_reset()

  IMX7D_GPR12_PCIE_PHY_REFCLK_SEL
    enabled by imx6_pcie_init_phy()
    disabled by imx6_pcie_clk_disable()

  IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE
    enabled by imx6_pcie_enable_ref_clk()

As far as I can tell, these uses are identical before and after
d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match
enable").

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 413db182ce9f..ab2c97a8c327 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -599,10 +599,9 @@ 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;
>  }
>  
> @@ -631,19 +630,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;
>  }
>  
> -- 
> 2.37.1
> 

Re: [PATCH v7 06/10] PCI: imx6: Fix the missing reference clock disable logic

[Frank Li]

On Thu, Jan 16, 2025 at 11:01:14AM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 26, 2024 at 03:56:58PM +0800, Richard Zhu wrote:
> > Ensure the *_enable_ref_clk() function is symmetric by addressing missing
> > disable parts on some platforms.
> >
> > Fixes: d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable")
>
> The patch below looks fine to me, and I guess it's more than just
> making the code prettier; it also actually *fixes* something, right?
>
> It looks like a functional change because imx_pcie_clk_enable() will
> now enable the IMX7D refclk when it didn't before, and
> imx_pcie_clk_disable() will disable the IMX6SX and IMX8M* refclk when
> it didn't before?
>
> But I don't think the Fixes: tag is correct.  I looked at uses of
> these symbols:
>
>   IMX6SX_GPR12_PCIE_TEST_POWERDOWN
>     enabled by imx6_pcie_enable_ref_clk()
>     disabled by imx6_pcie_assert_core_reset()
>
>   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL
>     enabled by imx6_pcie_init_phy()
>     disabled by imx6_pcie_clk_disable()
>
>   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE
>     enabled by imx6_pcie_enable_ref_clk()
>
> As far as I can tell, these uses are identical before and after
> d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match
> enable").

You can drop fixes tags

Frank
>
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 413db182ce9f..ab2c97a8c327 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -599,10 +599,9 @@ 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;
> >  }
> >
> > @@ -631,19 +630,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;
> >  }
> >
> > --
> > 2.37.1
> >

Re: [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Tue, Nov 26, 2024 at 03:56:54PM +0800, Richard Zhu wrote:
> Add "ref" clock to enable reference clock. To avoid breaking DT
> backwards compatibility, i.MX95 REF clock might be optional. Use
> devm_clk_get_optional() to fetch i.MX95 PCIe optional clocks in driver.
> 
> 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 | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 385f6323e3ca..f7e928e0a018 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -103,6 +103,7 @@ struct imx_pcie_drvdata {
>  	const char *gpr;
>  	const char * const *clk_names;
>  	const u32 clks_cnt;
> +	const u32 clks_optional_cnt;
>  	const u32 ltssm_off;
>  	const u32 ltssm_mask;
>  	const u32 mode_off[IMX_PCIE_MAX_INSTANCES];
> @@ -1306,9 +1307,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	struct device_node *np;
>  	struct resource *dbi_base;
>  	struct device_node *node = dev->of_node;
> -	int ret;
> +	int i, ret, req_cnt;
>  	u16 val;
> -	int i;
>  
>  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
>  	if (!imx_pcie)
> @@ -1358,9 +1358,13 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  		imx_pcie->clks[i].id = imx_pcie->drvdata->clk_names[i];
>  
>  	/* Fetch clocks */
> -	ret = devm_clk_bulk_get(dev, imx_pcie->drvdata->clks_cnt, imx_pcie->clks);
> +	req_cnt = imx_pcie->drvdata->clks_cnt - imx_pcie->drvdata->clks_optional_cnt;
> +	ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks);
>  	if (ret)
>  		return ret;
> +	imx_pcie->clks[req_cnt].clk = devm_clk_get_optional(dev, "ref");
> +	if (IS_ERR(imx_pcie->clks[req_cnt].clk))
> +		return PTR_ERR(imx_pcie->clks[req_cnt].clk);

I think you should just switch to devm_clk_bulk_get_all() instead of getting the
clks separately. As I told previously, the DT binding should ensure that correct
clocks for the platforms are defined in DT and the driver has no business in
validating it. Driver should trust the DT instead (unless there is a valid
reason to not do so).

>  
>  	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_PHYDRV)) {
>  		imx_pcie->phy = devm_phy_get(dev, "pcie-phy");
> @@ -1509,6 +1513,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"};

And these static clock defines will go away too.

- Mani

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

RE: [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2025年1月19日 15:03
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> shawnguo@kernel.org; Frank Li <frank.li@nxp.com>; s.hauer@pengutronix.de;
> festevam@gmail.com; imx@lists.linux.dev; kernel@pengutronix.de;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> 
> On Tue, Nov 26, 2024 at 03:56:54PM +0800, Richard Zhu wrote:
> > Add "ref" clock to enable reference clock. To avoid breaking DT
> > backwards compatibility, i.MX95 REF clock might be optional. Use
> > devm_clk_get_optional() to fetch i.MX95 PCIe optional clocks in driver.
> >
> > 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 | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 385f6323e3ca..f7e928e0a018 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -103,6 +103,7 @@ struct imx_pcie_drvdata {
> >  	const char *gpr;
> >  	const char * const *clk_names;
> >  	const u32 clks_cnt;
> > +	const u32 clks_optional_cnt;
> >  	const u32 ltssm_off;
> >  	const u32 ltssm_mask;
> >  	const u32 mode_off[IMX_PCIE_MAX_INSTANCES]; @@ -1306,9 +1307,8
> @@
> > static int imx_pcie_probe(struct platform_device *pdev)
> >  	struct device_node *np;
> >  	struct resource *dbi_base;
> >  	struct device_node *node = dev->of_node;
> > -	int ret;
> > +	int i, ret, req_cnt;
> >  	u16 val;
> > -	int i;
> >
> >  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
> >  	if (!imx_pcie)
> > @@ -1358,9 +1358,13 @@ static int imx_pcie_probe(struct platform_device
> *pdev)
> >  		imx_pcie->clks[i].id = imx_pcie->drvdata->clk_names[i];
> >
> >  	/* Fetch clocks */
> > -	ret = devm_clk_bulk_get(dev, imx_pcie->drvdata->clks_cnt, imx_pcie->clks);
> > +	req_cnt = imx_pcie->drvdata->clks_cnt -
> imx_pcie->drvdata->clks_optional_cnt;
> > +	ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks);
> >  	if (ret)
> >  		return ret;
> > +	imx_pcie->clks[req_cnt].clk = devm_clk_get_optional(dev, "ref");
> > +	if (IS_ERR(imx_pcie->clks[req_cnt].clk))
> > +		return PTR_ERR(imx_pcie->clks[req_cnt].clk);
> 
> I think you should just switch to devm_clk_bulk_get_all() instead of getting the
> clks separately. As I told previously, the DT binding should ensure that correct
> clocks for the platforms are defined in DT and the driver has no business in
> validating it. Driver should trust the DT instead (unless there is a valid reason to not
> do so).
> 
> >
> >  	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_PHYDRV)) {
> >  		imx_pcie->phy = devm_phy_get(dev, "pcie-phy"); @@ -1509,6 +1513,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"};
> 
> And these static clock defines will go away too.
> 
Hi Mani:
Thanks for your comments.
The suggestions are very nice. How about kick off further optimization later?
 Since the changes would impact all i.MX PCIes.
Meanwhile, I'm a little worry about that there is no consensus yet on relying
 entirely on the dt binding check.

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

Re: [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 02:49:09AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 2025年1月19日 15:03
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
> > kw@linux.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> > shawnguo@kernel.org; Frank Li <frank.li@nxp.com>; s.hauer@pengutronix.de;
> > festevam@gmail.com; imx@lists.linux.dev; kernel@pengutronix.de;
> > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v7 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> > 
> > On Tue, Nov 26, 2024 at 03:56:54PM +0800, Richard Zhu wrote:
> > > Add "ref" clock to enable reference clock. To avoid breaking DT
> > > backwards compatibility, i.MX95 REF clock might be optional. Use
> > > devm_clk_get_optional() to fetch i.MX95 PCIe optional clocks in driver.
> > >
> > > 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 | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 385f6323e3ca..f7e928e0a018 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -103,6 +103,7 @@ struct imx_pcie_drvdata {
> > >  	const char *gpr;
> > >  	const char * const *clk_names;
> > >  	const u32 clks_cnt;
> > > +	const u32 clks_optional_cnt;
> > >  	const u32 ltssm_off;
> > >  	const u32 ltssm_mask;
> > >  	const u32 mode_off[IMX_PCIE_MAX_INSTANCES]; @@ -1306,9 +1307,8
> > @@
> > > static int imx_pcie_probe(struct platform_device *pdev)
> > >  	struct device_node *np;
> > >  	struct resource *dbi_base;
> > >  	struct device_node *node = dev->of_node;
> > > -	int ret;
> > > +	int i, ret, req_cnt;
> > >  	u16 val;
> > > -	int i;
> > >
> > >  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
> > >  	if (!imx_pcie)
> > > @@ -1358,9 +1358,13 @@ static int imx_pcie_probe(struct platform_device
> > *pdev)
> > >  		imx_pcie->clks[i].id = imx_pcie->drvdata->clk_names[i];
> > >
> > >  	/* Fetch clocks */
> > > -	ret = devm_clk_bulk_get(dev, imx_pcie->drvdata->clks_cnt, imx_pcie->clks);
> > > +	req_cnt = imx_pcie->drvdata->clks_cnt -
> > imx_pcie->drvdata->clks_optional_cnt;
> > > +	ret = devm_clk_bulk_get(dev, req_cnt, imx_pcie->clks);
> > >  	if (ret)
> > >  		return ret;
> > > +	imx_pcie->clks[req_cnt].clk = devm_clk_get_optional(dev, "ref");
> > > +	if (IS_ERR(imx_pcie->clks[req_cnt].clk))
> > > +		return PTR_ERR(imx_pcie->clks[req_cnt].clk);
> > 
> > I think you should just switch to devm_clk_bulk_get_all() instead of getting the
> > clks separately. As I told previously, the DT binding should ensure that correct
> > clocks for the platforms are defined in DT and the driver has no business in
> > validating it. Driver should trust the DT instead (unless there is a valid reason to not
> > do so).
> > 
> > >
> > >  	if (imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_PHYDRV)) {
> > >  		imx_pcie->phy = devm_phy_get(dev, "pcie-phy"); @@ -1509,6 +1513,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"};
> > 
> > And these static clock defines will go away too.
> > 
> Hi Mani:
> Thanks for your comments.
> The suggestions are very nice. How about kick off further optimization later?

Sure. This series got merged already.

>  Since the changes would impact all i.MX PCIes.
> Meanwhile, I'm a little worry about that there is no consensus yet on relying
>  entirely on the dt binding check.

Consensus between whom?

- Mani

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

Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in imx_pcie_deassert_core_reset()

[Tim Harvey]

On Tue, Nov 26, 2024 at 12:03 AM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Since the apps_reset is asserted in imx_pcie_assert_core_reset(), it should
> be deasserted in imx_pcie_deassert_core_reset().
>
> Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 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 3538440601a7..413db182ce9f 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -776,6 +776,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
>
>

Hi Richard,

I've found that this patch causes a regression on i.MX8MM and i.MX8MP
boards with hotplug capable bridges:
i.MX8MM+PI7C9X2G404EV (this switch does not support hotplug) - no issues
i.MX8MM+PI7C9X2G608GP (hotplug) - fails to reliably enumerate
downstream devices about 80% of the time
^^^ when this occurs PCI_PRIMARY_BUS (0x18) for the root complex
0000:00:00.0 reads 0x00000000 instead of 0x00ff0100 (PCI_SECONDARY_BUS
is 0 instead of 1 and PCI_SUBBORDINATE_BUS is 0 instead of 0xff)
i.MX8MP+PI7C9X2G608GP (hotplug) - hangs at imx_pcie_ltssm_enable
deassert apps_reset

In both cases here reverting ef61c7d8d032 ("PCI: imx6: Deassert
apps_reset in imx_pcie_deassert_core_reset()") resolves this.

I notice the sequence of events here is:
imx_pcie_assert_core_reset asserts apps_reset (disables LTSSM)
imx_pcie_deassert_core_reset deasserts apps_reset (enables LTSSM)
imx_pcie_ltssm_enable deasserts apps_reset (enables LTSSM; this is
where it hangs on imx8mp)

Is there perhaps some issue with de-asserting this (enabling LTSSM)
when it's already in this state?

In the case where downstream devices do not enumerate some
investigation points to them not being happy that the link drops so
perhaps deasserting apps_reset when its already asserted drops the
link and restarts it?

Best Regards,

Tim

RE: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in imx_pcie_deassert_core_reset()

[Hongxing Zhu]

> -----Original Message-----
> From: Tim Harvey <tharvey@gateworks.com>
> Sent: 2025年6月7日 5:04
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
> kw@linux.com; manivannan.sadhasivam@linaro.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; Frank Li
> <frank.li@nxp.com>; s.hauer@pengutronix.de; festevam@gmail.com;
> imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> imx_pcie_deassert_core_reset()
> 
> On Tue, Nov 26, 2024 at 12:03 AM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > Since the apps_reset is asserted in imx_pcie_assert_core_reset(), it
> > should be deasserted in imx_pcie_deassert_core_reset().
> >
> > Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Reviewed-by: Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org>
> > 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 3538440601a7..413db182ce9f 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -776,6 +776,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
> >
> >
> 
> Hi Richard,
> 
> I've found that this patch causes a regression on i.MX8MM and i.MX8MP
> boards with hotplug capable bridges:
> i.MX8MM+PI7C9X2G404EV (this switch does not support hotplug) - no issues
> i.MX8MM+PI7C9X2G608GP (hotplug) - fails to reliably enumerate
> downstream devices about 80% of the time ^^^ when this occurs
> PCI_PRIMARY_BUS (0x18) for the root complex
> 0000:00:00.0 reads 0x00000000 instead of 0x00ff0100
> (PCI_SECONDARY_BUS is 0 instead of 1 and PCI_SUBBORDINATE_BUS is 0
> instead of 0xff) i.MX8MP+PI7C9X2G608GP (hotplug) - hangs at
> imx_pcie_ltssm_enable deassert apps_reset
> 
> In both cases here reverting ef61c7d8d032 ("PCI: imx6: Deassert apps_reset
> in imx_pcie_deassert_core_reset()") resolves this.
> 
[Richard Zhu] I'm afraid that the ltssm_en bit assert to 1b'1 in
 imx_pcie_deassert_core_reset() is not correct in your use case.

Actually, the apps_reset isn't a real reset. It's the ltssm_en bit.
From this perspective view, It's inappropriate to toggle the ltssm_en bit in
 imx_pcie_assert/deassert_core_reset() functions.
I consider to move the apps_reset out of _reset_ functions. 
Can you help to test the following changes in you use-case?

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -776,7 +776,6 @@ static int imx7d_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
 static void imx_pcie_assert_core_reset(struct imx_pcie *imx_pcie)
 {
        reset_control_assert(imx_pcie->pciephy_reset);
-       reset_control_assert(imx_pcie->apps_reset);

        if (imx_pcie->drvdata->core_reset)
                imx_pcie->drvdata->core_reset(imx_pcie, true);
@@ -788,7 +787,6 @@ 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);
@@ -1176,6 +1174,9 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
                }
        }

+       /* Make sure that PCIe LTSSM is cleared */
+       imx_pcie_ltssm_disable(dev);
+
        ret = imx_pcie_deassert_core_reset(imx_pcie);
        if (ret < 0) {
                dev_err(dev, "pcie deassert core reset failed: %d\n", ret);

> I notice the sequence of events here is:
> imx_pcie_assert_core_reset asserts apps_reset (disables LTSSM)
> imx_pcie_deassert_core_reset deasserts apps_reset (enables LTSSM)
> imx_pcie_ltssm_enable deasserts apps_reset (enables LTSSM; this is where it
> hangs on imx8mp)
> 
> Is there perhaps some issue with de-asserting this (enabling LTSSM) when it's
> already in this state?
[Richard Zhu]The apps_reset is updated by src driver by regmap_update_bits().
I can't find the exceptions to update one bit, already has the according value.

Best Regards
Richard Zhu
> 
> In the case where downstream devices do not enumerate some investigation
> points to them not being happy that the link drops so perhaps deasserting
> apps_reset when its already asserted drops the link and restarts it?
> 
> Best Regards,
> 
> Tim

Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in imx_pcie_deassert_core_reset()

[Tim Harvey]

On Mon, Jun 9, 2025 at 1:03 AM Hongxing Zhu <hongxing.zhu@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Tim Harvey <tharvey@gateworks.com>
> > Sent: 2025年6月7日 5:04
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
> > kw@linux.com; manivannan.sadhasivam@linaro.org; robh@kernel.org;
> > krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; Frank Li
> > <frank.li@nxp.com>; s.hauer@pengutronix.de; festevam@gmail.com;
> > imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> > imx_pcie_deassert_core_reset()
> >
> > On Tue, Nov 26, 2024 at 12:03 AM Richard Zhu <hongxing.zhu@nxp.com>
> > wrote:
> > >
> > > Since the apps_reset is asserted in imx_pcie_assert_core_reset(), it
> > > should be deasserted in imx_pcie_deassert_core_reset().
> > >
> > > Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Reviewed-by: Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org>
> > > 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 3538440601a7..413db182ce9f 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -776,6 +776,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
> > >
> > >
> >
> > Hi Richard,
> >
> > I've found that this patch causes a regression on i.MX8MM and i.MX8MP
> > boards with hotplug capable bridges:
> > i.MX8MM+PI7C9X2G404EV (this switch does not support hotplug) - no issues
> > i.MX8MM+PI7C9X2G608GP (hotplug) - fails to reliably enumerate
> > downstream devices about 80% of the time ^^^ when this occurs
> > PCI_PRIMARY_BUS (0x18) for the root complex
> > 0000:00:00.0 reads 0x00000000 instead of 0x00ff0100
> > (PCI_SECONDARY_BUS is 0 instead of 1 and PCI_SUBBORDINATE_BUS is 0
> > instead of 0xff) i.MX8MP+PI7C9X2G608GP (hotplug) - hangs at
> > imx_pcie_ltssm_enable deassert apps_reset
> >
> > In both cases here reverting ef61c7d8d032 ("PCI: imx6: Deassert apps_reset
> > in imx_pcie_deassert_core_reset()") resolves this.
> >
> [Richard Zhu] I'm afraid that the ltssm_en bit assert to 1b'1 in
>  imx_pcie_deassert_core_reset() is not correct in your use case.
>

Hi Richard,

Thanks for your quick response. Do you mean not correct for newer IP
core in i.MX8MM/i.MX8MP or in the case of a bridge?

> Actually, the apps_reset isn't a real reset. It's the ltssm_en bit.
> From this perspective view, It's inappropriate to toggle the ltssm_en bit in
>  imx_pcie_assert/deassert_core_reset() functions.
> I consider to move the apps_reset out of _reset_ functions.
> Can you help to test the following changes in you use-case?
>
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -776,7 +776,6 @@ static int imx7d_pcie_core_reset(struct imx_pcie *imx_pcie, bool assert)
>  static void imx_pcie_assert_core_reset(struct imx_pcie *imx_pcie)
>  {
>         reset_control_assert(imx_pcie->pciephy_reset);
> -       reset_control_assert(imx_pcie->apps_reset);
>
>         if (imx_pcie->drvdata->core_reset)
>                 imx_pcie->drvdata->core_reset(imx_pcie, true);
> @@ -788,7 +787,6 @@ 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);
> @@ -1176,6 +1174,9 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
>                 }
>         }
>
> +       /* Make sure that PCIe LTSSM is cleared */
> +       imx_pcie_ltssm_disable(dev);
> +
>         ret = imx_pcie_deassert_core_reset(imx_pcie);
>         if (ret < 0) {
>                 dev_err(dev, "pcie deassert core reset failed: %d\n", ret);
>

Yes this resolves the regression of failing to reliably enumerate
downstream devices. I think that should be submitted with a fixes tag.

The i.MX8MP+PI7C9X2G608GP switch hanging issue was hardware related...
i was sadly testing on an old board with a defect. I did previously
have a hang issue there discussed previously here [1] but it was
resolved with commit 9c03e30e3ade ("PCI: imx6: Skip link up workaround
for newer platforms").

How much testing is done with i.MX8M{M,P} board with a switch? I feel
like I'm the only one with these SoC's and a switch and I need to get
better at monitoring patches to the IMX6 PCI controller driver and
testing these scenarios.

Best Regards,

Tim
[1] https://www.spinics.net/lists/linux-pci/msg142764.html




> > I notice the sequence of events here is:
> > imx_pcie_assert_core_reset asserts apps_reset (disables LTSSM)
> > imx_pcie_deassert_core_reset deasserts apps_reset (enables LTSSM)
> > imx_pcie_ltssm_enable deasserts apps_reset (enables LTSSM; this is where it
> > hangs on imx8mp)
> >
> > Is there perhaps some issue with de-asserting this (enabling LTSSM) when it's
> > already in this state?
> [Richard Zhu]The apps_reset is updated by src driver by regmap_update_bits().
> I can't find the exceptions to update one bit, already has the according value.
>
> Best Regards
> Richard Zhu
> >
> > In the case where downstream devices do not enumerate some investigation
> > points to them not being happy that the link drops so perhaps deasserting
> > apps_reset when its already asserted drops the link and restarts it?
> >
> > Best Regards,
> >
> > Tim

RE: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in imx_pcie_deassert_core_reset()

[Hongxing Zhu]

> -----Original Message-----
> From: Tim Harvey <tharvey@gateworks.com>
> Sent: 2025年6月10日 8:24
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com; lpieralisi@kernel.org;
> kw@linux.com; manivannan.sadhasivam@linaro.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; Frank Li
> <frank.li@nxp.com>; s.hauer@pengutronix.de; festevam@gmail.com;
> imx@lists.linux.dev; kernel@pengutronix.de; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> imx_pcie_deassert_core_reset()
>
> On Mon, Jun 9, 2025 at 1:03 AM Hongxing Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Tim Harvey <tharvey@gateworks.com>
> > > Sent: 2025年6月7日 5:04
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> > > lpieralisi@kernel.org; kw@linux.com;
> > > manivannan.sadhasivam@linaro.org; robh@kernel.org;
> > > krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; Frank
> > > krzk+Li
> > > <frank.li@nxp.com>; s.hauer@pengutronix.de; festevam@gmail.com;
> > > imx@lists.linux.dev; kernel@pengutronix.de;
> > > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v7 05/10] PCI: imx6: Deassert apps_reset in
> > > imx_pcie_deassert_core_reset()
> > >
> > > On Tue, Nov 26, 2024 at 12:03 AM Richard Zhu <hongxing.zhu@nxp.com>
> > > wrote:
> > > >
> > > > Since the apps_reset is asserted in imx_pcie_assert_core_reset(),
> > > > it should be deasserted in imx_pcie_deassert_core_reset().
> > > >
> > > > Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Reviewed-by: Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org>
> > > > 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 3538440601a7..413db182ce9f 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -776,6 +776,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
> > > >
> > > >
> > >
> > > Hi Richard,
> > >
> > > I've found that this patch causes a regression on i.MX8MM and
> > > i.MX8MP boards with hotplug capable bridges:
> > > i.MX8MM+PI7C9X2G404EV (this switch does not support hotplug) - no
> > > issues i.MX8MM+PI7C9X2G608GP (hotplug) - fails to reliably enumerate
> > > downstream devices about 80% of the time ^^^ when this occurs
> > > PCI_PRIMARY_BUS (0x18) for the root complex
> > > 0000:00:00.0 reads 0x00000000 instead of 0x00ff0100
> > > (PCI_SECONDARY_BUS is 0 instead of 1 and PCI_SUBBORDINATE_BUS is 0
> > > instead of 0xff) i.MX8MP+PI7C9X2G608GP (hotplug) - hangs at
> > > imx_pcie_ltssm_enable deassert apps_reset
> > >
> > > In both cases here reverting ef61c7d8d032 ("PCI: imx6: Deassert
> > > apps_reset in imx_pcie_deassert_core_reset()") resolves this.
> > >
> > [Richard Zhu] I'm afraid that the ltssm_en bit assert to 1b'1 in
> >  imx_pcie_deassert_core_reset() is not correct in your use case.
> >
>
> Hi Richard,
>
> Thanks for your quick response. Do you mean not correct for newer IP core in
> i.MX8MM/i.MX8MP or in the case of a bridge?
[Richard Zhu] What's I mean is that the ltssm_en should be asserted in
 imx_pcie_star_link().
>
> > Actually, the apps_reset isn't a real reset. It's the ltssm_en bit.
> > From this perspective view, It's inappropriate to toggle the ltssm_en
> > bit in
> >  imx_pcie_assert/deassert_core_reset() functions.
> > I consider to move the apps_reset out of _reset_ functions.
> > Can you help to test the following changes in you use-case?
> >
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -776,7 +776,6 @@ static int imx7d_pcie_core_reset(struct imx_pcie
> > *imx_pcie, bool assert)  static void imx_pcie_assert_core_reset(struct
> > imx_pcie *imx_pcie)  {
> >         reset_control_assert(imx_pcie->pciephy_reset);
> > -       reset_control_assert(imx_pcie->apps_reset);
> >
> >         if (imx_pcie->drvdata->core_reset)
> >                 imx_pcie->drvdata->core_reset(imx_pcie, true); @@
> > -788,7 +787,6 @@ 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); @@
> > -1176,6 +1174,9 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
> >                 }
> >         }
> >
> > +       /* Make sure that PCIe LTSSM is cleared */
> > +       imx_pcie_ltssm_disable(dev);
> > +
> >         ret = imx_pcie_deassert_core_reset(imx_pcie);
> >         if (ret < 0) {
> >                 dev_err(dev, "pcie deassert core reset failed: %d\n",
> > ret);
> >
>
> Yes this resolves the regression of failing to reliably enumerate downstream
> devices. I think that should be submitted with a fixes tag.
[Richard Zhu] Thanks. That's right.

Best Regards
Richard
>
> The i.MX8MP+PI7C9X2G608GP switch hanging issue was hardware related...
> i was sadly testing on an old board with a defect. I did previously have a hang
> issue there discussed previously here [1] but it was resolved with commit
> 9c03e30e3ade ("PCI: imx6: Skip link up workaround for newer platforms").
>
> How much testing is done with i.MX8M{M,P} board with a switch? I feel like
> I'm the only one with these SoC's and a switch and I need to get better at
> monitoring patches to the IMX6 PCI controller driver and testing these
> scenarios.
>
> Best Regards,
>
> Tim
> [1]
> https://www.s/
> pinics.net%2Flists%2Flinux-pci%2Fmsg142764.html&data=05%7C02%7Chong
> xing.zhu%40nxp.com%7C2fd64741e96e47efc71208dda7b522f6%7C686ea1d
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638851118601391191%7CUnk
> nown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwM
> CIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C
> &sdata=01lfNA%2BfX8dgBStQ6Q7QhcsAU8gVWD0I4kJjJ6SnLck%3D&reserve
> d=0
>
>
>
>
> > > I notice the sequence of events here is:
> > > imx_pcie_assert_core_reset asserts apps_reset (disables LTSSM)
> > > imx_pcie_deassert_core_reset deasserts apps_reset (enables LTSSM)
> > > imx_pcie_ltssm_enable deasserts apps_reset (enables LTSSM; this is
> > > where it hangs on imx8mp)
> > >
> > > Is there perhaps some issue with de-asserting this (enabling LTSSM)
> > > when it's already in this state?
> > [Richard Zhu]The apps_reset is updated by src driver by
> regmap_update_bits().
> > I can't find the exceptions to update one bit, already has the according
> value.
> >
> > Best Regards
> > Richard Zhu
> > >
> > > In the case where downstream devices do not enumerate some
> > > investigation points to them not being happy that the link drops so
> > > perhaps deasserting apps_reset when its already asserted drops the link
> and restarts it?
> > >
> > > Best Regards,
> > >
> > > Tim