Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 08 Oct 2024 at 13:44, Chuan Liu <chuan.liu@amlogic.com> wrote:

> Hi Martin,
>
>
> On 2024/10/1 4:08, Martin Blumenstingl wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hello,
>>
>> On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
>> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>
>>> glitch free mux has two clock channels (channel 0 and channel 1) with
>>> the same configuration. When the frequency needs to be changed, the two
>>> channels ping-pong to ensure clock continuity and suppress glitch.
>> You describe the solution to this below:
>>> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
>>> switchover to suppress glitch.
>> It would be great to have this change in a separate patch.
>> The clocks to which you're adding CLK_SET_RATE_GATE aren't switched at
>> runtime in mainline kernels (at least I think so).
>
>
> Okay, I will separate it into two patches and submit it in the next version.
>
>
>>
>>> Channel 0 of glitch free mux is not only the clock source for the mux,
>>> but also the working clock for glitch free mux. Therefore, when glitch
>>> free mux switches, it is necessary to ensure that channel 0 has a clock
>>> input, otherwise glitch free mux will not work and cannot switch to the
>>> target channel.
>> [...]
>>> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
>>> has clock input when switching channels.
>> This describes a second problem. I think it's best to have this in a
>> separate commit/patch.
>> Also you're updating some mali clocks (e.g. on G12 and GX) but not all
>> of them (Meson8b for example is missing).
>
>
> Yes, M8 missed it, I will complete it in the next version.
>
>
>>
>> I still have some questions to the CLK_OPS_PARENT_ENABLE approach -
>> please share your findings on this.
>>
>> There's multiple clocks involved in a glitch-free mux hierarchy:
>> - a number of clock inputs (e.g. fclk, xtal, ...)
>> - two muxes (one for every channel of the glitch-free mux)
>> - two dividers (one for every channel of the glitch-free mux)
>> - two gates (one for every channel of the glitch-free mux)
>> - the glitch-free mux
>>
>> When switching from channel 0 (which is active and enabled) CCF
>> (common clock framework) will:
>> a) on channel 1:
>> - find the best input clock
>> - choose the best input clock in the mux
>> - set the divider
>> - enable the gate
>> b) switch the glitch-free mux
>> c) on channel 2:
>> - disable the gate
>>
>> To me it's not clear at which level the problem occurs (glitch-free
>> mux, gate, divider, mux, input clock).
>> Also I don't understand why enabling the clocks with
>> CLK_OPS_PARENT_ENABLE solves any problem since CCF is doing things
>> automatically for us.
>> Can you please explain (preferably with an example) what problem is
>> solved with this approach?
>
>
> If CLK_OPS_PARENT_ENABLE is configured in mux, 'new_parent' and
> 'old_parent' will be enabled first when __clk_set_parent_before() is
> called. And disable them at __clk_set_parent_after(). Our glitch free
> only has two clock sources, so adding this flag ensures that both
> channels 0 and 1 are enabled when mux switches.
>
> In fact, we just need to make sure that channel 0 is enabled. The
> purpose of CLK_OPS_PARENT_ENABLE may not be to solve our situation,
> but adding this flag does solve our current problem.

This is last point is important.

It is OK to use a side effect of CLK_OPS_PARENT_ENABLE but it needs to
be documented somehow, so what really matters is still known 2y from now.

Make sure the information appears in the code comments at least once.

>
>
>>
>> Last but not least: if we're running into bugs when
>> CLK_OPS_PARENT_ENABLE is missing then that patch should carry a Fixes
>> tag.
>
>
> Thanks for the heads-up. I'll keep an eye on it in the next version.
>
>
>>
>> Best regards,
>> Martin

-- 
Jerome