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?
> + 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.
> + 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);
> +}
> +
> +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt, u32 val, u8 prescale)
> +{
> + u64 tmp;
> +
/* This cannot overflow because ... */
> + 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?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
