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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20178015-4075-40e9-bbf4-20ae558c2bef@amlogic.com>
Date: Tue, 8 Oct 2024 13:44:54 +0800
From: Chuan Liu <chuan.liu@...ogic.com>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>,
 Jerome Brunet <jbrunet@...libre.com>, 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 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
purpose of CLK_OPS_PARENT_ENABLE may not be to solve our situation,
but adding this flag does solve our current problem.


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