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

[PATCH v6 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.

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 v6 01/10] dt-bindings: imx6q-pcie: Add ref clock for i.MX95
[PATCH v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
[PATCH v6 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT
[PATCH v6 04/10] PCI: imx6: Correct controller_id generation logic
[PATCH v6 05/10] PCI: imx6: Make core reset assertion deassertion
[PATCH v6 06/10] PCI: imx6: Fix the missing reference clock disable
[PATCH v6 07/10] PCI: imx6: Clean up codes by removing
[PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method
[PATCH v6 09/10] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 PCIe PM
[PATCH v6 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                         |  18 ++++++--
drivers/pci/controller/dwc/pci-imx6.c                            | 174 ++++++++++++++++++++++++++++------------------------------------------------
5 files changed, 102 insertions(+), 120 deletions(-)

[PATCH v6 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 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Richard Zhu]

Add "ref" clock to enable reference clock. To avoid the DT
compatibility, i.MX95 REF clock might be optional. Replace the
devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 808d1f105417..bc8567677a67 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -82,6 +82,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)
 
@@ -98,6 +99,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];
@@ -1278,9 +1280,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 ret, i, req_cnt;
 	u16 val;
-	int i;
 
 	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
 	if (!imx_pcie)
@@ -1330,7 +1331,10 @@ 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);
+	ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt,
+					  imx_pcie->clks + req_cnt);
 	if (ret)
 		return ret;
 
@@ -1480,6 +1484,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] = {
@@ -1592,9 +1597,11 @@ 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),
+		.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,
 		.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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote:
> Add "ref" clock to enable reference clock. To avoid the DT
> compatibility, i.MX95 REF clock might be optional.

Your wording is not correct. Perhaps you wanted to say, "To avoid breaking DT
backwards compatibility"?

> Replace the
> devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 808d1f105417..bc8567677a67 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -82,6 +82,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)
>  
> @@ -98,6 +99,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];
> @@ -1278,9 +1280,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 ret, i, req_cnt;
>  	u16 val;
> -	int i;
>  
>  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
>  	if (!imx_pcie)
> @@ -1330,7 +1331,10 @@ 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);
> +	ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt,
> +					  imx_pcie->clks + req_cnt);

Why do you need to use 'clk_bulk' API to get a single reference clock? Just use
devm_clk_get_optional(dev, "ref")

And who is going to supply the reference clock in the absence of this clockn in
DT?

- Mani

>  	if (ret)
>  		return ret;
>  
> @@ -1480,6 +1484,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] = {
> @@ -1592,9 +1597,11 @@ 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),
> +		.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,
>  		.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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年11月15日 14:38
> 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> 
> On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote:
> > Add "ref" clock to enable reference clock. To avoid the DT
> > compatibility, i.MX95 REF clock might be optional.
> 
> Your wording is not correct. Perhaps you wanted to say, "To avoid breaking
> DT backwards compatibility"?
> 
Yes, you're right. Thanks.

> > Replace the
> > devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 808d1f105417..bc8567677a67 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -82,6 +82,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)
> >
> > @@ -98,6 +99,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]; @@ -1278,9 +1280,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 ret, i, req_cnt;
> >  	u16 val;
> > -	int i;
> >
> >  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
> >  	if (!imx_pcie)
> > @@ -1330,7 +1331,10 @@ 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);
> > +	ret |= devm_clk_bulk_get_optional(dev,
> imx_pcie->drvdata->clks_optional_cnt,
> > +					  imx_pcie->clks + req_cnt);
> 
> Why do you need to use 'clk_bulk' API to get a single reference clock? Just
> use devm_clk_get_optional(dev, "ref")
It's easier to add more optional clks in future. I can change to use
devm_clk_get_optional(dev, "ref") here if you insistent.

> 
> And who is going to supply the reference clock in the absence of this clockn in
> DT?
When the "preview" version firmware is used, this clock is gated on in default.
In this case, hiso-blk-ctrl would gated on this clock in default state.

Best Regards
Richard Zhu
> 
> - Mani
> 
> >  	if (ret)
> >  		return ret;
> >
> > @@ -1480,6 +1484,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] = {
> > @@ -1592,9 +1597,11 @@ 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),
> > +		.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,
> >  		.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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Hongxing Zhu]

> -----Original Message-----
> From: Hongxing Zhu
> Sent: 2024年11月18日 10:59
> To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 2024年11月15日 14:38
> > 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> >
> > On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote:
> > > Add "ref" clock to enable reference clock. To avoid the DT
> > > compatibility, i.MX95 REF clock might be optional.
> >
> > Your wording is not correct. Perhaps you wanted to say, "To avoid
> > breaking DT backwards compatibility"?
> >
> Yes, you're right. Thanks.
> 
> > > Replace the
> > > devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 808d1f105417..bc8567677a67 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -82,6 +82,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)
> > >
> > > @@ -98,6 +99,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]; @@ -1278,9
> +1280,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 ret, i, req_cnt;
> > >  	u16 val;
> > > -	int i;
> > >
> > >  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
> > >  	if (!imx_pcie)
> > > @@ -1330,7 +1331,10 @@ 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);
> > > +	ret |= devm_clk_bulk_get_optional(dev,
> > imx_pcie->drvdata->clks_optional_cnt,
> > > +					  imx_pcie->clks + req_cnt);
> >
> > Why do you need to use 'clk_bulk' API to get a single reference clock?
> > Just use devm_clk_get_optional(dev, "ref")
> It's easier to add more optional clks in future. I can change to use
> devm_clk_get_optional(dev, "ref") here if you insistent.
Since the clock fetch is not distinguished by platforms explicitly.
devm_clk_get_optional(dev, "ref") can be used only when i.MX95 specification
 is added.
-       ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt,
-                                         imx_pcie->clks + req_cnt);
        if (ret)
                return ret;
+       for (i = 0; i < imx_pcie->drvdata->clks_optional_cnt; i++) {
+               imx_pcie->clks[req_cnt + i].clk = devm_clk_get_optional(dev,
+                               imx_pcie->drvdata->clk_names[req_cnt + i]);
+               if (IS_ERR(imx_pcie->clks[req_cnt + i].clk))
+                       return PTR_ERR(imx_pcie->clks[req_cnt + i].clk);
+       }

Or
-       ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt,
-                                         imx_pcie->clks + req_cnt);
        if (ret)
                return ret;
+       if (imx_pcie->drvdata->variant == IMX95) {
+               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);
+       }

Which one is better of these two changes?
Or
To keep codes simple and aligned, how about to keep the original one?

Best Regards
Richard Zhu

> 
> >
> > And who is going to supply the reference clock in the absence of this
> > clockn in DT?
> When the "preview" version firmware is used, this clock is gated on in default.
> In this case, hiso-blk-ctrl would gated on this clock in default state.
> 
> Best Regards
> Richard Zhu
> >
> > - Mani
> >
> > >  	if (ret)
> > >  		return ret;
> > >
> > > @@ -1480,6 +1484,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] = {
> > > @@ -1592,9 +1597,11 @@ 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),
> > > +		.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,
> > >  		.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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Tue, Nov 19, 2024 at 05:38:30AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Hongxing Zhu
> > Sent: 2024年11月18日 10:59
> > To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> > 
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Sent: 2024年11月15日 14:38
> > > 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 v6 02/10] PCI: imx6: Add ref clock for i.MX95 PCIe
> > >
> > > On Fri, Nov 01, 2024 at 03:06:02PM +0800, Richard Zhu wrote:
> > > > Add "ref" clock to enable reference clock. To avoid the DT
> > > > compatibility, i.MX95 REF clock might be optional.
> > >
> > > Your wording is not correct. Perhaps you wanted to say, "To avoid
> > > breaking DT backwards compatibility"?
> > >
> > Yes, you're right. Thanks.
> > 
> > > > Replace the
> > > > devm_clk_bulk_get() by devm_clk_bulk_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 | 19 +++++++++++++------
> > > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > > index 808d1f105417..bc8567677a67 100644
> > > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > > @@ -82,6 +82,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)
> > > >
> > > > @@ -98,6 +99,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]; @@ -1278,9
> > +1280,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 ret, i, req_cnt;
> > > >  	u16 val;
> > > > -	int i;
> > > >
> > > >  	imx_pcie = devm_kzalloc(dev, sizeof(*imx_pcie), GFP_KERNEL);
> > > >  	if (!imx_pcie)
> > > > @@ -1330,7 +1331,10 @@ 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);
> > > > +	ret |= devm_clk_bulk_get_optional(dev,
> > > imx_pcie->drvdata->clks_optional_cnt,
> > > > +					  imx_pcie->clks + req_cnt);
> > >
> > > Why do you need to use 'clk_bulk' API to get a single reference clock?
> > > Just use devm_clk_get_optional(dev, "ref")
> > It's easier to add more optional clks in future. I can change to use
> > devm_clk_get_optional(dev, "ref") here if you insistent.
> Since the clock fetch is not distinguished by platforms explicitly.
> devm_clk_get_optional(dev, "ref") can be used only when i.MX95 specification
>  is added.
> -       ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt,
> -                                         imx_pcie->clks + req_cnt);
>         if (ret)
>                 return ret;
> +       for (i = 0; i < imx_pcie->drvdata->clks_optional_cnt; i++) {
> +               imx_pcie->clks[req_cnt + i].clk = devm_clk_get_optional(dev,
> +                               imx_pcie->drvdata->clk_names[req_cnt + i]);
> +               if (IS_ERR(imx_pcie->clks[req_cnt + i].clk))
> +                       return PTR_ERR(imx_pcie->clks[req_cnt + i].clk);
> +       }
> 
> Or
> -       ret |= devm_clk_bulk_get_optional(dev, imx_pcie->drvdata->clks_optional_cnt,
> -                                         imx_pcie->clks + req_cnt);
>         if (ret)
>                 return ret;
> +       if (imx_pcie->drvdata->variant == IMX95) {

Why do you need this check? If there is no clock, devm_clk_get_optional() will
return NULL, so you can just do IS_ERR() without any checks.

- Mani

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

[PATCH v6 03/10] 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.

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>
---
 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 bc8567677a67..462decd1d589 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1115,7 +1115,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;
@@ -1125,25 +1124,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 v6 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:03PM +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.
> 

It'd be useful to mention where the base addresses were extraced. Like by the
DWC common driver.

> 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>

- Mani

> ---
>  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 bc8567677a67..462decd1d589 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1115,7 +1115,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;
> @@ -1125,25 +1124,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 v6 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年11月15日 14:41
> 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 v6 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses
> from DT
> 
> On Fri, Nov 01, 2024 at 03:06:03PM +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.
> >
> 
> It'd be useful to mention where the base addresses were extraced. Like by
> the DWC common driver.
You're right. How about change them to the below one?
The dw_pcie_get_resources() function of DWC core codes can fetch the dbi2 and
 iATU base addresses from DT directly, and remove the useless codes here.

> 
> > 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>
> 
> - Mani
> 
> > ---
> >  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 bc8567677a67..462decd1d589 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1115,7 +1115,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;
> > @@ -1125,25 +1124,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;

The check and the NULL assignment of "pci->dbi_base2" should be removed too
 refer to FIXME listed above. Would updated in v7 later, Sorry about that.

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

Re: [PATCH v6 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses from DT

[Manivannan Sadhasivam]

On Mon, Nov 18, 2024 at 02:59:35AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 2024年11月15日 14:41
> > 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 v6 03/10] PCI: imx6: Fetch dbi2 and iATU base addesses
> > from DT
> > 
> > On Fri, Nov 01, 2024 at 03:06:03PM +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.
> > >
> > 
> > It'd be useful to mention where the base addresses were extraced. Like by
> > the DWC common driver.
> You're right. How about change them to the below one?
> The dw_pcie_get_resources() function of DWC core codes can fetch the dbi2 and
>  iATU base addresses from DT directly, and remove the useless codes here.

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

- Mani

> 
> > 
> > > 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>
> > 
> > - Mani
> > 
> > > ---
> > >  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 bc8567677a67..462decd1d589 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -1115,7 +1115,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;
> > > @@ -1125,25 +1124,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;
> 
> The check and the NULL assignment of "pci->dbi_base2" should be removed too
>  refer to FIXME listed above. Would updated in v7 later, Sorry about that.
> 
> Best Regards
> Richard Zhu
> > >
> > > --
> > > 2.37.1
> > >
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

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

[PATCH v6 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.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
"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"

IMX7D here is wrong athough check IMX8MQ_PCIE2_BASE_ADDR is not good
method. Previously try to use 'linux,pci-domain' to replace this check
logic. Need more discussion to improve it and keep old compatiblity.
Let's fix this code error firstly.
---
 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 462decd1d589..996333e9017d 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1342,7 +1342,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

Re: [PATCH v6 04/10] PCI: imx6: Correct controller_id generation logic for i.MX7D

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:04PM +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.
> 

Worth adding a fixes tag?

> 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>
> ---
> "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"
> 
> IMX7D here is wrong athough check IMX8MQ_PCIE2_BASE_ADDR is not good
> method. Previously try to use 'linux,pci-domain' to replace this check
> logic. Need more discussion to improve it and keep old compatiblity.
> Let's fix this code error firstly.

I really hope that you'll fix it asap.

- Mani

> ---
>  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 462decd1d589..996333e9017d 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1342,7 +1342,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
> 

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

RE: [PATCH v6 04/10] PCI: imx6: Correct controller_id generation logic for i.MX7D

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年11月15日 14:43
> 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 v6 04/10] PCI: imx6: Correct controller_id generation
> logic for i.MX7D
> 
> On Fri, Nov 01, 2024 at 03:06:04PM +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.
> >
> 
> Worth adding a fixes tag?
> 
It's originated from commit 2d8ed461dbc9 ("PCI: imx6: Add support for i.MX8MQ")

How about to add one Fixes tag like this?
Fixes: commit 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>
> > ---
> > "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"
> >
> > IMX7D here is wrong athough check IMX8MQ_PCIE2_BASE_ADDR is not
> good
> > method. Previously try to use 'linux,pci-domain' to replace this check
> > logic. Need more discussion to improve it and keep old compatiblity.
> > Let's fix this code error firstly.
> 
> I really hope that you'll fix it asap.
Got that.

Best Regards
Richard Zhu
> 
> - Mani
> 
> > ---
> >  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 462decd1d589..996333e9017d 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -1342,7 +1342,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
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v6 04/10] PCI: imx6: Correct controller_id generation logic for i.MX7D

[Manivannan Sadhasivam]

On Mon, Nov 18, 2024 at 02:59:48AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 2024年11月15日 14:43
> > 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 v6 04/10] PCI: imx6: Correct controller_id generation
> > logic for i.MX7D
> > 
> > On Fri, Nov 01, 2024 at 03:06:04PM +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.
> > >
> > 
> > Worth adding a fixes tag?
> > 
> It's originated from commit 2d8ed461dbc9 ("PCI: imx6: Add support for i.MX8MQ")
> 
> How about to add one Fixes tag like this?
> Fixes: commit 2d8ed461dbc9 ("PCI: imx6: Add support for i.MX8MQ")
> 

Okay.

- Mani

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

[PATCH v6 05/10] 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().

In the commit first introduced apps_reset, apps_reset is asserted in
imx6_pcie_assert_core_reset(), but it is de-asserted in another place, in
stead of the according symmetric function imx6_pcie_deassert_core_reset().

Use this patch to fix it, and make core reset assertion deasertion
symmetric.

Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
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 996333e9017d..54039d2760d5 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -772,6 +772,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 v6 05/10] PCI: imx6: Make core reset assertion deassertion symmetric

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:05PM +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().
> 
> In the commit first introduced apps_reset, apps_reset is asserted in
> imx6_pcie_assert_core_reset(), but it is de-asserted in another place, in

I'd suggest rewording like below to make it easy to understand,

"PCI: imx6: Deassert apps_reset in imx_pcie_assert_core_reset()

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

> stead of the according symmetric function imx6_pcie_deassert_core_reset().
> 
> Use this patch to fix it, and make core reset assertion deasertion
> symmetric.
> 
> Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

With above change,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> 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 996333e9017d..54039d2760d5 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -772,6 +772,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 v6 05/10] PCI: imx6: Make core reset assertion deassertion symmetric

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年11月15日 14:52
> 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 v6 05/10] PCI: imx6: Make core reset assertion
> deassertion symmetric
> 
> On Fri, Nov 01, 2024 at 03:06:05PM +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().
> >
> > In the commit first introduced apps_reset, apps_reset is asserted in
> > imx6_pcie_assert_core_reset(), but it is de-asserted in another place,
> > in
> 
> I'd suggest rewording like below to make it easy to understand,
> 
> "PCI: imx6: Deassert apps_reset in imx_pcie_assert_core_reset()
I'm very appreciate for your rewords. Should the imx_pcie_assert_core_reset()
 be imx_pcie_deassert_core_reset()?

Best Regards
Richard Zhu
> 
> Since the apps_reset is asserted in imx_pcie_assert_core_reset(), it should be
> deasserted in imx_pcie_deassert_core_reset()."
> 
> > stead of the according symmetric function
> imx6_pcie_deassert_core_reset().
> >
> > Use this patch to fix it, and make core reset assertion deasertion
> > symmetric.
> >
> > Fixes: 9b3fe6796d7c ("PCI: imx6: Add code to support i.MX7D")
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> 
> With above change,
> 
> Reviewed-by: Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org>
> 
> - Mani
> 
> > 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 996333e9017d..54039d2760d5 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -772,6 +772,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 v6 05/10] PCI: imx6: Make core reset assertion deassertion symmetric

[Manivannan Sadhasivam]

On Mon, Nov 18, 2024 at 02:59:59AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 2024年11月15日 14:52
> > 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 v6 05/10] PCI: imx6: Make core reset assertion
> > deassertion symmetric
> > 
> > On Fri, Nov 01, 2024 at 03:06:05PM +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().
> > >
> > > In the commit first introduced apps_reset, apps_reset is asserted in
> > > imx6_pcie_assert_core_reset(), but it is de-asserted in another place,
> > > in
> > 
> > I'd suggest rewording like below to make it easy to understand,
> > 
> > "PCI: imx6: Deassert apps_reset in imx_pcie_assert_core_reset()
> I'm very appreciate for your rewords. Should the imx_pcie_assert_core_reset()
>  be imx_pcie_deassert_core_reset()?
> 

Yeah!

- Mani

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

[PATCH v6 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: 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 54039d2760d5..bb130c84c016 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -595,10 +595,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;
 }
 
@@ -627,19 +626,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 v6 06/10] PCI: imx6: Fix the missing reference clock disable logic

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:06PM +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")
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> 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 54039d2760d5..bb130c84c016 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -595,10 +595,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;
>  }
>  
> @@ -627,19 +626,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 v6 07/10] PCI: imx6: Clean up codes by removing imx7d_pcie_init_phy()

[Richard Zhu]

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 | 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 bb130c84c016..fde2f4eaf804 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -390,13 +390,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,
@@ -1526,7 +1519,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

Re: [PATCH v6 07/10] PCI: imx6: Clean up codes by removing imx7d_pcie_init_phy()

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:07PM +0800, Richard Zhu wrote:

> Remove the duplicate imx7d_pcie_init_phy() function as it is the same as
> imx7d_pcie_enable_ref_clk().
> 

How about.

"PCI: imx6: Remove imx7d_pcie_init_phy() function"

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

> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

With above change,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> 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 bb130c84c016..fde2f4eaf804 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -390,13 +390,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,
> @@ -1526,7 +1519,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 v6 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.

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

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index fde2f4eaf804..3c074cc2605f 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)
@@ -108,19 +109,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;
@@ -899,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));
@@ -1024,9 +1022,32 @@ static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
 	return cpu_addr - entry->offset;
 }
 
+/*
+ * 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 = {
@@ -1147,43 +1168,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;
@@ -1207,36 +1191,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 +1241,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) {
@@ -1343,13 +1320,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 =
@@ -1428,6 +1398,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;
@@ -1491,6 +1462,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,
@@ -1508,6 +1480,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

Re: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[kernel test robot]

Hi Richard,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus shawnguo/for-next mani-mhi/mhi-next linus/master v6.12-rc6 next-20241104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Zhu/dt-bindings-imx6q-pcie-Add-ref-clock-for-i-MX95-PCIe-RC/20241101-150006
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20241101070610.1267391-9-hongxing.zhu%40nxp.com
patch subject: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method
config: i386-buildonly-randconfig-004-20241104 (https://download.01.org/0day-ci/archive/20241104/202411042110.bIIyBWlv-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241104/202411042110.bIIyBWlv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411042110.bIIyBWlv-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx_pcie_resume_noirq':
>> pci-imx6.c:(.text+0x88): undefined reference to `dw_pcie_resume_noirq'
   ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx_pcie_suspend_noirq':
>> pci-imx6.c:(.text+0x111): undefined reference to `dw_pcie_suspend_noirq'
   ld: drivers/usb/misc/onboard_usb_dev.o: in function `onboard_dev_probe':
   onboard_usb_dev.c:(.text+0x6ae): undefined reference to `i2c_find_device_by_fwnode'
   ld: onboard_usb_dev.c:(.text+0x711): undefined reference to `i2c_smbus_write_block_data'
   ld: onboard_usb_dev.c:(.text+0x72f): undefined reference to `i2c_smbus_write_word_data'
   ld: onboard_usb_dev.c:(.text+0x74c): undefined reference to `i2c_smbus_write_word_data'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MODVERSIONS
   Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
   Selected by [y]:
   - RANDSTRUCT_PERFORMANCE [=y] && GCC_PLUGINS [=y] && MODULES [=y]
   WARNING: unmet direct dependencies detected for FB_IOMEM_HELPERS
   Depends on [n]: HAS_IOMEM [=y] && FB_CORE [=n]
   Selected by [m]:
   - DRM_XE_DISPLAY [=y] && HAS_IOMEM [=y] && DRM [=m] && DRM_XE [=m] && DRM_XE [=m]=m [=m]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:08PM +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. 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.
> 
> 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();
> 

Are these two changes (dw_pcie_suspend_noirq(), dw_pcie_resume_noirq()) related
to this patch? If not, these should be in a separate patch.

> 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 | 95 ++++++++++-----------------
>  1 file changed, 34 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index fde2f4eaf804..3c074cc2605f 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)
> @@ -108,19 +109,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;
> @@ -899,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));
> @@ -1024,9 +1022,32 @@ static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
>  	return cpu_addr - entry->offset;
>  }
>  
> +/*
> + * 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);
> +}
> +
> +

Stray newline.

>  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 = {
> @@ -1147,43 +1168,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);

Where these are handled in imx_pcie_pme_turn_off()? If you removed them
intentionally for a reason, it should be mentioned in commit message. 

> -		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;
> @@ -1207,36 +1191,26 @@ static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save)

[...]

> @@ -1267,11 +1241,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;

Use if..else pattern

> +
>  	/* Find the PHY if one is defined, only imx7d uses it */
>  	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
>  	if (np) {
> @@ -1343,13 +1320,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);
> -	}
> -

Same here. Reason not explained.

- Mani

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

Re: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[Frank Li]

On Fri, Nov 15, 2024 at 12:39:32PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Nov 01, 2024 at 03:06:08PM +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. 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.
> >
> > 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();
> >
>
> Are these two changes (dw_pcie_suspend_noirq(), dw_pcie_resume_noirq()) related
> to this patch? If not, these should be in a separate patch.


Sorry, this patch have not touch dw_pcie_suspend_noirq() and
dw_pcie_resume_noirq()'s implement, just call it. I have not understood
what's your means.

>
> > 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 | 95 ++++++++++-----------------
> >  1 file changed, 34 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index fde2f4eaf804..3c074cc2605f 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)
> > @@ -108,19 +109,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;
> > @@ -899,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));
> > @@ -1024,9 +1022,32 @@ static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> >  	return cpu_addr - entry->offset;
> >  }
> >
> > +/*
> > + * 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);
> > +}
> > +
> > +
>
> Stray newline.
>
> >  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 = {
> > @@ -1147,43 +1168,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);
>
> Where these are handled in imx_pcie_pme_turn_off()? If you removed them
> intentionally for a reason, it should be mentioned in commit message.
>
> > -		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;
> > @@ -1207,36 +1191,26 @@ static void imx_pcie_msi_save_restore(struct imx_pcie *imx_pcie, bool save)
>
> [...]
>
> > @@ -1267,11 +1241,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;
>
> Use if..else pattern
>
> > +
> >  	/* Find the PHY if one is defined, only imx7d uses it */
> >  	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
> >  	if (np) {
> > @@ -1343,13 +1320,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);
> > -	}
> > -
>
> Same here. Reason not explained.

"Some platforms wrongly use
devm_reset_control_get_optional_exclusive(dev, "turnoff")
reset_control_assert(imx_pcie->turnoff_reset) and
reset_control_deassert(imx_pcie->turnoff_reset) to send out PME_TURN_OFF
messge, after change to common API to do that, so remove these wrong
implment."

Is it okay to put above into commit message?

Frank



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

Re: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[Manivannan Sadhasivam]

On Fri, Nov 15, 2024 at 12:38:33PM -0500, Frank Li wrote:
> On Fri, Nov 15, 2024 at 12:39:32PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Nov 01, 2024 at 03:06:08PM +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. 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.
> > >
> > > 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();
> > >
> >
> > Are these two changes (dw_pcie_suspend_noirq(), dw_pcie_resume_noirq()) related
> > to this patch? If not, these should be in a separate patch.
> 
> 
> Sorry, this patch have not touch dw_pcie_suspend_noirq() and
> dw_pcie_resume_noirq()'s implement, just call it. I have not understood
> what's your means.
> 

Sorry, I got confused. Please ignore above comment.

- Mani

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

RE: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年11月15日 15:10
> 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 v6 08/10] PCI: imx6: Use dwc common suspend resume
> method
> 
> On Fri, Nov 01, 2024 at 03:06:08PM +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. 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.
> >
> > 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();
> >
> 
> Are these two changes (dw_pcie_suspend_noirq(), dw_pcie_resume_noirq())
> related to this patch? If not, these should be in a separate patch.
> 
This commit is used to simple imx_pcie_suspend_noirq() and
 imx_pcie_resume_noirq() by these replaces.

> > 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 | 95
> > ++++++++++-----------------
> >  1 file changed, 34 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index fde2f4eaf804..3c074cc2605f 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)
> > @@ -108,19 +109,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;
> > @@ -899,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)); @@ -1024,9
> +1022,32 @@
> > static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> >  	return cpu_addr - entry->offset;
> >  }
> >
> > +/*
> > + * 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); }
> > +
> > +
> 
> Stray newline.
Good caught. Thanks.
> 
> >  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 = { @@ -1147,43 +1168,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);
> 
> Where these are handled in imx_pcie_pme_turn_off()? If you removed them
> intentionally for a reason, it should be mentioned in commit message.
> 
How about add the following descriptions into commit message?
SRC interface is used to do the PME_TURN_OFF operations before. It's not very
 suitable. Now DWC common driver can do the PME_TURN_OFF kick off. Switch to
 this common methods, and remove the useless turnoff_reset manipulate codes.

> > -		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;
> > @@ -1207,36 +1191,26 @@ static void imx_pcie_msi_save_restore(struct
> > imx_pcie *imx_pcie, bool save)
> 
> [...]
> 
> > @@ -1267,11 +1241,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;
> 
> Use if..else pattern
Okay.
> 
> > +
> >  	/* Find the PHY if one is defined, only imx7d uses it */
> >  	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
> >  	if (np) {
> > @@ -1343,13 +1320,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);
> > -	}
> > -
> 
> Same here. Reason not explained.
See above.

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

Re: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[Manivannan Sadhasivam]

On Mon, Nov 18, 2024 at 03:00:44AM +0000, Hongxing Zhu wrote:

[...]

> > > -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);
> > 
> > Where these are handled in imx_pcie_pme_turn_off()? If you removed them
> > intentionally for a reason, it should be mentioned in commit message.
> > 
> How about add the following descriptions into commit message?
> SRC interface is used to do the PME_TURN_OFF operations before. It's not very

What is SRC?

>  suitable. Now DWC common driver can do the PME_TURN_OFF kick off. Switch to
>  this common methods, and remove the useless turnoff_reset manipulate codes.
> 

Hmm, so 'turnoff_reset' is used to send PME_Turn_Off msg?

If so, then you need to say in such a way that the reader should understand
'turnoff_reset' was used to send PME_Turn_Off and since the DWC implementation
is used, it is not needed now.

- Mani

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

RE: [PATCH v6 08/10] PCI: imx6: Use dwc common suspend resume method

[Hongxing Zhu]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 2024年11月23日 0:47
> 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 v6 08/10] PCI: imx6: Use dwc common suspend resume
> method
> 
> On Mon, Nov 18, 2024 at 03:00:44AM +0000, Hongxing Zhu wrote:
> 
> [...]
> 
> > > > -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);
> > >
> > > Where these are handled in imx_pcie_pme_turn_off()? If you removed
> > > them intentionally for a reason, it should be mentioned in commit
> message.
> > >
> > How about add the following descriptions into commit message?
> > SRC interface is used to do the PME_TURN_OFF operations before. It's
> > not very
> 
> What is SRC?
SRC is System Reset Control.
> 
> >  suitable. Now DWC common driver can do the PME_TURN_OFF kick off.
> > Switch to  this common methods, and remove the useless turnoff_reset
> manipulate codes.
> >
> 
> Hmm, so 'turnoff_reset' is used to send PME_Turn_Off msg?
Yes, it is.
> 
> If so, then you need to say in such a way that the reader should understand
> 'turnoff_reset' was used to send PME_Turn_Off and since the DWC
> implementation is used, it is not needed now.
Okay. Thanks.

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

[PATCH v6 09/10] 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3c074cc2605f..cf2a9918537e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1498,7 +1498,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),
@@ -1536,7 +1537,8 @@ 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),
 	},
-- 
2.37.1

Re: [PATCH v6 09/10] PCI: imx6: Add i.MX8MQ i.MX8Q and i.MX95 PCIe PM support

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:09PM +0800, Richard Zhu wrote:

In subject, do not use "PCIe PM" as it could imply "PCI-PM". Just use "PM"
instead. Prefix already mentioned that it is a PCIe driver.

> 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>

- Mani

> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 3c074cc2605f..cf2a9918537e 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1498,7 +1498,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),
> @@ -1536,7 +1537,8 @@ 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),
>  	},
> -- 
> 2.37.1
> 

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

[PATCH v6 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 | 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 v6 10/10] arm64: dts: imx95: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 03:06:10PM +0800, Richard Zhu wrote:
> 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 | 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>;

What does this 'dummy' clock do? Looks like it doesn't have a frequency at all.
Is bootloader updating it? But the name looks wierd.

- Mani

> +			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 v6 10/10] arm64: dts: imx95: Add ref clock for i.MX95 PCIe

[Frank Li]

On Fri, Nov 15, 2024 at 12:46:05PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Nov 01, 2024 at 03:06:10PM +0800, Richard Zhu wrote:
> > 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 | 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>;
>
> What does this 'dummy' clock do? Looks like it doesn't have a frequency at all.
> Is bootloader updating it? But the name looks wierd.

dummy clock is not used for this instance, which needn't at all. Leave here
just keep compatible with the other instance.

Some instance of "nxp,imx95-hsio-blk-ctl" required input clocks. but this
one is not, so put dummy here.

Frank
>
> - Mani
>
> > +			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 v6 10/10] arm64: dts: imx95: Add ref clock for i.MX95 PCIe

[Manivannan Sadhasivam]

On Fri, Nov 15, 2024 at 12:28:50PM -0500, Frank Li wrote:
> On Fri, Nov 15, 2024 at 12:46:05PM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Nov 01, 2024 at 03:06:10PM +0800, Richard Zhu wrote:
> > > 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 | 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>;
> >
> > What does this 'dummy' clock do? Looks like it doesn't have a frequency at all.
> > Is bootloader updating it? But the name looks wierd.
> 
> dummy clock is not used for this instance, which needn't at all. Leave here
> just keep compatible with the other instance.
> 
> Some instance of "nxp,imx95-hsio-blk-ctl" required input clocks. but this
> one is not, so put dummy here.
> 

DT should describe the hardware and hardware cannot have dummy clock. If the IP
requires a clock, then pass relevant clock (even if it is a fixed-clock).

- Mani

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