[PATCH 0/7] imx8mp: Enable PCIe/NVMe support

[PATCH 0/7] imx8mp: Enable PCIe/NVMe support

[Sumit Garg]

pcie_imx doesn't seem to share any useful code for iMX8MP SoC and it is
rather tied to quite old port of pcie_designware driver from Linux which
suffices only iMX6 specific needs.

But currently we have the common DWC specific bits which alligns pretty
well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
add support for other iMX8 variants to this driver.

iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
can reuse the generic PHY infrastructure to power on PCIe PHY.

Patch #1: Adds PCIe clocks support.
Patch #2: Adds i.MX8MP reset controller support.
Patch #3: Extend i.MX8MP power domain driver with PCIe support
Patch #4: Expose high performance PLL clock required for PCIe PHY
          on verdin board.
Patch #5: Adds standalone PCIe PHY support for i.MX8MP SoC.
Patch #6: Adds DW PCIe controller support for iMX8MP SoC.
Patch #7: Enable PCIe/NVMe support for verdin board.

Testing with this patch-set included:

Verdin iMX8MP # pci enum
PCIE-0: Link up (Gen1-x1, Bus0)
Verdin iMX8MP # 
Verdin iMX8MP # nvme scan
Verdin iMX8MP # 
Verdin iMX8MP # nvme info
Device 0: Vendor: 0x126f Rev: T0828A0  Prod: AA000000000000000720
            Type: Hard Disk
            Capacity: 122104.3 MB = 119.2 GB (250069680 x 512)
Verdin iMX8MP # 
Verdin iMX8MP # load nvme 0 $loadaddr <file-name>

Sumit Garg (7):
  clk: imx8mp: Add support for PCIe clocks
  reset: imx: Add support for i.MX8MP reset controller
  imx8mp: power-domain: Add PCIe support
  imx8mp: power-domain: Expose high performance PLL clock
  phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY
  pci: Add DW PCIe controller support for iMX8MP SoC
  verdin-imx8mp_defconfig: Enable PCIe/NVMe support

 configs/verdin-imx8mp_defconfig       |   9 +
 drivers/clk/imx/clk-imx8mp.c          |   6 +
 drivers/pci/Kconfig                   |   8 +
 drivers/pci/Makefile                  |   1 +
 drivers/pci/pcie_dw_imx8.c            | 348 ++++++++++++++++++++++++++
 drivers/phy/Kconfig                   |   9 +
 drivers/phy/Makefile                  |   1 +
 drivers/phy/phy-imx8m-pcie.c          | 246 ++++++++++++++++++
 drivers/power/domain/imx8mp-hsiomix.c | 121 ++++++++-
 drivers/reset/reset-imx7.c            | 114 +++++++++
 10 files changed, 859 insertions(+), 4 deletions(-)
 create mode 100644 drivers/pci/pcie_dw_imx8.c
 create mode 100644 drivers/phy/phy-imx8m-pcie.c

-- 
2.34.1

[PATCH 1/7] clk: imx8mp: Add support for PCIe clocks

[Sumit Garg]

Pre-requisite to enable PCIe support on iMX8MP SoC.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/clk/imx/clk-imx8mp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index a21a3ce34bb..7dfc829df2c 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -62,6 +62,10 @@ static const char *imx8mp_dram_apb_sels[] = {"clock-osc-24m", "sys_pll2_200m", "
 					     "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
 					     "sys_pll2_250m", "audio_pll2_out", };
 
+static const char * const imx8mp_pcie_aux_sels[] = {"clock-osc-24m", "sys_pll2_200m", "sys_pll2_50m",
+						    "sys_pll3_out", "sys_pll2_100m", "sys_pll1_80m",
+						    "sys_pll1_160m", "sys_pll1_200m", };
+
 static const char *imx8mp_i2c5_sels[] = {"clock-osc-24m", "sys_pll1_160m", "sys_pll2_50m",
 					 "sys_pll3_out", "audio_pll1_out", "video_pll1_out",
 					 "audio_pll2_out", "sys_pll1_133m", };
@@ -272,6 +276,7 @@ static int imx8mp_clk_probe(struct udevice *dev)
 
 	clk_dm(IMX8MP_CLK_DRAM_ALT, imx8m_clk_composite("dram_alt", imx8mp_dram_alt_sels, base + 0xa000));
 	clk_dm(IMX8MP_CLK_DRAM_APB, imx8m_clk_composite_critical("dram_apb", imx8mp_dram_apb_sels, base + 0xa080));
+	clk_dm(IMX8MP_CLK_PCIE_AUX, imx8m_clk_composite("pcie_aux", imx8mp_pcie_aux_sels, base + 0xa400));
 	clk_dm(IMX8MP_CLK_I2C5, imx8m_clk_composite("i2c5", imx8mp_i2c5_sels, base + 0xa480));
 	clk_dm(IMX8MP_CLK_I2C6, imx8m_clk_composite("i2c6", imx8mp_i2c6_sels, base + 0xa500));
 	clk_dm(IMX8MP_CLK_ENET_QOS, imx8m_clk_composite("enet_qos", imx8mp_enet_qos_sels, base + 0xa880));
@@ -322,6 +327,7 @@ static int imx8mp_clk_probe(struct udevice *dev)
 	clk_dm(IMX8MP_CLK_I2C2_ROOT, imx_clk_gate4("i2c2_root_clk", "i2c2", base + 0x4180, 0));
 	clk_dm(IMX8MP_CLK_I2C3_ROOT, imx_clk_gate4("i2c3_root_clk", "i2c3", base + 0x4190, 0));
 	clk_dm(IMX8MP_CLK_I2C4_ROOT, imx_clk_gate4("i2c4_root_clk", "i2c4", base + 0x41a0, 0));
+	clk_dm(IMX8MP_CLK_PCIE_ROOT, imx_clk_gate4("pcie_root_clk", "pcie_aux", base + 0x4250, 0));
 	clk_dm(IMX8MP_CLK_PWM1_ROOT, imx_clk_gate4("pwm1_root_clk", "pwm1", base + 0x4280, 0));
 	clk_dm(IMX8MP_CLK_PWM2_ROOT, imx_clk_gate4("pwm2_root_clk", "pwm2", base + 0x4290, 0));
 	clk_dm(IMX8MP_CLK_PWM3_ROOT, imx_clk_gate4("pwm3_root_clk", "pwm3", base + 0x42a0, 0));
-- 
2.34.1

[PATCH 2/7] reset: imx: Add support for i.MX8MP reset controller

[Sumit Garg]

Pre-requisite to enable PCIe support on iMX8MP SoC.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/reset/reset-imx7.c | 114 +++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index eaef2cc2cdf..c1de84dea8b 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -10,6 +10,7 @@
 #include <dm.h>
 #include <dt-bindings/reset/imx7-reset.h>
 #include <dt-bindings/reset/imx8mq-reset.h>
+#include <dt-bindings/reset/imx8mp-reset.h>
 #include <reset-uclass.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
@@ -252,6 +253,115 @@ static int imx7_reset_assert_imx8mq(struct reset_ctl *rst)
 	return 0;
 }
 
+enum imx8mp_src_registers {
+	SRC_SUPERMIX_RCR	= 0x0018,
+	SRC_AUDIOMIX_RCR	= 0x001c,
+	SRC_MLMIX_RCR		= 0x0028,
+	SRC_GPU2D_RCR		= 0x0038,
+	SRC_GPU3D_RCR		= 0x003c,
+	SRC_VPU_G1_RCR		= 0x0048,
+	SRC_VPU_G2_RCR		= 0x004c,
+	SRC_VPUVC8KE_RCR	= 0x0050,
+	SRC_NOC_RCR		= 0x0054,
+};
+
+static const struct imx7_src_signal imx8mp_src_signals[IMX8MP_RESET_NUM] = {
+	[IMX8MP_RESET_A53_CORE_POR_RESET0]	= { SRC_A53RCR0, BIT(0) },
+	[IMX8MP_RESET_A53_CORE_POR_RESET1]	= { SRC_A53RCR0, BIT(1) },
+	[IMX8MP_RESET_A53_CORE_POR_RESET2]	= { SRC_A53RCR0, BIT(2) },
+	[IMX8MP_RESET_A53_CORE_POR_RESET3]	= { SRC_A53RCR0, BIT(3) },
+	[IMX8MP_RESET_A53_CORE_RESET0]		= { SRC_A53RCR0, BIT(4) },
+	[IMX8MP_RESET_A53_CORE_RESET1]		= { SRC_A53RCR0, BIT(5) },
+	[IMX8MP_RESET_A53_CORE_RESET2]		= { SRC_A53RCR0, BIT(6) },
+	[IMX8MP_RESET_A53_CORE_RESET3]		= { SRC_A53RCR0, BIT(7) },
+	[IMX8MP_RESET_A53_DBG_RESET0]		= { SRC_A53RCR0, BIT(8) },
+	[IMX8MP_RESET_A53_DBG_RESET1]		= { SRC_A53RCR0, BIT(9) },
+	[IMX8MP_RESET_A53_DBG_RESET2]		= { SRC_A53RCR0, BIT(10) },
+	[IMX8MP_RESET_A53_DBG_RESET3]		= { SRC_A53RCR0, BIT(11) },
+	[IMX8MP_RESET_A53_ETM_RESET0]		= { SRC_A53RCR0, BIT(12) },
+	[IMX8MP_RESET_A53_ETM_RESET1]		= { SRC_A53RCR0, BIT(13) },
+	[IMX8MP_RESET_A53_ETM_RESET2]		= { SRC_A53RCR0, BIT(14) },
+	[IMX8MP_RESET_A53_ETM_RESET3]		= { SRC_A53RCR0, BIT(15) },
+	[IMX8MP_RESET_A53_SOC_DBG_RESET]	= { SRC_A53RCR0, BIT(20) },
+	[IMX8MP_RESET_A53_L2RESET]		= { SRC_A53RCR0, BIT(21) },
+	[IMX8MP_RESET_SW_NON_SCLR_M7C_RST]	= { SRC_M4RCR, BIT(0) },
+	[IMX8MP_RESET_OTG1_PHY_RESET]		= { SRC_USBOPHY1_RCR, BIT(0) },
+	[IMX8MP_RESET_OTG2_PHY_RESET]		= { SRC_USBOPHY2_RCR, BIT(0) },
+	[IMX8MP_RESET_SUPERMIX_RESET]		= { SRC_SUPERMIX_RCR, BIT(0) },
+	[IMX8MP_RESET_AUDIOMIX_RESET]		= { SRC_AUDIOMIX_RCR, BIT(0) },
+	[IMX8MP_RESET_MLMIX_RESET]		= { SRC_MLMIX_RCR, BIT(0) },
+	[IMX8MP_RESET_PCIEPHY]			= { SRC_PCIEPHY_RCR, BIT(2) },
+	[IMX8MP_RESET_PCIEPHY_PERST]		= { SRC_PCIEPHY_RCR, BIT(3) },
+	[IMX8MP_RESET_PCIE_CTRL_APPS_EN]	= { SRC_PCIEPHY_RCR, BIT(6) },
+	[IMX8MP_RESET_PCIE_CTRL_APPS_TURNOFF]	= { SRC_PCIEPHY_RCR, BIT(11) },
+	[IMX8MP_RESET_HDMI_PHY_APB_RESET]	= { SRC_HDMI_RCR, BIT(0) },
+	[IMX8MP_RESET_MEDIA_RESET]		= { SRC_DISP_RCR, BIT(0) },
+	[IMX8MP_RESET_GPU2D_RESET]		= { SRC_GPU2D_RCR, BIT(0) },
+	[IMX8MP_RESET_GPU3D_RESET]		= { SRC_GPU3D_RCR, BIT(0) },
+	[IMX8MP_RESET_GPU_RESET]		= { SRC_GPU_RCR, BIT(0) },
+	[IMX8MP_RESET_VPU_RESET]		= { SRC_VPU_RCR, BIT(0) },
+	[IMX8MP_RESET_VPU_G1_RESET]		= { SRC_VPU_G1_RCR, BIT(0) },
+	[IMX8MP_RESET_VPU_G2_RESET]		= { SRC_VPU_G2_RCR, BIT(0) },
+	[IMX8MP_RESET_VPUVC8KE_RESET]		= { SRC_VPUVC8KE_RCR, BIT(0) },
+	[IMX8MP_RESET_NOC_RESET]		= { SRC_NOC_RCR, BIT(0) },
+};
+
+static int imx7_reset_assert_imx8mp(struct reset_ctl *rst)
+{
+	struct imx7_reset_priv *priv = dev_get_priv(rst->dev);
+	const struct imx7_src_signal *sig = imx8mp_src_signals;
+	u32 val;
+
+	if (rst->id >= IMX8MP_RESET_NUM)
+		return -EINVAL;
+
+	val = readl(priv->base + sig[rst->id].offset);
+	switch (rst->id) {
+	case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
+	case IMX8MP_RESET_PCIEPHY_PERST:
+		val &= ~sig[rst->id].bit;
+		break;
+	default:
+		val |= sig[rst->id].bit;
+		break;
+	}
+	writel(val, priv->base + sig[rst->id].offset);
+
+	return 0;
+}
+
+static int imx7_reset_deassert_imx8mp(struct reset_ctl *rst)
+{
+	struct imx7_reset_priv *priv = dev_get_priv(rst->dev);
+	const struct imx7_src_signal *sig = imx8mp_src_signals;
+	u32 val;
+
+	if (rst->id >= IMX8MP_RESET_NUM)
+		return -EINVAL;
+
+	if (rst->id == IMX8MP_RESET_PCIEPHY) {
+		/*
+		 * wait for more than 10us to release phy g_rst and
+		 * btnrst
+		 */
+		udelay(10);
+	}
+
+	val = readl(priv->base + sig[rst->id].offset);
+	switch (rst->id) {
+	case IMX8MP_RESET_PCIE_CTRL_APPS_EN:
+	case IMX8MP_RESET_PCIEPHY_PERST:
+		val |= sig[rst->id].bit;
+		break;
+	default:
+		val &= ~sig[rst->id].bit;
+		break;
+	}
+	writel(val, priv->base + sig[rst->id].offset);
+
+	return 0;
+}
+
 static int imx7_reset_assert(struct reset_ctl *rst)
 {
 	struct imx7_reset_priv *priv = dev_get_priv(rst->dev);
@@ -272,6 +382,7 @@ static const struct reset_ops imx7_reset_reset_ops = {
 static const struct udevice_id imx7_reset_ids[] = {
 	{ .compatible = "fsl,imx7d-src" },
 	{ .compatible = "fsl,imx8mq-src" },
+	{ .compatible = "fsl,imx8mp-src" },
 	{ }
 };
 
@@ -289,6 +400,9 @@ static int imx7_reset_probe(struct udevice *dev)
 	} else if (device_is_compatible(dev, "fsl,imx7d-src")) {
 		priv->ops.rst_assert = imx7_reset_assert_imx7;
 		priv->ops.rst_deassert = imx7_reset_deassert_imx7;
+	} else if (device_is_compatible(dev, "fsl,imx8mp-src")) {
+		priv->ops.rst_assert = imx7_reset_assert_imx8mp;
+		priv->ops.rst_deassert = imx7_reset_deassert_imx8mp;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH 2/7] reset: imx: Add support for i.MX8MP reset controller

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> Pre-requisite to enable PCIe support on iMX8MP SoC.

Please write a proper commit message .

> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>   drivers/reset/reset-imx7.c | 114 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index eaef2cc2cdf..c1de84dea8b 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -10,6 +10,7 @@
>   #include <dm.h>
>   #include <dt-bindings/reset/imx7-reset.h>
>   #include <dt-bindings/reset/imx8mq-reset.h>
> +#include <dt-bindings/reset/imx8mp-reset.h>
>   #include <reset-uclass.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
> @@ -252,6 +253,115 @@ static int imx7_reset_assert_imx8mq(struct reset_ctl *rst)
>   	return 0;
>   }
>   
> +enum imx8mp_src_registers {
> +	SRC_SUPERMIX_RCR	= 0x0018,
> +	SRC_AUDIOMIX_RCR	= 0x001c,
> +	SRC_MLMIX_RCR		= 0x0028,
> +	SRC_GPU2D_RCR		= 0x0038,
> +	SRC_GPU3D_RCR		= 0x003c,
> +	SRC_VPU_G1_RCR		= 0x0048,
> +	SRC_VPU_G2_RCR		= 0x004c,
> +	SRC_VPUVC8KE_RCR	= 0x0050,
> +	SRC_NOC_RCR		= 0x0054,
> +};

This seems copied from Linux, include Linux commit ID as of which this 
was imported from in commit message.

Re: [PATCH 2/7] reset: imx: Add support for i.MX8MP reset controller

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > Pre-requisite to enable PCIe support on iMX8MP SoC.
>
> Please write a proper commit message .
>

How about the following?

    Add support for i.MX8MP reset controller. It is required
    to enable PCIe support on the iMX8MP SoC.

> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >   drivers/reset/reset-imx7.c | 114 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 114 insertions(+)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index eaef2cc2cdf..c1de84dea8b 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -10,6 +10,7 @@
> >   #include <dm.h>
> >   #include <dt-bindings/reset/imx7-reset.h>
> >   #include <dt-bindings/reset/imx8mq-reset.h>
> > +#include <dt-bindings/reset/imx8mp-reset.h>
> >   #include <reset-uclass.h>
> >   #include <linux/bitops.h>
> >   #include <linux/delay.h>
> > @@ -252,6 +253,115 @@ static int imx7_reset_assert_imx8mq(struct reset_ctl *rst)
> >       return 0;
> >   }
> >
> > +enum imx8mp_src_registers {
> > +     SRC_SUPERMIX_RCR        = 0x0018,
> > +     SRC_AUDIOMIX_RCR        = 0x001c,
> > +     SRC_MLMIX_RCR           = 0x0028,
> > +     SRC_GPU2D_RCR           = 0x0038,
> > +     SRC_GPU3D_RCR           = 0x003c,
> > +     SRC_VPU_G1_RCR          = 0x0048,
> > +     SRC_VPU_G2_RCR          = 0x004c,
> > +     SRC_VPUVC8KE_RCR        = 0x0050,
> > +     SRC_NOC_RCR             = 0x0054,
> > +};
>
> This seems copied from Linux, include Linux commit ID as of which this
> was imported from in commit message.

Do you expect imx8mp_src_registers to change? I expect them to be the
same, so does Linux commit ID add any value here?

-Sumit

Re: [PATCH 2/7] reset: imx: Add support for i.MX8MP reset controller

[Marek Vasut]

On 2/21/24 06:40, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/20/24 14:10, Sumit Garg wrote:
>>> Pre-requisite to enable PCIe support on iMX8MP SoC.
>>
>> Please write a proper commit message .
>>
> 
> How about the following?
> 
>      Add support for i.MX8MP reset controller. It is required
>      to enable PCIe support on the iMX8MP SoC.

The part about PCIe is not relevant, it is a reset controller and it is 
needed to reset various IPs. See e.g. Linux:

e08672c03981 ("reset: imx7: Add support for i.MX8MP SoC")

>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>> ---
>>>    drivers/reset/reset-imx7.c | 114 +++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 114 insertions(+)
>>>
>>> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
>>> index eaef2cc2cdf..c1de84dea8b 100644
>>> --- a/drivers/reset/reset-imx7.c
>>> +++ b/drivers/reset/reset-imx7.c
>>> @@ -10,6 +10,7 @@
>>>    #include <dm.h>
>>>    #include <dt-bindings/reset/imx7-reset.h>
>>>    #include <dt-bindings/reset/imx8mq-reset.h>
>>> +#include <dt-bindings/reset/imx8mp-reset.h>
>>>    #include <reset-uclass.h>
>>>    #include <linux/bitops.h>
>>>    #include <linux/delay.h>
>>> @@ -252,6 +253,115 @@ static int imx7_reset_assert_imx8mq(struct reset_ctl *rst)
>>>        return 0;
>>>    }
>>>
>>> +enum imx8mp_src_registers {
>>> +     SRC_SUPERMIX_RCR        = 0x0018,
>>> +     SRC_AUDIOMIX_RCR        = 0x001c,
>>> +     SRC_MLMIX_RCR           = 0x0028,
>>> +     SRC_GPU2D_RCR           = 0x0038,
>>> +     SRC_GPU3D_RCR           = 0x003c,
>>> +     SRC_VPU_G1_RCR          = 0x0048,
>>> +     SRC_VPU_G2_RCR          = 0x004c,
>>> +     SRC_VPUVC8KE_RCR        = 0x0050,
>>> +     SRC_NOC_RCR             = 0x0054,
>>> +};
>>
>> This seems copied from Linux, include Linux commit ID as of which this
>> was imported from in commit message.
> 
> Do you expect imx8mp_src_registers to change? I expect them to be the
> same, so does Linux commit ID add any value here?

The import commit ID makes it easier to cherry-pick follow up patches or 
check for changes. If that commit ID is missing, that task is harder.

[PATCH 3/7] imx8mp: power-domain: Add PCIe support

[Sumit Garg]

Pre-requisite to enable PCIe support on iMX8MP SoC.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
index e2d772c5ec7..62145e0261b 100644
--- a/drivers/power/domain/imx8mp-hsiomix.c
+++ b/drivers/power/domain/imx8mp-hsiomix.c
@@ -16,14 +16,19 @@
 #define GPR_REG0		0x0
 #define  PCIE_CLOCK_MODULE_EN	BIT(0)
 #define  USB_CLOCK_MODULE_EN	BIT(1)
+#define  PCIE_PHY_APB_RST	BIT(4)
+#define  PCIE_PHY_INIT_RST	BIT(5)
 
 struct imx8mp_hsiomix_priv {
 	void __iomem *base;
 	struct clk clk_usb;
+	struct clk clk_pcie;
 	struct power_domain pd_bus;
 	struct power_domain pd_usb;
+	struct power_domain pd_pcie;
 	struct power_domain pd_usb_phy1;
 	struct power_domain pd_usb_phy2;
+	struct power_domain pd_pcie_phy;
 };
 
 static int imx8mp_hsiomix_on(struct power_domain *power_domain)
@@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
 		domain = &priv->pd_usb_phy1;
 	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
 		domain = &priv->pd_usb_phy2;
+	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
+		domain = &priv->pd_pcie;
+	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
+		domain = &priv->pd_pcie_phy;
 	} else {
 		ret = -EINVAL;
 		goto err_pd;
@@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
 
 	ret = clk_enable(&priv->clk_usb);
 	if (ret)
-		goto err_clk;
+		goto err_clk_usb;
+
+	ret = clk_enable(&priv->clk_pcie);
+	if (ret)
+		goto err_clk_pcie;
 
 	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
 		setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+		setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+		setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
+						    PCIE_PHY_INIT_RST);
 
 	return 0;
 
-err_clk:
+err_clk_pcie:
+	clk_disable(&priv->clk_usb);
+err_clk_usb:
 	power_domain_off(domain);
 err_pd:
 	power_domain_off(&priv->pd_bus);
@@ -75,8 +95,14 @@ static int imx8mp_hsiomix_off(struct power_domain *power_domain)
 
 	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
 		clrbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+		clrbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+		clrbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
+						    PCIE_PHY_INIT_RST);
 
 	clk_disable(&priv->clk_usb);
+	clk_disable(&priv->clk_pcie);
 
 	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
 		power_domain_off(&priv->pd_usb);
@@ -84,6 +110,10 @@ static int imx8mp_hsiomix_off(struct power_domain *power_domain)
 		power_domain_off(&priv->pd_usb_phy1);
 	else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2)
 		power_domain_off(&priv->pd_usb_phy2);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+		power_domain_off(&priv->pd_usb_phy2);
+	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+		power_domain_off(&priv->pd_usb_phy2);
 
 	power_domain_off(&priv->pd_bus);
 
@@ -109,6 +139,10 @@ static int imx8mp_hsiomix_probe(struct udevice *dev)
 	if (ret < 0)
 		return ret;
 
+	ret = clk_get_by_name(dev, "pcie", &priv->clk_pcie);
+	if (ret < 0)
+		return ret;
+
 	ret = power_domain_get_by_name(dev, &priv->pd_bus, "bus");
 	if (ret < 0)
 		return ret;
@@ -125,8 +159,20 @@ static int imx8mp_hsiomix_probe(struct udevice *dev)
 	if (ret < 0)
 		goto err_pd_usb_phy2;
 
+	ret = power_domain_get_by_name(dev, &priv->pd_pcie, "pcie");
+	if (ret < 0)
+		goto err_pd_pcie;
+
+	ret = power_domain_get_by_name(dev, &priv->pd_pcie_phy, "pcie-phy");
+	if (ret < 0)
+		goto err_pd_pcie_phy;
+
 	return 0;
 
+err_pd_pcie_phy:
+	power_domain_free(&priv->pd_pcie);
+err_pd_pcie:
+	power_domain_free(&priv->pd_usb_phy2);
 err_pd_usb_phy2:
 	power_domain_free(&priv->pd_usb_phy1);
 err_pd_usb_phy1:
-- 
2.34.1

Re: [PATCH 3/7] imx8mp: power-domain: Add PCIe support

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> Pre-requisite to enable PCIe support on iMX8MP SoC.

This commit message is useless, write a proper one.

> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>   drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
>   1 file changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
> index e2d772c5ec7..62145e0261b 100644
> --- a/drivers/power/domain/imx8mp-hsiomix.c
> +++ b/drivers/power/domain/imx8mp-hsiomix.c
> @@ -16,14 +16,19 @@
>   #define GPR_REG0		0x0
>   #define  PCIE_CLOCK_MODULE_EN	BIT(0)
>   #define  USB_CLOCK_MODULE_EN	BIT(1)
> +#define  PCIE_PHY_APB_RST	BIT(4)
> +#define  PCIE_PHY_INIT_RST	BIT(5)
>   
>   struct imx8mp_hsiomix_priv {
>   	void __iomem *base;
>   	struct clk clk_usb;
> +	struct clk clk_pcie;
>   	struct power_domain pd_bus;
>   	struct power_domain pd_usb;
> +	struct power_domain pd_pcie;
>   	struct power_domain pd_usb_phy1;
>   	struct power_domain pd_usb_phy2;
> +	struct power_domain pd_pcie_phy;
>   };
>   
>   static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> @@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>   		domain = &priv->pd_usb_phy1;
>   	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
>   		domain = &priv->pd_usb_phy2;
> +	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
> +		domain = &priv->pd_pcie;
> +	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
> +		domain = &priv->pd_pcie_phy;
>   	} else {
>   		ret = -EINVAL;
>   		goto err_pd;
> @@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>   
>   	ret = clk_enable(&priv->clk_usb);
>   	if (ret)
> -		goto err_clk;
> +		goto err_clk_usb;
> +
> +	ret = clk_enable(&priv->clk_pcie);
> +	if (ret)
> +		goto err_clk_pcie;

Does this mean that when USB power domains get enabled, PCIe clock are 
also enabled ? Why ?

What if the PCIe clock enable fails, do USB clock remain enabled ?

>   	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
>   		setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> +	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
> +		setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
> +	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
> +		setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
> +						    PCIE_PHY_INIT_RST);

Shouldn't the reset bits be cleared here ?

[...]

Re: [PATCH 3/7] imx8mp: power-domain: Add PCIe support

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > Pre-requisite to enable PCIe support on iMX8MP SoC.
>
> This commit message is useless, write a proper one.
>

How about the following?

    Add support for GPCv2 power domains and clock handling
    for PCIe and PCIe PHY. It is required to enable PCIe support
    on the iMX8MP SoC.

> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >   drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
> >   1 file changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
> > index e2d772c5ec7..62145e0261b 100644
> > --- a/drivers/power/domain/imx8mp-hsiomix.c
> > +++ b/drivers/power/domain/imx8mp-hsiomix.c
> > @@ -16,14 +16,19 @@
> >   #define GPR_REG0            0x0
> >   #define  PCIE_CLOCK_MODULE_EN       BIT(0)
> >   #define  USB_CLOCK_MODULE_EN        BIT(1)
> > +#define  PCIE_PHY_APB_RST    BIT(4)
> > +#define  PCIE_PHY_INIT_RST   BIT(5)
> >
> >   struct imx8mp_hsiomix_priv {
> >       void __iomem *base;
> >       struct clk clk_usb;
> > +     struct clk clk_pcie;
> >       struct power_domain pd_bus;
> >       struct power_domain pd_usb;
> > +     struct power_domain pd_pcie;
> >       struct power_domain pd_usb_phy1;
> >       struct power_domain pd_usb_phy2;
> > +     struct power_domain pd_pcie_phy;
> >   };
> >
> >   static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> > @@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >               domain = &priv->pd_usb_phy1;
> >       } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
> >               domain = &priv->pd_usb_phy2;
> > +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
> > +             domain = &priv->pd_pcie;
> > +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
> > +             domain = &priv->pd_pcie_phy;
> >       } else {
> >               ret = -EINVAL;
> >               goto err_pd;
> > @@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >
> >       ret = clk_enable(&priv->clk_usb);
> >       if (ret)
> > -             goto err_clk;
> > +             goto err_clk_usb;
> > +
> > +     ret = clk_enable(&priv->clk_pcie);
> > +     if (ret)
> > +             goto err_clk_pcie;
>
> Does this mean that when USB power domains get enabled, PCIe clock are
> also enabled ? Why ?
>
> What if the PCIe clock enable fails, do USB clock remain enabled ?

Let me gate them behind corresponding power domain IDs.

>
> >       if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> >               setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> > +     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
> > +             setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
> > +     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
> > +             setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
> > +                                                 PCIE_PHY_INIT_RST);
>
> Shouldn't the reset bits be cleared here ?
>

Although I can't find their reference in the TRM, as per Linux commit
[1], setting the reset bit is actually deassertion of PCIe PHY reset.
You can think of it like an active low signal.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5506018d3dec41e6678efb92b836586e9ee1d628

-Sumit

Re: [PATCH 3/7] imx8mp: power-domain: Add PCIe support

[Marek Vasut]

On 2/21/24 07:01, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/20/24 14:10, Sumit Garg wrote:
>>> Pre-requisite to enable PCIe support on iMX8MP SoC.
>>
>> This commit message is useless, write a proper one.
>>
> 
> How about the following?
> 
>      Add support for GPCv2 power domains and clock handling
>      for PCIe and PCIe PHY. It is required to enable PCIe support
>      on the iMX8MP SoC.

The second sentence is redundant.

>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>> ---
>>>    drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++--
>>>    1 file changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
>>> index e2d772c5ec7..62145e0261b 100644
>>> --- a/drivers/power/domain/imx8mp-hsiomix.c
>>> +++ b/drivers/power/domain/imx8mp-hsiomix.c
>>> @@ -16,14 +16,19 @@
>>>    #define GPR_REG0            0x0
>>>    #define  PCIE_CLOCK_MODULE_EN       BIT(0)
>>>    #define  USB_CLOCK_MODULE_EN        BIT(1)
>>> +#define  PCIE_PHY_APB_RST    BIT(4)
>>> +#define  PCIE_PHY_INIT_RST   BIT(5)
>>>
>>>    struct imx8mp_hsiomix_priv {
>>>        void __iomem *base;
>>>        struct clk clk_usb;
>>> +     struct clk clk_pcie;
>>>        struct power_domain pd_bus;
>>>        struct power_domain pd_usb;
>>> +     struct power_domain pd_pcie;
>>>        struct power_domain pd_usb_phy1;
>>>        struct power_domain pd_usb_phy2;
>>> +     struct power_domain pd_pcie_phy;
>>>    };
>>>
>>>    static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>>> @@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>>>                domain = &priv->pd_usb_phy1;
>>>        } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
>>>                domain = &priv->pd_usb_phy2;
>>> +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
>>> +             domain = &priv->pd_pcie;
>>> +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
>>> +             domain = &priv->pd_pcie_phy;
>>>        } else {
>>>                ret = -EINVAL;
>>>                goto err_pd;
>>> @@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>>>
>>>        ret = clk_enable(&priv->clk_usb);
>>>        if (ret)
>>> -             goto err_clk;
>>> +             goto err_clk_usb;
>>> +
>>> +     ret = clk_enable(&priv->clk_pcie);
>>> +     if (ret)
>>> +             goto err_clk_pcie;
>>
>> Does this mean that when USB power domains get enabled, PCIe clock are
>> also enabled ? Why ?
>>
>> What if the PCIe clock enable fails, do USB clock remain enabled ?
> 
> Let me gate them behind corresponding power domain IDs.
> 
>>
>>>        if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
>>>                setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
>>> +     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
>>> +             setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
>>> +     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
>>> +             setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
>>> +                                                 PCIE_PHY_INIT_RST);
>>
>> Shouldn't the reset bits be cleared here ?
>>
> 
> Although I can't find their reference in the TRM, as per Linux commit
> [1], setting the reset bit is actually deassertion of PCIe PHY reset.
> You can think of it like an active low signal.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5506018d3dec41e6678efb92b836586e9ee1d628

Please add this ^ comment into the code.

[PATCH 4/7] imx8mp: power-domain: Expose high performance PLL clock

[Sumit Garg]

PCIe PHY can use it when there is no external refclock provided.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/power/domain/imx8mp-hsiomix.c | 79 +++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
index 62145e0261b..4cefe642724 100644
--- a/drivers/power/domain/imx8mp-hsiomix.c
+++ b/drivers/power/domain/imx8mp-hsiomix.c
@@ -9,6 +9,8 @@
 #include <dm.h>
 #include <dm/device.h>
 #include <dm/device_compat.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
 #include <power-domain-uclass.h>
 
 #include <dt-bindings/power/imx8mp-power.h>
@@ -18,6 +20,15 @@
 #define  USB_CLOCK_MODULE_EN	BIT(1)
 #define  PCIE_PHY_APB_RST	BIT(4)
 #define  PCIE_PHY_INIT_RST	BIT(5)
+#define GPR_REG1		0x4
+#define  PLL_LOCK		BIT(13)
+#define GPR_REG2		0x8
+#define  P_PLL_MASK		GENMASK(5, 0)
+#define  M_PLL_MASK		GENMASK(15, 6)
+#define  S_PLL_MASK		GENMASK(18, 16)
+#define GPR_REG3		0xc
+#define  PLL_CKE		BIT(17)
+#define  PLL_RST		BIT(31)
 
 struct imx8mp_hsiomix_priv {
 	void __iomem *base;
@@ -31,6 +42,53 @@ struct imx8mp_hsiomix_priv {
 	struct power_domain pd_pcie_phy;
 };
 
+static int hsio_pll_enable(struct udevice *dev)
+{
+	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
+	unsigned long start;
+	u32 val;
+
+	/* Setup HSIO PLL */
+	val = readl(priv->base + GPR_REG2);
+	val &= ~(P_PLL_MASK | M_PLL_MASK | S_PLL_MASK);
+	val |= (FIELD_PREP(P_PLL_MASK, 12) | FIELD_PREP(M_PLL_MASK, 800) |
+		FIELD_PREP(S_PLL_MASK, 4));
+	writel(val, priv->base + GPR_REG2);
+
+	/* de-assert PLL reset */
+	setbits_le32(priv->base + GPR_REG3, PLL_RST);
+
+	/* enable PLL */
+	setbits_le32(priv->base + GPR_REG3, PLL_CKE);
+
+	/* Check if PLL is locked */
+	start = get_timer(0);
+	for (;;) {
+		if (readl(priv->base + GPR_REG1) & PLL_LOCK)
+			break;
+
+		if (get_timer(start) > 100) {
+			dev_err(dev, "failed to lock HSIO PLL\n");
+			return -ETIMEDOUT;
+		}
+
+		udelay(10);
+	}
+
+	return 0;
+}
+
+static void hsio_pll_disable(struct udevice *dev)
+{
+	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
+
+	/* de-assert PLL reset */
+	clrbits_le32(priv->base + GPR_REG3, PLL_RST);
+
+	/* enable PLL */
+	clrbits_le32(priv->base + GPR_REG3, PLL_CKE);
+}
+
 static int imx8mp_hsiomix_on(struct power_domain *power_domain)
 {
 	struct udevice *dev = power_domain->dev;
@@ -69,16 +127,23 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
 	if (ret)
 		goto err_clk_pcie;
 
-	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
+	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {
 		setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
-	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
 		setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
-	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
 		setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
 						    PCIE_PHY_INIT_RST);
 
+		ret = hsio_pll_enable(dev);
+		if (ret)
+			goto err_hsio_pll;
+	}
+
 	return 0;
 
+err_hsio_pll:
+	clk_disable(&priv->clk_pcie);
 err_clk_pcie:
 	clk_disable(&priv->clk_usb);
 err_clk_usb:
@@ -93,13 +158,15 @@ static int imx8mp_hsiomix_off(struct power_domain *power_domain)
 	struct udevice *dev = power_domain->dev;
 	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
 
-	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
+	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {
 		clrbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
-	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
+	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
 		clrbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
-	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
+	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
 		clrbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
 						    PCIE_PHY_INIT_RST);
+		hsio_pll_disable(dev);
+	}
 
 	clk_disable(&priv->clk_usb);
 	clk_disable(&priv->clk_pcie);
-- 
2.34.1

Re: [PATCH 4/7] imx8mp: power-domain: Expose high performance PLL clock

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> PCIe PHY can use it when there is no external refclock provided.

Commit message needs to be fixed.

> +static int hsio_pll_enable(struct udevice *dev)
> +{
> +	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
> +	unsigned long start;
> +	u32 val;
> +
> +	/* Setup HSIO PLL */
> +	val = readl(priv->base + GPR_REG2);
> +	val &= ~(P_PLL_MASK | M_PLL_MASK | S_PLL_MASK);
> +	val |= (FIELD_PREP(P_PLL_MASK, 12) | FIELD_PREP(M_PLL_MASK, 800) |
> +		FIELD_PREP(S_PLL_MASK, 4));
> +	writel(val, priv->base + GPR_REG2);

clrsetbits_le32()

> +	/* de-assert PLL reset */
> +	setbits_le32(priv->base + GPR_REG3, PLL_RST);
> +
> +	/* enable PLL */
> +	setbits_le32(priv->base + GPR_REG3, PLL_CKE);
> +
> +	/* Check if PLL is locked */
> +	start = get_timer(0);

wait_for_bit() or readl_poll_timeout()

> +	for (;;) {
> +		if (readl(priv->base + GPR_REG1) & PLL_LOCK)
> +			break;
> +
> +		if (get_timer(start) > 100) {
> +			dev_err(dev, "failed to lock HSIO PLL\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		udelay(10);
> +	}
> +
> +	return 0;
> +}
> +
> +static void hsio_pll_disable(struct udevice *dev)
> +{
> +	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
> +
> +	/* de-assert PLL reset */
> +	clrbits_le32(priv->base + GPR_REG3, PLL_RST);
> +
> +	/* enable PLL */
> +	clrbits_le32(priv->base + GPR_REG3, PLL_CKE);
> +}
> +
>   static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>   {
>   	struct udevice *dev = power_domain->dev;
> @@ -69,16 +127,23 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>   	if (ret)
>   		goto err_clk_pcie;
>   
> -	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> +	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {
>   		setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> -	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
> +	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
>   		setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
> -	else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
> +	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
>   		setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
>   						    PCIE_PHY_INIT_RST);
>   
> +		ret = hsio_pll_enable(dev);

Is this how Linux handles this PLL ?

Seems like this should be either syscon or clock driver .

[...]

Re: [PATCH 4/7] imx8mp: power-domain: Expose high performance PLL clock

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > PCIe PHY can use it when there is no external refclock provided.
>
> Commit message needs to be fixed.

How about the following?

    Expose high performance PLL clock, so the PCIe PHY can
    use it when there is no external refclock provided.

>
> > +static int hsio_pll_enable(struct udevice *dev)
> > +{
> > +     struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
> > +     unsigned long start;
> > +     u32 val;
> > +
> > +     /* Setup HSIO PLL */
> > +     val = readl(priv->base + GPR_REG2);
> > +     val &= ~(P_PLL_MASK | M_PLL_MASK | S_PLL_MASK);
> > +     val |= (FIELD_PREP(P_PLL_MASK, 12) | FIELD_PREP(M_PLL_MASK, 800) |
> > +             FIELD_PREP(S_PLL_MASK, 4));
> > +     writel(val, priv->base + GPR_REG2);
>
> clrsetbits_le32()

Ack

>
> > +     /* de-assert PLL reset */
> > +     setbits_le32(priv->base + GPR_REG3, PLL_RST);
> > +
> > +     /* enable PLL */
> > +     setbits_le32(priv->base + GPR_REG3, PLL_CKE);
> > +
> > +     /* Check if PLL is locked */
> > +     start = get_timer(0);
>
> wait_for_bit() or readl_poll_timeout()

Let me use readl_poll_timeout() instead.

>
> > +     for (;;) {
> > +             if (readl(priv->base + GPR_REG1) & PLL_LOCK)
> > +                     break;
> > +
> > +             if (get_timer(start) > 100) {
> > +                     dev_err(dev, "failed to lock HSIO PLL\n");
> > +                     return -ETIMEDOUT;
> > +             }
> > +
> > +             udelay(10);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void hsio_pll_disable(struct udevice *dev)
> > +{
> > +     struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
> > +
> > +     /* de-assert PLL reset */
> > +     clrbits_le32(priv->base + GPR_REG3, PLL_RST);
> > +
> > +     /* enable PLL */
> > +     clrbits_le32(priv->base + GPR_REG3, PLL_CKE);
> > +}
> > +
> >   static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >   {
> >       struct udevice *dev = power_domain->dev;
> > @@ -69,16 +127,23 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
> >       if (ret)
> >               goto err_clk_pcie;
> >
> > -     if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> > +     if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {
> >               setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> > -     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
> > +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
> >               setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
> > -     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
> > +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
> >               setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
> >                                                   PCIE_PHY_INIT_RST);
> >
> > +             ret = hsio_pll_enable(dev);
>
> Is this how Linux handles this PLL ?
>
> Seems like this should be either syscon or clock driver .

It isn't similar to what Linux does but I can't find suitable
infrastructure in U-Boot to expose it as a regular clock. Are there
any APIs available similar to devm_of_clk_add_hw_provider() in Linux?

-Sumit

>
> [...]

Re: [PATCH 4/7] imx8mp: power-domain: Expose high performance PLL clock

[Marek Vasut]

On 2/21/24 07:14, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/20/24 14:10, Sumit Garg wrote:
>>> PCIe PHY can use it when there is no external refclock provided.
>>
>> Commit message needs to be fixed.
> 
> How about the following?
> 
>      Expose high performance PLL clock, so the PCIe PHY can
>      use it when there is no external refclock provided.

If this code is imported from Linux
2cbee26e5d59 ("soc: imx: imx8mp-blk-ctrl: expose high performance PLL 
clock")
then just include that reference too .

[...]

>>> @@ -69,16 +127,23 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
>>>        if (ret)
>>>                goto err_clk_pcie;
>>>
>>> -     if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
>>> +     if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {
>>>                setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
>>> -     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)
>>> +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {
>>>                setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
>>> -     else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)
>>> +     } else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {
>>>                setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
>>>                                                    PCIE_PHY_INIT_RST);
>>>
>>> +             ret = hsio_pll_enable(dev);
>>
>> Is this how Linux handles this PLL ?
>>
>> Seems like this should be either syscon or clock driver .
> 
> It isn't similar to what Linux does but I can't find suitable
> infrastructure in U-Boot to expose it as a regular clock. Are there
> any APIs available similar to devm_of_clk_add_hw_provider() in Linux?

Have a look at the very end of:

drivers/clk/renesas/clk-rcar-gen3.c

that registers clock and reset drivers for the same IP at the same 
address. In this case, you would register power domain and clock drivers 
instead .

[PATCH 5/7] phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY

[Sumit Garg]

Add initial support for i.MX8M{M/P} PCIe PHY. On i.MX8M{M/P} SoCs PCIe
PHY initialization moved to this standalone PHY driver.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/phy/Kconfig          |   9 ++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-imx8m-pcie.c | 246 +++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/phy/phy-imx8m-pcie.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 60138beca49..110ec8f5008 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -284,6 +284,15 @@ config PHY_IMX8MQ_USB
 	help
 	  Support the USB3.0 PHY in NXP i.MX8MQ or i.MX8MP SoC
 
+config PHY_IMX8M_PCIE
+	bool "NXP i.MX8MM/i.MX8MP PCIe PHY Driver"
+	depends on PHY
+	depends on IMX8MM || IMX8MP
+	help
+	  Support the PCIe PHY in NXP i.MX8MM or i.MX8MP SoC
+
+	  This PHY is found on i.MX8M devices supporting PCIe.
+
 config PHY_XILINX_ZYNQMP
 	tristate "Xilinx ZynqMP PHY driver"
 	depends on PHY && ARCH_ZYNQMP
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 2e8723186c0..7a2b764492b 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_PHY_DA8XX_USB) += phy-da8xx-usb.o
 obj-$(CONFIG_PHY_MTK_TPHY) += phy-mtk-tphy.o
 obj-$(CONFIG_PHY_NPCM_USB) += phy-npcm-usb.o
 obj-$(CONFIG_PHY_IMX8MQ_USB) += phy-imx8mq-usb.o
+obj-$(CONFIG_PHY_IMX8M_PCIE) += phy-imx8m-pcie.o
 obj-$(CONFIG_PHY_XILINX_ZYNQMP) += phy-zynqmp.o
 obj-y += cadence/
 obj-y += ti/
diff --git a/drivers/phy/phy-imx8m-pcie.c b/drivers/phy/phy-imx8m-pcie.c
new file mode 100644
index 00000000000..d1ad42ff339
--- /dev/null
+++ b/drivers/phy/phy-imx8m-pcie.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 Linaro Ltd.
+ *
+ * Derived from Linux counterpart driver
+ */
+
+#include <asm/io.h>
+#include <clk.h>
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <generic-phy.h>
+#include <linux/bitfield.h>
+#include <linux/clk-provider.h>
+#include <linux/iopoll.h>
+#include <syscon.h>
+#include <regmap.h>
+#include <reset.h>
+
+#include <dt-bindings/phy/phy-imx8-pcie.h>
+
+#define IMX8MM_PCIE_PHY_CMN_REG061	0x184
+#define  ANA_PLL_CLK_OUT_TO_EXT_IO_EN	BIT(0)
+#define IMX8MM_PCIE_PHY_CMN_REG062	0x188
+#define  ANA_PLL_CLK_OUT_TO_EXT_IO_SEL	BIT(3)
+#define IMX8MM_PCIE_PHY_CMN_REG063	0x18C
+#define  AUX_PLL_REFCLK_SEL_SYS_PLL	GENMASK(7, 6)
+#define IMX8MM_PCIE_PHY_CMN_REG064	0x190
+#define  ANA_AUX_RX_TX_SEL_TX		BIT(7)
+#define  ANA_AUX_RX_TERM_GND_EN		BIT(3)
+#define  ANA_AUX_TX_TERM		BIT(2)
+#define IMX8MM_PCIE_PHY_CMN_REG065	0x194
+#define  ANA_AUX_RX_TERM		(BIT(7) | BIT(4))
+#define  ANA_AUX_TX_LVL			GENMASK(3, 0)
+#define IMX8MM_PCIE_PHY_CMN_REG075	0x1D4
+#define  ANA_PLL_DONE			0x3
+#define PCIE_PHY_TRSV_REG5		0x414
+#define PCIE_PHY_TRSV_REG6		0x418
+
+#define IMX8MM_GPR_PCIE_REF_CLK_SEL	GENMASK(25, 24)
+#define IMX8MM_GPR_PCIE_REF_CLK_PLL	FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x3)
+#define IMX8MM_GPR_PCIE_REF_CLK_EXT	FIELD_PREP(IMX8MM_GPR_PCIE_REF_CLK_SEL, 0x2)
+#define IMX8MM_GPR_PCIE_AUX_EN		BIT(19)
+#define IMX8MM_GPR_PCIE_CMN_RST		BIT(18)
+#define IMX8MM_GPR_PCIE_POWER_OFF	BIT(17)
+#define IMX8MM_GPR_PCIE_SSC_EN		BIT(16)
+#define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE	BIT(9)
+
+#define IOMUXC_GPR14_OFFSET		0x38
+
+enum imx8_pcie_phy_type {
+	IMX8MM,
+	IMX8MP,
+};
+
+struct imx8_pcie_phy_drvdata {
+	const	char			*gpr;
+	enum	imx8_pcie_phy_type	variant;
+};
+
+struct imx8_pcie_phy {
+	ulong			base;
+	struct regmap		*iomuxc_gpr;
+	struct reset_ctl	perst;
+	struct reset_ctl	reset;
+	u32			refclk_pad_mode;
+	u32			tx_deemph_gen1;
+	u32			tx_deemph_gen2;
+	bool			clkreq_unused;
+	const struct imx8_pcie_phy_drvdata	*drvdata;
+};
+
+static int imx8_pcie_phy_power_on(struct phy *phy)
+{
+	int ret;
+	u32 val, pad_mode;
+	struct imx8_pcie_phy *imx8_phy = dev_get_priv(phy->dev);
+
+	pad_mode = imx8_phy->refclk_pad_mode;
+	switch (imx8_phy->drvdata->variant) {
+	case IMX8MM:
+		reset_assert(&imx8_phy->reset);
+
+		/* Tune PHY de-emphasis setting to pass PCIe compliance. */
+		if (imx8_phy->tx_deemph_gen1)
+			writel(imx8_phy->tx_deemph_gen1,
+			       imx8_phy->base + PCIE_PHY_TRSV_REG5);
+		if (imx8_phy->tx_deemph_gen2)
+			writel(imx8_phy->tx_deemph_gen2,
+			       imx8_phy->base + PCIE_PHY_TRSV_REG6);
+		break;
+	case IMX8MP: /* Do nothing. */
+		break;
+	}
+
+	if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ||
+	    pad_mode == IMX8_PCIE_REFCLK_PAD_UNUSED) {
+		/* Configure the pad as input */
+		val = readl(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+		writel(val & ~ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+	} else {
+		/* Configure the PHY to output the refclock via pad */
+		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG061);
+	}
+
+	if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT ||
+	    pad_mode == IMX8_PCIE_REFCLK_PAD_UNUSED) {
+		/* Source clock from SoC internal PLL */
+		writel(ANA_PLL_CLK_OUT_TO_EXT_IO_SEL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG062);
+		writel(AUX_PLL_REFCLK_SEL_SYS_PLL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG063);
+		val = ANA_AUX_RX_TX_SEL_TX | ANA_AUX_TX_TERM;
+		writel(val | ANA_AUX_RX_TERM_GND_EN,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG064);
+		writel(ANA_AUX_RX_TERM | ANA_AUX_TX_LVL,
+		       imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG065);
+	}
+
+	/* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't hooked */
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE,
+			   imx8_phy->clkreq_unused ?
+			   0 : IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8MM_GPR_PCIE_AUX_EN,
+			   IMX8MM_GPR_PCIE_AUX_EN);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8MM_GPR_PCIE_POWER_OFF, 0);
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8MM_GPR_PCIE_SSC_EN, 0);
+
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8MM_GPR_PCIE_REF_CLK_SEL,
+			   pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT ?
+			   IMX8MM_GPR_PCIE_REF_CLK_EXT :
+			   IMX8MM_GPR_PCIE_REF_CLK_PLL);
+	udelay(200);
+
+	/* Do the PHY common block reset */
+	regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8MM_GPR_PCIE_CMN_RST,
+			   IMX8MM_GPR_PCIE_CMN_RST);
+
+	switch (imx8_phy->drvdata->variant) {
+	case IMX8MP:
+		reset_deassert(&imx8_phy->perst);
+		fallthrough;
+	case IMX8MM:
+		reset_deassert(&imx8_phy->reset);
+		udelay(500);
+		break;
+	}
+
+	/* Polling to check the phy is ready or not. */
+	ret = readl_poll_timeout(imx8_phy->base + IMX8MM_PCIE_PHY_CMN_REG075,
+				 val, val == ANA_PLL_DONE, 20000);
+	return ret;
+}
+
+static const struct phy_ops imx8_pcie_phy_ops = {
+	.power_on	= imx8_pcie_phy_power_on,
+};
+
+static const struct imx8_pcie_phy_drvdata imx8mm_drvdata = {
+	.gpr = "fsl,imx8mm-iomuxc-gpr",
+	.variant = IMX8MM,
+};
+
+static const struct imx8_pcie_phy_drvdata imx8mp_drvdata = {
+	.gpr = "fsl,imx8mp-iomuxc-gpr",
+	.variant = IMX8MP,
+};
+
+static const struct udevice_id imx8_pcie_phy_of_match[] = {
+	{.compatible = "fsl,imx8mm-pcie-phy", .data = (ulong)&imx8mm_drvdata, },
+	{.compatible = "fsl,imx8mp-pcie-phy", .data = (ulong)&imx8mp_drvdata, },
+	{ },
+};
+
+static int imx8_pcie_phy_probe(struct udevice *dev)
+{
+	struct imx8_pcie_phy *imx8_phy = dev_get_priv(dev);
+	ofnode gpr;
+	int ret = 0;
+
+	imx8_phy->drvdata = (void *)dev_get_driver_data(dev);
+	imx8_phy->base = dev_read_addr(dev);
+	if (!imx8_phy->base)
+		return -EINVAL;
+
+	/* get PHY refclk pad mode */
+	dev_read_u32(dev, "fsl,refclk-pad-mode", &imx8_phy->refclk_pad_mode);
+
+	if (dev_read_u32(dev, "fsl,tx-deemph-gen1", &imx8_phy->tx_deemph_gen1))
+		imx8_phy->tx_deemph_gen1 = 0;
+
+	if (dev_read_u32(dev, "fsl,tx-deemph-gen2", &imx8_phy->tx_deemph_gen2))
+		imx8_phy->tx_deemph_gen2 = 0;
+
+	if (dev_read_bool(dev, "fsl,clkreq-unsupported"))
+		imx8_phy->clkreq_unused = true;
+	else
+		imx8_phy->clkreq_unused = false;
+
+	/* Grab GPR config register range */
+	gpr = ofnode_by_compatible(ofnode_null(), imx8_phy->drvdata->gpr);
+	if (ofnode_equal(gpr, ofnode_null())) {
+		dev_err(dev, "unable to find GPR node\n");
+		return -ENODEV;
+	}
+
+	imx8_phy->iomuxc_gpr = syscon_node_to_regmap(gpr);
+	if (IS_ERR(imx8_phy->iomuxc_gpr)) {
+		dev_err(dev, "unable to find iomuxc registers\n");
+		return PTR_ERR(imx8_phy->iomuxc_gpr);
+	}
+
+	ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
+	if (ret) {
+		dev_err(dev, "Failed to get PCIEPHY reset control\n");
+		return ret;
+	}
+
+	if (imx8_phy->drvdata->variant == IMX8MP) {
+		ret = reset_get_by_name(dev, "perst", &imx8_phy->perst);
+		if (ret) {
+			dev_err(dev,
+				"Failed to get PCIEPHY PHY PERST control\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+U_BOOT_DRIVER(nxp_imx8_pcie_phy) = {
+	.name = "nxp_imx8_pcie_phy",
+	.id = UCLASS_PHY,
+	.of_match = imx8_pcie_phy_of_match,
+	.probe = imx8_pcie_phy_probe,
+	.ops = &imx8_pcie_phy_ops,
+	.priv_auto	= sizeof(struct imx8_pcie_phy),
+};
-- 
2.34.1

Re: [PATCH 5/7] phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> Add initial support for i.MX8M{M/P} PCIe PHY. On i.MX8M{M/P} SoCs PCIe
> PHY initialization moved to this standalone PHY driver.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

Is this based on Linux ? If so, include Linux commit ID from which the 
code was imported.

Re: [PATCH 5/7] phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > Add initial support for i.MX8M{M/P} PCIe PHY. On i.MX8M{M/P} SoCs PCIe
> > PHY initialization moved to this standalone PHY driver.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> Is this based on Linux ? If so, include Linux commit ID from which the
> code was imported.

Yeah it is derived from the corresponding Linux driver (see header for
phy-imx8m-pcie.c file). I can add the corresponding Linux version
(v6.8-rc3) in the commit message.

-Sumit

Re: [PATCH 5/7] phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY

[Marek Vasut]

On 2/21/24 07:17, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/20/24 14:10, Sumit Garg wrote:
>>> Add initial support for i.MX8M{M/P} PCIe PHY. On i.MX8M{M/P} SoCs PCIe
>>> PHY initialization moved to this standalone PHY driver.
>>>
>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>
>> Is this based on Linux ? If so, include Linux commit ID from which the
>> code was imported.
> 
> Yeah it is derived from the corresponding Linux driver (see header for
> phy-imx8m-pcie.c file). I can add the corresponding Linux version
> (v6.8-rc3) in the commit message.

Please do, this is useful for further synchronizations with Linux.
I can imagine MX9x support will be one such synchronization.

[PATCH 6/7] pci: Add DW PCIe controller support for iMX8MP SoC

[Sumit Garg]

pcie_imx doesn't seem to share any useful code for iMX8 SoC and it is
tied to quite old port of pcie_designware driver from Linux which
suffices only iMX6 specific needs.

But currently we have the common DWC specific bits which alligns pretty
well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
add support for other iMX8 variants to this driver.

iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
can reuse the generic PHY infrastructure to power on PCIe PHY.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/pci/Kconfig        |   8 +
 drivers/pci/Makefile       |   1 +
 drivers/pci/pcie_dw_imx8.c | 348 +++++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+)
 create mode 100644 drivers/pci/pcie_dw_imx8.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 463ec47eb92..b7c7922b091 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -413,4 +413,12 @@ config PCIE_STARFIVE_JH7110
 	  Say Y here if you want to enable PLDA XpressRich PCIe controller
 	  support on StarFive JH7110 SoC.
 
+config PCIE_DW_IMX8
+	bool "i.MX8 PCIe support"
+	depends on ARCH_IMX8M
+	select PCIE_DW_COMMON
+	help
+	  Say Y here if you want to enable DW PCIe controller support on
+	  iMX8 SoCs.
+
 endif
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 72ef8b4bc77..cddbb902095 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -53,3 +53,4 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o
 obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
 obj-$(CONFIG_PCIE_PLDA_COMMON) += pcie_plda_common.o
 obj-$(CONFIG_PCIE_STARFIVE_JH7110) += pcie_starfive_jh7110.o
+obj-$(CONFIG_PCIE_DW_IMX8) += pcie_dw_imx8.o
diff --git a/drivers/pci/pcie_dw_imx8.c b/drivers/pci/pcie_dw_imx8.c
new file mode 100644
index 00000000000..b9921644765
--- /dev/null
+++ b/drivers/pci/pcie_dw_imx8.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024 Linaro Ltd.
+ *
+ * Author: Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#include <asm/io.h>
+#include <asm-generic/gpio.h>
+#include <clk.h>
+#include <dm.h>
+#include <dm/device_compat.h>
+#include <generic-phy.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <log.h>
+#include <pci.h>
+#include <regmap.h>
+#include <reset.h>
+#include <syscon.h>
+#include <time.h>
+
+#include "pcie_dw_common.h"
+
+#define PCIE_LINK_CAPABILITY		0x7c
+#define TARGET_LINK_SPEED_MASK		0xf
+#define LINK_SPEED_GEN_1		0x1
+#define LINK_SPEED_GEN_2		0x2
+#define LINK_SPEED_GEN_3		0x3
+
+#define PCIE_MISC_CONTROL_1_OFF		0x8bc
+#define PCIE_DBI_RO_WR_EN		BIT(0)
+
+#define PCIE_PORT_DEBUG0			0x728
+#define PCIE_PORT_DEBUG1			0x72c
+#define PCIE_PORT_DEBUG1_LINK_UP		BIT(4)
+#define PCIE_PORT_DEBUG1_LINK_IN_TRAINING	BIT(29)
+
+#define PCIE_LINK_UP_TIMEOUT_MS		100
+
+#define IOMUXC_GPR14_OFFSET			0x38
+#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE_EN	BIT(10)
+#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE		BIT(11)
+
+struct pcie_dw_imx8 {
+	/* Must be first member of the struct */
+	struct pcie_dw			dw;
+	struct regmap			*iomuxc_gpr;
+	struct clk			pcie;
+	struct clk			pcie_bus;
+	struct clk			pcie_aux;
+	struct gpio_desc		reset_gpio;
+	struct reset_ctl		apps_reset;
+	struct phy			phy;
+};
+
+static void pcie_dw_configure(struct pcie_dw_imx8 *priv, u32 cap_speed)
+{
+	u32 val;
+
+	dw_pcie_dbi_write_enable(&priv->dw, true);
+
+	val = readl(priv->dw.dbi_base + PCIE_LINK_CAPABILITY);
+	val &= ~TARGET_LINK_SPEED_MASK;
+	val |= cap_speed;
+	writel(val, priv->dw.dbi_base + PCIE_LINK_CAPABILITY);
+
+	dw_pcie_dbi_write_enable(&priv->dw, false);
+}
+
+static void imx8_pcie_ltssm_enable(struct pcie_dw_imx8 *priv)
+{
+	reset_deassert(&priv->apps_reset);
+}
+
+static void imx8_pcie_ltssm_disable(struct pcie_dw_imx8 *priv)
+{
+	reset_assert(&priv->apps_reset);
+}
+
+static int is_link_up(struct pcie_dw_imx8 *priv)
+{
+	u32 val;
+
+	val = readl(priv->dw.dbi_base + PCIE_PORT_DEBUG1);
+
+	return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
+		(!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
+}
+
+static int wait_link_up(struct pcie_dw_imx8 *priv)
+{
+	unsigned long timeout;
+
+	timeout = get_timer(0) + PCIE_LINK_UP_TIMEOUT_MS;
+	while (!is_link_up(priv)) {
+		if (get_timer(0) > timeout)
+			return 0;
+		mdelay(10);
+	};
+
+	return 1;
+}
+
+static int pcie_link_up(struct pcie_dw_imx8 *priv, u32 cap_speed)
+{
+	if (is_link_up(priv)) {
+		printf("PCI Link already up before configuration!\n");
+		return 1;
+	}
+
+	/* DW pre link configurations */
+	pcie_dw_configure(priv, cap_speed);
+
+	/* Initiate link training */
+	imx8_pcie_ltssm_enable(priv);
+
+	/* Check that link was established */
+	if (!wait_link_up(priv)) {
+		imx8_pcie_ltssm_disable(priv);
+		return 0;
+	}
+
+	return 1;
+}
+
+static int imx8_pcie_assert_core_reset(struct pcie_dw_imx8 *priv)
+{
+	if (dm_gpio_is_valid(&priv->reset_gpio)) {
+		dm_gpio_set_value(&priv->reset_gpio, 1);
+		mdelay(20);
+	}
+
+	return reset_assert(&priv->apps_reset);
+}
+
+static int imx8_pcie_clk_enable(struct pcie_dw_imx8 *priv)
+{
+	int ret;
+
+	ret = clk_enable(&priv->pcie);
+	if (ret) {
+		printf("unable to enable pcie clock\n");
+		return ret;
+	}
+
+	ret = clk_enable(&priv->pcie_bus);
+	if (ret) {
+		printf("unable to enable pcie_bus clock\n");
+		goto err_pcie;
+	}
+
+	ret = clk_enable(&priv->pcie_aux);
+	if (ret) {
+		printf("unable to enable pcie_aux clock\n");
+		goto err_pcie_bus;
+	}
+
+	/*
+	 * Set the over ride low and enabled make sure that
+	 * REF_CLK is turned on.
+	 */
+	regmap_update_bits(priv->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE, 0);
+	regmap_update_bits(priv->iomuxc_gpr, IOMUXC_GPR14_OFFSET,
+			   IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
+			   IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
+
+	/* allow the clocks to stabilize */
+	udelay(500);
+
+	return 0;
+
+err_pcie_bus:
+	clk_disable(&priv->pcie_bus);
+err_pcie:
+	clk_disable(&priv->pcie);
+
+	return ret;
+}
+
+static int imx8_pcie_deassert_core_reset(struct pcie_dw_imx8 *priv)
+{
+	if (dm_gpio_is_valid(&priv->reset_gpio)) {
+		mdelay(100);
+		dm_gpio_set_value(&priv->reset_gpio, 0);
+		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
+		mdelay(100);
+	}
+
+	return 0;
+}
+
+static int pcie_dw_imx8_probe(struct udevice *dev)
+{
+	struct pcie_dw_imx8 *priv = dev_get_priv(dev);
+	struct udevice *ctlr = pci_get_controller(dev);
+	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
+	int ret;
+
+	ret = imx8_pcie_assert_core_reset(priv);
+	if (ret) {
+		dev_err(dev, "failed to assert core reset\n");
+		return ret;
+	}
+
+	ret = imx8_pcie_clk_enable(priv);
+	if (ret) {
+		dev_err(dev, "failed to enable clocks\n");
+		return ret;
+	}
+
+	generic_phy_init(&priv->phy);
+	generic_phy_power_on(&priv->phy);
+
+	ret = imx8_pcie_deassert_core_reset(priv);
+	if (ret) {
+		dev_err(dev, "failed to assert core reset\n");
+		return ret;
+	}
+
+	priv->dw.first_busno = dev_seq(dev);
+	priv->dw.dev = dev;
+
+	pcie_dw_setup_host(&priv->dw);
+
+	if (!pcie_link_up(priv, LINK_SPEED_GEN_1)) {
+		printf("PCIE-%d: Link down\n", dev_seq(dev));
+		return -ENODEV;
+	}
+
+	printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev),
+	       pcie_dw_get_link_speed(&priv->dw),
+	       pcie_dw_get_link_width(&priv->dw),
+	       hose->first_busno);
+
+	pcie_dw_prog_outbound_atu_unroll(&priv->dw, PCIE_ATU_REGION_INDEX0,
+					 PCIE_ATU_TYPE_MEM,
+					 priv->dw.mem.phys_start,
+					 priv->dw.mem.bus_start, priv->dw.mem.size);
+
+	return 0;
+}
+
+static int pcie_dw_imx8_remove(struct udevice *dev)
+{
+	struct pcie_dw_imx8 *priv = dev_get_priv(dev);
+
+	imx8_pcie_assert_core_reset(priv);
+
+	return 0;
+}
+
+static int pcie_dw_imx8_of_to_plat(struct udevice *dev)
+{
+	struct pcie_dw_imx8 *priv = dev_get_priv(dev);
+	ofnode gpr;
+	int ret;
+
+	/* Get the controller base address */
+	priv->dw.dbi_base = (void *)dev_read_addr_name(dev, "dbi");
+	if ((fdt_addr_t)priv->dw.dbi_base == FDT_ADDR_T_NONE) {
+		dev_err(dev, "failed to get dbi_base address\n");
+		return -EINVAL;
+	}
+
+	/* Get the config space base address and size */
+	priv->dw.cfg_base = (void *)dev_read_addr_size_name(dev, "config",
+							    &priv->dw.cfg_size);
+	if ((fdt_addr_t)priv->dw.cfg_base == FDT_ADDR_T_NONE) {
+		dev_err(dev, "failed to get cfg_base address\n");
+		return -EINVAL;
+	}
+
+	ret = clk_get_by_name(dev, "pcie_bus", &priv->pcie_bus);
+	if (ret) {
+		dev_err(dev, "failed to get pcie_bus clk\n");
+		return ret;
+	}
+
+	ret = clk_get_by_name(dev, "pcie", &priv->pcie);
+	if (ret) {
+		dev_err(dev, "failed to get pcie clk\n");
+		return ret;
+	}
+
+	ret = clk_get_by_name(dev, "pcie", &priv->pcie_aux);
+	if (ret) {
+		dev_err(dev, "failed to get pcie clk\n");
+		return ret;
+	}
+
+	ret = reset_get_by_name(dev, "apps", &priv->apps_reset);
+	if (ret) {
+		dev_err(dev,
+			"Failed to get PCIe apps reset control\n");
+		return ret;
+	}
+
+	ret = gpio_request_by_name(dev, "reset-gpio", 0, &priv->reset_gpio,
+				   (GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE));
+	if (ret) {
+		dev_err(dev, "unable to get reset-gpio\n");
+		return ret;
+	}
+
+	ret = generic_phy_get_by_name(dev, "pcie-phy", &priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to get pcie phy\n");
+		return ret;
+	}
+
+	gpr = ofnode_by_compatible(ofnode_null(), "fsl,imx8mp-iomuxc-gpr");
+	if (ofnode_equal(gpr, ofnode_null())) {
+		dev_err(dev, "unable to find GPR node\n");
+		return -ENODEV;
+	}
+
+	priv->iomuxc_gpr = syscon_node_to_regmap(gpr);
+	if (IS_ERR(priv->iomuxc_gpr)) {
+		dev_err(dev, "unable to find iomuxc registers\n");
+		return PTR_ERR(priv->iomuxc_gpr);
+	}
+
+	return 0;
+}
+
+static const struct dm_pci_ops pcie_dw_imx8_ops = {
+	.read_config	= pcie_dw_read_config,
+	.write_config	= pcie_dw_write_config,
+};
+
+static const struct udevice_id pcie_dw_imx8_ids[] = {
+	{ .compatible = "fsl,imx8mp-pcie" },
+	{ }
+};
+
+U_BOOT_DRIVER(pcie_dw_imx8) = {
+	.name		= "pcie_dw_imx8",
+	.id		= UCLASS_PCI,
+	.of_match	= pcie_dw_imx8_ids,
+	.ops		= &pcie_dw_imx8_ops,
+	.of_to_plat	= pcie_dw_imx8_of_to_plat,
+	.probe		= pcie_dw_imx8_probe,
+	.remove		= pcie_dw_imx8_remove,
+	.priv_auto	= sizeof(struct pcie_dw_imx8),
+};
-- 
2.34.1

Re: [PATCH 6/7] pci: Add DW PCIe controller support for iMX8MP SoC

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> pcie_imx doesn't seem to share any useful code for iMX8 SoC and it is
> tied to quite old port of pcie_designware driver from Linux which
> suffices only iMX6 specific needs.
> 
> But currently we have the common DWC specific bits which alligns pretty
> well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
> bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
> add support for other iMX8 variants to this driver.
> 
> iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
> can reuse the generic PHY infrastructure to power on PCIe PHY.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>   drivers/pci/Kconfig        |   8 +
>   drivers/pci/Makefile       |   1 +
>   drivers/pci/pcie_dw_imx8.c | 348 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 357 insertions(+)
>   create mode 100644 drivers/pci/pcie_dw_imx8.c
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 463ec47eb92..b7c7922b091 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -413,4 +413,12 @@ config PCIE_STARFIVE_JH7110
>   	  Say Y here if you want to enable PLDA XpressRich PCIe controller
>   	  support on StarFive JH7110 SoC.
>   
> +config PCIE_DW_IMX8
> +	bool "i.MX8 PCIe support"
> +	depends on ARCH_IMX8M
> +	select PCIE_DW_COMMON
> +	help
> +	  Say Y here if you want to enable DW PCIe controller support on
> +	  iMX8 SoCs.
> +
>   endif
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 72ef8b4bc77..cddbb902095 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -53,3 +53,4 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o
>   obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
>   obj-$(CONFIG_PCIE_PLDA_COMMON) += pcie_plda_common.o
>   obj-$(CONFIG_PCIE_STARFIVE_JH7110) += pcie_starfive_jh7110.o
> +obj-$(CONFIG_PCIE_DW_IMX8) += pcie_dw_imx8.o
> diff --git a/drivers/pci/pcie_dw_imx8.c b/drivers/pci/pcie_dw_imx8.c
> new file mode 100644
> index 00000000000..b9921644765
> --- /dev/null
> +++ b/drivers/pci/pcie_dw_imx8.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2024 Linaro Ltd.
> + *
> + * Author: Sumit Garg <sumit.garg@linaro.org>
> + */
> +
> +#include <asm/io.h>
> +#include <asm-generic/gpio.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <generic-phy.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <log.h>
> +#include <pci.h>
> +#include <regmap.h>
> +#include <reset.h>
> +#include <syscon.h>
> +#include <time.h>
> +
> +#include "pcie_dw_common.h"
> +
> +#define PCIE_LINK_CAPABILITY		0x7c
> +#define TARGET_LINK_SPEED_MASK		0xf
> +#define LINK_SPEED_GEN_1		0x1
> +#define LINK_SPEED_GEN_2		0x2
> +#define LINK_SPEED_GEN_3		0x3
> +
> +#define PCIE_MISC_CONTROL_1_OFF		0x8bc
> +#define PCIE_DBI_RO_WR_EN		BIT(0)
> +
> +#define PCIE_PORT_DEBUG0			0x728
> +#define PCIE_PORT_DEBUG1			0x72c
> +#define PCIE_PORT_DEBUG1_LINK_UP		BIT(4)
> +#define PCIE_PORT_DEBUG1_LINK_IN_TRAINING	BIT(29)
> +
> +#define PCIE_LINK_UP_TIMEOUT_MS		100
> +
> +#define IOMUXC_GPR14_OFFSET			0x38
> +#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE_EN	BIT(10)
> +#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE		BIT(11)
> +
> +struct pcie_dw_imx8 {
> +	/* Must be first member of the struct */
> +	struct pcie_dw			dw;
> +	struct regmap			*iomuxc_gpr;
> +	struct clk			pcie;
> +	struct clk			pcie_bus;
> +	struct clk			pcie_aux;
> +	struct gpio_desc		reset_gpio;
> +	struct reset_ctl		apps_reset;
> +	struct phy			phy;
> +};
> +
> +static void pcie_dw_configure(struct pcie_dw_imx8 *priv, u32 cap_speed)
> +{
> +	u32 val;
> +
> +	dw_pcie_dbi_write_enable(&priv->dw, true);
> +
> +	val = readl(priv->dw.dbi_base + PCIE_LINK_CAPABILITY);
> +	val &= ~TARGET_LINK_SPEED_MASK;
> +	val |= cap_speed;
> +	writel(val, priv->dw.dbi_base + PCIE_LINK_CAPABILITY);

clrsetbits_le32()

> +
> +	dw_pcie_dbi_write_enable(&priv->dw, false);
> +}
> +
> +static void imx8_pcie_ltssm_enable(struct pcie_dw_imx8 *priv)
> +{
> +	reset_deassert(&priv->apps_reset);
> +}
> +
> +static void imx8_pcie_ltssm_disable(struct pcie_dw_imx8 *priv)
> +{
> +	reset_assert(&priv->apps_reset);
> +}
> +
> +static int is_link_up(struct pcie_dw_imx8 *priv)
> +{
> +	u32 val;
> +
> +	val = readl(priv->dw.dbi_base + PCIE_PORT_DEBUG1);
> +
> +	return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
> +		(!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
> +}
> +
> +static int wait_link_up(struct pcie_dw_imx8 *priv)
> +{
> +	unsigned long timeout;
> +
> +	timeout = get_timer(0) + PCIE_LINK_UP_TIMEOUT_MS;

wait_for_bit() or read_poll_timeout()

> +	while (!is_link_up(priv)) {
> +		if (get_timer(0) > timeout)
> +			return 0;
> +		mdelay(10);
> +	};
> +
> +	return 1;

return -ETIMEDOUT ?

> +}
> +
> +static int pcie_link_up(struct pcie_dw_imx8 *priv, u32 cap_speed)
> +{
> +	if (is_link_up(priv)) {
> +		printf("PCI Link already up before configuration!\n");
> +		return 1;
> +	}
> +
> +	/* DW pre link configurations */
> +	pcie_dw_configure(priv, cap_speed);
> +
> +	/* Initiate link training */
> +	imx8_pcie_ltssm_enable(priv);
> +
> +	/* Check that link was established */
> +	if (!wait_link_up(priv)) {
> +		imx8_pcie_ltssm_disable(priv);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int imx8_pcie_assert_core_reset(struct pcie_dw_imx8 *priv)
> +{
> +	if (dm_gpio_is_valid(&priv->reset_gpio)) {
> +		dm_gpio_set_value(&priv->reset_gpio, 1);
> +		mdelay(20);
> +	}
> +
> +	return reset_assert(&priv->apps_reset);
> +}
> +
> +static int imx8_pcie_clk_enable(struct pcie_dw_imx8 *priv)
> +{
> +	int ret;
> +
> +	ret = clk_enable(&priv->pcie);

Can you use clk_bulk functions here ?

[...]

> +static int imx8_pcie_deassert_core_reset(struct pcie_dw_imx8 *priv)
> +{
> +	if (dm_gpio_is_valid(&priv->reset_gpio)) {

Invert the match here and return early to reduce indent.

> +		mdelay(100);
> +		dm_gpio_set_value(&priv->reset_gpio, 0);
> +		/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> +		mdelay(100);
> +	}
> +
> +	return 0;
> +}
> +
> +static int pcie_dw_imx8_probe(struct udevice *dev)
> +{
> +	struct pcie_dw_imx8 *priv = dev_get_priv(dev);
> +	struct udevice *ctlr = pci_get_controller(dev);
> +	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> +	int ret;
> +
> +	ret = imx8_pcie_assert_core_reset(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to assert core reset\n");
> +		return ret;
> +	}
> +
> +	ret = imx8_pcie_clk_enable(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");

This needs a fail path, else reset remains released on failure here .

> +		return ret;
> +	}
> +
> +	generic_phy_init(&priv->phy);
> +	generic_phy_power_on(&priv->phy);
> +
> +	ret = imx8_pcie_deassert_core_reset(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to assert core reset\n");
> +		return ret;
> +	}
> +
> +	priv->dw.first_busno = dev_seq(dev);
> +	priv->dw.dev = dev;
> +
> +	pcie_dw_setup_host(&priv->dw);
> +
> +	if (!pcie_link_up(priv, LINK_SPEED_GEN_1)) {
> +		printf("PCIE-%d: Link down\n", dev_seq(dev));
> +		return -ENODEV;
> +	}
> +
> +	printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev),
> +	       pcie_dw_get_link_speed(&priv->dw),
> +	       pcie_dw_get_link_width(&priv->dw),
> +	       hose->first_busno);
> +
> +	pcie_dw_prog_outbound_atu_unroll(&priv->dw, PCIE_ATU_REGION_INDEX0,
> +					 PCIE_ATU_TYPE_MEM,
> +					 priv->dw.mem.phys_start,
> +					 priv->dw.mem.bus_start, priv->dw.mem.size);
> +
> +	return 0;
> +}
> +
> +static int pcie_dw_imx8_remove(struct udevice *dev)
> +{
> +	struct pcie_dw_imx8 *priv = dev_get_priv(dev);
> +
> +	imx8_pcie_assert_core_reset(priv);
> +
> +	return 0;
> +}

[...]

Re: [PATCH 6/7] pci: Add DW PCIe controller support for iMX8MP SoC

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > pcie_imx doesn't seem to share any useful code for iMX8 SoC and it is
> > tied to quite old port of pcie_designware driver from Linux which
> > suffices only iMX6 specific needs.
> >
> > But currently we have the common DWC specific bits which alligns pretty
> > well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
> > bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
> > add support for other iMX8 variants to this driver.
> >
> > iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
> > can reuse the generic PHY infrastructure to power on PCIe PHY.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >   drivers/pci/Kconfig        |   8 +
> >   drivers/pci/Makefile       |   1 +
> >   drivers/pci/pcie_dw_imx8.c | 348 +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 357 insertions(+)
> >   create mode 100644 drivers/pci/pcie_dw_imx8.c
> >
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index 463ec47eb92..b7c7922b091 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -413,4 +413,12 @@ config PCIE_STARFIVE_JH7110
> >         Say Y here if you want to enable PLDA XpressRich PCIe controller
> >         support on StarFive JH7110 SoC.
> >
> > +config PCIE_DW_IMX8
> > +     bool "i.MX8 PCIe support"
> > +     depends on ARCH_IMX8M
> > +     select PCIE_DW_COMMON
> > +     help
> > +       Say Y here if you want to enable DW PCIe controller support on
> > +       iMX8 SoCs.
> > +
> >   endif
> > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > index 72ef8b4bc77..cddbb902095 100644
> > --- a/drivers/pci/Makefile
> > +++ b/drivers/pci/Makefile
> > @@ -53,3 +53,4 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o
> >   obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o
> >   obj-$(CONFIG_PCIE_PLDA_COMMON) += pcie_plda_common.o
> >   obj-$(CONFIG_PCIE_STARFIVE_JH7110) += pcie_starfive_jh7110.o
> > +obj-$(CONFIG_PCIE_DW_IMX8) += pcie_dw_imx8.o
> > diff --git a/drivers/pci/pcie_dw_imx8.c b/drivers/pci/pcie_dw_imx8.c
> > new file mode 100644
> > index 00000000000..b9921644765
> > --- /dev/null
> > +++ b/drivers/pci/pcie_dw_imx8.c
> > @@ -0,0 +1,348 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2024 Linaro Ltd.
> > + *
> > + * Author: Sumit Garg <sumit.garg@linaro.org>
> > + */
> > +
> > +#include <asm/io.h>
> > +#include <asm-generic/gpio.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <dm/device_compat.h>
> > +#include <generic-phy.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <log.h>
> > +#include <pci.h>
> > +#include <regmap.h>
> > +#include <reset.h>
> > +#include <syscon.h>
> > +#include <time.h>
> > +
> > +#include "pcie_dw_common.h"
> > +
> > +#define PCIE_LINK_CAPABILITY         0x7c
> > +#define TARGET_LINK_SPEED_MASK               0xf
> > +#define LINK_SPEED_GEN_1             0x1
> > +#define LINK_SPEED_GEN_2             0x2
> > +#define LINK_SPEED_GEN_3             0x3
> > +
> > +#define PCIE_MISC_CONTROL_1_OFF              0x8bc
> > +#define PCIE_DBI_RO_WR_EN            BIT(0)
> > +
> > +#define PCIE_PORT_DEBUG0                     0x728
> > +#define PCIE_PORT_DEBUG1                     0x72c
> > +#define PCIE_PORT_DEBUG1_LINK_UP             BIT(4)
> > +#define PCIE_PORT_DEBUG1_LINK_IN_TRAINING    BIT(29)
> > +
> > +#define PCIE_LINK_UP_TIMEOUT_MS              100
> > +
> > +#define IOMUXC_GPR14_OFFSET                  0x38
> > +#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE_EN   BIT(10)
> > +#define IMX8M_GPR_PCIE_CLK_REQ_OVERRIDE              BIT(11)
> > +
> > +struct pcie_dw_imx8 {
> > +     /* Must be first member of the struct */
> > +     struct pcie_dw                  dw;
> > +     struct regmap                   *iomuxc_gpr;
> > +     struct clk                      pcie;
> > +     struct clk                      pcie_bus;
> > +     struct clk                      pcie_aux;
> > +     struct gpio_desc                reset_gpio;
> > +     struct reset_ctl                apps_reset;
> > +     struct phy                      phy;
> > +};
> > +
> > +static void pcie_dw_configure(struct pcie_dw_imx8 *priv, u32 cap_speed)
> > +{
> > +     u32 val;
> > +
> > +     dw_pcie_dbi_write_enable(&priv->dw, true);
> > +
> > +     val = readl(priv->dw.dbi_base + PCIE_LINK_CAPABILITY);
> > +     val &= ~TARGET_LINK_SPEED_MASK;
> > +     val |= cap_speed;
> > +     writel(val, priv->dw.dbi_base + PCIE_LINK_CAPABILITY);
>
> clrsetbits_le32()

Ack.

>
> > +
> > +     dw_pcie_dbi_write_enable(&priv->dw, false);
> > +}
> > +
> > +static void imx8_pcie_ltssm_enable(struct pcie_dw_imx8 *priv)
> > +{
> > +     reset_deassert(&priv->apps_reset);
> > +}
> > +
> > +static void imx8_pcie_ltssm_disable(struct pcie_dw_imx8 *priv)
> > +{
> > +     reset_assert(&priv->apps_reset);
> > +}
> > +
> > +static int is_link_up(struct pcie_dw_imx8 *priv)
> > +{
> > +     u32 val;
> > +
> > +     val = readl(priv->dw.dbi_base + PCIE_PORT_DEBUG1);
> > +
> > +     return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
> > +             (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
> > +}
> > +
> > +static int wait_link_up(struct pcie_dw_imx8 *priv)
> > +{
> > +     unsigned long timeout;
> > +
> > +     timeout = get_timer(0) + PCIE_LINK_UP_TIMEOUT_MS;
>
> wait_for_bit() or read_poll_timeout()

They won't appropriately fit here as I would like to add delay in
between consecutive polls too...

>
> > +     while (!is_link_up(priv)) {
> > +             if (get_timer(0) > timeout)
> > +                     return 0;
> > +             mdelay(10);

...here.

> > +     };
> > +
> > +     return 1;
>
> return -ETIMEDOUT ?

Here 0 represents timeout and 1 represents success condition.

>
> > +}
> > +
> > +static int pcie_link_up(struct pcie_dw_imx8 *priv, u32 cap_speed)
> > +{
> > +     if (is_link_up(priv)) {
> > +             printf("PCI Link already up before configuration!\n");
> > +             return 1;
> > +     }
> > +
> > +     /* DW pre link configurations */
> > +     pcie_dw_configure(priv, cap_speed);
> > +
> > +     /* Initiate link training */
> > +     imx8_pcie_ltssm_enable(priv);
> > +
> > +     /* Check that link was established */
> > +     if (!wait_link_up(priv)) {
> > +             imx8_pcie_ltssm_disable(priv);
> > +             return 0;
> > +     }
> > +
> > +     return 1;
> > +}
> > +
> > +static int imx8_pcie_assert_core_reset(struct pcie_dw_imx8 *priv)
> > +{
> > +     if (dm_gpio_is_valid(&priv->reset_gpio)) {
> > +             dm_gpio_set_value(&priv->reset_gpio, 1);
> > +             mdelay(20);
> > +     }
> > +
> > +     return reset_assert(&priv->apps_reset);
> > +}
> > +
> > +static int imx8_pcie_clk_enable(struct pcie_dw_imx8 *priv)
> > +{
> > +     int ret;
> > +
> > +     ret = clk_enable(&priv->pcie);
>
> Can you use clk_bulk functions here ?
>

Sure, I can try to use them.

> [...]
>
> > +static int imx8_pcie_deassert_core_reset(struct pcie_dw_imx8 *priv)
> > +{
> > +     if (dm_gpio_is_valid(&priv->reset_gpio)) {
>
> Invert the match here and return early to reduce indent.

Ack.

>
> > +             mdelay(100);
> > +             dm_gpio_set_value(&priv->reset_gpio, 0);
> > +             /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
> > +             mdelay(100);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int pcie_dw_imx8_probe(struct udevice *dev)
> > +{
> > +     struct pcie_dw_imx8 *priv = dev_get_priv(dev);
> > +     struct udevice *ctlr = pci_get_controller(dev);
> > +     struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> > +     int ret;
> > +
> > +     ret = imx8_pcie_assert_core_reset(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to assert core reset\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = imx8_pcie_clk_enable(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to enable clocks\n");
>
> This needs a fail path, else reset remains released on failure here .
>

Ack, I will add that.

-Sumit

> > +             return ret;
> > +     }
> > +
> > +     generic_phy_init(&priv->phy);
> > +     generic_phy_power_on(&priv->phy);
> > +
> > +     ret = imx8_pcie_deassert_core_reset(priv);
> > +     if (ret) {
> > +             dev_err(dev, "failed to assert core reset\n");
> > +             return ret;
> > +     }
> > +
> > +     priv->dw.first_busno = dev_seq(dev);
> > +     priv->dw.dev = dev;
> > +
> > +     pcie_dw_setup_host(&priv->dw);
> > +
> > +     if (!pcie_link_up(priv, LINK_SPEED_GEN_1)) {
> > +             printf("PCIE-%d: Link down\n", dev_seq(dev));
> > +             return -ENODEV;
> > +     }
> > +
> > +     printf("PCIE-%d: Link up (Gen%d-x%d, Bus%d)\n", dev_seq(dev),
> > +            pcie_dw_get_link_speed(&priv->dw),
> > +            pcie_dw_get_link_width(&priv->dw),
> > +            hose->first_busno);
> > +
> > +     pcie_dw_prog_outbound_atu_unroll(&priv->dw, PCIE_ATU_REGION_INDEX0,
> > +                                      PCIE_ATU_TYPE_MEM,
> > +                                      priv->dw.mem.phys_start,
> > +                                      priv->dw.mem.bus_start, priv->dw.mem.size);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pcie_dw_imx8_remove(struct udevice *dev)
> > +{
> > +     struct pcie_dw_imx8 *priv = dev_get_priv(dev);
> > +
> > +     imx8_pcie_assert_core_reset(priv);
> > +
> > +     return 0;
> > +}
>
> [...]

Re: [PATCH 6/7] pci: Add DW PCIe controller support for iMX8MP SoC

[Marek Vasut]

On 2/21/24 07:25, Sumit Garg wrote:

[...]

>>> +static int wait_link_up(struct pcie_dw_imx8 *priv)
>>> +{
>>> +     unsigned long timeout;
>>> +
>>> +     timeout = get_timer(0) + PCIE_LINK_UP_TIMEOUT_MS;
>>
>> wait_for_bit() or read_poll_timeout()
> 
> They won't appropriately fit here as I would like to add delay in
> between consecutive polls too...
> 
>>
>>> +     while (!is_link_up(priv)) {
>>> +             if (get_timer(0) > timeout)
>>> +                     return 0;
>>> +             mdelay(10);
> 
> ...here.

include/linux/iopoll.h:#define read_poll_timeout(op, val, cond, 
sleep_us, timeout_us, args...)  \

That's the "sleep_us" , is it not ?

>>> +     };
>>> +
>>> +     return 1;
>>
>> return -ETIMEDOUT ?
> 
> Here 0 represents timeout and 1 represents success condition.

0 generally represents success, non-zero error, please fix.

[...]

[PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Sumit Garg]

Also, enable reset driver which is a prerequisite for PCIe support.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 configs/verdin-imx8mp_defconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
index 22b8a334dfa..d8bd644322b 100644
--- a/configs/verdin-imx8mp_defconfig
+++ b/configs/verdin-imx8mp_defconfig
@@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
 CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
 CONFIG_IMX_WATCHDOG=y
 CONFIG_HEXDUMP=y
+CONFIG_DM_RESET=y
+CONFIG_RESET_IMX=y
+CONFIG_PCI=y
+CONFIG_PCIE_DW_IMX8=y
+CONFIG_PHY_IMX8M_PCIE=y
+CONFIG_CMD_PCI=y
+CONFIG_NVME=y
+CONFIG_NVME_PCI=y
+CONFIG_CMD_NVME=y
-- 
2.34.1

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> Also, enable reset driver which is a prerequisite for PCIe support.

Commit message needs to be fixed.

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > Also, enable reset driver which is a prerequisite for PCIe support.
>
> Commit message needs to be fixed.

Let me reiterate the header here too.

    Enable PCIe/NVMe support. Also, enable the reset driver which
    is a prerequisite for PCIe support.

-Sumit

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Francesco Dolcini]

On Wed, Feb 21, 2024 at 11:57:51AM +0530, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
> > On 2/20/24 14:10, Sumit Garg wrote:
> > > Also, enable reset driver which is a prerequisite for PCIe support.
> > Commit message needs to be fixed.
> 
> Let me reiterate the header here too.
> 
>     Enable PCIe/NVMe support. Also, enable the reset driver which
>     is a prerequisite for PCIe support.

With the commit message improved ..

On Wed, Feb 21, 2024 at 11:59:10AM +0530, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:34, Fabio Estevam <festevam@gmail.com> wrote:
> > On Tue, Feb 20, 2024 at 10:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > > index 22b8a334dfa..d8bd644322b 100644
> > > --- a/configs/verdin-imx8mp_defconfig
> > > +++ b/configs/verdin-imx8mp_defconfig
> > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> > >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> > >  CONFIG_IMX_WATCHDOG=y
> > >  CONFIG_HEXDUMP=y
> > > +CONFIG_DM_RESET=y
> > > +CONFIG_RESET_IMX=y
> > > +CONFIG_PCI=y
> > > +CONFIG_PCIE_DW_IMX8=y
> > > +CONFIG_PHY_IMX8M_PCIE=y
> > > +CONFIG_CMD_PCI=y
> > > +CONFIG_NVME=y
> > > +CONFIG_NVME_PCI=y
> > > +CONFIG_CMD_NVME=y
> >
> > Please don't group all these new config options at the end of the file.
> That sounds better, I will do that for v2.

... and this change, feel free to add in v2

Acked-by: Francesco Dolcini <francesco.dolcini@toradex.com>

Francesco

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Fabio Estevam]

On Tue, Feb 20, 2024 at 10:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Also, enable reset driver which is a prerequisite for PCIe support.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  configs/verdin-imx8mp_defconfig | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> index 22b8a334dfa..d8bd644322b 100644
> --- a/configs/verdin-imx8mp_defconfig
> +++ b/configs/verdin-imx8mp_defconfig
> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
>  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>  CONFIG_IMX_WATCHDOG=y
>  CONFIG_HEXDUMP=y
> +CONFIG_DM_RESET=y
> +CONFIG_RESET_IMX=y
> +CONFIG_PCI=y
> +CONFIG_PCIE_DW_IMX8=y
> +CONFIG_PHY_IMX8M_PCIE=y
> +CONFIG_CMD_PCI=y
> +CONFIG_NVME=y
> +CONFIG_NVME_PCI=y
> +CONFIG_CMD_NVME=y

Please don't group all these new config options at the end of the file.

Use 'make savedefconfig' and then 'cp defconfig
configs/verdin-imx8mp_defconfig' to properly
add these new config options.

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:34, Fabio Estevam <festevam@gmail.com> wrote:
>
> On Tue, Feb 20, 2024 at 10:51 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Also, enable reset driver which is a prerequisite for PCIe support.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  configs/verdin-imx8mp_defconfig | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > index 22b8a334dfa..d8bd644322b 100644
> > --- a/configs/verdin-imx8mp_defconfig
> > +++ b/configs/verdin-imx8mp_defconfig
> > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> >  CONFIG_IMX_WATCHDOG=y
> >  CONFIG_HEXDUMP=y
> > +CONFIG_DM_RESET=y
> > +CONFIG_RESET_IMX=y
> > +CONFIG_PCI=y
> > +CONFIG_PCIE_DW_IMX8=y
> > +CONFIG_PHY_IMX8M_PCIE=y
> > +CONFIG_CMD_PCI=y
> > +CONFIG_NVME=y
> > +CONFIG_NVME_PCI=y
> > +CONFIG_CMD_NVME=y
>
> Please don't group all these new config options at the end of the file.
>
> Use 'make savedefconfig' and then 'cp defconfig
> configs/verdin-imx8mp_defconfig' to properly
> add these new config options.

That sounds better, I will do that for v2.

-Sumit

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Francesco Dolcini]

Hello Sumit,

On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> Also, enable reset driver which is a prerequisite for PCIe support.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  configs/verdin-imx8mp_defconfig | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> index 22b8a334dfa..d8bd644322b 100644
> --- a/configs/verdin-imx8mp_defconfig
> +++ b/configs/verdin-imx8mp_defconfig
> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
>  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>  CONFIG_IMX_WATCHDOG=y
>  CONFIG_HEXDUMP=y
> +CONFIG_DM_RESET=y
> +CONFIG_RESET_IMX=y
> +CONFIG_PCI=y
> +CONFIG_PCIE_DW_IMX8=y
> +CONFIG_PHY_IMX8M_PCIE=y
> +CONFIG_CMD_PCI=y
> +CONFIG_NVME=y
> +CONFIG_NVME_PCI=y
> +CONFIG_CMD_NVME=y

This will increase the u-boot proper size and marginally increase the
boot time (because of a bigger binary to be read from the eMMC).

Apart of that do you expect any other impact on those changes? SPL
binary size should not be affected, correct?

Asking this out loudly to confirm that nothing unexpected is going to
happen because of these changes.

For my curiosity, care to share what's the use case? Do you plan to have
the OS stored into an NVME device?

Francesco

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marcel Ziswiler]

Hi Sumit

On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> Hello Sumit,
> 
> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> > Also, enable reset driver which is a prerequisite for PCIe support.
> > 
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >  configs/verdin-imx8mp_defconfig | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > index 22b8a334dfa..d8bd644322b 100644
> > --- a/configs/verdin-imx8mp_defconfig
> > +++ b/configs/verdin-imx8mp_defconfig
> > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> >  CONFIG_IMX_WATCHDOG=y
> >  CONFIG_HEXDUMP=y
> > +CONFIG_DM_RESET=y
> > +CONFIG_RESET_IMX=y
> > +CONFIG_PCI=y
> > +CONFIG_PCIE_DW_IMX8=y
> > +CONFIG_PHY_IMX8M_PCIE=y
> > +CONFIG_CMD_PCI=y
> > +CONFIG_NVME=y
> > +CONFIG_NVME_PCI=y
> > +CONFIG_CMD_NVME=y
> 
> This will increase the u-boot proper size

Yes, I checked and it is actually slightly more than 32 K.

> and marginally increase the
> boot time (because of a bigger binary to be read from the eMMC).

That was also my concern.

> Apart of that do you expect any other impact on those changes? SPL
> binary size should not be affected, correct?
> 
> Asking this out loudly to confirm that nothing unexpected is going to
> happen because of these changes.

Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
affected (other than the slight size and boot time increase, of course).

> For my curiosity, care to share what's the use case? Do you plan to have
> the OS stored into an NVME device?

For us the question is basically whether that use case does mandate enforcing such changes for each and every
customer. Plus the regular expected maintenance effort any such change brings with it, of course.

> Francesco

Cheers

Marcel

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Fathi Boudra]

Hi,

On Wed, 21 Feb 2024 at 10:19, Marcel Ziswiler
<marcel.ziswiler@toradex.com> wrote:
>
> Hi Sumit
>
> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> > Hello Sumit,
> >
> > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> > > Also, enable reset driver which is a prerequisite for PCIe support.
> > >
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >  configs/verdin-imx8mp_defconfig | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > > index 22b8a334dfa..d8bd644322b 100644
> > > --- a/configs/verdin-imx8mp_defconfig
> > > +++ b/configs/verdin-imx8mp_defconfig
> > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> > >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> > >  CONFIG_IMX_WATCHDOG=y
> > >  CONFIG_HEXDUMP=y
> > > +CONFIG_DM_RESET=y
> > > +CONFIG_RESET_IMX=y
> > > +CONFIG_PCI=y
> > > +CONFIG_PCIE_DW_IMX8=y
> > > +CONFIG_PHY_IMX8M_PCIE=y
> > > +CONFIG_CMD_PCI=y
> > > +CONFIG_NVME=y
> > > +CONFIG_NVME_PCI=y
> > > +CONFIG_CMD_NVME=y
> >
> > This will increase the u-boot proper size
>
> Yes, I checked and it is actually slightly more than 32 K.
>
> > and marginally increase the
> > boot time (because of a bigger binary to be read from the eMMC).
>
> That was also my concern.
>
> > Apart of that do you expect any other impact on those changes? SPL
> > binary size should not be affected, correct?
> >
> > Asking this out loudly to confirm that nothing unexpected is going to
> > happen because of these changes.
>
> Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> affected (other than the slight size and boot time increase, of course).
>
> > For my curiosity, care to share what's the use case? Do you plan to have
> > the OS stored into an NVME device?
>
> For us the question is basically whether that use case does mandate enforcing such changes for each and every
> customer. Plus the regular expected maintenance effort any such change brings with it, of course.

@Francesco that's correct, we have the OS stored on NVME SSD.
@Marcel wrt enforcing it, we can always fallback to have a config
fragment for our customers builds.
That's already the case, we fine tune the config and don't use U-Boot
defconfig out of the box for shipping to end users.

> > Francesco
>
> Cheers
>
> Marcel

Cheers,
-- 
Fathi Boudra

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Sumit Garg]

On Wed, 21 Feb 2024 at 15:06, Fathi Boudra <fathi.boudra@linaro.org> wrote:
>
> Hi,
>
> On Wed, 21 Feb 2024 at 10:19, Marcel Ziswiler
> <marcel.ziswiler@toradex.com> wrote:
> >
> > Hi Sumit
> >
> > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> > > Hello Sumit,
> > >
> > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> > > > Also, enable reset driver which is a prerequisite for PCIe support.
> > > >
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > ---
> > > >  configs/verdin-imx8mp_defconfig | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > > > index 22b8a334dfa..d8bd644322b 100644
> > > > --- a/configs/verdin-imx8mp_defconfig
> > > > +++ b/configs/verdin-imx8mp_defconfig
> > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> > > >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> > > >  CONFIG_IMX_WATCHDOG=y
> > > >  CONFIG_HEXDUMP=y
> > > > +CONFIG_DM_RESET=y
> > > > +CONFIG_RESET_IMX=y
> > > > +CONFIG_PCI=y
> > > > +CONFIG_PCIE_DW_IMX8=y
> > > > +CONFIG_PHY_IMX8M_PCIE=y
> > > > +CONFIG_CMD_PCI=y
> > > > +CONFIG_NVME=y
> > > > +CONFIG_NVME_PCI=y
> > > > +CONFIG_CMD_NVME=y
> > >
> > > This will increase the u-boot proper size
> >
> > Yes, I checked and it is actually slightly more than 32 K.
> >
> > > and marginally increase the
> > > boot time (because of a bigger binary to be read from the eMMC).
> >
> > That was also my concern.
> >
> > > Apart of that do you expect any other impact on those changes? SPL
> > > binary size should not be affected, correct?
> > >
> > > Asking this out loudly to confirm that nothing unexpected is going to
> > > happen because of these changes.
> >
> > Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> > affected (other than the slight size and boot time increase, of course).

Thanks for taking the time to test it. I suppose I can take that as a
Tested-by tag.

> >
> > > For my curiosity, care to share what's the use case? Do you plan to have
> > > the OS stored into an NVME device?
> >
> > For us the question is basically whether that use case does mandate enforcing such changes for each and every
> > customer.

> > Plus the regular expected maintenance effort any such change brings with it, of course.

From a PCIe maintenance point of view, I can add myself as a
maintainer for drivers added by this patch-set.

>
> @Francesco that's correct, we have the OS stored on NVME SSD.
> @Marcel wrt enforcing it, we can always fallback to have a config
> fragment for our customers builds.
> That's already the case, we fine tune the config and don't use U-Boot
> defconfig out of the box for shipping to end users.
>

Marcel, Francesco,

If you folks still have worries about updating the defconfig then I
can make it as a config fragment for mainline too. I suppose that
should work for other boards too based on the iMX8MP SoC.

-Sumit

> > > Francesco
> >
> > Cheers
> >
> > Marcel
>
> Cheers,
> --
> Fathi Boudra

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marek Vasut]

On 2/21/24 10:18, Marcel Ziswiler wrote:
> Hi Sumit
> 
> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
>> Hello Sumit,
>>
>> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
>>> Also, enable reset driver which is a prerequisite for PCIe support.
>>>
>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>> ---
>>>   configs/verdin-imx8mp_defconfig | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
>>> index 22b8a334dfa..d8bd644322b 100644
>>> --- a/configs/verdin-imx8mp_defconfig
>>> +++ b/configs/verdin-imx8mp_defconfig
>>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
>>>   CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>>>   CONFIG_IMX_WATCHDOG=y
>>>   CONFIG_HEXDUMP=y
>>> +CONFIG_DM_RESET=y
>>> +CONFIG_RESET_IMX=y
>>> +CONFIG_PCI=y
>>> +CONFIG_PCIE_DW_IMX8=y
>>> +CONFIG_PHY_IMX8M_PCIE=y
>>> +CONFIG_CMD_PCI=y
>>> +CONFIG_NVME=y
>>> +CONFIG_NVME_PCI=y
>>> +CONFIG_CMD_NVME=y
>>
>> This will increase the u-boot proper size
> 
> Yes, I checked and it is actually slightly more than 32 K.
> 
>> and marginally increase the
>> boot time (because of a bigger binary to be read from the eMMC).
> 
> That was also my concern.
> 
>> Apart of that do you expect any other impact on those changes? SPL
>> binary size should not be affected, correct?
>>
>> Asking this out loudly to confirm that nothing unexpected is going to
>> happen because of these changes.
> 
> Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> affected (other than the slight size and boot time increase, of course).
> 
>> For my curiosity, care to share what's the use case? Do you plan to have
>> the OS stored into an NVME device?
> 
> For us the question is basically whether that use case does mandate enforcing such changes for each and every
> customer. Plus the regular expected maintenance effort any such change brings with it, of course.

You can always enable this support on MX8MP EVK, it has M2 slot and this 
would add build coverage of this code too, without impacting Verdin.

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Sumit Garg]

On Wed, 21 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote:
>
> On 2/21/24 10:18, Marcel Ziswiler wrote:
> > Hi Sumit
> >
> > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> >> Hello Sumit,
> >>
> >> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> >>> Also, enable reset driver which is a prerequisite for PCIe support.
> >>>
> >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>> ---
> >>>   configs/verdin-imx8mp_defconfig | 9 +++++++++
> >>>   1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> >>> index 22b8a334dfa..d8bd644322b 100644
> >>> --- a/configs/verdin-imx8mp_defconfig
> >>> +++ b/configs/verdin-imx8mp_defconfig
> >>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> >>>   CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> >>>   CONFIG_IMX_WATCHDOG=y
> >>>   CONFIG_HEXDUMP=y
> >>> +CONFIG_DM_RESET=y
> >>> +CONFIG_RESET_IMX=y
> >>> +CONFIG_PCI=y
> >>> +CONFIG_PCIE_DW_IMX8=y
> >>> +CONFIG_PHY_IMX8M_PCIE=y
> >>> +CONFIG_CMD_PCI=y
> >>> +CONFIG_NVME=y
> >>> +CONFIG_NVME_PCI=y
> >>> +CONFIG_CMD_NVME=y
> >>
> >> This will increase the u-boot proper size
> >
> > Yes, I checked and it is actually slightly more than 32 K.
> >
> >> and marginally increase the
> >> boot time (because of a bigger binary to be read from the eMMC).
> >
> > That was also my concern.
> >
> >> Apart of that do you expect any other impact on those changes? SPL
> >> binary size should not be affected, correct?
> >>
> >> Asking this out loudly to confirm that nothing unexpected is going to
> >> happen because of these changes.
> >
> > Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> > affected (other than the slight size and boot time increase, of course).
> >
> >> For my curiosity, care to share what's the use case? Do you plan to have
> >> the OS stored into an NVME device?
> >
> > For us the question is basically whether that use case does mandate enforcing such changes for each and every
> > customer. Plus the regular expected maintenance effort any such change brings with it, of course.
>
> You can always enable this support on MX8MP EVK, it has M2 slot and this
> would add build coverage of this code too, without impacting Verdin.

I would have chosen that as the base platform to enable but
unfortunately I don't have that at my desk. However, if someone is
willing to test this patch-set on MX8MP EVK then I am happy to extend
corresponding defconfig too.

-Sumit

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marek Vasut]

On 2/21/24 13:12, Sumit Garg wrote:
> On Wed, 21 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/21/24 10:18, Marcel Ziswiler wrote:
>>> Hi Sumit
>>>
>>> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
>>>> Hello Sumit,
>>>>
>>>> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
>>>>> Also, enable reset driver which is a prerequisite for PCIe support.
>>>>>
>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>> ---
>>>>>    configs/verdin-imx8mp_defconfig | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
>>>>> index 22b8a334dfa..d8bd644322b 100644
>>>>> --- a/configs/verdin-imx8mp_defconfig
>>>>> +++ b/configs/verdin-imx8mp_defconfig
>>>>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
>>>>>    CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>>>>>    CONFIG_IMX_WATCHDOG=y
>>>>>    CONFIG_HEXDUMP=y
>>>>> +CONFIG_DM_RESET=y
>>>>> +CONFIG_RESET_IMX=y
>>>>> +CONFIG_PCI=y
>>>>> +CONFIG_PCIE_DW_IMX8=y
>>>>> +CONFIG_PHY_IMX8M_PCIE=y
>>>>> +CONFIG_CMD_PCI=y
>>>>> +CONFIG_NVME=y
>>>>> +CONFIG_NVME_PCI=y
>>>>> +CONFIG_CMD_NVME=y
>>>>
>>>> This will increase the u-boot proper size
>>>
>>> Yes, I checked and it is actually slightly more than 32 K.
>>>
>>>> and marginally increase the
>>>> boot time (because of a bigger binary to be read from the eMMC).
>>>
>>> That was also my concern.
>>>
>>>> Apart of that do you expect any other impact on those changes? SPL
>>>> binary size should not be affected, correct?
>>>>
>>>> Asking this out loudly to confirm that nothing unexpected is going to
>>>> happen because of these changes.
>>>
>>> Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
>>> affected (other than the slight size and boot time increase, of course).
>>>
>>>> For my curiosity, care to share what's the use case? Do you plan to have
>>>> the OS stored into an NVME device?
>>>
>>> For us the question is basically whether that use case does mandate enforcing such changes for each and every
>>> customer. Plus the regular expected maintenance effort any such change brings with it, of course.
>>
>> You can always enable this support on MX8MP EVK, it has M2 slot and this
>> would add build coverage of this code too, without impacting Verdin.
> 
> I would have chosen that as the base platform to enable but
> unfortunately I don't have that at my desk. However, if someone is
> willing to test this patch-set on MX8MP EVK then I am happy to extend
> corresponding defconfig too.

I think we can surely find a platform on which this can be enabled by 
default and tested by people.

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Adam Ford]

On Wed, Feb 21, 2024 at 6:27 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/21/24 13:12, Sumit Garg wrote:
> > On Wed, 21 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/21/24 10:18, Marcel Ziswiler wrote:
> >>> Hi Sumit
> >>>
> >>> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> >>>> Hello Sumit,
> >>>>
> >>>> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> >>>>> Also, enable reset driver which is a prerequisite for PCIe support.
> >>>>>
> >>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>>>> ---
> >>>>>    configs/verdin-imx8mp_defconfig | 9 +++++++++
> >>>>>    1 file changed, 9 insertions(+)
> >>>>>
> >>>>> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> >>>>> index 22b8a334dfa..d8bd644322b 100644
> >>>>> --- a/configs/verdin-imx8mp_defconfig
> >>>>> +++ b/configs/verdin-imx8mp_defconfig
> >>>>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> >>>>>    CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> >>>>>    CONFIG_IMX_WATCHDOG=y
> >>>>>    CONFIG_HEXDUMP=y
> >>>>> +CONFIG_DM_RESET=y
> >>>>> +CONFIG_RESET_IMX=y
> >>>>> +CONFIG_PCI=y
> >>>>> +CONFIG_PCIE_DW_IMX8=y
> >>>>> +CONFIG_PHY_IMX8M_PCIE=y
> >>>>> +CONFIG_CMD_PCI=y
> >>>>> +CONFIG_NVME=y
> >>>>> +CONFIG_NVME_PCI=y
> >>>>> +CONFIG_CMD_NVME=y
> >>>>
> >>>> This will increase the u-boot proper size
> >>>
> >>> Yes, I checked and it is actually slightly more than 32 K.
> >>>
> >>>> and marginally increase the
> >>>> boot time (because of a bigger binary to be read from the eMMC).
> >>>
> >>> That was also my concern.
> >>>
> >>>> Apart of that do you expect any other impact on those changes? SPL
> >>>> binary size should not be affected, correct?
> >>>>
> >>>> Asking this out loudly to confirm that nothing unexpected is going to
> >>>> happen because of these changes.
> >>>
> >>> Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> >>> affected (other than the slight size and boot time increase, of course).
> >>>
> >>>> For my curiosity, care to share what's the use case? Do you plan to have
> >>>> the OS stored into an NVME device?
> >>>
> >>> For us the question is basically whether that use case does mandate enforcing such changes for each and every
> >>> customer. Plus the regular expected maintenance effort any such change brings with it, of course.
> >>
> >> You can always enable this support on MX8MP EVK, it has M2 slot and this
> >> would add build coverage of this code too, without impacting Verdin.
> >
> > I would have chosen that as the base platform to enable but
> > unfortunately I don't have that at my desk. However, if someone is
> > willing to test this patch-set on MX8MP EVK then I am happy to extend
> > corresponding defconfig too.
>
> I think we can surely find a platform on which this can be enabled by
> default and tested by people.

My development machine crashed (my fault for running a pre-release
OS),but I have an i.MX8M Plus and an NVMe drive that I can test once I
get my machine running again.

adam

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marek Vasut]

On 2/21/24 14:15, Adam Ford wrote:
> On Wed, Feb 21, 2024 at 6:27 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/21/24 13:12, Sumit Garg wrote:
>>> On Wed, 21 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 2/21/24 10:18, Marcel Ziswiler wrote:
>>>>> Hi Sumit
>>>>>
>>>>> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
>>>>>> Hello Sumit,
>>>>>>
>>>>>> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
>>>>>>> Also, enable reset driver which is a prerequisite for PCIe support.
>>>>>>>
>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>>>> ---
>>>>>>>     configs/verdin-imx8mp_defconfig | 9 +++++++++
>>>>>>>     1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
>>>>>>> index 22b8a334dfa..d8bd644322b 100644
>>>>>>> --- a/configs/verdin-imx8mp_defconfig
>>>>>>> +++ b/configs/verdin-imx8mp_defconfig
>>>>>>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
>>>>>>>     CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>>>>>>>     CONFIG_IMX_WATCHDOG=y
>>>>>>>     CONFIG_HEXDUMP=y
>>>>>>> +CONFIG_DM_RESET=y
>>>>>>> +CONFIG_RESET_IMX=y
>>>>>>> +CONFIG_PCI=y
>>>>>>> +CONFIG_PCIE_DW_IMX8=y
>>>>>>> +CONFIG_PHY_IMX8M_PCIE=y
>>>>>>> +CONFIG_CMD_PCI=y
>>>>>>> +CONFIG_NVME=y
>>>>>>> +CONFIG_NVME_PCI=y
>>>>>>> +CONFIG_CMD_NVME=y
>>>>>>
>>>>>> This will increase the u-boot proper size
>>>>>
>>>>> Yes, I checked and it is actually slightly more than 32 K.
>>>>>
>>>>>> and marginally increase the
>>>>>> boot time (because of a bigger binary to be read from the eMMC).
>>>>>
>>>>> That was also my concern.
>>>>>
>>>>>> Apart of that do you expect any other impact on those changes? SPL
>>>>>> binary size should not be affected, correct?
>>>>>>
>>>>>> Asking this out loudly to confirm that nothing unexpected is going to
>>>>>> happen because of these changes.
>>>>>
>>>>> Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
>>>>> affected (other than the slight size and boot time increase, of course).
>>>>>
>>>>>> For my curiosity, care to share what's the use case? Do you plan to have
>>>>>> the OS stored into an NVME device?
>>>>>
>>>>> For us the question is basically whether that use case does mandate enforcing such changes for each and every
>>>>> customer. Plus the regular expected maintenance effort any such change brings with it, of course.
>>>>
>>>> You can always enable this support on MX8MP EVK, it has M2 slot and this
>>>> would add build coverage of this code too, without impacting Verdin.
>>>
>>> I would have chosen that as the base platform to enable but
>>> unfortunately I don't have that at my desk. However, if someone is
>>> willing to test this patch-set on MX8MP EVK then I am happy to extend
>>> corresponding defconfig too.
>>
>> I think we can surely find a platform on which this can be enabled by
>> default and tested by people.
> 
> My development machine crashed (my fault for running a pre-release
> OS),but I have an i.MX8M Plus and an NVMe drive that I can test once I
> get my machine running again.

I can test right away, although I'll wait for V2 with all the fixes.

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Francesco Dolcini]

On Wed, Feb 21, 2024 at 09:18:51AM +0000, Marcel Ziswiler wrote:
> On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> > > Also, enable reset driver which is a prerequisite for PCIe support.
> > > 
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > >  configs/verdin-imx8mp_defconfig | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > > index 22b8a334dfa..d8bd644322b 100644
> > > --- a/configs/verdin-imx8mp_defconfig
> > > +++ b/configs/verdin-imx8mp_defconfig
> > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> > >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> > >  CONFIG_IMX_WATCHDOG=y
> > >  CONFIG_HEXDUMP=y
> > > +CONFIG_DM_RESET=y
> > > +CONFIG_RESET_IMX=y
> > > +CONFIG_PCI=y
> > > +CONFIG_PCIE_DW_IMX8=y
> > > +CONFIG_PHY_IMX8M_PCIE=y
> > > +CONFIG_CMD_PCI=y
> > > +CONFIG_NVME=y
> > > +CONFIG_NVME_PCI=y
> > > +CONFIG_CMD_NVME=y
> > 
> > This will increase the u-boot proper size
> 
> Yes, I checked and it is actually slightly more than 32 K.
> 
> > and marginally increase the
> > boot time (because of a bigger binary to be read from the eMMC).
> 
> That was also my concern.
> 
> > Apart of that do you expect any other impact on those changes? SPL
> > binary size should not be affected, correct?
> > 
> > Asking this out loudly to confirm that nothing unexpected is going to
> > happen because of these changes.
> 
> Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> affected (other than the slight size and boot time increase, of course).
> 
> > For my curiosity, care to share what's the use case? Do you plan to have
> > the OS stored into an NVME device?
> 
> For us the question is basically whether that use case does mandate
> enforcing such changes for each and every customer. Plus the regular
> expected maintenance effort any such change brings with it, of course.

Marcel, given all you wrote here I would personally be fine
on having this enabled in the verdin_imx8mp defconfig.

What's your idea? Are you good with it?

Francesco

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marcel Ziswiler]

On Wed, 2024-02-21 at 15:31 +0100, Francesco Dolcini wrote:
> On Wed, Feb 21, 2024 at 09:18:51AM +0000, Marcel Ziswiler wrote:
> > On Wed, 2024-02-21 at 08:55 +0100, Francesco Dolcini wrote:
> > > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> > > > Also, enable reset driver which is a prerequisite for PCIe support.
> > > > 
> > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > ---
> > > >  configs/verdin-imx8mp_defconfig | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> > > > index 22b8a334dfa..d8bd644322b 100644
> > > > --- a/configs/verdin-imx8mp_defconfig
> > > > +++ b/configs/verdin-imx8mp_defconfig
> > > > @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> > > >  CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> > > >  CONFIG_IMX_WATCHDOG=y
> > > >  CONFIG_HEXDUMP=y
> > > > +CONFIG_DM_RESET=y
> > > > +CONFIG_RESET_IMX=y
> > > > +CONFIG_PCI=y
> > > > +CONFIG_PCIE_DW_IMX8=y
> > > > +CONFIG_PHY_IMX8M_PCIE=y
> > > > +CONFIG_CMD_PCI=y
> > > > +CONFIG_NVME=y
> > > > +CONFIG_NVME_PCI=y
> > > > +CONFIG_CMD_NVME=y
> > > 
> > > This will increase the u-boot proper size
> > 
> > Yes, I checked and it is actually slightly more than 32 K.
> > 
> > > and marginally increase the
> > > boot time (because of a bigger binary to be read from the eMMC).
> > 
> > That was also my concern.
> > 
> > > Apart of that do you expect any other impact on those changes? SPL
> > > binary size should not be affected, correct?
> > > 
> > > Asking this out loudly to confirm that nothing unexpected is going to
> > > happen because of these changes.
> > 
> > Other than that I actually gave it a quick try and PCIe/NVMe does indeed work and the regular boot is not
> > affected (other than the slight size and boot time increase, of course).
> > 
> > > For my curiosity, care to share what's the use case? Do you plan to have
> > > the OS stored into an NVME device?
> > 
> > For us the question is basically whether that use case does mandate
> > enforcing such changes for each and every customer. Plus the regular
> > expected maintenance effort any such change brings with it, of course.
> 
> Marcel, given all you wrote here I would personally be fine
> on having this enabled in the verdin_imx8mp defconfig.
> 
> What's your idea? Are you good with it?

Yes, I do agree.

> Francesco

Cheers

Marcel

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Marek Vasut]

On 2/21/24 08:55, Francesco Dolcini wrote:
> Hello Sumit,
> 
> On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
>> Also, enable reset driver which is a prerequisite for PCIe support.
>>
>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>> ---
>>   configs/verdin-imx8mp_defconfig | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
>> index 22b8a334dfa..d8bd644322b 100644
>> --- a/configs/verdin-imx8mp_defconfig
>> +++ b/configs/verdin-imx8mp_defconfig
>> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
>>   CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
>>   CONFIG_IMX_WATCHDOG=y
>>   CONFIG_HEXDUMP=y
>> +CONFIG_DM_RESET=y
>> +CONFIG_RESET_IMX=y
>> +CONFIG_PCI=y
>> +CONFIG_PCIE_DW_IMX8=y
>> +CONFIG_PHY_IMX8M_PCIE=y
>> +CONFIG_CMD_PCI=y
>> +CONFIG_NVME=y
>> +CONFIG_NVME_PCI=y
>> +CONFIG_CMD_NVME=y
> 
> This will increase the u-boot proper size and marginally increase the
> boot time (because of a bigger binary to be read from the eMMC).
> 
> Apart of that do you expect any other impact on those changes? SPL
> binary size should not be affected, correct?
> 
> Asking this out loudly to confirm that nothing unexpected is going to
> happen because of these changes.
> 
> For my curiosity, care to share what's the use case? Do you plan to have
> the OS stored into an NVME device?

Boot OS images from NVMe is useful if eMMC size no longer cuts it 
(because debug symbols or to collect logs over long periods of time).

Re: [PATCH 7/7] verdin-imx8mp_defconfig: Enable PCIe/NVMe support

[Sumit Garg]

On Wed, 21 Feb 2024 at 16:11, Marek Vasut <marex@denx.de> wrote:
>
> On 2/21/24 08:55, Francesco Dolcini wrote:
> > Hello Sumit,
> >
> > On Tue, Feb 20, 2024 at 06:40:56PM +0530, Sumit Garg wrote:
> >> Also, enable reset driver which is a prerequisite for PCIe support.
> >>
> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >> ---
> >>   configs/verdin-imx8mp_defconfig | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/configs/verdin-imx8mp_defconfig b/configs/verdin-imx8mp_defconfig
> >> index 22b8a334dfa..d8bd644322b 100644
> >> --- a/configs/verdin-imx8mp_defconfig
> >> +++ b/configs/verdin-imx8mp_defconfig
> >> @@ -185,3 +185,12 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x1b67
> >>   CONFIG_USB_GADGET_PRODUCT_NUM=0x4000
> >>   CONFIG_IMX_WATCHDOG=y
> >>   CONFIG_HEXDUMP=y
> >> +CONFIG_DM_RESET=y
> >> +CONFIG_RESET_IMX=y
> >> +CONFIG_PCI=y
> >> +CONFIG_PCIE_DW_IMX8=y
> >> +CONFIG_PHY_IMX8M_PCIE=y
> >> +CONFIG_CMD_PCI=y
> >> +CONFIG_NVME=y
> >> +CONFIG_NVME_PCI=y
> >> +CONFIG_CMD_NVME=y
> >
> > This will increase the u-boot proper size and marginally increase the
> > boot time (because of a bigger binary to be read from the eMMC).
> >
> > Apart of that do you expect any other impact on those changes? SPL
> > binary size should not be affected, correct?
> >
> > Asking this out loudly to confirm that nothing unexpected is going to
> > happen because of these changes.
> >
> > For my curiosity, care to share what's the use case? Do you plan to have
> > the OS stored into an NVME device?
>
> Boot OS images from NVMe is useful if eMMC size no longer cuts it
> (because debug symbols or to collect logs over long periods of time).

Yeah especially if one wants to run a fully fledged Linux distro image.

-Sumit

Re: [PATCH 0/7] imx8mp: Enable PCIe/NVMe support

[Marek Vasut]

On 2/20/24 14:10, Sumit Garg wrote:
> pcie_imx doesn't seem to share any useful code for iMX8MP SoC and it is
> rather tied to quite old port of pcie_designware driver from Linux which
> suffices only iMX6 specific needs.
> 
> But currently we have the common DWC specific bits which alligns pretty
> well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
> bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
> add support for other iMX8 variants to this driver.
> 
> iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
> can reuse the generic PHY infrastructure to power on PCIe PHY.
> 
> Patch #1: Adds PCIe clocks support.
> Patch #2: Adds i.MX8MP reset controller support.
> Patch #3: Extend i.MX8MP power domain driver with PCIe support
> Patch #4: Expose high performance PLL clock required for PCIe PHY
>            on verdin board.
> Patch #5: Adds standalone PCIe PHY support for i.MX8MP SoC.
> Patch #6: Adds DW PCIe controller support for iMX8MP SoC.
> Patch #7: Enable PCIe/NVMe support for verdin board.
> 
> Testing with this patch-set included:
> 
> Verdin iMX8MP # pci enum
> PCIE-0: Link up (Gen1-x1, Bus0)
> Verdin iMX8MP #
> Verdin iMX8MP # nvme scan
> Verdin iMX8MP #
> Verdin iMX8MP # nvme info
> Device 0: Vendor: 0x126f Rev: T0828A0  Prod: AA000000000000000720
>              Type: Hard Disk
>              Capacity: 122104.3 MB = 119.2 GB (250069680 x 512)
> Verdin iMX8MP #
> Verdin iMX8MP # load nvme 0 $loadaddr <file-name>
> 
> Sumit Garg (7):
>    clk: imx8mp: Add support for PCIe clocks
>    reset: imx: Add support for i.MX8MP reset controller
>    imx8mp: power-domain: Add PCIe support
>    imx8mp: power-domain: Expose high performance PLL clock
>    phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY
>    pci: Add DW PCIe controller support for iMX8MP SoC
>    verdin-imx8mp_defconfig: Enable PCIe/NVMe support
> 
>   configs/verdin-imx8mp_defconfig       |   9 +
>   drivers/clk/imx/clk-imx8mp.c          |   6 +
>   drivers/pci/Kconfig                   |   8 +
>   drivers/pci/Makefile                  |   1 +
>   drivers/pci/pcie_dw_imx8.c            | 348 ++++++++++++++++++++++++++

You can call this pcie_dw_imx.c , the imx6 support can be converted over 
to that driver too I guess ?

Re: [PATCH 0/7] imx8mp: Enable PCIe/NVMe support

[Sumit Garg]

On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>
> On 2/20/24 14:10, Sumit Garg wrote:
> > pcie_imx doesn't seem to share any useful code for iMX8MP SoC and it is
> > rather tied to quite old port of pcie_designware driver from Linux which
> > suffices only iMX6 specific needs.
> >
> > But currently we have the common DWC specific bits which alligns pretty
> > well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
> > bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
> > add support for other iMX8 variants to this driver.
> >
> > iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
> > can reuse the generic PHY infrastructure to power on PCIe PHY.
> >
> > Patch #1: Adds PCIe clocks support.
> > Patch #2: Adds i.MX8MP reset controller support.
> > Patch #3: Extend i.MX8MP power domain driver with PCIe support
> > Patch #4: Expose high performance PLL clock required for PCIe PHY
> >            on verdin board.
> > Patch #5: Adds standalone PCIe PHY support for i.MX8MP SoC.
> > Patch #6: Adds DW PCIe controller support for iMX8MP SoC.
> > Patch #7: Enable PCIe/NVMe support for verdin board.
> >
> > Testing with this patch-set included:
> >
> > Verdin iMX8MP # pci enum
> > PCIE-0: Link up (Gen1-x1, Bus0)
> > Verdin iMX8MP #
> > Verdin iMX8MP # nvme scan
> > Verdin iMX8MP #
> > Verdin iMX8MP # nvme info
> > Device 0: Vendor: 0x126f Rev: T0828A0  Prod: AA000000000000000720
> >              Type: Hard Disk
> >              Capacity: 122104.3 MB = 119.2 GB (250069680 x 512)
> > Verdin iMX8MP #
> > Verdin iMX8MP # load nvme 0 $loadaddr <file-name>
> >
> > Sumit Garg (7):
> >    clk: imx8mp: Add support for PCIe clocks
> >    reset: imx: Add support for i.MX8MP reset controller
> >    imx8mp: power-domain: Add PCIe support
> >    imx8mp: power-domain: Expose high performance PLL clock
> >    phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY
> >    pci: Add DW PCIe controller support for iMX8MP SoC
> >    verdin-imx8mp_defconfig: Enable PCIe/NVMe support
> >
> >   configs/verdin-imx8mp_defconfig       |   9 +
> >   drivers/clk/imx/clk-imx8mp.c          |   6 +
> >   drivers/pci/Kconfig                   |   8 +
> >   drivers/pci/Makefile                  |   1 +
> >   drivers/pci/pcie_dw_imx8.c            | 348 ++++++++++++++++++++++++++
>
> You can call this pcie_dw_imx.c , the imx6 support can be converted over
> to that driver too I guess ?

Yeah I suppose that should be possible, let me rename it as pcie_dw_imx.c.

-Sumit

Re: [PATCH 0/7] imx8mp: Enable PCIe/NVMe support

[Marek Vasut]

On 2/21/24 06:25, Sumit Garg wrote:
> On Tue, 20 Feb 2024 at 21:02, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/20/24 14:10, Sumit Garg wrote:
>>> pcie_imx doesn't seem to share any useful code for iMX8MP SoC and it is
>>> rather tied to quite old port of pcie_designware driver from Linux which
>>> suffices only iMX6 specific needs.
>>>
>>> But currently we have the common DWC specific bits which alligns pretty
>>> well with DW PCIe controller on iMX8MP SoC. So lets reuse those common
>>> bits instead as a new driver for iMX8 SoCs. It should be fairly easy to
>>> add support for other iMX8 variants to this driver.
>>>
>>> iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we
>>> can reuse the generic PHY infrastructure to power on PCIe PHY.
>>>
>>> Patch #1: Adds PCIe clocks support.
>>> Patch #2: Adds i.MX8MP reset controller support.
>>> Patch #3: Extend i.MX8MP power domain driver with PCIe support
>>> Patch #4: Expose high performance PLL clock required for PCIe PHY
>>>             on verdin board.
>>> Patch #5: Adds standalone PCIe PHY support for i.MX8MP SoC.
>>> Patch #6: Adds DW PCIe controller support for iMX8MP SoC.
>>> Patch #7: Enable PCIe/NVMe support for verdin board.
>>>
>>> Testing with this patch-set included:
>>>
>>> Verdin iMX8MP # pci enum
>>> PCIE-0: Link up (Gen1-x1, Bus0)
>>> Verdin iMX8MP #
>>> Verdin iMX8MP # nvme scan
>>> Verdin iMX8MP #
>>> Verdin iMX8MP # nvme info
>>> Device 0: Vendor: 0x126f Rev: T0828A0  Prod: AA000000000000000720
>>>               Type: Hard Disk
>>>               Capacity: 122104.3 MB = 119.2 GB (250069680 x 512)
>>> Verdin iMX8MP #
>>> Verdin iMX8MP # load nvme 0 $loadaddr <file-name>
>>>
>>> Sumit Garg (7):
>>>     clk: imx8mp: Add support for PCIe clocks
>>>     reset: imx: Add support for i.MX8MP reset controller
>>>     imx8mp: power-domain: Add PCIe support
>>>     imx8mp: power-domain: Expose high performance PLL clock
>>>     phy: phy-imx8m-pcie: Add support for i.MX8M{M/P} PCIe PHY
>>>     pci: Add DW PCIe controller support for iMX8MP SoC
>>>     verdin-imx8mp_defconfig: Enable PCIe/NVMe support
>>>
>>>    configs/verdin-imx8mp_defconfig       |   9 +
>>>    drivers/clk/imx/clk-imx8mp.c          |   6 +
>>>    drivers/pci/Kconfig                   |   8 +
>>>    drivers/pci/Makefile                  |   1 +
>>>    drivers/pci/pcie_dw_imx8.c            | 348 ++++++++++++++++++++++++++
>>
>> You can call this pcie_dw_imx.c , the imx6 support can be converted over
>> to that driver too I guess ?
> 
> Yeah I suppose that should be possible, let me rename it as pcie_dw_imx.c.

Thanks