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

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

Hi Uwe,

> Subject: RE: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT
> 
> 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.

What is your thought on caching the registers that needs counter to be stopped
for updating values. Basically, we don't stop the counter if the values are same?

This allows updating period/duty cycle on the fly without stopping the counter
even for the single channel use case.

Please share your thoughts.

Cheers,
Biju


> 
> 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/ |