[PATCH v3 8/8] pwm: stm32: Implementation of the waveform callbacks
Uwe Kleine-König
u.kleine-koenig at baylibre.com
Wed Sep 4 10:05:22 PDT 2024
Hello Fabrice,
On Tue, Aug 20, 2024 at 06:09:59PM +0200, Fabrice Gasnier wrote:
> On 7/29/24 16:34, Uwe Kleine-König wrote:
> > Convert the stm32 pwm driver to use the new callbacks for hardware
> > programming.
>
> Sorry it took me some time to start to have a look. I only had an
> overview on the series, and this patch. I'd have some overall question
> on the waveform support (on the delay offset).
No need to be sorry, I very appreciate you looking into my patch set.
> > + wfhw->ccer = TIM_CCER_CCxE(ch + 1);
> > + if (priv->have_complementary_output)
> > + wfhw->ccer = TIM_CCER_CCxNE(ch);
>
> Need to use (ch + 1 here), and avoid overriding ccer when
> have_complementary_output is true, e.g.
>
> if (priv->have_complementary_output)
> wfhw->ccer |= TIM_CCER_CCxNE(ch + 1);
Huh, indeed. Thanks.
> > + rate = clk_get_rate(priv->clk);
> > +
> > + if (active_channels(priv) & ~(1 << ch * 4)) {
> > + u64 arr;
> > +
> > + /*
> > + * Other channels are already enabled, so the configured PSC and
> > + * ARR must be used for this channel, too.
> > + */
> > + ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
> > + if (ret)
> > + goto out;
> > +
> > + ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
> > + if (ret)
> > + goto out;
> > +
> > + /*
> > + * calculate the best value for ARR for the given PSC, refuse if
> > + * the resulting period gets bigger than the requested one.
> > + */
> > + arr = mul_u64_u64_div_u64(wf->period_length_ns, rate,
> > + (u64)NSEC_PER_SEC * (wfhw->psc + 1));
> > + if (arr <= wfhw->arr) {
> > + /*
> > + * requested period is small than the currently
> > + * configured and unchangable period, report back the smallest
> > + * possible period, i.e. the current state; Initialize
> > + * ccr to anything valid.
> > + */
> > + wfhw->ccr = 0;
> > + ret = 1;
>
> I'm suprised, I'm more used to return negative error codes. This may
> cascade up to the sysfs interface. Is there some possible side effect ?
I'm not entirely happy with that 1, too, but I didn't want to use an
existing error code, because I wanted to catch exactly the condition
that no valid rounding exists and so having a dedicated value for it
looks right to me. Then I didn't want to use a negative value to be sure
to not interpret it as an errno value.
This shouldn't propagate to the sysfs interface (or even __pwm_apply()).
I need to fix that.
> I could see in your commit message in "pwm: New abstraction for PWM
> waveforms" that "... this fact is signaled by a return value of 1".
>
> Perhaps some define could be used, to better point that ?
I shortly considered that while implementing, but decided against it
because I didn't wanted to clobber the fact, that it's a positive value.
Reading your suggestion I'll think about it again.
> > + if (wf->duty_length_ns && wf->duty_offset_ns &&
> > + wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns) {
>
> The condition here (mixing && + >=) is rather hard to read, could it be
> more readable ?
>
> It's more clear when reading pwm_wf2state() and pwm_state2wf() the
> condition for normal/inversed polarity rather looks like:
>
> if (wf->period_length_ns) {
> if (wf->duty_length_ns + wf->duty_offset_ns < wf->period_length_ns)
> /* normal */
> else
> /* inversed */
>
> BTW I notice small difference here: (wf->duty_length_ns &&
> wf->duty_offset_ns)
>
> Suggestion: could use some (new) helper function or macro from the pwm
> core ? e.g. rather than implementing in the driver ?
Hmm, this will indeed be useful for all drivers that have no way to
configure the offset in a more flexible way than inverting the polarity
(which I'd guess are most of them). I'll try an implementation to judge
if I like it.
> > + wfhw->ccer |= TIM_CCER_CCxP(ch + 1);
> > + if (priv->have_complementary_output)
> > + wfhw->ccer |= TIM_CCER_CCxNP(ch + 1);
> > +
> > + ccr = wfhw->arr + 1 - duty;
> > + } else {
> > + ccr = duty;
> > + }
> > +
> > + wfhw->ccr = min_t(u64, ccr, wfhw->arr + 1);
> > +
> > + dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> CCER: %08x, PSC: %08x, ARR: %08x, CCR: %08x\n",
> > + pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> > + rate, wfhw->ccer, wfhw->psc, wfhw->arr, wfhw->ccr);
> > +
> > +out:
> > + clk_disable(priv->clk);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * This should be moved to lib/math/div64.c. Currently there are some changes
> > + * pending to mul_u64_u64_div_u64. Uwe will care for that when the dust settles.
> > + */
> > +static u64 stm32_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
> > +{
> > + u64 res = mul_u64_u64_div_u64(a, b, c);
> > + /* Those multiplications might overflow but it doesn't matter */
> > + u64 rem = a * b - c * res;
> > +
> > + if (rem)
> > + res += 1;
> > +
> > + return res;
> > +}
> > +
> > +static int stm32_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + const void *_wfhw,
> > + struct pwm_waveform *wf)
> > +{
> > + const struct stm32_pwm_waveform *wfhw = _wfhw;
> > + struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
> > + unsigned int ch = pwm->hwpwm;
> > +
> > + if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
> > + unsigned long rate = clk_get_rate(priv->clk);
> > + u64 ccr_ns;
> > +
> > + /* The result doesn't overflow for rate >= 15259 */
> > + wf->period_length_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1),
> > + NSEC_PER_SEC, rate);
> > +
> > + ccr_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * wfhw->ccr,
> > + NSEC_PER_SEC, rate);
> > +
> > + if (wfhw->ccer & TIM_CCER_CCxP(ch + 1)) {
> > + wf->duty_length_ns =
> > + stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1 - wfhw->ccr),
> > + NSEC_PER_SEC, rate);
> > +
> > + wf->duty_offset_ns = ccr_ns;
> > + } else {
> > + wf->duty_length_ns = ccr_ns;
> > + wf->duty_offset_ns = 0;
> > + }
>
> Well, my main confusion is around duty_offset_ns. Indeed, it's right
> here, that with PWM mode 1 (CCMR bit 5 and 6 set later on), only
> possibilty is to have either 0, or the period - duty cycle as delay when
> polarity is inversed.
>
> I gave it a try (basic testing), but user doesn't get information when
> setting a waveform (with a valid duty_offset_ns), but the hardware
> doesn't necessarily apply it when writing the waveform ? What's your
> advise / opinion ?
The intended behaviour is that if you pass a duty_offset_ns >= period -
duty_cycle_ns (together with duty_offset > 0), you get inversed polarity.
This isn't signaled indeed. But the same holds true for other hardware
specific adaptions (e.g. when you pass period = 12345 and that's rounded
down to 12300 because of input clock constraints). If a consumer cares
about that, there is the possiblity to use .round_waveform_tohw() +
.round_waveform_fromhw() to know beforehand.
> > + } else {
> > + *wf = (struct pwm_waveform){
> > + .period_length_ns = 0,
> > + };
> > + }
>
> Would be nice to add similar dev_dbg() as in
> stm32_pwm_round_waveform_tohw().
Ack.
Thanks for your two good catches and your opion on my design,
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20240904/cb4845ec/attachment.sig>
More information about the linux-arm-kernel
mailing list