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

[Jerome Brunet <jbrunet@baylibre.com>]

On Mon 29 Sep 2025 at 17:31, Chuan Liu <chuan.liu@amlogic.com> wrote:

> On 9/29/2025 4:48 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Sun 28 Sep 2025 at 22:55, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>
>>> Hello,
>>>
>>> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>>
>>>> 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.
>>> [...]
>>>> An example of the clock waveform is shown below:
>>>>
>>>>
>>                       1                  2
>>                       v                  v
>>>>          __    __    __    __    __    __    __    __
>>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>>                     ^
>>>>                     1 * cycle original channel.
>>>>          _   _   _   _   _   _   _   _   _   _   _   _
>>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>>                                         ^
>>>>                                         5 * cycles new channel.
>>>>          __    __                        _   _   _   _
>>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>>                ^                        ^
>>>>                start switching mux.     switch to new channel.
>> Ok ... but when is it safe to disable the "ori" clock ?
>> Can you do it at '1' already ? or do you have to wait for '2' ?
>
>
> It should wait for "2", because there is a state machine in the
> glitch-free mux, this state machine is driven by the working clock
> provided by its channel 0.

Then I don't think the 2 flags are enough to make it safe

Nothing guarantees that CCF will wait for those 5 cycles to turn off
the clock noted 'ori' above.

I think you need new specific ops for this mux

Something that would
* protect both parents before changing the mux
* do the actual change
* wait for it to settle
* remove the protection

>
>
>>
>>> Thank you for the detailed report!
>>> This is indeed problematic behavior. I guess the result is somewhat
>>> random: depending on load (power draw), silicon lottery (quality),
>>> temperature, voltage supply, ... - one may or may not see crashes
>>> caused by this.
>>>
>>> Based on the previous discussion on this topic, my suggestion is to
>>> split the original patch:
>>> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
>>> driver already has this where needed) to actually enable the
>>> glitch-free mux behavior
>>> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
>>> also need to be updated) to prevent the glitch-free mux from
>>> temporarily outputting an electrical low signal. Jerome also asked to
>>> document the behavior so we don't forget why we set this flag
>> Yes please split the changes and visit all the controllers shipping this
>> type of muxes.
>>
>>> Both patches should get the proper "Fixes" tags.
>> ... and proper fixes tag maybe different depending on the controller so
>> there might more that just 2 changes.
>>
>>> I think it would also be great if you could include the waveform
>>> example in at least the commit message as it helps understand the
>>> problem.
>>>
>>> Let's also give Jerome some time to comment before you send patches.
>>>
>>>
>>> Best regards,
>>> Martin
>> --
>> Jerome

-- 
Jerome