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 <[email protected]>
---
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 <[email protected]>
To: Andy Duan <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Pengutronix Kernel Team <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: NXP Linux Team <[email protected]>
Cc: [email protected]
Cc: [email protected]
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
From: Sven Van Asbroeck <[email protected]> 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 <[email protected]>
For the version:
Reviewed-by: Fugang Duan <[email protected]>
> ---
> 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 <[email protected]>
> To: Andy Duan <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: Pengutronix Kernel Team <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: NXP Linux Team <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
>
> 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
Hi Fabio,
On Mon, Jun 29, 2020 at 10:26 AM Fabio Estevam <[email protected]> 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.
Hi Fabio,
On Mon, Jun 29, 2020 at 9:10 AM Fabio Estevam <[email protected]> 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?
Hi Sven,
On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck <[email protected]> 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.
Hi Sven,
On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck <[email protected]> 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
Hi Sven,
On Thu, Jun 25, 2020 at 11:01 AM Sven Van Asbroeck <[email protected]> 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 <[email protected]>
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.
From: Fabio Estevam <[email protected]> Sent: Monday, June 29, 2020 10:26 PM
> Hi Sven,
>
> On Mon, Jun 29, 2020 at 10:40 AM Sven Van Asbroeck
> <[email protected]> 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&data=02%7C01%7Cfugang.d
> uan%40nx
> >
> p.com%7C8570de0304514796ea0208d81c385a11%7C686ea1d3bc2b4c6fa92
> cd99c5c3
> >
> 01635%7C0%7C1%7C637290375659016888&sdata=I9werBT%2FDkcWu
> LEKlFVzRi2
> > KD%2FLwPz2QCqw%2BHn0HY8U%3D&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.
From: Sven Van Asbroeck <[email protected]> Sent: Monday, June 29, 2020 10:37 PM
> On Mon, Jun 29, 2020 at 10:26 AM Fabio Estevam <[email protected]>
> 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&data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d
> 8e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637290382588751887&sdata=hCRNGa9WrQzQ0XYwR%2FQTQ8h
> jY7CDHjhIbXr0L33fr%2Bc%3D&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&data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d
> 8e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7
> C0%7C637290382588751887&sdata=qOfiLs%2FGi01h9hSA787PHfGxTN
> bfYWFXVA3IhUB553Q%3D&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&data=02%7C01%7Cfugang.duan%40nxp.com%7C16c43e8d9d8
> e4ff0b9ee08d81c39f7f2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637290382588751887&sdata=3PiAnDlxO8iqsVR6YQWjCk1xsg8iW
> RK8TSGee4LhkjI%3D&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.
Hi Andy,
On Tue, Jun 30, 2020 at 3:36 AM Andy Duan <[email protected]> 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.
Andy, Fabio,
On Tue, Jun 30, 2020 at 2:36 AM Andy Duan <[email protected]> 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...?
From: Fabio Estevam <[email protected]> Sent: Tuesday, June 30, 2020 7:49 PM
> Hi Andy,
>
> On Tue, Jun 30, 2020 at 3:36 AM Andy Duan <[email protected]> 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.
From: Sven Van Asbroeck <[email protected]> Sent: Tuesday, June 30, 2020 11:24 PM
> Andy, Fabio,
>
> On Tue, Jun 30, 2020 at 2:36 AM Andy Duan <[email protected]> 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";
Hi Andy,
On Wed, Jul 1, 2020 at 12:18 AM Andy Duan <[email protected]> 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.
From: Fabio Estevam <[email protected]> Sent: Wednesday, July 1, 2020 11:39 AM
> Hi Andy,
>
> On Wed, Jul 1, 2020 at 12:18 AM Andy Duan <[email protected]> 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.
On Wed, Jul 1, 2020 at 12:42 AM Andy Duan <[email protected]> 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.
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??
From: Sven Van Asbroeck <[email protected]> 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.
Hi Andy and Fabio,
On Wed, Jul 1, 2020 at 11:30 AM Andy Duan <[email protected]> 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.
Hi Sven,
On Wed, Jul 1, 2020 at 1:03 PM Sven Van Asbroeck <[email protected]> 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