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