-----Original Message-----
From: Tim Harvey <tharvey@gateworks.com>
Sent: Wednesday, October 27, 2021 11:41 PM
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 Tue, Oct 26, 2021 at 11:18 PM Richard Zhu [off-list ref]
wrote:

quoted

quoted

-----Original Message-----
From: Tim Harvey <tharvey@gateworks.com>
Sent: Wednesday, October 27, 2021 12:06 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

quoted

quoted

pcie support

On Mon, Oct 25, 2021 at 10:41 PM Richard Zhu

[off-list ref]

quoted

quoted

wrote:

quoted

quoted

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

quoted

quoted

or

quoted

quoted

the

quoted

quoted

quoted

chapter 5.5 L1 PM Substates of "PCI Express Base

Specification, Rev.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+++ tsi

@@ -448,7 +448,9 @@

        pinctrl_pcie0: pcie0grp {
                fsl,pins = <
+/*

MX8MM_IOMUXC_I2C4_SCL_PCIE1_CLKREQ_B

0x61

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+*/

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.

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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?

quoted

quoted

quoted

quoted

quoted

quoted

quoted

[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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

enabled that must gate the clock internally to the host
controller

block.

quoted

quoted

quoted

quoted

quoted

quoted

I can confirm that asserting those GPR14 bits does
resolve my

issue:

quoted

quoted

quoted

quoted

quoted

quoted

#define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_VAL

BIT(11)

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

#define IMX8MM_GPR_PCIE_CLKREQ_OVERRIDE_EN

BIT(10)

quoted

quoted

quoted

quoted

quoted

quoted

       /*
        * 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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

UART4 and because I2C4_SCL and UART4_RXD are the only two

pads

quoted

quoted

quoted

quoted

that could be pinmuxed for CLKREQ# I can't use the workaround of
pin muxing

it.

quoted

quoted

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

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

quoted

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"

quoted

quoted

quoted

and BIT19 is used as "GPR_PCIE1_PHY_FUNC_I_AUX_EN" on

i.MX8MM.

quoted

quoted

quoted

I think the two bits descriptions are used to describe the BIT19
and BIT9

together refer to my guess.

quoted

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

quoted

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.

quoted

Richard,

Ok, if this is the case then drivers/pci/controller/dwc/pci-imx6.c
for IMX8MM should not touch GPR14 and '[v3,9/9] PCI: imx: add the

imx8mm pcie support'

quoted

quoted

should have this on top and squashed:

diff --git a/drivers/pci/controller/dwc/pci-imx6.c

b/drivers/pci/controller/dwc/pci-imx6.c
index 7c89bd1a6441..458d54c8e385 100644

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c

@@ -452,8 +452,12 @@ static int imx6_pcie_enable_ref_clk(struct

imx6_pcie *imx6_pcie)
                break;
        case IMX7D:
                break;
-       case IMX8MQ:
        case IMX8MM:
+               ret = clk_prepare_enable(imx6_pcie->pcie_aux);
+               if (ret)
+                       dev_err(dev, "unable to enable

pcie_aux

quoted

quoted

clock\n");
+               break;
+       case IMX8MQ:
                ret = clk_prepare_enable(imx6_pcie->pcie_aux);
                if (ret) {
                        dev_err(dev, "unable to enable

pcie_aux

quoted

quoted

clock\n");

[Richard Zhu] Sorry, I might don't understand what's meaning of the

changes.

quoted

What're the differences between before and after the changes?

The above change to your patch 'only' calls clk_prepare_enable for
IMX8MM and does not touch GPR14 bits as the IMX8MQ case does
(because as you point out the GPR14 bits differ between IMX8MM and
IMX8MQ).

[Richard Zhu] Got that, thanks a lot.

quoted

quoted

And your '[v3,5/9] phy: freescale: pcie: initialize the imx8 pcie
standalone phy driver' should have this on top and squashed:

diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c

b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
index 317cf61bff37..27ca0b9f1d92 100644

--- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c

@@ -43,7 +43,7 @@
 #define IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
 #define IMX8MM_GPR_PCIE_POWER_OFF      BIT(17)
 #define IMX8MM_GPR_PCIE_SSC_EN         BIT(16)
-#define IMX8MM_GPR_PCIE_REF_USE_PAD    BIT(9)
+#define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)

 struct imx8_pcie_phy {
        u32                     refclk_pad_mode;

@@ -63,12 +63,12 @@ static int imx8_pcie_phy_init(struct phy *phy)
        reset_control_assert(imx8_phy->reset);

        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

quoted

-                          IMX8MM_GPR_PCIE_REF_USE_PAD,
-                          imx8_phy->refclk_pad_mode == 1 ?
-                          IMX8MM_GPR_PCIE_REF_USE_PAD :

0);

quoted

quoted

+

IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE,

quoted

quoted

+                          0);
        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

quoted

                           IMX8MM_GPR_PCIE_AUX_EN,
-                          IMX8MM_GPR_PCIE_AUX_EN);
+                          imx8_phy->refclk_pad_mode ==
IMX8_PCIE_REFCLK_PAD_INPUT ?
+                          IMX8MM_GPR_PCIE_AUX_EN : 0);
        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

quoted

                           IMX8MM_GPR_PCIE_POWER_OFF,

0);

quoted

quoted

        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14, @@

quoted

quoted

-76,7 +76,7 @@ static int imx8_pcie_phy_init(struct phy *phy)

        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

quoted

IMX8MM_GPR_PCIE_REF_CLK_SEL,

quoted

quoted

-                          imx8_phy->refclk_pad_mode == 1 ?
+                          imx8_phy->refclk_pad_mode ==
IMX8_PCIE_REFCLK_PAD_INPUT ?

IMX8MM_GPR_PCIE_REF_CLK_EXT :

quoted

quoted

IMX8MM_GPR_PCIE_REF_CLK_PLL);

quoted

quoted

        usleep_range(100, 200);

I tested this and it works both on imx8mm-evk and imx8mm-venice-*
which both have external clkgen.

However, the above does not set

IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE for

quoted

quoted

the case where CLKREQ# is connected and thus should be used so I
think we need to add a property for that to define if CLKREQ# is
hooked up or not. I tested enabling

IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE

quoted

quoted

and as expected that worked on the imx8mm-evk which hooks up

CLKREQ#

quoted

quoted

but not imx8mm-venice which does not hook up CLKREQ#.

What do you think about adding a property for this?

[Richard Zhu] First of all, thanks a lot for your help to figure out the

details.

quoted

Agree with your proposal.
One optional property "fsl,clkreq-unsupported" would be added for the

CLKREQ# not hooked case later.

quoted

diff --git
a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
index 097ba2a28fb4..2264452924cc 100644

--- a/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/fsl,imx8-pcie-phy.yaml

@@ -58,6 +58,11 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     default: 0

+  fsl,clkreq-unsupported:
+    type: boolean
+    description: A boolean property whoes presence indicates the

CLKREQ#

quoted

+      signal isn't supported in the HW board design (optional

required).

quoted

+

A boolean property indicating the CLKREQ# signal is not supported in the
board design (optional)

[Richard Zhu] Okay, would be updated later.

quoted

diff --git a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c

b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
index 07eea39283ed..4b4402eaddcc 100644

--- a/drivers/phy/freescale/phy-fsl-imx8m-pcie.c
+++ b/drivers/phy/freescale/phy-fsl-imx8m-pcie.c

@@ -43,7 +43,7 @@
 #define IMX8MM_GPR_PCIE_CMN_RST                BIT(18)
 #define IMX8MM_GPR_PCIE_POWER_OFF      BIT(17)
 #define IMX8MM_GPR_PCIE_SSC_EN         BIT(16)
-#define IMX8MM_GPR_PCIE_REF_USE_PAD    BIT(9)
+#define IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE        BIT(9)

 struct imx8_pcie_phy {
        void __iomem            *base;

@@ -54,6 +54,7 @@ struct imx8_pcie_phy {
        u32                     refclk_pad_mode;
        u32                     tx_deemph_gen1;
        u32                     tx_deemph_gen2;
+       bool                    clkreq_unused;
 };

 static int imx8_pcie_phy_init(struct phy *phy) @@ -65,13 +66,15 @@

static int imx8_pcie_phy_init(struct phy *phy)
        reset_control_assert(imx8_phy->reset);

        pad_mode = imx8_phy->refclk_pad_mode;
+       /* Set AUX_EN_OVERRIDE 1'b0, when the CLKREQ# isn't

hooked */

quoted

        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

-                          IMX8MM_GPR_PCIE_REF_USE_PAD,
-                          pad_mode ==

IMX8MM_GPR_PCIE_REF_USE_PAD ?

quoted

-                          IMX8MM_GPR_PCIE_REF_USE_PAD :

0);

quoted

+

IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE,

quoted

+                          imx8_phy->clkreq_unused ?
+                          0 :

IMX8MM_GPR_PCIE_AUX_EN_OVERRIDE);

quoted

        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

                           IMX8MM_GPR_PCIE_AUX_EN,
-                          IMX8MM_GPR_PCIE_AUX_EN);
+                          pad_mode ==

IMX8_PCIE_REFCLK_PAD_INPUT ?

quoted

+                          IMX8MM_GPR_PCIE_AUX_EN : 0);
        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14,

quoted

                           IMX8MM_GPR_PCIE_POWER_OFF,

0);

quoted

        regmap_update_bits(imx8_phy->iomuxc_gpr,

IOMUXC_GPR14, @@

quoted

-171,6 +174,11 @@ static int imx8_pcie_phy_probe(struct

platform_device *pdev)

quoted

&imx8_phy->tx_deemph_gen2))

quoted

                imx8_phy->tx_deemph_gen2 = 0;

+       if (of_property_read_bool(np, "fsl,clkreq-unsupported"))
+               imx8_phy->clkreq_unused = true;
+       else
+               imx8_phy->clkreq_unused = false;
+
        imx8_phy->clk = devm_clk_get(dev, "ref");
        if (IS_ERR(imx8_phy->clk)) {
                dev_err(dev, "failed to get imx pcie phy clock\n");

Yes this looks good and works both on venice (with
'fsl,clkreq-unsupported' added) and on imx8mm-evk.

Thanks for working through this with me and please cc me on your v4
submission.

[Richard Zhu] Welcome. I'm glad to have your help. Thanks a lot.

Best regards,

Tim

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