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