Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] PCIE support for i.MX8MQ
@ 2018-11-17 18:12 andrew.smirnov
  2018-11-17 18:12 ` [PATCH 1/3] PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D andrew.smirnov
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-17 18:12 UTC (permalink / raw)
  To: linux-arm-kernel


Everyone:

This series contains changes I made in order to enable support of PCIE
IP block on i.MX8MQ SoCs (full tree can be found at [github-v0]). This series
is _very_ preliminary and by no means is ready for inclusion (it also
has some unmet dependencies).  However is should be in OK enough shape
to get some early feedback on, which is the intent of this submission.

Specifically, I'd like to get some feedback on whether newly
introduced "fsl,iomux-gpr1x" and "fsl,gpr12-device-type" DT
properties, added to handle differences between PCIE0 and PCIE1, is a
good (acceptable) solution for the problem.

All other feedback is appreciated as well!

Thank you,
Andrey Smirnov

[github-v0] https://github.com/ndreys/linux/commits/imx8mq-pcie-v0

Andrey Smirnov (3):
  PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D
  PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
  PCI: imx: Add support for i.MX8MQ

 drivers/pci/controller/dwc/Kconfig    |   2 +-
 drivers/pci/controller/dwc/pci-imx6.c | 119 +++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 4 deletions(-)

-- 
2.19.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/3] PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D
  2018-11-17 18:12 [PATCH 0/3] PCIE support for i.MX8MQ andrew.smirnov
@ 2018-11-17 18:12 ` andrew.smirnov
  2018-11-17 18:12 ` [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() " andrew.smirnov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-17 18:12 UTC (permalink / raw)
  To: linux-arm-kernel

PCIE PHY IP block on i.MX7D differs from the one used on i.MX6 family,
so none of the code in current implementation of imx6_setup_phy_mpll()
is applicable.

Cc: bhelgaas at google.com
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: cphealy at gmail.com
Cc: l.stach at pengutronix.de
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: linux-imx at nxp.com
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
Cc: linux-pci at vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 2cbef2d7c207..c140f7987598 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -525,6 +525,9 @@ static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
 	int mult, div;
 	u32 val;
 
+	if (imx6_pcie->variant == IMX7D)
+		return 0;
+
 	switch (phy_rate) {
 	case 125000000:
 		/*
-- 
2.19.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
  2018-11-17 18:12 [PATCH 0/3] PCIE support for i.MX8MQ andrew.smirnov
  2018-11-17 18:12 ` [PATCH 1/3] PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D andrew.smirnov
@ 2018-11-17 18:12 ` " andrew.smirnov
  2018-11-20  1:22   ` tpiepho
  2018-11-17 18:12 ` [PATCH 3/3] PCI: imx: Add support for i.MX8MQ andrew.smirnov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: andrew.smirnov @ 2018-11-17 18:12 UTC (permalink / raw)
  To: linux-arm-kernel

PCIE PHY IP block on i.MX7D differs from the one used on i.MX6 family,
so none of the code in current implementation of imx6_pcie_reset_phy()
is applicable.

Cc: bhelgaas at google.com
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: cphealy at gmail.com
Cc: l.stach at pengutronix.de
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: linux-imx at nxp.com
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
Cc: linux-pci at vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index c140f7987598..3c3002861d25 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -245,6 +245,9 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
 {
 	u32 tmp;
 
+	if (imx6_pcie->variant == IMX7D)
+		return;
+
 	pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, &tmp);
 	tmp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN |
 		PHY_RX_OVRD_IN_LO_RX_PLL_EN);
-- 
2.19.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-17 18:12 [PATCH 0/3] PCIE support for i.MX8MQ andrew.smirnov
  2018-11-17 18:12 ` [PATCH 1/3] PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D andrew.smirnov
  2018-11-17 18:12 ` [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() " andrew.smirnov
@ 2018-11-17 18:12 ` andrew.smirnov
  2018-11-19  7:07   ` hongxing.zhu
  2018-11-20 10:48   ` leonard.crestez
  2018-12-03 12:20 ` [PATCH 0/3] PCIE " Lorenzo Pieralisi
  2018-12-07 11:37 ` Lorenzo Pieralisi
  4 siblings, 2 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-17 18:12 UTC (permalink / raw)
  To: linux-arm-kernel

Cc: bhelgaas at google.com
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: cphealy at gmail.com
Cc: l.stach at pengutronix.de
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: linux-imx at nxp.com
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
Cc: linux-pci at vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/pci/controller/dwc/Kconfig    |   2 +-
 drivers/pci/controller/dwc/pci-imx6.c | 117 ++++++++++++++++++++++++--
 2 files changed, 113 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 91b0194240a5..2b139acccf32 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -90,7 +90,7 @@ config PCI_EXYNOS
 
 config PCI_IMX6
 	bool "Freescale i.MX6 PCIe controller"
-	depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
+	depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
 	depends on PCI_MSI_IRQ_DOMAIN
 	select PCIE_DW_HOST
 
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 3c3002861d25..8d1f310e41a6 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -8,6 +8,7 @@
  * Author: Sean Cross <xobs@kosagi.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
@@ -30,6 +31,14 @@
 
 #include "pcie-designware.h"
 
+#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET		0x7C
+#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US		(0x6 << 15)
+
+#define IMX8MQ_GPR_PCIE_REF_USE_PAD		BIT(9)
+#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN	BIT(10)
+#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE	BIT(11)
+
+
 #define to_imx6_pcie(x)	dev_get_drvdata((x)->dev)
 
 enum imx6_pcie_variants {
@@ -37,6 +46,7 @@ enum imx6_pcie_variants {
 	IMX6SX,
 	IMX6QP,
 	IMX7D,
+	IMX8MQ,
 };
 
 struct imx6_pcie {
@@ -48,8 +58,10 @@ struct imx6_pcie {
 	struct clk		*pcie_inbound_axi;
 	struct clk		*pcie;
 	struct regmap		*iomuxc_gpr;
+	u32			gpr1x;
 	struct reset_control	*pciephy_reset;
 	struct reset_control	*apps_reset;
+	struct reset_control	*apps_clk_req;
 	struct reset_control	*turnoff_reset;
 	enum imx6_pcie_variants variant;
 	u32			tx_deemph_gen1;
@@ -59,6 +71,7 @@ struct imx6_pcie {
 	u32			tx_swing_low;
 	int			link_gen;
 	struct regulator	*vpcie;
+	u32			device_type[2];
 };
 
 /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
@@ -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
 {
 	u32 tmp;
 
-	if (imx6_pcie->variant == IMX7D)
+	if (imx6_pcie->variant == IMX7D ||
+	    imx6_pcie->variant == IMX8MQ)
 		return;
 
 	pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, &tmp);
@@ -261,6 +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
 	pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp);
 }
 
+#ifdef CONFIG_ARM
 /*  Added for PCI abort handling */
 static int imx6q_pcie_abort_handler(unsigned long addr,
 		unsigned int fsr, struct pt_regs *regs)
@@ -294,6 +309,7 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
 
 	return 1;
 }
+#endif
 
 static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
@@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 
 	switch (imx6_pcie->variant) {
 	case IMX7D:
+	case IMX8MQ: /* FALLTHROUGH */
 		reset_control_assert(imx6_pcie->pciephy_reset);
 		reset_control_assert(imx6_pcie->apps_reset);
 		break;
@@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 		break;
 	case IMX7D:
 		break;
+	case IMX8MQ:
+		/*
+		 * Set the over ride low and enabled
+		 * make sure that REF_CLK is turned on.
+		 */
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
+				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
+				   0);
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
+				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
+				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
+		break;
 	}
 
 	return ret;
@@ -397,6 +426,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct dw_pcie *pci = imx6_pcie->pci;
 	struct device *dev = pci->dev;
+	unsigned int val;
 	int ret;
 
 	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) {
@@ -445,6 +475,29 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	}
 
 	switch (imx6_pcie->variant) {
+	case IMX8MQ:
+		reset_control_deassert(imx6_pcie->pciephy_reset);
+		udelay(100);
+		/*
+		 * Configure the CLK_REQ# high, let the L1SS
+		 * automatically controlled by HW.
+		 */
+		reset_control_assert(imx6_pcie->apps_clk_req);
+
+		/*
+		 * Configure the L1 latency of rc to less than 64us
+		 * Otherwise, the L1/L1SUB wouldn't be enable by ASPM.
+		 */
+		val = dw_pcie_readl_dbi(imx6_pcie->pci,
+					SZ_1M +
+					IMX8MQ_PCIE_LINK_CAP_REG_OFFSET);
+		val &= ~PCI_EXP_LNKCAP_L1EL;
+		val |= IMX8MQ_PCIE_LINK_CAP_L1EL_64US;
+		dw_pcie_writel_dbi(imx6_pcie->pci,
+				   SZ_1M +
+				   IMX8MQ_PCIE_LINK_CAP_REG_OFFSET,
+				   val);
+		break;
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
@@ -483,6 +536,15 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 {
 	switch (imx6_pcie->variant) {
+	case IMX8MQ:
+		/*
+		 * TODO: Currently this code assumes external
+		 * oscillator is being used
+		 */
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
+				   IMX8MQ_GPR_PCIE_REF_USE_PAD,
+				   IMX8MQ_GPR_PCIE_REF_USE_PAD);
+		break;
 	case IMX7D:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
@@ -519,7 +581,8 @@ static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 	}
 
 	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
-			IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
+			   imx6_pcie->device_type[0],
+			   imx6_pcie->device_type[1]);
 }
 
 static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
@@ -528,7 +591,8 @@ static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
 	int mult, div;
 	u32 val;
 
-	if (imx6_pcie->variant == IMX7D)
+	if (imx6_pcie->variant == IMX7D ||
+	    imx6_pcie->variant == IMX8MQ)
 		return 0;
 
 	switch (phy_rate) {
@@ -616,6 +680,7 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
 				   IMX6Q_GPR12_PCIE_CTL_2);
 		break;
 	case IMX7D:
+	case IMX8MQ:		/* FALLTHROUGH */
 		reset_control_deassert(imx6_pcie->apps_reset);
 		break;
 	}
@@ -798,10 +863,24 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	clk_disable_unprepare(imx6_pcie->pcie_phy);
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
 
-	if (imx6_pcie->variant == IMX7D) {
+	switch (imx6_pcie->variant) {
+	case IMX7D:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+		break;
+	/*
+	 * Disable the override. Configure the CLK_REQ# high, let the
+	 * L1SS automatically controlled by HW when link is up.
+	 * Otherwise, turn off the REF_CLK to save power consumption.
+	 */
+	case IMX8MQ:
+		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
+				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
+				   0);
+		break;
+	default:
+		break;
 	}
 }
 
@@ -870,6 +949,10 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	imx6_pcie->variant =
 		(enum imx6_pcie_variants)of_device_get_match_data(dev);
 
+	imx6_pcie->device_type[0] = IMX6Q_GPR12_DEVICE_TYPE;
+	imx6_pcie->device_type[1] = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
+					       PCI_EXP_TYPE_ROOT_PORT);
+
 	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
 	if (IS_ERR(pci->dbi_base))
@@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 			return PTR_ERR(imx6_pcie->pcie_inbound_axi);
 		}
 		break;
-	case IMX7D:
+	case IMX8MQ:
+		if (of_property_read_u32(node, "fsl,iomux-gpr1x",
+					 &imx6_pcie->gpr1x)) {
+			dev_err(dev, "Failed to get GPR1x address\n");
+			return -EINVAL;
+		}
+
+		if (of_property_read_u32_array(
+			    node, "fsl,gpr12-device-type",
+			    imx6_pcie->device_type,
+			    ARRAY_SIZE(imx6_pcie->device_type))) {
+			dev_err(dev, "Failed to get device type mask/value\n");
+			return -EINVAL;
+		}
+
+		imx6_pcie->apps_clk_req =
+			devm_reset_control_get_exclusive(dev, "clkreq");
+		if (IS_ERR(imx6_pcie->apps_clk_req)) {
+			dev_err(dev, "Failed to get PCIE APPS CLK_REQ# reset control\n");
+			return PTR_ERR(imx6_pcie->apps_clk_req);
+		}
+	case IMX7D:		/* FALLTHROUGH */
 		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
 									    "pciephy");
 		if (IS_ERR(imx6_pcie->pciephy_reset)) {
@@ -1011,6 +1115,7 @@ static const struct of_device_id imx6_pcie_of_match[] = {
 	{ .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
 	{ .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
 	{ .compatible = "fsl,imx7d-pcie",  .data = (void *)IMX7D,  },
+	{ .compatible = "fsl,imx8mq-pcie", .data = (void *)IMX8MQ, } ,
 	{},
 };
 
@@ -1027,6 +1132,7 @@ static struct platform_driver imx6_pcie_driver = {
 
 static int __init imx6_pcie_init(void)
 {
+#ifdef CONFIG_ARM
 	/*
 	 * Since probe() can be deferred we need to make sure that
 	 * hook_fault_code is not called after __init memory is freed
@@ -1036,6 +1142,7 @@ static int __init imx6_pcie_init(void)
 	 */
 	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
 			"external abort on non-linefetch");
+#endif
 
 	return platform_driver_register(&imx6_pcie_driver);
 }
-- 
2.19.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-17 18:12 ` [PATCH 3/3] PCI: imx: Add support for i.MX8MQ andrew.smirnov
@ 2018-11-19  7:07   ` hongxing.zhu
  2018-11-26 18:09     ` andrew.smirnov
  2018-11-20 10:48   ` leonard.crestez
  1 sibling, 1 reply; 21+ messages in thread
From: hongxing.zhu @ 2018-11-19  7:07 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Andrey:
Thanks for your patch-set.
I have comment about the L1SS implementation below.
It's better to figure out one method to fix it.

BR
Richard

> -----Original Message-----
> From: Andrey Smirnov [mailto:andrew.smirnov at gmail.com]
> Sent: 2018?11?18? 2:12
> To: linux-kernel at vger.kernel.org
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>; bhelgaas at google.com;
> Fabio Estevam <fabio.estevam@nxp.com>; cphealy at gmail.com;
> l.stach at pengutronix.de; Leonard Crestez <leonard.crestez@nxp.com>;
> Aisheng DONG <aisheng.dong@nxp.com>; Richard Zhu
> <hongxing.zhu@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel at lists.infradead.org; linux-pci at vger.kernel.org
> Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> 
> Cc: bhelgaas at google.com
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: cphealy at gmail.com
> Cc: l.stach at pengutronix.de
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: linux-imx at nxp.com
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-pci at vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/pci/controller/dwc/Kconfig    |   2 +-
>  drivers/pci/controller/dwc/pci-imx6.c | 117 ++++++++++++++++++++++++--
>  2 files changed, 113 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig
> b/drivers/pci/controller/dwc/Kconfig
> index 91b0194240a5..2b139acccf32 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -90,7 +90,7 @@ config PCI_EXYNOS
> 
>  config PCI_IMX6
>  	bool "Freescale i.MX6 PCIe controller"
> -	depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> +	depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
>  	depends on PCI_MSI_IRQ_DOMAIN
>  	select PCIE_DW_HOST
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 3c3002861d25..8d1f310e41a6 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -8,6 +8,7 @@
>   * Author: Sean Cross <xobs@kosagi.com>
>   */
> 
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
> @@ -30,6 +31,14 @@
> 
>  #include "pcie-designware.h"
> 
> +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET		0x7C
> +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US		(0x6 << 15)
> +
> +#define IMX8MQ_GPR_PCIE_REF_USE_PAD		BIT(9)
> +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN	BIT(10)
> +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE	BIT(11)
> +
> +
>  #define to_imx6_pcie(x)	dev_get_drvdata((x)->dev)
> 
>  enum imx6_pcie_variants {
> @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
>  	IMX6SX,
>  	IMX6QP,
>  	IMX7D,
> +	IMX8MQ,
>  };
> 
>  struct imx6_pcie {
> @@ -48,8 +58,10 @@ struct imx6_pcie {
>  	struct clk		*pcie_inbound_axi;
>  	struct clk		*pcie;
>  	struct regmap		*iomuxc_gpr;
> +	u32			gpr1x;
>  	struct reset_control	*pciephy_reset;
>  	struct reset_control	*apps_reset;
> +	struct reset_control	*apps_clk_req;
>  	struct reset_control	*turnoff_reset;
>  	enum imx6_pcie_variants variant;
>  	u32			tx_deemph_gen1;
> @@ -59,6 +71,7 @@ struct imx6_pcie {
>  	u32			tx_swing_low;
>  	int			link_gen;
>  	struct regulator	*vpcie;
> +	u32			device_type[2];
>  };
> 
>  /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@
> -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> *imx6_pcie)  {
>  	u32 tmp;
> 
> -	if (imx6_pcie->variant == IMX7D)
> +	if (imx6_pcie->variant == IMX7D ||
> +	    imx6_pcie->variant == IMX8MQ)
>  		return;
> 
>  	pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, &tmp); @@ -261,6
> +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
>  	pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp);  }
> 
> +#ifdef CONFIG_ARM
>  /*  Added for PCI abort handling */
>  static int imx6q_pcie_abort_handler(unsigned long addr,
>  		unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@ static
> int imx6q_pcie_abort_handler(unsigned long addr,
> 
>  	return 1;
>  }
> +#endif
> 
>  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
> 
>  	switch (imx6_pcie->variant) {
>  	case IMX7D:
> +	case IMX8MQ: /* FALLTHROUGH */
>  		reset_control_assert(imx6_pcie->pciephy_reset);
>  		reset_control_assert(imx6_pcie->apps_reset);
>  		break;
> @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> imx6_pcie *imx6_pcie)
>  		break;
>  	case IMX7D:
>  		break;
> +	case IMX8MQ:
> +		/*
> +		 * Set the over ride low and enabled
> +		 * make sure that REF_CLK is turned on.
> +		 */
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> +				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
> +				   0);
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> +				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> +				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> +		break;
>  	}
> 
>  	return ret;
> @@ -397,6 +426,7 @@ static void imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)  {
>  	struct dw_pcie *pci = imx6_pcie->pci;
>  	struct device *dev = pci->dev;
> +	unsigned int val;
>  	int ret;
> 
>  	if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) { @@
> -445,6 +475,29 @@ static void imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)
>  	}
> 
>  	switch (imx6_pcie->variant) {
> +	case IMX8MQ:
> +		reset_control_deassert(imx6_pcie->pciephy_reset);
> +		udelay(100);
> +		/*
> +		 * Configure the CLK_REQ# high, let the L1SS
> +		 * automatically controlled by HW.
> +		 */
> +		reset_control_assert(imx6_pcie->apps_clk_req);
> +
> +		/*
> +		 * Configure the L1 latency of rc to less than 64us
> +		 * Otherwise, the L1/L1SUB wouldn't be enable by ASPM.
> +		 */
> +		val = dw_pcie_readl_dbi(imx6_pcie->pci,
> +					SZ_1M +
> +					IMX8MQ_PCIE_LINK_CAP_REG_OFFSET);
> +		val &= ~PCI_EXP_LNKCAP_L1EL;
> +		val |= IMX8MQ_PCIE_LINK_CAP_L1EL_64US;
> +		dw_pcie_writel_dbi(imx6_pcie->pci,
> +				   SZ_1M +
> +				   IMX8MQ_PCIE_LINK_CAP_REG_OFFSET,
> +				   val);
> +		break;
>  	case IMX7D:
>  		reset_control_deassert(imx6_pcie->pciephy_reset);
>  		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> @@ -483,6 +536,15 @@ static void imx6_pcie_deassert_core_reset(struct
> imx6_pcie *imx6_pcie)  static void imx6_pcie_init_phy(struct imx6_pcie
> *imx6_pcie)  {
>  	switch (imx6_pcie->variant) {
> +	case IMX8MQ:
> +		/*
> +		 * TODO: Currently this code assumes external
> +		 * oscillator is being used
> +		 */
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> +				   IMX8MQ_GPR_PCIE_REF_USE_PAD,
> +				   IMX8MQ_GPR_PCIE_REF_USE_PAD);
> +		break;
>  	case IMX7D:
>  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
>  				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0); @@
> -519,7 +581,8 @@ static void imx6_pcie_init_phy(struct imx6_pcie
> *imx6_pcie)
>  	}
> 
>  	regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> -			IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT <<
> 12);
> +			   imx6_pcie->device_type[0],
> +			   imx6_pcie->device_type[1]);
>  }
> 
>  static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie) @@ -528,7
> +591,8 @@ static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
>  	int mult, div;
>  	u32 val;
> 
> -	if (imx6_pcie->variant == IMX7D)
> +	if (imx6_pcie->variant == IMX7D ||
> +	    imx6_pcie->variant == IMX8MQ)
>  		return 0;
> 
>  	switch (phy_rate) {
> @@ -616,6 +680,7 @@ static void imx6_pcie_ltssm_enable(struct device
> *dev)
>  				   IMX6Q_GPR12_PCIE_CTL_2);
>  		break;
>  	case IMX7D:
> +	case IMX8MQ:		/* FALLTHROUGH */
>  		reset_control_deassert(imx6_pcie->apps_reset);
>  		break;
>  	}
> @@ -798,10 +863,24 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> *imx6_pcie)
>  	clk_disable_unprepare(imx6_pcie->pcie_phy);
>  	clk_disable_unprepare(imx6_pcie->pcie_bus);
> 
> -	if (imx6_pcie->variant == IMX7D) {
> +	switch (imx6_pcie->variant) {
> +	case IMX7D:
>  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
>  				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
>  				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> +		break;
> +	/*
> +	 * Disable the override. Configure the CLK_REQ# high, let the
> +	 * L1SS automatically controlled by HW when link is up.
> +	 * Otherwise, turn off the REF_CLK to save power consumption.
> +	 */
[Richard Zhu] About the L1SS support, there is a back-compatible issue.
When one none-L1SS capability EP device is used in one HW design solution.
In this case, EP device wouldn't manipulated its own CLK_REQ#.
The CLK_REQ# would be high when the override_en is disabled here.
That means the REFCLK would be turned off abnormally.
System would have problem when the REFCLK of PCIe is turned off abnormally after link is up.


> +	case IMX8MQ:
> +		regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> +				   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> +				   0);
> +		break;
> +	default:
> +		break;
>  	}
>  }
> 
> @@ -870,6 +949,10 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
>  	imx6_pcie->variant =
>  		(enum imx6_pcie_variants)of_device_get_match_data(dev);
> 
> +	imx6_pcie->device_type[0] = IMX6Q_GPR12_DEVICE_TYPE;
> +	imx6_pcie->device_type[1] = FIELD_PREP(IMX6Q_GPR12_DEVICE_TYPE,
> +					       PCI_EXP_TYPE_ROOT_PORT);
> +
>  	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
>  	if (IS_ERR(pci->dbi_base))
> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
>  			return PTR_ERR(imx6_pcie->pcie_inbound_axi);
>  		}
>  		break;
> -	case IMX7D:
> +	case IMX8MQ:
> +		if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> +					 &imx6_pcie->gpr1x)) {
> +			dev_err(dev, "Failed to get GPR1x address\n");
> +			return -EINVAL;
> +		}
> +
> +		if (of_property_read_u32_array(
> +			    node, "fsl,gpr12-device-type",
> +			    imx6_pcie->device_type,
> +			    ARRAY_SIZE(imx6_pcie->device_type))) {
> +			dev_err(dev, "Failed to get device type mask/value\n");
> +			return -EINVAL;
> +		}
> +
> +		imx6_pcie->apps_clk_req =
> +			devm_reset_control_get_exclusive(dev, "clkreq");
> +		if (IS_ERR(imx6_pcie->apps_clk_req)) {
> +			dev_err(dev, "Failed to get PCIE APPS CLK_REQ# reset
> control\n");
> +			return PTR_ERR(imx6_pcie->apps_clk_req);
> +		}
> +	case IMX7D:		/* FALLTHROUGH */
>  		imx6_pcie->pciephy_reset =
> devm_reset_control_get_exclusive(dev,
>  									    "pciephy");
>  		if (IS_ERR(imx6_pcie->pciephy_reset)) { @@ -1011,6 +1115,7 @@
> static const struct of_device_id imx6_pcie_of_match[] = {
>  	{ .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
>  	{ .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
>  	{ .compatible = "fsl,imx7d-pcie",  .data = (void *)IMX7D,  },
> +	{ .compatible = "fsl,imx8mq-pcie", .data = (void *)IMX8MQ, } ,
>  	{},
>  };
> 
> @@ -1027,6 +1132,7 @@ static struct platform_driver imx6_pcie_driver = {
> 
>  static int __init imx6_pcie_init(void)
>  {
> +#ifdef CONFIG_ARM
>  	/*
>  	 * Since probe() can be deferred we need to make sure that
>  	 * hook_fault_code is not called after __init memory is freed @@
> -1036,6 +1142,7 @@ static int __init imx6_pcie_init(void)
>  	 */
>  	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
>  			"external abort on non-linefetch");
> +#endif
> 
>  	return platform_driver_register(&imx6_pcie_driver);
>  }
> --
> 2.19.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
  2018-11-17 18:12 ` [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() " andrew.smirnov
@ 2018-11-20  1:22   ` tpiepho
  2018-11-26 18:32     ` andrew.smirnov
  0 siblings, 1 reply; 21+ messages in thread
From: tpiepho @ 2018-11-20  1:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> PCIE PHY IP block on i.MX7D differs from the one used on i.MX6 family,
> so none of the code in current implementation of imx6_pcie_reset_phy()
> is applicable.

Tested on IMX7d, still appears to work.

Note that your patches will collide with Stefan Agner's patch, "PCI:
imx6: limit DBI register length", which was recently posted.

He changed the way the variants are handled.  That method would allow
some of the IMX7D || IMX8MQ checks to be re-written as

 imx6_pcie->drvdata->boolean_attribute

Where the attribute can be set in a table and be re-used in every place
it comes into play and updated for new devices in one spot, instead of
keeping piles of this version or that version or this other version
checks up to date.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-17 18:12 ` [PATCH 3/3] PCI: imx: Add support for i.MX8MQ andrew.smirnov
  2018-11-19  7:07   ` hongxing.zhu
@ 2018-11-20 10:48   ` leonard.crestez
  2018-11-26 18:24     ` andrew.smirnov
  1 sibling, 1 reply; 21+ messages in thread
From: leonard.crestez @ 2018-11-20 10:48 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> -	case IMX7D:
> +	case IMX8MQ:
> +		if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> +					 &imx6_pcie->gpr1x)) {
> +			dev_err(dev, "Failed to get GPR1x address\n");
> +			return -EINVAL;
> +		}

This is for distinguishing multiple controllers on the SOC but other
registers and bits might differ. Isn't it preferable to have a property
for controller id instead of adding many registers to DT?

> +
> +		if (of_property_read_u32_array(
> +			    node, "fsl,gpr12-device-type",
> +			    imx6_pcie->device_type,
> +			    ARRAY_SIZE(imx6_pcie->device_type))) {
> +			dev_err(dev, "Failed to get device type
> mask/value\n");
> +			return -EINVAL;
> +		}

The device type can be set on multiple SOCs, why are you adding a
mandatory property only for 8m?

There should probably be a separate patch with documented DT bindings.

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-19  7:07   ` hongxing.zhu
@ 2018-11-26 18:09     ` andrew.smirnov
  2018-11-29 10:52       ` hongxing.zhu
  0 siblings, 1 reply; 21+ messages in thread
From: andrew.smirnov @ 2018-11-26 18:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu <hongxing.zhu@nxp.com> wrote:
>
> Hi Andrey:
> Thanks for your patch-set.
> I have comment about the L1SS implementation below.
> It's better to figure out one method to fix it.
>
> BR
> Richard
>
> > -----Original Message-----
> > From: Andrey Smirnov [mailto:andrew.smirnov at gmail.com]
> > Sent: 2018?11?18? 2:12
> > To: linux-kernel at vger.kernel.org
> > Cc: Andrey Smirnov <andrew.smirnov@gmail.com>; bhelgaas at google.com;
> > Fabio Estevam <fabio.estevam@nxp.com>; cphealy at gmail.com;
> > l.stach at pengutronix.de; Leonard Crestez <leonard.crestez@nxp.com>;
> > Aisheng DONG <aisheng.dong@nxp.com>; Richard Zhu
> > <hongxing.zhu@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> > linux-arm-kernel at lists.infradead.org; linux-pci at vger.kernel.org
> > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> >
> > Cc: bhelgaas at google.com
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: cphealy at gmail.com
> > Cc: l.stach at pengutronix.de
> > Cc: Leonard Crestez <leonard.crestez@nxp.com>
> > Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: linux-imx at nxp.com
> > Cc: linux-arm-kernel at lists.infradead.org
> > Cc: linux-kernel at vger.kernel.org
> > Cc: linux-pci at vger.kernel.org
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig    |   2 +-
> >  drivers/pci/controller/dwc/pci-imx6.c | 117 ++++++++++++++++++++++++--
> >  2 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index 91b0194240a5..2b139acccf32 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -90,7 +90,7 @@ config PCI_EXYNOS
> >
> >  config PCI_IMX6
> >       bool "Freescale i.MX6 PCIe controller"
> > -     depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> > +     depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
> >       depends on PCI_MSI_IRQ_DOMAIN
> >       select PCIE_DW_HOST
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3c3002861d25..8d1f310e41a6 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -8,6 +8,7 @@
> >   * Author: Sean Cross <xobs@kosagi.com>
> >   */
> >
> > +#include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/gpio.h>
> > @@ -30,6 +31,14 @@
> >
> >  #include "pcie-designware.h"
> >
> > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET              0x7C
> > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US               (0x6 << 15)
> > +
> > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD          BIT(9)
> > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN  BIT(10)
> > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE     BIT(11)
> > +
> > +
> >  #define to_imx6_pcie(x)      dev_get_drvdata((x)->dev)
> >
> >  enum imx6_pcie_variants {
> > @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> >       IMX6SX,
> >       IMX6QP,
> >       IMX7D,
> > +     IMX8MQ,
> >  };
> >
> >  struct imx6_pcie {
> > @@ -48,8 +58,10 @@ struct imx6_pcie {
> >       struct clk              *pcie_inbound_axi;
> >       struct clk              *pcie;
> >       struct regmap           *iomuxc_gpr;
> > +     u32                     gpr1x;
> >       struct reset_control    *pciephy_reset;
> >       struct reset_control    *apps_reset;
> > +     struct reset_control    *apps_clk_req;
> >       struct reset_control    *turnoff_reset;
> >       enum imx6_pcie_variants variant;
> >       u32                     tx_deemph_gen1;
> > @@ -59,6 +71,7 @@ struct imx6_pcie {
> >       u32                     tx_swing_low;
> >       int                     link_gen;
> >       struct regulator        *vpcie;
> > +     u32                     device_type[2];
> >  };
> >
> >  /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@
> > -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> > *imx6_pcie)  {
> >       u32 tmp;
> >
> > -     if (imx6_pcie->variant == IMX7D)
> > +     if (imx6_pcie->variant == IMX7D ||
> > +         imx6_pcie->variant == IMX8MQ)
> >               return;
> >
> >       pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, &tmp); @@ -261,6
> > +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> >       pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp);  }
> >
> > +#ifdef CONFIG_ARM
> >  /*  Added for PCI abort handling */
> >  static int imx6q_pcie_abort_handler(unsigned long addr,
> >               unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@ static
> > int imx6q_pcie_abort_handler(unsigned long addr,
> >
> >       return 1;
> >  }
> > +#endif
> >
> >  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> >
> >       switch (imx6_pcie->variant) {
> >       case IMX7D:
> > +     case IMX8MQ: /* FALLTHROUGH */
> >               reset_control_assert(imx6_pcie->pciephy_reset);
> >               reset_control_assert(imx6_pcie->apps_reset);
> >               break;
> > @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> > imx6_pcie *imx6_pcie)
> >               break;
> >       case IMX7D:
> >               break;
> > +     case IMX8MQ:
> > +             /*
> > +              * Set the over ride low and enabled
> > +              * make sure that REF_CLK is turned on.
> > +              */
> > +             regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> > +                                IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
> > +                                0);
> > +             regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> > +                                IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> > +                                IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> > +             break;
> >       }
> >
> >       return ret;
> > @@ -397,6 +426,7 @@ static void imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)  {
> >       struct dw_pcie *pci = imx6_pcie->pci;
> >       struct device *dev = pci->dev;
> > +     unsigned int val;
> >       int ret;
> >
> >       if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) { @@
> > -445,6 +475,29 @@ static void imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> >       }
> >
> >       switch (imx6_pcie->variant) {
> > +     case IMX8MQ:
> > +             reset_control_deassert(imx6_pcie->pciephy_reset);
> > +             udelay(100);
> > +             /*
> > +              * Configure the CLK_REQ# high, let the L1SS
> > +              * automatically controlled by HW.
> > +              */
> > +             reset_control_assert(imx6_pcie->apps_clk_req);
> > +
> > +             /*
> > +              * Configure the L1 latency of rc to less than 64us
> > +              * Otherwise, the L1/L1SUB wouldn't be enable by ASPM.
> > +              */
> > +             val = dw_pcie_readl_dbi(imx6_pcie->pci,
> > +                                     SZ_1M +
> > +                                     IMX8MQ_PCIE_LINK_CAP_REG_OFFSET);
> > +             val &= ~PCI_EXP_LNKCAP_L1EL;
> > +             val |= IMX8MQ_PCIE_LINK_CAP_L1EL_64US;
> > +             dw_pcie_writel_dbi(imx6_pcie->pci,
> > +                                SZ_1M +
> > +                                IMX8MQ_PCIE_LINK_CAP_REG_OFFSET,
> > +                                val);
> > +             break;
> >       case IMX7D:
> >               reset_control_deassert(imx6_pcie->pciephy_reset);
> >               imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > @@ -483,6 +536,15 @@ static void imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)  static void imx6_pcie_init_phy(struct imx6_pcie
> > *imx6_pcie)  {
> >       switch (imx6_pcie->variant) {
> > +     case IMX8MQ:
> > +             /*
> > +              * TODO: Currently this code assumes external
> > +              * oscillator is being used
> > +              */
> > +             regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> > +                                IMX8MQ_GPR_PCIE_REF_USE_PAD,
> > +                                IMX8MQ_GPR_PCIE_REF_USE_PAD);
> > +             break;
> >       case IMX7D:
> >               regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> >                                  IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0); @@
> > -519,7 +581,8 @@ static void imx6_pcie_init_phy(struct imx6_pcie
> > *imx6_pcie)
> >       }
> >
> >       regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > -                     IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT <<
> > 12);
> > +                        imx6_pcie->device_type[0],
> > +                        imx6_pcie->device_type[1]);
> >  }
> >
> >  static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie) @@ -528,7
> > +591,8 @@ static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie)
> >       int mult, div;
> >       u32 val;
> >
> > -     if (imx6_pcie->variant == IMX7D)
> > +     if (imx6_pcie->variant == IMX7D ||
> > +         imx6_pcie->variant == IMX8MQ)
> >               return 0;
> >
> >       switch (phy_rate) {
> > @@ -616,6 +680,7 @@ static void imx6_pcie_ltssm_enable(struct device
> > *dev)
> >                                  IMX6Q_GPR12_PCIE_CTL_2);
> >               break;
> >       case IMX7D:
> > +     case IMX8MQ:            /* FALLTHROUGH */
> >               reset_control_deassert(imx6_pcie->apps_reset);
> >               break;
> >       }
> > @@ -798,10 +863,24 @@ static void imx6_pcie_clk_disable(struct imx6_pcie
> > *imx6_pcie)
> >       clk_disable_unprepare(imx6_pcie->pcie_phy);
> >       clk_disable_unprepare(imx6_pcie->pcie_bus);
> >
> > -     if (imx6_pcie->variant == IMX7D) {
> > +     switch (imx6_pcie->variant) {
> > +     case IMX7D:
> >               regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> >                                  IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> >                                  IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > +             break;
> > +     /*
> > +      * Disable the override. Configure the CLK_REQ# high, let the
> > +      * L1SS automatically controlled by HW when link is up.
> > +      * Otherwise, turn off the REF_CLK to save power consumption.
> > +      */
> [Richard Zhu] About the L1SS support, there is a back-compatible issue.
> When one none-L1SS capability EP device is used in one HW design solution.
> In this case, EP device wouldn't manipulated its own CLK_REQ#.
> The CLK_REQ# would be high when the override_en is disabled here.
> That means the REFCLK would be turned off abnormally.
> System would have problem when the REFCLK of PCIe is turned off abnormally after link is up.
>

I don't have a lot of expertise in this area, so please take my
comment with a grain of salt. The way I understand it, is in legacy
systems that do not support L1SS with CLKREQ that feature shouldn't be
enabled/used. Unless I've missed something, I think it should be
possible to do so and disable the feature by configuring corresponding
CLKREQ_B pad as GPIO and adding a GPIO hog node to pull CLKREQ low. Do
you see any problems with this approach?

Regardless though, I wasn't really planning on including PM support in
this series. The code in question shouldn't even be in the patch since
it'll never be executed because imx6_pcie_clk_disable() is only called
by imx6_pcie_suspend_noirq() which is a no-op on all variants except
i.MX7D.

I'll drop all of the unused PM-related bits I missed from the series
and we can add them later in a separate submission.

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-20 10:48   ` leonard.crestez
@ 2018-11-26 18:24     ` andrew.smirnov
  2018-11-27 10:06       ` l.stach
  2018-11-27 10:16       ` leonard.crestez
  0 siblings, 2 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-26 18:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > -     case IMX7D:
> > +     case IMX8MQ:
> > +             if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > +                                      &imx6_pcie->gpr1x)) {
> > +                     dev_err(dev, "Failed to get GPR1x address\n");
> > +                     return -EINVAL;
> > +             }
>
> This is for distinguishing multiple controllers on the SOC but other
> registers and bits might differ. Isn't it preferable to have a property
> for controller id instead of adding many registers to DT?
>

I liked encoding necessary info in DT directly slightly better than
encoding abstract ID and then decoding it further in the driver code.
OTOH, I am not really attached to that path. Lucas, can you comment on
this please?

> > +
> > +             if (of_property_read_u32_array(
> > +                         node, "fsl,gpr12-device-type",
> > +                         imx6_pcie->device_type,
> > +                         ARRAY_SIZE(imx6_pcie->device_type))) {
> > +                     dev_err(dev, "Failed to get device type
> > mask/value\n");
> > +                     return -EINVAL;
> > +             }
>
> The device type can be set on multiple SOCs, why are you adding a
> mandatory property only for 8m?

My thinking was that other SoCs don't really have two controllers, so
they don't really need to have that property, but, more importantly,
not forcing them to have this property should preserve backwards
compatibility with old DTBs.

>
> There should probably be a separate patch with documented DT bindings.
>

Yes, definitely, I just wanted to come up with a set of bindings
agreed on by the driver maintainers first.

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
  2018-11-20  1:22   ` tpiepho
@ 2018-11-26 18:32     ` andrew.smirnov
  0 siblings, 0 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-26 18:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 19, 2018 at 5:22 PM Trent Piepho <tpiepho@impinj.com> wrote:
>
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > PCIE PHY IP block on i.MX7D differs from the one used on i.MX6 family,
> > so none of the code in current implementation of imx6_pcie_reset_phy()
> > is applicable.
>
> Tested on IMX7d, still appears to work.
>

Thanks for testing! Unless you object, I'll add your Tested-by tag to
this patch.

> Note that your patches will collide with Stefan Agner's patch, "PCI:
> imx6: limit DBI register length", which was recently posted.
>
> He changed the way the variants are handled.  That method would allow
> some of the IMX7D || IMX8MQ checks to be re-written as
>
>  imx6_pcie->drvdata->boolean_attribute
>
> Where the attribute can be set in a table and be re-used in every place
> it comes into play and updated for new devices in one spot, instead of
> keeping piles of this version or that version or this other version
> checks up to date.

Thanks for the heads up! I am expecting that I'd have to re-base this
series on "next" in PCI tree before it can be applied. This should
provide for a good opportunity to discover and resolve all of the
conflicts, I think.

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-26 18:24     ` andrew.smirnov
@ 2018-11-27 10:06       ` l.stach
  2018-11-27 10:46         ` leonard.crestez
  2018-11-27 10:16       ` leonard.crestez
  1 sibling, 1 reply; 21+ messages in thread
From: l.stach @ 2018-11-27 10:06 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Andrey,

Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > 
> > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > -?????case IMX7D:
> > > +?????case IMX8MQ:
> > > +?????????????if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > +??????????????????????????????????????&imx6_pcie->gpr1x)) {
> > > +?????????????????????dev_err(dev, "Failed to get GPR1x address\n");
> > > +?????????????????????return -EINVAL;
> > > +?????????????}
> > 
> > This is for distinguishing multiple controllers on the SOC but other
> > registers and bits might differ. Isn't it preferable to have a property
> > for controller id instead of adding many registers to DT?
> > 
> 
> I liked encoding necessary info in DT directly slightly better than
> encoding abstract ID and then decoding it further in the driver code.
> OTOH, I am not really attached to that path. Lucas, can you comment on
> this please?

Yes, after rereading the patch with this in mind I agree that having
the GPR offset on DT directly is IMO the better approach than an
abstract ID.

One other thing I noticed is that we probably need some property to
encode if the clock is supplied by an external clkgen, or if the clock
is provided by the i.MX. Hardcoding this in the driver will lead to DT
backward compatibility headaches later on if someone decides to build a
board with the clock provided from the i.MX.

Regards,
Lucas

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-26 18:24     ` andrew.smirnov
  2018-11-27 10:06       ` l.stach
@ 2018-11-27 10:16       ` leonard.crestez
  2018-11-27 21:03         ` andrew.smirnov
  1 sibling, 1 reply; 21+ messages in thread
From: leonard.crestez @ 2018-11-27 10:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/26/18 8:24 PM, Andrey Smirnov wrote:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:

>>> +             if (of_property_read_u32_array(
>>> +                         node, "fsl,gpr12-device-type",
>>> +                         imx6_pcie->device_type,
>>> +                         ARRAY_SIZE(imx6_pcie->device_type))) {
>>> +                     dev_err(dev, "Failed to get device type
>>> mask/value\n");
>>> +                     return -EINVAL;
>>> +             }
>>
>> The device type can be set on multiple SOCs, why are you adding a
>> mandatory property only for 8m?
> 
> My thinking was that other SoCs don't really have two controllers, so
> they don't really need to have that property, but, more importantly,
> not forcing them to have this property should preserve backwards
> compatibility with old DTBs.

The "device_type" registers determine Root Complex versus End Point 
mode. This is not yet supported on imx in upstream but it can be done on 
many soc versions.

Looking at other pcie controllers (and dwc core) this is normally done 
with a different compatible string with an "-ep" suffix. It feels wrong 
to expose bitmasks and values directly in DT instead.

For this patch you should probably just hardcode RC mode.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-27 10:06       ` l.stach
@ 2018-11-27 10:46         ` leonard.crestez
  2018-11-27 21:14           ` andrew.smirnov
  0 siblings, 1 reply; 21+ messages in thread
From: leonard.crestez @ 2018-11-27 10:46 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/27/18 12:06 PM, Lucas Stach wrote:
> Hi Andrey,
> 
> Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
>> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>>>
>>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>>>> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>>>> -?????case IMX7D:
>>>> +?????case IMX8MQ:
>>>> +?????????????if (of_property_read_u32(node, "fsl,iomux-gpr1x",
>>>> +??????????????????????????????????????&imx6_pcie->gpr1x)) {
>>>> +?????????????????????dev_err(dev, "Failed to get GPR1x address\n");
>>>> +?????????????????????return -EINVAL;
>>>> +?????????????}
>>>
>>> This is for distinguishing multiple controllers on the SOC but other
>>> registers and bits might differ. Isn't it preferable to have a property
>>> for controller id instead of adding many registers to DT?
>>
>> I liked encoding necessary info in DT directly slightly better than
>> encoding abstract ID and then decoding it further in the driver code.
>> OTOH, I am not really attached to that path. Lucas, can you comment on
>> this please?
> 
> Yes, after rereading the patch with this in mind I agree that having
> the GPR offset on DT directly is IMO the better approach than an
> abstract ID.

But it's not a single offset, for example the device_type (EP/RC) has 
bits for the two controllers side-by-side in GPR12.

Adding a "ctrl-id" property would fully describe the hardware from the 
start. If we add per-register information instead then when other 
registers are used we'll get DT compatibility headaches.

It's worth noting that 8qm/8qxp also have multiple controllers with a 
different set of bits placed differently in iomuxc.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-27 10:16       ` leonard.crestez
@ 2018-11-27 21:03         ` andrew.smirnov
  0 siblings, 0 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-27 21:03 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 27, 2018 at 2:16 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 11/26/18 8:24 PM, Andrey Smirnov wrote:
> > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> >> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>
> >>> +             if (of_property_read_u32_array(
> >>> +                         node, "fsl,gpr12-device-type",
> >>> +                         imx6_pcie->device_type,
> >>> +                         ARRAY_SIZE(imx6_pcie->device_type))) {
> >>> +                     dev_err(dev, "Failed to get device type
> >>> mask/value\n");
> >>> +                     return -EINVAL;
> >>> +             }
> >>
> >> The device type can be set on multiple SOCs, why are you adding a
> >> mandatory property only for 8m?
> >
> > My thinking was that other SoCs don't really have two controllers, so
> > they don't really need to have that property, but, more importantly,
> > not forcing them to have this property should preserve backwards
> > compatibility with old DTBs.
>
> The "device_type" registers determine Root Complex versus End Point
> mode. This is not yet supported on imx in upstream but it can be done on
> many soc versions.
>

Yeah, I am aware, I wasn't really trying to make it possible to
configure IP block to be a EP, that is just an unavoidable consequence
of the approach taken.

> Looking at other pcie controllers (and dwc core) this is normally done
> with a different compatible string with an "-ep" suffix. It feels wrong
> to expose bitmasks and values directly in DT instead.
>
> For this patch you should probably just hardcode RC mode.

Hmm, given how the mask/value have to be different for each
controller, the only way to hardcode it  that I can think of would be
to change the property pass a bit offset instead of mask/value pair.
Is that what you are suggesting?

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-27 10:46         ` leonard.crestez
@ 2018-11-27 21:14           ` andrew.smirnov
  2018-11-28 10:55             ` leonard.crestez
  2018-11-29 11:12             ` l.stach
  0 siblings, 2 replies; 21+ messages in thread
From: andrew.smirnov @ 2018-11-27 21:14 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 11/27/18 12:06 PM, Lucas Stach wrote:
> > Hi Andrey,
> >
> > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> >> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> >>>
> >>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> >>>> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >>>> -     case IMX7D:
> >>>> +     case IMX8MQ:
> >>>> +             if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> >>>> +                                      &imx6_pcie->gpr1x)) {
> >>>> +                     dev_err(dev, "Failed to get GPR1x address\n");
> >>>> +                     return -EINVAL;
> >>>> +             }
> >>>
> >>> This is for distinguishing multiple controllers on the SOC but other
> >>> registers and bits might differ. Isn't it preferable to have a property
> >>> for controller id instead of adding many registers to DT?
> >>
> >> I liked encoding necessary info in DT directly slightly better than
> >> encoding abstract ID and then decoding it further in the driver code.
> >> OTOH, I am not really attached to that path. Lucas, can you comment on
> >> this please?
> >
> > Yes, after rereading the patch with this in mind I agree that having
> > the GPR offset on DT directly is IMO the better approach than an
> > abstract ID.
>
> But it's not a single offset, for example the device_type (EP/RC) has
> bits for the two controllers side-by-side in GPR12.
>

Playing devil's advocate for a bit:

More specifically, currently the following per-controller bits need to
be configured:

- Location of the "device type" field within GPR12
- GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
- Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
via reset controller driver, we need to know which SRC register to use
to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-27 21:14           ` andrew.smirnov
@ 2018-11-28 10:55             ` leonard.crestez
  2018-11-29 11:15               ` l.stach
  2018-11-29 11:12             ` l.stach
  1 sibling, 1 reply; 21+ messages in thread
From: leonard.crestez @ 2018-11-28 10:55 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/27/18 11:15 PM, Andrey Smirnov wrote:
> On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>> On 11/27/18 12:06 PM, Lucas Stach wrote:
>>> Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
>>>> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>>>>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>>>>>> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>>>>>> -     case IMX7D:
>>>>>> +     case IMX8MQ:
>>>>>> +             if (of_property_read_u32(node, "fsl,iomux-gpr1x",
>>>>>> +                                      &imx6_pcie->gpr1x)) {
>>>>>> +                     dev_err(dev, "Failed to get GPR1x address\n");
>>>>>> +                     return -EINVAL;
>>>>>> +             }
>>>>>
>>>>> This is for distinguishing multiple controllers on the SOC but other
>>>>> registers and bits might differ. Isn't it preferable to have a property
>>>>> for controller id instead of adding many registers to DT?
>>>>
>>>> I liked encoding necessary info in DT directly slightly better than
>>>> encoding abstract ID and then decoding it further in the driver code.
>>>> OTOH, I am not really attached to that path. Lucas, can you comment on
>>>> this please?

>>> Yes, after rereading the patch with this in mind I agree that having
>>> the GPR offset on DT directly is IMO the better approach than an
>>> abstract ID.
>>
>> But it's not a single offset, for example the device_type (EP/RC) has
>> bits for the two controllers side-by-side in GPR12.
>>
> 
> Playing devil's advocate for a bit:
> 
> More specifically, currently the following per-controller bits need to
> be configured:
> 
> - Location of the "device type" field within GPR12
> - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> via reset controller driver, we need to know which SRC register to use
> to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)

I looked a bit through bindings and there some instances of syscon-$BLH 
properties which include detailed offsets or bitmasks for $BLAH relative 
to the target syscon node.

If you're going the route of adding properties points to IOMUXC/SRC bits 
it would sense to ensure that they're also usable on other SOCs, 
otherwise you're just making 8mq more complicated. But that's hard.

But I think it's easier to deal with such SOC-specific details behind a 
compat string. Maybe the DT list has some opinion on this?

I wonder if of_alias_get_id would be a reasonable way to distinguish 
between pcie0 and pcie1 instead of adding an ctrl-id property?

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-26 18:09     ` andrew.smirnov
@ 2018-11-29 10:52       ` hongxing.zhu
  0 siblings, 0 replies; 21+ messages in thread
From: hongxing.zhu @ 2018-11-29 10:52 UTC (permalink / raw)
  To: linux-arm-kernel



> -----Original Message-----
> From: Andrey Smirnov [mailto:andrew.smirnov at gmail.com]
> Sent: 2018?11?27? 2:10
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: linux-kernel <linux-kernel@vger.kernel.org>; Bjorn Helgaas
> <bhelgaas@google.com>; Fabio Estevam <fabio.estevam@nxp.com>; Chris
> Healy <cphealy@gmail.com>; Lucas Stach <l.stach@pengutronix.de>;
> Leonard Crestez <leonard.crestez@nxp.com>; Aisheng DONG
> <aisheng.dong@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> linux-pci at vger.kernel.org
> Subject: Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> 
> On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > Hi Andrey:
> > Thanks for your patch-set.
> > I have comment about the L1SS implementation below.
> > It's better to figure out one method to fix it.
> >
> > BR
> > Richard
> >
> > > -----Original Message-----
> > > From: Andrey Smirnov [mailto:andrew.smirnov at gmail.com]
> > > Sent: 2018?11?18? 2:12
> > > To: linux-kernel at vger.kernel.org
> > > Cc: Andrey Smirnov <andrew.smirnov@gmail.com>;
> bhelgaas at google.com;
> > > Fabio Estevam <fabio.estevam@nxp.com>; cphealy at gmail.com;
> > > l.stach at pengutronix.de; Leonard Crestez <leonard.crestez@nxp.com>;
> > > Aisheng DONG <aisheng.dong@nxp.com>; Richard Zhu
> > > <hongxing.zhu@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> > > linux-arm-kernel at lists.infradead.org; linux-pci at vger.kernel.org
> > > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> > >
> > > Cc: bhelgaas at google.com
> > > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > > Cc: cphealy at gmail.com
> > > Cc: l.stach at pengutronix.de
> > > Cc: Leonard Crestez <leonard.crestez@nxp.com>
> > > Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> > > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > > Cc: linux-imx at nxp.com
> > > Cc: linux-arm-kernel at lists.infradead.org
> > > Cc: linux-kernel at vger.kernel.org
> > > Cc: linux-pci at vger.kernel.org
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig    |   2 +-
> > >  drivers/pci/controller/dwc/pci-imx6.c | 117
> > > ++++++++++++++++++++++++--
> > >  2 files changed, 113 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig
> > > b/drivers/pci/controller/dwc/Kconfig
> > > index 91b0194240a5..2b139acccf32 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -90,7 +90,7 @@ config PCI_EXYNOS
> > >
> > >  config PCI_IMX6
> > >       bool "Freescale i.MX6 PCIe controller"
> > > -     depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> > > +     depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM &&
> COMPILE_TEST)
> > >       depends on PCI_MSI_IRQ_DOMAIN
> > >       select PCIE_DW_HOST
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 3c3002861d25..8d1f310e41a6 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -8,6 +8,7 @@
> > >   * Author: Sean Cross <xobs@kosagi.com>
> > >   */
> > >
> > > +#include <linux/bitfield.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/gpio.h>
> > > @@ -30,6 +31,14 @@
> > >
> > >  #include "pcie-designware.h"
> > >
> > > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET              0x7C
> > > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US               (0x6 <<
> 15)
> > > +
> > > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD          BIT(9)
> > > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN  BIT(10)
> > > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE     BIT(11)
> > > +
> > > +
> > >  #define to_imx6_pcie(x)      dev_get_drvdata((x)->dev)
> > >
> > >  enum imx6_pcie_variants {
> > > @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> > >       IMX6SX,
> > >       IMX6QP,
> > >       IMX7D,
> > > +     IMX8MQ,
> > >  };
> > >
> > >  struct imx6_pcie {
> > > @@ -48,8 +58,10 @@ struct imx6_pcie {
> > >       struct clk              *pcie_inbound_axi;
> > >       struct clk              *pcie;
> > >       struct regmap           *iomuxc_gpr;
> > > +     u32                     gpr1x;
> > >       struct reset_control    *pciephy_reset;
> > >       struct reset_control    *apps_reset;
> > > +     struct reset_control    *apps_clk_req;
> > >       struct reset_control    *turnoff_reset;
> > >       enum imx6_pcie_variants variant;
> > >       u32                     tx_deemph_gen1;
> > > @@ -59,6 +71,7 @@ struct imx6_pcie {
> > >       u32                     tx_swing_low;
> > >       int                     link_gen;
> > >       struct regulator        *vpcie;
> > > +     u32                     device_type[2];
> > >  };
> > >
> > >  /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
> > > @@
> > > -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> > > *imx6_pcie)  {
> > >       u32 tmp;
> > >
> > > -     if (imx6_pcie->variant == IMX7D)
> > > +     if (imx6_pcie->variant == IMX7D ||
> > > +         imx6_pcie->variant == IMX8MQ)
> > >               return;
> > >
> > >       pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, &tmp); @@
> -261,6
> > > +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> > > +*imx6_pcie)
> > >       pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp);  }
> > >
> > > +#ifdef CONFIG_ARM
> > >  /*  Added for PCI abort handling */  static int
> > > imx6q_pcie_abort_handler(unsigned long addr,
> > >               unsigned int fsr, struct pt_regs *regs) @@ -294,6
> > > +309,7 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
> > >
> > >       return 1;
> > >  }
> > > +#endif
> > >
> > >  static void imx6_pcie_assert_core_reset(struct imx6_pcie
> > > *imx6_pcie) { @@ -301,6 +317,7 @@ static void
> > > imx6_pcie_assert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > >
> > >       switch (imx6_pcie->variant) {
> > >       case IMX7D:
> > > +     case IMX8MQ: /* FALLTHROUGH */
> > >               reset_control_assert(imx6_pcie->pciephy_reset);
> > >               reset_control_assert(imx6_pcie->apps_reset);
> > >               break;
> > > @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> > > imx6_pcie *imx6_pcie)
> > >               break;
> > >       case IMX7D:
> > >               break;
> > > +     case IMX8MQ:
> > > +             /*
> > > +              * Set the over ride low and enabled
> > > +              * make sure that REF_CLK is turned on.
> > > +              */
> > > +             regmap_update_bits(imx6_pcie->iomuxc_gpr,
> imx6_pcie->gpr1x,
> > > +
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
> > > +                                0);
> > > +             regmap_update_bits(imx6_pcie->iomuxc_gpr,
> imx6_pcie->gpr1x,
> > > +
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
> > > +
> IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
> > > +             break;
> > >       }
> > >
> > >       return ret;
> > > @@ -397,6 +426,7 @@ static void imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)  {
> > >       struct dw_pcie *pci = imx6_pcie->pci;
> > >       struct device *dev = pci->dev;
> > > +     unsigned int val;
> > >       int ret;
> > >
> > >       if (imx6_pcie->vpcie &&
> > > !regulator_is_enabled(imx6_pcie->vpcie)) { @@
> > > -445,6 +475,29 @@ static void imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)
> > >       }
> > >
> > >       switch (imx6_pcie->variant) {
> > > +     case IMX8MQ:
> > > +             reset_control_deassert(imx6_pcie->pciephy_reset);
> > > +             udelay(100);
> > > +             /*
> > > +              * Configure the CLK_REQ# high, let the L1SS
> > > +              * automatically controlled by HW.
> > > +              */
> > > +             reset_control_assert(imx6_pcie->apps_clk_req);
> > > +
> > > +             /*
> > > +              * Configure the L1 latency of rc to less than 64us
> > > +              * Otherwise, the L1/L1SUB wouldn't be enable by
> ASPM.
> > > +              */
> > > +             val = dw_pcie_readl_dbi(imx6_pcie->pci,
> > > +                                     SZ_1M +
> > > +
> IMX8MQ_PCIE_LINK_CAP_REG_OFFSET);
> > > +             val &= ~PCI_EXP_LNKCAP_L1EL;
> > > +             val |= IMX8MQ_PCIE_LINK_CAP_L1EL_64US;
> > > +             dw_pcie_writel_dbi(imx6_pcie->pci,
> > > +                                SZ_1M +
> > > +
> IMX8MQ_PCIE_LINK_CAP_REG_OFFSET,
> > > +                                val);
> > > +             break;
> > >       case IMX7D:
> > >               reset_control_deassert(imx6_pcie->pciephy_reset);
> > >               imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > > @@ -483,6 +536,15 @@ static void
> > > imx6_pcie_deassert_core_reset(struct
> > > imx6_pcie *imx6_pcie)  static void imx6_pcie_init_phy(struct
> > > imx6_pcie
> > > *imx6_pcie)  {
> > >       switch (imx6_pcie->variant) {
> > > +     case IMX8MQ:
> > > +             /*
> > > +              * TODO: Currently this code assumes external
> > > +              * oscillator is being used
> > > +              */
> > > +             regmap_update_bits(imx6_pcie->iomuxc_gpr,
> imx6_pcie->gpr1x,
> > > +
> IMX8MQ_GPR_PCIE_REF_USE_PAD,
> > > +
> IMX8MQ_GPR_PCIE_REF_USE_PAD);
> > > +             break;
> > >       case IMX7D:
> > >               regmap_update_bits(imx6_pcie->iomuxc_gpr,
> IOMUXC_GPR12,
> > >
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > > 0); @@
> > > -519,7 +581,8 @@ static void imx6_pcie_init_phy(struct imx6_pcie
> > > *imx6_pcie)
> > >       }
> > >
> > >       regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > -                     IMX6Q_GPR12_DEVICE_TYPE,
> PCI_EXP_TYPE_ROOT_PORT <<
> > > 12);
> > > +                        imx6_pcie->device_type[0],
> > > +                        imx6_pcie->device_type[1]);
> > >  }
> > >
> > >  static int imx6_setup_phy_mpll(struct imx6_pcie *imx6_pcie) @@
> > > -528,7
> > > +591,8 @@ static int imx6_setup_phy_mpll(struct imx6_pcie
> > > +*imx6_pcie)
> > >       int mult, div;
> > >       u32 val;
> > >
> > > -     if (imx6_pcie->variant == IMX7D)
> > > +     if (imx6_pcie->variant == IMX7D ||
> > > +         imx6_pcie->variant == IMX8MQ)
> > >               return 0;
> > >
> > >       switch (phy_rate) {
> > > @@ -616,6 +680,7 @@ static void imx6_pcie_ltssm_enable(struct device
> > > *dev)
> > >                                  IMX6Q_GPR12_PCIE_CTL_2);
> > >               break;
> > >       case IMX7D:
> > > +     case IMX8MQ:            /* FALLTHROUGH */
> > >               reset_control_deassert(imx6_pcie->apps_reset);
> > >               break;
> > >       }
> > > @@ -798,10 +863,24 @@ static void imx6_pcie_clk_disable(struct
> > > imx6_pcie
> > > *imx6_pcie)
> > >       clk_disable_unprepare(imx6_pcie->pcie_phy);
> > >       clk_disable_unprepare(imx6_pcie->pcie_bus);
> > >
> > > -     if (imx6_pcie->variant == IMX7D) {
> > > +     switch (imx6_pcie->variant) {
> > > +     case IMX7D:
> > >               regmap_update_bits(imx6_pcie->iomuxc_gpr,
> IOMUXC_GPR12,
> > >
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
> > >
> IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
> > > +             break;
> > > +     /*
> > > +      * Disable the override. Configure the CLK_REQ# high, let the
> > > +      * L1SS automatically controlled by HW when link is up.
> > > +      * Otherwise, turn off the REF_CLK to save power consumption.
> > > +      */
> > [Richard Zhu] About the L1SS support, there is a back-compatible issue.
> > When one none-L1SS capability EP device is used in one HW design solution.
> > In this case, EP device wouldn't manipulated its own CLK_REQ#.
> > The CLK_REQ# would be high when the override_en is disabled here.
> > That means the REFCLK would be turned off abnormally.
> > System would have problem when the REFCLK of PCIe is turned off
> abnormally after link is up.
> >
> 
> I don't have a lot of expertise in this area, so please take my comment with a
> grain of salt. The way I understand it, is in legacy systems that do not support
> L1SS with CLKREQ that feature shouldn't be enabled/used. Unless I've missed
> something, I think it should be possible to do so and disable the feature by
> configuring corresponding CLKREQ_B pad as GPIO and adding a GPIO hog
> node to pull CLKREQ low. Do you see any problems with this approach?
[Richard Zhu]Sorry to reply late. You're right. The L1SS shouldn't be enabled when
 L1SS none-supported EP device is inserted.
The CLKREQ# can be configured as INPUT and OD, I think.
But the L1SS capability should be figured out by SW in a proper mechanism.
And SW should enable the L1SS automatically refer to the capability.
I found the below notice in the L1SS ECN.
" 5. Analysis of the Software Implications
Capability discovery and enabling are required. 
"

> 
> Regardless though, I wasn't really planning on including PM support in this
> series. The code in question shouldn't even be in the patch since it'll never be
> executed because imx6_pcie_clk_disable() is only called by
> imx6_pcie_suspend_noirq() which is a no-op on all variants except i.MX7D.
> 
> I'll drop all of the unused PM-related bits I missed from the series and we can
> add them later in a separate submission.
> 
[Richard Zhu] Agree with that.
The PM feature can be added later in a standalone patch-set.
Thanks
Richard

> Thanks,
> Andrey Smirnov

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-27 21:14           ` andrew.smirnov
  2018-11-28 10:55             ` leonard.crestez
@ 2018-11-29 11:12             ` l.stach
  1 sibling, 0 replies; 21+ messages in thread
From: l.stach @ 2018-11-29 11:12 UTC (permalink / raw)
  To: linux-arm-kernel

Am Dienstag, den 27.11.2018, 13:14 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > 
> > On 11/27/18 12:06 PM, Lucas Stach wrote:
> > > Hi Andrey,
> > > 
> > > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> > > > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > > > 
> > > > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > > > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > > > > -?????case IMX7D:
> > > > > > +?????case IMX8MQ:
> > > > > > +?????????????if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > > > > +??????????????????????????????????????&imx6_pcie->gpr1x)) {
> > > > > > +?????????????????????dev_err(dev, "Failed to get GPR1x address\n");
> > > > > > +?????????????????????return -EINVAL;
> > > > > > +?????????????}
> > > > > 
> > > > > This is for distinguishing multiple controllers on the SOC but other
> > > > > registers and bits might differ. Isn't it preferable to have a property
> > > > > for controller id instead of adding many registers to DT?
> > > > 
> > > > I liked encoding necessary info in DT directly slightly better than
> > > > encoding abstract ID and then decoding it further in the driver code.
> > > > OTOH, I am not really attached to that path. Lucas, can you comment on
> > > > this please?
> > > 
> > > Yes, after rereading the patch with this in mind I agree that having
> > > the GPR offset on DT directly is IMO the better approach than an
> > > abstract ID.
> > 
> > But it's not a single offset, for example the device_type (EP/RC) has
> > bits for the two controllers side-by-side in GPR12.
> > 
> 
> Playing devil's advocate for a bit:
> 
> More specifically, currently the following per-controller bits need to
> be configured:
> 
> - Location of the "device type" field within GPR12
> - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> via reset controller driver, we need to know which SRC register to use
> to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)

If if we need a lot of different GPR offsets, I revise my earlier
opinion. We certainly don't want lots of bit offsets and masks in the
DT. The whole GPR thing is ugly, but I would rather hide this in the
driver code, keyed by compatible and a controller id property, where we
can change things without any worries about DT backward compatibility.

Regards,
Lucas

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
  2018-11-28 10:55             ` leonard.crestez
@ 2018-11-29 11:15               ` l.stach
  0 siblings, 0 replies; 21+ messages in thread
From: l.stach @ 2018-11-29 11:15 UTC (permalink / raw)
  To: linux-arm-kernel

Am Mittwoch, den 28.11.2018, 10:55 +0000 schrieb Leonard Crestez:
> On 11/27/18 11:15 PM, Andrey Smirnov wrote:
> > On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > On 11/27/18 12:06 PM, Lucas Stach wrote:
> > > > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> > > > > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> > > > > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > > > > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > > > > > -?????case IMX7D:
> > > > > > > +?????case IMX8MQ:
> > > > > > > +?????????????if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > > > > > +??????????????????????????????????????&imx6_pcie->gpr1x)) {
> > > > > > > +?????????????????????dev_err(dev, "Failed to get GPR1x address\n");
> > > > > > > +?????????????????????return -EINVAL;
> > > > > > > +?????????????}
> > > > > > 
> > > > > > This is for distinguishing multiple controllers on the SOC but other
> > > > > > registers and bits might differ. Isn't it preferable to have a property
> > > > > > for controller id instead of adding many registers to DT?
> > > > > 
> > > > > I liked encoding necessary info in DT directly slightly better than
> > > > > encoding abstract ID and then decoding it further in the driver code.
> > > > > OTOH, I am not really attached to that path. Lucas, can you comment on
> > > > > this please?
> > > > Yes, after rereading the patch with this in mind I agree that having
> > > > the GPR offset on DT directly is IMO the better approach than an
> > > > abstract ID.
> > > 
> > > But it's not a single offset, for example the device_type (EP/RC) has
> > > bits for the two controllers side-by-side in GPR12.
> > > 
> > 
> > Playing devil's advocate for a bit:
> > 
> > More specifically, currently the following per-controller bits need to
> > be configured:
> > 
> > - Location of the "device type" field within GPR12
> > - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> > PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> > - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> > via reset controller driver, we need to know which SRC register to use
> > to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
> 
> I looked a bit through bindings and there some instances of syscon-$BLH?
> properties which include detailed offsets or bitmasks for $BLAH relative?
> to the target syscon node.
> 
> If you're going the route of adding properties points to IOMUXC/SRC bits?
> it would sense to ensure that they're also usable on other SOCs,?
> otherwise you're just making 8mq more complicated. But that's hard.
> 
> But I think it's easier to deal with such SOC-specific details behind a?
> compat string. Maybe the DT list has some opinion on this?

I agree with this. The number of bits and offsets is too large to keep
it in DT properties, without running into backwards compat issues later
on.

> I wonder if of_alias_get_id would be a reasonable way to distinguish?
> between pcie0 and pcie1 instead of adding an ctrl-id property?

No, the alias is a arbitrary number that can change from board to
board. It can, but doesn't need to, correspond to any real hardware ID.
If we need a hardware controller ID, then there is no way around adding
a property to the PCIe controller node for this.

Regards,
Lucas

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/3] PCIE support for i.MX8MQ
  2018-11-17 18:12 [PATCH 0/3] PCIE support for i.MX8MQ andrew.smirnov
                   ` (2 preceding siblings ...)
  2018-11-17 18:12 ` [PATCH 3/3] PCI: imx: Add support for i.MX8MQ andrew.smirnov
@ 2018-12-03 12:20 ` " Lorenzo Pieralisi
  2018-12-07 11:37 ` Lorenzo Pieralisi
  4 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2018-12-03 12:20 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: A.s. Dong, Richard Zhu, linux-arm-kernel, linux-pci,
	linux-kernel, Fabio Estevam, linux-imx, bhelgaas,
	Leonard Crestez, cphealy, l.stach

On Sat, Nov 17, 2018 at 10:12:22AM -0800, Andrey Smirnov wrote:
> 
> Everyone:
> 
> This series contains changes I made in order to enable support of PCIE
> IP block on i.MX8MQ SoCs (full tree can be found at [github-v0]). This series
> is _very_ preliminary and by no means is ready for inclusion (it also
> has some unmet dependencies).  However is should be in OK enough shape
> to get some early feedback on, which is the intent of this submission.
> 
> Specifically, I'd like to get some feedback on whether newly
> introduced "fsl,iomux-gpr1x" and "fsl,gpr12-device-type" DT
> properties, added to handle differences between PCIE0 and PCIE1, is a
> good (acceptable) solution for the problem.
> 
> All other feedback is appreciated as well!
> 
> Thank you,
> Andrey Smirnov
> 
> [github-v0] https://github.com/ndreys/linux/commits/imx8mq-pcie-v0
> 
> Andrey Smirnov (3):
>   PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D
>   PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
>   PCI: imx: Add support for i.MX8MQ
> 
>  drivers/pci/controller/dwc/Kconfig    |   2 +-
>  drivers/pci/controller/dwc/pci-imx6.c | 119 +++++++++++++++++++++++++-
>  2 files changed, 117 insertions(+), 4 deletions(-)

I am expecting you to post of v2 of this series, addressing review
comments so I will mark these patches with "Changes Requested", please
let me know if you have any objections.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/3] PCIE support for i.MX8MQ
  2018-11-17 18:12 [PATCH 0/3] PCIE support for i.MX8MQ andrew.smirnov
                   ` (3 preceding siblings ...)
  2018-12-03 12:20 ` [PATCH 0/3] PCIE " Lorenzo Pieralisi
@ 2018-12-07 11:37 ` Lorenzo Pieralisi
  4 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2018-12-07 11:37 UTC (permalink / raw)
  To: Andrey Smirnov
  Cc: A.s. Dong, Richard Zhu, linux-arm-kernel, linux-pci,
	linux-kernel, Fabio Estevam, linux-imx, bhelgaas,
	Leonard Crestez, cphealy, l.stach

On Sat, Nov 17, 2018 at 10:12:22AM -0800, Andrey Smirnov wrote:
> 
> Everyone:
> 
> This series contains changes I made in order to enable support of PCIE
> IP block on i.MX8MQ SoCs (full tree can be found at [github-v0]). This series
> is _very_ preliminary and by no means is ready for inclusion (it also
> has some unmet dependencies).  However is should be in OK enough shape
> to get some early feedback on, which is the intent of this submission.
> 
> Specifically, I'd like to get some feedback on whether newly
> introduced "fsl,iomux-gpr1x" and "fsl,gpr12-device-type" DT
> properties, added to handle differences between PCIE0 and PCIE1, is a
> good (acceptable) solution for the problem.
> 
> All other feedback is appreciated as well!
> 
> Thank you,
> Andrey Smirnov
> 
> [github-v0] https://github.com/ndreys/linux/commits/imx8mq-pcie-v0
> 
> Andrey Smirnov (3):
>   PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D
>   PCI: imx: No-op imx6_pcie_reset_phy() on i.MX7D
>   PCI: imx: Add support for i.MX8MQ
> 
>  drivers/pci/controller/dwc/Kconfig    |   2 +-
>  drivers/pci/controller/dwc/pci-imx6.c | 119 +++++++++++++++++++++++++-
>  2 files changed, 117 insertions(+), 4 deletions(-)

Applied to pci/dwc for v4.21, thanks.

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-17 18:12 [PATCH 0/3] PCIE support for i.MX8MQ andrew.smirnov
2018-11-17 18:12 ` [PATCH 1/3] PCI: imx: No-op imx6_setup_phy_mpll() on i.MX7D andrew.smirnov
2018-11-17 18:12 ` [PATCH 2/3] PCI: imx: No-op imx6_pcie_reset_phy() " andrew.smirnov
2018-11-20  1:22   ` tpiepho
2018-11-26 18:32     ` andrew.smirnov
2018-11-17 18:12 ` [PATCH 3/3] PCI: imx: Add support for i.MX8MQ andrew.smirnov
2018-11-19  7:07   ` hongxing.zhu
2018-11-26 18:09     ` andrew.smirnov
2018-11-29 10:52       ` hongxing.zhu
2018-11-20 10:48   ` leonard.crestez
2018-11-26 18:24     ` andrew.smirnov
2018-11-27 10:06       ` l.stach
2018-11-27 10:46         ` leonard.crestez
2018-11-27 21:14           ` andrew.smirnov
2018-11-28 10:55             ` leonard.crestez
2018-11-29 11:15               ` l.stach
2018-11-29 11:12             ` l.stach
2018-11-27 10:16       ` leonard.crestez
2018-11-27 21:03         ` andrew.smirnov
2018-12-03 12:20 ` [PATCH 0/3] PCIE " Lorenzo Pieralisi
2018-12-07 11:37 ` Lorenzo Pieralisi

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox