Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer
From: Sean Anderson
Date: Tue Jul 13 2021 - 17:49:52 EST
On 7/12/21 3:49 PM, Uwe Kleine-König wrote:
Hello Sean,
On Mon, Jul 12, 2021 at 12:26:47PM -0400, Sean Anderson wrote:
On 7/8/21 3:43 PM, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Thu, Jul 08, 2021 at 12:59:18PM -0400, Sean Anderson wrote:
> > And what if the consumer comes and requests 49 for their period in the
> > first place? You have the same problem. The rescaling made it worse in
> > this instance, but this is just an unfortunate case study.
>
> I cannot follow. There are cases that are easy and others are hard.
> Obviously I presented a hard case, and just because there are simpler
> cases, too, doesn't mean that implementing the algorithm that must cover
> all cases becomes simple, too. Maybe I just didn't understand what you
> want to say?!
My point is that you cannot just pick a bad case and call the whole
process poor.
Yeah, there are surprises with both approaches. My point is mostly that
if you cannot prevent these surprises with a more complicate algorithm,
then better live with the surprises and stick to the simple algorithm.
(For the sake of completeness: If the request for my 16.4 ns PWM was
.period = 100 ns
.duty_cycle = 49 ns
the hardware should be configured with
.period = 98.4 ns
.duty_cycle = 32.8 ns
. The key difference to the scaling approach is: The computation is easy
and it's clear how it should be implemented. So I don't see how my
approach yields problems.)
It doesn't, which is why I suggested using it below.
I can do the same thing for your proposed process.
In any case, I don't wish to propose that drivers do rescaling in this
manner; I hoped that my below discussion had made that clear.
Yes, After reading the start of your mail I was positively surprised to
see you reasoning for nearly the same things as I do :-)
Though I really would like if we could pick a different name for "the
duration of the initial part of the PWM's cycle". I know "high time" is
not strictly correct for inverted polarity, but duty cycle refers to
percentage everywhere but the Linux kernel...
"active time" would be a term that should be fine. But I think
"duty-cycle" in contrast to "relative duty-cycle" is fine enough and
IMHO we should keep the terms as they are.
It is very easy to misread one for the other, as I did above. I think
active time is a good term.
> > * Hypothetical future users of some kind of round_state function. These
> > users have some kind of algorithm which determines whether a PWM state
> > is acceptable for the driver. Most of the time this will be some kind
> > of accuracy check. What the round_state function returns is not
> > particularly important, because users have the opportunity to revise
> > their request based on what the state is rounded to. However, it is
> > important that each round rate function is consistent in manner that
> > it rounds so that these users
>
> This sentence isn't complete, is it?
this should be finished like
... can manipulate it programmatically.
Then ack.
> > * The apply_state function shall only round the requested period down, and
> > may do so by no more than one unit cycle. If the requested period is
> > unrepresentable by the PWM, the apply_state function shall return
> > -ERANGE.
>
> I don't understand what you mean by "more than one unit cycle", but
> that doesn't really matter for what I think is wrong with that
> approach: I think this is a bad idea if with "apply_state" you mean
> the callback each driver has to implement: Once you made all drivers
> conformant to this, someone will argue that one unit cycle is too
> strict.
The intent here is to provide guidance against drivers which round
excessively. That is, a driver which always rounded down to its minimum
period would not be very interesting. And neither would a driver which
did not make a very good effort (such as always rounding to multiples of
10 when it could round to multiples of 3 or whatever). So perhaps
s/shall/should/.
Ah, that's what I formalized as "return the *biggest possible* period
not bigger than the request". fine.
> Or that it's ok to increase the period iff the duty_cycle is 0.
IMO it doesn't matter what the period is for a duty cycle of 0 or 100.
Whatever policy we decide on, the behavior in that case will
So it might even be you who thinks that the request
.period = 15
.duty_cycle = 0
should not be refused just because the smallest implementable period is
16.4 ns :-)
If the general policy is to refuse periods less than the minimum
representable, then this should be refused. Otherwise, it should be
allowed. This edge case is not worth complicating drivers.
> Then you have to adapt all 50 or so drivers to adapt the policy.
Of course, as I understand it, this must be done for your policy as
well.
Well to be fair, yes. But the advantage of my policy is that it
returns success in more situations and so the core (and so the consumer)
can work with that in more cases.
Failure is as important for working with an API as success is.
> Better let .apply_state() do the same as .round_state() and then you can
> have in the core (i.e. in a single place):
>
> def pwm_apply_state(pwm, state):
> rounded_state = pwm_round_state(pwm, state)
> if some_condition(rounded_state, state):
> return -ERANGE
> else:
> pwm->apply(pwm, state)
>
> Having said that I think some_condition should always return False, but
> independant of the discussion how some_condition should actually behave
> this is definitively better than to hardcode some_condition in each
> driver.
And IMO the condition should just be "is the period different"?
So a request of .period = X must result in a real period that's bigger
than X - 1 and not bigger than X, correct?
Yes, if 1 is replaced by a suitable
I think a nice interface for many existing users would be something like
# this ignores polarity and intermediate errors, but that should
# not be terribly difficult to add
def pwm_apply_relative_duty_cycle(pwm, duty_cycle, scale):
state = pwm_get_state(pwm)
state.enabled = True
state = pwm_set_relative_duty_cycle(state, duty_cycle, scale)
rounded_state = pwm_round_state(pwm, state)
if rounded_state.period != state.period:
state = pwm_set_relative_duty_cycle(rounded_state, duty_cycle, scale)
rounded_state = pwm_round_state(pwm, state)
This should be rounded_state, right?! -----------------^^^^^
No, since we just recalculated the duty cycle on the line above.
if duty_cycle and not rounded_state.duty_cycle:
return -ERANGE
return pwm_apply_state(pwm, rounded_state)
(Fixed tabs vs space indention)
I oppose to the duty_cycle and not rounded_state.duty_cycle check. Zero
shouldn't be handled differently to other values. If it's ok to round 32
ns down to 16.4 ns, rounding down 2 ns to 0 should be fine, too.
Also for these consumers it might make sense to allow rounding up
period, so if a consumer requests .period = 32 ns, better yield 32.8 ns
instead of 16.4 ns.
It might. Allowing apply_state to round closest while round_state rounds
down would be a good useability improvement IMO. The exact rounding
behavior of apply_state is not as important as round_state, so the
requirements detailed below could certainly be loosened.
which of course could be implemented both with your proposed semantics
or with mine.
Yeah, and each pwm_state that doesn't yield an error is an advantage.
(OK, you could argue now that period should be round up if a too small
value for period is requested. That's a weighing to reduce complexity in
the lowlevel drivers.)
Again, I don't think we should round period in apply_state, because that
throws % duty cycle (which most drivers care about) out the window. And
we both agree that rescaling duty cycle adds too much complexity for
drivers.
> > * The apply_state function shall only round the requested duty cycle
> > down. The apply_state function shall not return an error unless there
> > is no duty cycle less than the requested duty cycle which is
> > representable by the PWM.
>
> ack. (Side note: Most drivers can implement duty_cycle = 0, so for them
> duty_cycle isn't a critical thing.)
Yes, and unfortunately the decision is not as clear-cut as for period.
Oh, note that up to now we consider different options as the right thing
to do with period. That's not what I would call clear-cut :-)
> > * After applying a state returned by round_state with apply_state,
> > get_state must return that state.
>
> ack.
>
> > The reason that we must return an error when the period is
> > unrepresentable is that generally the duty cycle is calculated based on
> > the period. This change has no affect on future users of round_state,
> > since that function will only return valid periods. Those users will
> > have the opportunity to detect that the period has changed and determine
> > if the duty cycle is still acceptable.
>
> ack up to here.
>
> > However, for existing users, we
> > should also provide the same opportunity.
>
> Here you say: If the period has changed they should get a return value
> of -ERANGE, right? Now what should they do with that. Either they give
> up (which is bad)
No, this is exactly what we want. Consider how period is set. Either
it is whatever the default is (e.g. set by PoR or the bootloader), in
which case it is a driver bug if we think it is unrepresentable, or it
is set from the device tree (or platform info), in which case it is a
bug in the configuration. This is not something like duty cycle where
you could make a case depending on the user, but an actual case of
misconfiguration.
That is very little cooperative. The result is that the pwm-led driver
fails to blink because today the UART driver was probed before the
pwm-led and changed a clk in a way that the pwm-led cannot achieve the
configured period any more.
This is fine. There are two options for such a board. First, don't set
the period explicitly. You can always just leave the period at whatever
the default is. If you don't actually care about the period (since it
could change every boot), then you shouldn't be setting it explicitly.
Second, add an `assigned-clock-frequencies` property to the clocks node
and remove the race condition.
If you don't grab the clock rate, then you can easily have some
interaction like
pwm_round_rate()
clk_set_rate()
pwm_apply_state()
where suddenly the period you requested might be unattainable, or could
be rounded completely differently.
> or they need to resort to pwm_round_state to
> find a possible way forward. So they have to belong in the group of
> round_state users and so they can do this from the start and then don't
> need to care about some_condition at all.
>
> > This requirement simplifies
> > the behavior of apply_state, since there is no longer any chance that
> > the % duty cycle is rounded up.
>
> This is either wrong, or I didn't understand you. For my hypothetical
> hardware that can implement periods and duty_cycles that are multiples
> of 16.4 ns the following request:
>
> period = 1650
> duty_cycle = 164
>
> (with relative duty_cycle = 9.9393939393939 %)
> will be round to:
>
> period = 1640
> duty_cycle = 164
>
> which has a higher relative duty_cycle (i.e. 10%).
This is effectively bound by the clause above to be no more than the
underlying precision of the PWM. Existing users expect to be able to
pass unrounded periods/duty cycles, so we need to round in some manner.
Any way we round is OK, as long as it is not terribly excessive (hence
the clause above). We could have chosen to round up (and in fact this is
exactly what happens for inverted polarity PWMs). But I think that for
ease of implementation is is better to mostly round in the same manner
as round_state.
> > This requirement is easy to implement in
> > drivers as well. Instead of writing something like
> >
> > period = clamp(period, min_period, max_period);
> >
> > they will instead write
> >
> > if (period < min_period || period > max_period)
> > return -ERANGE;
>
> Are you aware what this means for drivers that only support a single
> fixed period?
This is working as designed. Either the period comes from configuration
(e.g. pwm_init_state), which is specialized to the board in question, in
which case it is OK to return an error because the writer of the dts
either should leave it as the default or specify it correctly, or it
comes from pwm_get_state in which case it is a driver error for
returning a a period which that driver cannot support.
There are two exceptions to the above. First, a fixed period PWM driver
could have its period changed by the parent clock frequency changing.
But I think such driver should just clk_rate_exclusive_get because
otherwise all bets are off. You just have to hope your consumer doesn't
care about the period.
The other exception is pwm_clk. In this case, I think it is reasonable
to pass an error on if the user tries to change the frequency of what is
effectively a fixed-rate clock.
> I still think it should be:
>
> if (period < min_period)
> return -ERANGE;
>
> if (period > max_period)
> period = max_period;
>
> There are two reasons for this compared to your suggestion:
>
> a) Consider again the 16.4 ns driver and that it is capable to
> implement periods up to 16400 ns. With your approach a request of
> 16404 ns will yield -ERANGE.
> Now compare that with a different 16.4 ns driver with max_period =
> 164000 ns. The request of 16404 ns will yield 16400 ns, just because
> this driver could also do 16416.4 ns. This is strange, because the
> possibility to do 16416.4 ns is totally irrelevant here, isn't it?
Ah, it looks like I mis-specified this a little bit. My intent was
The apply_state function shall only round the requested period
down, and should do so by no more than one unit cycle. If the
period *rounded as such* is unrepresentable by the PWM, the
apply_state function shall return -ERANGE.
I don't understand "one unit cycle". What is a unit cycle for a PWM that
can implement periods in the form 10 s / X for X in [1, ... 4096]? What
is a unit cycle for a fixed period PWM?
One... quanta? step? detent? The idea is that the driver should give its
best approximation which doesn't just round everything down to the
maximum period. So if I were to better capture my above sentiment, I
would suggest
The apply_state function shall round the requested period down
to the largest representable period less than or equal to the
requested period. However, it may not round a period larger than
the largest representable period by more than the difference
between the largest representable period and the next largest
representable period. If no such period exists, or the period is
too large to be rounded, the apply_state function shall return
-ERANGE.
In your example, periods of 15s would be rounded down to 10s, since the
difference between the largest period (10s) and the next largest (5s) is
5s. Unfortunately, when the difference between adjacent periods is a
substantial fraction of the period itself, whatever the user requested
goes out the window if they don't calculate their duty cycle with a
period which can be represented exactly.
But the above specification would necessitate implementations like
if (period < 15ULL * NSEC_PER_SEC)
period = min(period, 10ULL * NSEC_PER_SEC);
X = 10ULL * NSEC_PER_SEC / period;
which is arbitrary and requires extra instructions to enforce. So I
would prefer this clause as originally stated, since it makes for easier
implementation. And I suspect that the number of users hitting this edge
case is fairly slim.
> b) If a consumer asked for a certain state and gets back -ENORANGE they
> don't know if they should increase or decrease the period to guess a
> state that might be implementable instead.
Because I believe this is effectively a configuration issue, it should
be obvious to the user which direction they need to go. Programmatic
users which want to automatically pick a better period would need to use
round_state instead.
You only consider consumers with a fixed period. Do you want to
explicitly configure all possible periods for a a driver that uses ~ 50%
relative duty cycle but varies period (e.g. the pwm-ir-tx driver)?
From what I can tell this driver doesn't really change the period very
much (mostly just to change standard). I think if you are using a
fixed-period PWM as a carrier frequency, your PWM had better be very
nearly the frequency it should. I would certainly rather get an error
about how my pwm was 40KHz instead of 36KHz than have the driver merrily
continue on. This driver is a very good candidate for round_state,
because it (should) care about what the period is. Even normal PWM
rounding could cause you to completely miss your target frequency (e.g.
request 38KHz, rounded to 36KHz). Of course, this driver doesn't even
check the result of pwm_config, so all bets are off.
And yes, I admit that if you had (e.g.) a driver with a fixed-period PWM
which you were using for IR, and you applied these rules, then it would
break your setup. But I think it is fairly easy to hold off on
converting fixed-frequency PWMs used for IR until the consumer drivers
were converted to round_state.
(OK, these drivers could use pwm_round_rate(), but then that argument
could be applied to all consumers and the result of an unrounded request
doesn't really matter any more.)
> (Hmm, or are you only talking about .apply_state and only .round_state
> should do if (period < min_period) return -ERANGE; if (period >
> max_period) period = max_period;?
Yes.
I really want to have .apply_state() and .round_state() to behave
exactly the same. Everything else I don't want to ask from driver
authors. I don't believe you can argue enough that I will drop this
claim.
And yet, round_state does not exist yet. Why should we match apply_state
behavior with something which is not implemented? You've noted how
certain checks can be implemented with round_state, but until then it is
better to raise an error in the driver.
> If so, I'd like to have this in the framework, not in each driver.
> Then .round_state and .apply_state can be identical which is good for
> reducing complexity.)
So you would like each PWM driver to have a "max_period" and
"min_period" parameter?
No, I don't want this explicitly. What did I write to make you think
that? With .round_state() these values can be found out though.
And what if the clock rate changes?
See discussion above about exclusive get.
Otherwise, how do you propose that
the framework detect when a requested period is out of range?
I call round_rate() with
.period = requested_period
.duty_cycle = requested_period
and if that returns -ERANGE the PWM doesn't support this rate (and
smaller ones). And a big requested period is never out of range.
And what do you do about e.g. pwm-sifive where the min/max can change at
any time?
--Sean