LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5 1/3] ARM: imx: mach-imx6q: Search for fsl,imx6q-iomuxc-gpr earlier
@ 2020-07-02 17:53 Sven Van Asbroeck
  2020-07-02 17:53 ` [PATCH v5 2/3] dt-bindings: fec: add fsl,ptpclk-bypass-pad boolean property Sven Van Asbroeck
  2020-07-02 17:53 ` [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref Sven Van Asbroeck
  0 siblings, 2 replies; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-02 17:53 UTC (permalink / raw)
  To: shawnguo, fugang.duan, robh+dt
  Cc: Fabio Estevam, David S. Miller, Jakub Kicinski, netdev,
	devicetree, Sascha Hauer, Pengutronix Kernel Team,
	NXP Linux Team, linux-arm-kernel, linux-kernel

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

Tree: v5.8-rc3

Patch history: see [PATCH v5 3/3]

To: Shawn Guo <shawnguo@kernel.org>
To: Andy Duan <fugang.duan@nxp.com>
To: Rob Herring <robh+dt@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Cc: devicetree@vger.kernel.org
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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v5 2/3] dt-bindings: fec: add fsl,ptpclk-bypass-pad boolean property
  2020-07-02 17:53 [PATCH v5 1/3] ARM: imx: mach-imx6q: Search for fsl,imx6q-iomuxc-gpr earlier Sven Van Asbroeck
@ 2020-07-02 17:53 ` Sven Van Asbroeck
  2020-07-02 17:53 ` [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref Sven Van Asbroeck
  1 sibling, 0 replies; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-02 17:53 UTC (permalink / raw)
  To: shawnguo, fugang.duan, robh+dt
  Cc: David S. Miller, Jakub Kicinski, netdev, devicetree,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel, linux-kernel

If present, sources the fec's PTP clock straight from
the enet PLL, instead of having to be routed via a SoC pad.

This is only possible on certain SoCs, notably the imx6 (quad) plus.

Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---

Tree: v5.8-rc3

Patch history: see [PATCH v5 3/3]

To: Shawn Guo <shawnguo@kernel.org>
To: Andy Duan <fugang.duan@nxp.com>
To: Rob Herring <robh+dt@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Cc: devicetree@vger.kernel.org
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

 Documentation/devicetree/bindings/net/fsl-fec.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 9b543789cd52..e34df25a38f6 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -39,6 +39,9 @@ Optional properties:
   tx/rx queues 1 and 2. "int0" will be used for queue 0 and ENET_MII interrupts.
   For imx6sx, "int0" handles all 3 queues and ENET_MII. "pps" is for the pulse
   per second interrupt associated with 1588 precision time protocol(PTP).
+- fsl,ptpclk-bypass-pad: If present, sources the fec's PTP clock straight from
+  the enet PLL, instead of having to be routed via a SoC pad. This is only
+  possible on certain SoCs, notably the imx6 (quad) plus.
 
 Optional subnodes:
 - mdio : specifies the mdio bus in the FEC, used as a container for phy nodes
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-02 17:53 [PATCH v5 1/3] ARM: imx: mach-imx6q: Search for fsl,imx6q-iomuxc-gpr earlier Sven Van Asbroeck
  2020-07-02 17:53 ` [PATCH v5 2/3] dt-bindings: fec: add fsl,ptpclk-bypass-pad boolean property Sven Van Asbroeck
@ 2020-07-02 17:53 ` Sven Van Asbroeck
  2020-07-02 22:29   ` Fabio Estevam
  1 sibling, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-02 17:53 UTC (permalink / raw)
  To: shawnguo, fugang.duan, robh+dt
  Cc: David S. Miller, Jakub Kicinski, netdev, devicetree,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, linux-arm-kernel, linux-kernel

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.

Enable internal routing if the fsl,ptpclk-bypass-pad boolean
property is present in the "fsl,imx6q-fec" devicetree node.

Link: https://lore.kernel.org/lkml/CAOMZO5BYC3DmE_G0XRwRH9vSJiVVvKbtznODyntsAuorF=HoqA@mail.gmail.com/
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---

Tree: v5.8-rc3

v4 -> v5:
  - identified that existing imx6q-plus boards could break ethernet if v4
    patch is applied.
    reached consensus: prevent breakage by requiring an explicit devicetree
    property for internal ptp clk routing.
    Link: https://lore.kernel.org/lkml/CAOMZO5BYC3DmE_G0XRwRH9vSJiVVvKbtznODyntsAuorF=HoqA@mail.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>
To: Rob Herring <robh+dt@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Cc: devicetree@vger.kernel.org
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 |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index ae89ad93ca83..ac62994eb7ba 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.
+	 */
+	if (of_machine_is_compatible("fsl,imx6qp")) {
+		clksel = of_property_read_bool(np, "fsl,ptpclk-bypass-pad") ?
+				IMX6Q_GPR5_ENET_TXCLK_SEL_PLL :
+				IMX6Q_GPR5_ENET_TXCLK_SEL_PAD;
+		regmap_update_bits(gpr, IOMUXC_GPR5,
+				   IMX6Q_GPR5_ENET_TXCLK_SEL_MASK, clksel);
+	}
+
+
 	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..58377002427f 100644
--- a/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
+++ b/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
@@ -240,6 +240,9 @@
 #define IMX6Q_GPR4_IPU_RD_CACHE_CTL		BIT(0)
 
 #define IMX6Q_GPR5_L2_CLK_STOP			BIT(8)
+#define IMX6Q_GPR5_ENET_TXCLK_SEL_MASK		BIT(9)
+#define IMX6Q_GPR5_ENET_TXCLK_SEL_PAD		0
+#define IMX6Q_GPR5_ENET_TXCLK_SEL_PLL		BIT(9)
 #define IMX6Q_GPR5_SATA_SW_PD			BIT(10)
 #define IMX6Q_GPR5_SATA_SW_RST			BIT(11)
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-02 17:53 ` [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref Sven Van Asbroeck
@ 2020-07-02 22:29   ` Fabio Estevam
  2020-07-03  0:50     ` Sven Van Asbroeck
  2020-07-04 14:08     ` Sven Van Asbroeck
  0 siblings, 2 replies; 17+ messages in thread
From: Fabio Estevam @ 2020-07-02 22:29 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Shawn Guo, Fugang Duan, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Sven,

On Thu, Jul 2, 2020 at 2:53 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:

> +       /*
> +        * 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.
> +        */
> +       if (of_machine_is_compatible("fsl,imx6qp")) {
> +               clksel = of_property_read_bool(np, "fsl,ptpclk-bypass-pad") ?
> +                               IMX6Q_GPR5_ENET_TXCLK_SEL_PLL :
> +                               IMX6Q_GPR5_ENET_TXCLK_SEL_PAD;
> +               regmap_update_bits(gpr, IOMUXC_GPR5,
> +                                  IMX6Q_GPR5_ENET_TXCLK_SEL_MASK, clksel);
> +       }

With the device tree approach, I think that a better place to touch
GPR5 would be inside the fec driver.

You can refer to drivers/pci/controller/dwc/pci-imx6.c and follow the
same approach for accessing the GPR register:
...
/* Grab GPR config register range */
imx6_pcie->iomuxc_gpr =
syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr")

For the property name, what about fsl,txclk-from-pll?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-02 22:29   ` Fabio Estevam
@ 2020-07-03  0:50     ` Sven Van Asbroeck
  2020-07-03  2:01       ` [EXT] " Andy Duan
  2020-07-04 14:08     ` Sven Van Asbroeck
  1 sibling, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-03  0:50 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Shawn Guo, Fugang Duan, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Fabio,

On Thu, Jul 2, 2020 at 6:29 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> With the device tree approach, I think that a better place to touch
> GPR5 would be inside the fec driver.
>

Cool idea. I notice that the latest FEC driver (v5.8-rc3) accesses individual
bits inside the gpr (via fsl,stop-mode). So perhaps I can do the same here,
and populate that gpr node in imx6qp.dtsi - because it doesn't exist on other
SoCs.

> For the property name, what about fsl,txclk-from-pll?

Sounds good. Does anyone have more suggestions?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-03  0:50     ` Sven Van Asbroeck
@ 2020-07-03  2:01       ` Andy Duan
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Duan @ 2020-07-03  2:01 UTC (permalink / raw)
  To: Sven Van Asbroeck, Fabio Estevam
  Cc: Shawn Guo, Rob Herring, David S. Miller, Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Friday, July 3, 2020 8:51 AM
> Hi Fabio,
> 
> On Thu, Jul 2, 2020 at 6:29 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > With the device tree approach, I think that a better place to touch
> > GPR5 would be inside the fec driver.
> >
> 
> Cool idea. I notice that the latest FEC driver (v5.8-rc3) accesses individual bits
> inside the gpr (via fsl,stop-mode). So perhaps I can do the same here, and
> populate that gpr node in imx6qp.dtsi - because it doesn't exist on other SoCs.
> 
> > For the property name, what about fsl,txclk-from-pll?
> 
> Sounds good. Does anyone have more suggestions?

The property seems good, don't let the property confuse somebody for other
platforms, binding doc describe the property is limited to use for imx6qp only.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-02 22:29   ` Fabio Estevam
  2020-07-03  0:50     ` Sven Van Asbroeck
@ 2020-07-04 14:08     ` Sven Van Asbroeck
  2020-07-05 14:45       ` [EXT] " Andy Duan
  1 sibling, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-04 14:08 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Shawn Guo, Fugang Duan, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Fabio, Andy,

On Thu, Jul 2, 2020 at 6:29 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> With the device tree approach, I think that a better place to touch
> GPR5 would be inside the fec driver.
>

Are we 100% sure this is the best way forward, though?

All the FEC driver should care about is the FEC logic block
inside the SoC. It should not concern itself with the way a SoC
happens to bring a clock (PTP clock) to the input of the FEC
logic block - that is purely a SoC implementation detail.

It makes sense to add fsl,stop-mode (= a GPR bit) to the FEC driver,
as this bit is a logic input to the FEC block, which the driver needs
to dynamically flip.

But the PTP clock is different, because it's statically routed by
the SoC.

Maybe this problem needs a clock routing solution?
Isn't there an imx6q plus clock mux we're not modelling?

  enet_ref-o------>ext>---pad_clk--| \
           |                       |M |----fec_ptp_clk
           o-----------------------|_/

Where M = mux controlled by GPR5[9]

The issue here is that pad_clk is routed externally, so its parent
could be any internal or external clock. I have no idea how to
model this in the clock framework.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-04 14:08     ` Sven Van Asbroeck
@ 2020-07-05 14:45       ` Andy Duan
  2020-07-05 15:34         ` Sven Van Asbroeck
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Duan @ 2020-07-05 14:45 UTC (permalink / raw)
  To: Sven Van Asbroeck, Fabio Estevam
  Cc: Shawn Guo, Rob Herring, David S. Miller, Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

From: Sven Van Asbroeck <thesven73@gmail.com>
> Hi Fabio, Andy,
> 
> On Thu, Jul 2, 2020 at 6:29 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > With the device tree approach, I think that a better place to touch
> > GPR5 would be inside the fec driver.
> >
> 
> Are we 100% sure this is the best way forward, though?
> 
> All the FEC driver should care about is the FEC logic block inside the SoC. It
> should not concern itself with the way a SoC happens to bring a clock (PTP
> clock) to the input of the FEC logic block - that is purely a SoC implementation
> detail.

I also agree with that relates to SOC integration. 
> 
> It makes sense to add fsl,stop-mode (= a GPR bit) to the FEC driver, as this bit
> is a logic input to the FEC block, which the driver needs to dynamically flip.
> 
> But the PTP clock is different, because it's statically routed by the SoC.
> 
> Maybe this problem needs a clock routing solution?
> Isn't there an imx6q plus clock mux we're not modelling?
> 
>   enet_ref-o------>ext>---pad_clk--| \
>            |                       |M |----fec_ptp_clk
>            o-----------------------|_/
> 
> Where M = mux controlled by GPR5[9]
> 
> The issue here is that pad_clk is routed externally, so its parent could be any
> internal or external clock. I have no idea how to model this in the clock
> framework.
Don't consider it complex, GPR5[9] just select the rgmii gtx source from PAD or internal
Like:
GPR5[9] is cleared: PAD -> MAC gtx
GPR5[9] is set: Pll_enet -> MAC gtx
As you said, register one clock mux for the selection, assign the clock parent by board dts
file, but now current clock driver doesn't support GPR clock. 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-05 14:45       ` [EXT] " Andy Duan
@ 2020-07-05 15:34         ` Sven Van Asbroeck
  2020-07-06  5:30           ` Andy Duan
  0 siblings, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-05 15:34 UTC (permalink / raw)
  To: Andy Duan
  Cc: Fabio Estevam, Shawn Guo, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Andy, thank you so much for your time and attention. See below.

On Sun, Jul 5, 2020 at 10:45 AM Andy Duan <fugang.duan@nxp.com> wrote:
>
> Don't consider it complex, GPR5[9] just select the rgmii gtx source from PAD or internal
> Like:
> GPR5[9] is cleared: PAD -> MAC gtx
> GPR5[9] is set: Pll_enet -> MAC gtx
> As you said, register one clock mux for the selection, assign the clock parent by board dts
> file, but now current clock driver doesn't support GPR clock.

Ok, so for imx6q plus only, we create two new clocks (MAC_GTX and PAD)
and a new clock mux, controlled by GPR5[9]:

  enet_ref-o------>ext>---pad------| \
           |                       |M |----mac_gtx
           o-----------------------|_/

Where M = mux controlled by GPR5[9]

clk_mac_gtx -> clk_pad -> clk_enet_ref is the default. when a board
wants internal routing, it can just do:

&fec {
assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
assigned-clock-parents = <&clks IMX6QDL_CLK_ENET_REF>;
};

But, how do we manage clk_pad? It is routed externally, and can be
connected to:
- enet_ref (typically via GPIO_16)
- an external oscillator
- an external PHY clock

  ext phy---------| \
                  |  |
  enet_ref-o------|M |----pad------| \
           |      |_/              |  |
           |                       |M |----mac_gtx
           |                       |  |
           o-----------------------|_/


How do we tell the clock framework that clk_pad has a mux that can
be connected to _any_ external clock? and also enet_ref?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-05 15:34         ` Sven Van Asbroeck
@ 2020-07-06  5:30           ` Andy Duan
  2020-07-06 13:46             ` Fabio Estevam
  2020-07-06 14:53             ` Sven Van Asbroeck
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Duan @ 2020-07-06  5:30 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Fabio Estevam, Shawn Guo, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Sunday, July 5, 2020 11:34 PM
> 
>   ext phy---------| \
>                   |  |
>   enet_ref-o------|M |----pad------| \
>            |      |_/              |  |
>            |                       |M |----mac_gtx
>            |                       |  |
>            o-----------------------|_/
> 
> 
> How do we tell the clock framework that clk_pad has a mux that can be
> connected to _any_ external clock? and also enet_ref?

The clock depends on board design, HW refer guide can describe the clk
usage in detail and customer select one clk path as their board design.

To make thing simple, we can just control the second "M" that is controlled
by gpr bit.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-06  5:30           ` Andy Duan
@ 2020-07-06 13:46             ` Fabio Estevam
  2020-07-06 14:58               ` Sven Van Asbroeck
  2020-07-06 14:53             ` Sven Van Asbroeck
  1 sibling, 1 reply; 17+ messages in thread
From: Fabio Estevam @ 2020-07-06 13:46 UTC (permalink / raw)
  To: Andy Duan
  Cc: Sven Van Asbroeck, Shawn Guo, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

On Mon, Jul 6, 2020 at 2:30 AM Andy Duan <fugang.duan@nxp.com> wrote:
>
> From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Sunday, July 5, 2020 11:34 PM
> >
> >   ext phy---------| \
> >                   |  |
> >   enet_ref-o------|M |----pad------| \
> >            |      |_/              |  |
> >            |                       |M |----mac_gtx
> >            |                       |  |
> >            o-----------------------|_/
> >
> >
> > How do we tell the clock framework that clk_pad has a mux that can be
> > connected to _any_ external clock? and also enet_ref?
>
> The clock depends on board design, HW refer guide can describe the clk
> usage in detail and customer select one clk path as their board design.
>
> To make thing simple, we can just control the second "M" that is controlled
> by gpr bit.

Would it make sense to use compatible = "mmio-mux"; like we do on
imx7s, imx6qdl.dtsi and imx8mq.dtsi?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-06  5:30           ` Andy Duan
  2020-07-06 13:46             ` Fabio Estevam
@ 2020-07-06 14:53             ` Sven Van Asbroeck
  1 sibling, 0 replies; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-06 14:53 UTC (permalink / raw)
  To: Andy Duan
  Cc: Fabio Estevam, Shawn Guo, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Andy,

On Mon, Jul 6, 2020 at 1:30 AM Andy Duan <fugang.duan@nxp.com> wrote:
>
> The clock depends on board design, HW refer guide can describe the clk
> usage in detail and customer select one clk path as their board design.
>

Yes, I agree. But how does a board designer practically describe this
in the devicetree?

Example: mac_gtx -> clk_pad (the default)

Imagine I have a fixed oscillator on the board:

phy_ref_clk: oscillator {
    compatible = "fixed-clock";
    #clock-cells = <0>;
    clock-frequency = <50000000>;
};

How can I now make <&phy_ref_clk> the parent of
<&clks CLK_PAD> ?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-06 13:46             ` Fabio Estevam
@ 2020-07-06 14:58               ` Sven Van Asbroeck
  2020-07-06 14:59                 ` Sven Van Asbroeck
  0 siblings, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-06 14:58 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Andy Duan, Shawn Guo, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Fabio,

On Mon, Jul 6, 2020 at 9:46 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Would it make sense to use compatible = "mmio-mux"; like we do on
> imx7s, imx6qdl.dtsi and imx8mq.dtsi?

Maybe "fixed-mmio-clock" is a better candidate ?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-06 14:58               ` Sven Van Asbroeck
@ 2020-07-06 14:59                 ` Sven Van Asbroeck
  2020-07-07  3:38                   ` Andy Duan
  0 siblings, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-06 14:59 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Andy Duan, Shawn Guo, Rob Herring, David S. Miller,
	Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

On Mon, Jul 6, 2020 at 10:58 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Hi Fabio,
>
> On Mon, Jul 6, 2020 at 9:46 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Would it make sense to use compatible = "mmio-mux"; like we do on
> > imx7s, imx6qdl.dtsi and imx8mq.dtsi?
>
> Maybe "fixed-mmio-clock" is a better candidate ?

Actually no, the mmio memory there only controls the frequency...

I don't think the clock framework has a ready-made abstraction
suitable for a GPR clock mux yet?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-06 14:59                 ` Sven Van Asbroeck
@ 2020-07-07  3:38                   ` Andy Duan
  2020-07-07 15:21                     ` Sven Van Asbroeck
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Duan @ 2020-07-07  3:38 UTC (permalink / raw)
  To: Sven Van Asbroeck, Fabio Estevam
  Cc: Shawn Guo, Rob Herring, David S. Miller, Jakub Kicinski, netdev,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Sascha Hauer, Pengutronix Kernel Team, dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Monday, July 6, 2020 11:00 PM
> On Mon, Jul 6, 2020 at 10:58 AM Sven Van Asbroeck <thesven73@gmail.com>
> wrote:
> >
> > Hi Fabio,
> >
> > On Mon, Jul 6, 2020 at 9:46 AM Fabio Estevam <festevam@gmail.com>
> wrote:
> > >
> > > Would it make sense to use compatible = "mmio-mux"; like we do on
> > > imx7s, imx6qdl.dtsi and imx8mq.dtsi?
> >
> > Maybe "fixed-mmio-clock" is a better candidate ?
> 
> Actually no, the mmio memory there only controls the frequency...
> 
> I don't think the clock framework has a ready-made abstraction suitable for a
> GPR clock mux yet?

That needs to add GPR clock API support.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-07  3:38                   ` Andy Duan
@ 2020-07-07 15:21                     ` Sven Van Asbroeck
  2020-07-08  5:16                       ` Andy Duan
  0 siblings, 1 reply; 17+ messages in thread
From: Sven Van Asbroeck @ 2020-07-07 15:21 UTC (permalink / raw)
  To: Andy Duan
  Cc: Fabio Estevam, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel, Michael Turquette, linux-clk

Andy, Fabio,

Sounds like we now have a solution which makes logical sense, although it
requires changes and additions to drivers/clk/imx/. Before I create a patch,
can you read the plan below and check that it makes sense, please?

Problem
=======
On the imx6q plus, the NXP hw designers introduced an alternative way to clock
the built-in ethernet (FEC). One single customer (archronix) seems to be
currently using this functionality, and would like to add mainline support.

Changing the ethernet (FEC) driver appears illogical, as this change is
unrelated to the FEC itself: fundamentally, it's a clock tree change.

We also need to prevent breaking existing boards with mis-configured FEC
clocking:

Some board designers indicate in the devicetree that ENET_REF_CLK is connected
to the FEC ptp clock, but in reality, they are providing an unrelated external
clock through the pad. This will work in many cases, because the FEC driver
does not really care where its PTP clock comes from, and the enet ref clock is
set up to mirror the external clk frequency by the NXP U-Boot fork.

Proposed changes must not break these cases.

Proposed Solution
=================
On non-plus imx6q there are no changes. On imx6q plus however:

Create two new clocks (mac_gtx and pad) and a new clock mux:

  enet_ref-o--------->pad----->| \
           |                   |  |
           |                   |M1|----mac_gtx --- to FEC
           |                   |  |
           o-------------------|_/

The defaults (indicated with ">") are carefully chosen, so these changes will
not break any existing plus designs.

We then tie the gtx clock to the FEC driver ptp clock, like so:

in imx6qp.dtsi:
&fec {
    clocks = <&clks IMX6QDL_CLK_ENET>,
        <&clks IMX6QDL_CLK_ENET>,
        <&clks IMX6QDL_CLK_MAC_GTX>;
};

Mux M1 is controlled by GPR5[9]. This mux can just write to the GPR5 memory
address. However, the GPR registers are already exposed as a mux controller
compatible = "mmio-mux". This mux does currently not use GPR5.

So we have to make a choice here: write straight to the memory address (easy),
or create a new clock mux type, which uses an underlying mux controller.

When that is done, board designers can choose between:

1. internal clocking (GTX clock routed internally)

&fec {
    assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
    assigned-clock-parents = <&clks IMX6QDL_CLK_ENET_REF>;
};

2. external clocking (GTX clock from pad, pad connected to enet_ref,
   typically via GPIO_16). This is the default and does not need to be present.

&fec {
    assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
    assigned-clock-parents = <&clks IMX6QDL_CLK_PAD>;
};


3. external clocking (GTX clock from pad, pad connected to external PHY
   clock or external oscillator).

phy_osc: oscillator {
#clock-cells = <0>;
compatible = "fixed-clock";
clock-frequency = <50000000>;
};

&fec {
    clocks = <&clks IMX6QDL_CLK_ENET>,
        <&clks IMX6QDL_CLK_ENET>,
        <&phy_osc>;
};

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref
  2020-07-07 15:21                     ` Sven Van Asbroeck
@ 2020-07-08  5:16                       ` Andy Duan
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Duan @ 2020-07-08  5:16 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Fabio Estevam, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	dl-linux-imx,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel, Michael Turquette, linux-clk

From: Sven Van Asbroeck <thesven73@gmail.com> Sent: Tuesday, July 7, 2020 11:21 PM
> Andy, Fabio,
> 
> Sounds like we now have a solution which makes logical sense, although it
> requires changes and additions to drivers/clk/imx/. Before I create a patch,
> can you read the plan below and check that it makes sense, please?
> 
> Problem
> =======
> On the imx6q plus, the NXP hw designers introduced an alternative way to
> clock the built-in ethernet (FEC). One single customer (archronix) seems to be
> currently using this functionality, and would like to add mainline support.
> 
> Changing the ethernet (FEC) driver appears illogical, as this change is
> unrelated to the FEC itself: fundamentally, it's a clock tree change.
> 
> We also need to prevent breaking existing boards with mis-configured FEC
> clocking:
> 
> Some board designers indicate in the devicetree that ENET_REF_CLK is
> connected to the FEC ptp clock, but in reality, they are providing an unrelated
> external clock through the pad. This will work in many cases, because the FEC
> driver does not really care where its PTP clock comes from, and the enet ref
> clock is set up to mirror the external clk frequency by the NXP U-Boot fork.
> 
> Proposed changes must not break these cases.
> 
> Proposed Solution
> =================
> On non-plus imx6q there are no changes. On imx6q plus however:
> 
> Create two new clocks (mac_gtx and pad) and a new clock mux:
> 
>   enet_ref-o--------->pad----->| \
>            |                   |  |
>            |                   |M1|----mac_gtx --- to FEC
>            |                   |  |
>            o-------------------|_/
> 
> The defaults (indicated with ">") are carefully chosen, so these changes will
> not break any existing plus designs.
> 
> We then tie the gtx clock to the FEC driver ptp clock, like so:
> 
> in imx6qp.dtsi:
> &fec {
>     clocks = <&clks IMX6QDL_CLK_ENET>,
>         <&clks IMX6QDL_CLK_ENET>,
>         <&clks IMX6QDL_CLK_MAC_GTX>;
> };
> 
> Mux M1 is controlled by GPR5[9]. This mux can just write to the GPR5
> memory address. However, the GPR registers are already exposed as a mux
> controller compatible = "mmio-mux". This mux does currently not use GPR5.
> 
> So we have to make a choice here: write straight to the memory address
> (easy), or create a new clock mux type, which uses an underlying mux
> controller.
> 
> When that is done, board designers can choose between:
> 
> 1. internal clocking (GTX clock routed internally)
> 
> &fec {
>     assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
>     assigned-clock-parents = <&clks IMX6QDL_CLK_ENET_REF>; };

The one is our requirement that gtx is from internal pll and only for
6qp boards. It is required to set in board dts file due to depends on board
design.

Here also need to assign enet_ref clk rate to 125Mhz.
> 
> 2. external clocking (GTX clock from pad, pad connected to enet_ref,
>    typically via GPIO_16). This is the default and does not need to be
> present.
> 
> &fec {
>     assigned-clocks = <&clks IMX6QDL_CLK_MAC_GTX>;
>     assigned-clock-parents = <&clks IMX6QDL_CLK_PAD>; };
> 
As above, here also need to assign the enet_ref clk rate to 125Mhz.
The clk tree:
Pll6 -> enet_ref -> clk_pad -> mac_gtx
Pll6 -> enet_ref -> clk_pad -> route back to supply clk for ptp

> 
> 3. external clocking (GTX clock from pad, pad connected to external PHY
>    clock or external oscillator).
> 
> phy_osc: oscillator {
> #clock-cells = <0>;
> compatible = "fixed-clock";
> clock-frequency = <50000000>;
> };
> 
> &fec {
>     clocks = <&clks IMX6QDL_CLK_ENET>,
>         <&clks IMX6QDL_CLK_ENET>,
>         <&phy_osc>;
> };

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02 17:53 [PATCH v5 1/3] ARM: imx: mach-imx6q: Search for fsl,imx6q-iomuxc-gpr earlier Sven Van Asbroeck
2020-07-02 17:53 ` [PATCH v5 2/3] dt-bindings: fec: add fsl,ptpclk-bypass-pad boolean property Sven Van Asbroeck
2020-07-02 17:53 ` [PATCH v5 3/3] ARM: imx6plus: optionally enable internal routing of clk_enet_ref Sven Van Asbroeck
2020-07-02 22:29   ` Fabio Estevam
2020-07-03  0:50     ` Sven Van Asbroeck
2020-07-03  2:01       ` [EXT] " Andy Duan
2020-07-04 14:08     ` Sven Van Asbroeck
2020-07-05 14:45       ` [EXT] " Andy Duan
2020-07-05 15:34         ` Sven Van Asbroeck
2020-07-06  5:30           ` Andy Duan
2020-07-06 13:46             ` Fabio Estevam
2020-07-06 14:58               ` Sven Van Asbroeck
2020-07-06 14:59                 ` Sven Van Asbroeck
2020-07-07  3:38                   ` Andy Duan
2020-07-07 15:21                     ` Sven Van Asbroeck
2020-07-08  5:16                       ` Andy Duan
2020-07-06 14:53             ` Sven Van Asbroeck

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git