Re: [PATCH v1 6/6] clk: meson: a1: add Amlogic A1 CPU clock controller driver

[Dmitry Rokosov <ddrokosov@salutedevices.com>]

Hello Martin,

Thank you for quick response. Please find my thoughts below.

On Sun, Mar 31, 2024 at 11:40:13PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry,
> 
> On Fri, Mar 29, 2024 at 9:59 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
> [...]
> > +static struct clk_regmap cpu_fclk = {
> > +       .data = &(struct clk_regmap_mux_data) {
> > +               .offset = CPUCTRL_CLK_CTRL0,
> > +               .mask = 0x1,
> > +               .shift = 10,
> > +       },
> > +       .hw.init = &(struct clk_init_data) {
> > +               .name = "cpu_fclk",
> > +               .ops = &clk_regmap_mux_ops,
> > +               .parent_hws = (const struct clk_hw *[]) {
> > +                       &cpu_fsel0.hw,
> > +                       &cpu_fsel1.hw,
> Have you considered the CLK_SET_RATE_GATE flag for &cpu_fsel0.hw and
> &cpu_fsel1.hw and then dropping the clock notifier below?
> We use that approach with the Mali GPU clock on other SoCs, see for
> example commit 8daeaea99caa ("clk: meson: meson8b: make the CCF use
> the glitch-free mali mux").
> It may differ from what Amlogic does in their BSP,

Amlogic in their BSP takes a different approach, which is slightly
different from mine. They cleverly change the parent of cpu_clk directly
by forking the cpufreq driver to a custom version. I must admit, it's
quite an "interesting and amazing" idea :) but it's not architecturally
correct totally.

> but I don't think
> that there's any harm (if it works in general) because CCF (common
> clock framework) will set all clocks in the "inactive" tree and then
> as a last step just change the mux (&cpu_fclk.hw). So at no point in
> time will we get any other rate than a) the original CPU clock rate
> before the rate change b) the new desired CPU clock rate. This is
> because we have two symmetric clock trees.

Now, let's dive into the specifics of the issue we're facing. I've
examined the CLK_SET_RATE_GATE flag, which, to my understanding, blocks
rate changes for the entire clock chain. However, in this particular
situation, it doesn't provide the solution we need.

Here's the problem we're dealing with:

1) The CPU clock can have the following frequency points:

  available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz

When we run the cpupower, we get the following information:
# cpupower -c 0,1 frequency-info
analyzing CPU 0:
  driver: cpufreq-dt
  CPUs which run at the same hardware frequency: 0 1
  CPUs which need to have their frequency coordinated by software: 0 1
  maximum transition latency: 50.0 us
  hardware limits: 128 MHz - 1.20 GHz
  available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
  available cpufreq governors: conservative ondemand userspace performance schedutil
  current policy: frequency should be within 128 MHz and 128 MHz.
                  The governor "schedutil" may decide which speed to use
                  within this range.
  current CPU frequency: 128 MHz (asserted by call to hardware)
analyzing CPU 1:
  driver: cpufreq-dt
  CPUs which run at the same hardware frequency: 0 1
  CPUs which need to have their frequency coordinated by software: 0 1
  maximum transition latency: 50.0 us
  hardware limits: 128 MHz - 1.20 GHz
  available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
  available cpufreq governors: conservative ondemand userspace performance schedutil
  current policy: frequency should be within 128 MHz and 128 MHz.
                  The governor "schedutil" may decide which speed to use
                  within this range.
  current CPU frequency: 128 MHz (asserted by call to hardware)

2) For the frequency points 128 MHz, 256 MHz, and 512 MHz, the CPU fixed
clock should be used. Fortunately, we don't encounter any freeze
problems when we attempt to change its rate at these frequencies.

3) However, for the frequency points 768 MHz, 1.01 GHz, and 1.20 GHz,
the sys_pll is used as the clock source because it's a faster option.
Now, let's imagine that we want to change the CPU clock from 768 MHz to
1.01 GHz. Unfortunately, it's not possible due to the broken sys_pll,
and any execution attempts will result in a hang.

4) As you can observe, in this case, we actually don't need to lock the
rate for the sys_pll chain. We want to change the rate instead. Hence,
I'm not aware of any other method to achieve this except by switching
the cpu_clk parent to a stable clock using clock notifier block.
Interestingly, I've noticed a similar approach in the CPU clock drivers
of Rockchip, Qualcomm, and Mediatek.

-- 
Thank you,
Dmitry