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@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.
> 

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