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

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

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

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 ?

Adding "ck_ker_eth" would impose a new clock to take in DT ?

regards
alex

> 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 ?

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