Message-ID: <661e52c3-cd79-c2aa-e031-64eef5617be0@seco.com>
Date:   Mon, 28 Jun 2021 13:41:43 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Emil Lenngren <emil.lenngren@...il.com>,
        michal.simek@...inx.com, Alvaro Gamez <alvaro.gamez@...ent.com>,
        Thierry Reding <thierry.reding@...il.com>,
        kernel@...gutronix.de, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 3/3] pwm: Add support for Xilinx AXI Timer



On 6/28/21 1:20 PM, Uwe Kleine-König wrote:
 > Hello Sean,
 >
 > 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.

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

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;



 >
 >> Any way you slice it, there will be situations where there is nothing
 >> reasonable to do other than return an error.
 >
 > ack.
 >
 >> > > > > 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;
 >
 > If you follow my suggested policy this isn't an error and you should
 > yield the biggest duty_cycle here even if it is zero.

So like this?

	if (tlr0 == tlr1) {
		if (tlr1)
			tlr1--;
		else if (tlr0 != priv->max)
			tlr0++;
		else
			return -ERANGE;
	}

And I would really appreciate if you could write up some documentation
with common errors and how to handle them. It's not at all obvious to me
what all the implications of the above guidelines are.

--Sean

Powered by blists - more mailing lists