Hi Uwe,
Thanks for the feedback.
> Subject: Re: [PATCH 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> On Wed, Jun 01, 2022 at 07:25:28PM +0000, Biju Das wrote:
> > > On Tue, May 10, 2022 at 03:42:59PM +0100, Biju Das wrote:
> > > > + clk_rate = clk_get_rate(pc->clk);
> > > > + c = clk_rate * period_ns;
> > >
> > > This might overflow (once you keep period_ns as u64).
> > OK, the logic is changed like below to avoid overflow.
> >
> > freq = div_u64(clk_get_rate(pc->clk), 1000000);
> > period_cycles = div_u64(freq * period_ns, 1000);
>
> This might still overflow for big period_ns and freq > 1.
>
> Better use mul_u64_u64_div_u64 and limit clkrate to prevent an overflow.
> This yields a higher precision without overflow. Something like:
>
> rate = clk_get_rate(pc->clk);
>
> /*
> * Refuse clk rates > 1 GHz to prevent overflowing the following
> * calculation.
> */
> if (rate > NSEC_PER_SEC)
> return -EINVAL;
>
> period_cycles = mul_u64_u64_div_u64(rate, period_ns, NSEC_PER_SEC);
OK. Will use this logic.
>
> > > > + period_cycles = div_u64(c, NANO);
> > >
> > > Please use NSEC_PER_SEC here.
> > >
> > > > +
> > > > + if (period_cycles < 1)
> > > > + period_cycles = 1;
> > > > +
> > > > + prescale = -1;
> > > > + /* prescale 1, 4, 16, 64, 256 and 1024 */
> > > > + for (i = 0, prod = 1; i < 6; i++) {
> > > > + if ((period_cycles / GTPR_MAX_VALUE * prod) == 0) {
> > > > + prescale = i;
> > > > + break;
> > > > + }
> > > > +
> > > > + prod *= 4;
> > > > + }
> > >
> > > This would be better understandable if you used:
> > >
> > > for (i = 0; i < 6; i++) {
> > > prod = 1 << (2 * i);
> > > ...
> > >
> > > }
> > >
> > > Have you tested this? The division by GTPR_MAX_VALUE (= 0xFFFFFFFF)
> > > looks suspicious. Unless I'm missing something
> > >
> > > if ((period_cycles / GTPR_MAX_VALUE * prod) == 0)
> > >
> > > is equivalent to:
> > >
> > > if (period_cycles < GTPR_MAX_VALUE)
> > >
> > > . Is this really what you want here?
> >
> > On the next version, I have changed the logic to check this condition in
> caller function.
> >
> > if (period_cycles > 1UL * GTPR_MAX_VALUE * GPT_MAX_PRESCALE_VAL) {
> > dev_warn(chip->dev, "ch=%d period exceed limit\n", pwm-
> >hwpwm);
> > return -EINVAL;
> > }
>
> Please always round down period, that is, if the request it's too big to
> implement, do the biggest period that is possible.
>
OK, will use round down.
> With GTPR_MAX_VALUE = 0xffffffff and unsigned long being 32 bit on some
> arch, this isn't a sensible test.
OK.
>
> > > > + /* Set counting mode */
> > > > + rzg2l_gpt_write(pc, GTUDDTYC, UP_COUNTING);
> > > > + /* Set period */
> > > > + rzg2l_gpt_write(pc, GTPR, pv);
> > > > +
> > > > + /* Enable pin output */
> > > > + rzg2l_gpt_modify(pc, GTIOR, pwm->ph->mask, pwm->ph->value);
> > > > +
> > > > + /* Set duty cycle */
> > > > + rzg2l_gpt_write(pc, pwm->ph->duty_reg_offset, dc);
> > > > +
> > > > + /* Set initial value for counter */
> > > > + rzg2l_gpt_write(pc, GTCNT, 0);
> > > > + /* Set no buffer operation */
> > > > + rzg2l_gpt_write(pc, GTBER, 0);
> > >
> > > How does the output behave on reprogramming? Does it complete the
> > > currently programmed period? Please document this behaviour as e.g.
> > > drivers/pwm/pwm-sl28cpld.c does.
> >
> > Mode and Prescalar must be set, only when the GTCNT is stopped.
> > This condition will document.
>
> Please document this at the top of the driver. Take e.g. pwm-sifive.c as a
> template. (And please stick to the format used there for easy greppability,
> i.e. use "Limitations:" and a list of items with no empty line between
> them.)
OK. Agreed.
>
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *pc) {
> > > > + /* Start count */
> > > > + rzg2l_gpt_modify(pc, GTCR, GTCR_CST, GTCR_CST);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *pc) {
> > > > + /* Stop count */
> > > > + rzg2l_gpt_modify(pc, GTCR, GTCR_CST, 0);
> > >
> > > Same question here: How does the hardware behave? Does it complete
> > > the currently running period? How does the output behave? (Typical
> > > candidates
> > > are: freeze at the level where it is currently, constant 0,
> > > high-z.)
> >
> > It is set to output low during stop.
>
> OK, that should go to the Limitations section, too (even though it's not a
> limitation).
OK. Agreed.
>
> > > > +}
> > > > +
> > > > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm,
> > > > + const struct pwm_state *state) {
> > > > + struct rzg2l_gpt_chip *pc = to_rzg2l_gpt_chip(chip);
> > > > + int ret;
> > >
> > > As you don't support different polarities, there is the following
> > > missing:
> >
> > There is a plan to add polarity in later version.
> >
> > >
> > > if (state->polarity != PWM_POLARITY_NORMAL)
> > > return -EINVAL;
> >
> > Agree, will add this check in initial version.
>
> Does it output low or inactive level on disable if inversed polarity is
> configured?
As output low on counter stop is controlled by GTIOR register(currently it is configured output low for Normal polarity on disable). We could make it to either output low or output high on disable for inversed polarity. I am planning to configure it as output low for inversed polarity, when we add the support for same.
Setting GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH--> gives normal polarity
Setting GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH--> gives inverse polarity which is 180 degree out of phase.
Cheers,
Biju
