Message-ID: <ad61c979-62e0-d2e4-7d20-72304e515ded@seco.com>
Date:   Mon, 12 Jul 2021 12:26:47 -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/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. 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.

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

>> > You might find a way around that (maybe you have to round up in the
>> > adaption of duty_cycle, I didn't convince myself this is good enough
>> > though).
>> >
>> > So your suggestion to adapt the duty_cycle to keep the relative
>> > duty_cycle constant (as good as possible within the bounds the hardware
>> > dictates) implies additional complication at the driver level.
>> >
>> >  From a framework maintainer's point of view (and also from a low-level
>> > driver maintainer's point of view) I prefer one complication in a
>> > generic function over a complication that I have to care for in each and
>> > every low-level driver by a big margin.
>>
>> FWIW what you're suggesting is also complex for the low-level driver.
>
> Well, it is as complex as necessary and simpler than adapting the
> duty_cycle as you suggested.
>> [...]
>> > Can you please come up with an algorithm to judge if a given deviation
>> > is reasonable or surprising? I agree there are surprises and some of
>> > them are obviously bad. For most cases however the judgement depends on
>> > the use case so I fail to see how someone should program such a check
>> > that should cover all consumers and use cases. I prefer no precautions +
>> > an easy relation between pwm_round_state and pwm_apply_state (i.e.
>> > behave identically) over a most of the time(?) useless precaution and
>> > some policy defined differences between pwm_round_state and
>> > pwm_apply_state
>>
>> After thinking it over, I believe I agree with you on most things, but I
>> think your proposed API has room for additional checks without any loss
>> of generality.
>
> \o/
>
>> The PWM subsystem has several major players:
>>
>> * Existing users of the PWM API. Most of these do not especially care
>>   about the PWM period, usually just leaving at the default. The
>>   exception is of course the pwm-clk driver. Many of these users care
>>   about % duty cycle, and they all calculate the high time based on the
>>   configured period of the PWM. I suspect that while many of these users
>>   have substantial leeway in what accuracy they expect from the % duty
>>   cycle, significant errors (in the 25-50% range) are probably unusual
>>   and indicative of a misconfigured period. Unfortunately, we cannot
>>   make a general judgement about what sort of accuracy is OK in most
>>   cases.
>
> ack.
>
>> * 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.

> One thing I consider important is
> that there is a policy which of the implementable states is returned for
> a given request to make it efficient to search for a best state
> (depending on what the consumer driver considers best). Otherwise this
> yields to too much distinctions of cases.
>
>> * Existing drivers for the PWM subsystem. These drivers must implement
>>   an apply_state function which is correct for both existing and future
>>   users. In addition, they may implement some kind of round_state
>>   function in the future. it is important to reduce the complexity of
>>   the calculations these drivers perform so that it is easier to
>>   implement and review them.
>
> It's hard to know what "correct" means. But ack for "They should not be
> more complex than necessary".
>
>> I believe the following requirements satisfy the above constraints:
>>
>> * The round_state function shall round the period to the largest period
>>   representable by the PWM less than the requested period. It shall also
>>   round the duty cycle to the largest duty cycle representable by the
>>   PWM less than the requested duty cycle. No attempt shall be made to
>>   preserve the % duty cycle.
>
> ack if you replace "less" by "less or equal" twice.

Yes.

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

> 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

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

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

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)
             if duty_cycle and not rounded_state.duty_cycle:
	        return -ERANGE
	    return pwm_apply_state(pwm, rounded_state)

which of course could be implemented both with your proposed semantics
or with mine.

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

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

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

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

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

> 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? And what if the clock rate changes? Otherwise,
how do you propose that the framework detect when a requested period is
out of range?

>> Instead of viewing round_state as "what get_state would return if I
>> passed this state to apply_state", it is better to view it as "what is
>> the closest exactly representable state with parameters less than this
>> state."
>> I believe that this latter representation is effectively identical for
>> users of round_state, but it allows for implementations of apply_state
>> which provide saner defaults for existing users.
>
> I look forward to how you modify your claim here after reading my
> reasoning above.

I believe it stands as-is.

--Sean

Powered by blists - more mailing lists