Re: [PATCH 0/2] clk: Fix issues related to CLK_IGNORE_UNUSED failures and amlogic glitch free mux

[Jerome Brunet <jbrunet@baylibre.com>]

On Sun 29 Sep 2024 at 14:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> If CLK_OPS_PARENT_ENABLE is configured for clock,
> clk_core_disable_unprepare() is called in clk_disable_unused_subtree().
> Even clocks that are configured with CLK_IGNORE_UNUSED are disabled,
> resulting in the failure of CLK_IGNORE_UNUSED.
>
> To ensure that amlogic glitch free mux can switch clock channels
> properly, add flag CLK_OPS_PARENT_ENABLE to glitch free mux. The issue
> that CLK_OPS_PARENT_ENABLE in CCF causes CLK_IGNORE_UNUSED to fail is
> also exposed.

The problem is that you are mixing problems together and it makes things
rather difficult to follow. There are 2 distinct problem, there should
have been to distinct patchset, even if you reference the CCF one in the
Amlogic change.

CLK_IGNORE_UNUSED is no guarantee that a clock will stay on, no matter
what happens in the clock tree. I explained that to you several times,
and it is the very reason why you are being asked to justify each usage
of the flag. Most of the time, using it is wrong.

That being said, there seems to be problems with CLK_OPS_PARENT_ENABLE
in clk_disable_unused_subtree(). As it is, I think
* a clock with CLK_IGNORE_UNUSED and CLK_OPS_PARENT_ENABLE, would 'leak'
  an enable, essentially making the parent subtree critical.
* All parents of a CLK_OPS_PARENT_ENABLE clock would have its
  CLK_IGNORE_UNUSED ignored as a result of the enable/disable sequence
  (note that in any other circumstance, enable/disable should indeed
  disable an CLK_IGNORE_UNUSED clock).
* Parent of ignored clocks may still get disabled.

I'll be sending a proposal to address these problems soon.

>
> glitch free mux channel switchover failure issue(Test vpu_clk on S4):
> step 1:
> $ cat /sys/kernel/debug/clk/vpu/clk_parent 
> vpu_0
> $ cat /sys/kernel/debug/clk/vpu_0/clk_rate 
> 399999994
> $ cat /sys/kernel/debug/clk/vpu_1/clk_rate 
> 666666656
> $ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
> $ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
> 399987500       +/-12500Hz
>
> step 2:
> $ echo 0 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
> $ echo 1 > /sys/kernel/debug/clk/vpu/clk_parent 
> $ cat /sys/kernel/debug/clk/vpu/clk_parent 
> vpu_1
> $ cat /sys/kernel/debug/clk/vpu/clk_rate 
> 666666656
> $ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
> $ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
> 0       +/-3125Hz
>
> In step2, vpu_0 is disabled, and the vpu is not switched to vpu_1. At
> this time, the vpu is still connected to vpu_0 and vpu_0 is disabled at
> this time, resulting in the clk-measure not measuring the clock.
>

If keep the display on and managed at all time is critical, then may you
should consider having the module managing it as built-in.

You would not need CLK_IGNORE_USED at all because device would be
present before clk_disable_unused() is executed.

Past the late init, if no device actively use the clock, disabling it is
the sane thing to do because nothing says it will ever be used.

> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
> Chuan Liu (2):
>       clk: Fix the CLK_IGNORE_UNUSED failure issue
>       clk: meson: Fix glitch free mux related issues
>
>  drivers/clk/clk.c                  | 67 ++++++++++++++++++++++++++++++++++++--
>  drivers/clk/meson/a1-peripherals.c | 12 +++----
>  drivers/clk/meson/axg.c            | 16 +++++----
>  drivers/clk/meson/c3-peripherals.c |  6 ++--
>  drivers/clk/meson/g12a.c           | 18 ++++++----
>  drivers/clk/meson/gxbb.c           | 18 ++++++----
>  drivers/clk/meson/s4-peripherals.c | 32 +++++++++---------
>  7 files changed, 122 insertions(+), 47 deletions(-)
> ---
> base-commit: e736da1956cf0880a02ec5023f3487eea7611b5f
> change-id: 20240929-fix_glitch_free-290c88923c31
>
> Best regards,

-- 
Jerome