Hello Biju,
sorry it took so long until you got 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?
> + 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?
> +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.
> + 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/>/<=/ ?
> + /* 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().
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
