[PATCH 1/4] ARM: dts: stm32: Add alternate pinmux for ethernet0 pins

[PATCH 1/4] ARM: dts: stm32: Add alternate pinmux for ethernet0 pins

[Marek Vasut]

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@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
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 769d30593e66..fe0cc9c5991e 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -342,6 +342,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.29.2


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

[PATCH 2/4] ARM: dts: stm32: Add alternate pinmux for mco2 pins

[Marek Vasut]

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@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
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 fe0cc9c5991e..d8297dfff3e6 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -886,6 +886,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.29.2


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

[PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently does not
permit selecting the clock input from SoC pad. To make things worse, the
implementation of this is partly present and is split between the clock
driver and dwmac4 driver. Moreover, the ETHRX clock parent is incorrect.

First, the ETHRX clock in clk-stm32mp1.c only represents the ETHRXEN gate,
however it should represent also ETH_REF_CLK_SEL mux. The problem is that
the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 driver and
the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or the
clock block.

Second, the ETHRX parent clock is either eth_clk_fb (ETHCK_K) or external
ETH_RX_CLK/ETH_REF_CLK_SEL, it is never CK_AXI.

This patch attempts to address the clock selection by adding fixed factor
clock to DT, which allows the user to select its upstream clock.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp151.dtsi | 8 ++++++++
 drivers/clk/clk-stm32mp1.c        | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stm32mp151.dtsi b/arch/arm/boot/dts/stm32mp151.dtsi
index 32875eabd357..8c2a5d0875d8 100644
--- a/arch/arm/boot/dts/stm32mp151.dtsi
+++ b/arch/arm/boot/dts/stm32mp151.dtsi
@@ -82,6 +82,14 @@ clk_csi: clk-csi {
 			compatible = "fixed-clock";
 			clock-frequency = <4000000>;
 		};
+
+		clk_eth_rx: eth-rx-clk {
+			compatible = "fixed-factor-clock";
+			clocks = <&rcc ETHCK_K>;
+			#clock-cells = <0>;
+			clock-div = <1>;
+			clock-mult = <1>;
+		};
 	};
 
 	thermal-zones {
diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
index a875649df8b8..63971d40f15c 100644
--- a/drivers/clk/clk-stm32mp1.c
+++ b/drivers/clk/clk-stm32mp1.c
@@ -1892,7 +1892,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(ETHRX, "ethrx", "eth-rx-clk", 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.29.2


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

[PATCH 4/4] [RFC] ARM: dts: stm32: Switch DWMAC RMII clock to MCO2 on DHCOM

[Marek Vasut]

Using the MCO2 output for the LAN8710i PHY clock and feedback clock
into the DWMAC reduces EMI, switch to MCO2.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
index dc70ddd09e9d..ff70bd03a017 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi
@@ -110,15 +110,21 @@ &dts {
 	status = "okay";
 };
 
+&clk_eth_rx {
+	clocks = <&rcc CK_MCO2>;
+	assigned-clocks = <&rcc CK_MCO2>;
+	assigned-clock-parents = <&rcc PLL4_P>;
+	assigned-clock-rates = <50000000>;
+};
+
 &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.29.2


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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

Hi Marek

On 1/6/21 9:43 PM, Marek Vasut wrote:
> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently does not
> permit selecting the clock input from SoC pad. To make things worse, the
> implementation of this is partly present and is split between the clock
> driver and dwmac4 driver. Moreover, the ETHRX clock parent is incorrect.

Sorry but I don't understand which configuration is missing. I think we 
can handle all possible cases for RMII. At the glue layer 
(dwmac-stm32.c) clocks gates and syscfg are set regarding device tree 
binding (see the tab in dwmac-stm32.c). You could have a look here for 
more details: 
https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration

Regarding the clock parent, yes it is not at the well frequency if you 
want to select this path. Our current "clock tree" is done to fit with 
our ST reference boards (we have more peripherals than PLL outputs so we 
have to make choices). So yes for customer/partners boards this clock 
tree has to be modified to better fit with the need (either using 
assigned-clock-parent or by modifying bootloader clock tree (tf-a or 
u-boot)).

> 
> First, the ETHRX clock in clk-stm32mp1.c only represents the ETHRXEN gate,
> however it should represent also ETH_REF_CLK_SEL mux. The problem is that
> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 driver and
> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or the
> clock block.

dwmac4-stm32 doesn't contain code for dwmac4 but it contains the glue 
around the dwmac4: syscfg, clocks ...

> 
> Second, the ETHRX parent clock is either eth_clk_fb (ETHCK_K) or external
> ETH_RX_CLK/ETH_REF_CLK_SEL, it is never CK_AXI.

Why CK_AXI ?

Regards
Alex

> 
> This patch attempts to address the clock selection by adding fixed factor
> clock to DT, which allows the user to select its upstream clock.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-arm-kernel@lists.infradead.org
> ---
>   arch/arm/boot/dts/stm32mp151.dtsi | 8 ++++++++
>   drivers/clk/clk-stm32mp1.c        | 2 +-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/stm32mp151.dtsi b/arch/arm/boot/dts/stm32mp151.dtsi
> index 32875eabd357..8c2a5d0875d8 100644
> --- a/arch/arm/boot/dts/stm32mp151.dtsi
> +++ b/arch/arm/boot/dts/stm32mp151.dtsi
> @@ -82,6 +82,14 @@ clk_csi: clk-csi {
>   			compatible = "fixed-clock";
>   			clock-frequency = <4000000>;
>   		};
> +
> +		clk_eth_rx: eth-rx-clk {
> +			compatible = "fixed-factor-clock";
> +			clocks = <&rcc ETHCK_K>;
> +			#clock-cells = <0>;
> +			clock-div = <1>;
> +			clock-mult = <1>;
> +		};
>   	};
>   
>   	thermal-zones {
> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
> index a875649df8b8..63971d40f15c 100644
> --- a/drivers/clk/clk-stm32mp1.c
> +++ b/drivers/clk/clk-stm32mp1.c
> @@ -1892,7 +1892,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(ETHRX, "ethrx", "eth-rx-clk", 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),
> 

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/14/21 6:08 PM, Alexandre TORGUE wrote:
> Hi Marek

Hi,

> On 1/6/21 9:43 PM, Marek Vasut wrote:
>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently does not
>> permit selecting the clock input from SoC pad. To make things worse, the
>> implementation of this is partly present and is split between the clock
>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is incorrect.
> 
> Sorry but I don't understand which configuration is missing. I think we 
> can handle all possible cases for RMII. At the glue layer 
> (dwmac-stm32.c) clocks gates and syscfg are set regarding device tree 
> binding (see the tab in dwmac-stm32.c). You could have a look here for 
> more details: 
> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
> 
> Regarding the clock parent, yes it is not at the well frequency if you 
> want to select this path. Our current "clock tree" is done to fit with 
> our ST reference boards (we have more peripherals than PLL outputs so we 
> have to make choices). So yes for customer/partners boards this clock 
> tree has to be modified to better fit with the need (either using 
> assigned-clock-parent or by modifying bootloader clock tree (tf-a or 
> u-boot)).

I don't think you handle all the configuration options, but I might also 
be confused.

See Figure 83. Peripheral clock distribution for Ethernet in the MP1 
datasheet for the below.

The current setup I have needs 50 MHz on SoC pad PA1 to drive the PHY 
clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 50 MHz is 
sourced directly from PLL4P, which then has to run at 50 MHz and that in 
turn reduces clock frequency for other blocks connected to PLL4P (e.g. 
SDMMC, where the impact is noticable).

So, what I want to model here is this:

PLL4P = 100 MHz
MCO2 is supplied by PLL4P and set to /2 , so MCO2 = 50 MHz
SoC pad PG2 is set as MCO2 output, thus a source of 50 MHz signal
SoC pad PA1 is set as ETH_RX_CLK and connected to PG2

This works fine in practice, except it cannot be modeled using current 
DT bindings, even though it should be possible to model it.

>> First, the ETHRX clock in clk-stm32mp1.c only represents the ETHRXEN 
>> gate,
>> however it should represent also ETH_REF_CLK_SEL mux. The problem is that
>> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 driver and
>> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or the
>> clock block.
> 
> dwmac4-stm32 doesn't contain code for dwmac4 but it contains the glue 
> around the dwmac4: syscfg, clocks ...

The problem is that dwmac4-stm32 isn't the right place to configure the 
ETHRX clock mux, that should be in the clock driver. So the stm32 clock 
driver should have SYSCFG handle and configure ETH_REF_CLK_SEL mux. The 
"st,eth-ref-clk-sel" DT prop would then not be needed at all, as the 
reference clock select would be configured using assigned-clocks in DT.

The default assigned-clocks should be eth_clk_fb , but the user can 
override it in the DT and provide another clock source (e.g. in my case, 
that would be PLL4P->MCO2->ETHRX).

>> Second, the ETHRX parent clock is either eth_clk_fb (ETHCK_K) or external
>> ETH_RX_CLK/ETH_REF_CLK_SEL, it is never CK_AXI.
> 
> Why CK_AXI ?

See drivers/clk/clk-stm32mp1.c:
   1895          PCLK(ETHRX, "ethrx", "ck_axi", 0, G_ETHRX),

[...]

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/15/21 1:15 PM, Marek Vasut wrote:
> On 1/14/21 6:08 PM, Alexandre TORGUE wrote:
>> Hi Marek
> 
> Hi,
> 
>> On 1/6/21 9:43 PM, Marek Vasut wrote:
>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently does not
>>> permit selecting the clock input from SoC pad. To make things worse, the
>>> implementation of this is partly present and is split between the clock
>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is incorrect.
>>
>> Sorry but I don't understand which configuration is missing. I think 
>> we can handle all possible cases for RMII. At the glue layer 
>> (dwmac-stm32.c) clocks gates and syscfg are set regarding device tree 
>> binding (see the tab in dwmac-stm32.c). You could have a look here for 
>> more details: 
>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
>>
>> Regarding the clock parent, yes it is not at the well frequency if you 
>> want to select this path. Our current "clock tree" is done to fit with 
>> our ST reference boards (we have more peripherals than PLL outputs so 
>> we have to make choices). So yes for customer/partners boards this 
>> clock tree has to be modified to better fit with the need (either 
>> using assigned-clock-parent or by modifying bootloader clock tree 
>> (tf-a or u-boot)).
> 
> I don't think you handle all the configuration options, but I might also 
> be confused.
> 
> See Figure 83. Peripheral clock distribution for Ethernet in the MP1 
> datasheet for the below.
> 
> The current setup I have needs 50 MHz on SoC pad PA1 to drive the PHY 
> clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 50 MHz is 
> sourced directly from PLL4P, which then has to run at 50 MHz and that in 
> turn reduces clock frequency for other blocks connected to PLL4P (e.g. 
> SDMMC, where the impact is noticable).

Ok that's the common path to clock a PHY a 50MHz without using the 
ref_clk coming from the PHY. And yes I can understand that the drawback 
is huge).

> 
> So, what I want to model here is this:
> 
> PLL4P = 100 MHz
> MCO2 is supplied by PLL4P and set to /2 , so MCO2 = 50 MHz
> SoC pad PG2 is set as MCO2 output, thus a source of 50 MHz signal
> SoC pad PA1 is set as ETH_RX_CLK and connected to PG2

Ok I see (to be honest IIWR we didn't test i :$) but it should work.

> This works fine in practice, except it cannot be modeled using current 
> DT bindings, even though it should be possible to model it.

For dwmac point of view it's quite the same thing to have your PHY 
clocking by MCO or by a crystal. You just need to configure RX_REF pad 
and ETH_CLK_SEL to get the 50 MHz RMII reference clock.

> 
>>> First, the ETHRX clock in clk-stm32mp1.c only represents the ETHRXEN 
>>> gate,
>>> however it should represent also ETH_REF_CLK_SEL mux. The problem is 
>>> that
>>> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 driver and
>>> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or the
>>> clock block.
>>
>> dwmac4-stm32 doesn't contain code for dwmac4 but it contains the glue 
>> around the dwmac4: syscfg, clocks ...
> 
> The problem is that dwmac4-stm32 isn't the right place to configure the 
> ETHRX clock mux, that should be in the clock driver. So the stm32 clock 
> driver should have SYSCFG handle and configure ETH_REF_CLK_SEL mux. The 
> "st,eth-ref-clk-sel" DT prop would then not be needed at all, as the 
> reference clock select would be configured using assigned-clocks in DT.

Idea was to keep at the same place the Ethernet glue configuration. We 
can't move all this glue into clock driver as phy interface is needed to 
well configure some sysconf registers. Current dwamc-stm32 glue is 
working and documented. I'm not convinced to develop a new one by 
splitting clock sysconf in clock driver and phy interface management at 
ethernet level. I think we will get the same functional result (but yes 
maybe more understandable at dt-bindings level). We could maybe update 
binding name to be more clear.

> 
> The default assigned-clocks should be eth_clk_fb , but the user can 
> override it in the DT and provide another clock source (e.g. in my case, 
> that would be PLL4P->MCO2->ETHRX).
> 
>>> Second, the ETHRX parent clock is either eth_clk_fb (ETHCK_K) or 
>>> external
>>> ETH_RX_CLK/ETH_REF_CLK_SEL, it is never CK_AXI.
>>
>> Why CK_AXI ?
> 
> See drivers/clk/clk-stm32mp1.c:
>    1895          PCLK(ETHRX, "ethrx", "ck_axi", 0, G_ETHRX),
>

Ok I see, and it is the same case for TX also. Discussing with our clock 
expert it was done for simplification.

regards
Alex


> [...]

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/15/21 4:22 PM, Alexandre TORGUE wrote:

Hi,

[...]

>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently does 
>>>> not
>>>> permit selecting the clock input from SoC pad. To make things worse, 
>>>> the
>>>> implementation of this is partly present and is split between the clock
>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>> incorrect.
>>>
>>> Sorry but I don't understand which configuration is missing. I think 
>>> we can handle all possible cases for RMII. At the glue layer 
>>> (dwmac-stm32.c) clocks gates and syscfg are set regarding device tree 
>>> binding (see the tab in dwmac-stm32.c). You could have a look here 
>>> for more details: 
>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
>>>
>>> Regarding the clock parent, yes it is not at the well frequency if 
>>> you want to select this path. Our current "clock tree" is done to fit 
>>> with our ST reference boards (we have more peripherals than PLL 
>>> outputs so we have to make choices). So yes for customer/partners 
>>> boards this clock tree has to be modified to better fit with the need 
>>> (either using assigned-clock-parent or by modifying bootloader clock 
>>> tree (tf-a or u-boot)).
>>
>> I don't think you handle all the configuration options, but I might 
>> also be confused.
>>
>> See Figure 83. Peripheral clock distribution for Ethernet in the MP1 
>> datasheet for the below.
>>
>> The current setup I have needs 50 MHz on SoC pad PA1 to drive the PHY 
>> clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 50 MHz 
>> is sourced directly from PLL4P, which then has to run at 50 MHz and 
>> that in turn reduces clock frequency for other blocks connected to 
>> PLL4P (e.g. SDMMC, where the impact is noticable).
> 
> Ok that's the common path to clock a PHY a 50MHz without using the 
> ref_clk coming from the PHY. And yes I can understand that the drawback 
> is huge).

So lets fix it.

>> So, what I want to model here is this:
>>
>> PLL4P = 100 MHz
>> MCO2 is supplied by PLL4P and set to /2 , so MCO2 = 50 MHz
>> SoC pad PG2 is set as MCO2 output, thus a source of 50 MHz signal
>> SoC pad PA1 is set as ETH_RX_CLK and connected to PG2
> 
> Ok I see (to be honest IIWR we didn't test i :$) but it should work.

It does work, I have boards which use this setup already.

>> This works fine in practice, except it cannot be modeled using current 
>> DT bindings, even though it should be possible to model it.
> 
> For dwmac point of view it's quite the same thing to have your PHY 
> clocking by MCO or by a crystal. You just need to configure RX_REF pad 
> and ETH_CLK_SEL to get the 50 MHz RMII reference clock.

Yes

>>>> First, the ETHRX clock in clk-stm32mp1.c only represents the ETHRXEN 
>>>> gate,
>>>> however it should represent also ETH_REF_CLK_SEL mux. The problem is 
>>>> that
>>>> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 driver 
>>>> and
>>>> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or the
>>>> clock block.
>>>
>>> dwmac4-stm32 doesn't contain code for dwmac4 but it contains the glue 
>>> around the dwmac4: syscfg, clocks ...
>>
>> The problem is that dwmac4-stm32 isn't the right place to configure 
>> the ETHRX clock mux, that should be in the clock driver. So the stm32 
>> clock driver should have SYSCFG handle and configure ETH_REF_CLK_SEL 
>> mux. The "st,eth-ref-clk-sel" DT prop would then not be needed at all, 
>> as the reference clock select would be configured using 
>> assigned-clocks in DT.
> 
> Idea was to keep at the same place the Ethernet glue configuration. We 
> can't move all this glue into clock driver as phy interface is needed to 
> well configure some sysconf registers.

This configuration can be done by the clock driver too. And in fact, I 
believe it should be done by the clock driver, just like it's done for 
all the other clock muxes with gates in the clock driver, except in this 
case the mux is in syscfg and gate is in rcc.

> Current dwamc-stm32 glue is 
> working and documented. I'm not convinced to develop a new one by 
> splitting clock sysconf in clock driver and phy interface management at 
> ethernet level. I think we will get the same functional result (but yes 
> maybe more understandable at dt-bindings level). We could maybe update 
> binding name to be more clear.

You don't get the same result, since you cannot model the MCO2 input 
into ETHRX. Or can you ?

I also think that we won't need new binding altogether, just a slight 
tweak to the existing ones which would permit modeling the MCO2 input 
into ETHRX, I am open to suggestions how to do it. Note that the clock 
framework must be able to turn off both ETHRX gate, MCO2 and all the way 
up the tree if ETHRX is turned off.

>> The default assigned-clocks should be eth_clk_fb , but the user can 
>> override it in the DT and provide another clock source (e.g. in my 
>> case, that would be PLL4P->MCO2->ETHRX).
>>
>>>> Second, the ETHRX parent clock is either eth_clk_fb (ETHCK_K) or 
>>>> external
>>>> ETH_RX_CLK/ETH_REF_CLK_SEL, it is never CK_AXI.
>>>
>>> Why CK_AXI ?
>>
>> See drivers/clk/clk-stm32mp1.c:
>>    1895          PCLK(ETHRX, "ethrx", "ck_axi", 0, G_ETHRX),
>>
> 
> Ok I see, and it is the same case for TX also. Discussing with our clock 
> expert it was done for simplification.

So, how shall we proceed here ?

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/16/21 6:01 PM, Marek Vasut wrote:
> On 1/15/21 4:22 PM, Alexandre TORGUE wrote:
> 
> Hi,
> 
> [...]
> 
>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently 
>>>>> does not
>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>> worse, the
>>>>> implementation of this is partly present and is split between the 
>>>>> clock
>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>> incorrect.
>>>>
>>>> Sorry but I don't understand which configuration is missing. I think 
>>>> we can handle all possible cases for RMII. At the glue layer 
>>>> (dwmac-stm32.c) clocks gates and syscfg are set regarding device 
>>>> tree binding (see the tab in dwmac-stm32.c). You could have a look 
>>>> here for more details: 
>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
>>>>
>>>> Regarding the clock parent, yes it is not at the well frequency if 
>>>> you want to select this path. Our current "clock tree" is done to 
>>>> fit with our ST reference boards (we have more peripherals than PLL 
>>>> outputs so we have to make choices). So yes for customer/partners 
>>>> boards this clock tree has to be modified to better fit with the 
>>>> need (either using assigned-clock-parent or by modifying bootloader 
>>>> clock tree (tf-a or u-boot)).
>>>
>>> I don't think you handle all the configuration options, but I might 
>>> also be confused.
>>>
>>> See Figure 83. Peripheral clock distribution for Ethernet in the MP1 
>>> datasheet for the below.
>>>
>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive the PHY 
>>> clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 50 MHz 
>>> is sourced directly from PLL4P, which then has to run at 50 MHz and 
>>> that in turn reduces clock frequency for other blocks connected to 
>>> PLL4P (e.g. SDMMC, where the impact is noticable).
>>
>> Ok that's the common path to clock a PHY a 50MHz without using the 
>> ref_clk coming from the PHY. And yes I can understand that the 
>> drawback is huge).
> 
> So lets fix it.

There is no issue in code. It is just clock tree configuration issue. 
Either you don't use PLL4P for Ethernet (what you're doing) or you don't 
use PLL4P for SDMMC. But yes, there are not a lot of possibilities.

> 
>>> So, what I want to model here is this:
>>>
>>> PLL4P = 100 MHz
>>> MCO2 is supplied by PLL4P and set to /2 , so MCO2 = 50 MHz
>>> SoC pad PG2 is set as MCO2 output, thus a source of 50 MHz signal
>>> SoC pad PA1 is set as ETH_RX_CLK and connected to PG2
>>
>> Ok I see (to be honest IIWR we didn't test i :$) but it should work.
> 
> It does work, I have boards which use this setup already.
> 
>>> This works fine in practice, except it cannot be modeled using 
>>> current DT bindings, even though it should be possible to model it.
>>
>> For dwmac point of view it's quite the same thing to have your PHY 
>> clocking by MCO or by a crystal. You just need to configure RX_REF pad 
>> and ETH_CLK_SEL to get the 50 MHz RMII reference clock.
> 
> Yes
> 
>>>>> First, the ETHRX clock in clk-stm32mp1.c only represents the 
>>>>> ETHRXEN gate,
>>>>> however it should represent also ETH_REF_CLK_SEL mux. The problem 
>>>>> is that
>>>>> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 
>>>>> driver and
>>>>> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or the
>>>>> clock block.
>>>>
>>>> dwmac4-stm32 doesn't contain code for dwmac4 but it contains the 
>>>> glue around the dwmac4: syscfg, clocks ...
>>>
>>> The problem is that dwmac4-stm32 isn't the right place to configure 
>>> the ETHRX clock mux, that should be in the clock driver. So the stm32 
>>> clock driver should have SYSCFG handle and configure ETH_REF_CLK_SEL 
>>> mux. The "st,eth-ref-clk-sel" DT prop would then not be needed at 
>>> all, as the reference clock select would be configured using 
>>> assigned-clocks in DT.
>>
>> Idea was to keep at the same place the Ethernet glue configuration. We 
>> can't move all this glue into clock driver as phy interface is needed 
>> to well configure some sysconf registers.
> 
> This configuration can be done by the clock driver too. And in fact, I 
> believe it should be done by the clock driver, just like it's done for 
> all the other clock muxes with gates in the clock driver, except in this 
> case the mux is in syscfg and gate is in rcc.

As said, choice has been done to do it in dwmac-stm32, and sorry I see 
more drawbacks than benefits to move it now.

> 
>> Current dwamc-stm32 glue is working and documented. I'm not convinced 
>> to develop a new one by splitting clock sysconf in clock driver and 
>> phy interface management at ethernet level. I think we will get the 
>> same functional result (but yes maybe more understandable at 
>> dt-bindings level). We could maybe update binding name to be more clear.
> 
> You don't get the same result, since you cannot model the MCO2 input 
> into ETHRX. Or can you ?

Why do you want to model MCO2 into ETHRX ? MCO2 just replace a crystal, 
and when a crystal is used, it is not modeled. I think is it the same 
case for MCO2.

> 
> I also think that we won't need new binding altogether, just a slight 
> tweak to the existing ones which would permit modeling the MCO2 input 
> into ETHRX, I am open to suggestions how to do it. Note that the clock 
> framework must be able to turn off both ETHRX gate, MCO2 and all the way 
> up the tree if ETHRX is turned off.
> 
>>> The default assigned-clocks should be eth_clk_fb , but the user can 
>>> override it in the DT and provide another clock source (e.g. in my 
>>> case, that would be PLL4P->MCO2->ETHRX).
>>>
>>>>> Second, the ETHRX parent clock is either eth_clk_fb (ETHCK_K) or 
>>>>> external
>>>>> ETH_RX_CLK/ETH_REF_CLK_SEL, it is never CK_AXI.
>>>>
>>>> Why CK_AXI ?
>>>
>>> See drivers/clk/clk-stm32mp1.c:
>>>    1895          PCLK(ETHRX, "ethrx", "ck_axi", 0, G_ETHRX),
>>>
>>
>> Ok I see, and it is the same case for TX also. Discussing with our 
>> clock expert it was done for simplification.
> 
> So, how shall we proceed here ?

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/26/21 11:17 AM, Alexandre TORGUE wrote:
> 
> 
> On 1/16/21 6:01 PM, Marek Vasut wrote:
>> On 1/15/21 4:22 PM, Alexandre TORGUE wrote:
>>
>> Hi,
>>
>> [...]
>>
>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently 
>>>>>> does not
>>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>>> worse, the
>>>>>> implementation of this is partly present and is split between the 
>>>>>> clock
>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>>> incorrect.
>>>>>
>>>>> Sorry but I don't understand which configuration is missing. I 
>>>>> think we can handle all possible cases for RMII. At the glue layer 
>>>>> (dwmac-stm32.c) clocks gates and syscfg are set regarding device 
>>>>> tree binding (see the tab in dwmac-stm32.c). You could have a look 
>>>>> here for more details: 
>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
>>>>>
>>>>> Regarding the clock parent, yes it is not at the well frequency if 
>>>>> you want to select this path. Our current "clock tree" is done to 
>>>>> fit with our ST reference boards (we have more peripherals than PLL 
>>>>> outputs so we have to make choices). So yes for customer/partners 
>>>>> boards this clock tree has to be modified to better fit with the 
>>>>> need (either using assigned-clock-parent or by modifying bootloader 
>>>>> clock tree (tf-a or u-boot)).
>>>>
>>>> I don't think you handle all the configuration options, but I might 
>>>> also be confused.
>>>>
>>>> See Figure 83. Peripheral clock distribution for Ethernet in the MP1 
>>>> datasheet for the below.
>>>>
>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive the 
>>>> PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 50 
>>>> MHz is sourced directly from PLL4P, which then has to run at 50 MHz 
>>>> and that in turn reduces clock frequency for other blocks connected 
>>>> to PLL4P (e.g. SDMMC, where the impact is noticable).
>>>
>>> Ok that's the common path to clock a PHY a 50MHz without using the 
>>> ref_clk coming from the PHY. And yes I can understand that the 
>>> drawback is huge).
>>
>> So lets fix it.
> 
> There is no issue in code. It is just clock tree configuration issue. 
> Either you don't use PLL4P for Ethernet (what you're doing) or you don't 
> use PLL4P for SDMMC. But yes, there are not a lot of possibilities.

I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To enable 
this entire chain of clock, I need the correct clock tree. Currently 
that cannot be modeled, can it?

>>>> So, what I want to model here is this:
>>>>
>>>> PLL4P = 100 MHz
>>>> MCO2 is supplied by PLL4P and set to /2 , so MCO2 = 50 MHz
>>>> SoC pad PG2 is set as MCO2 output, thus a source of 50 MHz signal
>>>> SoC pad PA1 is set as ETH_RX_CLK and connected to PG2
>>>
>>> Ok I see (to be honest IIWR we didn't test i :$) but it should work.
>>
>> It does work, I have boards which use this setup already.
>>
>>>> This works fine in practice, except it cannot be modeled using 
>>>> current DT bindings, even though it should be possible to model it.
>>>
>>> For dwmac point of view it's quite the same thing to have your PHY 
>>> clocking by MCO or by a crystal. You just need to configure RX_REF 
>>> pad and ETH_CLK_SEL to get the 50 MHz RMII reference clock.
>>
>> Yes
>>
>>>>>> First, the ETHRX clock in clk-stm32mp1.c only represents the 
>>>>>> ETHRXEN gate,
>>>>>> however it should represent also ETH_REF_CLK_SEL mux. The problem 
>>>>>> is that
>>>>>> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 
>>>>>> driver and
>>>>>> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 or 
>>>>>> the
>>>>>> clock block.
>>>>>
>>>>> dwmac4-stm32 doesn't contain code for dwmac4 but it contains the 
>>>>> glue around the dwmac4: syscfg, clocks ...
>>>>
>>>> The problem is that dwmac4-stm32 isn't the right place to configure 
>>>> the ETHRX clock mux, that should be in the clock driver. So the 
>>>> stm32 clock driver should have SYSCFG handle and configure 
>>>> ETH_REF_CLK_SEL mux. The "st,eth-ref-clk-sel" DT prop would then not 
>>>> be needed at all, as the reference clock select would be configured 
>>>> using assigned-clocks in DT.
>>>
>>> Idea was to keep at the same place the Ethernet glue configuration. 
>>> We can't move all this glue into clock driver as phy interface is 
>>> needed to well configure some sysconf registers.
>>
>> This configuration can be done by the clock driver too. And in fact, I 
>> believe it should be done by the clock driver, just like it's done for 
>> all the other clock muxes with gates in the clock driver, except in 
>> this case the mux is in syscfg and gate is in rcc.
> 
> As said, choice has been done to do it in dwmac-stm32, and sorry I see 
> more drawbacks than benefits to move it now.

Surely a backward-compatible implementation would be possible, how do 
you feel about that ?

>>> Current dwamc-stm32 glue is working and documented. I'm not convinced 
>>> to develop a new one by splitting clock sysconf in clock driver and 
>>> phy interface management at ethernet level. I think we will get the 
>>> same functional result (but yes maybe more understandable at 
>>> dt-bindings level). We could maybe update binding name to be more clear.
>>
>> You don't get the same result, since you cannot model the MCO2 input 
>> into ETHRX. Or can you ?
> 
> Why do you want to model MCO2 into ETHRX ? MCO2 just replace a crystal, 
> and when a crystal is used, it is not modeled. I think is it the same 
> case for MCO2.

I need to correctly enable all the clock instead of keeping MCO2 enabled 
all the time. If ethrx is not needed, the clock are disabled and if even 
the upstream clock are no longer needed, they (MCO2, and then PLL4P) can 
be disabled too.

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/26/21 11:40 AM, Marek Vasut wrote:
> On 1/26/21 11:17 AM, Alexandre TORGUE wrote:
>>
>>
>> On 1/16/21 6:01 PM, Marek Vasut wrote:
>>> On 1/15/21 4:22 PM, Alexandre TORGUE wrote:
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently 
>>>>>>> does not
>>>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>>>> worse, the
>>>>>>> implementation of this is partly present and is split between the 
>>>>>>> clock
>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>>>> incorrect.
>>>>>>
>>>>>> Sorry but I don't understand which configuration is missing. I 
>>>>>> think we can handle all possible cases for RMII. At the glue layer 
>>>>>> (dwmac-stm32.c) clocks gates and syscfg are set regarding device 
>>>>>> tree binding (see the tab in dwmac-stm32.c). You could have a look 
>>>>>> here for more details: 
>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
>>>>>>
>>>>>> Regarding the clock parent, yes it is not at the well frequency if 
>>>>>> you want to select this path. Our current "clock tree" is done to 
>>>>>> fit with our ST reference boards (we have more peripherals than 
>>>>>> PLL outputs so we have to make choices). So yes for 
>>>>>> customer/partners boards this clock tree has to be modified to 
>>>>>> better fit with the need (either using assigned-clock-parent or by 
>>>>>> modifying bootloader clock tree (tf-a or u-boot)).
>>>>>
>>>>> I don't think you handle all the configuration options, but I might 
>>>>> also be confused.
>>>>>
>>>>> See Figure 83. Peripheral clock distribution for Ethernet in the 
>>>>> MP1 datasheet for the below.
>>>>>
>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive the 
>>>>> PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 
>>>>> 50 MHz is sourced directly from PLL4P, which then has to run at 50 
>>>>> MHz and that in turn reduces clock frequency for other blocks 
>>>>> connected to PLL4P (e.g. SDMMC, where the impact is noticable).
>>>>
>>>> Ok that's the common path to clock a PHY a 50MHz without using the 
>>>> ref_clk coming from the PHY. And yes I can understand that the 
>>>> drawback is huge).
>>>
>>> So lets fix it.
>>
>> There is no issue in code. It is just clock tree configuration issue. 
>> Either you don't use PLL4P for Ethernet (what you're doing) or you 
>> don't use PLL4P for SDMMC. But yes, there are not a lot of possibilities.
> 
> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To enable 
> this entire chain of clock, I need the correct clock tree. Currently 
> that cannot be modeled, can it?
> 

Maybe I miss something, I thought your setup was like that:

First clock path to your PHY:
--------------------

PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
It is not directly linked to the dwmac-stm32. You "just" provide a clock 
to MCO2. After that you can use MCO2 pins for any usages.

Second clock patch:
--------------------

50MHz (refclk coming from phy) --> ETH_REF_CLK pad
This one is already covered in dwmac-stm32.

Why do you want to link the both clock paths ?

>>>>> So, what I want to model here is this:
>>>>>
>>>>> PLL4P = 100 MHz
>>>>> MCO2 is supplied by PLL4P and set to /2 , so MCO2 = 50 MHz
>>>>> SoC pad PG2 is set as MCO2 output, thus a source of 50 MHz signal
>>>>> SoC pad PA1 is set as ETH_RX_CLK and connected to PG2
>>>>
>>>> Ok I see (to be honest IIWR we didn't test i :$) but it should work.
>>>
>>> It does work, I have boards which use this setup already.
>>>
>>>>> This works fine in practice, except it cannot be modeled using 
>>>>> current DT bindings, even though it should be possible to model it.
>>>>
>>>> For dwmac point of view it's quite the same thing to have your PHY 
>>>> clocking by MCO or by a crystal. You just need to configure RX_REF 
>>>> pad and ETH_CLK_SEL to get the 50 MHz RMII reference clock.
>>>
>>> Yes
>>>
>>>>>>> First, the ETHRX clock in clk-stm32mp1.c only represents the 
>>>>>>> ETHRXEN gate,
>>>>>>> however it should represent also ETH_REF_CLK_SEL mux. The problem 
>>>>>>> is that
>>>>>>> the ETH_REF_CLK_SEL mux is currently configured in the DWMAC4 
>>>>>>> driver and
>>>>>>> the ETH_REF_CLK_SEL bit is part of SYSCFG block, not the DWMAC4 
>>>>>>> or the
>>>>>>> clock block.
>>>>>>
>>>>>> dwmac4-stm32 doesn't contain code for dwmac4 but it contains the 
>>>>>> glue around the dwmac4: syscfg, clocks ...
>>>>>
>>>>> The problem is that dwmac4-stm32 isn't the right place to configure 
>>>>> the ETHRX clock mux, that should be in the clock driver. So the 
>>>>> stm32 clock driver should have SYSCFG handle and configure 
>>>>> ETH_REF_CLK_SEL mux. The "st,eth-ref-clk-sel" DT prop would then 
>>>>> not be needed at all, as the reference clock select would be 
>>>>> configured using assigned-clocks in DT.
>>>>
>>>> Idea was to keep at the same place the Ethernet glue configuration. 
>>>> We can't move all this glue into clock driver as phy interface is 
>>>> needed to well configure some sysconf registers.
>>>
>>> This configuration can be done by the clock driver too. And in fact, 
>>> I believe it should be done by the clock driver, just like it's done 
>>> for all the other clock muxes with gates in the clock driver, except 
>>> in this case the mux is in syscfg and gate is in rcc.
>>
>> As said, choice has been done to do it in dwmac-stm32, and sorry I see 
>> more drawbacks than benefits to move it now.
> 
> Surely a backward-compatible implementation would be possible, how do 
> you feel about that ?

Well done I can't say "no" in this case.

> 
>>>> Current dwamc-stm32 glue is working and documented. I'm not 
>>>> convinced to develop a new one by splitting clock sysconf in clock 
>>>> driver and phy interface management at ethernet level. I think we 
>>>> will get the same functional result (but yes maybe more 
>>>> understandable at dt-bindings level). We could maybe update binding 
>>>> name to be more clear.
>>>
>>> You don't get the same result, since you cannot model the MCO2 input 
>>> into ETHRX. Or can you ?
>>
>> Why do you want to model MCO2 into ETHRX ? MCO2 just replace a 
>> crystal, and when a crystal is used, it is not modeled. I think is it 
>> the same case for MCO2.
> 
> I need to correctly enable all the clock instead of keeping MCO2 enabled 
> all the time. If ethrx is not needed, the clock are disabled and if even 
> the upstream clock are no longer needed, they (MCO2, and then PLL4P) can 
> be disabled too.

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
[...]
>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently 
>>>>>>>> does not
>>>>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>>>>> worse, the
>>>>>>>> implementation of this is partly present and is split between 
>>>>>>>> the clock
>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>>>>> incorrect.
>>>>>>>
>>>>>>> Sorry but I don't understand which configuration is missing. I 
>>>>>>> think we can handle all possible cases for RMII. At the glue 
>>>>>>> layer (dwmac-stm32.c) clocks gates and syscfg are set regarding 
>>>>>>> device tree binding (see the tab in dwmac-stm32.c). You could 
>>>>>>> have a look here for more details: 
>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration
>>>>>>>
>>>>>>> Regarding the clock parent, yes it is not at the well frequency 
>>>>>>> if you want to select this path. Our current "clock tree" is done 
>>>>>>> to fit with our ST reference boards (we have more peripherals 
>>>>>>> than PLL outputs so we have to make choices). So yes for 
>>>>>>> customer/partners boards this clock tree has to be modified to 
>>>>>>> better fit with the need (either using assigned-clock-parent or 
>>>>>>> by modifying bootloader clock tree (tf-a or u-boot)).
>>>>>>
>>>>>> I don't think you handle all the configuration options, but I 
>>>>>> might also be confused.
>>>>>>
>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in the 
>>>>>> MP1 datasheet for the below.
>>>>>>
>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive the 
>>>>>> PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 
>>>>>> 50 MHz is sourced directly from PLL4P, which then has to run at 50 
>>>>>> MHz and that in turn reduces clock frequency for other blocks 
>>>>>> connected to PLL4P (e.g. SDMMC, where the impact is noticable).
>>>>>
>>>>> Ok that's the common path to clock a PHY a 50MHz without using the 
>>>>> ref_clk coming from the PHY. And yes I can understand that the 
>>>>> drawback is huge).
>>>>
>>>> So lets fix it.
>>>
>>> There is no issue in code. It is just clock tree configuration issue. 
>>> Either you don't use PLL4P for Ethernet (what you're doing) or you 
>>> don't use PLL4P for SDMMC. But yes, there are not a lot of 
>>> possibilities.
>>
>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To enable 
>> this entire chain of clock, I need the correct clock tree. Currently 
>> that cannot be modeled, can it?
>>
> 
> Maybe I miss something, I thought your setup was like that:
> 
> First clock path to your PHY:
> --------------------
> 
> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
> It is not directly linked to the dwmac-stm32. You "just" provide a clock 
> to MCO2. After that you can use MCO2 pins for any usages.
> 
> Second clock patch:
> --------------------
> 
> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
> This one is already covered in dwmac-stm32.
> 
> Why do you want to link the both clock paths ?

Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
input. MCO2 output is routed on a SoC pin and that is connected with a 
wire to ETH_REF_CLK SoC pin (input).

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/26/21 1:59 PM, Marek Vasut wrote:
> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
> [...]
>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling currently 
>>>>>>>>> does not
>>>>>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>>>>>> worse, the
>>>>>>>>> implementation of this is partly present and is split between 
>>>>>>>>> the clock
>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>>>>>> incorrect.
>>>>>>>>
>>>>>>>> Sorry but I don't understand which configuration is missing. I 
>>>>>>>> think we can handle all possible cases for RMII. At the glue 
>>>>>>>> layer (dwmac-stm32.c) clocks gates and syscfg are set regarding 
>>>>>>>> device tree binding (see the tab in dwmac-stm32.c). You could 
>>>>>>>> have a look here for more details: 
>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>
>>>>>>>>
>>>>>>>> Regarding the clock parent, yes it is not at the well frequency 
>>>>>>>> if you want to select this path. Our current "clock tree" is 
>>>>>>>> done to fit with our ST reference boards (we have more 
>>>>>>>> peripherals than PLL outputs so we have to make choices). So yes 
>>>>>>>> for customer/partners boards this clock tree has to be modified 
>>>>>>>> to better fit with the need (either using assigned-clock-parent 
>>>>>>>> or by modifying bootloader clock tree (tf-a or u-boot)).
>>>>>>>
>>>>>>> I don't think you handle all the configuration options, but I 
>>>>>>> might also be confused.
>>>>>>>
>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in the 
>>>>>>> MP1 datasheet for the below.
>>>>>>>
>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive the 
>>>>>>> PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. However, the 
>>>>>>> 50 MHz is sourced directly from PLL4P, which then has to run at 
>>>>>>> 50 MHz and that in turn reduces clock frequency for other blocks 
>>>>>>> connected to PLL4P (e.g. SDMMC, where the impact is noticable).
>>>>>>
>>>>>> Ok that's the common path to clock a PHY a 50MHz without using the 
>>>>>> ref_clk coming from the PHY. And yes I can understand that the 
>>>>>> drawback is huge).
>>>>>
>>>>> So lets fix it.
>>>>
>>>> There is no issue in code. It is just clock tree configuration 
>>>> issue. Either you don't use PLL4P for Ethernet (what you're doing) 
>>>> or you don't use PLL4P for SDMMC. But yes, there are not a lot of 
>>>> possibilities.
>>>
>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>> enable this entire chain of clock, I need the correct clock tree. 
>>> Currently that cannot be modeled, can it?
>>>
>>
>> Maybe I miss something, I thought your setup was like that:
>>
>> First clock path to your PHY:
>> --------------------
>>
>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>> It is not directly linked to the dwmac-stm32. You "just" provide a 
>> clock to MCO2. After that you can use MCO2 pins for any usages.
>>
>> Second clock patch:
>> --------------------
>>
>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>> This one is already covered in dwmac-stm32.
>>
>> Why do you want to link the both clock paths ?
> 
> Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
> input. MCO2 output is routed on a SoC pin and that is connected with a 
> wire to ETH_REF_CLK SoC pin (input).

Ok I see, but I don't think you have to link both clocks.




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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/26/21 4:40 PM, Alexandre TORGUE wrote:
> 
> 
> On 1/26/21 1:59 PM, Marek Vasut wrote:
>> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
>> [...]
>>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling 
>>>>>>>>>> currently does not
>>>>>>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>>>>>>> worse, the
>>>>>>>>>> implementation of this is partly present and is split between 
>>>>>>>>>> the clock
>>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>>>>>>> incorrect.
>>>>>>>>>
>>>>>>>>> Sorry but I don't understand which configuration is missing. I 
>>>>>>>>> think we can handle all possible cases for RMII. At the glue 
>>>>>>>>> layer (dwmac-stm32.c) clocks gates and syscfg are set regarding 
>>>>>>>>> device tree binding (see the tab in dwmac-stm32.c). You could 
>>>>>>>>> have a look here for more details: 
>>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Regarding the clock parent, yes it is not at the well frequency 
>>>>>>>>> if you want to select this path. Our current "clock tree" is 
>>>>>>>>> done to fit with our ST reference boards (we have more 
>>>>>>>>> peripherals than PLL outputs so we have to make choices). So 
>>>>>>>>> yes for customer/partners boards this clock tree has to be 
>>>>>>>>> modified to better fit with the need (either using 
>>>>>>>>> assigned-clock-parent or by modifying bootloader clock tree 
>>>>>>>>> (tf-a or u-boot)).
>>>>>>>>
>>>>>>>> I don't think you handle all the configuration options, but I 
>>>>>>>> might also be confused.
>>>>>>>>
>>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in the 
>>>>>>>> MP1 datasheet for the below.
>>>>>>>>
>>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive 
>>>>>>>> the PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. 
>>>>>>>> However, the 50 MHz is sourced directly from PLL4P, which then 
>>>>>>>> has to run at 50 MHz and that in turn reduces clock frequency 
>>>>>>>> for other blocks connected to PLL4P (e.g. SDMMC, where the 
>>>>>>>> impact is noticable).
>>>>>>>
>>>>>>> Ok that's the common path to clock a PHY a 50MHz without using 
>>>>>>> the ref_clk coming from the PHY. And yes I can understand that 
>>>>>>> the drawback is huge).
>>>>>>
>>>>>> So lets fix it.
>>>>>
>>>>> There is no issue in code. It is just clock tree configuration 
>>>>> issue. Either you don't use PLL4P for Ethernet (what you're doing) 
>>>>> or you don't use PLL4P for SDMMC. But yes, there are not a lot of 
>>>>> possibilities.
>>>>
>>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>>> enable this entire chain of clock, I need the correct clock tree. 
>>>> Currently that cannot be modeled, can it?
>>>>
>>>
>>> Maybe I miss something, I thought your setup was like that:
>>>
>>> First clock path to your PHY:
>>> --------------------
>>>
>>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>>> It is not directly linked to the dwmac-stm32. You "just" provide a 
>>> clock to MCO2. After that you can use MCO2 pins for any usages.
>>>
>>> Second clock patch:
>>> --------------------
>>>
>>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>>> This one is already covered in dwmac-stm32.
>>>
>>> Why do you want to link the both clock paths ?
>>
>> Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
>> input. MCO2 output is routed on a SoC pin and that is connected with a 
>> wire to ETH_REF_CLK SoC pin (input).
> 
> Ok I see, but I don't think you have to link both clocks.

If I don't, then MCO2 will not have any consumer and would be turned off 
by the kernel.

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/26/21 4:42 PM, Marek Vasut wrote:
> On 1/26/21 4:40 PM, Alexandre TORGUE wrote:
>>
>>
>> On 1/26/21 1:59 PM, Marek Vasut wrote:
>>> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
>>> [...]
>>>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling 
>>>>>>>>>>> currently does not
>>>>>>>>>>> permit selecting the clock input from SoC pad. To make things 
>>>>>>>>>>> worse, the
>>>>>>>>>>> implementation of this is partly present and is split between 
>>>>>>>>>>> the clock
>>>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent is 
>>>>>>>>>>> incorrect.
>>>>>>>>>>
>>>>>>>>>> Sorry but I don't understand which configuration is missing. I 
>>>>>>>>>> think we can handle all possible cases for RMII. At the glue 
>>>>>>>>>> layer (dwmac-stm32.c) clocks gates and syscfg are set 
>>>>>>>>>> regarding device tree binding (see the tab in dwmac-stm32.c). 
>>>>>>>>>> You could have a look here for more details: 
>>>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regarding the clock parent, yes it is not at the well 
>>>>>>>>>> frequency if you want to select this path. Our current "clock 
>>>>>>>>>> tree" is done to fit with our ST reference boards (we have 
>>>>>>>>>> more peripherals than PLL outputs so we have to make choices). 
>>>>>>>>>> So yes for customer/partners boards this clock tree has to be 
>>>>>>>>>> modified to better fit with the need (either using 
>>>>>>>>>> assigned-clock-parent or by modifying bootloader clock tree 
>>>>>>>>>> (tf-a or u-boot)).
>>>>>>>>>
>>>>>>>>> I don't think you handle all the configuration options, but I 
>>>>>>>>> might also be confused.
>>>>>>>>>
>>>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in 
>>>>>>>>> the MP1 datasheet for the below.
>>>>>>>>>
>>>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive 
>>>>>>>>> the PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. 
>>>>>>>>> However, the 50 MHz is sourced directly from PLL4P, which then 
>>>>>>>>> has to run at 50 MHz and that in turn reduces clock frequency 
>>>>>>>>> for other blocks connected to PLL4P (e.g. SDMMC, where the 
>>>>>>>>> impact is noticable).
>>>>>>>>
>>>>>>>> Ok that's the common path to clock a PHY a 50MHz without using 
>>>>>>>> the ref_clk coming from the PHY. And yes I can understand that 
>>>>>>>> the drawback is huge).
>>>>>>>
>>>>>>> So lets fix it.
>>>>>>
>>>>>> There is no issue in code. It is just clock tree configuration 
>>>>>> issue. Either you don't use PLL4P for Ethernet (what you're doing) 
>>>>>> or you don't use PLL4P for SDMMC. But yes, there are not a lot of 
>>>>>> possibilities.
>>>>>
>>>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>>>> enable this entire chain of clock, I need the correct clock tree. 
>>>>> Currently that cannot be modeled, can it?
>>>>>
>>>>
>>>> Maybe I miss something, I thought your setup was like that:
>>>>
>>>> First clock path to your PHY:
>>>> --------------------
>>>>
>>>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>>>> It is not directly linked to the dwmac-stm32. You "just" provide a 
>>>> clock to MCO2. After that you can use MCO2 pins for any usages.
>>>>
>>>> Second clock patch:
>>>> --------------------
>>>>
>>>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>>>> This one is already covered in dwmac-stm32.
>>>>
>>>> Why do you want to link the both clock paths ?
>>>
>>> Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
>>> input. MCO2 output is routed on a SoC pin and that is connected with 
>>> a wire to ETH_REF_CLK SoC pin (input).
>>
>> Ok I see, but I don't think you have to link both clocks.
> 
> If I don't, then MCO2 will not have any consumer and would be turned off 
> by the kernel.

I agree, but IMO the MCO clock should be declared with CLK_IGNORE_UNUSED 
flag in stm32mp1 clock driver.

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/26/21 5:47 PM, Alexandre TORGUE wrote:
> 
> 
> On 1/26/21 4:42 PM, Marek Vasut wrote:
>> On 1/26/21 4:40 PM, Alexandre TORGUE wrote:
>>>
>>>
>>> On 1/26/21 1:59 PM, Marek Vasut wrote:
>>>> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
>>>> [...]
>>>>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling 
>>>>>>>>>>>> currently does not
>>>>>>>>>>>> permit selecting the clock input from SoC pad. To make 
>>>>>>>>>>>> things worse, the
>>>>>>>>>>>> implementation of this is partly present and is split 
>>>>>>>>>>>> between the clock
>>>>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent 
>>>>>>>>>>>> is incorrect.
>>>>>>>>>>>
>>>>>>>>>>> Sorry but I don't understand which configuration is missing. 
>>>>>>>>>>> I think we can handle all possible cases for RMII. At the 
>>>>>>>>>>> glue layer (dwmac-stm32.c) clocks gates and syscfg are set 
>>>>>>>>>>> regarding device tree binding (see the tab in dwmac-stm32.c). 
>>>>>>>>>>> You could have a look here for more details: 
>>>>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Regarding the clock parent, yes it is not at the well 
>>>>>>>>>>> frequency if you want to select this path. Our current "clock 
>>>>>>>>>>> tree" is done to fit with our ST reference boards (we have 
>>>>>>>>>>> more peripherals than PLL outputs so we have to make 
>>>>>>>>>>> choices). So yes for customer/partners boards this clock tree 
>>>>>>>>>>> has to be modified to better fit with the need (either using 
>>>>>>>>>>> assigned-clock-parent or by modifying bootloader clock tree 
>>>>>>>>>>> (tf-a or u-boot)).
>>>>>>>>>>
>>>>>>>>>> I don't think you handle all the configuration options, but I 
>>>>>>>>>> might also be confused.
>>>>>>>>>>
>>>>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in 
>>>>>>>>>> the MP1 datasheet for the below.
>>>>>>>>>>
>>>>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive 
>>>>>>>>>> the PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. 
>>>>>>>>>> However, the 50 MHz is sourced directly from PLL4P, which then 
>>>>>>>>>> has to run at 50 MHz and that in turn reduces clock frequency 
>>>>>>>>>> for other blocks connected to PLL4P (e.g. SDMMC, where the 
>>>>>>>>>> impact is noticable).
>>>>>>>>>
>>>>>>>>> Ok that's the common path to clock a PHY a 50MHz without using 
>>>>>>>>> the ref_clk coming from the PHY. And yes I can understand that 
>>>>>>>>> the drawback is huge).
>>>>>>>>
>>>>>>>> So lets fix it.
>>>>>>>
>>>>>>> There is no issue in code. It is just clock tree configuration 
>>>>>>> issue. Either you don't use PLL4P for Ethernet (what you're 
>>>>>>> doing) or you don't use PLL4P for SDMMC. But yes, there are not a 
>>>>>>> lot of possibilities.
>>>>>>
>>>>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>>>>> enable this entire chain of clock, I need the correct clock tree. 
>>>>>> Currently that cannot be modeled, can it?
>>>>>>
>>>>>
>>>>> Maybe I miss something, I thought your setup was like that:
>>>>>
>>>>> First clock path to your PHY:
>>>>> --------------------
>>>>>
>>>>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>>>>> It is not directly linked to the dwmac-stm32. You "just" provide a 
>>>>> clock to MCO2. After that you can use MCO2 pins for any usages.
>>>>>
>>>>> Second clock patch:
>>>>> --------------------
>>>>>
>>>>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>>>>> This one is already covered in dwmac-stm32.
>>>>>
>>>>> Why do you want to link the both clock paths ?
>>>>
>>>> Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
>>>> input. MCO2 output is routed on a SoC pin and that is connected with 
>>>> a wire to ETH_REF_CLK SoC pin (input).
>>>
>>> Ok I see, but I don't think you have to link both clocks.
>>
>> If I don't, then MCO2 will not have any consumer and would be turned 
>> off by the kernel.
> 
> I agree, but IMO the MCO clock should be declared with CLK_IGNORE_UNUSED 
> flag in stm32mp1 clock driver.

Why? It can be safely turned off if it is only used to supply ETHRX. And 
if the clock tree is correctly modeled, that is what happens.

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/26/21 8:11 PM, Marek Vasut wrote:
> On 1/26/21 5:47 PM, Alexandre TORGUE wrote:
>>
>>
>> On 1/26/21 4:42 PM, Marek Vasut wrote:
>>> On 1/26/21 4:40 PM, Alexandre TORGUE wrote:
>>>>
>>>>
>>>> On 1/26/21 1:59 PM, Marek Vasut wrote:
>>>>> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
>>>>> [...]
>>>>>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling 
>>>>>>>>>>>>> currently does not
>>>>>>>>>>>>> permit selecting the clock input from SoC pad. To make 
>>>>>>>>>>>>> things worse, the
>>>>>>>>>>>>> implementation of this is partly present and is split 
>>>>>>>>>>>>> between the clock
>>>>>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent 
>>>>>>>>>>>>> is incorrect.
>>>>>>>>>>>>
>>>>>>>>>>>> Sorry but I don't understand which configuration is missing. 
>>>>>>>>>>>> I think we can handle all possible cases for RMII. At the 
>>>>>>>>>>>> glue layer (dwmac-stm32.c) clocks gates and syscfg are set 
>>>>>>>>>>>> regarding device tree binding (see the tab in 
>>>>>>>>>>>> dwmac-stm32.c). You could have a look here for more details: 
>>>>>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Regarding the clock parent, yes it is not at the well 
>>>>>>>>>>>> frequency if you want to select this path. Our current 
>>>>>>>>>>>> "clock tree" is done to fit with our ST reference boards (we 
>>>>>>>>>>>> have more peripherals than PLL outputs so we have to make 
>>>>>>>>>>>> choices). So yes for customer/partners boards this clock 
>>>>>>>>>>>> tree has to be modified to better fit with the need (either 
>>>>>>>>>>>> using assigned-clock-parent or by modifying bootloader clock 
>>>>>>>>>>>> tree (tf-a or u-boot)).
>>>>>>>>>>>
>>>>>>>>>>> I don't think you handle all the configuration options, but I 
>>>>>>>>>>> might also be confused.
>>>>>>>>>>>
>>>>>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in 
>>>>>>>>>>> the MP1 datasheet for the below.
>>>>>>>>>>>
>>>>>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to drive 
>>>>>>>>>>> the PHY clock, and uses eth_clk_fb to supply ETH_RX_CLK. 
>>>>>>>>>>> However, the 50 MHz is sourced directly from PLL4P, which 
>>>>>>>>>>> then has to run at 50 MHz and that in turn reduces clock 
>>>>>>>>>>> frequency for other blocks connected to PLL4P (e.g. SDMMC, 
>>>>>>>>>>> where the impact is noticable).
>>>>>>>>>>
>>>>>>>>>> Ok that's the common path to clock a PHY a 50MHz without using 
>>>>>>>>>> the ref_clk coming from the PHY. And yes I can understand that 
>>>>>>>>>> the drawback is huge).
>>>>>>>>>
>>>>>>>>> So lets fix it.
>>>>>>>>
>>>>>>>> There is no issue in code. It is just clock tree configuration 
>>>>>>>> issue. Either you don't use PLL4P for Ethernet (what you're 
>>>>>>>> doing) or you don't use PLL4P for SDMMC. But yes, there are not 
>>>>>>>> a lot of possibilities.
>>>>>>>
>>>>>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>>>>>> enable this entire chain of clock, I need the correct clock tree. 
>>>>>>> Currently that cannot be modeled, can it?
>>>>>>>
>>>>>>
>>>>>> Maybe I miss something, I thought your setup was like that:
>>>>>>
>>>>>> First clock path to your PHY:
>>>>>> --------------------
>>>>>>
>>>>>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>>>>>> It is not directly linked to the dwmac-stm32. You "just" provide a 
>>>>>> clock to MCO2. After that you can use MCO2 pins for any usages.
>>>>>>
>>>>>> Second clock patch:
>>>>>> --------------------
>>>>>>
>>>>>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>>>>>> This one is already covered in dwmac-stm32.
>>>>>>
>>>>>> Why do you want to link the both clock paths ?
>>>>>
>>>>> Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
>>>>> input. MCO2 output is routed on a SoC pin and that is connected 
>>>>> with a wire to ETH_REF_CLK SoC pin (input).
>>>>
>>>> Ok I see, but I don't think you have to link both clocks.
>>>
>>> If I don't, then MCO2 will not have any consumer and would be turned 
>>> off by the kernel.
>>
>> I agree, but IMO the MCO clock should be declared with 
>> CLK_IGNORE_UNUSED flag in stm32mp1 clock driver.
> 
> Why? It can be safely turned off if it is only used to supply ETHRX. And 
> if the clock tree is correctly modeled, that is what happens.

You're right. I think we could only add an optional clock inside dwmac 
stm32 glue to take this phy clock (here MCO2)


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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Marek Vasut]

On 1/29/21 4:19 PM, Alexandre TORGUE wrote:
> 
> 
> On 1/26/21 8:11 PM, Marek Vasut wrote:
>> On 1/26/21 5:47 PM, Alexandre TORGUE wrote:
>>>
>>>
>>> On 1/26/21 4:42 PM, Marek Vasut wrote:
>>>> On 1/26/21 4:40 PM, Alexandre TORGUE wrote:
>>>>>
>>>>>
>>>>> On 1/26/21 1:59 PM, Marek Vasut wrote:
>>>>>> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
>>>>>> [...]
>>>>>>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling 
>>>>>>>>>>>>>> currently does not
>>>>>>>>>>>>>> permit selecting the clock input from SoC pad. To make 
>>>>>>>>>>>>>> things worse, the
>>>>>>>>>>>>>> implementation of this is partly present and is split 
>>>>>>>>>>>>>> between the clock
>>>>>>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock parent 
>>>>>>>>>>>>>> is incorrect.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Sorry but I don't understand which configuration is 
>>>>>>>>>>>>> missing. I think we can handle all possible cases for RMII. 
>>>>>>>>>>>>> At the glue layer (dwmac-stm32.c) clocks gates and syscfg 
>>>>>>>>>>>>> are set regarding device tree binding (see the tab in 
>>>>>>>>>>>>> dwmac-stm32.c). You could have a look here for more 
>>>>>>>>>>>>> details: 
>>>>>>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regarding the clock parent, yes it is not at the well 
>>>>>>>>>>>>> frequency if you want to select this path. Our current 
>>>>>>>>>>>>> "clock tree" is done to fit with our ST reference boards 
>>>>>>>>>>>>> (we have more peripherals than PLL outputs so we have to 
>>>>>>>>>>>>> make choices). So yes for customer/partners boards this 
>>>>>>>>>>>>> clock tree has to be modified to better fit with the need 
>>>>>>>>>>>>> (either using assigned-clock-parent or by modifying 
>>>>>>>>>>>>> bootloader clock tree (tf-a or u-boot)).
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think you handle all the configuration options, but 
>>>>>>>>>>>> I might also be confused.
>>>>>>>>>>>>
>>>>>>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet in 
>>>>>>>>>>>> the MP1 datasheet for the below.
>>>>>>>>>>>>
>>>>>>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to 
>>>>>>>>>>>> drive the PHY clock, and uses eth_clk_fb to supply 
>>>>>>>>>>>> ETH_RX_CLK. However, the 50 MHz is sourced directly from 
>>>>>>>>>>>> PLL4P, which then has to run at 50 MHz and that in turn 
>>>>>>>>>>>> reduces clock frequency for other blocks connected to PLL4P 
>>>>>>>>>>>> (e.g. SDMMC, where the impact is noticable).
>>>>>>>>>>>
>>>>>>>>>>> Ok that's the common path to clock a PHY a 50MHz without 
>>>>>>>>>>> using the ref_clk coming from the PHY. And yes I can 
>>>>>>>>>>> understand that the drawback is huge).
>>>>>>>>>>
>>>>>>>>>> So lets fix it.
>>>>>>>>>
>>>>>>>>> There is no issue in code. It is just clock tree configuration 
>>>>>>>>> issue. Either you don't use PLL4P for Ethernet (what you're 
>>>>>>>>> doing) or you don't use PLL4P for SDMMC. But yes, there are not 
>>>>>>>>> a lot of possibilities.
>>>>>>>>
>>>>>>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>>>>>>> enable this entire chain of clock, I need the correct clock 
>>>>>>>> tree. Currently that cannot be modeled, can it?
>>>>>>>>
>>>>>>>
>>>>>>> Maybe I miss something, I thought your setup was like that:
>>>>>>>
>>>>>>> First clock path to your PHY:
>>>>>>> --------------------
>>>>>>>
>>>>>>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>>>>>>> It is not directly linked to the dwmac-stm32. You "just" provide 
>>>>>>> a clock to MCO2. After that you can use MCO2 pins for any usages.
>>>>>>>
>>>>>>> Second clock patch:
>>>>>>> --------------------
>>>>>>>
>>>>>>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>>>>>>> This one is already covered in dwmac-stm32.
>>>>>>>
>>>>>>> Why do you want to link the both clock paths ?
>>>>>>
>>>>>> Because the X1 (MCO2 output) is the same net as 50 MHz ETH_REF_CLK 
>>>>>> input. MCO2 output is routed on a SoC pin and that is connected 
>>>>>> with a wire to ETH_REF_CLK SoC pin (input).
>>>>>
>>>>> Ok I see, but I don't think you have to link both clocks.
>>>>
>>>> If I don't, then MCO2 will not have any consumer and would be turned 
>>>> off by the kernel.
>>>
>>> I agree, but IMO the MCO clock should be declared with 
>>> CLK_IGNORE_UNUSED flag in stm32mp1 clock driver.
>>
>> Why? It can be safely turned off if it is only used to supply ETHRX. 
>> And if the clock tree is correctly modeled, that is what happens.
> 
> You're right. I think we could only add an optional clock inside dwmac 
> stm32 glue to take this phy clock (here MCO2)

But you already do have clock in the glue, it's the ETHRX clock. There 
are no additional clock that have to be added to the glue.

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

Re: [PATCH 3/4] [RFC] ARM: dts: stm32: Add mux for ETHRX clock

[Alexandre TORGUE]

On 1/30/21 7:36 PM, Marek Vasut wrote:
> On 1/29/21 4:19 PM, Alexandre TORGUE wrote:
>>
>>
>> On 1/26/21 8:11 PM, Marek Vasut wrote:
>>> On 1/26/21 5:47 PM, Alexandre TORGUE wrote:
>>>>
>>>>
>>>> On 1/26/21 4:42 PM, Marek Vasut wrote:
>>>>> On 1/26/21 4:40 PM, Alexandre TORGUE wrote:
>>>>>>
>>>>>>
>>>>>> On 1/26/21 1:59 PM, Marek Vasut wrote:
>>>>>>> On 1/26/21 11:54 AM, Alexandre TORGUE wrote:
>>>>>>> [...]
>>>>>>>>>>>>>>> The implementation of ETH_RX_CLK/ETH_REF_CLK handling 
>>>>>>>>>>>>>>> currently does not
>>>>>>>>>>>>>>> permit selecting the clock input from SoC pad. To make 
>>>>>>>>>>>>>>> things worse, the
>>>>>>>>>>>>>>> implementation of this is partly present and is split 
>>>>>>>>>>>>>>> between the clock
>>>>>>>>>>>>>>> driver and dwmac4 driver. Moreover, the ETHRX clock 
>>>>>>>>>>>>>>> parent is incorrect.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Sorry but I don't understand which configuration is 
>>>>>>>>>>>>>> missing. I think we can handle all possible cases for 
>>>>>>>>>>>>>> RMII. At the glue layer (dwmac-stm32.c) clocks gates and 
>>>>>>>>>>>>>> syscfg are set regarding device tree binding (see the tab 
>>>>>>>>>>>>>> in dwmac-stm32.c). You could have a look here for more 
>>>>>>>>>>>>>> details: 
>>>>>>>>>>>>>> https://wiki.st.com/stm32mpu/wiki/Ethernet_device_tree_configuration 
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Regarding the clock parent, yes it is not at the well 
>>>>>>>>>>>>>> frequency if you want to select this path. Our current 
>>>>>>>>>>>>>> "clock tree" is done to fit with our ST reference boards 
>>>>>>>>>>>>>> (we have more peripherals than PLL outputs so we have to 
>>>>>>>>>>>>>> make choices). So yes for customer/partners boards this 
>>>>>>>>>>>>>> clock tree has to be modified to better fit with the need 
>>>>>>>>>>>>>> (either using assigned-clock-parent or by modifying 
>>>>>>>>>>>>>> bootloader clock tree (tf-a or u-boot)).
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't think you handle all the configuration options, but 
>>>>>>>>>>>>> I might also be confused.
>>>>>>>>>>>>>
>>>>>>>>>>>>> See Figure 83. Peripheral clock distribution for Ethernet 
>>>>>>>>>>>>> in the MP1 datasheet for the below.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The current setup I have needs 50 MHz on SoC pad PA1 to 
>>>>>>>>>>>>> drive the PHY clock, and uses eth_clk_fb to supply 
>>>>>>>>>>>>> ETH_RX_CLK. However, the 50 MHz is sourced directly from 
>>>>>>>>>>>>> PLL4P, which then has to run at 50 MHz and that in turn 
>>>>>>>>>>>>> reduces clock frequency for other blocks connected to PLL4P 
>>>>>>>>>>>>> (e.g. SDMMC, where the impact is noticable).
>>>>>>>>>>>>
>>>>>>>>>>>> Ok that's the common path to clock a PHY a 50MHz without 
>>>>>>>>>>>> using the ref_clk coming from the PHY. And yes I can 
>>>>>>>>>>>> understand that the drawback is huge).
>>>>>>>>>>>
>>>>>>>>>>> So lets fix it.
>>>>>>>>>>
>>>>>>>>>> There is no issue in code. It is just clock tree configuration 
>>>>>>>>>> issue. Either you don't use PLL4P for Ethernet (what you're 
>>>>>>>>>> doing) or you don't use PLL4P for SDMMC. But yes, there are 
>>>>>>>>>> not a lot of possibilities.
>>>>>>>>>
>>>>>>>>> I am supplying MCO2 with PLL4P, that is PLL4P->MCO2->ETHRX . To 
>>>>>>>>> enable this entire chain of clock, I need the correct clock 
>>>>>>>>> tree. Currently that cannot be modeled, can it?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Maybe I miss something, I thought your setup was like that:
>>>>>>>>
>>>>>>>> First clock path to your PHY:
>>>>>>>> --------------------
>>>>>>>>
>>>>>>>> PLL4P ---> MCO2 ---> X1 (PHY input clock which replaces crystal)
>>>>>>>> It is not directly linked to the dwmac-stm32. You "just" provide 
>>>>>>>> a clock to MCO2. After that you can use MCO2 pins for any usages.
>>>>>>>>
>>>>>>>> Second clock patch:
>>>>>>>> --------------------
>>>>>>>>
>>>>>>>> 50MHz (refclk coming from phy) --> ETH_REF_CLK pad
>>>>>>>> This one is already covered in dwmac-stm32.
>>>>>>>>
>>>>>>>> Why do you want to link the both clock paths ?
>>>>>>>
>>>>>>> Because the X1 (MCO2 output) is the same net as 50 MHz 
>>>>>>> ETH_REF_CLK input. MCO2 output is routed on a SoC pin and that is 
>>>>>>> connected with a wire to ETH_REF_CLK SoC pin (input).
>>>>>>
>>>>>> Ok I see, but I don't think you have to link both clocks.
>>>>>
>>>>> If I don't, then MCO2 will not have any consumer and would be 
>>>>> turned off by the kernel.
>>>>
>>>> I agree, but IMO the MCO clock should be declared with 
>>>> CLK_IGNORE_UNUSED flag in stm32mp1 clock driver.
>>>
>>> Why? It can be safely turned off if it is only used to supply ETHRX. 
>>> And if the clock tree is correctly modeled, that is what happens.
>>
>> You're right. I think we could only add an optional clock inside dwmac 
>> stm32 glue to take this phy clock (here MCO2)
> 
> But you already do have clock in the glue, it's the ETHRX clock. There 
> are no additional clock that have to be added to the glue.

Yes this one is for ETHRX/REFCLK, but the one I talk about is the one to 
clock the PHY (which replaces the crystal). In your case it is the same 
net but it is not always the case.

cheers
alex


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