Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Jul 09, 2020 at 03:23:13PM +0200, Hans de Goede wrote:
> On 7/9/20 2:53 PM, Andy Shevchenko wrote:
> > On Wed, Jul 08, 2020 at 11:14:20PM +0200, Hans de Goede wrote:
> > > When the user requests a high enough period ns value, then the
> > > calculations in pwm_lpss_prepare() might result in a base_unit value of 0.
> > > 
> > > But according to the data-sheet the way the PWM controller works is that
> > > each input clock-cycle the base_unit gets added to a N bit counter and
> > > that counter overflowing determines the PWM output frequency. Adding 0
> > > to the counter is a no-op. The data-sheet even explicitly states that
> > > writing 0 to the base_unit bits will result in the PWM outputting a
> > > continuous 0 signal.
> > 
> > And I don't see how you can get duty 100% / 0% (I don't remember which one is
> > equivalent to 0 in base unit) after this change. IIRC the problem here that
> > base unit when non-zero is always being added to the counter and it will
> > trigger the change of output at some point which is not what we want for 100% /
> > 0% cases.
> 
> The base_unit controls the output frequency, not the duty-cycle. So clamping
> the base_unit, as calculated from the period here, which also only configures
> output-frequency does not impact the duty-cycle at all.
> 
> note that AFAICT currently no (in kernel) users actually try to set a period value
> which would hit the clamp, so for existing users the clamp is a no-op. I just
> added it to this patch-set for correctness sake and because userspace
> (sysfs interface) users could in theory set out of range values.
> 
> As for the duty-cycle thing, first of all let me say that that is a
> question / issue which is completely orthogonal to this patch, this
> patch only impacts the period/output frequency NOT the duty-cycle,

Unfortunately the base unit settings affects duty cycle.

Documentation says about integer part and fractional, where integer is
8 bit and this what's being compared to on time divisor. Thus, if on time
divisor is 255 and base unit is 1 (in integer part) or 0.25, we can't get 0%.
(It looks like if 'on time divisor MOD base unit == 0' we won't get 0%)

> With that said, the documentation is not really helpful here,
> we need to set the on_time_div to 255 to get a duty-cycle close to 0
> (and to 0 to get a duty cycle of 100%) but if setting this to 255 gives
> us a duty-cycle of really really 0%, or just close to 0% is uncleaer.

It depends on base unit value.

> We could do a separate patch add ing a hack where if the user asks for
> 0% duty-cycle we program the base_unit to 0, but that seems like a bad
> idea for 2 reasons:

> 1. If the user really wants the output to be constantly 0 the user should
> just disable the pwm

I can't take this as an argument. Disabling PWM is orthogonal to what duty cycle is.

> 2. New base_unit values are latched and not applied until the counter
> overflows, with a base_unit of 0 the counter never overflows. I have
> not tested this but I would not be surprised if after programming a
> base_unit value of 0, we are unable to ever change the value again
> through any other means then power-cycling the PWM controller.
> Even if I could test this on some revisions, we already know that
> not all revisions work the same wrt the latching. So it is best to
> just never set base_unit to 0, that is just a recipe asking for trouble.

This what doc says about zeros:
• Programming either the PWM_base_unit value or the PWM_on_time_divisor to ‘0’
will generate an always zero output.

So, what I'm talking seems about correlation between base unit and on time
divisor rather than zeros.

I agree with this patch.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> > > When the user requestes a low enough period ns value, then the
> > > calculations in pwm_lpss_prepare() might result in a base_unit value
> > > which is bigger then base_unit_range - 1. Currently the codes for this
> > > deals with this by applying a mask:
> > > 
> > > 	base_unit &= (base_unit_range - 1);
> > > 
> > > But this means that we let the value overflow the range, we throw away the
> > > higher bits and store whatever value is left in the lower bits into the
> > > register leading to a random output frequency, rather then clamping the
> > > output frequency to the highest frequency which the hardware can do.
> > 
> > It would be nice to have an example of calculus here.
> > 
> > > This commit fixes both issues by clamping the base_unit value to be
> > > between 1 and (base_unit_range - 1).
> > 
> > Eventually I sat and wrote all this on paper. I see now that the problem
> > is in out of range of the period. And strongly we should clamp rather period
> > to the supported range, but your solution is an equivalent.
> 
> Right, the advantage of doing the clamping on the register value is that we
> avoid some tricky math with possible rounding errors and which is different
> per controller revision because the number of bits in the base unit being
> different per controller revision.

...

> > > +	base_unit = clamp_t(unsigned long long, base_unit, 1,
> > > +			    base_unit_range - 1);
> > 
> > A nit: one line.
> 
> Doesn't fit in 80 chars, I guess we could make this one line now with the new 100 chars
> limit, but that does make it harder to read for people using standard terminal widths
> and a terminal based editors. So I would prefer to keep this as is.

You can use clamp_val().

-- 
With Best Regards,
Andy Shevchenko