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

[gabriel.fernandez at foss.st.com]

Hi Marek,

On 4/16/21 5:31 PM, Marek Vasut wrote:
> 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.

The reparenting is managed. This mux has specific ops.
root at stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll4_p
pll4_p
root at stm32mp1-disco-oss:~# echo pll3_q > 
/sys/kernel/debug/clk/ethptp_k/clk_set_parent
root at stm32mp1-disco-oss:~# cat /sys/kernel/debug/clk/ethck_k/clk_parent 
&& cat /sys/kernel/debug/clk/ethptp_k/clk_parent
pll3_q
pll3_q

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