RE: [PATCH v15 6/6] 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 v15 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
> 
> Hello,
> 
> On Fri, Apr 14, 2023 at 09:53:09AM +0000, Biju Das wrote:
> > > On Thu, Mar 30, 2023 at 12:16:32PM +0100, Biju Das wrote:
> > > > +	val = RZ_MTU3_TCR_CKEG_RISING | prescale;
> > > > +	if (priv->map->channel == pwm->hwpwm) {
> > > > +		rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> > > > +				      RZ_MTU3_TCR_CCLR_TGRA | val);
> > >
> > > If the sibling PWM on the same channel is on, you're overwriting its
> > > prescale value here, are you not?
> >
> > Yes, you are correct. Will cache prescale and add the below code in
> > rz_mtu3_pwm_config(). Is it ok?
> >
> > +        * Prescalar is shared by multiple channels, so prescale can
> > +        * NOT be modified when there are multiple channels in use with
> > +        * different settings.
> > +        */
> > +       if (prescale != rz_mtu3_pwm->prescale[ch] && rz_mtu3_pwm-
> >user_count[ch] > 1)
> > +               return -EBUSY;
> 
> If the other PWM is off, you can (and should) change the prescale value.
> Also if the current prescale value is less than the one you want to set, you
> can handle that.
> 

You mean like below??

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 current
	 * prescale value is less than the one we want to set.
	 */
	if (rz_mtu3_pwm->enable_count[ch] > 1 &&
	    rz_mtu3_pwm->prescale[ch] > prescale)
		return -EBUSY;

	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);

	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);

		/* Update pv and dc of other PWM based on new prescalar */
		if (rz_mtu3_pwm->prescale[ch] != prescale &&
		    rz_mtu3_pwm->enable_count[ch] > 1) {
			rz_mtu3_pwm_read_tgr_registers(priv, RZ_MTU3_TGRC, &pv,
						       RZ_MTU3_TGRD, &dc);
			rz_mtu3_pwm_calculate_new_pv_dc(rz_mtu3_pwm->rate, &pv,
							&dc, prescale);
			rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRC,
							pv, RZ_MTU3_TGRD, 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);

		/* Update pv and dc of other PWM based on new prescalar */
		if (rz_mtu3_pwm->prescale[ch] != prescale &&
		    rz_mtu3_pwm->enable_count[ch] > 1) {
			rz_mtu3_pwm_read_tgr_registers(priv, RZ_MTU3_TGRA, &pv,
						       RZ_MTU3_TGRB, &dc);
			rz_mtu3_pwm_calculate_new_pv_dc(rz_mtu3_pwm->rate, &pv,
							&dc, prescale);
			rz_mtu3_pwm_write_tgr_registers(priv, RZ_MTU3_TGRA, pv,
							RZ_MTU3_TGRB, dc);
		}
	}

	if (rz_mtu3_pwm->prescale[ch] != prescale) {
		/*
		 * Prescalar is shared by multiple channels, we cache the
		 * prescalar value 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;
}

Helper functions:
static void rz_mtu3_pwm_read_tgr_registers(struct rz_mtu3_pwm_channel *priv,
					   u16 reg_pv_offset, u16 *pv_val,
					   u16 reg_dc_offset, u16 *dc_val)
{
	*pv_val = rz_mtu3_16bit_ch_read(priv->mtu, reg_pv_offset);
	*dc_val = rz_mtu3_16bit_ch_read(priv->mtu, reg_dc_offset);
}

static void rz_mtu3_pwm_write_tgr_registers(struct rz_mtu3_pwm_channel *priv,
					    u16 reg_pv_offset, u16 pv_val,
					    u16 reg_dc_offset, u16 dc_val)
{
	rz_mtu3_16bit_ch_write(priv->mtu, reg_pv_offset, pv_val);
	rz_mtu3_16bit_ch_write(priv->mtu, reg_dc_offset, dc_val);
}

static u64 rz_mtu3_pwm_get_period_or_duty_cycle(unsigned long pwm_rate,
						u16 val, u8 prescale)
{
	u64 tmp;

	/* With prescale <= 7 and pv <= 0xffff this doesn't overflow. */
	tmp = NSEC_PER_SEC * (u64)val << (2 * prescale);

	return DIV_ROUND_UP_ULL(tmp, pwm_rate);
}

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;
}

static void rz_mtu3_pwm_calculate_new_pv_dc(unsigned long pwm_rate,
					    u16 *pv, u16 *dc, u8 prescale)
{
	u64 tmp;

	tmp = rz_mtu3_pwm_get_period_or_duty_cycle(pwm_rate, *pv, prescale);
	tmp = mul_u64_u32_div(tmp, pwm_rate, NSEC_PER_SEC);
	*pv = rz_mtu3_pwm_calculate_pv_or_dc(tmp, prescale);

	tmp = rz_mtu3_pwm_get_period_or_duty_cycle(pwm_rate, *dc, prescale);
	tmp = mul_u64_u32_div(tmp, pwm_rate, NSEC_PER_SEC);
	*dc = rz_mtu3_pwm_calculate_pv_or_dc(tmp, prescale);
}

And in apply, add lock around config()

	mutex_lock(&rz_mtu3_pwm->lock);
	ret = rz_mtu3_pwm_config(chip, pwm, state);
	mutex_unlock(&rz_mtu3_pwm->lock);

Cheers,
Biju