Hi Uwe,
Thanks for the feedback.
> Subject: Re: [PATCH v15 6/6] pwm: Add Renesas RZ/G2L MTU3a PWM driver
>
> Hello,
>
> On Thu, Mar 30, 2023 at 12:16:32PM +0100, Biju Das wrote:
> > +static struct rz_mtu3_pwm_channel *
> > +rz_mtu3_get_channel(struct rz_mtu3_pwm_chip *rz_mtu3_pwm, u32 hwpwm)
> > +{
> > + struct rz_mtu3_pwm_channel *priv = rz_mtu3_pwm->channel_data;
> > + unsigned int ch;
> > +
> > + for (ch = 0; ch < RZ_MTU3_MAX_HW_CHANNELS; ch++, priv++) {
> > + if (priv->map->channel + priv->map->num_channel_ios > hwpwm)
>
> s/ / /
OK, will fix this.
>
> > + break;
> > + }
> > +
> > + return priv;
> > +}
> > +
> > +static bool rz_mtu3_pwm_is_ch_enabled(struct rz_mtu3_pwm_chip
> *rz_mtu3_pwm,
> > + u32 hwpwm)
> > +{
> > + struct rz_mtu3_pwm_channel *priv;
> > + bool is_channel_en;
> > + u8 val;
> > +
> > + priv = rz_mtu3_get_channel(rz_mtu3_pwm, hwpwm);
> > + is_channel_en = rz_mtu3_is_enabled(priv->mtu);
> > + if (!is_channel_en)
> > + return false;
> > +
> > + if (priv->map->channel == hwpwm)
> > + val = rz_mtu3_8bit_ch_read(priv->mtu, RZ_MTU3_TIORH);
> > + else
> > + val = rz_mtu3_8bit_ch_read(priv->mtu, RZ_MTU3_TIORL);
> > +
> > + return (is_channel_en && (val & RZ_MTU3_TIOR_IOA));
>
> Here you already know that is_channel_en == true, so it can be dropped here.
Agreed.
>
> > +}
> > +
> > [...]
> > +static int rz_mtu3_pwm_enable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> > + struct pwm_device *pwm)
> > +{
> > + struct rz_mtu3_pwm_channel *priv;
> > + u32 ch;
> > + u8 val;
> > + int rc;
> > +
> > + rc = pm_runtime_resume_and_get(rz_mtu3_pwm->chip.dev);
> > + if (rc)
> > + return rc;
> > +
> > + priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > + ch = priv - rz_mtu3_pwm->channel_data;
> > + val = RZ_MTU3_TIOR_OC_IOB_TOGGLE | RZ_MTU3_TIOR_OC_IOA_H_COMP_MATCH;
> > +
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TMDR1,
> RZ_MTU3_TMDR1_MD_PWMMODE1);
> > + if (priv->map->channel == pwm->hwpwm)
>
> This condition identifies the first PWM of a channel. I was surprised here
> that the channel numbering has a hole after each channel that manages two
> IOs. OTOH, in the comment at the top of the driver there is:
>
> (The channels are MTU{0..4, 6, 7}.)
>
> which I would have expected to see in channel_map. I think the first member
> of this struct is really the base pwm number and so is misnamed?
Ok will change the variable to base_pwm_number.
* @base_pwm_number: First PWM of a channel
and add the below comment in channel_map.
+/*
+ * The MTU channels are {0..4, 6, 7} and The number of IO on MTU1
+ * and MTU2 channel is 1 compared to 2 on others.
+ */
>
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORH, val);
> > + else
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORL, val);
> > +
> > + mutex_lock(&rz_mtu3_pwm->lock);
> > + if (!rz_mtu3_pwm->enable_count[ch])
> > + rz_mtu3_enable(priv->mtu);
> > +
> > + rz_mtu3_pwm->enable_count[ch]++;
> > + mutex_unlock(&rz_mtu3_pwm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void rz_mtu3_pwm_disable(struct rz_mtu3_pwm_chip *rz_mtu3_pwm,
> > + struct pwm_device *pwm)
> > +{
> > + struct rz_mtu3_pwm_channel *priv;
> > + u32 ch;
> > +
> > + priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > + ch = priv - rz_mtu3_pwm->channel_data;
> > +
> > + /* Return to normal mode and disable output pins of MTU3 channel */
> > + if (rz_mtu3_pwm->user_count[ch] <= 1)
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TMDR1,
> > +RZ_MTU3_TMDR1_MD_NORMAL);
>
> This never triggers if both PWMs of a channel are requested, even if both
> are disabled. Is this intended?
It is not required. Will take out this.
>
> > + if (priv->map->channel == pwm->hwpwm)
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORH,
> RZ_MTU3_TIOR_OC_RETAIN);
> > + else
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TIORL,
> > +RZ_MTU3_TIOR_OC_RETAIN);
> > +
> > + mutex_lock(&rz_mtu3_pwm->lock);
> > + rz_mtu3_pwm->enable_count[ch]--;
> > + if (!rz_mtu3_pwm->enable_count[ch])
> > + rz_mtu3_disable(priv->mtu);
> > +
> > + mutex_unlock(&rz_mtu3_pwm->lock);
> > +
> > + pm_runtime_put_sync(rz_mtu3_pwm->chip.dev);
> > +}
> > +
> > [...]
> > +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;
> > + unsigned long pv, dc;
> > + u64 period_cycles;
> > + u64 duty_cycles;
> > + u8 prescale;
> > + u8 val;
> > +
> > + priv = rz_mtu3_get_channel(rz_mtu3_pwm, pwm->hwpwm);
> > + 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);
> > +
> > + if (period_cycles >> (2 * prescale) <= U16_MAX)
> > + pv = period_cycles >> (2 * prescale);
> > + else
> > + pv = U16_MAX;
> > +
> > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rz_mtu3_pwm->rate,
> > + NSEC_PER_SEC);
> > + if (duty_cycles >> (2 * prescale) <= U16_MAX)
> > + dc = duty_cycles >> (2 * prescale);
> > + else
> > + dc = U16_MAX;
> > +
> > + /*
> > + * 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;
> > + 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;
+
+ /*
+ * Prescalar is shared by multiple channels, we cache the prescalar value
+ * from the first enabled channel and use the same value for both
+ * channels.
+ */
+ rz_mtu3_pwm->prescale[ch] = prescale;
+
>
> > + rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRB, dc);
> > + rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRA, pv);
> > + } else {
> > + rz_mtu3_8bit_ch_write(priv->mtu, RZ_MTU3_TCR,
> > + RZ_MTU3_TCR_CCLR_TGRC | val);
> > + rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRD, dc);
> > + rz_mtu3_16bit_ch_write(priv->mtu, RZ_MTU3_TGRC, pv);
> > + }
> > +
> > + /* 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;
> > +}
> > +
> > [...]
> > +static int rz_mtu3_pwm_probe(struct platform_device *pdev) {
> > + struct rz_mtu3 *ddata = dev_get_drvdata(pdev->dev.parent);
>
> Nitpick: I would have called that parent_ddata.
Agreed.
Cheers,
Biju
>
> > + struct rz_mtu3_pwm_chip *rz_mtu3_pwm;
> > + struct device *dev = &pdev->dev;
> > + unsigned int i, j = 0;
> > + int ret;
> > +
> > + rz_mtu3_pwm = devm_kzalloc(&pdev->dev, sizeof(*rz_mtu3_pwm),
> GFP_KERNEL);
> > + if (!rz_mtu3_pwm)
> > + return -ENOMEM;
> > +
> > + rz_mtu3_pwm->clk = ddata->clk;
> > +
> > + for (i = 0; i < RZ_MTU_NUM_CHANNELS; i++) {
> > + if (i == RZ_MTU3_CHAN_5 || i == RZ_MTU3_CHAN_8)
> > + continue;
> > +
> > + rz_mtu3_pwm->channel_data[j].mtu = &ddata->channels[i];
> > + rz_mtu3_pwm->channel_data[j].mtu->dev = dev;
> > + rz_mtu3_pwm->channel_data[j].map = &channel_map[j];
> > + j++;
> > + }
> > +
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
