Date:   Sat, 5 Dec 2020 20:25:10 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Sean Young <sean@...s.org>
Cc:     Lino Sanfilippo <LinoSanfilippo@....de>, thierry.reding@...il.com,
        lee.jones@...aro.org, nsaenzjulienne@...e.de, f.fainelli@...il.com,
        rjui@...adcom.com, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com, linux-pwm@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic
 configuration

Hello Sean,

On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> On Sat, Dec 05, 2020 at 12:28:34AM +0100, Uwe Kleine-König wrote:
> > On Fri, Dec 04, 2020 at 11:38:46AM +0000, Sean Young wrote:
> > > On Fri, Dec 04, 2020 at 12:13:26PM +0100, Uwe Kleine-König wrote:
> > > > On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> > > > > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > > > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > > > > you are sure that this won't discard relevant bits, please explain
> > > > > > > this in a comment for the cursory reader.
> > > > > > 
> > > > > > > Also note that round_closed is probably wrong, as .apply() is
> > > > > > > supposed to round down the period to the next achievable period. (But
> > > > > > > fixing this has to do done in a separate patch.)
> > > > > > 
> > > > > > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > > > > > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > > > > > I dont know how strong the requirement is to round down the period in apply(), but I
> > > > > > can imagine that this may be a good reason to deviate from this rule.
> > > > > > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> > > > > 
> > > > > There was a problem where the carrier is incorrect for some IR hardware
> > > > > which uses a carrier of 455kHz. With periods that small, rounding errors
> > > > > do really matter and rounding down might cause problems.
> > > > > 
> > > > > A policy of rounding down the carrier is not the right thing to do
> > > > > for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> > > > > edge cases.
> > > > 
> > > > IMO it's not an option to say: pwm-driver A is used for IR, so A's
> > > > .apply uses round-nearest and pwm-driver B is used for $somethingelse,
> > > > so B's .apply uses round-down.
> > > 
> > > I'm not saying that one driver should have one it one way and another driver
> > > another way.
> > 
> > I read between your lines that you think that round-nearest is the
> > single best strategy, is that right?
> 
> Certain the default strategy. When setting a pwm of period X, I would
> expect it set it to the closest period it can match to X. Doing anything
> else by default is a surprising API.

This reminds me of a similar discussion about rounding in the clk
framework which is an unsolved problem, too. It's unspecified how
clk_set_rate and clk_round_rate behave. If you want to operate an IP
block you usually have a fixed upper limit for the clock frequency and
you want the clk as fast as possible. If you operate an UART you want
the nearest match (for some definition of near, see below) to match the
baud rate.

> What real life uses-cases are there for round down? If you want to round
> down, is there any need for round up?

The scenario I have in mind is for driving a motor. I have to admit
however that usually the period doesn't matter much and it's the
duty_cycle that defines the motor's speed. So for this case the
conservative behaviour is round-down to not make the motor run faster
than expected.

For other usecases (fan, backlight, LED) exactness typically doesn't
matter that much.

So we could find a compromise: round period to nearest and duty_cycle
down. But I'm convinced this is a bad compromise because it's quite
unintuitive.

> Hypothetically you may want e.g. nearest to 100kHz but absolutely no less
> than 100kHz. I don't know when this comes up, it would be interesting to
> hear where this is needed.

ack.

> > > Why is is easier to implement?
> > 
> > If pwm_apply_state (and so pwm_round_state) rounds down, you can achieve
> > round-nearest (simplified: Ignoring polarity, just looking for period) using:
> > 
> > 	lower_state = pwm_round_state(pwm, target_state);
> > 	upper_state = {
> > 		.period = 2 * target_state.period - lower_state.period,
> > 		...
> > 	}
> > 	tmp = pwm_round_state(pwm, upper)
> > 
> > 	if tmp.period < target_state.period:
> > 		# tmp == lower_state
> > 		return lower_state
> > 
> > 	else while tmp.period > target_state.period:
> > 		upper = tmp;
> > 		tmp.period -= 1
> > 		tmp = pwm_round_state(pwm, tmp)
> > 
> > I admit it is not pretty. But please try to implement it the other way
> > around (i.e. pwm_round_state rounding to nearest and search for a
> > setting that yields the biggest period not above target.period without
> > just trying all steps). I spend a few brain cycles and the corner cases
> > are harder. (But maybe I'm not smart enough, so please convince me.)
> 
> Ok. Does pwm hardware always work on a linear scale?

No. A quite usual setup is that the PWM hardware has a built-in divider.
The details here vary heavily (range of the divider, some can only
divide by powers of two, or by little integer multiples of powers of two
...)

> > Note that with round-nearest there is another complication: Assume a PWM
> > that can implement period = 500 µs and period = 1000 µs (and nothing
> > inbetween). That corresponds to the frequencies 2000 Hz and 1000 Hz.
> > round_nearest for state with period = 700 µs (corresponding to 1428.5714
> > Hz) would then pick 500 µs (corresponding to 2000 Hz), right? So is
> > round-nearest really what you prefer?
> 
> That is an interesting point. So, I guess the question is: do you want the
> nearest period or the nearest frequency.

I think to match a carrier frequency you want to minimize the deviation
in period, not frequency. (That is, if you want to match 1000 Hz, 950 Hz
is worse than 1050 Hz because with 950 Hz it takes little more than 19
periods ((1/1000) / abs(1/950 - 1/1000)) until you have more than one
period difference compared to 1000 Hz while with 1050 Hz it takes nearly
21 periods ((1/1000) / abs(1/1050 - 1/1000)). (So this was a bit of a
trick question because yes, you should prefer round-nearest, but it
nicely shows the complexity of the topic.)

> > For a quick (and maybe unreliable) overview:
> > 
> > 	$ git grep -l _CLOSEST drivers/pwm/ | wc -l
> > 	15
> > 
> > so we might have 15 drivers that round to nearest and the remaining 40
> > round down. (I checked a few and didn't find a false diagnose.)
> > 
> > For me this isn't a clear indication that round-nearest is
> > unconditionally better.
> 
> Just because some drivers don't use DIV_ROUND_CLOSEST() doesn't mean
> it was considered by the driver author.
> 
> I think some drivers use DIV_ROUND_UP, e.g. pwm-sl28cpld.c.

Yes, still the intention there (see the comment above DIV_ROUND_UP) is
to round down the period. And rounding up the prescaler is right because
a bigger divisor yields a lower period.
 
> So there is no concensus between the pwm drivers as to what should be the
> default.

yes. And that's mostly because for a long time nobody cared for
uniformity. Since some time I ensure for new drivers that they implement
round-down, but touching older drivers is difficult because often there
is no contact to someone who can test it, and even if there is someone,
this doesn't mean others don't depend on the current behaviour.

So this is kind of a chicken-and-egg problem. We should provide the
option to consumers to choose their preferred rounding, but adding an
API function with having lowlevel drivers implementing different
behaviour is quite hard.

> > What is the fact that convinces you that
> > round-nearest is better in general?
> 
> Surely the general use-case is match frequency (or period!) as closely
> as possible.

Sounds tempting, but I'm not convinced enough to think this to be
universally right.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists