From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Fabrizio Castro <fabrizio.castro.jz@renesas.com>,
Magnus Damm <magnus.damm@gmail.com>,
linux-pwm@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
Biju Das <biju.das.au@gmail.com>
Subject: Re: [PATCH v19 3/4] pwm: Add support for RZ/G2L GPT
Date: Thu, 18 Apr 2024 19:03:50 +0200 [thread overview]
Message-ID: <xy7qcimczmqyzk2zvysr3mdkzl3m54cjvh2pxx4d42dc7atyvs@itbq2jr4ghmo> (raw)
In-Reply-To: <20240315143558.221340-4-biju.das.jz@bp.renesas.com>
[-- Attachment #1: Type: text/plain, Size: 6867 bytes --]
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2024-04-18 17:03 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-15 14:35 [PATCH v19 0/4] Add support for RZ/G2L GPT Biju Das
2024-03-15 14:35 ` [PATCH v19 1/4] dt-bindings: pwm: Add RZ/G2L GPT binding Biju Das
2024-03-15 14:35 ` [PATCH v19 2/4] dt-bindings: pwm: rzg2l-gpt: Document renesas,poegs property Biju Das
2024-03-15 14:35 ` [PATCH v19 3/4] pwm: Add support for RZ/G2L GPT Biju Das
2024-04-18 17:03 ` Uwe Kleine-König [this message]
2024-03-15 14:35 ` [PATCH v19 4/4] pwm: rzg2l-gpt: Add support for gpt linking with poeg Biju Das
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xy7qcimczmqyzk2zvysr3mdkzl3m54cjvh2pxx4d42dc7atyvs@itbq2jr4ghmo \
--to=u.kleine-koenig@pengutronix.de \
--cc=biju.das.au@gmail.com \
--cc=biju.das.jz@bp.renesas.com \
--cc=fabrizio.castro.jz@renesas.com \
--cc=geert+renesas@glider.be \
--cc=linux-pwm@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=p.zabel@pengutronix.de \
--cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).