Hi Uwe Kleine-König,
Thanks for the feedback.
> On Fri, Jul 21, 2023 at 07:08:39AM +0100, Biju Das wrote:
> > [...]
> > +static int rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + int rc;
> > +
> > + rc = pm_runtime_resume_and_get(chip->dev);
> > + if (rc)
> > + return rc;
> > +
> > + state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
> > + if (state->enabled) {
> > + u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > + u8 prescale;
> > + u64 tmp;
> > + u32 val;
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTCR);
> > + prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTPR);
> > + tmp = NSEC_PER_SEC * (u64)val;
> > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate) << (2 *
> > +prescale);
>
> You're loosing precision here. If for example we have: GTPR = 0xf, TPCS = 5
> and rzg2l_gpt->rate = 26000000, the output wave's period is:
>
> period = 0xf * 1000000000 / 26000000 << 10
>
> The exact value is 590769.2307692308, so the right value to return is
> 590770. However your calculation yields 590848.
>
> The problem is that the rounded value is further processed. Maybe we need a
> function mul_u64_u64_div_u64_roundup(), or do you see a clever alternative?
I think DIV64_U64_ROUND_UP() will do as it is giving right
value 590770 where as DIV64_U64_ROUND_CLOSEST() is giving
590669.
>
> > + val = rzg2l_gpt_read(rzg2l_gpt,
> > + offs + RZG2L_GTCCR(RZG2L_IS_IOB(pwm-
> >hwpwm)));
> > +
> > + tmp = NSEC_PER_SEC * (u64)val;
> > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate) <<
> (2 *
> > +prescale);
> > +
> > + if (state->duty_cycle > state->period)
> > + state->duty_cycle = state->period;
> > + }
> > +
> > + state->polarity = PWM_POLARITY_NORMAL;
> > + pm_runtime_put(chip->dev);
> > +
> > + return 0;
> > +}
> > +
> > +static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8
> > +prescale) {
> > + return min(period_or_duty_cycle >> (2 * prescale), (u64)U32_MAX); }
> > +
>
> Maybe mention in a comment here that rzg2l_gpt_config() is only called
> holding the lock?
Agreed. OK will add the comment
/* Caller holds the lock while calling rzg2l_gpt_config() */
>
> > +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + const struct pwm_state *state) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > + unsigned long pv, dc;
> > + u64 period_cycles;
> > + u64 duty_cycles;
> > + u8 prescale;
> > +
> > + /*
> > + * GPT counter is shared by multiple channels, so prescale and period
> > + * can NOT be modified when there are multiple channels in use with
> > + * different settings.
> > + */
> > + if (state->period != rzg2l_gpt->state_period[ch] && rzg2l_gpt-
> >user_count[ch] > 1)
> > + return -EBUSY;
>
> this is stricter than necessary, but if you don't want to spend additional
> brain cycles (we're at v15 already), that's ok. Can be addressed later if
> the need arises.
Agreed. Will address it later based on the need.
>
> > + period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > + prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_cycles);
> > +
> > + pv = rzg2l_gpt_calculate_pv_or_dc(period_cycles, prescale);
> > +
> > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > + dc = rzg2l_gpt_calculate_pv_or_dc(duty_cycles, prescale);
> > +
> > + /*
> > + * GPT counter is shared by multiple channels, we cache the period
> value
> > + * from the first enabled channel and use the same value for both
> > + * channels.
> > + */
> > + rzg2l_gpt->state_period[ch] = state->period;
> > +
> > + /*
> > + * If the PWM channel is disabled, make sure to turn on the clock
> > + * before writing the register.
> > + */
> > + if (!pwm->state.enabled) {
> > + int rc;
> > +
> > + rc = pm_runtime_resume_and_get(chip->dev);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > + /*
> > + * 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->enable_count[ch] > 1)
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_CST,
> 0);
>
> It's already late here, but I wonder if the condition is wrong here?!
> s/>/<=/ ?
Agreed. Will fix.
>
> > + /* GPT set operating mode (saw-wave up-counting) */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_MD,
> > + RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> > +
> > + /* Set count direction */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> > + /* Select count clock */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_TPCS,
> > + FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > +
> > + /* Set period */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTPR, pv);
> > +
> > + /* Set duty cycle */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTCCR(RZG2L_IS_IOB(pwm-
> >hwpwm)),
> > + dc);
> > +
> > + /* Set initial value for counter */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTCNT, 0);
> > +
> > + /* Set no buffer operation */
> > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTBER, 0);
> > +
> > + /* Restart the counter after updating the registers */
> > + if (rzg2l_gpt->enable_count[ch] > 1)
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR,
> > + RZG2L_GTCR_CST, RZG2L_GTCR_CST);
> > +
> > + /* If the PWM is not enabled, turn the clock off again to save power.
> */
> > + if (!pwm->state.enabled)
> > + pm_runtime_put(chip->dev);
>
> rzg2l_gpt_config() is only called if state->enabled is true, i.e. the
> hardware is about to be enabled. I think it's not sensible in this case to
> call pm_runtime_put().
OK will drop it.
Cheers,
Biju
>
> > + return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
