* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 9:17 ` Uwe Kleine-König
0 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 9:17 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel, dl-linux-imx
On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> inside, add TPM PWM driver support.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V1:
> - improve coding style, function name's prefix;
> - improve Kconfig's help info;
> - use .apply instead for .enable/.disable/.config etc. to simply the code;
> - improve clock operation, make clock enabled during probe phase and ONLY disabled
> when suspend, as register read/write need to sync with clock, keeping it enabled
> makes the register read/write simple;
> - improve prescale calculation;
> - add error message print during probe for ioremap and clk get;
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-imx-tpm.c | 332 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 343 insertions(+)
> create mode 100644 drivers/pwm/pwm-imx-tpm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..c1cbb43 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -201,6 +201,16 @@ config PWM_IMX
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx.
>
> +config PWM_IMX_TPM
> + tristate "i.MX TPM PWM support"
> + depends on ARCH_MXC
Can you please make this
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 9c676a0..64e036c 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,332 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + */
> +
> +#include <linux/bitops.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>
> +#include <linux/spinlock.h>
> +
> +#define TPM_GLOBAL 0x8
> +#define TPM_SC 0x10
> +#define TPM_CNT 0x14
> +#define TPM_MOD 0x18
> +#define TPM_C0SC 0x20
> +#define TPM_C0V 0x24
PWM_IMX_TPM_COV etc. please
> +
> +#define TPM_SC_CMOD_SHIFT 3
> +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
If you use the macros that are documented in <linux/bitops.h> you don't
need these MASK and SHIFT stuff.
> +#define TPM_SC_CPWMS BIT(5)
> +
> +#define TPM_CnSC_CHF BIT(7)
> +#define TPM_CnSC_MSnB BIT(5)
> +#define TPM_CnSC_MSnA BIT(4)
> +#define TPM_CnSC_ELSnB BIT(3)
> +#define TPM_CnSC_ELSnA BIT(2)
> +
> +#define TPM_SC_PS_MASK 0x7
> +#define TPM_MOD_MOD_MASK 0xffff
> +
> +#define TPM_COUNT_MAX 0xffff
> +
> +#define TPM_CHn_ADDR_OFFSET 0x8
> +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This
is used as array size below and when reading
u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
I wonder if the actual number of PWMs can be bigger than the default.
> +struct imx_tpm_pwm_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *base;
> + spinlock_t lock;
> + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> +};
> +
> +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct imx_tpm_pwm_chip, chip)
> +
> +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32 period)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + unsigned int period_cnt;
> + u32 val, div;
> + u64 tmp;
> +
> + tmp = clk_get_rate(tpm->clk);
> + tmp *= period;
> + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> + if (val < TPM_COUNT_MAX)
> + div = 0;
> + else
> + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
Are you sure you have to divide by TPM_COUNT_MAX and not by
(TPM_COUNT_MAX + 1)?
> + if (div > TPM_SC_PS_MASK) {
> + dev_err(chip->dev,
> + "failed to find valid prescale value!\n");
> + return;
I think you should handle this failure and make imx_tpm_pwm_apply() fail.
> + }
> + /* set TPM counter prescale */
> + val = readl(tpm->base + TPM_SC);
> + val &= ~TPM_SC_PS_MASK;
> + val |= div;
> + writel(val, tpm->base + TPM_SC);
According to the documentation PS can only be written if the counter is
disabled, I think this isn't ensured here.
> + /* set period counter */
> + do_div(tmp, NSEC_PER_SEC);
> + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base + TPM_MOD);
If there is already a value pending in the MOD register, another write
to it is ignored. A comment, why this cannot happen, would be
appropriate. (Note, I'm not sure this cannot happen.)
> +}
> +
> +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + static bool tpm_cnt_initialized;
> + unsigned int duty_cnt;
> + u32 val;
> + u64 tmp;
> +
> + /*
> + * TPM counter is shared by multi channels, let's make it to be
> + * ONLY first channel can config TPM counter's precale and period
> + * count.
> + */
> + if (!tpm_cnt_initialized) {
> + imx_tpm_pwm_config_counter(chip, state->period);
> + tpm_cnt_initialized = true;
> + }
So the period can only be configured once. That is not as good as it
could be. You should allow a change whenever there is exactly one PWM in
use.
> + /* set duty counter */
> + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> + tmp *= state->duty_cycle;
> + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
Uah, you use state->period here even though for the 2nd PWM the Divider
might not be setup appropriately.
> [...]
> +static int imx_tpm_pwm_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;
> + unsigned long flags;
> +
> + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> +
> + spin_lock_irqsave(&tpm->lock, flags);
> +
> + if (state->period != curstate.period ||
> + state->duty_cycle != curstate.duty_cycle ||
> + state->polarity != curstate.polarity)
> + imx_tpm_pwm_config(chip, pwm, state);
> +
> + if (state->enabled != curstate.enabled)
> + imx_tpm_pwm_enable(chip, pwm, state->enabled);
This is wrong. This sequence:
pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled = true });
pwm_apply_state(pwm, { .duty_cycle = 10000, .period = 10000, .enabled = false });
must keep the output pin low which isn't guaranteed here.
> +
> + spin_unlock_irqrestore(&tpm->lock, flags);
> +
> + return 0;
> +}
I didn't look in depth through the complete driver yet, but there is
IIRC still some feedback on v1 that wasn't addressed in v2 (because v2
was sent before the last feedback). So I will look in more depth in v3
(assuming it comes late enough address the concerns from this mail).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
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] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 9:17 ` Uwe Kleine-König
0 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 9:17 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel@lists.infradead.org
On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> inside, add TPM PWM driver support.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> Changes since V1:
> - improve coding style, function name's prefix;
> - improve Kconfig's help info;
> - use .apply instead for .enable/.disable/.config etc. to simply the code;
> - improve clock operation, make clock enabled during probe phase and ONLY disabled
> when suspend, as register read/write need to sync with clock, keeping it enabled
> makes the register read/write simple;
> - improve prescale calculation;
> - add error message print during probe for ioremap and clk get;
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-imx-tpm.c | 332 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 343 insertions(+)
> create mode 100644 drivers/pwm/pwm-imx-tpm.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a8f47df..c1cbb43 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -201,6 +201,16 @@ config PWM_IMX
> To compile this driver as a module, choose M here: the module
> will be called pwm-imx.
>
> +config PWM_IMX_TPM
> + tristate "i.MX TPM PWM support"
> + depends on ARCH_MXC
Can you please make this
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 9c676a0..64e036c 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,332 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + */
> +
> +#include <linux/bitops.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>
> +#include <linux/spinlock.h>
> +
> +#define TPM_GLOBAL 0x8
> +#define TPM_SC 0x10
> +#define TPM_CNT 0x14
> +#define TPM_MOD 0x18
> +#define TPM_C0SC 0x20
> +#define TPM_C0V 0x24
PWM_IMX_TPM_COV etc. please
> +
> +#define TPM_SC_CMOD_SHIFT 3
> +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
If you use the macros that are documented in <linux/bitops.h> you don't
need these MASK and SHIFT stuff.
> +#define TPM_SC_CPWMS BIT(5)
> +
> +#define TPM_CnSC_CHF BIT(7)
> +#define TPM_CnSC_MSnB BIT(5)
> +#define TPM_CnSC_MSnA BIT(4)
> +#define TPM_CnSC_ELSnB BIT(3)
> +#define TPM_CnSC_ELSnA BIT(2)
> +
> +#define TPM_SC_PS_MASK 0x7
> +#define TPM_MOD_MOD_MASK 0xffff
> +
> +#define TPM_COUNT_MAX 0xffff
> +
> +#define TPM_CHn_ADDR_OFFSET 0x8
> +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This
is used as array size below and when reading
u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
I wonder if the actual number of PWMs can be bigger than the default.
> +struct imx_tpm_pwm_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *base;
> + spinlock_t lock;
> + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> +};
> +
> +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct imx_tpm_pwm_chip, chip)
> +
> +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32 period)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + unsigned int period_cnt;
> + u32 val, div;
> + u64 tmp;
> +
> + tmp = clk_get_rate(tpm->clk);
> + tmp *= period;
> + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> + if (val < TPM_COUNT_MAX)
> + div = 0;
> + else
> + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
Are you sure you have to divide by TPM_COUNT_MAX and not by
(TPM_COUNT_MAX + 1)?
> + if (div > TPM_SC_PS_MASK) {
> + dev_err(chip->dev,
> + "failed to find valid prescale value!\n");
> + return;
I think you should handle this failure and make imx_tpm_pwm_apply() fail.
> + }
> + /* set TPM counter prescale */
> + val = readl(tpm->base + TPM_SC);
> + val &= ~TPM_SC_PS_MASK;
> + val |= div;
> + writel(val, tpm->base + TPM_SC);
According to the documentation PS can only be written if the counter is
disabled, I think this isn't ensured here.
> + /* set period counter */
> + do_div(tmp, NSEC_PER_SEC);
> + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base + TPM_MOD);
If there is already a value pending in the MOD register, another write
to it is ignored. A comment, why this cannot happen, would be
appropriate. (Note, I'm not sure this cannot happen.)
> +}
> +
> +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + static bool tpm_cnt_initialized;
> + unsigned int duty_cnt;
> + u32 val;
> + u64 tmp;
> +
> + /*
> + * TPM counter is shared by multi channels, let's make it to be
> + * ONLY first channel can config TPM counter's precale and period
> + * count.
> + */
> + if (!tpm_cnt_initialized) {
> + imx_tpm_pwm_config_counter(chip, state->period);
> + tpm_cnt_initialized = true;
> + }
So the period can only be configured once. That is not as good as it
could be. You should allow a change whenever there is exactly one PWM in
use.
> + /* set duty counter */
> + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> + tmp *= state->duty_cycle;
> + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
Uah, you use state->period here even though for the 2nd PWM the Divider
might not be setup appropriately.
> [...]
> +static int imx_tpm_pwm_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;
> + unsigned long flags;
> +
> + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> +
> + spin_lock_irqsave(&tpm->lock, flags);
> +
> + if (state->period != curstate.period ||
> + state->duty_cycle != curstate.duty_cycle ||
> + state->polarity != curstate.polarity)
> + imx_tpm_pwm_config(chip, pwm, state);
> +
> + if (state->enabled != curstate.enabled)
> + imx_tpm_pwm_enable(chip, pwm, state->enabled);
This is wrong. This sequence:
pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled = true });
pwm_apply_state(pwm, { .duty_cycle = 10000, .period = 10000, .enabled = false });
must keep the output pin low which isn't guaranteed here.
> +
> + spin_unlock_irqrestore(&tpm->lock, flags);
> +
> + return 0;
> +}
I didn't look in depth through the complete driver yet, but there is
IIRC still some feedback on v1 that wasn't addressed in v2 (because v2
was sent before the last feedback). So I will look in more depth in v3
(assuming it comes late enough address the concerns from this mail).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-14 9:17 ` Uwe Kleine-König
(?)
@ 2019-03-14 9:19 ` Uwe Kleine-König
-1 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 9:19 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel, dl-linux-imx
On Thu, Mar 14, 2019 at 10:17:02AM +0100, Uwe Kleine-König wrote:
> I didn't look in depth through the complete driver yet, but there is
> IIRC still some feedback on v1 that wasn't addressed in v2 (because v2
> was sent before the last feedback). So I will look in more depth in v3
> (assuming it comes late enough address the concerns from this mail).
I just noticed v3 in my inbox. So my concerns here are not addressed.
So I will wait for v4.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 9:19 ` Uwe Kleine-König
0 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 9:19 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Leonard Crestez, schnitzeltony, otavio,
devicetree, shawnguo, s.hauer, jan.tuerk, linux, stefan,
linux-kernel, robh+dt, thierry.reding, dl-linux-imx, kernel,
Robin Gong, festevam, linux-arm-kernel
On Thu, Mar 14, 2019 at 10:17:02AM +0100, Uwe Kleine-König wrote:
> I didn't look in depth through the complete driver yet, but there is
> IIRC still some feedback on v1 that wasn't addressed in v2 (because v2
> was sent before the last feedback). So I will look in more depth in v3
> (assuming it comes late enough address the concerns from this mail).
I just noticed v3 in my inbox. So my concerns here are not addressed.
So I will wait for v4.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
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] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 9:19 ` Uwe Kleine-König
0 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 9:19 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez
On Thu, Mar 14, 2019 at 10:17:02AM +0100, Uwe Kleine-König wrote:
> I didn't look in depth through the complete driver yet, but there is
> IIRC still some feedback on v1 that wasn't addressed in v2 (because v2
> was sent before the last feedback). So I will look in more depth in v3
> (assuming it comes late enough address the concerns from this mail).
I just noticed v3 in my inbox. So my concerns here are not addressed.
So I will wait for v4.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-14 9:17 ` Uwe Kleine-König
(?)
@ 2019-03-14 9:49 ` Anson Huang
-1 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 9:49 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, stefan, otavio, Leonard Crestez, schnitzeltony,
jan.tuerk, Robin Gong, linux-pwm, devicetree, linux-arm-kernel,
linux-kernel, dl-linux-imx
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > - improve coding style, function name's prefix;
> > - improve Kconfig's help info;
> > - use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > - improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > makes the register read/write simple;
> > - improve prescale calculation;
> > - add error message print during probe for ioremap and clk get;
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC
>
> Can you please make this
>
> depends on ARCH_MXC || COMPILE_TEST
> depends on HAVE_CLK && HAS_IOMEM
>
OK.
> > + 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
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.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>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL 0x8
> > +#define TPM_SC 0x10
> > +#define TPM_CNT 0x14
> > +#define TPM_MOD 0x18
> > +#define TPM_C0SC 0x20
> > +#define TPM_C0V 0x24
>
> PWM_IMX_TPM_COV etc. please
I thought with IMX_ as prefix should be enough, but OK, I can change it to PWM_IMX_
>
> > +
> > +#define TPM_SC_CMOD_SHIFT 3
> > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
>
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.
>
Already use GENMASK(h,l) in V3.
> > +#define TPM_SC_CPWMS BIT(5)
> > +
> > +#define TPM_CnSC_CHF BIT(7)
> > +#define TPM_CnSC_MSnB BIT(5)
> > +#define TPM_CnSC_MSnA BIT(4)
> > +#define TPM_CnSC_ELSnB BIT(3)
> > +#define TPM_CnSC_ELSnA BIT(2)
> > +
> > +#define TPM_SC_PS_MASK 0x7
> > +#define TPM_MOD_MOD_MASK 0xffff
> > +
> > +#define TPM_COUNT_MAX 0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET 0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
>
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
>
> u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
>
> I wonder if the actual number of PWMs can be bigger than the default.
OK, will use _MAX_ for this, I double checked i.MX7ULP RM, looks like some TPM
modules have ONLY 2 channel, but some have 6 channels, so I will change it to 6.
>
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + spinlock_t lock;
> > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + unsigned int period_cnt;
> > + u32 val, div;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val < TPM_COUNT_MAX)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
>
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?
Ah, yes, when counter reach the TPM_COUNT_MAX, next value will from 0 in
counting up mode we used, will use TPM_COUNT_MAX + 1.
>
> > + if (div > TPM_SC_PS_MASK) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return;
>
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.
OK.
>
> > + }
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + TPM_SC);
> > + val &= ~TPM_SC_PS_MASK;
> > + val |= div;
> > + writel(val, tpm->base + TPM_SC);
>
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.
OK, I will make sure counter is disabled before writing it.
>
> > + /* set period counter */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
>
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)
Currently, we ONLY initialize the MOD register ONCE, so it does NOT have this case,
if we make the MOD register can be updated, then maybe I can read back the register
value after writing the new value, to make sure the register value is what we want to write?
>
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + static bool tpm_cnt_initialized;
> > + unsigned int duty_cnt;
> > + u32 val;
> > + u64 tmp;
> > +
> > + /*
> > + * TPM counter is shared by multi channels, let's make it to be
> > + * ONLY first channel can config TPM counter's precale and period
> > + * count.
> > + */
> > + if (!tpm_cnt_initialized) {
> > + imx_tpm_pwm_config_counter(chip, state->period);
> > + tpm_cnt_initialized = true;
> > + }
>
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.
OK, maybe I can add check for other channels' statue here, and allow the period
update if ONLY 1 channel is enabled.
>
> > + /* set duty counter */
> > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > + tmp *= state->duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
>
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
I think that is 1 limitation here, the dts should make sure the period used for
different channels are same or at least they can share same divider, otherwise,
what if multiple channels can NOT find a divider good for every channel? How to
deal with this case?
>
> > [...]
> > +static int imx_tpm_pwm_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;
> > + unsigned long flags;
> > +
> > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > + spin_lock_irqsave(&tpm->lock, flags);
> > +
> > + if (state->period != curstate.period ||
> > + state->duty_cycle != curstate.duty_cycle ||
> > + state->polarity != curstate.polarity)
> > + imx_tpm_pwm_config(chip, pwm, state);
> > +
> > + if (state->enabled != curstate.enabled)
> > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
>
> This is wrong. This sequence:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
>
> must keep the output pin low which isn't guaranteed here.
So you mean for every .apply operation, the channel MUST be disabled first, then config
it, then enable it?
>
> > +
> > + spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > + return 0;
> > +}
>
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).
Thanks, will address all comments before sending out V4.
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 9:49 ` Anson Huang
0 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 9:49 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel, dl-linux-imx
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > - improve coding style, function name's prefix;
> > - improve Kconfig's help info;
> > - use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > - improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > makes the register read/write simple;
> > - improve prescale calculation;
> > - add error message print during probe for ioremap and clk get;
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC
>
> Can you please make this
>
> depends on ARCH_MXC || COMPILE_TEST
> depends on HAVE_CLK && HAS_IOMEM
>
OK.
> > + 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
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.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>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL 0x8
> > +#define TPM_SC 0x10
> > +#define TPM_CNT 0x14
> > +#define TPM_MOD 0x18
> > +#define TPM_C0SC 0x20
> > +#define TPM_C0V 0x24
>
> PWM_IMX_TPM_COV etc. please
I thought with IMX_ as prefix should be enough, but OK, I can change it to PWM_IMX_
>
> > +
> > +#define TPM_SC_CMOD_SHIFT 3
> > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
>
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.
>
Already use GENMASK(h,l) in V3.
> > +#define TPM_SC_CPWMS BIT(5)
> > +
> > +#define TPM_CnSC_CHF BIT(7)
> > +#define TPM_CnSC_MSnB BIT(5)
> > +#define TPM_CnSC_MSnA BIT(4)
> > +#define TPM_CnSC_ELSnB BIT(3)
> > +#define TPM_CnSC_ELSnA BIT(2)
> > +
> > +#define TPM_SC_PS_MASK 0x7
> > +#define TPM_MOD_MOD_MASK 0xffff
> > +
> > +#define TPM_COUNT_MAX 0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET 0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
>
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
>
> u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
>
> I wonder if the actual number of PWMs can be bigger than the default.
OK, will use _MAX_ for this, I double checked i.MX7ULP RM, looks like some TPM
modules have ONLY 2 channel, but some have 6 channels, so I will change it to 6.
>
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + spinlock_t lock;
> > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + unsigned int period_cnt;
> > + u32 val, div;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val < TPM_COUNT_MAX)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
>
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?
Ah, yes, when counter reach the TPM_COUNT_MAX, next value will from 0 in
counting up mode we used, will use TPM_COUNT_MAX + 1.
>
> > + if (div > TPM_SC_PS_MASK) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return;
>
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.
OK.
>
> > + }
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + TPM_SC);
> > + val &= ~TPM_SC_PS_MASK;
> > + val |= div;
> > + writel(val, tpm->base + TPM_SC);
>
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.
OK, I will make sure counter is disabled before writing it.
>
> > + /* set period counter */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
>
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)
Currently, we ONLY initialize the MOD register ONCE, so it does NOT have this case,
if we make the MOD register can be updated, then maybe I can read back the register
value after writing the new value, to make sure the register value is what we want to write?
>
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + static bool tpm_cnt_initialized;
> > + unsigned int duty_cnt;
> > + u32 val;
> > + u64 tmp;
> > +
> > + /*
> > + * TPM counter is shared by multi channels, let's make it to be
> > + * ONLY first channel can config TPM counter's precale and period
> > + * count.
> > + */
> > + if (!tpm_cnt_initialized) {
> > + imx_tpm_pwm_config_counter(chip, state->period);
> > + tpm_cnt_initialized = true;
> > + }
>
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.
OK, maybe I can add check for other channels' statue here, and allow the period
update if ONLY 1 channel is enabled.
>
> > + /* set duty counter */
> > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > + tmp *= state->duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
>
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
I think that is 1 limitation here, the dts should make sure the period used for
different channels are same or at least they can share same divider, otherwise,
what if multiple channels can NOT find a divider good for every channel? How to
deal with this case?
>
> > [...]
> > +static int imx_tpm_pwm_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;
> > + unsigned long flags;
> > +
> > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > + spin_lock_irqsave(&tpm->lock, flags);
> > +
> > + if (state->period != curstate.period ||
> > + state->duty_cycle != curstate.duty_cycle ||
> > + state->polarity != curstate.polarity)
> > + imx_tpm_pwm_config(chip, pwm, state);
> > +
> > + if (state->enabled != curstate.enabled)
> > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
>
> This is wrong. This sequence:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
>
> must keep the output pin low which isn't guaranteed here.
So you mean for every .apply operation, the channel MUST be disabled first, then config
it, then enable it?
>
> > +
> > + spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > + return 0;
> > +}
>
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).
Thanks, will address all comments before sending out V4.
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
_______________________________________________
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] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 9:49 ` Anson Huang
0 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 9:49 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel@lists.infradead.org
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > - improve coding style, function name's prefix;
> > - improve Kconfig's help info;
> > - use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > - improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > makes the register read/write simple;
> > - improve prescale calculation;
> > - add error message print during probe for ioremap and clk get;
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC
>
> Can you please make this
>
> depends on ARCH_MXC || COMPILE_TEST
> depends on HAVE_CLK && HAS_IOMEM
>
OK.
> > + 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
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.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>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL 0x8
> > +#define TPM_SC 0x10
> > +#define TPM_CNT 0x14
> > +#define TPM_MOD 0x18
> > +#define TPM_C0SC 0x20
> > +#define TPM_C0V 0x24
>
> PWM_IMX_TPM_COV etc. please
I thought with IMX_ as prefix should be enough, but OK, I can change it to PWM_IMX_
>
> > +
> > +#define TPM_SC_CMOD_SHIFT 3
> > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
>
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.
>
Already use GENMASK(h,l) in V3.
> > +#define TPM_SC_CPWMS BIT(5)
> > +
> > +#define TPM_CnSC_CHF BIT(7)
> > +#define TPM_CnSC_MSnB BIT(5)
> > +#define TPM_CnSC_MSnA BIT(4)
> > +#define TPM_CnSC_ELSnB BIT(3)
> > +#define TPM_CnSC_ELSnA BIT(2)
> > +
> > +#define TPM_SC_PS_MASK 0x7
> > +#define TPM_MOD_MOD_MASK 0xffff
> > +
> > +#define TPM_COUNT_MAX 0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET 0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
>
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
>
> u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
>
> I wonder if the actual number of PWMs can be bigger than the default.
OK, will use _MAX_ for this, I double checked i.MX7ULP RM, looks like some TPM
modules have ONLY 2 channel, but some have 6 channels, so I will change it to 6.
>
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + spinlock_t lock;
> > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + unsigned int period_cnt;
> > + u32 val, div;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val < TPM_COUNT_MAX)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
>
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?
Ah, yes, when counter reach the TPM_COUNT_MAX, next value will from 0 in
counting up mode we used, will use TPM_COUNT_MAX + 1.
>
> > + if (div > TPM_SC_PS_MASK) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return;
>
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.
OK.
>
> > + }
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + TPM_SC);
> > + val &= ~TPM_SC_PS_MASK;
> > + val |= div;
> > + writel(val, tpm->base + TPM_SC);
>
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.
OK, I will make sure counter is disabled before writing it.
>
> > + /* set period counter */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
>
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)
Currently, we ONLY initialize the MOD register ONCE, so it does NOT have this case,
if we make the MOD register can be updated, then maybe I can read back the register
value after writing the new value, to make sure the register value is what we want to write?
>
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + static bool tpm_cnt_initialized;
> > + unsigned int duty_cnt;
> > + u32 val;
> > + u64 tmp;
> > +
> > + /*
> > + * TPM counter is shared by multi channels, let's make it to be
> > + * ONLY first channel can config TPM counter's precale and period
> > + * count.
> > + */
> > + if (!tpm_cnt_initialized) {
> > + imx_tpm_pwm_config_counter(chip, state->period);
> > + tpm_cnt_initialized = true;
> > + }
>
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.
OK, maybe I can add check for other channels' statue here, and allow the period
update if ONLY 1 channel is enabled.
>
> > + /* set duty counter */
> > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > + tmp *= state->duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
>
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
I think that is 1 limitation here, the dts should make sure the period used for
different channels are same or at least they can share same divider, otherwise,
what if multiple channels can NOT find a divider good for every channel? How to
deal with this case?
>
> > [...]
> > +static int imx_tpm_pwm_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;
> > + unsigned long flags;
> > +
> > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > + spin_lock_irqsave(&tpm->lock, flags);
> > +
> > + if (state->period != curstate.period ||
> > + state->duty_cycle != curstate.duty_cycle ||
> > + state->polarity != curstate.polarity)
> > + imx_tpm_pwm_config(chip, pwm, state);
> > +
> > + if (state->enabled != curstate.enabled)
> > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
>
> This is wrong. This sequence:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
>
> must keep the output pin low which isn't guaranteed here.
So you mean for every .apply operation, the channel MUST be disabled first, then config
it, then enable it?
>
> > +
> > + spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > + return 0;
> > +}
>
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).
Thanks, will address all comments before sending out V4.
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
_______________________________________________
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] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-14 9:49 ` Anson Huang
(?)
@ 2019-03-14 10:00 ` Uwe Kleine-König
-1 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 10:00 UTC (permalink / raw)
To: Anson Huang
Cc: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, stefan, otavio, Leonard Crestez, schnitzeltony,
jan.tuerk, Robin Gong, linux-pwm, devicetree, linux-arm-kernel,
linux-kernel, dl-linux-imx
Hello,
On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + static bool tpm_cnt_initialized;
> > > + unsigned int duty_cnt;
> > > + u32 val;
> > > + u64 tmp;
> > > +
> > > + /*
> > > + * TPM counter is shared by multi channels, let's make it to be
> > > + * ONLY first channel can config TPM counter's precale and period
> > > + * count.
> > > + */
> > > + if (!tpm_cnt_initialized) {
> > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > + tpm_cnt_initialized = true;
> > > + }
> >
> > So the period can only be configured once. That is not as good as it could be.
> > You should allow a change whenever there is exactly one PWM in use.
>
> OK, maybe I can add check for other channels' statue here, and allow the period
> update if ONLY 1 channel is enabled.
See how the SiFive patch that I already pointed out solves this same
problem.
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > + tmp *= state->duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> >
> > Uah, you use state->period here even though for the 2nd PWM the Divider
> > might not be setup appropriately.
>
> I think that is 1 limitation here, the dts should make sure the period used for
> different channels are same or at least they can share same divider, otherwise,
> what if multiple channels can NOT find a divider good for every channel? How to
> deal with this case?
You should return -ERANGE or -EINVAL for the calls that cannot be
satisfied.
> > > [...]
> > > +static int imx_tpm_pwm_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;
> > > + unsigned long flags;
> > > +
> > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > +
> > > + spin_lock_irqsave(&tpm->lock, flags);
> > > +
> > > + if (state->period != curstate.period ||
> > > + state->duty_cycle != curstate.duty_cycle ||
> > > + state->polarity != curstate.polarity)
> > > + imx_tpm_pwm_config(chip, pwm, state);
> > > +
> > > + if (state->enabled != curstate.enabled)
> > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> >
> > This is wrong. This sequence:
> >
> > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > true });
> > pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> > 10000, .enabled = false });
> >
> > must keep the output pin low which isn't guaranteed here.
>
> So you mean for every .apply operation, the channel MUST be disabled first, then config
> it, then enable it?
No. I only say that you should not configure the new period and duty
cycle if in the end the hardware should be disabled. Always disabling is
wrong, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 10:00 ` Uwe Kleine-König
0 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 10:00 UTC (permalink / raw)
To: Anson Huang
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel, dl-linux-imx
Hello,
On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + static bool tpm_cnt_initialized;
> > > + unsigned int duty_cnt;
> > > + u32 val;
> > > + u64 tmp;
> > > +
> > > + /*
> > > + * TPM counter is shared by multi channels, let's make it to be
> > > + * ONLY first channel can config TPM counter's precale and period
> > > + * count.
> > > + */
> > > + if (!tpm_cnt_initialized) {
> > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > + tpm_cnt_initialized = true;
> > > + }
> >
> > So the period can only be configured once. That is not as good as it could be.
> > You should allow a change whenever there is exactly one PWM in use.
>
> OK, maybe I can add check for other channels' statue here, and allow the period
> update if ONLY 1 channel is enabled.
See how the SiFive patch that I already pointed out solves this same
problem.
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > + tmp *= state->duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> >
> > Uah, you use state->period here even though for the 2nd PWM the Divider
> > might not be setup appropriately.
>
> I think that is 1 limitation here, the dts should make sure the period used for
> different channels are same or at least they can share same divider, otherwise,
> what if multiple channels can NOT find a divider good for every channel? How to
> deal with this case?
You should return -ERANGE or -EINVAL for the calls that cannot be
satisfied.
> > > [...]
> > > +static int imx_tpm_pwm_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;
> > > + unsigned long flags;
> > > +
> > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > +
> > > + spin_lock_irqsave(&tpm->lock, flags);
> > > +
> > > + if (state->period != curstate.period ||
> > > + state->duty_cycle != curstate.duty_cycle ||
> > > + state->polarity != curstate.polarity)
> > > + imx_tpm_pwm_config(chip, pwm, state);
> > > +
> > > + if (state->enabled != curstate.enabled)
> > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> >
> > This is wrong. This sequence:
> >
> > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > true });
> > pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> > 10000, .enabled = false });
> >
> > must keep the output pin low which isn't guaranteed here.
>
> So you mean for every .apply operation, the channel MUST be disabled first, then config
> it, then enable it?
No. I only say that you should not configure the new period and duty
cycle if in the end the hardware should be disabled. Always disabling is
wrong, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
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] 36+ messages in thread
* Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 10:00 ` Uwe Kleine-König
0 siblings, 0 replies; 36+ messages in thread
From: Uwe Kleine-König @ 2019-03-14 10:00 UTC (permalink / raw)
To: Anson Huang
Cc: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, stefan, otavio, Leonard Crestez, schnitzeltony,
jan.tuerk, Robin Gong, linux-pwm, devicetree
Hello,
On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + static bool tpm_cnt_initialized;
> > > + unsigned int duty_cnt;
> > > + u32 val;
> > > + u64 tmp;
> > > +
> > > + /*
> > > + * TPM counter is shared by multi channels, let's make it to be
> > > + * ONLY first channel can config TPM counter's precale and period
> > > + * count.
> > > + */
> > > + if (!tpm_cnt_initialized) {
> > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > + tpm_cnt_initialized = true;
> > > + }
> >
> > So the period can only be configured once. That is not as good as it could be.
> > You should allow a change whenever there is exactly one PWM in use.
>
> OK, maybe I can add check for other channels' statue here, and allow the period
> update if ONLY 1 channel is enabled.
See how the SiFive patch that I already pointed out solves this same
problem.
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > + tmp *= state->duty_cycle;
> > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> >
> > Uah, you use state->period here even though for the 2nd PWM the Divider
> > might not be setup appropriately.
>
> I think that is 1 limitation here, the dts should make sure the period used for
> different channels are same or at least they can share same divider, otherwise,
> what if multiple channels can NOT find a divider good for every channel? How to
> deal with this case?
You should return -ERANGE or -EINVAL for the calls that cannot be
satisfied.
> > > [...]
> > > +static int imx_tpm_pwm_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;
> > > + unsigned long flags;
> > > +
> > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > +
> > > + spin_lock_irqsave(&tpm->lock, flags);
> > > +
> > > + if (state->period != curstate.period ||
> > > + state->duty_cycle != curstate.duty_cycle ||
> > > + state->polarity != curstate.polarity)
> > > + imx_tpm_pwm_config(chip, pwm, state);
> > > +
> > > + if (state->enabled != curstate.enabled)
> > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> >
> > This is wrong. This sequence:
> >
> > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > true });
> > pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> > 10000, .enabled = false });
> >
> > must keep the output pin low which isn't guaranteed here.
>
> So you mean for every .apply operation, the channel MUST be disabled first, then config
> it, then enable it?
No. I only say that you should not configure the new period and duty
cycle if in the end the hardware should be disabled. Always disabling is
wrong, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-14 10:00 ` Uwe Kleine-König
(?)
@ 2019-03-14 14:25 ` Anson Huang
-1 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 14:25 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, stefan, otavio, Leonard Crestez, schnitzeltony,
jan.tuerk, Robin Gong, linux-pwm, devicetree, linux-arm-kernel,
linux-kernel, dl-linux-imx
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 18:01
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + static bool tpm_cnt_initialized;
> > > > + unsigned int duty_cnt;
> > > > + u32 val;
> > > > + u64 tmp;
> > > > +
> > > > + /*
> > > > + * TPM counter is shared by multi channels, let's make it to be
> > > > + * ONLY first channel can config TPM counter's precale and period
> > > > + * count.
> > > > + */
> > > > + if (!tpm_cnt_initialized) {
> > > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > > + tpm_cnt_initialized = true;
> > > > + }
> > >
> > > So the period can only be configured once. That is not as good as it could
> be.
> > > You should allow a change whenever there is exactly one PWM in use.
> >
> > OK, maybe I can add check for other channels' statue here, and allow
> > the period update if ONLY 1 channel is enabled.
>
> See how the SiFive patch that I already pointed out solves this same problem.
Thanks, I will add a user_count to determine how many channels are used.
>
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > > + tmp *= state->duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > >
> > > Uah, you use state->period here even though for the 2nd PWM the
> > > Divider might not be setup appropriately.
> >
> > I think that is 1 limitation here, the dts should make sure the period
> > used for different channels are same or at least they can share same
> > divider, otherwise, what if multiple channels can NOT find a divider
> > good for every channel? How to deal with this case?
>
> You should return -ERANGE or -EINVAL for the calls that cannot be satisfied.
In my latest implementation, I have added the period setting check at the beginning
of the .apply, as the limitation states, all channels should use same period settings, so
the period here is same for all channels, once it is different, the .apply call will return failed
and the duty cycle setting will be skipped as well.
232 if (state->period != curstate.period) {
233 /*
234 * TPM counter is shared by multiple channels, so the prescale
235 * and period can NOT be modified when any other channel is used.
236 */
237 if (tpm->user_count != 1)
238 return -EBUSY;
239 ret = imx_tpm_pwm_config_counter(chip, state->period);
240 if (ret)
241 return ret;
242 }
243
244 if (!state->enabled)
245 duty_cycle = 0;
246
247 if (state->duty_cycle != curstate.duty_cycle ||
248 state->polarity != curstate.polarity)
249 imx_tpm_pwm_config(chip, pwm,
250 state->period, duty_cycle, state->polarity);
>
> > > > [...]
> > > > +static int imx_tpm_pwm_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;
> > > > + unsigned long flags;
> > > > +
> > > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + spin_lock_irqsave(&tpm->lock, flags);
> > > > +
> > > > + if (state->period != curstate.period ||
> > > > + state->duty_cycle != curstate.duty_cycle ||
> > > > + state->polarity != curstate.polarity)
> > > > + imx_tpm_pwm_config(chip, pwm, state);
> > > > +
> > > > + if (state->enabled != curstate.enabled)
> > > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> > >
> > > This is wrong. This sequence:
> > >
> > > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > > true });
> > > pwm_apply_state(pwm, { .duty_cycle = 10000, .period = 10000,
> > > .enabled = false });
> > >
> > > must keep the output pin low which isn't guaranteed here.
> >
> > So you mean for every .apply operation, the channel MUST be disabled
> > first, then config it, then enable it?
>
> No. I only say that you should not configure the new period and duty cycle if
> in the end the hardware should be disabled. Always disabling is wrong, too.
>
Understand now, I will check the state->enable first, if it is false, I will set the duty_cycle
to 0 before configuring the channel.
Anson.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C6f3
> 0bb40fb9d460cd73a08d6a863f4c9%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636881544600449875&sdata=SRBTn%2BcquzRRRLrzOTG
> vlC5WNvof28J0%2B77iSxtFVYA%3D&reserved=0 |
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 14:25 ` Anson Huang
0 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 14:25 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel, dl-linux-imx
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 18:01
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + static bool tpm_cnt_initialized;
> > > > + unsigned int duty_cnt;
> > > > + u32 val;
> > > > + u64 tmp;
> > > > +
> > > > + /*
> > > > + * TPM counter is shared by multi channels, let's make it to be
> > > > + * ONLY first channel can config TPM counter's precale and period
> > > > + * count.
> > > > + */
> > > > + if (!tpm_cnt_initialized) {
> > > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > > + tpm_cnt_initialized = true;
> > > > + }
> > >
> > > So the period can only be configured once. That is not as good as it could
> be.
> > > You should allow a change whenever there is exactly one PWM in use.
> >
> > OK, maybe I can add check for other channels' statue here, and allow
> > the period update if ONLY 1 channel is enabled.
>
> See how the SiFive patch that I already pointed out solves this same problem.
Thanks, I will add a user_count to determine how many channels are used.
>
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > > + tmp *= state->duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > >
> > > Uah, you use state->period here even though for the 2nd PWM the
> > > Divider might not be setup appropriately.
> >
> > I think that is 1 limitation here, the dts should make sure the period
> > used for different channels are same or at least they can share same
> > divider, otherwise, what if multiple channels can NOT find a divider
> > good for every channel? How to deal with this case?
>
> You should return -ERANGE or -EINVAL for the calls that cannot be satisfied.
In my latest implementation, I have added the period setting check at the beginning
of the .apply, as the limitation states, all channels should use same period settings, so
the period here is same for all channels, once it is different, the .apply call will return failed
and the duty cycle setting will be skipped as well.
232 if (state->period != curstate.period) {
233 /*
234 * TPM counter is shared by multiple channels, so the prescale
235 * and period can NOT be modified when any other channel is used.
236 */
237 if (tpm->user_count != 1)
238 return -EBUSY;
239 ret = imx_tpm_pwm_config_counter(chip, state->period);
240 if (ret)
241 return ret;
242 }
243
244 if (!state->enabled)
245 duty_cycle = 0;
246
247 if (state->duty_cycle != curstate.duty_cycle ||
248 state->polarity != curstate.polarity)
249 imx_tpm_pwm_config(chip, pwm,
250 state->period, duty_cycle, state->polarity);
>
> > > > [...]
> > > > +static int imx_tpm_pwm_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;
> > > > + unsigned long flags;
> > > > +
> > > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + spin_lock_irqsave(&tpm->lock, flags);
> > > > +
> > > > + if (state->period != curstate.period ||
> > > > + state->duty_cycle != curstate.duty_cycle ||
> > > > + state->polarity != curstate.polarity)
> > > > + imx_tpm_pwm_config(chip, pwm, state);
> > > > +
> > > > + if (state->enabled != curstate.enabled)
> > > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> > >
> > > This is wrong. This sequence:
> > >
> > > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > > true });
> > > pwm_apply_state(pwm, { .duty_cycle = 10000, .period = 10000,
> > > .enabled = false });
> > >
> > > must keep the output pin low which isn't guaranteed here.
> >
> > So you mean for every .apply operation, the channel MUST be disabled
> > first, then config it, then enable it?
>
> No. I only say that you should not configure the new period and duty cycle if
> in the end the hardware should be disabled. Always disabling is wrong, too.
>
Understand now, I will check the state->enable first, if it is false, I will set the duty_cycle
to 0 before configuring the channel.
Anson.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C6f3
> 0bb40fb9d460cd73a08d6a863f4c9%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636881544600449875&sdata=SRBTn%2BcquzRRRLrzOTG
> vlC5WNvof28J0%2B77iSxtFVYA%3D&reserved=0 |
_______________________________________________
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] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 14:25 ` Anson Huang
0 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 14:25 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, stefan, otavio, Leonard Crestez, schnitzeltony,
jan.tuerk, Robin Gong, linux-pwm, devicetree
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 18:01
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Thu, Mar 14, 2019 at 09:49:06AM +0000, Anson Huang wrote:
> > > On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > > > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + struct pwm_state *state) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + static bool tpm_cnt_initialized;
> > > > + unsigned int duty_cnt;
> > > > + u32 val;
> > > > + u64 tmp;
> > > > +
> > > > + /*
> > > > + * TPM counter is shared by multi channels, let's make it to be
> > > > + * ONLY first channel can config TPM counter's precale and period
> > > > + * count.
> > > > + */
> > > > + if (!tpm_cnt_initialized) {
> > > > + imx_tpm_pwm_config_counter(chip, state->period);
> > > > + tpm_cnt_initialized = true;
> > > > + }
> > >
> > > So the period can only be configured once. That is not as good as it could
> be.
> > > You should allow a change whenever there is exactly one PWM in use.
> >
> > OK, maybe I can add check for other channels' statue here, and allow
> > the period update if ONLY 1 channel is enabled.
>
> See how the SiFive patch that I already pointed out solves this same problem.
Thanks, I will add a user_count to determine how many channels are used.
>
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > > > + tmp *= state->duty_cycle;
> > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > >
> > > Uah, you use state->period here even though for the 2nd PWM the
> > > Divider might not be setup appropriately.
> >
> > I think that is 1 limitation here, the dts should make sure the period
> > used for different channels are same or at least they can share same
> > divider, otherwise, what if multiple channels can NOT find a divider
> > good for every channel? How to deal with this case?
>
> You should return -ERANGE or -EINVAL for the calls that cannot be satisfied.
In my latest implementation, I have added the period setting check at the beginning
of the .apply, as the limitation states, all channels should use same period settings, so
the period here is same for all channels, once it is different, the .apply call will return failed
and the duty cycle setting will be skipped as well.
232 if (state->period != curstate.period) {
233 /*
234 * TPM counter is shared by multiple channels, so the prescale
235 * and period can NOT be modified when any other channel is used.
236 */
237 if (tpm->user_count != 1)
238 return -EBUSY;
239 ret = imx_tpm_pwm_config_counter(chip, state->period);
240 if (ret)
241 return ret;
242 }
243
244 if (!state->enabled)
245 duty_cycle = 0;
246
247 if (state->duty_cycle != curstate.duty_cycle ||
248 state->polarity != curstate.polarity)
249 imx_tpm_pwm_config(chip, pwm,
250 state->period, duty_cycle, state->polarity);
>
> > > > [...]
> > > > +static int imx_tpm_pwm_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;
> > > > + unsigned long flags;
> > > > +
> > > > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > > > +
> > > > + spin_lock_irqsave(&tpm->lock, flags);
> > > > +
> > > > + if (state->period != curstate.period ||
> > > > + state->duty_cycle != curstate.duty_cycle ||
> > > > + state->polarity != curstate.polarity)
> > > > + imx_tpm_pwm_config(chip, pwm, state);
> > > > +
> > > > + if (state->enabled != curstate.enabled)
> > > > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
> > >
> > > This is wrong. This sequence:
> > >
> > > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> > > true });
> > > pwm_apply_state(pwm, { .duty_cycle = 10000, .period = 10000,
> > > .enabled = false });
> > >
> > > must keep the output pin low which isn't guaranteed here.
> >
> > So you mean for every .apply operation, the channel MUST be disabled
> > first, then config it, then enable it?
>
> No. I only say that you should not configure the new period and duty cycle if
> in the end the hardware should be disabled. Always disabling is wrong, too.
>
Understand now, I will check the state->enable first, if it is false, I will set the duty_cycle
to 0 before configuring the channel.
Anson.
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C6f3
> 0bb40fb9d460cd73a08d6a863f4c9%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636881544600449875&sdata=SRBTn%2BcquzRRRLrzOTG
> vlC5WNvof28J0%2B77iSxtFVYA%3D&reserved=0 |
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
2019-03-14 9:17 ` Uwe Kleine-König
(?)
@ 2019-03-14 14:42 ` Anson Huang
-1 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 14:42 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
festevam, linux, stefan, otavio, Leonard Crestez, schnitzeltony,
jan.tuerk, Robin Gong, linux-pwm, devicetree, linux-arm-kernel,
linux-kernel, dl-linux-imx
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > - improve coding style, function name's prefix;
> > - improve Kconfig's help info;
> > - use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > - improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > makes the register read/write simple;
> > - improve prescale calculation;
> > - add error message print during probe for ioremap and clk get;
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC
>
> Can you please make this
>
> depends on ARCH_MXC || COMPILE_TEST
> depends on HAVE_CLK && HAS_IOMEM
Will fix it in V4.
>
> > + 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
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.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>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL 0x8
> > +#define TPM_SC 0x10
> > +#define TPM_CNT 0x14
> > +#define TPM_MOD 0x18
> > +#define TPM_C0SC 0x20
> > +#define TPM_C0V 0x24
>
> PWM_IMX_TPM_COV etc. please
Will fix it in V4.
>
> > +
> > +#define TPM_SC_CMOD_SHIFT 3
> > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
>
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.
I will use below in V4:
#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
>
> > +#define TPM_SC_CPWMS BIT(5)
> > +
> > +#define TPM_CnSC_CHF BIT(7)
> > +#define TPM_CnSC_MSnB BIT(5)
> > +#define TPM_CnSC_MSnA BIT(4)
> > +#define TPM_CnSC_ELSnB BIT(3)
> > +#define TPM_CnSC_ELSnA BIT(2)
> > +
> > +#define TPM_SC_PS_MASK 0x7
> > +#define TPM_MOD_MOD_MASK 0xffff
> > +
> > +#define TPM_COUNT_MAX 0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET 0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
>
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
>
> u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
>
> I wonder if the actual number of PWMs can be bigger than the default.
>
Will use below in V4:
#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + spinlock_t lock;
> > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + unsigned int period_cnt;
> > + u32 val, div;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val < TPM_COUNT_MAX)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
>
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?
Fix it in V4.
>
> > + if (div > TPM_SC_PS_MASK) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return;
>
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.
I will use below for V4:
if (div > PWM_IMX_TPM_SC_PS_MASK) {
dev_err(chip->dev,
"failed to find valid prescale value!\n");
return -EINVAL;
>
> > + }
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + TPM_SC);
> > + val &= ~TPM_SC_PS_MASK;
> > + val |= div;
> > + writel(val, tpm->base + TPM_SC);
>
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.
In V4, I will disable the counter before programming PS, then restore the counter's
CMOD after PS is written.
>
> > + /* set period counter */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
>
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)
From the RM, when CMOD[1:0] = 2b'00, which means when counter is disabled,
the MOD register will be updated once it is written, so I just put the comments
there to avoid confusion, no need to check if there is value pending in MOD register,
as in V4, I already make sure counter is disabled before programming MOD.
>
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + static bool tpm_cnt_initialized;
> > + unsigned int duty_cnt;
> > + u32 val;
> > + u64 tmp;
> > +
> > + /*
> > + * TPM counter is shared by multi channels, let's make it to be
> > + * ONLY first channel can config TPM counter's precale and period
> > + * count.
> > + */
> > + if (!tpm_cnt_initialized) {
> > + imx_tpm_pwm_config_counter(chip, state->period);
> > + tpm_cnt_initialized = true;
> > + }
>
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.
In V4, I added user_count to determine the PWM channels used, if there is ONLY
1 channel used, then period and prescale will can be configured anytime.
>
> > + /* set duty counter */
> > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > + tmp *= state->duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
>
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
>
> > [...]
> > +static int imx_tpm_pwm_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;
> > + unsigned long flags;
> > +
> > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > + spin_lock_irqsave(&tpm->lock, flags);
> > +
> > + if (state->period != curstate.period ||
> > + state->duty_cycle != curstate.duty_cycle ||
> > + state->polarity != curstate.polarity)
> > + imx_tpm_pwm_config(chip, pwm, state);
> > +
> > + if (state->enabled != curstate.enabled)
> > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
>
> This is wrong. This sequence:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
>
> must keep the output pin low which isn't guaranteed here.
>
Already add checking to make sure the upper case NOT happen in V4.
> > +
> > + spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > + return 0;
> > +}
>
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).
Since there are different comments in some mails, I double checked it with the
latest V4 patch and try to make sure I don't miss any comment, please help review
V4 patch, if I missed any comment, then I am sorry for wasting your time. I will send
out V4 patch after I done the function test on board tomorrow.
Thanks,
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 14:42 ` Anson Huang
0 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 14:42 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel, dl-linux-imx
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > - improve coding style, function name's prefix;
> > - improve Kconfig's help info;
> > - use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > - improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > makes the register read/write simple;
> > - improve prescale calculation;
> > - add error message print during probe for ioremap and clk get;
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC
>
> Can you please make this
>
> depends on ARCH_MXC || COMPILE_TEST
> depends on HAVE_CLK && HAS_IOMEM
Will fix it in V4.
>
> > + 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
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.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>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL 0x8
> > +#define TPM_SC 0x10
> > +#define TPM_CNT 0x14
> > +#define TPM_MOD 0x18
> > +#define TPM_C0SC 0x20
> > +#define TPM_C0V 0x24
>
> PWM_IMX_TPM_COV etc. please
Will fix it in V4.
>
> > +
> > +#define TPM_SC_CMOD_SHIFT 3
> > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
>
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.
I will use below in V4:
#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
>
> > +#define TPM_SC_CPWMS BIT(5)
> > +
> > +#define TPM_CnSC_CHF BIT(7)
> > +#define TPM_CnSC_MSnB BIT(5)
> > +#define TPM_CnSC_MSnA BIT(4)
> > +#define TPM_CnSC_ELSnB BIT(3)
> > +#define TPM_CnSC_ELSnA BIT(2)
> > +
> > +#define TPM_SC_PS_MASK 0x7
> > +#define TPM_MOD_MOD_MASK 0xffff
> > +
> > +#define TPM_COUNT_MAX 0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET 0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
>
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
>
> u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
>
> I wonder if the actual number of PWMs can be bigger than the default.
>
Will use below in V4:
#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + spinlock_t lock;
> > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + unsigned int period_cnt;
> > + u32 val, div;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val < TPM_COUNT_MAX)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
>
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?
Fix it in V4.
>
> > + if (div > TPM_SC_PS_MASK) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return;
>
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.
I will use below for V4:
if (div > PWM_IMX_TPM_SC_PS_MASK) {
dev_err(chip->dev,
"failed to find valid prescale value!\n");
return -EINVAL;
>
> > + }
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + TPM_SC);
> > + val &= ~TPM_SC_PS_MASK;
> > + val |= div;
> > + writel(val, tpm->base + TPM_SC);
>
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.
In V4, I will disable the counter before programming PS, then restore the counter's
CMOD after PS is written.
>
> > + /* set period counter */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
>
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)
From the RM, when CMOD[1:0] = 2b'00, which means when counter is disabled,
the MOD register will be updated once it is written, so I just put the comments
there to avoid confusion, no need to check if there is value pending in MOD register,
as in V4, I already make sure counter is disabled before programming MOD.
>
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + static bool tpm_cnt_initialized;
> > + unsigned int duty_cnt;
> > + u32 val;
> > + u64 tmp;
> > +
> > + /*
> > + * TPM counter is shared by multi channels, let's make it to be
> > + * ONLY first channel can config TPM counter's precale and period
> > + * count.
> > + */
> > + if (!tpm_cnt_initialized) {
> > + imx_tpm_pwm_config_counter(chip, state->period);
> > + tpm_cnt_initialized = true;
> > + }
>
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.
In V4, I added user_count to determine the PWM channels used, if there is ONLY
1 channel used, then period and prescale will can be configured anytime.
>
> > + /* set duty counter */
> > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > + tmp *= state->duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
>
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
>
> > [...]
> > +static int imx_tpm_pwm_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;
> > + unsigned long flags;
> > +
> > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > + spin_lock_irqsave(&tpm->lock, flags);
> > +
> > + if (state->period != curstate.period ||
> > + state->duty_cycle != curstate.duty_cycle ||
> > + state->polarity != curstate.polarity)
> > + imx_tpm_pwm_config(chip, pwm, state);
> > +
> > + if (state->enabled != curstate.enabled)
> > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
>
> This is wrong. This sequence:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
>
> must keep the output pin low which isn't guaranteed here.
>
Already add checking to make sure the upper case NOT happen in V4.
> > +
> > + spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > + return 0;
> > +}
>
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).
Since there are different comments in some mails, I double checked it with the
latest V4 patch and try to make sure I don't miss any comment, please help review
V4 patch, if I missed any comment, then I am sorry for wasting your time. I will send
out V4 patch after I done the function test on board tomorrow.
Thanks,
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
_______________________________________________
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] 36+ messages in thread
* RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
@ 2019-03-14 14:42 ` Anson Huang
0 siblings, 0 replies; 36+ messages in thread
From: Anson Huang @ 2019-03-14 14:42 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: mark.rutland, linux-pwm, Robin Gong, schnitzeltony, otavio,
devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt,
linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez,
shawnguo, linux-arm-kernel@lists.infradead.org
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; 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; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > - improve coding style, function name's prefix;
> > - improve Kconfig's help info;
> > - use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > - improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > makes the register read/write simple;
> > - improve prescale calculation;
> > - add error message print during probe for ioremap and clk get;
> > ---
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > + tristate "i.MX TPM PWM support"
> > + depends on ARCH_MXC
>
> Can you please make this
>
> depends on ARCH_MXC || COMPILE_TEST
> depends on HAVE_CLK && HAS_IOMEM
Will fix it in V4.
>
> > + 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
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o
> > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o
> > obj-$(CONFIG_PWM_IMG) += pwm-img.o
> > obj-$(CONFIG_PWM_IMX) += pwm-imx.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..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.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>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL 0x8
> > +#define TPM_SC 0x10
> > +#define TPM_CNT 0x14
> > +#define TPM_MOD 0x18
> > +#define TPM_C0SC 0x20
> > +#define TPM_C0V 0x24
>
> PWM_IMX_TPM_COV etc. please
Will fix it in V4.
>
> > +
> > +#define TPM_SC_CMOD_SHIFT 3
> > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT)
>
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.
I will use below in V4:
#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3)
#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3)
>
> > +#define TPM_SC_CPWMS BIT(5)
> > +
> > +#define TPM_CnSC_CHF BIT(7)
> > +#define TPM_CnSC_MSnB BIT(5)
> > +#define TPM_CnSC_MSnA BIT(4)
> > +#define TPM_CnSC_ELSnB BIT(3)
> > +#define TPM_CnSC_ELSnA BIT(2)
> > +
> > +#define TPM_SC_PS_MASK 0x7
> > +#define TPM_MOD_MOD_MASK 0xffff
> > +
> > +#define TPM_COUNT_MAX 0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET 0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2
>
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
>
> u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
>
> I wonder if the actual number of PWMs can be bigger than the default.
>
Will use below in V4:
#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6
> > +struct imx_tpm_pwm_chip {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + spinlock_t lock;
> > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + unsigned int period_cnt;
> > + u32 val, div;
> > + u64 tmp;
> > +
> > + tmp = clk_get_rate(tpm->clk);
> > + tmp *= period;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (val < TPM_COUNT_MAX)
> > + div = 0;
> > + else
> > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
>
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?
Fix it in V4.
>
> > + if (div > TPM_SC_PS_MASK) {
> > + dev_err(chip->dev,
> > + "failed to find valid prescale value!\n");
> > + return;
>
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.
I will use below for V4:
if (div > PWM_IMX_TPM_SC_PS_MASK) {
dev_err(chip->dev,
"failed to find valid prescale value!\n");
return -EINVAL;
>
> > + }
> > + /* set TPM counter prescale */
> > + val = readl(tpm->base + TPM_SC);
> > + val &= ~TPM_SC_PS_MASK;
> > + val |= div;
> > + writel(val, tpm->base + TPM_SC);
>
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.
In V4, I will disable the counter before programming PS, then restore the counter's
CMOD after PS is written.
>
> > + /* set period counter */
> > + do_div(tmp, NSEC_PER_SEC);
> > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
>
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)
From the RM, when CMOD[1:0] = 2b'00, which means when counter is disabled,
the MOD register will be updated once it is written, so I just put the comments
there to avoid confusion, no need to check if there is value pending in MOD register,
as in V4, I already make sure counter is disabled before programming MOD.
>
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + static bool tpm_cnt_initialized;
> > + unsigned int duty_cnt;
> > + u32 val;
> > + u64 tmp;
> > +
> > + /*
> > + * TPM counter is shared by multi channels, let's make it to be
> > + * ONLY first channel can config TPM counter's precale and period
> > + * count.
> > + */
> > + if (!tpm_cnt_initialized) {
> > + imx_tpm_pwm_config_counter(chip, state->period);
> > + tpm_cnt_initialized = true;
> > + }
>
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.
In V4, I added user_count to determine the PWM channels used, if there is ONLY
1 channel used, then period and prescale will can be configured anytime.
>
> > + /* set duty counter */
> > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > + tmp *= state->duty_cycle;
> > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
>
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
>
> > [...]
> > +static int imx_tpm_pwm_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;
> > + unsigned long flags;
> > +
> > + imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > + spin_lock_irqsave(&tpm->lock, flags);
> > +
> > + if (state->period != curstate.period ||
> > + state->duty_cycle != curstate.duty_cycle ||
> > + state->polarity != curstate.polarity)
> > + imx_tpm_pwm_config(chip, pwm, state);
> > +
> > + if (state->enabled != curstate.enabled)
> > + imx_tpm_pwm_enable(chip, pwm, state->enabled);
>
> This is wrong. This sequence:
>
> pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
>
> must keep the output pin low which isn't guaranteed here.
>
Already add checking to make sure the upper case NOT happen in V4.
> > +
> > + spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > + return 0;
> > +}
>
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).
Since there are different comments in some mails, I double checked it with the
latest V4 patch and try to make sure I don't miss any comment, please help review
V4 patch, if I missed any comment, then I am sorry for wasting your time. I will send
out V4 patch after I done the function test on board tomorrow.
Thanks,
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
_______________________________________________
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] 36+ messages in thread