-----Original Message-----
From: Tim Harvey <tharvey@gateworks.com>
Sent: Tuesday, October 26, 2021 1:15 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 Mon, Oct 25, 2021 at 12:23 AM Richard Zhu [off-list ref]
wrote:

quoted

Snipped...

quoted

quoted

quoted

quoted

quoted

quoted

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

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

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

quoted

quoted

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

quoted

quoted

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

mandatory required.

quoted

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

quoted

You can make reference to the
"ECN_L1_PM_Substates_with_CLKREQ_31_May_2013_Rev10a", or

the

quoted

quoted

quoted

chapter 5.5 L1 PM Substates of "PCI Express Base Specification, Rev.
4.0 Version 1.0".

quoted

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

quoted

quoted

quoted

quoted

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

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?

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

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

be low.

quoted

quoted

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

CLKREQ_OVERRIDE(bit11).

quoted

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,

quoted

quoted

quoted

quoted

IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL, 0);

quoted

       regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

quoted

quoted

quoted

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

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

quoted

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?

quoted

" (I have boards that use the only two possible pins for CLKREQ as other

features)"

quoted

Did the override configuration of the clkreq# will bring unexpected results

for other features on your board?

quoted

What I mean is that imx8mm-venice-gw7901.dts uses both I2C4 and UART4
and because I2C4_SCL and UART4_RXD are the only two pads that could be
pinmuxed for CLKREQ# I can't use the workaround of pin muxing it.

Currently your driver only works on my imx8mm-venice-* boards if I add one
of the following on boards that don't connect those pads (or if I clear
IMX8MM_GPR_PCIE_REF_USE_PAD):
MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B
MX8MM_IOMUXC_UART4_RXD_PCIE1_CLKREQ_B

Note your 'PCI: imx: add the imx8mm pcie support' patch [1] does enable this
code already in the imx6_pcie_enable_ref_clk function to override REF_CLK
and drive it low:

offset = imx6_pcie_grp_offset(imx6_pcie);
/*
* Set the over ride low and enabled
* make sure that REF_CLK is turned on.
*/
regmap_update_bits(imx6_pcie->iomuxc_gpr, offset,
   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
   0);
regmap_update_bits(imx6_pcie->iomuxc_gpr, offset,
   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
   IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);

So this is already being run and yet my boards still do not work unless I clr
IMX8MM_GPR_PCIE_REF_USE_PAD like this which is what the NXP
downstream driver does:
regmap_update_bits(imx8_phy->iomuxc_gpr, IOMUXC_GPR14,
IMX8MM_GPR_PCIE_REF_USE_PAD, 0);

This is why I'm not sure that bit does what you think it does... I feel like that
bit enables 'Use CLKREQ# to enable CLK'.

You tell me the descriptions for GPR14 are wrong in the reference manual.
Please provide correct descriptions for us so we can sort this out.

[Richard Zhu] Hi Tim:
The BIT9 of GPR14 is used as "GPR_PCIE1_PHY_I_AUX_EN_OVERRIDE_EN"
and BIT19 is used as "GPR_PCIE1_PHY_FUNC_I_AUX_EN" on i.MX8MM.
I think the two bits descriptions are used to describe the BIT19 and BIT9 together refer to my guess.
{GPR_PCIE1_PHY_I_AUX_EN_OVERRIDE_EN(BIT9), GPR_PCIE1_PHY_FUNC_I_AUX_EN(BIT19) }
2'b00: External Reference Clock I/O (for PLL) Disable
2'b01: External Reference Clock I/O (for PLL) Enable
2'b10: External Reference Clock I/O (for PLL) Disable
2'b11: External Reference Clock I/O (for PLL) output is controlled by CLKREQ#

The option1&3 should be forbidden, since the external REF CLK I/O should be enabled on your board and EVK board.
In the option2&4, the BIT19 should be set to be 1'b1.

So, regarding my understand, if the CLKREQ# is not pinmuxed in your use case, the IMX8MM_GPR_PCIE_REF_USE_PAD (BIT9) should be 1'b0.

BR
Richard

Best regards,

Tim
[1]
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
work.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F1634028078-23
87-10-git-send-email-hongxing.zhu%40nxp.com%2F&data=04%7C01%7
Chongxing.zhu%40nxp.com%7Cb796532d98124790154a08d997daf1e4%7C6
86ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637707788885954739%
7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJ
BTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NRK9mIMkYsgcXzBsL
I7GE62hy64Bumxr8WdCD1oh59w%3D&reserved=0

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy