* Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-18 10:27 ` Thierry Reding
0 siblings, 0 replies; 41+ messages in thread
From: Thierry Reding @ 2019-03-18 10:27 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree,
shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel,
robh+dt, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez,
festevam, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 18622 bytes --]
On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> inside, add TPM PWM driver support.
I think this could be improved. You're basically restating the subject
here, but the commit message gives you a good place to provide more
information about the capabilities and restrictions. For example if this
supports polarity, mention it in the commit message.
Something that also looks very interesting is how this supports
detection of the number of channels, so make sure to mention that as
well.
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V4:
> - improve register read/write using bit field operations;
> - correct some logic issue;
> - ONLY disable clock when PWM is NOT in use during suspend;
> - add some comments for PWM mode settings;
> - fix some spelling errors;
> - reading channel number from register instead of using fix value.
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-imx-tpm.c | 436 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 448 insertions(+)
> create mode 100644 drivers/pwm/pwm-imx-tpm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 54f8238..3ea0391 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -210,6 +210,17 @@ config PWM_IMX27
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx27.
>
> +config PWM_IMX_TPM
> + tristate "i.MX TPM PWM support"
> + depends on ARCH_MXC || COMPILE_TEST
> + depends on HAVE_CLK && HAS_IOMEM
> + help
> + Generic PWM framework driver for i.MX7ULP TPM module, TPM's full
> + name is Low Power Timer/Pulse Width Modulation Module.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-imx-tpm.
> +
> config PWM_JZ4740
> tristate "Ingenic JZ47xx PWM support"
> depends on MACH_INGENIC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 448825e..c368599 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new file mode 100644
> index 0000000..12cb16c
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,436 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + *
> + * Limitations:
> + * - The TPM counter and period counter are shared between
> + * multiple channels, so all channels should use same period
> + * settings.
This sounds like something that you may want to also highlight in the
device tree bindings.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#define PWM_IMX_TPM_PARAM 0x4
> +#define PWM_IMX_TPM_GLOBAL 0x8
> +#define PWM_IMX_TPM_SC 0x10
> +#define PWM_IMX_TPM_CNT 0x14
> +#define PWM_IMX_TPM_MOD 0x18
> +#define PWM_IMX_TPM_C0SC 0x20
> +#define PWM_IMX_TPM_C0V 0x24
> +
> +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7, 0)
> +
> +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0)
> +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
> +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
> +#define PWM_IMX_TPM_SC_CPWMS BIT(5)
> +
> +#define PWM_IMX_TPM_CnSC_CHF BIT(7)
> +#define PWM_IMX_TPM_CnSC_MSB BIT(5)
> +#define PWM_IMX_TPM_CnSC_MSA BIT(4)
> +#define PWM_IMX_TPM_CnSC_ELSB BIT(3)
> +#define PWM_IMX_TPM_CnSC_ELSA BIT(2)
> +
> +#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
> +
> +#define PWM_IMX_TPM_MAX_COUNT 0xffff
> +
> +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> +
> +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + (n) * 0x8)
> +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + (n) * 0x8)
You never use the C0SC and C0V registers other than as the base of these
indexed registers. I think it makes more sense to replace the C0SC and
C0V register definitions above with these.
> +struct imx_tpm_pwm_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *base;
> + struct mutex lock;
> + u32 user_count;
> + u32 enable_count;
> + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM];
> + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM];
I suggest you use per PWM data. That way you can attach the data
directly per PWM channel instead of having to carry the array here.
> +};
> +
> +#define to_imx_tpm_pwm_chip(_chip) \
> + container_of(_chip, struct imx_tpm_pwm_chip, chip)
I'd prefer this to be a static inline function.
> +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 period)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 period_cnt, val, div, saved_cmod;
> + u64 tmp;
> +
> + tmp = clk_get_rate(tpm->clk);
> + tmp *= period;
> + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> + if (val <= PWM_IMX_TPM_MAX_COUNT)
> + div = 0;
> + else
> + div = ilog2(roundup_pow_of_two(val /
> + (PWM_IMX_TPM_MAX_COUNT + 1)));
Maybe use some temporary variables to make this easier to read.
> + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, div))) {
> + dev_err(chip->dev,
> + "failed to find valid prescale value!\n");
> + return -EINVAL;
> + }
> +
> + /* make sure counter is disabled for programming prescale */
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> + if (saved_cmod) {
> + val &= ~PWM_IMX_TPM_SC_CMOD;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> +
> + /* set TPM counter prescale */
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + val &= ~PWM_IMX_TPM_SC_PS;
> + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, div);
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> +
> + /*
> + * set period counter: according to RM, the MOD register is
> + * updated immediately after CMOD[1:0] = 2b'00 above
> + */
> + do_div(tmp, NSEC_PER_SEC);
> + period_cnt = (tmp + ((1 << div) >> 1)) >> div;
> + if (period_cnt > PWM_IMX_TPM_MOD_MOD) {
> + dev_err(chip->dev,
> + "failed to find valid period count!\n");
> + return -EINVAL;
> + }
I think you should move the computations and the check to an earlier
point so that you avoid erroring out after you've already written some
of the values. Part of the idea of the atomic API is that it allows you
to check a configuration for validity before you write any registers so
that you either apply a working configuration or none at all. With the
above you could easily end up halfway through the configuration and
leave the PWM in a non-working state.
> + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD);
> +
> + /* restore the clock mode if necessary */
> + if (saved_cmod) {
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> +
> + return 0;
> +}
> +
> +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + u32 period,
> + u32 duty_cycle,
> + enum pwm_polarity polarity)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 duty_cnt, val;
> + u64 tmp;
> +
> + /* set duty counter */
> + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
> + tmp *= duty_cycle;
> + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> +
> + /*
> + * set polarity (for edge-aligned PWM modes)
> + *
> + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> + * 0 10 10 PWM High-true pulse
> + * 0 10 00 PWM Reserved
> + * 0 10 01 PWM Low-true pulse
> + * 0 10 11 PWM Reserved
> + *
> + * High-true pulse: clear output on counter match, set output on
> + * counter reload, set output when counter first enabled or paused.
> + *
> + * Low-true pulse: set output on counter match, clear output on
> + * counter reload, clear output when counter first enabled or paused.
> + */
> +
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA |
> + PWM_IMX_TPM_CnSC_MSA);
> + val |= PWM_IMX_TPM_CnSC_MSB;
> + val |= (polarity == PWM_POLARITY_NORMAL) ?
> + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> + /*
> + * polarity settings will enabled/disable output status
> + * immediately, so here ONLY save the config and write
> + * it into register when channel is enabled/disabled.
> + */
> + tpm->chn_config[pwm->hwpwm] = val;
> +}
> +
> +/*
> + * When a channel's polarity is configured, the polarity settings
> + * will be saved and ONLY write into the register when the channel
> + * is enabled.
> + *
> + * When a channel is disabled, its polarity settings will be saved
> + * and its output will be disabled by clearing polarity settings.
> + *
> + * when a channel is enabled, its polarity settings will be restored
"when" -> "When".
> + * and output will be enabled again.
> + */
> +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + bool enable)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 val;
> +
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + if (enable) {
> + /* restore channel config */
> + writel(tpm->chn_config[pwm->hwpwm],
> + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +
> + if (++tpm->enable_count == 1) {
> + /* start TPM counter */
> + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> + } else {
> + /* disable channel */
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + val &= ~(PWM_IMX_TPM_CnSC_MSA | PWM_IMX_TPM_CnSC_MSB |
> + PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA);
> + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +
> + if (--tpm->enable_count == 0) {
> + /* stop TPM counter since all channels are disabled */
> + val &= ~PWM_IMX_TPM_SC_CMOD;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> + }
> +
> + /* update channel status */
> + tpm->chn_status[pwm->hwpwm] = enable;
> +}
> +
> +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u64 tmp;
> + u32 val, rate;
> +
> + /* get period */
> + rate = clk_get_rate(tpm->clk);
> + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + val &= PWM_IMX_TPM_SC_PS;
> + tmp *= (1 << val) * NSEC_PER_SEC;
> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> + /* get duty cycle */
> + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> + tmp *= (1 << val) * NSEC_PER_SEC;
> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> + /* get polarity */
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + if (val & PWM_IMX_TPM_CnSC_ELSA)
> + state->polarity = PWM_POLARITY_INVERSED;
> + else
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + /* get channel status */
> + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false;
> +}
> +
> +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + struct pwm_state curstate;
> + int ret;
> +
> + mutex_lock(&tpm->lock);
> +
> + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> +
> + if (state->period != curstate.period) {
> + /*
> + * TPM counter is shared by multiple channels, so
> + * prescale and period can NOT be modified when
> + * there are multiple channels in use.
> + */
> + if (tpm->user_count != 1)
> + return -EBUSY;
> + ret = pwm_imx_tpm_config_counter(chip, state->period);
> + if (ret)
> + return ret;
> + }
> +
> + if (state->enabled == false) {
> + /*
> + * if eventually the PWM output is LOW, either
> + * duty cycle is 0 or status is disabled, need
> + * to make sure the output pin is LOW.
> + */
> + pwm_imx_tpm_config(chip, pwm, state->period,
> + 0, state->polarity);
> + if (curstate.enabled)
> + pwm_imx_tpm_enable(chip, pwm, false);
> + } else {
> + pwm_imx_tpm_config(chip, pwm, state->period,
> + state->duty_cycle, state->polarity);
> + if (!curstate.enabled)
> + pwm_imx_tpm_enable(chip, pwm, true);
Doesn't this mean that you won't be applying changes to the polarity
while a PWM is enabled? That seems wrong. Granted, you may usually not
run into that, but if you can't support it I think you should at least
return an error if you detect that the user wants to change polarity
while the PWM is enabled.
> + }
> +
> + mutex_unlock(&tpm->lock);
> +
> + return 0;
> +}
> +
> +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +
> + mutex_lock(&tpm->lock);
> + tpm->user_count++;
> + mutex_unlock(&tpm->lock);
> +
> + return 0;
> +}
> +
> +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +
> + mutex_lock(&tpm->lock);
> + tpm->user_count--;
> + mutex_unlock(&tpm->lock);
> +}
> +
> +static const struct pwm_ops imx_tpm_pwm_ops = {
> + .request = pwm_imx_tpm_request,
> + .free = pwm_imx_tpm_free,
> + .get_state = pwm_imx_tpm_get_state,
> + .apply = pwm_imx_tpm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static int pwm_imx_tpm_probe(struct platform_device *pdev)
> +{
> + struct imx_tpm_pwm_chip *tpm;
> + int ret;
> + u32 val;
> +
> + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
> + if (!tpm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, tpm);
> +
> + tpm->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(tpm->base)) {
> + ret = PTR_ERR(tpm->base);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "pwm ioremap failed %d\n", ret);
I don't think devm_platform_ioremap_resource() will ever return
-EPROBE_DEFER. Also, there's no need to print an error here since
devm_ioremap_resource() prints one for any possible error condition
anyway.
> + return ret;
> + }
> +
> + tpm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(tpm->clk)) {
> + ret = PTR_ERR(tpm->clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to get pwm clk %d\n", ret);
I think it's better to clarify that you're outputting an error (rather
than a clock index) by separating the error code with a ':'. Also these
error messages are easier to read if you spell out words: "clk" ->
"clock". Also: "pwm" -> "PWM".
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(tpm->clk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "failed to prepare or enable clk %d\n", ret);
Same comments as above.
> + return ret;
> + }
> +
> + tpm->chip.dev = &pdev->dev;
> + tpm->chip.ops = &imx_tpm_pwm_ops;
> + tpm->chip.base = -1;
> + /* get channel number */
> + val = readl(tpm->base + PWM_IMX_TPM_PARAM);
> + tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
The comment is misleading, better would be "get number of channels".
Also, please use blank lines to increase readability. You could leave a
blank line before the comment, for example.
> +
> + mutex_init(&tpm->lock);
> +
> + ret = pwmchip_add(&tpm->chip);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
"failed to add PWM chip: %d"
> + clk_disable_unprepare(tpm->clk);
> + }
> +
> + return ret;
> +}
> +
> +static int pwm_imx_tpm_remove(struct platform_device *pdev)
> +{
> + struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
> + int ret = pwmchip_remove(&tpm->chip);
> +
> + clk_disable_unprepare(tpm->clk);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> +
> + if (tpm->enable_count == 0)
> + clk_disable_unprepare(tpm->clk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused pwm_imx_tpm_resume(struct device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + if (tpm->enable_count == 0) {
> + ret = clk_prepare_enable(tpm->clk);
> + if (ret)
> + dev_err(dev,
> + "failed to prepare or enable clk %d\n",
"failed to prepare or enable clock: %d"
> + ret);
> + }
> +
> + return ret;
> +};
Your handling of the clock seems strange here. Basically in the above
you always keep the clock on and you only disable it if there are no
users and you're going to suspend.
Why do you need to keep an extra reference count anyway? Or why keep the
clock on all the time? Can't you just do a clk_prepare_enable() every
time somebody enables the PWM? The CCF already has built-in reference
counting, so I'm not sure you really need to duplicate that here.
Thierry
> +
> +static SIMPLE_DEV_PM_OPS(imx_tpm_pwm_pm,
> + pwm_imx_tpm_suspend, pwm_imx_tpm_resume);
> +
> +static const struct of_device_id imx_tpm_pwm_dt_ids[] = {
> + { .compatible = "fsl,imx-tpm-pwm", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_tpm_pwm_dt_ids);
> +
> +static struct platform_driver imx_tpm_pwm_driver = {
> + .driver = {
> + .name = "imx-tpm-pwm",
> + .of_match_table = imx_tpm_pwm_dt_ids,
> + .pm = &imx_tpm_pwm_pm,
> + },
> + .probe = pwm_imx_tpm_probe,
> + .remove = pwm_imx_tpm_remove,
> +};
> +module_platform_driver(imx_tpm_pwm_driver);
> +
> +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> +MODULE_DESCRIPTION("i.MX TPM PWM Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-18 10:27 ` Thierry Reding
0 siblings, 0 replies; 41+ messages in thread
From: Thierry Reding @ 2019-03-18 10:27 UTC (permalink / raw)
To: Anson Huang
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 18622 bytes --]
On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> inside, add TPM PWM driver support.
I think this could be improved. You're basically restating the subject
here, but the commit message gives you a good place to provide more
information about the capabilities and restrictions. For example if this
supports polarity, mention it in the commit message.
Something that also looks very interesting is how this supports
detection of the number of channels, so make sure to mention that as
well.
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V4:
> - improve register read/write using bit field operations;
> - correct some logic issue;
> - ONLY disable clock when PWM is NOT in use during suspend;
> - add some comments for PWM mode settings;
> - fix some spelling errors;
> - reading channel number from register instead of using fix value.
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-imx-tpm.c | 436 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 448 insertions(+)
> create mode 100644 drivers/pwm/pwm-imx-tpm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 54f8238..3ea0391 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -210,6 +210,17 @@ config PWM_IMX27
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx27.
>
> +config PWM_IMX_TPM
> + tristate "i.MX TPM PWM support"
> + depends on ARCH_MXC || COMPILE_TEST
> + depends on HAVE_CLK && HAS_IOMEM
> + help
> + Generic PWM framework driver for i.MX7ULP TPM module, TPM's full
> + name is Low Power Timer/Pulse Width Modulation Module.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-imx-tpm.
> +
> config PWM_JZ4740
> tristate "Ingenic JZ47xx PWM support"
> depends on MACH_INGENIC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 448825e..c368599 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new file mode 100644
> index 0000000..12cb16c
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,436 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + *
> + * Limitations:
> + * - The TPM counter and period counter are shared between
> + * multiple channels, so all channels should use same period
> + * settings.
This sounds like something that you may want to also highlight in the
device tree bindings.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#define PWM_IMX_TPM_PARAM 0x4
> +#define PWM_IMX_TPM_GLOBAL 0x8
> +#define PWM_IMX_TPM_SC 0x10
> +#define PWM_IMX_TPM_CNT 0x14
> +#define PWM_IMX_TPM_MOD 0x18
> +#define PWM_IMX_TPM_C0SC 0x20
> +#define PWM_IMX_TPM_C0V 0x24
> +
> +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7, 0)
> +
> +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0)
> +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
> +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
> +#define PWM_IMX_TPM_SC_CPWMS BIT(5)
> +
> +#define PWM_IMX_TPM_CnSC_CHF BIT(7)
> +#define PWM_IMX_TPM_CnSC_MSB BIT(5)
> +#define PWM_IMX_TPM_CnSC_MSA BIT(4)
> +#define PWM_IMX_TPM_CnSC_ELSB BIT(3)
> +#define PWM_IMX_TPM_CnSC_ELSA BIT(2)
> +
> +#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
> +
> +#define PWM_IMX_TPM_MAX_COUNT 0xffff
> +
> +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> +
> +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + (n) * 0x8)
> +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + (n) * 0x8)
You never use the C0SC and C0V registers other than as the base of these
indexed registers. I think it makes more sense to replace the C0SC and
C0V register definitions above with these.
> +struct imx_tpm_pwm_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *base;
> + struct mutex lock;
> + u32 user_count;
> + u32 enable_count;
> + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM];
> + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM];
I suggest you use per PWM data. That way you can attach the data
directly per PWM channel instead of having to carry the array here.
> +};
> +
> +#define to_imx_tpm_pwm_chip(_chip) \
> + container_of(_chip, struct imx_tpm_pwm_chip, chip)
I'd prefer this to be a static inline function.
> +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 period)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 period_cnt, val, div, saved_cmod;
> + u64 tmp;
> +
> + tmp = clk_get_rate(tpm->clk);
> + tmp *= period;
> + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> + if (val <= PWM_IMX_TPM_MAX_COUNT)
> + div = 0;
> + else
> + div = ilog2(roundup_pow_of_two(val /
> + (PWM_IMX_TPM_MAX_COUNT + 1)));
Maybe use some temporary variables to make this easier to read.
> + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, div))) {
> + dev_err(chip->dev,
> + "failed to find valid prescale value!\n");
> + return -EINVAL;
> + }
> +
> + /* make sure counter is disabled for programming prescale */
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> + if (saved_cmod) {
> + val &= ~PWM_IMX_TPM_SC_CMOD;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> +
> + /* set TPM counter prescale */
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + val &= ~PWM_IMX_TPM_SC_PS;
> + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, div);
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> +
> + /*
> + * set period counter: according to RM, the MOD register is
> + * updated immediately after CMOD[1:0] = 2b'00 above
> + */
> + do_div(tmp, NSEC_PER_SEC);
> + period_cnt = (tmp + ((1 << div) >> 1)) >> div;
> + if (period_cnt > PWM_IMX_TPM_MOD_MOD) {
> + dev_err(chip->dev,
> + "failed to find valid period count!\n");
> + return -EINVAL;
> + }
I think you should move the computations and the check to an earlier
point so that you avoid erroring out after you've already written some
of the values. Part of the idea of the atomic API is that it allows you
to check a configuration for validity before you write any registers so
that you either apply a working configuration or none at all. With the
above you could easily end up halfway through the configuration and
leave the PWM in a non-working state.
> + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD);
> +
> + /* restore the clock mode if necessary */
> + if (saved_cmod) {
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> +
> + return 0;
> +}
> +
> +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + u32 period,
> + u32 duty_cycle,
> + enum pwm_polarity polarity)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 duty_cnt, val;
> + u64 tmp;
> +
> + /* set duty counter */
> + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
> + tmp *= duty_cycle;
> + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> +
> + /*
> + * set polarity (for edge-aligned PWM modes)
> + *
> + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> + * 0 10 10 PWM High-true pulse
> + * 0 10 00 PWM Reserved
> + * 0 10 01 PWM Low-true pulse
> + * 0 10 11 PWM Reserved
> + *
> + * High-true pulse: clear output on counter match, set output on
> + * counter reload, set output when counter first enabled or paused.
> + *
> + * Low-true pulse: set output on counter match, clear output on
> + * counter reload, clear output when counter first enabled or paused.
> + */
> +
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA |
> + PWM_IMX_TPM_CnSC_MSA);
> + val |= PWM_IMX_TPM_CnSC_MSB;
> + val |= (polarity == PWM_POLARITY_NORMAL) ?
> + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> + /*
> + * polarity settings will enabled/disable output status
> + * immediately, so here ONLY save the config and write
> + * it into register when channel is enabled/disabled.
> + */
> + tpm->chn_config[pwm->hwpwm] = val;
> +}
> +
> +/*
> + * When a channel's polarity is configured, the polarity settings
> + * will be saved and ONLY write into the register when the channel
> + * is enabled.
> + *
> + * When a channel is disabled, its polarity settings will be saved
> + * and its output will be disabled by clearing polarity settings.
> + *
> + * when a channel is enabled, its polarity settings will be restored
"when" -> "When".
> + * and output will be enabled again.
> + */
> +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + bool enable)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 val;
> +
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + if (enable) {
> + /* restore channel config */
> + writel(tpm->chn_config[pwm->hwpwm],
> + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +
> + if (++tpm->enable_count == 1) {
> + /* start TPM counter */
> + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> + } else {
> + /* disable channel */
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + val &= ~(PWM_IMX_TPM_CnSC_MSA | PWM_IMX_TPM_CnSC_MSB |
> + PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA);
> + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +
> + if (--tpm->enable_count == 0) {
> + /* stop TPM counter since all channels are disabled */
> + val &= ~PWM_IMX_TPM_SC_CMOD;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> + }
> +
> + /* update channel status */
> + tpm->chn_status[pwm->hwpwm] = enable;
> +}
> +
> +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u64 tmp;
> + u32 val, rate;
> +
> + /* get period */
> + rate = clk_get_rate(tpm->clk);
> + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + val &= PWM_IMX_TPM_SC_PS;
> + tmp *= (1 << val) * NSEC_PER_SEC;
> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> + /* get duty cycle */
> + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> + tmp *= (1 << val) * NSEC_PER_SEC;
> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> + /* get polarity */
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + if (val & PWM_IMX_TPM_CnSC_ELSA)
> + state->polarity = PWM_POLARITY_INVERSED;
> + else
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + /* get channel status */
> + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false;
> +}
> +
> +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + struct pwm_state curstate;
> + int ret;
> +
> + mutex_lock(&tpm->lock);
> +
> + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> +
> + if (state->period != curstate.period) {
> + /*
> + * TPM counter is shared by multiple channels, so
> + * prescale and period can NOT be modified when
> + * there are multiple channels in use.
> + */
> + if (tpm->user_count != 1)
> + return -EBUSY;
> + ret = pwm_imx_tpm_config_counter(chip, state->period);
> + if (ret)
> + return ret;
> + }
> +
> + if (state->enabled == false) {
> + /*
> + * if eventually the PWM output is LOW, either
> + * duty cycle is 0 or status is disabled, need
> + * to make sure the output pin is LOW.
> + */
> + pwm_imx_tpm_config(chip, pwm, state->period,
> + 0, state->polarity);
> + if (curstate.enabled)
> + pwm_imx_tpm_enable(chip, pwm, false);
> + } else {
> + pwm_imx_tpm_config(chip, pwm, state->period,
> + state->duty_cycle, state->polarity);
> + if (!curstate.enabled)
> + pwm_imx_tpm_enable(chip, pwm, true);
Doesn't this mean that you won't be applying changes to the polarity
while a PWM is enabled? That seems wrong. Granted, you may usually not
run into that, but if you can't support it I think you should at least
return an error if you detect that the user wants to change polarity
while the PWM is enabled.
> + }
> +
> + mutex_unlock(&tpm->lock);
> +
> + return 0;
> +}
> +
> +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +
> + mutex_lock(&tpm->lock);
> + tpm->user_count++;
> + mutex_unlock(&tpm->lock);
> +
> + return 0;
> +}
> +
> +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct pwm_device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +
> + mutex_lock(&tpm->lock);
> + tpm->user_count--;
> + mutex_unlock(&tpm->lock);
> +}
> +
> +static const struct pwm_ops imx_tpm_pwm_ops = {
> + .request = pwm_imx_tpm_request,
> + .free = pwm_imx_tpm_free,
> + .get_state = pwm_imx_tpm_get_state,
> + .apply = pwm_imx_tpm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +static int pwm_imx_tpm_probe(struct platform_device *pdev)
> +{
> + struct imx_tpm_pwm_chip *tpm;
> + int ret;
> + u32 val;
> +
> + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
> + if (!tpm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, tpm);
> +
> + tpm->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(tpm->base)) {
> + ret = PTR_ERR(tpm->base);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "pwm ioremap failed %d\n", ret);
I don't think devm_platform_ioremap_resource() will ever return
-EPROBE_DEFER. Also, there's no need to print an error here since
devm_ioremap_resource() prints one for any possible error condition
anyway.
> + return ret;
> + }
> +
> + tpm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(tpm->clk)) {
> + ret = PTR_ERR(tpm->clk);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to get pwm clk %d\n", ret);
I think it's better to clarify that you're outputting an error (rather
than a clock index) by separating the error code with a ':'. Also these
error messages are easier to read if you spell out words: "clk" ->
"clock". Also: "pwm" -> "PWM".
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(tpm->clk);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "failed to prepare or enable clk %d\n", ret);
Same comments as above.
> + return ret;
> + }
> +
> + tpm->chip.dev = &pdev->dev;
> + tpm->chip.ops = &imx_tpm_pwm_ops;
> + tpm->chip.base = -1;
> + /* get channel number */
> + val = readl(tpm->base + PWM_IMX_TPM_PARAM);
> + tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
The comment is misleading, better would be "get number of channels".
Also, please use blank lines to increase readability. You could leave a
blank line before the comment, for example.
> +
> + mutex_init(&tpm->lock);
> +
> + ret = pwmchip_add(&tpm->chip);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
"failed to add PWM chip: %d"
> + clk_disable_unprepare(tpm->clk);
> + }
> +
> + return ret;
> +}
> +
> +static int pwm_imx_tpm_remove(struct platform_device *pdev)
> +{
> + struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
> + int ret = pwmchip_remove(&tpm->chip);
> +
> + clk_disable_unprepare(tpm->clk);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> +
> + if (tpm->enable_count == 0)
> + clk_disable_unprepare(tpm->clk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused pwm_imx_tpm_resume(struct device *dev)
> +{
> + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + if (tpm->enable_count == 0) {
> + ret = clk_prepare_enable(tpm->clk);
> + if (ret)
> + dev_err(dev,
> + "failed to prepare or enable clk %d\n",
"failed to prepare or enable clock: %d"
> + ret);
> + }
> +
> + return ret;
> +};
Your handling of the clock seems strange here. Basically in the above
you always keep the clock on and you only disable it if there are no
users and you're going to suspend.
Why do you need to keep an extra reference count anyway? Or why keep the
clock on all the time? Can't you just do a clk_prepare_enable() every
time somebody enables the PWM? The CCF already has built-in reference
counting, so I'm not sure you really need to duplicate that here.
Thierry
> +
> +static SIMPLE_DEV_PM_OPS(imx_tpm_pwm_pm,
> + pwm_imx_tpm_suspend, pwm_imx_tpm_resume);
> +
> +static const struct of_device_id imx_tpm_pwm_dt_ids[] = {
> + { .compatible = "fsl,imx-tpm-pwm", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx_tpm_pwm_dt_ids);
> +
> +static struct platform_driver imx_tpm_pwm_driver = {
> + .driver = {
> + .name = "imx-tpm-pwm",
> + .of_match_table = imx_tpm_pwm_dt_ids,
> + .pm = &imx_tpm_pwm_pm,
> + },
> + .probe = pwm_imx_tpm_probe,
> + .remove = pwm_imx_tpm_remove,
> +};
> +module_platform_driver(imx_tpm_pwm_driver);
> +
> +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> +MODULE_DESCRIPTION("i.MX TPM PWM Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-18 10:27 ` Thierry Reding
(?)
@ 2019-03-18 11:33 ` Anson Huang
-1 siblings, 0 replies; 41+ messages in thread
From: Anson Huang @ 2019-03-18 11:33 UTC (permalink / raw)
To: Thierry Reding
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel,
u.kleine-koenig, dl-linux-imx
Hi, Thierry
Best Regards!
Anson Huang
> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: 2019年3月18日 18:28
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
>
> I think this could be improved. You're basically restating the subject here, but
> the commit message gives you a good place to provide more information
> about the capabilities and restrictions. For example if this supports polarity,
> mention it in the commit message.
>
> Something that also looks very interesting is how this supports detection of
> the number of channels, so make sure to mention that as well.
>
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V4:
> > - improve register read/write using bit field operations;
> > - correct some logic issue;
> > - ONLY disable clock when PWM is NOT in use during suspend;
> > - add some comments for PWM mode settings;
> > - fix some spelling errors;
> > - reading channel number from register instead of using fix value.
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 436
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 448 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 54f8238..3ea0391 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -210,6 +210,17 @@ config PWM_IMX27
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx27.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + depends on HAVE_CLK && HAS_IOMEM
> > + help
> > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's
> full
> > + name is Low Power Timer/Pulse Width Modulation Module.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-imx-tpm.
> > +
> > config PWM_JZ4740
> > tristate "Ingenic JZ47xx PWM support"
> > depends on MACH_INGENIC
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 448825e..c368599 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-
> hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new
> > file mode 100644 index 0000000..12cb16c
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,436 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + *
> > + * Limitations:
> > + * - The TPM counter and period counter are shared between
> > + * multiple channels, so all channels should use same period
> > + * settings.
>
> This sounds like something that you may want to also highlight in the device
> tree bindings.
OK.
>
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +#define PWM_IMX_TPM_PARAM 0x4
> > +#define PWM_IMX_TPM_GLOBAL 0x8
> > +#define PWM_IMX_TPM_SC 0x10
> > +#define PWM_IMX_TPM_CNT 0x14
> > +#define PWM_IMX_TPM_MOD 0x18
> > +#define PWM_IMX_TPM_C0SC 0x20
> > +#define PWM_IMX_TPM_C0V 0x24
> > +
> > +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7,
> 0)
> > +
> > +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0)
> > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
> > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
> > +#define PWM_IMX_TPM_SC_CPWMS BIT(5)
> > +
> > +#define PWM_IMX_TPM_CnSC_CHF BIT(7)
> > +#define PWM_IMX_TPM_CnSC_MSB BIT(5)
> > +#define PWM_IMX_TPM_CnSC_MSA BIT(4)
> > +#define PWM_IMX_TPM_CnSC_ELSB BIT(3)
> > +#define PWM_IMX_TPM_CnSC_ELSA BIT(2)
> > +
> > +#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
> > +
> > +#define PWM_IMX_TPM_MAX_COUNT 0xffff
> > +
> > +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> > +
> > +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + (n) * 0x8)
> > +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + (n) * 0x8)
>
> You never use the C0SC and C0V registers other than as the base of these
> indexed registers. I think it makes more sense to replace the C0SC and C0V
> register definitions above with these.
OK.
>
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + struct mutex lock;
> > + u32 user_count;
> > + u32 enable_count;
> > + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM];
> > + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM];
>
> I suggest you use per PWM data. That way you can attach the data directly
> per PWM channel instead of having to carry the array here.
OK, I will try it.
>
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) \
> > + container_of(_chip, struct imx_tpm_pwm_chip, chip)
>
> I'd prefer this to be a static inline function.
>
> > +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 period_cnt, val, div, saved_cmod;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val <= PWM_IMX_TPM_MAX_COUNT)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val /
> > + (PWM_IMX_TPM_MAX_COUNT + 1)));
>
> Maybe use some temporary variables to make this easier to read.
OK.
>
> > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, div))) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* make sure counter is disabled for programming prescale */
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > + if (saved_cmod) {
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > +
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val &= ~PWM_IMX_TPM_SC_PS;
> > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, div);
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +
> > + /*
> > + * set period counter: according to RM, the MOD register is
> > + * updated immediately after CMOD[1:0] = 2b'00 above
> > + */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = (tmp + ((1 << div) >> 1)) >> div;
> > + if (period_cnt > PWM_IMX_TPM_MOD_MOD) {
> > + dev_err(chip->dev,
> > + "failed to find valid period count!\n");
> > + return -EINVAL;
> > + }
>
> I think you should move the computations and the check to an earlier point
> so that you avoid erroring out after you've already written some of the
> values. Part of the idea of the atomic API is that it allows you to check a
> configuration for validity before you write any registers so that you either
> apply a working configuration or none at all. With the above you could easily
> end up halfway through the configuration and leave the PWM in a non-
> working state.
OK.
>
> > + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD);
> > +
> > + /* restore the clock mode if necessary */
> > + if (saved_cmod) {
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + u32 period,
> > + u32 duty_cycle,
> > + enum pwm_polarity polarity) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 duty_cnt, val;
> > + u64 tmp;
> > +
> > + /* set duty counter */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> PWM_IMX_TPM_MOD_MOD;
> > + tmp *= duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > +
> > + /*
> > + * set polarity (for edge-aligned PWM modes)
> > + *
> > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > + * 0 10 10 PWM High-true pulse
> > + * 0 10 00 PWM Reserved
> > + * 0 10 01 PWM Low-true pulse
> > + * 0 10 11 PWM Reserved
> > + *
> > + * High-true pulse: clear output on counter match, set output on
> > + * counter reload, set output when counter first enabled or paused.
> > + *
> > + * Low-true pulse: set output on counter match, clear output on
> > + * counter reload, clear output when counter first enabled or paused.
> > + */
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> |
> > + PWM_IMX_TPM_CnSC_MSA);
> > + val |= PWM_IMX_TPM_CnSC_MSB;
> > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > + /*
> > + * polarity settings will enabled/disable output status
> > + * immediately, so here ONLY save the config and write
> > + * it into register when channel is enabled/disabled.
> > + */
> > + tpm->chn_config[pwm->hwpwm] = val;
> > +}
> > +
> > +/*
> > + * When a channel's polarity is configured, the polarity settings
> > + * will be saved and ONLY write into the register when the channel
> > + * is enabled.
> > + *
> > + * When a channel is disabled, its polarity settings will be saved
> > + * and its output will be disabled by clearing polarity settings.
> > + *
> > + * when a channel is enabled, its polarity settings will be restored
>
> "when" -> "When".
Will fix it.
>
> > + * and output will be enabled again.
> > + */
> > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + bool enable)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 val;
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + if (enable) {
> > + /* restore channel config */
> > + writel(tpm->chn_config[pwm->hwpwm],
> > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > +
> > + if (++tpm->enable_count == 1) {
> > + /* start TPM counter */
> > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + } else {
> > + /* disable channel */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> PWM_IMX_TPM_CnSC_MSB |
> > + PWM_IMX_TPM_CnSC_ELSB |
> PWM_IMX_TPM_CnSC_ELSA);
> > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +
> > + if (--tpm->enable_count == 0) {
> > + /* stop TPM counter since all channels are disabled
> */
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + }
> > +
> > + /* update channel status */
> > + tpm->chn_status[pwm->hwpwm] = enable; }
> > +
> > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u64 tmp;
> > + u32 val, rate;
> > +
> > + /* get period */
> > + rate = clk_get_rate(tpm->clk);
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val &= PWM_IMX_TPM_SC_PS;
> > + tmp *= (1 << val) * NSEC_PER_SEC;
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + /* get duty cycle */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > + tmp *= (1 << val) * NSEC_PER_SEC;
> > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + /* get polarity */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + else
> > + state->polarity = PWM_POLARITY_NORMAL;
> > +
> > + /* get channel status */
> > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > +
> > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + struct pwm_state curstate;
> > + int ret;
> > +
> > + mutex_lock(&tpm->lock);
> > +
> > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > +
> > + if (state->period != curstate.period) {
> > + /*
> > + * TPM counter is shared by multiple channels, so
> > + * prescale and period can NOT be modified when
> > + * there are multiple channels in use.
> > + */
> > + if (tpm->user_count != 1)
> > + return -EBUSY;
> > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (state->enabled == false) {
> > + /*
> > + * if eventually the PWM output is LOW, either
> > + * duty cycle is 0 or status is disabled, need
> > + * to make sure the output pin is LOW.
> > + */
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + 0, state->polarity);
> > + if (curstate.enabled)
> > + pwm_imx_tpm_enable(chip, pwm, false);
> > + } else {
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + state->duty_cycle, state->polarity);
> > + if (!curstate.enabled)
> > + pwm_imx_tpm_enable(chip, pwm, true);
>
> Doesn't this mean that you won't be applying changes to the polarity while a
> PWM is enabled? That seems wrong. Granted, you may usually not run into
> that, but if you can't support it I think you should at least return an error if
> you detect that the user wants to change polarity while the PWM is enabled.
I thought below function call already set the polarity? No matter its status is enabled
or disabled, the polarity setting will be always applied.
pwm_imx_tpm_config(chip, pwm, state->period,
state->duty_cycle, state->polarity);
>
> > + }
> > +
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct
> > +pwm_device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +
> > + mutex_lock(&tpm->lock);
> > + tpm->user_count++;
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct
> pwm_device
> > +*dev) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +
> > + mutex_lock(&tpm->lock);
> > + tpm->user_count--;
> > + mutex_unlock(&tpm->lock);
> > +}
> > +
> > +static const struct pwm_ops imx_tpm_pwm_ops = {
> > + .request = pwm_imx_tpm_request,
> > + .free = pwm_imx_tpm_free,
> > + .get_state = pwm_imx_tpm_get_state,
> > + .apply = pwm_imx_tpm_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int pwm_imx_tpm_probe(struct platform_device *pdev) {
> > + struct imx_tpm_pwm_chip *tpm;
> > + int ret;
> > + u32 val;
> > +
> > + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
> > + if (!tpm)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, tpm);
> > +
> > + tpm->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(tpm->base)) {
> > + ret = PTR_ERR(tpm->base);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "pwm ioremap failed %d\n",
> ret);
>
> I don't think devm_platform_ioremap_resource() will ever return -
> EPROBE_DEFER. Also, there's no need to print an error here since
> devm_ioremap_resource() prints one for any possible error condition
> anyway.
OK.
>
> > + return ret;
> > + }
> > +
> > + tpm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(tpm->clk)) {
> > + ret = PTR_ERR(tpm->clk);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "failed to get pwm clk %d\n",
> ret);
>
> I think it's better to clarify that you're outputting an error (rather than a clock
> index) by separating the error code with a ':'. Also these error messages are
> easier to read if you spell out words: "clk" -> "clock". Also: "pwm" -> "PWM".
>
OK.
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(tpm->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "failed to prepare or enable clk %d\n", ret);
>
> Same comments as above.
OK.
>
> > + return ret;
> > + }
> > +
> > + tpm->chip.dev = &pdev->dev;
> > + tpm->chip.ops = &imx_tpm_pwm_ops;
> > + tpm->chip.base = -1;
> > + /* get channel number */
> > + val = readl(tpm->base + PWM_IMX_TPM_PARAM);
> > + tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
>
> The comment is misleading, better would be "get number of channels".
> Also, please use blank lines to increase readability. You could leave a blank
> line before the comment, for example.
OK.
>
> > +
> > + mutex_init(&tpm->lock);
> > +
> > + ret = pwmchip_add(&tpm->chip);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
>
> "failed to add PWM chip: %d"
OK.
>
> > + clk_disable_unprepare(tpm->clk);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int pwm_imx_tpm_remove(struct platform_device *pdev) {
> > + struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
> > + int ret = pwmchip_remove(&tpm->chip);
> > +
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > +
> > + if (tpm->enable_count == 0)
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused pwm_imx_tpm_resume(struct device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (tpm->enable_count == 0) {
> > + ret = clk_prepare_enable(tpm->clk);
> > + if (ret)
> > + dev_err(dev,
> > + "failed to prepare or enable clk %d\n",
>
> "failed to prepare or enable clock: %d"
OK.
>
> > + ret);
> > + }
> > +
> > + return ret;
> > +};
>
> Your handling of the clock seems strange here. Basically in the above you
> always keep the clock on and you only disable it if there are no users and
> you're going to suspend.
>
> Why do you need to keep an extra reference count anyway? Or why keep the
> clock on all the time? Can't you just do a clk_prepare_enable() every time
> somebody enables the PWM? The CCF already has built-in reference
> counting, so I'm not sure you really need to duplicate that here.
Keeping clock always ON since driver probe is because, many TMP registers'
write needs clock to be ON for sync into register hardware, just enable the clock
before writing register and disable the clock immediately does NOT work, unless
we keep reading the register value until the register value is what we want to write,
but that makes code much more complicated, and the PWM clock normally is from
OSC which does NOT consume too much power, so I keep the clock always on and ONLY
disable it after suspend.
Anson.
>
> Thierry
>
> > +
> > +static SIMPLE_DEV_PM_OPS(imx_tpm_pwm_pm,
> > + pwm_imx_tpm_suspend, pwm_imx_tpm_resume);
> > +
> > +static const struct of_device_id imx_tpm_pwm_dt_ids[] = {
> > + { .compatible = "fsl,imx-tpm-pwm", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_tpm_pwm_dt_ids);
> > +
> > +static struct platform_driver imx_tpm_pwm_driver = {
> > + .driver = {
> > + .name = "imx-tpm-pwm",
> > + .of_match_table = imx_tpm_pwm_dt_ids,
> > + .pm = &imx_tpm_pwm_pm,
> > + },
> > + .probe = pwm_imx_tpm_probe,
> > + .remove = pwm_imx_tpm_remove,
> > +};
> > +module_platform_driver(imx_tpm_pwm_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("i.MX TPM PWM Driver"); MODULE_LICENSE("GPL
> v2");
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-18 11:33 ` Anson Huang
0 siblings, 0 replies; 41+ messages in thread
From: Anson Huang @ 2019-03-18 11:33 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree,
shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel,
robh+dt, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez,
festevam, linux-arm-kernel
Hi, Thierry
Best Regards!
Anson Huang
> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: 2019年3月18日 18:28
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
>
> I think this could be improved. You're basically restating the subject here, but
> the commit message gives you a good place to provide more information
> about the capabilities and restrictions. For example if this supports polarity,
> mention it in the commit message.
>
> Something that also looks very interesting is how this supports detection of
> the number of channels, so make sure to mention that as well.
>
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V4:
> > - improve register read/write using bit field operations;
> > - correct some logic issue;
> > - ONLY disable clock when PWM is NOT in use during suspend;
> > - add some comments for PWM mode settings;
> > - fix some spelling errors;
> > - reading channel number from register instead of using fix value.
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 436
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 448 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 54f8238..3ea0391 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -210,6 +210,17 @@ config PWM_IMX27
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx27.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + depends on HAVE_CLK && HAS_IOMEM
> > + help
> > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's
> full
> > + name is Low Power Timer/Pulse Width Modulation Module.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-imx-tpm.
> > +
> > config PWM_JZ4740
> > tristate "Ingenic JZ47xx PWM support"
> > depends on MACH_INGENIC
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 448825e..c368599 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-
> hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new
> > file mode 100644 index 0000000..12cb16c
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,436 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + *
> > + * Limitations:
> > + * - The TPM counter and period counter are shared between
> > + * multiple channels, so all channels should use same period
> > + * settings.
>
> This sounds like something that you may want to also highlight in the device
> tree bindings.
OK.
>
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +#define PWM_IMX_TPM_PARAM 0x4
> > +#define PWM_IMX_TPM_GLOBAL 0x8
> > +#define PWM_IMX_TPM_SC 0x10
> > +#define PWM_IMX_TPM_CNT 0x14
> > +#define PWM_IMX_TPM_MOD 0x18
> > +#define PWM_IMX_TPM_C0SC 0x20
> > +#define PWM_IMX_TPM_C0V 0x24
> > +
> > +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7,
> 0)
> > +
> > +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0)
> > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
> > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
> > +#define PWM_IMX_TPM_SC_CPWMS BIT(5)
> > +
> > +#define PWM_IMX_TPM_CnSC_CHF BIT(7)
> > +#define PWM_IMX_TPM_CnSC_MSB BIT(5)
> > +#define PWM_IMX_TPM_CnSC_MSA BIT(4)
> > +#define PWM_IMX_TPM_CnSC_ELSB BIT(3)
> > +#define PWM_IMX_TPM_CnSC_ELSA BIT(2)
> > +
> > +#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
> > +
> > +#define PWM_IMX_TPM_MAX_COUNT 0xffff
> > +
> > +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> > +
> > +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + (n) * 0x8)
> > +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + (n) * 0x8)
>
> You never use the C0SC and C0V registers other than as the base of these
> indexed registers. I think it makes more sense to replace the C0SC and C0V
> register definitions above with these.
OK.
>
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + struct mutex lock;
> > + u32 user_count;
> > + u32 enable_count;
> > + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM];
> > + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM];
>
> I suggest you use per PWM data. That way you can attach the data directly
> per PWM channel instead of having to carry the array here.
OK, I will try it.
>
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) \
> > + container_of(_chip, struct imx_tpm_pwm_chip, chip)
>
> I'd prefer this to be a static inline function.
>
> > +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 period_cnt, val, div, saved_cmod;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val <= PWM_IMX_TPM_MAX_COUNT)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val /
> > + (PWM_IMX_TPM_MAX_COUNT + 1)));
>
> Maybe use some temporary variables to make this easier to read.
OK.
>
> > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, div))) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* make sure counter is disabled for programming prescale */
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > + if (saved_cmod) {
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > +
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val &= ~PWM_IMX_TPM_SC_PS;
> > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, div);
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +
> > + /*
> > + * set period counter: according to RM, the MOD register is
> > + * updated immediately after CMOD[1:0] = 2b'00 above
> > + */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = (tmp + ((1 << div) >> 1)) >> div;
> > + if (period_cnt > PWM_IMX_TPM_MOD_MOD) {
> > + dev_err(chip->dev,
> > + "failed to find valid period count!\n");
> > + return -EINVAL;
> > + }
>
> I think you should move the computations and the check to an earlier point
> so that you avoid erroring out after you've already written some of the
> values. Part of the idea of the atomic API is that it allows you to check a
> configuration for validity before you write any registers so that you either
> apply a working configuration or none at all. With the above you could easily
> end up halfway through the configuration and leave the PWM in a non-
> working state.
OK.
>
> > + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD);
> > +
> > + /* restore the clock mode if necessary */
> > + if (saved_cmod) {
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + u32 period,
> > + u32 duty_cycle,
> > + enum pwm_polarity polarity) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 duty_cnt, val;
> > + u64 tmp;
> > +
> > + /* set duty counter */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> PWM_IMX_TPM_MOD_MOD;
> > + tmp *= duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > +
> > + /*
> > + * set polarity (for edge-aligned PWM modes)
> > + *
> > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > + * 0 10 10 PWM High-true pulse
> > + * 0 10 00 PWM Reserved
> > + * 0 10 01 PWM Low-true pulse
> > + * 0 10 11 PWM Reserved
> > + *
> > + * High-true pulse: clear output on counter match, set output on
> > + * counter reload, set output when counter first enabled or paused.
> > + *
> > + * Low-true pulse: set output on counter match, clear output on
> > + * counter reload, clear output when counter first enabled or paused.
> > + */
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> |
> > + PWM_IMX_TPM_CnSC_MSA);
> > + val |= PWM_IMX_TPM_CnSC_MSB;
> > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > + /*
> > + * polarity settings will enabled/disable output status
> > + * immediately, so here ONLY save the config and write
> > + * it into register when channel is enabled/disabled.
> > + */
> > + tpm->chn_config[pwm->hwpwm] = val;
> > +}
> > +
> > +/*
> > + * When a channel's polarity is configured, the polarity settings
> > + * will be saved and ONLY write into the register when the channel
> > + * is enabled.
> > + *
> > + * When a channel is disabled, its polarity settings will be saved
> > + * and its output will be disabled by clearing polarity settings.
> > + *
> > + * when a channel is enabled, its polarity settings will be restored
>
> "when" -> "When".
Will fix it.
>
> > + * and output will be enabled again.
> > + */
> > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + bool enable)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 val;
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + if (enable) {
> > + /* restore channel config */
> > + writel(tpm->chn_config[pwm->hwpwm],
> > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > +
> > + if (++tpm->enable_count == 1) {
> > + /* start TPM counter */
> > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + } else {
> > + /* disable channel */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> PWM_IMX_TPM_CnSC_MSB |
> > + PWM_IMX_TPM_CnSC_ELSB |
> PWM_IMX_TPM_CnSC_ELSA);
> > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +
> > + if (--tpm->enable_count == 0) {
> > + /* stop TPM counter since all channels are disabled
> */
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + }
> > +
> > + /* update channel status */
> > + tpm->chn_status[pwm->hwpwm] = enable; }
> > +
> > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u64 tmp;
> > + u32 val, rate;
> > +
> > + /* get period */
> > + rate = clk_get_rate(tpm->clk);
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val &= PWM_IMX_TPM_SC_PS;
> > + tmp *= (1 << val) * NSEC_PER_SEC;
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + /* get duty cycle */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > + tmp *= (1 << val) * NSEC_PER_SEC;
> > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + /* get polarity */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + else
> > + state->polarity = PWM_POLARITY_NORMAL;
> > +
> > + /* get channel status */
> > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > +
> > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + struct pwm_state curstate;
> > + int ret;
> > +
> > + mutex_lock(&tpm->lock);
> > +
> > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > +
> > + if (state->period != curstate.period) {
> > + /*
> > + * TPM counter is shared by multiple channels, so
> > + * prescale and period can NOT be modified when
> > + * there are multiple channels in use.
> > + */
> > + if (tpm->user_count != 1)
> > + return -EBUSY;
> > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (state->enabled == false) {
> > + /*
> > + * if eventually the PWM output is LOW, either
> > + * duty cycle is 0 or status is disabled, need
> > + * to make sure the output pin is LOW.
> > + */
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + 0, state->polarity);
> > + if (curstate.enabled)
> > + pwm_imx_tpm_enable(chip, pwm, false);
> > + } else {
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + state->duty_cycle, state->polarity);
> > + if (!curstate.enabled)
> > + pwm_imx_tpm_enable(chip, pwm, true);
>
> Doesn't this mean that you won't be applying changes to the polarity while a
> PWM is enabled? That seems wrong. Granted, you may usually not run into
> that, but if you can't support it I think you should at least return an error if
> you detect that the user wants to change polarity while the PWM is enabled.
I thought below function call already set the polarity? No matter its status is enabled
or disabled, the polarity setting will be always applied.
pwm_imx_tpm_config(chip, pwm, state->period,
state->duty_cycle, state->polarity);
>
> > + }
> > +
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct
> > +pwm_device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +
> > + mutex_lock(&tpm->lock);
> > + tpm->user_count++;
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct
> pwm_device
> > +*dev) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +
> > + mutex_lock(&tpm->lock);
> > + tpm->user_count--;
> > + mutex_unlock(&tpm->lock);
> > +}
> > +
> > +static const struct pwm_ops imx_tpm_pwm_ops = {
> > + .request = pwm_imx_tpm_request,
> > + .free = pwm_imx_tpm_free,
> > + .get_state = pwm_imx_tpm_get_state,
> > + .apply = pwm_imx_tpm_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int pwm_imx_tpm_probe(struct platform_device *pdev) {
> > + struct imx_tpm_pwm_chip *tpm;
> > + int ret;
> > + u32 val;
> > +
> > + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
> > + if (!tpm)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, tpm);
> > +
> > + tpm->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(tpm->base)) {
> > + ret = PTR_ERR(tpm->base);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "pwm ioremap failed %d\n",
> ret);
>
> I don't think devm_platform_ioremap_resource() will ever return -
> EPROBE_DEFER. Also, there's no need to print an error here since
> devm_ioremap_resource() prints one for any possible error condition
> anyway.
OK.
>
> > + return ret;
> > + }
> > +
> > + tpm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(tpm->clk)) {
> > + ret = PTR_ERR(tpm->clk);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "failed to get pwm clk %d\n",
> ret);
>
> I think it's better to clarify that you're outputting an error (rather than a clock
> index) by separating the error code with a ':'. Also these error messages are
> easier to read if you spell out words: "clk" -> "clock". Also: "pwm" -> "PWM".
>
OK.
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(tpm->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "failed to prepare or enable clk %d\n", ret);
>
> Same comments as above.
OK.
>
> > + return ret;
> > + }
> > +
> > + tpm->chip.dev = &pdev->dev;
> > + tpm->chip.ops = &imx_tpm_pwm_ops;
> > + tpm->chip.base = -1;
> > + /* get channel number */
> > + val = readl(tpm->base + PWM_IMX_TPM_PARAM);
> > + tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
>
> The comment is misleading, better would be "get number of channels".
> Also, please use blank lines to increase readability. You could leave a blank
> line before the comment, for example.
OK.
>
> > +
> > + mutex_init(&tpm->lock);
> > +
> > + ret = pwmchip_add(&tpm->chip);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
>
> "failed to add PWM chip: %d"
OK.
>
> > + clk_disable_unprepare(tpm->clk);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int pwm_imx_tpm_remove(struct platform_device *pdev) {
> > + struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
> > + int ret = pwmchip_remove(&tpm->chip);
> > +
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > +
> > + if (tpm->enable_count == 0)
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused pwm_imx_tpm_resume(struct device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (tpm->enable_count == 0) {
> > + ret = clk_prepare_enable(tpm->clk);
> > + if (ret)
> > + dev_err(dev,
> > + "failed to prepare or enable clk %d\n",
>
> "failed to prepare or enable clock: %d"
OK.
>
> > + ret);
> > + }
> > +
> > + return ret;
> > +};
>
> Your handling of the clock seems strange here. Basically in the above you
> always keep the clock on and you only disable it if there are no users and
> you're going to suspend.
>
> Why do you need to keep an extra reference count anyway? Or why keep the
> clock on all the time? Can't you just do a clk_prepare_enable() every time
> somebody enables the PWM? The CCF already has built-in reference
> counting, so I'm not sure you really need to duplicate that here.
Keeping clock always ON since driver probe is because, many TMP registers'
write needs clock to be ON for sync into register hardware, just enable the clock
before writing register and disable the clock immediately does NOT work, unless
we keep reading the register value until the register value is what we want to write,
but that makes code much more complicated, and the PWM clock normally is from
OSC which does NOT consume too much power, so I keep the clock always on and ONLY
disable it after suspend.
Anson.
>
> Thierry
>
> > +
> > +static SIMPLE_DEV_PM_OPS(imx_tpm_pwm_pm,
> > + pwm_imx_tpm_suspend, pwm_imx_tpm_resume);
> > +
> > +static const struct of_device_id imx_tpm_pwm_dt_ids[] = {
> > + { .compatible = "fsl,imx-tpm-pwm", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_tpm_pwm_dt_ids);
> > +
> > +static struct platform_driver imx_tpm_pwm_driver = {
> > + .driver = {
> > + .name = "imx-tpm-pwm",
> > + .of_match_table = imx_tpm_pwm_dt_ids,
> > + .pm = &imx_tpm_pwm_pm,
> > + },
> > + .probe = pwm_imx_tpm_probe,
> > + .remove = pwm_imx_tpm_remove,
> > +};
> > +module_platform_driver(imx_tpm_pwm_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("i.MX TPM PWM Driver"); MODULE_LICENSE("GPL
> v2");
> > --
> > 2.7.4
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-18 11:33 ` Anson Huang
0 siblings, 0 replies; 41+ messages in thread
From: Anson Huang @ 2019-03-18 11:33 UTC (permalink / raw)
To: Thierry Reding
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel
Hi, Thierry
Best Regards!
Anson Huang
> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: 2019年3月18日 18:28
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
>
> I think this could be improved. You're basically restating the subject here, but
> the commit message gives you a good place to provide more information
> about the capabilities and restrictions. For example if this supports polarity,
> mention it in the commit message.
>
> Something that also looks very interesting is how this supports detection of
> the number of channels, so make sure to mention that as well.
>
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V4:
> > - improve register read/write using bit field operations;
> > - correct some logic issue;
> > - ONLY disable clock when PWM is NOT in use during suspend;
> > - add some comments for PWM mode settings;
> > - fix some spelling errors;
> > - reading channel number from register instead of using fix value.
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 436
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 448 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 54f8238..3ea0391 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -210,6 +210,17 @@ config PWM_IMX27
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx27.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + depends on HAVE_CLK && HAS_IOMEM
> > + help
> > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's
> full
> > + name is Low Power Timer/Pulse Width Modulation Module.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-imx-tpm.
> > +
> > config PWM_JZ4740
> > tristate "Ingenic JZ47xx PWM support"
> > depends on MACH_INGENIC
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 448825e..c368599 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-
> hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o
> > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o
> > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new
> > file mode 100644 index 0000000..12cb16c
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,436 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + *
> > + * Limitations:
> > + * - The TPM counter and period counter are shared between
> > + * multiple channels, so all channels should use same period
> > + * settings.
>
> This sounds like something that you may want to also highlight in the device
> tree bindings.
OK.
>
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +
> > +#define PWM_IMX_TPM_PARAM 0x4
> > +#define PWM_IMX_TPM_GLOBAL 0x8
> > +#define PWM_IMX_TPM_SC 0x10
> > +#define PWM_IMX_TPM_CNT 0x14
> > +#define PWM_IMX_TPM_MOD 0x18
> > +#define PWM_IMX_TPM_C0SC 0x20
> > +#define PWM_IMX_TPM_C0V 0x24
> > +
> > +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7,
> 0)
> > +
> > +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0)
> > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
> > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
> > +#define PWM_IMX_TPM_SC_CPWMS BIT(5)
> > +
> > +#define PWM_IMX_TPM_CnSC_CHF BIT(7)
> > +#define PWM_IMX_TPM_CnSC_MSB BIT(5)
> > +#define PWM_IMX_TPM_CnSC_MSA BIT(4)
> > +#define PWM_IMX_TPM_CnSC_ELSB BIT(3)
> > +#define PWM_IMX_TPM_CnSC_ELSA BIT(2)
> > +
> > +#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
> > +
> > +#define PWM_IMX_TPM_MAX_COUNT 0xffff
> > +
> > +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> > +
> > +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + (n) * 0x8)
> > +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + (n) * 0x8)
>
> You never use the C0SC and C0V registers other than as the base of these
> indexed registers. I think it makes more sense to replace the C0SC and C0V
> register definitions above with these.
OK.
>
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + struct mutex lock;
> > + u32 user_count;
> > + u32 enable_count;
> > + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM];
> > + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM];
>
> I suggest you use per PWM data. That way you can attach the data directly
> per PWM channel instead of having to carry the array here.
OK, I will try it.
>
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) \
> > + container_of(_chip, struct imx_tpm_pwm_chip, chip)
>
> I'd prefer this to be a static inline function.
>
> > +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 period_cnt, val, div, saved_cmod;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val <= PWM_IMX_TPM_MAX_COUNT)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val /
> > + (PWM_IMX_TPM_MAX_COUNT + 1)));
>
> Maybe use some temporary variables to make this easier to read.
OK.
>
> > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, div))) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* make sure counter is disabled for programming prescale */
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > + if (saved_cmod) {
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > +
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val &= ~PWM_IMX_TPM_SC_PS;
> > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, div);
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +
> > + /*
> > + * set period counter: according to RM, the MOD register is
> > + * updated immediately after CMOD[1:0] = 2b'00 above
> > + */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = (tmp + ((1 << div) >> 1)) >> div;
> > + if (period_cnt > PWM_IMX_TPM_MOD_MOD) {
> > + dev_err(chip->dev,
> > + "failed to find valid period count!\n");
> > + return -EINVAL;
> > + }
>
> I think you should move the computations and the check to an earlier point
> so that you avoid erroring out after you've already written some of the
> values. Part of the idea of the atomic API is that it allows you to check a
> configuration for validity before you write any registers so that you either
> apply a working configuration or none at all. With the above you could easily
> end up halfway through the configuration and leave the PWM in a non-
> working state.
OK.
>
> > + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD);
> > +
> > + /* restore the clock mode if necessary */
> > + if (saved_cmod) {
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + u32 period,
> > + u32 duty_cycle,
> > + enum pwm_polarity polarity) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 duty_cnt, val;
> > + u64 tmp;
> > +
> > + /* set duty counter */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> PWM_IMX_TPM_MOD_MOD;
> > + tmp *= duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > +
> > + /*
> > + * set polarity (for edge-aligned PWM modes)
> > + *
> > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > + * 0 10 10 PWM High-true pulse
> > + * 0 10 00 PWM Reserved
> > + * 0 10 01 PWM Low-true pulse
> > + * 0 10 11 PWM Reserved
> > + *
> > + * High-true pulse: clear output on counter match, set output on
> > + * counter reload, set output when counter first enabled or paused.
> > + *
> > + * Low-true pulse: set output on counter match, clear output on
> > + * counter reload, clear output when counter first enabled or paused.
> > + */
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> |
> > + PWM_IMX_TPM_CnSC_MSA);
> > + val |= PWM_IMX_TPM_CnSC_MSB;
> > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > + /*
> > + * polarity settings will enabled/disable output status
> > + * immediately, so here ONLY save the config and write
> > + * it into register when channel is enabled/disabled.
> > + */
> > + tpm->chn_config[pwm->hwpwm] = val;
> > +}
> > +
> > +/*
> > + * When a channel's polarity is configured, the polarity settings
> > + * will be saved and ONLY write into the register when the channel
> > + * is enabled.
> > + *
> > + * When a channel is disabled, its polarity settings will be saved
> > + * and its output will be disabled by clearing polarity settings.
> > + *
> > + * when a channel is enabled, its polarity settings will be restored
>
> "when" -> "When".
Will fix it.
>
> > + * and output will be enabled again.
> > + */
> > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + bool enable)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 val;
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + if (enable) {
> > + /* restore channel config */
> > + writel(tpm->chn_config[pwm->hwpwm],
> > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > +
> > + if (++tpm->enable_count == 1) {
> > + /* start TPM counter */
> > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + } else {
> > + /* disable channel */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> PWM_IMX_TPM_CnSC_MSB |
> > + PWM_IMX_TPM_CnSC_ELSB |
> PWM_IMX_TPM_CnSC_ELSA);
> > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +
> > + if (--tpm->enable_count == 0) {
> > + /* stop TPM counter since all channels are disabled
> */
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + }
> > +
> > + /* update channel status */
> > + tpm->chn_status[pwm->hwpwm] = enable; }
> > +
> > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u64 tmp;
> > + u32 val, rate;
> > +
> > + /* get period */
> > + rate = clk_get_rate(tpm->clk);
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + val &= PWM_IMX_TPM_SC_PS;
> > + tmp *= (1 << val) * NSEC_PER_SEC;
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + /* get duty cycle */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > + tmp *= (1 << val) * NSEC_PER_SEC;
> > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + /* get polarity */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + else
> > + state->polarity = PWM_POLARITY_NORMAL;
> > +
> > + /* get channel status */
> > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > +
> > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + struct pwm_state curstate;
> > + int ret;
> > +
> > + mutex_lock(&tpm->lock);
> > +
> > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > +
> > + if (state->period != curstate.period) {
> > + /*
> > + * TPM counter is shared by multiple channels, so
> > + * prescale and period can NOT be modified when
> > + * there are multiple channels in use.
> > + */
> > + if (tpm->user_count != 1)
> > + return -EBUSY;
> > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (state->enabled == false) {
> > + /*
> > + * if eventually the PWM output is LOW, either
> > + * duty cycle is 0 or status is disabled, need
> > + * to make sure the output pin is LOW.
> > + */
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + 0, state->polarity);
> > + if (curstate.enabled)
> > + pwm_imx_tpm_enable(chip, pwm, false);
> > + } else {
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + state->duty_cycle, state->polarity);
> > + if (!curstate.enabled)
> > + pwm_imx_tpm_enable(chip, pwm, true);
>
> Doesn't this mean that you won't be applying changes to the polarity while a
> PWM is enabled? That seems wrong. Granted, you may usually not run into
> that, but if you can't support it I think you should at least return an error if
> you detect that the user wants to change polarity while the PWM is enabled.
I thought below function call already set the polarity? No matter its status is enabled
or disabled, the polarity setting will be always applied.
pwm_imx_tpm_config(chip, pwm, state->period,
state->duty_cycle, state->polarity);
>
> > + }
> > +
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct
> > +pwm_device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +
> > + mutex_lock(&tpm->lock);
> > + tpm->user_count++;
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct
> pwm_device
> > +*dev) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +
> > + mutex_lock(&tpm->lock);
> > + tpm->user_count--;
> > + mutex_unlock(&tpm->lock);
> > +}
> > +
> > +static const struct pwm_ops imx_tpm_pwm_ops = {
> > + .request = pwm_imx_tpm_request,
> > + .free = pwm_imx_tpm_free,
> > + .get_state = pwm_imx_tpm_get_state,
> > + .apply = pwm_imx_tpm_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int pwm_imx_tpm_probe(struct platform_device *pdev) {
> > + struct imx_tpm_pwm_chip *tpm;
> > + int ret;
> > + u32 val;
> > +
> > + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
> > + if (!tpm)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, tpm);
> > +
> > + tpm->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(tpm->base)) {
> > + ret = PTR_ERR(tpm->base);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "pwm ioremap failed %d\n",
> ret);
>
> I don't think devm_platform_ioremap_resource() will ever return -
> EPROBE_DEFER. Also, there's no need to print an error here since
> devm_ioremap_resource() prints one for any possible error condition
> anyway.
OK.
>
> > + return ret;
> > + }
> > +
> > + tpm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(tpm->clk)) {
> > + ret = PTR_ERR(tpm->clk);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "failed to get pwm clk %d\n",
> ret);
>
> I think it's better to clarify that you're outputting an error (rather than a clock
> index) by separating the error code with a ':'. Also these error messages are
> easier to read if you spell out words: "clk" -> "clock". Also: "pwm" -> "PWM".
>
OK.
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(tpm->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "failed to prepare or enable clk %d\n", ret);
>
> Same comments as above.
OK.
>
> > + return ret;
> > + }
> > +
> > + tpm->chip.dev = &pdev->dev;
> > + tpm->chip.ops = &imx_tpm_pwm_ops;
> > + tpm->chip.base = -1;
> > + /* get channel number */
> > + val = readl(tpm->base + PWM_IMX_TPM_PARAM);
> > + tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
>
> The comment is misleading, better would be "get number of channels".
> Also, please use blank lines to increase readability. You could leave a blank
> line before the comment, for example.
OK.
>
> > +
> > + mutex_init(&tpm->lock);
> > +
> > + ret = pwmchip_add(&tpm->chip);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret);
>
> "failed to add PWM chip: %d"
OK.
>
> > + clk_disable_unprepare(tpm->clk);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int pwm_imx_tpm_remove(struct platform_device *pdev) {
> > + struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
> > + int ret = pwmchip_remove(&tpm->chip);
> > +
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > +
> > + if (tpm->enable_count == 0)
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused pwm_imx_tpm_resume(struct device *dev) {
> > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (tpm->enable_count == 0) {
> > + ret = clk_prepare_enable(tpm->clk);
> > + if (ret)
> > + dev_err(dev,
> > + "failed to prepare or enable clk %d\n",
>
> "failed to prepare or enable clock: %d"
OK.
>
> > + ret);
> > + }
> > +
> > + return ret;
> > +};
>
> Your handling of the clock seems strange here. Basically in the above you
> always keep the clock on and you only disable it if there are no users and
> you're going to suspend.
>
> Why do you need to keep an extra reference count anyway? Or why keep the
> clock on all the time? Can't you just do a clk_prepare_enable() every time
> somebody enables the PWM? The CCF already has built-in reference
> counting, so I'm not sure you really need to duplicate that here.
Keeping clock always ON since driver probe is because, many TMP registers'
write needs clock to be ON for sync into register hardware, just enable the clock
before writing register and disable the clock immediately does NOT work, unless
we keep reading the register value until the register value is what we want to write,
but that makes code much more complicated, and the PWM clock normally is from
OSC which does NOT consume too much power, so I keep the clock always on and ONLY
disable it after suspend.
Anson.
>
> Thierry
>
> > +
> > +static SIMPLE_DEV_PM_OPS(imx_tpm_pwm_pm,
> > + pwm_imx_tpm_suspend, pwm_imx_tpm_resume);
> > +
> > +static const struct of_device_id imx_tpm_pwm_dt_ids[] = {
> > + { .compatible = "fsl,imx-tpm-pwm", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx_tpm_pwm_dt_ids);
> > +
> > +static struct platform_driver imx_tpm_pwm_driver = {
> > + .driver = {
> > + .name = "imx-tpm-pwm",
> > + .of_match_table = imx_tpm_pwm_dt_ids,
> > + .pm = &imx_tpm_pwm_pm,
> > + },
> > + .probe = pwm_imx_tpm_probe,
> > + .remove = pwm_imx_tpm_remove,
> > +};
> > +module_platform_driver(imx_tpm_pwm_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
> > +MODULE_DESCRIPTION("i.MX TPM PWM Driver"); MODULE_LICENSE("GPL
> v2");
> > --
> > 2.7.4
> >
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-18 11:33 ` Anson Huang
(?)
@ 2019-03-18 15:30 ` Thierry Reding
-1 siblings, 0 replies; 41+ messages in thread
From: Thierry Reding @ 2019-03-18 15:30 UTC (permalink / raw)
To: Anson Huang
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel,
u.kleine-koenig, dl-linux-imx
[-- Attachment #1: Type: text/plain, Size: 9844 bytes --]
On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote:
> Hi, Thierry
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Thierry Reding [mailto:thierry.reding@gmail.com]
> > Sent: 2019年3月18日 18:28
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> > Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> > <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
[...]
> > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + u32 period,
> > > + u32 duty_cycle,
> > > + enum pwm_polarity polarity) {
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u32 duty_cnt, val;
> > > + u64 tmp;
> > > +
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > PWM_IMX_TPM_MOD_MOD;
> > > + tmp *= duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +
> > > + /*
> > > + * set polarity (for edge-aligned PWM modes)
> > > + *
> > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > + * 0 10 10 PWM High-true pulse
> > > + * 0 10 00 PWM Reserved
> > > + * 0 10 01 PWM Low-true pulse
> > > + * 0 10 11 PWM Reserved
> > > + *
> > > + * High-true pulse: clear output on counter match, set output on
> > > + * counter reload, set output when counter first enabled or paused.
> > > + *
> > > + * Low-true pulse: set output on counter match, clear output on
> > > + * counter reload, clear output when counter first enabled or paused.
> > > + */
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> > |
> > > + PWM_IMX_TPM_CnSC_MSA);
> > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + tpm->chn_config[pwm->hwpwm] = val;
> > > +}
> > > +
> > > +/*
> > > + * When a channel's polarity is configured, the polarity settings
> > > + * will be saved and ONLY write into the register when the channel
> > > + * is enabled.
> > > + *
> > > + * When a channel is disabled, its polarity settings will be saved
> > > + * and its output will be disabled by clearing polarity settings.
> > > + *
> > > + * when a channel is enabled, its polarity settings will be restored
> >
> > "when" -> "When".
>
> Will fix it.
>
> >
> > > + * and output will be enabled again.
> > > + */
> > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + bool enable)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u32 val;
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + if (enable) {
> > > + /* restore channel config */
> > > + writel(tpm->chn_config[pwm->hwpwm],
> > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > +
> > > + if (++tpm->enable_count == 1) {
> > > + /* start TPM counter */
> > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > + }
> > > + } else {
> > > + /* disable channel */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> > PWM_IMX_TPM_CnSC_MSB |
> > > + PWM_IMX_TPM_CnSC_ELSB |
> > PWM_IMX_TPM_CnSC_ELSA);
> > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > +
> > > + if (--tpm->enable_count == 0) {
> > > + /* stop TPM counter since all channels are disabled
> > */
> > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > + }
> > > + }
> > > +
> > > + /* update channel status */
> > > + tpm->chn_status[pwm->hwpwm] = enable; }
> > > +
> > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u64 tmp;
> > > + u32 val, rate;
> > > +
> > > + /* get period */
> > > + rate = clk_get_rate(tpm->clk);
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + val &= PWM_IMX_TPM_SC_PS;
> > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > + /* get duty cycle */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > + /* get polarity */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + else
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + /* get channel status */
> > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > > +
> > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + struct pwm_state curstate;
> > > + int ret;
> > > +
> > > + mutex_lock(&tpm->lock);
> > > +
> > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > +
> > > + if (state->period != curstate.period) {
> > > + /*
> > > + * TPM counter is shared by multiple channels, so
> > > + * prescale and period can NOT be modified when
> > > + * there are multiple channels in use.
> > > + */
> > > + if (tpm->user_count != 1)
> > > + return -EBUSY;
> > > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + if (state->enabled == false) {
> > > + /*
> > > + * if eventually the PWM output is LOW, either
> > > + * duty cycle is 0 or status is disabled, need
> > > + * to make sure the output pin is LOW.
> > > + */
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + 0, state->polarity);
> > > + if (curstate.enabled)
> > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > + } else {
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + state->duty_cycle, state->polarity);
> > > + if (!curstate.enabled)
> > > + pwm_imx_tpm_enable(chip, pwm, true);
> >
> > Doesn't this mean that you won't be applying changes to the polarity while a
> > PWM is enabled? That seems wrong. Granted, you may usually not run into
> > that, but if you can't support it I think you should at least return an error if
> > you detect that the user wants to change polarity while the PWM is enabled.
>
> I thought below function call already set the polarity? No matter its status is enabled
> or disabled, the polarity setting will be always applied.
>
> pwm_imx_tpm_config(chip, pwm, state->period,
> state->duty_cycle, state->polarity);
That's not what it seems to do. In fact there's a comment that explains
why it doesn't do that. Quoting here:
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + tpm->chn_config[pwm->hwpwm] = val;
Looks to me like that only stores the value for that register so that it
can be applied at a later point. Or am I missing something?
> > > + ret);
> > > + }
> > > +
> > > + return ret;
> > > +};
> >
> > Your handling of the clock seems strange here. Basically in the above you
> > always keep the clock on and you only disable it if there are no users and
> > you're going to suspend.
> >
> > Why do you need to keep an extra reference count anyway? Or why keep the
> > clock on all the time? Can't you just do a clk_prepare_enable() every time
> > somebody enables the PWM? The CCF already has built-in reference
> > counting, so I'm not sure you really need to duplicate that here.
>
> Keeping clock always ON since driver probe is because, many TMP registers'
> write needs clock to be ON for sync into register hardware, just enable the clock
> before writing register and disable the clock immediately does NOT work, unless
> we keep reading the register value until the register value is what we want to write,
> but that makes code much more complicated, and the PWM clock normally is from
> OSC which does NOT consume too much power, so I keep the clock always on and ONLY
> disable it after suspend.
Why do you bother with keeping the enable reference count, then? Can't
you just enable the clock on probe, then on suspend disable it, enable
it again on resume and on remove you also disable it? Why does it need
to be dependent on whether there are any active PWMs or not?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-18 15:30 ` Thierry Reding
0 siblings, 0 replies; 41+ messages in thread
From: Thierry Reding @ 2019-03-18 15:30 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree,
shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel,
robh+dt, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez,
festevam, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 9844 bytes --]
On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote:
> Hi, Thierry
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Thierry Reding [mailto:thierry.reding@gmail.com]
> > Sent: 2019年3月18日 18:28
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> > Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> > <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
[...]
> > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + u32 period,
> > > + u32 duty_cycle,
> > > + enum pwm_polarity polarity) {
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u32 duty_cnt, val;
> > > + u64 tmp;
> > > +
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > PWM_IMX_TPM_MOD_MOD;
> > > + tmp *= duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +
> > > + /*
> > > + * set polarity (for edge-aligned PWM modes)
> > > + *
> > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > + * 0 10 10 PWM High-true pulse
> > > + * 0 10 00 PWM Reserved
> > > + * 0 10 01 PWM Low-true pulse
> > > + * 0 10 11 PWM Reserved
> > > + *
> > > + * High-true pulse: clear output on counter match, set output on
> > > + * counter reload, set output when counter first enabled or paused.
> > > + *
> > > + * Low-true pulse: set output on counter match, clear output on
> > > + * counter reload, clear output when counter first enabled or paused.
> > > + */
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> > |
> > > + PWM_IMX_TPM_CnSC_MSA);
> > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + tpm->chn_config[pwm->hwpwm] = val;
> > > +}
> > > +
> > > +/*
> > > + * When a channel's polarity is configured, the polarity settings
> > > + * will be saved and ONLY write into the register when the channel
> > > + * is enabled.
> > > + *
> > > + * When a channel is disabled, its polarity settings will be saved
> > > + * and its output will be disabled by clearing polarity settings.
> > > + *
> > > + * when a channel is enabled, its polarity settings will be restored
> >
> > "when" -> "When".
>
> Will fix it.
>
> >
> > > + * and output will be enabled again.
> > > + */
> > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + bool enable)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u32 val;
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + if (enable) {
> > > + /* restore channel config */
> > > + writel(tpm->chn_config[pwm->hwpwm],
> > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > +
> > > + if (++tpm->enable_count == 1) {
> > > + /* start TPM counter */
> > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > + }
> > > + } else {
> > > + /* disable channel */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> > PWM_IMX_TPM_CnSC_MSB |
> > > + PWM_IMX_TPM_CnSC_ELSB |
> > PWM_IMX_TPM_CnSC_ELSA);
> > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > +
> > > + if (--tpm->enable_count == 0) {
> > > + /* stop TPM counter since all channels are disabled
> > */
> > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > + }
> > > + }
> > > +
> > > + /* update channel status */
> > > + tpm->chn_status[pwm->hwpwm] = enable; }
> > > +
> > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u64 tmp;
> > > + u32 val, rate;
> > > +
> > > + /* get period */
> > > + rate = clk_get_rate(tpm->clk);
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + val &= PWM_IMX_TPM_SC_PS;
> > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > + /* get duty cycle */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > + /* get polarity */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + else
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + /* get channel status */
> > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > > +
> > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + struct pwm_state curstate;
> > > + int ret;
> > > +
> > > + mutex_lock(&tpm->lock);
> > > +
> > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > +
> > > + if (state->period != curstate.period) {
> > > + /*
> > > + * TPM counter is shared by multiple channels, so
> > > + * prescale and period can NOT be modified when
> > > + * there are multiple channels in use.
> > > + */
> > > + if (tpm->user_count != 1)
> > > + return -EBUSY;
> > > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + if (state->enabled == false) {
> > > + /*
> > > + * if eventually the PWM output is LOW, either
> > > + * duty cycle is 0 or status is disabled, need
> > > + * to make sure the output pin is LOW.
> > > + */
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + 0, state->polarity);
> > > + if (curstate.enabled)
> > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > + } else {
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + state->duty_cycle, state->polarity);
> > > + if (!curstate.enabled)
> > > + pwm_imx_tpm_enable(chip, pwm, true);
> >
> > Doesn't this mean that you won't be applying changes to the polarity while a
> > PWM is enabled? That seems wrong. Granted, you may usually not run into
> > that, but if you can't support it I think you should at least return an error if
> > you detect that the user wants to change polarity while the PWM is enabled.
>
> I thought below function call already set the polarity? No matter its status is enabled
> or disabled, the polarity setting will be always applied.
>
> pwm_imx_tpm_config(chip, pwm, state->period,
> state->duty_cycle, state->polarity);
That's not what it seems to do. In fact there's a comment that explains
why it doesn't do that. Quoting here:
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + tpm->chn_config[pwm->hwpwm] = val;
Looks to me like that only stores the value for that register so that it
can be applied at a later point. Or am I missing something?
> > > + ret);
> > > + }
> > > +
> > > + return ret;
> > > +};
> >
> > Your handling of the clock seems strange here. Basically in the above you
> > always keep the clock on and you only disable it if there are no users and
> > you're going to suspend.
> >
> > Why do you need to keep an extra reference count anyway? Or why keep the
> > clock on all the time? Can't you just do a clk_prepare_enable() every time
> > somebody enables the PWM? The CCF already has built-in reference
> > counting, so I'm not sure you really need to duplicate that here.
>
> Keeping clock always ON since driver probe is because, many TMP registers'
> write needs clock to be ON for sync into register hardware, just enable the clock
> before writing register and disable the clock immediately does NOT work, unless
> we keep reading the register value until the register value is what we want to write,
> but that makes code much more complicated, and the PWM clock normally is from
> OSC which does NOT consume too much power, so I keep the clock always on and ONLY
> disable it after suspend.
Why do you bother with keeping the enable reference count, then? Can't
you just enable the clock on probe, then on suspend disable it, enable
it again on resume and on remove you also disable it? Why does it need
to be dependent on whether there are any active PWMs or not?
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-18 15:30 ` Thierry Reding
0 siblings, 0 replies; 41+ messages in thread
From: Thierry Reding @ 2019-03-18 15:30 UTC (permalink / raw)
To: Anson Huang
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 9844 bytes --]
On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote:
> Hi, Thierry
>
> Best Regards!
> Anson Huang
>
> > -----Original Message-----
> > From: Thierry Reding [mailto:thierry.reding@gmail.com]
> > Sent: 2019年3月18日 18:28
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> > Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> > <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
[...]
> > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + u32 period,
> > > + u32 duty_cycle,
> > > + enum pwm_polarity polarity) {
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u32 duty_cnt, val;
> > > + u64 tmp;
> > > +
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > PWM_IMX_TPM_MOD_MOD;
> > > + tmp *= duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +
> > > + /*
> > > + * set polarity (for edge-aligned PWM modes)
> > > + *
> > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > + * 0 10 10 PWM High-true pulse
> > > + * 0 10 00 PWM Reserved
> > > + * 0 10 01 PWM Low-true pulse
> > > + * 0 10 11 PWM Reserved
> > > + *
> > > + * High-true pulse: clear output on counter match, set output on
> > > + * counter reload, set output when counter first enabled or paused.
> > > + *
> > > + * Low-true pulse: set output on counter match, clear output on
> > > + * counter reload, clear output when counter first enabled or paused.
> > > + */
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> > |
> > > + PWM_IMX_TPM_CnSC_MSA);
> > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + tpm->chn_config[pwm->hwpwm] = val;
> > > +}
> > > +
> > > +/*
> > > + * When a channel's polarity is configured, the polarity settings
> > > + * will be saved and ONLY write into the register when the channel
> > > + * is enabled.
> > > + *
> > > + * When a channel is disabled, its polarity settings will be saved
> > > + * and its output will be disabled by clearing polarity settings.
> > > + *
> > > + * when a channel is enabled, its polarity settings will be restored
> >
> > "when" -> "When".
>
> Will fix it.
>
> >
> > > + * and output will be enabled again.
> > > + */
> > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + bool enable)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u32 val;
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + if (enable) {
> > > + /* restore channel config */
> > > + writel(tpm->chn_config[pwm->hwpwm],
> > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > +
> > > + if (++tpm->enable_count == 1) {
> > > + /* start TPM counter */
> > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > + }
> > > + } else {
> > > + /* disable channel */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> > PWM_IMX_TPM_CnSC_MSB |
> > > + PWM_IMX_TPM_CnSC_ELSB |
> > PWM_IMX_TPM_CnSC_ELSA);
> > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > +
> > > + if (--tpm->enable_count == 0) {
> > > + /* stop TPM counter since all channels are disabled
> > */
> > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > + }
> > > + }
> > > +
> > > + /* update channel status */
> > > + tpm->chn_status[pwm->hwpwm] = enable; }
> > > +
> > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + u64 tmp;
> > > + u32 val, rate;
> > > +
> > > + /* get period */
> > > + rate = clk_get_rate(tpm->clk);
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + val &= PWM_IMX_TPM_SC_PS;
> > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > + /* get duty cycle */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > + /* get polarity */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + else
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + /* get channel status */
> > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > > +
> > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct pwm_device
> > *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + struct pwm_state curstate;
> > > + int ret;
> > > +
> > > + mutex_lock(&tpm->lock);
> > > +
> > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > +
> > > + if (state->period != curstate.period) {
> > > + /*
> > > + * TPM counter is shared by multiple channels, so
> > > + * prescale and period can NOT be modified when
> > > + * there are multiple channels in use.
> > > + */
> > > + if (tpm->user_count != 1)
> > > + return -EBUSY;
> > > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + if (state->enabled == false) {
> > > + /*
> > > + * if eventually the PWM output is LOW, either
> > > + * duty cycle is 0 or status is disabled, need
> > > + * to make sure the output pin is LOW.
> > > + */
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + 0, state->polarity);
> > > + if (curstate.enabled)
> > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > + } else {
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + state->duty_cycle, state->polarity);
> > > + if (!curstate.enabled)
> > > + pwm_imx_tpm_enable(chip, pwm, true);
> >
> > Doesn't this mean that you won't be applying changes to the polarity while a
> > PWM is enabled? That seems wrong. Granted, you may usually not run into
> > that, but if you can't support it I think you should at least return an error if
> > you detect that the user wants to change polarity while the PWM is enabled.
>
> I thought below function call already set the polarity? No matter its status is enabled
> or disabled, the polarity setting will be always applied.
>
> pwm_imx_tpm_config(chip, pwm, state->period,
> state->duty_cycle, state->polarity);
That's not what it seems to do. In fact there's a comment that explains
why it doesn't do that. Quoting here:
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + tpm->chn_config[pwm->hwpwm] = val;
Looks to me like that only stores the value for that register so that it
can be applied at a later point. Or am I missing something?
> > > + ret);
> > > + }
> > > +
> > > + return ret;
> > > +};
> >
> > Your handling of the clock seems strange here. Basically in the above you
> > always keep the clock on and you only disable it if there are no users and
> > you're going to suspend.
> >
> > Why do you need to keep an extra reference count anyway? Or why keep the
> > clock on all the time? Can't you just do a clk_prepare_enable() every time
> > somebody enables the PWM? The CCF already has built-in reference
> > counting, so I'm not sure you really need to duplicate that here.
>
> Keeping clock always ON since driver probe is because, many TMP registers'
> write needs clock to be ON for sync into register hardware, just enable the clock
> before writing register and disable the clock immediately does NOT work, unless
> we keep reading the register value until the register value is what we want to write,
> but that makes code much more complicated, and the PWM clock normally is from
> OSC which does NOT consume too much power, so I keep the clock always on and ONLY
> disable it after suspend.
Why do you bother with keeping the enable reference count, then? Can't
you just enable the clock on probe, then on suspend disable it, enable
it again on resume and on remove you also disable it? Why does it need
to be dependent on whether there are any active PWMs or not?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-18 15:30 ` Thierry Reding
(?)
@ 2019-03-19 0:55 ` Anson Huang
-1 siblings, 0 replies; 41+ messages in thread
From: Anson Huang @ 2019-03-19 0:55 UTC (permalink / raw)
To: Thierry Reding
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel,
u.kleine-koenig, dl-linux-imx
Hi, Thierry
Best Regards!
Anson Huang
> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: 2019年3月18日 23:31
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote:
> > Hi, Thierry
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Thierry Reding [mailto:thierry.reding@gmail.com]
> > > Sent: 2019年3月18日 18:28
> > > To: Anson Huang <anson.huang@nxp.com>
> > > Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > > linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> > > Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> > > <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> > > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> > > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
> > >
> > > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> [...]
> > > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + u32 period,
> > > > + u32 duty_cycle,
> > > > + enum pwm_polarity polarity) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u32 duty_cnt, val;
> > > > + u64 tmp;
> > > > +
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > PWM_IMX_TPM_MOD_MOD;
> > > > + tmp *= duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > +
> > > > + /*
> > > > + * set polarity (for edge-aligned PWM modes)
> > > > + *
> > > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > > + * 0 10 10 PWM High-true pulse
> > > > + * 0 10 00 PWM Reserved
> > > > + * 0 10 01 PWM Low-true pulse
> > > > + * 0 10 11 PWM Reserved
> > > > + *
> > > > + * High-true pulse: clear output on counter match, set output on
> > > > + * counter reload, set output when counter first enabled or paused.
> > > > + *
> > > > + * Low-true pulse: set output on counter match, clear output on
> > > > + * counter reload, clear output when counter first enabled or paused.
> > > > + */
> > > > +
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> > > |
> > > > + PWM_IMX_TPM_CnSC_MSA);
> > > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + tpm->chn_config[pwm->hwpwm] = val; }
> > > > +
> > > > +/*
> > > > + * When a channel's polarity is configured, the polarity settings
> > > > + * will be saved and ONLY write into the register when the
> > > > +channel
> > > > + * is enabled.
> > > > + *
> > > > + * When a channel is disabled, its polarity settings will be
> > > > +saved
> > > > + * and its output will be disabled by clearing polarity settings.
> > > > + *
> > > > + * when a channel is enabled, its polarity settings will be
> > > > +restored
> > >
> > > "when" -> "When".
> >
> > Will fix it.
> >
> > >
> > > > + * and output will be enabled again.
> > > > + */
> > > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + bool enable)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u32 val;
> > > > +
> > > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > + if (enable) {
> > > > + /* restore channel config */
> > > > + writel(tpm->chn_config[pwm->hwpwm],
> > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > +
> > > > + if (++tpm->enable_count == 1) {
> > > > + /* start TPM counter */
> > > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > + }
> > > > + } else {
> > > > + /* disable channel */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> > > PWM_IMX_TPM_CnSC_MSB |
> > > > + PWM_IMX_TPM_CnSC_ELSB |
> > > PWM_IMX_TPM_CnSC_ELSA);
> > > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > +
> > > > + if (--tpm->enable_count == 0) {
> > > > + /* stop TPM counter since all channels are disabled
> > > */
> > > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > + }
> > > > + }
> > > > +
> > > > + /* update channel status */
> > > > + tpm->chn_status[pwm->hwpwm] = enable; }
> > > > +
> > > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u64 tmp;
> > > > + u32 val, rate;
> > > > +
> > > > + /* get period */
> > > > + rate = clk_get_rate(tpm->clk);
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > + val &= PWM_IMX_TPM_SC_PS;
> > > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > + /* get duty cycle */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > + /* get polarity */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > > > + state->polarity = PWM_POLARITY_INVERSED;
> > > > + else
> > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > + /* get channel status */
> > > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > > > +
> > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + struct pwm_state curstate;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&tpm->lock);
> > > > +
> > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + if (state->period != curstate.period) {
> > > > + /*
> > > > + * TPM counter is shared by multiple channels, so
> > > > + * prescale and period can NOT be modified when
> > > > + * there are multiple channels in use.
> > > > + */
> > > > + if (tpm->user_count != 1)
> > > > + return -EBUSY;
> > > > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (state->enabled == false) {
> > > > + /*
> > > > + * if eventually the PWM output is LOW, either
> > > > + * duty cycle is 0 or status is disabled, need
> > > > + * to make sure the output pin is LOW.
> > > > + */
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + 0, state->polarity);
> > > > + if (curstate.enabled)
> > > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > > + } else {
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + state->duty_cycle, state->polarity);
> > > > + if (!curstate.enabled)
> > > > + pwm_imx_tpm_enable(chip, pwm, true);
> > >
> > > Doesn't this mean that you won't be applying changes to the polarity
> > > while a PWM is enabled? That seems wrong. Granted, you may usually
> > > not run into that, but if you can't support it I think you should at
> > > least return an error if you detect that the user wants to change polarity
> while the PWM is enabled.
> >
> > I thought below function call already set the polarity? No matter its
> > status is enabled or disabled, the polarity setting will be always applied.
> >
> > pwm_imx_tpm_config(chip, pwm, state->period,
> > state->duty_cycle, state->polarity);
>
> That's not what it seems to do. In fact there's a comment that explains why it
> doesn't do that. Quoting here:
>
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + tpm->chn_config[pwm->hwpwm] = val;
>
> Looks to me like that only stores the value for that register so that it can be
> applied at a later point. Or am I missing something?
The comment is what I intend to say, since the hardware does NOT provide
the channel enable/disable function, but different polarity settings can act as
channel enable/disable, that is why I store the polarity setting and ONLY
apply it to the hardware when the channel is enabled.
>
> > > > + ret);
> > > > + }
> > > > +
> > > > + return ret;
> > > > +};
> > >
> > > Your handling of the clock seems strange here. Basically in the
> > > above you always keep the clock on and you only disable it if there
> > > are no users and you're going to suspend.
> > >
> > > Why do you need to keep an extra reference count anyway? Or why keep
> > > the clock on all the time? Can't you just do a clk_prepare_enable()
> > > every time somebody enables the PWM? The CCF already has built-in
> > > reference counting, so I'm not sure you really need to duplicate that here.
> >
> > Keeping clock always ON since driver probe is because, many TMP registers'
> > write needs clock to be ON for sync into register hardware, just
> > enable the clock before writing register and disable the clock
> > immediately does NOT work, unless we keep reading the register value
> > until the register value is what we want to write, but that makes code
> > much more complicated, and the PWM clock normally is from OSC which
> > does NOT consume too much power, so I keep the clock always on and
> ONLY disable it after suspend.
>
> Why do you bother with keeping the enable reference count, then? Can't you
> just enable the clock on probe, then on suspend disable it, enable it again on
> resume and on remove you also disable it? Why does it need to be
> dependent on whether there are any active PWMs or not?
That was what I did in previous patch version, but I received comments saying that
PWM clock should NOT be disabled if it is in use, and the channel enable count is
introduced for determine whether to disable the shared HW counter, so I also use
the channel enable count to determine whether to turn off clock in suspend. Although
I think it should be OK to just turn off clock in suspend without any checking, if there is
any active PWM, then the user should fix it, as I think PWM should be OFF for suspend.
So you also think I can skip the check for the PWM active count and just disable clock
in suspend?
Thanks,
Anson.
>
> Thierry
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-19 0:55 ` Anson Huang
0 siblings, 0 replies; 41+ messages in thread
From: Anson Huang @ 2019-03-19 0:55 UTC (permalink / raw)
To: Thierry Reding
Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree,
shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel,
robh+dt, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez,
festevam, linux-arm-kernel
Hi, Thierry
Best Regards!
Anson Huang
> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: 2019年3月18日 23:31
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote:
> > Hi, Thierry
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Thierry Reding [mailto:thierry.reding@gmail.com]
> > > Sent: 2019年3月18日 18:28
> > > To: Anson Huang <anson.huang@nxp.com>
> > > Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > > linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> > > Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> > > <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> > > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> > > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
> > >
> > > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> [...]
> > > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + u32 period,
> > > > + u32 duty_cycle,
> > > > + enum pwm_polarity polarity) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u32 duty_cnt, val;
> > > > + u64 tmp;
> > > > +
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > PWM_IMX_TPM_MOD_MOD;
> > > > + tmp *= duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > +
> > > > + /*
> > > > + * set polarity (for edge-aligned PWM modes)
> > > > + *
> > > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > > + * 0 10 10 PWM High-true pulse
> > > > + * 0 10 00 PWM Reserved
> > > > + * 0 10 01 PWM Low-true pulse
> > > > + * 0 10 11 PWM Reserved
> > > > + *
> > > > + * High-true pulse: clear output on counter match, set output on
> > > > + * counter reload, set output when counter first enabled or paused.
> > > > + *
> > > > + * Low-true pulse: set output on counter match, clear output on
> > > > + * counter reload, clear output when counter first enabled or paused.
> > > > + */
> > > > +
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> > > |
> > > > + PWM_IMX_TPM_CnSC_MSA);
> > > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + tpm->chn_config[pwm->hwpwm] = val; }
> > > > +
> > > > +/*
> > > > + * When a channel's polarity is configured, the polarity settings
> > > > + * will be saved and ONLY write into the register when the
> > > > +channel
> > > > + * is enabled.
> > > > + *
> > > > + * When a channel is disabled, its polarity settings will be
> > > > +saved
> > > > + * and its output will be disabled by clearing polarity settings.
> > > > + *
> > > > + * when a channel is enabled, its polarity settings will be
> > > > +restored
> > >
> > > "when" -> "When".
> >
> > Will fix it.
> >
> > >
> > > > + * and output will be enabled again.
> > > > + */
> > > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + bool enable)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u32 val;
> > > > +
> > > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > + if (enable) {
> > > > + /* restore channel config */
> > > > + writel(tpm->chn_config[pwm->hwpwm],
> > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > +
> > > > + if (++tpm->enable_count == 1) {
> > > > + /* start TPM counter */
> > > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > + }
> > > > + } else {
> > > > + /* disable channel */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> > > PWM_IMX_TPM_CnSC_MSB |
> > > > + PWM_IMX_TPM_CnSC_ELSB |
> > > PWM_IMX_TPM_CnSC_ELSA);
> > > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > +
> > > > + if (--tpm->enable_count == 0) {
> > > > + /* stop TPM counter since all channels are disabled
> > > */
> > > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > + }
> > > > + }
> > > > +
> > > > + /* update channel status */
> > > > + tpm->chn_status[pwm->hwpwm] = enable; }
> > > > +
> > > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u64 tmp;
> > > > + u32 val, rate;
> > > > +
> > > > + /* get period */
> > > > + rate = clk_get_rate(tpm->clk);
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > + val &= PWM_IMX_TPM_SC_PS;
> > > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > + /* get duty cycle */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > + /* get polarity */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > > > + state->polarity = PWM_POLARITY_INVERSED;
> > > > + else
> > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > + /* get channel status */
> > > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > > > +
> > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + struct pwm_state curstate;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&tpm->lock);
> > > > +
> > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + if (state->period != curstate.period) {
> > > > + /*
> > > > + * TPM counter is shared by multiple channels, so
> > > > + * prescale and period can NOT be modified when
> > > > + * there are multiple channels in use.
> > > > + */
> > > > + if (tpm->user_count != 1)
> > > > + return -EBUSY;
> > > > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (state->enabled == false) {
> > > > + /*
> > > > + * if eventually the PWM output is LOW, either
> > > > + * duty cycle is 0 or status is disabled, need
> > > > + * to make sure the output pin is LOW.
> > > > + */
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + 0, state->polarity);
> > > > + if (curstate.enabled)
> > > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > > + } else {
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + state->duty_cycle, state->polarity);
> > > > + if (!curstate.enabled)
> > > > + pwm_imx_tpm_enable(chip, pwm, true);
> > >
> > > Doesn't this mean that you won't be applying changes to the polarity
> > > while a PWM is enabled? That seems wrong. Granted, you may usually
> > > not run into that, but if you can't support it I think you should at
> > > least return an error if you detect that the user wants to change polarity
> while the PWM is enabled.
> >
> > I thought below function call already set the polarity? No matter its
> > status is enabled or disabled, the polarity setting will be always applied.
> >
> > pwm_imx_tpm_config(chip, pwm, state->period,
> > state->duty_cycle, state->polarity);
>
> That's not what it seems to do. In fact there's a comment that explains why it
> doesn't do that. Quoting here:
>
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + tpm->chn_config[pwm->hwpwm] = val;
>
> Looks to me like that only stores the value for that register so that it can be
> applied at a later point. Or am I missing something?
The comment is what I intend to say, since the hardware does NOT provide
the channel enable/disable function, but different polarity settings can act as
channel enable/disable, that is why I store the polarity setting and ONLY
apply it to the hardware when the channel is enabled.
>
> > > > + ret);
> > > > + }
> > > > +
> > > > + return ret;
> > > > +};
> > >
> > > Your handling of the clock seems strange here. Basically in the
> > > above you always keep the clock on and you only disable it if there
> > > are no users and you're going to suspend.
> > >
> > > Why do you need to keep an extra reference count anyway? Or why keep
> > > the clock on all the time? Can't you just do a clk_prepare_enable()
> > > every time somebody enables the PWM? The CCF already has built-in
> > > reference counting, so I'm not sure you really need to duplicate that here.
> >
> > Keeping clock always ON since driver probe is because, many TMP registers'
> > write needs clock to be ON for sync into register hardware, just
> > enable the clock before writing register and disable the clock
> > immediately does NOT work, unless we keep reading the register value
> > until the register value is what we want to write, but that makes code
> > much more complicated, and the PWM clock normally is from OSC which
> > does NOT consume too much power, so I keep the clock always on and
> ONLY disable it after suspend.
>
> Why do you bother with keeping the enable reference count, then? Can't you
> just enable the clock on probe, then on suspend disable it, enable it again on
> resume and on remove you also disable it? Why does it need to be
> dependent on whether there are any active PWMs or not?
That was what I did in previous patch version, but I received comments saying that
PWM clock should NOT be disabled if it is in use, and the channel enable count is
introduced for determine whether to disable the shared HW counter, so I also use
the channel enable count to determine whether to turn off clock in suspend. Although
I think it should be OK to just turn off clock in suspend without any checking, if there is
any active PWM, then the user should fix it, as I think PWM should be OFF for suspend.
So you also think I can skip the check for the PWM active count and just disable clock
in suspend?
Thanks,
Anson.
>
> Thierry
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-19 0:55 ` Anson Huang
0 siblings, 0 replies; 41+ messages in thread
From: Anson Huang @ 2019-03-19 0:55 UTC (permalink / raw)
To: Thierry Reding
Cc: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
linux, otavio, stefan, Leonard Crestez, Robin Gong, jan.tuerk,
linux-pwm, devicetree, linux-arm-kernel, linux-kernel
Hi, Thierry
Best Regards!
Anson Huang
> -----Original Message-----
> From: Thierry Reding [mailto:thierry.reding@gmail.com]
> Sent: 2019年3月18日 23:31
> To: Anson Huang <anson.huang@nxp.com>
> Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote:
> > Hi, Thierry
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Thierry Reding [mailto:thierry.reding@gmail.com]
> > > Sent: 2019年3月18日 18:28
> > > To: Anson Huang <anson.huang@nxp.com>
> > > Cc: robh+dt@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org;
> > > s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> > > linux@armlinux.org.uk; otavio@ossystems.com.br; stefan@agner.ch;
> > > Leonard Crestez <leonard.crestez@nxp.com>; Robin Gong
> > > <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux-
> > > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine-
> > > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support
> > >
> > > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote:
> [...]
> > > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + u32 period,
> > > > + u32 duty_cycle,
> > > > + enum pwm_polarity polarity) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u32 duty_cnt, val;
> > > > + u64 tmp;
> > > > +
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > PWM_IMX_TPM_MOD_MOD;
> > > > + tmp *= duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD,
> > > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > +
> > > > + /*
> > > > + * set polarity (for edge-aligned PWM modes)
> > > > + *
> > > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration
> > > > + * 0 10 10 PWM High-true pulse
> > > > + * 0 10 00 PWM Reserved
> > > > + * 0 10 01 PWM Low-true pulse
> > > > + * 0 10 11 PWM Reserved
> > > > + *
> > > > + * High-true pulse: clear output on counter match, set output on
> > > > + * counter reload, set output when counter first enabled or paused.
> > > > + *
> > > > + * Low-true pulse: set output on counter match, clear output on
> > > > + * counter reload, clear output when counter first enabled or paused.
> > > > + */
> > > > +
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA
> > > |
> > > > + PWM_IMX_TPM_CnSC_MSA);
> > > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA;
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + tpm->chn_config[pwm->hwpwm] = val; }
> > > > +
> > > > +/*
> > > > + * When a channel's polarity is configured, the polarity settings
> > > > + * will be saved and ONLY write into the register when the
> > > > +channel
> > > > + * is enabled.
> > > > + *
> > > > + * When a channel is disabled, its polarity settings will be
> > > > +saved
> > > > + * and its output will be disabled by clearing polarity settings.
> > > > + *
> > > > + * when a channel is enabled, its polarity settings will be
> > > > +restored
> > >
> > > "when" -> "When".
> >
> > Will fix it.
> >
> > >
> > > > + * and output will be enabled again.
> > > > + */
> > > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + bool enable)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u32 val;
> > > > +
> > > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > + if (enable) {
> > > > + /* restore channel config */
> > > > + writel(tpm->chn_config[pwm->hwpwm],
> > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > +
> > > > + if (++tpm->enable_count == 1) {
> > > > + /* start TPM counter */
> > > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > + }
> > > > + } else {
> > > > + /* disable channel */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> > > PWM_IMX_TPM_CnSC_MSB |
> > > > + PWM_IMX_TPM_CnSC_ELSB |
> > > PWM_IMX_TPM_CnSC_ELSA);
> > > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > +
> > > > + if (--tpm->enable_count == 0) {
> > > > + /* stop TPM counter since all channels are disabled
> > > */
> > > > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > + }
> > > > + }
> > > > +
> > > > + /* update channel status */
> > > > + tpm->chn_status[pwm->hwpwm] = enable; }
> > > > +
> > > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + u64 tmp;
> > > > + u32 val, rate;
> > > > +
> > > > + /* get period */
> > > > + rate = clk_get_rate(tpm->clk);
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD);
> > > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > + val &= PWM_IMX_TPM_SC_PS;
> > > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > + /* get duty cycle */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > + tmp *= (1 << val) * NSEC_PER_SEC;
> > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > + /* get polarity */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + if (val & PWM_IMX_TPM_CnSC_ELSA)
> > > > + state->polarity = PWM_POLARITY_INVERSED;
> > > > + else
> > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > + /* get channel status */
> > > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; }
> > > > +
> > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + struct pwm_state curstate;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&tpm->lock);
> > > > +
> > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + if (state->period != curstate.period) {
> > > > + /*
> > > > + * TPM counter is shared by multiple channels, so
> > > > + * prescale and period can NOT be modified when
> > > > + * there are multiple channels in use.
> > > > + */
> > > > + if (tpm->user_count != 1)
> > > > + return -EBUSY;
> > > > + ret = pwm_imx_tpm_config_counter(chip, state->period);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (state->enabled == false) {
> > > > + /*
> > > > + * if eventually the PWM output is LOW, either
> > > > + * duty cycle is 0 or status is disabled, need
> > > > + * to make sure the output pin is LOW.
> > > > + */
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + 0, state->polarity);
> > > > + if (curstate.enabled)
> > > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > > + } else {
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + state->duty_cycle, state->polarity);
> > > > + if (!curstate.enabled)
> > > > + pwm_imx_tpm_enable(chip, pwm, true);
> > >
> > > Doesn't this mean that you won't be applying changes to the polarity
> > > while a PWM is enabled? That seems wrong. Granted, you may usually
> > > not run into that, but if you can't support it I think you should at
> > > least return an error if you detect that the user wants to change polarity
> while the PWM is enabled.
> >
> > I thought below function call already set the polarity? No matter its
> > status is enabled or disabled, the polarity setting will be always applied.
> >
> > pwm_imx_tpm_config(chip, pwm, state->period,
> > state->duty_cycle, state->polarity);
>
> That's not what it seems to do. In fact there's a comment that explains why it
> doesn't do that. Quoting here:
>
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + tpm->chn_config[pwm->hwpwm] = val;
>
> Looks to me like that only stores the value for that register so that it can be
> applied at a later point. Or am I missing something?
The comment is what I intend to say, since the hardware does NOT provide
the channel enable/disable function, but different polarity settings can act as
channel enable/disable, that is why I store the polarity setting and ONLY
apply it to the hardware when the channel is enabled.
>
> > > > + ret);
> > > > + }
> > > > +
> > > > + return ret;
> > > > +};
> > >
> > > Your handling of the clock seems strange here. Basically in the
> > > above you always keep the clock on and you only disable it if there
> > > are no users and you're going to suspend.
> > >
> > > Why do you need to keep an extra reference count anyway? Or why keep
> > > the clock on all the time? Can't you just do a clk_prepare_enable()
> > > every time somebody enables the PWM? The CCF already has built-in
> > > reference counting, so I'm not sure you really need to duplicate that here.
> >
> > Keeping clock always ON since driver probe is because, many TMP registers'
> > write needs clock to be ON for sync into register hardware, just
> > enable the clock before writing register and disable the clock
> > immediately does NOT work, unless we keep reading the register value
> > until the register value is what we want to write, but that makes code
> > much more complicated, and the PWM clock normally is from OSC which
> > does NOT consume too much power, so I keep the clock always on and
> ONLY disable it after suspend.
>
> Why do you bother with keeping the enable reference count, then? Can't you
> just enable the clock on probe, then on suspend disable it, enable it again on
> resume and on remove you also disable it? Why does it need to be
> dependent on whether there are any active PWMs or not?
That was what I did in previous patch version, but I received comments saying that
PWM clock should NOT be disabled if it is in use, and the channel enable count is
introduced for determine whether to disable the shared HW counter, so I also use
the channel enable count to determine whether to turn off clock in suspend. Although
I think it should be OK to just turn off clock in suspend without any checking, if there is
any active PWM, then the user should fix it, as I think PWM should be OFF for suspend.
So you also think I can skip the check for the PWM active count and just disable clock
in suspend?
Thanks,
Anson.
>
> Thierry
^ permalink raw reply [flat|nested] 41+ messages in thread