Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer

[Uwe Kleine-König]

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

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

> > > * 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 :-)

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

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

> 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?! -----------------^^^^^

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

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

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

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

> >   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)?
(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.

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

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature