RE: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie support

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

> -----Original Message-----
> From: Tim Harvey <tharvey@gateworks.com>
> Sent: Friday, October 22, 2021 12:25 AM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>; Kishon Vijay Abraham I
> <kishon@ti.com>; vkoul@kernel.org; Rob Herring <robh@kernel.org>;
> galak@kernel.crashing.org; Shawn Guo <shawnguo@kernel.org>;
> linux-phy@lists.infradead.org; Device Tree Mailing List
> <devicetree@vger.kernel.org>; Linux ARM Mailing List
> <linux-arm-kernel@lists.infradead.org>; open list
> <linux-kernel@vger.kernel.org>; Sascha Hauer <kernel@pengutronix.de>;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie
> support
> 
> On Wed, Oct 20, 2021 at 8:32 PM Richard Zhu <hongxing.zhu@nxp.com>
> wrote:
> >
> > <snipped...>
> > >
> > > Richard,
> > >
> > > What is this 'invalid resource' about? I see that with my downstream
> > > IMX8MM PCIe driver as well and have been asked about it.
> > >
> > [Richard Zhu] Hi Tim:
> > This complain is caused by the following codes in pcie-designware.c driver.
> > I'm not sure that why there is only size assignment after the res valid check,
> and do nothing if the res is invalid.
> > It seems that it is an expected design logic refer to the later codes.
> >                 if (!pci->atu_base) {
> >                         struct resource *res =
> >
> platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> >                         if (res)
> >                                 pci->atu_size = resource_size(res);
> >                         pci->atu_base =
> devm_ioremap_resource(dev, res);
> >                         if (IS_ERR(pci->atu_base))
> >                                 pci->atu_base = pci->dbi_base +
> DEFAULT_DBI_ATU_OFFSET;
> >                 }
> >
> > Since the default offset is used on i.MX8MM, the "atu" is not specified in
> i.MX8MM PCIe DT node, so there is no real res at all.
> > Then, devm_ioremap_resource() would complain the invalid resource.
> 
> I think you are saying a change should be made like this:
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> b/drivers/pci/controller/dwc/pcie-designware.c
> index a945f0c0e73d..3254f60d1713 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -671,10 +671,11 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>                 if (!pci->atu_base) {
>                         struct resource *res =
> 
> platform_get_resource_byname(pdev,
> IORESOURCE_MEM, "atu");
> -                       if (res)
> +                       if (res) {
>                                 pci->atu_size = resource_size(res);
> -                       pci->atu_base = devm_ioremap_resource(dev,
> res);
> -                       if (IS_ERR(pci->atu_base))
> +                               pci->atu_base =
> devm_ioremap_resource(dev, res);
> +                       }
> +                       if (!pci->atu_base || IS_ERR(pci->atu_base))
>                                 pci->atu_base = pci->dbi_base +
> DEFAULT_DBI_ATU_OFFSET;
>                 }
> 
> so that it looks like this:
>                 if (!pci->atu_base) {
>                         struct resource *res =
> 
> platform_get_resource_byname(pdev,
> IORESOURCE_MEM, "atu");
>                         if (res) {
>                                 pci->atu_size = resource_size(res);
>                                 pci->atu_base =
> devm_ioremap_resource(dev, res);
>                         }
>                         if (!pci->atu_base || IS_ERR(pci->atu_base))
>                                 pci->atu_base = pci->dbi_base +
> DEFAULT_DBI_ATU_OFFSET;
>                 }
> 
> Right?
[Richard Zhu] Yes, it is. The res shouldn't be remapped if it is invalid resource memory.

> 
> >
> > > > [    1.316305] imx6q-pcie 33800000.pcie: iATU unroll: enabled
> > > > [    1.321799] imx6q-pcie 33800000.pcie: Detected iATU regions: 4
> > > outbound, 4 inbound
> > > > [    1.429803] imx6q-pcie 33800000.pcie: Link up
> > > > [    1.534497] imx6q-pcie 33800000.pcie: Link up
> > > > [    1.538870] imx6q-pcie 33800000.pcie: Link up, Gen2
> > > > [    1.550364] imx6q-pcie 33800000.pcie: Link up
> > > > [    1.550487] imx6q-pcie 33800000.pcie: PCI host bridge to bus
> 0000:00
> > > > [    1.565545] pci_bus 0000:00: root bus resource [bus 00-ff]
> > > > [    1.573834] pci_bus 0000:00: root bus resource [io  0x0000-0xffff]
> > > > [    1.580055] pci_bus 0000:00: root bus resource [mem
> > > 0x18000000-0x1fefffff]
> > > > [    1.586968] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
> > > > [    1.592997] pci 0000:00:00.0: reg 0x10: [mem
> 0x00000000-0x000fffff]
> > > > [    1.599282] pci 0000:00:00.0: reg 0x38: [mem
> 0x00000000-0x0000ffff
> > > pref]
> > > > [    1.606033] pci 0000:00:00.0: supports D1
> > > > [    1.610053] pci 0000:00:00.0: PME# supported from D0 D1 D3hot
> > > D3cold
> > > > [    1.618206] pci 0000:01:00.0: [15b7:5002] type 00 class 0x010802
> > > > [    1.624293] pci 0000:01:00.0: reg 0x10: [mem
> 0x00000000-0x00003fff
> > > 64bit]
> > > > [    1.631177] pci 0000:01:00.0: reg 0x20: [mem
> 0x00000000-0x000000ff
> > > 64bit]
> > > > [    1.638409] pci 0000:01:00.0: 4.000 Gb/s available PCIe bandwidth,
> > > limited by 5.0 GT/s PCIe x1 link at 0000:00:00.0 (capable of 31.504
> > > Gb/s with
> > > 8.0 GT/s PCIe x4 link)
> > > > [    1.664931] pci 0000:00:00.0: BAR 0: assigned [mem
> > > 0x18000000-0x180fffff]
> > > > [    1.671745] pci 0000:00:00.0: BAR 14: assigned [mem
> > > 0x18100000-0x181fffff]
> > > > [    1.678634] pci 0000:00:00.0: BAR 6: assigned [mem
> > > 0x18200000-0x1820ffff pref]
> > > > [    1.685873] pci 0000:01:00.0: BAR 0: assigned [mem
> > > 0x18100000-0x18103fff 64bit]
> > > > [    1.693222] pci 0000:01:00.0: BAR 4: assigned [mem
> > > 0x18104000-0x181040ff 64bit]
> > > > [    1.700577] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> > > > [    1.705814] pci 0000:00:00.0:   bridge window [mem
> > > 0x18100000-0x181fffff]
> > > > [    1.712972] pcieport 0000:00:00.0: PME: Signaling with IRQ 216
> > > > "
> > > > Regarding the log you pasted, it seems that the clock is not feed
> > > > to PHY
> > > properly.
> > > >
> > > > Anyway, let's waiting for the v4 series, then make a try. Thanks
> > > > for your
> > > great help to make the double tests.
> > > >
> > >
> > > My boards do not use CLKREQ# so I do not have that defined in pinmux
> > > and I found that if I add MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B
> PCIe
> > > works on my board but this isn't a solution just a work-around (I
> > > have boards that use the only two possible pins for CLKREQ as other
> features).
> > >
> > > Similarly you will find on the imx8mm-evk if you comment out the
> > > CLKREQ (which isn't required) the imx8mmevk will end up hanging like my
> boards:
> > [Richard Zhu] Hi Tim:
> > Regarding the SPEC, the CLKREQ# is mandatory required, and should be
> configured as an open drain, active low signal.
> > And this signal should be driven low by the PCIe M.2 device to request the
> REF clock be available(active low).
> > So, there is such kind of CLKREQ# pin definition on i.MX8MM EVK board.
> >
> > Anyway, I think the external OSC circuit should be always running if there is
> no CLKREQ# on your HW board design.
> >
> 
> The way I understand it is CLKREQ# allows the host to disable the REFCLK
> when not needed for power savings so it would seem optional to implement
> that and if not implemented should be left unconnected on the card.
> 
[Richard Zhu] No, not that way. Regarding the SPEC, this signal is mandatory required.
Especially for the L1ss usages. This signal would be OD(open drain), bi-directional, and might be
driven low/high by RC or EP automatically if L1ss modes are enabled.
You can make reference to the "ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a", or
the chapter 5.5 L1 PM Substates of "PCI Express Base Specification, Rev. 4.0 Version 1.0".

> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> > > index 5ce43daa0c8b..f0023b48f475 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
> > > @@ -448,7 +448,9 @@
> > >
> > >         pinctrl_pcie0: pcie0grp {
> > >                 fsl,pins = <
> > > +/*
> > >
> > > MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B    0x61
> > > +*/
> > >
> MX8MM_IOMUXC_SAI2_RXFS_GPIO4_IO21
> > > 0x41
> > >                 >;
> > >         };
> > >
> > > I have PCIe working with a driver that I ported from NXP's kernel
> > > which differs from your driver in that the PCIe PHY is not
> > > abstracted to its own driver so I think this has something to do
> > > with the order in which the phy is reset or initialized? The configuration of
> gpr14 bits looks correct to me.
> > [Richard Zhu] The CLKREQ# PIN definition shouldn't be masked.
> > In the NXP's local BSP kernel, I just force CLKREQ# low to level up the HW
> compatibility.
> > That's might the reason why the PCIe works on your HW board although the
> CLKREQ# PIN is not defined.
> > This method is a little rude and violate the SPEC, and not recommended
> although it levels up the HW compatibility.
> > So I drop this method in this series.
> >
> 
> Sorry, I don't understand what you are saying here. Is there a change you are
> going to make to v4 that will make this work for the evk and my boards? What
> is that change exactly?
[Richard Zhu] No. What I said above is that the CLKREQ# is forced to be low in NXP
local BSP kernel. I guess this might be the reason why your board works.

BIT11 and BIT10 of IOMUXC_GPR14 can be used to force the CLKREQ# to be low.
Set CLKREQ_OVERRIDE_EN(bit10) 1b1, then write one zero to CLKREQ_OVERRIDE(bit11).

BR
Richard> 
> I responded to your "phy: freescale: pcie: initialize the imx8 pcie standalone
> phy driver" submission as I don't understand the GPR14 bit documentation
> from the IMX8MMRM.
> 
> Best regards,
> 
> Tim