[U-Boot] [PATCH] bugfix i.mx6 pwm: prevent overflow of period_c * duty_ns by casting duty_ns to ull first. This bug came up when trying to create a 200 Hz PWM.

[U-Boot] [PATCH] bugfix i.mx6 pwm: prevent overflow of period_c * duty_ns by casting duty_ns to ull first. This bug came up when trying to create a 200 Hz PWM.

[Brecht Neyrinck]

---
 drivers/pwm/pwm-imx-util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 mode change 100644 => 100755 drivers/pwm/pwm-imx-util.c

diff --git a/drivers/pwm/pwm-imx-util.c b/drivers/pwm/pwm-imx-util.c
index f1d0b35..777a8bf 100644
--- a/drivers/pwm/pwm-imx-util.c
+++ b/drivers/pwm/pwm-imx-util.c
@@ -56,7 +56,7 @@ int pwm_imx_get_parms(int period_ns, int duty_ns, unsigned long *period_c,
 	*prescale = *period_c / 0x10000 + 1;
 
 	*period_c /= *prescale;
-	c = (unsigned long long)(*period_c * duty_ns);
+	c = *period_c * (unsigned long long) duty_ns;
 	do_div(c, period_ns);
 	*duty_c = c;
 
-- 
1.8.2.3




**** DISCLAIMER **** The contents of this e-mail are intended for the named
addressee only. It contains information which may be confidential and which
may also be privileged. Unless you are the named addressee (or authorised
to receive for the addressee) you may not copy or use it, or disclose it to
anyone else. If you received it in error please notify us immediately and then
destroy it. Further, we make every effort to keep our network free from
viruses. However, you do need to verify that this email and any attachments
are free of viruses as we can take no responsibility for any computer virus
which might be transferred by way of this e-mail.

[U-Boot] [PATCH] bugfix i.mx6 pwm: prevent overflow of period_c * duty_ns by casting duty_ns to ull first. This bug came up when trying to create a 200 Hz PWM.

[Heiko Schocher]

Hello Brecht Neyrinck,

Am 27.04.2015 14:11, schrieb Brecht Neyrinck:
> ---
>   drivers/pwm/pwm-imx-util.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>   mode change 100644 => 100755 drivers/pwm/pwm-imx-util.c
>
> diff --git a/drivers/pwm/pwm-imx-util.c b/drivers/pwm/pwm-imx-util.c
> index f1d0b35..777a8bf 100644
> --- a/drivers/pwm/pwm-imx-util.c
> +++ b/drivers/pwm/pwm-imx-util.c
> @@ -56,7 +56,7 @@ int pwm_imx_get_parms(int period_ns, int duty_ns, unsigned long *period_c,
>   	*prescale = *period_c / 0x10000 + 1;
>
>   	*period_c /= *prescale;
> -	c = (unsigned long long)(*period_c * duty_ns);
> +	c = *period_c * (unsigned long long) duty_ns;

Thanks for this ... Hmm... this code is directly from linux
drivers/pwm/pwm-imx.c ... Do you have running a linux on your hw?
Could you verify this in linux too?

Thanks!

Acked-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
>   	do_div(c, period_ns);
>   	*duty_c = c;
>
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] bugfix i.mx6 pwm: prevent overflow of period_c * duty_ns by casting duty_ns to ull first. This bug came up when trying to create a 200 Hz PWM.

[NEYRINCK Brecht]

Hi Heiko,

We didn't need the 200 Hz PWM in Linux, so I didn't test it right away.
I did now, and the problem isn't present there, which I found weird given the fact that it is indeed similar code. So I dug into the kernel:
The devil is in the details. :)

Kernel: 
c = (unsigned long long)period_cycles * duty_ns;

U-boot:
c = (unsigned long long)(*period_c * duty_ns);

The braces cause the casting to be performed after the (truncated) multiplication, while without braces, the casting is done upfront.

Thank you for your reply and giving attention to this small bug.

Kind regards,
Brecht

-----Original Message-----
From: Heiko Schocher [mailto:hs at denx.de] 
Sent: dinsdag 28 april 2015 7:32
To: NEYRINCK Brecht
Cc: u-boot at lists.denx.de
Subject: Re: [U-Boot] [PATCH] bugfix i.mx6 pwm: prevent overflow of period_c * duty_ns by casting duty_ns to ull first. This bug came up when trying to create a 200 Hz PWM.

Hello Brecht Neyrinck,

Am 27.04.2015 14:11, schrieb Brecht Neyrinck:
> ---
>   drivers/pwm/pwm-imx-util.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>   mode change 100644 => 100755 drivers/pwm/pwm-imx-util.c
>
> diff --git a/drivers/pwm/pwm-imx-util.c b/drivers/pwm/pwm-imx-util.c 
> index f1d0b35..777a8bf 100644
> --- a/drivers/pwm/pwm-imx-util.c
> +++ b/drivers/pwm/pwm-imx-util.c
> @@ -56,7 +56,7 @@ int pwm_imx_get_parms(int period_ns, int duty_ns, unsigned long *period_c,
>   	*prescale = *period_c / 0x10000 + 1;
>
>   	*period_c /= *prescale;
> -	c = (unsigned long long)(*period_c * duty_ns);
> +	c = *period_c * (unsigned long long) duty_ns;

Thanks for this ... Hmm... this code is directly from linux drivers/pwm/pwm-imx.c ... Do you have running a linux on your hw?
Could you verify this in linux too?

Thanks!

Acked-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
>   	do_div(c, period_ns);
>   	*duty_c = c;
>
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany



**** DISCLAIMER **** The contents of this e-mail are intended for the named
addressee only. It contains information which may be confidential and which
may also be privileged. Unless you are the named addressee (or authorised
to receive for the addressee) you may not copy or use it, or disclose it to
anyone else. If you received it in error please notify us immediately and then
destroy it. Further, we make every effort to keep our network free from
viruses. However, you do need to verify that this email and any attachments
are free of viruses as we can take no responsibility for any computer virus
which might be transferred by way of this e-mail.

[U-Boot] [PATCH] bugfix i.mx6 pwm: prevent overflow of period_c * duty_ns by casting duty_ns to ull first. This bug came up when trying to create a 200 Hz PWM.

[Fabio Estevam]

Hi Brecht,

On Mon, Apr 27, 2015 at 9:11 AM, Brecht Neyrinck <bnrn@psicontrol.com> wrote:
> ---
>  drivers/pwm/pwm-imx-util.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>  mode change 100644 => 100755 drivers/pwm/pwm-imx-util.c
>
> diff --git a/drivers/pwm/pwm-imx-util.c b/drivers/pwm/pwm-imx-util.c
> index f1d0b35..777a8bf 100644
> --- a/drivers/pwm/pwm-imx-util.c
> +++ b/drivers/pwm/pwm-imx-util.c
> @@ -56,7 +56,7 @@ int pwm_imx_get_parms(int period_ns, int duty_ns, unsigned long *period_c,
>         *prescale = *period_c / 0x10000 + 1;
>
>         *period_c /= *prescale;
> -       c = (unsigned long long)(*period_c * duty_ns);
> +       c = *period_c * (unsigned long long) duty_ns;
>         do_div(c, period_ns);
>         *duty_c = c;

Your fix looks good, but your patch misses a Signed-off-by.

Also the commit log seems to be mixed with the subject.

Please check:
http://www.denx.de/wiki/U-Boot/Patches

You can also send it to the list using git send-email.

Regards,

Fabio Estevam