Hi Uwe,
> Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
>
> 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.
OK.
>
> > 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.
I think, we should be able to prevent resetting the counter value
By not stopping it for the second channel. For second channel
We just update duty cycle and GTIO pins.
>
> > > > + 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.
OK, Thanks for the pointer.
Cheers,
Biju
>
> > > > + 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/ |
