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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Dec 2021 13:39:57 +0100
From:   Philip Molloy <philip@...utronix.de>
To:     Richard Zhu <hongxing.zhu@....com>
Cc:     l.stach@...gutronix.de, bhelgaas@...gle.com,
        lorenzo.pieralisi@....com, tharvey@...eworks.com,
        marcel.ziswiler@...adex.com, kishon@...com, vkoul@...nel.org,
        robh@...nel.org, galak@...nel.crashing.org, shawnguo@...nel.org,
        linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, kernel@...gutronix.de,
        linux-imx@....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.

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