Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230527132747.83196-1-frank@oltmanns.dev>
 <flngzi4henkzcpzwdexencdkw77h52g3nduup7pwctpwfiuznk@eewnnut5mvsq>
From:   Frank Oltmanns <frank@oltmanns.dev>
To:     Maxime Ripard <mripard@kernel.org>
Cc:     linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-sunxi@lists.linux.dev,
        Andre Przywara <andre.przywara@arm.com>,
        Chen-Yu Tsai <wens@csie.org>, Icenowy Zheng <icenowy@aosc.io>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Rob Herring <robh@kernel.org>,
        Samuel Holland <samuel@sholland.org>,
        Stephen Boyd <sboyd@kernel.org>
Subject: Re: [RFC PATCH 0/3] clk: sunxi-ng: Optimize rate selection for NKM
 clocks
Date:   Thu, 01 Jun 2023 07:16:45 +0200
In-reply-to: <flngzi4henkzcpzwdexencdkw77h52g3nduup7pwctpwfiuznk@eewnnut5mvsq>
Message-ID: <87mt1jbf18.fsf@oltmanns.dev>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Maxime,

On 2023-05-31 at 15:48:43 +0200, Maxime Ripard <mripard@kernel.org> wrote:
> [[PGP Signed Part:Undecided]]
> Hi Frank,
>
> On Sat, May 27, 2023 at 03:27:44PM +0200, Frank Oltmanns wrote:
>> I would like to bring your attention to the current process of setting
>> the rate of an NKM clock. As it stands, when setting the rate of an
>> NKM clock, the rate nearest but less than or equal to the requested
>> rate is found, instead of the nearest rate.
>
> Yeah, it's actually pretty common, see clk_mux_determine_rate_flags()
> for example. Some devices require that we don't overshoot, while some
> prefer to have the closest rate.
>
> Both are fine, and it's a bit context specific which one we should
> favour. If we were to do anything, it would be to support both and let
> the clock driver select which behaviour it wants.
>

Ok, understood. Thank you for the explanation! Now, I'm wondering if
anyone would be using such a flag, if I added it.

>
>> Moreover, ccu_nkm_find_best() is called multiple times (footnote [1])
>> when setting a rate, each time iterating over all combinations of n,
>> k, and m.
>
> Yeah, that's expected as well.

I'm wondering though, if iterating over all combinations is set in
stone, or if some kind of optimization would be in order.

>
>> In response to this, I propose the following refinements to optimize the NKM
>> clock setting:
>>  a. when finding the best rate use the nearest rate, even if it is greater than
>>     the requested rate (PATCH 1)
>>  b. utilize binary search to find the best rate by going through a
>>     precalculated, ordered list of all meaningful combinations of n, k, and m
>>     (PATCH 2)
>
> One thing you haven't really addressed is why we would be doing this? Is
> there some clocks that require a more precise clock and don't? Is the
> factor calculation a bottleneck for some workloads?

Background
==========
I'm a pinephone user (ccu-sun50i-a64). I'm using U-Boot which sets the
pll-video0 to 294 MHz on boot. The phone's panel requires DCLK to run at
108 MHz to get a nice 60 Hz vertical refresh rate. The clock structure
is this:

    clock                       clock type
    --------------------------------------
    pll-video0                  ccu_nm
       pll-mipi                 ccu_nkm
          tcon0                 ccu_mux
             tcon-data-clock    sun4i_dclk

The divider between tcon0 and tcon-data-clock is fixed at 4. So, I need
pll-mipi to run at 432 MHz to get the desired vertical refresh rate.
When pll-vdeo0 is at 294 MHz this is that rate cannot be matched exactly
with any combination. The best we can get is 431.2 MHz (n=11, k=2,
m=15).

The pinephone has some "vendor" patches (megi kernel) that
 a. add HDMI
 b. allow re-setting pll-mipi's rate when pll-video0 changes

Re: Who needs a more precise clock?
===================================
When plugging in HDMI, pll-video's rate is set to 297 MHz, which - in
the vendor kernel, not mainline - triggers recalculation of pll-mipi
(trying to set it to 431.2 MHz). It ends up with a rate of 424.285714
MHz, because this is the nearest, but less than 431.2 MHz (n=5, k=2,
m=7). The nearest rate would be 432 MHz.

So, while analyzing the whole situation that I described above, I found
out that the NKM clocks are not set to the closest rate and wondered why
that is. Hence my request for comments.

Now, one could argue that pll-video0 should be set to 297MHz at boot or
that pll-mipi should try to set the *requested* rate instead of the
previous rate when the pll-video0 changes. And I think that both are
valid or even better approaches than my proposal in this RFC to address
this specific problem and I'll probably sent patches to discuss this as
well.


Re: Why speed up factor calculation?
====================================
I'm not aware that the current implementation of calculating n, k, and m
poses a bottleneck in any situation. Again, while going through the
code, I wondered why not save a few CPU cycles by precalculating the
meaningful combinations. In my opinion, it does not have any side
effects, so we might as well do it. (There is of course the side effect
of using a higher rate, but this is unrelated to precalculation as I
could as well employ a rate comparison that only allows lower rates, or
only optionally higher rates.)

> Clocks in general are very regression-prone, so I'd rather be a bit
> conservative there, and "if it ain't broke, don't fix it".

Sure, I get that.

As I stated in my cover letter:
"The motivation for these proposed changes lies in the current behavior
of rate selection for NKM clocks, which doesn't observe the
CLK_SET_RATE_PARENT flag. I.e. it does not select a different rate for
the parent clock to find the optimal rate."

I thought that this required this optimization to be implemented, but by
now, I'm no longer sure. I'll probably continue investigating different
paths for CLK_SET_RATE_PARENT for NKM clocks and follow up with new
findings.

Thanks,
  Frank

>
> Maxime
>
> [[End of PGP Signed Part]]