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: Thu, 22 Sep 2022 06:15:29 +0000	[thread overview]
Message-ID: <OS0PR01MB59220ECD0B2D42DF5012B6C7864E9@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20220922053605.qivxzwon52orbdgz@pengutronix.de>

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 01:46:54PM +0000, Biju Das wrote:
> > > 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);
> 
> So if the PWM is already running (e.g. from the bootloader) and the
> mode is wrong, this isn't fixed? Similar problems in the if blocks
> below.

This is taken care by the above code. It stops the counter for first enabled channel in Linux
and then changes the mode as per Linux.

<snippet>
	if (rzg2l_gpt->user_count <= 1)
		rzg2l_gpt_disable(rzg2l_gpt);
</snippet>

Cheers,
Biju

> 
> > 	/* 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);
> > 	}
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://www.pengutronix.de/ |