| Message ID | 6ce73b2688f059e7169935699044104cf37b2425.1697193646.git.sean@mess.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve pwm-ir-tx precision | expand |
Hi Alexander, Am 13.10.23 um 13:13 schrieb Alexander Stein: > Hi, > > Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren: >> Hi Sean, >> >> Am 13.10.23 um 12:46 schrieb Sean Young: >>> clk_get_rate() may do a mutex lock. Since the clock rate cannot change on >>> an rpi, simply fetch it once. >> does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837, >> BCM2711, BCM2712) for this change? >> >> Is it impossible that the real clock can never be influenced by turbo >> mode like SPI? > Assuming the clock can change, which I would, then a clock notifier seems > appropriate. See [1] for an example. i remember a similiar approach for the CPU frequency for the RPi. At end the we decided to let the firmware handle it and don't use clock notifier, see [2] and the related links for more background. The fact that the VideoCore has the real control makes it hard. I don't want to say that's impossible. [2] - https://patchwork.kernel.org/project/linux-arm-kernel/cover/20190520104708.11980-1-nsaenzjulienne@suse.de/ > > Best regards, > Alexander > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ > commit/?id=90ad2cbe88c22d0215225ab9594eeead0eb24fde > >> Best regards >> >>> Signed-off-by: Sean Young <sean@mess.org> >>> --- >>> >>> drivers/pwm/pwm-bcm2835.c | 21 ++++++++++++--------- >>> 1 file changed, 12 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c >>> index bdfc2a5ec0d6..59ea154dd657 100644 >>> --- a/drivers/pwm/pwm-bcm2835.c >>> +++ b/drivers/pwm/pwm-bcm2835.c >>> @@ -28,6 +28,7 @@ struct bcm2835_pwm { >>> >>> struct device *dev; >>> void __iomem *base; >>> struct clk *clk; >>> >>> + unsigned long rate; >>> >>> }; >>> >>> static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip) >>> >>> @@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(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 long period_cycles; >>> u64 max_period; >>> >>> u32 val; >>> >>> - if (!rate) { >>> - dev_err(pc->dev, "failed to get clock rate\n"); >>> - return -EINVAL; >>> - } >>> - >>> >>> /* >>> >>> * period_cycles must be a 32 bit value, so period * rate / >>> NSEC_PER_SEC >>> * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the >>> >>> @@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, >>> struct pwm_device *pwm,> >>> * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate >>> * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / > rate) >>> - 1 >>> */ >>> >>> - max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + > NSEC_PER_SEC >>> / 2, rate) - 1; + max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * >>> NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;> >>> if (state->period > max_period) >>> >>> return -EINVAL; >>> >>> /* set period */ >>> >>> - period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, >>> NSEC_PER_SEC); + period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * >>> pc->rate, NSEC_PER_SEC);> >>> /* don't accept a period that is too small */ >>> if (period_cycles < PERIOD_MIN) >>> >>> @@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, >>> struct pwm_device *pwm,> >>> writel(period_cycles, pc->base + PERIOD(pwm->hwpwm)); >>> >>> /* set duty cycle */ >>> >>> - val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC); >>> + val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, > NSEC_PER_SEC); >>> writel(val, pc->base + DUTY(pwm->hwpwm)); >>> >>> /* set polarity */ >>> >>> @@ -129,6 +124,7 @@ static const struct pwm_ops bcm2835_pwm_ops = { >>> >>> .request = bcm2835_pwm_request, >>> .free = bcm2835_pwm_free, >>> .apply = bcm2835_pwm_apply, >>> >>> + .atomic = true, >>> >>> .owner = THIS_MODULE, >>> >>> }; >>> >>> @@ -156,6 +152,13 @@ static int bcm2835_pwm_probe(struct platform_device >>> *pdev)> >>> if (ret) >>> >>> return ret; >>> >>> + pc->rate = clk_get_rate(pc->clk); >>> + if (!pc->rate) { >>> + dev_err(pc->dev, "failed to get clock rate\n"); >>> + ret = -EINVAL; >>> + goto add_fail; >>> + } >>> + >>> >>> pc->chip.dev = &pdev->dev; >>> pc->chip.ops = &bcm2835_pwm_ops; >>> pc->chip.npwm = 2; >
Hello Ivaylo, On Sat, Oct 14, 2023 at 09:51:12AM +0300, Ivaylo Dimitrov wrote: > On 13.10.23 г. 20:51 ч., Uwe Kleine-König wrote: > > Hello, > > > > On Fri, Oct 13, 2023 at 01:13:50PM +0200, Alexander Stein wrote: > > > Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren: > > > > Am 13.10.23 um 12:46 schrieb Sean Young: > > > > > clk_get_rate() may do a mutex lock. Since the clock rate cannot change on > > > > > an rpi, simply fetch it once. > > > > > > > > does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837, > > > > BCM2711, BCM2712) for this change? > > > > > > > > Is it impossible that the real clock can never be influenced by turbo > > > > mode like SPI? > > > > > > Assuming the clock can change, which I would, then a clock notifier seems > > > appropriate. See [1] for an example. > > > > I'm not a fan. If the clock changes, the output also changes. With a > > clock notifier you can soften the issue and reconfigure to something > > similar as the original wave form, but a glitch happens for sure. > > > > Right, but without notifier, everything rate related after the change will > be wrong So we agree clk_rate_exclusive_get() is the way to go?! It's simple, no need for a notifier and no glitches. Best regards Uwe
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c index bdfc2a5ec0d6..59ea154dd657 100644 --- a/drivers/pwm/pwm-bcm2835.c +++ b/drivers/pwm/pwm-bcm2835.c @@ -28,6 +28,7 @@ struct bcm2835_pwm { struct device *dev; void __iomem *base; struct clk *clk; + unsigned long rate; }; static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip) @@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(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 long period_cycles; u64 max_period; u32 val; - if (!rate) { - dev_err(pc->dev, "failed to get clock rate\n"); - return -EINVAL; - } - /* * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the @@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1 */ - max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1; + max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1; if (state->period > max_period) return -EINVAL; /* set period */ - period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC); + period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * pc->rate, NSEC_PER_SEC); /* don't accept a period that is too small */ if (period_cycles < PERIOD_MIN) @@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, writel(period_cycles, pc->base + PERIOD(pwm->hwpwm)); /* set duty cycle */ - val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC); + val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, NSEC_PER_SEC); writel(val, pc->base + DUTY(pwm->hwpwm)); /* set polarity */ @@ -129,6 +124,7 @@ static const struct pwm_ops bcm2835_pwm_ops = { .request = bcm2835_pwm_request, .free = bcm2835_pwm_free, .apply = bcm2835_pwm_apply, + .atomic = true, .owner = THIS_MODULE, }; @@ -156,6 +152,13 @@ static int bcm2835_pwm_probe(struct platform_device *pdev) if (ret) return ret; + pc->rate = clk_get_rate(pc->clk); + if (!pc->rate) { + dev_err(pc->dev, "failed to get clock rate\n"); + ret = -EINVAL; + goto add_fail; + } + pc->chip.dev = &pdev->dev; pc->chip.ops = &bcm2835_pwm_ops; pc->chip.npwm = 2;
clk_get_rate() may do a mutex lock. Since the clock rate cannot change on an rpi, simply fetch it once. Signed-off-by: Sean Young <sean@mess.org> --- drivers/pwm/pwm-bcm2835.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)