Message-ID: <20190813140903.rdwy7p3mhwetmlnt@pengutronix.de>
Date:   Tue, 13 Aug 2019 16:09:03 +0200
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Paul Cercueil <paul@...pouillou.net>,
        Stephen Boyd <sboyd@...nel.org>
Cc:     Thierry Reding <thierry.reding@...il.com>, od@...c.me,
        linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mathieu Malaterre <malat@...ian.org>,
        Artur Rojek <contact@...ur-rojek.eu>
Subject: Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

Hello Paul,

On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote:
> Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit :
> > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote:
> > > 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'd be really interested in these reasons as I think the clk framework
should make it easy to solve common tasks related to clocks. And finding
out the biggest supported rate not bigger than a given maxrate is
something I consider such a common task.

The first hit I found when searching was
https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that
clk_round_rate with the current semantic is hardly useful and suggested
clk_round_rate_up() and clk_round_rate_down() himself.
 
> > >  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.

I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I
grepped for clk_set_rate before claiming the code was broken. Sorry.

So I think the code works indeed, but it feels like abusing
clk_set_max_rate. So I'd like to see some words from Stephen about this
procedure.

Also I think this is kind of inelegant to set the maximal rate twice. At
least call clk_set_max_rate only once please.

> > > > > > >  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.

I didn't request you should guarantee this behaviour. I want you to make
it obvious for readers of your code that you rely on something that
isn't guaranteed. That your code works today isn't a good enough excuse.
There are various examples like these. If you want a few:

 - printf("string: %s\n", NULL); works fine with glibc, but segfaults on
   other libcs.
 - setenv("MYVAR", NULL) used to work (and was equivalent to
   setenv("MYVAR", "")) but that was never guaranteed. Then at some
   point of time it started to segfault.
 - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. This
   used to work fine until compilers were changed to optimize more
   aggressively.

Now if you use a clk and know that all rates smaller than the requested
one are divisors of the fast one and your code only works (here: is
optimal) when this condition is given, you're walking on thin ice just
because this fact it's not guaranteed.
The least you can do is to add a code comment to make people aware who
debug the breakage or copy your code.

I admit this wasn't optimal already before, but at least the logic was
in the same code and not hidden behind the clk API.

Please do people who review or copy your code the favour to document the
assumptions you're relying on. And if it's only to save some time for
someone who stumbles over your code who knows the clk API and starts
thinking about improving the driver.

Best regards
Uwe

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

Powered by blists - more mailing lists