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

[Philip Molloy]

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.

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