| Message ID | 20190602102350.zzwmfvlky3mnlqln@gofer.mess.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: bcm2835: increase precision of pwm | expand |
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
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.
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
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; }
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; }
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(-)