[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