[PATCH v4 1/2] ARM: imx: mach-imx6q: Search for fsl, imx6q-iomuxc-gpr earlier

[PATCH v4 1/2] ARM: imx: mach-imx6q: Search for fsl, imx6q-iomuxc-gpr earlier

[Sven Van Asbroeck]

From: Fabio Estevam <festevam@gmail.com>

Check the presence of fsl,imx6q-iomuxc-gpr earlier and exit in case
of failure.

This is done in preparation for adding support for configuring the
GPR5 register for i.MX6QP a bit easier.

Signed-off-by: Fabio Estevam <festevam@gmail.com>
---

Patch history: see
  [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref

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 | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 85c084a716ab..ae89ad93ca83 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -169,6 +169,12 @@ static void __init imx6q_1588_init(void)
 	struct regmap *gpr;
 	u32 clksel;
 
+	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+	if (IS_ERR(gpr)) {
+		pr_err("failed to find fsl,imx6q-iomuxc-gpr regmap\n");
+		return;
+	}
+
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-fec");
 	if (!np) {
 		pr_warn("%s: failed to find fec node\n", __func__);
@@ -195,13 +201,8 @@ static void __init imx6q_1588_init(void)
 	clksel = clk_is_match(ptp_clk, enet_ref) ?
 				IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
 				IMX6Q_GPR1_ENET_CLK_SEL_PAD;
-	gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
-	if (!IS_ERR(gpr))
-		regmap_update_bits(gpr, IOMUXC_GPR1,
-				IMX6Q_GPR1_ENET_CLK_SEL_MASK,
-				clksel);
-	else
-		pr_err("failed to find fsl,imx6q-iomuxc-gpr regmap\n");
+	regmap_update_bits(gpr, IOMUXC_GPR1, IMX6Q_GPR1_ENET_CLK_SEL_MASK,
+			   clksel);
 
 	clk_put(enet_ref);
 put_ptp_clk:
-- 
2.17.1


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

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

[Sven Van Asbroeck]

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(+)

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


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

RE: [EXT] [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

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


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

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

[Fabio Estevam]

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.

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

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

[Sven Van Asbroeck]

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?

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

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

[Fabio Estevam]

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

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

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

[Fabio Estevam]

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.

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

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

[Sven Van Asbroeck]

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.

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

RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Fabio Estevam]

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.

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

RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Sven Van Asbroeck]

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

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

RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Tuesday, June 30, 2020 11:24 PM
> 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...?

Fabio, the reason is that you don't update uboot that why we cannot reproduce
the issue on imx6qp sabresd.

Sven, uboot board file set the clock rate.
board/freescale/mx6sabresd/mx6sabresd.c:
        if (is_mx6dqp()) {
                int ret;

                /* select ENET MAC0 TX clock from PLL */
                imx_iomux_set_gpr_register(5, 9, 1, 1);
                ret = enable_fec_anatop_clock(0, ENET_125MHZ);
                if (ret)
                    printf("Error fec anatop clock settings!\n");
        }

Sven, to avoid to depend on uboot setting, for the patch, it is better to bind
below change for dts (even if non imx6qp, ptp clock can be set to 125Mhz):

--- 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>;
        phy-mode = "rgmii-id"; 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Fabio Estevam]

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.

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

RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Fabio Estevam]

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.

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

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Sven Van Asbroeck]

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??

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

RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Sven Van Asbroeck]

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.

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

Re: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Fabio Estevam]

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

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

RE: [EXT] Re: [PATCH v4 2/2] ARM: imx6plus: enable internal routing of clk_enet_ref where possible

[Andy Duan]

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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel