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

From: Anson Huang <anson.huang@nxp.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: "thierry.reding@gmail.com" <thierry.reding@gmail.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"stefan@agner.ch" <stefan@agner.ch>,
	"otavio@ossystems.com.br" <otavio@ossystems.com.br>,
	Leonard Crestez <leonard.crestez@nxp.com>,
	"schnitzeltony@gmail.com" <schnitzeltony@gmail.com>,
	"jan.tuerk@emtrion.com" <jan.tuerk@emtrion.com>,
	Robin Gong <yibin.gong@nxp.com>,
	"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
Date: Thu, 14 Mar 2019 14:42:04 +0000	[thread overview]
Message-ID: <DB3PR0402MB3916A861384007BE0F8F7EB0F54B0@DB3PR0402MB3916.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20190314091702.sooxvnzd4mqr2ilz@pengutronix.de>

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月14日 17:17
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux@armlinux.org.uk; stefan@agner.ch;
> otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; jan.tuerk@emtrion.com; Robin Gong
> <yibin.gong@nxp.com>; linux-pwm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support
> 
> On Wed, Mar 13, 2019 at 07:31:16AM +0000, Anson Huang wrote:
> > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module)
> > inside, add TPM PWM driver support.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> > Changes since V1:
> > 	- improve coding style, function name's prefix;
> > 	- improve Kconfig's help info;
> > 	- use .apply instead for .enable/.disable/.config etc. to simply the
> code;
> > 	- improve clock operation, make clock enabled during probe phase
> and ONLY disabled
> > 	  when suspend, as register read/write need to sync with clock,
> keeping it enabled
> > 	  makes the register read/write simple;
> > 	- improve prescale calculation;
> > 	- add error message print during probe for ioremap and clk get;
> > ---
> >  drivers/pwm/Kconfig       |  10 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-imx-tpm.c | 332
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 343 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > a8f47df..c1cbb43 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -201,6 +201,16 @@ config PWM_IMX
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-imx.
> >
> > +config PWM_IMX_TPM
> > +	tristate "i.MX TPM PWM support"
> > +	depends on ARCH_MXC
> 
> Can you please make this
> 
> 	depends on ARCH_MXC || COMPILE_TEST
> 	depends on HAVE_CLK && HAS_IOMEM

Will fix it in V4.

> 
> > +	help
> > +	  Generic PWM framework driver for i.MX7ULP TPM module, TPM's
> full
> > +	  name is Low Power Timer/Pulse Width Modulation Module.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-imx-tpm.
> > +
> >  config PWM_JZ4740
> >  	tristate "Ingenic JZ47xx PWM support"
> >  	depends on MACH_INGENIC
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index
> > 9c676a0..64e036c 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
> >  obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
> >  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> >  obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
> > +obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> >  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> >  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c
> new
> > file mode 100644 index 0000000..8c1a1ba
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,332 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +
> > +#define TPM_GLOBAL	0x8
> > +#define TPM_SC		0x10
> > +#define TPM_CNT		0x14
> > +#define TPM_MOD		0x18
> > +#define TPM_C0SC	0x20
> > +#define TPM_C0V		0x24
> 
> PWM_IMX_TPM_COV etc. please

Will fix it in V4.

> 
> > +
> > +#define TPM_SC_CMOD_SHIFT	3
> > +#define TPM_SC_CMOD_MASK	(0x3 << TPM_SC_CMOD_SHIFT)
> 
> If you use the macros that are documented in <linux/bitops.h> you don't
> need these MASK and SHIFT stuff.

I will use below in V4:
#define PWM_IMX_TPM_SC_CMOD                     GENMASK(4, 3)
#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK       BIT(3)

> 
> > +#define TPM_SC_CPWMS		BIT(5)
> > +
> > +#define TPM_CnSC_CHF		BIT(7)
> > +#define TPM_CnSC_MSnB		BIT(5)
> > +#define TPM_CnSC_MSnA		BIT(4)
> > +#define TPM_CnSC_ELSnB		BIT(3)
> > +#define TPM_CnSC_ELSnA		BIT(2)
> > +
> > +#define TPM_SC_PS_MASK		0x7
> > +#define TPM_MOD_MOD_MASK	0xffff
> > +
> > +#define TPM_COUNT_MAX		0xffff
> > +
> > +#define TPM_CHn_ADDR_OFFSET	0x8
> > +#define TPM_DEFAULT_PWM_CHANNEL_NUM	2
> 
> Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as
> array size below and when reading
> 
> 	u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> 
> I wonder if the actual number of PWMs can be bigger than the default.
> 

Will use below in V4:
#define PWM_IMX_TPM_MAX_CHANNEL_NUM     6

> > +struct imx_tpm_pwm_chip {
> > +	struct pwm_chip chip;
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	spinlock_t lock;
> > +	u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +	bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM];
> > +};
> > +
> > +#define to_imx_tpm_pwm_chip(_chip)	container_of(_chip, struct
> imx_tpm_pwm_chip, chip)
> > +
> > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	unsigned int period_cnt;
> > +	u32 val, div;
> > +	u64 tmp;
> > +
> > +	tmp = clk_get_rate(tpm->clk);
> > +	tmp *= period;
> > +	val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > +	if (val < TPM_COUNT_MAX)
> > +		div = 0;
> > +	else
> > +		div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX));
> 
> Are you sure you have to divide by TPM_COUNT_MAX and not by
> (TPM_COUNT_MAX + 1)?

Fix it in V4.

> 
> > +	if (div > TPM_SC_PS_MASK) {
> > +		dev_err(chip->dev,
> > +			"failed to find valid prescale value!\n");
> > +		return;
> 
> I think you should handle this failure and make imx_tpm_pwm_apply() fail.

I will use below for V4:

if (div > PWM_IMX_TPM_SC_PS_MASK) {
                 dev_err(chip->dev,
                         "failed to find valid prescale value!\n");
                 return -EINVAL;

> 
> > +	}
> > +	/* set TPM counter prescale */
> > +	val = readl(tpm->base + TPM_SC);
> > +	val &= ~TPM_SC_PS_MASK;
> > +	val |= div;
> > +	writel(val, tpm->base + TPM_SC);
> 
> According to the documentation PS can only be written if the counter is
> disabled, I think this isn't ensured here.

In V4, I will disable the counter before programming PS, then restore the counter's
CMOD after PS is written.

> 
> > +	/* set period counter */
> > +	do_div(tmp, NSEC_PER_SEC);
> > +	period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div);
> > +	writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base +
> TPM_MOD);
> 
> If there is already a value pending in the MOD register, another write to it is
> ignored. A comment, why this cannot happen, would be appropriate. (Note,
> I'm not sure this cannot happen.)

From the RM, when CMOD[1:0] = 2b'00, which means when counter is disabled,
the MOD register will be updated once it is written, so I just put the comments
there to avoid confusion, no need to check if there is value pending in MOD register,
as in V4, I already make sure counter is disabled before programming MOD.

> 
> > +}
> > +
> > +static void imx_tpm_pwm_config(struct pwm_chip *chip,
> > +			       struct pwm_device *pwm,
> > +			       struct pwm_state *state)
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	static bool tpm_cnt_initialized;
> > +	unsigned int duty_cnt;
> > +	u32 val;
> > +	u64 tmp;
> > +
> > +	/*
> > +	 * TPM counter is shared by multi channels, let's make it to be
> > +	 * ONLY first channel can config TPM counter's precale and period
> > +	 * count.
> > +	 */
> > +	if (!tpm_cnt_initialized) {
> > +		imx_tpm_pwm_config_counter(chip, state->period);
> > +		tpm_cnt_initialized = true;
> > +	}
> 
> So the period can only be configured once. That is not as good as it could be.
> You should allow a change whenever there is exactly one PWM in use.

In V4, I added user_count to determine the PWM channels used, if there is ONLY
1 channel used, then period and prescale will can be configured anytime.

> 
> > +	/* set duty counter */
> > +	tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK;
> > +	tmp *= state->duty_cycle;
> > +	duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period);
> 
> Uah, you use state->period here even though for the 2nd PWM the Divider
> might not be setup appropriately.
> 
> > [...]
> > +static int imx_tpm_pwm_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > +			  struct pwm_state *state)
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	struct pwm_state curstate;
> > +	unsigned long flags;
> > +
> > +	imx_tpm_pwm_get_state(chip, pwm, &curstate);
> > +
> > +	spin_lock_irqsave(&tpm->lock, flags);
> > +
> > +	if (state->period != curstate.period ||
> > +	    state->duty_cycle != curstate.duty_cycle ||
> > +	    state->polarity != curstate.polarity)
> > +		imx_tpm_pwm_config(chip, pwm, state);
> > +
> > +	if (state->enabled != curstate.enabled)
> > +		imx_tpm_pwm_enable(chip, pwm, state->enabled);
> 
> This is wrong. This sequence:
> 
> 	pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled =
> true });
> 	pwm_apply_state(pwm, { .duty_cycle = 10000, .period =
> 10000, .enabled = false });
> 
> must keep the output pin low which isn't guaranteed here.
> 

Already add checking to make sure the upper case NOT happen in V4.

> > +
> > +	spin_unlock_irqrestore(&tpm->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> I didn't look in depth through the complete driver yet, but there is IIRC still
> some feedback on v1 that wasn't addressed in v2 (because v2 was sent
> before the last feedback). So I will look in more depth in v3 (assuming it
> comes late enough address the concerns from this mail).

Since there are different comments in some mails, I double checked it with the
latest V4 patch and try to make sure I don't miss any comment, please help review
V4 patch, if I missed any comment, then I am sorry for wasting your time. I will send
out V4 patch after I done the function test on board tomorrow.

Thanks,
Anson.

> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C8a
> 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636881518529747245&amp;sdata=ozlYfyVaAgMP6JL%2FxS%
> 2FThygVfpqvuyvL7AVpJkHZRtw%3D&amp;reserved=0  |