RE: [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2019年3月25日 19:59
> To: 'Uwe Kleine-König' <u.kleine-koenig@xxxxxxxxxxxxxx>
> Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; stefan@xxxxxxxx;
> otavio@xxxxxxxxxxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>;
> Robin Gong <yibin.gong@xxxxxxx>; jan.tuerk@xxxxxxxxxxx; linux-
> pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> Subject: RE: [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hi, Uwe
> 
> Best Regards!
> Anson Huang
> 
> > -----Original Message-----
> > From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx]
> > Sent: 2019年3月25日 17:30
> > To: Anson Huang <anson.huang@xxxxxxx>
> > Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx;
> > mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; linux@xxxxxxxxxxxxxxx;
> > stefan@xxxxxxxx; otavio@xxxxxxxxxxxxxxxx; Leonard Crestez
> > <leonard.crestez@xxxxxxx>; Robin Gong <yibin.gong@xxxxxxx>;
> > jan.tuerk@xxxxxxxxxxx; linux- pwm@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-arm- kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> > Subject: Re: [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > On Fri, Mar 22, 2019 at 01:48:11AM +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@xxxxxxx>
> > > ---
> > > Changes since V8:
> > > 	- add more limitation notes for period/duty update un-atomic
> > limitations;
> > > 	- add waiting for period/duty update actually applied to HW;
> > > 	- move the duty update into period update function to make them to
> > be updated
> > > 	  as together as possiable;
> > > 	- don't allow PS change if counter is running;
> > > 	- save channel polarity settings and return it directly when
> > > .get_state
> > is called,
> > > 	  as the HW polarity setting could be impacted by enable status.
> > > ---
> > >  drivers/pwm/Kconfig       |  11 +
> > >  drivers/pwm/Makefile      |   1 +
> > >  drivers/pwm/pwm-imx-tpm.c | 518
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 530 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..58af0915
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-imx-tpm.c
> > > @@ -0,0 +1,518 @@
> > > +// 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.
> > > + * - Changes to polarity cannot be latched at the time of the
> > > + *   next period start.
> > > + * - The period and duty changes are NOT atomic, if new period and
> > > + *   new duty are requested simultaneously when counter is running,
> > > + *   there could be a small window of running old duty with new
> > > + *   period, as the period is updated before duty in this driver, the
> > > + *   probability is very low, ONLY happen when the TPM counter changes
> > > + *   from MOD to zero between the consecutive update of period and
> > > + *   duty.
> >
> > The window that this bug triggers is small, but if it does, the window
> > where the invalid combination is applied, isn't small---it's one
> > complete period if I'm not mistaken. So I'd write:
> >
> >  - Changing period and duty cycle together isn't atomic. With the wrong
> >    timing it might happen that a period is produced with old duty cycle
> >    but new period settings.
> >
> 
> OK, thanks.
> 
> > > + */
> > > +
> > > +#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
> >
> > s/samantic/semantic/
> >
> > > + * 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;
> > > +	u32 val;
> > > +};
> > > +
> > > +struct imx_tpm_pwm_channel {
> > > +	enum pwm_polarity polarity;
> > > +};
> > > +
> > > +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;
> > > +
> > > +	tmp = (u64)p->mod * real_state->duty_cycle;
> > > +	p->val = DIV_ROUND_CLOSEST_ULL(tmp, real_state->period);
> > > +
> > > +	real_state->polarity = state->polarity;
> > > +	real_state->enabled = state->enabled;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/* this function is supposed to be called with mutex hold */ static
> > > +int pwm_imx_tpm_setup_period_duty(struct pwm_chip *chip,
> > > +					 struct pwm_device *pwm,
> > > +					 struct imx_tpm_pwm_param *p)
> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	unsigned long timeout;
> > > +	u32 val, cmod, cur_prescale;
> > > +
> > > +	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;
> > > +
> > > +	/* 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);
> > > +
> > > +	/*
> > > +	 * set period count:
> > > +	 * if (CMOD[1:0] = 0:0) then MOD register is updated when MOD
> > > +	 * register is written.
> > > +	 *
> > > +	 * if (CMOD[1:0] ≠ 0:0), 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).
> >
> > Given that the driver doesn't make use of CPWM, this comment could be
> > simplified. I'd write:
> >
> > 	/*
> > 	 * If the PWM is enabled (CMOD[1:0] ≠ 2b00), the period length
> > 	 * is latched into hardware when the next period starts.
> > 	 */
> >
> 
> OK, thanks.
> 
> > This is even true for the (here unused) CPWM mode. (The reference
> > manual isn't entirely clear here IMHO. I assume if MOD == 4000 and CNT
> > == 2001 then MOD is then changed to 2000, the currently running period
> > is completed with a length of 4000 prescaled clk cycles?!)
> 
> Based on my understanding from RM, I think so.
> 
> >
> > > +	 */
> > > +	writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
> > > +
> > > +	/*
> > > +	 * set channel value:
> > > +	 * 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).
> >
> > This is similar to the above too verbose and covers stuff that is not
> > relevant for this driver. Also the used wording is not obvious if you
> > don't look into the reference manual.
> 
> 
> OK, thanks.
> 
> >
> > > +	 */
> > > +	writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +
> > > +	/* make sure MOD & CnV registers are updated */
> > > +	timeout = jiffies + msecs_to_jiffies(tpm->real_period /
> > > +					     NSEC_PER_MSEC + 1);
> > > +	while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod
> > > +	       || readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm))
> > > +	       != p->val) {
> > > +		if (time_after(jiffies, timeout))
> > > +			return -ETIME;
> > > +		cpu_relax();
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > [...]
> > > +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;
> > > +		}
> > > +
> > > +		ret = pwm_imx_tpm_setup_period_duty(chip, pwm,
> > &param);
> > > +		if (ret)
> > > +			goto exit;
> > > +
> > > +		tpm->real_period = real_state.period;
> > > +	}
> > > +
> > > +	ret = pwm_imx_tpm_apply_hw(chip, pwm, &real_state);
> >
> > It's unintuitive here that both pwm_imx_tpm_setup_period_duty and
> > pwm_imx_tpm_apply_hw (potentially) configure the duty cycle. I didn't
> > thought it to an end, but maybe this could be optimised?
> 
> 
> This is also my concern when implementing it, since period needs to be
> configured before duty, and we want to put these 2 configurations as close
> as possible, so for the period change, I also configure the duty together, but
> for normal use cases, period does NOT change, so we also need to consider
> duty change ONLY case, that is why I put a current duty and requested duty
> check in the pwm_imx_tpm_apply_hw() function.
> 
>          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));
> 
> 
> We can also put all the configurations together in 1 function, but that also
> introduce some confusion I think, current implementation separate the
> period settings (for all channels) and other settings (for each channel) in 2
> function, it is just because that the duty change is better to be put as close as
> period change, so I did it this way. Maybe add some comments for it is
> acceptable?

I just sent out V10 patch set to remove the  pwm_imx_tpm_setup_period_duty()
function, and put all the configurations in pwm_imx_tpm_apply_hw() function,
the defect introduced would be a slightly latency between period update and duty
update, it is trivial I think, but it can avoid the duplicated code/function of setting duty.

Thanks.
Anson.


> 
> 
> >
> > > +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);
> > > +	struct imx_tpm_pwm_channel *chan;
> > > +
> > > +	chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL);
> >
> > There is no advantage in using the devm variant here as the requested
> > memory is freed in .free anyhow. So this only adds additional memory
> > foodprint and runtime overhead.
> 
> Makes sense, I will use kzalloc directly, looks like other pwm
> driver(drivers/pwm/pwm-samsung.c) also has such "issue".
> 
> >
> > > +	if (!chan)
> > > +		return -ENOMEM;
> > > +
> > > +	pwm_set_chip_data(pwm, chan);
> > > +
> > > +	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);
> > > +
> > > +	devm_kfree(chip->dev, pwm_get_chip_data(pwm));
> > > +	pwm_set_chip_data(pwm, NULL);
> >
> > The call to pwm_set_chip_data could better be done in the PWM core.
> 
> You meant doing a new patch to add it in PWM core.c after ->free call?
> If yes, then I think this should be another topic, as many other pwm drivers
> also call it in their own driver, maybe it can be improved by another patch?
> 
> Thanks,
> Anson.
> 
> >
> > > +}
> >
> > --
> > 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%7Ca7
> >
> 1937c9d5e84033309008d6b1048c3a%7C686ea1d3bc2b4c6fa92cd99c5c30163
> >
> 5%7C0%7C0%7C636891030423087284&amp;sdata=a3xsu9iaAGvfUYv%2FNo6
> > T5Uvw6k%2F5VbyI2cFzsrnA4IM%3D&amp;reserved=0  |




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux