[PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Cosmin Tanislav]

enable_count is only incremented after rz_mtu3_pwm_config() is called
for the current PWM channel, causing prescale to not be checked if one
PWM channel is enabled and we're enabling the second PWM channel of the
same HW channel.

To handle this edge case, if the user_count of the HW channel is larger
than 1 and the sibling PWM channel is enabled, check that the new
prescale is not smaller than the sibling's prescale.

If the new prescale is larger than the sibling's prescale, use the
sibling's prescale.

The user_count check is ensures that we are indeed dealing with a HW
channel that has two IOs.

Cc: stable@vger.kernel.org
Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
---
 drivers/pwm/pwm-rz-mtu3.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
index ab39bd37edaf..f6073be1c2f8 100644
--- a/drivers/pwm/pwm-rz-mtu3.c
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -142,6 +142,14 @@ rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
 	return priv;
 }
 
+static u32 rz_mtu3_sibling_hwpwm(u32 hwpwm, bool is_primary)
+{
+	if (is_primary)
+		return hwpwm + 1;
+	else
+		return hwpwm - 1;
+}
+
 static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
 				      u32 hwpwm)
 {
@@ -322,6 +330,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct rz_mtu3_pwm_channel *priv;
 	u64 period_cycles;
 	u64 duty_cycles;
+	bool is_primary;
 	u8 prescale;
 	u16 pv, dc;
 	u8 val;
@@ -329,6 +338,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
 	ch = priv - rz_mtu3_pwm->channel_data;
+	is_primary = priv->map->base_pwm_number == pwm->hwpwm;
 
 	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
 					NSEC_PER_SEC);
@@ -340,11 +350,15 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * different settings. Modify prescalar if other PWM is off or handle
 	 * it, if current prescale value is less than the one we want to set.
 	 */
-	if (rz_mtu3_pwm->enable_count[ch] > 1) {
-		if (rz_mtu3_pwm->prescale[ch] > prescale)
-			return -EBUSY;
+	if (rz_mtu3_pwm->user_count[ch] > 1) {
+		u32 sibling_hwpwm = rz_mtu3_sibling_hwpwm(pwm->hwpwm, is_primary);
 
-		prescale = rz_mtu3_pwm->prescale[ch];
+		if (rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
+			if (rz_mtu3_pwm->prescale[ch] > prescale)
+				return -EBUSY;
+
+			prescale = rz_mtu3_pwm->prescale[ch];
+		}
 	}
 
 	pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
@@ -371,7 +385,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (rz_mtu3_pwm->prescale[ch] != prescale && rz_mtu3_pwm->enable_count[ch])
 		rz_mtu3_disable(priv->mtu);
 
-	if (priv->map->base_pwm_number == pwm->hwpwm) {
+	if (is_primary) {
 		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
 				      RZ_MTU3_TCR_CCLR_TGRA | val);
 		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,
-- 
2.52.0

Re: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Uwe Kleine-König]

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

Hello Cosmin,

On Fri, Jan 30, 2026 at 02:23:49PM +0200, Cosmin Tanislav wrote:
> enable_count is only incremented after rz_mtu3_pwm_config() is called
> for the current PWM channel, causing prescale to not be checked if one
> PWM channel is enabled and we're enabling the second PWM channel of the
> same HW channel.

I don't understand the issue. If the second PWM channel is enabled
while the first is only requested, changing the period is fine?!

Can you please show a sequence of events that result in bad behaviour?

Best regards
Uwe

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

RE: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Cosmin-Gabriel Tanislav]

> From: Uwe Kleine-König <ukleinek@kernel.org>
> Sent: Thursday, March 5, 2026 10:57 AM
> 
> Hello Cosmin,
> 
> On Fri, Jan 30, 2026 at 02:23:49PM +0200, Cosmin Tanislav wrote:
> > enable_count is only incremented after rz_mtu3_pwm_config() is called
> > for the current PWM channel, causing prescale to not be checked if one
> > PWM channel is enabled and we're enabling the second PWM channel of the
> > same HW channel.
> 
> I don't understand the issue. If the second PWM channel is enabled
> while the first is only requested, changing the period is fine?!
> 
> Can you please show a sequence of events that result in bad behaviour?
> 

Hello Uwe. The issue happens when a PWM channel is already enabled,
and we're trying to enable a second PWM channel backed by the same
HW channel, but with a different prescale. Although, because of
other HW limitations we cannot really have differing periods, but
that's handled in the following patch, 2/5.

Here's a sequence of commands that results in bad behavior.

I've added a print for the enable count and period before the
enable_count check, and prints for the actual period / duty cycle
register writes, just to show that it gets thar far.

root@rzt2h-evk:~# echo 0 > /sys/class/pwm/pwmchip0/export
root@rzt2h-evk:~# echo 1 > /sys/class/pwm/pwmchip0/export

root@rzt2h-evk:~# echo 0xffff0 > /sys/class/pwm/pwmchip0/pwm0/period
root@rzt2h-evk:~# echo 0x7fff0 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
root@rzt2h-evk:~# echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
[   71.916095] pwm pwmchip0: enable_count: 0, period: ffff0, prescale: 1
[   71.924085] pwm pwmchip0: TGRA: ffff, TGRB: 7fff

root@rzt2h-evk:~# echo 0xffff00 > /sys/class/pwm/pwmchip0/pwm1/period
root@rzt2h-evk:~# echo 0x7fff00 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
root@rzt2h-evk:~# echo 1 > /sys/class/pwm/pwmchip0/pwm1/enable
[   80.063208] pwm pwmchip0: enable_count: 1, period: ffff00, prescale: 3
[   80.071028] pwm pwmchip0: TGRC: ffff, TGRD: 7fff

As you can notice, at the time of the enable_count check for the second
PWM channel, enable_count is equal to 1, so it does not pass the > 1
condition, the prescale value is not validated, and it ends up overriding
the previous prescale, messing up the already set period and duty cycle
of the previously enabled PWM channel.

> Best regards
> Uwe

Re: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Uwe Kleine-König]

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

Hello,

On Fri, Jan 30, 2026 at 02:23:49PM +0200, Cosmin Tanislav wrote:
> enable_count is only incremented after rz_mtu3_pwm_config() is called
> for the current PWM channel, causing prescale to not be checked if one
> PWM channel is enabled and we're enabling the second PWM channel of the
> same HW channel.
> 
> To handle this edge case, if the user_count of the HW channel is larger
> than 1 and the sibling PWM channel is enabled, check that the new
> prescale is not smaller than the sibling's prescale.
> 
> If the new prescale is larger than the sibling's prescale, use the
> sibling's prescale.
> 
> The user_count check is ensures that we are indeed dealing with a HW
> channel that has two IOs.
> 
> Cc: stable@vger.kernel.org
> Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
> Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
> ---
>  drivers/pwm/pwm-rz-mtu3.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
> index ab39bd37edaf..f6073be1c2f8 100644
> --- a/drivers/pwm/pwm-rz-mtu3.c
> +++ b/drivers/pwm/pwm-rz-mtu3.c
> @@ -142,6 +142,14 @@ rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
>  	return priv;
>  }
>  
> +static u32 rz_mtu3_sibling_hwpwm(u32 hwpwm, bool is_primary)
> +{
> +	if (is_primary)
> +		return hwpwm + 1;
> +	else
> +		return hwpwm - 1;
> +}

Can we please make this function a bit more sophisticated to not need
is_primary? Something like:

static u32 rz_mtu3_sibling_hwpwm(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
{
	struct rz_mtu3_pwm_channel *priv = rz_mtu3_get_channel(rz_mtu3_pwm, hwpwm);

	BUG_ON(priv->map->num_channel_ios != 2);

	if (priv->map->base_pwm_number == hwpwm)
		return hwpwm + 1;
	else
		return hwpwm - 1;
}

(Or if you want to save the rz_mtu3_get_channel() call, pass priv to
rz_mtu3_sibling_hwpwm() which is already available at the call sites.)

And well, BUG_ON isn't very loved, so either it should be dropped or the
issue escalated in a more civilized manner. I keep it for the sake of
simplicity during the discussion.

> +
>  static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
>  				      u32 hwpwm)
>  {
> @@ -322,6 +330,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct rz_mtu3_pwm_channel *priv;
>  	u64 period_cycles;
>  	u64 duty_cycles;
> +	bool is_primary;
>  	u8 prescale;
>  	u16 pv, dc;
>  	u8 val;
> @@ -329,6 +338,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
>  	ch = priv - rz_mtu3_pwm->channel_data;
> +	is_primary = priv->map->base_pwm_number == pwm->hwpwm;
>  
>  	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
>  					NSEC_PER_SEC);
> @@ -340,11 +350,15 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * different settings. Modify prescalar if other PWM is off or handle
>  	 * it, if current prescale value is less than the one we want to set.
>  	 */
> -	if (rz_mtu3_pwm->enable_count[ch] > 1) {
> -		if (rz_mtu3_pwm->prescale[ch] > prescale)
> -			return -EBUSY;

OK, I understood the issue. If the sibling is already on and the current
IO is still off, enable_count doesn't account yet for the current
IO and thus is 1 but still the prescaler must not be changed.

The commit log needs updating to make this clearer.

An alternative would be to check for

	if (rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1) > 1)

but I'm not sure this is better.

> +	if (rz_mtu3_pwm->user_count[ch] > 1) {
> +		u32 sibling_hwpwm = rz_mtu3_sibling_hwpwm(pwm->hwpwm, is_primary);

Maybe add a comment here saying something like:

	Not all channels have a sibling, but if user_count > 1 there is
	one.
>  
> -		prescale = rz_mtu3_pwm->prescale[ch];
> +		if (rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
> +			if (rz_mtu3_pwm->prescale[ch] > prescale)
> +				return -EBUSY;
> +
> +			prescale = rz_mtu3_pwm->prescale[ch];
> +		}
>  	}
>  
>  	pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
> @@ -371,7 +385,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (rz_mtu3_pwm->prescale[ch] != prescale && rz_mtu3_pwm->enable_count[ch])
>  		rz_mtu3_disable(priv->mtu);
>  
> -	if (priv->map->base_pwm_number == pwm->hwpwm) {
> +	if (is_primary) {
>  		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
>  				      RZ_MTU3_TCR_CCLR_TGRA | val);
>  		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,

All in all I'm unhappy with the hwpwm to channel+IO mapping, this makes
this all more complicated. This is something that already bugged me when
this driver was created.

It's out of scope for this series of fixes, but I wonder if we could
create a mapping from hwpwm to an IO-id like this:

	hwpwm | IO-id
	------+------
	   0  |    0	(channel 0, io 0)
	   1  |    1	(channel 0, io 1)
	   2  |    2	(channel 1, io 0)
	   3  |    4	(channel 2, io 0)
           4  |    6	(channel 3, io 0)
	   5  |    7	(channel 3, io 1)
	   6  |    8	(channel 4, io 0)
	   7  |    9	(channel 4, io 1)
	   8  |   12	(channel 6, io 0)
	   9  |   13	(channel 6, io 1)
	  10  |   14	(channel 7, io 0)
	  11  |   15	(channel 7, io 1)

then the sibling would be just `io_id ^ 1` and the channel could
be computed by `io_id >> 1` and the base id for a given io is just
`io_id & ~1`.

Tracking of an IO being enabled could be done using

	enabled_io & (1 << io_id)

I think this would be a simpler scheme that needs less memory and less
pointer dereferencing and the check for the sibling being enabled would
also be a trivial bit operation.

Best regards
Uwe

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

RE: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Cosmin-Gabriel Tanislav]

> From: Uwe Kleine-König <ukleinek@kernel.org>
> Sent: Friday, March 6, 2026 11:30 AM
> 
> Hello,
> 
> On Fri, Jan 30, 2026 at 02:23:49PM +0200, Cosmin Tanislav wrote:
> > enable_count is only incremented after rz_mtu3_pwm_config() is called
> > for the current PWM channel, causing prescale to not be checked if one
> > PWM channel is enabled and we're enabling the second PWM channel of the
> > same HW channel.
> >
> > To handle this edge case, if the user_count of the HW channel is larger
> > than 1 and the sibling PWM channel is enabled, check that the new
> > prescale is not smaller than the sibling's prescale.
> >
> > If the new prescale is larger than the sibling's prescale, use the
> > sibling's prescale.
> >
> > The user_count check is ensures that we are indeed dealing with a HW
> > channel that has two IOs.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
> > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
> > ---
> >  drivers/pwm/pwm-rz-mtu3.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
> > index ab39bd37edaf..f6073be1c2f8 100644
> > --- a/drivers/pwm/pwm-rz-mtu3.c
> > +++ b/drivers/pwm/pwm-rz-mtu3.c
> > @@ -142,6 +142,14 @@ rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> >  	return priv;
> >  }
> >
> > +static u32 rz_mtu3_sibling_hwpwm(u32 hwpwm, bool is_primary)
> > +{
> > +	if (is_primary)
> > +		return hwpwm + 1;
> > +	else
> > +		return hwpwm - 1;
> > +}
> 
> Can we please make this function a bit more sophisticated to not need
> is_primary? Something like:
> 
> static u32 rz_mtu3_sibling_hwpwm(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> {
> 	struct rz_mtu3_pwm_channel *priv = rz_mtu3_get_channel(rz_mtu3_pwm, hwpwm);
> 
> 	BUG_ON(priv->map->num_channel_ios != 2);
> 
> 	if (priv->map->base_pwm_number == hwpwm)
> 		return hwpwm + 1;
> 	else
> 		return hwpwm - 1;
> }
> 
> (Or if you want to save the rz_mtu3_get_channel() call, pass priv to
> rz_mtu3_sibling_hwpwm() which is already available at the call sites.)
> 
> And well, BUG_ON isn't very loved, so either it should be dropped or the
> issue escalated in a more civilized manner. I keep it for the sake of
> simplicity during the discussion.
> 

I can do that. And, to avoid having the BUG_ON(), we can make it return
an int and receive a sibling_hwpwm pointer as an output parameter.

With that in mind, this patch could be simplified to the following diff
(approximatively, I haven't tested it yet).

Please let me know what you think the best solution would be.

diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
index ab39bd37edaf..4548af0c3b3c 100644
--- a/drivers/pwm/pwm-rz-mtu3.c
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -142,6 +142,20 @@ rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
 	return priv;
 }
 
+static int rz_mtu3_sibling_hwpwm(struct rz_mtu3_pwm_channel *priv, u32 hwpwm,
+				 u32 *sibling_hwpwm)
+{
+	if (priv->map->num_channel_ios != 2)
+		return -EINVAL;
+
+	if (priv->map->base_pwm_number == hwpwm)
+		*sibling_hwpwm = hwpwm + 1;
+	else
+		*sibling_hwpwm = hwpwm - 1;
+
+	return 0;
+}
+
 static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
 				      u32 hwpwm)
 {
@@ -321,6 +335,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
 	struct rz_mtu3_pwm_channel *priv;
 	u64 period_cycles;
+	u32 sibling_hwpwm;
 	u64 duty_cycles;
 	u8 prescale;
 	u16 pv, dc;
@@ -340,7 +355,9 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * different settings. Modify prescalar if other PWM is off or handle
 	 * it, if current prescale value is less than the one we want to set.
 	 */
-	if (rz_mtu3_pwm->enable_count[ch] > 1) {
+	if (rz_mtu3_pwm->user_count[ch] > 1 &&
+	    !rz_mtu3_sibling_hwpwm(priv, pwm->hwpwm, &sibling_hwpwm) &&
+	    rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
 		if (rz_mtu3_pwm->prescale[ch] > prescale)
 			return -EBUSY;


> > +
> >  static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> >  				      u32 hwpwm)
> >  {
> > @@ -322,6 +330,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	struct rz_mtu3_pwm_channel *priv;
> >  	u64 period_cycles;
> >  	u64 duty_cycles;
> > +	bool is_primary;
> >  	u8 prescale;
> >  	u16 pv, dc;
> >  	u8 val;
> > @@ -329,6 +338,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >
> >  	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> >  	ch = priv - rz_mtu3_pwm->channel_data;
> > +	is_primary = priv->map->base_pwm_number == pwm->hwpwm;
> >
> >  	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
> >  					NSEC_PER_SEC);
> > @@ -340,11 +350,15 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	 * different settings. Modify prescalar if other PWM is off or handle
> >  	 * it, if current prescale value is less than the one we want to set.
> >  	 */
> > -	if (rz_mtu3_pwm->enable_count[ch] > 1) {
> > -		if (rz_mtu3_pwm->prescale[ch] > prescale)
> > -			return -EBUSY;
> 
> OK, I understood the issue. If the sibling is already on and the current
> IO is still off, enable_count doesn't account yet for the current
> IO and thus is 1 but still the prescaler must not be changed.
> 
> The commit log needs updating to make this clearer.
> 

I'll try to rephrase it to make it clearer.

> An alternative would be to check for
> 
> 	if (rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1) > 1)
> 
> but I'm not sure this is better.
> 

This was essentially my initial solution internally, but it was argued by
my colleagues that it would be difficult to understand.

The solution that I ended up submitting here is more explicit and easier
to grasp at a glance, at the expense of being lengthier.

I still quite prefer the shorter solution, as it is not necessary to query
the actual hardware state in this scenario, as the PWM state should always
be in sync with it.

The PWM state is enough to figure out the effective enable_count, as we can
only make it into this function when
a) the PWM channel is already enabled and it is being updated OR
b) when the PWM channel is being enabled (and it was previously disabled).

> > +	if (rz_mtu3_pwm->user_count[ch] > 1) {
> > +		u32 sibling_hwpwm = rz_mtu3_sibling_hwpwm(pwm->hwpwm, is_primary);
> 
> Maybe add a comment here saying something like:
> 
> 	Not all channels have a sibling, but if user_count > 1 there is
> 	one.

Let's figure out which solution would be the best, and I will add comments
for any of the unclear things.

> >
> > -		prescale = rz_mtu3_pwm->prescale[ch];
> > +		if (rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
> > +			if (rz_mtu3_pwm->prescale[ch] > prescale)
> > +				return -EBUSY;
> > +
> > +			prescale = rz_mtu3_pwm->prescale[ch];
> > +		}
> >  	}
> >
> >  	pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
> > @@ -371,7 +385,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	if (rz_mtu3_pwm->prescale[ch] != prescale && rz_mtu3_pwm->enable_count[ch])
> >  		rz_mtu3_disable(priv->mtu);
> >
> > -	if (priv->map->base_pwm_number == pwm->hwpwm) {
> > +	if (is_primary) {
> >  		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> >  				      RZ_MTU3_TCR_CCLR_TGRA | val);
> >  		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,
> 
> All in all I'm unhappy with the hwpwm to channel+IO mapping, this makes
> this all more complicated. This is something that already bugged me when
> this driver was created.
> 
> It's out of scope for this series of fixes, but I wonder if we could
> create a mapping from hwpwm to an IO-id like this:
> 
> 	hwpwm | IO-id
> 	------+------
> 	   0  |    0	(channel 0, io 0)
> 	   1  |    1	(channel 0, io 1)
> 	   2  |    2	(channel 1, io 0)
> 	   3  |    4	(channel 2, io 0)
>            4  |    6	(channel 3, io 0)
> 	   5  |    7	(channel 3, io 1)
> 	   6  |    8	(channel 4, io 0)
> 	   7  |    9	(channel 4, io 1)
> 	   8  |   12	(channel 6, io 0)
> 	   9  |   13	(channel 6, io 1)
> 	  10  |   14	(channel 7, io 0)
> 	  11  |   15	(channel 7, io 1)
> 
> then the sibling would be just `io_id ^ 1` and the channel could
> be computed by `io_id >> 1` and the base id for a given io is just
> `io_id & ~1`.
> 
> Tracking of an IO being enabled could be done using
> 
> 	enabled_io & (1 << io_id)
> 
> I think this would be a simpler scheme that needs less memory and less
> pointer dereferencing and the check for the sibling being enabled would
> also be a trivial bit operation.
> 

I agree that the current setup is not the best. Especially the loop inside
rz_mtu3_get_channel() is quite sub-optimal, in my opinion.

I have many more patches already implemented and prepared to be sent for
MTU3, including conversion to waveform APIs, a lot of cleanups, support
for more prescale values, bootloader handoff support, etc, but I have
sent the fixes first as they are higher priority.

I will try to implement your mapping improvement idea and integrate it in
one of the later series of patches.

Please let me know which solution you think is the best for dealing with
the issue the current patch is trying to solve, and I'll continue from
there.

> Best regards
> Uwe

RE: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Cosmin-Gabriel Tanislav]

> From: Cosmin-Gabriel Tanislav
> Sent: Friday, March 6, 2026 3:27 PM
> 
> > From: Uwe Kleine-König <ukleinek@kernel.org>
> > Sent: Friday, March 6, 2026 11:30 AM
> >
> > Hello,
> >
> > On Fri, Jan 30, 2026 at 02:23:49PM +0200, Cosmin Tanislav wrote:
> > > enable_count is only incremented after rz_mtu3_pwm_config() is called
> > > for the current PWM channel, causing prescale to not be checked if one
> > > PWM channel is enabled and we're enabling the second PWM channel of the
> > > same HW channel.
> > >
> > > To handle this edge case, if the user_count of the HW channel is larger
> > > than 1 and the sibling PWM channel is enabled, check that the new
> > > prescale is not smaller than the sibling's prescale.
> > >
> > > If the new prescale is larger than the sibling's prescale, use the
> > > sibling's prescale.
> > >
> > > The user_count check is ensures that we are indeed dealing with a HW
> > > channel that has two IOs.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
> > > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
> > > ---
> > >  drivers/pwm/pwm-rz-mtu3.c | 24 +++++++++++++++++++-----
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
> > > index ab39bd37edaf..f6073be1c2f8 100644
> > > --- a/drivers/pwm/pwm-rz-mtu3.c
> > > +++ b/drivers/pwm/pwm-rz-mtu3.c
> > > @@ -142,6 +142,14 @@ rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> > >  	return priv;
> > >  }
> > >
> > > +static u32 rz_mtu3_sibling_hwpwm(u32 hwpwm, bool is_primary)
> > > +{
> > > +	if (is_primary)
> > > +		return hwpwm + 1;
> > > +	else
> > > +		return hwpwm - 1;
> > > +}
> >
> > Can we please make this function a bit more sophisticated to not need
> > is_primary? Something like:
> >
> > static u32 rz_mtu3_sibling_hwpwm(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> > {
> > 	struct rz_mtu3_pwm_channel *priv = rz_mtu3_get_channel(rz_mtu3_pwm, hwpwm);
> >
> > 	BUG_ON(priv->map->num_channel_ios != 2);
> >
> > 	if (priv->map->base_pwm_number == hwpwm)
> > 		return hwpwm + 1;
> > 	else
> > 		return hwpwm - 1;
> > }
> >
> > (Or if you want to save the rz_mtu3_get_channel() call, pass priv to
> > rz_mtu3_sibling_hwpwm() which is already available at the call sites.)
> >
> > And well, BUG_ON isn't very loved, so either it should be dropped or the
> > issue escalated in a more civilized manner. I keep it for the sake of
> > simplicity during the discussion.
> >
> 
> I can do that. And, to avoid having the BUG_ON(), we can make it return
> an int and receive a sibling_hwpwm pointer as an output parameter.
> 
> With that in mind, this patch could be simplified to the following diff
> (approximatively, I haven't tested it yet).
> 
> Please let me know what you think the best solution would be.
> 
> diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
> index ab39bd37edaf..4548af0c3b3c 100644
> --- a/drivers/pwm/pwm-rz-mtu3.c
> +++ b/drivers/pwm/pwm-rz-mtu3.c
> @@ -142,6 +142,20 @@ rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
>  	return priv;
>  }
> 
> +static int rz_mtu3_sibling_hwpwm(struct rz_mtu3_pwm_channel *priv, u32 hwpwm,
> +				 u32 *sibling_hwpwm)
> +{
> +	if (priv->map->num_channel_ios != 2)
> +		return -EINVAL;
> +
> +	if (priv->map->base_pwm_number == hwpwm)
> +		*sibling_hwpwm = hwpwm + 1;
> +	else
> +		*sibling_hwpwm = hwpwm - 1;
> +
> +	return 0;
> +}
> +
>  static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
>  				      u32 hwpwm)
>  {
> @@ -321,6 +335,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
>  	struct rz_mtu3_pwm_channel *priv;
>  	u64 period_cycles;
> +	u32 sibling_hwpwm;
>  	u64 duty_cycles;
>  	u8 prescale;
>  	u16 pv, dc;
> @@ -340,7 +355,9 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * different settings. Modify prescalar if other PWM is off or handle
>  	 * it, if current prescale value is less than the one we want to set.
>  	 */
> -	if (rz_mtu3_pwm->enable_count[ch] > 1) {
> +	if (rz_mtu3_pwm->user_count[ch] > 1 &&
> +	    !rz_mtu3_sibling_hwpwm(priv, pwm->hwpwm, &sibling_hwpwm) &&
> +	    rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
>  		if (rz_mtu3_pwm->prescale[ch] > prescale)
>  			return -EBUSY;
> 
> 
> > > +
> > >  static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> > >  				      u32 hwpwm)
> > >  {
> > > @@ -322,6 +330,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  	struct rz_mtu3_pwm_channel *priv;
> > >  	u64 period_cycles;
> > >  	u64 duty_cycles;
> > > +	bool is_primary;
> > >  	u8 prescale;
> > >  	u16 pv, dc;
> > >  	u8 val;
> > > @@ -329,6 +338,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >
> > >  	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > >  	ch = priv - rz_mtu3_pwm->channel_data;
> > > +	is_primary = priv->map->base_pwm_number == pwm->hwpwm;
> > >
> > >  	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
> > >  					NSEC_PER_SEC);
> > > @@ -340,11 +350,15 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  	 * different settings. Modify prescalar if other PWM is off or handle
> > >  	 * it, if current prescale value is less than the one we want to set.
> > >  	 */
> > > -	if (rz_mtu3_pwm->enable_count[ch] > 1) {
> > > -		if (rz_mtu3_pwm->prescale[ch] > prescale)
> > > -			return -EBUSY;
> >
> > OK, I understood the issue. If the sibling is already on and the current
> > IO is still off, enable_count doesn't account yet for the current
> > IO and thus is 1 but still the prescaler must not be changed.
> >
> > The commit log needs updating to make this clearer.
> >
> 
> I'll try to rephrase it to make it clearer.
> 
> > An alternative would be to check for
> >
> > 	if (rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1) > 1)
> >
> > but I'm not sure this is better.
> >
> 
> This was essentially my initial solution internally, but it was argued by
> my colleagues that it would be difficult to understand.
> 
> The solution that I ended up submitting here is more explicit and easier
> to grasp at a glance, at the expense of being lengthier.
> 
> I still quite prefer the shorter solution, as it is not necessary to query
> the actual hardware state in this scenario, as the PWM state should always
> be in sync with it.
> 
> The PWM state is enough to figure out the effective enable_count, as we can
> only make it into this function when
> a) the PWM channel is already enabled and it is being updated OR
> b) when the PWM channel is being enabled (and it was previously disabled).
> 
> > > +	if (rz_mtu3_pwm->user_count[ch] > 1) {
> > > +		u32 sibling_hwpwm = rz_mtu3_sibling_hwpwm(pwm->hwpwm, is_primary);
> >
> > Maybe add a comment here saying something like:
> >
> > 	Not all channels have a sibling, but if user_count > 1 there is
> > 	one.
> 
> Let's figure out which solution would be the best, and I will add comments
> for any of the unclear things.
> 
> > >
> > > -		prescale = rz_mtu3_pwm->prescale[ch];
> > > +		if (rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
> > > +			if (rz_mtu3_pwm->prescale[ch] > prescale)
> > > +				return -EBUSY;
> > > +
> > > +			prescale = rz_mtu3_pwm->prescale[ch];
> > > +		}
> > >  	}
> > >
> > >  	pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
> > > @@ -371,7 +385,7 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  	if (rz_mtu3_pwm->prescale[ch] != prescale && rz_mtu3_pwm->enable_count[ch])
> > >  		rz_mtu3_disable(priv->mtu);
> > >
> > > -	if (priv->map->base_pwm_number == pwm->hwpwm) {
> > > +	if (is_primary) {
> > >  		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> > >  				      RZ_MTU3_TCR_CCLR_TGRA | val);
> > >  		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,
> >
> > All in all I'm unhappy with the hwpwm to channel+IO mapping, this makes
> > this all more complicated. This is something that already bugged me when
> > this driver was created.
> >
> > It's out of scope for this series of fixes, but I wonder if we could
> > create a mapping from hwpwm to an IO-id like this:
> >
> > 	hwpwm | IO-id
> > 	------+------
> > 	   0  |    0	(channel 0, io 0)
> > 	   1  |    1	(channel 0, io 1)
> > 	   2  |    2	(channel 1, io 0)
> > 	   3  |    4	(channel 2, io 0)
> >            4  |    6	(channel 3, io 0)
> > 	   5  |    7	(channel 3, io 1)
> > 	   6  |    8	(channel 4, io 0)
> > 	   7  |    9	(channel 4, io 1)
> > 	   8  |   12	(channel 6, io 0)
> > 	   9  |   13	(channel 6, io 1)
> > 	  10  |   14	(channel 7, io 0)
> > 	  11  |   15	(channel 7, io 1)
> >
> > then the sibling would be just `io_id ^ 1` and the channel could
> > be computed by `io_id >> 1` and the base id for a given io is just
> > `io_id & ~1`.
> >
> > Tracking of an IO being enabled could be done using
> >
> > 	enabled_io & (1 << io_id)
> >
> > I think this would be a simpler scheme that needs less memory and less
> > pointer dereferencing and the check for the sibling being enabled would
> > also be a trivial bit operation.
> >
> 
> I agree that the current setup is not the best. Especially the loop inside
> rz_mtu3_get_channel() is quite sub-optimal, in my opinion.
> 
> I have many more patches already implemented and prepared to be sent for
> MTU3, including conversion to waveform APIs, a lot of cleanups, support
> for more prescale values, bootloader handoff support, etc, but I have
> sent the fixes first as they are higher priority.
> 
> I will try to implement your mapping improvement idea and integrate it in
> one of the later series of patches.
> 
> Please let me know which solution you think is the best for dealing with
> the issue the current patch is trying to solve, and I'll continue from
> there.
> 

Hello Uwe.

What do you think about this setup for mapping from PWM to HW?

#define RZ_MTU3_PWM_IO(ch, secondary) \
	(((ch) << 1) | (secondary))

#define RZ_MTU3_PWM_1_IO(ch) \
	RZ_MTU3_PWM_IO(ch, 0)

#define RZ_MTU3_PWM_2_IO(ch) \
	RZ_MTU3_PWM_IO(ch, 0), \
	RZ_MTU3_PWM_IO(ch, 1)

static const u8 rz_mtu3_pwm_io_map[] = {
	RZ_MTU3_PWM_2_IO(0), /* MTU0 */
	RZ_MTU3_PWM_1_IO(1), /* MTU1 */
	RZ_MTU3_PWM_1_IO(2), /* MTU2 */
	RZ_MTU3_PWM_2_IO(3), /* MTU3 */
	RZ_MTU3_PWM_2_IO(4), /* MTU4 */
	RZ_MTU3_PWM_2_IO(5), /* MTU6 */
	RZ_MTU3_PWM_2_IO(6), /* MTU7 */
};
static_assert(ARRAY_SIZE(rz_mtu3_pwm_io_map) == RZ_MTU3_MAX_PWM_CHANNELS);

static unsigned int rz_mtu3_hwpwm_ch(u32 hwpwm)
{
	return rz_mtu3_pwm_io_map[hwpwm] >> 1;
}

static bool rz_mtu3_hwpwm_is_primary(u32 hwpwm)
{
	return !(rz_mtu3_pwm_io_map[hwpwm] & 1);
}

static struct rz_mtu3_pwm_channel *
rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
{
	unsigned int ch = rz_mtu3_hwpwm_ch(hwpwm);

	return &rz_mtu3_pwm->channel_data[ch];
}

This gets rid of the loop inside rz_mtu3_get_channel() quite nicely.

priv->map->base_pwm_number == hwpwm checks will in turn be reduced to
rz_mtu3_hwpwm_is_primary(hwpwm).

If you decide that we should keep the sibling check inside
rz_mtu3_pwm_config() as-is then we can do the following, without having
to encode the number of channels for each HW channel explicitly.

Please note that hwpwm + 1 is fine in this case as the last hwpwm of
rz_mtu3_pwm_io_map is never primary.

static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
{
	if (!rz_mtu3_hwpwm_is_primary(hwpwm))
		return hwpwm - 1;

	if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
		return -EINVAL;

	return hwpwm + 1;
}

Although, I would much rather have the following logic rather than the
sibling check, which matches one of the alternatives you proposed earlier.

Hopefully, the comment explains the situation well.

static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
			      const struct pwm_state *state)
{
	...

	u32 enable_count;

	...

	/*
	 * Account for the case where one IO is already enabled and this call
	 * enables the second one, to prevent the prescale from being changed.
	 * If this PWM is currently disabled it will be enabled by this call,
	 * so include it in the enable count. If it is already enabled, it has
	 * already been accounted for.
	 */
	enable_count = rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);

	...

	if (enable_count > 1) {
		if (rz_mtu3_pwm->prescale[ch] > prescale)
			return -EBUSY;

		prescale = rz_mtu3_pwm->prescale[ch];
	}

Please let me know what you think so we can proceed with the work
internally.

> > Best regards
> > Uwe

Re: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Geert Uytterhoeven]

Hi Cosmin,

On Mon, 16 Mar 2026 at 16:52, Cosmin-Gabriel Tanislav
<cosmin-gabriel.tanislav.xa@renesas.com> wrote:
> static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)

Unused sibling_hwpwm?

> {
>         if (!rz_mtu3_hwpwm_is_primary(hwpwm))
>                 return hwpwm - 1;
>
>         if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
>                 return -EINVAL;
>
>         return hwpwm + 1;
> }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Cosmin-Gabriel Tanislav]

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, March 16, 2026 8:26 PM
> 
> Hi Cosmin,
> 
> On Mon, 16 Mar 2026 at 16:52, Cosmin-Gabriel Tanislav
> <cosmin-gabriel.tanislav.xa@renesas.com> wrote:
> > static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
> 
> Unused sibling_hwpwm?
> 
> > {
> >         if (!rz_mtu3_hwpwm_is_primary(hwpwm))
> >                 return hwpwm - 1;
> >
> >         if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
> >                 return -EINVAL;
> >
> >         return hwpwm + 1;
> > }
> 
> Gr{oetje,eeting}s,
> 

Hello Geert. Thanks for catching that.

It's funny how even after triple-checking the message I was about to
send, I didn't notice it.

This should have been what I sent.

static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
{
	if (!rz_mtu3_hwpwm_is_primary(hwpwm)) {
		*sibling_hwpwm = hwpwm - 1;
		return 0;
	}

	if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
		return -EINVAL;

	*sibling_hwpwm = hwpwm + 1;

	return 0;
}

>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Geert Uytterhoeven]

Hi Cosmin,

On Mon, 16 Mar 2026 at 20:13, Cosmin-Gabriel Tanislav
<cosmin-gabriel.tanislav.xa@renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Monday, March 16, 2026 8:26 PM
> >
> > Hi Cosmin,
> >
> > On Mon, 16 Mar 2026 at 16:52, Cosmin-Gabriel Tanislav
> > <cosmin-gabriel.tanislav.xa@renesas.com> wrote:
> > > static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
> >
> > Unused sibling_hwpwm?
> >
> > > {
> > >         if (!rz_mtu3_hwpwm_is_primary(hwpwm))
> > >                 return hwpwm - 1;
> > >
> > >         if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
> > >                 return -EINVAL;
> > >
> > >         return hwpwm + 1;
> > > }

> It's funny how even after triple-checking the message I was about to
> send, I didn't notice it.
>
> This should have been what I sent.
>
> static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
> {
>         if (!rz_mtu3_hwpwm_is_primary(hwpwm)) {
>                 *sibling_hwpwm = hwpwm - 1;
>                 return 0;
>         }
>
>         if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
>                 return -EINVAL;
>
>         *sibling_hwpwm = hwpwm + 1;
>
>         return 0;
> }

Thanks, now I can see what you intended ;-)
As the output parameter value is unsigned, and never very large,
returning that value or a negative error code as the return value may
be simpler (i.e. use the original "bad" version, and drop the unused
output parameter)?



Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Uwe Kleine-König]

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

Hello,

On Mon, Mar 16, 2026 at 03:49:35PM +0000, Cosmin-Gabriel Tanislav wrote:
> What do you think about this setup for mapping from PWM to HW?
> 
> #define RZ_MTU3_PWM_IO(ch, secondary) \
> 	(((ch) << 1) | (secondary))
> 
> #define RZ_MTU3_PWM_1_IO(ch) \
> 	RZ_MTU3_PWM_IO(ch, 0)
> 
> #define RZ_MTU3_PWM_2_IO(ch) \
> 	RZ_MTU3_PWM_IO(ch, 0), \
> 	RZ_MTU3_PWM_IO(ch, 1)
> 
> static const u8 rz_mtu3_pwm_io_map[] = {
> 	RZ_MTU3_PWM_2_IO(0), /* MTU0 */
> 	RZ_MTU3_PWM_1_IO(1), /* MTU1 */
> 	RZ_MTU3_PWM_1_IO(2), /* MTU2 */
> 	RZ_MTU3_PWM_2_IO(3), /* MTU3 */
> 	RZ_MTU3_PWM_2_IO(4), /* MTU4 */
> 	RZ_MTU3_PWM_2_IO(5), /* MTU6 */
> 	RZ_MTU3_PWM_2_IO(6), /* MTU7 */
> };
> static_assert(ARRAY_SIZE(rz_mtu3_pwm_io_map) == RZ_MTU3_MAX_PWM_CHANNELS);

I think the RZ_MTU3_PWM_1_IO and RZ_MTU3_PWM_2_IO macros are a bit too
magic and would use

static const u8 rz_mtu3_pwm_io_map[] = {
	RZ_MTU3_PWM_IO(0, 0),
	RZ_MTU3_PWM_IO(0, 1),
	RZ_MTU3_PWM_IO(1, 0),
	RZ_MTU3_PWM_IO(2, 0),
	RZ_MTU3_PWM_IO(3, 0),
	RZ_MTU3_PWM_IO(3, 1),
	RZ_MTU3_PWM_IO(4, 0),
	RZ_MTU3_PWM_IO(4, 1),
	RZ_MTU3_PWM_IO(5, 0),
	RZ_MTU3_PWM_IO(5, 1),
	RZ_MTU3_PWM_IO(6, 0),
	RZ_MTU3_PWM_IO(6, 1),
};

and then maybe just:

#define RZ_MTU3_NUM_PWM_CHANNELS ARRAY_SIZE(rz_mtu3_pwm_io_map)

instead of the static_assert. But I guess this is mostly subjective and
I won't try to convince you of my approach if you prefer your
suggestion.

> static unsigned int rz_mtu3_hwpwm_ch(u32 hwpwm)
> {
> 	return rz_mtu3_pwm_io_map[hwpwm] >> 1;
> }
> 
> static bool rz_mtu3_hwpwm_is_primary(u32 hwpwm)
> {
> 	return !(rz_mtu3_pwm_io_map[hwpwm] & 1);
> }
> 
> static struct rz_mtu3_pwm_channel *
> rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> {
> 	unsigned int ch = rz_mtu3_hwpwm_ch(hwpwm);
> 
> 	return &rz_mtu3_pwm->channel_data[ch];
> }
> 
> This gets rid of the loop inside rz_mtu3_get_channel() quite nicely.
> 
> priv->map->base_pwm_number == hwpwm checks will in turn be reduced to
> rz_mtu3_hwpwm_is_primary(hwpwm).
> 
> If you decide that we should keep the sibling check inside
> rz_mtu3_pwm_config() as-is then we can do the following, without having
> to encode the number of channels for each HW channel explicitly.
> 
> Please note that hwpwm + 1 is fine in this case as the last hwpwm of
> rz_mtu3_pwm_io_map is never primary.

This needs a comment plus (if possible) an assert.

> static int rz_mtu3_sibling_hwpwm(u32 hwpwm, u32 *sibling_hwpwm)
> {
> 	if (!rz_mtu3_hwpwm_is_primary(hwpwm))
> 		return hwpwm - 1;
> 
> 	if (rz_mtu3_hwpwm_is_primary(hwpwm + 1))
> 		return -EINVAL;
> 
> 	return hwpwm + 1;
> }
> 
> Although, I would much rather have the following logic rather than the
> sibling check, which matches one of the alternatives you proposed earlier.
> 
> Hopefully, the comment explains the situation well.

I got it, thanks.

> static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> 			      const struct pwm_state *state)
> {
> 	...
> 
> 	u32 enable_count;
> 
> 	...
> 
> 	/*
> 	 * Account for the case where one IO is already enabled and this call
> 	 * enables the second one, to prevent the prescale from being changed.
> 	 * If this PWM is currently disabled it will be enabled by this call,
> 	 * so include it in the enable count. If it is already enabled, it has
> 	 * already been accounted for.
> 	 */
> 	enable_count = rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);
> 
> 	...
> 
> 	if (enable_count > 1) {
> 		if (rz_mtu3_pwm->prescale[ch] > prescale)
> 			return -EBUSY;
> 
> 		prescale = rz_mtu3_pwm->prescale[ch];
> 	}
> 
> Please let me know what you think so we can proceed with the work
> internally.

I'd prefer the `rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);`
variant. I understand that this is also the variant you prefer, so
that's great, but I wouldn't stop you using the sibling option.

You can gain some extra points for not using pwm->state. This is a relic
from the legacy pwm abstraction and doesn't make much sense with the
waveform callbacks. The alternative would be to check the hardware for
being on or not. But there are many users of this member, that I also
won't yell at you for also using it.

Best regards
Uwe

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

RE: [PATCH 1/5] pwm: rz-mtu3: fix prescale check when enabling 2nd channel

[Cosmin-Gabriel Tanislav]

> From: Uwe Kleine-König <ukleinek@kernel.org>
> Sent: Tuesday, March 17, 2026 11:12 AM
> 
> Hello,
> 
> On Mon, Mar 16, 2026 at 03:49:35PM +0000, Cosmin-Gabriel Tanislav wrote:
> > What do you think about this setup for mapping from PWM to HW?
> >
> > #define RZ_MTU3_PWM_IO(ch, secondary) \
> > 	(((ch) << 1) | (secondary))
> >
> > #define RZ_MTU3_PWM_1_IO(ch) \
> > 	RZ_MTU3_PWM_IO(ch, 0)
> >
> > #define RZ_MTU3_PWM_2_IO(ch) \
> > 	RZ_MTU3_PWM_IO(ch, 0), \
> > 	RZ_MTU3_PWM_IO(ch, 1)
> >
> > static const u8 rz_mtu3_pwm_io_map[] = {
> > 	RZ_MTU3_PWM_2_IO(0), /* MTU0 */
> > 	RZ_MTU3_PWM_1_IO(1), /* MTU1 */
> > 	RZ_MTU3_PWM_1_IO(2), /* MTU2 */
> > 	RZ_MTU3_PWM_2_IO(3), /* MTU3 */
> > 	RZ_MTU3_PWM_2_IO(4), /* MTU4 */
> > 	RZ_MTU3_PWM_2_IO(5), /* MTU6 */
> > 	RZ_MTU3_PWM_2_IO(6), /* MTU7 */
> > };
> > static_assert(ARRAY_SIZE(rz_mtu3_pwm_io_map) == RZ_MTU3_MAX_PWM_CHANNELS);
> 
> I think the RZ_MTU3_PWM_1_IO and RZ_MTU3_PWM_2_IO macros are a bit too
> magic and would use
> 
> static const u8 rz_mtu3_pwm_io_map[] = {
> 	RZ_MTU3_PWM_IO(0, 0),
> 	RZ_MTU3_PWM_IO(0, 1),
> 	RZ_MTU3_PWM_IO(1, 0),
> 	RZ_MTU3_PWM_IO(2, 0),
> 	RZ_MTU3_PWM_IO(3, 0),
> 	RZ_MTU3_PWM_IO(3, 1),
> 	RZ_MTU3_PWM_IO(4, 0),
> 	RZ_MTU3_PWM_IO(4, 1),
> 	RZ_MTU3_PWM_IO(5, 0),
> 	RZ_MTU3_PWM_IO(5, 1),
> 	RZ_MTU3_PWM_IO(6, 0),
> 	RZ_MTU3_PWM_IO(6, 1),
> };
> 

Sure, I'll remove the extra macros.

> and then maybe just:
> 
> #define RZ_MTU3_NUM_PWM_CHANNELS ARRAY_SIZE(rz_mtu3_pwm_io_map)
> 

Good idea, I'll change to this.

> instead of the static_assert. But I guess this is mostly subjective and
> I won't try to convince you of my approach if you prefer your
> suggestion.
> 

...

> 
> > static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > 			      const struct pwm_state *state)
> > {
> > 	...
> >
> > 	u32 enable_count;
> >
> > 	...
> >
> > 	/*
> > 	 * Account for the case where one IO is already enabled and this call
> > 	 * enables the second one, to prevent the prescale from being changed.
> > 	 * If this PWM is currently disabled it will be enabled by this call,
> > 	 * so include it in the enable count. If it is already enabled, it has
> > 	 * already been accounted for.
> > 	 */
> > 	enable_count = rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);
> >
> > 	...
> >
> > 	if (enable_count > 1) {
> > 		if (rz_mtu3_pwm->prescale[ch] > prescale)
> > 			return -EBUSY;
> >
> > 		prescale = rz_mtu3_pwm->prescale[ch];
> > 	}
> >
> > Please let me know what you think so we can proceed with the work
> > internally.
> 
> I'd prefer the `rz_mtu3_pwm->enable_count[ch] + (pwm->state.enabled ? 0 : 1);`
> variant. I understand that this is also the variant you prefer, so
> that's great, but I wouldn't stop you using the sibling option.
> 

I realized the check could be simplified quite a bit while achieving
the same outcome.

	if (rz_mtu3_pwm->enable_count[ch] > pwm->state.enabled) {
		...
	}

2 > 1 -> true, prescale gets checked when updating one of the IOs if
both are enabled

1 > 0 -> true, prescale gets checked when enabling the second IO

1 > 1 -> false, prescale is not checked when updating a single enabled
IO

0 > 0 -> false, prescale is not checked when enabling the first IO

2 > 0 and 0 > 1 -> impossible since enable_count is always in sync
with PWM state

> You can gain some extra points for not using pwm->state. This is a relic
> from the legacy pwm abstraction and doesn't make much sense with the
> waveform callbacks. 

I can switch from enable_count to an enable_mask in a later commit, and
that will allow us to both get rid of PWM state access entirely and also
make the sibling check more obvious, by doing something like:

	if (rz_mtu3_pwm->enable_mask[ch] & ~BIT(rz_mtu3_hwpwm_io(pwm->hwpwm))) {
		...
	}

Which would read like "is any other IO enabled?". If yes, don't touch
prescale.

But for the scope of these fixes we need to keep accessing PWM state as I
would like them to be backported to stable.

enable_mask must remain per-HW channel because it makes the enable /
disable checks simpler.

If this sounds good to you, I will proceed with all of these changes.

Thank you.

[PATCH 2/5] pwm: rz-mtu3: impose period restrictions

[Cosmin Tanislav]

The counter is shared by all IOs of a HW channel, and we cannot clear
it from multiple sources, as the TCR register for each HW channel can
only select one clearing source between TGRA, TGRB, TGRC, and TGRD, or
the counter being cleared in another channel when synchronous clearing is
enabled.

Because of this hardware limitation, both IOs of a HW channel must share
the same period.

To provide some flexibility, allow setting different periods on each PWM
channel, with the following restrictions.

If the requested period is smaller than the already programmed period of
the sibling PWM channel, return -EBUSY.

Otherwise, if the requested period is larger to the already programmed
period of the sibling PWM channel, adjust the requested period to match
the already programmed period, and adjust the duty cycle to not exceed
the already programmed period.

Since only one period is being used, always use TGRA for resetting the
counter, and program TGRA for secondary IOs too.

Cc: stable@vger.kernel.org
Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
---
 drivers/pwm/pwm-rz-mtu3.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
index f6073be1c2f8..7558e28f4786 100644
--- a/drivers/pwm/pwm-rz-mtu3.c
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -18,6 +18,13 @@
  * - MTU{1, 2} channels have a single IO, whereas all other HW channels have
  *   2 IOs.
  * - Each IO is modelled as an independent PWM channel.
+ * - Sibling IOs must use the same period as they share a common counter.
+ *   The counter can be reset on one of the following conditions: TGRA or TGRB
+ *   or TGRC or TGRD compare match, or when the counter is cleared in another
+ *   channel when synchronous clearing is enabled.
+ *   The driver always uses TGRA compare match to reset the counter.
+ *   The driver adjusts the period and duty cycle of the sibling IO when
+ *   appropriate.
  * - rz_mtu3_channel_io_map table is used to map the PWM channel to the
  *   corresponding HW channel as there are difference in number of IOs
  *   between HW channels.
@@ -64,6 +71,7 @@ struct rz_mtu3_pwm_channel {
  * @clk: MTU3 module clock
  * @lock: Lock to prevent concurrent access for usage count
  * @rate: MTU3 clock rate
+ * @period_cycles: MTU3 period cycles
  * @user_count: MTU3 usage count
  * @enable_count: MTU3 enable count
  * @prescale: MTU3 prescale
@@ -74,6 +82,7 @@ struct rz_mtu3_pwm_chip {
 	struct clk *clk;
 	struct mutex lock;
 	unsigned long rate;
+	u64 period_cycles[RZ_MTU3_MAX_HW_CHANNELS];
 	u32 user_count[RZ_MTU3_MAX_HW_CHANNELS];
 	u32 enable_count[RZ_MTU3_MAX_HW_CHANNELS];
 	u8 prescale[RZ_MTU3_MAX_HW_CHANNELS];
@@ -333,7 +342,6 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	bool is_primary;
 	u8 prescale;
 	u16 pv, dc;
-	u8 val;
 	u32 ch;
 
 	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
@@ -342,29 +350,31 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
 					NSEC_PER_SEC);
-	prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles);
 
 	/*
-	 * Prescalar is shared by multiple channels, so prescale can
-	 * NOT be modified when there are multiple channels in use with
-	 * different settings. Modify prescalar if other PWM is off or handle
-	 * it, if current prescale value is less than the one we want to set.
+	 * The counter is shared by all IOs of a HW channel, and we cannot clear
+	 * it from multiple sources, as the TCR register for each HW channel can
+	 * only select one clearing source between TGRA, TGRB, TGRC, and TGRD.
+	 * Enforce that all IOs use the same period cycle.
 	 */
 	if (rz_mtu3_pwm->user_count[ch] > 1) {
 		u32 sibling_hwpwm = rz_mtu3_sibling_hwpwm(pwm->hwpwm, is_primary);
 
 		if (rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, sibling_hwpwm)) {
-			if (rz_mtu3_pwm->prescale[ch] > prescale)
+			if (rz_mtu3_pwm->period_cycles[ch] > period_cycles)
 				return -EBUSY;
 
-			prescale = rz_mtu3_pwm->prescale[ch];
+			period_cycles = rz_mtu3_pwm->period_cycles[ch];
 		}
 	}
 
+	prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm, period_cycles);
 	pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
 
 	duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate,
 				      NSEC_PER_SEC);
+	if (duty_cycles > period_cycles)
+		duty_cycles = period_cycles;
 	dc = rz_mtu3_pwm_calculate_pv_or_dc(duty_cycles, prescale);
 
 	/*
@@ -379,20 +389,19 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			return rc;
 	}
 
-	val = RZ_MTU3_TCR_CKEG_RISING | prescale;
-
 	/* Counter must be stopped while updating TCR register */
 	if (rz_mtu3_pwm->prescale[ch] != prescale && rz_mtu3_pwm->enable_count[ch])
 		rz_mtu3_disable(priv->mtu);
 
+	rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA |
+			      RZ_MTU3_TCR_CKEG_RISING | prescale);
+
 	if (is_primary) {
-		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
-				      RZ_MTU3_TCR_CCLR_TGRA | val);
 		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,
 						RZ_MTU3_TGRB, dc);
 	} else {
-		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
-				      RZ_MTU3_TCR_CCLR_TGRC | val);
+		/* TGRA is used to reset the counter for both IOs. */
+		rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRA, pv);
 		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRC, pv,
 						RZ_MTU3_TGRD, dc);
 	}
@@ -409,6 +418,8 @@ static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			rz_mtu3_enable(priv->mtu);
 	}
 
+	rz_mtu3_pwm->period_cycles[ch] = period_cycles;
+
 	/* If the PWM is not enabled, turn the clock off again to save power. */
 	if (!pwm->state.enabled)
 		pm_runtime_put(pwmchip_parent(chip));
-- 
2.52.0

[PATCH 3/5] pwm: rz-mtu3: correctly enable HW channel 4 and 7

[Cosmin Tanislav]

HW channels 4 and 7 require an additional bit to be set in the TOER{A,B}
registers in order to enable PWM output.

Add the necessary logic to update these bits when enabling or disabling
PWM on these channels.

Cc: stable@vger.kernel.org
Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
---
 drivers/pwm/pwm-rz-mtu3.c   | 40 +++++++++++++++++++++++++++++++++++--
 include/linux/mfd/rz-mtu3.h |  2 ++
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-rz-mtu3.c b/drivers/pwm/pwm-rz-mtu3.c
index 7558e28f4786..ed5fbc4015aa 100644
--- a/drivers/pwm/pwm-rz-mtu3.c
+++ b/drivers/pwm/pwm-rz-mtu3.c
@@ -226,10 +226,37 @@ static void rz_mtu3_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	mutex_unlock(&rz_mtu3_pwm->lock);
 }
 
+static void rz_mtu3_pwm_set_toer_bit(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
+				     struct rz_mtu3_pwm_channel *priv,
+				     bool is_primary, bool set)
+{
+	u8 bitpos;
+	u16 reg;
+
+	/*
+	 * HW channels 4 and 7 require an additional register write to enable
+	 * PWM output.
+	 */
+	if (priv->mtu->channel_number == RZ_MTU3_CHAN_4)
+		reg = RZ_MTU3_TOERA;
+	else if (priv->mtu->channel_number == RZ_MTU3_CHAN_7)
+		reg = RZ_MTU3_TOERB;
+	else
+		return;
+
+	if (is_primary)
+		bitpos = 1;
+	else
+		bitpos = 4;
+
+	rz_mtu3_shared_reg_update_bit(priv->mtu, reg, bitpos, set);
+}
+
 static int rz_mtu3_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
 	struct rz_mtu3_pwm_channel *priv;
+	bool is_primary;
 	u32 ch;
 	u8 val;
 	int rc;
@@ -240,10 +267,15 @@ static int rz_mtu3_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
 	ch = priv - rz_mtu3_pwm->channel_data;
+	is_primary = priv->map->base_pwm_number == pwm->hwpwm;
+
 	val = RZ_MTU3_TIOR_OC_IOB_TOGGLE | RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH;
 
 	rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TMDR1, RZ_MTU3_TMDR1_MD_PWMMODE1);
-	if (priv->map->base_pwm_number == pwm->hwpwm)
+
+	rz_mtu3_pwm_set_toer_bit(rz_mtu3_pwm, priv, is_primary, true);
+
+	if (is_primary)
 		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORH, val);
 	else
 		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORL, val);
@@ -262,17 +294,21 @@ static void rz_mtu3_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
 	struct rz_mtu3_pwm_channel *priv;
+	bool is_primary;
 	u32 ch;
 
 	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
 	ch = priv - rz_mtu3_pwm->channel_data;
+	is_primary = priv->map->base_pwm_number == pwm->hwpwm;
 
 	/* Disable output pins of MTU3 channel */
-	if (priv->map->base_pwm_number == pwm->hwpwm)
+	if (is_primary)
 		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORH, RZ_MTU3_TIOR_OC_RETAIN);
 	else
 		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORL, RZ_MTU3_TIOR_OC_RETAIN);
 
+	rz_mtu3_pwm_set_toer_bit(rz_mtu3_pwm, priv, is_primary, false);
+
 	mutex_lock(&rz_mtu3_pwm->lock);
 	rz_mtu3_pwm->enable_count[ch]--;
 	if (!rz_mtu3_pwm->enable_count[ch])
diff --git a/include/linux/mfd/rz-mtu3.h b/include/linux/mfd/rz-mtu3.h
index 8421d49500bf..37da5f7bb83a 100644
--- a/include/linux/mfd/rz-mtu3.h
+++ b/include/linux/mfd/rz-mtu3.h
@@ -10,6 +10,8 @@
 #include <linux/mutex.h>
 
 /* 8-bit shared register offsets macros */
+#define RZ_MTU3_TOERA	0x00A /* Timer output master enable register A */
+#define RZ_MTU3_TOERB	0x80A /* Timer output master enable register B */
 #define RZ_MTU3_TSTRA	0x080 /* Timer start register A */
 #define RZ_MTU3_TSTRB	0x880 /* Timer start register B */
 
-- 
2.52.0

[PATCH 4/5] counter: rz-mtu3-cnt: prevent counter from being toggled multiple times

[Cosmin Tanislav]

Runtime PM counter is incremented / decremented each time the sysfs
enable file is written to.

If user writes 0 to the sysfs enable file multiple times, runtime PM
usage count underflows, generating the following message.

rz-mtu3-counter rz-mtu3-counter.0: Runtime PM usage count underflow!

At the same time, hardware registers end up being accessed with clocks
off in rz_mtu3_terminate_counter() to disable an already disabled
channel.

If user writes 1 to the sysfs enable file multiple times, runtime PM
usage count will be incremented each time, requiring the same number of
0 writes to get it back to 0.

If user writes 0 to the sysfs enable file while PWM is in progress, PWM
is stopped without counter being the owner of the underlying MTU3
channel.

Check against the cached count_is_enabled value and exit if the user
is trying to set the same enable value.

Cc: stable@vger.kernel.org
Fixes: 0be8907359df ("counter: Add Renesas RZ/G2L MTU3a counter driver")
Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
---
 drivers/counter/rz-mtu3-cnt.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
index e755d54dfece..a4a8ef2d88f0 100644
--- a/drivers/counter/rz-mtu3-cnt.c
+++ b/drivers/counter/rz-mtu3-cnt.c
@@ -499,21 +499,25 @@ static int rz_mtu3_count_enable_write(struct counter_device *counter,
 	struct rz_mtu3_cnt *const priv = counter_priv(counter);
 	int ret = 0;
 
+	mutex_lock(&priv->lock);
+
+	if (priv->count_is_enabled[count->id] == enable)
+		goto exit;
+
 	if (enable) {
-		mutex_lock(&priv->lock);
 		pm_runtime_get_sync(ch->dev);
 		ret = rz_mtu3_initialize_counter(counter, count->id);
 		if (ret == 0)
 			priv->count_is_enabled[count->id] = true;
-		mutex_unlock(&priv->lock);
 	} else {
-		mutex_lock(&priv->lock);
 		rz_mtu3_terminate_counter(counter, count->id);
 		priv->count_is_enabled[count->id] = false;
 		pm_runtime_put(ch->dev);
-		mutex_unlock(&priv->lock);
 	}
 
+exit:
+	mutex_unlock(&priv->lock);
+
 	return ret;
 }
 
-- 
2.52.0

[PATCH 5/5] counter: rz-mtu3-cnt: do not use struct rz_mtu3_channel's dev member

[Cosmin Tanislav]

The counter driver can use HW channels 1 and 2, while the PWM driver can
use HW channels 0, 1, 2, 3, 4, 6, 7.

The dev member is assigned both by the counter driver and the PWM driver
for channels 1 and 2, to their own struct device instance, overwriting
the previous value.

The sub-drivers race to assign their own struct device pointer to the
same struct rz_mtu3_channel's dev member.

The dev member of struct rz_mtu3_channel is used by the counter
sub-driver for runtime PM.

Depending on the probe order of the counter and PWM sub-drivers, the
dev member may point to the wrong struct device instance, causing the
counter sub-driver to do runtime PM actions on the wrong device.

To fix this, use the parent pointer of the counter, which is assigned
during probe to the correct struct device, not the struct device pointer
inside the shared struct rz_mtu3_channel.

Cc: stable@vger.kernel.org
Fixes: 0be8907359df ("counter: Add Renesas RZ/G2L MTU3a counter driver")
Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@renesas.com>
---
 drivers/counter/rz-mtu3-cnt.c | 55 +++++++++++++++++------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c
index a4a8ef2d88f0..7bfb6979193c 100644
--- a/drivers/counter/rz-mtu3-cnt.c
+++ b/drivers/counter/rz-mtu3-cnt.c
@@ -107,9 +107,9 @@ static bool rz_mtu3_is_counter_invalid(struct counter_device *counter, int id)
 	struct rz_mtu3_cnt *const priv = counter_priv(counter);
 	unsigned long tmdr;
 
-	pm_runtime_get_sync(priv->ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
-	pm_runtime_put(priv->ch->dev);
+	pm_runtime_put(counter->parent);
 
 	if (id == RZ_MTU3_32_BIT_CH && test_bit(RZ_MTU3_TMDR3_LWA, &tmdr))
 		return false;
@@ -165,12 +165,12 @@ static int rz_mtu3_count_read(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	if (count->id == RZ_MTU3_32_BIT_CH)
 		*val = rz_mtu3_32bit_ch_read(ch, RZ_MTU3_TCNTLW);
 	else
 		*val = rz_mtu3_16bit_ch_read(ch, RZ_MTU3_TCNT);
-	pm_runtime_put(ch->dev);
+	pm_runtime_put(counter->parent);
 	mutex_unlock(&priv->lock);
 
 	return 0;
@@ -187,26 +187,26 @@ static int rz_mtu3_count_write(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	if (count->id == RZ_MTU3_32_BIT_CH)
 		rz_mtu3_32bit_ch_write(ch, RZ_MTU3_TCNTLW, val);
 	else
 		rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TCNT, val);
-	pm_runtime_put(ch->dev);
+	pm_runtime_put(counter->parent);
 	mutex_unlock(&priv->lock);
 
 	return 0;
 }
 
 static int rz_mtu3_count_function_read_helper(struct rz_mtu3_channel *const ch,
-					      struct rz_mtu3_cnt *const priv,
+					      struct counter_device *const counter,
 					      enum counter_function *function)
 {
 	u8 timer_mode;
 
-	pm_runtime_get_sync(ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	timer_mode = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TMDR1);
-	pm_runtime_put(ch->dev);
+	pm_runtime_put(counter->parent);
 
 	switch (timer_mode & RZ_MTU3_TMDR1_PH_CNT_MODE_MASK) {
 	case RZ_MTU3_TMDR1_PH_CNT_MODE_1:
@@ -240,7 +240,7 @@ static int rz_mtu3_count_function_read(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	ret = rz_mtu3_count_function_read_helper(ch, priv, function);
+	ret = rz_mtu3_count_function_read_helper(ch, counter, function);
 	mutex_unlock(&priv->lock);
 
 	return ret;
@@ -279,9 +279,9 @@ static int rz_mtu3_count_function_write(struct counter_device *counter,
 		return -EINVAL;
 	}
 
-	pm_runtime_get_sync(ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TMDR1, timer_mode);
-	pm_runtime_put(ch->dev);
+	pm_runtime_put(counter->parent);
 	mutex_unlock(&priv->lock);
 
 	return 0;
@@ -300,9 +300,9 @@ static int rz_mtu3_count_direction_read(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	tsr = rz_mtu3_8bit_ch_read(ch, RZ_MTU3_TSR);
-	pm_runtime_put(ch->dev);
+	pm_runtime_put(counter->parent);
 
 	*direction = (tsr & RZ_MTU3_TSR_TCFD) ?
 		COUNTER_COUNT_DIRECTION_FORWARD : COUNTER_COUNT_DIRECTION_BACKWARD;
@@ -377,14 +377,14 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter,
 		return -EINVAL;
 	}
 
-	pm_runtime_get_sync(ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	if (count->id == RZ_MTU3_32_BIT_CH)
 		rz_mtu3_32bit_ch_write(ch, RZ_MTU3_TGRALW, ceiling);
 	else
 		rz_mtu3_16bit_ch_write(ch, RZ_MTU3_TGRA, ceiling);
 
 	rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA);
-	pm_runtime_put(ch->dev);
+	pm_runtime_put(counter->parent);
 	mutex_unlock(&priv->lock);
 
 	return 0;
@@ -495,7 +495,6 @@ static int rz_mtu3_count_enable_read(struct counter_device *counter,
 static int rz_mtu3_count_enable_write(struct counter_device *counter,
 				      struct counter_count *count, u8 enable)
 {
-	struct rz_mtu3_channel *const ch = rz_mtu3_get_ch(counter, count->id);
 	struct rz_mtu3_cnt *const priv = counter_priv(counter);
 	int ret = 0;
 
@@ -505,14 +504,14 @@ static int rz_mtu3_count_enable_write(struct counter_device *counter,
 		goto exit;
 
 	if (enable) {
-		pm_runtime_get_sync(ch->dev);
+		pm_runtime_get_sync(counter->parent);
 		ret = rz_mtu3_initialize_counter(counter, count->id);
 		if (ret == 0)
 			priv->count_is_enabled[count->id] = true;
 	} else {
 		rz_mtu3_terminate_counter(counter, count->id);
 		priv->count_is_enabled[count->id] = false;
-		pm_runtime_put(ch->dev);
+		pm_runtime_put(counter->parent);
 	}
 
 exit:
@@ -544,9 +543,9 @@ static int rz_mtu3_cascade_counts_enable_get(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(priv->ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
-	pm_runtime_put(priv->ch->dev);
+	pm_runtime_put(counter->parent);
 	*cascade_enable = test_bit(RZ_MTU3_TMDR3_LWA, &tmdr);
 	mutex_unlock(&priv->lock);
 
@@ -563,10 +562,10 @@ static int rz_mtu3_cascade_counts_enable_set(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(priv->ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	rz_mtu3_shared_reg_update_bit(priv->ch, RZ_MTU3_TMDR3,
 				      RZ_MTU3_TMDR3_LWA, cascade_enable);
-	pm_runtime_put(priv->ch->dev);
+	pm_runtime_put(counter->parent);
 	mutex_unlock(&priv->lock);
 
 	return 0;
@@ -583,9 +582,9 @@ static int rz_mtu3_ext_input_phase_clock_select_get(struct counter_device *count
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(priv->ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	tmdr = rz_mtu3_shared_reg_read(priv->ch, RZ_MTU3_TMDR3);
-	pm_runtime_put(priv->ch->dev);
+	pm_runtime_put(counter->parent);
 	*ext_input_phase_clock_select = test_bit(RZ_MTU3_TMDR3_PHCKSEL, &tmdr);
 	mutex_unlock(&priv->lock);
 
@@ -602,11 +601,11 @@ static int rz_mtu3_ext_input_phase_clock_select_set(struct counter_device *count
 	if (ret)
 		return ret;
 
-	pm_runtime_get_sync(priv->ch->dev);
+	pm_runtime_get_sync(counter->parent);
 	rz_mtu3_shared_reg_update_bit(priv->ch, RZ_MTU3_TMDR3,
 				      RZ_MTU3_TMDR3_PHCKSEL,
 				      ext_input_phase_clock_select);
-	pm_runtime_put(priv->ch->dev);
+	pm_runtime_put(counter->parent);
 	mutex_unlock(&priv->lock);
 
 	return 0;
@@ -644,7 +643,7 @@ static int rz_mtu3_action_read(struct counter_device *counter,
 	if (ret)
 		return ret;
 
-	ret = rz_mtu3_count_function_read_helper(ch, priv, &function);
+	ret = rz_mtu3_count_function_read_helper(ch, counter, &function);
 	if (ret) {
 		mutex_unlock(&priv->lock);
 		return ret;
-- 
2.52.0

Re: [PATCH 5/5] counter: rz-mtu3-cnt: do not use struct rz_mtu3_channel's dev member

[William Breathitt Gray]

On Fri, Jan 30, 2026 at 02:23:53PM +0200, Cosmin Tanislav wrote:
> The counter driver can use HW channels 1 and 2, while the PWM driver can
> use HW channels 0, 1, 2, 3, 4, 6, 7.
> 
> The dev member is assigned both by the counter driver and the PWM driver
> for channels 1 and 2, to their own struct device instance, overwriting
> the previous value.
> 
> The sub-drivers race to assign their own struct device pointer to the
> same struct rz_mtu3_channel's dev member.
> 
> The dev member of struct rz_mtu3_channel is used by the counter
> sub-driver for runtime PM.
> 
> Depending on the probe order of the counter and PWM sub-drivers, the
> dev member may point to the wrong struct device instance, causing the
> counter sub-driver to do runtime PM actions on the wrong device.
> 
> To fix this, use the parent pointer of the counter, which is assigned
> during probe to the correct struct device, not the struct device pointer
> inside the shared struct rz_mtu3_channel.

It looks like you replace every instance of ch->dev in the file,
except in rz_mtu3_cnt_probe where it is initially set as ch->dev = dev.
Is that line in rz_mtu3_cnt_probe still needed, or can it now be removed
too?

William Breathitt Gray

RE: [PATCH 5/5] counter: rz-mtu3-cnt: do not use struct rz_mtu3_channel's dev member

[Cosmin-Gabriel Tanislav]

> From: William Breathitt Gray <wbg@kernel.org>
> Sent: Sunday, March 22, 2026 8:58 AM
> 
> On Fri, Jan 30, 2026 at 02:23:53PM +0200, Cosmin Tanislav wrote:
> > The counter driver can use HW channels 1 and 2, while the PWM driver can
> > use HW channels 0, 1, 2, 3, 4, 6, 7.
> >
> > The dev member is assigned both by the counter driver and the PWM driver
> > for channels 1 and 2, to their own struct device instance, overwriting
> > the previous value.
> >
> > The sub-drivers race to assign their own struct device pointer to the
> > same struct rz_mtu3_channel's dev member.
> >
> > The dev member of struct rz_mtu3_channel is used by the counter
> > sub-driver for runtime PM.
> >
> > Depending on the probe order of the counter and PWM sub-drivers, the
> > dev member may point to the wrong struct device instance, causing the
> > counter sub-driver to do runtime PM actions on the wrong device.
> >
> > To fix this, use the parent pointer of the counter, which is assigned
> > during probe to the correct struct device, not the struct device pointer
> > inside the shared struct rz_mtu3_channel.
> 
> It looks like you replace every instance of ch->dev in the file,
> except in rz_mtu3_cnt_probe where it is initially set as ch->dev = dev.
> Is that line in rz_mtu3_cnt_probe still needed, or can it now be removed
> too?

The MTU3 MFD driver still depends on struct rz_mtu3_channel::dev being
initialized for retrieving private data from the channels.

I have patches prepared to remove that dependency and removing the dev member
will come afterwards.

Thank you for applying the patches!