pwm: lpss: fix base_unit calculation for PWM frequency

PATCH Superseded Dan O'Donovan 3 participants 5 messages May 31, 2016 – Jun 10, 2016

Subsystems (2)

PWM Intel LPSS (Low Power SubSystem) drivers

lore

Share & tools

lore

Dan O'Donovan

author

2016-05-31 15:11 UTC #0 lore raw

PATCH 1/1 pwm: lpss: fix base_unit calculation for PWM frequency

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 <[email protected]>

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

drivers/pwm/pwm-lpss.c +3 -5

index 295b963..01cc708 100644
@@ -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;

Signature · 1 line

2.1.4

Mika Westerberg

Acked-by on v1 1/1

2016-06-01 10:20 UTC #1 lore raw

Quoted 3 lines · 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.

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 :)

Quoted 24 lines

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 <[email protected]>
---
 drivers/pwm/pwm-lpss.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/pwm/pwm-lpss.c +0 -1
index 295b963..01cc708 100644
@@ -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

drivers/pwm/pwm-lpss.c static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 103

Quoted 5 lines

drivers/pwm/pwm-lpss.c +1 -1

@@ -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 <[email protected]>

Quoted 17 lines

drivers/pwm/pwm-lpss.c +2 -3

 	 */
 	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;

Signature · 1 line

2.1.4

Dan O'Donovan

author

2016-06-01 10:33 UTC #2 lore raw

Hi Mika

Quoted 38 lines · Mika Westerberg

Quoted 3 lines · 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.
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 :)
Quoted 24 lines
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 <[email protected]>
---
 drivers/pwm/pwm-lpss.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/pwm/pwm-lpss.c +0 -1
index 295b963..01cc708 100644
@@ -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
drivers/pwm/pwm-lpss.c static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 103
Quoted 5 lines
drivers/pwm/pwm-lpss.c +1 -1
@@ -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

Quoted 3 lines

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.

Quoted 22 lines

Otherwise, looks good to me:

Acked-by Mika Westerberg <[email protected]>

Quoted 17 lines

drivers/pwm/pwm-lpss.c +2 -3

 	 */
 	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;

Signature · 1 line

2.1.4

Dan O'Donovan

author

2016-06-01 14:29 UTC #3 lore raw

PATCH v2 1/1 pwm: lpss: fix base_unit calculation for PWM frequency

In reply to 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 <[email protected]>

Acked-by Mika Westerberg <[email protected]>

---
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(-)

drivers/pwm/pwm-lpss.c +3 -5

index 295b963..98dc8b8 100644
@@ -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;

Signature · 1 line

2.1.4

Thierry Reding

Applied

2016-06-10 13:38 UTC #4 lore raw

Quoted 10 lines · 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).

Quoted 8 lines

Signed-off-by Dan O'Donovan <[email protected]>
Acked-by Mika Westerberg <[email protected]>
---
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