RE: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie standalone phy driver

[Hongxing Zhu <hongxing.zhu@nxp.com>]

> -----Original Message-----
> From: Philip Molloy <philip@linutronix.de>
> Sent: Wednesday, December 29, 2021 8:40 PM
> To: Hongxing Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; tharvey@gateworks.com; Marcel Ziswiler
> <marcel.ziswiler@toradex.com>; kishon@ti.com; vkoul@kernel.org;
> robh@kernel.org; galak@kernel.crashing.org; shawnguo@kernel.org;
> linux-phy@lists.infradead.org; devicetree@vger.kernel.org;
> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH v7 5/8] phy: freescale: pcie: Initialize the imx8 pcie
> standalone phy driver
> 
> Hi Richard,
> 
> I've run into an issue that appears to indicate a functional difference
> between the existing integrated pci-imx6.c implementation and this new
> implementation with the separate phy driver.
> 
> I'm working with a SOM and baseboard from Phytec that is based on the
> IMX8MM. The board does not have an external PCIe clock and has a
> ethernet controller hanging off the PCIe bus.
> 
> When booting from a 5.4 NXP-based kernel from Phytec the ethernet
> controller is probed and functions as expected.
> 
> A co-worker backported a slightly earlier version of this patchset to
> 5.10.[1] With our kernel both the controller driver and new PHY driver are
> probed, but a timeout occurs in dw_pcie_wait_for_link() which indicates
> that the "Phy link never came up".
> 
> After reproducing this issue I configured pcie_phy with
> IMX8_PCIE_REFCLK_PAD_OUTPUT. With that configured, phy register
> CMN_REG062/0x188 matches the 5.4 NXP/Phytec kernel. I then
> compared the controller and PHY registers between the two kernels and
> noticed that CMN_REG063/0x18c is set to AUX_IN/0x0 in the 5.4
> NXP/Phytec kernel, but the new PHY driver writes
> I_PLL_REFCLK_FROM_SYSPLL/0xc0 to that register.
> 
> If I modify the phy driver to not write I_PLL_REFCLK_FROM_SYSPLL/0xc0
> then the system behaves as expected.
[Richard Zhu] 
Hi Philip:
The address 0x018C is a register to control the output mode of Refclk IO.
When internal syspll is used as PHY REF clock.
Regarding my understand, this register should select the syspll, and route
it out of SOC from CLK N/P pads.
Then the remote EP device can use the clock from CLK N/P pads.

If the bit7-6 is set to 2b'00, there wouldn't clock output from CLK N/P pads.
What's the hardware design of the CLK N/P pads in your project?
Can you monitor the situation of the CLK N/P if it is possible?

Best Regards
Richard Zhu

> 
> Best,
> Philip
> 
> [1]: I plan on rebasing our branch with the latest patches that have been
> applied upstream. Note that I did not see any difference in the following
> code with what we have applied.
> 
> On 12/2/21 09:02, Richard Zhu wrote:
> > +#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)
> ...
> > +	if (pad_mode == IMX8_PCIE_REFCLK_PAD_INPUT) {
> > +		/* 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 if (pad_mode == IMX8_PCIE_REFCLK_PAD_OUTPUT) {
> > +		/* 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);
> > +		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);
> 
> If I comment out this writel() then the register defaults to 0x0/AUX_IN
> and then the system behaves as expected.
> 
> > +		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);
> > +	}