diff mbox series

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

Message ID 20240226080433.3307154-6-sumit.garg@linaro.org
State Superseded
Headers show
Series imx8mp: Enable PCIe/NVMe support | expand

Commit Message

Sumit Garg Feb. 26, 2024, 8:04 a.m. UTC 
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.

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

Comments

Marek Vasut Feb. 26, 2024, 8:42 a.m. UTC | #1 
On 2/26/24 9:04 AM, 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.
> 
> Inspired from counterpart Linux kernel v6.8-rc3 driver:
> drivers/phy/freescale/phy-fsl-imx8m-pcie.c.

Commit ID , see previous comments.

[...]

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

Use dev_read_u32_default() and you won't need the if (...)

> +	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;
> +	}

Some clk_put() and co. fail path is missing here. Also .remove is missing .
Sumit Garg Feb. 26, 2024, 12:50 p.m. UTC | #2 
On Mon, 26 Feb 2024 at 14:24, Marek Vasut <marex@denx.de> wrote:
>
> On 2/26/24 9:04 AM, 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.
> >
> > Inspired from counterpart Linux kernel v6.8-rc3 driver:
> > drivers/phy/freescale/phy-fsl-imx8m-pcie.c.
>
> Commit ID , see previous comments.
>
> [...]
>
> > +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;
>
> Use dev_read_u32_default() and you won't need the if (...)
>

Ack.

> > +     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;
> > +     }
>
> Some clk_put() and co. fail path is missing here.

Ditto for clk_put() not being available. However, I can add the fail
path for the other reset.

> Also .remove is missing .

Sure, I will add that to release resets.

-Sumit
Marek Vasut Feb. 26, 2024, 1:04 p.m. UTC | #3 
On 2/26/24 1:50 PM, Sumit Garg wrote:

[...]

>>> +     ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
>>> +     if (ret) {
>>> +             dev_err(dev, "Failed to get PCIEPHY reset control\n");
>>> +             return ret;
>>> +     }
>>
>> Some clk_put() and co. fail path is missing here.
> 
> Ditto for clk_put() not being available. However, I can add the fail
> path for the other reset.

Oh, maybe reset_free() and clk_release*() then ?
Sumit Garg Feb. 26, 2024, 1:30 p.m. UTC | #4 
On Mon, 26 Feb 2024 at 18:34, Marek Vasut <marex@denx.de> wrote:
>
> On 2/26/24 1:50 PM, Sumit Garg wrote:
>
> [...]
>
> >>> +     ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
> >>> +     if (ret) {
> >>> +             dev_err(dev, "Failed to get PCIEPHY reset control\n");
> >>> +             return ret;
> >>> +     }
> >>
> >> Some clk_put() and co. fail path is missing here.
> >
> > Ditto for clk_put() not being available. However, I can add the fail
> > path for the other reset.
>
> Oh, maybe reset_free() and clk_release*() then ?

AFAICS, clk_release*() APIs are internally only a wrapper around
clk_disable(). So actually those APIs don't seem to do anything useful
in the fail path here.

However, I will let clock tree maintainers chime in here if they plan
to remove clk_release*() APIs on similar lines as with clk_free().

-Sumit
Marek Vasut Feb. 26, 2024, 1:36 p.m. UTC | #5 
On 2/26/24 2:30 PM, Sumit Garg wrote:
> On Mon, 26 Feb 2024 at 18:34, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/26/24 1:50 PM, Sumit Garg wrote:
>>
>> [...]
>>
>>>>> +     ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
>>>>> +     if (ret) {
>>>>> +             dev_err(dev, "Failed to get PCIEPHY reset control\n");
>>>>> +             return ret;
>>>>> +     }
>>>>
>>>> Some clk_put() and co. fail path is missing here.
>>>
>>> Ditto for clk_put() not being available. However, I can add the fail
>>> path for the other reset.
>>
>> Oh, maybe reset_free() and clk_release*() then ?
> 
> AFAICS, clk_release*() APIs are internally only a wrapper around
> clk_disable(). So actually those APIs don't seem to do anything useful
> in the fail path here.
> 
> However, I will let clock tree maintainers chime in here if they plan
> to remove clk_release*() APIs on similar lines as with clk_free().

Maybe they should be in place so in case in the future clock should be 
torn down, it wouldn't be necessary to fix so many drivers ?
Sumit Garg Feb. 26, 2024, 1:49 p.m. UTC | #6 
On Mon, 26 Feb 2024 at 19:06, Marek Vasut <marex@denx.de> wrote:
>
> On 2/26/24 2:30 PM, Sumit Garg wrote:
> > On Mon, 26 Feb 2024 at 18:34, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 2/26/24 1:50 PM, Sumit Garg wrote:
> >>
> >> [...]
> >>
> >>>>> +     ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
> >>>>> +     if (ret) {
> >>>>> +             dev_err(dev, "Failed to get PCIEPHY reset control\n");
> >>>>> +             return ret;
> >>>>> +     }
> >>>>
> >>>> Some clk_put() and co. fail path is missing here.
> >>>
> >>> Ditto for clk_put() not being available. However, I can add the fail
> >>> path for the other reset.
> >>
> >> Oh, maybe reset_free() and clk_release*() then ?
> >
> > AFAICS, clk_release*() APIs are internally only a wrapper around
> > clk_disable(). So actually those APIs don't seem to do anything useful
> > in the fail path here.
> >
> > However, I will let clock tree maintainers chime in here if they plan
> > to remove clk_release*() APIs on similar lines as with clk_free().
>
> Maybe they should be in place so in case in the future clock should be
> torn down, it wouldn't be necessary to fix so many drivers ?

See the following comparison:

$ git grep -nr clk_get_by_name | wc -l
229

$ git grep -nr clk_release | wc -l
59

There are already many driver instances which don't use clk_release*() APIs.

-Sumit
Marek Vasut Feb. 26, 2024, 2:21 p.m. UTC | #7 
On 2/26/24 2:49 PM, Sumit Garg wrote:
> On Mon, 26 Feb 2024 at 19:06, Marek Vasut <marex@denx.de> wrote:
>>
>> On 2/26/24 2:30 PM, Sumit Garg wrote:
>>> On Mon, 26 Feb 2024 at 18:34, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 2/26/24 1:50 PM, Sumit Garg wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> +     ret = reset_get_by_name(dev, "pciephy", &imx8_phy->reset);
>>>>>>> +     if (ret) {
>>>>>>> +             dev_err(dev, "Failed to get PCIEPHY reset control\n");
>>>>>>> +             return ret;
>>>>>>> +     }
>>>>>>
>>>>>> Some clk_put() and co. fail path is missing here.
>>>>>
>>>>> Ditto for clk_put() not being available. However, I can add the fail
>>>>> path for the other reset.
>>>>
>>>> Oh, maybe reset_free() and clk_release*() then ?
>>>
>>> AFAICS, clk_release*() APIs are internally only a wrapper around
>>> clk_disable(). So actually those APIs don't seem to do anything useful
>>> in the fail path here.
>>>
>>> However, I will let clock tree maintainers chime in here if they plan
>>> to remove clk_release*() APIs on similar lines as with clk_free().
>>
>> Maybe they should be in place so in case in the future clock should be
>> torn down, it wouldn't be necessary to fix so many drivers ?
> 
> See the following comparison:
> 
> $ git grep -nr clk_get_by_name | wc -l
> 229
> 
> $ git grep -nr clk_release | wc -l
> 59
> 
> There are already many driver instances which don't use clk_release*() APIs.

And that's a good argument to make the situation worse ?
Adam Ford Feb. 27, 2024, 11:12 p.m. UTC | #8 
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
>
Sumit Garg Feb. 28, 2024, 6:59 a.m. UTC | #9 
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");

>
> >
> > 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.
>

Ack, I will add those.

-Sumit

> > +       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
> >
Sumit Garg March 5, 2024, 1:48 p.m. UTC | #10 
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
Adam Ford March 5, 2024, 1:51 p.m. UTC | #11 
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);
> > +
> >         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.

adam
>
> -Sumit
Adam Ford March 6, 2024, 12:47 a.m. UTC | #12 
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
Sumit Garg March 6, 2024, 6:30 a.m. UTC | #13 
On Wed, 6 Mar 2024 at 06:17, 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.
>

Yeah I agree, this was sort of a minimal diff to test with.

> > > > +
> > > >         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.

Sure, I can add that.

-Sumit

>
> I'll investigate a bit more and let you know if I make any progress.
>
> adam
> >
> > adam
> > >
> > > -Sumit
Tim Harvey March 6, 2024, 5:26 p.m. UTC | #14 
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
Sumit Garg March 7, 2024, 6:35 a.m. UTC | #15 
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?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm64?id=63c46b51c7995d8aeb4b44493633f4ce1dcf62bc

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

Thanks again.

-Sumit

> Best regards,
>
> Tim
Adam Ford March 7, 2024, 11:42 a.m. UTC | #16 
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/arch/arm64?id=63c46b51c7995d8aeb4b44493633f4ce1dcf62bc
>

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
Sumit Garg March 7, 2024, 12:10 p.m. UTC | #17 
On Thu, 7 Mar 2024 at 17:12, Adam Ford <aford173@gmail.com> wrote:
>
> 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/arch/arm64?id=63c46b51c7995d8aeb4b44493633f4ce1dcf62bc
> >
>
> 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.
>

No worries.

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

Thanks for taking time to test this series.

-Sumit

> adam
>
>
> > >
> > > tested-by: Tim Harvey <tharvey@gateworks.com> : imx8mp-venice*
> > >
> >
> > Thanks again.
> >
> > -Sumit
> >
> > > Best regards,
> > >
> > > Tim
Tim Harvey March 7, 2024, 4:13 p.m. UTC | #18 
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/arch/arm64?id=63c46b51c7995d8aeb4b44493633f4ce1dcf62bc
>
> >
> > tested-by: Tim Harvey <tharvey@gateworks.com> : imx8mp-venice*
> >
>
> Thanks again.
>
> -Sumit
>
> > Best regards,
> >
> > Tim
diff mbox series

Patch

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
+	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),
+};