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

Stefan Wahren
2023-10-13T11:44:17Z

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;
>