[PATCH] pwm: lpss: fix base_unit calculation for PWM frequency

[PATCH] pwm: lpss: fix base_unit calculation for PWM frequency

[Dan O'Donovan]

The base_unit calculation applies an offset of 0x2 which adds
significant error for lower frequencies and doesn't appear to be
warranted - rounding the division result gives a correct value.

Also, the upper limit check for base_unit is off-by-one; the upper
nibble of base_unit is invalid if >=128 according to the Table 88
in the Z8000 Processor Series Datasheet Volume 1 (Rev. 2).

Verified on UP Board (Cherry Trail) and Minnowboard Max (Bay Trail).

Signed-off-by: Dan O'Donovan <dan@emutex.com>
---
 drivers/pwm/pwm-lpss.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 295b963..01cc708 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -27,7 +27,6 @@
 #define PWM_SW_UPDATE			BIT(30)
 #define PWM_BASE_UNIT_SHIFT		8
 #define PWM_ON_TIME_DIV_MASK		0x000000ff
-#define PWM_DIVISION_CORRECTION		0x2
 
 /* Size of each PWM register space if multiple */
 #define PWM_SIZE			0x400
@@ -101,17 +100,16 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/*
 	 * The equation is:
-	 * base_unit = ((freq / c) * base_unit_range) + correction
+	 * base_unit = (round(freq / c) * 65536)
 	 */
 	base_unit_range = BIT(lpwm->info->base_unit_bits);
-	base_unit = freq * base_unit_range;
+	freq *= base_unit_range;
 
 	c = lpwm->info->clk_rate;
 	if (!c)
 		return -EINVAL;
 
-	do_div(base_unit, c);
-	base_unit += PWM_DIVISION_CORRECTION;
+	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
 
 	if (duty_ns <= 0)
 		duty_ns = 1;
-- 
2.1.4

Re: [PATCH] pwm: lpss: fix base_unit calculation for PWM frequency

[Mika Westerberg]

On Tue, May 31, 2016 at 03:51:04PM +0100, Dan O'Donovan wrote:
> The base_unit calculation applies an offset of 0x2 which adds
> significant error for lower frequencies and doesn't appear to be
> warranted - rounding the division result gives a correct value.

I don't even remember where that 2 originally came from. Maybe it was
fix for some early silicon issue. Happy to see it go away :)

> Also, the upper limit check for base_unit is off-by-one; the upper
> nibble of base_unit is invalid if >=128 according to the Table 88
> in the Z8000 Processor Series Datasheet Volume 1 (Rev. 2).
> 
> Verified on UP Board (Cherry Trail) and Minnowboard Max (Bay Trail).
> 
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> ---
>  drivers/pwm/pwm-lpss.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 295b963..01cc708 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -27,7 +27,6 @@
>  #define PWM_SW_UPDATE			BIT(30)
>  #define PWM_BASE_UNIT_SHIFT		8
>  #define PWM_ON_TIME_DIV_MASK		0x000000ff
> -#define PWM_DIVISION_CORRECTION		0x2
>  
>  /* Size of each PWM register space if multiple */
>  #define PWM_SIZE			0x400
> @@ -101,17 +100,16 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	/*
>  	 * The equation is:
> -	 * base_unit = ((freq / c) * base_unit_range) + correction
> +	 * base_unit = (round(freq / c) * 65536)

Please leave this as 

	base_unit = (round(freq / c) * base_unit_range)

because Broxton uses 22 bits for base unit.

Otherwise, looks good to me:

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

>  	 */
>  	base_unit_range = BIT(lpwm->info->base_unit_bits);
> -	base_unit = freq * base_unit_range;
> +	freq *= base_unit_range;
>  
>  	c = lpwm->info->clk_rate;
>  	if (!c)
>  		return -EINVAL;
>  
> -	do_div(base_unit, c);
> -	base_unit += PWM_DIVISION_CORRECTION;
> +	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
>  
>  	if (duty_ns <= 0)
>  		duty_ns = 1;
> -- 
> 2.1.4

Re: [PATCH] pwm: lpss: fix base_unit calculation for PWM frequency

[Dan O'Donovan]

Hi Mika

On 06/01/2016 11:20 AM, Mika Westerberg wrote:
> On Tue, May 31, 2016 at 03:51:04PM +0100, Dan O'Donovan wrote:
>> The base_unit calculation applies an offset of 0x2 which adds
>> significant error for lower frequencies and doesn't appear to be
>> warranted - rounding the division result gives a correct value.
> I don't even remember where that 2 originally came from. Maybe it was
> fix for some early silicon issue. Happy to see it go away :)
>
>> Also, the upper limit check for base_unit is off-by-one; the upper
>> nibble of base_unit is invalid if >=128 according to the Table 88
>> in the Z8000 Processor Series Datasheet Volume 1 (Rev. 2).
>>
>> Verified on UP Board (Cherry Trail) and Minnowboard Max (Bay Trail).
>>
>> Signed-off-by: Dan O'Donovan <dan@emutex.com>
>> ---
>>  drivers/pwm/pwm-lpss.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
>> index 295b963..01cc708 100644
>> --- a/drivers/pwm/pwm-lpss.c
>> +++ b/drivers/pwm/pwm-lpss.c
>> @@ -27,7 +27,6 @@
>>  #define PWM_SW_UPDATE			BIT(30)
>>  #define PWM_BASE_UNIT_SHIFT		8
>>  #define PWM_ON_TIME_DIV_MASK		0x000000ff
>> -#define PWM_DIVISION_CORRECTION		0x2
>>  
>>  /* Size of each PWM register space if multiple */
>>  #define PWM_SIZE			0x400
>> @@ -101,17 +100,16 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>  
>>  	/*
>>  	 * The equation is:
>> -	 * base_unit = ((freq / c) * base_unit_range) + correction
>> +	 * base_unit = (round(freq / c) * 65536)
> Please leave this as 
>
> 	base_unit = (round(freq / c) * base_unit_range)
>
> because Broxton uses 22 bits for base unit.
Oops, thanks for pointing that out!  I had developed the patch on an older kernel without Broxton support, and missed this when rebasing.  I'll submit a new version with that change.
>
> Otherwise, looks good to me:
>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
>>  	 */
>>  	base_unit_range = BIT(lpwm->info->base_unit_bits);
>> -	base_unit = freq * base_unit_range;
>> +	freq *= base_unit_range;
>>  
>>  	c = lpwm->info->clk_rate;
>>  	if (!c)
>>  		return -EINVAL;
>>  
>> -	do_div(base_unit, c);
>> -	base_unit += PWM_DIVISION_CORRECTION;
>> +	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
>>  
>>  	if (duty_ns <= 0)
>>  		duty_ns = 1;
>> -- 
>> 2.1.4

[PATCH v2] pwm: lpss: fix base_unit calculation for PWM frequency

[Dan O'Donovan]

The base_unit calculation applies an offset of 0x2 which adds
significant error for lower frequencies and doesn't appear to be
warranted - rounding the division result gives a correct value.

Also, the upper limit check for base_unit is off-by-one; the upper
nibble of base_unit is invalid if >=128 according to the Table 88
in the Z8000 Processor Series Datasheet Volume 1 (Rev. 2).

Verified on UP Board (Cherry Trail) and Minnowboard Max (Bay Trail).

Signed-off-by: Dan O'Donovan <dan@emutex.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
v2:
 * fixed error in a code comment regarding base_unit, spotted by Mika

 drivers/pwm/pwm-lpss.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 295b963..98dc8b8 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -27,7 +27,6 @@
 #define PWM_SW_UPDATE			BIT(30)
 #define PWM_BASE_UNIT_SHIFT		8
 #define PWM_ON_TIME_DIV_MASK		0x000000ff
-#define PWM_DIVISION_CORRECTION		0x2
 
 /* Size of each PWM register space if multiple */
 #define PWM_SIZE			0x400
@@ -101,17 +100,16 @@ static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/*
 	 * The equation is:
-	 * base_unit = ((freq / c) * base_unit_range) + correction
+	 * base_unit = round(base_unit_range * freq / c)
 	 */
 	base_unit_range = BIT(lpwm->info->base_unit_bits);
-	base_unit = freq * base_unit_range;
+	freq *= base_unit_range;
 
 	c = lpwm->info->clk_rate;
 	if (!c)
 		return -EINVAL;
 
-	do_div(base_unit, c);
-	base_unit += PWM_DIVISION_CORRECTION;
+	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);
 
 	if (duty_ns <= 0)
 		duty_ns = 1;
-- 
2.1.4

Re: [PATCH v2] pwm: lpss: fix base_unit calculation for PWM frequency

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Wed, Jun 01, 2016 at 03:31:12PM +0100, Dan O'Donovan wrote:
> The base_unit calculation applies an offset of 0x2 which adds
> significant error for lower frequencies and doesn't appear to be
> warranted - rounding the division result gives a correct value.
> 
> Also, the upper limit check for base_unit is off-by-one; the upper
> nibble of base_unit is invalid if >=128 according to the Table 88
> in the Z8000 Processor Series Datasheet Volume 1 (Rev. 2).
> 
> Verified on UP Board (Cherry Trail) and Minnowboard Max (Bay Trail).
> 
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> v2:
>  * fixed error in a code comment regarding base_unit, spotted by Mika
> 
>  drivers/pwm/pwm-lpss.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]