diff mbox

[U-Boot,3/9,v2] Exynos: pwm: Fix two bugs in the exynos pwm configuration code

Message ID 1362049164-10297-4-git-send-email-akshay.s@samsung.com
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Commit Message

Akshay Saraswat Feb. 28, 2013, 10:59 a.m. UTC 
First, the "div" value was being used incorrectly to compute the frequency of
the PWM timer. The value passed in is a constant which reflects the value
that would be found in a configuration register, 0 to 4. That should
correspond to a scaling factor of 1, 2, 4, 8, or 16, 1 << div, but div + 1 was
being used instead.

Second, the reset value of the timers were being calculated to give an overall
frequency, thrown out, and set to a maximum value. This was done so that PWM 4
could be used as the system clock by counting down from a high value, but it
was applied indiscriminantly. It should at most be applied only to PWM 4.

This change also takes the opportunity to tidy up the pwm_init function.

Test with command "sf probe 1:0; time sf read 40008000 0 1000".
Try with different numbers of bytes and see that sane values are obtained
Build and boot U-boot with this patch, backlight works properly.

Signed-off-by: Gabe Black <gabeblack@google.com>
Signed-off-by: Akshay Saraswat <akshay.s@samsung.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
Changes since v1:
        - Added "Acked-by: Simon Glass".

 arch/arm/cpu/armv7/s5p-common/pwm.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

Comments

Minkyu Kang March 28, 2013, 5:53 a.m. UTC | #1 
Dear Akshay,

On 28/02/13 19:59, Akshay Saraswat wrote:
> First, the "div" value was being used incorrectly to compute the frequency of
> the PWM timer. The value passed in is a constant which reflects the value
> that would be found in a configuration register, 0 to 4. That should
> correspond to a scaling factor of 1, 2, 4, 8, or 16, 1 << div, but div + 1 was
> being used instead.
> 
> Second, the reset value of the timers were being calculated to give an overall
> frequency, thrown out, and set to a maximum value. This was done so that PWM 4
> could be used as the system clock by counting down from a high value, but it
> was applied indiscriminantly. It should at most be applied only to PWM 4.
> 
> This change also takes the opportunity to tidy up the pwm_init function.
> 
> Test with command "sf probe 1:0; time sf read 40008000 0 1000".
> Try with different numbers of bytes and see that sane values are obtained
> Build and boot U-boot with this patch, backlight works properly.
> 
> Signed-off-by: Gabe Black <gabeblack@google.com>
> Signed-off-by: Akshay Saraswat <akshay.s@samsung.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> ---
> Changes since v1:
>         - Added "Acked-by: Simon Glass".
> 
>  arch/arm/cpu/armv7/s5p-common/pwm.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/s5p-common/pwm.c b/arch/arm/cpu/armv7/s5p-common/pwm.c
> index 3147f59..02156d1 100644
> --- a/arch/arm/cpu/armv7/s5p-common/pwm.c
> +++ b/arch/arm/cpu/armv7/s5p-common/pwm.c
> @@ -143,7 +143,7 @@ int pwm_init(int pwm_id, int div, int invert)
>  	u32 val;
>  	const struct s5p_timer *pwm =
>  			(struct s5p_timer *)samsung_get_base_timer();
> -	unsigned long timer_rate_hz;
> +	unsigned long ticks_per_period;
>  	unsigned int offset, prescaler;
>  
>  	/*
> @@ -167,20 +167,24 @@ int pwm_init(int pwm_id, int div, int invert)
>  	val |= (div & 0xf) << MUX_DIV_SHIFT(pwm_id);
>  	writel(val, &pwm->tcfg1);
>  
> -	timer_rate_hz = get_pwm_clk() / ((prescaler + 1) *
> -			(div + 1));
> +	if (pwm_id == 4) {
> +		/*
> +		 * TODO(sjg): Use this as a countdown timer for now. We count
> +		 * down from the maximum value to 0, then reset.
> +		 */
> +		ticks_per_period = -1UL;
> +	} else {
> +		const unsigned long pwm_hz = 1000;
> +		unsigned long timer_rate_hz = get_pwm_clk() /
> +			((prescaler + 1) * (1 << div));

good catch. thanks.

>  
> -	timer_rate_hz = timer_rate_hz / CONFIG_SYS_HZ;
> +		ticks_per_period = timer_rate_hz / pwm_hz;

why don't you use CONFIG_SYS_HZ?
pwm_hz seems to constant.

> +	}
>  
>  	/* set count value */
>  	offset = pwm_id * 3;
>  
> -	/*
> -	 * TODO(sjg): Use this as a countdown timer for now. We count down
> -	 * from the maximum value to 0, then reset.
> -	 */
> -	timer_rate_hz = -1;
> -	writel(timer_rate_hz, &pwm->tcntb0 + offset);
> +	writel(ticks_per_period, &pwm->tcntb0 + offset);
>  
>  	val = readl(&pwm->tcon) & ~(0xf << TCON_OFFSET(pwm_id));
>  	if (invert && (pwm_id < 4))
> 

Thanks,
Minkyu Kang.
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/s5p-common/pwm.c b/arch/arm/cpu/armv7/s5p-common/pwm.c
index 3147f59..02156d1 100644
--- a/arch/arm/cpu/armv7/s5p-common/pwm.c
+++ b/arch/arm/cpu/armv7/s5p-common/pwm.c
@@ -143,7 +143,7 @@  int pwm_init(int pwm_id, int div, int invert)
 	u32 val;
 	const struct s5p_timer *pwm =
 			(struct s5p_timer *)samsung_get_base_timer();
-	unsigned long timer_rate_hz;
+	unsigned long ticks_per_period;
 	unsigned int offset, prescaler;
 
 	/*
@@ -167,20 +167,24 @@  int pwm_init(int pwm_id, int div, int invert)
 	val |= (div & 0xf) << MUX_DIV_SHIFT(pwm_id);
 	writel(val, &pwm->tcfg1);
 
-	timer_rate_hz = get_pwm_clk() / ((prescaler + 1) *
-			(div + 1));
+	if (pwm_id == 4) {
+		/*
+		 * TODO(sjg): Use this as a countdown timer for now. We count
+		 * down from the maximum value to 0, then reset.
+		 */
+		ticks_per_period = -1UL;
+	} else {
+		const unsigned long pwm_hz = 1000;
+		unsigned long timer_rate_hz = get_pwm_clk() /
+			((prescaler + 1) * (1 << div));
 
-	timer_rate_hz = timer_rate_hz / CONFIG_SYS_HZ;
+		ticks_per_period = timer_rate_hz / pwm_hz;
+	}
 
 	/* set count value */
 	offset = pwm_id * 3;
 
-	/*
-	 * TODO(sjg): Use this as a countdown timer for now. We count down
-	 * from the maximum value to 0, then reset.
-	 */
-	timer_rate_hz = -1;
-	writel(timer_rate_hz, &pwm->tcntb0 + offset);
+	writel(ticks_per_period, &pwm->tcntb0 + offset);
 
 	val = readl(&pwm->tcon) & ~(0xf << TCON_OFFSET(pwm_id));
 	if (invert && (pwm_id < 4))