[PATCH 1/7] PCI: imx6: Move i.MX8MQ controller instance check to correct case statement

[PATCH 1/7] PCI: imx6: Move i.MX8MQ controller instance check to correct case statement

[Lucas Stach]

While the i.MX8MQ case falls through to the i.MX7D case, it's quite confusing
to have the i.MX8MQ specific controller instance check in that statement.
Move it to the 8MQ case.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0cf1333c0440..46b5f070939e 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1060,11 +1060,11 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		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");
-		fallthrough;
-	case IMX7D:
 		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
 			imx6_pcie->controller_id = 1;
 
+		fallthrough;
+	case IMX7D:
 		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
 									    "pciephy");
 		if (IS_ERR(imx6_pcie->pciephy_reset)) {
-- 
2.29.2

[PATCH 2/7] PCI: imx6: Initialize ATU unroll offset

[Lucas Stach]

This gets rid of a warning printed when the common code tries to get
this address via non-existent DT reg region, before falling back to
the default offset.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 46b5f070939e..922c14361cd3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1063,6 +1063,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
 			imx6_pcie->controller_id = 1;
 
+		pci->atu_base =  pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
+
 		fallthrough;
 	case IMX7D:
 		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
-- 
2.29.2

[PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Lucas Stach]

We don't need to have a phandle of the PHY, as we know the compatible
of the node we are looking for. This will make it easier to put add
more PHY handling for new generations later on, where the
"fsl,imx7d-pcie-phy" phandle would be a misnomer.

Also we can use a helper function to get the resource for us,
simplifying out driver code a bit.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt  |  5 ++---
 drivers/pci/controller/dwc/pci-imx6.c           | 17 +++++------------
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index de4b2baf91e8..308540df99ef 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -54,7 +54,6 @@ Additional required properties for imx7d-pcie and imx8mq-pcie:
 	       - "pciephy"
 	       - "apps"
 	       - "turnoff"
-- fsl,imx7d-pcie-phy: A phandle to an fsl,imx7d-pcie-phy node.
 
 Additional required properties for imx8mq-pcie:
 - clock-names: Must include the following additional entries:
@@ -88,8 +87,8 @@ Example:
 
 * Freescale i.MX7d PCIe PHY
 
-This is the PHY associated with the IMX7d PCIe controller.  It's used by the
-PCI-e controller via the fsl,imx7d-pcie-phy phandle.
+This is the PHY associated with the IMX7d PCIe controller.  It's looked up by
+the PCI-e controller via the fsl,imx7d-pcie-phy compatible.
 
 Required properties:
 - compatible:
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 922c14361cd3..5e13758222e8 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -555,7 +555,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 			writel(PCIE_PHY_CMN_REG26_ATT_MODE,
 			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG26);
 		} else {
-			dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy phandle ?\n");
+			dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy node?\n");
 		}
 
 		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
@@ -970,7 +970,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct dw_pcie *pci;
 	struct imx6_pcie *imx6_pcie;
-	struct device_node *np;
+	struct device_node *np = NULL;
 	struct resource *dbi_base;
 	struct device_node *node = dev->of_node;
 	int ret;
@@ -991,17 +991,10 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	imx6_pcie->pci = pci;
 	imx6_pcie->drvdata = of_device_get_match_data(dev);
 
-	/* Find the PHY if one is defined, only imx7d uses it */
-	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
+	/* Find the PHY if one is present in DT, only imx7d uses it */
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy");
 	if (np) {
-		struct resource res;
-
-		ret = of_address_to_resource(np, 0, &res);
-		if (ret) {
-			dev_err(dev, "Unable to map PCIe PHY\n");
-			return ret;
-		}
-		imx6_pcie->phy_base = devm_ioremap_resource(dev, &res);
+		imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL);
 		if (IS_ERR(imx6_pcie->phy_base)) {
 			dev_err(dev, "Unable to map PCIe PHY\n");
 			return PTR_ERR(imx6_pcie->phy_base);
-- 
2.29.2

[PATCH 4/7] dt-bindings: imx6q-pcie: add a property configure refclk pad usage mode

[Lucas Stach]

Starting with the i.MX7, arch PCIe instance has a differential refclk pad,
which can beused in multiple ways:

- It's not used at all and the PHY reference clock is provided by a SoC
  internal source, like on the previous SOCs.
- It's used as a clock input, for the board to provide a reference clock
  for the PHY.
- It's used as a clock output, where the PHY reference clock is provided
  by a SoC internal source and the same clock is also routed to the
  refclk pad for consumption of board-level components.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 308540df99ef..3ebd8553a818 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -38,6 +38,11 @@ Optional properties:
   The regulator will be enabled when initializing the PCIe host and
   disabled either as part of the init process or when shutting down the
   host.
+- fsl,refclk-pad-mode: Usage mode of the refclk pad. Valid values:
+  - 0: pad not used. PHY refclock is derived from SoC internal source.
+  - 1: pad input. PHY refclock is provided externally via the refclk pad.
+  - 2: pad output. PHY refclock is derived from SoC internal source and
+       provided on the refclk pad.
 
 Additional required properties for imx6sx-pcie:
 - clock names: Must include the following additional entries:
-- 
2.29.2

[PATCH 5/7] PCI: imx6: Configure PHY refclock according to DT property

[Lucas Stach]

This configures the PHY reference clock input according to the newly
introduced fsl,refclk-pad-mode DT property. The default in absence
of this property is to use the refclk pad on i.MX8MQ and the internal
reference clock on all other SoCs, keeping compatibility with existing
devicetrees, but allowing boards to configure this when they differ
from the default.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 5e13758222e8..f184077f6d17 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -79,6 +79,7 @@ struct imx6_pcie {
 	u32			tx_deemph_gen2_6db;
 	u32			tx_swing_full;
 	u32			tx_swing_low;
+	u32			refclk_pad_mode;
 	struct regulator	*vpcie;
 	void __iomem		*phy_base;
 
@@ -613,18 +614,17 @@ static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 {
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX8MQ:
-		/*
-		 * TODO: Currently this code assumes external
-		 * oscillator is being used
-		 */
 		regmap_update_bits(imx6_pcie->iomuxc_gpr,
 				   imx6_pcie_grp_offset(imx6_pcie),
 				   IMX8MQ_GPR_PCIE_REF_USE_PAD,
-				   IMX8MQ_GPR_PCIE_REF_USE_PAD);
+				   imx6_pcie->refclk_pad_mode == 1 ?
+				   IMX8MQ_GPR_PCIE_REF_USE_PAD : 0);
 		break;
 	case IMX7D:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+				   imx6_pcie->refclk_pad_mode == 1 ?
+				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL : 0);
 		break;
 	case IMX6SX:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
@@ -1049,6 +1049,12 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 					     "pcie_inbound_axi clock missing or invalid\n");
 		break;
 	case IMX8MQ:
+		/*
+		 * i.MX8MQ is special, as the default refclk pad mode is set to
+		 * input in order to keep compatibility with old devicetrees.
+		 */
+		imx6_pcie->refclk_pad_mode = 1;
+
 		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),
@@ -1114,6 +1120,10 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 				 &imx6_pcie->tx_swing_low))
 		imx6_pcie->tx_swing_low = 127;
 
+	/* get PHY refclk pad mode */
+	of_property_read_u32(node, "fsl,refclk-pad-mode",
+			     &imx6_pcie->refclk_pad_mode);
+
 	/* Limit link speed */
 	pci->link_gen = 1;
 	ret = of_property_read_u32(node, "fsl,max-link-speed", &pci->link_gen);
-- 
2.29.2

[PATCH 6/7] dt-bindings: imx6q-pcie: add compatibles for i.MX8MM PCIe

[Lucas Stach]

This adds compatibles for both the PCIe host controller and PCIe
PHY found on the i.MX8MM SoC.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 3ebd8553a818..f1f5651031a5 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -10,6 +10,7 @@ Required properties:
 	- "fsl,imx6qp-pcie"
 	- "fsl,imx7d-pcie"
 	- "fsl,imx8mq-pcie"
+	- "fsl,imx8mm-pcie"
 - reg: base address and length of the PCIe controller
 - interrupts: A list of interrupt outputs of the controller. Must contain an
   entry for each entry in the interrupt-names property.
@@ -90,12 +91,13 @@ Example:
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
 	};
 
-* Freescale i.MX7d PCIe PHY
+* Freescale i.MX PCIe PHY
 
-This is the PHY associated with the IMX7d PCIe controller.  It's looked up by
-the PCI-e controller via the fsl,imx7d-pcie-phy compatible.
+This is the PHY associated with the IMX PCIe controller.  It's looked up by
+the PCI-e controller via the compatible.
 
 Required properties:
 - compatible:
 	- "fsl,imx7d-pcie-phy"
+	- "fsl,imx8mm-pcie-phy"
 - reg: base address and length of the PCIe PHY controller
-- 
2.29.2

[PATCH 7/7] PCI: imx6: Add i.MX8MM support

[Lucas Stach]

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.

Based on work in the downstream kernel by Richard Zhu.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/pci/controller/dwc/pci-imx6.c | 105 +++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index f184077f6d17..1f3e806ade64 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -40,6 +40,14 @@
 #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE	GENMASK(11, 8)
 #define IMX8MQ_PCIE2_BASE_ADDR			0x33c00000
 
+#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 to_imx6_pcie(x)	dev_get_drvdata((x)->dev)
 
 enum imx6_pcie_variants {
@@ -48,6 +56,7 @@ enum imx6_pcie_variants {
 	IMX6QP,
 	IMX7D,
 	IMX8MQ,
+	IMX8MM,
 };
 
 #define IMX6_PCIE_FLAG_IMX6_PHY			BIT(0)
@@ -138,6 +147,17 @@ struct imx6_pcie {
 #define PCIE_PHY_CMN_REG26		0x98
 #define PCIE_PHY_CMN_REG26_ATT_MODE	0xBC
 
+#define PCIE_PHY_CMN_REG62			0x188
+#define PCIE_PHY_CMN_REG62_PLL_CLK_OUT		BIT(3)
+#define PCIE_PHY_CMN_REG64			0x190
+#define PCIE_PHY_CMN_REG64_AUX_RX_TX_TERM	0x8C
+#define 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 PHY_RX_OVRD_IN_LO 0x1005
 #define PHY_RX_OVRD_IN_LO_RX_DATA_EN		BIT(5)
 #define PHY_RX_OVRD_IN_LO_RX_PLL_EN		BIT(3)
@@ -370,6 +390,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 	case IMX8MQ:
+	case IMX8MM:
 		reset_control_assert(imx6_pcie->pciephy_reset);
 		reset_control_assert(imx6_pcie->apps_reset);
 		break;
@@ -406,7 +427,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 +468,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 	case IMX7D:
 		break;
 	case IMX8MQ:
+	case IMX8MM:
 		ret = clk_prepare_enable(imx6_pcie->pcie_aux);
 		if (ret) {
 			dev_err(dev, "unable to enable pcie_aux clock\n");
@@ -482,6 +505,19 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
 		dev_err(dev, "PCIe PLL lock timeout\n");
 }
 
+static void imx8mm_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie)
+{
+	struct device *dev = imx6_pcie->pci->dev;
+	int ret;
+	u32 val;
+
+	ret = readl_poll_timeout(imx6_pcie->phy_base + PCIE_PHY_CMN_REG75,
+				 val, val == PCIE_PHY_CMN_REG75_PLL_DONE,
+				 10, 20000);
+	if (ret)
+		dev_err(dev, "PCIe PLL lock timeout\n");
+}
+
 static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
@@ -537,6 +573,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:
+		reset_control_deassert(imx6_pcie->pciephy_reset);
+		imx8mm_pcie_wait_for_phy_pll_lock(imx6_pcie);
+		break;
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 
@@ -613,6 +653,47 @@ 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:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR14,
+				   IMX8MQ_GPR_PCIE_REF_USE_PAD,
+				   imx6_pcie->refclk_pad_mode == 1 ?
+				   IMX8MQ_GPR_PCIE_REF_USE_PAD :0);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR14,
+				   IMX8MM_GPR_PCIE_AUX_EN,
+				   IMX8MM_GPR_PCIE_AUX_EN);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR14,
+				   IMX8MM_GPR_PCIE_POWER_OFF, 0);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR14,
+				   IMX8MM_GPR_PCIE_SSC_EN, 0);
+
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR14,
+				   IMX8MM_GPR_PCIE_REF_CLK_SEL,
+				   imx6_pcie->refclk_pad_mode == 1 ?
+				   IMX8MM_GPR_PCIE_REF_CLK_EXT :
+				   IMX8MM_GPR_PCIE_REF_CLK_PLL);
+
+		usleep_range(100, 200);
+
+		if (imx6_pcie->refclk_pad_mode == 2) {
+			/* Configure the PHY to output the refclock via pad */
+			writel(PCIE_PHY_CMN_REG62_PLL_CLK_OUT,
+			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG62);
+			writel(PCIE_PHY_CMN_REG64_AUX_RX_TX_TERM,
+			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG64);
+		}
+
+		/* Do the PHY common block reset */
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR14,
+				   IMX8MM_GPR_PCIE_CMN_RST,
+				   IMX8MM_GPR_PCIE_CMN_RST);
+		usleep_range(200, 500);
+
+		/* Tune PHY de-emphasis setting to pass PCIe compliance. */
+		writel(PCIE_PHY_TRSV_REG5_GEN1_DEEMP,
+		       imx6_pcie->phy_base + PCIE_PHY_TRSV_REG5);
+		writel(PCIE_PHY_TRSV_REG6_GEN2_DEEMP,
+		       imx6_pcie->phy_base + PCIE_PHY_TRSV_REG6);
+		break;
 	case IMX8MQ:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr,
 				   imx6_pcie_grp_offset(imx6_pcie),
@@ -740,6 +821,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;
 	}
@@ -858,6 +940,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:
@@ -917,6 +1000,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:
@@ -991,8 +1075,17 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	imx6_pcie->pci = pci;
 	imx6_pcie->drvdata = of_device_get_match_data(dev);
 
-	/* Find the PHY if one is present in DT, only imx7d uses it */
-	np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy");
+	/* Find the PHY if one is present in DT */
+	switch (imx6_pcie->drvdata->variant) {
+	case IMX7D:
+		np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy");
+		break;
+	case IMX8MM:
+		np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-pcie-phy");
+		break;
+	default:
+		break;
+	}
 	if (np) {
 		imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL);
 		if (IS_ERR(imx6_pcie->phy_base)) {
@@ -1055,6 +1148,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		 */
 		imx6_pcie->refclk_pad_mode = 1;
 
+		fallthrough;
+	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),
@@ -1188,6 +1283,9 @@ static const struct imx6_pcie_drvdata drvdata[] = {
 	[IMX8MQ] = {
 		.variant = IMX8MQ,
 	},
+	[IMX8MM] = {
+		.variant = IMX8MM,
+	},
 };
 
 static const struct of_device_id imx6_pcie_of_match[] = {
@@ -1196,6 +1294,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
 	{ .compatible = "fsl,imx6qp-pcie", .data = &drvdata[IMX6QP], },
 	{ .compatible = "fsl,imx7d-pcie",  .data = &drvdata[IMX7D],  },
 	{ .compatible = "fsl,imx8mq-pcie", .data = &drvdata[IMX8MQ], } ,
+	{ .compatible = "fsl,imx8mm-pcie", .data = &drvdata[IMX8MM], } ,
 	{},
 };
 
-- 
2.29.2

Re: [PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Rob Herring]

On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote:
> We don't need to have a phandle of the PHY, as we know the compatible
> of the node we are looking for. This will make it easier to put add
> more PHY handling for new generations later on, where the
> "fsl,imx7d-pcie-phy" phandle would be a misnomer.
> 
> Also we can use a helper function to get the resource for us,
> simplifying out driver code a bit.

Better yes, but really all the phy handling should be split out to 
its own driver even in the older h/w with shared phy registers.

Soon as there's a chip with 2 PCI hosts, you're going to need the phy 
binding.

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt  |  5 ++---
>  drivers/pci/controller/dwc/pci-imx6.c           | 17 +++++------------
>  2 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index de4b2baf91e8..308540df99ef 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -54,7 +54,6 @@ Additional required properties for imx7d-pcie and imx8mq-pcie:
>  	       - "pciephy"
>  	       - "apps"
>  	       - "turnoff"
> -- fsl,imx7d-pcie-phy: A phandle to an fsl,imx7d-pcie-phy node.
>  
>  Additional required properties for imx8mq-pcie:
>  - clock-names: Must include the following additional entries:
> @@ -88,8 +87,8 @@ Example:
>  
>  * Freescale i.MX7d PCIe PHY
>  
> -This is the PHY associated with the IMX7d PCIe controller.  It's used by the
> -PCI-e controller via the fsl,imx7d-pcie-phy phandle.
> +This is the PHY associated with the IMX7d PCIe controller.  It's looked up by
> +the PCI-e controller via the fsl,imx7d-pcie-phy compatible.
>  
>  Required properties:
>  - compatible:
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 922c14361cd3..5e13758222e8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -555,7 +555,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
>  			writel(PCIE_PHY_CMN_REG26_ATT_MODE,
>  			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG26);
>  		} else {
> -			dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy phandle ?\n");
> +			dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy node?\n");
>  		}
>  
>  		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> @@ -970,7 +970,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct dw_pcie *pci;
>  	struct imx6_pcie *imx6_pcie;
> -	struct device_node *np;
> +	struct device_node *np = NULL;
>  	struct resource *dbi_base;
>  	struct device_node *node = dev->of_node;
>  	int ret;
> @@ -991,17 +991,10 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  	imx6_pcie->pci = pci;
>  	imx6_pcie->drvdata = of_device_get_match_data(dev);
>  
> -	/* Find the PHY if one is defined, only imx7d uses it */
> -	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
> +	/* Find the PHY if one is present in DT, only imx7d uses it */
> +	np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy");
>  	if (np) {
> -		struct resource res;
> -
> -		ret = of_address_to_resource(np, 0, &res);
> -		if (ret) {
> -			dev_err(dev, "Unable to map PCIe PHY\n");
> -			return ret;
> -		}
> -		imx6_pcie->phy_base = devm_ioremap_resource(dev, &res);
> +		imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL);
>  		if (IS_ERR(imx6_pcie->phy_base)) {
>  			dev_err(dev, "Unable to map PCIe PHY\n");
>  			return PTR_ERR(imx6_pcie->phy_base);
> -- 
> 2.29.2
> 

Re: [PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Lucas Stach]

Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring:
> On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote:
> > We don't need to have a phandle of the PHY, as we know the compatible
> > of the node we are looking for. This will make it easier to put add
> > more PHY handling for new generations later on, where the
> > "fsl,imx7d-pcie-phy" phandle would be a misnomer.
> > 
> > Also we can use a helper function to get the resource for us,
> > simplifying out driver code a bit.
> 
> Better yes, but really all the phy handling should be split out to 
> its own driver even in the older h/w with shared phy registers.
> 
That would be a quite massive DT binding changing break, possibly even
a separate driver. Maybe it's time to do this for i.MX8MM, as the
current driver just kept piling on special cases for "almost the same"
hardware that by now looks quite different to the original i.MX6 PCIe
integration this driver was supposed to handle.

> Soon as there's a chip with 2 PCI hosts, you're going to need the phy 
> binding.

Uh, there even is a chip like that already (i.MX8MQ), it just happened
to not require any handling of the PHY registers. Don't know why I
didn't think of this. :/

Regards,
Lucas

> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt  |  5 ++---
> >  drivers/pci/controller/dwc/pci-imx6.c           | 17 +++++------------
> >  2 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > index de4b2baf91e8..308540df99ef 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > @@ -54,7 +54,6 @@ Additional required properties for imx7d-pcie and imx8mq-pcie:
> >  	       - "pciephy"
> >  	       - "apps"
> >  	       - "turnoff"
> > -- fsl,imx7d-pcie-phy: A phandle to an fsl,imx7d-pcie-phy node.
> >  
> >  Additional required properties for imx8mq-pcie:
> >  - clock-names: Must include the following additional entries:
> > @@ -88,8 +87,8 @@ Example:
> >  
> >  * Freescale i.MX7d PCIe PHY
> >  
> > -This is the PHY associated with the IMX7d PCIe controller.  It's used by the
> > -PCI-e controller via the fsl,imx7d-pcie-phy phandle.
> > +This is the PHY associated with the IMX7d PCIe controller.  It's looked up by
> > +the PCI-e controller via the fsl,imx7d-pcie-phy compatible.
> >  
> >  Required properties:
> >  - compatible:
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 922c14361cd3..5e13758222e8 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -555,7 +555,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> >  			writel(PCIE_PHY_CMN_REG26_ATT_MODE,
> >  			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG26);
> >  		} else {
> > -			dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy phandle ?\n");
> > +			dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy node?\n");
> >  		}
> >  
> >  		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > @@ -970,7 +970,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct dw_pcie *pci;
> >  	struct imx6_pcie *imx6_pcie;
> > -	struct device_node *np;
> > +	struct device_node *np = NULL;
> >  	struct resource *dbi_base;
> >  	struct device_node *node = dev->of_node;
> >  	int ret;
> > @@ -991,17 +991,10 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  	imx6_pcie->pci = pci;
> >  	imx6_pcie->drvdata = of_device_get_match_data(dev);
> >  
> > -	/* Find the PHY if one is defined, only imx7d uses it */
> > -	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
> > +	/* Find the PHY if one is present in DT, only imx7d uses it */
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy");
> >  	if (np) {
> > -		struct resource res;
> > -
> > -		ret = of_address_to_resource(np, 0, &res);
> > -		if (ret) {
> > -			dev_err(dev, "Unable to map PCIe PHY\n");
> > -			return ret;
> > -		}
> > -		imx6_pcie->phy_base = devm_ioremap_resource(dev, &res);
> > +		imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL);
> >  		if (IS_ERR(imx6_pcie->phy_base)) {
> >  			dev_err(dev, "Unable to map PCIe PHY\n");
> >  			return PTR_ERR(imx6_pcie->phy_base);
> > -- 
> > 2.29.2
> > 

Re: [PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Rob Herring]

On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring:
> > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote:
> > > We don't need to have a phandle of the PHY, as we know the compatible
> > > of the node we are looking for. This will make it easier to put add
> > > more PHY handling for new generations later on, where the
> > > "fsl,imx7d-pcie-phy" phandle would be a misnomer.
> > >
> > > Also we can use a helper function to get the resource for us,
> > > simplifying out driver code a bit.
> >
> > Better yes, but really all the phy handling should be split out to
> > its own driver even in the older h/w with shared phy registers.
> >
> That would be a quite massive DT binding changing break, possibly even
> a separate driver. Maybe it's time to do this for i.MX8MM, as the
> current driver just kept piling on special cases for "almost the same"
> hardware that by now looks quite different to the original i.MX6 PCIe
> integration this driver was supposed to handle.

No, you don't need to change DT, and a DT change adding a phy node
wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a
separate PHY driver would have to create its own platform device in
its initcall (if the iMX6 PCI compatible is found). Then the PCI
driver would need to use a non-DT based phy_get() lookup. For the
cases with a phandle to the phy, I'd assume a phy driver could be
instantiated for that node. You'll again need a non-DT phy_get() if
not using the phy binding.

Rob

Re: [PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Lucas Stach]

Am Dienstag, dem 11.05.2021 um 09:21 -0500 schrieb Rob Herring:
> On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring:
> > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote:
> > > > We don't need to have a phandle of the PHY, as we know the compatible
> > > > of the node we are looking for. This will make it easier to put add
> > > > more PHY handling for new generations later on, where the
> > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer.
> > > > 
> > > > Also we can use a helper function to get the resource for us,
> > > > simplifying out driver code a bit.
> > > 
> > > Better yes, but really all the phy handling should be split out to
> > > its own driver even in the older h/w with shared phy registers.
> > > 
> > That would be a quite massive DT binding changing break, possibly even
> > a separate driver. Maybe it's time to do this for i.MX8MM, as the
> > current driver just kept piling on special cases for "almost the same"
> > hardware that by now looks quite different to the original i.MX6 PCIe
> > integration this driver was supposed to handle.
> 
> No, you don't need to change DT, and a DT change adding a phy node
> wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a
> separate PHY driver would have to create its own platform device in
> its initcall (if the iMX6 PCI compatible is found). Then the PCI
> driver would need to use a non-DT based phy_get() lookup. For the
> cases with a phandle to the phy, I'd assume a phy driver could be
> instantiated for that node. You'll again need a non-DT phy_get() if
> not using the phy binding.

The original i.MX6 PCIe with the internal PHY is the easy case, as you
laid out above.

What I'm more concerned about is the i.MX7 and i.MX8MQ, where we have a
MMIO mapped PHY and quite a bit of the clocks/reset/GPR handling would
need to move from the controller to the PHY driver. Without a binding
change I fear that we end up in a worst of both worlds situation, where
we have lots of code in the driver to separate resources that are
currently all attached to the PCIe controller node in the DT, without a
real gain in making the driver any simpler or easier to maintain.

But right now that's all speculation. Maybe I need to type something up
and see where it falls on the shiny/horrible scale.

Regards,
Lucas

Re: [PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Rob Herring]

On Tue, May 11, 2021 at 9:54 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Dienstag, dem 11.05.2021 um 09:21 -0500 schrieb Rob Herring:
> > On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > >
> > > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring:
> > > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote:
> > > > > We don't need to have a phandle of the PHY, as we know the compatible
> > > > > of the node we are looking for. This will make it easier to put add
> > > > > more PHY handling for new generations later on, where the
> > > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer.
> > > > >
> > > > > Also we can use a helper function to get the resource for us,
> > > > > simplifying out driver code a bit.
> > > >
> > > > Better yes, but really all the phy handling should be split out to
> > > > its own driver even in the older h/w with shared phy registers.
> > > >
> > > That would be a quite massive DT binding changing break, possibly even
> > > a separate driver. Maybe it's time to do this for i.MX8MM, as the
> > > current driver just kept piling on special cases for "almost the same"
> > > hardware that by now looks quite different to the original i.MX6 PCIe
> > > integration this driver was supposed to handle.
> >
> > No, you don't need to change DT, and a DT change adding a phy node
> > wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a
> > separate PHY driver would have to create its own platform device in
> > its initcall (if the iMX6 PCI compatible is found). Then the PCI
> > driver would need to use a non-DT based phy_get() lookup. For the
> > cases with a phandle to the phy, I'd assume a phy driver could be
> > instantiated for that node. You'll again need a non-DT phy_get() if
> > not using the phy binding.
>
> The original i.MX6 PCIe with the internal PHY is the easy case, as you
> laid out above.
>
> What I'm more concerned about is the i.MX7 and i.MX8MQ, where we have a
> MMIO mapped PHY and quite a bit of the clocks/reset/GPR handling would
> need to move from the controller to the PHY driver. Without a binding
> change I fear that we end up in a worst of both worlds situation, where
> we have lots of code in the driver to separate resources that are
> currently all attached to the PCIe controller node in the DT, without a
> real gain in making the driver any simpler or easier to maintain.

One option for handling compatibility is making an overlay for old DTs
instead of coding the old DT handling. There's an example of this for
rcar-du in drivers/gpu/drm/rcar-du/rcar_du_of_lvds_*.dts.

Rob

Re: [PATCH 4/7] dt-bindings: imx6q-pcie: add a property configure refclk pad usage mode

[Rob Herring]

On Mon, May 10, 2021 at 04:15:06PM +0200, Lucas Stach wrote:
> Starting with the i.MX7, arch PCIe instance has a differential refclk pad,
> which can beused in multiple ways:
> 
> - It's not used at all and the PHY reference clock is provided by a SoC
>   internal source, like on the previous SOCs.
> - It's used as a clock input, for the board to provide a reference clock
>   for the PHY.
> - It's used as a clock output, where the PHY reference clock is provided
>   by a SoC internal source and the same clock is also routed to the
>   refclk pad for consumption of board-level components.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 308540df99ef..3ebd8553a818 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -38,6 +38,11 @@ Optional properties:
>    The regulator will be enabled when initializing the PCIe host and
>    disabled either as part of the init process or when shutting down the
>    host.
> +- fsl,refclk-pad-mode: Usage mode of the refclk pad. Valid values:
> +  - 0: pad not used. PHY refclock is derived from SoC internal source.
> +  - 1: pad input. PHY refclock is provided externally via the refclk pad.
> +  - 2: pad output. PHY refclock is derived from SoC internal source and
> +       provided on the refclk pad.

Seems like this belongs in the PHY's node?

Or you could determine this based on the PHY's clock source. At least 
for the first 2 cases. Is there a known user for the 3rd case? If so, 
it's possible that what it's connected to needs a clock provider as 
well.

Rob

Re: [PATCH 2/7] PCI: imx6: Initialize ATU unroll offset

[Rob Herring]

On Mon, May 10, 2021 at 04:15:04PM +0200, Lucas Stach wrote:
> This gets rid of a warning printed when the common code tries to get
> this address via non-existent DT reg region, before falling back to
> the default offset.

I guess I caused this... If this is a problem for i.MX, then isn't it a 
problem for everyone without a separate 'atu' region? I think the right 
fix is having an optional variant like we do with other resources.

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 46b5f070939e..922c14361cd3 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1063,6 +1063,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
>  			imx6_pcie->controller_id = 1;
>  
> +		pci->atu_base =  pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> +
>  		fallthrough;
>  	case IMX7D:
>  		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
> -- 
> 2.29.2
> 

Re: [PATCH 1/7] PCI: imx6: Move i.MX8MQ controller instance check to correct case statement

[Lorenzo Pieralisi]

On Mon, May 10, 2021 at 04:15:03PM +0200, Lucas Stach wrote:
> While the i.MX8MQ case falls through to the i.MX7D case, it's quite confusing
> to have the i.MX8MQ specific controller instance check in that statement.
> Move it to the 8MQ case.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Hi Lucas,

it looks like some of the patches in this series need a respin,
therefore I will mark it as "Changes Requested" unless there
are some patches I can cherry pick - please let me know your plan.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0cf1333c0440..46b5f070939e 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1060,11 +1060,11 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  		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");
> -		fallthrough;
> -	case IMX7D:
>  		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
>  			imx6_pcie->controller_id = 1;
>  
> +		fallthrough;
> +	case IMX7D:
>  		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
>  									    "pciephy");
>  		if (IS_ERR(imx6_pcie->pciephy_reset)) {
> -- 
> 2.29.2
> 

Re: [PATCH 3/7] PCI: imx6: Rework PHY search and mapping

[Lorenzo Pieralisi]

On Tue, May 11, 2021 at 04:54:08PM +0200, Lucas Stach wrote:
> Am Dienstag, dem 11.05.2021 um 09:21 -0500 schrieb Rob Herring:
> > On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> > > 
> > > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring:
> > > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote:
> > > > > We don't need to have a phandle of the PHY, as we know the compatible
> > > > > of the node we are looking for. This will make it easier to put add
> > > > > more PHY handling for new generations later on, where the
> > > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer.
> > > > > 
> > > > > Also we can use a helper function to get the resource for us,
> > > > > simplifying out driver code a bit.
> > > > 
> > > > Better yes, but really all the phy handling should be split out to
> > > > its own driver even in the older h/w with shared phy registers.
> > > > 
> > > That would be a quite massive DT binding changing break, possibly even
> > > a separate driver. Maybe it's time to do this for i.MX8MM, as the
> > > current driver just kept piling on special cases for "almost the same"
> > > hardware that by now looks quite different to the original i.MX6 PCIe
> > > integration this driver was supposed to handle.
> > 
> > No, you don't need to change DT, and a DT change adding a phy node
> > wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a
> > separate PHY driver would have to create its own platform device in
> > its initcall (if the iMX6 PCI compatible is found). Then the PCI
> > driver would need to use a non-DT based phy_get() lookup. For the
> > cases with a phandle to the phy, I'd assume a phy driver could be
> > instantiated for that node. You'll again need a non-DT phy_get() if
> > not using the phy binding.
> 
> The original i.MX6 PCIe with the internal PHY is the easy case, as you
> laid out above.
> 
> What I'm more concerned about is the i.MX7 and i.MX8MQ, where we have a
> MMIO mapped PHY and quite a bit of the clocks/reset/GPR handling would
> need to move from the controller to the PHY driver. Without a binding
> change I fear that we end up in a worst of both worlds situation, where
> we have lots of code in the driver to separate resources that are
> currently all attached to the PCIe controller node in the DT, without a
> real gain in making the driver any simpler or easier to maintain.
> 
> But right now that's all speculation. Maybe I need to type something up
> and see where it falls on the shiny/horrible scale.

Hi Lucas,

given the feedback I will mark this series as "Changes requested"
waiting with what follows, please let me know if that's what you
expected.

Thanks,
Lorenzo