Re: [PATCH 1/2] clk: sunxi-ng: nkm: consider alternative parent rates when finding rate

[Maxime Ripard]

On Wed, Jun 07, 2023 at 09:39:35AM +0200, Frank Oltmanns wrote:
> Hi Maxime,
> 
> On 2023-06-07 at 08:38:39 +0200, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Mon, Jun 05, 2023 at 09:07:44PM +0200, Frank Oltmanns wrote:
> >> In case the CLK_SET_RATE_PARENT flag is set, consider using a different
> >> parent rate when determining a new rate.
> >>
> >> To find the best match for the requested rate, perform the following
> >> steps for each NKM combination:
> >>  - calculate the optimal parent rate,
> >>  - find the best parent rate that the parent clock actually supports
> >>  - use that parent rate to calculate the effective rate.
> >>
> >> In case the clk does not support setting the parent rate, use the same
> >> algorithm as before.
> >>
> >> Signed-off-by: Frank Oltmanns <frank@xxxxxxxxxxxx>
> >> ---
> >>  drivers/clk/sunxi-ng/ccu_nkm.c | 40 ++++++++++++++++++++++++++--------
> >>  1 file changed, 31 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> index a0978a50edae..c71e237226f2 100644
> >> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> >> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> @@ -16,10 +16,10 @@ struct _ccu_nkm {
> >>  	unsigned long	m, min_m, max_m;
> >>  };
> >>
> >> -static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
> >> -				       struct _ccu_nkm *nkm)
> >> +static unsigned long ccu_nkm_find_best(unsigned long *parent, unsigned long rate,
> >> +				       struct _ccu_nkm *nkm, struct clk_hw *parent_hw)
> >>  {
> >> -	unsigned long best_rate = 0;
> >> +	unsigned long best_rate = 0, best_parent_rate = 0, tmp_parent = *parent;
> >>  	unsigned long best_n = 0, best_k = 0, best_m = 0;
> >>  	unsigned long _n, _k, _m;
> >>
> >> @@ -28,12 +28,29 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
> >>  			for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
> >>  				unsigned long tmp_rate;
> >>
> >> -				tmp_rate = parent * _n * _k / _m;
> >> +				if (parent_hw) {
> >
> > NKM clocks always have a parent
> >
> > You should test if the CLK_SET_RATE_PARENT flag is set.
> 
> ccu_nkm_find_best is called in the following two situations:
>  a. from ccu_nkm_set_rate when setting the rate
>  b. from ccu_nkm_round_rate when determining the rate
> 
> In situation a. we never want ccu_nkm_find_best to try different parent
> rates because setting the parent rate is a done deal (at least that's my
> understanding).
> 
> In situation b. we only want ccu_nkm_find_best to try different parent
> rates when, as you mentioned, the CLK_SET_RATE_PARENT flag is set.

It doesn't really matter though. The output of that function must be
stable and must return the same set of factors and parent rate for a
given target rate.

So you can call it as many times as you want, it doesn't really matter.

> So, what this patch does, it provides a NULL pointer as parent_hw when
> we don't want ccu_nkm_find_best to try alternative parent rates.

At best, the argument is misleading then. You're not passing a pointer
to the parent, you're telling it whether it should look for other
parents or not. And it's not a pointer, it's a boolean.

> Is it ok if I add a comment to ccu_nkm_find_best that explains the
> function and explicitly also the parameters?

Sure

> I also thought about using two different functions for the two
> situations. I have no strong opinion which is better.
> 
> However, I don't think we should hand over the flags to this function,
> because we'd still only need to provide the parent_hw if we want to
> find the optimal parent rate, so having two parametes for the same
> purpose seems redundant. Unless, there is a rule to not use NULL
> pointers.

Again, the behaviour must be stable across all calling sites.

> >
> >> +					// We must round up the desired parent rate, because the
> >> +					// rounding down happens when calculating tmp_rate. If we
> >> +					// round down also here, we'd round down twice.
> >> +					unsigned long optimal_parent =
> >> +							(rate * _m + (_n * _k - 1)) / _n / _k;
> >
> > I assume the addition of n * k - 1 is to round up, but I'm not sure we
> > should hack around like that.
> >
> > You should compute the ideal parent rate for a given set of timings, and
> > then just call round_rate on it. If the parent wants to round it one way
> > or another, that's the parent concern.
> 
> I admit that the comment explaining this is not doing the complexity of
> this issue any justice. Let me try to explain:
> 
> Let's say for our panel the optimal rate for pll-mipi is 449064000. The
> best closest we can get is 449035712 with a parent rate of 217714285
> (n=11, k=3, m=16).
> 
> Eventually, ccu_nkm_find_best is going to be called with 449035712 as
> the rate. If we don't round up, like I proposend, but instead calculate:
>   optimal_parent = rate * m / n / k
> (which is, I think, what you you're proposing) leading to an optimal
> parent of 217714284 (!). We can't get 217714284 from the parent (we
> could get 217714285, but we're not asking for that) so the parent rounds
> down.
> 
> To make things worse, this story continues for the new "best rate" as
> well.
> 
> In the end, ccu_nkm_find_best claims:
>  - the optimal rate for 449064000 is 449035712 (parent=217714285, n=11,
>    k=3, m=16)
>  - but ccu_nkm_find_best would claim that the optimal rate for 449035712
>    is 449018181 (parent=235200000, n=7, k=3, m=11)
>  - and finally, the optimal rate for 449018181 is 449018180
>    (parent=213818181, n=7, k=3, m=10)
> 
> This doesn't seem right to me.
> 
> But you're also right, in that we can't just always round up. In a
> hypothetical example that we request a parent rate of 450000000. With
> rounding up, we'd get an optimal parent rate of 218181819 for n=11, k=3,
> m=16. And let's now further claim that the parent could provide exactly
> that rate, we'd end up with a rate of 450000001. So, we'd overshoot,
> which (currently) is not acceptable.
> 
> Hmm... I currently can't think of a clever way to solve this other than
> this:
> 
>     optimal_parent = (rate * _m + (_n * _k - 1)) / _n / _k;
>     tmp_parent = clk_hw_round_rate(parent_hw, optimal_parent);
>     tmp_rate = tmp_parent * _n * _k / _m;
>     if (tmp_rate > rate) {
>         optimal_parent = rate * m / n / k
>         tmp_parent = clk_hw_round_rate(parent_hw, optimal_parent);
>         tmp_rate = tmp_parent * _n * _k / _m;
>     }
>     if (tmp_parent > optimal_parent)
>         continue;
> 
> This seems ugly, but at least it should work in all cases. Any opinions?

Again, you shouldn't work around the issue.

It's very simple really: you already computed the optimal parent rate,
you ask the parent to compute whatever is closest to that optimal parent
rate.

It's the parent responsibility now. It's the parent decision to figure
out what "the closest" means, if it can change rate, if it has any range
limitation, etc. You can't work around that.

What you actually want there is the parent to actually provide the
closest rate, even if it means overshooting. That's fine, we have a flag
for that: CLK_(MUX|DIVIDER)_ROUND_CLOSEST. We just need to set it on the
parent and be done with it.

Maxime

Attachment: signature.asc
Description: PGP signature