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

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

On 9/29/2025 8:36 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon 29 Sep 2025 at 11:15, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> Hi Martin:
>>
>> Thanks for the detailed explanation.
>>
>>
>> On 9/29/2025 4:55 AM, Martin Blumenstingl wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> 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:
>>>>
>>>>
>>>>           __    __    __    __    __    __    __    __
>>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>>                      ^
>>>>                      1 * cycle original channel.
>>>>           _   _   _   _   _   _   _   _   _   _   _   _
>>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>>                                          ^
>>>>                                          5 * cycles new channel.
>>>>           __    __                        _   _   _   _
>>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>>                 ^                        ^
>>>>                 start switching mux.     switch to new channel.
>>> 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.
>>
>> Yes, our glitch-free mux is designed to prevent glitches caused by
>> excessively short high or low levels in the clock output.
>>
>>
>>> 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
>>>
>>> Both patches should get the proper "Fixes" tags.
>>> 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.
>>
>> A V2 version was submitted later with changes based on your suggestions.
>> Regarding the "Fixes" tag, Jerome had proposed some modifications.
>>
>> [PATCH v2 0/3] clk: Fix issues related to CLK_IGNORE_UNUSED failures and
>> amlogic glitch free mux - Chuan Liu via B4 Relay
>> <https://lore.kernel.org/all/20241111-fix_glitch_free-v2-0-0099fd9ad3e5@amlogic.com/>
>>
> The comments I've provided on this still stands.
>
>> Adding CLK_OPS_PARENT_ENABLE causes the CLK_IGNORE_UNUSED configuration
>> of it's parent clocks on the chain to become ineffective, so this patch
>> depends on fixing that issue before it can proceed.
> Unused clocks are NOT a configuration.
>
> They are by-product of the bootloader. You cannot rely on them. If
> anything depends on them, you have a(nother) problem to solve.
>
>>
>> Jerome and I have submitted patches to address the issue of
>> CLK_IGNORE_UNUSED becoming ineffective. I originally planned to wait
>> for progress on this patch and then incorporate Jerome's feedback before
>> sending the V3 version.
> I've provided a suggestion but this something happening in clock core.
> I suggest that you split this out of your series so things that need to
> go through Stephen are not mixed with Amlogic stuff.
>
> But again, you cannot rely on the state of clock just because it has
> CLK_IGNORE_UNUSED:
>
> * Nothing says it is enabled to begin with
> * Nothing says it will stay on if a consumer comes and goes
> * ... and yes, it does not survive CCF usage checking down the road.
>
> It is unreliable and it is not meant to be more than that, AFAIK


ok, I see what you mean, I will try to do that later.


>> Hi Jerome, sorry if this caused any misunderstanding on your part; I
>> will provide timely feedback moving forward.
>>
>>
>>>
>>> Best regards,
>>> Martin
> --
> Jerome