Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues
From: Chuan Liu
Date: Sun Sep 28 2025 - 02:41:24 EST
On 9/28/2025 2:05 PM, Chuan Liu wrote:
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@xxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
From: Chuan Liu <chuan.liu@xxxxxxxxxxx>You describe the solution to this below:
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.
Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-PongIt would be great to have this change in a separate patch.
switchover to suppress glitch.
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 0This describes a second problem. I think it's best to have this in a
has clock input when switching channels.
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.
Sorry, there seems to be something wrong with the following format parsing.
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,This is last point is important.
but adding this flag does solve our current problem.
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