Message-ID: <e70e9aaa-f448-4f67-9149-cb5970c9bbd6@amlogic.com>
Date: Sun, 28 Sep 2025 14:05:03 +0800
From: Chuan Liu <chuan.liu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>,
 Kevin Hilman <khilman@...libre.com>, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

Hi Jerome & Martin:

Sorry for the imprecise description of the glitch-free mux earlier.

Recently, while troubleshooting a CPU hang issue caused by glitches,
I realized there was a discrepancy from our previous understanding,
so I'd like to clarify it here.

On 10/8/2024 2:02 PM, Jerome Brunet wrote:

> [ EXTERNAL EMAIL ]
>
> On Tue 08 Oct 2024 at 13:44, Chuan Liu <chuan.liu@...ogic.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@...nel.org> wrote:
>>>> From: Chuan Liu <chuan.liu@...ogic.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

It is indeed necessary to enable the glitch-free mux on *both* channel 0 
and channel 1 to allow proper switching. multiple original channel clock 
cycles and new channel clock cycles will be filtered during mux 
switching. An example of the clock waveform is shown below: __ __ __ __ 
__ __ __ __ ori: ↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ ^ 1 * cycle 
original channel. _ _ _ _ _ _ _ _ _ _ _ _ new: ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ 
|_↑ |_↑ |_↑ |_↑ |_↑ |_↑ ^ 5 * cycles new channel. __ __ _ _ _ _ out: ↑ 
|__↑ |______________________↑ |_↑ |_↑ |_↑ |_↑ ^ ^ start switching mux. 
switch to new channel.

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

Powered by blists - more mailing lists