Received-SPF: pass (google.com: domain of linux-kernel+bounces-127640-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
References: <20240329205904.25002-1-ddrokosov@salutedevices.com>
 <20240329205904.25002-7-ddrokosov@salutedevices.com>
 <CAFBinCCC5KK-4_w41B-+ZJ3zdZckq_EDuAW+Kak2C0Ow8uuD6Q@mail.gmail.com>
 <20240401171237.qoewp2pgcdrqvc3e@CAB-WSD-L081021>
User-agent: mu4e 1.10.8; emacs 29.2
From: Jerome Brunet <jbrunet@baylibre.com>
To: Dmitry Rokosov <ddrokosov@salutedevices.com>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
 neil.armstrong@linaro.org, jbrunet@baylibre.com, mturquette@baylibre.com,
 sboyd@kernel.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
 khilman@baylibre.com, kernel@salutedevices.com, rockosov@gmail.com,
 linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v1 6/6] clk: meson: a1: add Amlogic A1 CPU clock
 controller driver
Date: Tue, 02 Apr 2024 11:27:24 +0200
In-reply-to: <20240401171237.qoewp2pgcdrqvc3e@CAB-WSD-L081021>
Message-ID: <1jzfuchztl.fsf@starbuckisacylon.baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


On Mon 01 Apr 2024 at 20:12, Dmitry Rokosov <ddrokosov@salutedevices.com> w=
rote:

> 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,
>>=20
>> On Fri, Mar 29, 2024 at 9:59=E2=80=AFPM Dmitry Rokosov
>> <ddrokosov@salutedevices.com> wrote:
>> [...]
>> > +static struct clk_regmap cpu_fclk =3D {
>> > +       .data =3D &(struct clk_regmap_mux_data) {
>> > +               .offset =3D CPUCTRL_CLK_CTRL0,
>> > +               .mask =3D 0x1,
>> > +               .shift =3D 10,
>> > +       },
>> > +       .hw.init =3D &(struct clk_init_data) {
>> > +               .name =3D "cpu_fclk",
>> > +               .ops =3D &clk_regmap_mux_ops,
>> > +               .parent_hws =3D (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.

>
>> 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 GH=
z, 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 GH=
z, 1.20 GHz
>   available cpufreq governors: conservative ondemand userspace performanc=
e 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 GH=
z, 1.20 GHz
>   available cpufreq governors: conservative ondemand userspace performanc=
e 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 =3D 768MHz. It would be
nice to clearly say so.

> Fortunately, we don't encounter any freeze
> problems when we attempt to change its rate at these frequencies.

That does not sound very solid ...

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

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

> We want to change the rate instead.

.. How are you going to do that without relocking the PLL ?

> 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

--=20
Jerome