RE: [PATCH v16] pwm: Add Renesas RZ/G2L MTU3a PWM driver

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Uwe,

Thanks for the feedback.

> Subject: Re: [PATCH v16] pwm: Add Renesas RZ/G2L MTU3a PWM driver
> 
> Hello Biju,
> 
> here now comes my promised review. Took a bit longer than anticipated,
> sorry for that.

I know you are busy with "Convert to platform remove callback returning void".

> 
> On Tue, Apr 18, 2023 at 11:20:37AM +0100, Biju Das wrote:
> > +static u8 rz_mtu3_pwm_calculate_prescale(struct rz_mtu3_pwm_chip
> *rz_mtu3,
> > +					 u64 period_cycles)
> > +{
> > +	u32 prescaled_period_cycles;
> > +	u8 prescale;
> > +
> > +	prescaled_period_cycles = period_cycles >> 16;
> > +	if (prescaled_period_cycles >= 16)
> > +		prescale = 3;
> > +	else
> > +		prescale = (fls(prescaled_period_cycles) + 1) / 2;
> > +
> > +	return prescale;
> 
> That value is supposed to be written to RZ_MTU3_TCR_TPCS, right? This is
> a 3bit register field and in .get_state() you handle values up to 7. I
> wonder why the max here is 3 only.

I thought, for the initial basic driver, support bit values {0, 1, 2, 3} as
It is same for all MTU channels and later plan to support the complicated 
external and internal clocks as it different for each channels.

Are you ok with this plan? Please let me know.

Eg:

MTU0:
TCR2[2:0]      	: TCR[2:0]
0              	:{4,5,6,7}	:- External clock: counts on {MTCLKA.. MTCLKD} pin input   
{1,2,3,4,5}    	: x		:- Internal clock: P0φ {2, 8, 32, 256, 1024}
6			: x		:- Setting Prohibited
7			: x		:- External clock: counts on MTIOC1A pin input

MTU1:
TCR2[2:0]      	: TCR[2:0]
0              	: {4, 5}	:- External clock: counts on {MTCLKA.. MTCLKB} pin input
0              	: 6  		:- Internal clock: counts on P0φ/256   
0              	: 7		:- Overflow/underflow of MTU2.TCNT   
{1,2,3,4}      	: x	   	:- Internal clock: P0φ {2, 8, 32, 1024}
{5,6,7}		: x		:- Setting Prohibited

MTU2:
TCR2[2:0]      	: TCR[2:0]
0              	: {4, 5,6}	:- External clock: counts on {MTCLKA.. MTCLKC} pin input   
0              	: 7 		:- Internal clock: P0φ/1024   
{1,2,3,4}      	: x	   	:- Internal clock: P0φ/ {2, 8, 32, 256}
{5,6,7}		: x		:- Setting Prohibited


MTU3, MTU4, MTU6, MTU7, and MTU8
TCR2[2:0]      	: TCR[2:0]
0			: {4, 5}	:- Internal clock: P0φ/ {256, 1024}		
0              	: {6, 7}    :- External clock: counts on {MTCLKA.. MTCLKB} pin input   
{1,2,3}        	: x	   	:- Internal clock: P0φ/ {2, 8, 32}
{4,5,6,7}		: x		:- Setting Prohibited


MTU5:
TCR2[2:0]      	: TCR[1:0]
{1, 2, 3, 4, 5}  	:  x	   	:- Internal clock: P0φ/ {2, 8, 32}
6			:  x		:- Setting Prohibited
7			:  x		:- Internal clock: counts on MTIOC1A pin input

> 
> > +}
> > +
> > [...]
> > +static int rz_mtu3_pwm_get_state(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +				 struct pwm_state *state)
> > +{
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > +
> > +	pm_runtime_get_sync(chip->dev);
> 
> Return value checking?

OK, will switch to error_check version "pm_runtime_resume_and_get()"
as Geert suggested ??

Normally "pm_runtime_get_sync" is non error check version
and user make sure the usage count is balanced.

> 
> > +	state->enabled = rz_mtu3_pwm_is_ch_enabled(rz_mtu3_pwm, pwm-
> >hwpwm);
> > +	if (state->enabled) {
> > +		struct rz_mtu3_pwm_channel *priv;
> > +		u8 prescale, val;
> > +		u16 dc, pv;
> > +		u64 tmp;
> > +
> > +		priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +		if (priv->map->base_pwm_number == pwm->hwpwm)
> > +			rz_mtu3_pwm_read_tgr_registers(priv, RZ_MTU3_TGRA,
> &pv,
> > +						       RZ_MTU3_TGRB, &dc);
> > +		else
> > +			rz_mtu3_pwm_read_tgr_registers(priv, RZ_MTU3_TGRC,
> &pv,
> > +						       RZ_MTU3_TGRD, &dc);
> > +
> > +		val = rz_mtu3_8bit_ch_read(priv->mtu, RZ_MTU3_TCR);
> > +		prescale = FIELD_GET(RZ_MTU3_TCR_TPCS, val);
> > +
> > +		/* With prescale <= 7 and pv <= 0xffff this doesn't
> overflow. */
> > +		tmp = NSEC_PER_SEC * (u64)pv << (2 * prescale);
> > +		state->period = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm->rate);
> > +		tmp = NSEC_PER_SEC * (u64)dc << (2 * prescale);
> > +		state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rz_mtu3_pwm-
> >rate);
> > +	}
> > +
> > +	if (state->duty_cycle > state->period)
> > +		state->duty_cycle = state->period;
> 
> Without having assigned a value to duty_cycle and period this looks a
> bit strange. Maybe move it into the if block above?

Agreed. 

> 
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +	pm_runtime_put(chip->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static u16 rz_mtu3_pwm_calculate_pv_or_dc(u64 period_or_duty_cycle,
> > +u8 prescale) {
> > +	return (period_or_duty_cycle >> (2 * prescale)) <= U16_MAX ?
> > +		period_or_duty_cycle >> (2 * prescale) : U16_MAX;
> 
> This is equivalent to
> 
> 	return min(period_or_duty_cycle >> (2 * prescale), (u64)U16_MAX);
> 
> I like this variant better, but maybe that's subjective?

OK, will change.

> 
> > +}
> > +
> > +static int rz_mtu3_pwm_config(struct pwm_chip *chip, struct
> pwm_device *pwm,
> > +			      const struct pwm_state *state) {
> > +	struct rz_mtu3_pwm_chip *rz_mtu3_pwm = to_rz_mtu3_pwm_chip(chip);
> > +	struct rz_mtu3_pwm_channel *priv;
> > +	u64 period_cycles;
> > +	u64 duty_cycles;
> > +	u8 prescale;
> > +	u16 pv, dc;
> > +	u8 val;
> > +	u32 ch;
> > +
> > +	priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > +	ch = priv - rz_mtu3_pwm->channel_data;
> > +
> > +	period_cycles = mul_u64_u32_div(state->period, rz_mtu3_pwm->rate,
> > +					NSEC_PER_SEC);
> > +	prescale = rz_mtu3_pwm_calculate_prescale(rz_mtu3_pwm,
> > +period_cycles);
> > +
> > +	/*
> > +	 * Prescalar is shared by multiple channels, so prescale can
> > +	 * NOT be modified when there are multiple channels in use with
> > +	 * different settings. Modify prescalar if other PWM is off or
> handle
> > +	 * it, if current prescale value is less than the one we want to
> set.
> > +	 */
> > +	if (rz_mtu3_pwm->enable_count[ch] > 1) {
> > +		if (rz_mtu3_pwm->prescale[ch] > prescale)
> > +			return -EBUSY;
> > +
> > +		prescale = rz_mtu3_pwm->prescale[ch];
> > +	}
> > +
> > +	pv = rz_mtu3_pwm_calculate_pv_or_dc(period_cycles, prescale);
> > +
> > +	duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm-
> >rate,
> > +				      NSEC_PER_SEC);
> > +	dc = rz_mtu3_pwm_calculate_pv_or_dc(duty_cycles, prescale);
> > +
> > +	/*
> > +	 * If the PWM channel is disabled, make sure to turn on the clock
> > +	 * before writing the register.
> > +	 */
> > +	if (!pwm->state.enabled)
> > +		pm_runtime_get_sync(chip->dev);
> 
> Error checking?

OK will switch to error check version " pm_runtime_resume_and_get()"??

> 
> > +
> > +	val = RZ_MTU3_TCR_CKEG_RISING | prescale;
> > +
> > +	/* Counter must be stopped while updating TCR register */
> > +	if (rz_mtu3_pwm->prescale[ch] != prescale && rz_mtu3_pwm-
> >enable_count[ch])
> > +		rz_mtu3_disable(priv->mtu);
> > +
> > +	if (priv->map->base_pwm_number == pwm->hwpwm) {
> > +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> > +				      RZ_MTU3_TCR_CCLR_TGRA | val);
> > +		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,
> > +						RZ_MTU3_TGRB, dc);
> > +	} else {
> > +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> > +				      RZ_MTU3_TCR_CCLR_TGRC | val);
> > +		rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRC, pv,
> > +						RZ_MTU3_TGRD, dc);
> > +	}
> > +
> > +	if (rz_mtu3_pwm->prescale[ch] != prescale) {
> > +		/*
> > +		 * Prescalar is shared by multiple channels, we cache the
> > +		 * prescalar value from first enabled channel and use the
> same
> > +		 * value for both channels.
> > +		 */
> > +		rz_mtu3_pwm->prescale[ch] = prescale;
> > +
> > +		if (rz_mtu3_pwm->enable_count[ch])
> > +			rz_mtu3_enable(priv->mtu);
> > +	}
> > +
> > +	/* If the PWM is not enabled, turn the clock off again to save
> power. */
> > +	if (!pwm->state.enabled)
> > +		pm_runtime_put(chip->dev);
> > +
> > +	return 0;
> > +}
> 

Cheers,
Biju