Hello Uwe,
Thanks for the feedback.
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> Sent: Thursday, April 18, 2024 6:04 PM
> Subject: Re: [PATCH v19 3/4] pwm: Add support for RZ/G2L GPT
>
> Hello Biju,
>
> thanks for your patience. I'm quite behind on my review tasks.
>
> On Fri, Mar 15, 2024 at 02:35:57PM +0000, Biju Das wrote:
> > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c
> > b/drivers/pwm/pwm-rzg2l-gpt.c new file mode 100644 index
> > 000000000000..8c88f5d536fc
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > @@ -0,0 +1,542 @@
> > [...]
> > +static int rzg2l_gpt_enable(struct rzg2l_gpt_chip *rzg2l_gpt,
> > + struct pwm_device *pwm)
> > +{
> > + u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > + u32 val = RZG2L_GTIOR_GTIOx(sub_ch) | RZG2L_GTIOR_OxE(sub_ch);
> > + u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > +
> > + /* Enable pin output */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR, val,
> > + RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(sub_ch));
>
> That doesn't need protection by the lock?
OK, will add this inside lock, similar for below disable case.
>
> > + mutex_lock(&rzg2l_gpt->lock);
> > + if (!rzg2l_gpt->enable_count[ch])
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, 0,
> > +RZG2L_GTCR_CST);
> > +
> > + rzg2l_gpt->enable_count[ch]++;
> > + mutex_unlock(&rzg2l_gpt->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt,
> > + struct pwm_device *pwm)
> > +{
> > + u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > + u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 offs = RZG2L_GET_CH_OFFS(ch);
> > +
> > + /* Disable pin output */
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR,
> > +RZG2L_GTIOR_OxE(sub_ch), 0);
> > +
> > + /* Stop count, Output low on GTIOCx pin when counting stops */
> > + mutex_lock(&rzg2l_gpt->lock);
> > + /* Don't decrement, if ch_en_bits is set by the probe */
> > + if (!test_bit(pwm->hwpwm, rzg2l_gpt->ch_en_bits))
> > + rzg2l_gpt->enable_count[ch]--;
>
> I don't get the reason why this is skipped if ch_en_bits is set.
During testing I found a issue, where disable is called and its value
is going negative. I will remove this check, increment enable_count in probe()
for bootloader enabling pwm case.
>
> > + if (!rzg2l_gpt->enable_count[ch])
> > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_CST,
> > +0);
> > +
> > + mutex_unlock(&rzg2l_gpt->lock);
> > +
> > + /*
> > + * Probe() set these bits, if pwm is enabled by bootloader. In such
> > + * case, clearing the bits will avoid errors during unbind.
> > + */
> > + if (test_bit(pwm->hwpwm, rzg2l_gpt->ch_en_bits))
> > + clear_bit(pwm->hwpwm, rzg2l_gpt->ch_en_bits); }
I will move this in apply.
> > +
> > +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip
> > +*rzg2l_gpt,
> > +u32 val, u8 prescale) {
> > + u64 tmp;
> > +
>
> /* This cannot overflow because ... */
[1] < [2]
The max calculated value is
[1] 2^32 * 2^10 * 10^6 = 4,398,046,511,104,000,000
[2] 2^64 =18,446,744,073,709,551,616
I haven't add this as a comment, as it is clear that calculated value is < 2 ^64.
If it is NSEC_PER_SEC, then it will overflow.
Basically to avoid overflow. we scaled down by 1000
for all calculation as rate is multiple of 10^6.
Please let me know, do I need to comment this?
>
> > + tmp = (u64)val << (2 * prescale);
> > + tmp *= USEC_PER_SEC;
> > +
> > + return DIV64_U64_ROUND_UP(tmp, rzg2l_gpt->rate_khz); }
> > +
> > [...]
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt;
> > + struct device *dev = &pdev->dev;
> > + struct pwm_chip *chip;
> > + unsigned long rate;
> > + struct clk *clk;
> > + int ret;
> > + u32 i;
> > +
> > + chip = devm_pwmchip_alloc(dev, RZG2L_MAX_PWM_CHANNELS, sizeof(*rzg2l_gpt));
> > + if (IS_ERR(chip))
> > + return PTR_ERR(chip);
> > + rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > +
> > + rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(rzg2l_gpt->mmio))
> > + return PTR_ERR(rzg2l_gpt->mmio);
> > +
> > + rzg2l_gpt->rstc = devm_reset_control_get_exclusive(dev, NULL);
> > + if (IS_ERR(rzg2l_gpt->rstc))
> > + return dev_err_probe(dev, PTR_ERR(rzg2l_gpt->rstc),
> > + "get reset failed\n");
> > +
> > + clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(clk))
> > + return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n");
> > +
> > + ret = reset_control_deassert(rzg2l_gpt->rstc);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "cannot deassert reset
> > +control\n");
> > +
> > + pm_runtime_enable(dev);
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret)
> > + goto err_reset;
> > +
> > + ret = devm_clk_rate_exclusive_get(dev, clk);
> > + if (ret)
> > + goto err_pm_put;
> > +
> > + rate = clk_get_rate(clk);
> > + if (!rate) {
> > + ret = dev_err_probe(dev, -EINVAL, "gpt clk rate is 0");
> > + goto err_pm_put;
> > + }
> > +
> > + /*
> > + * Refuse clk rates > 1 GHz to prevent overflow later for computing
> > + * period and duty cycle.
> > + */
> > + if (rate > NSEC_PER_SEC) {
> > + ret = dev_err_probe(dev, -EINVAL, "gpt clk rate is > 1GHz");
> > + goto err_pm_put;
> > + }
> > +
> > + /*
> > + * Rate is in MHz and is always integer for peripheral clk
> > + * 2^32 * 2^10 (prescalar) * 10^6 (rate_khz) < 2^64
> > + * So make sure rate is multiple of 1000.
> > + */
> > + rzg2l_gpt->rate_khz = rate / KILO;
> > + if (rzg2l_gpt->rate_khz * KILO != rate) {
> > + ret = dev_err_probe(dev, -EINVAL, "rate is not multiple of 1000");
> > + goto err_pm_put;
> > + }
> > +
> > + rzg2l_gpt->max_val = div64_u64((u64)U32_MAX * USEC_PER_SEC,
> > + rzg2l_gpt->rate_khz) * RZG2L_MAX_SCALE_FACTOR;
> > +
> > + /*
> > + * We need to keep the clock on, in case the bootloader has enabled the
> > + * PWM and is running during probe().
> > + */
> > + for (i = 0; i < RZG2L_MAX_PWM_CHANNELS; i++) {
> > + if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, i)) {
> > + set_bit(i, rzg2l_gpt->ch_en_bits);
>
> The tracking of which channels were enabled by the bootloader is more
> extensive than that of other drivers. (That's good from a correctness point of view.) I consider doing something like:
>
> for (i = 0; i < npwm; ++i) {
> pwm = &chip->pwm[i];
>
> pwm->state = { 0, };
>
> ret = chip->ops->get_state(chip, pwm, &state);
> if (!ret && state->enabled)
> chip->ops->apply(chip, pwm, &state);
> }
>
> (with some more error checking) in pwmchip_register(). That should get
> the usage count's right, but would (maybe?) conflict with your
> handling here. Anyhow, that's orthogonal to this patch for now (and
> needs some more thoughs. For example it might not be a good idea to
> call
> .get_state() and .apply() without request before. Also it might not
> work for chips that cannot be disabled in hardware).
>
> Back to your patch: Maybe call .ch_en_bits
> .bootloader_enabled_channels instead? Also I think this could be
> simplified (but not entirely sure I grabbed all the details, so take
> this with a grain of
> salt):
>
> - In .probe() set .bootloader_enabled_channels[i] if pwm#i is enabled and
> ensure it stays on.
>
> - In .apply() replace the code that is supposed to enable the HW by:
>
> if (...->bootloader_enabled_channels[i]) {
> /*
> * it's already be on. Instead of reenabling in hardware
> * just take over from the bootloader
> */
> ...->bootloader_enabled_channels[i] = 0;
> } else {
> enable_hw();
> ... get pm_runtime reference etc.
> }
>
> - in .remove():
>
> for (i = 0; i < npwm; ++i) {
> if (...->bootloader_enabled_channels[i]) {
>
> ... drop pm_runtime reference, but don't disable HW
>
> }
> }
>
> Does that make sense? Did I miss anything?
I agree, it is cleaner now. Will send v20, if you are ok with it.
On the probe()
- set_bit(i, rzg2l_gpt->ch_en_bits);
+ u8 ch = RZG2L_GET_CH(i);
+
+ rzg2l_gpt->bootloader_enabled_channels[i] = 1;
+ rzg2l_gpt->enable_count[ch]++;
On Apply()
if (!state->enabled) {
if (enabled) {
+ /*
+ * Probe() sets bootloader_enabled_channels. In such case,
+ * clearing the flag will avoid errors during unbind.
+ */
+ if (rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm])
+ rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = 0;
+
rzg2l_gpt_disable(rzg2l_gpt, pwm);
pm_runtime_put_sync(pwmchip_parent(chip));
}
@@ -366,10 +363,18 @@ static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
- if (!enabled) {
- ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
- if (ret)
- return ret;
+ if (rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm]) {
+ /*
+ * it's already be on. Instead of reenabling in hardware
+ * just take over from the bootloader
+ */
+ rzg2l_gpt->bootloader_enabled_channels[pwm->hwpwm] = 0;
+ } else {
+ if (!enabled) {
+ ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
+ if (ret)
+ return ret;
+ }
}
- .remove
@@ -410,8 +415,8 @@ static void rzg2l_gpt_reset_assert_pm_disable(void *data)
* count. Before apply, if there is unbind/remove callback we need to
* decrement the PM usage count.
*/
- for (i = 0; i < RZG2L_MAX_PWM_CHANNELS; i++) {
- if (test_bit(i, rzg2l_gpt->ch_en_bits))
+ for (i = 0; i < chip->npwm; i++) {
+ if (rzg2l_gpt->bootloader_enabled_channels[i])
Cheers,
Biju
