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:

quoted

[ EXTERNAL EMAIL ]

On Tue 08 Oct 2024 at 13:44, Chuan Liu [off-list ref] wrote:

quoted

Hi Martin,


On 2024/10/1 4:08, Martin Blumenstingl wrote:

quoted

[ EXTERNAL EMAIL ]

Hello,

On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
[off-list ref] wrote:

quoted

From: Chuan Liu <redacted>

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:

quoted

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.

quoted

quoted

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.

[...]

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

Best regards,
Martin

-- 
Jerome