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

[Sumit Garg]

On Wed, 6 Mar 2024 at 06:17, Adam Ford <aford173 at gmail.com> wrote:
>
> On Tue, Mar 5, 2024 at 7:51 AM Adam Ford <aford173 at gmail.com> wrote:
> >
> > On Tue, Mar 5, 2024 at 7:48 AM Sumit Garg <sumit.garg at linaro.org> wrote:
> > >
> > > Hi Adam,
> > >
> > > On Wed, 28 Feb 2024 at 12:29, Sumit Garg <sumit.garg at linaro.org> wrote:
> > > >
> > > > Hi Adam,
> > > >
> > > > On Wed, 28 Feb 2024 at 04:42, Adam Ford <aford173 at gmail.com> wrote:
> > > > >
> > > > > On Mon, Feb 26, 2024 at 2:05 AM Sumit Garg <sumit.garg at 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 at 32f00000: PHY: Failed to power on
> > > > > pcie-phy at 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