Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

Alexander Stein
2023-10-13T11:14:01Z

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.

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;

--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider