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

[Marek Vasut <marex@denx.de>]

On 4/16/21 8:44 AM, gabriel.fernandez@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@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.

Please ask.

>>>> This patch converts ETHCK_K from composite clock into a ETHCKEN gate,
>>>> ETHPTP_K from composite clock into ETHPTPDIV divider, and adds another
>>>> NO_ID clock "ck_ker_eth" which models the ETHSRC mux and is parent 
>>>> clock
>>>> to both ETHCK_K and ETHPTP_K. Therefore, all references to ETHCK_K and
>>>> ETHPTP_K remain functional as before.
>>>>
>>>> [1] STM32MP1 Reference Manual RM0436 Rev 3, Page 574,
>>>>      Figure 83. Peripheral clock distribution for Ethernet
>>>> https://www.st.com/resource/en/reference_manual/dm00327659-stm32mp157-advanced-armbased-32bit-mpus-stmicroelectronics.pdf 
>>>>
>>
>> [...]
>>
>>>> diff --git a/drivers/clk/clk-stm32mp1.c b/drivers/clk/clk-stm32mp1.c
>>>> index a875649df8b8..a7c7f544ee5d 100644
>>>> --- a/drivers/clk/clk-stm32mp1.c
>>>> +++ b/drivers/clk/clk-stm32mp1.c
>>>> @@ -1949,7 +1949,6 @@ static const struct clock_config 
>>>> stm32mp1_clock_cfg[] = {
>>>>       KCLK(DSI_K, "dsi_k", dsi_src, 0, G_DSI, M_DSI),
>>>>       KCLK(ADFSDM_K, "adfsdm_k", sai_src, 0, G_ADFSDM, M_SAI1),
>>>>       KCLK(USBO_K, "usbo_k", usbo_src, 0, G_USBO, M_USBO),
>>>> -    KCLK(ETHCK_K, "ethck_k", eth_src, 0, G_ETHCK, M_ETHCK),
>>>>       /* Particulary Kernel Clocks (no mux or no gate) */
>>>>       MGATE_MP1(DFSDM_K, "dfsdm_k", "ck_mcu", 0, G_DFSDM),
>>>> @@ -1958,11 +1957,16 @@ static const struct clock_config 
>>>> stm32mp1_clock_cfg[] = {
>>>>       MGATE_MP1(GPU_K, "gpu_k", "pll2_q", 0, G_GPU),
>>>>       MGATE_MP1(DAC12_K, "dac12_k", "ck_lsi", 0, G_DAC12),
>>>> -    COMPOSITE(ETHPTP_K, "ethptp_k", eth_src, CLK_OPS_PARENT_ENABLE |
>>>> +    COMPOSITE(NO_ID, "ck_ker_eth", eth_src, CLK_OPS_PARENT_ENABLE |
>>>>             CLK_SET_RATE_NO_REPARENT,
>>>>             _NO_GATE,
>>>>             _MMUX(M_ETHCK),
>>>> -          _DIV(RCC_ETHCKSELR, 4, 4, 0, NULL)),
>>>> +          _NO_DIV),
>>>> +
>>>> +    MGATE_MP1(ETHCK_K, "ethck_k", "ck_ker_eth", 0, G_ETHCK),
>>> assigned parent with ETHCK_K will not work
>>>> +
>>>> +    DIV(ETHPTP_K, "ethptp_k", "ck_ker_eth", CLK_OPS_PARENT_ENABLE |
>>>
>>> CLK_OPS_PARENT_ENABLE flags not useful with a divider.
>>
>> How so ?