RE: [PATCH v15 3/4] pwm: Add support for RZ/G2L GPT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/ |




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux