Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

Hello,

On Tue, Sep 20, 2022 at 03:31:16PM +0000, Biju Das wrote:
> > On Mon, Sep 05, 2022 at 06:13:28PM +0100, Biju Das wrote:
> > > +	if (period_cycles >= (1024ULL << 32))
> > > +		pv = U32_MAX;
> > > +	else
> > > +		pv = period_cycles >> (2 * prescale);
> > 
> > You're assuming that pv <= U32_MAX after this block, right? Then maybe
> Yes, That is correct.
> 
> > 
> > 	if (period_cycles >> (2 * prescale) <= U32_MAX)
> > 
> > is the more intuitive check?
> 
> Ok will add like below, so we support up to (U32_MAX * 1024);
> Is it ok for you?
> 
>   if (!(period_cycles >> (2 * prescale) <= U32_MAX))
> +               return -EINVAL;
> +
> +       pv = period_cycles >> (2 * prescale);

Not -EINVAL, using pv = U32_MAX is correct.

> Same case for duty cycle.
> > 
> > > +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> > > +NSEC_PER_SEC);
> > > +
> > > +	if (duty_cycles >= (1024ULL << 32))
> > > +		dc = U32_MAX;
> > > +	else
> > > +		dc = duty_cycles >> (2 * prescale);
> > > +
> > > +	/* Counter must be stopped before modifying Mode and Prescaler */
> > > +	if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> > > +		rzg2l_gpt_disable(rzg2l_gpt);
> > 
> > For v5 I asked if this affects other channels, you said yes and in the
> > follow up I failed to reply how to improve this.
> > 
> > I wonder how this affects other channels. Does it restart a period
> > afterwards, or is the effect only that the currently running period is
> > a bit stretched? 
> 
> If we stops the counter, it resets to starting count position.

So if I update pwm#1, pwm#0 doesn't only freeze for a moment, but
starts a new period. Hui.

> >At least point that this stops the global counter and
> > so affects the other PWMs provided by this chip.
> 
> We should not allow Counter to stop if it is running. 
> We should allow changing mode and prescalar only for the first 
> enabled channel in Linux.
> 
> Also as per the HW manual, we should not change RZG2L_GTCNT, RZG2L_GTBER while
> Counter is running.
> 
> Will add bool is_counter_running to take care of this conditions.
> 
> Is it ok with you?

I'm torn here. Resetting the period for the other counter is quite
disturbing. If you cannot prevent that, please document that in the
Limitations section above.

> > > +	pm_runtime_get_sync(chip->dev);
> > > +	val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR);
> > > +	state->enabled = val & RZG2L_GTCR_CST;
> > > +	if (state->enabled) {
> > > +		prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > > +
> > > +		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR);
> > > +		tmp = NSEC_PER_SEC * val << (2 * prescale);
> > 
> > This can overflow.
> 
> OK will use inverse calculation to avoid overflow.
> mul_u64_u32_div(val << (2 * prescale), NSEC_PER_SEC, rzg2l_gpt->rate);
> 
> Is it ok?

It uses the wrong rounding direction :-\ Using

	tmp = NSEC_PER_SEC * (u64)val << (2 * prescale);

should be enough to fix the problem I pointed out.

> > > +		state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > > +
> > > +		val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));
> > > +		tmp = NSEC_PER_SEC * val << (2 * prescale);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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