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@xxxxxxxxxxxxxx> > > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>; Lee Jones > > <lee.jones@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; linux- > > pwm@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; > > Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Biju Das > > <biju.das@xxxxxxxxxxxxxx>; Prabhakar Mahadev Lad <prabhakar.mahadev- > > lad.rj@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx > > 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:
signature.asc
Description: PGP signature
