Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil <paul@crapouillou.net>]

Le mar. 13 août 2019 à 14:33, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@pengutronix.de> a écrit :
> Hello Paul,
> 
> On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
>>  Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > [adding Stephen Boyd to Cc]
>>  >
>>  > On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote:
>>  > > Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit :
>>  > > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
>>  > > > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit :
>>  > > > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil 
>> wrote:
>>  > > > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a 
>> écrit :
>>  > > > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul 
>> Cercueil wrote:
>>  > > > > > > > > [...]
>>  > > > > > > > >  +	/* Reset the clock to the maximum rate, and 
>> we'll reduce it if needed */
>>  > > > > > > > >  +	ret = clk_set_max_rate(clk, parent_rate);
>>  > > > > > > >
>>  > > > > > > > What is the purpose of this call? IIUC this limits 
>> the allowed range of
>>  > > > > > > > rates for clk. I assume the idea is to prevent other 
>> consumers to change
>>  > > > > > > > the rate in a way that makes it unsuitable for this 
>> pwm. But this only
>>  > > > > > > > makes sense if you had a notifier for clk changes, 
>> doesn't it? I'm
>>  > > > > > > > confused.
>>  > > > > > >
>>  > > > > > > Nothing like that. The second call to 
>> clk_set_max_rate() might have set
>>  > > > > > > a maximum clock rate that's lower than the parent's 
>> rate, and we want to
>>  > > > > > > undo that.
>>  > > > > >
>>  > > > > > I still don't get the purpose of this call. Why do you 
>> limit the clock
>>  > > > > > rate at all?
>>  > > > >
>>  > > > > As it says below, we "limit the clock to a maximum rate 
>> that still gives
>>  > > > > us a period value which fits in 16 bits". So that the 
>> computed hardware
>>  > > > > values won't overflow.
>>  > > >
>>  > > > But why not just using clk_set_rate? You want to have the 
>> clock running
>>  > > > at a certain rate, not any rate below that certain rate, 
>> don't you?
>>  > >
>>  > >  I'll let yourself answer yourself:
>>  > >  https://patchwork.ozlabs.org/patch/1018969/
>>  >
>>  > In that thread I claimed that you used clk_round_rate wrongly, 
>> not that
>>  > you should use clk_set_max_rate(). (The claim was somewhat 
>> weakend by
>>  > Stephen, but still I think that clk_round_rate is the right 
>> approach.)
>> 
>>  Well, you said that I shouln't rely on the fact that 
>> clk_round_rate() will
>>  round down. That completely defeats the previous algorithm. So 
>> please tell
>>  me how to use it correctly, because I don't see it.
> 
> Using clk_round_rate correctly without additional knowledge is hard. 
> If
> you assume at least some sane behaviour you'd still have to call it
> multiple times. Assuming maxrate is the maximal rate you can handle
> without overflowing your PWM registers you have to do:
> 
> 	rate = maxrate;
> 	rounded_rate = clk_round_rate(clk, rate);
> 	while (rounded_rate > rate) {
> 		if (rate < rounded_rate - rate) {
> 			/*
> 			 * clk doesn't support a rate smaller than
> 			 * maxrate (or the round_rate callback doesn't
> 			 * round consistently).
> 			 */
> 			 return -ESOMETHING;
> 		}
> 		rate = rate - (rounded_rate - rate)
> 		rounded_rate = clk_round_rate(clk, rate);
> 	}
> 
> 	return rate;
> 
> Probably it would be sensible to put that in a function provided by 
> the
> clk framework (maybe call it clk_round_rate_down and maybe with
> additional checks).

clk_round_rate_down() has been refused multiple times in the past for 
reasons that Stephen can explain.


> 
>>  I came up with a much smarter alternative, that doesn't rely on the 
>> rounding
>>  method of clk_round_rate, and which is better overall (no loop 
>> needed). It
>>  sounds to me like you're bashing the code without making the effort 
>> to
>>  understand what it does.
>> 
>>  Thierry called it a "neat trick"
>>  (https://patchwork.kernel.org/patch/10836879/) so it cannot be as 
>> bad as you
>>  say.
> 
> Either that or Thierry failed to see the downside. The obvious 
> downside
> is that once you set the period to something long (and so the clk was
> limited to a small frequency) you never make the clock any faster
> afterwards.

Read the algorithm again.


> 
> Also I wonder how clk_set_max_rate() is supposed to be used like that 
> or
> if instead some work should be invested to make it easier for clk
> consumers to use clk_round_rate() (e.g. by providing helper functions
> like the above). Stephen, can you shed some light into this?
> 
>>  > The upside of clk_round_rate is that it allows you to test for the
>>  > capabilities of the clock without actually changing it before you 
>> found
>>  > a setting you consider to be good.
>> 
>>  I know what clk_round_rate() is for. But here we don't do 
>> trial-and-error to
>>  find the first highest clock rate that works, we compute the 
>> maximum clock
>>  we can use and limit the clock rate to that.
>> 
>>  >
>>  > >  It's enough to run it below a certain rate, yes. The actual 
>> rate
>>  > > doesn't
>>  > >  actually matter that much.
>>  >
>>  > 1 Hz would be fine? I doubt it.
>> 
>>  We use the highest possible clock rate. We wouldn't use 1 Hz unless 
>> it's the
>>  highest clock rate available.
> 
> That's wrong. If the clk already runs at 1 Hz and you call
> clk_set_max_rate(rate, somethingincrediblehigh); it still runs at 1 Hz
> afterwards. (Unless I missed something.)

You missed something. I reset the max rate to the parent clock's rate 
at the beginning of the algorithm. It works just fine.


> 
>>  > > > >  E.g. if at a rate of 12 MHz your computed hardware value 
>> for the period
>>  > > > >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 
>> bits. So the clock
>>  > > > >  rate must be reduced to the highest possible that will 
>> still give you a
>>  > > > >  < 16-bit value.
>>  > > > >
>>  > > > >  We always want the highest possible clock rate that works, 
>> for the sake of
>>  > > > >  precision.
>>  > > >
>>  > > > This is dubious; but ok to keep the driver simple. (Consider 
>> a PWM that
>>  > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns 
>> and a duty
>>  > > > cycle of 40 ns is requested you can get an exact match with 
>> 25 MHz, but
>>  > > > not with 30 MHz.)
>>  > >
>>  > > The clock rate is actually (parent_rate >> (2 * x) )
>>  > > for x = 0, 1, 2, ...
>>  > >
>>  > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, 
>> and the
>>  > > next one is 1.875 MHz. It'd be very unlikely that you get a 
>> better match at
>>  > > a lower clock.
>>  >
>>  > If the smaller freqs are all dividers of the fastest that's fine. 
>> Please
>>  > note in a code comment that you're assuming this.
>> 
>>  No, I am not assuming this. The current driver just picks the 
>> highest clock
>>  rate that works. We're not changing the behaviour here.
> 
> But you hide it behind clk API functions that don't guarantee this
> behaviour. And even if it works for you it might not for the next 
> person
> who copies your code to support another hardware.

Again, I'm not *trying* to guarantee this behaviour.


> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |