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

From: Richard Zhu <hongxing.zhu@nxp.com>
Date: 2021-10-25 02:13:06
Also in: linux-arm-kernel, linux-phy, lkml

-----Original Message-----
From: Tim Harvey <tharvey@gateworks.com>
Sent: Saturday, October 23, 2021 12:55 AM
To: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Lucas Stach <l.stach@pengutronix.de>; Kishon Vijay Abraham I
[off-list ref]; vkoul@kernel.org; Rob Herring [off-list ref];
galak@kernel.crashing.org; Shawn Guo [off-list ref];
linux-phy@lists.infradead.org; Device Tree Mailing List
[off-list ref]; Linux ARM Mailing List
[off-list ref]; open list
[off-list ref]; Sascha Hauer [off-list ref];
dl-linux-imx [off-list ref]
Subject: Re: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie
support

On Fri, Oct 22, 2021 at 8:59 AM Tim Harvey [off-list ref]
wrote:

quoted

On Thu, Oct 21, 2021 at 5:43 PM Richard Zhu [off-list ref]

wrote:

quoted

-----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
[off-list ref]; vkoul@kernel.org; Rob Herring [off-list ref];
galak@kernel.crashing.org; Shawn Guo [off-list ref];
linux-phy@lists.infradead.org; Device Tree Mailing List
[off-list ref]; Linux ARM Mailing List
[off-list ref]; open list
[off-list ref]; Sascha Hauer
[off-list ref]; dl-linux-imx [off-list ref]
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 [off-list ref]
wrote:

quoted

<snipped...>

quoted

Richard,

What is this 'invalid resource' about? I see that with my
downstream IMX8MM PCIe driver as well and have been asked

about it.

quoted

[Richard Zhu] Hi Tim:
This complain is caused by the following codes in pcie-designware.c

driver.

quoted

I'm not sure that why there is only size assignment after the
res valid check,

and do nothing if the res is invalid.

quoted

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");

quoted

                        if (res)
                                pci->atu_size =

resource_size(res);

quoted

                        pci->atu_base =

devm_ioremap_resource(dev, res);

quoted

                        if (IS_ERR(pci->atu_base))
                                pci->atu_base =

pci->dbi_base +

quoted

DEFAULT_DBI_ATU_OFFSET;

quoted

                }

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.

quoted

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)

quoted

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

quoted

-                       pci->atu_base =

devm_ioremap_resource(dev,

quoted

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

quoted

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

quoted

                                pci->atu_base =
devm_ioremap_resource(dev, res);
                        }
                        if (!pci->atu_base ||

IS_ERR(pci->atu_base))

quoted

                                pci->atu_base = pci->dbi_base

quoted

DEFAULT_DBI_ATU_OFFSET;
                }

Right?

[Richard Zhu] Yes, it is. The res shouldn't be remapped if it is invalid

resource memory.

quoted

Ok, I will submit a patch for that.

[Richard Zhu] Thanks for your help. Please cc me, if you issue that patch.

quoted

[    1.316305] imx6q-pcie 33800000.pcie: iATU unroll: enabled
[    1.321799] imx6q-pcie 33800000.pcie: Detected iATU regions:

quoted

outbound, 4 inbound

quoted

[    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

quoted

[    1.565545] pci_bus 0000:00: root bus resource [bus 00-ff]
[    1.573834] pci_bus 0000:00: root bus resource [io

0x0000-0xffff]

quoted

[    1.580055] pci_bus 0000:00: root bus resource [mem

0x18000000-0x1fefffff]

quoted

[    1.586968] pci 0000:00:00.0: [16c3:abcd] type 01 class

0x060400

quoted

[    1.592997] pci 0000:00:00.0: reg 0x10: [mem

0x00000000-0x000fffff]

quoted

[    1.599282] pci 0000:00:00.0: reg 0x38: [mem

0x00000000-0x0000ffff

quoted

pref]

quoted

[    1.606033] pci 0000:00:00.0: supports D1
[    1.610053] pci 0000:00:00.0: PME# supported from D0 D1

D3hot

quoted

quoted
quoted
quoted
quoted
D3cold
quoted
[    1.618206] pci 0000:01:00.0: [15b7:5002] type 00 class

0x010802

quoted

[    1.624293] pci 0000:01:00.0: reg 0x10: [mem

0x00000000-0x00003fff

quoted

64bit]

quoted

[    1.631177] pci 0000:01:00.0: reg 0x20: [mem

0x00000000-0x000000ff

quoted

64bit]

quoted

[    1.638409] pci 0000:01:00.0: 4.000 Gb/s available PCIe

bandwidth,

quoted

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)

quoted

[    1.664931] pci 0000:00:00.0: BAR 0: assigned [mem

0x18000000-0x180fffff]

quoted

[    1.671745] pci 0000:00:00.0: BAR 14: assigned [mem

0x18100000-0x181fffff]

quoted

[    1.678634] pci 0000:00:00.0: BAR 6: assigned [mem

0x18200000-0x1820ffff pref]

quoted

[    1.685873] pci 0000:01:00.0: BAR 0: assigned [mem

0x18100000-0x18103fff 64bit]

quoted

[    1.693222] pci 0000:01:00.0: BAR 4: assigned [mem

0x18104000-0x181040ff 64bit]

quoted

[    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]

quoted

[    1.712972] pcieport 0000:00:00.0: PME: Signaling with IRQ

quoted

"
Regarding the log you pasted, it seems that the clock is not
feed to PHY

properly.

quoted

Anyway, let's waiting for the v4 series, then make a try.
Thanks for your

great help to make the double tests.

quoted

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

quoted

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).

quoted

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:

quoted

[Richard Zhu] Hi Tim:
Regarding the SPEC, the CLKREQ# is mandatory required, and
should be

configured as an open drain, active low signal.

quoted

And this signal should be driven low by the PCIe M.2 device to
request the

REF clock be available(active low).

quoted

So, there is such kind of CLKREQ# pin definition on i.MX8MM EVK

board.

quoted

Anyway, I think the external OSC circuit should be always
running if there is

no CLKREQ# on your HW board design.

quoted

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.

quoted

[Richard Zhu] No, not that way. Regarding the SPEC, this signal is

mandatory required.

quoted

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.

quoted

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".

quoted

CLKREQ is only mandatory if you wish to support clock power
management. Many boards with a PCI host controller do not support
this.

[Richard Zhu] Okay, understood.

quoted

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

quoted

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.

quoted

[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.

quoted

That's might the reason why the PCIe works on your HW board
although the

CLKREQ# PIN is not defined.

quoted

This method is a little rude and violate the SPEC, and not
recommended

although it levels up the HW compatibility.

quoted

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.

quoted

BIT11 and BIT10 of IOMUXC_GPR14 can be used to force the CLKREQ# to

be low.

quoted

Set CLKREQ_OVERRIDE_EN(bit10) 1b1, then write one zero to

CLKREQ_OVERRIDE(bit11).

quoted

Ok, that makes sense. Those bits are not explained well in the
IMX8MMRM. As my board's external REFCLK is always enabled that must
gate the clock internally to the host controller block.

I can confirm that asserting those GPR14 bits does resolve my issue:

#define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL    BIT(11)
#define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN     BIT(10)

       /*
        * for boards that do not connect CLKREQ#,
        * override CLKREQ# and drive it low internally
        */
       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,

IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL, 0);

quoted

       regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,

IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN, 1);

[Richard Zhu] regmap bits operations should manipulate according bits.
The BIT(10) and BIT(11) should be touched actually.

quoted

Should this be added as a 'fsl,clkreq-unsupported' flag that needs to
be set true to implement the above code?

Richard,

Sorry - spoke too soon. My test was flawed as I still was pinmuxing CLKREQ in
my dt to work around the issue and after removed the above did not resolve
my issue. The setting of OVERRIDE_EN was wrong above (should not be set to
'1' but BIT(10) instead) but this code already exists in
imx6_pcie_enable_ref_clk and is used for IMX8MM per your patch so this is
not the issue.

What makes my board work is to clear GPR14 bit9 (like the NXP kernel
does) so I don't think this bit does what we think it does (select between
internal and ext clk). I think setting it enables clock gating via CLKREQ#.

This also points out that perhaps the CLKREQ_OVERRIDE logic should be
moved to the new phy driver for IMX8MM.

[Richard Zhu] It sounds reasonable to consider to force the CLKREQ# to be low.
I will think about that and add this in later v5 patch-set if nobody has different concerns.
Thanks.

BR
Richard

Best regards,

Tim

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help