diff mbox series

[V8,2/5] pwm: Add i.MX TPM PWM driver support

Message ID 1553128960-17923-3-git-send-email-Anson.Huang@nxp.com (mailing list archive)
State New, archived
Headers show
Series Add i.MX7ULP EVK PWM backlight support | expand

Commit Message

Anson Huang March 21, 2019, 12:47 a.m. UTC
i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
inside, it can support multiple PWM channels, all the channels
share same counter and period setting, but each channel can
configure its duty and polarity independently.

There are several TPM modules in i.MX7ULP, the number of channels
in TPM modules are different, it can be read from each TPM module's
PARAM register.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
changes since V7:
	- improve prescale computation;
	- improve some register definitions;
	- remove unnecessary check for period count check;
	- improve function parameter to use pointer instead of value;
---
 drivers/pwm/Kconfig       |  11 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-imx-tpm.c | 435 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 447 insertions(+)
 create mode 100644 drivers/pwm/pwm-imx-tpm.c

Comments

Uwe Kleine-König March 21, 2019, 9:19 a.m. UTC | #1
Hello,

On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> inside, it can support multiple PWM channels, all the channels
> share same counter and period setting, but each channel can
> configure its duty and polarity independently.
> 
> There are several TPM modules in i.MX7ULP, the number of channels
> in TPM modules are different, it can be read from each TPM module's
> PARAM register.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> changes since V7:
> 	- improve prescale computation;
> 	- improve some register definitions;
> 	- remove unnecessary check for period count check;
> 	- improve function parameter to use pointer instead of value;
> ---
>  drivers/pwm/Kconfig       |  11 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-imx-tpm.c | 435 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 447 insertions(+)
>  create mode 100644 drivers/pwm/pwm-imx-tpm.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 54f8238..3ea0391 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -210,6 +210,17 @@ config PWM_IMX27
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-imx27.
>  
> +config PWM_IMX_TPM
> +	tristate "i.MX TPM PWM support"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	depends on HAVE_CLK && HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for i.MX7ULP TPM module, TPM's full
> +	  name is Low Power Timer/Pulse Width Modulation Module.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-imx-tpm.
> +
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MACH_INGENIC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 448825e..c368599 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
>  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
>  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
> +obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
>  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new file mode 100644
> index 0000000..0efea36
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,435 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + *
> + * Limitations:
> + * - The TPM counter and period counter are shared between
> + *   multiple channels, so all channels should use same period
> + *   settings.

What about:

 - Not all parameters to change the period length can be changed
   atomically. The counter must be stopped to change SC.PS.

 - Changes to polarity cannot be latched at the time of the next period
   start.

?

> + */
> +
> +#include <linux/bitfield.h>
> +#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>
> +
> +#define PWM_IMX_TPM_PARAM	0x4
> +#define PWM_IMX_TPM_GLOBAL	0x8
> +#define PWM_IMX_TPM_SC		0x10
> +#define PWM_IMX_TPM_CNT		0x14
> +#define PWM_IMX_TPM_MOD		0x18
> +#define PWM_IMX_TPM_CnSC(n)	(0x20 + (n) * 0x8)
> +#define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
> +
> +#define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7, 0)
> +
> +#define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
> +#define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
> +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK	FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1)
> +#define PWM_IMX_TPM_SC_CPWMS			BIT(5)
> +
> +#define PWM_IMX_TPM_CnSC_CHF	BIT(7)
> +#define PWM_IMX_TPM_CnSC_MSB	BIT(5)
> +#define PWM_IMX_TPM_CnSC_MSA	BIT(4)
> +
> +/*
> + * The reference manual describes this field as two separate bits. The
> + * samantic of the two bits isn't orthogonal though, so they are treated
> + * together as a 2-bit field here.
> + */
> +#define PWM_IMX_TPM_CnSC_ELS	GENMASK(3, 2)
> +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED	0x1
> +#define PWM_IMX_TPM_CnSC_ELS_INVERSED	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1)
> +#define PWM_IMX_TPM_CnSC_ELS_NORMAL	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2)
> +
> +
> +#define PWM_IMX_TPM_MOD_WIDTH	16
> +#define PWM_IMX_TPM_MOD_MOD	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)
> +
> +struct imx_tpm_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	void __iomem *base;
> +	struct mutex lock;
> +	u32 user_count;
> +	u32 enable_count;
> +	u32 real_period;
> +};
> +
> +struct imx_tpm_pwm_param {
> +	u8 prescale;
> +	u32 mod;
> +};
> +
> +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct imx_tpm_pwm_chip, chip);
> +}
> +
> +static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
> +				   struct imx_tpm_pwm_param *p,
> +				   struct pwm_state *state,
> +				   struct pwm_state *real_state)
> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	u32 rate, prescale, period_count, clock_unit;
> +	u64 tmp;
> +
> +	rate = clk_get_rate(tpm->clk);
> +	tmp = (u64)state->period * rate;
> +	clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> +	if (clock_unit <= PWM_IMX_TPM_MOD_MOD)
> +		prescale = 0;
> +	else
> +		prescale = ilog2(clock_unit) + 1 - PWM_IMX_TPM_MOD_WIDTH;
> +
> +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> +		return -ERANGE;
> +	p->prescale = prescale;
> +
> +	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
> +	p->mod = period_count;
> +
> +	/* calculate real period HW can support */
> +	tmp = (u64)period_count << prescale;
> +	tmp *= NSEC_PER_SEC;
> +	real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> +	/*
> +	 * if eventually the PWM output is inactive, either
> +	 * duty cycle is 0 or status is disabled, need to
> +	 * make sure the output pin is inactive.
> +	 */
> +	if (!state->enabled)
> +		real_state->duty_cycle = 0;
> +	else
> +		real_state->duty_cycle = state->duty_cycle;

You're maybe lying about the duty cycle here. Also it would be more
consistent to calculate the value to be written into the CnV register
that defines the duty cycle here.

Regarding the period computation I'm happy with this function. Unless I
miss something this function is idempotent (i.e. doing

	pwm_imx_tpm_round_state(chip, &p, some_state, &real_state1);
	pwm_imx_tpm_round_state(chip, &p, &real_state1, &real_state2);

results in real_state1 == real_state2) given that
clk_get_rate(tpm->clk) < NSEC_PER_SEC.

> +	real_state->polarity = state->polarity;
> +	real_state->enabled = state->enabled;
> +
> +	return 0;
> +}
> +

A comment here noting that pwm_imx_tpm_setup_period is supposed to be
called with the mutex hold would be good here.

> +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> +				     struct imx_tpm_pwm_param *p)
> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	u32 val, saved_cmod, cur_prescale;
> +
> +	/* make sure counter is disabled for programming prescale */

@Thierry: What is your thought here? IMHO this should only be allowed if
all affected PWMs are off.

> +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> +	cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> +	if (saved_cmod && cur_prescale != p->prescale) {
> +		val &= ~PWM_IMX_TPM_SC_CMOD;
> +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> +	}
> +
> +	/* set TPM counter prescale */
> +	val &= ~PWM_IMX_TPM_SC_PS;
> +	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
> +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> +
> +	/* restore the clock mode if necessary */
> +	if (saved_cmod && cur_prescale != p->prescale) {
> +		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> +	}
> +
> +	/*
> +	 * set period count:
> +	 * according to RM, the MOD register is updated immediately
> +	 * if CMOD[1:0] = 2b'00. if CMOD[1:0] != 2b'00, then MOD
> +	 * register is updated according to the CPWMS bit, that is:
> +	 *
> +	 * if the selected mode is not CPWM then MOD register is
> +	 * updated after MOD register was written and the TPM
> +	 * counter changes from MOD to zero.
> +	 *
> +	 * if the selected mode is CPWM then MOD register is updated
> +	 * after MOD register was written and the TPM counter changes
> +	 * from MOD to (MOD – 1).
> +	 */
> +	writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
> +}
> +
> +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  struct pwm_state *state)
> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	u32 rate, val, prescale;
> +	u64 tmp;
> +
> +	/* get period */
> +	state->period = tpm->real_period;
> +
> +	/* get duty cycle */
> +	rate = clk_get_rate(tpm->clk);
> +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> +	prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> +	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> +	tmp = (tmp << prescale) * NSEC_PER_SEC;
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> +	/* get polarity */
> +	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +	if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
> +	    PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	else
> +		/*
> +		 * Assume reserved values (2b00 and 2b11) to yield
> +		 * normal polarity.

Given that this driver writes PWM_IMX_TPM_CnSC_ELS = 2b00 in some
situations assuming that this results in an constant inactive output,
this should be recognized here. (Not entirely sure the output is
inactive because of only PWM_IMX_TPM_CnSC_ELS = 2b00.)

> +		 */
> +		state->polarity = PWM_POLARITY_NORMAL;
> +
> +	/* get channel status */
> +	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
> +}
> +
> +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> +				 struct pwm_device *pwm,
> +				 struct pwm_state *state)

pwm_imx_tpm_apply_hw is called with the mutex hold. Is this necessary?
Please either call it without the mutex or annotate the function that
the caller is supposed to hold it.

> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	struct pwm_state c;
> +	u32 val, sc_val;
> +	u64 tmp;
> +
> +	pwm_imx_tpm_get_state(chip, pwm, &c);
> +
> +	if (state->duty_cycle != c.duty_cycle) {
> +		/* set duty counter */
> +		tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
> +		tmp *= state->duty_cycle;
> +		val = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> +		writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));

How does this affect a currently running PWM? Consider it runs at
duty_cycle=500 + period=1000 and now should change to duty_cycle=700 +
period=800. Can it happen that we see a or even several periods with
duty_cycle=700 and period=1000?

> +	}
> +
> +	if (state->enabled != c.enabled) {

If the PWM was already on and is changed to another enabled state,
you're ignoring the (maybe) new polarity here.

> +		/*
> +		 * set polarity (for edge-aligned PWM modes)
> +		 *
> +		 * ELS[1:0] = 2b10 yields normal polarity behaviour,
> +		 * ELS[1:0] = 2b01 yields inversed polarity.
> +		 * The other values are reserved.
> +		 *
> +		 * polarity settings will enabled/disable output status
> +		 * immediately, so if the channel is disabled, need to
> +		 * make sure MSA/MSB/ELS are set to 0 which means channel
> +		 * disabled.
> +		 */
> +		val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +		val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA |
> +			 PWM_IMX_TPM_CnSC_MSB);
> +		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
> +		if (state->enabled) {
> +			val |= PWM_IMX_TPM_CnSC_MSB;
> +			val |= (state->polarity == PWM_POLARITY_NORMAL) ?
> +				PWM_IMX_TPM_CnSC_ELS_NORMAL :
> +				PWM_IMX_TPM_CnSC_ELS_INVERSED;
> +			if (++tpm->enable_count == 1) {
> +				/* start TPM counter */
> +				sc_val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> +				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
> +			}
> +		} else {
> +			if (--tpm->enable_count == 0) {
> +				/* stop TPM counter */
> +				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
> +				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
> +			}
> +		}
> +		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +	}
> +}
> +
> +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> +			      struct pwm_device *pwm,
> +			     struct pwm_state *state)
> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	struct imx_tpm_pwm_param param;
> +	struct pwm_state real_state;
> +	int ret;
> +
> +	ret = pwm_imx_tpm_round_state(chip, &param, state, &real_state);
> +	if (ret)
> +		return -EINVAL;
> +
> +	mutex_lock(&tpm->lock);
> +
> +	/*
> +	 * TPM counter is shared by multiple channels, so
> +	 * prescale and period can NOT be modified when
> +	 * there are multiple channels in use with different
> +	 * period settings.
> +	 */
> +	if (real_state.period != tpm->real_period) {
> +		if (tpm->user_count > 1) {
> +			ret = -EBUSY;
> +			goto exit;
> +		}
> +
> +		pwm_imx_tpm_setup_period(chip, &param);
> +		tpm->real_period = real_state.period;
> +	}
> +
> +	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> +
> +exit:
> +	mutex_unlock(&tpm->lock);

.apply is supposed to sleep until the newly configured state is active.
This is missing here, right?

Best regards
Uwe
Anson Huang March 21, 2019, 9:54 a.m. UTC | #2
Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月21日 17:20
> 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 V8 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hello,
> 
> On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, it can support multiple PWM channels, all the channels share
> > same counter and period setting, but each channel can configure its
> > duty and polarity independently.
> >
> > There are several TPM modules in i.MX7ULP, the number of channels in
> > TPM modules are different, it can be read from each TPM module's PARAM
> > register.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > changes since V7:
> > 	- improve prescale computation;
> > 	- improve some register definitions;
> > 	- remove unnecessary check for period count check;
> > 	- improve function parameter to use pointer instead of value;
> > ---
> >  drivers/pwm/Kconfig       |  11 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-imx-tpm.c | 435
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 447 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > 54f8238..3ea0391 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -210,6 +210,17 @@ config PWM_IMX27
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-imx27.
> >
> > +config PWM_IMX_TPM
> > +	tristate "i.MX TPM PWM support"
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	depends on HAVE_CLK && HAS_IOMEM
> > +	help
> > +	  Generic PWM framework driver for i.MX7ULP TPM module, TPM's
> full
> > +	  name is Low Power Timer/Pulse Width Modulation Module.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-imx-tpm.
> > +
> >  config PWM_JZ4740
> >  	tristate "Ingenic JZ47xx PWM support"
> >  	depends on MACH_INGENIC
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 448825e..c368599 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT)		+= pwm-
> hibvt.o
> >  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> >  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> >  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
> > +obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> >  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> >  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new
> > file mode 100644 index 0000000..0efea36
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,435 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + *
> > + * Limitations:
> > + * - The TPM counter and period counter are shared between
> > + *   multiple channels, so all channels should use same period
> > + *   settings.
> 
> What about:
> 
>  - Not all parameters to change the period length can be changed
>    atomically. The counter must be stopped to change SC.PS.
> 
>  - Changes to polarity cannot be latched at the time of the next period
>    start.

OK.

> 
> ?
> 
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#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>
> > +
> > +#define PWM_IMX_TPM_PARAM	0x4
> > +#define PWM_IMX_TPM_GLOBAL	0x8
> > +#define PWM_IMX_TPM_SC		0x10
> > +#define PWM_IMX_TPM_CNT		0x14
> > +#define PWM_IMX_TPM_MOD		0x18
> > +#define PWM_IMX_TPM_CnSC(n)	(0x20 + (n) * 0x8)
> > +#define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
> > +
> > +#define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7,
> 0)
> > +
> > +#define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
> > +#define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
> > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK
> 	FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1)
> > +#define PWM_IMX_TPM_SC_CPWMS			BIT(5)
> > +
> > +#define PWM_IMX_TPM_CnSC_CHF	BIT(7)
> > +#define PWM_IMX_TPM_CnSC_MSB	BIT(5)
> > +#define PWM_IMX_TPM_CnSC_MSA	BIT(4)
> > +
> > +/*
> > + * The reference manual describes this field as two separate bits.
> > +The
> > + * samantic of the two bits isn't orthogonal though, so they are
> > +treated
> > + * together as a 2-bit field here.
> > + */
> > +#define PWM_IMX_TPM_CnSC_ELS	GENMASK(3, 2)
> > +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED	0x1
> > +#define PWM_IMX_TPM_CnSC_ELS_INVERSED
> 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1)
> > +#define PWM_IMX_TPM_CnSC_ELS_NORMAL
> 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2)
> > +
> > +
> > +#define PWM_IMX_TPM_MOD_WIDTH	16
> > +#define PWM_IMX_TPM_MOD_MOD
> 	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)
> > +
> > +struct imx_tpm_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	struct mutex lock;
> > +	u32 user_count;
> > +	u32 enable_count;
> > +	u32 real_period;
> > +};
> > +
> > +struct imx_tpm_pwm_param {
> > +	u8 prescale;
> > +	u32 mod;
> > +};
> > +
> > +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct
> > +pwm_chip *chip) {
> > +	return container_of(chip, struct imx_tpm_pwm_chip, chip); }
> > +
> > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
> > +				   struct imx_tpm_pwm_param *p,
> > +				   struct pwm_state *state,
> > +				   struct pwm_state *real_state)
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	u32 rate, prescale, period_count, clock_unit;
> > +	u64 tmp;
> > +
> > +	rate = clk_get_rate(tpm->clk);
> > +	tmp = (u64)state->period * rate;
> > +	clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > +	if (clock_unit <= PWM_IMX_TPM_MOD_MOD)
> > +		prescale = 0;
> > +	else
> > +		prescale = ilog2(clock_unit) + 1 -
> PWM_IMX_TPM_MOD_WIDTH;
> > +
> > +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > +		return -ERANGE;
> > +	p->prescale = prescale;
> > +
> > +	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
> > +	p->mod = period_count;
> > +
> > +	/* calculate real period HW can support */
> > +	tmp = (u64)period_count << prescale;
> > +	tmp *= NSEC_PER_SEC;
> > +	real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > +	/*
> > +	 * if eventually the PWM output is inactive, either
> > +	 * duty cycle is 0 or status is disabled, need to
> > +	 * make sure the output pin is inactive.
> > +	 */
> > +	if (!state->enabled)
> > +		real_state->duty_cycle = 0;
> > +	else
> > +		real_state->duty_cycle = state->duty_cycle;
> 
> You're maybe lying about the duty cycle here. Also it would be more
> consistent to calculate the value to be written into the CnV register that
> defines the duty cycle here.
> 
> Regarding the period computation I'm happy with this function. Unless I miss
> something this function is idempotent (i.e. doing
> 
> 	pwm_imx_tpm_round_state(chip, &p, some_state, &real_state1);
> 	pwm_imx_tpm_round_state(chip, &p, &real_state1, &real_state2);
> 
> results in real_state1 == real_state2) given that
> clk_get_rate(tpm->clk) < NSEC_PER_SEC.
> 

Sorry, I can NOT get your point, can you be more detail?


> > +	real_state->polarity = state->polarity;
> > +	real_state->enabled = state->enabled;
> > +
> > +	return 0;
> > +}
> > +
> 
> A comment here noting that pwm_imx_tpm_setup_period is supposed to be
> called with the mutex hold would be good here.

Fine.

> 
> > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > +				     struct imx_tpm_pwm_param *p) {
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	u32 val, saved_cmod, cur_prescale;
> > +
> > +	/* make sure counter is disabled for programming prescale */
> 
> @Thierry: What is your thought here? IMHO this should only be allowed if all
> affected PWMs are off.

As we already make sure that ONLY when there is ONLY 1 user and the requested
period is different from the current period, then this function will be called, so there
is impossible that multiple PWMs are active and the period is requested to be changed.
Am I right? 

> 
> > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > +	cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > +	if (saved_cmod && cur_prescale != p->prescale) {
> > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +	}
> > +
> > +	/* set TPM counter prescale */
> > +	val &= ~PWM_IMX_TPM_SC_PS;
> > +	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
> > +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +
> > +	/* restore the clock mode if necessary */
> > +	if (saved_cmod && cur_prescale != p->prescale) {
> > +		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +	}
> > +
> > +	/*
> > +	 * set period count:
> > +	 * according to RM, the MOD register is updated immediately
> > +	 * if CMOD[1:0] = 2b'00. if CMOD[1:0] != 2b'00, then MOD
> > +	 * register is updated according to the CPWMS bit, that is:
> > +	 *
> > +	 * if the selected mode is not CPWM then MOD register is
> > +	 * updated after MOD register was written and the TPM
> > +	 * counter changes from MOD to zero.
> > +	 *
> > +	 * if the selected mode is CPWM then MOD register is updated
> > +	 * after MOD register was written and the TPM counter changes
> > +	 * from MOD to (MOD – 1).
> > +	 */
> > +	writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); }
> > +
> > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > +				  struct pwm_device *pwm,
> > +				  struct pwm_state *state)
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	u32 rate, val, prescale;
> > +	u64 tmp;
> > +
> > +	/* get period */
> > +	state->period = tpm->real_period;
> > +
> > +	/* get duty cycle */
> > +	rate = clk_get_rate(tpm->clk);
> > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > +	prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > +	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > +	tmp = (tmp << prescale) * NSEC_PER_SEC;
> > +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > +	/* get polarity */
> > +	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > +	if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
> > +	    PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
> > +		state->polarity = PWM_POLARITY_INVERSED;
> > +	else
> > +		/*
> > +		 * Assume reserved values (2b00 and 2b11) to yield
> > +		 * normal polarity.
> 
> Given that this driver writes PWM_IMX_TPM_CnSC_ELS = 2b00 in some
> situations assuming that this results in an constant inactive output, this
> should be recognized here. (Not entirely sure the output is inactive because
> of only PWM_IMX_TPM_CnSC_ELS = 2b00.)

Fine, then I can save the original requested polarity in each channel, and then return
the last requested polarity here, NOT read from hardware, since the polarity could be
different according to enable status.


> 
> > +		 */
> > +		state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	/* get channel status */
> > +	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > +false; }
> > +
> > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > +				 struct pwm_device *pwm,
> > +				 struct pwm_state *state)
> 
> pwm_imx_tpm_apply_hw is called with the mutex hold. Is this necessary?
> Please either call it without the mutex or annotate the function that the caller
> is supposed to hold it.

OK, will make sure call it without mutex hold.

> 
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	struct pwm_state c;
> > +	u32 val, sc_val;
> > +	u64 tmp;
> > +
> > +	pwm_imx_tpm_get_state(chip, pwm, &c);
> > +
> > +	if (state->duty_cycle != c.duty_cycle) {
> > +		/* set duty counter */
> > +		tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> PWM_IMX_TPM_MOD_MOD;
> > +		tmp *= state->duty_cycle;
> > +		val = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > +		writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm-
> >hwpwm));
> 
> How does this affect a currently running PWM? Consider it runs at
> duty_cycle=500 + period=1000 and now should change to duty_cycle=700 +
> period=800. Can it happen that we see a or even several periods with
> duty_cycle=700 and period=1000?


The reference manual says, if counter is disabled, it will be updated immediately.
If counter is enabled, and for edge aligned PWM mode(EPWM), the register is updated
After written and TPM counter changes from MOD to zero, same as period count update,
HW will make sure the period finish..

If (CMOD[1:0] = 0:0) then CnV register is updated when CnV register is written.
If (CMOD[1:0] ≠ 0:0), then CnV register is updated according to the selected mode, that
is:
• If the selected mode is output compare then CnV register is updated on the next TPM
counter increment (end of the prescaler counting) after CnV register was written.
Chapter 64 Low Power Timer/Pulse Width Modulation Module (TPM)
i.MX 7ULP Applications Processor Reference Manual 2.0, Rev. A, 12/2017
NXP Semiconductors Preliminary 2817
Confidential Proprietary
• If the selected mode is EPWM then CnV register is updated after CnV register was
written and the TPM counter changes from MOD to zero.
• If the selected mode is CPWM then CnV register is updated after CnV register was
written and the TPM counter changes from MOD to (MOD – 1).
64.5.14



> 
> > +	}
> > +
> > +	if (state->enabled != c.enabled) {
> 
> If the PWM was already on and is changed to another enabled state, you're
> ignoring the (maybe) new polarity here.
> 
> > +		/*
> > +		 * set polarity (for edge-aligned PWM modes)
> > +		 *
> > +		 * ELS[1:0] = 2b10 yields normal polarity behaviour,
> > +		 * ELS[1:0] = 2b01 yields inversed polarity.
> > +		 * The other values are reserved.
> > +		 *
> > +		 * polarity settings will enabled/disable output status
> > +		 * immediately, so if the channel is disabled, need to
> > +		 * make sure MSA/MSB/ELS are set to 0 which means channel
> > +		 * disabled.
> > +		 */
> > +		val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +		val &= ~(PWM_IMX_TPM_CnSC_ELS |
> PWM_IMX_TPM_CnSC_MSA |
> > +			 PWM_IMX_TPM_CnSC_MSB);
> > +		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
> > +		if (state->enabled) {
> > +			val |= PWM_IMX_TPM_CnSC_MSB;
> > +			val |= (state->polarity == PWM_POLARITY_NORMAL) ?
> > +				PWM_IMX_TPM_CnSC_ELS_NORMAL :
> > +				PWM_IMX_TPM_CnSC_ELS_INVERSED;
> > +			if (++tpm->enable_count == 1) {
> > +				/* start TPM counter */
> > +				sc_val |=
> PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > +				writel(sc_val, tpm->base +
> PWM_IMX_TPM_SC);
> > +			}
> > +		} else {
> > +			if (--tpm->enable_count == 0) {
> > +				/* stop TPM counter */
> > +				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
> > +				writel(sc_val, tpm->base +
> PWM_IMX_TPM_SC);
> > +			}
> > +		}
> > +		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +	}
> > +}
> > +
> > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > +			      struct pwm_device *pwm,
> > +			     struct pwm_state *state)
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	struct imx_tpm_pwm_param param;
> > +	struct pwm_state real_state;
> > +	int ret;
> > +
> > +	ret = pwm_imx_tpm_round_state(chip, &param, state, &real_state);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&tpm->lock);
> > +
> > +	/*
> > +	 * TPM counter is shared by multiple channels, so
> > +	 * prescale and period can NOT be modified when
> > +	 * there are multiple channels in use with different
> > +	 * period settings.
> > +	 */
> > +	if (real_state.period != tpm->real_period) {
> > +		if (tpm->user_count > 1) {
> > +			ret = -EBUSY;
> > +			goto exit;
> > +		}
> > +
> > +		pwm_imx_tpm_setup_period(chip, &param);
> > +		tpm->real_period = real_state.period;
> > +	}
> > +
> > +	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > +
> > +exit:
> > +	mutex_unlock(&tpm->lock);
> 
> .apply is supposed to sleep until the newly configured state is active.
> This is missing here, right?

NOT quite understand, you meant .apply could be sleep if mutex is hold by
other thread?

BTW, can you have as more of your comments together as possible? That can
save some time of doing new patch, many thanks for your time and patience.

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&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cf0
> d30304712d433c125108d6adde6cb0%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636887568203336549&amp;sdata=9sTAoftcgUyHt9gYHjR3FE
> d9%2BAC2kDhlFYCF1FU%2Bcws%3D&amp;reserved=0  |
Uwe Kleine-König March 21, 2019, 10:41 a.m. UTC | #3
Hello Anson,

On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote:
> > -----Original Message-----
> > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> > Sent: 2019年3月21日 17:20
> > 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 V8 2/5] pwm: Add i.MX TPM PWM driver support
> > 
> > Hello,
> > 
> > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > > inside, it can support multiple PWM channels, all the channels share
> > > same counter and period setting, but each channel can configure its
> > > duty and polarity independently.
> > >
> > > There are several TPM modules in i.MX7ULP, the number of channels in
> > > TPM modules are different, it can be read from each TPM module's PARAM
> > > register.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > > changes since V7:
> > > 	- improve prescale computation;
> > > 	- improve some register definitions;
> > > 	- remove unnecessary check for period count check;
> > > 	- improve function parameter to use pointer instead of value;
> > > ---
> > >  drivers/pwm/Kconfig       |  11 ++
> > >  drivers/pwm/Makefile      |   1 +
> > >  drivers/pwm/pwm-imx-tpm.c | 435
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 447 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-imx-tpm.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > > 54f8238..3ea0391 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -210,6 +210,17 @@ config PWM_IMX27
> > >  	  To compile this driver as a module, choose M here: the module
> > >  	  will be called pwm-imx27.
> > >
> > > +config PWM_IMX_TPM
> > > +	tristate "i.MX TPM PWM support"
> > > +	depends on ARCH_MXC || COMPILE_TEST
> > > +	depends on HAVE_CLK && HAS_IOMEM
> > > +	help
> > > +	  Generic PWM framework driver for i.MX7ULP TPM module, TPM's full
> > > +	  name is Low Power Timer/Pulse Width Modulation Module.
> > > +
> > > +	  To compile this driver as a module, choose M here: the module
> > > +	  will be called pwm-imx-tpm.
> > > +
> > >  config PWM_JZ4740
> > >  	tristate "Ingenic JZ47xx PWM support"
> > >  	depends on MACH_INGENIC
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > > 448825e..c368599 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT)		+= pwm-
> > hibvt.o
> > >  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> > >  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> > >  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
> > > +obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> > >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> > >  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> > >  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> > new
> > > file mode 100644 index 0000000..0efea36
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-imx-tpm.c
> > > @@ -0,0 +1,435 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2018-2019 NXP.
> > > + *
> > > + * Limitations:
> > > + * - The TPM counter and period counter are shared between
> > > + *   multiple channels, so all channels should use same period
> > > + *   settings.
> > 
> > What about:
> > 
> >  - Not all parameters to change the period length can be changed
> >    atomically. The counter must be stopped to change SC.PS.
> > 
> >  - Changes to polarity cannot be latched at the time of the next period
> >    start.
> 
> OK.
> 
> > 
> > ?
> > 
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#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>
> > > +
> > > +#define PWM_IMX_TPM_PARAM	0x4
> > > +#define PWM_IMX_TPM_GLOBAL	0x8
> > > +#define PWM_IMX_TPM_SC		0x10
> > > +#define PWM_IMX_TPM_CNT		0x14
> > > +#define PWM_IMX_TPM_MOD		0x18
> > > +#define PWM_IMX_TPM_CnSC(n)	(0x20 + (n) * 0x8)
> > > +#define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
> > > +
> > > +#define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7,
> > 0)
> > > +
> > > +#define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
> > > +#define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
> > > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK
> > 	FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1)
> > > +#define PWM_IMX_TPM_SC_CPWMS			BIT(5)
> > > +
> > > +#define PWM_IMX_TPM_CnSC_CHF	BIT(7)
> > > +#define PWM_IMX_TPM_CnSC_MSB	BIT(5)
> > > +#define PWM_IMX_TPM_CnSC_MSA	BIT(4)
> > > +
> > > +/*
> > > + * The reference manual describes this field as two separate bits.
> > > +The
> > > + * samantic of the two bits isn't orthogonal though, so they are
> > > +treated
> > > + * together as a 2-bit field here.
> > > + */
> > > +#define PWM_IMX_TPM_CnSC_ELS	GENMASK(3, 2)
> > > +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED	0x1
> > > +#define PWM_IMX_TPM_CnSC_ELS_INVERSED
> > 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1)
> > > +#define PWM_IMX_TPM_CnSC_ELS_NORMAL
> > 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2)
> > > +
> > > +
> > > +#define PWM_IMX_TPM_MOD_WIDTH	16
> > > +#define PWM_IMX_TPM_MOD_MOD
> > 	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)
> > > +
> > > +struct imx_tpm_pwm_chip {
> > > +	struct pwm_chip chip;
> > > +	struct clk *clk;
> > > +	void __iomem *base;
> > > +	struct mutex lock;
> > > +	u32 user_count;
> > > +	u32 enable_count;
> > > +	u32 real_period;
> > > +};
> > > +
> > > +struct imx_tpm_pwm_param {
> > > +	u8 prescale;
> > > +	u32 mod;
> > > +};
> > > +
> > > +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct
> > > +pwm_chip *chip) {
> > > +	return container_of(chip, struct imx_tpm_pwm_chip, chip); }
> > > +
> > > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
> > > +				   struct imx_tpm_pwm_param *p,
> > > +				   struct pwm_state *state,
> > > +				   struct pwm_state *real_state)
> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	u32 rate, prescale, period_count, clock_unit;
> > > +	u64 tmp;
> > > +
> > > +	rate = clk_get_rate(tpm->clk);
> > > +	tmp = (u64)state->period * rate;
> > > +	clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > > +	if (clock_unit <= PWM_IMX_TPM_MOD_MOD)
> > > +		prescale = 0;
> > > +	else
> > > +		prescale = ilog2(clock_unit) + 1 -
> > PWM_IMX_TPM_MOD_WIDTH;
> > > +
> > > +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > > +		return -ERANGE;
> > > +	p->prescale = prescale;
> > > +
> > > +	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
> > > +	p->mod = period_count;
> > > +
> > > +	/* calculate real period HW can support */
> > > +	tmp = (u64)period_count << prescale;
> > > +	tmp *= NSEC_PER_SEC;
> > > +	real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > +	/*
> > > +	 * if eventually the PWM output is inactive, either
> > > +	 * duty cycle is 0 or status is disabled, need to
> > > +	 * make sure the output pin is inactive.
> > > +	 */
> > > +	if (!state->enabled)
> > > +		real_state->duty_cycle = 0;
> > > +	else
> > > +		real_state->duty_cycle = state->duty_cycle;
> > 
> > You're maybe lying about the duty cycle here. Also it would be more
> > consistent to calculate the value to be written into the CnV register that
> > defines the duty cycle here.
> > 
> > Regarding the period computation I'm happy with this function. Unless I miss
> > something this function is idempotent (i.e. doing
> > 
> > 	pwm_imx_tpm_round_state(chip, &p, some_state, &real_state1);
> > 	pwm_imx_tpm_round_state(chip, &p, &real_state1, &real_state2);
> > 
> > results in real_state1 == real_state2) given that
> > clk_get_rate(tpm->clk) < NSEC_PER_SEC.
> 
> Sorry, I can NOT get your point, can you be more detail?

I assume you ask about the idempotent stuff and my critics about the
duty cycle above is clear.

IMHO a PWM driver should have the property that if I do:

	pwm_get_state(my_pwm, &state);
	pwm_apply_state(my_pwm, &state);

it should not result in any changes. This could for example happen if I
request the period of a PWM to be 3333334 ns, then the output might be
configured (because of hardware constraints) to 3333333.333333333 ns.
Then get_state returns 3333333 ns (which is fine) and then configuring
for these 3333333 ns results in a period of length 331666 ns.

This is not the case here as pwm_imx_tpm_round_state is idempotent which
is actually a good property and my comment was pointing out something
good, not requesting a change.

> > > +	real_state->polarity = state->polarity;
> > > +	real_state->enabled = state->enabled;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > A comment here noting that pwm_imx_tpm_setup_period is supposed to be
> > called with the mutex hold would be good here.
> 
> Fine.
> 
> > 
> > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > > +				     struct imx_tpm_pwm_param *p) {
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	u32 val, saved_cmod, cur_prescale;
> > > +
> > > +	/* make sure counter is disabled for programming prescale */
> > 
> > @Thierry: What is your thought here? IMHO this should only be allowed if all
> > affected PWMs are off.
> 
> As we already make sure that ONLY when there is ONLY 1 user and the requested
> period is different from the current period, then this function will be called, so there
> is impossible that multiple PWMs are active and the period is requested to be changed.
> Am I right? 

This problem is not about two PWMs. If you reconfigure a running PWM the
requirement is that the hardware completes a whole period with the old
configuration and then immediately starts a new period with the new
parameters. If you stop the counter, the last period with the old
parameters is disturbed.

> 
> > 
> > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > +	cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > > +	if (saved_cmod && cur_prescale != p->prescale) {
> > > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > +	}
> > > +
> > > +	/* set TPM counter prescale */
> > > +	val &= ~PWM_IMX_TPM_SC_PS;
> > > +	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
> > > +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > +
> > > +	/* restore the clock mode if necessary */
> > > +	if (saved_cmod && cur_prescale != p->prescale) {
> > > +		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > +	}
> > > +
> > > +	/*
> > > +	 * set period count:
> > > +	 * according to RM, the MOD register is updated immediately
> > > +	 * if CMOD[1:0] = 2b'00. if CMOD[1:0] != 2b'00, then MOD
> > > +	 * register is updated according to the CPWMS bit, that is:
> > > +	 *
> > > +	 * if the selected mode is not CPWM then MOD register is
> > > +	 * updated after MOD register was written and the TPM
> > > +	 * counter changes from MOD to zero.
> > > +	 *
> > > +	 * if the selected mode is CPWM then MOD register is updated
> > > +	 * after MOD register was written and the TPM counter changes
> > > +	 * from MOD to (MOD – 1).
> > > +	 */
> > > +	writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); }
> > > +
> > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > +				  struct pwm_device *pwm,
> > > +				  struct pwm_state *state)
> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	u32 rate, val, prescale;
> > > +	u64 tmp;
> > > +
> > > +	/* get period */
> > > +	state->period = tpm->real_period;
> > > +
> > > +	/* get duty cycle */
> > > +	rate = clk_get_rate(tpm->clk);
> > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > +	prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > > +	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +	tmp = (tmp << prescale) * NSEC_PER_SEC;
> > > +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > +
> > > +	/* get polarity */
> > > +	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > +	if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
> > > +	    PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
> > > +		state->polarity = PWM_POLARITY_INVERSED;
> > > +	else
> > > +		/*
> > > +		 * Assume reserved values (2b00 and 2b11) to yield
> > > +		 * normal polarity.
> > 
> > Given that this driver writes PWM_IMX_TPM_CnSC_ELS = 2b00 in some
> > situations assuming that this results in an constant inactive output, this
> > should be recognized here. (Not entirely sure the output is inactive because
> > of only PWM_IMX_TPM_CnSC_ELS = 2b00.)
> 
> Fine, then I can save the original requested polarity in each channel, and then return
> the last requested polarity here, NOT read from hardware, since the polarity could be
> different according to enable status.

This works iff there was a polarity requested before.
 
> > > +		 */
> > > +		state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > +	/* get channel status */
> > > +	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > > +false; }
> > > +
> > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > +				 struct pwm_device *pwm,
> > > +				 struct pwm_state *state)
> > 
> > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this necessary?
> > Please either call it without the mutex or annotate the function that the caller
> > is supposed to hold it.
> 
> OK, will make sure call it without mutex hold.
> 
> > 
> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	struct pwm_state c;
> > > +	u32 val, sc_val;
> > > +	u64 tmp;
> > > +
> > > +	pwm_imx_tpm_get_state(chip, pwm, &c);
> > > +
> > > +	if (state->duty_cycle != c.duty_cycle) {
> > > +		/* set duty counter */
> > > +		tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > PWM_IMX_TPM_MOD_MOD;
> > > +		tmp *= state->duty_cycle;
> > > +		val = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > > +		writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm-
> > >hwpwm));
> > 
> > How does this affect a currently running PWM? Consider it runs at
> > duty_cycle=500 + period=1000 and now should change to duty_cycle=700 +
> > period=800. Can it happen that we see a or even several periods with
> > duty_cycle=700 and period=1000?
> 
> 
> The reference manual says, if counter is disabled, it will be updated immediately.

The counter is not disabled, right?

> If counter is enabled, and for edge aligned PWM mode(EPWM), the register is updated
> After written and TPM counter changes from MOD to zero, same as period count update,
> HW will make sure the period finish..

Looking into my concern again, it is actually the other way around:
Assuming a single used PWM channel that runs at duty_cycle=500 +
period=1000. Then pwm_imx_tpm_apply() is called with state->duty_cycle=700
and state->period=800. pwm_imx_tpm_apply() calls
pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the
PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV to the
value corresponding to duty_cycle=700, it produces a waveform with
duty_cycle=500 and period=800 which is bad. This is another limitation
that can be worked around in software with some effort (which might or
might not be worth to spend).

> If (CMOD[1:0] = 0:0) then CnV register is updated when CnV register is written.
> If (CMOD[1:0] ≠ 0:0), then CnV register is updated according to the selected mode, that
> is:
> • If the selected mode is output compare then CnV register is updated on the next TPM
> counter increment (end of the prescaler counting) after CnV register was written.
> • If the selected mode is EPWM then CnV register is updated after CnV register was
> written and the TPM counter changes from MOD to zero.
> • If the selected mode is CPWM then CnV register is updated after CnV register was
> written and the TPM counter changes from MOD to (MOD – 1).
> 64.5.14

The case "selected mode is EPWM" is the one relevant for us, right?

> > > +	}
> > > +
> > > +	if (state->enabled != c.enabled) {
> > 
> > If the PWM was already on and is changed to another enabled state, you're
> > ignoring the (maybe) new polarity here.
> > 
> > > +		/*
> > > +		 * set polarity (for edge-aligned PWM modes)
> > > +		 *
> > > +		 * ELS[1:0] = 2b10 yields normal polarity behaviour,
> > > +		 * ELS[1:0] = 2b01 yields inversed polarity.
> > > +		 * The other values are reserved.
> > > +		 *
> > > +		 * polarity settings will enabled/disable output status
> > > +		 * immediately, so if the channel is disabled, need to
> > > +		 * make sure MSA/MSB/ELS are set to 0 which means channel
> > > +		 * disabled.
> > > +		 */
> > > +		val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > +		val &= ~(PWM_IMX_TPM_CnSC_ELS |
> > PWM_IMX_TPM_CnSC_MSA |
> > > +			 PWM_IMX_TPM_CnSC_MSB);
> > > +		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > +		if (state->enabled) {
> > > +			val |= PWM_IMX_TPM_CnSC_MSB;
> > > +			val |= (state->polarity == PWM_POLARITY_NORMAL) ?
> > > +				PWM_IMX_TPM_CnSC_ELS_NORMAL :
> > > +				PWM_IMX_TPM_CnSC_ELS_INVERSED;
> > > +			if (++tpm->enable_count == 1) {
> > > +				/* start TPM counter */
> > > +				sc_val |=
> > PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > +				writel(sc_val, tpm->base +
> > PWM_IMX_TPM_SC);
> > > +			}
> > > +		} else {
> > > +			if (--tpm->enable_count == 0) {
> > > +				/* stop TPM counter */
> > > +				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
> > > +				writel(sc_val, tpm->base +
> > PWM_IMX_TPM_SC);
> > > +			}
> > > +		}
> > > +		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > >hwpwm));
> > > +	}
> > > +}
> > > +
> > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > > +			      struct pwm_device *pwm,
> > > +			     struct pwm_state *state)
> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	struct imx_tpm_pwm_param param;
> > > +	struct pwm_state real_state;
> > > +	int ret;
> > > +
> > > +	ret = pwm_imx_tpm_round_state(chip, &param, state, &real_state);
> > > +	if (ret)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&tpm->lock);
> > > +
> > > +	/*
> > > +	 * TPM counter is shared by multiple channels, so
> > > +	 * prescale and period can NOT be modified when
> > > +	 * there are multiple channels in use with different
> > > +	 * period settings.
> > > +	 */
> > > +	if (real_state.period != tpm->real_period) {
> > > +		if (tpm->user_count > 1) {
> > > +			ret = -EBUSY;
> > > +			goto exit;
> > > +		}
> > > +
> > > +		pwm_imx_tpm_setup_period(chip, &param);
> > > +		tpm->real_period = real_state.period;
> > > +	}
> > > +
> > > +	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > > +
> > > +exit:
> > > +	mutex_unlock(&tpm->lock);
> > 
> > .apply is supposed to sleep until the newly configured state is active.
> > This is missing here, right?
> 
> NOT quite understand, you meant .apply could be sleep if mutex is hold by
> other thread?

No. .apply is supposed to only return when the new configuration is
active. So if the PWM is running in its previous configuration, you
setup the registers such that the new configuration gets active in the
next period, you must not yet return to the caller until the new period
started.

> BTW, can you have as more of your comments together as possible? That can
> save some time of doing new patch, many thanks for your time and patience.

I understand that coming up with new issues in V8 that were already
present in V5 is annoying. On the other hand when having already pointed
out quite some issues in V5 (and V6 and V7) it's not always easy to
still see the more complicated issues. In this review round the driver
got in my eyes considerably cleaner with the introduction of the
round_state function and earlier the drop of _config and _enable. That
only after this I was able to identify more issues is bad for your work
flow, but a prime show case why a well structured driver is good for
maintainability. Also note that my feedback is (for you) free of charge.
My day's calendar also contains other items. So I can better spend
several small time slices reviewing your work than a single big one. Of
course I could delay sending out my findings till the next small slot,
but I think this is not beneficial in the sum for several reasons. If I
give small (but maybe incomplete) feedback:

 - you are not (completely) delayed;
 - it is less likely that someone else looks at your patch and so
   duplicates my work;
 - it is less likely that the maintainer comes around in a hurry,
   doesn't notice remaining issues and applies the non-optimal patch;
 - you can provide a new iteration that is already better than the last
   and so give the chance to find issues that where somewhat hidden
   behind things the driver suffered before.

In my eyes this outweighs the drawbacks of this workflow.

Best regards
Uwe
Anson Huang March 21, 2019, 12:47 p.m. UTC | #4
Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月21日 18:41
> 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 V8 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hello Anson,
> 
> On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> > > Sent: 2019年3月21日 17:20
> > > 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 V8 2/5] pwm: Add i.MX TPM PWM driver support
> > >
> > > Hello,
> > >
> > > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > > > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > > > inside, it can support multiple PWM channels, all the channels
> > > > share same counter and period setting, but each channel can
> > > > configure its duty and polarity independently.
> > > >
> > > > There are several TPM modules in i.MX7ULP, the number of channels
> > > > in TPM modules are different, it can be read from each TPM
> > > > module's PARAM register.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > ---
> > > > changes since V7:
> > > > 	- improve prescale computation;
> > > > 	- improve some register definitions;
> > > > 	- remove unnecessary check for period count check;
> > > > 	- improve function parameter to use pointer instead of value;
> > > > ---
> > > >  drivers/pwm/Kconfig       |  11 ++
> > > >  drivers/pwm/Makefile      |   1 +
> > > >  drivers/pwm/pwm-imx-tpm.c | 435
> > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 447 insertions(+)  create mode 100644
> > > > drivers/pwm/pwm-imx-tpm.c
> > > >
> > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > > > 54f8238..3ea0391 100644
> > > > --- a/drivers/pwm/Kconfig
> > > > +++ b/drivers/pwm/Kconfig
> > > > @@ -210,6 +210,17 @@ config PWM_IMX27
> > > >  	  To compile this driver as a module, choose M here: the module
> > > >  	  will be called pwm-imx27.
> > > >
> > > > +config PWM_IMX_TPM
> > > > +	tristate "i.MX TPM PWM support"
> > > > +	depends on ARCH_MXC || COMPILE_TEST
> > > > +	depends on HAVE_CLK && HAS_IOMEM
> > > > +	help
> > > > +	  Generic PWM framework driver for i.MX7ULP TPM module, TPM's
> full
> > > > +	  name is Low Power Timer/Pulse Width Modulation Module.
> > > > +
> > > > +	  To compile this driver as a module, choose M here: the module
> > > > +	  will be called pwm-imx-tpm.
> > > > +
> > > >  config PWM_JZ4740
> > > >  	tristate "Ingenic JZ47xx PWM support"
> > > >  	depends on MACH_INGENIC
> > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > > > 448825e..c368599 100644
> > > > --- a/drivers/pwm/Makefile
> > > > +++ b/drivers/pwm/Makefile
> > > > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT)		+=
> pwm-
> > > hibvt.o
> > > >  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> > > >  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> > > >  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
> > > > +obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> > > >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> > > >  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> > > >  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> > > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-
> tpm.c
> > > new
> > > > file mode 100644 index 0000000..0efea36
> > > > --- /dev/null
> > > > +++ b/drivers/pwm/pwm-imx-tpm.c
> > > > @@ -0,0 +1,435 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright 2018-2019 NXP.
> > > > + *
> > > > + * Limitations:
> > > > + * - The TPM counter and period counter are shared between
> > > > + *   multiple channels, so all channels should use same period
> > > > + *   settings.
> > >
> > > What about:
> > >
> > >  - Not all parameters to change the period length can be changed
> > >    atomically. The counter must be stopped to change SC.PS.
> > >
> > >  - Changes to polarity cannot be latched at the time of the next period
> > >    start.
> >
> > OK.
> >
> > >
> > > ?
> > >
> > > > + */
> > > > +
> > > > +#include <linux/bitfield.h>
> > > > +#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>
> > > > +
> > > > +#define PWM_IMX_TPM_PARAM	0x4
> > > > +#define PWM_IMX_TPM_GLOBAL	0x8
> > > > +#define PWM_IMX_TPM_SC		0x10
> > > > +#define PWM_IMX_TPM_CNT		0x14
> > > > +#define PWM_IMX_TPM_MOD		0x18
> > > > +#define PWM_IMX_TPM_CnSC(n)	(0x20 + (n) * 0x8)
> > > > +#define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
> > > > +
> > > > +#define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7,
> > > 0)
> > > > +
> > > > +#define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
> > > > +#define PWM_IMX_TPM_SC_CMOD			GENMASK(4,
> 3)
> > > > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK
> > > 	FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1)
> > > > +#define PWM_IMX_TPM_SC_CPWMS			BIT(5)
> > > > +
> > > > +#define PWM_IMX_TPM_CnSC_CHF	BIT(7)
> > > > +#define PWM_IMX_TPM_CnSC_MSB	BIT(5)
> > > > +#define PWM_IMX_TPM_CnSC_MSA	BIT(4)
> > > > +
> > > > +/*
> > > > + * The reference manual describes this field as two separate bits.
> > > > +The
> > > > + * samantic of the two bits isn't orthogonal though, so they are
> > > > +treated
> > > > + * together as a 2-bit field here.
> > > > + */
> > > > +#define PWM_IMX_TPM_CnSC_ELS	GENMASK(3, 2)
> > > > +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED	0x1
> > > > +#define PWM_IMX_TPM_CnSC_ELS_INVERSED
> > > 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1)
> > > > +#define PWM_IMX_TPM_CnSC_ELS_NORMAL
> > > 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2)
> > > > +
> > > > +
> > > > +#define PWM_IMX_TPM_MOD_WIDTH	16
> > > > +#define PWM_IMX_TPM_MOD_MOD
> > > 	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)
> > > > +
> > > > +struct imx_tpm_pwm_chip {
> > > > +	struct pwm_chip chip;
> > > > +	struct clk *clk;
> > > > +	void __iomem *base;
> > > > +	struct mutex lock;
> > > > +	u32 user_count;
> > > > +	u32 enable_count;
> > > > +	u32 real_period;
> > > > +};
> > > > +
> > > > +struct imx_tpm_pwm_param {
> > > > +	u8 prescale;
> > > > +	u32 mod;
> > > > +};
> > > > +
> > > > +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct
> > > > +pwm_chip *chip) {
> > > > +	return container_of(chip, struct imx_tpm_pwm_chip, chip); }
> > > > +
> > > > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
> > > > +				   struct imx_tpm_pwm_param *p,
> > > > +				   struct pwm_state *state,
> > > > +				   struct pwm_state *real_state) {
> > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > +	u32 rate, prescale, period_count, clock_unit;
> > > > +	u64 tmp;
> > > > +
> > > > +	rate = clk_get_rate(tpm->clk);
> > > > +	tmp = (u64)state->period * rate;
> > > > +	clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > > > +	if (clock_unit <= PWM_IMX_TPM_MOD_MOD)
> > > > +		prescale = 0;
> > > > +	else
> > > > +		prescale = ilog2(clock_unit) + 1 -
> > > PWM_IMX_TPM_MOD_WIDTH;
> > > > +
> > > > +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > > > +		return -ERANGE;
> > > > +	p->prescale = prescale;
> > > > +
> > > > +	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
> > > > +	p->mod = period_count;
> > > > +
> > > > +	/* calculate real period HW can support */
> > > > +	tmp = (u64)period_count << prescale;
> > > > +	tmp *= NSEC_PER_SEC;
> > > > +	real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > +	/*
> > > > +	 * if eventually the PWM output is inactive, either
> > > > +	 * duty cycle is 0 or status is disabled, need to
> > > > +	 * make sure the output pin is inactive.
> > > > +	 */
> > > > +	if (!state->enabled)
> > > > +		real_state->duty_cycle = 0;
> > > > +	else
> > > > +		real_state->duty_cycle = state->duty_cycle;
> > >
> > > You're maybe lying about the duty cycle here. Also it would be more
> > > consistent to calculate the value to be written into the CnV
> > > register that defines the duty cycle here.
> > >
> > > Regarding the period computation I'm happy with this function.
> > > Unless I miss something this function is idempotent (i.e. doing
> > >
> > > 	pwm_imx_tpm_round_state(chip, &p, some_state, &real_state1);
> > > 	pwm_imx_tpm_round_state(chip, &p, &real_state1, &real_state2);
> > >
> > > results in real_state1 == real_state2) given that
> > > clk_get_rate(tpm->clk) < NSEC_PER_SEC.
> >
> > Sorry, I can NOT get your point, can you be more detail?
> 
> I assume you ask about the idempotent stuff and my critics about the duty
> cycle above is clear.
> 
> IMHO a PWM driver should have the property that if I do:
> 
> 	pwm_get_state(my_pwm, &state);
> 	pwm_apply_state(my_pwm, &state);
> 
> it should not result in any changes. This could for example happen if I
> request the period of a PWM to be 3333334 ns, then the output might be
> configured (because of hardware constraints) to 3333333.333333333 ns.
> Then get_state returns 3333333 ns (which is fine) and then configuring for
> these 3333333 ns results in a period of length 331666 ns.
> 
> This is not the case here as pwm_imx_tpm_round_state is idempotent which
> is actually a good property and my comment was pointing out something
> good, not requesting a change.

Thanks for your detail explanation.

> 
> > > > +	real_state->polarity = state->polarity;
> > > > +	real_state->enabled = state->enabled;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > >
> > > A comment here noting that pwm_imx_tpm_setup_period is supposed to
> > > be called with the mutex hold would be good here.
> >
> > Fine.
> >
> > >
> > > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > > > +				     struct imx_tpm_pwm_param *p) {
> > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > +	u32 val, saved_cmod, cur_prescale;
> > > > +
> > > > +	/* make sure counter is disabled for programming prescale */
> > >
> > > @Thierry: What is your thought here? IMHO this should only be
> > > allowed if all affected PWMs are off.
> >
> > As we already make sure that ONLY when there is ONLY 1 user and the
> > requested period is different from the current period, then this
> > function will be called, so there is impossible that multiple PWMs are active
> and the period is requested to be changed.
> > Am I right?
> 
> This problem is not about two PWMs. If you reconfigure a running PWM the
> requirement is that the hardware completes a whole period with the old
> configuration and then immediately starts a new period with the new
> parameters. If you stop the counter, the last period with the old parameters
> is disturbed.

So, I think simply return error if the counter is running and there is a new PS change
request, right?

> 
> >
> > >
> > > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > > +	cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > > > +	if (saved_cmod && cur_prescale != p->prescale) {
> > > > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > +	}
> > > > +
> > > > +	/* set TPM counter prescale */
> > > > +	val &= ~PWM_IMX_TPM_SC_PS;
> > > > +	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
> > > > +	writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > +
> > > > +	/* restore the clock mode if necessary */
> > > > +	if (saved_cmod && cur_prescale != p->prescale) {
> > > > +		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * set period count:
> > > > +	 * according to RM, the MOD register is updated immediately
> > > > +	 * if CMOD[1:0] = 2b'00. if CMOD[1:0] != 2b'00, then MOD
> > > > +	 * register is updated according to the CPWMS bit, that is:
> > > > +	 *
> > > > +	 * if the selected mode is not CPWM then MOD register is
> > > > +	 * updated after MOD register was written and the TPM
> > > > +	 * counter changes from MOD to zero.
> > > > +	 *
> > > > +	 * if the selected mode is CPWM then MOD register is updated
> > > > +	 * after MOD register was written and the TPM counter changes
> > > > +	 * from MOD to (MOD – 1).
> > > > +	 */
> > > > +	writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); }
> > > > +
> > > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
> > > > +				  struct pwm_device *pwm,
> > > > +				  struct pwm_state *state)
> > > > +{
> > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > +	u32 rate, val, prescale;
> > > > +	u64 tmp;
> > > > +
> > > > +	/* get period */
> > > > +	state->period = tpm->real_period;
> > > > +
> > > > +	/* get duty cycle */
> > > > +	rate = clk_get_rate(tpm->clk);
> > > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > +	prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > > > +	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > +	tmp = (tmp << prescale) * NSEC_PER_SEC;
> > > > +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > > > +
> > > > +	/* get polarity */
> > > > +	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > +	if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
> > > > +	    PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
> > > > +		state->polarity = PWM_POLARITY_INVERSED;
> > > > +	else
> > > > +		/*
> > > > +		 * Assume reserved values (2b00 and 2b11) to yield
> > > > +		 * normal polarity.
> > >
> > > Given that this driver writes PWM_IMX_TPM_CnSC_ELS = 2b00 in some
> > > situations assuming that this results in an constant inactive
> > > output, this should be recognized here. (Not entirely sure the
> > > output is inactive because of only PWM_IMX_TPM_CnSC_ELS = 2b00.)
> >
> > Fine, then I can save the original requested polarity in each channel,
> > and then return the last requested polarity here, NOT read from
> > hardware, since the polarity could be different according to enable status.
> 
> This works iff there was a polarity requested before.

Will do that in next version.

> 
> > > > +		 */
> > > > +		state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > +	/* get channel status */
> > > > +	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > > > +false; }
> > > > +
> > > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > > +				 struct pwm_device *pwm,
> > > > +				 struct pwm_state *state)
> > >
> > > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this necessary?
> > > Please either call it without the mutex or annotate the function
> > > that the caller is supposed to hold it.
> >
> > OK, will make sure call it without mutex hold.
> >
> > >
> > > > +{
> > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > +	struct pwm_state c;
> > > > +	u32 val, sc_val;
> > > > +	u64 tmp;
> > > > +
> > > > +	pwm_imx_tpm_get_state(chip, pwm, &c);
> > > > +
> > > > +	if (state->duty_cycle != c.duty_cycle) {
> > > > +		/* set duty counter */
> > > > +		tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> > > PWM_IMX_TPM_MOD_MOD;
> > > > +		tmp *= state->duty_cycle;
> > > > +		val = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> > > > +		writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm-
> > > >hwpwm));
> > >
> > > How does this affect a currently running PWM? Consider it runs at
> > > duty_cycle=500 + period=1000 and now should change to duty_cycle=700
> > > + period=800. Can it happen that we see a or even several periods
> > > with
> > > duty_cycle=700 and period=1000?
> >
> >
> > The reference manual says, if counter is disabled, it will be updated
> immediately.
> 
> The counter is not disabled, right?

The counter could be enabled or disabled, for disabled case, I think there is
no issue for duty change immediately.

> 
> > If counter is enabled, and for edge aligned PWM mode(EPWM), the
> > register is updated After written and TPM counter changes from MOD to
> > zero, same as period count update, HW will make sure the period finish..
> 
> Looking into my concern again, it is actually the other way around:
> Assuming a single used PWM channel that runs at duty_cycle=500 +
> period=1000. Then pwm_imx_tpm_apply() is called with state-
> >duty_cycle=700 and state->period=800. pwm_imx_tpm_apply() calls
> pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the
> PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV to
> the value corresponding to duty_cycle=700, it produces a waveform with
> duty_cycle=500 and period=800 which is bad. This is another limitation that
> can be worked around in software with some effort (which might or might
> not be worth to spend).

I am sure that on i.MX7ULP platform we used for backlight ONLY, it should NOT
that matter if this case happen, unless the counter is disabled, then the effort spend
on this case will be huge, so I plan to leave it as what it is if you don't mind.

> 
> > If (CMOD[1:0] = 0:0) then CnV register is updated when CnV register is
> written.
> > If (CMOD[1:0] ≠ 0:0), then CnV register is updated according to the
> > selected mode, that
> > is:
> > • If the selected mode is output compare then CnV register is updated
> > on the next TPM counter increment (end of the prescaler counting) after
> CnV register was written.
> > • If the selected mode is EPWM then CnV register is updated after CnV
> > register was written and the TPM counter changes from MOD to zero.
> > • If the selected mode is CPWM then CnV register is updated after CnV
> > register was written and the TPM counter changes from MOD to (MOD – 1).
> > 64.5.14
> 
> The case "selected mode is EPWM" is the one relevant for us, right?

Yes.

> 
> > > > +	}
> > > > +
> > > > +	if (state->enabled != c.enabled) {
> > >
> > > If the PWM was already on and is changed to another enabled state,
> > > you're ignoring the (maybe) new polarity here.
> > >
> > > > +		/*
> > > > +		 * set polarity (for edge-aligned PWM modes)
> > > > +		 *
> > > > +		 * ELS[1:0] = 2b10 yields normal polarity behaviour,
> > > > +		 * ELS[1:0] = 2b01 yields inversed polarity.
> > > > +		 * The other values are reserved.
> > > > +		 *
> > > > +		 * polarity settings will enabled/disable output status
> > > > +		 * immediately, so if the channel is disabled, need to
> > > > +		 * make sure MSA/MSB/ELS are set to 0 which means channel
> > > > +		 * disabled.
> > > > +		 */
> > > > +		val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > +		val &= ~(PWM_IMX_TPM_CnSC_ELS |
> > > PWM_IMX_TPM_CnSC_MSA |
> > > > +			 PWM_IMX_TPM_CnSC_MSB);
> > > > +		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > +		if (state->enabled) {
> > > > +			val |= PWM_IMX_TPM_CnSC_MSB;
> > > > +			val |= (state->polarity == PWM_POLARITY_NORMAL) ?
> > > > +				PWM_IMX_TPM_CnSC_ELS_NORMAL :
> > > > +				PWM_IMX_TPM_CnSC_ELS_INVERSED;
> > > > +			if (++tpm->enable_count == 1) {
> > > > +				/* start TPM counter */
> > > > +				sc_val |=
> > > PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > > > +				writel(sc_val, tpm->base +
> > > PWM_IMX_TPM_SC);
> > > > +			}
> > > > +		} else {
> > > > +			if (--tpm->enable_count == 0) {
> > > > +				/* stop TPM counter */
> > > > +				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > +				writel(sc_val, tpm->base +
> > > PWM_IMX_TPM_SC);
> > > > +			}
> > > > +		}
> > > > +		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> > > >hwpwm));
> > > > +	}
> > > > +}
> > > > +
> > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > > > +			      struct pwm_device *pwm,
> > > > +			     struct pwm_state *state)
> > > > +{
> > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > +	struct imx_tpm_pwm_param param;
> > > > +	struct pwm_state real_state;
> > > > +	int ret;
> > > > +
> > > > +	ret = pwm_imx_tpm_round_state(chip, &param, state, &real_state);
> > > > +	if (ret)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&tpm->lock);
> > > > +
> > > > +	/*
> > > > +	 * TPM counter is shared by multiple channels, so
> > > > +	 * prescale and period can NOT be modified when
> > > > +	 * there are multiple channels in use with different
> > > > +	 * period settings.
> > > > +	 */
> > > > +	if (real_state.period != tpm->real_period) {
> > > > +		if (tpm->user_count > 1) {
> > > > +			ret = -EBUSY;
> > > > +			goto exit;
> > > > +		}
> > > > +
> > > > +		pwm_imx_tpm_setup_period(chip, &param);
> > > > +		tpm->real_period = real_state.period;
> > > > +	}
> > > > +
> > > > +	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > > > +
> > > > +exit:
> > > > +	mutex_unlock(&tpm->lock);
> > >
> > > .apply is supposed to sleep until the newly configured state is active.
> > > This is missing here, right?
> >
> > NOT quite understand, you meant .apply could be sleep if mutex is hold
> > by other thread?
> 
> No. .apply is supposed to only return when the new configuration is active.
> So if the PWM is running in its previous configuration, you setup the registers
> such that the new configuration gets active in the next period, you must not
> yet return to the caller until the new period started.

That bring me back to previous question, we can add waiting for period finish
And then return from .apply, but we also need a timeout for the wait, what should
The timeout value be? 100mS? Or even several seconds?


> 
> > BTW, can you have as more of your comments together as possible? That
> > can save some time of doing new patch, many thanks for your time and
> patience.
> 
> I understand that coming up with new issues in V8 that were already present
> in V5 is annoying. On the other hand when having already pointed out quite
> some issues in V5 (and V6 and V7) it's not always easy to still see the more
> complicated issues. In this review round the driver got in my eyes
> considerably cleaner with the introduction of the round_state function and
> earlier the drop of _config and _enable. That only after this I was able to
> identify more issues is bad for your work flow, but a prime show case why a
> well structured driver is good for maintainability. Also note that my feedback
> is (for you) free of charge.
> My day's calendar also contains other items. So I can better spend several
> small time slices reviewing your work than a single big one. Of course I could
> delay sending out my findings till the next small slot, but I think this is not
> beneficial in the sum for several reasons. If I give small (but maybe
> incomplete) feedback:
> 
>  - you are not (completely) delayed;
>  - it is less likely that someone else looks at your patch and so
>    duplicates my work;
>  - it is less likely that the maintainer comes around in a hurry,
>    doesn't notice remaining issues and applies the non-optimal patch;
>  - you can provide a new iteration that is already better than the last
>    and so give the chance to find issues that where somewhat hidden
>    behind things the driver suffered before.
> 
> In my eyes this outweighs the drawbacks of this workflow

Thanks again, I understand all of these, just hope that too many patches/mails
does NOT bother reviewer too much, I like to send out new patches once I got
new comments and have time to do that.

Thanks for your time.
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&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cc1
> 57c45d74144150178008d6ade9c4e8%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636887616885599339&amp;sdata=MuRkM5uI%2FnJYcpkFgh
> qQdcrGJyGkdr5d3XyvtTpn%2F4I%3D&amp;reserved=0  |
Uwe Kleine-König March 21, 2019, 1:42 p.m. UTC | #5
Hello,

On Thu, Mar 21, 2019 at 12:47:32PM +0000, Anson Huang wrote:
> > On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote:
> > > > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > > > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > > > > +				     struct imx_tpm_pwm_param *p) {
> > > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > > +	u32 val, saved_cmod, cur_prescale;
> > > > > +
> > > > > +	/* make sure counter is disabled for programming prescale */
> > > >
> > > > @Thierry: What is your thought here? IMHO this should only be
> > > > allowed if all affected PWMs are off.
> > >
> > > As we already make sure that ONLY when there is ONLY 1 user and the
> > > requested period is different from the current period, then this
> > > function will be called, so there is impossible that multiple PWMs are active
> > and the period is requested to be changed.
> > > Am I right?
> > 
> > This problem is not about two PWMs. If you reconfigure a running PWM the
> > requirement is that the hardware completes a whole period with the old
> > configuration and then immediately starts a new period with the new
> > parameters. If you stop the counter, the last period with the old parameters
> > is disturbed.
> 
> So, I think simply return error if the counter is running and there is a new PS change
> request, right?

That's the general idea yes. If you cannot fulfil the request without
violating the guarantees you have to adhere, refuse the request with an
error.

> > > > > +		 */
> > > > > +		state->polarity = PWM_POLARITY_NORMAL;
> > > > > +
> > > > > +	/* get channel status */
> > > > > +	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > > > > +false; }
> > > > > +
> > > > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > > > +				 struct pwm_device *pwm,
> > > > > +				 struct pwm_state *state)
> > > >
> > > > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this necessary?
> > > > Please either call it without the mutex or annotate the function
> > > > that the caller is supposed to hold it.
> > >
> > > OK, will make sure call it without mutex hold.

Reading through the reference manual I noticed that there might be a
stall: If you write two values to CnV the second write is ignored if the
first wasn't latched yet. That might mean that you cannot release the
mutex before the newly configured state is active. This is related to
the request to not let .apply return before the configured state is
active, but I didn't thought this to an end what the real consequences
have to be.

> > > If counter is enabled, and for edge aligned PWM mode(EPWM), the
> > > register is updated After written and TPM counter changes from MOD to
> > > zero, same as period count update, HW will make sure the period finish..
> > 
> > Looking into my concern again, it is actually the other way around:
> > Assuming a single used PWM channel that runs at duty_cycle=500 +
> > period=1000. Then pwm_imx_tpm_apply() is called with state-
> > >duty_cycle=700 and state->period=800. pwm_imx_tpm_apply() calls
> > pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the
> > PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV to
> > the value corresponding to duty_cycle=700, it produces a waveform with
> > duty_cycle=500 and period=800 which is bad. This is another limitation that
> > can be worked around in software with some effort (which might or might
> > not be worth to spend).
> 
> I am sure that on i.MX7ULP platform we used for backlight ONLY, it should NOT
> that matter if this case happen, unless the counter is disabled, then the effort spend
> on this case will be huge, so I plan to leave it as what it is if you don't mind.

That means you have to add this to the list of limitations.

> > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > > > > +			      struct pwm_device *pwm,
> > > > > +			     struct pwm_state *state)
> > > > > +{
> > > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > > +	struct imx_tpm_pwm_param param;
> > > > > +	struct pwm_state real_state;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = pwm_imx_tpm_round_state(chip, &param, state, &real_state);
> > > > > +	if (ret)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	mutex_lock(&tpm->lock);
> > > > > +
> > > > > +	/*
> > > > > +	 * TPM counter is shared by multiple channels, so
> > > > > +	 * prescale and period can NOT be modified when
> > > > > +	 * there are multiple channels in use with different
> > > > > +	 * period settings.
> > > > > +	 */
> > > > > +	if (real_state.period != tpm->real_period) {
> > > > > +		if (tpm->user_count > 1) {
> > > > > +			ret = -EBUSY;
> > > > > +			goto exit;
> > > > > +		}
> > > > > +
> > > > > +		pwm_imx_tpm_setup_period(chip, &param);
> > > > > +		tpm->real_period = real_state.period;
> > > > > +	}
> > > > > +
> > > > > +	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > > > > +
> > > > > +exit:
> > > > > +	mutex_unlock(&tpm->lock);
> > > >
> > > > .apply is supposed to sleep until the newly configured state is active.
> > > > This is missing here, right?
> > >
> > > NOT quite understand, you meant .apply could be sleep if mutex is hold
> > > by other thread?
> > 
> > No. .apply is supposed to only return when the new configuration is active.
> > So if the PWM is running in its previous configuration, you setup the registers
> > such that the new configuration gets active in the next period, you must not
> > yet return to the caller until the new period started.
> 
> That bring me back to previous question, we can add waiting for period finish
> And then return from .apply, but we also need a timeout for the wait, what should
> The timeout value be? 100mS? Or even several seconds?

A sane value would be the duration of the previously configured period
length as this is the theoretical upper limit for this delay.

Best regards
Uwe
Anson Huang March 21, 2019, 2:53 p.m. UTC | #6
Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月21日 21:42
> 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 V8 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hello,
> 
> On Thu, Mar 21, 2019 at 12:47:32PM +0000, Anson Huang wrote:
> > > On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote:
> > > > > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote:
> > > > > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
> > > > > > +				     struct imx_tpm_pwm_param *p) {
> > > > > > +	struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > > +	u32 val, saved_cmod, cur_prescale;
> > > > > > +
> > > > > > +	/* make sure counter is disabled for programming prescale
> */
> > > > >
> > > > > @Thierry: What is your thought here? IMHO this should only be
> > > > > allowed if all affected PWMs are off.
> > > >
> > > > As we already make sure that ONLY when there is ONLY 1 user and
> > > > the requested period is different from the current period, then
> > > > this function will be called, so there is impossible that multiple
> > > > PWMs are active
> > > and the period is requested to be changed.
> > > > Am I right?
> > >
> > > This problem is not about two PWMs. If you reconfigure a running PWM
> > > the requirement is that the hardware completes a whole period with
> > > the old configuration and then immediately starts a new period with
> > > the new parameters. If you stop the counter, the last period with
> > > the old parameters is disturbed.
> >
> > So, I think simply return error if the counter is running and there is
> > a new PS change request, right?
> 
> That's the general idea yes. If you cannot fulfil the request without violating
> the guarantees you have to adhere, refuse the request with an error.

Already add below check in next version:

         val = readl(tpm->base + PWM_IMX_TPM_SC);
         cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
         cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
         if (cmod && cur_prescale != p->prescale)
                 return -EBUSY;

> 
> > > > > > +		 */
> > > > > > +		state->polarity = PWM_POLARITY_NORMAL;
> > > > > > +
> > > > > > +	/* get channel status */
> > > > > > +	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ?
> true :
> > > > > > +false; }
> > > > > > +
> > > > > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > > > > +				 struct pwm_device *pwm,
> > > > > > +				 struct pwm_state *state)
> > > > >
> > > > > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this
> necessary?
> > > > > Please either call it without the mutex or annotate the function
> > > > > that the caller is supposed to hold it.
> > > >
> > > > OK, will make sure call it without mutex hold.

After further check, since this function will call get_state and it will access
shared registers, so mutex is necessary I think, so I will add annotate for this
function.

> 
> Reading through the reference manual I noticed that there might be a
> stall: If you write two values to CnV the second write is ignored if the first
> wasn't latched yet. That might mean that you cannot release the mutex
> before the newly configured state is active. This is related to the request to
> not let .apply return before the configured state is active, but I didn't thought
> this to an end what the real consequences have to be.

The reference manual says the register is NOT updated until the current period finished
If counter is running, so I added below check for both period update and duty update, we
Can just wait the register value read matches what we write:

Period update:
         writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);

         /* make sure MOD register is updated */
         timeout = jiffies + msecs_to_jiffies(tpm->real_period /
                                              NSEC_PER_MSEC + 1);
         while (readl(tpm->base + PWM_IMX_TPM_MOD != p->mod)) {
                 if (time_after(jiffies, timeout))
                         return = -ETIME;
                 cpu_relax();
         }

Duty update:
                 writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));

                 /* make sure CnV register is updated */
                 timeout = jiffies + msecs_to_jiffies(tpm->real_period /
                                                      NSEC_PER_MSEC + 1);
                 while (readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) != val) {
                         if (time_after(jiffies, timeout))
                                 return = -ETIME;
                         cpu_relax();
                 }


> 
> > > > If counter is enabled, and for edge aligned PWM mode(EPWM), the
> > > > register is updated After written and TPM counter changes from MOD
> > > > to zero, same as period count update, HW will make sure the period
> finish..
> > >
> > > Looking into my concern again, it is actually the other way around:
> > > Assuming a single used PWM channel that runs at duty_cycle=500 +
> > > period=1000. Then pwm_imx_tpm_apply() is called with state-
> > > >duty_cycle=700 and state->period=800. pwm_imx_tpm_apply() calls
> > > pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the
> > > PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV
> to
> > > the value corresponding to duty_cycle=700, it produces a waveform
> > > with
> > > duty_cycle=500 and period=800 which is bad. This is another
> > > limitation that can be worked around in software with some effort
> > > (which might or might not be worth to spend).
> >
> > I am sure that on i.MX7ULP platform we used for backlight ONLY, it
> > should NOT that matter if this case happen, unless the counter is
> > disabled, then the effort spend on this case will be huge, so I plan to leave
> it as what it is if you don't mind.
> 
> That means you have to add this to the list of limitations.

OK, will try best to describe this scenario in limitation note;

> 
> > > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip,
> > > > > > +			      struct pwm_device *pwm,
> > > > > > +			     struct pwm_state *state) {
> > > > > > +	struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > > +	struct imx_tpm_pwm_param param;
> > > > > > +	struct pwm_state real_state;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = pwm_imx_tpm_round_state(chip, &param, state,
> &real_state);
> > > > > > +	if (ret)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	mutex_lock(&tpm->lock);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * TPM counter is shared by multiple channels, so
> > > > > > +	 * prescale and period can NOT be modified when
> > > > > > +	 * there are multiple channels in use with different
> > > > > > +	 * period settings.
> > > > > > +	 */
> > > > > > +	if (real_state.period != tpm->real_period) {
> > > > > > +		if (tpm->user_count > 1) {
> > > > > > +			ret = -EBUSY;
> > > > > > +			goto exit;
> > > > > > +		}
> > > > > > +
> > > > > > +		pwm_imx_tpm_setup_period(chip, &param);
> > > > > > +		tpm->real_period = real_state.period;
> > > > > > +	}
> > > > > > +
> > > > > > +	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> > > > > > +
> > > > > > +exit:
> > > > > > +	mutex_unlock(&tpm->lock);
> > > > >
> > > > > .apply is supposed to sleep until the newly configured state is active.
> > > > > This is missing here, right?
> > > >
> > > > NOT quite understand, you meant .apply could be sleep if mutex is
> > > > hold by other thread?
> > >
> > > No. .apply is supposed to only return when the new configuration is
> active.
> > > So if the PWM is running in its previous configuration, you setup
> > > the registers such that the new configuration gets active in the
> > > next period, you must not yet return to the caller until the new period
> started.
> >
> > That bring me back to previous question, we can add waiting for period
> > finish And then return from .apply, but we also need a timeout for the
> > wait, what should The timeout value be? 100mS? Or even several seconds?
> 
> A sane value would be the duration of the previously configured period
> length as this is the theoretical upper limit for this delay.

I will just use tpm->real_period saved in driver data, for first time update, although
Tpm->real_period is 0, but the counter is disabled, so the register update is immediately,
From second time, the tpm->real_period will be previous period configured.

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&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cae
> 5949f109c5401a2b5d08d6ae030d7f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636887725468509805&amp;sdata=4qU1eJzlIle9v%2BHqEqwb
> Jm%2FkkAQlCH2LtsFDMaeSGHE%3D&amp;reserved=0  |
Uwe Kleine-König March 21, 2019, 3:47 p.m. UTC | #7
Hello,

On Thu, Mar 21, 2019 at 02:53:06PM +0000, Anson Huang wrote:
> > Reading through the reference manual I noticed that there might be a
> > stall: If you write two values to CnV the second write is ignored if the first
> > wasn't latched yet. That might mean that you cannot release the mutex
> > before the newly configured state is active. This is related to the request to
> > not let .apply return before the configured state is active, but I didn't thought
> > this to an end what the real consequences have to be.
> 
> The reference manual says the register is NOT updated until the current period finished
> If counter is running, so I added below check for both period update and duty update, we
> Can just wait the register value read matches what we write:
> 
> Period update:
>          writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
> 
>          /* make sure MOD register is updated */
>          timeout = jiffies + msecs_to_jiffies(tpm->real_period /
>                                               NSEC_PER_MSEC + 1);
>          while (readl(tpm->base + PWM_IMX_TPM_MOD != p->mod)) {
>                  if (time_after(jiffies, timeout))
>                          return = -ETIME;
>                  cpu_relax();
>          }

Hmm, I'm not convinced. There are several things to keep in mind:

 a) you should try to update period and duty_cycle atomically, that is,
    the new values should get both active at the same time. So if the
    PWM is running and the first of the two parameters are written to
    the hardware, the second parameter must be written, too, before the
    current period ends.

 b) A write to CnV or MOD blocks further writes until the respective
    value is "updated" (which I think means "latched into the hardware's
    logic to take effect). So you need to make sure that when updating
    the parameters of a running PWM, you didn't already configure it
    since the current period started.

b) is automatically addressed if you only return from .apply() when the
new configuration is active and hold the mutex during that time.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 54f8238..3ea0391 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -210,6 +210,17 @@  config PWM_IMX27
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-imx27.
 
+config PWM_IMX_TPM
+	tristate "i.MX TPM PWM support"
+	depends on ARCH_MXC || COMPILE_TEST
+	depends on HAVE_CLK && HAS_IOMEM
+	help
+	  Generic PWM framework driver for i.MX7ULP TPM module, TPM's full
+	  name is Low Power Timer/Pulse Width Modulation Module.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-imx-tpm.
+
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 448825e..c368599 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -19,6 +19,7 @@  obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
 obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
 obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
+obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
 obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
new file mode 100644
index 0000000..0efea36
--- /dev/null
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -0,0 +1,435 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018-2019 NXP.
+ *
+ * Limitations:
+ * - The TPM counter and period counter are shared between
+ *   multiple channels, so all channels should use same period
+ *   settings.
+ */
+
+#include <linux/bitfield.h>
+#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>
+
+#define PWM_IMX_TPM_PARAM	0x4
+#define PWM_IMX_TPM_GLOBAL	0x8
+#define PWM_IMX_TPM_SC		0x10
+#define PWM_IMX_TPM_CNT		0x14
+#define PWM_IMX_TPM_MOD		0x18
+#define PWM_IMX_TPM_CnSC(n)	(0x20 + (n) * 0x8)
+#define PWM_IMX_TPM_CnV(n)	(0x24 + (n) * 0x8)
+
+#define PWM_IMX_TPM_PARAM_CHAN			GENMASK(7, 0)
+
+#define PWM_IMX_TPM_SC_PS			GENMASK(2, 0)
+#define PWM_IMX_TPM_SC_CMOD			GENMASK(4, 3)
+#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK	FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1)
+#define PWM_IMX_TPM_SC_CPWMS			BIT(5)
+
+#define PWM_IMX_TPM_CnSC_CHF	BIT(7)
+#define PWM_IMX_TPM_CnSC_MSB	BIT(5)
+#define PWM_IMX_TPM_CnSC_MSA	BIT(4)
+
+/*
+ * The reference manual describes this field as two separate bits. The
+ * samantic of the two bits isn't orthogonal though, so they are treated
+ * together as a 2-bit field here.
+ */
+#define PWM_IMX_TPM_CnSC_ELS	GENMASK(3, 2)
+#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED	0x1
+#define PWM_IMX_TPM_CnSC_ELS_INVERSED	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1)
+#define PWM_IMX_TPM_CnSC_ELS_NORMAL	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2)
+
+
+#define PWM_IMX_TPM_MOD_WIDTH	16
+#define PWM_IMX_TPM_MOD_MOD	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)
+
+struct imx_tpm_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	void __iomem *base;
+	struct mutex lock;
+	u32 user_count;
+	u32 enable_count;
+	u32 real_period;
+};
+
+struct imx_tpm_pwm_param {
+	u8 prescale;
+	u32 mod;
+};
+
+static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct imx_tpm_pwm_chip, chip);
+}
+
+static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
+				   struct imx_tpm_pwm_param *p,
+				   struct pwm_state *state,
+				   struct pwm_state *real_state)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+	u32 rate, prescale, period_count, clock_unit;
+	u64 tmp;
+
+	rate = clk_get_rate(tpm->clk);
+	tmp = (u64)state->period * rate;
+	clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
+	if (clock_unit <= PWM_IMX_TPM_MOD_MOD)
+		prescale = 0;
+	else
+		prescale = ilog2(clock_unit) + 1 - PWM_IMX_TPM_MOD_WIDTH;
+
+	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
+		return -ERANGE;
+	p->prescale = prescale;
+
+	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
+	p->mod = period_count;
+
+	/* calculate real period HW can support */
+	tmp = (u64)period_count << prescale;
+	tmp *= NSEC_PER_SEC;
+	real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+	/*
+	 * if eventually the PWM output is inactive, either
+	 * duty cycle is 0 or status is disabled, need to
+	 * make sure the output pin is inactive.
+	 */
+	if (!state->enabled)
+		real_state->duty_cycle = 0;
+	else
+		real_state->duty_cycle = state->duty_cycle;
+
+	real_state->polarity = state->polarity;
+	real_state->enabled = state->enabled;
+
+	return 0;
+}
+
+static void pwm_imx_tpm_setup_period(struct pwm_chip *chip,
+				     struct imx_tpm_pwm_param *p)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+	u32 val, saved_cmod, cur_prescale;
+
+	/* make sure counter is disabled for programming prescale */
+	val = readl(tpm->base + PWM_IMX_TPM_SC);
+	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
+	cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
+	if (saved_cmod && cur_prescale != p->prescale) {
+		val &= ~PWM_IMX_TPM_SC_CMOD;
+		writel(val, tpm->base + PWM_IMX_TPM_SC);
+	}
+
+	/* set TPM counter prescale */
+	val &= ~PWM_IMX_TPM_SC_PS;
+	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
+	writel(val, tpm->base + PWM_IMX_TPM_SC);
+
+	/* restore the clock mode if necessary */
+	if (saved_cmod && cur_prescale != p->prescale) {
+		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
+		writel(val, tpm->base + PWM_IMX_TPM_SC);
+	}
+
+	/*
+	 * set period count:
+	 * according to RM, the MOD register is updated immediately
+	 * if CMOD[1:0] = 2b'00. if CMOD[1:0] != 2b'00, then MOD
+	 * register is updated according to the CPWMS bit, that is:
+	 *
+	 * if the selected mode is not CPWM then MOD register is
+	 * updated after MOD register was written and the TPM
+	 * counter changes from MOD to zero.
+	 *
+	 * if the selected mode is CPWM then MOD register is updated
+	 * after MOD register was written and the TPM counter changes
+	 * from MOD to (MOD – 1).
+	 */
+	writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
+}
+
+static void pwm_imx_tpm_get_state(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+	u32 rate, val, prescale;
+	u64 tmp;
+
+	/* get period */
+	state->period = tpm->real_period;
+
+	/* get duty cycle */
+	rate = clk_get_rate(tpm->clk);
+	val = readl(tpm->base + PWM_IMX_TPM_SC);
+	prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
+	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
+	tmp = (tmp << prescale) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+	/* get polarity */
+	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+	if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
+	    PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
+		state->polarity = PWM_POLARITY_INVERSED;
+	else
+		/*
+		 * Assume reserved values (2b00 and 2b11) to yield
+		 * normal polarity.
+		 */
+		state->polarity = PWM_POLARITY_NORMAL;
+
+	/* get channel status */
+	state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
+}
+
+static void pwm_imx_tpm_apply_hw(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 c;
+	u32 val, sc_val;
+	u64 tmp;
+
+	pwm_imx_tpm_get_state(chip, pwm, &c);
+
+	if (state->duty_cycle != c.duty_cycle) {
+		/* set duty counter */
+		tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
+		tmp *= state->duty_cycle;
+		val = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
+		writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
+	}
+
+	if (state->enabled != c.enabled) {
+		/*
+		 * set polarity (for edge-aligned PWM modes)
+		 *
+		 * ELS[1:0] = 2b10 yields normal polarity behaviour,
+		 * ELS[1:0] = 2b01 yields inversed polarity.
+		 * The other values are reserved.
+		 *
+		 * polarity settings will enabled/disable output status
+		 * immediately, so if the channel is disabled, need to
+		 * make sure MSA/MSB/ELS are set to 0 which means channel
+		 * disabled.
+		 */
+		val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+		val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA |
+			 PWM_IMX_TPM_CnSC_MSB);
+		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
+		if (state->enabled) {
+			val |= PWM_IMX_TPM_CnSC_MSB;
+			val |= (state->polarity == PWM_POLARITY_NORMAL) ?
+				PWM_IMX_TPM_CnSC_ELS_NORMAL :
+				PWM_IMX_TPM_CnSC_ELS_INVERSED;
+			if (++tpm->enable_count == 1) {
+				/* start TPM counter */
+				sc_val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
+				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
+			}
+		} else {
+			if (--tpm->enable_count == 0) {
+				/* stop TPM counter */
+				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
+				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
+			}
+		}
+		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+	}
+}
+
+static int pwm_imx_tpm_apply(struct pwm_chip *chip,
+			      struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+	struct imx_tpm_pwm_param param;
+	struct pwm_state real_state;
+	int ret;
+
+	ret = pwm_imx_tpm_round_state(chip, &param, state, &real_state);
+	if (ret)
+		return -EINVAL;
+
+	mutex_lock(&tpm->lock);
+
+	/*
+	 * TPM counter is shared by multiple channels, so
+	 * prescale and period can NOT be modified when
+	 * there are multiple channels in use with different
+	 * period settings.
+	 */
+	if (real_state.period != tpm->real_period) {
+		if (tpm->user_count > 1) {
+			ret = -EBUSY;
+			goto exit;
+		}
+
+		pwm_imx_tpm_setup_period(chip, &param);
+		tpm->real_period = real_state.period;
+	}
+
+	pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
+
+exit:
+	mutex_unlock(&tpm->lock);
+
+	return ret;
+}
+
+static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+
+	mutex_lock(&tpm->lock);
+	tpm->user_count++;
+	mutex_unlock(&tpm->lock);
+
+	return 0;
+}
+
+static void pwm_imx_tpm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+
+	mutex_lock(&tpm->lock);
+	tpm->user_count--;
+	mutex_unlock(&tpm->lock);
+}
+
+static const struct pwm_ops imx_tpm_pwm_ops = {
+	.request = pwm_imx_tpm_request,
+	.free = pwm_imx_tpm_free,
+	.get_state = pwm_imx_tpm_get_state,
+	.apply = pwm_imx_tpm_apply,
+	.owner = THIS_MODULE,
+};
+
+static int pwm_imx_tpm_probe(struct platform_device *pdev)
+{
+	struct imx_tpm_pwm_chip *tpm;
+	int ret;
+	u32 val;
+
+	tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL);
+	if (!tpm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, tpm);
+
+	tpm->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(tpm->base))
+		return PTR_ERR(tpm->base);
+
+	tpm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(tpm->clk)) {
+		ret = PTR_ERR(tpm->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev,
+				"failed to get PWM clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(tpm->clk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to prepare or enable clock: %d\n", ret);
+		return ret;
+	}
+
+	tpm->chip.dev = &pdev->dev;
+	tpm->chip.ops = &imx_tpm_pwm_ops;
+	tpm->chip.base = -1;
+	tpm->chip.of_xlate = of_pwm_xlate_with_flags;
+	tpm->chip.of_pwm_n_cells = 3;
+
+	/* get number of channels */
+	val = readl(tpm->base + PWM_IMX_TPM_PARAM);
+	tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val);
+
+	mutex_init(&tpm->lock);
+
+	ret = pwmchip_add(&tpm->chip);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+		clk_disable_unprepare(tpm->clk);
+	}
+
+	return ret;
+}
+
+static int pwm_imx_tpm_remove(struct platform_device *pdev)
+{
+	struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev);
+	int ret = pwmchip_remove(&tpm->chip);
+
+	clk_disable_unprepare(tpm->clk);
+
+	return ret;
+}
+
+static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev)
+{
+	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
+
+	if (tpm->enable_count > 0)
+		return -EBUSY;
+
+	clk_disable_unprepare(tpm->clk);
+
+	return 0;
+}
+
+static int __maybe_unused pwm_imx_tpm_resume(struct device *dev)
+{
+	struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev);
+	int ret = 0;
+
+	ret = clk_prepare_enable(tpm->clk);
+	if (ret)
+		dev_err(dev,
+			"failed to prepare or enable clock: %d\n",
+			ret);
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(imx_tpm_pwm_pm,
+			 pwm_imx_tpm_suspend, pwm_imx_tpm_resume);
+
+static const struct of_device_id imx_tpm_pwm_dt_ids[] = {
+	{ .compatible = "fsl,imx-tpm", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_tpm_pwm_dt_ids);
+
+static struct platform_driver imx_tpm_pwm_driver = {
+	.driver = {
+		.name = "imx-tpm-pwm",
+		.of_match_table = imx_tpm_pwm_dt_ids,
+		.pm = &imx_tpm_pwm_pm,
+	},
+	.probe	= pwm_imx_tpm_probe,
+	.remove = pwm_imx_tpm_remove,
+};
+module_platform_driver(imx_tpm_pwm_driver);
+
+MODULE_AUTHOR("Anson Huang <Anson.Huang@nxp.com>");
+MODULE_DESCRIPTION("i.MX TPM PWM Driver");
+MODULE_LICENSE("GPL v2");