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 <dan@emutex.com>
---
 drivers/pwm/pwm-lpss.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html