diff mbox series

[v4,2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

Message ID 20200625140105.14999-2-TheSven73@gmail.com
State New
Headers show
Series [v4,1/2] ARM: imx: mach-imx6q: Search for fsl, imx6q-iomuxc-gpr earlier | expand

Commit Message

Sven Van Asbroeck June 25, 2020, 2:01 p.m. UTC
On imx6, the ethernet reference clock (clk_enet_ref) can be generated
by either the imx6, or an external source (e.g. an oscillator or the
PHY). When generated by the imx6, the clock source (from ANATOP)
must be routed to the input of clk_enet_ref via two pads on the SoC,
typically via a dedicated track on the PCB.

On an imx6 plus however, there is a new setting which enables this
clock to be routed internally on the SoC, from its ANATOP clock
source, straight to clk_enet_ref, without having to go through
the SoC pads.

Board designs where the clock is generated by the imx6 should not
be affected by routing the clock internally. Therefore on a plus,
we can enable internal routing by default.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
v3 -> v4:
  - avoid double-check for IS_ERR(gpr) by including Fabio Estevam's
    patch.
v2 -> v3:
  - remove check for imx6q, which is already implied when
    of_machine_is_compatible("fsl,imx6qp")
v1 -> v2:
  - Fabio Estevam: use of_machine_is_compatible() to determine if we
    are running on an imx6 plus.

To: Shawn Guo <shawnguo@kernel.org>
To: Andy Duan <fugang.duan@nxp.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

 arch/arm/mach-imx/mach-imx6q.c              | 14 ++++++++++++++
 include/linux/mfd/syscon/imx6q-iomuxc-gpr.h |  1 +
 2 files changed, 15 insertions(+)

Comments

Andy Duan June 28, 2020, 5:05 a.m. UTC | #1
From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Thursday, June 25, 2020 10:01 PM
> On imx6, the ethernet reference clock (clk_enet_ref) can be generated by
> either the imx6, or an external source (e.g. an oscillator or the PHY). When
> generated by the imx6, the clock source (from ANATOP) must be routed to the
> input of clk_enet_ref via two pads on the SoC, typically via a dedicated track
> on the PCB.
> 
> On an imx6 plus however, there is a new setting which enables this clock to
> be routed internally on the SoC, from its ANATOP clock source, straight to
> clk_enet_ref, without having to go through the SoC pads.
> 
> Board designs where the clock is generated by the imx6 should not be
> affected by routing the clock internally. Therefore on a plus, we can enable
> internal routing by default.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>

For the version:

Reviewed-by: Fugang Duan <fugang.duan@nxp.com>
> ---
> v3 -> v4:
>   - avoid double-check for IS_ERR(gpr) by including Fabio Estevam's
>     patch.
> v2 -> v3:
>   - remove check for imx6q, which is already implied when
>     of_machine_is_compatible("fsl,imx6qp")
> v1 -> v2:
>   - Fabio Estevam: use of_machine_is_compatible() to determine if we
>     are running on an imx6 plus.
> 
> To: Shawn Guo <shawnguo@kernel.org>
> To: Andy Duan <fugang.duan@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 
>  arch/arm/mach-imx/mach-imx6q.c              | 14 ++++++++++++++
>  include/linux/mfd/syscon/imx6q-iomuxc-gpr.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c
> b/arch/arm/mach-imx/mach-imx6q.c index ae89ad93ca83..07cfe0d349c3
> 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -204,6 +204,20 @@ static void __init imx6q_1588_init(void)
>         regmap_update_bits(gpr, IOMUXC_GPR1,
> IMX6Q_GPR1_ENET_CLK_SEL_MASK,
>                            clksel);
> 
> +       /*
> +        * On imx6 plus, enet_ref from ANATOP/CCM can be internally
> routed to
> +        * be the PTP clock source, instead of having to be routed through
> +        * pads.
> +        * Board designs which route the ANATOP/CCM clock through pads
> are
> +        * unaffected when routing happens internally. So on these designs,
> +        * route internally by default.
> +        */
> +       if (clksel == IMX6Q_GPR1_ENET_CLK_SEL_ANATOP &&
> +                       of_machine_is_compatible("fsl,imx6qp"))
> +               regmap_update_bits(gpr, IOMUXC_GPR5,
> +                               IMX6Q_GPR5_ENET_TXCLK_SEL,
> +                               IMX6Q_GPR5_ENET_TXCLK_SEL);
> +
>         clk_put(enet_ref);
>  put_ptp_clk:
>         clk_put(ptp_clk);
> diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> index d4b5e527a7a3..eb65d48da0df 100644
> --- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> +++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> @@ -240,6 +240,7 @@
>  #define IMX6Q_GPR4_IPU_RD_CACHE_CTL            BIT(0)
> 
>  #define IMX6Q_GPR5_L2_CLK_STOP                 BIT(8)
> +#define IMX6Q_GPR5_ENET_TXCLK_SEL              BIT(9)
>  #define IMX6Q_GPR5_SATA_SW_PD                  BIT(10)
>  #define IMX6Q_GPR5_SATA_SW_RST                 BIT(11)
> 
> --
> 2.17.1
Fabio Estevam June 29, 2020, 1:09 p.m. UTC | #2
Hi Sven,

On Thu, Jun 25, 2020 at 11:01 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> On imx6, the ethernet reference clock (clk_enet_ref) can be generated
> by either the imx6, or an external source (e.g. an oscillator or the
> PHY). When generated by the imx6, the clock source (from ANATOP)
> must be routed to the input of clk_enet_ref via two pads on the SoC,
> typically via a dedicated track on the PCB.
>
> On an imx6 plus however, there is a new setting which enables this
> clock to be routed internally on the SoC, from its ANATOP clock
> source, straight to clk_enet_ref, without having to go through
> the SoC pads.
>
> Board designs where the clock is generated by the imx6 should not
> be affected by routing the clock internally. Therefore on a plus,
> we can enable internal routing by default.
>
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>

I have tested this series on an imx6qp sabresd and unfortunately, it
breaks Ethernet as I can no longer get an IP address from the DHCP
server.

Without this series, IP address can normally be retrieved.

Therefore I suggest to create a device tree property for this feature
and only enable it when such device tree property is present.
Sven Van Asbroeck June 29, 2020, 1:40 p.m. UTC | #3
Hi Fabio,

On Mon, Jun 29, 2020 at 9:10 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> I have tested this series on an imx6qp sabresd and unfortunately, it
> breaks Ethernet as I can no longer get an IP address from the DHCP
> server.

Thank you for testing this out on a different platform !

I had a look at how things are done in the Freescale fork of the kernel
(5.4.24_2.1.0) and I noticed that this kernel has almost the same
behaviour as this proposed patch: the GPR5 bit is _always_ set
on a plus. The code does not check how the enet clock is generated.

https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm/mach-imx/mach-imx6q.c?h=rel_imx_5.4.24_2.1.0&id=babac008e5cf168abca1a85bda2e8071ca27a5c0#n269

Now, I'm assuming that the sabresd-plus can run on the Freescale
kernel fork. The GPR5 bit will always be set there.

So why won't mainline work with this patch? What have I overlooked?

I'm sure you've checked that sabresd ethernet works ok on mainline
even without this patch?
Fabio Estevam June 29, 2020, 2:04 p.m. UTC | #4
Hi Sven,

On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:

> I'm sure you've checked that sabresd ethernet works ok on mainline
> even without this patch?

Yes, I have tested several times: without this patch series, DHCP works fine.

Applying this series causes DHCP to fail. The test is 100% reproducible.

Cheers
Fabio Estevam June 29, 2020, 2:25 p.m. UTC | #5
Hi Sven,

On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:

> Thank you for testing this out on a different platform !
>
> I had a look at how things are done in the Freescale fork of the kernel
> (5.4.24_2.1.0) and I noticed that this kernel has almost the same
> behaviour as this proposed patch: the GPR5 bit is _always_ set
> on a plus. The code does not check how the enet clock is generated.
>
> https://source.codeaurora.org/external/imx/linux-imx/tree/arch/arm/mach-imx/mach-imx6q.c?h=rel_imx_5.4.24_2.1.0&id=babac008e5cf168abca1a85bda2e8071ca27a5c0#n269
>
> Now, I'm assuming that the sabresd-plus can run on the Freescale
> kernel fork. The GPR5 bit will always be set there.

Just tested 5.4.24_2.1.0 on an imx6qp sabresd and DHCP also fails there.
Sven Van Asbroeck June 29, 2020, 2:37 p.m. UTC | #6
Hi Fabio,

On Mon, Jun 29, 2020 at 10:26 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Just tested 5.4.24_2.1.0 on an imx6qp sabresd and DHCP also fails there.

I think I discovered the problem !

When I compare the sabresd devicetree on mainline with the actual sabresd
schematics, the devicetree is incorrect ! Things still work, but only
by accident.

The sabresd has an AR8131 PHY, which generates the enet ref clock, not the
imx6. So on the schematic we see that the clock output of the PHY is wired
to imx6 ENET_REF_CLK, so it can be used as a clock source. And GPIO_16
is disconnected, as it should, because the imx6 is not generating the ref clk.

But the devicetree is written as if the imx6 is providing the clock ! See
here:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/boot/dts/imx6qdl-sabresd.dtsi?h=v5.7.6#n513

Also there is no override of the fec PTP clock:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm/boot/dts/imx6qdl-sabresd.dtsi?h=v5.7.6#n202

Although Shawn's mainline patch mandates this?
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=v5.7.6&id=810c0ca879098a993e2ce0a190d24d11c17df748

This will work, but only by accident. So on a plus, when we
(incorrectly) switch the
bypass bit on, things stop working.
Andy Duan June 30, 2020, 2:24 a.m. UTC | #7
From: Fabio Estevam <festevam@gmail.com> Sent: Monday, June 29, 2020 10:26 PM
> Hi Sven,
> 
> On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck
> <thesven73@gmail.com> wrote:
> 
> > Thank you for testing this out on a different platform !
> >
> > I had a look at how things are done in the Freescale fork of the
> > kernel
> > (5.4.24_2.1.0) and I noticed that this kernel has almost the same
> > behaviour as this proposed patch: the GPR5 bit is _always_ set on a
> > plus. The code does not check how the enet clock is generated.
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsour
> >
> ce.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Ftree%2Farch%2Farm
> %2Fm
> >
> ach-imx%2Fmach-imx6q.c%3Fh%3Drel_imx_5.4.24_2.1.0%26id%3Dbabac00
> 8e5cf1
> >
> 68abca1a85bda2e8071ca27a5c0%23n269&amp;data=02%7C01%7Cfugang.d
> uan%40nx
> >
> p.com%7C8570de0304514796ea0208d81c385a11%7C686ea1d3bc2b4c6fa92
> cd99c5c3
> >
> 01635%7C0%7C1%7C637290375659016888&amp;sdata=I9werBT%2FDkcWu
> LEKlFVzRi2
> > KD%2FLwPz2QCqw%2BHn0HY8U%3D&amp;reserved=0
> >
> > Now, I'm assuming that the sabresd-plus can run on the Freescale
> > kernel fork. The GPR5 bit will always be set there.
> 
> Just tested 5.4.24_2.1.0 on an imx6qp sabresd and DHCP also fails there.

Fabio, we have LAVA daily test by networking for imx6qp sabresd board, no any issue found.
Please double check the issue on your local.
Andy Duan June 30, 2020, 6:36 a.m. UTC | #8
From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Monday, June 29, 2020 10:37 PM
> On Mon, Jun 29, 2020 at 10:26 AM Fabio Estevam <festevam@gmail.com>
> wrote:
> >
> > Just tested 5.4.24_2.1.0 on an imx6qp sabresd and DHCP also fails there.
> 
> I think I discovered the problem !
> 
> When I compare the sabresd devicetree on mainline with the actual sabresd
> schematics, the devicetree is incorrect ! Things still work, but only by
> accident.
> 
> The sabresd has an AR8131 PHY, which generates the enet ref clock, not the
> imx6. So on the schematic we see that the clock output of the PHY is wired to
> imx6 ENET_REF_CLK, so it can be used as a clock source. And GPIO_16 is
> disconnected, as it should, because the imx6 is not generating the ref clk.
> 
> But the devicetree is written as if the imx6 is providing the clock ! See
> here:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Ftr
> ee%2Farch%2Farm%2Fboot%2Fdts%2Fimx6qdl-sabresd.dtsi%3Fh%3Dv5.7.6
> %23n513&amp;data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d
> 8e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637290382588751887&amp;sdata=hCRNGa9WrQzQ0XYwR%2FQTQ8h
> jY7CDHjhIbXr0L33fr%2Bc%3D&amp;reserved=0
> 
> Also there is no override of the fec PTP clock:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Ftr
> ee%2Farch%2Farm%2Fboot%2Fdts%2Fimx6qdl-sabresd.dtsi%3Fh%3Dv5.7.6
> %23n202&amp;data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d
> 8e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637290382588751887&amp;sdata=qOfiLs%2FGi01h9hSA787PHfGxTN
> bfYWFXVA3IhUB553Q%3D&amp;reserved=0
> 
> Although Shawn's mainline patch mandates this?
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fstable%2Flinux.git%2Fco
> mmit%2F%3Fh%3Dv5.7.6%26id%3D810c0ca879098a993e2ce0a190d24d11c
> 17df748&amp;data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d8
> e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637290382588751887&amp;sdata=3PiAnDlxO8iqsVR6YQWjCk1xsg8iW
> RK8TSGee4LhkjI%3D&amp;reserved=0
> 
> This will work, but only by accident. So on a plus, when we
> (incorrectly) switch the
> bypass bit on, things stop working.

Fabio, our QA double verify 5.4.24_2.1.0, no matter SD boot or tftp/nfs boot,
both work fine on i.MX6QP sabresd board,  please double check your environment.

Sven, no matter PHY supply 125Mhz clock to pad or not,  GPR5[9] is to select RGMII
gtx clock source from:
- 0 Clock from pad
- 1 Clock from PLL

Since i.MX6QP can internally supply clock to MAC, we can set GPR5[9] bit by default.
Fabio Estevam June 30, 2020, 11:49 a.m. UTC | #9
Hi Andy,

On Tue, Jun 30, 2020 at 3:36 AM Andy Duan <fugang.duan@nxp.com> wrote:

> Fabio, our QA double verify 5.4.24_2.1.0, no matter SD boot or tftp/nfs boot,
> both work fine on i.MX6QP sabresd board,  please double check your environment.

Is static IP or DHCP used on these tests?

I have an SCH-28857 REV A2 imx6qp sabresd board here and "udhcpc -i
eth0" fails 100% of the times if GPR5[9] is set.
Sven Van Asbroeck June 30, 2020, 3:23 p.m. UTC | #10
Andy, Fabio,

On Tue, Jun 30, 2020 at 2:36 AM Andy Duan <fugang.duan@nxp.com> wrote:
>
> Sven, no matter PHY supply 125Mhz clock to pad or not,  GPR5[9] is to select RGMII
> gtx clock source from:
> - 0 Clock from pad
> - 1 Clock from PLL
>
> Since i.MX6QP can internally supply clock to MAC, we can set GPR5[9] bit by default.

That's true. But on the sabresd I notice that the PHY's ref_clk output
is from CLK_25M.
The default ref_clk freq for that PHY is 25 MHz, and I don't see anyone change
the default in the devicetree. I also see that a 25 MHz crystal is fitted, which
also suggests 25 Mhz output.

On the imx6, the default ref_clk frequency from ANATOP is 50Mhz. I don't
see anyone change that default in the devicetree either.

So is it possible that, when we switch GPR5[9] on, the external 25MHz clock
is replaced by the internal 50MHz clock? If so, I'm not sure it'll work...?
Andy Duan July 1, 2020, 1:31 a.m. UTC | #11
From: Fabio Estevam <festevam@gmail.com> Sent: Tuesday, June 30, 2020 7:49 PM
> Hi Andy,
> 
> On Tue, Jun 30, 2020 at 3:36 AM Andy Duan <fugang.duan@nxp.com> wrote:
> 
> > Fabio, our QA double verify 5.4.24_2.1.0, no matter SD boot or
> > tftp/nfs boot, both work fine on i.MX6QP sabresd board,  please double
> check your environment.
> 
> Is static IP or DHCP used on these tests?
>
SD boot into yocto system, then run dhcp.
Or, nfs boot.

Both works fine.
Fabio Estevam July 1, 2020, 3:39 a.m. UTC | #12
Hi Andy,

On Wed, Jul 1, 2020 at 12:18 AM Andy Duan <fugang.duan@nxp.com> wrote:

> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -202,6 +202,8 @@
>  &fec {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_enet>;
> +       assigned-clocks = <&clks IMX6QDL_CLK_ENET_REF>;
> +       assigned-clock-rates = <125000000>;

I don't think this is an acceptable solution as it breaks old dtb's.
Andy Duan July 1, 2020, 3:42 a.m. UTC | #13
From: Fabio Estevam <festevam@gmail.com> Sent: Wednesday, July 1, 2020 11:39 AM 
> Hi Andy,
> 
> On Wed, Jul 1, 2020 at 12:18 AM Andy Duan <fugang.duan@nxp.com> wrote:
> 
> > --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> > @@ -202,6 +202,8 @@
> >  &fec {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_enet>;
> > +       assigned-clocks = <&clks IMX6QDL_CLK_ENET_REF>;
> > +       assigned-clock-rates = <125000000>;
> 
> I don't think this is an acceptable solution as it breaks old dtb's.

It doesn't break old dtbs, and doesn't break imx6q/dl/solo.
Fabio Estevam July 1, 2020, 3:45 a.m. UTC | #14
On Wed, Jul 1, 2020 at 12:42 AM Andy Duan <fugang.duan@nxp.com> wrote:

> It doesn't break old dtbs, and doesn't break imx6q/dl/solo.

Well, it breaks imx6qp as I said multiple times.

It does not break in your case because you are using NXP U-Boot.

You cannot assume people are using NXP U-Boot.
Sven Van Asbroeck July 1, 2020, 1:51 p.m. UTC | #15
Andy, Fabio,

Does the following describe the mainline situation?
Please correct if not.

1. imx6 ethernet ref_clk can be generated internally (by imx6) or
   externally (by PHY or oscillator on PCB)
2. if internal, fec's "ptp" clock in devicetree should be
   <&clks IMX6QDL_CLK_ENET_REF>
3. if external, fec's "ptp" clock should be that external clock,
   see 810c0ca879098 ("ARM: imx6q: support ptp and rmii clock from pad")
4. sabresd devicetree describes "ptp" clock as IMX6QDL_CLK_ENET_REF,
   although it's externally supplied (by PHY). This is incorrect.
5. nevertheless sabresd will work, because FEC driver can still work
   when the PTP clock in devicetree is different from supplied PTP clock
6. sabresd plus believes FEC is clocked internally, so flips the bit
   which routes the ptp clock internally
7. this breaks sabresd plus, as default internal clock is unsuitable
8. sabresd is sample board, so all boards based on sabresd may have
   the same issue, and break

Solution 1:
- describe sabresd ptp clock correctly in devicetree
- "clean/correct" solution
- may break other imx6q plus boards in mainline
- may break out-of-tree (private) imx6q plus devicetrees based on
  sabresd

Solution 2:
- on plus, never route PTP clock internally by default
  use a devicetree property instead
- complex solution, hard to understand if newcomer
- prevents sabresd / clones breakage

Solution 3:
- set sabresd ptp clock freq to same as externally supplied clock
- may still break designs based on sabresd
- hard to understand what's actually happening

Other solutions??
Andy Duan July 1, 2020, 3:30 p.m. UTC | #16
From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Wednesday, July 1, 2020 9:52 PM
> Andy, Fabio,
> 
> Does the following describe the mainline situation?
> Please correct if not.
> 
> 1. imx6 ethernet ref_clk can be generated internally (by imx6) or
>    externally (by PHY or oscillator on PCB) 2. if internal, fec's "ptp" clock in
> devicetree should be
>    <&clks IMX6QDL_CLK_ENET_REF>
> 3. if external, fec's "ptp" clock should be that external clock,
>    see 810c0ca879098 ("ARM: imx6q: support ptp and rmii clock from pad")
> 4. sabresd devicetree describes "ptp" clock as IMX6QDL_CLK_ENET_REF,
>    although it's externally supplied (by PHY). This is incorrect.
No, ptp_clk is the same as enet_ref, it means ptp clock source from internal pll.
So, for current upstream status for imx6q/6dl/6qp, ptp clock is from internal pll,
rgmii gtx clock is from phy. 

For 6qp, soc already support to route internal pll to rgmii gtx by setting gpr5[9],
the patch force to use internal pll instead of external clk from phy. It doesn't
break imx6q/6dl. But as Fabio's said, it break old 6qp sabresd dtb.

Discuss with Fabio, an existing(old) dtb in mainline has to work in future kernels,
without the need of being updated, so to add internal pll support for 6qp rgmii gtx,
and not to break 6qp old dtb, add new property is one solution.
Sven Van Asbroeck July 1, 2020, 4:03 p.m. UTC | #17
Hi Andy and Fabio,

On Wed, Jul 1, 2020 at 11:30 AM Andy Duan <fugang.duan@nxp.com> wrote:
>
> Discuss with Fabio, an existing(old) dtb in mainline has to work in future kernels,
> without the need of being updated, so to add internal pll support for 6qp rgmii gtx,
> and not to break 6qp old dtb, add new property is one solution.

Andy, many thanks for your time and attention on this issue, much appreciated !

Fabio has already indicated that he's ok with adding a new property.
Fabio, is that still the case?

If so, I will re-spin the patch to use a new property.
Hopefully Rob Herring will be ok with this.
Fabio Estevam July 1, 2020, 4:39 p.m. UTC | #18
Hi Sven,

On Wed, Jul 1, 2020 at 1:03 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Fabio has already indicated that he's ok with adding a new property.
> Fabio, is that still the case?

Yes, please proceed with adding the new device tree property.

This way we prevent existing imx6qp dtb's breakage.

Thanks
diff mbox series

Patch

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index ae89ad93ca83..07cfe0d349c3 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -204,6 +204,20 @@  static void __init imx6q_1588_init(void)
 	regmap_update_bits(gpr, IOMUXC_GPR1, IMX6Q_GPR1_ENET_CLK_SEL_MASK,
 			   clksel);
 
+	/*
+	 * On imx6 plus, enet_ref from ANATOP/CCM can be internally routed to
+	 * be the PTP clock source, instead of having to be routed through
+	 * pads.
+	 * Board designs which route the ANATOP/CCM clock through pads are
+	 * unaffected when routing happens internally. So on these designs,
+	 * route internally by default.
+	 */
+	if (clksel == IMX6Q_GPR1_ENET_CLK_SEL_ANATOP &&
+			of_machine_is_compatible("fsl,imx6qp"))
+		regmap_update_bits(gpr, IOMUXC_GPR5,
+				IMX6Q_GPR5_ENET_TXCLK_SEL,
+				IMX6Q_GPR5_ENET_TXCLK_SEL);
+
 	clk_put(enet_ref);
 put_ptp_clk:
 	clk_put(ptp_clk);
diff --git a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
index d4b5e527a7a3..eb65d48da0df 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -240,6 +240,7 @@ 
 #define IMX6Q_GPR4_IPU_RD_CACHE_CTL		BIT(0)
 
 #define IMX6Q_GPR5_L2_CLK_STOP			BIT(8)
+#define IMX6Q_GPR5_ENET_TXCLK_SEL		BIT(9)
 #define IMX6Q_GPR5_SATA_SW_PD			BIT(10)
 #define IMX6Q_GPR5_SATA_SW_RST			BIT(11)