RE: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie support | linux-arm-kernel

quoted

Snipped...

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

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

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.
[Richard Zhu] Hi Tim:
As you mentioned above, the CLKREQ# GPIO PIN is not used for PCIe on your board, right?
" (I have boards that use the only two possible pins for CLKREQ as other features)"

Did the override configuration of the clkreq# will bring unexpected results for other features on your board?

BR
Richard 

BR
Richard

Best regards,

Tim
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`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