All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-08 18:57 ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

The implementation of ETH_RX_CLK/ETH_REF_CLK handling on STM32MP1 currently
does not permit selecting the RX clock input from SoC pad, the RX clock are
hard-wired to eth_clk_fb. This cover letter describes multiple unsuccessful
attempts at solving this problem and a proposed partial solution.

The problem is that in stm32mp151.dtsi, the "st,stm32mp1-dwmac" compatible
ethernet requests mac-clk-rx = <&rcc ETHRX> as MAC RX clock, which per [1]
page 576 maps to clk_rx_i signal. Upon closer inspection of clk-stm32mp1.c,
the ETHRX clock 1) only models G_ETHRX gate, which represents ETHRXEN bit,
and 2) sets ETHRX clock parent to ck_axi. Both are wrong for the following
reasons:
1) ETHRX clock are composite clock of gate and two muxes, which ultimately
   permit selecting ETHRX clock parent to be either eth_clk_fb or external
   ETH_RX_CLK/ETH_REF_CLOCK clock from SoC pad. Because only the gate part
   is modeled, the clock topology is wrong and it is not possible to select
   the ETHRX clock parent using the clock framework at all.
2) ETHRX clock are supplied from a mux controlled by ETH_SEL[2] (from
   SYSCFG), which is supplied from a mux controlled by ETH_REF_CLK_SEL
   (from SYSCFG), which is supplied either by ETH_RX_CLK/ETH_REF_CLK
   from pad OR internally from eth_clk_fb clock, which maps to ETHCK_K
   clock. Hence, ETHRX can never be supplied by ck_axi.

Another unique feature of ETHRX clock muxes is that they are controlled
via SYSCFG block instead of being part of the RCC block. This is a rather
unfortunate design, as it creates dependency between RCC, SYSCFG and DWMAC.
Currently, those clock muxes are configured in the dwmac4 driver through
syscon interface to SYSCFG block, based on custom device tree properties --
st,eth-clk-sel and st,eth-ref-clk-sel -- this is clearly not the correct
approach.

First approach was to implement ETHRX clock including muxes in clk-stm32mp1.c.
This means RCC clock driver, clk-stm32mp1.c, must obtain access to SYSCFG IP
registers via some syscon_regmap_lookup_by_phandle() when registering ETHRX
clock in RCC driver probe, so that it can read out SYSCFG register state to
determine the current mux state, and later toggle the mux bits. The problem
here is that the SYSCFG requires clock which are provided by RCC, so
syscon_regmap_lookup_by_phandle() fails with EPROBE_DEFER because the clock
are not available yet in RCC clock driver probe, so there is cyclic dependency
between RCC and SYSCFG.

Similar approach would be to turn SYSCFG into simple-bus and implement
separate SYSCFG clock sub-driver, which would register single clock mux with
two parents -- ETHCK_K and ETH_RX_CLK -- and would only control the SYSCFG
clock mux bits. The problem here is similar, for RCC driver to register the
ETHRX clock, the ETHRX clock must have a parent clock, however the parent
clock are not ready yet, because this new SYSCFG clock sub-driver requires
SYSCFG register clock, which only become available once RCC finishes probing,
so there again is a cyclic dependency between RCC and SYSCFG.

One possible solution could be to defer obtaining the SYSCFG regmap handle
until later point, i.e. only once a register access to SYSCFG is required
the first time. This could permit us to register the mux in the RCC driver
including a mux which requires SYSCFG access via syscon, although this would
likely still run into problems during probe, since the clock framework would
call .get_parent() during mux registration and trigger the SYSCFG access.

All the above still only discusses the clock part of the problem. Even if
the clock cyclic dependencies could be solved, it would be necessary to
resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
and avoid DT ABI break.

So in the end, this series takes a different approach and only partly solves
the problem. This series splits ETHCK_K, so the ETHCK_K gate could be turned
off and adds support for selecting ETHRX clock parent clock via DT. This then
permits e.g. DHCOM to select MCO2 as ETHRX clock source and the clock tree is
then correctly set up.

[1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
    Figure 83. Peripheral clock distribution for Ethernet
    https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Marek Vasut (7):
  clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  clk: stm32mp1: The dev is always NULL, replace it with np
  clk: stm32mp1: Register clock with device_node pointer
  clk: stm32mp1: Add parent_data to ETHRX clock
  ARM: dts: stm32: Add alternate pinmux for ethernet0 pins
  ARM: dts: stm32: Add alternate pinmux for mco2 pins
  ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM

 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi     |  49 +++++++
 arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi |  20 ++-
 drivers/clk/clk-stm32mp1.c                   | 134 +++++++++++--------
 3 files changed, 146 insertions(+), 57 deletions(-)

Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org

-- 
2.30.2


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

* [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-08 18:57 ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

The implementation of ETH_RX_CLK/ETH_REF_CLK handling on STM32MP1 currently
does not permit selecting the RX clock input from SoC pad, the RX clock are
hard-wired to eth_clk_fb. This cover letter describes multiple unsuccessful
attempts at solving this problem and a proposed partial solution.

The problem is that in stm32mp151.dtsi, the "st,stm32mp1-dwmac" compatible
ethernet requests mac-clk-rx = <&rcc ETHRX> as MAC RX clock, which per [1]
page 576 maps to clk_rx_i signal. Upon closer inspection of clk-stm32mp1.c,
the ETHRX clock 1) only models G_ETHRX gate, which represents ETHRXEN bit,
and 2) sets ETHRX clock parent to ck_axi. Both are wrong for the following
reasons:
1) ETHRX clock are composite clock of gate and two muxes, which ultimately
   permit selecting ETHRX clock parent to be either eth_clk_fb or external
   ETH_RX_CLK/ETH_REF_CLOCK clock from SoC pad. Because only the gate part
   is modeled, the clock topology is wrong and it is not possible to select
   the ETHRX clock parent using the clock framework at all.
2) ETHRX clock are supplied from a mux controlled by ETH_SEL[2] (from
   SYSCFG), which is supplied from a mux controlled by ETH_REF_CLK_SEL
   (from SYSCFG), which is supplied either by ETH_RX_CLK/ETH_REF_CLK
   from pad OR internally from eth_clk_fb clock, which maps to ETHCK_K
   clock. Hence, ETHRX can never be supplied by ck_axi.

Another unique feature of ETHRX clock muxes is that they are controlled
via SYSCFG block instead of being part of the RCC block. This is a rather
unfortunate design, as it creates dependency between RCC, SYSCFG and DWMAC.
Currently, those clock muxes are configured in the dwmac4 driver through
syscon interface to SYSCFG block, based on custom device tree properties --
st,eth-clk-sel and st,eth-ref-clk-sel -- this is clearly not the correct
approach.

First approach was to implement ETHRX clock including muxes in clk-stm32mp1.c.
This means RCC clock driver, clk-stm32mp1.c, must obtain access to SYSCFG IP
registers via some syscon_regmap_lookup_by_phandle() when registering ETHRX
clock in RCC driver probe, so that it can read out SYSCFG register state to
determine the current mux state, and later toggle the mux bits. The problem
here is that the SYSCFG requires clock which are provided by RCC, so
syscon_regmap_lookup_by_phandle() fails with EPROBE_DEFER because the clock
are not available yet in RCC clock driver probe, so there is cyclic dependency
between RCC and SYSCFG.

Similar approach would be to turn SYSCFG into simple-bus and implement
separate SYSCFG clock sub-driver, which would register single clock mux with
two parents -- ETHCK_K and ETH_RX_CLK -- and would only control the SYSCFG
clock mux bits. The problem here is similar, for RCC driver to register the
ETHRX clock, the ETHRX clock must have a parent clock, however the parent
clock are not ready yet, because this new SYSCFG clock sub-driver requires
SYSCFG register clock, which only become available once RCC finishes probing,
so there again is a cyclic dependency between RCC and SYSCFG.

One possible solution could be to defer obtaining the SYSCFG regmap handle
until later point, i.e. only once a register access to SYSCFG is required
the first time. This could permit us to register the mux in the RCC driver
including a mux which requires SYSCFG access via syscon, although this would
likely still run into problems during probe, since the clock framework would
call .get_parent() during mux registration and trigger the SYSCFG access.

All the above still only discusses the clock part of the problem. Even if
the clock cyclic dependencies could be solved, it would be necessary to
resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
and avoid DT ABI break.

So in the end, this series takes a different approach and only partly solves
the problem. This series splits ETHCK_K, so the ETHCK_K gate could be turned
off and adds support for selecting ETHRX clock parent clock via DT. This then
permits e.g. DHCOM to select MCO2 as ETHRX clock source and the clock tree is
then correctly set up.

[1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
    Figure 83. Peripheral clock distribution for Ethernet
    https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Marek Vasut (7):
  clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  clk: stm32mp1: The dev is always NULL, replace it with np
  clk: stm32mp1: Register clock with device_node pointer
  clk: stm32mp1: Add parent_data to ETHRX clock
  ARM: dts: stm32: Add alternate pinmux for ethernet0 pins
  ARM: dts: stm32: Add alternate pinmux for mco2 pins
  ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM

 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi     |  49 +++++++
 arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi |  20 ++-
 drivers/clk/clk-stm32mp1.c                   | 134 +++++++++++--------
 3 files changed, 146 insertions(+), 57 deletions(-)

Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org

-- 
2.30.2


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

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

* [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

The ETHCK_K are modeled as composite clock of MUX and GATE, however per
STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
clock distribution for Ethernet, ETHPTPDIV divider is attached past the
ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
in use, ETHCKEN gate can be turned off. Current driver does not permit
that, fix it.

This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
ETHPTP_K remain functional as before.

[1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
    Figure 83. Peripheral clock distribution for Ethernet
    https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 drivers/clk/clk-stm32mp1.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index a875649df8b8..a7c7f544ee5d 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -1949,7 +1949,6 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
 	KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
 	KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
-	KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
 
 	/* Particulary Kernel Clocks (no mux or no gate) */
 	MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
@@ -1958,11 +1957,16 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
 	MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
 
-	COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
+	COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
 		  CLK_SET_RATE_NO_REPARENT,
 		  _NO_GATE,
 		  _MMUX(M_ETHCK),
-		  _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
+		  _NO_DIV),
+
+	MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
+
+	DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
+	    CLK_SET_RATE_NO_REPARENT, RCC_ETHCKSELR, 4, 4, 0),
 
 	/* RTC clock */
 	DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0),
-- 
2.30.2


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

* [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

The ETHCK_K are modeled as composite clock of MUX and GATE, however per
STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
clock distribution for Ethernet, ETHPTPDIV divider is attached past the
ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
in use, ETHCKEN gate can be turned off. Current driver does not permit
that, fix it.

This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
ETHPTP_K remain functional as before.

[1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
    Figure 83. Peripheral clock distribution for Ethernet
    https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 drivers/clk/clk-stm32mp1.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index a875649df8b8..a7c7f544ee5d 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -1949,7 +1949,6 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
 	KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
 	KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
-	KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
 
 	/* Particulary Kernel Clocks (no mux or no gate) */
 	MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
@@ -1958,11 +1957,16 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
 	MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
 
-	COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
+	COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
 		  CLK_SET_RATE_NO_REPARENT,
 		  _NO_GATE,
 		  _MMUX(M_ETHCK),
-		  _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
+		  _NO_DIV),
+
+	MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
+
+	DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
+	    CLK_SET_RATE_NO_REPARENT, RCC_ETHCKSELR, 4, 4, 0),
 
 	/* RTC clock */
 	DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0),
-- 
2.30.2


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

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

* [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Instead of passing around $dev to all the registration functions, which
is always NULL, pass around struct device_node pointer $np. This way it
is possible to use of_clk_hw_register*() functions and/or register clock
with associated $np pointer.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 drivers/clk/clk-stm32mp1.c | 56 +++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index a7c7f544ee5d..cf5a1d055c5a 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -317,7 +317,7 @@ struct clock_config {
 	int num_parents;
 	unsigned long flags;
 	void *cfg;
-	struct clk_hw * (*func)(struct device *dev,
+	struct clk_hw * (*func)(struct device_node *np,
 				struct clk_hw_onecell_data *clk_data,
 				void __iomem *base, spinlock_t *lock,
 				const struct clock_config *cfg);
@@ -377,14 +377,14 @@ struct stm32_composite_cfg {
 };
 
 static struct clk_hw *
-_clk_hw_register_gate(struct device *dev,
+_clk_hw_register_gate(struct device_node *np,
 		      struct clk_hw_onecell_data *clk_data,
 		      void __iomem *base, spinlock_t *lock,
 		      const struct clock_config *cfg)
 {
 	struct gate_cfg *gate_cfg = cfg->cfg;
 
-	return clk_hw_register_gate(dev,
+	return clk_hw_register_gate(NULL,
 				    cfg->name,
 				    cfg->parent_name,
 				    cfg->flags,
@@ -395,27 +395,27 @@ _clk_hw_register_gate(struct device *dev,
 }
 
 static struct clk_hw *
-_clk_hw_register_fixed_factor(struct device *dev,
+_clk_hw_register_fixed_factor(struct device_node *np,
 			      struct clk_hw_onecell_data *clk_data,
 			      void __iomem *base, spinlock_t *lock,
 			      const struct clock_config *cfg)
 {
 	struct fixed_factor_cfg *ff_cfg = cfg->cfg;
 
-	return clk_hw_register_fixed_factor(dev, cfg->name, cfg->parent_name,
+	return clk_hw_register_fixed_factor(NULL, cfg->name, cfg->parent_name,
 					    cfg->flags, ff_cfg->mult,
 					    ff_cfg->div);
 }
 
 static struct clk_hw *
-_clk_hw_register_divider_table(struct device *dev,
+_clk_hw_register_divider_table(struct device_node *np,
 			       struct clk_hw_onecell_data *clk_data,
 			       void __iomem *base, spinlock_t *lock,
 			       const struct clock_config *cfg)
 {
 	struct div_cfg *div_cfg = cfg->cfg;
 
-	return clk_hw_register_divider_table(dev,
+	return clk_hw_register_divider_table(NULL,
 					     cfg->name,
 					     cfg->parent_name,
 					     cfg->flags,
@@ -428,14 +428,14 @@ _clk_hw_register_divider_table(struct device *dev,
 }
 
 static struct clk_hw *
-_clk_hw_register_mux(struct device *dev,
+_clk_hw_register_mux(struct device_node *np,
 		     struct clk_hw_onecell_data *clk_data,
 		     void __iomem *base, spinlock_t *lock,
 		     const struct clock_config *cfg)
 {
 	struct mux_cfg *mux_cfg = cfg->cfg;
 
-	return clk_hw_register_mux(dev, cfg->name, cfg->parent_names,
+	return clk_hw_register_mux(NULL, cfg->name, cfg->parent_names,
 				   cfg->num_parents, cfg->flags,
 				   mux_cfg->reg_off + base, mux_cfg->shift,
 				   mux_cfg->width, mux_cfg->mux_flags, lock);
@@ -570,7 +570,7 @@ _get_stm32_gate(void __iomem *base,
 }
 
 static struct clk_hw *
-clk_stm32_register_gate_ops(struct device *dev,
+clk_stm32_register_gate_ops(struct device_node *np,
 			    const char *name,
 			    const char *parent_name,
 			    unsigned long flags,
@@ -598,7 +598,7 @@ clk_stm32_register_gate_ops(struct device *dev,
 
 	hw->init = &init;
 
-	ret = clk_hw_register(dev, hw);
+	ret = clk_hw_register(NULL, hw);
 	if (ret)
 		hw = ERR_PTR(ret);
 
@@ -606,7 +606,7 @@ clk_stm32_register_gate_ops(struct device *dev,
 }
 
 static struct clk_hw *
-clk_stm32_register_composite(struct device *dev,
+clk_stm32_register_composite(struct device_node *np,
 			     const char *name, const char * const *parent_names,
 			     int num_parents, void __iomem *base,
 			     const struct stm32_composite_cfg *cfg,
@@ -655,7 +655,7 @@ clk_stm32_register_composite(struct device *dev,
 		}
 	}
 
-	return clk_hw_register_composite(dev, name, parent_names, num_parents,
+	return clk_hw_register_composite(NULL, name, parent_names, num_parents,
 				       mux_hw, mux_ops, div_hw, div_ops,
 				       gate_hw, gate_ops, flags);
 }
@@ -863,7 +863,7 @@ static const struct clk_ops pll_ops = {
 	.is_enabled	= pll_is_enabled,
 };
 
-static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
+static struct clk_hw *clk_register_pll(struct device_node *np, const char *name,
 				       const char *parent_name,
 				       void __iomem *reg,
 				       unsigned long flags,
@@ -889,7 +889,7 @@ static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
 	element->lock = lock;
 
 	hw = &element->hw;
-	err = clk_hw_register(dev, hw);
+	err = clk_hw_register(NULL, hw);
 
 	if (err) {
 		kfree(element);
@@ -993,7 +993,7 @@ static const struct clk_ops timer_ker_ops = {
 
 };
 
-static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
+static struct clk_hw *clk_register_cktim(struct device_node *np, const char *name,
 					 const char *parent_name,
 					 unsigned long flags,
 					 void __iomem *apbdiv,
@@ -1021,7 +1021,7 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
 	tim_ker->timpre = timpre;
 
 	hw = &tim_ker->hw;
-	err = clk_hw_register(dev, hw);
+	err = clk_hw_register(NULL, hw);
 
 	if (err) {
 		kfree(tim_ker);
@@ -1035,14 +1035,14 @@ struct stm32_pll_cfg {
 	u32 offset;
 };
 
-static struct clk_hw *_clk_register_pll(struct device *dev,
+static struct clk_hw *_clk_register_pll(struct device_node *np,
 					struct clk_hw_onecell_data *clk_data,
 					void __iomem *base, spinlock_t *lock,
 					const struct clock_config *cfg)
 {
 	struct stm32_pll_cfg *stm_pll_cfg = cfg->cfg;
 
-	return clk_register_pll(dev, cfg->name, cfg->parent_name,
+	return clk_register_pll(np, cfg->name, cfg->parent_name,
 				base + stm_pll_cfg->offset, cfg->flags, lock);
 }
 
@@ -1051,25 +1051,25 @@ struct stm32_cktim_cfg {
 	u32 offset_timpre;
 };
 
-static struct clk_hw *_clk_register_cktim(struct device *dev,
+static struct clk_hw *_clk_register_cktim(struct device_node *np,
 					  struct clk_hw_onecell_data *clk_data,
 					  void __iomem *base, spinlock_t *lock,
 					  const struct clock_config *cfg)
 {
 	struct stm32_cktim_cfg *cktim_cfg = cfg->cfg;
 
-	return clk_register_cktim(dev, cfg->name, cfg->parent_name, cfg->flags,
+	return clk_register_cktim(np, cfg->name, cfg->parent_name, cfg->flags,
 				  cktim_cfg->offset_apbdiv + base,
 				  cktim_cfg->offset_timpre + base, lock);
 }
 
 static struct clk_hw *
-_clk_stm32_register_gate(struct device *dev,
+_clk_stm32_register_gate(struct device_node *np,
 			 struct clk_hw_onecell_data *clk_data,
 			 void __iomem *base, spinlock_t *lock,
 			 const struct clock_config *cfg)
 {
-	return clk_stm32_register_gate_ops(dev,
+	return clk_stm32_register_gate_ops(np,
 				    cfg->name,
 				    cfg->parent_name,
 				    cfg->flags,
@@ -1079,12 +1079,12 @@ _clk_stm32_register_gate(struct device *dev,
 }
 
 static struct clk_hw *
-_clk_stm32_register_composite(struct device *dev,
+_clk_stm32_register_composite(struct device_node *np,
 			      struct clk_hw_onecell_data *clk_data,
 			      void __iomem *base, spinlock_t *lock,
 			      const struct clock_config *cfg)
 {
-	return clk_stm32_register_composite(dev, cfg->name, cfg->parent_names,
+	return clk_stm32_register_composite(np, cfg->name, cfg->parent_names,
 					    cfg->num_parents, base, cfg->cfg,
 					    cfg->flags, lock);
 }
@@ -2020,7 +2020,7 @@ static const struct of_device_id stm32mp1_match_data[] = {
 	{ }
 };
 
-static int stm32_register_hw_clk(struct device *dev,
+static int stm32_register_hw_clk(struct device_node *np,
 				 struct clk_hw_onecell_data *clk_data,
 				 void __iomem *base, spinlock_t *lock,
 				 const struct clock_config *cfg)
@@ -2031,7 +2031,7 @@ static int stm32_register_hw_clk(struct device *dev,
 	hws = clk_data->hws;
 
 	if (cfg->func)
-		hw = (*cfg->func)(dev, clk_data, base, lock, cfg);
+		hw = (*cfg->func)(np, clk_data, base, lock, cfg);
 
 	if (IS_ERR(hw)) {
 		pr_err("Unable to register %s\n", cfg->name);
@@ -2077,7 +2077,7 @@ static int stm32_rcc_init(struct device_node *np,
 		hws[n] = ERR_PTR(-ENOENT);
 
 	for (n = 0; n < data->num; n++) {
-		err = stm32_register_hw_clk(NULL, clk_data, base, &rlock,
+		err = stm32_register_hw_clk(np, clk_data, base, &rlock,
 					    &data->cfg[n]);
 		if (err) {
 			pr_err("%s: can't register  %s\n", __func__,
-- 
2.30.2


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

* [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Instead of passing around $dev to all the registration functions, which
is always NULL, pass around struct device_node pointer $np. This way it
is possible to use of_clk_hw_register*() functions and/or register clock
with associated $np pointer.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 drivers/clk/clk-stm32mp1.c | 56 +++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index a7c7f544ee5d..cf5a1d055c5a 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -317,7 +317,7 @@ struct clock_config {
 	int num_parents;
 	unsigned long flags;
 	void *cfg;
-	struct clk_hw * (*func)(struct device *dev,
+	struct clk_hw * (*func)(struct device_node *np,
 				struct clk_hw_onecell_data *clk_data,
 				void __iomem *base, spinlock_t *lock,
 				const struct clock_config *cfg);
@@ -377,14 +377,14 @@ struct stm32_composite_cfg {
 };
 
 static struct clk_hw *
-_clk_hw_register_gate(struct device *dev,
+_clk_hw_register_gate(struct device_node *np,
 		      struct clk_hw_onecell_data *clk_data,
 		      void __iomem *base, spinlock_t *lock,
 		      const struct clock_config *cfg)
 {
 	struct gate_cfg *gate_cfg = cfg->cfg;
 
-	return clk_hw_register_gate(dev,
+	return clk_hw_register_gate(NULL,
 				    cfg->name,
 				    cfg->parent_name,
 				    cfg->flags,
@@ -395,27 +395,27 @@ _clk_hw_register_gate(struct device *dev,
 }
 
 static struct clk_hw *
-_clk_hw_register_fixed_factor(struct device *dev,
+_clk_hw_register_fixed_factor(struct device_node *np,
 			      struct clk_hw_onecell_data *clk_data,
 			      void __iomem *base, spinlock_t *lock,
 			      const struct clock_config *cfg)
 {
 	struct fixed_factor_cfg *ff_cfg = cfg->cfg;
 
-	return clk_hw_register_fixed_factor(dev, cfg->name, cfg->parent_name,
+	return clk_hw_register_fixed_factor(NULL, cfg->name, cfg->parent_name,
 					    cfg->flags, ff_cfg->mult,
 					    ff_cfg->div);
 }
 
 static struct clk_hw *
-_clk_hw_register_divider_table(struct device *dev,
+_clk_hw_register_divider_table(struct device_node *np,
 			       struct clk_hw_onecell_data *clk_data,
 			       void __iomem *base, spinlock_t *lock,
 			       const struct clock_config *cfg)
 {
 	struct div_cfg *div_cfg = cfg->cfg;
 
-	return clk_hw_register_divider_table(dev,
+	return clk_hw_register_divider_table(NULL,
 					     cfg->name,
 					     cfg->parent_name,
 					     cfg->flags,
@@ -428,14 +428,14 @@ _clk_hw_register_divider_table(struct device *dev,
 }
 
 static struct clk_hw *
-_clk_hw_register_mux(struct device *dev,
+_clk_hw_register_mux(struct device_node *np,
 		     struct clk_hw_onecell_data *clk_data,
 		     void __iomem *base, spinlock_t *lock,
 		     const struct clock_config *cfg)
 {
 	struct mux_cfg *mux_cfg = cfg->cfg;
 
-	return clk_hw_register_mux(dev, cfg->name, cfg->parent_names,
+	return clk_hw_register_mux(NULL, cfg->name, cfg->parent_names,
 				   cfg->num_parents, cfg->flags,
 				   mux_cfg->reg_off + base, mux_cfg->shift,
 				   mux_cfg->width, mux_cfg->mux_flags, lock);
@@ -570,7 +570,7 @@ _get_stm32_gate(void __iomem *base,
 }
 
 static struct clk_hw *
-clk_stm32_register_gate_ops(struct device *dev,
+clk_stm32_register_gate_ops(struct device_node *np,
 			    const char *name,
 			    const char *parent_name,
 			    unsigned long flags,
@@ -598,7 +598,7 @@ clk_stm32_register_gate_ops(struct device *dev,
 
 	hw->init = &init;
 
-	ret = clk_hw_register(dev, hw);
+	ret = clk_hw_register(NULL, hw);
 	if (ret)
 		hw = ERR_PTR(ret);
 
@@ -606,7 +606,7 @@ clk_stm32_register_gate_ops(struct device *dev,
 }
 
 static struct clk_hw *
-clk_stm32_register_composite(struct device *dev,
+clk_stm32_register_composite(struct device_node *np,
 			     const char *name, const char * const *parent_names,
 			     int num_parents, void __iomem *base,
 			     const struct stm32_composite_cfg *cfg,
@@ -655,7 +655,7 @@ clk_stm32_register_composite(struct device *dev,
 		}
 	}
 
-	return clk_hw_register_composite(dev, name, parent_names, num_parents,
+	return clk_hw_register_composite(NULL, name, parent_names, num_parents,
 				       mux_hw, mux_ops, div_hw, div_ops,
 				       gate_hw, gate_ops, flags);
 }
@@ -863,7 +863,7 @@ static const struct clk_ops pll_ops = {
 	.is_enabled	= pll_is_enabled,
 };
 
-static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
+static struct clk_hw *clk_register_pll(struct device_node *np, const char *name,
 				       const char *parent_name,
 				       void __iomem *reg,
 				       unsigned long flags,
@@ -889,7 +889,7 @@ static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
 	element->lock = lock;
 
 	hw = &element->hw;
-	err = clk_hw_register(dev, hw);
+	err = clk_hw_register(NULL, hw);
 
 	if (err) {
 		kfree(element);
@@ -993,7 +993,7 @@ static const struct clk_ops timer_ker_ops = {
 
 };
 
-static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
+static struct clk_hw *clk_register_cktim(struct device_node *np, const char *name,
 					 const char *parent_name,
 					 unsigned long flags,
 					 void __iomem *apbdiv,
@@ -1021,7 +1021,7 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
 	tim_ker->timpre = timpre;
 
 	hw = &tim_ker->hw;
-	err = clk_hw_register(dev, hw);
+	err = clk_hw_register(NULL, hw);
 
 	if (err) {
 		kfree(tim_ker);
@@ -1035,14 +1035,14 @@ struct stm32_pll_cfg {
 	u32 offset;
 };
 
-static struct clk_hw *_clk_register_pll(struct device *dev,
+static struct clk_hw *_clk_register_pll(struct device_node *np,
 					struct clk_hw_onecell_data *clk_data,
 					void __iomem *base, spinlock_t *lock,
 					const struct clock_config *cfg)
 {
 	struct stm32_pll_cfg *stm_pll_cfg = cfg->cfg;
 
-	return clk_register_pll(dev, cfg->name, cfg->parent_name,
+	return clk_register_pll(np, cfg->name, cfg->parent_name,
 				base + stm_pll_cfg->offset, cfg->flags, lock);
 }
 
@@ -1051,25 +1051,25 @@ struct stm32_cktim_cfg {
 	u32 offset_timpre;
 };
 
-static struct clk_hw *_clk_register_cktim(struct device *dev,
+static struct clk_hw *_clk_register_cktim(struct device_node *np,
 					  struct clk_hw_onecell_data *clk_data,
 					  void __iomem *base, spinlock_t *lock,
 					  const struct clock_config *cfg)
 {
 	struct stm32_cktim_cfg *cktim_cfg = cfg->cfg;
 
-	return clk_register_cktim(dev, cfg->name, cfg->parent_name, cfg->flags,
+	return clk_register_cktim(np, cfg->name, cfg->parent_name, cfg->flags,
 				  cktim_cfg->offset_apbdiv + base,
 				  cktim_cfg->offset_timpre + base, lock);
 }
 
 static struct clk_hw *
-_clk_stm32_register_gate(struct device *dev,
+_clk_stm32_register_gate(struct device_node *np,
 			 struct clk_hw_onecell_data *clk_data,
 			 void __iomem *base, spinlock_t *lock,
 			 const struct clock_config *cfg)
 {
-	return clk_stm32_register_gate_ops(dev,
+	return clk_stm32_register_gate_ops(np,
 				    cfg->name,
 				    cfg->parent_name,
 				    cfg->flags,
@@ -1079,12 +1079,12 @@ _clk_stm32_register_gate(struct device *dev,
 }
 
 static struct clk_hw *
-_clk_stm32_register_composite(struct device *dev,
+_clk_stm32_register_composite(struct device_node *np,
 			      struct clk_hw_onecell_data *clk_data,
 			      void __iomem *base, spinlock_t *lock,
 			      const struct clock_config *cfg)
 {
-	return clk_stm32_register_composite(dev, cfg->name, cfg->parent_names,
+	return clk_stm32_register_composite(np, cfg->name, cfg->parent_names,
 					    cfg->num_parents, base, cfg->cfg,
 					    cfg->flags, lock);
 }
@@ -2020,7 +2020,7 @@ static const struct of_device_id stm32mp1_match_data[] = {
 	{ }
 };
 
-static int stm32_register_hw_clk(struct device *dev,
+static int stm32_register_hw_clk(struct device_node *np,
 				 struct clk_hw_onecell_data *clk_data,
 				 void __iomem *base, spinlock_t *lock,
 				 const struct clock_config *cfg)
@@ -2031,7 +2031,7 @@ static int stm32_register_hw_clk(struct device *dev,
 	hws = clk_data->hws;
 
 	if (cfg->func)
-		hw = (*cfg->func)(dev, clk_data, base, lock, cfg);
+		hw = (*cfg->func)(np, clk_data, base, lock, cfg);
 
 	if (IS_ERR(hw)) {
 		pr_err("Unable to register %s\n", cfg->name);
@@ -2077,7 +2077,7 @@ static int stm32_rcc_init(struct device_node *np,
 		hws[n] = ERR_PTR(-ENOENT);
 
 	for (n = 0; n < data->num; n++) {
-		err = stm32_register_hw_clk(NULL, clk_data, base, &rlock,
+		err = stm32_register_hw_clk(np, clk_data, base, &rlock,
 					    &data->cfg[n]);
 		if (err) {
 			pr_err("%s: can't register  %s\n", __func__,
-- 
2.30.2


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

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

* [PATCH 3/7] clk: stm32mp1: Register clock with device_node pointer
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Use of_clk_hw_register() where applicable to associate device_node with
the newly registered clock, elsewhere use functions which permit passing
the device node to newly registered clock.

There are two exceptions, _clk_hw_register_fixed_factor() does not pass
the device_node pointer to new fixed factor clock and neither does
clk_stm32_register_composite(), because there is so far no way to do
that.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
NOTE: But if this patch is acceptable, the _clk_hw_register_fixed_factor()
      and clk_stm32_register_composite() can be easily fixed up too.
---
 drivers/clk/clk-stm32mp1.c | 44 ++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index cf5a1d055c5a..85bba1ee5fbd 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -384,14 +384,11 @@ _clk_hw_register_gate(struct device_node *np,
 {
 	struct gate_cfg *gate_cfg = cfg->cfg;
 
-	return clk_hw_register_gate(NULL,
-				    cfg->name,
-				    cfg->parent_name,
-				    cfg->flags,
-				    gate_cfg->reg_off + base,
-				    gate_cfg->bit_idx,
-				    gate_cfg->gate_flags,
-				    lock);
+	return __clk_hw_register_gate(NULL, np, cfg->name, cfg->parent_name,
+				      NULL, NULL, cfg->flags,
+				      gate_cfg->reg_off + base,
+				      gate_cfg->bit_idx,
+				      gate_cfg->gate_flags, lock);
 }
 
 static struct clk_hw *
@@ -415,16 +412,12 @@ _clk_hw_register_divider_table(struct device_node *np,
 {
 	struct div_cfg *div_cfg = cfg->cfg;
 
-	return clk_hw_register_divider_table(NULL,
-					     cfg->name,
-					     cfg->parent_name,
-					     cfg->flags,
-					     div_cfg->reg_off + base,
-					     div_cfg->shift,
-					     div_cfg->width,
-					     div_cfg->div_flags,
-					     div_cfg->table,
-					     lock);
+	return __clk_hw_register_divider(NULL, np, cfg->name, cfg->parent_name,
+					 NULL, NULL, cfg->flags,
+					 div_cfg->reg_off + base,
+					 div_cfg->shift, div_cfg->width,
+					 div_cfg->div_flags, div_cfg->table,
+					 lock);
 }
 
 static struct clk_hw *
@@ -435,10 +428,11 @@ _clk_hw_register_mux(struct device_node *np,
 {
 	struct mux_cfg *mux_cfg = cfg->cfg;
 
-	return clk_hw_register_mux(NULL, cfg->name, cfg->parent_names,
-				   cfg->num_parents, cfg->flags,
-				   mux_cfg->reg_off + base, mux_cfg->shift,
-				   mux_cfg->width, mux_cfg->mux_flags, lock);
+	return __clk_hw_register_mux(NULL, np, cfg->name, cfg->num_parents,
+				     cfg->parent_names, NULL, NULL, cfg->flags,
+				     mux_cfg->reg_off + base, mux_cfg->shift,
+				     BIT(mux_cfg->width) - 1,
+				     mux_cfg->mux_flags, NULL, lock);
 }
 
 /* MP1 Gate clock with set & clear registers */
@@ -598,7 +592,7 @@ clk_stm32_register_gate_ops(struct device_node *np,
 
 	hw->init = &init;
 
-	ret = clk_hw_register(NULL, hw);
+	ret = of_clk_hw_register(np, hw);
 	if (ret)
 		hw = ERR_PTR(ret);
 
@@ -889,7 +883,7 @@ static struct clk_hw *clk_register_pll(struct device_node *np, const char *name,
 	element->lock = lock;
 
 	hw = &element->hw;
-	err = clk_hw_register(NULL, hw);
+	err = of_clk_hw_register(np, hw);
 
 	if (err) {
 		kfree(element);
@@ -1021,7 +1015,7 @@ static struct clk_hw *clk_register_cktim(struct device_node *np, const char *nam
 	tim_ker->timpre = timpre;
 
 	hw = &tim_ker->hw;
-	err = clk_hw_register(NULL, hw);
+	err = of_clk_hw_register(np, hw);
 
 	if (err) {
 		kfree(tim_ker);
-- 
2.30.2


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

* [PATCH 3/7] clk: stm32mp1: Register clock with device_node pointer
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Use of_clk_hw_register() where applicable to associate device_node with
the newly registered clock, elsewhere use functions which permit passing
the device node to newly registered clock.

There are two exceptions, _clk_hw_register_fixed_factor() does not pass
the device_node pointer to new fixed factor clock and neither does
clk_stm32_register_composite(), because there is so far no way to do
that.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
NOTE: But if this patch is acceptable, the _clk_hw_register_fixed_factor()
      and clk_stm32_register_composite() can be easily fixed up too.
---
 drivers/clk/clk-stm32mp1.c | 44 ++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index cf5a1d055c5a..85bba1ee5fbd 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -384,14 +384,11 @@ _clk_hw_register_gate(struct device_node *np,
 {
 	struct gate_cfg *gate_cfg = cfg->cfg;
 
-	return clk_hw_register_gate(NULL,
-				    cfg->name,
-				    cfg->parent_name,
-				    cfg->flags,
-				    gate_cfg->reg_off + base,
-				    gate_cfg->bit_idx,
-				    gate_cfg->gate_flags,
-				    lock);
+	return __clk_hw_register_gate(NULL, np, cfg->name, cfg->parent_name,
+				      NULL, NULL, cfg->flags,
+				      gate_cfg->reg_off + base,
+				      gate_cfg->bit_idx,
+				      gate_cfg->gate_flags, lock);
 }
 
 static struct clk_hw *
@@ -415,16 +412,12 @@ _clk_hw_register_divider_table(struct device_node *np,
 {
 	struct div_cfg *div_cfg = cfg->cfg;
 
-	return clk_hw_register_divider_table(NULL,
-					     cfg->name,
-					     cfg->parent_name,
-					     cfg->flags,
-					     div_cfg->reg_off + base,
-					     div_cfg->shift,
-					     div_cfg->width,
-					     div_cfg->div_flags,
-					     div_cfg->table,
-					     lock);
+	return __clk_hw_register_divider(NULL, np, cfg->name, cfg->parent_name,
+					 NULL, NULL, cfg->flags,
+					 div_cfg->reg_off + base,
+					 div_cfg->shift, div_cfg->width,
+					 div_cfg->div_flags, div_cfg->table,
+					 lock);
 }
 
 static struct clk_hw *
@@ -435,10 +428,11 @@ _clk_hw_register_mux(struct device_node *np,
 {
 	struct mux_cfg *mux_cfg = cfg->cfg;
 
-	return clk_hw_register_mux(NULL, cfg->name, cfg->parent_names,
-				   cfg->num_parents, cfg->flags,
-				   mux_cfg->reg_off + base, mux_cfg->shift,
-				   mux_cfg->width, mux_cfg->mux_flags, lock);
+	return __clk_hw_register_mux(NULL, np, cfg->name, cfg->num_parents,
+				     cfg->parent_names, NULL, NULL, cfg->flags,
+				     mux_cfg->reg_off + base, mux_cfg->shift,
+				     BIT(mux_cfg->width) - 1,
+				     mux_cfg->mux_flags, NULL, lock);
 }
 
 /* MP1 Gate clock with set & clear registers */
@@ -598,7 +592,7 @@ clk_stm32_register_gate_ops(struct device_node *np,
 
 	hw->init = &init;
 
-	ret = clk_hw_register(NULL, hw);
+	ret = of_clk_hw_register(np, hw);
 	if (ret)
 		hw = ERR_PTR(ret);
 
@@ -889,7 +883,7 @@ static struct clk_hw *clk_register_pll(struct device_node *np, const char *name,
 	element->lock = lock;
 
 	hw = &element->hw;
-	err = clk_hw_register(NULL, hw);
+	err = of_clk_hw_register(np, hw);
 
 	if (err) {
 		kfree(element);
@@ -1021,7 +1015,7 @@ static struct clk_hw *clk_register_cktim(struct device_node *np, const char *nam
 	tim_ker->timpre = timpre;
 
 	hw = &tim_ker->hw;
-	err = clk_hw_register(NULL, hw);
+	err = of_clk_hw_register(np, hw);
 
 	if (err) {
 		kfree(tim_ker);
-- 
2.30.2


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

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

* [PATCH 4/7] clk: stm32mp1: Add parent_data to ETHRX clock
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Pass parent_data to ETHRX clock with new fw_name = "ETH_RX_CLK/ETH_REF_CLK".
By default, this change has no impact on the operation of the clock driver.
However, due to the fw_name, it permits DT to override ETHRX clock parent,
which might be needed in case the ETHRX clock are supplied by external clock
source.

Example of MCO2 supplying clock to ETH_RX_CLK via external pad-to-pad wire:
&rcc {
         clocks = <&rcc CK_MCO2>;
         clock-names = "ETH_RX_CLK/ETH_REF_CLK";
};

Note that while this patch permits to implement this rare usecase, the issue
with ethernet RX and TX input clock modeling on MP1 is far more complex and
requires more core plumbing.

[1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
    Figure 83. Peripheral clock distribution for Ethernet
    https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 drivers/clk/clk-stm32mp1.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index 85bba1ee5fbd..f9a9960945c6 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -152,6 +152,10 @@ static const char * const eth_src[] = {
 	"pll4_p", "pll3_q"
 };
 
+const struct clk_parent_data ethrx_src[] = {
+	{ .name = "ethck_k", .fw_name = "ETH_RX_CLK/ETH_REF_CLK" },
+};
+
 static const char * const rng_src[] = {
 	"ck_csi", "pll4_r", "ck_lse", "ck_lsi"
 };
@@ -314,6 +318,7 @@ struct clock_config {
 	const char *name;
 	const char *parent_name;
 	const char * const *parent_names;
+	const struct clk_parent_data *parent_data;
 	int num_parents;
 	unsigned long flags;
 	void *cfg;
@@ -567,6 +572,7 @@ static struct clk_hw *
 clk_stm32_register_gate_ops(struct device_node *np,
 			    const char *name,
 			    const char *parent_name,
+			    const struct clk_parent_data *parent_data,
 			    unsigned long flags,
 			    void __iomem *base,
 			    const struct stm32_gate_cfg *cfg,
@@ -577,7 +583,10 @@ clk_stm32_register_gate_ops(struct device_node *np,
 	int ret;
 
 	init.name = name;
-	init.parent_names = &parent_name;
+	if (parent_name)
+		init.parent_names = &parent_name;
+	if (parent_data)
+		init.parent_data = parent_data;
 	init.num_parents = 1;
 	init.flags = flags;
 
@@ -602,6 +611,7 @@ clk_stm32_register_gate_ops(struct device_node *np,
 static struct clk_hw *
 clk_stm32_register_composite(struct device_node *np,
 			     const char *name, const char * const *parent_names,
+			     const struct clk_parent_data *parent_data,
 			     int num_parents, void __iomem *base,
 			     const struct stm32_composite_cfg *cfg,
 			     unsigned long flags, spinlock_t *lock)
@@ -1066,6 +1076,7 @@ _clk_stm32_register_gate(struct device_node *np,
 	return clk_stm32_register_gate_ops(np,
 				    cfg->name,
 				    cfg->parent_name,
+				    cfg->parent_data,
 				    cfg->flags,
 				    base,
 				    cfg->cfg,
@@ -1079,8 +1090,8 @@ _clk_stm32_register_composite(struct device_node *np,
 			      const struct clock_config *cfg)
 {
 	return clk_stm32_register_composite(np, cfg->name, cfg->parent_names,
-					    cfg->num_parents, base, cfg->cfg,
-					    cfg->flags, lock);
+					    cfg->parent_data, cfg->num_parents,
+					    base, cfg->cfg, cfg->flags, lock);
 }
 
 #define GATE(_id, _name, _parent, _flags, _offset, _bit_idx, _gate_flags)\
@@ -1187,6 +1198,16 @@ _clk_stm32_register_composite(struct device_node *np,
 	.func		= _clk_stm32_register_gate,\
 }
 
+#define STM32_GATE_PDATA(_id, _name, _parent, _flags, _gate)\
+{\
+	.id		= _id,\
+	.name		= _name,\
+	.parent_data	= _parent,\
+	.flags		= _flags,\
+	.cfg		= (struct stm32_gate_cfg *) {_gate},\
+	.func		= _clk_stm32_register_gate,\
+}
+
 #define _STM32_GATE(_gate_offset, _gate_bit_idx, _gate_flags, _mgate, _ops)\
 	(&(struct stm32_gate_cfg) {\
 		&(struct gate_cfg) {\
@@ -1220,6 +1241,10 @@ _clk_stm32_register_composite(struct device_node *np,
 	STM32_GATE(_id, _name, _parent, _flags,\
 		   _STM32_MGATE(_mgate))
 
+#define MGATE_MP1_PDATA(_id, _name, _parent, _flags, _mgate)\
+	STM32_GATE_PDATA(_id, _name, _parent, _flags,\
+		   _STM32_MGATE(_mgate))
+
 #define _STM32_DIV(_div_offset, _div_shift, _div_width,\
 		   _div_flags, _div_table, _ops)\
 	.div = &(struct stm32_div_cfg) {\
@@ -1279,6 +1304,9 @@ _clk_stm32_register_composite(struct device_node *np,
 #define PCLK(_id, _name, _parent, _flags, _mgate)\
 	MGATE_MP1(_id, _name, _parent, _flags, _mgate)
 
+#define PCLK_PDATA(_id, _name, _parent, _flags, _mgate)\
+	MGATE_MP1_PDATA(_id, _name, _parent, _flags, _mgate)
+
 #define KCLK(_id, _name, _parents, _flags, _mgate, _mmux)\
 	     COMPOSITE(_id, _name, _parents, CLK_OPS_PARENT_ENABLE |\
 		       CLK_SET_RATE_NO_REPARENT | _flags,\
@@ -1886,7 +1914,7 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	PCLK(MDMA, "mdma", "ck_axi", 0, G_MDMA),
 	PCLK(GPU, "gpu", "ck_axi", 0, G_GPU),
 	PCLK(ETHTX, "ethtx", "ck_axi", 0, G_ETHTX),
-	PCLK(ETHRX, "ethrx", "ck_axi", 0, G_ETHRX),
+	PCLK_PDATA(ETHRX, "ethrx", ethrx_src, 0, G_ETHRX),
 	PCLK(ETHMAC, "ethmac", "ck_axi", 0, G_ETHMAC),
 	PCLK(FMC, "fmc", "ck_axi", CLK_IGNORE_UNUSED, G_FMC),
 	PCLK(QSPI, "qspi", "ck_axi", CLK_IGNORE_UNUSED, G_QSPI),
-- 
2.30.2


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

* [PATCH 4/7] clk: stm32mp1: Add parent_data to ETHRX clock
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Pass parent_data to ETHRX clock with new fw_name = "ETH_RX_CLK/ETH_REF_CLK".
By default, this change has no impact on the operation of the clock driver.
However, due to the fw_name, it permits DT to override ETHRX clock parent,
which might be needed in case the ETHRX clock are supplied by external clock
source.

Example of MCO2 supplying clock to ETH_RX_CLK via external pad-to-pad wire:
&rcc {
         clocks = <&rcc CK_MCO2>;
         clock-names = "ETH_RX_CLK/ETH_REF_CLK";
};

Note that while this patch permits to implement this rare usecase, the issue
with ethernet RX and TX input clock modeling on MP1 is far more complex and
requires more core plumbing.

[1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
    Figure 83. Peripheral clock distribution for Ethernet
    https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 drivers/clk/clk-stm32mp1.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index 85bba1ee5fbd..f9a9960945c6 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -152,6 +152,10 @@ static const char * const eth_src[] = {
 	"pll4_p", "pll3_q"
 };
 
+const struct clk_parent_data ethrx_src[] = {
+	{ .name = "ethck_k", .fw_name = "ETH_RX_CLK/ETH_REF_CLK" },
+};
+
 static const char * const rng_src[] = {
 	"ck_csi", "pll4_r", "ck_lse", "ck_lsi"
 };
@@ -314,6 +318,7 @@ struct clock_config {
 	const char *name;
 	const char *parent_name;
 	const char * const *parent_names;
+	const struct clk_parent_data *parent_data;
 	int num_parents;
 	unsigned long flags;
 	void *cfg;
@@ -567,6 +572,7 @@ static struct clk_hw *
 clk_stm32_register_gate_ops(struct device_node *np,
 			    const char *name,
 			    const char *parent_name,
+			    const struct clk_parent_data *parent_data,
 			    unsigned long flags,
 			    void __iomem *base,
 			    const struct stm32_gate_cfg *cfg,
@@ -577,7 +583,10 @@ clk_stm32_register_gate_ops(struct device_node *np,
 	int ret;
 
 	init.name = name;
-	init.parent_names = &parent_name;
+	if (parent_name)
+		init.parent_names = &parent_name;
+	if (parent_data)
+		init.parent_data = parent_data;
 	init.num_parents = 1;
 	init.flags = flags;
 
@@ -602,6 +611,7 @@ clk_stm32_register_gate_ops(struct device_node *np,
 static struct clk_hw *
 clk_stm32_register_composite(struct device_node *np,
 			     const char *name, const char * const *parent_names,
+			     const struct clk_parent_data *parent_data,
 			     int num_parents, void __iomem *base,
 			     const struct stm32_composite_cfg *cfg,
 			     unsigned long flags, spinlock_t *lock)
@@ -1066,6 +1076,7 @@ _clk_stm32_register_gate(struct device_node *np,
 	return clk_stm32_register_gate_ops(np,
 				    cfg->name,
 				    cfg->parent_name,
+				    cfg->parent_data,
 				    cfg->flags,
 				    base,
 				    cfg->cfg,
@@ -1079,8 +1090,8 @@ _clk_stm32_register_composite(struct device_node *np,
 			      const struct clock_config *cfg)
 {
 	return clk_stm32_register_composite(np, cfg->name, cfg->parent_names,
-					    cfg->num_parents, base, cfg->cfg,
-					    cfg->flags, lock);
+					    cfg->parent_data, cfg->num_parents,
+					    base, cfg->cfg, cfg->flags, lock);
 }
 
 #define GATE(_id, _name, _parent, _flags, _offset, _bit_idx, _gate_flags)\
@@ -1187,6 +1198,16 @@ _clk_stm32_register_composite(struct device_node *np,
 	.func		= _clk_stm32_register_gate,\
 }
 
+#define STM32_GATE_PDATA(_id, _name, _parent, _flags, _gate)\
+{\
+	.id		= _id,\
+	.name		= _name,\
+	.parent_data	= _parent,\
+	.flags		= _flags,\
+	.cfg		= (struct stm32_gate_cfg *) {_gate},\
+	.func		= _clk_stm32_register_gate,\
+}
+
 #define _STM32_GATE(_gate_offset, _gate_bit_idx, _gate_flags, _mgate, _ops)\
 	(&(struct stm32_gate_cfg) {\
 		&(struct gate_cfg) {\
@@ -1220,6 +1241,10 @@ _clk_stm32_register_composite(struct device_node *np,
 	STM32_GATE(_id, _name, _parent, _flags,\
 		   _STM32_MGATE(_mgate))
 
+#define MGATE_MP1_PDATA(_id, _name, _parent, _flags, _mgate)\
+	STM32_GATE_PDATA(_id, _name, _parent, _flags,\
+		   _STM32_MGATE(_mgate))
+
 #define _STM32_DIV(_div_offset, _div_shift, _div_width,\
 		   _div_flags, _div_table, _ops)\
 	.div = &(struct stm32_div_cfg) {\
@@ -1279,6 +1304,9 @@ _clk_stm32_register_composite(struct device_node *np,
 #define PCLK(_id, _name, _parent, _flags, _mgate)\
 	MGATE_MP1(_id, _name, _parent, _flags, _mgate)
 
+#define PCLK_PDATA(_id, _name, _parent, _flags, _mgate)\
+	MGATE_MP1_PDATA(_id, _name, _parent, _flags, _mgate)
+
 #define KCLK(_id, _name, _parents, _flags, _mgate, _mmux)\
 	     COMPOSITE(_id, _name, _parents, CLK_OPS_PARENT_ENABLE |\
 		       CLK_SET_RATE_NO_REPARENT | _flags,\
@@ -1886,7 +1914,7 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
 	PCLK(MDMA, "mdma", "ck_axi", 0, G_MDMA),
 	PCLK(GPU, "gpu", "ck_axi", 0, G_GPU),
 	PCLK(ETHTX, "ethtx", "ck_axi", 0, G_ETHTX),
-	PCLK(ETHRX, "ethrx", "ck_axi", 0, G_ETHRX),
+	PCLK_PDATA(ETHRX, "ethrx", ethrx_src, 0, G_ETHRX),
 	PCLK(ETHMAC, "ethmac", "ck_axi", 0, G_ETHMAC),
 	PCLK(FMC, "fmc", "ck_axi", CLK_IGNORE_UNUSED, G_FMC),
 	PCLK(QSPI, "qspi", "ck_axi", CLK_IGNORE_UNUSED, G_QSPI),
-- 
2.30.2


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

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

* [PATCH 5/7] ARM: dts: stm32: Add alternate pinmux for ethernet0 pins
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Add another mux option for ethernet0 pins, this is used on DHCOM when
the ethernet PHY 50 MHz clock is generated by the MCO2 on PG2 pin and
then fed back via PA1 pin.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
index 060baa8b7e9d..08ab62c9f254 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -305,6 +305,40 @@ pins1 {
 		};
 	};
 
+	ethernet0_rmii_pins_b: rmii-1 {
+		pins1 {
+			pinmux = <STM32_PINMUX('G', 13, AF11)>, /* ETH1_RMII_TXD0 */
+				 <STM32_PINMUX('G', 14, AF11)>, /* ETH1_RMII_TXD1 */
+				 <STM32_PINMUX('B', 11, AF11)>, /* ETH1_RMII_TX_EN */
+				 <STM32_PINMUX('A', 1, AF11)>,  /* ETH1_RMII_REF_CLK */
+				 <STM32_PINMUX('A', 2, AF11)>,  /* ETH1_MDIO */
+				 <STM32_PINMUX('C', 1, AF11)>;  /* ETH1_MDC */
+			bias-disable;
+			drive-push-pull;
+			slew-rate = <2>;
+		};
+		pins2 {
+			pinmux = <STM32_PINMUX('C', 4, AF11)>,  /* ETH1_RMII_RXD0 */
+				 <STM32_PINMUX('C', 5, AF11)>,  /* ETH1_RMII_RXD1 */
+				 <STM32_PINMUX('A', 7, AF11)>;  /* ETH1_RMII_CRS_DV */
+			bias-disable;
+		};
+	};
+
+	ethernet0_rmii_sleep_pins_b: rmii-sleep-1 {
+		pins1 {
+			pinmux = <STM32_PINMUX('G', 13, ANALOG)>, /* ETH1_RMII_TXD0 */
+				 <STM32_PINMUX('G', 14, ANALOG)>, /* ETH1_RMII_TXD1 */
+				 <STM32_PINMUX('B', 11, ANALOG)>, /* ETH1_RMII_TX_EN */
+				 <STM32_PINMUX('A', 2, ANALOG)>,  /* ETH1_MDIO */
+				 <STM32_PINMUX('C', 1, ANALOG)>,  /* ETH1_MDC */
+				 <STM32_PINMUX('C', 4, ANALOG)>,  /* ETH1_RMII_RXD0 */
+				 <STM32_PINMUX('C', 5, ANALOG)>,  /* ETH1_RMII_RXD1 */
+				 <STM32_PINMUX('A', 1, ANALOG)>,  /* ETH1_RMII_REF_CLK */
+				 <STM32_PINMUX('A', 7, ANALOG)>;  /* ETH1_RMII_CRS_DV */
+		};
+	};
+
 	fmc_pins_a: fmc-0 {
 		pins1 {
 			pinmux = <STM32_PINMUX('D', 4, AF12)>, /* FMC_NOE */
-- 
2.30.2


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

* [PATCH 5/7] ARM: dts: stm32: Add alternate pinmux for ethernet0 pins
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Add another mux option for ethernet0 pins, this is used on DHCOM when
the ethernet PHY 50 MHz clock is generated by the MCO2 on PG2 pin and
then fed back via PA1 pin.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 34 ++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
index 060baa8b7e9d..08ab62c9f254 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -305,6 +305,40 @@ pins1 {
 		};
 	};
 
+	ethernet0_rmii_pins_b: rmii-1 {
+		pins1 {
+			pinmux = <STM32_PINMUX('G', 13, AF11)>, /* ETH1_RMII_TXD0 */
+				 <STM32_PINMUX('G', 14, AF11)>, /* ETH1_RMII_TXD1 */
+				 <STM32_PINMUX('B', 11, AF11)>, /* ETH1_RMII_TX_EN */
+				 <STM32_PINMUX('A', 1, AF11)>,  /* ETH1_RMII_REF_CLK */
+				 <STM32_PINMUX('A', 2, AF11)>,  /* ETH1_MDIO */
+				 <STM32_PINMUX('C', 1, AF11)>;  /* ETH1_MDC */
+			bias-disable;
+			drive-push-pull;
+			slew-rate = <2>;
+		};
+		pins2 {
+			pinmux = <STM32_PINMUX('C', 4, AF11)>,  /* ETH1_RMII_RXD0 */
+				 <STM32_PINMUX('C', 5, AF11)>,  /* ETH1_RMII_RXD1 */
+				 <STM32_PINMUX('A', 7, AF11)>;  /* ETH1_RMII_CRS_DV */
+			bias-disable;
+		};
+	};
+
+	ethernet0_rmii_sleep_pins_b: rmii-sleep-1 {
+		pins1 {
+			pinmux = <STM32_PINMUX('G', 13, ANALOG)>, /* ETH1_RMII_TXD0 */
+				 <STM32_PINMUX('G', 14, ANALOG)>, /* ETH1_RMII_TXD1 */
+				 <STM32_PINMUX('B', 11, ANALOG)>, /* ETH1_RMII_TX_EN */
+				 <STM32_PINMUX('A', 2, ANALOG)>,  /* ETH1_MDIO */
+				 <STM32_PINMUX('C', 1, ANALOG)>,  /* ETH1_MDC */
+				 <STM32_PINMUX('C', 4, ANALOG)>,  /* ETH1_RMII_RXD0 */
+				 <STM32_PINMUX('C', 5, ANALOG)>,  /* ETH1_RMII_RXD1 */
+				 <STM32_PINMUX('A', 1, ANALOG)>,  /* ETH1_RMII_REF_CLK */
+				 <STM32_PINMUX('A', 7, ANALOG)>;  /* ETH1_RMII_CRS_DV */
+		};
+	};
+
 	fmc_pins_a: fmc-0 {
 		pins1 {
 			pinmux = <STM32_PINMUX('D', 4, AF12)>, /* FMC_NOE */
-- 
2.30.2


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

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

* [PATCH 6/7] ARM: dts: stm32: Add alternate pinmux for mco2 pins
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Add pinmux option for MCO2 pin. This is used on DHCOM when the
ethernet PHY 50 MHz clock is generated by the MCO2 on PG2 pin.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
index 08ab62c9f254..f003c05c24c3 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -849,6 +849,21 @@ pins {
 		};
 	};
 
+	mco2_pins_a: mco2-0 {
+		pins {
+			pinmux = <STM32_PINMUX('G', 2, AF1)>; /* MCO2 */
+			bias-disable;
+			drive-push-pull;
+			slew-rate = <2>;
+		};
+	};
+
+	mco2_sleep_pins_a: mco2-sleep-0 {
+		pins {
+			pinmux = <STM32_PINMUX('G', 2, ANALOG)>; /* MCO2 */
+		};
+	};
+
 	m_can1_pins_a: m-can1-0 {
 		pins1 {
 			pinmux = <STM32_PINMUX('H', 13, AF9)>; /* CAN1_TX */
-- 
2.30.2


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

* [PATCH 6/7] ARM: dts: stm32: Add alternate pinmux for mco2 pins
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Add pinmux option for MCO2 pin. This is used on DHCOM when the
ethernet PHY 50 MHz clock is generated by the MCO2 on PG2 pin.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
index 08ab62c9f254..f003c05c24c3 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -849,6 +849,21 @@ pins {
 		};
 	};
 
+	mco2_pins_a: mco2-0 {
+		pins {
+			pinmux = <STM32_PINMUX('G', 2, AF1)>; /* MCO2 */
+			bias-disable;
+			drive-push-pull;
+			slew-rate = <2>;
+		};
+	};
+
+	mco2_sleep_pins_a: mco2-sleep-0 {
+		pins {
+			pinmux = <STM32_PINMUX('G', 2, ANALOG)>; /* MCO2 */
+		};
+	};
+
 	m_can1_pins_a: m-can1-0 {
 		pins1 {
 			pinmux = <STM32_PINMUX('H', 13, AF9)>; /* CAN1_TX */
-- 
2.30.2


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

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

* [PATCH 7/7] ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 18:57   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

The DHCOM SoM has two options for supplying ETHRX clock to the DWMAC
block and PHY. Either (1) ETHCK_K generates 50 MHz clock on ETH_CLK
pad for the PHY and the same 50 MHz clock are fed back to ETHRX via
internal eth_clk_fb clock connection OR (2) ETH_CLK is not used at
all, MCO2 generates 50 MHz clock on MCO2 output pad for the PHY and
the same MCO2 clock are fed back into ETHRX via ETH_RX_CLK input pad
using external pad-to-pad connection.

Option (1) has two downsides. ETHCK_K is supplied directly from either
PLL3_Q or PLL4_P, hence the PLL output is limited to exactly 50 MHz and
since the same PLL output is also used to supply SDMMC blocks, the
performance of SD and eMMC access is affected. The second downside is
that using this option, the EMI of the SoM is higher.

Option (2) solves both of those problems, so implement it here. In this
case, the PLL4_P is no longer limited and can be operated faster, at
100 MHz, which improves SDMMC performance (read performance is improved
from ~41 MiB/s to ~57 MiB/s with dd if=/dev/mmcblk1 of=/dev/null bs=64M
count=1). The EMI interference also decreases.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
index 272a1a67a9ad..4c7c06a7aea0 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
@@ -114,15 +114,29 @@ &dts {
 	status = "okay";
 };
 
+&rcc {
+	/* Connect MCO2 output to ETH_RX_CLK input via pad-pad connection */
+	clocks = <&rcc CK_MCO2>;
+	clock-names = "ETH_RX_CLK/ETH_REF_CLK";
+
+	/*
+	 * Set PLL4P output to 100 MHz to supply SDMMC with faster clock,
+	 * set MCO2 output to 50 MHz to supply ETHRX clock with PLL4P/2,
+	 * so that MCO2 behaves as a divider for the ETHRX clock here.
+	 */
+	assigned-clocks = <&rcc CK_MCO2>, <&rcc PLL4_P>;
+	assigned-clock-parents = <&rcc PLL4_P>;
+	assigned-clock-rates = <50000000>, <100000000>;
+};
+
 &ethernet0 {
 	status = "okay";
-	pinctrl-0 = <&ethernet0_rmii_pins_a>;
-	pinctrl-1 = <&ethernet0_rmii_sleep_pins_a>;
+	pinctrl-0 = <&ethernet0_rmii_pins_b &mco2_pins_a>;
+	pinctrl-1 = <&ethernet0_rmii_sleep_pins_b &mco2_sleep_pins_a>;
 	pinctrl-names = "default", "sleep";
 	phy-mode = "rmii";
 	max-speed = <100>;
 	phy-handle = <&phy0>;
-	st,eth-ref-clk-sel;
 	phy-reset-gpios = <&gpioh 3 GPIO_ACTIVE_LOW>;
 
 	mdio0 {
-- 
2.30.2


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

* [PATCH 7/7] ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM
@ 2021-04-08 18:57   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-08 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

The DHCOM SoM has two options for supplying ETHRX clock to the DWMAC
block and PHY. Either (1) ETHCK_K generates 50 MHz clock on ETH_CLK
pad for the PHY and the same 50 MHz clock are fed back to ETHRX via
internal eth_clk_fb clock connection OR (2) ETH_CLK is not used at
all, MCO2 generates 50 MHz clock on MCO2 output pad for the PHY and
the same MCO2 clock are fed back into ETHRX via ETH_RX_CLK input pad
using external pad-to-pad connection.

Option (1) has two downsides. ETHCK_K is supplied directly from either
PLL3_Q or PLL4_P, hence the PLL output is limited to exactly 50 MHz and
since the same PLL output is also used to supply SDMMC blocks, the
performance of SD and eMMC access is affected. The second downside is
that using this option, the EMI of the SoM is higher.

Option (2) solves both of those problems, so implement it here. In this
case, the PLL4_P is no longer limited and can be operated faster, at
100 MHz, which improves SDMMC performance (read performance is improved
from ~41 MiB/s to ~57 MiB/s with dd if=/dev/mmcblk1 of=/dev/null bs=64M
count=1). The EMI interference also decreases.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
index 272a1a67a9ad..4c7c06a7aea0 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
@@ -114,15 +114,29 @@ &dts {
 	status = "okay";
 };
 
+&rcc {
+	/* Connect MCO2 output to ETH_RX_CLK input via pad-pad connection */
+	clocks = <&rcc CK_MCO2>;
+	clock-names = "ETH_RX_CLK/ETH_REF_CLK";
+
+	/*
+	 * Set PLL4P output to 100 MHz to supply SDMMC with faster clock,
+	 * set MCO2 output to 50 MHz to supply ETHRX clock with PLL4P/2,
+	 * so that MCO2 behaves as a divider for the ETHRX clock here.
+	 */
+	assigned-clocks = <&rcc CK_MCO2>, <&rcc PLL4_P>;
+	assigned-clock-parents = <&rcc PLL4_P>;
+	assigned-clock-rates = <50000000>, <100000000>;
+};
+
 &ethernet0 {
 	status = "okay";
-	pinctrl-0 = <&ethernet0_rmii_pins_a>;
-	pinctrl-1 = <&ethernet0_rmii_sleep_pins_a>;
+	pinctrl-0 = <&ethernet0_rmii_pins_b &mco2_pins_a>;
+	pinctrl-1 = <&ethernet0_rmii_sleep_pins_b &mco2_sleep_pins_a>;
 	pinctrl-names = "default", "sleep";
 	phy-mode = "rmii";
 	max-speed = <100>;
 	phy-handle = <&phy0>;
-	st,eth-ref-clk-sel;
 	phy-reset-gpios = <&gpioh 3 GPIO_ACTIVE_LOW>;
 
 	mdio0 {
-- 
2.30.2


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

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-08 20:32   ` Stephen Boyd
  -1 siblings, 0 replies; 52+ messages in thread
From: Stephen Boyd @ 2021-04-08 20:32 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay, linux-clk,
	linux-stm32

Quoting Marek Vasut (2021-04-08 11:57:24)
> The implementation of ETH_RX_CLK/ETH_REF_CLK handling on STM32MP1 currently
> does not permit selecting the RX clock input from SoC pad, the RX clock are
> hard-wired to eth_clk_fb. This cover letter describes multiple unsuccessful
> attempts at solving this problem and a proposed partial solution.

Please use <sboyd@kernel.org> for clk patches, or run get_maintainer.pl

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-08 20:32   ` Stephen Boyd
  0 siblings, 0 replies; 52+ messages in thread
From: Stephen Boyd @ 2021-04-08 20:32 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Marek Vasut, Alexandre Torgue, Christophe Roullier,
	Gabriel Fernandez, Patrice Chotard, Patrick Delaunay, linux-clk,
	linux-stm32

Quoting Marek Vasut (2021-04-08 11:57:24)
> The implementation of ETH_RX_CLK/ETH_REF_CLK handling on STM32MP1 currently
> does not permit selecting the RX clock input from SoC pad, the RX clock are
> hard-wired to eth_clk_fb. This cover letter describes multiple unsuccessful
> attempts at solving this problem and a proposed partial solution.

Please use <sboyd@kernel.org> for clk patches, or run get_maintainer.pl

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

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
  2021-04-08 18:57 ` Marek Vasut
@ 2021-04-12  8:09   ` Alexandre TORGUE
  -1 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-12  8:09 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek

On 4/8/21 8:57 PM, Marek Vasut wrote:
> The implementation of ETH_RX_CLK/ETH_REF_CLK handling on STM32MP1 currently
> does not permit selecting the RX clock input from SoC pad, the RX clock are
> hard-wired to eth_clk_fb. This cover letter describes multiple unsuccessful
> attempts at solving this problem and a proposed partial solution.
> 
> The problem is that in stm32mp151.dtsi, the "st,stm32mp1-dwmac" compatible
> ethernet requests mac-clk-rx = <&rcc ETHRX> as MAC RX clock, which per [1]
> page 576 maps to clk_rx_i signal. Upon closer inspection of clk-stm32mp1.c,
> the ETHRX clock 1) only models G_ETHRX gate, which represents ETHRXEN bit,
> and 2) sets ETHRX clock parent to ck_axi. Both are wrong for the following
> reasons:
> 1) ETHRX clock are composite clock of gate and two muxes, which ultimately
>     permit selecting ETHRX clock parent to be either eth_clk_fb or external
>     ETH_RX_CLK/ETH_REF_CLOCK clock from SoC pad. Because only the gate part
>     is modeled, the clock topology is wrong and it is not possible to select
>     the ETHRX clock parent using the clock framework at all.
> 2) ETHRX clock are supplied from a mux controlled by ETH_SEL[2] (from
>     SYSCFG), which is supplied from a mux controlled by ETH_REF_CLK_SEL
>     (from SYSCFG), which is supplied either by ETH_RX_CLK/ETH_REF_CLK
>     from pad OR internally from eth_clk_fb clock, which maps to ETHCK_K
>     clock. Hence, ETHRX can never be supplied by ck_axi.
> 
> Another unique feature of ETHRX clock muxes is that they are controlled
> via SYSCFG block instead of being part of the RCC block. This is a rather
> unfortunate design, as it creates dependency between RCC, SYSCFG and DWMAC.
> Currently, those clock muxes are configured in the dwmac4 driver through
> syscon interface to SYSCFG block, based on custom device tree properties --
> st,eth-clk-sel and st,eth-ref-clk-sel -- this is clearly not the correct
> approach.
> 
> First approach was to implement ETHRX clock including muxes in clk-stm32mp1.c.
> This means RCC clock driver, clk-stm32mp1.c, must obtain access to SYSCFG IP
> registers via some syscon_regmap_lookup_by_phandle() when registering ETHRX
> clock in RCC driver probe, so that it can read out SYSCFG register state to
> determine the current mux state, and later toggle the mux bits. The problem
> here is that the SYSCFG requires clock which are provided by RCC, so
> syscon_regmap_lookup_by_phandle() fails with EPROBE_DEFER because the clock
> are not available yet in RCC clock driver probe, so there is cyclic dependency
> between RCC and SYSCFG.
> 
> Similar approach would be to turn SYSCFG into simple-bus and implement
> separate SYSCFG clock sub-driver, which would register single clock mux with
> two parents -- ETHCK_K and ETH_RX_CLK -- and would only control the SYSCFG
> clock mux bits. The problem here is similar, for RCC driver to register the
> ETHRX clock, the ETHRX clock must have a parent clock, however the parent
> clock are not ready yet, because this new SYSCFG clock sub-driver requires
> SYSCFG register clock, which only become available once RCC finishes probing,
> so there again is a cyclic dependency between RCC and SYSCFG.
> 
> One possible solution could be to defer obtaining the SYSCFG regmap handle
> until later point, i.e. only once a register access to SYSCFG is required
> the first time. This could permit us to register the mux in the RCC driver
> including a mux which requires SYSCFG access via syscon, although this would
> likely still run into problems during probe, since the clock framework would
> call .get_parent() during mux registration and trigger the SYSCFG access.
> 
> All the above still only discusses the clock part of the problem. Even if
> the clock cyclic dependencies could be solved, it would be necessary to
> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
> and avoid DT ABI break.

Thanks for those clear explanations and for this series. As discussed, 
this approach looks good to me as it doesn't break our current strategy 
for dwmac clock integration. I don't know if those cyclic redundancies 
will be fixed one day but we can have a look on dwmac DT properties (the 
gain to change them, the effort to keep the backward compatibility, code 
readability, ...).

Your DT patches looks good. I'll merge them soon.

regards
alex


> So in the end, this series takes a different approach and only partly solves
> the problem. This series splits ETHCK_K, so the ETHCK_K gate could be turned
> off and adds support for selecting ETHRX clock parent clock via DT. This then
> permits e.g. DHCOM to select MCO2 as ETHRX clock source and the clock tree is
> then correctly set up.
> 
> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>      Figure 83. Peripheral clock distribution for Ethernet
>      https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf
> 
> Marek Vasut (7):
>    clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
>    clk: stm32mp1: The dev is always NULL, replace it with np
>    clk: stm32mp1: Register clock with device_node pointer
>    clk: stm32mp1: Add parent_data to ETHRX clock
>    ARM: dts: stm32: Add alternate pinmux for ethernet0 pins
>    ARM: dts: stm32: Add alternate pinmux for mco2 pins
>    ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM
> 
>   arch/arm/boot/dts/stm32mp15-pinctrl.dtsi     |  49 +++++++
>   arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi |  20 ++-
>   drivers/clk/clk-stm32mp1.c                   | 134 +++++++++++--------
>   3 files changed, 146 insertions(+), 57 deletions(-)
> 
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-arm-kernel@lists.infradead.org
> 



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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-12  8:09   ` Alexandre TORGUE
  0 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-12  8:09 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek

On 4/8/21 8:57 PM, Marek Vasut wrote:
> The implementation of ETH_RX_CLK/ETH_REF_CLK handling on STM32MP1 currently
> does not permit selecting the RX clock input from SoC pad, the RX clock are
> hard-wired to eth_clk_fb. This cover letter describes multiple unsuccessful
> attempts at solving this problem and a proposed partial solution.
> 
> The problem is that in stm32mp151.dtsi, the "st,stm32mp1-dwmac" compatible
> ethernet requests mac-clk-rx = <&rcc ETHRX> as MAC RX clock, which per [1]
> page 576 maps to clk_rx_i signal. Upon closer inspection of clk-stm32mp1.c,
> the ETHRX clock 1) only models G_ETHRX gate, which represents ETHRXEN bit,
> and 2) sets ETHRX clock parent to ck_axi. Both are wrong for the following
> reasons:
> 1) ETHRX clock are composite clock of gate and two muxes, which ultimately
>     permit selecting ETHRX clock parent to be either eth_clk_fb or external
>     ETH_RX_CLK/ETH_REF_CLOCK clock from SoC pad. Because only the gate part
>     is modeled, the clock topology is wrong and it is not possible to select
>     the ETHRX clock parent using the clock framework at all.
> 2) ETHRX clock are supplied from a mux controlled by ETH_SEL[2] (from
>     SYSCFG), which is supplied from a mux controlled by ETH_REF_CLK_SEL
>     (from SYSCFG), which is supplied either by ETH_RX_CLK/ETH_REF_CLK
>     from pad OR internally from eth_clk_fb clock, which maps to ETHCK_K
>     clock. Hence, ETHRX can never be supplied by ck_axi.
> 
> Another unique feature of ETHRX clock muxes is that they are controlled
> via SYSCFG block instead of being part of the RCC block. This is a rather
> unfortunate design, as it creates dependency between RCC, SYSCFG and DWMAC.
> Currently, those clock muxes are configured in the dwmac4 driver through
> syscon interface to SYSCFG block, based on custom device tree properties --
> st,eth-clk-sel and st,eth-ref-clk-sel -- this is clearly not the correct
> approach.
> 
> First approach was to implement ETHRX clock including muxes in clk-stm32mp1.c.
> This means RCC clock driver, clk-stm32mp1.c, must obtain access to SYSCFG IP
> registers via some syscon_regmap_lookup_by_phandle() when registering ETHRX
> clock in RCC driver probe, so that it can read out SYSCFG register state to
> determine the current mux state, and later toggle the mux bits. The problem
> here is that the SYSCFG requires clock which are provided by RCC, so
> syscon_regmap_lookup_by_phandle() fails with EPROBE_DEFER because the clock
> are not available yet in RCC clock driver probe, so there is cyclic dependency
> between RCC and SYSCFG.
> 
> Similar approach would be to turn SYSCFG into simple-bus and implement
> separate SYSCFG clock sub-driver, which would register single clock mux with
> two parents -- ETHCK_K and ETH_RX_CLK -- and would only control the SYSCFG
> clock mux bits. The problem here is similar, for RCC driver to register the
> ETHRX clock, the ETHRX clock must have a parent clock, however the parent
> clock are not ready yet, because this new SYSCFG clock sub-driver requires
> SYSCFG register clock, which only become available once RCC finishes probing,
> so there again is a cyclic dependency between RCC and SYSCFG.
> 
> One possible solution could be to defer obtaining the SYSCFG regmap handle
> until later point, i.e. only once a register access to SYSCFG is required
> the first time. This could permit us to register the mux in the RCC driver
> including a mux which requires SYSCFG access via syscon, although this would
> likely still run into problems during probe, since the clock framework would
> call .get_parent() during mux registration and trigger the SYSCFG access.
> 
> All the above still only discusses the clock part of the problem. Even if
> the clock cyclic dependencies could be solved, it would be necessary to
> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
> and avoid DT ABI break.

Thanks for those clear explanations and for this series. As discussed, 
this approach looks good to me as it doesn't break our current strategy 
for dwmac clock integration. I don't know if those cyclic redundancies 
will be fixed one day but we can have a look on dwmac DT properties (the 
gain to change them, the effort to keep the backward compatibility, code 
readability, ...).

Your DT patches looks good. I'll merge them soon.

regards
alex


> So in the end, this series takes a different approach and only partly solves
> the problem. This series splits ETHCK_K, so the ETHCK_K gate could be turned
> off and adds support for selecting ETHRX clock parent clock via DT. This then
> permits e.g. DHCOM to select MCO2 as ETHRX clock source and the clock tree is
> then correctly set up.
> 
> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>      Figure 83. Peripheral clock distribution for Ethernet
>      https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf
> 
> Marek Vasut (7):
>    clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
>    clk: stm32mp1: The dev is always NULL, replace it with np
>    clk: stm32mp1: Register clock with device_node pointer
>    clk: stm32mp1: Add parent_data to ETHRX clock
>    ARM: dts: stm32: Add alternate pinmux for ethernet0 pins
>    ARM: dts: stm32: Add alternate pinmux for mco2 pins
>    ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM
> 
>   arch/arm/boot/dts/stm32mp15-pinctrl.dtsi     |  49 +++++++
>   arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi |  20 ++-
>   drivers/clk/clk-stm32mp1.c                   | 134 +++++++++++--------
>   3 files changed, 146 insertions(+), 57 deletions(-)
> 
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-arm-kernel@lists.infradead.org
> 



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

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
  2021-04-12  8:09   ` Alexandre TORGUE
@ 2021-04-12 18:44     ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-12 18:44 UTC (permalink / raw)
  To: Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

On 4/12/21 10:09 AM, Alexandre TORGUE wrote:
> Hi Marek

Hello Alex,

[...]

>> All the above still only discusses the clock part of the problem. Even if
>> the clock cyclic dependencies could be solved, it would be necessary to
>> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
>> and avoid DT ABI break.
> 
> Thanks for those clear explanations and for this series. As discussed, 
> this approach looks good to me as it doesn't break our current strategy 
> for dwmac clock integration. I don't know if those cyclic redundancies 
> will be fixed one day but we can have a look on dwmac DT properties (the 
> gain to change them, the effort to keep the backward compatibility, code 
> readability, ...).
> 
> Your DT patches looks good. I'll merge them soon.
+CC Stephen ; the DT patches depend on the clock driver changes. Would 
it make sense to pick the clock patches through the same tree or how 
should that be handled ?

[...]

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-12 18:44     ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-12 18:44 UTC (permalink / raw)
  To: Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

On 4/12/21 10:09 AM, Alexandre TORGUE wrote:
> Hi Marek

Hello Alex,

[...]

>> All the above still only discusses the clock part of the problem. Even if
>> the clock cyclic dependencies could be solved, it would be necessary to
>> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
>> and avoid DT ABI break.
> 
> Thanks for those clear explanations and for this series. As discussed, 
> this approach looks good to me as it doesn't break our current strategy 
> for dwmac clock integration. I don't know if those cyclic redundancies 
> will be fixed one day but we can have a look on dwmac DT properties (the 
> gain to change them, the effort to keep the backward compatibility, code 
> readability, ...).
> 
> Your DT patches looks good. I'll merge them soon.
+CC Stephen ; the DT patches depend on the clock driver changes. Would 
it make sense to pick the clock patches through the same tree or how 
should that be handled ?

[...]

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

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
  2021-04-12 18:44     ` Marek Vasut
@ 2021-04-13  7:48       ` Alexandre TORGUE
  -1 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-13  7:48 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

Hi Marek

On 4/12/21 8:44 PM, Marek Vasut wrote:
> On 4/12/21 10:09 AM, Alexandre TORGUE wrote:
>> Hi Marek
> 
> Hello Alex,
> 
> [...]
> 
>>> All the above still only discusses the clock part of the problem. 
>>> Even if
>>> the clock cyclic dependencies could be solved, it would be necessary to
>>> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
>>> and avoid DT ABI break.
>>
>> Thanks for those clear explanations and for this series. As discussed, 
>> this approach looks good to me as it doesn't break our current 
>> strategy for dwmac clock integration. I don't know if those cyclic 
>> redundancies will be fixed one day but we can have a look on dwmac DT 
>> properties (the gain to change them, the effort to keep the backward 
>> compatibility, code readability, ...).
>>
>> Your DT patches looks good. I'll merge them soon.
> +CC Stephen ; the DT patches depend on the clock driver changes. Would 
> it make sense to pick the clock patches through the same tree or how 
> should that be handled ?

In this situation, I prefer to wait that Stephen takes clock patches in 
his tree. Then I'll take DT ones in mine. (I assume that taking only 
clock patches will not break mp1 boot or Ethernet usage).

Regards
Alex


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

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-13  7:48       ` Alexandre TORGUE
  0 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-13  7:48 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

Hi Marek

On 4/12/21 8:44 PM, Marek Vasut wrote:
> On 4/12/21 10:09 AM, Alexandre TORGUE wrote:
>> Hi Marek
> 
> Hello Alex,
> 
> [...]
> 
>>> All the above still only discusses the clock part of the problem. 
>>> Even if
>>> the clock cyclic dependencies could be solved, it would be necessary to
>>> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT properties
>>> and avoid DT ABI break.
>>
>> Thanks for those clear explanations and for this series. As discussed, 
>> this approach looks good to me as it doesn't break our current 
>> strategy for dwmac clock integration. I don't know if those cyclic 
>> redundancies will be fixed one day but we can have a look on dwmac DT 
>> properties (the gain to change them, the effort to keep the backward 
>> compatibility, code readability, ...).
>>
>> Your DT patches looks good. I'll merge them soon.
> +CC Stephen ; the DT patches depend on the clock driver changes. Would 
> it make sense to pick the clock patches through the same tree or how 
> should that be handled ?

In this situation, I prefer to wait that Stephen takes clock patches in 
his tree. Then I'll take DT ones in mine. (I assume that taking only 
clock patches will not break mp1 boot or Ethernet usage).

Regards
Alex


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

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

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
  2021-04-13  7:48       ` Alexandre TORGUE
@ 2021-04-13 12:05         ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-13 12:05 UTC (permalink / raw)
  To: Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

On 4/13/21 9:48 AM, Alexandre TORGUE wrote:
> Hi Marek

Hello Alexandre,

> On 4/12/21 8:44 PM, Marek Vasut wrote:
>> On 4/12/21 10:09 AM, Alexandre TORGUE wrote:
>>> Hi Marek
>>
>> Hello Alex,
>>
>> [...]
>>
>>>> All the above still only discusses the clock part of the problem. 
>>>> Even if
>>>> the clock cyclic dependencies could be solved, it would be necessary to
>>>> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT 
>>>> properties
>>>> and avoid DT ABI break.
>>>
>>> Thanks for those clear explanations and for this series. As 
>>> discussed, this approach looks good to me as it doesn't break our 
>>> current strategy for dwmac clock integration. I don't know if those 
>>> cyclic redundancies will be fixed one day but we can have a look on 
>>> dwmac DT properties (the gain to change them, the effort to keep the 
>>> backward compatibility, code readability, ...).
>>>
>>> Your DT patches looks good. I'll merge them soon.
>> +CC Stephen ; the DT patches depend on the clock driver changes. Would 
>> it make sense to pick the clock patches through the same tree or how 
>> should that be handled ?
> 
> In this situation, I prefer to wait that Stephen takes clock patches in 
> his tree. Then I'll take DT ones in mine. (I assume that taking only 
> clock patches will not break mp1 boot or Ethernet usage).

That's fine by me, thanks !

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

* Re: [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM
@ 2021-04-13 12:05         ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-13 12:05 UTC (permalink / raw)
  To: Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Gabriel Fernandez, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

On 4/13/21 9:48 AM, Alexandre TORGUE wrote:
> Hi Marek

Hello Alexandre,

> On 4/12/21 8:44 PM, Marek Vasut wrote:
>> On 4/12/21 10:09 AM, Alexandre TORGUE wrote:
>>> Hi Marek
>>
>> Hello Alex,
>>
>> [...]
>>
>>>> All the above still only discusses the clock part of the problem. 
>>>> Even if
>>>> the clock cyclic dependencies could be solved, it would be necessary to
>>>> resolve legacy dwmac st,eth-clk-sel and st,eth-ref-clk-sel DT 
>>>> properties
>>>> and avoid DT ABI break.
>>>
>>> Thanks for those clear explanations and for this series. As 
>>> discussed, this approach looks good to me as it doesn't break our 
>>> current strategy for dwmac clock integration. I don't know if those 
>>> cyclic redundancies will be fixed one day but we can have a look on 
>>> dwmac DT properties (the gain to change them, the effort to keep the 
>>> backward compatibility, code readability, ...).
>>>
>>> Your DT patches looks good. I'll merge them soon.
>> +CC Stephen ; the DT patches depend on the clock driver changes. Would 
>> it make sense to pick the clock patches through the same tree or how 
>> should that be handled ?
> 
> In this situation, I prefer to wait that Stephen takes clock patches in 
> his tree. Then I'll take DT ones in mine. (I assume that taking only 
> clock patches will not break mp1 boot or Ethernet usage).

That's fine by me, thanks !

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-08 18:57   ` Marek Vasut
@ 2021-04-14 13:03     ` gabriel.fernandez
  -1 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-14 13:03 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek,

Thanks for the patchset

On 4/8/21 8:57 PM, Marek Vasut wrote:
> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
> in use, ETHCKEN gate can be turned off. Current driver does not permit
> that, fix it.

I don"t understand, it's already the case.

ETHCK_K it's a composite with a MUX and a GATE.

ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no gate)

If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.

  

> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
> ETHPTP_K remain functional as before.
>
> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>      Figure 83. Peripheral clock distribution for Ethernet
>      https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf
>
> Signed-off-by: Marek Vasut<marex@denx.de>
> Cc: Alexandre Torgue<alexandre.torgue@foss.st.com>
> Cc: Christophe Roullier<christophe.roullier@foss.st.com>
> Cc: Gabriel Fernandez<gabriel.fernandez@foss.st.com>
> Cc: Patrice Chotard<patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay<patrick.delaunay@foss.st.com>
> Cc: Stephen Boyd<swboyd@chromium.org>
> Cc:linux-clk@vger.kernel.org
> Cc:linux-stm32@st-md-mailman.stormreply.com
> To:linux-arm-kernel@lists.infradead.org
> ---
>   drivers/clk/clk-stm32mp1.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
> index a875649df8b8..a7c7f544ee5d 100644
> --- a/drivers/clk/clk-stm32mp1.c
> +++ b/drivers/clk/clk-stm32mp1.c
> @@ -1949,7 +1949,6 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
>   	KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>   	KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>   	KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
> -	KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>   
>   	/* Particulary Kernel Clocks (no mux or no gate) */
>   	MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
> @@ -1958,11 +1957,16 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
>   	MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>   	MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>   
> -	COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
> +	COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>   		  CLK_SET_RATE_NO_REPARENT,
>   		  _NO_GATE,
>   		  _MMUX(M_ETHCK),
> -		  _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
> +		  _NO_DIV),
> +
> +	MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
assigned parent with ETHCK_K will not work
> +
> +	DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |

CLK_OPS_PARENT_ENABLE flags not useful with a divider.

best regards

Gabriel

> +	    CLK_SET_RATE_NO_REPARENT, RCC_ETHCKSELR, 4, 4, 0),
>   
>   	/* RTC clock */
>   	DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0),

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-14 13:03     ` gabriel.fernandez
  0 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-14 13:03 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek,

Thanks for the patchset

On 4/8/21 8:57 PM, Marek Vasut wrote:
> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
> in use, ETHCKEN gate can be turned off. Current driver does not permit
> that, fix it.

I don"t understand, it's already the case.

ETHCK_K it's a composite with a MUX and a GATE.

ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no gate)

If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.

  

> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
> ETHPTP_K remain functional as before.
>
> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>      Figure 83. Peripheral clock distribution for Ethernet
>      https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf
>
> Signed-off-by: Marek Vasut<marex@denx.de>
> Cc: Alexandre Torgue<alexandre.torgue@foss.st.com>
> Cc: Christophe Roullier<christophe.roullier@foss.st.com>
> Cc: Gabriel Fernandez<gabriel.fernandez@foss.st.com>
> Cc: Patrice Chotard<patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay<patrick.delaunay@foss.st.com>
> Cc: Stephen Boyd<swboyd@chromium.org>
> Cc:linux-clk@vger.kernel.org
> Cc:linux-stm32@st-md-mailman.stormreply.com
> To:linux-arm-kernel@lists.infradead.org
> ---
>   drivers/clk/clk-stm32mp1.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
> index a875649df8b8..a7c7f544ee5d 100644
> --- a/drivers/clk/clk-stm32mp1.c
> +++ b/drivers/clk/clk-stm32mp1.c
> @@ -1949,7 +1949,6 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
>   	KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>   	KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>   	KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
> -	KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>   
>   	/* Particulary Kernel Clocks (no mux or no gate) */
>   	MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
> @@ -1958,11 +1957,16 @@ static const struct clock_config stm32mp1_clock_cfg[] = {
>   	MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>   	MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>   
> -	COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
> +	COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>   		  CLK_SET_RATE_NO_REPARENT,
>   		  _NO_GATE,
>   		  _MMUX(M_ETHCK),
> -		  _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
> +		  _NO_DIV),
> +
> +	MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
assigned parent with ETHCK_K will not work
> +
> +	DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |

CLK_OPS_PARENT_ENABLE flags not useful with a divider.

best regards

Gabriel

> +	    CLK_SET_RATE_NO_REPARENT, RCC_ETHCKSELR, 4, 4, 0),
>   
>   	/* RTC clock */
>   	DIV(NO_ID, "ck_hse_rtc", "ck_hse", 0, RCC_RTCDIVR, 0, 6, 0),

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-14 13:03     ` gabriel.fernandez
@ 2021-04-14 14:04       ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-14 14:04 UTC (permalink / raw)
  To: gabriel.fernandez, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
> Hi Marek,

Hello Gabriel,

> Thanks for the patchset
> 
> On 4/8/21 8:57 PM, Marek Vasut wrote:
>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>> that, fix it.
> 
> I don"t understand, it's already the case.
> 
> ETHCK_K it's a composite with a MUX and a GATE.

But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
datasheet again and schematic below.

> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no gate)

But ETHPTP_K shouldn't control any mux, it is only a divider.

> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.

Look, this is what you have today:

            .------------ ETHCK_K -----------.
            |_______               _______   |
pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
            | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
            |               |
            |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
            |                                          |
            '------------ ETHPTP_K --------------------'

And this is what you should have, to avoid having two composite clock 
which control the same mux using the same register bit, i.e. what this 
patch implements:

            .- ck_ker_eth -.  .--- ETHCK_K --.
            |_______       |  |    _______   |
pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
            | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
                            |
                            '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
                             |                         |
                             '---- ETHPTP_K -----------'

>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>> ETHPTP_K remain functional as before.
>>
>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>      Figure 83. Peripheral clock distribution for Ethernet
>>      
>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>

[...]

>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>> index a875649df8b8..a7c7f544ee5d 100644
>> --- a/drivers/clk/clk-stm32mp1.c
>> +++ b/drivers/clk/clk-stm32mp1.c
>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>> stm32mp1_clock_cfg[] = {
>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>> stm32mp1_clock_cfg[] = {
>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>             CLK_SET_RATE_NO_REPARENT,
>>             _NO_GATE,
>>             _MMUX(M_ETHCK),
>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>> +          _NO_DIV),
>> +
>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
> assigned parent with ETHCK_K will not work
>> +
>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
> 
> CLK_OPS_PARENT_ENABLE flags not useful with a divider.

How so ?

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-14 14:04       ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-14 14:04 UTC (permalink / raw)
  To: gabriel.fernandez, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
> Hi Marek,

Hello Gabriel,

> Thanks for the patchset
> 
> On 4/8/21 8:57 PM, Marek Vasut wrote:
>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>> that, fix it.
> 
> I don"t understand, it's already the case.
> 
> ETHCK_K it's a composite with a MUX and a GATE.

But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
datasheet again and schematic below.

> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no gate)

But ETHPTP_K shouldn't control any mux, it is only a divider.

> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.

Look, this is what you have today:

            .------------ ETHCK_K -----------.
            |_______               _______   |
pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
            | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
            |               |
            |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
            |                                          |
            '------------ ETHPTP_K --------------------'

And this is what you should have, to avoid having two composite clock 
which control the same mux using the same register bit, i.e. what this 
patch implements:

            .- ck_ker_eth -.  .--- ETHCK_K --.
            |_______       |  |    _______   |
pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
            | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
                            |
                            '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
                             |                         |
                             '---- ETHPTP_K -----------'

>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>> ETHPTP_K remain functional as before.
>>
>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>      Figure 83. Peripheral clock distribution for Ethernet
>>      
>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>

[...]

>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>> index a875649df8b8..a7c7f544ee5d 100644
>> --- a/drivers/clk/clk-stm32mp1.c
>> +++ b/drivers/clk/clk-stm32mp1.c
>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>> stm32mp1_clock_cfg[] = {
>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>> stm32mp1_clock_cfg[] = {
>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>             CLK_SET_RATE_NO_REPARENT,
>>             _NO_GATE,
>>             _MMUX(M_ETHCK),
>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>> +          _NO_DIV),
>> +
>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
> assigned parent with ETHCK_K will not work
>> +
>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
> 
> CLK_OPS_PARENT_ENABLE flags not useful with a divider.

How so ?

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-14 14:04       ` Marek Vasut
@ 2021-04-16  6:44         ` gabriel.fernandez
  -1 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-16  6:44 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek

On 4/14/21 4:04 PM, Marek Vasut wrote:
> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>> Hi Marek,
> 
> Hello Gabriel,
> 
>> Thanks for the patchset
>>
>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>>> that, fix it.
>>
>> I don"t understand, it's already the case.
>>
>> ETHCK_K it's a composite with a MUX and a GATE.
> 
> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
> datasheet again and schematic below.
> 
>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no 
>> gate)
> 
> But ETHPTP_K shouldn't control any mux, it is only a divider.
> 
>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
> 
> Look, this is what you have today:
> 
>             .------------ ETHCK_K -----------.
>             |_______               _______   |
> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>             |               |
>             |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>             |                                          |
>             '------------ ETHPTP_K --------------------'
> 
> And this is what you should have, to avoid having two composite clock 
> which control the same mux using the same register bit, i.e. what this 
> patch implements:
> 
>             .- ck_ker_eth -.  .--- ETHCK_K --.
>             |_______       |  |    _______   |
> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>                             |
>                             '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>                              |                         |
>                              '---- ETHPTP_K -----------'
> 

These 2 solutions are valid. I made the choice to implement the first 
one to be able to change parent with the kernel clock of the IP (no need 
to add an intermediate binding). It's the same principle for all kernel 
of this soc.
I can ask to Alexandre to comeback of this principle, but i 'm not 
favorable.

>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>> ETHPTP_K remain functional as before.
>>>
>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>      Figure 83. Peripheral clock distribution for Ethernet
>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>
> 
> [...]
> 
>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>> index a875649df8b8..a7c7f544ee5d 100644
>>> --- a/drivers/clk/clk-stm32mp1.c
>>> +++ b/drivers/clk/clk-stm32mp1.c
>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>> stm32mp1_clock_cfg[] = {
>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>> stm32mp1_clock_cfg[] = {
>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>             CLK_SET_RATE_NO_REPARENT,
>>>             _NO_GATE,
>>>             _MMUX(M_ETHCK),
>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>> +          _NO_DIV),
>>> +
>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>> assigned parent with ETHCK_K will not work
>>> +
>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>
>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
> 
> How so ?

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-16  6:44         ` gabriel.fernandez
  0 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-16  6:44 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek

On 4/14/21 4:04 PM, Marek Vasut wrote:
> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>> Hi Marek,
> 
> Hello Gabriel,
> 
>> Thanks for the patchset
>>
>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>>> that, fix it.
>>
>> I don"t understand, it's already the case.
>>
>> ETHCK_K it's a composite with a MUX and a GATE.
> 
> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
> datasheet again and schematic below.
> 
>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no 
>> gate)
> 
> But ETHPTP_K shouldn't control any mux, it is only a divider.
> 
>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
> 
> Look, this is what you have today:
> 
>             .------------ ETHCK_K -----------.
>             |_______               _______   |
> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>             |               |
>             |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>             |                                          |
>             '------------ ETHPTP_K --------------------'
> 
> And this is what you should have, to avoid having two composite clock 
> which control the same mux using the same register bit, i.e. what this 
> patch implements:
> 
>             .- ck_ker_eth -.  .--- ETHCK_K --.
>             |_______       |  |    _______   |
> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>                             |
>                             '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>                              |                         |
>                              '---- ETHPTP_K -----------'
> 

These 2 solutions are valid. I made the choice to implement the first 
one to be able to change parent with the kernel clock of the IP (no need 
to add an intermediate binding). It's the same principle for all kernel 
of this soc.
I can ask to Alexandre to comeback of this principle, but i 'm not 
favorable.

>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent clock
>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>> ETHPTP_K remain functional as before.
>>>
>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>      Figure 83. Peripheral clock distribution for Ethernet
>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>
> 
> [...]
> 
>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>> index a875649df8b8..a7c7f544ee5d 100644
>>> --- a/drivers/clk/clk-stm32mp1.c
>>> +++ b/drivers/clk/clk-stm32mp1.c
>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>> stm32mp1_clock_cfg[] = {
>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>> stm32mp1_clock_cfg[] = {
>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>             CLK_SET_RATE_NO_REPARENT,
>>>             _NO_GATE,
>>>             _MMUX(M_ETHCK),
>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>> +          _NO_DIV),
>>> +
>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>> assigned parent with ETHCK_K will not work
>>> +
>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>
>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
> 
> How so ?

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

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
  2021-04-08 18:57   ` Marek Vasut
@ 2021-04-16  6:44     ` gabriel.fernandez
  -1 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-16  6:44 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek,

I have a patch-set in progress using $dev (convertion into module driver).

https://patchwork.kernel.org/project/linux-clk/list/?series=421767

Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data to 
ETHRX clock, can be done easily.

Best regards
Gabriel

On 4/8/21 8:57 PM, Marek Vasut wrote:
> Instead of passing around $dev to all the registration functions, which
> is always NULL, pass around struct device_node pointer $np. This way it
> is possible to use of_clk_hw_register*() functions and/or register clock
> with associated $np pointer.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-arm-kernel@lists.infradead.org
> ---
>   drivers/clk/clk-stm32mp1.c | 56 +++++++++++++++++++-------------------
>   1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
> index a7c7f544ee5d..cf5a1d055c5a 100644
> --- a/drivers/clk/clk-stm32mp1.c
> +++ b/drivers/clk/clk-stm32mp1.c
> @@ -317,7 +317,7 @@ struct clock_config {
>   	int num_parents;
>   	unsigned long flags;
>   	void *cfg;
> -	struct clk_hw * (*func)(struct device *dev,
> +	struct clk_hw * (*func)(struct device_node *np,
>   				struct clk_hw_onecell_data *clk_data,
>   				void __iomem *base, spinlock_t *lock,
>   				const struct clock_config *cfg);
> @@ -377,14 +377,14 @@ struct stm32_composite_cfg {
>   };
>   
>   static struct clk_hw *
> -_clk_hw_register_gate(struct device *dev,
> +_clk_hw_register_gate(struct device_node *np,
>   		      struct clk_hw_onecell_data *clk_data,
>   		      void __iomem *base, spinlock_t *lock,
>   		      const struct clock_config *cfg)
>   {
>   	struct gate_cfg *gate_cfg = cfg->cfg;
>   
> -	return clk_hw_register_gate(dev,
> +	return clk_hw_register_gate(NULL,
>   				    cfg->name,
>   				    cfg->parent_name,
>   				    cfg->flags,
> @@ -395,27 +395,27 @@ _clk_hw_register_gate(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -_clk_hw_register_fixed_factor(struct device *dev,
> +_clk_hw_register_fixed_factor(struct device_node *np,
>   			      struct clk_hw_onecell_data *clk_data,
>   			      void __iomem *base, spinlock_t *lock,
>   			      const struct clock_config *cfg)
>   {
>   	struct fixed_factor_cfg *ff_cfg = cfg->cfg;
>   
> -	return clk_hw_register_fixed_factor(dev, cfg->name, cfg->parent_name,
> +	return clk_hw_register_fixed_factor(NULL, cfg->name, cfg->parent_name,
>   					    cfg->flags, ff_cfg->mult,
>   					    ff_cfg->div);
>   }
>   
>   static struct clk_hw *
> -_clk_hw_register_divider_table(struct device *dev,
> +_clk_hw_register_divider_table(struct device_node *np,
>   			       struct clk_hw_onecell_data *clk_data,
>   			       void __iomem *base, spinlock_t *lock,
>   			       const struct clock_config *cfg)
>   {
>   	struct div_cfg *div_cfg = cfg->cfg;
>   
> -	return clk_hw_register_divider_table(dev,
> +	return clk_hw_register_divider_table(NULL,
>   					     cfg->name,
>   					     cfg->parent_name,
>   					     cfg->flags,
> @@ -428,14 +428,14 @@ _clk_hw_register_divider_table(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -_clk_hw_register_mux(struct device *dev,
> +_clk_hw_register_mux(struct device_node *np,
>   		     struct clk_hw_onecell_data *clk_data,
>   		     void __iomem *base, spinlock_t *lock,
>   		     const struct clock_config *cfg)
>   {
>   	struct mux_cfg *mux_cfg = cfg->cfg;
>   
> -	return clk_hw_register_mux(dev, cfg->name, cfg->parent_names,
> +	return clk_hw_register_mux(NULL, cfg->name, cfg->parent_names,
>   				   cfg->num_parents, cfg->flags,
>   				   mux_cfg->reg_off + base, mux_cfg->shift,
>   				   mux_cfg->width, mux_cfg->mux_flags, lock);
> @@ -570,7 +570,7 @@ _get_stm32_gate(void __iomem *base,
>   }
>   
>   static struct clk_hw *
> -clk_stm32_register_gate_ops(struct device *dev,
> +clk_stm32_register_gate_ops(struct device_node *np,
>   			    const char *name,
>   			    const char *parent_name,
>   			    unsigned long flags,
> @@ -598,7 +598,7 @@ clk_stm32_register_gate_ops(struct device *dev,
>   
>   	hw->init = &init;
>   
> -	ret = clk_hw_register(dev, hw);
> +	ret = clk_hw_register(NULL, hw);
>   	if (ret)
>   		hw = ERR_PTR(ret);
>   
> @@ -606,7 +606,7 @@ clk_stm32_register_gate_ops(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -clk_stm32_register_composite(struct device *dev,
> +clk_stm32_register_composite(struct device_node *np,
>   			     const char *name, const char * const *parent_names,
>   			     int num_parents, void __iomem *base,
>   			     const struct stm32_composite_cfg *cfg,
> @@ -655,7 +655,7 @@ clk_stm32_register_composite(struct device *dev,
>   		}
>   	}
>   
> -	return clk_hw_register_composite(dev, name, parent_names, num_parents,
> +	return clk_hw_register_composite(NULL, name, parent_names, num_parents,
>   				       mux_hw, mux_ops, div_hw, div_ops,
>   				       gate_hw, gate_ops, flags);
>   }
> @@ -863,7 +863,7 @@ static const struct clk_ops pll_ops = {
>   	.is_enabled	= pll_is_enabled,
>   };
>   
> -static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
> +static struct clk_hw *clk_register_pll(struct device_node *np, const char *name,
>   				       const char *parent_name,
>   				       void __iomem *reg,
>   				       unsigned long flags,
> @@ -889,7 +889,7 @@ static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
>   	element->lock = lock;
>   
>   	hw = &element->hw;
> -	err = clk_hw_register(dev, hw);
> +	err = clk_hw_register(NULL, hw);
>   
>   	if (err) {
>   		kfree(element);
> @@ -993,7 +993,7 @@ static const struct clk_ops timer_ker_ops = {
>   
>   };
>   
> -static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
> +static struct clk_hw *clk_register_cktim(struct device_node *np, const char *name,
>   					 const char *parent_name,
>   					 unsigned long flags,
>   					 void __iomem *apbdiv,
> @@ -1021,7 +1021,7 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
>   	tim_ker->timpre = timpre;
>   
>   	hw = &tim_ker->hw;
> -	err = clk_hw_register(dev, hw);
> +	err = clk_hw_register(NULL, hw);
>   
>   	if (err) {
>   		kfree(tim_ker);
> @@ -1035,14 +1035,14 @@ struct stm32_pll_cfg {
>   	u32 offset;
>   };
>   
> -static struct clk_hw *_clk_register_pll(struct device *dev,
> +static struct clk_hw *_clk_register_pll(struct device_node *np,
>   					struct clk_hw_onecell_data *clk_data,
>   					void __iomem *base, spinlock_t *lock,
>   					const struct clock_config *cfg)
>   {
>   	struct stm32_pll_cfg *stm_pll_cfg = cfg->cfg;
>   
> -	return clk_register_pll(dev, cfg->name, cfg->parent_name,
> +	return clk_register_pll(np, cfg->name, cfg->parent_name,
>   				base + stm_pll_cfg->offset, cfg->flags, lock);
>   }
>   
> @@ -1051,25 +1051,25 @@ struct stm32_cktim_cfg {
>   	u32 offset_timpre;
>   };
>   
> -static struct clk_hw *_clk_register_cktim(struct device *dev,
> +static struct clk_hw *_clk_register_cktim(struct device_node *np,
>   					  struct clk_hw_onecell_data *clk_data,
>   					  void __iomem *base, spinlock_t *lock,
>   					  const struct clock_config *cfg)
>   {
>   	struct stm32_cktim_cfg *cktim_cfg = cfg->cfg;
>   
> -	return clk_register_cktim(dev, cfg->name, cfg->parent_name, cfg->flags,
> +	return clk_register_cktim(np, cfg->name, cfg->parent_name, cfg->flags,
>   				  cktim_cfg->offset_apbdiv + base,
>   				  cktim_cfg->offset_timpre + base, lock);
>   }
>   
>   static struct clk_hw *
> -_clk_stm32_register_gate(struct device *dev,
> +_clk_stm32_register_gate(struct device_node *np,
>   			 struct clk_hw_onecell_data *clk_data,
>   			 void __iomem *base, spinlock_t *lock,
>   			 const struct clock_config *cfg)
>   {
> -	return clk_stm32_register_gate_ops(dev,
> +	return clk_stm32_register_gate_ops(np,
>   				    cfg->name,
>   				    cfg->parent_name,
>   				    cfg->flags,
> @@ -1079,12 +1079,12 @@ _clk_stm32_register_gate(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -_clk_stm32_register_composite(struct device *dev,
> +_clk_stm32_register_composite(struct device_node *np,
>   			      struct clk_hw_onecell_data *clk_data,
>   			      void __iomem *base, spinlock_t *lock,
>   			      const struct clock_config *cfg)
>   {
> -	return clk_stm32_register_composite(dev, cfg->name, cfg->parent_names,
> +	return clk_stm32_register_composite(np, cfg->name, cfg->parent_names,
>   					    cfg->num_parents, base, cfg->cfg,
>   					    cfg->flags, lock);
>   }
> @@ -2020,7 +2020,7 @@ static const struct of_device_id stm32mp1_match_data[] = {
>   	{ }
>   };
>   
> -static int stm32_register_hw_clk(struct device *dev,
> +static int stm32_register_hw_clk(struct device_node *np,
>   				 struct clk_hw_onecell_data *clk_data,
>   				 void __iomem *base, spinlock_t *lock,
>   				 const struct clock_config *cfg)
> @@ -2031,7 +2031,7 @@ static int stm32_register_hw_clk(struct device *dev,
>   	hws = clk_data->hws;
>   
>   	if (cfg->func)
> -		hw = (*cfg->func)(dev, clk_data, base, lock, cfg);
> +		hw = (*cfg->func)(np, clk_data, base, lock, cfg);
>   
>   	if (IS_ERR(hw)) {
>   		pr_err("Unable to register %s\n", cfg->name);
> @@ -2077,7 +2077,7 @@ static int stm32_rcc_init(struct device_node *np,
>   		hws[n] = ERR_PTR(-ENOENT);
>   
>   	for (n = 0; n < data->num; n++) {
> -		err = stm32_register_hw_clk(NULL, clk_data, base, &rlock,
> +		err = stm32_register_hw_clk(np, clk_data, base, &rlock,
>   					    &data->cfg[n]);
>   		if (err) {
>   			pr_err("%s: can't register  %s\n", __func__,
> 

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
@ 2021-04-16  6:44     ` gabriel.fernandez
  0 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-16  6:44 UTC (permalink / raw)
  To: Marek Vasut, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

Hi Marek,

I have a patch-set in progress using $dev (convertion into module driver).

https://patchwork.kernel.org/project/linux-clk/list/?series=421767

Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data to 
ETHRX clock, can be done easily.

Best regards
Gabriel

On 4/8/21 8:57 PM, Marek Vasut wrote:
> Instead of passing around $dev to all the registration functions, which
> is always NULL, pass around struct device_node pointer $np. This way it
> is possible to use of_clk_hw_register*() functions and/or register clock
> with associated $np pointer.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> Cc: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: linux-clk@vger.kernel.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-arm-kernel@lists.infradead.org
> ---
>   drivers/clk/clk-stm32mp1.c | 56 +++++++++++++++++++-------------------
>   1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
> index a7c7f544ee5d..cf5a1d055c5a 100644
> --- a/drivers/clk/clk-stm32mp1.c
> +++ b/drivers/clk/clk-stm32mp1.c
> @@ -317,7 +317,7 @@ struct clock_config {
>   	int num_parents;
>   	unsigned long flags;
>   	void *cfg;
> -	struct clk_hw * (*func)(struct device *dev,
> +	struct clk_hw * (*func)(struct device_node *np,
>   				struct clk_hw_onecell_data *clk_data,
>   				void __iomem *base, spinlock_t *lock,
>   				const struct clock_config *cfg);
> @@ -377,14 +377,14 @@ struct stm32_composite_cfg {
>   };
>   
>   static struct clk_hw *
> -_clk_hw_register_gate(struct device *dev,
> +_clk_hw_register_gate(struct device_node *np,
>   		      struct clk_hw_onecell_data *clk_data,
>   		      void __iomem *base, spinlock_t *lock,
>   		      const struct clock_config *cfg)
>   {
>   	struct gate_cfg *gate_cfg = cfg->cfg;
>   
> -	return clk_hw_register_gate(dev,
> +	return clk_hw_register_gate(NULL,
>   				    cfg->name,
>   				    cfg->parent_name,
>   				    cfg->flags,
> @@ -395,27 +395,27 @@ _clk_hw_register_gate(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -_clk_hw_register_fixed_factor(struct device *dev,
> +_clk_hw_register_fixed_factor(struct device_node *np,
>   			      struct clk_hw_onecell_data *clk_data,
>   			      void __iomem *base, spinlock_t *lock,
>   			      const struct clock_config *cfg)
>   {
>   	struct fixed_factor_cfg *ff_cfg = cfg->cfg;
>   
> -	return clk_hw_register_fixed_factor(dev, cfg->name, cfg->parent_name,
> +	return clk_hw_register_fixed_factor(NULL, cfg->name, cfg->parent_name,
>   					    cfg->flags, ff_cfg->mult,
>   					    ff_cfg->div);
>   }
>   
>   static struct clk_hw *
> -_clk_hw_register_divider_table(struct device *dev,
> +_clk_hw_register_divider_table(struct device_node *np,
>   			       struct clk_hw_onecell_data *clk_data,
>   			       void __iomem *base, spinlock_t *lock,
>   			       const struct clock_config *cfg)
>   {
>   	struct div_cfg *div_cfg = cfg->cfg;
>   
> -	return clk_hw_register_divider_table(dev,
> +	return clk_hw_register_divider_table(NULL,
>   					     cfg->name,
>   					     cfg->parent_name,
>   					     cfg->flags,
> @@ -428,14 +428,14 @@ _clk_hw_register_divider_table(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -_clk_hw_register_mux(struct device *dev,
> +_clk_hw_register_mux(struct device_node *np,
>   		     struct clk_hw_onecell_data *clk_data,
>   		     void __iomem *base, spinlock_t *lock,
>   		     const struct clock_config *cfg)
>   {
>   	struct mux_cfg *mux_cfg = cfg->cfg;
>   
> -	return clk_hw_register_mux(dev, cfg->name, cfg->parent_names,
> +	return clk_hw_register_mux(NULL, cfg->name, cfg->parent_names,
>   				   cfg->num_parents, cfg->flags,
>   				   mux_cfg->reg_off + base, mux_cfg->shift,
>   				   mux_cfg->width, mux_cfg->mux_flags, lock);
> @@ -570,7 +570,7 @@ _get_stm32_gate(void __iomem *base,
>   }
>   
>   static struct clk_hw *
> -clk_stm32_register_gate_ops(struct device *dev,
> +clk_stm32_register_gate_ops(struct device_node *np,
>   			    const char *name,
>   			    const char *parent_name,
>   			    unsigned long flags,
> @@ -598,7 +598,7 @@ clk_stm32_register_gate_ops(struct device *dev,
>   
>   	hw->init = &init;
>   
> -	ret = clk_hw_register(dev, hw);
> +	ret = clk_hw_register(NULL, hw);
>   	if (ret)
>   		hw = ERR_PTR(ret);
>   
> @@ -606,7 +606,7 @@ clk_stm32_register_gate_ops(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -clk_stm32_register_composite(struct device *dev,
> +clk_stm32_register_composite(struct device_node *np,
>   			     const char *name, const char * const *parent_names,
>   			     int num_parents, void __iomem *base,
>   			     const struct stm32_composite_cfg *cfg,
> @@ -655,7 +655,7 @@ clk_stm32_register_composite(struct device *dev,
>   		}
>   	}
>   
> -	return clk_hw_register_composite(dev, name, parent_names, num_parents,
> +	return clk_hw_register_composite(NULL, name, parent_names, num_parents,
>   				       mux_hw, mux_ops, div_hw, div_ops,
>   				       gate_hw, gate_ops, flags);
>   }
> @@ -863,7 +863,7 @@ static const struct clk_ops pll_ops = {
>   	.is_enabled	= pll_is_enabled,
>   };
>   
> -static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
> +static struct clk_hw *clk_register_pll(struct device_node *np, const char *name,
>   				       const char *parent_name,
>   				       void __iomem *reg,
>   				       unsigned long flags,
> @@ -889,7 +889,7 @@ static struct clk_hw *clk_register_pll(struct device *dev, const char *name,
>   	element->lock = lock;
>   
>   	hw = &element->hw;
> -	err = clk_hw_register(dev, hw);
> +	err = clk_hw_register(NULL, hw);
>   
>   	if (err) {
>   		kfree(element);
> @@ -993,7 +993,7 @@ static const struct clk_ops timer_ker_ops = {
>   
>   };
>   
> -static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
> +static struct clk_hw *clk_register_cktim(struct device_node *np, const char *name,
>   					 const char *parent_name,
>   					 unsigned long flags,
>   					 void __iomem *apbdiv,
> @@ -1021,7 +1021,7 @@ static struct clk_hw *clk_register_cktim(struct device *dev, const char *name,
>   	tim_ker->timpre = timpre;
>   
>   	hw = &tim_ker->hw;
> -	err = clk_hw_register(dev, hw);
> +	err = clk_hw_register(NULL, hw);
>   
>   	if (err) {
>   		kfree(tim_ker);
> @@ -1035,14 +1035,14 @@ struct stm32_pll_cfg {
>   	u32 offset;
>   };
>   
> -static struct clk_hw *_clk_register_pll(struct device *dev,
> +static struct clk_hw *_clk_register_pll(struct device_node *np,
>   					struct clk_hw_onecell_data *clk_data,
>   					void __iomem *base, spinlock_t *lock,
>   					const struct clock_config *cfg)
>   {
>   	struct stm32_pll_cfg *stm_pll_cfg = cfg->cfg;
>   
> -	return clk_register_pll(dev, cfg->name, cfg->parent_name,
> +	return clk_register_pll(np, cfg->name, cfg->parent_name,
>   				base + stm_pll_cfg->offset, cfg->flags, lock);
>   }
>   
> @@ -1051,25 +1051,25 @@ struct stm32_cktim_cfg {
>   	u32 offset_timpre;
>   };
>   
> -static struct clk_hw *_clk_register_cktim(struct device *dev,
> +static struct clk_hw *_clk_register_cktim(struct device_node *np,
>   					  struct clk_hw_onecell_data *clk_data,
>   					  void __iomem *base, spinlock_t *lock,
>   					  const struct clock_config *cfg)
>   {
>   	struct stm32_cktim_cfg *cktim_cfg = cfg->cfg;
>   
> -	return clk_register_cktim(dev, cfg->name, cfg->parent_name, cfg->flags,
> +	return clk_register_cktim(np, cfg->name, cfg->parent_name, cfg->flags,
>   				  cktim_cfg->offset_apbdiv + base,
>   				  cktim_cfg->offset_timpre + base, lock);
>   }
>   
>   static struct clk_hw *
> -_clk_stm32_register_gate(struct device *dev,
> +_clk_stm32_register_gate(struct device_node *np,
>   			 struct clk_hw_onecell_data *clk_data,
>   			 void __iomem *base, spinlock_t *lock,
>   			 const struct clock_config *cfg)
>   {
> -	return clk_stm32_register_gate_ops(dev,
> +	return clk_stm32_register_gate_ops(np,
>   				    cfg->name,
>   				    cfg->parent_name,
>   				    cfg->flags,
> @@ -1079,12 +1079,12 @@ _clk_stm32_register_gate(struct device *dev,
>   }
>   
>   static struct clk_hw *
> -_clk_stm32_register_composite(struct device *dev,
> +_clk_stm32_register_composite(struct device_node *np,
>   			      struct clk_hw_onecell_data *clk_data,
>   			      void __iomem *base, spinlock_t *lock,
>   			      const struct clock_config *cfg)
>   {
> -	return clk_stm32_register_composite(dev, cfg->name, cfg->parent_names,
> +	return clk_stm32_register_composite(np, cfg->name, cfg->parent_names,
>   					    cfg->num_parents, base, cfg->cfg,
>   					    cfg->flags, lock);
>   }
> @@ -2020,7 +2020,7 @@ static const struct of_device_id stm32mp1_match_data[] = {
>   	{ }
>   };
>   
> -static int stm32_register_hw_clk(struct device *dev,
> +static int stm32_register_hw_clk(struct device_node *np,
>   				 struct clk_hw_onecell_data *clk_data,
>   				 void __iomem *base, spinlock_t *lock,
>   				 const struct clock_config *cfg)
> @@ -2031,7 +2031,7 @@ static int stm32_register_hw_clk(struct device *dev,
>   	hws = clk_data->hws;
>   
>   	if (cfg->func)
> -		hw = (*cfg->func)(dev, clk_data, base, lock, cfg);
> +		hw = (*cfg->func)(np, clk_data, base, lock, cfg);
>   
>   	if (IS_ERR(hw)) {
>   		pr_err("Unable to register %s\n", cfg->name);
> @@ -2077,7 +2077,7 @@ static int stm32_rcc_init(struct device_node *np,
>   		hws[n] = ERR_PTR(-ENOENT);
>   
>   	for (n = 0; n < data->num; n++) {
> -		err = stm32_register_hw_clk(NULL, clk_data, base, &rlock,
> +		err = stm32_register_hw_clk(np, clk_data, base, &rlock,
>   					    &data->cfg[n]);
>   		if (err) {
>   			pr_err("%s: can't register  %s\n", __func__,
> 

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

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
  2021-04-16  6:44     ` gabriel.fernandez
@ 2021-04-16 13:39       ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 13:39 UTC (permalink / raw)
  To: gabriel.fernandez, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
> Hi Marek,

Hello Gabriel,

> I have a patch-set in progress using $dev (convertion into module driver).
> 
> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
> 
> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data to 
> ETHRX clock, can be done easily.

I suspect the aforementioned patchset will have to be reworked, since 
the current approach to push SCMI onto every system and renumerate the 
clock has been rejected, because it caused DT ABI break and boot 
problems on many systems.

btw please don't top-post.

> Best regards
> Gabriel
> 
> On 4/8/21 8:57 PM, Marek Vasut wrote:
>> Instead of passing around $dev to all the registration functions, which
>> is always NULL, pass around struct device_node pointer $np. This way it
>> is possible to use of_clk_hw_register*() functions and/or register clock
>> with associated $np pointer.
[...]

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
@ 2021-04-16 13:39       ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 13:39 UTC (permalink / raw)
  To: gabriel.fernandez, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32

On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
> Hi Marek,

Hello Gabriel,

> I have a patch-set in progress using $dev (convertion into module driver).
> 
> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
> 
> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data to 
> ETHRX clock, can be done easily.

I suspect the aforementioned patchset will have to be reworked, since 
the current approach to push SCMI onto every system and renumerate the 
clock has been rejected, because it caused DT ABI break and boot 
problems on many systems.

btw please don't top-post.

> Best regards
> Gabriel
> 
> On 4/8/21 8:57 PM, Marek Vasut wrote:
>> Instead of passing around $dev to all the registration functions, which
>> is always NULL, pass around struct device_node pointer $np. This way it
>> is possible to use of_clk_hw_register*() functions and/or register clock
>> with associated $np pointer.
[...]

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-16  6:44         ` gabriel.fernandez
@ 2021-04-16 13:47           ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 13:47 UTC (permalink / raw)
  To: gabriel.fernandez, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
> Hi Marek

Hello Gabriel,

> On 4/14/21 4:04 PM, Marek Vasut wrote:
>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>> Hi Marek,
>>
>> Hello Gabriel,
>>
>>> Thanks for the patchset
>>>
>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>>>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>>>> that, fix it.
>>>
>>> I don"t understand, it's already the case.
>>>
>>> ETHCK_K it's a composite with a MUX and a GATE.
>>
>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>> datasheet again and schematic below.
>>
>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no 
>>> gate)
>>
>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>
>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>
>> Look, this is what you have today:
>>
>>             .------------ ETHCK_K -----------.
>>             |_______               _______   |
>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>             |               |
>>             |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>             |                                          |
>>             '------------ ETHPTP_K --------------------'
>>
>> And this is what you should have, to avoid having two composite clock 
>> which control the same mux using the same register bit, i.e. what this 
>> patch implements:
>>
>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>             |_______       |  |    _______   |
>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>                             |
>>                             '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>                              |                         |
>>                              '---- ETHPTP_K -----------'
>>
> 
> These 2 solutions are valid. I made the choice to implement the first 
> one to be able to change parent with the kernel clock of the IP (no need 
> to add an intermediate binding).

Which IP are you talking about in here ?

> It's the same principle for all kernel 
> of this soc.

The first option is wrong, because in that model, you have two composite 
clock which control the same one mux bit in the same register. Basically 
you register two distinct clock which operate the same hardware knob.

> I can ask to Alexandre to comeback of this principle, but i 'm not 
> favorable.

Please ask.

>>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent 
>>>> clock
>>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>>> ETHPTP_K remain functional as before.
>>>>
>>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>>      Figure 83. Peripheral clock distribution for Ethernet
>>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>>
>>
>> [...]
>>
>>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>>> index a875649df8b8..a7c7f544ee5d 100644
>>>> --- a/drivers/clk/clk-stm32mp1.c
>>>> +++ b/drivers/clk/clk-stm32mp1.c
>>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>>> stm32mp1_clock_cfg[] = {
>>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>>> stm32mp1_clock_cfg[] = {
>>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>             CLK_SET_RATE_NO_REPARENT,
>>>>             _NO_GATE,
>>>>             _MMUX(M_ETHCK),
>>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>>> +          _NO_DIV),
>>>> +
>>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>>> assigned parent with ETHCK_K will not work
>>>> +
>>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>>
>>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
>>
>> How so ?

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-16 13:47           ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 13:47 UTC (permalink / raw)
  To: gabriel.fernandez, linux-arm-kernel
  Cc: Alexandre Torgue, Christophe Roullier, Patrice Chotard,
	Patrick Delaunay, Stephen Boyd, linux-clk, linux-stm32,
	Stephen Boyd

On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
> Hi Marek

Hello Gabriel,

> On 4/14/21 4:04 PM, Marek Vasut wrote:
>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>> Hi Marek,
>>
>> Hello Gabriel,
>>
>>> Thanks for the patchset
>>>
>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however per
>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. Peripheral
>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past the
>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN gate.
>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>>>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>>>> that, fix it.
>>>
>>> I don"t understand, it's already the case.
>>>
>>> ETHCK_K it's a composite with a MUX and a GATE.
>>
>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>> datasheet again and schematic below.
>>
>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV (no 
>>> gate)
>>
>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>
>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>
>> Look, this is what you have today:
>>
>>             .------------ ETHCK_K -----------.
>>             |_______               _______   |
>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>             |               |
>>             |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>             |                                          |
>>             '------------ ETHPTP_K --------------------'
>>
>> And this is what you should have, to avoid having two composite clock 
>> which control the same mux using the same register bit, i.e. what this 
>> patch implements:
>>
>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>             |_______       |  |    _______   |
>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>                             |
>>                             '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>                              |                         |
>>                              '---- ETHPTP_K -----------'
>>
> 
> These 2 solutions are valid. I made the choice to implement the first 
> one to be able to change parent with the kernel clock of the IP (no need 
> to add an intermediate binding).

Which IP are you talking about in here ?

> It's the same principle for all kernel 
> of this soc.

The first option is wrong, because in that model, you have two composite 
clock which control the same one mux bit in the same register. Basically 
you register two distinct clock which operate the same hardware knob.

> I can ask to Alexandre to comeback of this principle, but i 'm not 
> favorable.

Please ask.

>>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent 
>>>> clock
>>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>>> ETHPTP_K remain functional as before.
>>>>
>>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>>      Figure 83. Peripheral clock distribution for Ethernet
>>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>>
>>
>> [...]
>>
>>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>>> index a875649df8b8..a7c7f544ee5d 100644
>>>> --- a/drivers/clk/clk-stm32mp1.c
>>>> +++ b/drivers/clk/clk-stm32mp1.c
>>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>>> stm32mp1_clock_cfg[] = {
>>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>>> stm32mp1_clock_cfg[] = {
>>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>             CLK_SET_RATE_NO_REPARENT,
>>>>             _NO_GATE,
>>>>             _MMUX(M_ETHCK),
>>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>>> +          _NO_DIV),
>>>> +
>>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>>> assigned parent with ETHCK_K will not work
>>>> +
>>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>>
>>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
>>
>> How so ?

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

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
  2021-04-16 13:39       ` Marek Vasut
@ 2021-04-16 14:39         ` Alexandre TORGUE
  -1 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-16 14:39 UTC (permalink / raw)
  To: Marek Vasut, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Hi Marek

On 4/16/21 3:39 PM, Marek Vasut wrote:
> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>> Hi Marek,
> 
> Hello Gabriel,
> 
>> I have a patch-set in progress using $dev (convertion into module 
>> driver).
>>
>> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
>>
>> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data 
>> to ETHRX clock, can be done easily.
> 
> I suspect the aforementioned patchset will have to be reworked, since 
> the current approach to push SCMI onto every system and renumerate the 
> clock has been rejected, because it caused DT ABI break and boot 
> problems on many systems.

SCMI patches for clock drivers will be pushed (and merged one day :)). 
We only drop the DT part which will be done through dtbo in uboot/tfa/optee.

regards
alex

> 
> btw please don't top-post.
> 
>> Best regards
>> Gabriel
>>
>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>> Instead of passing around $dev to all the registration functions, which
>>> is always NULL, pass around struct device_node pointer $np. This way it
>>> is possible to use of_clk_hw_register*() functions and/or register clock
>>> with associated $np pointer.
> [...]

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
@ 2021-04-16 14:39         ` Alexandre TORGUE
  0 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-16 14:39 UTC (permalink / raw)
  To: Marek Vasut, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

Hi Marek

On 4/16/21 3:39 PM, Marek Vasut wrote:
> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>> Hi Marek,
> 
> Hello Gabriel,
> 
>> I have a patch-set in progress using $dev (convertion into module 
>> driver).
>>
>> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
>>
>> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data 
>> to ETHRX clock, can be done easily.
> 
> I suspect the aforementioned patchset will have to be reworked, since 
> the current approach to push SCMI onto every system and renumerate the 
> clock has been rejected, because it caused DT ABI break and boot 
> problems on many systems.

SCMI patches for clock drivers will be pushed (and merged one day :)). 
We only drop the DT part which will be done through dtbo in uboot/tfa/optee.

regards
alex

> 
> btw please don't top-post.
> 
>> Best regards
>> Gabriel
>>
>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>> Instead of passing around $dev to all the registration functions, which
>>> is always NULL, pass around struct device_node pointer $np. This way it
>>> is possible to use of_clk_hw_register*() functions and/or register clock
>>> with associated $np pointer.
> [...]

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

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
  2021-04-16 14:39         ` Alexandre TORGUE
@ 2021-04-16 14:54           ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 14:54 UTC (permalink / raw)
  To: Alexandre TORGUE, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

On 4/16/21 4:39 PM, Alexandre TORGUE wrote:
> Hi Marek

Hello Alexandre,

> On 4/16/21 3:39 PM, Marek Vasut wrote:
>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>> Hi Marek,
>>
>> Hello Gabriel,
>>
>>> I have a patch-set in progress using $dev (convertion into module 
>>> driver).
>>>
>>> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
>>>
>>> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data 
>>> to ETHRX clock, can be done easily.
>>
>> I suspect the aforementioned patchset will have to be reworked, since 
>> the current approach to push SCMI onto every system and renumerate the 
>> clock has been rejected, because it caused DT ABI break and boot 
>> problems on many systems.
> 
> SCMI patches for clock drivers will be pushed (and merged one day :)). 
> We only drop the DT part which will be done through dtbo in 
> uboot/tfa/optee.
If the result works for both options (without SCMI and opt-in with 
SCMI), then that's fine. Does the aforementioned patchset already 
implement that ?

If so (or if there is a rebase + repost of the above patchset which 
does), then I am happy to rebase this one on top.

-- 
Best regards,
Marek Vasut

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
@ 2021-04-16 14:54           ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 14:54 UTC (permalink / raw)
  To: Alexandre TORGUE, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32

On 4/16/21 4:39 PM, Alexandre TORGUE wrote:
> Hi Marek

Hello Alexandre,

> On 4/16/21 3:39 PM, Marek Vasut wrote:
>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>> Hi Marek,
>>
>> Hello Gabriel,
>>
>>> I have a patch-set in progress using $dev (convertion into module 
>>> driver).
>>>
>>> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
>>>
>>> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add parent_data 
>>> to ETHRX clock, can be done easily.
>>
>> I suspect the aforementioned patchset will have to be reworked, since 
>> the current approach to push SCMI onto every system and renumerate the 
>> clock has been rejected, because it caused DT ABI break and boot 
>> problems on many systems.
> 
> SCMI patches for clock drivers will be pushed (and merged one day :)). 
> We only drop the DT part which will be done through dtbo in 
> uboot/tfa/optee.
If the result works for both options (without SCMI and opt-in with 
SCMI), then that's fine. Does the aforementioned patchset already 
implement that ?

If so (or if there is a rebase + repost of the above patchset which 
does), then I am happy to rebase this one on top.

-- 
Best regards,
Marek Vasut

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

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
  2021-04-16 14:54           ` Marek Vasut
@ 2021-04-16 15:01             ` Alexandre TORGUE
  -1 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-16 15:01 UTC (permalink / raw)
  To: Marek Vasut, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32



On 4/16/21 4:54 PM, Marek Vasut wrote:
> On 4/16/21 4:39 PM, Alexandre TORGUE wrote:
>> Hi Marek
> 
> Hello Alexandre,
> 
>> On 4/16/21 3:39 PM, Marek Vasut wrote:
>>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>>> Hi Marek,
>>>
>>> Hello Gabriel,
>>>
>>>> I have a patch-set in progress using $dev (convertion into module 
>>>> driver).
>>>>
>>>> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
>>>>
>>>> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add 
>>>> parent_data to ETHRX clock, can be done easily.
>>>
>>> I suspect the aforementioned patchset will have to be reworked, since 
>>> the current approach to push SCMI onto every system and renumerate 
>>> the clock has been rejected, because it caused DT ABI break and boot 
>>> problems on many systems.
>>
>> SCMI patches for clock drivers will be pushed (and merged one day :)). 
>> We only drop the DT part which will be done through dtbo in 
>> uboot/tfa/optee.
> If the result works for both options (without SCMI and opt-in with 
> SCMI), then that's fine. Does the aforementioned patchset already 
> implement that ?

yes it will work with basic boot. You can have a try with on your DH board.

> 
> If so (or if there is a rebase + repost of the above patchset which 
> does), then I am happy to rebase this one on top.
> 

thanks

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

* Re: [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np
@ 2021-04-16 15:01             ` Alexandre TORGUE
  0 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-16 15:01 UTC (permalink / raw)
  To: Marek Vasut, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32



On 4/16/21 4:54 PM, Marek Vasut wrote:
> On 4/16/21 4:39 PM, Alexandre TORGUE wrote:
>> Hi Marek
> 
> Hello Alexandre,
> 
>> On 4/16/21 3:39 PM, Marek Vasut wrote:
>>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>>> Hi Marek,
>>>
>>> Hello Gabriel,
>>>
>>>> I have a patch-set in progress using $dev (convertion into module 
>>>> driver).
>>>>
>>>> https://patchwork.kernel.org/project/linux-clk/list/?series=421767
>>>>
>>>> Then rebase of your patch, [PATCH 4/7] clk: stm32mp1: Add 
>>>> parent_data to ETHRX clock, can be done easily.
>>>
>>> I suspect the aforementioned patchset will have to be reworked, since 
>>> the current approach to push SCMI onto every system and renumerate 
>>> the clock has been rejected, because it caused DT ABI break and boot 
>>> problems on many systems.
>>
>> SCMI patches for clock drivers will be pushed (and merged one day :)). 
>> We only drop the DT part which will be done through dtbo in 
>> uboot/tfa/optee.
> If the result works for both options (without SCMI and opt-in with 
> SCMI), then that's fine. Does the aforementioned patchset already 
> implement that ?

yes it will work with basic boot. You can have a try with on your DH board.

> 
> If so (or if there is a rebase + repost of the above patchset which 
> does), then I am happy to rebase this one on top.
> 

thanks

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-16 13:47           ` Marek Vasut
@ 2021-04-16 15:23             ` Alexandre TORGUE
  -1 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-16 15:23 UTC (permalink / raw)
  To: Marek Vasut, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd



On 4/16/21 3:47 PM, Marek Vasut wrote:
> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>> Hi Marek
> 
> Hello Gabriel,
> 
>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>>> Hi Marek,
>>>
>>> Hello Gabriel,
>>>
>>>> Thanks for the patchset
>>>>
>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however 
>>>>> per
>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>> Peripheral
>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past 
>>>>> the
>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>> gate.
>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>>>>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>>>>> that, fix it.
>>>>
>>>> I don"t understand, it's already the case.
>>>>
>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>
>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>>> datasheet again and schematic below.
>>>
>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>> (no gate)
>>>
>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>
>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>
>>> Look, this is what you have today:
>>>
>>>             .------------ ETHCK_K -----------.
>>>             |_______               _______   |
>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>             |               |
>>>             |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>>             |                                          |
>>>             '------------ ETHPTP_K --------------------'
>>>
>>> And this is what you should have, to avoid having two composite clock 
>>> which control the same mux using the same register bit, i.e. what 
>>> this patch implements:
>>>
>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>             |_______       |  |    _______   |
>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>                             |
>>>                             '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>>                              |                         |
>>>                              '---- ETHPTP_K -----------'
>>>
>>
>> These 2 solutions are valid. I made the choice to implement the first 
>> one to be able to change parent with the kernel clock of the IP (no 
>> need to add an intermediate binding).
> 
> Which IP are you talking about in here ?
> 
>> It's the same principle for all kernel of this soc.
> 
> The first option is wrong, because in that model, you have two composite 
> clock which control the same one mux bit in the same register. Basically 
> you register two distinct clock which operate the same hardware knob.
> 
>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>> favorable.
> 

The only discussing thing is how the clock is shown. I mean either two 
composites or one mux plus two gates. Gabriel made a choice to abstract 
the mux in two composite clocks. But it seems that at the end we have 
the same behaviour, isn't ?

Adding "ck_ker_eth" would impose a new clock to take in DT ?

regards
alex

> Please ask.
> 
>>>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent 
>>>>> clock
>>>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>>>> ETHPTP_K remain functional as before.
>>>>>
>>>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>>>      Figure 83. Peripheral clock distribution for Ethernet
>>>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>>>
>>>
>>> [...]
>>>
>>>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>>>> index a875649df8b8..a7c7f544ee5d 100644
>>>>> --- a/drivers/clk/clk-stm32mp1.c
>>>>> +++ b/drivers/clk/clk-stm32mp1.c
>>>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>>>> stm32mp1_clock_cfg[] = {
>>>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>>>> stm32mp1_clock_cfg[] = {
>>>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>>             CLK_SET_RATE_NO_REPARENT,
>>>>>             _NO_GATE,
>>>>>             _MMUX(M_ETHCK),
>>>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>>>> +          _NO_DIV),
>>>>> +
>>>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>>>> assigned parent with ETHCK_K will not work
>>>>> +
>>>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>>>
>>>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
>>>
>>> How so ?

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-16 15:23             ` Alexandre TORGUE
  0 siblings, 0 replies; 52+ messages in thread
From: Alexandre TORGUE @ 2021-04-16 15:23 UTC (permalink / raw)
  To: Marek Vasut, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd



On 4/16/21 3:47 PM, Marek Vasut wrote:
> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>> Hi Marek
> 
> Hello Gabriel,
> 
>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>>> Hi Marek,
>>>
>>> Hello Gabriel,
>>>
>>>> Thanks for the patchset
>>>>
>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, however 
>>>>> per
>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>> Peripheral
>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached past 
>>>>> the
>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>> gate.
>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock are
>>>>> in use, ETHCKEN gate can be turned off. Current driver does not permit
>>>>> that, fix it.
>>>>
>>>> I don"t understand, it's already the case.
>>>>
>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>
>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>>> datasheet again and schematic below.
>>>
>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>> (no gate)
>>>
>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>
>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>
>>> Look, this is what you have today:
>>>
>>>             .------------ ETHCK_K -----------.
>>>             |_______               _______   |
>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>             |               |
>>>             |               '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>>             |                                          |
>>>             '------------ ETHPTP_K --------------------'
>>>
>>> And this is what you should have, to avoid having two composite clock 
>>> which control the same mux using the same register bit, i.e. what 
>>> this patch implements:
>>>
>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>             |_______       |  |    _______   |
>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>                             |
>>>                             '--(ETHCKSELR[7:4] divider)--[x] clk_ptp_ref
>>>                              |                         |
>>>                              '---- ETHPTP_K -----------'
>>>
>>
>> These 2 solutions are valid. I made the choice to implement the first 
>> one to be able to change parent with the kernel clock of the IP (no 
>> need to add an intermediate binding).
> 
> Which IP are you talking about in here ?
> 
>> It's the same principle for all kernel of this soc.
> 
> The first option is wrong, because in that model, you have two composite 
> clock which control the same one mux bit in the same register. Basically 
> you register two distinct clock which operate the same hardware knob.
> 
>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>> favorable.
> 

The only discussing thing is how the clock is shown. I mean either two 
composites or one mux plus two gates. Gabriel made a choice to abstract 
the mux in two composite clocks. But it seems that at the end we have 
the same behaviour, isn't ?

Adding "ck_ker_eth" would impose a new clock to take in DT ?

regards
alex

> Please ask.
> 
>>>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent 
>>>>> clock
>>>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>>>> ETHPTP_K remain functional as before.
>>>>>
>>>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>>>      Figure 83. Peripheral clock distribution for Ethernet
>>>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>>>
>>>
>>> [...]
>>>
>>>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>>>> index a875649df8b8..a7c7f544ee5d 100644
>>>>> --- a/drivers/clk/clk-stm32mp1.c
>>>>> +++ b/drivers/clk/clk-stm32mp1.c
>>>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>>>> stm32mp1_clock_cfg[] = {
>>>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>>>> stm32mp1_clock_cfg[] = {
>>>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>>             CLK_SET_RATE_NO_REPARENT,
>>>>>             _NO_GATE,
>>>>>             _MMUX(M_ETHCK),
>>>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>>>> +          _NO_DIV),
>>>>> +
>>>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>>>> assigned parent with ETHCK_K will not work
>>>>> +
>>>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>>>
>>>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
>>>
>>> How so ?

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-16 15:23             ` Alexandre TORGUE
@ 2021-04-16 15:31               ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 15:31 UTC (permalink / raw)
  To: Alexandre TORGUE, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd

On 4/16/21 5:23 PM, Alexandre TORGUE wrote:

Hello Alexandre,

> On 4/16/21 3:47 PM, Marek Vasut wrote:
>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>> Hi Marek
>>
>> Hello Gabriel,
>>
>>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>>>> Hi Marek,
>>>>
>>>> Hello Gabriel,
>>>>
>>>>> Thanks for the patchset
>>>>>
>>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, 
>>>>>> however per
>>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>>> Peripheral
>>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached 
>>>>>> past the
>>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>>> gate.
>>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock 
>>>>>> are
>>>>>> in use, ETHCKEN gate can be turned off. Current driver does not 
>>>>>> permit
>>>>>> that, fix it.
>>>>>
>>>>> I don"t understand, it's already the case.
>>>>>
>>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>>
>>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>>>> datasheet again and schematic below.
>>>>
>>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>>> (no gate)
>>>>
>>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>>
>>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>>
>>>> Look, this is what you have today:
>>>>
>>>>             .------------ ETHCK_K -----------.
>>>>             |_______               _______   |
>>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>             |               |
>>>>             |               '--(ETHCKSELR[7:4] divider)--[x] 
>>>> clk_ptp_ref
>>>>             |                                          |
>>>>             '------------ ETHPTP_K --------------------'
>>>>
>>>> And this is what you should have, to avoid having two composite 
>>>> clock which control the same mux using the same register bit, i.e. 
>>>> what this patch implements:
>>>>
>>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>>             |_______       |  |    _______   |
>>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>                             |
>>>>                             '--(ETHCKSELR[7:4] divider)--[x] 
>>>> clk_ptp_ref
>>>>                              |                         |
>>>>                              '---- ETHPTP_K -----------'
>>>>
>>>
>>> These 2 solutions are valid. I made the choice to implement the first 
>>> one to be able to change parent with the kernel clock of the IP (no 
>>> need to add an intermediate binding).
>>
>> Which IP are you talking about in here ?
>>
>>> It's the same principle for all kernel of this soc.
>>
>> The first option is wrong, because in that model, you have two 
>> composite clock which control the same one mux bit in the same 
>> register. Basically you register two distinct clock which operate the 
>> same hardware knob.
>>
>>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>>> favorable.
>>
> 
> The only discussing thing is how the clock is shown. I mean either two 
> composites or one mux plus two gates. Gabriel made a choice to abstract 
> the mux in two composite clocks. But it seems that at the end we have 
> the same behaviour, isn't ?

Not really. Since the two composite clock control the same mux bit, 
consider what would happen if you were to select pll4_p_ck as parent for 
one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. 
ETHPTP_K), what would be the result ? I guess the result would depend on 
when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I 
don't think that's how it should work. With a single mux controlling 
that one single bit, such situation wouldn't happen.

> Adding "ck_ker_eth" would impose a new clock to take in DT ?
Nope, the ck_ker_eth is without ID and internal to the driver. They 
exist only to describe the clock tree correctly.

[...]

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-16 15:31               ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2021-04-16 15:31 UTC (permalink / raw)
  To: Alexandre TORGUE, gabriel.fernandez, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd

On 4/16/21 5:23 PM, Alexandre TORGUE wrote:

Hello Alexandre,

> On 4/16/21 3:47 PM, Marek Vasut wrote:
>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>> Hi Marek
>>
>> Hello Gabriel,
>>
>>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>>>> Hi Marek,
>>>>
>>>> Hello Gabriel,
>>>>
>>>>> Thanks for the patchset
>>>>>
>>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, 
>>>>>> however per
>>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>>> Peripheral
>>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached 
>>>>>> past the
>>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>>> gate.
>>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock 
>>>>>> are
>>>>>> in use, ETHCKEN gate can be turned off. Current driver does not 
>>>>>> permit
>>>>>> that, fix it.
>>>>>
>>>>> I don"t understand, it's already the case.
>>>>>
>>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>>
>>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>>>> datasheet again and schematic below.
>>>>
>>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>>> (no gate)
>>>>
>>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>>
>>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>>
>>>> Look, this is what you have today:
>>>>
>>>>             .------------ ETHCK_K -----------.
>>>>             |_______               _______   |
>>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>             |               |
>>>>             |               '--(ETHCKSELR[7:4] divider)--[x] 
>>>> clk_ptp_ref
>>>>             |                                          |
>>>>             '------------ ETHPTP_K --------------------'
>>>>
>>>> And this is what you should have, to avoid having two composite 
>>>> clock which control the same mux using the same register bit, i.e. 
>>>> what this patch implements:
>>>>
>>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>>             |_______       |  |    _______   |
>>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>                             |
>>>>                             '--(ETHCKSELR[7:4] divider)--[x] 
>>>> clk_ptp_ref
>>>>                              |                         |
>>>>                              '---- ETHPTP_K -----------'
>>>>
>>>
>>> These 2 solutions are valid. I made the choice to implement the first 
>>> one to be able to change parent with the kernel clock of the IP (no 
>>> need to add an intermediate binding).
>>
>> Which IP are you talking about in here ?
>>
>>> It's the same principle for all kernel of this soc.
>>
>> The first option is wrong, because in that model, you have two 
>> composite clock which control the same one mux bit in the same 
>> register. Basically you register two distinct clock which operate the 
>> same hardware knob.
>>
>>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>>> favorable.
>>
> 
> The only discussing thing is how the clock is shown. I mean either two 
> composites or one mux plus two gates. Gabriel made a choice to abstract 
> the mux in two composite clocks. But it seems that at the end we have 
> the same behaviour, isn't ?

Not really. Since the two composite clock control the same mux bit, 
consider what would happen if you were to select pll4_p_ck as parent for 
one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. 
ETHPTP_K), what would be the result ? I guess the result would depend on 
when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I 
don't think that's how it should work. With a single mux controlling 
that one single bit, such situation wouldn't happen.

> Adding "ck_ker_eth" would impose a new clock to take in DT ?
Nope, the ck_ker_eth is without ID and internal to the driver. They 
exist only to describe the clock tree correctly.

[...]

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-16 15:31               ` Marek Vasut
@ 2021-04-19  7:46                 ` gabriel.fernandez
  -1 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-19  7:46 UTC (permalink / raw)
  To: Marek Vasut, Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd

Hi Marek,

On 4/16/21 5:31 PM, Marek Vasut wrote:
> On 4/16/21 5:23 PM, Alexandre TORGUE wrote:
> 
> Hello Alexandre,
> 
>> On 4/16/21 3:47 PM, Marek Vasut wrote:
>>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>>> Hi Marek
>>>
>>> Hello Gabriel,
>>>
>>>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hello Gabriel,
>>>>>
>>>>>> Thanks for the patchset
>>>>>>
>>>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, 
>>>>>>> however per
>>>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>>>> Peripheral
>>>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached 
>>>>>>> past the
>>>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>>>> gate.
>>>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP 
>>>>>>> clock are
>>>>>>> in use, ETHCKEN gate can be turned off. Current driver does not 
>>>>>>> permit
>>>>>>> that, fix it.
>>>>>>
>>>>>> I don"t understand, it's already the case.
>>>>>>
>>>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>>>
>>>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in 
>>>>> the datasheet again and schematic below.
>>>>>
>>>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>>>> (no gate)
>>>>>
>>>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>>>
>>>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>>>
>>>>> Look, this is what you have today:
>>>>>
>>>>>             .------------ ETHCK_K -----------.
>>>>>             |_______               _______   |
>>>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>>             |               |
>>>>>             |               '--(ETHCKSELR[7:4] divider)--[x] 
>>>>> clk_ptp_ref
>>>>>             |                                          |
>>>>>             '------------ ETHPTP_K --------------------'
>>>>>
>>>>> And this is what you should have, to avoid having two composite 
>>>>> clock which control the same mux using the same register bit, i.e. 
>>>>> what this patch implements:
>>>>>
>>>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>>>             |_______       |  |    _______   |
>>>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>>                             |
>>>>>                             '--(ETHCKSELR[7:4] divider)--[x] 
>>>>> clk_ptp_ref
>>>>>                              |                         |
>>>>>                              '---- ETHPTP_K -----------'
>>>>>
>>>>
>>>> These 2 solutions are valid. I made the choice to implement the 
>>>> first one to be able to change parent with the kernel clock of the 
>>>> IP (no need to add an intermediate binding).
>>>
>>> Which IP are you talking about in here ?
>>>
>>>> It's the same principle for all kernel of this soc.
>>>
>>> The first option is wrong, because in that model, you have two 
>>> composite clock which control the same one mux bit in the same 
>>> register. Basically you register two distinct clock which operate the 
>>> same hardware knob.
>>>
>>>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>>>> favorable.
>>>
>>
>> The only discussing thing is how the clock is shown. I mean either two 
>> composites or one mux plus two gates. Gabriel made a choice to 
>> abstract the mux in two composite clocks. But it seems that at the end 
>> we have the same behaviour, isn't ?
> 
> Not really. Since the two composite clock control the same mux bit, 
> consider what would happen if you were to select pll4_p_ck as parent for 
> one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. 
> ETHPTP_K), what would be the result ? I guess the result would depend on 
> when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I 
> don't think that's how it should work. With a single mux controlling 
> that one single bit, such situation wouldn't happen.

The reparenting is managed. This mux has specific ops.
root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll4_p
pll4_p
root@stm32mp1-disco-oss:~# echo pll3_q > 
/sys/kernel/debug/clk/ethptp_k/clk_set_parent
root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll3_q
pll3_q

> 
>> Adding "ck_ker_eth" would impose a new clock to take in DT ?
> Nope, the ck_ker_eth is without ID and internal to the driver. They 
> exist only to describe the clock tree correctly.
> 
> [...]

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2021-04-19  7:46                 ` gabriel.fernandez
  0 siblings, 0 replies; 52+ messages in thread
From: gabriel.fernandez @ 2021-04-19  7:46 UTC (permalink / raw)
  To: Marek Vasut, Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd

Hi Marek,

On 4/16/21 5:31 PM, Marek Vasut wrote:
> On 4/16/21 5:23 PM, Alexandre TORGUE wrote:
> 
> Hello Alexandre,
> 
>> On 4/16/21 3:47 PM, Marek Vasut wrote:
>>> On 4/16/21 8:44 AM, gabriel.fernandez@foss.st.com wrote:
>>>> Hi Marek
>>>
>>> Hello Gabriel,
>>>
>>>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>>>> On 4/14/21 3:03 PM, gabriel.fernandez@foss.st.com wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hello Gabriel,
>>>>>
>>>>>> Thanks for the patchset
>>>>>>
>>>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, 
>>>>>>> however per
>>>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>>>> Peripheral
>>>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached 
>>>>>>> past the
>>>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>>>> gate.
>>>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP 
>>>>>>> clock are
>>>>>>> in use, ETHCKEN gate can be turned off. Current driver does not 
>>>>>>> permit
>>>>>>> that, fix it.
>>>>>>
>>>>>> I don"t understand, it's already the case.
>>>>>>
>>>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>>>
>>>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in 
>>>>> the datasheet again and schematic below.
>>>>>
>>>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>>>> (no gate)
>>>>>
>>>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>>>
>>>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>>>
>>>>> Look, this is what you have today:
>>>>>
>>>>>             .------------ ETHCK_K -----------.
>>>>>             |_______               _______   |
>>>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>>             |               |
>>>>>             |               '--(ETHCKSELR[7:4] divider)--[x] 
>>>>> clk_ptp_ref
>>>>>             |                                          |
>>>>>             '------------ ETHPTP_K --------------------'
>>>>>
>>>>> And this is what you should have, to avoid having two composite 
>>>>> clock which control the same mux using the same register bit, i.e. 
>>>>> what this patch implements:
>>>>>
>>>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>>>             |_______       |  |    _______   |
>>>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>>                             |
>>>>>                             '--(ETHCKSELR[7:4] divider)--[x] 
>>>>> clk_ptp_ref
>>>>>                              |                         |
>>>>>                              '---- ETHPTP_K -----------'
>>>>>
>>>>
>>>> These 2 solutions are valid. I made the choice to implement the 
>>>> first one to be able to change parent with the kernel clock of the 
>>>> IP (no need to add an intermediate binding).
>>>
>>> Which IP are you talking about in here ?
>>>
>>>> It's the same principle for all kernel of this soc.
>>>
>>> The first option is wrong, because in that model, you have two 
>>> composite clock which control the same one mux bit in the same 
>>> register. Basically you register two distinct clock which operate the 
>>> same hardware knob.
>>>
>>>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>>>> favorable.
>>>
>>
>> The only discussing thing is how the clock is shown. I mean either two 
>> composites or one mux plus two gates. Gabriel made a choice to 
>> abstract the mux in two composite clocks. But it seems that at the end 
>> we have the same behaviour, isn't ?
> 
> Not really. Since the two composite clock control the same mux bit, 
> consider what would happen if you were to select pll4_p_ck as parent for 
> one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. 
> ETHPTP_K), what would be the result ? I guess the result would depend on 
> when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I 
> don't think that's how it should work. With a single mux controlling 
> that one single bit, such situation wouldn't happen.

The reparenting is managed. This mux has specific ops.
root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll4_p
pll4_p
root@stm32mp1-disco-oss:~# echo pll3_q > 
/sys/kernel/debug/clk/ethptp_k/clk_set_parent
root@stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll3_q
pll3_q

> 
>> Adding "ck_ker_eth" would impose a new clock to take in DT ?
> Nope, the ck_ker_eth is without ID and internal to the driver. They 
> exist only to describe the clock tree correctly.
> 
> [...]

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

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
  2021-04-19  7:46                 ` gabriel.fernandez
@ 2022-01-18 22:11                   ` Marek Vasut
  -1 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2022-01-18 22:11 UTC (permalink / raw)
  To: gabriel.fernandez, Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd

On 4/19/21 09:46, gabriel.fernandez@foss.st.com wrote:

Hello again,

[...]

I sent out an rebased (and much shorter) patch series now:

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=606380

-- 
Best regards,
Marek Vasut

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

* Re: [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock
@ 2022-01-18 22:11                   ` Marek Vasut
  0 siblings, 0 replies; 52+ messages in thread
From: Marek Vasut @ 2022-01-18 22:11 UTC (permalink / raw)
  To: gabriel.fernandez, Alexandre TORGUE, linux-arm-kernel
  Cc: Christophe Roullier, Patrice Chotard, Patrick Delaunay,
	Stephen Boyd, linux-clk, linux-stm32, Stephen Boyd

On 4/19/21 09:46, gabriel.fernandez@foss.st.com wrote:

Hello again,

[...]

I sent out an rebased (and much shorter) patch series now:

https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=606380

-- 
Best regards,
Marek Vasut

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

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

end of thread, other threads:[~2022-01-18 22:12 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08 18:57 [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM Marek Vasut
2021-04-08 18:57 ` Marek Vasut
2021-04-08 18:57 ` [PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-14 13:03   ` gabriel.fernandez
2021-04-14 13:03     ` gabriel.fernandez
2021-04-14 14:04     ` Marek Vasut
2021-04-14 14:04       ` Marek Vasut
2021-04-16  6:44       ` gabriel.fernandez
2021-04-16  6:44         ` gabriel.fernandez
2021-04-16 13:47         ` Marek Vasut
2021-04-16 13:47           ` Marek Vasut
2021-04-16 15:23           ` Alexandre TORGUE
2021-04-16 15:23             ` Alexandre TORGUE
2021-04-16 15:31             ` Marek Vasut
2021-04-16 15:31               ` Marek Vasut
2021-04-19  7:46               ` gabriel.fernandez
2021-04-19  7:46                 ` gabriel.fernandez
2022-01-18 22:11                 ` Marek Vasut
2022-01-18 22:11                   ` Marek Vasut
2021-04-08 18:57 ` [PATCH 2/7] clk: stm32mp1: The dev is always NULL, replace it with np Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-16  6:44   ` gabriel.fernandez
2021-04-16  6:44     ` gabriel.fernandez
2021-04-16 13:39     ` Marek Vasut
2021-04-16 13:39       ` Marek Vasut
2021-04-16 14:39       ` Alexandre TORGUE
2021-04-16 14:39         ` Alexandre TORGUE
2021-04-16 14:54         ` Marek Vasut
2021-04-16 14:54           ` Marek Vasut
2021-04-16 15:01           ` Alexandre TORGUE
2021-04-16 15:01             ` Alexandre TORGUE
2021-04-08 18:57 ` [PATCH 3/7] clk: stm32mp1: Register clock with device_node pointer Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-08 18:57 ` [PATCH 4/7] clk: stm32mp1: Add parent_data to ETHRX clock Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-08 18:57 ` [PATCH 5/7] ARM: dts: stm32: Add alternate pinmux for ethernet0 pins Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-08 18:57 ` [PATCH 6/7] ARM: dts: stm32: Add alternate pinmux for mco2 pins Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-08 18:57 ` [PATCH 7/7] ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM Marek Vasut
2021-04-08 18:57   ` Marek Vasut
2021-04-08 20:32 ` [PATCH 0/7] ARM: dts: stm32: clk: Switch ETHRX clock parent from ETHCK_K to MCO2 on DHCOM SoM Stephen Boyd
2021-04-08 20:32   ` Stephen Boyd
2021-04-12  8:09 ` Alexandre TORGUE
2021-04-12  8:09   ` Alexandre TORGUE
2021-04-12 18:44   ` Marek Vasut
2021-04-12 18:44     ` Marek Vasut
2021-04-13  7:48     ` Alexandre TORGUE
2021-04-13  7:48       ` Alexandre TORGUE
2021-04-13 12:05       ` Marek Vasut
2021-04-13 12:05         ` Marek Vasut

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.