| Message ID | 20200702175352.19223-3-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/3] ARM: imx: mach-imx6q: Search for fsl, imx6q-iomuxc-gpr earlier | expand |
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?
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?
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.
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.
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.
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?
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.
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?
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> ?
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 ?
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?
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.
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>;
};
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>; > };
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)
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(+)