Hello Sean,

On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
> On 6/25/21 2:19 AM, Uwe Kleine-K?nig wrote:
> > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
> >> + * Hardware limitations:

Please make this "* Limitations:" to match what the other drivers do and
so ease grepping for this info.

> >> + * - When changing both duty cycle and period, we may end up with one cycle
> >> + * with the old duty cycle and the new period.
> >
> > That means it doesn't reset the counter when a new period is set, right?
>
> Correct. The only way to write to the counter is to stop the timer and
> restart it.

ok.

> >> + * - Cannot produce 100% duty cycle.
> >
> > Can it produce a 0% duty cycle? Below you're calling
> > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
>
> Yes. This is what you get when you try to specify 100% duty cycle (e.g.
> TLR0 == TLR1).

OK, so the hardware can do it, but your driver doesn't make use of it,
right?

> >> + * - Only produces "normal" output.
> >
> > Does the output emit a low level when it's disabled?
>
> I believe so.

Is there a possibility to be sure? I'd like to know that to complete my
picture about the behaviour of the supported PWMs.

> >> + */
> >> +
> >> [...]
> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> >> + const struct pwm_state *state)
> >> +{
> >> + int ret;
> >> + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> >> + u32 tlr0, tlr1;
> >> + u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> >> + u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> >> + bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> >> +
> >> + if (state->polarity != PWM_POLARITY_NORMAL)
> >> + return -EINVAL;
> >> +
> >> + ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
> >> + if (ret)
> >> + return ret;
> >
> > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
> > -ERANGE for big periods. The good behaviour to implement is to cap to
> > the biggest period possible in this case.
>
> Ok. Is this documented anywhere?

I tried but Thierry didn't like the result and I didn't retry. The
problem is also that many drivers we already have in the tree don't
behave like this (because for a long time nobody cared). That new
drivers should behave this way is my effort to get some consistent
behaviour.

> And wouldn't this result in the wrong duty cycle? E.g. say the max
> value is 100 and I try to apply a period of 150 and a duty_cycle of 75
> (for a 50% duty cycle). If we cap at 100, then I will instead have a
> 75% duty cycle, and there will be no error.

Yes that is right. That there is no feedback is a problem that we have
for a long time. I have a prototype patch that implements a
pwm_round_state() function that lets a consumer know the result of
applying a certain pwm_state in advance. But we're not there yet.

> So I will silently get the wrong duty cycle, even when that duty cycle
> is probably more important than the period.

It depends on the use case and every policy is wrong for some cases. So
I picked the policy I already explained because it is a) easy to
implement for lowlevel drivers and b) it's easy to work with for
consumers once we have pwm_round_state().

> > Also note that state->period is an u64 but it is casted to unsigned int
> > as this is the type of the forth parameter of xilinx_timer_tlr_period.
>
> Hm, it looks like I immediately cast period to a u64. I will change the
> signature for this function next revision.

Then note that period * clk_get_rate(priv->clk) might overflow.

> >> + ret = xilinx_timer_tlr_period(priv, &tlr1, tcsr1, state->duty_cycle);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + xilinx_timer_write(priv, tlr0, TLR0);
> >> + xilinx_timer_write(priv, tlr1, TLR1);
> >> +
> >> + if (state->enabled) {
> >> + /* Only touch the TCSRs if we aren't already running */
> >> + if (!enabled) {
> >> + /* Load TLR into TCR */
> >> + xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
> >> + xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
> >> + /* Enable timers all at once with ENALL */
> >> + tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> >> + tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> >> + xilinx_timer_write(priv, tcsr0, TCSR0);
> >> + xilinx_timer_write(priv, tcsr1, TCSR1);
> >> + }
> >> + } else {
> >> + xilinx_timer_write(priv, 0, TCSR0);
> >> + xilinx_timer_write(priv, 0, TCSR1);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> >> + struct pwm_device *unused,
> >> + struct pwm_state *state)
> >> +{
> >> + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> >> + u32 tlr0 = xilinx_timer_read(priv, TLR0);
> >> + u32 tlr1 = xilinx_timer_read(priv, TLR1);
> >> + u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
> >> + u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
> >> +
> >> + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> >> + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> >> + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> >> + state->polarity = PWM_POLARITY_NORMAL;
> >
> > Are the values returned here sensible if the hardware isn't in PWM mode?
>
> Yes. If the hardware isn't in PWM mode, then state->enabled will be
> false.

Ah right. Good enough.

> >> + else if (pwm_cells)
> >> + return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
> >
> > What is the rationale here to not support #pwm-cells = <2>?
>
> Only one PWM is supported. But otherwise there is no particular
> reason.

The usual binding is to have 3 additional parameters.
1) chip-local pwm number (which can only be 0 for a pwmchip having
.npwm = 1)
2) the "typical" period
3) some flags (like PWM_POLARITY_*)

I don't care much if you implement it with or without 1), but 2) and 3)
should IMHO be here. If you don't want 1),
http://patchwork.ozlabs.org/project/linux-pwm/patch/[email protected]/
might be interesting for you. (But note, Thierry didn't give feedback to
this yet, it might be possible he wants 1)-3) for new drivers.)

Best regards
Uwe

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

Attachments:

(No filename) (6.41 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-25 17:47:13

by Sean Anderson

[permalink] [raw]

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

On 6/25/21 12:56 PM, Uwe Kleine-K?nig wrote:
> Hello Sean,
>
> On Fri, Jun 25, 2021 at 11:13:33AM -0400, Sean Anderson wrote:
>> On 6/25/21 2:19 AM, Uwe Kleine-K?nig wrote:
>> > On Fri, May 28, 2021 at 05:45:22PM -0400, Sean Anderson wrote:
>> >> + * Hardware limitations:
>
> Please make this "* Limitations:" to match what the other drivers do and
> so ease grepping for this info.
>
>> >> + * - When changing both duty cycle and period, we may end up with one cycle
>> >> + * with the old duty cycle and the new period.
>> >
>> > That means it doesn't reset the counter when a new period is set, right?
>>
>> Correct. The only way to write to the counter is to stop the timer and
>> restart it.
>
> ok.
>
>> >> + * - Cannot produce 100% duty cycle.
>> >
>> > Can it produce a 0% duty cycle? Below you're calling
>> > xilinx_timer_tlr_period(..., ..., ..., 0) then which returns -ERANGE.
>>
>> Yes. This is what you get when you try to specify 100% duty cycle (e.g.
>> TLR0 == TLR1).
>
> OK, so the hardware can do it, but your driver doesn't make use of it,
> right?

So to clarify, I have observed the following behavior

* When TCSR == 0, the output is constant low.
* When TLR1 (duty_cycle) >= TLR0 (period), the output is constant low.

It might be possible to achieve constant high output by stopping the
counters during the high time, but leaving the PWM bit set. However, I
do not have a use case for this. I think it might be a nice follow-up
for someone who wants that feature.

That being said, is there a standard way to specify 100% or 0% duty
cycle? It seems like one would have to detect these situations and use a
different code path.

>
>> >> + * - Only produces "normal" output.
>> >
>> > Does the output emit a low level when it's disabled?
>>
>> I believe so.
>
> Is there a possibility to be sure? I'd like to know that to complete my
> picture about the behaviour of the supported PWMs.

See above.

>
>> >> + */
>> >> +
>> >> [...]
>> >> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> >> + const struct pwm_state *state)
>> >> +{
>> >> + int ret;
>> >> + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> >> + u32 tlr0, tlr1;
>> >> + u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> >> + u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> >> + bool enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> >> +
>> >> + if (state->polarity != PWM_POLARITY_NORMAL)
>> >> + return -EINVAL;
>> >> +
>> >> + ret = xilinx_timer_tlr_period(priv, &tlr0, tcsr0, state->period);
>> >> + if (ret)
>> >> + return ret;
>> >
>> > The implementation of xilinx_timer_tlr_period (in patch 2/3) returns
>> > -ERANGE for big periods. The good behaviour to implement is to cap to
>> > the biggest period possible in this case.
>>
>> Ok. Is this documented anywhere?
>
> I tried but Thierry didn't like the result and I didn't retry. The
> problem is also that many drivers we already have in the tree don't
> behave like this (because for a long time nobody cared). That new
> drivers should behave this way is my effort to get some consistent
> behaviour.

Do you have a link to the thread? IMO if you would like to specify
behavior like this, is is very helpful to write it down so new authors
don't have to get to v4 before finding out about it ;)

>> And wouldn't this result in the wrong duty cycle? E.g. say the max
>> value is 100 and I try to apply a period of 150 and a duty_cycle of 75
>> (for a 50% duty cycle). If we cap at 100, then I will instead have a
>> 75% duty cycle, and there will be no error.
>
> Yes that is right. That there is no feedback is a problem that we have
> for a long time. I have a prototype patch that implements a
> pwm_round_state() function that lets a consumer know the result of
> applying a certain pwm_state in advance. But we're not there yet.

So for the moment, why not give an error? This will be legal code both
now and after round_state is implemented.

>
>> So I will silently get the wrong duty cycle, even when that duty cycle
>> is probably more important than the period.
>
> It depends on the use case and every policy is wrong for some cases. So
> I picked the policy I already explained because it is a) easy to
> implement for lowlevel drivers and b) it's easy to work with for
> consumers once we have pwm_round_state().

What about sysfs? Right now if you try to specify an inexpressible
period you get an error message. I saw [1], but unfortunately there do
not appear to be any patches associated with it. Do you have plans to
implement such an interface?

[1] https://lore.kernel.org/linux-pwm/CAO1O6seyi+1amAY5YLz0K1dkNd7ewAvot4K1eZMpAAQquz0-9g@mail.gmail.com/

>
>> > Also note that state->period is an u64 but it is casted to unsigned int
>> > as this is the type of the forth parameter of xilinx_timer_tlr_period.
>>
>> Hm, it looks like I immediately cast period to a u64. I will change the
>> signature for this function next revision.
>
> Then note that period * clk_get_rate(priv->clk) might overflow.

Ok, so is mult_frac the correct macro to use here?

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

Perhaps I should add

if (tlr0 <= tlr1)
return -EINVAL;

here to prevent accidentally getting 0% duty cycle.

>> >> +
>> >> + xilinx_timer_write(priv, tlr0, TLR0);
>> >> + xilinx_timer_write(priv, tlr1, TLR1);
>> >> +
>> >> + if (state->enabled) {
>> >> + /* Only touch the TCSRs if we aren't already running */
>> >> + if (!enabled) {
>> >> + /* Load TLR into TCR */
>> >> + xilinx_timer_write(priv, tcsr0 | TCSR_LOAD, TCSR0);
>> >> + xilinx_timer_write(priv, tcsr1 | TCSR_LOAD, TCSR1);
>> >> + /* Enable timers all at once with ENALL */
>> >> + tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
>> >> + tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
>> >> + xilinx_timer_write(priv, tcsr0, TCSR0);
>> >> + xilinx_timer_write(priv, tcsr1, TCSR1);
>> >> + }
>> >> + } else {
>> >> + xilinx_timer_write(priv, 0, TCSR0);
>> >> + xilinx_timer_write(priv, 0, TCSR1);
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
>> >> + struct pwm_device *unused,
>> >> + struct pwm_state *state)
>> >> +{
>> >> + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> >> + u32 tlr0 = xilinx_timer_read(priv, TLR0);
>> >> + u32 tlr1 = xilinx_timer_read(priv, TLR1);
>> >> + u32 tcsr0 = xilinx_timer_read(priv, TCSR0);
>> >> + u32 tcsr1 = xilinx_timer_read(priv, TCSR1);
>> >> +
>> >> + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
>> >> + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
>> >> + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
>> >> + state->polarity = PWM_POLARITY_NORMAL;
>> >
>> > Are the values returned here sensible if the hardware isn't in PWM mode?
>>
>> Yes. If the hardware isn't in PWM mode, then state->enabled will be
>> false.
>
> Ah right. Good enough.
>
>> >> + else if (pwm_cells)
>> >> + return dev_err_probe(dev, -EINVAL, "#pwm-cells must be 0\n");
>> >
>> > What is the rationale here to not support #pwm-cells = <2>?
>>
>> Only one PWM is supported. But otherwise there is no particular
>> reason.
>
> The usual binding is to have 3 additional parameters.
> 1) chip-local pwm number (which can only be 0 for a pwmchip having
> .npwm = 1)
> 2) the "typical" period
> 3) some flags (like PWM_POLARITY_*)
>
> I don't care much if you implement it with or without 1), but 2) and 3)
> should IMHO be here. If you don't want 1),
> http://patchwork.ozlabs.org/project/linux-pwm/patch/[email protected]/
> might be interesting for you. (But note, Thierry didn't give feedback to
> this yet, it might be possible he wants 1)-3) for new drivers.)

Ok, so since I let of_pwmchip_add set xlate, I don't need to change
anything in this driver? Then I will remove this check for v5.

--Sean

2021-06-25 17:47:46

by Sean Anderson

[permalink] [raw]

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

2021-06-27 18:24:55

Hello Sean,

On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> On 6/27/21 2:19 PM, Uwe Kleine-K?nig wrote:
> > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> > > So for the moment, why not give an error? This will be legal code both
> > > now and after round_state is implemented.
> >
> > The problem is where to draw the line. To stay with your example: If a
> > request for period = 150 ns comes in, and let X be the biggest period <=
> > 150 ns that the hardware can configure. For which values of X should an
> > error be returned and for which values the setting should be
> > implemented.
> >
> > In my eyes the only sensible thing to implement here is to tell the
> > consumer about X and let it decide if it's good enough. If you have a
> > better idea let me hear about it.
>
> Sure. And I think it's ok to tell the consumer that X is the best we can
> do. But if they go along and request an unconfigurable state anyway, we
> should tell them as much.

I have the impression you didn't understand where I see the problem. If
you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
expects that it can configure the duty_cycle in 1/256 steps of the
period, and then maybe only steps 27 and 213 of the 256 possible steps
work. (This example doesn't really match because the led-pwm driver
varies duty_cycle and not period, but the principle becomes clear I
assume.) If no: Should we accept 151 ns? Isn't that ridiculous?

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

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

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

> > > Perhaps I should add
> > >
> > > if (tlr0 <= tlr1)
> > > return -EINVAL;
> > >
> > > here to prevent accidentally getting 0% duty cycle.
> >
> > You can assume that duty_cycle <= period when .apply is called.
>
> Ok, I will only check for == then.

You just have to pay attention to the case that you had to decrement
.period to the next possible value. Then .duty_cycle might be bigger
than the corrected period.

Best regards
Uwe

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

Attachments:

(No filename) (3.96 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-28 23:38:14

by Sean Anderson

[permalink] [raw]

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

On 6/28/21 12:24 PM, Uwe Kleine-K?nig wrote:
> Hello Sean,
>
> On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>> On 6/27/21 2:19 PM, Uwe Kleine-K?nig wrote:
>> > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
>> > > So for the moment, why not give an error? This will be legal code both
>> > > now and after round_state is implemented.
>> >
>> > The problem is where to draw the line. To stay with your example: If a
>> > request for period = 150 ns comes in, and let X be the biggest period <=
>> > 150 ns that the hardware can configure. For which values of X should an
>> > error be returned and for which values the setting should be
>> > implemented.
>> >
>> > In my eyes the only sensible thing to implement here is to tell the
>> > consumer about X and let it decide if it's good enough. If you have a
>> > better idea let me hear about it.
>>
>> Sure. And I think it's ok to tell the consumer that X is the best we can
>> do. But if they go along and request an unconfigurable state anyway, we
>> should tell them as much.
>
> I have the impression you didn't understand where I see the problem. If
> you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
> should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
> expects that it can configure the duty_cycle in 1/256 steps of the
> period, and then maybe only steps 27 and 213 of the 256 possible steps
> work. (This example doesn't really match because the led-pwm driver
> varies duty_cycle and not period, but the principle becomes clear I
> assume.) If no: Should we accept 151 ns? Isn't that ridiculous?

I am fine with this sort of rounding. The part I take issue with is when
the consumer requests (e.g.) a 10ns period, but the best we can do is
20ns. Or at the other end if they request a 4s period but the best we
can do is 2s. Here, there is no obvious way to round it, so I think we
should just say "come back with a reasonable period" and let whoever
wrote the device tree pick a better period.

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

Any way you slice it, there will be situations where there is nothing
reasonable to do other than return an error.

>> > > Perhaps I should add
>> > >
>> > > if (tlr0 <= tlr1)
>> > > return -EINVAL;
>> > >
>> > > here to prevent accidentally getting 0% duty cycle.
>> >
>> > You can assume that duty_cycle <= period when .apply is called.
>>
>> Ok, I will only check for == then.
>
> You just have to pay attention to the case that you had to decrement
> .period to the next possible value. Then .duty_cycle might be bigger
> than the corrected period.

This is specifically to prevent 100% duty cycle from turning into 0%. My
current draft is

/*
* If TLR0 == TLR1, then we will produce 0% duty cycle instead of 100%
* duty cycle. Try and reduce the high time to compensate. If we can't
* do that because the high time is already 0 cycles, then just error
* out.
*/
if (tlr0 == tlr1 && !tlr1--)
return -EINVAL;

--Sean

2021-06-28 23:40:34

by Sean Anderson

Hello Sean,

On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
> On 6/29/21 4:31 AM, Uwe Kleine-K?nig wrote:
> > Hello Sean,
> >
> > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
> >> On 6/28/21 1:20 PM, Uwe Kleine-K?nig wrote:
> >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> >> >> On 6/28/21 12:24 PM, Uwe Kleine-K?nig wrote:
> >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-K?nig wrote:
> >> >> > > > On Fri, Jun 25, 2021 at 01:46:26PM -0400, Sean Anderson wrote:
> >> >> > > > > So for the moment, why not give an error? This will be legal code both
> >> >> > > > > now and after round_state is implemented.
> >> >> > > >
> >> >> > > > The problem is where to draw the line. To stay with your example: If a
> >> >> > > > request for period = 150 ns comes in, and let X be the biggest period <=
> >> >> > > > 150 ns that the hardware can configure. For which values of X should an
> >> >> > > > error be returned and for which values the setting should be
> >> >> > > > implemented.
> >> >> > > >
> >> >> > > > In my eyes the only sensible thing to implement here is to tell the
> >> >> > > > consumer about X and let it decide if it's good enough. If you have a
> >> >> > > > better idea let me hear about it.
> >> >> > >
> >> >> > > Sure. And I think it's ok to tell the consumer that X is the best we can
> >> >> > > do. But if they go along and request an unconfigurable state anyway, we
> >> >> > > should tell them as much.
> >> >> >
> >> >> > I have the impression you didn't understand where I see the problem. If
> >> >> > you request 150 ns and the controller can only do 149 ns (or 149.6667 ns)
> >> >> > should we refuse? If yes: This is very unusable, e.g. the led-pwm driver
> >> >> > expects that it can configure the duty_cycle in 1/256 steps of the
> >> >> > period, and then maybe only steps 27 and 213 of the 256 possible steps
> >> >> > work. (This example doesn't really match because the led-pwm driver
> >> >> > varies duty_cycle and not period, but the principle becomes clear I
> >> >> > assume.) If no: Should we accept 151 ns? Isn't that ridiculous?
> >> >>
> >> >> I am fine with this sort of rounding. The part I take issue with is when
> >> >> the consumer requests (e.g.) a 10ns period, but the best we can do is
> >> >> 20ns. Or at the other end if they request a 4s period but the best we
> >> >> can do is 2s. Here, there is no obvious way to round it, so I think we
> >> >> should just say "come back with a reasonable period" and let whoever
> >> >> wrote the device tree pick a better period.
> >> >
> >> > Note that giving ridiculus examples is easy, but this doesn't help to
> >> > actually implement something sensible. Please tell us for your example
> >> > where the driver can only implement 20 ns what is the smallest requested
> >> > period the driver should accept.
> >>
> >> 20ns :)
> >>
> >> In the case of this device, that would result in 0% duty cycle with a
> >> 100MHz input. So the smallest reasonable period is 30ns with a duty
> >> cycle of 20ns.
> >
> > I took the time to understand the hardware a bit better, also to be able
> > to reply to your formulae below. So to recap (and simplify slightly
> > assuming TCSR_UDT = 1):
> >
> >
> > TLR0 + 2
> > period = --------
> > clkrate
> >
> > TLR1 + 2
> > duty_cycle = -------- if TLR1 < TLR0, else 0
> > clkrate
> >
> >
> > where TLRx has the range [0..0xffffffff] (for some devices the range is
> > smaller). So clkrate seems to be 100 MHz?
>
> On my system, yes.
>
> >
> >> >> > > 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.
> >> >> >
> >> >> > > 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.
> >> >> >
> >> >> > > 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.
> >
> >> 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.

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

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?

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

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

Attachments:

(No filename) (10.29 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-29 22:24:50

by Sean Anderson

[permalink] [raw]

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

On 6/29/21 4:51 PM, Uwe Kleine-K?nig wrote:
> Hello Sean,
>
> On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
>> On 6/29/21 4:31 AM, Uwe Kleine-K?nig wrote:
>> > Hello Sean,
>> >
>> > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
>> >> On 6/28/21 1:20 PM, Uwe Kleine-K?nig wrote:
>> >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
>> >> >> On 6/28/21 12:24 PM, Uwe Kleine-K?nig wrote:
>> >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>> >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-K?nig wrote:
>> >> >> > > > 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.
>> >> >> >
>> >> >> > > 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.
>> >> >> >
>> >> >> > > 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.
>> >
>> >> 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.

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

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

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

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

2021-06-29 23:39:43

by Sean Anderson

[permalink] [raw]

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

On 6/29/21 6:21 PM, Sean Anderson wrote:
>
>
> On 6/29/21 4:51 PM, Uwe Kleine-K?nig wrote:
>> Hello Sean,
>>
>> On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
>>> On 6/29/21 4:31 AM, Uwe Kleine-K?nig wrote:
>>> > Hello Sean,
>>> >
>>> > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
>>> >> On 6/28/21 1:20 PM, Uwe Kleine-K?nig wrote:
>>> >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
>>> >> >> On 6/28/21 12:24 PM, Uwe Kleine-K?nig wrote:
>>> >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
>>> >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-K?nig wrote:
>>> >> >> > > > 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.
>>> >> >> >
>>> >> >> > > 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.
>>> >> >> >
>>> >> >> > > 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.
>>> >
>>> >> 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.
>
>>>
>>> >???? 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.
>
>> 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
>
>>
>> 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
>>

2021-06-30 08:36:28

Hello Sean,

On Thu, Jul 08, 2021 at 12:59:18PM -0400, Sean Anderson wrote:
> On 6/30/21 4:35 AM, Uwe Kleine-K?nig wrote:
> > I often mistype the name of the rounding function as "pwm_round_rate",
> > the better name is "pwm_round_state" of course. That's just me thinking
> > about clk_round_rate where ".._rate" is the right term. I'll try harder
> > to get this right from now on.
> >
> > On Tue, Jun 29, 2021 at 06:21:15PM -0400, Sean Anderson wrote:
> > > On 6/29/21 4:51 PM, Uwe Kleine-K?nig wrote:
> > > > On Tue, Jun 29, 2021 at 02:01:31PM -0400, Sean Anderson wrote:
> > > > > On 6/29/21 4:31 AM, Uwe Kleine-K?nig wrote:
> > > > > > On Mon, Jun 28, 2021 at 01:41:43PM -0400, Sean Anderson wrote:
> > > > > >> On 6/28/21 1:20 PM, Uwe Kleine-K?nig wrote:
> > > > > >> > On Mon, Jun 28, 2021 at 12:35:19PM -0400, Sean Anderson wrote:
> > > > > >> >> On 6/28/21 12:24 PM, Uwe Kleine-K?nig wrote:
> > > > > >> >> > On Mon, Jun 28, 2021 at 11:50:33AM -0400, Sean Anderson wrote:
> > > > > >> >> > > On 6/27/21 2:19 PM, Uwe Kleine-K?nig wrote:
> > > > > >> >> > > > 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.
> > > > > >> >> >
> > > > > >> >> > > 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.
> > > > > >> >> >
> > > > > >> >> > > 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.
> > > > > >
> > > > > >> 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.
> >
> > Indeed there is no mainline driver that relies on this. There are some
> > smart LED controllers (e.g. WS2812B) where the duty_cycle is more
> > important than the period. (I admit a PWM is not really the right driver
> > for that one as it could only completely enable and complete disable
> > white color.) Also there are some servo motor chips where the absolute
> > duty is relevant but the period isn't (in some range). (See
> > https://www.mikrocontroller.net/articles/Modellbauservo_Ansteuerung#Signalaufbau
> > for a article about that (in German though).)
>
> > In case you want to argue that out-of-mainline users don't count:
>
> They don't :)
>
> This is a kernel-internal API.
>
> But my point is that the vast majority of PWM users, and 100% of the
> in-kernel users care about the % duty cycle and not about the absolute
> high time. We should design around the common use case first and
> foremost.
>
> > I think in the design of an API they do count to place the bar to
> > enter the mainline low. Frameworks should be generic enough to cover
> > as much use cases as possible.
>
> They can cover many use-cases as possible, but they should be ergonomic
> firstly for the most common use case.

First I think going the easy way now and accepting to have to spend
quite more work later isn't a sensible approach. Second: Your approach
isn't even easier. And third: I agree that it should be easy for
consumers who care more about relative duty cycle than absolute numers.
But that doesn't imply that each and every device driver must provide
exactly that algorithm. The possible ways forward are:

a) The framework has no complexity and all lowlevel drivers have to do
complicated maths;

b) The framework contains the complexity and the lowlevel drivers are
simpler.

I'd choose b) at any time.

> > And note that if you want a nearest to (say) 50% relative duty cycle and
> > don't care much about the period it doesn't really matter if you scale
> > duty_cycle in pwm_round_state() to the period change or not because in
> > general you need several calls to pwm_round_state() anyhow to find a
> > setting with 51% if the next lower possibility is 47%. So in the end you
> > save (I think) one call in generic PWM code.
> >
> > In contrast the math gets quite a bit more complicated because there is
> > rounding involved in scaling the duty cycle. Consider a PWM that can
> > configure period and duty in 16.4 ns steps and you ask for
> >
> > .period = 100 ns
> > .duty_cycle = 50 ns
> >
> > Then the best period you can provide is 98.4 ns, so you return .period =
> > 99 from pwm_round_state(). (Yes, you don't return 98, because
> > round-nearest is much harder to handle than round down.)
> > To determine the adapted duty_cycle you have to do
> >
> > 50 * realperiod / 100
> >
> > which independently of choosing 98, 98.4 or 99 for realperiod is 49. Then
> > to approximate 49 without rounding up you end up with 32.8 while 49.2
> > would have be perfectly fine.
>
> 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?!

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

> * 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. Or that it's
ok to increase the period iff the duty_cycle is 0.

Then you have to adapt all 50 or so drivers to adapt the policy.
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.

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

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

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?

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.

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

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

Best regards
Uwe

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

Attachments:

(No filename) (19.07 kB)
signature.asc (499.00 B)
Download all attachments

2021-07-12 16:28:43

by Sean Anderson

[permalink] [raw]

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

2021-07-12 19:50:28

by Uwe Kleine-König

[permalink] [raw]

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

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

Attachments:

(No filename) (15.26 kB)
signature.asc (499.00 B)
Download all attachments

2021-07-13 21:54:00

by Sean Anderson

[permalink] [raw]

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