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:
signature.asc
Description: PGP signature
