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

[Cosmin-Gabriel Tanislav]

> From: Uwe Kleine-König <ukleinek@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx
> > Fixes: 254d3a727421 ("pwm: Add Renesas RZ/G2L MTU3a PWM driver")
> > Signed-off-by: Cosmin Tanislav <cosmin-gabriel.tanislav.xa@xxxxxxxxxxx>
> > ---
> >  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