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

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

[-- 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 --]