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

[Maxime Ripard]

On Thu, Jun 15, 2023 at 06:04:53PM +0200, Frank Oltmanns wrote:
> 15.06.2023 16:47:33 Maxime Ripard <maxime@xxxxxxxxxx>:
> > On Tue, Jun 13, 2023 at 12:17:06PM +0200, Frank Oltmanns wrote:
> >> Hi Maxime,
> >>
> >> I'll now only respond to one aspect of your mail, because it's the
> >> foundation for the whole behaviour.
> >>
> >> On 2023-06-13 at 11:10:08 +0200, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> >> [...]
> >>>>>> …
> >>>>>
> >>>>> 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.
> >>>>>
> >>>>
> >>>> I'm not sure if we're talking about the same thing here. Of course the
> >>>> set of factors and parent rate for a given target rate will be different
> >>>> depending on the fact if we can or cannot adjust the parent rate,
> >>>> agreed?
> >>>
> >>> Yes, but here you also have a different behaviour in clk_round_rate()
> >>> and in clk_set_rate(), which isn't ok.
> >>>
> >>> Basically, clk_set_rate() + clk_get_rate() must be equal to
> >>> clk_round_rate().
> >>>
> >>> If you change if you look for parents depending on whether you're being
> >>> called in clk_round_rate() and clk_set_rate(), then you're breaking that
> >>> expectation.
> >>>
> >>>> Let me compare my implementation to ccu_mp.
> >>>>
> >>>> ccu_mp_round_rate either calls the function ccu_mp_find_best or
> >>>> ccu_mp_find_best_with_parent_adj, depending on CLK_SET_RATE_PARENT.
> >>>
> >>> Yes, and it's fine: the flag is per-clock, and the output is the same
> >>> depending on whether we're being called by clk_round_rate() and
> >>> clk_set_rate().
> >>>
> >>
> >> The output is really not the same.
> >>
> >> ccu_mp_set_rate() always calls ccu_mp_find_best(). It never (!) considers
> >> changing the parent, independent of any flags.
> >>
> >> ccu_mp_round_rate() is calling ccu_mp_find_best() OR
> >> ccu_mp_find_best_with_parent_adj() depending on the flag.
> >>
> >> If I understand you correctly, you consider that a bug.
> >
> > No, sorry, you're right.
> >
> > clk_set_rate will call round_rate first, which will (possibly) pick up a
> > new parent, and by the time set_rate is called our parent will have been
> > changed already so we will just call find_best again considering only
> > that parent.
> 
> Ok, no worries. That was my understanding, so your previous statement shattered my worldview. ;) That's why I may have seemed a bit alarmed.
> 
> >
> > The set of factors and dividers should remain the same there, but I
> > don't think that's a concern.
> 
> Ack. The output is stable when called with the same rate.
> 
> > That leaves us with the rounding stuff, and the overall function
> > arguments. I like the structure of ccu_mp better, is there a reason to
> > deviate from it?
> 
> I'm still pondering the rounding stuff. I'm just not sure why you are
> so relaxed about the fact that when calling round_rate with 449064000
> we get 449035712, but when we call get round_rate with 449035712 we
> get 449018181, and when we call round_rate with 449018181, we get
> 449018180.

I guess there's a couple of reasons:

  - You mentioned that you were going to fix the rate issue later :)

  - At the end of the day, it's not a huge offset and shouldn't cause
    any big trouble

> But ultimately, you have the final word, of course. But I need some
> time to be sure, that this does not become a problem in some cases.

Don't get me wrong, it should be fixed (and ideally, we should get some
unit tests to make sure that it doesn't behave that way). I don't think
it's urgent though, or that we introduce workarounds in one particular
clock type.

So we can definitely focus on the parent stuff first, and then get the
rate stuff figured out.

Maxime

Attachment: signature.asc
Description: PGP signature