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

[Dmitry Rokosov <ddrokosov@salutedevices.com>]

On Tue, Apr 02, 2024 at 11:27:24AM +0200, Jerome Brunet wrote:
> 
> On Mon 01 Apr 2024 at 20:12, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > 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.
> 
> I disagree. Martin's suggestion is correct for the fsel part which is
> symetric.
> 

It seems that I didn't fully understand Martin's suggestion. I was
confused by the advice to remove the notifier block and tried to explain
why it's not possible. However, I believe it is reasonable to protect
the cpu_fselX from rate propagation, because it is symmetric, as you
mentioned.

> >
> >> 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.
> 
> Apparently, you are relying on the SYS PLL lowest possible rate to
> enfore this contraint, which I suppose is 24 * 32 = 768MHz. It would be
> nice to clearly say so.
> 

Based on my understanding, the minimum frequency that sys_pll can
provide is not relevant. The CPU fixed clock is considered a "safety"
clock, and I can confidently connect the cpu_clk parent to that stable
clock without any issues. CCF will decide which parent will be used in
the end of rate changing process.

> > Fortunately, we don't encounter any freeze
> > problems when we attempt to change its rate at these frequencies.
> 
> That does not sound very solid ...
> 

Why? Per my understanding, CPU fixed clock guarantees this behaviour.

> >
> > 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.
> 
> ... Because PLL needs to relock, it is going to be off for a while. That
> is not "broken", unless there is something else ?
> 

Sorry for wrong terminology. I meant that sys_pll cannot be used as a
clock source (clock parent) while we are changing its rate.

> >
> > 4) As you can observe, in this case, we actually don't need to lock the
> > rate for the sys_pll chain.
> 
> In which case ? I'm lost.
> 

In the case for which notifier block was applied - cpu_clk and sys_pll
rate propagation.

> > We want to change the rate instead.
> 
> ... How are you going to do that without relocking the PLL ?
> 

I'm afraid, this is terminology miss from my side again. By 'sys_pll
lock' I mean rate lock using CLK_SET_RATE_GATE. I want to say that we
can't prohibit rate propagation of sys_pll chain, because we want to
change the rate, this is our main goal.

> > 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.
> 
> There is an example of syspll notifier in the g12 clock controller.
> You should have a look at it

Okay. As I mentioned in another email reply, in order to make it happen,
it is required to move the sys_pll clock to the a1-cpu driver. However,
I thought that this approach may not be correct from a logical
perspective. I will try.

-- 
Thank you,
Dmitry