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

[Chuan Liu <chuan.liu@amlogic.com>]

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.


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