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