[PATCH v7 0/8] Add the imx8m pcie phy driver and imx8mm pcie support

[PATCH v7 0/8] Add the imx8m pcie phy driver and imx8mm pcie support

[Richard Zhu]

Refer to the discussion [1] when try to enable i.MX8MM PCIe support,
one standalone PCIe PHY driver should be seperated from i.MX PCIe
driver when enable i.MX8MM PCIe support.

This patch-set adds the standalone PCIe PHY driver suport[1-5], and i.MX8MM
PCIe support[6-8] to have whole view to review this patch-set.

The PCIe works on i.MX8MM EVK board based the the blkctrl power driver
[2] and this patch-set. And tested by Tim and Marcel on the different
reference clock modes boards.

[1] https://patchwork.ozlabs.org/project/linux-pci/patch/20210510141509.929120-3-l.stach@pengutronix.de/
[2] https://patchwork.kernel.org/project/linux-arm-kernel/cover/20210910202640.980366-1-l.stach@pengutronix.de/

Main changes v6 --> v7:
- Add "Reviewed-by: Rob Herring <robh@kernel.org>" into #2 patches.
- Regarding Vinod's review comments do the following changes.
  - Don't build in the PHY driver in default.
  - Remove the extra blank line
  - Correct the license tag.

Main changes v5 --> v6:
- Add "Reviewed-by: Rob Herring <robh@kernel.org>" into #1 and #3 patches.
- Merge Rob's review comments to the #2 patch.

Main changes v4 --> v5:
- Set the AUX_EN always 1b'1, thus it can fix the regression introduced in v4
  series on Marcel's board.
- Use the lower-case letter in the devicetreee refer to Marcel's comments.
- Since the default value of the deemphasis parameters are zero, only set
  the deemphasis registers when the input paramters are none zero.

Main changes v3 --> v4:
- Update the yaml to fix syntax error, add maxitems and drop description of phy
- Correct the clock name in PHY DT node.
- Squash the EVK board relalted dts changes into one patch, and drop the
  useless dummy clock and gpio suffix in DT nodes.
- Add board specific de-emphasis parameters as DT properties. Thus each board
  can specify its actual de-emphasis values.
- Update the commit log of PHY driver.
- Remove the useless codes from PCIe driver, since they are moved to PHY driver
- After the discussion and verification of the CLKREQ# configurations with Tim,
  agree to add an optional boolean property "fsl,clkreq-unsupported", indicates
  the CLKREQ# signal is hooked or not in HW designs.
- Add "Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>" tag, since
  Marcel help to test the v3 patch-set.

Main changes v2 --> v3:
- Regarding Lucas' comments.
 - to have a whole view to review the patches, send out the i.MX8MM PCIe support too.
 - move the PHY related bits manipulations of the GPR/SRC to standalone PHY driver.
 - split the dts changes to SOC and board DT, and use the enum instead of raw value.
 - update the license of the dt-binding header file.

Changes v1 --> v2:
- Update the license of the dt-binding header file to make the license
  compatible with dts files.
- Fix the dt_binding_check errors.

Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml    |   6 +++
Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml |  92 +++++++++++++++++++++++++++++++
arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi                |  55 +++++++++++++++++++
arch/arm64/boot/dts/freescale/imx8mm.dtsi                    |  46 +++++++++++++++-
drivers/pci/controller/dwc/pci-imx6.c                        |  83 +++++++++++++++++++++++++---
drivers/phy/freescale/Kconfig                                |   8 +++
drivers/phy/freescale/Makefile                               |   1 +
drivers/phy/freescale/phy-fsl-imx8m-pcie.c                   | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
include/dt-bindings/phy/phy-imx8-pcie.h                      |  14 +++++
9 files changed, 532 insertions(+), 9 deletions(-)

[PATCH v7 1/8] dt-bindings: phy: phy-imx8-pcie: Add binding for the
[PATCH v7 2/8] dt-bindings: phy: Add imx8 pcie phy driver support
[PATCH v7 3/8] dt-bindings: imx6q-pcie: Add PHY phandles and name
[PATCH v7 4/8] arm64: dts: imx8mm: Add the pcie phy support
[PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie
[PATCH v7 6/8] arm64: dts: imx8mm: Add the pcie support
[PATCH v7 7/8] arm64: dts: imx8mm-evk: Add the pcie support on imx8mm
[PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

[PATCH v7 1/8] dt-bindings: phy: phy-imx8-pcie: Add binding for the pad modes of imx8 pcie phy

[Richard Zhu]

Add binding for reference clock PAD modes of the i.MX8 PCIe PHY.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 include/dt-bindings/phy/phy-imx8-pcie.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 include/dt-bindings/phy/phy-imx8-pcie.h

diff --git a/include/dt-bindings/phy/phy-imx8-pcie.h b/include/dt-bindings/phy/phy-imx8-pcie.h
new file mode 100644
index 000000000000..8bbe2d6538d8
--- /dev/null
+++ b/include/dt-bindings/phy/phy-imx8-pcie.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * This header provides constants for i.MX8 PCIe.
+ */
+
+#ifndef _DT_BINDINGS_IMX8_PCIE_H
+#define _DT_BINDINGS_IMX8_PCIE_H
+
+/* Reference clock PAD mode */
+#define IMX8_PCIE_REFCLK_PAD_UNUSED	0
+#define IMX8_PCIE_REFCLK_PAD_INPUT	1
+#define IMX8_PCIE_REFCLK_PAD_OUTPUT	2
+
+#endif /* _DT_BINDINGS_IMX8_PCIE_H */
-- 
2.25.1

[PATCH v7 2/8] dt-bindings: phy: Add imx8 pcie phy driver support

[Richard Zhu]

Add dt-binding for the standalone i.MX8 PCIe PHY driver.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/phy/fsl,imx8-pcie-phy.yaml       | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
new file mode 100644
index 000000000000..b6421eedece3
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/fsl,imx8-pcie-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX8 SoC series PCIe PHY Device Tree Bindings
+
+maintainers:
+  - Richard Zhu <hongxing.zhu@nxp.com>
+
+properties:
+  "#phy-cells":
+    const: 0
+
+  compatible:
+    enum:
+      - fsl,imx8mm-pcie-phy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: ref
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: pciephy
+
+  fsl,refclk-pad-mode:
+    description: |
+      Specifies the mode of the refclk pad used. It can be UNUSED(PHY
+      refclock is derived from SoC internal source), INPUT(PHY refclock
+      is provided externally via the refclk pad) or OUTPUT(PHY refclock
+      is derived from SoC internal source and provided on the refclk pad).
+      Refer include/dt-bindings/phy/phy-imx8-pcie.h for the constants
+      to be used.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1, 2 ]
+
+  fsl,tx-deemph-gen1:
+    description: Gen1 De-emphasis value (optional).
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+
+  fsl,tx-deemph-gen2:
+    description: Gen2 De-emphasis value (optional).
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+
+  fsl,clkreq-unsupported:
+    type: boolean
+    description: A boolean property indicating the CLKREQ# signal is
+      not supported in the board design (optional)
+
+required:
+  - "#phy-cells"
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - fsl,refclk-pad-mode
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    #include <dt-bindings/phy/phy-imx8-pcie.h>
+    #include <dt-bindings/reset/imx8mq-reset.h>
+
+    pcie_phy: pcie-phy@32f00000 {
+            compatible = "fsl,imx8mm-pcie-phy";
+            reg = <0x32f00000 0x10000>;
+            clocks = <&clk IMX8MM_CLK_PCIE1_PHY>;
+            clock-names = "ref";
+            assigned-clocks = <&clk IMX8MM_CLK_PCIE1_PHY>;
+            assigned-clock-rates = <100000000>;
+            assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_100M>;
+            resets = <&src IMX8MQ_RESET_PCIEPHY>;
+            reset-names = "pciephy";
+            fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>;
+            #phy-cells = <0>;
+    };
+...
-- 
2.25.1

[PATCH v7 3/8] dt-bindings: imx6q-pcie: Add PHY phandles and name properties

[Richard Zhu]

i.MX8MM PCIe has the PHY. Add a PHY phandle and name properties
in the binding document.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
index acea1cd444fd..643a6333b07b 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml
@@ -127,6 +127,12 @@ properties:
     enum: [1, 2, 3, 4]
     default: 1
 
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pcie-phy
+
   reset-gpio:
     description: Should specify the GPIO for controlling the PCI bus device
       reset signal. It's not polarity aware and defaults to active-low reset
-- 
2.25.1

[PATCH v7 4/8] arm64: dts: imx8mm: Add the pcie phy support

[Richard Zhu]

Add the PCIe PHY support on iMX8MM platforms.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index c2f3f118f82e..0844f3144887 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -1135,6 +1135,19 @@ usbmisc2: usbmisc@32e50200 {
 				reg = <0x32e50200 0x200>;
 			};
 
+			pcie_phy: pcie-phy@32f00000 {
+				compatible = "fsl,imx8mm-pcie-phy";
+				reg = <0x32f00000 0x10000>;
+				clocks = <&clk IMX8MM_CLK_PCIE1_PHY>;
+				clock-names = "ref";
+				assigned-clocks = <&clk IMX8MM_CLK_PCIE1_PHY>;
+				assigned-clock-rates = <100000000>;
+				assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_100M>;
+				resets = <&src IMX8MQ_RESET_PCIEPHY>;
+				reset-names = "pciephy";
+				#phy-cells = <0>;
+				status = "disabled";
+			};
 		};
 
 		dma_apbh: dma-controller@33000000 {
-- 
2.25.1

[PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

[Richard Zhu]

Add the standalone i.MX8 PCIe PHY driver.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/phy/freescale/Kconfig              |   8 +
 drivers/phy/freescale/Makefile             |   1 +
 drivers/phy/freescale/phy-fsl-imx8m-pcie.c | 236 +++++++++++++++++++++
 3 files changed, 245 insertions(+)
 create mode 100644 drivers/phy/freescale/phy-fsl-imx8m-pcie.c

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 320630ffe3cd..c3669c28ea9f 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -14,3 +14,11 @@ config PHY_MIXEL_MIPI_DPHY
 	help
 	  Enable this to add support for the Mixel DSI PHY as found
 	  on NXP's i.MX8 family of SOCs.
+
+config PHY_FSL_IMX8M_PCIE
+	tristate "Freescale i.MX8M PCIE PHY"
+	depends on OF && HAS_IOMEM
+	select GENERIC_PHY
+	help
+	  Enable this to add support for the PCIE PHY as found on
+	  i.MX8M family of SOCs.
diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
index 1d02e3869b45..55d07c742ab0 100644
--- a/drivers/phy/freescale/Makefile
+++ b/drivers/phy/freescale/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)	+= phy-fsl-imx8mq-usb.o
 obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)	+= phy-fsl-imx8-mipi-dphy.o
+obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)	+= phy-fsl-imx8m-pcie.o
diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
new file mode 100644
index 000000000000..f6502463d49a
--- /dev/null
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
@@ -0,0 +1,236 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/delay.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/imx7-iomuxc-gpr.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <dt-bindings/phy/phy-imx8-pcie.h>
+
+#define IMX8MM_PCIE_PHY_CMN_REG061	0x184
+#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN	BIT(0)
+#define IMX8MM_PCIE_PHY_CMN_REG062	0x188
+#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL	BIT(3)
+#define IMX8MM_PCIE_PHY_CMN_REG063	0x18C
+#define  AUX_PLL_REFCLK_SEL_SYS_PLL	GENMASK(7, 6)
+#define IMX8MM_PCIE_PHY_CMN_REG064	0x190
+#define  ANA_AUX_RX_TX_SEL_TX		BIT(7)
+#define  ANA_AUX_RX_TERM_GND_EN		BIT(3)
+#define  ANA_AUX_TX_TERM		BIT(2)
+#define IMX8MM_PCIE_PHY_CMN_REG065	0x194
+#define  ANA_AUX_RX_TERM		(BIT(7) | BIT(4))
+#define  ANA_AUX_TX_LVL			GENMASK(3, 0)
+#define IMX8MM_PCIE_PHY_CMN_REG75	0x1D4
+#define  PCIE_PHY_CMN_REG75_PLL_DONE	0x3
+#define PCIE_PHY_TRSV_REG5		0x414
+#define  PCIE_PHY_TRSV_REG5_GEN1_DEEMP	0x2D
+#define PCIE_PHY_TRSV_REG6		0x418
+#define  PCIE_PHY_TRSV_REG6_GEN2_DEEMP	0xF
+
+#define IMX8MM_GPR_PCIE_REF_CLK_SEL	GENMASK(25, 24)
+#define IMX8MM_GPR_PCIE_REF_CLK_PLL	FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3)
+#define IMX8MM_GPR_PCIE_REF_CLK_EXT	FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2)
+#define IMX8MM_GPR_PCIE_AUX_EN		BIT(19)
+#define IMX8MM_GPR_PCIE_CMN_RST		BIT(18)
+#define IMX8MM_GPR_PCIE_POWER_OFF	BIT(17)
+#define IMX8MM_GPR_PCIE_SSC_EN		BIT(16)
+#define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE	BIT(9)
+
+struct imx8_pcie_phy {
+	void __iomem		*base;
+	struct clk		*clk;
+	struct phy		*phy;
+	struct regmap		*iomuxc_gpr;
+	struct reset_control	*reset;
+	u32			refclk_pad_mode;
+	u32			tx_deemph_gen1;
+	u32			tx_deemph_gen2;
+	bool			clkreq_unused;
+};
+
+static int imx8_pcie_phy_init(struct phy *phy)
+{
+	int ret;
+	u32 val, pad_mode;
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	reset_control_assert(imx8_phy->reset);
+
+	pad_mode = imx8_phy->refclk_pad_mode;
+	/* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't hooked */
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE,
+			   imx8_phy->clkreq_unused ?
+			   0 : IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_AUX_EN,
+			   IMX8MM_GPR_PCIE_AUX_EN);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_POWER_OFF, 0);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_SSC_EN, 0);
+
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_REF_CLK_SEL,
+			   pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
+			   IMX8MM_GPR_PCIE_REF_CLK_EXT :
+			   IMX8MM_GPR_PCIE_REF_CLK_PLL);
+	usleep_range(100, 200);
+
+	/* Do the PHY common block reset */
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
+			   IMX8MM_GPR_PCIE_CMN_RST,
+			   IMX8MM_GPR_PCIE_CMN_RST);
+	usleep_range(200, 500);
+
+	if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
+		/* Configure the pad as input */
+		val = readl(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+		writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+	} else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
+		/* Configure the PHY to output the refclock via pad */
+		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG062);
+		writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);
+		val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
+		writel(val | ANA_AUX_RX_TERM_GND_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG064);
+		writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG065);
+	}
+
+	/* Tune PHY de-emphasis setting to pass PCIe compliance. */
+	if (imx8_phy->tx_deemph_gen1)
+		writel(imx8_phy->tx_deemph_gen1,
+		       imx8_phy->base + PCIE_PHY_TRSV_REG5);
+	if (imx8_phy->tx_deemph_gen2)
+		writel(imx8_phy->tx_deemph_gen2,
+		       imx8_phy->base + PCIE_PHY_TRSV_REG6);
+
+	reset_control_deassert(imx8_phy->reset);
+
+	/* Polling to check the phy is ready or not. */
+	ret = readl_poll_timeout(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG75,
+				 val, val == PCIE_PHY_CMN_REG75_PLL_DONE,
+				 10, 20000);
+	return ret;
+}
+
+static int imx8_pcie_phy_power_on(struct phy *phy)
+{
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	return clk_prepare_enable(imx8_phy->clk);
+}
+
+static int imx8_pcie_phy_power_off(struct phy *phy)
+{
+	struct imx8_pcie_phy *imx8_phy = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(imx8_phy->clk);
+
+	return 0;
+}
+
+static const struct phy_ops imx8_pcie_phy_ops = {
+	.init		= imx8_pcie_phy_init,
+	.power_on	= imx8_pcie_phy_power_on,
+	.power_off	= imx8_pcie_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int imx8_pcie_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx8_pcie_phy *imx8_phy;
+	struct resource *res;
+
+	imx8_phy = devm_kzalloc(dev, sizeof(*imx8_phy), GFP_KERNEL);
+	if (!imx8_phy)
+		return -ENOMEM;
+
+	/* get PHY refclk pad mode */
+	of_property_read_u32(np, "fsl,refclk-pad-mode",
+			     &imx8_phy->refclk_pad_mode);
+
+	if (of_property_read_u32(np, "fsl,tx-deemph-gen1",
+				 &imx8_phy->tx_deemph_gen1))
+		imx8_phy->tx_deemph_gen1 = 0;
+
+	if (of_property_read_u32(np, "fsl,tx-deemph-gen2",
+				 &imx8_phy->tx_deemph_gen2))
+		imx8_phy->tx_deemph_gen2 = 0;
+
+	if (of_property_read_bool(np, "fsl,clkreq-unsupported"))
+		imx8_phy->clkreq_unused = true;
+	else
+		imx8_phy->clkreq_unused = false;
+
+	imx8_phy->clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(imx8_phy->clk)) {
+		dev_err(dev, "failed to get imx pcie phy clock\n");
+		return PTR_ERR(imx8_phy->clk);
+	}
+
+	/* Grab GPR config register range */
+	imx8_phy->iomuxc_gpr =
+		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+	if (IS_ERR(imx8_phy->iomuxc_gpr)) {
+		dev_err(dev, "unable to find iomuxc registers\n");
+		return PTR_ERR(imx8_phy->iomuxc_gpr);
+	}
+
+	imx8_phy->reset = devm_reset_control_get_exclusive(dev, "pciephy");
+	if (IS_ERR(imx8_phy->reset)) {
+		dev_err(dev, "Failed to get PCIEPHY reset control\n");
+		return PTR_ERR(imx8_phy->reset);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	imx8_phy->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(imx8_phy->base))
+		return PTR_ERR(imx8_phy->base);
+
+	imx8_phy->phy = devm_phy_create(dev, NULL, &imx8_pcie_phy_ops);
+	if (IS_ERR(imx8_phy->phy))
+		return PTR_ERR(imx8_phy->phy);
+
+	phy_set_drvdata(imx8_phy->phy, imx8_phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id imx8_pcie_phy_of_match[] = {
+	{.compatible = "fsl,imx8mm-pcie-phy",},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx8_pcie_phy_of_match);
+
+static struct platform_driver imx8_pcie_phy_driver = {
+	.probe	= imx8_pcie_phy_probe,
+	.driver = {
+		.name	= "imx8-pcie-phy",
+		.of_match_table	= imx8_pcie_phy_of_match,
+	}
+};
+module_platform_driver(imx8_pcie_phy_driver);
+
+MODULE_DESCRIPTION("FSL IMX8 PCIE PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH v7 6/8] arm64: dts: imx8mm: Add the pcie support

[Richard Zhu]

Add the PCIe support on i.MX8MM platforms.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 33 ++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 0844f3144887..75f4317215ac 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -520,7 +520,7 @@ iomuxc: pinctrl@30330000 {
 			};
 
 			gpr: iomuxc-gpr@30340000 {
-				compatible = "fsl,imx8mm-iomuxc-gpr", "syscon";
+				compatible = "fsl,imx8mm-iomuxc-gpr", "fsl,imx6q-iomuxc-gpr", "syscon";
 				reg = <0x30340000 0x10000>;
 			};
 
@@ -1179,6 +1179,37 @@ gpmi: nand-controller@33002000{
 			status = "disabled";
 		};
 
+		pcie0: pcie@33800000 {
+			compatible = "fsl,imx8mm-pcie";
+			reg = <0x33800000 0x400000>, <0x1ff00000 0x80000>;
+			reg-names = "dbi", "config";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			bus-range = <0x00 0xff>;
+			ranges =  <0x81000000 0 0x00000000 0x1ff80000 0 0x00010000 /* downstream I/O 64KB */
+				   0x82000000 0 0x18000000 0x18000000 0 0x07f00000>; /* non-prefetchable memory */
+			num-lanes = <1>;
+			num-viewport = <4>;
+			interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &gic GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 2 &gic GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 3 &gic GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+					<0 0 0 4 &gic GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
+			fsl,max-link-speed = <2>;
+			linux,pci-domain = <0>;
+			power-domains = <&pgc_pcie>;
+			resets = <&src IMX8MQ_RESET_PCIE_CTRL_APPS_EN>,
+				 <&src IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF>;
+			reset-names = "apps", "turnoff";
+			phys = <&pcie_phy>;
+			phy-names = "pcie-phy";
+			status = "disabled";
+		};
+
 		gpu_3d: gpu@38000000 {
 			compatible = "vivante,gc";
 			reg = <0x38000000 0x8000>;
-- 
2.25.1

[PATCH v7 7/8] arm64: dts: imx8mm-evk: Add the pcie support on imx8mm evk board

[Richard Zhu]

Add the PCIe support on iMX8MM EVK boards.
And set the default reference clock mode.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
index e033d0257b5a..87a30daf0b3c 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
@@ -5,6 +5,7 @@
 
 /dts-v1/;
 
+#include <dt-bindings/phy/phy-imx8-pcie.h>
 #include <dt-bindings/usb/pd.h>
 #include "imx8mm.dtsi"
 
@@ -30,6 +31,23 @@ status {
 		};
 	};
 
+	pcie0_refclk: pcie0-refclk {
+		compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <100000000>;
+	};
+
+	reg_pcie0: regulator-pcie {
+		compatible = "regulator-fixed";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_pcie0_reg>;
+		regulator-name = "MPCIE_3V3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&gpio1 5 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	reg_usdhc2_vmmc: regulator-usdhc2 {
 		compatible = "regulator-fixed";
 		pinctrl-names = "default";
@@ -289,6 +307,30 @@ pca6416: gpio@20 {
 	};
 };
 
+&pcie_phy {
+	fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>;
+	fsl,tx-deemph-gen1 = <0x2d>;
+	fsl,tx-deemph-gen2 = <0xf>;
+	clocks = <&pcie0_refclk>;
+	status = "okay";
+};
+
+&pcie0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pcie0>;
+	reset-gpio = <&gpio4 21 GPIO_ACTIVE_LOW>;
+	clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>, <&clk IMX8MM_CLK_PCIE1_AUX>,
+		 <&pcie0_refclk>;
+	clock-names = "pcie", "pcie_aux", "pcie_bus";
+	assigned-clocks = <&clk IMX8MM_CLK_PCIE1_AUX>,
+			  <&clk IMX8MM_CLK_PCIE1_CTRL>;
+	assigned-clock-rates = <10000000>, <250000000>;
+	assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_50M>,
+				 <&clk IMX8MM_SYS_PLL2_250M>;
+	vpcie-supply = <&reg_pcie0>;
+	status = "okay";
+};
+
 &sai3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_sai3>;
@@ -406,6 +448,19 @@ MX8MM_IOMUXC_I2C3_SDA_I2C3_SDA			0x400001c3
 		>;
 	};
 
+	pinctrl_pcie0: pcie0grp {
+		fsl,pins = <
+			MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B    0x61
+			MX8MM_IOMUXC_SAI2_RXFS_GPIO4_IO21       0x41
+		>;
+	};
+
+	pinctrl_pcie0_reg: pcie0reggrp {
+		fsl,pins = <
+			MX8MM_IOMUXC_GPIO1_IO05_GPIO1_IO5       0x41
+		>;
+	};
+
 	pinctrl_pmic: pmicirqgrp {
 		fsl,pins = <
 			MX8MM_IOMUXC_GPIO1_IO03_GPIO1_IO3		0x141
-- 
2.25.1

[PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

[Richard Zhu]

i.MX8MM PCIe works mostly like the i.MX8MQ one, but has a different PHY
and allows to output the internal PHY reference clock via the refclk pad.
Add the i.MX8MM PCIe support based on the standalone PHY driver.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
Tested-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 83 ++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 26f49f797b0f..2ac081510632 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -29,6 +29,7 @@
 #include <linux/types.h>
 #include <linux/interrupt.h>
 #include <linux/reset.h>
+#include <linux/phy/phy.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 
@@ -49,6 +50,7 @@ enum imx6_pcie_variants {
 	IMX6QP,
 	IMX7D,
 	IMX8MQ,
+	IMX8MM,
 };
 
 #define IMX6_PCIE_FLAG_IMX6_PHY			BIT(0)
@@ -88,6 +90,7 @@ struct imx6_pcie {
 	struct device		*pd_pcie;
 	/* power domain for pcie phy */
 	struct device		*pd_pcie_phy;
+	struct phy		*phy;
 	const struct imx6_pcie_drvdata *drvdata;
 };
 
@@ -372,6 +375,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 	case IMX7D:
 	case IMX8MQ:
 		reset_control_assert(imx6_pcie->pciephy_reset);
+		fallthrough;
+	case IMX8MM:
 		reset_control_assert(imx6_pcie->apps_reset);
 		break;
 	case IMX6SX:
@@ -407,7 +412,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 
 static unsigned int imx6_pcie_grp_offset(const struct imx6_pcie *imx6_pcie)
 {
-	WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ);
+	WARN_ON(imx6_pcie->drvdata->variant != IMX8MQ &&
+		imx6_pcie->drvdata->variant != IMX8MM);
 	return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;
 }
 
@@ -446,6 +452,13 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 		break;
 	case IMX7D:
 		break;
+	case IMX8MM:
+		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
+		if (ret) {
+			dev_err(dev, "unable to enable pcie_aux clock\n");
+			break;
+		}
+		break;
 	case IMX8MQ:
 		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
 		if (ret) {
@@ -522,6 +535,14 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 		goto err_ref_clk;
 	}
 
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		if (phy_power_on(imx6_pcie->phy))
+			dev_err(dev, "unable to power on PHY\n");
+		break;
+	default:
+		break;
+	}
 	/* allow the clocks to stabilize */
 	usleep_range(200, 500);
 
@@ -538,6 +559,10 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	case IMX8MQ:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 		break;
+	case IMX8MM:
+		if (phy_init(imx6_pcie->phy) != 0)
+			dev_err(dev, "Waiting for PHY ready timeout!\n");
+		break;
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 
@@ -614,6 +639,8 @@ static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
 static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 {
 	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		break;
 	case IMX8MQ:
 		/*
 		 * TODO: Currently this code assumes external
@@ -753,6 +780,7 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
 		break;
 	case IMX7D:
 	case IMX8MQ:
+	case IMX8MM:
 		reset_control_deassert(imx6_pcie->apps_reset);
 		break;
 	}
@@ -871,6 +899,7 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
 				   IMX6Q_GPR12_PCIE_CTL_2, 0);
 		break;
 	case IMX7D:
+	case IMX8MM:
 		reset_control_assert(imx6_pcie->apps_reset);
 		break;
 	default:
@@ -930,6 +959,7 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
 		break;
 	case IMX8MQ:
+	case IMX8MM:
 		clk_disable_unprepare(imx6_pcie->pcie_aux);
 		break;
 	default:
@@ -945,8 +975,16 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
 		return 0;
 
 	imx6_pcie_pm_turnoff(imx6_pcie);
-	imx6_pcie_clk_disable(imx6_pcie);
 	imx6_pcie_ltssm_disable(dev);
+	imx6_pcie_clk_disable(imx6_pcie);
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX8MM:
+		if (phy_power_off(imx6_pcie->phy))
+			dev_err(dev, "unable to power on PHY\n");
+		break;
+	default:
+		break;
+	}
 
 	return 0;
 }
@@ -1043,11 +1081,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	}
 
 	/* Fetch clocks */
-	imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
-	if (IS_ERR(imx6_pcie->pcie_phy))
-		return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
-				     "pcie_phy clock source missing or invalid\n");
-
 	imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
 	if (IS_ERR(imx6_pcie->pcie_bus))
 		return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
@@ -1089,10 +1122,39 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 			dev_err(dev, "Failed to get PCIE APPS reset control\n");
 			return PTR_ERR(imx6_pcie->apps_reset);
 		}
+		break;
+	case IMX8MM:
+		imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
+		if (IS_ERR(imx6_pcie->pcie_aux))
+			return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
+					     "pcie_aux clock source missing or invalid\n");
+		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
+									 "apps");
+		if (IS_ERR(imx6_pcie->apps_reset)) {
+			dev_err(dev, "Failed to get PCIE APPS reset control\n");
+			return PTR_ERR(imx6_pcie->apps_reset);
+		}
+
+		imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
+		if (IS_ERR(imx6_pcie->phy)) {
+			if (PTR_ERR(imx6_pcie->phy) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			dev_err(dev, "Failed to get PCIE PHY\n");
+			return PTR_ERR(imx6_pcie->phy);
+		}
+
 		break;
 	default:
 		break;
 	}
+	/* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
+	if (imx6_pcie->phy == NULL) {
+		imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
+		if (IS_ERR(imx6_pcie->pcie_phy))
+			return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
+					     "pcie_phy clock source missing or invalid\n");
+	}
+
 
 	/* Grab turnoff reset */
 	imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
@@ -1202,6 +1264,10 @@ static const struct imx6_pcie_drvdata drvdata[] = {
 	[IMX8MQ] = {
 		.variant = IMX8MQ,
 	},
+	[IMX8MM] = {
+		.variant = IMX8MM,
+		.flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
+	},
 };
 
 static const struct of_device_id imx6_pcie_of_match[] = {
@@ -1209,7 +1275,8 @@ static const struct of_device_id imx6_pcie_of_match[] = {
 	{ .compatible = "fsl,imx6sx-pcie", .data = &drvdata[IMX6SX], },
 	{ .compatible = "fsl,imx6qp-pcie", .data = &drvdata[IMX6QP], },
 	{ .compatible = "fsl,imx7d-pcie",  .data = &drvdata[IMX7D],  },
-	{ .compatible = "fsl,imx8mq-pcie", .data = &drvdata[IMX8MQ], } ,
+	{ .compatible = "fsl,imx8mq-pcie", .data = &drvdata[IMX8MQ], },
+	{ .compatible = "fsl,imx8mm-pcie", .data = &drvdata[IMX8MM], },
 	{},
 };
 
-- 
2.25.1

Re: [PATCH v7 1/8] dt-bindings: phy: phy-imx8-pcie: Add binding for the pad modes of imx8 pcie phy

[Vinod Koul]

On 02-12-21, 16:02, Richard Zhu wrote:
> Add binding for reference clock PAD modes of the i.MX8 PCIe PHY.

Applied, thanks

-- 
~Vinod

Re: [PATCH v7 2/8] dt-bindings: phy: Add imx8 pcie phy driver support

[Vinod Koul]

On 02-12-21, 16:02, Richard Zhu wrote:
> Add dt-binding for the standalone i.MX8 PCIe PHY driver.

Applied, thanks

> 
-- 
~Vinod

Re: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

[Vinod Koul]

On 02-12-21, 16:02, Richard Zhu wrote:
> Add the standalone i.MX8 PCIe PHY driver.

Applied, thanks

-- 
~Vinod

Re: (subset) [PATCH v7 0/8] Add the imx8m pcie phy driver and imx8mm pcie support

[Lorenzo Pieralisi]

On Thu, 2 Dec 2021 16:02:30 +0800, Richard Zhu wrote:
> Refer to the discussion [1] when try to enable i.MX8MM PCIe support,
> one standalone PCIe PHY driver should be seperated from i.MX PCIe
> driver when enable i.MX8MM PCIe support.
> 
> This patch-set adds the standalone PCIe PHY driver suport[1-5], and i.MX8MM
> PCIe support[6-8] to have whole view to review this patch-set.
> 
> [...]

Applied to pci/dwc, thanks!

[3/8] dt-bindings: imx6q-pcie: Add PHY phandles and name properties
      https://git.kernel.org/lpieralisi/pci/c/3e15f623bb
[8/8] PCI: imx: Add the imx8mm pcie support
      https://git.kernel.org/lpieralisi/pci/c/d5a8dec49b

Thanks,
Lorenzo

Re: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

[Krzysztof Wilczyński]

Hi Richard,

Apologies for a very late review!  Especially since Lorenzo already took
patches as per:

  https://lore.kernel.org/linux-pci/163965080404.20006.5241609551643501749.b4-ty@arm.com/

However, perhaps it's not too late.

[...]
> @@ -446,6 +452,13 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
>  		break;
>  	case IMX7D:
>  		break;
> +	case IMX8MM:
> +		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> +		if (ret) {
> +			dev_err(dev, "unable to enable pcie_aux clock\n");
> +			break;
> +		}
> +		break;

You can drop the inner break, it wouldn't do much here, unless this was
intended to be a return?

> @@ -538,6 +559,10 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  	case IMX8MQ:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
>  		break;
> +	case IMX8MM:
> +		if (phy_init(imx6_pcie->phy) != 0)
> +			dev_err(dev, "Waiting for PHY ready timeout!\n");
> +		break;

If the above, you can keep the same style as used throughout the file
already, so it would just simply be:

  if (phy_init(imx6_pcie->phy))

Also, a nitpick: to be consistent with other such messages here, the error
message would be all lower-case letters.

[...]
> @@ -614,6 +639,8 @@ static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie)
>  static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
>  {
>  	switch (imx6_pcie->drvdata->variant) {
> +	case IMX8MM:
> +		break;
>  	case IMX8MQ:

Would it warrant a comment that adds a note there to this single bare
break?  Perhaps this version is not support, lack this particular
functionality, etc.

[...]
> @@ -1089,10 +1122,39 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  			dev_err(dev, "Failed to get PCIE APPS reset control\n");
>  			return PTR_ERR(imx6_pcie->apps_reset);
>  		}
> +		break;
> +	case IMX8MM:
> +		imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> +		if (IS_ERR(imx6_pcie->pcie_aux))
> +			return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> +					     "pcie_aux clock source missing or invalid\n");
> +		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> +									 "apps");
> +		if (IS_ERR(imx6_pcie->apps_reset)) {
> +			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> +			return PTR_ERR(imx6_pcie->apps_reset);
> +		}
> +
> +		imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> +		if (IS_ERR(imx6_pcie->phy)) {
> +			if (PTR_ERR(imx6_pcie->phy) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			dev_err(dev, "Failed to get PCIE PHY\n");
> +			return PTR_ERR(imx6_pcie->phy);
> +		}

A question about handling of the -EPROBE_DEFER above: why not to use the
dev_err_probe() helper similarly to the code above and below?  Would there
be something different preventing the use of dev_err_probe() here too?

>  		break;
>  	default:
>  		break;
>  	}
> +	/* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> +	if (imx6_pcie->phy == NULL) {
> +		imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> +		if (IS_ERR(imx6_pcie->pcie_phy))
> +			return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> +					     "pcie_phy clock source missing or invalid\n");
> +	}

Thank you for another amazing patch!

	Krzysztof

RE: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

[Hongxing Zhu]

> -----Original Message-----
> From: Krzysztof Wilczyński <kw@linux.com>
> Sent: Friday, December 17, 2021 12:52 AM
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; Marcel Ziswiler
> <marcel.ziswiler@toradex.com>; tharvey@gateworks.com;
> kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> galak@kernel.crashing.org; shawnguo@kernel.org;
> linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support
> 
> Hi Richard,
> 
> Apologies for a very late review!  Especially since Lorenzo already took
> patches as per:
> 
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flor
> e.kernel.org%2Flinux-pci%2F163965080404.20006.52416095516435017
> 49.b4-ty%40arm.com%2F&amp;data=04%7C01%7Chongxing.zhu%40nxp
> .com%7C8afb673348214261883608d9c0b45b1d%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637752703124166805%7CUnknown%7
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> WwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=rfmN1Xojubap2vi3J4Jol3ozy
> N1Q2q7YiBM5bqMm22s%3D&amp;reserved=0
> 
> However, perhaps it's not too late.
[Richard Zhu] Hi Krzysztof: 
Thanks for your review.
But I don't know how to handle this situation.
How about that I add this refine patch into the following bug fix and
 refine patch-set later?
PCI: imx6: refine codes and add compliance tests mode support
" https://patchwork.kernel.org/project/linux-arm-kernel/cover/1635747478-25562-1-git-send-email-hongxing.zhu@nxp.com/"

> 
> [...]
> > @@ -446,6 +452,13 @@ static int imx6_pcie_enable_ref_clk(struct
> imx6_pcie *imx6_pcie)
> >  		break;
> >  	case IMX7D:
> >  		break;
> > +	case IMX8MM:
> > +		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> > +		if (ret) {
> > +			dev_err(dev, "unable to enable pcie_aux clock\n");
> > +			break;
> > +		}
> > +		break;
> 
> You can drop the inner break, it wouldn't do much here, unless this was
> intended to be a return?
[Richard Zhu] Yes, it is. The inner break can be dropped. The error return
would be handled in the end.

> 
> > @@ -538,6 +559,10 @@ static void
> imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> >  	case IMX8MQ:
> >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> >  		break;
> > +	case IMX8MM:
> > +		if (phy_init(imx6_pcie->phy) != 0)
> > +			dev_err(dev, "Waiting for PHY ready timeout!\n");
> > +		break;
> 
> If the above, you can keep the same style as used throughout the file
> already, so it would just simply be:
> 
>   if (phy_init(imx6_pcie->phy))
> 
> Also, a nitpick: to be consistent with other such messages here, the error
> message would be all lower-case letters.
[Richard Zhu] Yes, it is.
> 
> [...]
> > @@ -614,6 +639,8 @@ static void imx6_pcie_configure_type(struct
> > imx6_pcie *imx6_pcie)  static void imx6_pcie_init_phy(struct
> imx6_pcie
> > *imx6_pcie)  {
> >  	switch (imx6_pcie->drvdata->variant) {
> > +	case IMX8MM:
> > +		break;
> >  	case IMX8MQ:
> 
> Would it warrant a comment that adds a note there to this single bare
> break?  Perhaps this version is not support, lack this particular
> functionality, etc.
[Richard Zhu] Yes, it's easier to understand after add one comment.
> 
> [...]
> > @@ -1089,10 +1122,39 @@ static int imx6_pcie_probe(struct
> platform_device *pdev)
> >  			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> >  			return PTR_ERR(imx6_pcie->apps_reset);
> >  		}
> > +		break;
> > +	case IMX8MM:
> > +		imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > +		if (IS_ERR(imx6_pcie->pcie_aux))
> > +			return dev_err_probe(dev,
> PTR_ERR(imx6_pcie->pcie_aux),
> > +					     "pcie_aux clock source missing or
> invalid\n");
> > +		imx6_pcie->apps_reset =
> devm_reset_control_get_exclusive(dev,
> > +									 "apps");
> > +		if (IS_ERR(imx6_pcie->apps_reset)) {
> > +			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> > +			return PTR_ERR(imx6_pcie->apps_reset);
> > +		}
> > +
> > +		imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > +		if (IS_ERR(imx6_pcie->phy)) {
> > +			if (PTR_ERR(imx6_pcie->phy) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +			dev_err(dev, "Failed to get PCIE PHY\n");
> > +			return PTR_ERR(imx6_pcie->phy);
> > +		}
> 
> A question about handling of the -EPROBE_DEFER above: why not to use
> the
> dev_err_probe() helper similarly to the code above and below?  Would
> there be something different preventing the use of dev_err_probe() here
> too?
[Richard Zhu] To be aligned, the above one can be replaced totally.
I didn't want to dump the error message when -EPROBE_DEFFER occurs.
Anyway, I can make them aligned later.

Best Regards
Richard

> 
> >  		break;
> >  	default:
> >  		break;
> >  	}
> > +	/* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > +	if (imx6_pcie->phy == NULL) {
> > +		imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > +		if (IS_ERR(imx6_pcie->pcie_phy))
> > +			return dev_err_probe(dev,
> PTR_ERR(imx6_pcie->pcie_phy),
> > +					     "pcie_phy clock source missing or
> invalid\n");
> > +	}
> 
> Thank you for another amazing patch!
> 
> 	Krzysztof

Re: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

[Lorenzo Pieralisi]

On Fri, Dec 17, 2021 at 05:54:21AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Krzysztof Wilczyński <kw@linux.com>
> > Sent: Friday, December 17, 2021 12:52 AM
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> > lorenzo.pieralisi@arm.com; Marcel Ziswiler
> > <marcel.ziswiler@toradex.com>; tharvey@gateworks.com;
> > kishon@ti.com; vkoul@kernel.org; robh@kernel.org;
> > galak@kernel.crashing.org; shawnguo@kernel.org;
> > linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support
> > 
> > Hi Richard,
> > 
> > Apologies for a very late review!  Especially since Lorenzo already took
> > patches as per:
> > 
> > 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flor
> > e.kernel.org%2Flinux-pci%2F163965080404.20006.52416095516435017
> > 49.b4-ty%40arm.com%2F&amp;data=04%7C01%7Chongxing.zhu%40nxp
> > .com%7C8afb673348214261883608d9c0b45b1d%7C686ea1d3bc2b4c6fa
> > 92cd99c5c301635%7C0%7C0%7C637752703124166805%7CUnknown%7
> > CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > WwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=rfmN1Xojubap2vi3J4Jol3ozy
> > N1Q2q7YiBM5bqMm22s%3D&amp;reserved=0
> > 
> > However, perhaps it's not too late.
> [Richard Zhu] Hi Krzysztof: 
> Thanks for your review.
> But I don't know how to handle this situation.
> How about that I add this refine patch into the following bug fix and
>  refine patch-set later?
> PCI: imx6: refine codes and add compliance tests mode support
> " https://patchwork.kernel.org/project/linux-arm-kernel/cover/1635747478-25562-1-git-send-email-hongxing.zhu@nxp.com/"
> 
> > 
> > [...]
> > > @@ -446,6 +452,13 @@ static int imx6_pcie_enable_ref_clk(struct
> > imx6_pcie *imx6_pcie)
> > >  		break;
> > >  	case IMX7D:
> > >  		break;
> > > +	case IMX8MM:
> > > +		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> > > +		if (ret) {
> > > +			dev_err(dev, "unable to enable pcie_aux clock\n");
> > > +			break;
> > > +		}
> > > +		break;
> > 
> > You can drop the inner break, it wouldn't do much here, unless this was
> > intended to be a return?
> [Richard Zhu] Yes, it is. The inner break can be dropped. The error return
> would be handled in the end.
> 
> > 
> > > @@ -538,6 +559,10 @@ static void
> > imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > >  	case IMX8MQ:
> > >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> > >  		break;
> > > +	case IMX8MM:
> > > +		if (phy_init(imx6_pcie->phy) != 0)
> > > +			dev_err(dev, "Waiting for PHY ready timeout!\n");
> > > +		break;
> > 
> > If the above, you can keep the same style as used throughout the file
> > already, so it would just simply be:
> > 
> >   if (phy_init(imx6_pcie->phy))
> > 
> > Also, a nitpick: to be consistent with other such messages here, the error
> > message would be all lower-case letters.
> [Richard Zhu] Yes, it is.
> > 
> > [...]
> > > @@ -614,6 +639,8 @@ static void imx6_pcie_configure_type(struct
> > > imx6_pcie *imx6_pcie)  static void imx6_pcie_init_phy(struct
> > imx6_pcie
> > > *imx6_pcie)  {
> > >  	switch (imx6_pcie->drvdata->variant) {
> > > +	case IMX8MM:
> > > +		break;
> > >  	case IMX8MQ:
> > 
> > Would it warrant a comment that adds a note there to this single bare
> > break?  Perhaps this version is not support, lack this particular
> > functionality, etc.
> [Richard Zhu] Yes, it's easier to understand after add one comment.
> > 
> > [...]
> > > @@ -1089,10 +1122,39 @@ static int imx6_pcie_probe(struct
> > platform_device *pdev)
> > >  			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> > >  			return PTR_ERR(imx6_pcie->apps_reset);
> > >  		}
> > > +		break;
> > > +	case IMX8MM:
> > > +		imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > > +		if (IS_ERR(imx6_pcie->pcie_aux))
> > > +			return dev_err_probe(dev,
> > PTR_ERR(imx6_pcie->pcie_aux),
> > > +					     "pcie_aux clock source missing or
> > invalid\n");
> > > +		imx6_pcie->apps_reset =
> > devm_reset_control_get_exclusive(dev,
> > > +									 "apps");
> > > +		if (IS_ERR(imx6_pcie->apps_reset)) {
> > > +			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> > > +			return PTR_ERR(imx6_pcie->apps_reset);
> > > +		}
> > > +
> > > +		imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > > +		if (IS_ERR(imx6_pcie->phy)) {
> > > +			if (PTR_ERR(imx6_pcie->phy) == -EPROBE_DEFER)
> > > +				return -EPROBE_DEFER;
> > > +			dev_err(dev, "Failed to get PCIE PHY\n");
> > > +			return PTR_ERR(imx6_pcie->phy);
> > > +		}
> > 
> > A question about handling of the -EPROBE_DEFER above: why not to use
> > the
> > dev_err_probe() helper similarly to the code above and below?  Would
> > there be something different preventing the use of dev_err_probe() here
> > too?
> [Richard Zhu] To be aligned, the above one can be replaced totally.
> I didn't want to dump the error message when -EPROBE_DEFFER occurs.
> Anyway, I can make them aligned later.

Can you send me an update for this patch only so that I can update
the corresponding commit according to this review please ?

Thanks,
Lorenzo

> Best Regards
> Richard
> 
> > 
> > >  		break;
> > >  	default:
> > >  		break;
> > >  	}
> > > +	/* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > > +	if (imx6_pcie->phy == NULL) {
> > > +		imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > > +		if (IS_ERR(imx6_pcie->pcie_phy))
> > > +			return dev_err_probe(dev,
> > PTR_ERR(imx6_pcie->pcie_phy),
> > > +					     "pcie_phy clock source missing or
> > invalid\n");
> > > +	}
> > 
> > Thank you for another amazing patch!
> > 
> > 	Krzysztof

RE: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

[Hongxing Zhu]

> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Thursday, December 23, 2021 7:50 PM
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: Krzysztof Wilczyński <kw@linux.com>; l.stach@pengutronix.de;
> bhelgaas@google.com; Marcel Ziswiler <marcel.ziswiler@toradex.com>;
> tharvey@gateworks.com; kishon@ti.com; vkoul@kernel.org;
> robh@kernel.org; galak@kernel.crashing.org; shawnguo@kernel.org;
> linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support
> 
> On Fri, Dec 17, 2021 at 05:54:21AM +0000, Hongxing Zhu wrote:
> > > -----Original Message-----
> > > From: Krzysztof Wilczyński <kw@linux.com>
> > > Sent: Friday, December 17, 2021 12:52 AM
> > > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > > Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> > > lorenzo.pieralisi@arm.com; Marcel Ziswiler
> > > <marcel.ziswiler@toradex.com>; tharvey@gateworks.com;
> kishon@ti.com;
> > > vkoul@kernel.org; robh@kernel.org; galak@kernel.crashing.org;
> > > shawnguo@kernel.org; linux-phy@lists.infradead.org;
> > > devicetree@vger.kernel.org; linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > > <linux-imx@nxp.com>
> > > Subject: Re: [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support
> > >
> > > Hi Richard,
> > >
> > > Apologies for a very late review!  Especially since Lorenzo already
> > > took patches as per:
> > >
> > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flo
> > > r
> > >
> e.kernel.org%2Flinux-pci%2F163965080404.20006.52416095516435017
> > >
> 49.b4-ty%40arm.com%2F&amp;data=04%7C01%7Chongxing.zhu%40nxp
> > > .com%7C8afb673348214261883608d9c0b45b1d%7C686ea1d3bc2b4
> c6fa
> > >
> 92cd99c5c301635%7C0%7C0%7C637752703124166805%7CUnknown%7
> > >
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > >
> WwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=rfmN1Xojubap2vi3J4Jol3ozy
> > > N1Q2q7YiBM5bqMm22s%3D&amp;reserved=0
> > >
> > > However, perhaps it's not too late.
> > [Richard Zhu] Hi Krzysztof:
> > Thanks for your review.
> > But I don't know how to handle this situation.
> > How about that I add this refine patch into the following bug fix and
> > refine patch-set later?
> > PCI: imx6: refine codes and add compliance tests mode support "
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> tchwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fcover%2F1635747
> 478-25562-1-git-send-email-hongxing.zhu%40nxp.com%2F&amp;data=0
> 4%7C01%7Chongxing.zhu%40nxp.com%7Ce04c6cda020b4f3849bf08d9c
> 60a5da0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C6377
> 58570100053598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp;
> sdata=joTJZcYJhmYlzSTWY%2BhGCFV5ch12HPR4mbbOwZ5vcO0%3D&a
> mp;reserved=0"
> >
> > >
> > > [...]
> > > > @@ -446,6 +452,13 @@ static int imx6_pcie_enable_ref_clk(struct
> > > imx6_pcie *imx6_pcie)
> > > >  		break;
> > > >  	case IMX7D:
> > > >  		break;
> > > > +	case IMX8MM:
> > > > +		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "unable to enable pcie_aux clock\n");
> > > > +			break;
> > > > +		}
> > > > +		break;
> > >
> > > You can drop the inner break, it wouldn't do much here, unless this
> > > was intended to be a return?
> > [Richard Zhu] Yes, it is. The inner break can be dropped. The error
> > return would be handled in the end.
> >
> > >
> > > > @@ -538,6 +559,10 @@ static void
> > > imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > > >  	case IMX8MQ:
> > > >  		reset_control_deassert(imx6_pcie->pciephy_reset);
> > > >  		break;
> > > > +	case IMX8MM:
> > > > +		if (phy_init(imx6_pcie->phy) != 0)
> > > > +			dev_err(dev, "Waiting for PHY ready timeout!\n");
> > > > +		break;
> > >
> > > If the above, you can keep the same style as used throughout the
> > > file already, so it would just simply be:
> > >
> > >   if (phy_init(imx6_pcie->phy))
> > >
> > > Also, a nitpick: to be consistent with other such messages here, the
> > > error message would be all lower-case letters.
> > [Richard Zhu] Yes, it is.
> > >
> > > [...]
> > > > @@ -614,6 +639,8 @@ static void imx6_pcie_configure_type(struct
> > > > imx6_pcie *imx6_pcie)  static void imx6_pcie_init_phy(struct
> > > imx6_pcie
> > > > *imx6_pcie)  {
> > > >  	switch (imx6_pcie->drvdata->variant) {
> > > > +	case IMX8MM:
> > > > +		break;
> > > >  	case IMX8MQ:
> > >
> > > Would it warrant a comment that adds a note there to this single
> > > bare break?  Perhaps this version is not support, lack this
> > > particular functionality, etc.
> > [Richard Zhu] Yes, it's easier to understand after add one comment.
> > >
> > > [...]
> > > > @@ -1089,10 +1122,39 @@ static int imx6_pcie_probe(struct
> > > platform_device *pdev)
> > > >  			dev_err(dev, "Failed to get PCIE APPS reset
> control\n");
> > > >  			return PTR_ERR(imx6_pcie->apps_reset);
> > > >  		}
> > > > +		break;
> > > > +	case IMX8MM:
> > > > +		imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > > > +		if (IS_ERR(imx6_pcie->pcie_aux))
> > > > +			return dev_err_probe(dev,
> > > PTR_ERR(imx6_pcie->pcie_aux),
> > > > +					     "pcie_aux clock source missing or
> > > invalid\n");
> > > > +		imx6_pcie->apps_reset =
> > > devm_reset_control_get_exclusive(dev,
> > > > +									 "apps");
> > > > +		if (IS_ERR(imx6_pcie->apps_reset)) {
> > > > +			dev_err(dev, "Failed to get PCIE APPS reset
> control\n");
> > > > +			return PTR_ERR(imx6_pcie->apps_reset);
> > > > +		}
> > > > +
> > > > +		imx6_pcie->phy = devm_phy_get(dev, "pcie-phy");
> > > > +		if (IS_ERR(imx6_pcie->phy)) {
> > > > +			if (PTR_ERR(imx6_pcie->phy) == -EPROBE_DEFER)
> > > > +				return -EPROBE_DEFER;
> > > > +			dev_err(dev, "Failed to get PCIE PHY\n");
> > > > +			return PTR_ERR(imx6_pcie->phy);
> > > > +		}
> > >
> > > A question about handling of the -EPROBE_DEFER above: why not to
> use
> > > the
> > > dev_err_probe() helper similarly to the code above and below?
> Would
> > > there be something different preventing the use of dev_err_probe()
> > > here too?
> > [Richard Zhu] To be aligned, the above one can be replaced totally.
> > I didn't want to dump the error message when -EPROBE_DEFFER
> occurs.
> > Anyway, I can make them aligned later.
> 
> Can you send me an update for this patch only so that I can update the
> corresponding commit according to this review please ?
[Richard Zhu] Sure, thanks a lot for your kindly help.

Best Regards
Richard Zhu

> 
> Thanks,
> Lorenzo
> 
> > Best Regards
> > Richard
> >
> > >
> > > >  		break;
> > > >  	default:
> > > >  		break;
> > > >  	}
> > > > +	/* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > > > +	if (imx6_pcie->phy == NULL) {
> > > > +		imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > > > +		if (IS_ERR(imx6_pcie->pcie_phy))
> > > > +			return dev_err_probe(dev,
> > > PTR_ERR(imx6_pcie->pcie_phy),
> > > > +					     "pcie_phy clock source missing or
> > > invalid\n");
> > > > +	}
> > >
> > > Thank you for another amazing patch!
> > >
> > > 	Krzysztof

Re: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

[Philip Molloy]

Hi Richard,

I've run into an issue that appears to indicate a functional difference
between the existing integrated pci-imx6.c implementation and this new
implementation with the separate phy driver.

I'm working with a SOM and baseboard from Phytec that is based on the
IMX8MM. The board does not have an external PCIe clock and has a
ethernet controller hanging off the PCIe bus.

When booting from a 5.4 NXP-based kernel from Phytec the ethernet
controller is probed and functions as expected.

A co-worker backported a slightly earlier version of this patchset to
5.10.[1] With our kernel both the controller driver and new PHY driver
are probed, but a timeout occurs in dw_pcie_wait_for_link() which
indicates that the "Phy link never came up".

After reproducing this issue I configured pcie_phy with
IMX8_PCIE_REFCLK_PAD_OUTPUT. With that configured, phy register
CMN_REG062/0x188 matches the 5.4 NXP/Phytec kernel. I then compared the
controller and PHY registers between the two kernels and noticed that
CMN_REG063/0x18c is set to AUX_IN/0x0 in the 5.4 NXP/Phytec kernel, but
the new PHY driver writes I_PLL_REFCLK_FROM_SYSPLL/0xc0 to that register.

If I modify the phy driver to not write I_PLL_REFCLK_FROM_SYSPLL/0xc0
then the system behaves as expected.

Best,
Philip

[1]: I plan on rebasing our branch with the latest patches that have
been applied upstream. Note that I did not see any difference in the
following code with what we have applied.

On 12/2/21 09:02, Richard Zhu wrote:
> +#define IMX8MM_PCIE_PHY_CMN_REG061	0x184
> +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN	BIT(0)
> +#define IMX8MM_PCIE_PHY_CMN_REG062	0x188
> +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL	BIT(3)
> +#define IMX8MM_PCIE_PHY_CMN_REG063	0x18C
> +#define  AUX_PLL_REFCLK_SEL_SYS_PLL	GENMASK(7, 6)
> +#define IMX8MM_PCIE_PHY_CMN_REG064	0x190
> +#define  ANA_AUX_RX_TX_SEL_TX		BIT(7)
> +#define  ANA_AUX_RX_TERM_GND_EN		BIT(3)
> +#define  ANA_AUX_TX_TERM		BIT(2)
> +#define IMX8MM_PCIE_PHY_CMN_REG065	0x194
> +#define  ANA_AUX_RX_TERM		(BIT(7) | BIT(4))
> +#define  ANA_AUX_TX_LVL			GENMASK(3, 0)
...
> +	if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
> +		/* Configure the pad as input */
> +		val = readl(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
> +		writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> +		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
> +	} else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
> +		/* Configure the PHY to output the refclock via pad */
> +		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> +		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
> +		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
> +		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG062);
> +		writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
> +		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);

If I comment out this writel() then the register defaults to 0x0/AUX_IN
and then the system behaves as expected.

> +		val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
> +		writel(val | ANA_AUX_RX_TERM_GND_EN,
> +		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG064);
> +		writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
> +		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG065);
> +	}

RE: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

[Hongxing Zhu]

> -----Original Message-----
> From: Philip Molloy <philip@linutronix.de>
> Sent: Wednesday, December 29, 2021 8:40 PM
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; tharvey@gateworks.com; Marcel Ziswiler
> <marcel.ziswiler@toradex.com>; kishon@ti.com; vkoul@kernel.org;
> robh@kernel.org; galak@kernel.crashing.org; shawnguo@kernel.org;
> linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> standalone phy driver
> 
> Hi Richard,
> 
> I've run into an issue that appears to indicate a functional difference
> between the existing integrated pci-imx6.c implementation and this new
> implementation with the separate phy driver.
> 
> I'm working with a SOM and baseboard from Phytec that is based on the
> IMX8MM. The board does not have an external PCIe clock and has a
> ethernet controller hanging off the PCIe bus.
> 
> When booting from a 5.4 NXP-based kernel from Phytec the ethernet
> controller is probed and functions as expected.
> 
> A co-worker backported a slightly earlier version of this patchset to
> 5.10.[1] With our kernel both the controller driver and new PHY driver are
> probed, but a timeout occurs in dw_pcie_wait_for_link() which indicates
> that the "Phy link never came up".
> 
> After reproducing this issue I configured pcie_phy with
> IMX8_PCIE_REFCLK_PAD_OUTPUT. With that configured, phy register
> CMN_REG062/0x188 matches the 5.4 NXP/Phytec kernel. I then
> compared the controller and PHY registers between the two kernels and
> noticed that CMN_REG063/0x18c is set to AUX_IN/0x0 in the 5.4
> NXP/Phytec kernel, but the new PHY driver writes
> I_PLL_REFCLK_FROM_SYSPLL/0xc0 to that register.
> 
> If I modify the phy driver to not write I_PLL_REFCLK_FROM_SYSPLL/0xc0
> then the system behaves as expected.
[Richard Zhu] 
Hi Philip:
The address 0x018C is a register to control the output mode of Refclk IO.
When internal syspll is used as PHY REF clock.
Regarding my understand, this register should select the syspll, and route
it out of SOC from CLK N/P pads.
Then the remote EP device can use the clock from CLK N/P pads.

If the bit7-6 is set to 2b'00, there wouldn't clock output from CLK N/P pads.
What's the hardware design of the CLK N/P pads in your project?
Can you monitor the situation of the CLK N/P if it is possible?

Best Regards
Richard Zhu

> 
> Best,
> Philip
> 
> [1]: I plan on rebasing our branch with the latest patches that have been
> applied upstream. Note that I did not see any difference in the following
> code with what we have applied.
> 
> On 12/2/21 09:02, Richard Zhu wrote:
> > +#define IMX8MM_PCIE_PHY_CMN_REG061	0x184
> > +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN	BIT(0)
> > +#define IMX8MM_PCIE_PHY_CMN_REG062	0x188
> > +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL	BIT(3)
> > +#define IMX8MM_PCIE_PHY_CMN_REG063	0x18C
> > +#define  AUX_PLL_REFCLK_SEL_SYS_PLL	GENMASK(7, 6)
> > +#define IMX8MM_PCIE_PHY_CMN_REG064	0x190
> > +#define  ANA_AUX_RX_TX_SEL_TX		BIT(7)
> > +#define  ANA_AUX_RX_TERM_GND_EN		BIT(3)
> > +#define  ANA_AUX_TX_TERM		BIT(2)
> > +#define IMX8MM_PCIE_PHY_CMN_REG065	0x194
> > +#define  ANA_AUX_RX_TERM		(BIT(7) | BIT(4))
> > +#define  ANA_AUX_TX_LVL			GENMASK(3, 0)
> ...
> > +	if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
> > +		/* Configure the pad as input */
> > +		val = readl(imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG061);
> > +		writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> > +		       imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG061);
> > +	} else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
> > +		/* Configure the PHY to output the refclock via pad */
> > +		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> > +		       imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG061);
> > +		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
> > +		       imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG062);
> > +		writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
> > +		       imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG063);
> 
> If I comment out this writel() then the register defaults to 0x0/AUX_IN
> and then the system behaves as expected.
> 
> > +		val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
> > +		writel(val | ANA_AUX_RX_TERM_GND_EN,
> > +		       imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG064);
> > +		writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
> > +		       imx8_phy->base +
> IMX8MM_PCIE_PHY_CMN_REG065);
> > +	}

Re: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

[Marcel Ziswiler]

On Thu, 2021-12-30 at 04:58 +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Philip Molloy <philip@linutronix.de>
> > Sent: Wednesday, December 29, 2021 8:40 PM
> > To: Hongxing Zhu <hongxing.zhu@nxp.com>
> > Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> > lorenzo.pieralisi@arm.com; tharvey@gateworks.com; Marcel Ziswiler
> > <marcel.ziswiler@toradex.com>; kishon@ti.com; vkoul@kernel.org;
> > robh@kernel.org; galak@kernel.crashing.org; shawnguo@kernel.org;
> > linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> > <linux-imx@nxp.com>
> > Subject: Re: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> > standalone phy driver
> > 
> > Hi Richard,
> > 
> > I've run into an issue that appears to indicate a functional difference
> > between the existing integrated pci-imx6.c implementation and this new
> > implementation with the separate phy driver.
> > 
> > I'm working with a SOM and baseboard from Phytec that is based on the
> > IMX8MM. The board does not have an external PCIe clock and has a
> > ethernet controller hanging off the PCIe bus.
> > 
> > When booting from a 5.4 NXP-based kernel from Phytec the ethernet
> > controller is probed and functions as expected.
> > 
> > A co-worker backported a slightly earlier version of this patchset to
> > 5.10.[1] With our kernel both the controller driver and new PHY driver are
> > probed, but a timeout occurs in dw_pcie_wait_for_link() which indicates
> > that the "Phy link never came up".
> > 
> > After reproducing this issue I configured pcie_phy with
> > IMX8_PCIE_REFCLK_PAD_OUTPUT. With that configured, phy register
> > CMN_REG062/0x188 matches the 5.4 NXP/Phytec kernel. I then
> > compared the controller and PHY registers between the two kernels and
> > noticed that CMN_REG063/0x18c is set to AUX_IN/0x0 in the 5.4
> > NXP/Phytec kernel, but the new PHY driver writes
> > I_PLL_REFCLK_FROM_SYSPLL/0xc0 to that register.
> > 
> > If I modify the phy driver to not write I_PLL_REFCLK_FROM_SYSPLL/0xc0
> > then the system behaves as expected.
> [Richard Zhu] 
> Hi Philip:
> The address 0x018C is a register to control the output mode of Refclk IO.
> When internal syspll is used as PHY REF clock.
> Regarding my understand, this register should select the syspll, and route
> it out of SOC from CLK N/P pads.
> Then the remote EP device can use the clock from CLK N/P pads.
> 
> If the bit7-6 is set to 2b'00, there wouldn't clock output from CLK N/P pads.
> What's the hardware design of the CLK N/P pads in your project?
> Can you monitor the situation of the CLK N/P if it is possible?

I can confirm that for me similar hardware which also "does not have an external PCIe clock" works just fine
with the following device tree sniped:

&pcie0 {
	assigned-clocks = <&clk IMX8MM_CLK_PCIE1_AUX>,
			  <&clk IMX8MM_CLK_PCIE1_CTRL>;
	assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_50M>,
				 <&clk IMX8MM_SYS_PLL2_250M>;
	assigned-clock-rates = <10000000>, <250000000>;
	clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>, <&clk IMX8MM_CLK_PCIE1_AUX>,
		 <&clk IMX8MM_CLK_PCIE1_PHY>;
	clock-names = "pcie", "pcie_aux", "pcie_bus";
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_pcie0>;
	/* PCIE_1_RESET# (SODIMM 244) */
	reset-gpio = <&gpio3 19 GPIO_ACTIVE_LOW>;
	status = "okay";
};

&pcie_phy {
	clocks = <&clk IMX8MM_CLK_PCIE1_PHY>;
	fsl,clkreq-unsupported;
	fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_OUTPUT>;
	fsl,tx-deemph-gen1 = <0x2d>;
	fsl,tx-deemph-gen2 = <0xf>;
	status = "okay";
};

During boot that looks as follows:

[    1.858312] imx6q-pcie 33800000.pcie: host bridge /soc@0/pcie@33800000 ranges:

[    1.865630] imx6q-pcie 33800000.pcie:       IO 0x001ff80000..0x001ff8ffff -> 0x0000000000
[    1.877833] imx6q-pcie 33800000.pcie:      MEM 0x0018000000..0x001fefffff -> 0x0018000000

[    1.992010] imx6q-pcie 33800000.pcie: iATU unroll: enabled
[    1.997523] imx6q-pcie 33800000.pcie: Detected iATU regions: 4 outbound, 4 inbound

[    2.103140] imx6q-pcie 33800000.pcie: Link up
[    2.107527] imx6q-pcie 33800000.pcie: Link up
[    2.111895] imx6q-pcie 33800000.pcie: Link up, Gen1
[    2.116786] imx6q-pcie 33800000.pcie: Link up
[    2.121298] imx6q-pcie 33800000.pcie: PCI host bridge to bus 0000:00
[    2.127671] pci_bus 0000:00: root bus resource [bus 00-ff]
[    2.133171] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    2.139363] pci_bus 0000:00: root bus resource [mem 0x18000000-0x1fefffff]
[    2.146282] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
[    2.152322] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x000fffff]
[    2.158612] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref]
[    2.165373] pci 0000:00:00.0: supports D1
[    2.169395] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold
[    2.178101] pci 0000:01:00.0: [10ec:8168] type 00 class 0x020000
[    2.184196] pci 0000:01:00.0: reg 0x10: [io  0x0000-0x00ff]
[    2.189864] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x00000fff 64bit pref]
[    2.197152] pci 0000:01:00.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit pref]
[    2.204777] pci 0000:01:00.0: supports D1 D2
[    2.209059] pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot D3cold
[    2.229114] pci 0000:00:00.0: BAR 0: assigned [mem 0x18000000-0x180fffff]
[    2.235934] pci 0000:00:00.0: BAR 15: assigned [mem 0x18100000-0x181fffff pref]
[    2.243260] pci 0000:00:00.0: BAR 6: assigned [mem 0x18200000-0x1820ffff pref]
[    2.250501] pci 0000:00:00.0: BAR 13: assigned [io  0x1000-0x1fff]
[    2.256734] pci 0000:01:00.0: BAR 4: assigned [mem 0x18100000-0x18103fff 64bit pref]
[    2.264534] pci 0000:01:00.0: BAR 2: assigned [mem 0x18104000-0x18104fff 64bit pref]
[    2.272338] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
[    2.278460] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[    2.283698] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
[    2.289808] pci 0000:00:00.0:   bridge window [mem 0x18100000-0x181fffff pref]
[    2.297444] pcieport 0000:00:00.0: PME: Signaling with IRQ 225

And my mini-PCIe Ethernet card gets detected and is fully functional:

[    4.433899] r8169 0000:01:00.0: enabling device (0000 -> 0003)
[    4.466731] libphy: r8169: probed
[    4.476288] r8169 0000:01:00.0 eth1: RTL8168e/8111e, 00:e0:4c:80:f4:0d, XID 2c2, IRQ 229
[    4.484485] r8169 0000:01:00.0 eth1: jumbo features [frames: 9194 bytes, tx checksumming: ko]

[    5.023616] r8169 0000:01:00.0 enp1s0: renamed from eth1

[    6.713601] r8169 0000:01:00.0: Direct firmware load for rtl_nic/rtl8168e-2.fw failed with error -2
[    6.722786] r8169 0000:01:00.0: Unable to load firmware rtl_nic/rtl8168e-2.fw (-2)
[    6.731391] RTL8211DN Gigabit Ethernet r8169-0-100:00: attached PHY driver (mii_bus:phy_addr=r8169-0-100:00,
irq=MAC)
[    6.808798] r8169 0000:01:00.0 enp1s0: Link is Down

root@verdin-imx8mm-06760554:~# lspci
00:00.0 PCI bridge: Synopsys, Inc. DWC_usb3 / PCIe bridge (rev 01)
01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet
Controller (rev 06)

Cheers

Marcel

> Best Regards
> Richard Zhu
> 
> > 
> > Best,
> > Philip
> > 
> > [1]: I plan on rebasing our branch with the latest patches that have been
> > applied upstream. Note that I did not see any difference in the following
> > code with what we have applied.
> > 
> > On 12/2/21 09:02, Richard Zhu wrote:
> > > +#define IMX8MM_PCIE_PHY_CMN_REG061     0x184
> > > +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN  BIT(0)
> > > +#define IMX8MM_PCIE_PHY_CMN_REG062     0x188
> > > +#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL BIT(3)
> > > +#define IMX8MM_PCIE_PHY_CMN_REG063     0x18C
> > > +#define  AUX_PLL_REFCLK_SEL_SYS_PLL    GENMASK(7, 6)
> > > +#define IMX8MM_PCIE_PHY_CMN_REG064     0x190
> > > +#define  ANA_AUX_RX_TX_SEL_TX          BIT(7)
> > > +#define  ANA_AUX_RX_TERM_GND_EN                BIT(3)
> > > +#define  ANA_AUX_TX_TERM               BIT(2)
> > > +#define IMX8MM_PCIE_PHY_CMN_REG065     0x194
> > > +#define  ANA_AUX_RX_TERM               (BIT(7) | BIT(4))
> > > +#define  ANA_AUX_TX_LVL                        GENMASK(3, 0)
> > ...
> > > +       if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
> > > +               /* Configure the pad as input */
> > > +               val = readl(imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG061);
> > > +               writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> > > +                      imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG061);
> > > +       } else if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
> > > +               /* Configure the PHY to output the refclock via pad */
> > > +               writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
> > > +                      imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG061);
> > > +               writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
> > > +                      imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG062);
> > > +               writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
> > > +                      imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG063);
> > 
> > If I comment out this writel() then the register defaults to 0x0/AUX_IN
> > and then the system behaves as expected.
> > 
> > > +               val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
> > > +               writel(val | ANA_AUX_RX_TERM_GND_EN,
> > > +                      imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG064);
> > > +               writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
> > > +                      imx8_phy->base +
> > IMX8MM_PCIE_PHY_CMN_REG065);
> > > +       }

Re: [PATCH v7 0/8] Add the imx8m pcie phy driver and imx8mm pcie support

[Marcel Ziswiler]

Hi Richard

On Thu, 2021-12-02 at 16:02 +0800, Richard Zhu wrote:
> Refer to the discussion [1] when try to enable i.MX8MM PCIe support,
> one standalone PCIe PHY driver should be seperated from i.MX PCIe
> driver when enable i.MX8MM PCIe support.
> 
> This patch-set adds the standalone PCIe PHY driver suport[1-5], and i.MX8MM
> PCIe support[6-8] to have whole view to review this patch-set.
> 
> The PCIe works on i.MX8MM EVK board based the the blkctrl power driver
> [2] and this patch-set. And tested by Tim and Marcel on the different
> reference clock modes boards.
> 
> [1] https://patchwork.ozlabs.org/project/linux-pci/patch/20210510141509.929120-3-l.stach@pengutronix.de/
> [2]
> https://patchwork.kernel.org/project/linux-arm-kernel/cover/20210910202640.980366-1-l.stach@pengutronix.de/
> 
> Main changes v6 --> v7:
> - Add "Reviewed-by: Rob Herring <robh@kernel.org>" into #2 patches.
> - Regarding Vinod's review comments do the following changes.
>   - Don't build in the PHY driver in default.
>   - Remove the extra blank line
>   - Correct the license tag.
> 
> Main changes v5 --> v6:
> - Add "Reviewed-by: Rob Herring <robh@kernel.org>" into #1 and #3 patches.
> - Merge Rob's review comments to the #2 patch.
> 
> Main changes v4 --> v5:
> - Set the AUX_EN always 1b'1, thus it can fix the regression introduced in v4
>   series on Marcel's board.
> - Use the lower-case letter in the devicetreee refer to Marcel's comments.
> - Since the default value of the deemphasis parameters are zero, only set
>   the deemphasis registers when the input paramters are none zero.
> 
> Main changes v3 --> v4:
> - Update the yaml to fix syntax error, add maxitems and drop description of phy
> - Correct the clock name in PHY DT node.
> - Squash the EVK board relalted dts changes into one patch, and drop the
>   useless dummy clock and gpio suffix in DT nodes.
> - Add board specific de-emphasis parameters as DT properties. Thus each board
>   can specify its actual de-emphasis values.
> - Update the commit log of PHY driver.
> - Remove the useless codes from PCIe driver, since they are moved to PHY driver
> - After the discussion and verification of the CLKREQ# configurations with Tim,
>   agree to add an optional boolean property "fsl,clkreq-unsupported", indicates
>   the CLKREQ# signal is hooked or not in HW designs.
> - Add "Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>" tag, since
>   Marcel help to test the v3 patch-set.
> 
> Main changes v2 --> v3:
> - Regarding Lucas' comments.
>  - to have a whole view to review the patches, send out the i.MX8MM PCIe support too.
>  - move the PHY related bits manipulations of the GPR/SRC to standalone PHY driver.
>  - split the dts changes to SOC and board DT, and use the enum instead of raw value.
>  - update the license of the dt-binding header file.
> 
> Changes v1 --> v2:
> - Update the license of the dt-binding header file to make the license
>   compatible with dts files.
> - Fix the dt_binding_check errors.
> 
> Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml    |   6 +++
> Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml |  92 +++++++++++++++++++++++++++++++
> arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi                |  55 +++++++++++++++++++
> arch/arm64/boot/dts/freescale/imx8mm.dtsi                    |  46 +++++++++++++++-
> drivers/pci/controller/dwc/pci-imx6.c                        |  83 +++++++++++++++++++++++++---
> drivers/phy/freescale/Kconfig                                |   8 +++
> drivers/phy/freescale/Makefile                               |   1 +
> drivers/phy/freescale/phy-fsl-imx8m-pcie.c                   | 236
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> include/dt-bindings/phy/phy-imx8-pcie.h                      |  14 +++++
> 9 files changed, 532 insertions(+), 9 deletions(-)
> 
> [PATCH v7 1/8] dt-bindings: phy: phy-imx8-pcie: Add binding for the
> [PATCH v7 2/8] dt-bindings: phy: Add imx8 pcie phy driver support
> [PATCH v7 3/8] dt-bindings: imx6q-pcie: Add PHY phandles and name
> [PATCH v7 4/8] arm64: dts: imx8mm: Add the pcie phy support
> [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> [PATCH v7 6/8] arm64: dts: imx8mm: Add the pcie support
> [PATCH v7 7/8] arm64: dts: imx8mm-evk: Add the pcie support on imx8mm
> [PATCH v7 8/8] PCI: imx: Add the imx8mm pcie support

What is the status of patches 4, 6 and 7? While the rest has been pulled those are still missing in today's -
next.

Cheers

Marcel

RE: [PATCH v7 0/8] Add the imx8m pcie phy driver and imx8mm pcie support

[Hongxing Zhu]

> -----Original Message-----
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Sent: Thursday, January 13, 2022 4:07 PM
> To: kishon@ti.com; vkoul@kernel.org; lorenzo.pieralisi@arm.com;
> l.stach@pengutronix.de; tharvey@gateworks.com; robh@kernel.org;
> galak@kernel.crashing.org; Hongxing Zhu <hongxing.zhu@nxp.com>;
> bhelgaas@google.com; shawnguo@kernel.org
> Cc: linux-phy@lists.infradead.org; linux-pci@vger.kernel.org;
> kernel@pengutronix.de; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v7 0/8] Add the imx8m pcie phy driver and imx8mm
> pcie support
> 
> Hi Richard
> 
> On Thu, 2021-12-02 at 16:02 +0800, Richard Zhu wrote:
> > Refer to the discussion [1] when try to enable i.MX8MM PCIe support,
> > one standalone PCIe PHY driver should be seperated from i.MX PCIe
> > driver when enable i.MX8MM PCIe support.
> >
> > This patch-set adds the standalone PCIe PHY driver suport[1-5], and
> > i.MX8MM PCIe support[6-8] to have whole view to review this
> patch-set.
> >
> > The PCIe works on i.MX8MM EVK board based the the blkctrl power
> driver
> > [2] and this patch-set. And tested by Tim and Marcel on the different
> > reference clock modes boards.
> >
> > [1]
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> tc
> >
> hwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F20210510141509.
> 929120
> >
> -3-l.stach%40pengutronix.de%2F&amp;data=04%7C01%7Chongxing.zhu
> %40nxp.c
> >
> om%7C487b28aecfd14bdfe1b808d9d66bb4ce%7C686ea1d3bc2b4c6fa92
> cd99c5c3016
> >
> 35%7C0%7C0%7C637776580350900040%7CUnknown%7CTWFpbGZsb3d
> 8eyJWIjoiMC4wLj
> >
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&
> amp;sdata=
> >
> 8pMSNCoRVpWBld2dSGUUw2Dpq%2FlRAqsVWLqAJ0njEgo%3D&amp;re
> served=0
> > [2]
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> tc
> >
> hwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fcover%2F20210910
> 202640
> > .980366-1-l.stach%40pengutronix.de%2F&amp;data=04%7C01%7Chon
> gxing.zhu%
> >
> 40nxp.com%7C487b28aecfd14bdfe1b808d9d66bb4ce%7C686ea1d3bc2b
> 4c6fa92cd99
> >
> c5c301635%7C0%7C0%7C637776580350900040%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjo
> >
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000&amp
> > ;sdata=MHPLXbIyL2pnZK%2FdVkDcJBJBjlqtPIOzzwLsNEKlCqs%3D&amp
> ;reserved=0
> >
> > Main changes v6 --> v7:
> > - Add "Reviewed-by: Rob Herring <robh@kernel.org>" into #2 patches.
> > - Regarding Vinod's review comments do the following changes.
> >   - Don't build in the PHY driver in default.
> >   - Remove the extra blank line
> >   - Correct the license tag.
> >
> > Main changes v5 --> v6:
> > - Add "Reviewed-by: Rob Herring <robh@kernel.org>" into #1 and #3
> patches.
> > - Merge Rob's review comments to the #2 patch.
> >
> > Main changes v4 --> v5:
> > - Set the AUX_EN always 1b'1, thus it can fix the regression
> > introduced in v4
> >   series on Marcel's board.
> > - Use the lower-case letter in the devicetreee refer to Marcel's
> comments.
> > - Since the default value of the deemphasis parameters are zero, only
> > set
> >   the deemphasis registers when the input paramters are none zero.
> >
> > Main changes v3 --> v4:
> > - Update the yaml to fix syntax error, add maxitems and drop
> > description of phy
> > - Correct the clock name in PHY DT node.
> > - Squash the EVK board relalted dts changes into one patch, and drop
> > the
> >   useless dummy clock and gpio suffix in DT nodes.
> > - Add board specific de-emphasis parameters as DT properties. Thus
> > each board
> >   can specify its actual de-emphasis values.
> > - Update the commit log of PHY driver.
> > - Remove the useless codes from PCIe driver, since they are moved to
> > PHY driver
> > - After the discussion and verification of the CLKREQ# configurations
> > with Tim,
> >   agree to add an optional boolean property "fsl,clkreq-unsupported",
> > indicates
> >   the CLKREQ# signal is hooked or not in HW designs.
> > - Add "Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>" tag,
> > since
> >   Marcel help to test the v3 patch-set.
> >
> > Main changes v2 --> v3:
> > - Regarding Lucas' comments.
> >  - to have a whole view to review the patches, send out the i.MX8MM
> PCIe support too.
> >  - move the PHY related bits manipulations of the GPR/SRC to
> standalone PHY driver.
> >  - split the dts changes to SOC and board DT, and use the enum
> instead of raw value.
> >  - update the license of the dt-binding header file.
> >
> > Changes v1 --> v2:
> > - Update the license of the dt-binding header file to make the license
> >   compatible with dts files.
> > - Fix the dt_binding_check errors.
> >
> >
> Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.yaml    |   6
> +++
> > Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml |  92
> > +++++++++++++++++++++++++++++++
> >
> arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
>  |  55
> > +++++++++++++++++++
> arch/arm64/boot/dts/freescale/imx8mm.dtsi
> 
> > |  46 +++++++++++++++-
> drivers/pci/controller/dwc/pci-imx6.c
> > |  83 +++++++++++++++++++++++++---
> drivers/phy/freescale/Kconfig
> 
> > |   8 +++
> drivers/phy/freescale/Makefile
> 
> > |   1 +
> drivers/phy/freescale/phy-fsl-imx8m-pcie.c                   |
> > 236
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++
> >
> include/dt-bindings/phy/phy-imx8-pcie.h
>  |  14
> > +++++
> > 9 files changed, 532 insertions(+), 9 deletions(-)
> >
> > [PATCH v7 1/8] dt-bindings: phy: phy-imx8-pcie: Add binding for the
> > [PATCH v7 2/8] dt-bindings: phy: Add imx8 pcie phy driver support
> > [PATCH v7 3/8] dt-bindings: imx6q-pcie: Add PHY phandles and name
> > [PATCH v7 4/8] arm64: dts: imx8mm: Add the pcie phy support [PATCH
> v7
> > 5/8] phy: freescale: pcie: Initialize the imx8 pcie [PATCH v7 6/8]
> > arm64: dts: imx8mm: Add the pcie support [PATCH v7 7/8] arm64: dts:
> > imx8mm-evk: Add the pcie support on imx8mm [PATCH v7 8/8] PCI:
> imx:
> > Add the imx8mm pcie support
> 
> What is the status of patches 4, 6 and 7? While the rest has been pulled
> those are still missing in today's - next.
[Richard Zhu] Thanks for your care.
I used to ping Shawn twice on Dec17/27 2021, but couldn't receive his response.
Anyway, I would continue to ping him later.

Best Regards
Richard Zhu

> 
> Cheers
> 
> Marcel

Re: [PATCH v7 4/8] arm64: dts: imx8mm: Add the pcie phy support

[Shawn Guo]

On Thu, Dec 02, 2021 at 04:02:34PM +0800, Richard Zhu wrote:
> Add the PCIe PHY support on iMX8MM platforms.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> Tested-by: Tim Harvey <tharvey@gateworks.com>

Applied, thanks!

Re: [PATCH v7 6/8] arm64: dts: imx8mm: Add the pcie support

[Shawn Guo]

On Thu, Dec 02, 2021 at 04:02:36PM +0800, Richard Zhu wrote:
> Add the PCIe support on i.MX8MM platforms.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> Tested-by: Tim Harvey <tharvey@gateworks.com>

Applied, thanks!

Re: [PATCH v7 7/8] arm64: dts: imx8mm-evk: Add the pcie support on imx8mm evk board

[Shawn Guo]

On Thu, Dec 02, 2021 at 04:02:37PM +0800, Richard Zhu wrote:
> Add the PCIe support on iMX8MM EVK boards.
> And set the default reference clock mode.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> Tested-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> index e033d0257b5a..87a30daf0b3c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> @@ -5,6 +5,7 @@
>  
>  /dts-v1/;
>  
> +#include <dt-bindings/phy/phy-imx8-pcie.h>
>  #include <dt-bindings/usb/pd.h>
>  #include "imx8mm.dtsi"
>  
> @@ -30,6 +31,23 @@ status {
>  		};
>  	};
>  
> +	pcie0_refclk: pcie0-refclk {
> +		compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <100000000>;

Incorrect indent.  Fixed them up and applied.

Shawn

> +	};
> +
> +	reg_pcie0: regulator-pcie {
> +		compatible = "regulator-fixed";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_pcie0_reg>;
> +		regulator-name = "MPCIE_3V3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&gpio1 5 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +
>  	reg_usdhc2_vmmc: regulator-usdhc2 {
>  		compatible = "regulator-fixed";
>  		pinctrl-names = "default";
> @@ -289,6 +307,30 @@ pca6416: gpio@20 {
>  	};
>  };
>  
> +&pcie_phy {
> +	fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>;
> +	fsl,tx-deemph-gen1 = <0x2d>;
> +	fsl,tx-deemph-gen2 = <0xf>;
> +	clocks = <&pcie0_refclk>;
> +	status = "okay";
> +};
> +
> +&pcie0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_pcie0>;
> +	reset-gpio = <&gpio4 21 GPIO_ACTIVE_LOW>;
> +	clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>, <&clk IMX8MM_CLK_PCIE1_AUX>,
> +		 <&pcie0_refclk>;
> +	clock-names = "pcie", "pcie_aux", "pcie_bus";
> +	assigned-clocks = <&clk IMX8MM_CLK_PCIE1_AUX>,
> +			  <&clk IMX8MM_CLK_PCIE1_CTRL>;
> +	assigned-clock-rates = <10000000>, <250000000>;
> +	assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_50M>,
> +				 <&clk IMX8MM_SYS_PLL2_250M>;
> +	vpcie-supply = <&reg_pcie0>;
> +	status = "okay";
> +};
> +
>  &sai3 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_sai3>;
> @@ -406,6 +448,19 @@ MX8MM_IOMUXC_I2C3_SDA_I2C3_SDA			0x400001c3
>  		>;
>  	};
>  
> +	pinctrl_pcie0: pcie0grp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B    0x61
> +			MX8MM_IOMUXC_SAI2_RXFS_GPIO4_IO21       0x41
> +		>;
> +	};
> +
> +	pinctrl_pcie0_reg: pcie0reggrp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_GPIO1_IO05_GPIO1_IO5       0x41
> +		>;
> +	};
> +
>  	pinctrl_pmic: pmicirqgrp {
>  		fsl,pins = <
>  			MX8MM_IOMUXC_GPIO1_IO03_GPIO1_IO3		0x141
> -- 
> 2.25.1
>