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

[Marcel Ziswiler <marcel.ziswiler@toradex.com>]

On Thu, 2021-12-30 at 04:58 +0000, Hongxing Zhu wrote:
> > -----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?

I can confirm that for me similar hardware which also "does not have an external PCIe clock" works just fine
with the following device tree sniped:

&pcie0 {
	assigned-clocks = <&clk IMX8MM_CLK_PCIE1_AUX>,
			  <&clk IMX8MM_CLK_PCIE1_CTRL>;
	assigned-clock-parents = <&clk IMX8MM_SYS_PLL2_50M>,
				 <&clk IMX8MM_SYS_PLL2_250M>;
	assigned-clock-rates = <10000000>, <250000000>;
	clocks = <&clk IMX8MM_CLK_PCIE1_ROOT>, <&clk IMX8MM_CLK_PCIE1_AUX>,
		 <&clk IMX8MM_CLK_PCIE1_PHY>;
	clock-names = "pcie", "pcie_aux", "pcie_bus";
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_pcie0>;
	/* PCIE_1_RESET# (SODIMM 244) */
	reset-gpio = <&gpio3 19 GPIO_ACTIVE_LOW>;
	status = "okay";
};

&pcie_phy {
	clocks = <&clk IMX8MM_CLK_PCIE1_PHY>;
	fsl,clkreq-unsupported;
	fsl,refclk-pad-mode = <IMX8_PCIE_REFCLK_PAD_OUTPUT>;
	fsl,tx-deemph-gen1 = <0x2d>;
	fsl,tx-deemph-gen2 = <0xf>;
	status = "okay";
};

During boot that looks as follows:

[    1.858312] imx6q-pcie 33800000.pcie: host bridge /soc@0/pcie@33800000 ranges:

[    1.865630] imx6q-pcie 33800000.pcie:       IO 0x001ff80000..0x001ff8ffff -> 0x0000000000
[    1.877833] imx6q-pcie 33800000.pcie:      MEM 0x0018000000..0x001fefffff -> 0x0018000000

[    1.992010] imx6q-pcie 33800000.pcie: iATU unroll: enabled
[    1.997523] imx6q-pcie 33800000.pcie: Detected iATU regions: 4 outbound, 4 inbound

[    2.103140] imx6q-pcie 33800000.pcie: Link up
[    2.107527] imx6q-pcie 33800000.pcie: Link up
[    2.111895] imx6q-pcie 33800000.pcie: Link up, Gen1
[    2.116786] imx6q-pcie 33800000.pcie: Link up
[    2.121298] imx6q-pcie 33800000.pcie: PCI host bridge to bus 0000:00
[    2.127671] pci_bus 0000:00: root bus resource [bus 00-ff]
[    2.133171] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
[    2.139363] pci_bus 0000:00: root bus resource [mem 0x18000000-0x1fefffff]
[    2.146282] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
[    2.152322] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x000fffff]
[    2.158612] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref]
[    2.165373] pci 0000:00:00.0: supports D1
[    2.169395] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold
[    2.178101] pci 0000:01:00.0: [10ec:8168] type 00 class 0x020000
[    2.184196] pci 0000:01:00.0: reg 0x10: [io  0x0000-0x00ff]
[    2.189864] pci 0000:01:00.0: reg 0x18: [mem 0x00000000-0x00000fff 64bit pref]
[    2.197152] pci 0000:01:00.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit pref]
[    2.204777] pci 0000:01:00.0: supports D1 D2
[    2.209059] pci 0000:01:00.0: PME# supported from D0 D1 D2 D3hot D3cold
[    2.229114] pci 0000:00:00.0: BAR 0: assigned [mem 0x18000000-0x180fffff]
[    2.235934] pci 0000:00:00.0: BAR 15: assigned [mem 0x18100000-0x181fffff pref]
[    2.243260] pci 0000:00:00.0: BAR 6: assigned [mem 0x18200000-0x1820ffff pref]
[    2.250501] pci 0000:00:00.0: BAR 13: assigned [io  0x1000-0x1fff]
[    2.256734] pci 0000:01:00.0: BAR 4: assigned [mem 0x18100000-0x18103fff 64bit pref]
[    2.264534] pci 0000:01:00.0: BAR 2: assigned [mem 0x18104000-0x18104fff 64bit pref]
[    2.272338] pci 0000:01:00.0: BAR 0: assigned [io  0x1000-0x10ff]
[    2.278460] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[    2.283698] pci 0000:00:00.0:   bridge window [io  0x1000-0x1fff]
[    2.289808] pci 0000:00:00.0:   bridge window [mem 0x18100000-0x181fffff pref]
[    2.297444] pcieport 0000:00:00.0: PME: Signaling with IRQ 225

And my mini-PCIe Ethernet card gets detected and is fully functional:

[    4.433899] r8169 0000:01:00.0: enabling device (0000 -> 0003)
[    4.466731] libphy: r8169: probed
[    4.476288] r8169 0000:01:00.0 eth1: RTL8168e/8111e, 00:e0:4c:80:f4:0d, XID 2c2, IRQ 229
[    4.484485] r8169 0000:01:00.0 eth1: jumbo features [frames: 9194 bytes, tx checksumming: ko]

[    5.023616] r8169 0000:01:00.0 enp1s0: renamed from eth1

[    6.713601] r8169 0000:01:00.0: Direct firmware load for rtl_nic/rtl8168e-2.fw failed with error -2
[    6.722786] r8169 0000:01:00.0: Unable to load firmware rtl_nic/rtl8168e-2.fw (-2)
[    6.731391] RTL8211DN Gigabit Ethernet r8169-0-100:00: attached PHY driver (mii_bus:phy_addr=r8169-0-100:00,
irq=MAC)
[    6.808798] r8169 0000:01:00.0 enp1s0: Link is Down

root@verdin-imx8mm-06760554:~# lspci
00:00.0 PCI bridge: Synopsys, Inc. DWC_usb3 / PCIe bridge (rev 01)
01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet
Controller (rev 06)

Cheers

Marcel

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