Re: [PATCH v9 2/7] pwm: rzg2l-gpt: Convert to waveform callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pwm: rzg2l-gpt: sub-tick period rounds to 0 in rzg2l_gpt_round_waveform_tohw()
- [Medium] pwm: rzg2l-gpt: zeroed duty in rzg2l_gpt_round_waveform_tohw() on small period
- [Medium] pwm: rzg2l-gpt: sibling state mismatch in rzg2l_gpt_round_waveform_tohw()
--

commit 584269914af54fde24d28d7d11319bda863cde58
Author: Biju Das <biju.das.jz@bp.renesas.com>

pwm: rzg2l-gpt: Convert to waveform callbacks

This commit migrates the rzg2l-gpt PWM driver from the legacy .get_state and
.apply callbacks to the waveform callback interface using struct
rzg2l_gpt_waveform.

> diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> index 108fa99f832d9..e9ba0f3bdeb1d 100644
> --- a/drivers/pwm/pwm-rzg2l-gpt.c
> +++ b/drivers/pwm/pwm-rzg2l-gpt.c

[ ... ]

> @@ -219,159 +231,214 @@ static u64 rzg2l_gpt_calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
>  static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8 prescale)
>  {
>  	return min_t(u64, DIV_ROUND_DOWN_ULL(period_or_duty_cycle, 1 << (2 * prescale)),
>  		     U32_MAX);
>  }
>  
> -/* Caller holds the lock while calling rzg2l_gpt_config() */
> -static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			    const struct pwm_state *state)
> +static int rzg2l_gpt_round_waveform_tohw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf,
> +					 void *_wfhw)
>  {
>  	struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> -	u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> +	struct rzg2l_gpt_waveform *wfhw = _wfhw;
> +	bool is_small_second_period = false;
>  	u8 ch = RZG2L_GET_CH(pwm->hwpwm);
>  	u64 period_ticks, duty_ticks;
> -	unsigned long pv, dc;
> -	u8 prescale;
> +
> +	if (wf->period_length_ns == 0) {
> +		*wfhw = (struct rzg2l_gpt_waveform){
> +			.gtpr = 0,
> +			.gtccr = 0,
> +			.prescale = 0,
> +		};
> +
> +		return 0;
> +	}
>  
>  	/* Limit period/duty cycle to max value supported by the HW */
> -	period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, NSEC_PER_MSEC);
> +	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns,
> +					   rzg2l_gpt->rate_khz, NSEC_PER_MSEC);
>  	if (period_ticks > RZG2L_MAX_TICKS)
>  		period_ticks = RZG2L_MAX_TICKS;

[Severity: High]
What happens in rzg2l_gpt_round_waveform_tohw() in drivers/pwm/pwm-rzg2l-gpt.c
when a caller requests a non-zero wf->period_length_ns that is shorter than
one timer clock cycle (0 < wf->period_length_ns < NSEC_PER_MSEC /
rzg2l_gpt->rate_khz, for example 1..9 ns at a 100 MHz GPT clock)?

In that case, mul_u64_u64_div_u64() truncates period_ticks to 0. When the
sibling subchannel is not running, rzg2l_gpt_round_waveform_tohw() computes
wfhw->prescale = 0, wfhw->gtpr = 0, and wfhw->gtccr = 0, and returns 0.

Because wfhw->gtpr == 0 is also used as the sentinel for a disabled waveform
in rzg2l_gpt_write_waveform():

	/* Disable request: shared registers are never touched. */
	if (!wfhw->gtpr) {
		if (self_enabled)
			rzg2l_gpt_disable(rzg2l_gpt, pwm);

		return 0;
	}

and rzg2l_gpt_round_waveform_fromhw() converts wfhw->gtpr == 0 into
wf->period_length_ns = 0, doesn't a sub-tick period request end up disabling
the PWM channel instead of rounding period_ticks up to 1 and returning 1?

For example:
- pwm_gpio_set() passes wf = { .period_length_ns = 1 } to
  pwm_round_waveform_might_sleep() to query the smallest supported hardware
  period, which rounds to period_length_ns = 0 and causes pwm_gpio_set(..., 1)
  to set duty_length_ns = 0 and disable the output instead of driving the pin
  high.
- __pwm_apply() and __pwm_set_waveform() with a non-zero sub-tick period
  disable the hardware channel via rzg2l_gpt_write_waveform() while recording
  pwm->state.enabled = true.

> +
> +	guard(mutex)(&rzg2l_gpt->lock);
>  	/*
>  	 * GPT counter is shared by the two IOs of a single channel, so
>  	 * prescale and period can NOT be modified when there are multiple IOs
>  	 * in use with different settings.
>  	 */
> -	if (rzg2l_gpt->channel_request_count[ch] > 1) {
> -		u8 sibling_ch = rzg2l_gpt_sibling(pwm->hwpwm);
> -
> -		if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, sibling_ch)) {
> -			if (period_ticks < rzg2l_gpt->period_ticks[ch])
> -				return -EBUSY;
> -
> -			period_ticks = rzg2l_gpt->period_ticks[ch];
> -		}
> +	if (rzg2l_gpt->channel_request_count[ch] > 1 &&
> +	    rzg2l_gpt_is_ch_enabled(rzg2l_gpt, rzg2l_gpt_sibling(pwm->hwpwm), NULL)) {
> +		if (period_ticks < rzg2l_gpt->period_ticks[ch])
> +			is_small_second_period = true;
> +
> +		period_ticks = rzg2l_gpt->period_ticks[ch];
> +		wfhw->prescale = rzg2l_gpt->prescale[ch];
> +	} else {
> +		wfhw->prescale = rzg2l_gpt_calculate_prescale(period_ticks);
>  	}

[Severity: Medium]
Can rzg2l_gpt_round_waveform_tohw() and rzg2l_gpt_write_waveform() in
drivers/pwm/pwm-rzg2l-gpt.c disagree on whether the sibling subchannel is
active and what its period and prescaler are?

In rzg2l_gpt_round_waveform_tohw(), the sibling's period and prescaler are
only used if rzg2l_gpt->channel_request_count[ch] > 1, and they are read from
the software cache rzg2l_gpt->period_ticks[ch] and rzg2l_gpt->prescale[ch].

In contrast, rzg2l_gpt_write_waveform() checks whether the sibling is enabled
in hardware without checking channel_request_count[ch], and validates against
the hardware registers RZG2L_GTCR(ch) and RZG2L_GTPR(ch):

	sibling_enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, sibling_ch, &gtcr);
	...
	if (sibling_enabled) {
		if (FIELD_GET(RZG2L_GTCR_TPCS, gtcr) != wfhw->prescale ||
		    rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch)) != wfhw->gtpr)
			return -EBUSY;

If a subchannel is enabled and then freed via rzg2l_gpt_free() (which
decrements channel_request_count[ch] while leaving the hardware channel
running), or if a subchannel was left enabled by the bootloader:
- When only the sibling subchannel is requested (channel_request_count[ch] ==
  1), rzg2l_gpt_round_waveform_tohw() skips the sibling check and computes a
  new period/prescale, which rzg2l_gpt_write_waveform() then rejects with
  -EBUSY.
- If a subchannel was enabled by the bootloader before probe and both
  subchannels are requested (channel_request_count[ch] == 2) before
  rzg2l_gpt_write_waveform() has ever populated period_ticks[ch] and
  prescale[ch], rzg2l_gpt_round_waveform_tohw() reads 0 from
  rzg2l_gpt->period_ticks[ch] and sets wfhw->gtpr = 0, causing
  rzg2l_gpt_write_waveform() to treat the request as a disable.

Should rzg2l_gpt_round_waveform_tohw() check rzg2l_gpt_is_ch_enabled() and
read the running period and prescaler from the hardware registers in the same
way as rzg2l_gpt_write_waveform()?

>  
> -	prescale = rzg2l_gpt_calculate_prescale(period_ticks);
> -	pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> +	wfhw->gtpr = rzg2l_gpt_calculate_pv_or_dc(period_ticks, wfhw->prescale);
> +	if (is_small_second_period) {
> +		wfhw->gtccr = 0;
> +		return 1;
> +	}
>  
> -	duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, NSEC_PER_MSEC);
> +	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, rzg2l_gpt->rate_khz, NSEC_PER_MSEC);
>  	if (duty_ticks > period_ticks)
>  		duty_ticks = period_ticks;
> -	dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> +	wfhw->gtccr = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, wfhw->prescale);
>  
> -	return 0;
> +	return 0;
> +}

[Severity: Medium]
When is_small_second_period is true in rzg2l_gpt_round_waveform_tohw() in
drivers/pwm/pwm-rzg2l-gpt.c (because the sibling subchannel is enabled and the
requested wf->period_length_ns is smaller than the running period), why does
this return early with wfhw->gtccr = 0 instead of rounding wf->duty_length_ns
against the sibling's period and returning 1 at the end of the function?

When a caller uses pwm_round_waveform_might_sleep() or
pwm_set_waveform_might_sleep() with exact = false and wf->duty_length_ns > 0,
the period is rounded up to the running sibling's period, so shouldn't
wfhw->gtccr still be computed from wf->duty_length_ns rather than forcing a 0%
duty cycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-gpt_rzg3e-v9-0-f3a5520388d3@bp.renesas.com?part=2