From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v4 04/16] pwm: lpss: Add range limit check for the base_unit register value Date: Thu, 9 Jul 2020 16:33:50 +0200 Message-ID: References: <20200708211432.28612-1-hdegoede@redhat.com> <20200708211432.28612-5-hdegoede@redhat.com> <20200709125342.GX3703480@smile.fi.intel.com> <4ff9dc18-fa59-d9a3-c7bf-9f95c62fc356@redhat.com> <20200709142136.GZ3703480@smile.fi.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20200709142136.GZ3703480@smile.fi.intel.com> Content-Language: en-US Sender: linux-acpi-owner@vger.kernel.org To: Andy Shevchenko Cc: Thierry Reding , =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , "Rafael J . Wysocki" , Len Brown , linux-pwm@vger.kernel.org, intel-gfx , dri-devel@lists.freedesktop.org, Mika Westerberg , linux-acpi@vger.kernel.org List-Id: linux-pwm@vger.kernel.org Hi, On 7/9/20 4:21 PM, Andy Shevchenko wrote: > 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 Thank you. >>>> 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(). I did not know about that, that will work nicely I will switch to clamp_val for the next version. I assume it is ok to keep your Reviewed-by with this very minor change? Regards, Hans >