On 6/29/21 6:21 PM, Sean Anderson wrote:

On 6/29/21 4:51 PM, Uwe Kleine-König wrote:

quoted

Hello Sean,

On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:

quoted

On 6/29/21 4:31 AM, Uwe Kleine-König wrote:

quoted

Hello Sean,

On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:

quoted

On 6/28/21 1:20 PM, Uwe Kleine-König wrote:

quoted

On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:

quoted

On 6/28/21 12:24 PM, Uwe Kleine-König wrote:

quoted

On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:

quoted

On 6/27/21 2:19 PM, Uwe Kleine-König wrote:

quoted

On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:

IMO, this is the best way to prevent surprising results in the API.

I think it's not possible in practise to refuse "near" misses and every
definition of "near" is in some case ridiculous. Also if you consider
the pwm_round_state() case you don't want to refuse any request to tell
as much as possible about your controller's capabilities. And then it's
straight forward to let apply behave in the same way to keep complexity
low.

quoted

The real issue here is that it is impossible to determine the correct
way to round the PWM a priori, and in particular, without considering
both duty_cycle and period. If a consumer requests very small
period/duty cycle which we cannot produce, how should it be rounded?

Yeah, because there is no obviously right one, I picked one that is as
wrong as the other possibilities but is easy to work with.

quoted

Should we just set TLR0=1 and TLR1=0 to give them 66% duty cycle with
the least period? Or should we try and increase the period to better
approximate the % duty cycle? And both of these decisions must be made
knowing both parameters. We cannot (for example) just always round up,
since we may produce a configuration with TLR0 == TLR1, which would
produce 0% duty cycle instead of whatever was requested. Rounding rate
will introduce significant complexity into the driver. Most of the time
if a consumer requests an invalid rate, it is due to misconfiguration
which is best solved by fixing the configuration.

In the first step pick the biggest period not bigger than the requested
and then pick the biggest duty cycle that is not bigger than the
requested and that can be set with the just picked period. That is the
behaviour that all new drivers should do. This is somewhat arbitrary but
after quite some thought the most sensible in my eyes.

And if there are no periods smaller than the requested period?

Then return -ERANGE.

Ok, so instead of

    if (cycles < 2 || cycles > priv->max + 2)
        return -ERANGE;

you would prefer

    if (cycles < 2)
        return -ERANGE;
    else if (cycles > priv->max + 2)
        cycles = priv->max;

The actual calculation is a bit harder to handle TCSR_UDT = 0 but in
principle, yes, but see below.

quoted

But if we do the above clamping for TLR0, then we have to recalculate
the duty cycle for TLR1. Which I guess means doing something like

    ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
    if (ret)
        return ret;

    state->duty_cycle = mult_frac(state->duty_cycle,
                      xilinx_timer_get_period(priv, tlr0, tcsr0),
                      state->period);

    ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
    if (ret)
        return ret;

No, you need something like:

    /*
     * The multiplication cannot overflow as both priv_max and
     * NSEC_PER_SEC fit into an u32.
     */
    max_period = div64_ul((u64)priv->max * NSEC_PER_SEC, clkrate);

    /* cap period to the maximal possible value */
    if (state->period > max_period)
        period = max_period;
    else
        period = state->period;

    /* cap duty_cycle to the maximal possible value */
    if (state->duty_cycle > max_period)
        duty_cycle = max_period;
    else
        duty_cycle = state->duty_cycle;

These caps may increase the % duty cycle.

Correct.

For some usecases keeping the relative duty cycle might be better, for
others it might not. I'm still convinced that in general my solution
makes sense, is computationally cheaper and easier to work with.

Can you please describe one of those use cases? Every PWM user I looked
(grepping for pwm_apply_state and pwm_config) set the duty cycle as a
percentage of the period, and not as an absolute time. Keeping the high
time the same while changing the duty cycle runs contrary to the
assumptions of all of those users.

quoted

quoted

quoted

    period_cycles = period * clkrate / NSEC_PER_SEC;

    if (period_cycles < 2)
        return -ERANGE;

    duty_cycles = duty_cycle * clkrate / NSEC_PER_SEC;

    /*
     * The hardware cannot emit a 100% relative duty cycle, if
     * duty_cycle >= period_cycles is programmed the hardware emits
     * a 0% relative duty cycle.
     */
    if (duty_cycle == period_cycles)
        duty_cycles = period_cycles - 1;

    /*
     * The hardware cannot emit a duty_cycle of one clk step, so
     * emit 0 instead.
     */
    if (duty_cycles < 2)
        duty_cycles = period_cycles;

Of course, the above may result in 100% duty cycle being rounded down to
0%. I feel like that is too big of a jump to ignore. Perhaps if we
cannot return -ERANGE we should at least dev_warn.

You did it again. You picked one single case that you consider bad but
didn't provide a constructive way to make it better.

Sure I did. I suggested that we warn. Something like

if (duty_cycles == period_cycles)
     if (--duty_cycles < 2)
         dev_warn(chip->dev, "Rounding 100%% duty cycle down to 0%%; pick a longer period\n");

or

if (period_cycles < 2)
     return -ERANGE;
else if (period_cycles < 10)
     dev_notice(chip->dev,
            "very short period of %u cycles; duty cycle may be rounded to 0%%\n",
            period_cycles);

Because 90% of the time, if a user requests such a short period it is
due to a typo or something similar. And if they really are doing it
intentionally, then they should just set duty_cycle=0.

quoted

Assume there was already a pwm_round_state function (that returns the
state that pwm_apply_state would implement for a given request) Consider
a consumer that wants say a 50% relative duty together with a small
period. So it first might call:

    ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)

to find out if .period = 20 can be implemented with the given PWM. If
this returns rounded state as:

    .period = 20
    .duty_cycle = 0

this says quite a lot about the pwm if the driver implements my policy.
(i.e.: The driver can do 20ns, but the biggest duty_cycle is only 0).
If however it returns -ERANGE this means (assuming the driver implements
the policy I try to convice you to be the right one) it means: The
hardware cannot implement 20 ns (or something smaller) and so the next
call probably tries 40 ns.

With your suggested semantic -ERANGE might mean:

  - The driver doesn't support .period = 20 ns
    (Follow up questions: What period should be tried next? 10 ns? 40
    ns? What if this returns -ERANGE again?)
  - The driver supports .period = 20 ns, but the biggest possible
    duty_cycle is "too different from 20 ns to ignore".

Then how should the search continue?

round_rate does not have to use the same logic as apply_state. That is,

     ret = pwm_round_rate(pwm, { .period = 20, .duty_cycle = 20, ... }, &rounded_state)

could be rounded to

     { .period = 20, .duty_cycle = 0 }

but calling

     ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 20, ... })

could return -ERANGE. However, calling

     ret = pwm_apply_state(pwm, { .period = 20, .duty_cycle = 0, ... })

should work just fine, as the caller clearly knows what they are getting
into. IMO this is the best way to allow hypothetical round_rate users to
find out the edges of the PWM while still protecting existing users.

It's perfectly fine to round

     { .period = 150, .duty_cycle = 75 }

to

     { .period = 100, .duty_cycle = 75 }

in round_rate. But doing the same thing for apply_state would be very
surprising to every existing PWM user.

IMO the following invariant should hold

     apply_state(round_rate(x))
     assert(x == get_state())

this should be

       apply_state(round_rate(x))
       assert(round_rate(x) == get_state())

but the following should not necessarily hold

     apply_state(x)
     assert(round_rate(x) == get_state())

Of course, where it is reasonable to round down, we should do so. But
where the result may be surprising, then the caller should specify the
rounded state specifically. It is better to fail loudly and noisily than
to silently accept garbage.

--Sean

quoted

So: As soon as there is a pwm_round_rate function this can be catched
and then it's important that the drivers adhere to the expected policy.
Implementing this is a big thing and believe me I already spend quite
some brain cycles into it. Once the core is extended accordingly I will
be happy about each driver already doing the right thing.

Best regards
Uwe