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