[PATCH 1/7] clk: stm32mp1: Split ETHCK_K into separate MUX and GATE clock

[Marek Vasut]

On 4/16/21 5:23 PM, Alexandre TORGUE wrote:

Hello Alexandre,

> On 4/16/21 3:47 PM, Marek Vasut wrote:
>> On 4/16/21 8:44 AM, gabriel.fernandez at foss.st.com wrote:
>>> Hi Marek
>>
>> Hello Gabriel,
>>
>>> On 4/14/21 4:04 PM, Marek Vasut wrote:
>>>> On 4/14/21 3:03 PM, gabriel.fernandez at foss.st.com wrote:
>>>>> Hi Marek,
>>>>
>>>> Hello Gabriel,
>>>>
>>>>> Thanks for the patchset
>>>>>
>>>>> On 4/8/21 8:57 PM, Marek Vasut wrote:
>>>>>> The ETHCK_K are modeled as composite clock of MUX and GATE, 
>>>>>> however per
>>>>>> STM32MP1 Reference Manual RM0436 Rev 3, Page 574, Figure 83. 
>>>>>> Peripheral
>>>>>> clock distribution for Ethernet, ETHPTPDIV divider is attached 
>>>>>> past the
>>>>>> ETHCK_K mux, and ETH_CLK/eth_clk_fb clock are output past ETHCKEN 
>>>>>> gate.
>>>>>> Therefore, in case ETH_CLK/eth_clk_fb are not in use AND PTP clock 
>>>>>> are
>>>>>> in use, ETHCKEN gate can be turned off. Current driver does not 
>>>>>> permit
>>>>>> that, fix it.
>>>>>
>>>>> I don"t understand, it's already the case.
>>>>>
>>>>> ETHCK_K it's a composite with a MUX and a GATE.
>>>>
>>>> But ETHCK_K is _not_ a composite clock, look at the Figure 83 in the 
>>>> datasheet again and schematic below.
>>>>
>>>>> ETHPTP_K (ETHPTPDIV) it's a composite with the same MUX and a DIV 
>>>>> (no gate)
>>>>
>>>> But ETHPTP_K shouldn't control any mux, it is only a divider.
>>>>
>>>>> If you use only ETHPTPDIV,  ETHCKEN gate can be turned off.
>>>>
>>>> Look, this is what you have today:
>>>>
>>>>             .------------ ETHCK_K -----------.
>>>>             |_______               _______   |
>>>> pll4_p_ck--|M_ETHCK\             |G_ETHCK\  |
>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>             |               |
>>>>             |               '--(ETHCKSELR[7:4] divider)--[x] 
>>>> clk_ptp_ref
>>>>             |                                          |
>>>>             '------------ ETHPTP_K --------------------'
>>>>
>>>> And this is what you should have, to avoid having two composite 
>>>> clock which control the same mux using the same register bit, i.e. 
>>>> what this patch implements:
>>>>
>>>>             .- ck_ker_eth -.  .--- ETHCK_K --.
>>>>             |_______       |  |    _______   |
>>>> pll4_p_ck--|M_ETHCK\      |  |   |G_ETHCK\  |
>>>>             | MUX    |------+-----| GATE   |-------------[x] ETH_CLK
>>>> pll3_q_ck--|_______/       |     |_______/                  eth_clk_fb
>>>>                             |
>>>>                             '--(ETHCKSELR[7:4] divider)--[x] 
>>>> clk_ptp_ref
>>>>                              |                         |
>>>>                              '---- ETHPTP_K -----------'
>>>>
>>>
>>> These 2 solutions are valid. I made the choice to implement the first 
>>> one to be able to change parent with the kernel clock of the IP (no 
>>> need to add an intermediate binding).
>>
>> Which IP are you talking about in here ?
>>
>>> It's the same principle for all kernel of this soc.
>>
>> The first option is wrong, because in that model, you have two 
>> composite clock which control the same one mux bit in the same 
>> register. Basically you register two distinct clock which operate the 
>> same hardware knob.
>>
>>> I can ask to Alexandre to comeback of this principle, but i 'm not 
>>> favorable.
>>
> 
> The only discussing thing is how the clock is shown. I mean either two 
> composites or one mux plus two gates. Gabriel made a choice to abstract 
> the mux in two composite clocks. But it seems that at the end we have 
> the same behaviour, isn't ?

Not really. Since the two composite clock control the same mux bit, 
consider what would happen if you were to select pll4_p_ck as parent for 
one (e.g. ETHCK_K), and pll3_q_ck as parent for the other (e.g. 
ETHPTP_K), what would be the result ? I guess the result would depend on 
when the reparenting of each ETHCK_K/ETHPTP_K happens on boot, and I 
don't think that's how it should work. With a single mux controlling 
that one single bit, such situation wouldn't happen.

> Adding "ck_ker_eth" would impose a new clock to take in DT ?
Nope, the ck_ker_eth is without ID and internal to the driver. They 
exist only to describe the clock tree correctly.

[...]