Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx] > Sent: 2019年3月14日 17:17 > 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>; > schnitzeltony@xxxxxxxxx; jan.tuerk@xxxxxxxxxxx; Robin Gong > <yibin.gong@xxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > 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@xxxxxxx> > > --- > > Changes since V1: > > - improve coding style, function name's prefix; > > - improve Kconfig's help info; > > - use .apply instead for .enable/.disable/.config etc. to simply the > code; > > - improve clock operation, make clock enabled during probe phase > and ONLY disabled > > when suspend, as register read/write need to sync with clock, > keeping it enabled > > makes the register read/write simple; > > - improve prescale calculation; > > - add error message print during probe for ioremap and clk get; > > --- > > drivers/pwm/Kconfig | 10 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-imx-tpm.c | 332 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 343 insertions(+) > > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > a8f47df..c1cbb43 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -201,6 +201,16 @@ config PWM_IMX > > To compile this driver as a module, choose M here: the module > > will be called pwm-imx. > > > > +config PWM_IMX_TPM > > + tristate "i.MX TPM PWM support" > > + depends on ARCH_MXC > > Can you please make this > > depends on ARCH_MXC || COMPILE_TEST > depends on HAVE_CLK && HAS_IOMEM Will fix it in V4. > > > + help > > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's > full > > + name is Low Power Timer/Pulse Width Modulation Module. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-imx-tpm. > > + > > config PWM_JZ4740 > > tristate "Ingenic JZ47xx PWM support" > > depends on MACH_INGENIC > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > > 9c676a0..64e036c 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o > > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o > > obj-$(CONFIG_PWM_IMG) += pwm-img.o > > obj-$(CONFIG_PWM_IMX) += pwm-imx.o > > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c > new > > file mode 100644 index 0000000..8c1a1ba > > --- /dev/null > > +++ b/drivers/pwm/pwm-imx-tpm.c > > @@ -0,0 +1,332 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright 2018-2019 NXP. > > + */ > > + > > +#include <linux/bitops.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/log2.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/slab.h> > > +#include <linux/spinlock.h> > > + > > +#define TPM_GLOBAL 0x8 > > +#define TPM_SC 0x10 > > +#define TPM_CNT 0x14 > > +#define TPM_MOD 0x18 > > +#define TPM_C0SC 0x20 > > +#define TPM_C0V 0x24 > > PWM_IMX_TPM_COV etc. please Will fix it in V4. > > > + > > +#define TPM_SC_CMOD_SHIFT 3 > > +#define TPM_SC_CMOD_MASK (0x3 << TPM_SC_CMOD_SHIFT) > > If you use the macros that are documented in <linux/bitops.h> you don't > need these MASK and SHIFT stuff. I will use below in V4: #define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) #define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3) > > > +#define TPM_SC_CPWMS BIT(5) > > + > > +#define TPM_CnSC_CHF BIT(7) > > +#define TPM_CnSC_MSnB BIT(5) > > +#define TPM_CnSC_MSnA BIT(4) > > +#define TPM_CnSC_ELSnB BIT(3) > > +#define TPM_CnSC_ELSnA BIT(2) > > + > > +#define TPM_SC_PS_MASK 0x7 > > +#define TPM_MOD_MOD_MASK 0xffff > > + > > +#define TPM_COUNT_MAX 0xffff > > + > > +#define TPM_CHn_ADDR_OFFSET 0x8 > > +#define TPM_DEFAULT_PWM_CHANNEL_NUM 2 > > Is this better called "..._MAX_..." instead of "..._DEFAULT_..."? This is used as > array size below and when reading > > u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM]; > > I wonder if the actual number of PWMs can be bigger than the default. > Will use below in V4: #define PWM_IMX_TPM_MAX_CHANNEL_NUM 6 > > +struct imx_tpm_pwm_chip { > > + struct pwm_chip chip; > > + struct clk *clk; > > + void __iomem *base; > > + spinlock_t lock; > > + u32 chn_config[TPM_DEFAULT_PWM_CHANNEL_NUM]; > > + bool chn_status[TPM_DEFAULT_PWM_CHANNEL_NUM]; > > +}; > > + > > +#define to_imx_tpm_pwm_chip(_chip) container_of(_chip, struct > imx_tpm_pwm_chip, chip) > > + > > +static void imx_tpm_pwm_config_counter(struct pwm_chip *chip, u32 > > +period) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + unsigned int period_cnt; > > + u32 val, div; > > + u64 tmp; > > + > > + tmp = clk_get_rate(tpm->clk); > > + tmp *= period; > > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > > + if (val < TPM_COUNT_MAX) > > + div = 0; > > + else > > + div = ilog2(roundup_pow_of_two(val / TPM_COUNT_MAX)); > > Are you sure you have to divide by TPM_COUNT_MAX and not by > (TPM_COUNT_MAX + 1)? Fix it in V4. > > > + if (div > TPM_SC_PS_MASK) { > > + dev_err(chip->dev, > > + "failed to find valid prescale value!\n"); > > + return; > > I think you should handle this failure and make imx_tpm_pwm_apply() fail. I will use below for V4: if (div > PWM_IMX_TPM_SC_PS_MASK) { dev_err(chip->dev, "failed to find valid prescale value!\n"); return -EINVAL; > > > + } > > + /* set TPM counter prescale */ > > + val = readl(tpm->base + TPM_SC); > > + val &= ~TPM_SC_PS_MASK; > > + val |= div; > > + writel(val, tpm->base + TPM_SC); > > According to the documentation PS can only be written if the counter is > disabled, I think this isn't ensured here. In V4, I will disable the counter before programming PS, then restore the counter's CMOD after PS is written. > > > + /* set period counter */ > > + do_div(tmp, NSEC_PER_SEC); > > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div); > > + writel(period_cnt & TPM_MOD_MOD_MASK, tpm->base + > TPM_MOD); > > If there is already a value pending in the MOD register, another write to it is > ignored. A comment, why this cannot happen, would be appropriate. (Note, > I'm not sure this cannot happen.) >From the RM, when CMOD[1:0] = 2b'00, which means when counter is disabled, the MOD register will be updated once it is written, so I just put the comments there to avoid confusion, no need to check if there is value pending in MOD register, as in V4, I already make sure counter is disabled before programming MOD. > > > +} > > + > > +static void imx_tpm_pwm_config(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + static bool tpm_cnt_initialized; > > + unsigned int duty_cnt; > > + u32 val; > > + u64 tmp; > > + > > + /* > > + * TPM counter is shared by multi channels, let's make it to be > > + * ONLY first channel can config TPM counter's precale and period > > + * count. > > + */ > > + if (!tpm_cnt_initialized) { > > + imx_tpm_pwm_config_counter(chip, state->period); > > + tpm_cnt_initialized = true; > > + } > > So the period can only be configured once. That is not as good as it could be. > You should allow a change whenever there is exactly one PWM in use. In V4, I added user_count to determine the PWM channels used, if there is ONLY 1 channel used, then period and prescale will can be configured anytime. > > > + /* set duty counter */ > > + tmp = readl(tpm->base + TPM_MOD) & TPM_MOD_MOD_MASK; > > + tmp *= state->duty_cycle; > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, state->period); > > Uah, you use state->period here even though for the 2nd PWM the Divider > might not be setup appropriately. > > > [...] > > +static int imx_tpm_pwm_apply(struct pwm_chip *chip, struct pwm_device > *pwm, > > + struct pwm_state *state) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + struct pwm_state curstate; > > + unsigned long flags; > > + > > + imx_tpm_pwm_get_state(chip, pwm, &curstate); > > + > > + spin_lock_irqsave(&tpm->lock, flags); > > + > > + if (state->period != curstate.period || > > + state->duty_cycle != curstate.duty_cycle || > > + state->polarity != curstate.polarity) > > + imx_tpm_pwm_config(chip, pwm, state); > > + > > + if (state->enabled != curstate.enabled) > > + imx_tpm_pwm_enable(chip, pwm, state->enabled); > > This is wrong. This sequence: > > pwm_apply_state(pwm, { .duty_cycle = 0, .period = 10000, .enabled = > true }); > pwm_apply_state(pwm, { .duty_cycle = 10000, .period = > 10000, .enabled = false }); > > must keep the output pin low which isn't guaranteed here. > Already add checking to make sure the upper case NOT happen in V4. > > + > > + spin_unlock_irqrestore(&tpm->lock, flags); > > + > > + return 0; > > +} > > I didn't look in depth through the complete driver yet, but there is IIRC still > some feedback on v1 that wasn't addressed in v2 (because v2 was sent > before the last feedback). So I will look in more depth in v3 (assuming it > comes late enough address the concerns from this mail). Since there are different comments in some mails, I double checked it with the latest V4 patch and try to make sure I don't miss any comment, please help review V4 patch, if I missed any comment, then I am sorry for wasting your time. I will send out V4 patch after I done the function test on board tomorrow. Thanks, Anson. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C8a > 834d252f5b4b7c446b08d6a85dde9e%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636881518529747245&sdata=ozlYfyVaAgMP6JL%2FxS% > 2FThygVfpqvuyvL7AVpJkHZRtw%3D&reserved=0 |
