On 1/26/21 11:40 AM, Marek Vasut wrote:

On 1/26/21 11:17 AM, Alexandre TORGUE wrote:

quoted

On 1/16/21 6:01 PM, Marek Vasut wrote:

quoted

On 1/15/21 4:22 PM, Alexandre TORGUE wrote:

Hi,

[...]

quoted

quoted

quoted

quoted

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 ?

quoted

quoted

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel