> -----Original Message-----
> From: Biju Das
> Sent: 20 September 2022 18:01
> To: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>; Lee Jones
> <lee.jones@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; linux-
> pwm@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Biju Das
> <biju.das@xxxxxxxxxxxxxx>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
>
> 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.
>
OK, I will document this in limitation section.
* - While using dual channels, both the channels should be enabled and
* disabled at the same time as it uses shared register for controlling
* counter start/stop.
Cheers,
Biju
> >
> > > > > + 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/ |
