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

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> 
> Hello,
> 
> On Wed, Sep 21, 2022 at 10:50:48AM +0000, Biju Das wrote:
> > > -----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.
> 
> Actually it's worse:
> 
> - When both channels are used, setting the duty-cycle on one aborts
> the
>   currently running period on the other and starts it anew.
> 
> (Did I get this correctly?)

I think, I have fixed that issue with the below logic
Which allows to update duty cycle on the fly.

Now the only limitation is w.r.to disabling channels
as we need to disable together as stopping the counter
affects both.

      /*
	 * Counter must be stopped before modifying mode, prescaler, timer
	 * counter and buffer enable registers. These registers are shared
	 * between both channels. So allow updating these registers only for the
	 * first enabled channel.
	 */
	if (rzg2l_gpt->user_count <= 1)
		rzg2l_gpt_disable(rzg2l_gpt);

	is_counter_running = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST;
	if (!is_counter_running)
		/* GPT set operating mode (saw-wave up-counting) */
		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_MD,
				 RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);

	/* Set count direction */
	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);

	if (!is_counter_running)
		/* Select count clock */
		rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_TPCS,
				 FIELD_PREP(RZG2L_GTCR_TPCS, prescale));

	/* Set period */
	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR, pv);

	/* Set duty cycle */
	rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm), dc);

	if (!is_counter_running) {
		/* Set initial value for counter */
		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT, 0);

		/* Set no buffer operation */
		rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER, 0);
	}

Cheers,
Biju