On 2/26/24 9:04 AM, Sumit Garg wrote:

Add support for GPCv2 power domains and clock handling for PCIe and PCIe PHY.

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/power/domain/imx8mp-hsiomix.c | 101 ++++++++++++++++++++------ 1 file changed, 78 insertions(+), 23 deletions(-)

diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c index e2d772c5ec78..58cc3f63bb56 100644 --- a/drivers/power/domain/imx8mp-hsiomix.c +++ b/drivers/power/domain/imx8mp-hsiomix.c @@ -16,48 +16,73 @@ #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) { struct udevice *dev = power_domain->dev; struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);

• struct power_domain *domain;

• struct power_domain *domain = NULL;
• struct clk *clk = NULL;
• u32 gpr_reg0_bits = 0; int ret;

• ret = power_domain_on(&priv->pd_bus);
• if (ret)

• return ret;
  

• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {

• switch (power_domain->id) {
• case IMX8MP_HSIOBLK_PD_USB: domain = &priv->pd_usb;

• } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY1) {

• clk = &priv->clk_usb;
  

• gpr_reg0_bits |= USB_CLOCK_MODULE_EN;
  

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY1: domain = &priv->pd_usb_phy1;

• } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY2: domain = &priv->pd_usb_phy2;

• } else {

• ret = -EINVAL;
  

• goto err_pd;
  

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE:

• domain = &priv->pd_pcie;
  

• clk = &priv->clk_pcie;
  

• gpr_reg0_bits |= PCIE_CLOCK_MODULE_EN;
  

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE_PHY:

• domain = &priv->pd_pcie_phy;
  

• /* Bits to deassert PCIe PHY reset */
  

• gpr_reg0_bits |= PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST;
  

• break;
  

• default:

• dev_err(dev, "unknown power domain id: %ld\n",
  

• 	power_domain->id);
  

• return -EINVAL;
  

  }

• ret = power_domain_on(&priv->pd_bus);

• if (ret)

• return ret;
  

• ret = power_domain_on(domain); if (ret) goto err_pd;

• ret = clk_enable(&priv->clk_usb);
• if (ret)

• goto err_clk;
  

• if (clk) {

• ret = clk_enable(clk);
  

• if (ret)
  

• 	goto err_clk;
  

• }

• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)

• setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
  

• setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);

Are you sure it is OK to do this unconditionally ?

Why not this:

if (gpr_reg0_bits) setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);

return 0;

@@ -73,17 +98,31 @@ 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)

• switch (power_domain->id) {
• case IMX8MP_HSIOBLK_PD_USB: clrbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);

• clk_disable(&priv->clk_usb);
• if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)

• clk_disable(&priv->clk_usb);
  

  power_domain_off(&priv->pd_usb);

• else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY1)

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY1: power_domain_off(&priv->pd_usb_phy1);

• else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2)

• break;
  

• case IMX8MP_HSIOBLK_PD_USB_PHY2: power_domain_off(&priv->pd_usb_phy2);

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE:

Maybe we need some function ... imx8mp_hsiomix_id_to_pd() to avoid this duplicate switch statement:

pd = imx8mp_hsiomix_id_to_pd(...) ... power_domain_off(fd);

• clrbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
  

• clk_disable(&priv->clk_pcie);
  

• power_domain_off(&priv->pd_pcie);
  

• break;
  

• case IMX8MP_HSIOBLK_PD_PCIE_PHY:

• clrbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
  

• 				    PCIE_PHY_INIT_RST);
  

• power_domain_off(&priv->pd_pcie_phy);
  

Is it OK to turn off the bus PD after this ? Please double-check if power_domain_on/off() is refcounted .

• break;
  

• default:

• break;
  

• }

  power_domain_off(&priv->pd_bus);

@@ -109,6 +148,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;

Some clk_put() seems to be missing for clk_pcie .

On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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.

Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/phy/freescale/phy-fsl-imx8m-pcie.c.

I have a PCIe device that works just fine in Linux. I have applied your series an enabled the same config options as you did along with some others to resolve some build issues.

Any ideas how to address:

nxp_imx8_pcie_phy pcie-phy@32f00000: PHY: Failed to power on pcie-phy@32f00000: -110.

My PHY node looks like this

&pcie_phy { fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>; clocks = <&pcie0_refclk>; clock-names = "ref"; status = "okay"; };

The pcie_refcllk is defined as a 100MHz fixed clock in U-Boot. clk dump shows it to be:

100000000 0 |-- clock-pcie

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/phy/Kconfig | 9 ++ drivers/phy/Makefile | 1 + drivers/phy/phy-imx8m-pcie.c | 269 +++++++++++++++++++++++++++++++++++ 3 files changed, 279 insertions(+) create mode 100644 drivers/phy/phy-imx8m-pcie.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 60138beca498..110ec8f5008d 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
  

This should also depend on SYSCON and REGMAP to prevent build errors.

•   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 2e8723186c05..7a2b764492be 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 000000000000..e09a7ac97235 --- /dev/null +++ b/drivers/phy/phy-imx8m-pcie.c @@ -0,0 +1,269 @@ +// 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 clk              hsio_clk;
  

•   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 int imx8_pcie_phy_init(struct phy *phy) +{

•   struct imx8_pcie_phy *imx8_phy = dev_get_priv(phy->dev);
  

•   return clk_enable(&imx8_phy->hsio_clk);
  

+}

+static int imx8_pcie_phy_exit(struct phy *phy) +{

•   struct imx8_pcie_phy *imx8_phy = dev_get_priv(phy->dev);
  

•   return clk_disable(&imx8_phy->hsio_clk);
  

+}

+static const struct phy_ops imx8_pcie_phy_ops = {

•   .init           = imx8_pcie_phy_init,
  

•   .exit           = imx8_pcie_phy_exit,
  

•   .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 = clk_get_by_name(dev, "ref", &imx8_phy->hsio_clk);
  

•   if (ret) {
  

•           dev_err(dev, "Failed to get PCIEPHY ref clock\n");
  

•           return ret;
  

•   }
  

•   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

Hi Adam,

On Wed, 28 Feb 2024 at 12:29, Sumit Garg sumit.garg@linaro.org wrote:

Hi Adam,

On Wed, 28 Feb 2024 at 04:42, Adam Ford aford173@gmail.com wrote:

...

On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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.

Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/phy/freescale/phy-fsl-imx8m-pcie.c.

I have a PCIe device that works just fine in Linux. I have applied your series an enabled the same config options as you did along with some others to resolve some build issues.

Any ideas how to address:

nxp_imx8_pcie_phy pcie-phy@32f00000: PHY: Failed to power on pcie-phy@32f00000: -110.

My PHY node looks like this

&pcie_phy { fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>; clocks = <&pcie0_refclk>; clock-names = "ref"; status = "okay"; };

The pcie_refcllk is defined as a 100MHz fixed clock in U-Boot. clk dump shows it to be:

100000000 0 |-- clock-pcie

I suppose that's an EVK board, try enabling MPCIE_3V3 regulator required for EVK board via following patch:

diff --git a/drivers/pci/pcie_dw_imx.c b/drivers/pci/pcie_dw_imx.c index 7856823c9188..32fd2cb05d4b 100644 --- a/drivers/pci/pcie_dw_imx.c +++ b/drivers/pci/pcie_dw_imx.c @@ -17,6 +17,7 @@ #include <linux/iopoll.h> #include <log.h> #include <pci.h> +#include <power/regulator.h> #include <regmap.h> #include <reset.h> #include <syscon.h> @@ -158,6 +159,8 @@ static int pcie_dw_imx_probe(struct udevice *dev) struct pci_controller *hose = dev_get_uclass_priv(ctlr); int ret;

•   regulator_autoset_by_name("MPCIE_3V3", NULL);
  

•   ret = imx_pcie_assert_core_reset(priv);
    if (ret) {
            dev_err(dev, "failed to assert core reset\n");
  
  

Were you able to give a retry with the above diff?

-Sumit

On Tue, Mar 5, 2024 at 7:51 AM Adam Ford aford173@gmail.com wrote:

On Tue, Mar 5, 2024 at 7:48 AM Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 12:29, Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 04:42, Adam Ford aford173@gmail.com wrote:

...

On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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.

Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/phy/freescale/phy-fsl-imx8m-pcie.c.

I have a PCIe device that works just fine in Linux. I have applied your series an enabled the same config options as you did along with some others to resolve some build issues.

Any ideas how to address:

nxp_imx8_pcie_phy pcie-phy@32f00000: PHY: Failed to power on pcie-phy@32f00000: -110.

My PHY node looks like this

&pcie_phy { fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>; clocks = <&pcie0_refclk>; clock-names = "ref"; status = "okay"; };

The pcie_refcllk is defined as a 100MHz fixed clock in U-Boot. clk dump shows it to be:

100000000 0 |-- clock-pcie

I suppose that's an EVK board, try enabling MPCIE_3V3 regulator required for EVK board via following patch:

diff --git a/drivers/pci/pcie_dw_imx.c b/drivers/pci/pcie_dw_imx.c index 7856823c9188..32fd2cb05d4b 100644 --- a/drivers/pci/pcie_dw_imx.c +++ b/drivers/pci/pcie_dw_imx.c @@ -17,6 +17,7 @@ #include <linux/iopoll.h> #include <log.h> #include <pci.h> +#include <power/regulator.h> #include <regmap.h> #include <reset.h> #include <syscon.h> @@ -158,6 +159,8 @@ static int pcie_dw_imx_probe(struct udevice *dev) struct pci_controller *hose = dev_get_uclass_priv(ctlr); int ret;

•   regulator_autoset_by_name("MPCIE_3V3", NULL);
  

I think you should search the device tree for "vpcie-supply" and enable the corresponding regulator, because is more flexible than hard-coding regulator names. This is also more of a standard practice.

...

...

•   ret = imx_pcie_assert_core_reset(priv);
    if (ret) {
            dev_err(dev, "failed to assert core reset\n");
  
  

Were you able to give a retry with the above diff?

Not yet, but I'll try to do it tonight.

That didn't work for me. I am using a Beacon Embedded kit which does not use a regulator, so this had no impact, but I think having the vpcie-supply regulator is a good idea.

I'll investigate a bit more and let you know if I make any progress.

adam

adam

...

-Sumit

On Tue, Mar 5, 2024 at 4:47 PM Adam Ford aford173@gmail.com wrote:

On Tue, Mar 5, 2024 at 7:51 AM Adam Ford aford173@gmail.com wrote:

...

On Tue, Mar 5, 2024 at 7:48 AM Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 12:29, Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 04:42, Adam Ford aford173@gmail.com wrote:

...

On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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.

Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/phy/freescale/phy-fsl-imx8m-pcie.c.

I have a PCIe device that works just fine in Linux. I have applied your series an enabled the same config options as you did along with some others to resolve some build issues.

Any ideas how to address:

nxp_imx8_pcie_phy pcie-phy@32f00000: PHY: Failed to power on pcie-phy@32f00000: -110.

My PHY node looks like this

&pcie_phy { fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>; clocks = <&pcie0_refclk>; clock-names = "ref"; status = "okay"; };

The pcie_refcllk is defined as a 100MHz fixed clock in U-Boot. clk dump shows it to be:

100000000 0 |-- clock-pcie

I suppose that's an EVK board, try enabling MPCIE_3V3 regulator required for EVK board via following patch:

diff --git a/drivers/pci/pcie_dw_imx.c b/drivers/pci/pcie_dw_imx.c index 7856823c9188..32fd2cb05d4b 100644 --- a/drivers/pci/pcie_dw_imx.c +++ b/drivers/pci/pcie_dw_imx.c @@ -17,6 +17,7 @@ #include <linux/iopoll.h> #include <log.h> #include <pci.h> +#include <power/regulator.h> #include <regmap.h> #include <reset.h> #include <syscon.h> @@ -158,6 +159,8 @@ static int pcie_dw_imx_probe(struct udevice *dev) struct pci_controller *hose = dev_get_uclass_priv(ctlr); int ret;

•   regulator_autoset_by_name("MPCIE_3V3", NULL);
  

I think you should search the device tree for "vpcie-supply" and enable the corresponding regulator, because is more flexible than hard-coding regulator names. This is also more of a standard practice.

...

...

...

•   ret = imx_pcie_assert_core_reset(priv);
    if (ret) {
            dev_err(dev, "failed to assert core reset\n");
  
  

Were you able to give a retry with the above diff?

Not yet, but I'll try to do it tonight.

That didn't work for me. I am using a Beacon Embedded kit which does not use a regulator, so this had no impact, but I think having the vpcie-supply regulator is a good idea.

I'll investigate a bit more and let you know if I make any progress.

Adam and Sumit,

In case it helps this series works for PCI bus scanning for the imx8mp-venice-* boards with the following patch: diff --git a/configs/imx8mp_venice_defconfig b/configs/imx8mp_venice_defconfig index 11b356b77856..ff7ea9811ed6 100644 --- a/configs/imx8mp_venice_defconfig +++ b/configs/imx8mp_venice_defconfig @@ -12,6 +12,7 @@ CONFIG_DEFAULT_DEVICE_TREE="imx8mp-venice" CONFIG_SPL_TEXT_BASE=0x920000 CONFIG_TARGET_IMX8MP_VENICE=y CONFIG_OF_LIBFDT_OVERLAY=y +CONFIG_DM_RESET=y CONFIG_SYS_MONITOR_LEN=524288 CONFIG_SPL_MMC=y CONFIG_SPL_SERIAL=y @@ -21,6 +22,7 @@ CONFIG_SPL=y CONFIG_ENV_OFFSET_REDUND=0x3f8000 CONFIG_SPL_IMX_ROMAPI_LOADADDR=0x48000000 CONFIG_SYS_LOAD_ADDR=0x40480000 +CONFIG_PCI=y CONFIG_SYS_MEMTEST_START=0x40000000 CONFIG_SYS_MEMTEST_END=0x80000000 CONFIG_LTO=y @@ -65,6 +67,7 @@ CONFIG_CMD_FUSE=y CONFIG_CMD_GPIO=y CONFIG_CMD_I2C=y CONFIG_CMD_MMC=y +CONFIG_CMD_PCI=y CONFIG_CMD_USB=y CONFIG_SYS_DISABLE_AUTOLOAD=y CONFIG_CMD_CACHE=y @@ -86,6 +89,8 @@ CONFIG_NET_RANDOM_ETHADDR=y CONFIG_IP_DEFRAG=y CONFIG_TFTP_BLOCKSIZE=4096 CONFIG_SPL_DM=y +CONFIG_REGMAP=y +CONFIG_SYSCON=y CONFIG_CLK_COMPOSITE_CCF=y CONFIG_CLK_IMX8MP=y CONFIG_GPIO_HOG=y @@ -114,7 +119,10 @@ CONFIG_FEC_MXC=y CONFIG_KSZ9477=y CONFIG_RGMII=y CONFIG_MII=y +CONFIG_NVME_PCI=y +CONFIG_PCIE_DW_IMX=y CONFIG_PHY_IMX8MQ_USB=y +CONFIG_PHY_IMX8M_PCIE=y CONFIG_PINCTRL=y CONFIG_SPL_PINCTRL=y CONFIG_PINCTRL_IMX8M=y

on an imx8mp-venice-gw71xx-2x (imx8mp with miniPCIe socket using a miniPCIe to NVMe adapter) u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> pci BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.00.00 0x16c3 0xabcd Bridge device 0x04 01.00.00 0x10ec 0x5765 Mass storage controller 0x08 u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x10ec Rev: 0426-D00 Prod: 60272-0005 Type: Hard Disk Capacity: 57241.8 MB = 55.9 GB (117231408 x 512)

I also verified this works imx8mp-venice-gw74xx (imx8mp with PCIe switch to multiple miniPCIe sockets using a miniPCIe to NVMe adapter) u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> pci BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.00.00 0x16c3 0xabcd Bridge device 0x04 01.00.00 0x12d8 0x2608 Bridge device 0x04 02.01.00 0x12d8 0x2608 Bridge device 0x04 02.02.00 0x12d8 0x2608 Bridge device 0x04 02.03.00 0x12d8 0x2608 Bridge device 0x04 02.04.00 0x12d8 0x2608 Bridge device 0x04 05.00.00 0x10ec 0x5765 Mass storage controller 0x08 u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x10ec Rev: 0426-D00 Prod: 60272-0005 Type: Hard Disk Capacity: 57241.8 MB = 55.9 GB (117231408 x 512)

It looks like the imx8mp-beacon-kit has almost the same setup as the imx8mp-venice-gw74xx with the exception of you having a PCIe_nDIS gpio defined in pinctrl (used nowhere) and the gw74xx does not support CLKREQ so has 'fsl,clkreq-unsupported'.

tested-by: Tim Harvey tharvey@gateworks.com : imx8mp-venice*

Best regards,

Tim

On Thu, Mar 7, 2024 at 12:35 AM Sumit Garg sumit.garg@linaro.org wrote:

On Wed, 6 Mar 2024 at 22:56, Tim Harvey tharvey@gateworks.com wrote:

...

On Tue, Mar 5, 2024 at 4:47 PM Adam Ford aford173@gmail.com wrote:

...

On Tue, Mar 5, 2024 at 7:51 AM Adam Ford aford173@gmail.com wrote:

...

On Tue, Mar 5, 2024 at 7:48 AM Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 12:29, Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 04:42, Adam Ford aford173@gmail.com wrote: > > On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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. > > > > Inspired from counterpart Linux kernel v6.8-rc3 driver: > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c. > > I have a PCIe device that works just fine in Linux. I have applied > your series an enabled the same config options as you did along with > some others to resolve some build issues. > > Any ideas how to address: > > nxp_imx8_pcie_phy pcie-phy@32f00000: PHY: Failed to power on > pcie-phy@32f00000: -110. > > My PHY node looks like this > > &pcie_phy { > fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>; > clocks = <&pcie0_refclk>; > clock-names = "ref"; > status = "okay"; > }; > > The pcie_refcllk is defined as a 100MHz fixed clock in U-Boot. clk > dump shows it to be: > > 100000000 0 |-- clock-pcie >

I suppose that's an EVK board, try enabling MPCIE_3V3 regulator required for EVK board via following patch:

diff --git a/drivers/pci/pcie_dw_imx.c b/drivers/pci/pcie_dw_imx.c index 7856823c9188..32fd2cb05d4b 100644 --- a/drivers/pci/pcie_dw_imx.c +++ b/drivers/pci/pcie_dw_imx.c @@ -17,6 +17,7 @@ #include <linux/iopoll.h> #include <log.h> #include <pci.h> +#include <power/regulator.h> #include <regmap.h> #include <reset.h> #include <syscon.h> @@ -158,6 +159,8 @@ static int pcie_dw_imx_probe(struct udevice *dev) struct pci_controller *hose = dev_get_uclass_priv(ctlr); int ret;

•   regulator_autoset_by_name("MPCIE_3V3", NULL);
  

I think you should search the device tree for "vpcie-supply" and enable the corresponding regulator, because is more flexible than hard-coding regulator names. This is also more of a standard practice.

...

...

...

•   ret = imx_pcie_assert_core_reset(priv);
    if (ret) {
            dev_err(dev, "failed to assert core reset\n");
  
  

Were you able to give a retry with the above diff?

Not yet, but I'll try to do it tonight.

That didn't work for me. I am using a Beacon Embedded kit which does not use a regulator, so this had no impact, but I think having the vpcie-supply regulator is a good idea.

I'll investigate a bit more and let you know if I make any progress.

Adam and Sumit,

In case it helps this series works for PCI bus scanning for the imx8mp-venice-* boards with the following patch:

Thanks Tim for taking time to test this series. I hope you are fine with me taking your below patch with you being the author.

...

diff --git a/configs/imx8mp_venice_defconfig b/configs/imx8mp_venice_defconfig index 11b356b77856..ff7ea9811ed6 100644 --- a/configs/imx8mp_venice_defconfig +++ b/configs/imx8mp_venice_defconfig @@ -12,6 +12,7 @@ CONFIG_DEFAULT_DEVICE_TREE="imx8mp-venice" CONFIG_SPL_TEXT_BASE=0x920000 CONFIG_TARGET_IMX8MP_VENICE=y CONFIG_OF_LIBFDT_OVERLAY=y +CONFIG_DM_RESET=y CONFIG_SYS_MONITOR_LEN=524288 CONFIG_SPL_MMC=y CONFIG_SPL_SERIAL=y @@ -21,6 +22,7 @@ CONFIG_SPL=y CONFIG_ENV_OFFSET_REDUND=0x3f8000 CONFIG_SPL_IMX_ROMAPI_LOADADDR=0x48000000 CONFIG_SYS_LOAD_ADDR=0x40480000 +CONFIG_PCI=y CONFIG_SYS_MEMTEST_START=0x40000000 CONFIG_SYS_MEMTEST_END=0x80000000 CONFIG_LTO=y @@ -65,6 +67,7 @@ CONFIG_CMD_FUSE=y CONFIG_CMD_GPIO=y CONFIG_CMD_I2C=y CONFIG_CMD_MMC=y +CONFIG_CMD_PCI=y CONFIG_CMD_USB=y CONFIG_SYS_DISABLE_AUTOLOAD=y CONFIG_CMD_CACHE=y @@ -86,6 +89,8 @@ CONFIG_NET_RANDOM_ETHADDR=y CONFIG_IP_DEFRAG=y CONFIG_TFTP_BLOCKSIZE=4096 CONFIG_SPL_DM=y +CONFIG_REGMAP=y +CONFIG_SYSCON=y CONFIG_CLK_COMPOSITE_CCF=y CONFIG_CLK_IMX8MP=y CONFIG_GPIO_HOG=y @@ -114,7 +119,10 @@ CONFIG_FEC_MXC=y CONFIG_KSZ9477=y CONFIG_RGMII=y CONFIG_MII=y +CONFIG_NVME_PCI=y +CONFIG_PCIE_DW_IMX=y CONFIG_PHY_IMX8MQ_USB=y +CONFIG_PHY_IMX8M_PCIE=y CONFIG_PINCTRL=y CONFIG_SPL_PINCTRL=y CONFIG_PINCTRL_IMX8M=y

on an imx8mp-venice-gw71xx-2x (imx8mp with miniPCIe socket using a miniPCIe to NVMe adapter) u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> pci BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.00.00 0x16c3 0xabcd Bridge device 0x04 01.00.00 0x10ec 0x5765 Mass storage controller 0x08 u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x10ec Rev: 0426-D00 Prod: 60272-0005 Type: Hard Disk Capacity: 57241.8 MB = 55.9 GB (117231408 x 512)

I also verified this works imx8mp-venice-gw74xx (imx8mp with PCIe switch to multiple miniPCIe sockets using a miniPCIe to NVMe adapter) u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> pci BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.00.00 0x16c3 0xabcd Bridge device 0x04 01.00.00 0x12d8 0x2608 Bridge device 0x04 02.01.00 0x12d8 0x2608 Bridge device 0x04 02.02.00 0x12d8 0x2608 Bridge device 0x04 02.03.00 0x12d8 0x2608 Bridge device 0x04 02.04.00 0x12d8 0x2608 Bridge device 0x04 05.00.00 0x10ec 0x5765 Mass storage controller 0x08 u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x10ec Rev: 0426-D00 Prod: 60272-0005 Type: Hard Disk Capacity: 57241.8 MB = 55.9 GB (117231408 x 512)

It looks like the imx8mp-beacon-kit has almost the same setup as the imx8mp-venice-gw74xx with the exception of you having a PCIe_nDIS gpio defined in pinctrl (used nowhere) and the gw74xx does not support CLKREQ so has 'fsl,clkreq-unsupported'.

Ah, I see. So it turns out that it's Adam Linux kernel DTS patch [1] which is missing for U-Boot DTS. This is one of the reasons why we should switch to OF_UPSTREAM to start regular sync with Linux kernel DTS.

Adam,

Can you make corresponding changes to U-Boot DTS for imx8mp-beacon-kit? Or better just switch to OF_UPSTREAM instead?

I have it working now.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ar...

This is one of the first things I did when I tried the patch, but the renesas clock driver isn't available in U-Boot. With and without that above patch, the Linux driver enumerates, but the real issue appears to be something related to ATF (or lack thereof). I had experimented with a series of patches which bypassed ATF, and pushed them upstream a while ago. Unfortunately, it appears to have caused more issues. When I revert those it all works just fine. Sorry for the noise.

u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x15b7 Rev: 20120022 Prod: 184960441105 Type: Hard Disk Capacity: 122104.3 MB = 119.2 GB (250069680 x 512) u-boot=>

WIth my suggested regulator and build-dependency fixes added you can add

Tested-by: Adam Ford aford173@gmail.com #imx8mp-beacon-kit

adam

...

tested-by: Tim Harvey tharvey@gateworks.com : imx8mp-venice*

Thanks again.

-Sumit

...

Best regards,

Tim

On Wed, Mar 6, 2024 at 10:35 PM Sumit Garg sumit.garg@linaro.org wrote:

On Wed, 6 Mar 2024 at 22:56, Tim Harvey tharvey@gateworks.com wrote:

...

On Tue, Mar 5, 2024 at 4:47 PM Adam Ford aford173@gmail.com wrote:

...

On Tue, Mar 5, 2024 at 7:51 AM Adam Ford aford173@gmail.com wrote:

...

On Tue, Mar 5, 2024 at 7:48 AM Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 12:29, Sumit Garg sumit.garg@linaro.org wrote:

...

Hi Adam,

On Wed, 28 Feb 2024 at 04:42, Adam Ford aford173@gmail.com wrote: > > On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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. > > > > Inspired from counterpart Linux kernel v6.8-rc3 driver: > > drivers/phy/freescale/phy-fsl-imx8m-pcie.c. > > I have a PCIe device that works just fine in Linux. I have applied > your series an enabled the same config options as you did along with > some others to resolve some build issues. > > Any ideas how to address: > > nxp_imx8_pcie_phy pcie-phy@32f00000: PHY: Failed to power on > pcie-phy@32f00000: -110. > > My PHY node looks like this > > &pcie_phy { > fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_INPUT>; > clocks = <&pcie0_refclk>; > clock-names = "ref"; > status = "okay"; > }; > > The pcie_refcllk is defined as a 100MHz fixed clock in U-Boot. clk > dump shows it to be: > > 100000000 0 |-- clock-pcie >

I suppose that's an EVK board, try enabling MPCIE_3V3 regulator required for EVK board via following patch:

diff --git a/drivers/pci/pcie_dw_imx.c b/drivers/pci/pcie_dw_imx.c index 7856823c9188..32fd2cb05d4b 100644 --- a/drivers/pci/pcie_dw_imx.c +++ b/drivers/pci/pcie_dw_imx.c @@ -17,6 +17,7 @@ #include <linux/iopoll.h> #include <log.h> #include <pci.h> +#include <power/regulator.h> #include <regmap.h> #include <reset.h> #include <syscon.h> @@ -158,6 +159,8 @@ static int pcie_dw_imx_probe(struct udevice *dev) struct pci_controller *hose = dev_get_uclass_priv(ctlr); int ret;

•   regulator_autoset_by_name("MPCIE_3V3", NULL);
  

I think you should search the device tree for "vpcie-supply" and enable the corresponding regulator, because is more flexible than hard-coding regulator names. This is also more of a standard practice.

...

...

...

•   ret = imx_pcie_assert_core_reset(priv);
    if (ret) {
            dev_err(dev, "failed to assert core reset\n");
  
  

Were you able to give a retry with the above diff?

Not yet, but I'll try to do it tonight.

That didn't work for me. I am using a Beacon Embedded kit which does not use a regulator, so this had no impact, but I think having the vpcie-supply regulator is a good idea.

I'll investigate a bit more and let you know if I make any progress.

Adam and Sumit,

In case it helps this series works for PCI bus scanning for the imx8mp-venice-* boards with the following patch:

Thanks Tim for taking time to test this series. I hope you are fine with me taking your below patch with you being the author.

Sumit,

Yes you can take that patch if you want to add it to your series - thanks!

Tim

...

diff --git a/configs/imx8mp_venice_defconfig b/configs/imx8mp_venice_defconfig index 11b356b77856..ff7ea9811ed6 100644 --- a/configs/imx8mp_venice_defconfig +++ b/configs/imx8mp_venice_defconfig @@ -12,6 +12,7 @@ CONFIG_DEFAULT_DEVICE_TREE="imx8mp-venice" CONFIG_SPL_TEXT_BASE=0x920000 CONFIG_TARGET_IMX8MP_VENICE=y CONFIG_OF_LIBFDT_OVERLAY=y +CONFIG_DM_RESET=y CONFIG_SYS_MONITOR_LEN=524288 CONFIG_SPL_MMC=y CONFIG_SPL_SERIAL=y @@ -21,6 +22,7 @@ CONFIG_SPL=y CONFIG_ENV_OFFSET_REDUND=0x3f8000 CONFIG_SPL_IMX_ROMAPI_LOADADDR=0x48000000 CONFIG_SYS_LOAD_ADDR=0x40480000 +CONFIG_PCI=y CONFIG_SYS_MEMTEST_START=0x40000000 CONFIG_SYS_MEMTEST_END=0x80000000 CONFIG_LTO=y @@ -65,6 +67,7 @@ CONFIG_CMD_FUSE=y CONFIG_CMD_GPIO=y CONFIG_CMD_I2C=y CONFIG_CMD_MMC=y +CONFIG_CMD_PCI=y CONFIG_CMD_USB=y CONFIG_SYS_DISABLE_AUTOLOAD=y CONFIG_CMD_CACHE=y @@ -86,6 +89,8 @@ CONFIG_NET_RANDOM_ETHADDR=y CONFIG_IP_DEFRAG=y CONFIG_TFTP_BLOCKSIZE=4096 CONFIG_SPL_DM=y +CONFIG_REGMAP=y +CONFIG_SYSCON=y CONFIG_CLK_COMPOSITE_CCF=y CONFIG_CLK_IMX8MP=y CONFIG_GPIO_HOG=y @@ -114,7 +119,10 @@ CONFIG_FEC_MXC=y CONFIG_KSZ9477=y CONFIG_RGMII=y CONFIG_MII=y +CONFIG_NVME_PCI=y +CONFIG_PCIE_DW_IMX=y CONFIG_PHY_IMX8MQ_USB=y +CONFIG_PHY_IMX8M_PCIE=y CONFIG_PINCTRL=y CONFIG_SPL_PINCTRL=y CONFIG_PINCTRL_IMX8M=y

on an imx8mp-venice-gw71xx-2x (imx8mp with miniPCIe socket using a miniPCIe to NVMe adapter) u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> pci BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.00.00 0x16c3 0xabcd Bridge device 0x04 01.00.00 0x10ec 0x5765 Mass storage controller 0x08 u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x10ec Rev: 0426-D00 Prod: 60272-0005 Type: Hard Disk Capacity: 57241.8 MB = 55.9 GB (117231408 x 512)

I also verified this works imx8mp-venice-gw74xx (imx8mp with PCIe switch to multiple miniPCIe sockets using a miniPCIe to NVMe adapter) u-boot=> pci enum PCIE-0: Link up (Gen1-x1, Bus0) u-boot=> pci BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.00.00 0x16c3 0xabcd Bridge device 0x04 01.00.00 0x12d8 0x2608 Bridge device 0x04 02.01.00 0x12d8 0x2608 Bridge device 0x04 02.02.00 0x12d8 0x2608 Bridge device 0x04 02.03.00 0x12d8 0x2608 Bridge device 0x04 02.04.00 0x12d8 0x2608 Bridge device 0x04 05.00.00 0x10ec 0x5765 Mass storage controller 0x08 u-boot=> nvme scan u-boot=> nvme info Device 0: Vendor: 0x10ec Rev: 0426-D00 Prod: 60272-0005 Type: Hard Disk Capacity: 57241.8 MB = 55.9 GB (117231408 x 512)

It looks like the imx8mp-beacon-kit has almost the same setup as the imx8mp-venice-gw74xx with the exception of you having a PCIe_nDIS gpio defined in pinctrl (used nowhere) and the gw74xx does not support CLKREQ so has 'fsl,clkreq-unsupported'.

Ah, I see. So it turns out that it's Adam Linux kernel DTS patch [1] which is missing for U-Boot DTS. This is one of the reasons why we should switch to OF_UPSTREAM to start regular sync with Linux kernel DTS.

Adam,

Can you make corresponding changes to U-Boot DTS for imx8mp-beacon-kit? Or better just switch to OF_UPSTREAM instead?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/ar...

...

tested-by: Tim Harvey tharvey@gateworks.com : imx8mp-venice*

Thanks again.

-Sumit

...

Best regards,

Tim

On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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_imx.c | 314 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+) create mode 100644 drivers/pci/pcie_dw_imx.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 463ec47eb92d..7edbf35004a9 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_IMX

•   bool "i.MX DW PCIe controller support"
  

•   depends on ARCH_IMX8M
  

I think this also needs to depend on REGMAP to prevent build errors.

adam

•   select PCIE_DW_COMMON
  

•   help
  

•     Say Y here if you want to enable DW PCIe controller support on
  

•     iMX SoCs.
  

endif diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 72ef8b4bc772..2927c519129b 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_IMX) += pcie_dw_imx.o diff --git a/drivers/pci/pcie_dw_imx.c b/drivers/pci/pcie_dw_imx.c new file mode 100644 index 000000000000..7856823c9188 --- /dev/null +++ b/drivers/pci/pcie_dw_imx.c @@ -0,0 +1,314 @@ +// 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 <linux/iopoll.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_imx {

•   /* Must be first member of the struct */
  

•   struct pcie_dw                  dw;
  

•   struct regmap                   *iomuxc_gpr;
  

•   struct clk_bulk                 clks;
  

•   struct gpio_desc                reset_gpio;
  

•   struct reset_ctl                apps_reset;
  

•   struct phy                      phy;
  

+};

+static void pcie_dw_configure(struct pcie_dw_imx *priv, u32 cap_speed) +{

•   dw_pcie_dbi_write_enable(&priv->dw, true);
  

•   clrsetbits_le32(priv->dw.dbi_base + PCIE_LINK_CAPABILITY,
  

•                   TARGET_LINK_SPEED_MASK, cap_speed);
  

•   dw_pcie_dbi_write_enable(&priv->dw, false);
  

+}

+static void imx_pcie_ltssm_enable(struct pcie_dw_imx *priv) +{

•   reset_deassert(&priv->apps_reset);
  

+}

+static void imx_pcie_ltssm_disable(struct pcie_dw_imx *priv) +{

•   reset_assert(&priv->apps_reset);
  

+}

+static bool is_link_up(u32 val) +{

•   return ((val & PCIE_PORT_DEBUG1_LINK_UP) &&
  

•           (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING)));
  

+}

+static int wait_link_up(struct pcie_dw_imx *priv) +{

•   u32 val;
  

•   return readl_poll_sleep_timeout(priv->dw.dbi_base + PCIE_PORT_DEBUG1,
  

•                                   val, is_link_up(val), 10000, 100000);
  

+}

+static int pcie_link_up(struct pcie_dw_imx *priv, u32 cap_speed) +{

•   int ret;
  

•   /* DW pre link configurations */
  

•   pcie_dw_configure(priv, cap_speed);
  

•   /* Initiate link training */
  

•   imx_pcie_ltssm_enable(priv);
  

•   /* Check that link was established */
  

•   ret = wait_link_up(priv);
  

•   if (ret)
  

•           imx_pcie_ltssm_disable(priv);
  

•   return ret;
  

+}

+static int imx_pcie_assert_core_reset(struct pcie_dw_imx *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 imx_pcie_clk_enable(struct pcie_dw_imx *priv) +{

•   int ret;
  

•   ret = clk_enable_bulk(&priv->clks);
  

•   if (ret)
  

•           return ret;
  

•   /*
  

•    * 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;
  

+}

+static void imx_pcie_deassert_core_reset(struct pcie_dw_imx *priv) +{

•   if (!dm_gpio_is_valid(&priv->reset_gpio))
  

•           return;
  

•   mdelay(100);
  

•   dm_gpio_set_value(&priv->reset_gpio, 0);
  

•   /* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
  

•   mdelay(100);
  

+}

+static int pcie_dw_imx_probe(struct udevice *dev) +{

•   struct pcie_dw_imx *priv = dev_get_priv(dev);
  

•   struct udevice *ctlr = pci_get_controller(dev);
  

•   struct pci_controller *hose = dev_get_uclass_priv(ctlr);
  

•   int ret;
  

•   ret = imx_pcie_assert_core_reset(priv);
  

•   if (ret) {
  

•           dev_err(dev, "failed to assert core reset\n");
  

•           return ret;
  

•   }
  

•   ret = imx_pcie_clk_enable(priv);
  

•   if (ret) {
  

•           dev_err(dev, "failed to enable clocks\n");
  

•           goto err_clk;
  

•   }
  

•   ret = generic_phy_init(&priv->phy);
  

•   if (ret) {
  

•           dev_err(dev, "failed to initialize PHY\n");
  

•           goto err_phy_init;
  

•   }
  

•   ret = generic_phy_power_on(&priv->phy);
  

•   if (ret) {
  

•           dev_err(dev, "failed to power on PHY\n");
  

•           goto err_phy_power;
  

•   }
  

•   imx_pcie_deassert_core_reset(priv);
  

•   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));
  

•           ret = -ENODEV;
  

•           goto err_link;
  

•   }
  

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

+err_link:

•   generic_shutdown_phy(&priv->phy);
  

+err_phy_power:

•   generic_phy_exit(&priv->phy);
  

+err_phy_init:

•   clk_disable_bulk(&priv->clks);
  

+err_clk:

•   imx_pcie_deassert_core_reset(priv);
  

•   return ret;
  

+}

+static int pcie_dw_imx_remove(struct udevice *dev) +{

•   struct pcie_dw_imx *priv = dev_get_priv(dev);
  

•   generic_shutdown_phy(&priv->phy);
  

•   dm_gpio_free(dev, &priv->reset_gpio);
  

•   reset_release_all(&priv->apps_reset, 1);
  

•   clk_release_bulk(&priv->clks);
  

•   return 0;
  

+}

+static int pcie_dw_imx_of_to_plat(struct udevice *dev) +{

•   struct pcie_dw_imx *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_bulk(dev, &priv->clks);
  

•   if (ret) {
  

•           dev_err(dev, "failed to get PCIe clks\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_imx_ops = {

•   .read_config    = pcie_dw_read_config,
  

•   .write_config   = pcie_dw_write_config,
  

+};

+static const struct udevice_id pcie_dw_imx_ids[] = {

•   { .compatible = "fsl,imx8mp-pcie" },
  

•   { }
  

+};

+U_BOOT_DRIVER(pcie_dw_imx) = {

•   .name           = "pcie_dw_imx",
  

•   .id             = UCLASS_PCI,
  

•   .of_match       = pcie_dw_imx_ids,
  

•   .ops            = &pcie_dw_imx_ops,
  

•   .of_to_plat     = pcie_dw_imx_of_to_plat,
  

•   .probe          = pcie_dw_imx_probe,
  

•   .remove         = pcie_dw_imx_remove,
  

•   .priv_auto      = sizeof(struct pcie_dw_imx),
  

+};

2.34.1

On Tue, Feb 27, 2024 at 3:19 PM Adam Ford aford173@gmail.com wrote:

On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg sumit.garg@linaro.org 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_imx.c | 314 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 323 insertions(+) create mode 100644 drivers/pci/pcie_dw_imx.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 463ec47eb92d..7edbf35004a9 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_IMX

•   bool "i.MX DW PCIe controller support"
  

•   depends on ARCH_IMX8M
  

I think this also needs to depend on REGMAP to prevent build errors.

Sumit,

both REGMAP and SYSCON are needed to build.

Best Regards,

Tim