Message-ID: <cbe91df4-2a9b-0c75-ce8e-524334681dff@seco.com>
Date:   Tue, 13 Jul 2021 17:49:41 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Emil Lenngren <emil.lenngren@...il.com>,
        michal.simek@...inx.com, Alvaro Gamez <alvaro.gamez@...ent.com>,
        Thierry Reding <thierry.reding@...il.com>,
        kernel@...gutronix.de, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer



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

Powered by blists - more mailing lists