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

From: Biju Das <biju.das.jz@bp.renesas.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
Date: Wed, 21 Sep 2022 10:50:48 +0000	[thread overview]
Message-ID: <OS0PR01MB5922289B89061F6B3DF4819F864F9@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OS0PR01MB5922A9B3314F2F2B32F6B0DE864C9@OS0PR01MB5922.jpnprd01.prod.outlook.com>

> -----Original Message-----
> From: Biju Das
> Sent: 20 September 2022 18:01
> To: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Thierry Reding <thierry.reding@gmail.com>; Lee Jones
> <lee.jones@linaro.org>; Philipp Zabel <p.zabel@pengutronix.de>; linux-
> pwm@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Biju Das
> <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> 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/ |