Date:   Mon, 25 Oct 2021 07:23:47 +0000
From:   Richard Zhu <hongxing.zhu@....com>
To:     "tharvey@...eworks.com" <tharvey@...eworks.com>
CC:     Lucas Stach <l.stach@...gutronix.de>,
        Kishon Vijay Abraham I <kishon@...com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        Rob Herring <robh@...nel.org>,
        "galak@...nel.crashing.org" <galak@...nel.crashing.org>,
        Shawn Guo <shawnguo@...nel.org>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        Device Tree Mailing List <devicetree@...r.kernel.org>,
        Linux ARM Mailing List <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>,
        Sascha Hauer <kernel@...gutronix.de>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH v3 0/9] add the imx8m pcie phy driver and imx8mm pcie
 support

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

Powered by blists - more mailing lists