Re: [PATCH 0/5] Renesas MTU3 PWM / counter fixes

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4691 bytes --]

On Fri, Jan 30, 2026 at 02:23:48PM +0200, Cosmin Tanislav wrote:
> The Renesas MTU3 PWM and counter drivers have some issues which have
> been observed while adding support for the Renesas RZ/T2H and RZ/N2H
> SoCs.
> 
> This series fixes the most important issues which prevent normal
> functioning of the driver, while other less important issues will be
> handled in a future series.
> 
> Questions for the PWM maintainer:
> 
> 1)
> 
> The handling introduced in patches 1 and 2 is similar to the approach
> taken in commits [1] and [2].
> 
> Is this the right approach?
> 
> This code snippet below extracted from drivers/pwm/pwm-rzg2l-gpt.c
> entirely prevents the period ticks from changing at all when two PWM
> channels backed by the same HW channel are in use.

Yes, this is a known issue. But there is no sane alternative I'm aware
of as one consumer must not change the settings affecting a different
consumer.

> if (rzg2l_gpt->channel_request_count[ch] > 1) {
>   u8 sibling_ch = rzg2l_gpt_sibling(pwm->hwpwm);
> 
>   if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, sibling_ch)) {
>     if (period_ticks < rzg2l_gpt->period_ticks[ch])
>       return -EBUSY;
> 
>     period_ticks = rzg2l_gpt->period_ticks[ch];
>   }
> }
> 
> Same logic has been imposed in patches 1 and 2 as the HW limitation is
> similar.
> 
> Based on the logic here, a second channel can be enabled as long as its
> period is larger than the period of the first enabled channel, and the
> period and duty cycle will be adjusted to be <= to the period of the
> first enabled channel.
> 
> But once the second channel is enabled, there's no way to adjust the
> period of either channels anymore.
> 
> Wouldn't a better solution be that the smallest period between the two
> channels is used, so that adjustment is still possible dynamically?
> 
> Here is an example.
> 
> echo 0 > /sys/class/pwm/pwmchip0/export
> echo 1 > /sys/class/pwm/pwmchip0/export
> echo 0xFFF000 > /sys/class/pwm/pwmchip0/pwm0/period
> echo 0x7FF800 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
> echo 0xFFF000 > /sys/class/pwm/pwmchip0/pwm1/period
> echo 0x7FF800 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
> echo 1 > /sys/class/pwm/pwmchip0/pwm1/enable
> 
> Now both LEDs are on, let's increase the period.
> 
> echo 0xFFFF000 > /sys/class/pwm/pwmchip0/pwm0/period
> echo 0x7FFF800 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
> 
> The effective period did not change.

Yeah, so pwm0 still runs at 0xFFF000. And setting the duty_cycle to
0x7FFF800 also results in it being 0xFFF000.
 
> echo 0xFFFF000 > /sys/class/pwm/pwmchip0/pwm1/period
> echo 0x7FFF800 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
> 
> The effective period didn't change now either.
> 
> echo 0 > /sys/class/pwm/pwmchip0/pwm0/enable
> echo 0 > /sys/class/pwm/pwmchip0/pwm1/enable
> 
> echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
> echo 1 > /sys/class/pwm/pwmchip0/pwm1/enable
> 
> After turning the PWMs off and on again, the effective period is
> updated.

Yes, let's note that the sysfs API is strange.

> If we were to change the period dynamically to the smallest one, the
> LEDs would have changed their effective period without needing to be
> turned off and on again.

Not all consumers only care about the relative duty cycle. So changing
the period behind the back of a consumer even when keeping the relative
duty cycle is a no go.

> Would this approach be better than the current approach? I can see that
> other drivers just refuse updating the period entirely when the PWM
> channels must share the same period.
> 
> 
> 2)
> 
> Another issue that I've encountered is that PWM is left enabled even if
> the channel is unexported.
> 
> Here is an example.
> 
> echo 0 > /sys/class/pwm/pwmchip0/export
> echo 0xFFFF000 > /sys/class/pwm/pwmchip0/pwm0/period
> echo 0x7FFF800 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
> echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
> echo 0 > /sys/class/pwm/pwmchip0/unexport
> 
> The connected LED is kept blinking as 0 was not written to enable.
> 
> Is this intended? Or should the PWM turn off on unexport?

For in-kernel users of a PWM it's the consumer's responsibility to
disable a PWM before pwm_put(). I think it's more natural to have the
same for /sys (and also the chardev interface).

> 3)
> 
> Should the .get_state() ops read the period and duty cycle from the
> hardware if the PWM is not enabled?

It doesn't need to.

> Currently the MTU3 driver guards reading period and duty cycle based on
> whether the PWM is enabled.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]