Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
Hello Hans, On Tue, Jul 07, 2020 at 07:31:29PM +0200, Hans de Goede wrote: > On 7/7/20 9:34 AM, Uwe Kleine-König wrote: > > On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote: > > > But if we do then I think closest to the truth would be: > > > > > > state->period = UINT_MAX; > > > state->duty_cycle = 0; > > > > I'd say state->period = 1 & state->duty_cycle = 0 is a better > > representation. > > But that would suggest the output is configured for an > infinitely high output frequency, but the frequency is > actually 0, the reason why get_state needs to treat a > base_unit val of 0 special at all is to avoid a division > by 0, and in math dividing by 0 gives infinite, isn't > UINT_MAX a better way to represent infinity ? Given that duty_cycle is 0, how can to tell anything about the period when only seeing the signal (= a constant low)? Given that (ideally) a period is completed when pwm_apply_state() is called, a short period is much more sensible. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
Hi, On 7/7/20 9:09 PM, Uwe Kleine-König wrote: Hello Hans, On Tue, Jul 07, 2020 at 07:31:29PM +0200, Hans de Goede wrote: On 7/7/20 9:34 AM, Uwe Kleine-König wrote: On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote: But if we do then I think closest to the truth would be: state->period = UINT_MAX; state->duty_cycle = 0; I'd say state->period = 1 & state->duty_cycle = 0 is a better representation. But that would suggest the output is configured for an infinitely high output frequency, but the frequency is actually 0, the reason why get_state needs to treat a base_unit val of 0 special at all is to avoid a division by 0, and in math dividing by 0 gives infinite, isn't UINT_MAX a better way to represent infinity ? Given that duty_cycle is 0, how can to tell anything about the period when only seeing the signal (= a constant low)? Given that (ideally) a period is completed when pwm_apply_state() is called, a short period is much more sensible. Ok, I will add a patch to v4 of the patch-set to adjust the pwm-lpss driver's get_state method accordingly. Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
Hi, On 7/7/20 9:34 AM, Uwe Kleine-König wrote: On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote: Hi, Thank you for your review and sorry for the slow reply. No problem for me, I didn't hold my breath :-) diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 43b1fc634af1..80d0f9c64f9d 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, freq *= base_unit_range; base_unit = DIV_ROUND_CLOSEST_ULL(freq, c); DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend the time to actually confirm that. Yes I saw your comment elsewhere that the PWM API defines rounding in a certain direction, but fixing that falls outside of this patch. Yeah, sure. [...] I hope this helps to explain what is going on a bit. I will try to make sense of that and reply to the patch directly when I succeeded. ### As for the behavior on base_unit==0 in the get_state method, as mentioned above I wrote that when I did not fully understood how the controller works. We really should never encounter this. But if we do then I think closest to the truth would be: state->period = UINT_MAX; state->duty_cycle = 0; I'd say state->period = 1 & state->duty_cycle = 0 is a better representation. But that would suggest the output is configured for an infinitely high output frequency, but the frequency is actually 0, the reason why get_state needs to treat a base_unit val of 0 special at all is to avoid a division by 0, and in math dividing by 0 gives infinite, isn't UINT_MAX a better way to represent infinity ? Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote: > Hi, > > Thank you for your review and sorry for the slow reply. No problem for me, I didn't hold my breath :-) > > > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > > > index 43b1fc634af1..80d0f9c64f9d 100644 > > > --- a/drivers/pwm/pwm-lpss.c > > > +++ b/drivers/pwm/pwm-lpss.c > > > @@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip > > > *lpwm, struct pwm_device *pwm, > > > freq *= base_unit_range; > > > base_unit = DIV_ROUND_CLOSEST_ULL(freq, c); > > > > DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend > > the time to actually confirm that. > > Yes I saw your comment elsewhere that the PWM API defines rounding > in a certain direction, but fixing that falls outside of this patch. Yeah, sure. > [...] > I hope this helps to explain what is going on a bit. I will try to make sense of that and reply to the patch directly when I succeeded. > ### > > As for the behavior on base_unit==0 in the get_state method, > as mentioned above I wrote that when I did not fully understood > how the controller works. > > We really should never encounter this. > > But if we do then I think closest to the truth would be: > > state->period = UINT_MAX; > state->duty_cycle = 0; I'd say state->period = 1 & state->duty_cycle = 0 is a better representation. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
Hi, On 7/7/20 9:34 AM, Uwe Kleine-König wrote: On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote: Hi, Thank you for your review and sorry for the slow reply. No problem for me, I didn't hold my breath :-) diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c index 43b1fc634af1..80d0f9c64f9d 100644 --- a/drivers/pwm/pwm-lpss.c +++ b/drivers/pwm/pwm-lpss.c @@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm, freq *= base_unit_range; base_unit = DIV_ROUND_CLOSEST_ULL(freq, c); DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend the time to actually confirm that. Yes I saw your comment elsewhere that the PWM API defines rounding in a certain direction, but fixing that falls outside of this patch. Yeah, sure. [...] I hope this helps to explain what is going on a bit. I will try to make sense of that and reply to the patch directly when I succeeded. In case it helps here is the datasheet for the LPSS PWM controller (somewhat hard to find if you don't know what you are looking for): https://cdrdv2.intel.com/v1/dl/getcontent/332065 https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/atom-z8000-datasheet-vol-2.pdf The first link contains a description about how the PWM controller works in section 17.5 "SIO - Pulse Width Modulation (PWM)", the second link contains all register definitions for the SoC and is not all that interesting other then for verifying the existing register bits defines. Regards, Hans ### As for the behavior on base_unit==0 in the get_state method, as mentioned above I wrote that when I did not fully understood how the controller works. We really should never encounter this. But if we do then I think closest to the truth would be: state->period = UINT_MAX; state->duty_cycle = 0; I'd say state->period = 1 & state->duty_cycle = 0 is a better representation. Best regards Uwe ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
Hi,
Thank you for your review and sorry for the slow reply.
I would like to get this series upstream this cycle, so
I will do my best to be a lot faster with responding from
now on.
On 6/22/20 9:35 AM, Uwe Kleine-König wrote:
On Sat, Jun 20, 2020 at 02:17:47PM +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.
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.
This commit fixes both issues by clamping the base_unit value to be
between 1 and (base_unit_range - 1).
Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
Signed-off-by: Hans de Goede
---
Changes in v3:
- Change upper limit of clamp to (base_unit_range - 1)
- Add Fixes tag
---
drivers/pwm/pwm-lpss.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 43b1fc634af1..80d0f9c64f9d 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm,
struct pwm_device *pwm,
freq *= base_unit_range;
base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend
the time to actually confirm that.
Yes I saw your comment elsewhere that the PWM API defines rounding
in a certain direction, but fixing that falls outside of this patch.
+ /* base_unit must not be 0 and we also want to avoid overflowing it */
+ base_unit = clamp_t(unsigned long long, base_unit, 1,
+ base_unit_range - 1);
.get_state seems to handle base_unit == 0 just fine?!
It tries to do something with it to avoid a divide by 0, back when
I wrote the get_state code I wasn't fully aware of how the PWM
controller works. I did have access to the same datasheets as today
(the datasheets for this one are public) but the datasheet needs to
be read and then left to sync in for a couple of months and then read
again, or iow the datasheet does not explain things all that well.
As I tried to explain in the commit msg the way this PWM controller
works is it takes its input clock and then each input clock-cycle the
"base_unit" gets added to a n-bit register lets say a 16-bit register
at that is the case for the HW on which I've done all my testing.
The 8 most significant bits of the 16 bit register are compared with
a 8 bit value programmed by the PWM driver / coming from a ctrl
register and the output of that comparator is the PWM output.
The problem with a base_unit value of '0' is that adding 0 to the
16 bit register is a no-op, so the register never increments
(iow is always 0) and thus can never become bigger then the comparator
input and thus the PWM output is always 0.
The datasheet does helpfully contain a note explicitly warning of
this behavior.
So when we are programming the base_unit value, it seems best to
clamp the lower end to 1, which gives an PWM output frequency of
e.g. 1920 / 65536 = 293 Hz
If the user has request an even lower output frequency, which
would result in our base_unit calculation outputting 0, then we
can either output always low, which is an infinite low output
frequency, or give the user 293 Hz and a working PWM.
This is the low end of the clamp. The high end clamp simply is
there because base_unit itself is e.g. a 16 bit value too.
The looks a bid weird because instead of 65536 (for the divides)
/ 65535 (for the clamp / masking) it uses base_unit_range and
(base_unit_range - 1). This is because different versions of
the SoCs using this driver have a different register size for the
base_unit value.
I hope this helps to explain what is going on a bit.
###
As for the behavior on base_unit==0 in the get_state method,
as mentioned above I wrote that when I did not fully understood
how the controller works.
We really should never encounter this.
But if we do then I think closest to the truth would be:
state->period = UINT_MAX;
state->duty_cycle = 0;
I can submit a separate patc
Re: [Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
On Sat, Jun 20, 2020 at 02:17:47PM +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.
>
> 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.
>
> This commit fixes both issues by clamping the base_unit value to be
> between 1 and (base_unit_range - 1).
>
> Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
> Signed-off-by: Hans de Goede
> ---
> Changes in v3:
> - Change upper limit of clamp to (base_unit_range - 1)
> - Add Fixes tag
> ---
> drivers/pwm/pwm-lpss.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 43b1fc634af1..80d0f9c64f9d 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm,
> struct pwm_device *pwm,
> freq *= base_unit_range;
>
> base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend
the time to actually confirm that.
> + /* base_unit must not be 0 and we also want to avoid overflowing it */
> + base_unit = clamp_t(unsigned long long, base_unit, 1,
> + base_unit_range - 1);
.get_state seems to handle base_unit == 0 just fine?! Though this
doesn't look right either ...
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value
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.
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.
This commit fixes both issues by clamping the base_unit value to be
between 1 and (base_unit_range - 1).
Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
Signed-off-by: Hans de Goede
---
Changes in v3:
- Change upper limit of clamp to (base_unit_range - 1)
- Add Fixes tag
---
drivers/pwm/pwm-lpss.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 43b1fc634af1..80d0f9c64f9d 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm,
struct pwm_device *pwm,
freq *= base_unit_range;
base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
+ /* base_unit must not be 0 and we also want to avoid overflowing it */
+ base_unit = clamp_t(unsigned long long, base_unit, 1,
+ base_unit_range - 1);
on_time_div = 255ULL * duty_ns;
do_div(on_time_div, period_ns);
@@ -105,7 +108,6 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm,
struct pwm_device *pwm,
orig_ctrl = ctrl = pwm_lpss_read(pwm);
ctrl &= ~PWM_ON_TIME_DIV_MASK;
ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT);
- base_unit &= (base_unit_range - 1);
ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
ctrl |= on_time_div;
--
2.26.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
