Re: [PATCH v6 2/2] pwm: Add support for RZ/G2L GPT - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Biju Das <biju.das.jz@bp.renesas.com>
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 15:35:42 +0200	[thread overview]
Message-ID: <20220921133542.3glfgeddnlhrebkz@pengutronix.de> (raw)
In-Reply-To: <OS0PR01MB5922289B89061F6B3DF4819F864F9@OS0PR01MB5922.jpnprd01.prod.outlook.com>

[-- Attachment #1: Type: text/plain, Size: 4236 bytes --]

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?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]