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
