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, ¶m, 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, > > ¶m); > > > + 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&data=02%7C01%7Canson.huang%40nxp.com%7Ca7 > > > 1937c9d5e84033309008d6b1048c3a%7C686ea1d3bc2b4c6fa92cd99c5c30163 > > > 5%7C0%7C0%7C636891030423087284&sdata=a3xsu9iaAGvfUYv%2FNo6 > > T5Uvw6k%2F5VbyI2cFzsrnA4IM%3D&reserved=0 |
