[PATCH] pwm: bcm2835: increase precision of pwm

[PATCH] pwm: bcm2835: increase precision of pwm

[Sean Young]

If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
carrier ends up being 476kHz.

A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
rounding errors, and we have a much more accurate carrier of 454.5kHz.

Reported-by: Andreas Christ <andreas@christ-faesch.ch>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/pwm/pwm-bcm2835.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 5652f461d994..edb2387c49a2 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
 	unsigned long rate = clk_get_rate(pc->clk);
-	unsigned long scaler;
+	unsigned int scaler;
 
 	if (!rate) {
 		dev_err(pc->dev, "failed to get clock rate\n");
 		return -EINVAL;
 	}
 
-	scaler = NSEC_PER_SEC / rate;
+	scaler = DIV_ROUND_CLOSEST(NSEC_PER_SEC, rate);
 
 	if (period_ns <= MIN_PERIOD) {
 		dev_err(pc->dev, "period %d not supported, minimum %d\n",
@@ -78,8 +78,10 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 	}
 
-	writel(duty_ns / scaler, pc->base + DUTY(pwm->hwpwm));
-	writel(period_ns / scaler, pc->base + PERIOD(pwm->hwpwm));
+	writel(DIV_ROUND_CLOSEST(duty_ns, scaler),
+	       pc->base + DUTY(pwm->hwpwm));
+	writel(DIV_ROUND_CLOSEST(period_ns, scaler),
+	       pc->base + PERIOD(pwm->hwpwm));
 
 	return 0;
 }
-- 
2.20.1

[PATCH] pwm: bcm2835: increase precision of pwm

[Sean Young]

If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
carrier ends up being 476kHz.

A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
rounding errors, and we have a much more accurate carrier of 454.5kHz.

Reported-by: Andreas Christ <andreas@christ-faesch.ch>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/pwm/pwm-bcm2835.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 5652f461d994..edb2387c49a2 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
 	unsigned long rate = clk_get_rate(pc->clk);
-	unsigned long scaler;
+	unsigned int scaler;
 
 	if (!rate) {
 		dev_err(pc->dev, "failed to get clock rate\n");
 		return -EINVAL;
 	}
 
-	scaler = NSEC_PER_SEC / rate;
+	scaler = DIV_ROUND_CLOSEST(NSEC_PER_SEC, rate);
 
 	if (period_ns <= MIN_PERIOD) {
 		dev_err(pc->dev, "period %d not supported, minimum %d\n",
@@ -78,8 +78,10 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 	}
 
-	writel(duty_ns / scaler, pc->base + DUTY(pwm->hwpwm));
-	writel(period_ns / scaler, pc->base + PERIOD(pwm->hwpwm));
+	writel(DIV_ROUND_CLOSEST(duty_ns, scaler),
+	       pc->base + DUTY(pwm->hwpwm));
+	writel(DIV_ROUND_CLOSEST(period_ns, scaler),
+	       pc->base + PERIOD(pwm->hwpwm));
 
 	return 0;
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Uwe Kleine-König]

Hello Thierry,

On Sun, Jun 02, 2019 at 11:23:50AM +0100, Sean Young wrote:
> If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
> carrier ends up being 476kHz.
> 
> A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
> this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
> rounding errors, and we have a much more accurate carrier of 454.5kHz.

can we please fix the rules how to round such that not every driver
invents its own measurement of what is best?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Uwe Kleine-König]

Hello Thierry,

On Sun, Jun 02, 2019 at 11:23:50AM +0100, Sean Young wrote:
> If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
> carrier ends up being 476kHz.
> 
> A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
> this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
> rounding errors, and we have a much more accurate carrier of 454.5kHz.

can we please fix the rules how to round such that not every driver
invents its own measurement of what is best?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Stefan Wahren]

Hi Sean,

Am 02.06.19 um 12:23 schrieb Sean Young:
> If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
> carrier ends up being 476kHz.
>
> A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
> this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
> rounding errors, and we have a much more accurate carrier of 454.5kHz.
>
> Reported-by: Andreas Christ <andreas@christ-faesch.ch>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/pwm/pwm-bcm2835.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> index 5652f461d994..edb2387c49a2 100644
> --- a/drivers/pwm/pwm-bcm2835.c
> +++ b/drivers/pwm/pwm-bcm2835.c
> @@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
>  	unsigned long rate = clk_get_rate(pc->clk);
> -	unsigned long scaler;
> +	unsigned int scaler;
according to the commit log i wouldn't expect this change. Maybe it's
worth to mention.

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Stefan Wahren]

Hi Sean,

Am 02.06.19 um 12:23 schrieb Sean Young:
> If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
> carrier ends up being 476kHz.
>
> A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
> this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
> rounding errors, and we have a much more accurate carrier of 454.5kHz.
>
> Reported-by: Andreas Christ <andreas@christ-faesch.ch>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/pwm/pwm-bcm2835.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> index 5652f461d994..edb2387c49a2 100644
> --- a/drivers/pwm/pwm-bcm2835.c
> +++ b/drivers/pwm/pwm-bcm2835.c
> @@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
>  	unsigned long rate = clk_get_rate(pc->clk);
> -	unsigned long scaler;
> +	unsigned int scaler;
according to the commit log i wouldn't expect this change. Maybe it's
worth to mention.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Sean Young]

Hi Stefan,

On Sun, Jun 02, 2019 at 06:45:44PM +0200, Stefan Wahren wrote:
> Am 02.06.19 um 12:23 schrieb Sean Young:
> > If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
> > carrier ends up being 476kHz.
> >
> > A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
> > this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
> > rounding errors, and we have a much more accurate carrier of 454.5kHz.
> >
> > Reported-by: Andreas Christ <andreas@christ-faesch.ch>
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/pwm/pwm-bcm2835.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> > index 5652f461d994..edb2387c49a2 100644
> > --- a/drivers/pwm/pwm-bcm2835.c
> > +++ b/drivers/pwm/pwm-bcm2835.c
> > @@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  {
> >  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> >  	unsigned long rate = clk_get_rate(pc->clk);
> > -	unsigned long scaler;
> > +	unsigned int scaler;
> according to the commit log i wouldn't expect this change. Maybe it's
> worth to mention.

Yes, you are right, that needs explaining. I am trying to avoid an
unnecessary 64 bit division. I'll roll a v2.


Sean

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Sean Young]

Hi Stefan,

On Sun, Jun 02, 2019 at 06:45:44PM +0200, Stefan Wahren wrote:
> Am 02.06.19 um 12:23 schrieb Sean Young:
> > If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
> > carrier ends up being 476kHz.
> >
> > A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
> > this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
> > rounding errors, and we have a much more accurate carrier of 454.5kHz.
> >
> > Reported-by: Andreas Christ <andreas@christ-faesch.ch>
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/pwm/pwm-bcm2835.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> > index 5652f461d994..edb2387c49a2 100644
> > --- a/drivers/pwm/pwm-bcm2835.c
> > +++ b/drivers/pwm/pwm-bcm2835.c
> > @@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  {
> >  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> >  	unsigned long rate = clk_get_rate(pc->clk);
> > -	unsigned long scaler;
> > +	unsigned int scaler;
> according to the commit log i wouldn't expect this change. Maybe it's
> worth to mention.

Yes, you are right, that needs explaining. I am trying to avoid an
unnecessary 64 bit division. I'll roll a v2.


Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pwm: bcm2835: increase precision of pwm

[Stefan Wahren]

Hi Sean,

Am 02.06.19 um 22:03 schrieb Sean Young:
> Hi Stefan,
>
> On Sun, Jun 02, 2019 at 06:45:44PM +0200, Stefan Wahren wrote:
>> Am 02.06.19 um 12:23 schrieb Sean Young:
>>> If sending IR with carrier of 455kHz using the pwm-ir-tx driver, the
>>> carrier ends up being 476kHz.
>>>
>>> A carrier of 455kHz has a period of 2198ns, but the arithmetic truncates
>>> this to 2.1ns rather than 2.2ns. So, use DIV_ROUND_CLOSEST() to reduce
>>> rounding errors, and we have a much more accurate carrier of 454.5kHz.
>>>
>>> Reported-by: Andreas Christ <andreas@christ-faesch.ch>
>>> Signed-off-by: Sean Young <sean@mess.org>
>>> ---
>>>  drivers/pwm/pwm-bcm2835.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
>>> index 5652f461d994..edb2387c49a2 100644
>>> --- a/drivers/pwm/pwm-bcm2835.c
>>> +++ b/drivers/pwm/pwm-bcm2835.c
>>> @@ -63,14 +63,14 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>>  {
>>>  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
>>>  	unsigned long rate = clk_get_rate(pc->clk);
>>> -	unsigned long scaler;
>>> +	unsigned int scaler;
>> according to the commit log i wouldn't expect this change. Maybe it's
>> worth to mention.
> Yes, you are right, that needs explaining. I am trying to avoid an
> unnecessary 64 bit division. I'll roll a v2.

okay, but please give potential reviewer some time and don't post V2 too
soon.

In order to reproduce your results it would be helpful to know the
parent of the pwm clock (i assume plld_per) and the assigned clock rate
of pwm clock which differ between down- and upstream. I'm only
interested, so there is no need to add this to the commit log.

While looking at the code, i noticed another issue. The pwm driver was
written before the bcm2835 clk driver and used a fixed clock. Now with a
dynamic clock the range check for period_ns is wrong. I think the best
way is to check the calculated period value, which shouldn't be smaller
than 2.

I know this is a different issue, but while at this we should fix that
too (my untested draft below).

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 5652f46..d6ee85f 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -21,7 +21,7 @@
 #define PERIOD(x)        (((x) * 0x10) + 0x10)
 #define DUTY(x)            (((x) * 0x10) + 0x14)
 
-#define MIN_PERIOD        108        /* 9.2 MHz max. PWM clock */
+#define PERIOD_MIN        0x2
 
 struct bcm2835_pwm {
     struct pwm_chip chip;
@@ -64,6 +64,7 @@ static int bcm2835_pwm_config(struct pwm_chip *chip,
struct pwm_device *pwm,
     struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
     unsigned long rate = clk_get_rate(pc->clk);
     unsigned long scaler;
+    u32 period;
 
     if (!rate) {
         dev_err(pc->dev, "failed to get clock rate\n");
@@ -71,15 +72,13 @@ static int bcm2835_pwm_config(struct pwm_chip *chip,
struct pwm_device *pwm,
     }
 
     scaler = NSEC_PER_SEC / rate;
+    period = period_ns / scaler;
 
-    if (period_ns <= MIN_PERIOD) {
-        dev_err(pc->dev, "period %d not supported, minimum %d\n",
-            period_ns, MIN_PERIOD);
+    if (period < PERIOD_MIN)
         return -EINVAL;
-    }
 
     writel(duty_ns / scaler, pc->base + DUTY(pwm->hwpwm));
-    writel(period_ns / scaler, pc->base + PERIOD(pwm->hwpwm));
+    writel(period, pc->base + PERIOD(pwm->hwpwm));
 
     return 0;
 }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel