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

[Alexandre TORGUE <alexandre.torgue@foss.st.com>]

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