Updated Uwe's Mail address as "u.kleine-koenig@xxxxxxxxxxxxxx" is
returned with below error
Error Details
Error: 550 5.0.350 Remote server returned an error -> 550 account closed - please contact info@xxxxxxxxxxxxxx instead
Message rejected by: metis.whiteo.stw.pengutronix.de
Cheers,
Biju
> -----Original Message-----
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Sent: Wednesday, June 5, 2024 3:53 PM
> Subject: RE: [PATCH v19 3/4] pwm: Add support for RZ/G2L GPT
>
> 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
