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

[Paul Cercueil]

Le mar. 13 août 2019 à 14:33, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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/  |