| Message ID | 1552610505-13568-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 |
On Fri, Mar 15, 2019 at 12:46:51AM +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 V3: > - use "PWM_IMX_" as macro definition prefix and "pwm_imx_" as function prefix; > - improve the limitation txt; > - return error for configuring period/prescale fail; > - disable clock when driver probe failed and remove; > - improve module build dependency; > - introduce user_count to determine whether configuing period is allowed; > - some logic improvement for setting duty/status etc.; > --- > drivers/pwm/Kconfig | 12 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-imx-tpm.c | 396 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 409 insertions(+) > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a8f47df..6117fe6 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -201,6 +201,18 @@ 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 || COMPILE_TEST > + depends on HAVE_CLK && HAS_IOMEM > + I think this empty newline is unusual. > + 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..f108f75 > --- /dev/null > +++ b/drivers/pwm/pwm-imx-tpm.c > @@ -0,0 +1,396 @@ > +// 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/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_GLOBAL 0x8 > +#define PWM_IMX_TPM_SC 0x10 > +#define PWM_IMX_TPM_CNT 0x14 > +#define PWM_IMX_TPM_MOD 0x18 > +#define PWM_IMX_TPM_C0SC 0x20 > +#define PWM_IMX_TPM_C0V 0x24 > + > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3) > +#define PWM_IMX_TPM_SC_CPWMS BIT(5) > + > +#define PWM_IMX_TPM_CnSC_CHF BIT(7) > +#define PWM_IMX_TPM_CnSC_MSnB BIT(5) > +#define PWM_IMX_TPM_CnSC_MSnA BIT(4) > +#define PWM_IMX_TPM_CnSC_ELSnB BIT(3) > +#define PWM_IMX_TPM_CnSC_ELSnA BIT(2) > + > +#define PWM_IMX_TPM_SC_PS_MASK 0x7 > +#define PWM_IMX_TPM_MOD_MOD_MASK 0xffff > + > +#define PWM_IMX_TPM_MAX_COUNT 0xffff > + > +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6 > + > +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + n * 0x8) > +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + n * 0x8) parenthesis around n please. > +struct imx_tpm_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *base; > + struct mutex lock; > + u32 user_count; > + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM]; > + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM]; > +}; > + > +#define to_imx_tpm_pwm_chip(_chip) \ > + container_of(_chip, struct imx_tpm_pwm_chip, chip) > + > +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 period) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + u32 period_cnt, val, div, saved_cmod; > + u64 tmp; > + > + tmp = clk_get_rate(tpm->clk); > + tmp *= period; > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > + if (val < PWM_IMX_TPM_MAX_COUNT) <= ? > + div = 0; > + else > + div = ilog2(roundup_pow_of_two(val / > + (PWM_IMX_TPM_MAX_COUNT + 1))); > + if (div > PWM_IMX_TPM_SC_PS_MASK) { #define PWM_IMX_TPM_SC_PS GENMASK(0, 2) if (!FIELD_FIT(PWM_IMX_TPM_SC_PS, div)) { ... > + dev_err(chip->dev, > + "failed to find valid prescale value!\n"); > + return -EINVAL; > + } > + > + /* make sure counter is disabled for programming prescale */ > + val = readl(tpm->base + PWM_IMX_TPM_SC); > + saved_cmod = val & PWM_IMX_TPM_SC_CMOD; saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val) ? > + val &= ~PWM_IMX_TPM_SC_CMOD; > + writel(val, tpm->base + PWM_IMX_TPM_SC); As this interrupts the output, please only do it if necessary. > + /* set TPM counter prescale */ > + val = readl(tpm->base + PWM_IMX_TPM_SC); > + val &= ~PWM_IMX_TPM_SC_PS_MASK; > + val |= div; val |= FIELD_PREP(PWM_IMX_TPM_SC_PS_MASK, div); > + writel(val, tpm->base + PWM_IMX_TPM_SC); > + > + /* > + * set period counter: according to RM, the MOD register is > + * updated immediately when CMOD[1:0] = 2b'00 (counter disabled). updated immediately after CMOD[1:0] = 2b'00 above > + */ > + do_div(tmp, NSEC_PER_SEC); > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div) The (partial) RHS is equivalent to (tmp + (1 << div) >> 1) >> div which might be cheaper for the CPU to calculate. > + & PWM_IMX_TPM_MOD_MOD_MASK; If it can happen, that this masking changes the result, this is an error that needs handling. (And if not, drop it; maybe in favour of a comment.) > + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD); > + > + /* restore the clock mode */ > + val = readl(tpm->base + PWM_IMX_TPM_SC); > + val |= saved_cmod; > + writel(val, tpm->base + PWM_IMX_TPM_SC); > + > + return 0; > +} > + > +static void pwm_imx_tpm_config(struct pwm_chip *chip, > + struct pwm_device *pwm, > + u32 period, > + u32 duty_cycle, > + enum pwm_polarity polarity) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + u32 duty_cnt, val; > + u64 tmp; > + > + /* set duty counter */ > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD_MASK; I recommend storing this value in driver data. > + tmp *= duty_cycle; > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period); > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD_MASK, > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); Please align the 2nd line to the opening parenthesis. > + > + /* set polarity */ > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > + val &= ~(PWM_IMX_TPM_CnSC_ELSnB | PWM_IMX_TPM_CnSC_ELSnA | > + PWM_IMX_TPM_CnSC_MSnA); > + val |= PWM_IMX_TPM_CnSC_MSnB; > + val |= polarity ? PWM_IMX_TPM_CnSC_ELSnA : PWM_IMX_TPM_CnSC_ELSnB; I'd recommend not hard coding here that PWM_POLARITY_NORMAL evaluates to false. In the reference manual I have (Rev. F, 07/2017) MSnA is called MSA only. Ditto for MSnB -> MSB, ELSnA -> ELSA, ELSnB -> ELSB. (Hmm, but it is not entirely consistent. So Table 64-4 indeed has the 'n's.) I wonder why MSA and MSB are two bits instead of making this a field of width 2 with 2b10 meaning PWM mode. But maybe it's just me not understanding the independent semantic of these two bits? Reading the reference manual I'd say in PWM mode the semantic of ELSA and ELSB is: On counter reload set the output to ELSB On counter match set the output to ELSA Noting that in a comment would ease understanding the code here. > + /* > + * polarity settings will enabled/disable output statue s/statue/status/ > + * immediately, so here ONLY save the config and will be > + * written into register when channel is enabled/disabled. s/will be written/write it/ > + */ > + tpm->chn_config[pwm->hwpwm] = val; > +} A comment here about how and when the values written in pwm_imx_tpm_config take effect would be good. > +static void pwm_imx_tpm_enable(struct pwm_chip *chip, > + struct pwm_device *pwm, > + bool enable) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + u32 val, i; > + > + val = readl(tpm->base + PWM_IMX_TPM_SC); > + if (enable) { > + /* restore channel config */ > + writel(tpm->chn_config[pwm->hwpwm], > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > + > + /* start TPM counter anyway */ > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK; > + writel(val, tpm->base + PWM_IMX_TPM_SC); > + } else { > + /* > + * When a channel is disabled, its polarity settings will be > + * saved and its output will be disabled by clearing polarity > + * setting, when channel is enabled, polarity settings will be > + * restored and output will be enabled again. > + */ > + /* save channel config */ > + tpm->chn_config[pwm->hwpwm] = readl(tpm->base + > + PWM_IMX_TPM_CnSC(pwm->hwpwm)); Doesn't tpm->chn_config[pwm->hwpwm] already contain the right value? Please align the 2nd line to the opening parenthesis. > + /* disable channel */ > + writel(PWM_IMX_TPM_CnSC_CHF, > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); Clearing CHF doens't disable the channel as I read the manual. > + for (i = 0; i < chip->npwm; i++) > + if (i != pwm->hwpwm && tpm->chn_status[i]) If you set tpm->chn_status[i] = false before this loop, you don't have to care for i != pwm->hwpwm. If you maintain an "enable count" you don't have to loop at all. > + break; > + if (i == chip->npwm) { > + /* stop TPM counter since all channels are disabled */ > + val &= ~PWM_IMX_TPM_SC_CMOD; > + writel(val, tpm->base + PWM_IMX_TPM_SC); > + } > + } > + > + /* update channel statue */ > + tpm->chn_status[pwm->hwpwm] = enable; > +} > + > +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); > + u64 tmp; > + u32 val, rate; > + > + mutex_lock(&tpm->lock); > + > + /* get period */ > + rate = clk_get_rate(tpm->clk); > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD); > + val = readl(tpm->base + PWM_IMX_TPM_SC); > + val &= PWM_IMX_TPM_SC_PS_MASK; > + tmp *= (1 << val) * NSEC_PER_SEC; > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > + > + /* get duty cycle */ > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > + tmp *= (1 << val) * 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 (val & PWM_IMX_TPM_CnSC_ELSnA) > + state->polarity = PWM_POLARITY_INVERSED; > + else > + state->polarity = PWM_POLARITY_NORMAL; > + > + /* get channel status */ > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; > + > + mutex_unlock(&tpm->lock); > +} > + > +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 pwm_state curstate; > + u32 duty_cycle = state->duty_cycle; > + int ret; > + > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > + > + mutex_lock(&tpm->lock); What should this lock protect? Does it hurt if the state changes between pwm_imx_tpm_get_state releasing the lock and pwm_imx_tpm_apply taking it? > + > + if (state->period != curstate.period) { > + /* > + * TPM counter is shared by multiple channels, so > + * the prescale and period can NOT be modified when > + * there are multiple channels used. s/the //; s/used/in use/ > + */ > + if (tpm->user_count != 1) > + return -EBUSY; Ideally if say period = 37 is requested but currently we have period = 36 and configuring 37 would result in 36 anyhow, don't return EBUSY. > + ret = pwm_imx_tpm_config_counter(chip, state->period); > + if (ret) > + return ret; > + } > + > + if (!state->enabled) > + duty_cycle = 0; A comment above this block that explains why this is done would be great (but see below). > + > + if (state->duty_cycle != curstate.duty_cycle || > + state->polarity != curstate.polarity) > + pwm_imx_tpm_config(chip, pwm, > + state->period, duty_cycle, state->polarity); > + > + if (state->enabled != curstate.enabled) > + pwm_imx_tpm_enable(chip, pwm, state->enabled); This is a bit unintuitive I think. For example I had to think for a while why you pass duty_cycle to pwm_imx_tpm_config() but check state->duty_cycle in the if condition. I'd suggest: if (state->enabled == false) { /* * configure for duty_cycle == 0 here? Wait until this * setting is active? */ if (curstate.enabled) pwm_imx_tpm_enable(chip, pwm, false); } else { ... } > + > + mutex_unlock(&tpm->lock); > + > + return 0; > +} > + > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *dev) > +{ > + 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 *dev) > +{ > + 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 = { > + .get_state = pwm_imx_tpm_get_state, > + .request = pwm_imx_tpm_request, > + .apply = pwm_imx_tpm_apply, > + .free = pwm_imx_tpm_free, > + .owner = THIS_MODULE, Can you please group "request" with "free"? The order as defined in struct pwm_ops would be optimal. > +}; > + > +static int pwm_imx_tpm_probe(struct platform_device *pdev) > +{ > + struct imx_tpm_pwm_chip *tpm; > + struct resource *res; > + int ret; > + > + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL); > + if (!tpm) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, tpm); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + tpm->base = devm_ioremap_resource(&pdev->dev, res); You can use devm_platform_ioremap_resource instead of the two previous calls. > + if (IS_ERR(tpm->base)) { > + ret = PTR_ERR(tpm->base); > + if (ret != -EPROBE_DEFER) > + dev_err(&pdev->dev, "pwm ioremap failed %d\n", ret); > + return ret; > + } > + > + 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 clk %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(tpm->clk); > + if (ret) { > + dev_err(&pdev->dev, > + "failed to prepare or enable clk %d\n", ret); > + return ret; > + } > + > + tpm->chip.dev = &pdev->dev; > + tpm->chip.ops = &imx_tpm_pwm_ops; > + tpm->chip.base = -1; > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; This is wrong, as some only have 2 channels? > + 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); > + > + clk_disable_unprepare(tpm->clk); > + > + return pwmchip_remove(&tpm->chip); Wrong order. Before pwmchip_remove returns the PWM must stay functional. > +} > + > +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) > +{ > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev); > + > + clk_disable_unprepare(tpm->clk); You must not disable the clock if the PWM is in use. > + return 0; > +} The time I want to/can spend on community review is over now for this week. I didn't look at all details yet but I think it is still worth to send this mail out to not make you bored :-) Also I think further thoughts by me will be eased if you address my comments here first. Best regards Uwe
Hi,Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: 2019年3月15日 17:36 > 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; otavio@ossystems.com.br; > stefan@agner.ch; 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 V4 2/5] pwm: Add i.MX TPM PWM driver support > > On Fri, Mar 15, 2019 at 12:46:51AM +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 V3: > > - use "PWM_IMX_" as macro definition prefix and "pwm_imx_" as > function prefix; > > - improve the limitation txt; > > - return error for configuring period/prescale fail; > > - disable clock when driver probe failed and remove; > > - improve module build dependency; > > - introduce user_count to determine whether configuing period is > allowed; > > - some logic improvement for setting duty/status etc.; > > --- > > drivers/pwm/Kconfig | 12 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-imx-tpm.c | 396 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 409 insertions(+) > > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > a8f47df..6117fe6 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -201,6 +201,18 @@ 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 || COMPILE_TEST > > + depends on HAVE_CLK && HAS_IOMEM > > + > > I think this empty newline is unusual. Agreed. > > > + 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..f108f75 > > --- /dev/null > > +++ b/drivers/pwm/pwm-imx-tpm.c > > @@ -0,0 +1,396 @@ > > +// 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/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_GLOBAL 0x8 > > +#define PWM_IMX_TPM_SC 0x10 > > +#define PWM_IMX_TPM_CNT 0x14 > > +#define PWM_IMX_TPM_MOD 0x18 > > +#define PWM_IMX_TPM_C0SC 0x20 > > +#define PWM_IMX_TPM_C0V 0x24 > > + > > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) > > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3) > > +#define PWM_IMX_TPM_SC_CPWMS BIT(5) > > + > > +#define PWM_IMX_TPM_CnSC_CHF BIT(7) > > +#define PWM_IMX_TPM_CnSC_MSnB BIT(5) > > +#define PWM_IMX_TPM_CnSC_MSnA BIT(4) > > +#define PWM_IMX_TPM_CnSC_ELSnB BIT(3) > > +#define PWM_IMX_TPM_CnSC_ELSnA BIT(2) > > + > > +#define PWM_IMX_TPM_SC_PS_MASK 0x7 > > +#define PWM_IMX_TPM_MOD_MOD_MASK 0xffff > > + > > +#define PWM_IMX_TPM_MAX_COUNT 0xffff > > + > > +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6 > > + > > +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + n * 0x8) > > +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + n * 0x8) > > parenthesis around n please. OK. > > > +struct imx_tpm_pwm_chip { > > + struct pwm_chip chip; > > + struct clk *clk; > > + void __iomem *base; > > + struct mutex lock; > > + u32 user_count; > > + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM]; > > + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM]; > > +}; > > + > > +#define to_imx_tpm_pwm_chip(_chip) \ > > + container_of(_chip, struct imx_tpm_pwm_chip, chip) > > + > > +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 > > +period) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 period_cnt, val, div, saved_cmod; > > + u64 tmp; > > + > > + tmp = clk_get_rate(tpm->clk); > > + tmp *= period; > > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > > + if (val < PWM_IMX_TPM_MAX_COUNT) > > <= ? Agreed. > > > + div = 0; > > + else > > + div = ilog2(roundup_pow_of_two(val / > > + (PWM_IMX_TPM_MAX_COUNT + 1))); > > + if (div > PWM_IMX_TPM_SC_PS_MASK) { > > #define PWM_IMX_TPM_SC_PS GENMASK(0, 2) > > if (!FIELD_FIT(PWM_IMX_TPM_SC_PS, div)) { > ... OK. > > > + dev_err(chip->dev, > > + "failed to find valid prescale value!\n"); > > + return -EINVAL; > > + } > > + > > + /* make sure counter is disabled for programming prescale */ > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + saved_cmod = val & PWM_IMX_TPM_SC_CMOD; > > saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val) ? OK. > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > As this interrupts the output, please only do it if necessary. OK, will do it ONLY when it is enabled previously. > > > + /* set TPM counter prescale */ > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + val &= ~PWM_IMX_TPM_SC_PS_MASK; > > + val |= div; > > val |= FIELD_PREP(PWM_IMX_TPM_SC_PS_MASK, div); OK. > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + > > + /* > > + * set period counter: according to RM, the MOD register is > > + * updated immediately when CMOD[1:0] = 2b'00 (counter disabled). > > updated immediately after CMOD[1:0] = 2b'00 above > > > + */ > > + do_div(tmp, NSEC_PER_SEC); > > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div) > > The (partial) RHS is equivalent to > > (tmp + (1 << div) >> 1) >> div > > which might be cheaper for the CPU to calculate. OK > > > + & PWM_IMX_TPM_MOD_MOD_MASK; > > If it can happen, that this masking changes the result, this is an error that > needs handling. (And if not, drop it; maybe in favour of a > comment.) Will use below for error check: if (period_cnt > PWM_IMX_TPM_MOD_MOD) { dev_err(chip->dev, "failed to find valid period count!\n"); return -EINVAL; } > > > + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD); > > + > > + /* restore the clock mode */ > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + val |= saved_cmod; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + > > + return 0; > > +} > > + > > +static void pwm_imx_tpm_config(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + u32 period, > > + u32 duty_cycle, > > + enum pwm_polarity polarity) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 duty_cnt, val; > > + u64 tmp; > > + > > + /* set duty counter */ > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > PWM_IMX_TPM_MOD_MOD_MASK; > > I recommend storing this value in driver data. NOT quite understand, as we did NOT use it in other places except the get_state, just reading the register once should be OK there. > > > + tmp *= duty_cycle; > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period); > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD_MASK, > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > Please align the 2nd line to the opening parenthesis. OK. > > > + > > + /* set polarity */ > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > + val &= ~(PWM_IMX_TPM_CnSC_ELSnB | > PWM_IMX_TPM_CnSC_ELSnA | > > + PWM_IMX_TPM_CnSC_MSnA); > > + val |= PWM_IMX_TPM_CnSC_MSnB; > > + val |= polarity ? PWM_IMX_TPM_CnSC_ELSnA : > PWM_IMX_TPM_CnSC_ELSnB; > > I'd recommend not hard coding here that PWM_POLARITY_NORMAL > evaluates to false. OK, I will add below compare for it: 157 val |= (polarity == PWM_POLARITY_NORMAL) ? 158 PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA; > > In the reference manual I have (Rev. F, 07/2017) MSnA is called MSA only. > Ditto for MSnB -> MSB, ELSnA -> ELSA, ELSnB -> ELSB. (Hmm, but it is not > entirely consistent. So Table 64-4 indeed has the 'n's.) I will remove the 'n's. > > I wonder why MSA and MSB are two bits instead of making this a field of > width 2 with 2b10 meaning PWM mode. But maybe it's just me not > understanding the independent semantic of these two bits? I think making them a field makes more sense, but anyway we just follow the RM. > > Reading the reference manual I'd say in PWM mode the semantic of ELSA > and ELSB is: > > On counter reload set the output to ELSB > On counter match set the output to ELSA > > Noting that in a comment would ease understanding the code here. I added below comment for PWM modes: 137 /* 138 * set polarity (for edge-aligned PWM modes) 139 * 140 * CPWMS MSB:MSA ELSB:ELSA Mode Configuration 141 * 0 10 10 PWM High-true pulse 142 * 0 10 00 PWM Reserved 143 * 0 10 01 PWM Low-true pulse 144 * 0 10 11 PWM Reserved 145 * 146 * High-true pulse: clear output on counter match, set output on 147 * counter reload, set output when counter first enabled or paused. 148 * 149 * Low-true pulse: set output on counter match, clear output on 150 * counter reload, clear output when counter first enabled or paused. 151 */ > > > + /* > > + * polarity settings will enabled/disable output statue > > s/statue/status/ Fixed. > > > + * immediately, so here ONLY save the config and will be > > + * written into register when channel is enabled/disabled. > > s/will be written/write it/ Fixed. > > > + */ > > + tpm->chn_config[pwm->hwpwm] = val; > > +} > > A comment here about how and when the values written in > pwm_imx_tpm_config take effect would be good. OK. > > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + bool enable) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 val, i; > > + > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + if (enable) { > > + /* restore channel config */ > > + writel(tpm->chn_config[pwm->hwpwm], > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > + > > + /* start TPM counter anyway */ > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + } else { > > + /* > > + * When a channel is disabled, its polarity settings will be > > + * saved and its output will be disabled by clearing polarity > > + * setting, when channel is enabled, polarity settings will be > > + * restored and output will be enabled again. > > + */ > > + /* save channel config */ > > + tpm->chn_config[pwm->hwpwm] = readl(tpm->base + > > + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > Doesn't tpm->chn_config[pwm->hwpwm] already contain the right value? > Please align the 2nd line to the opening parenthesis. Ah, yes, no need to read it again. > > > + /* disable channel */ > > + writel(PWM_IMX_TPM_CnSC_CHF, > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > Clearing CHF doens't disable the channel as I read the manual. This write clears CHF as well as writing other bits 0, to disable the output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid confusion. > > > + for (i = 0; i < chip->npwm; i++) > > + if (i != pwm->hwpwm && tpm->chn_status[i]) > > If you set tpm->chn_status[i] = false before this loop, you don't have to care > for i != pwm->hwpwm. If you maintain an "enable count" you don't have to > loop at all. I think we can introduce a enable count for it. > > > + break; > > + if (i == chip->npwm) { > > + /* stop TPM counter since all channels are disabled > */ > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + } > > + } > > + > > + /* update channel statue */ > > + tpm->chn_status[pwm->hwpwm] = enable; } > > + > > +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); > > + u64 tmp; > > + u32 val, rate; > > + > > + mutex_lock(&tpm->lock); > > + > > + /* get period */ > > + rate = clk_get_rate(tpm->clk); > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD); > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + val &= PWM_IMX_TPM_SC_PS_MASK; > > + tmp *= (1 << val) * NSEC_PER_SEC; > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > + > > + /* get duty cycle */ > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > + tmp *= (1 << val) * 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 (val & PWM_IMX_TPM_CnSC_ELSnA) > > + state->polarity = PWM_POLARITY_INVERSED; > > + else > > + state->polarity = PWM_POLARITY_NORMAL; > > + > > + /* get channel status */ > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; > > + > > + mutex_unlock(&tpm->lock); > > +} > > + > > +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 pwm_state curstate; > > + u32 duty_cycle = state->duty_cycle; > > + int ret; > > + > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > + > > + mutex_lock(&tpm->lock); > > What should this lock protect? Does it hurt if the state changes between > pwm_imx_tpm_get_state releasing the lock and pwm_imx_tpm_apply taking > it? The idea is to protect the share resourced by multiple channels, but I think I can make the mutex_lock includes get_state and remove the lock in get_state function. > > > + > > + if (state->period != curstate.period) { > > + /* > > + * TPM counter is shared by multiple channels, so > > + * the prescale and period can NOT be modified when > > + * there are multiple channels used. > > s/the //; s/used/in use/ Fixed. > > > + */ > > + if (tpm->user_count != 1) > > + return -EBUSY; > > Ideally if say period = 37 is requested but currently we have period = > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY. I think here the protection is just for making sure that is there are multiple users, period can NOT be changed, since all channels will be impacted. > > > + ret = pwm_imx_tpm_config_counter(chip, state->period); > > + if (ret) > > + return ret; > > + } > > + > > + if (!state->enabled) > > + duty_cycle = 0; > > A comment above this block that explains why this is done would be great > (but see below). > > > + > > + if (state->duty_cycle != curstate.duty_cycle || > > + state->polarity != curstate.polarity) > > + pwm_imx_tpm_config(chip, pwm, > > + state->period, duty_cycle, state->polarity); > > + > > + if (state->enabled != curstate.enabled) > > + pwm_imx_tpm_enable(chip, pwm, state->enabled); > > This is a bit unintuitive I think. For example I had to think for a while why you > pass duty_cycle to pwm_imx_tpm_config() but check > state->duty_cycle in the if condition. I'd suggest: > > if (state->enabled == false) { > /* > * configure for duty_cycle == 0 here? Wait until this > * setting is active? > */ > if (curstate.enabled) > pwm_imx_tpm_enable(chip, pwm, false); > } else { > ... > > } OK. > > > > + > > + mutex_unlock(&tpm->lock); > > + > > + return 0; > > +} > > + > > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct > > +pwm_device *dev) { > > + 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 > > +*dev) { > > + 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 = { > > + .get_state = pwm_imx_tpm_get_state, > > + .request = pwm_imx_tpm_request, > > + .apply = pwm_imx_tpm_apply, > > + .free = pwm_imx_tpm_free, > > + .owner = THIS_MODULE, > > Can you please group "request" with "free"? The order as defined in struct > pwm_ops would be optimal. OK. > > > +}; > > + > > +static int pwm_imx_tpm_probe(struct platform_device *pdev) { > > + struct imx_tpm_pwm_chip *tpm; > > + struct resource *res; > > + int ret; > > + > > + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL); > > + if (!tpm) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, tpm); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + tpm->base = devm_ioremap_resource(&pdev->dev, res); > > You can use devm_platform_ioremap_resource instead of the two previous > calls. OK, just notice that there is such API newly added. > > > + if (IS_ERR(tpm->base)) { > > + ret = PTR_ERR(tpm->base); > > + if (ret != -EPROBE_DEFER) > > + dev_err(&pdev->dev, "pwm ioremap failed %d\n", > ret); > > + return ret; > > + } > > + > > + 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 clk %d\n", > ret); > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(tpm->clk); > > + if (ret) { > > + dev_err(&pdev->dev, > > + "failed to prepare or enable clk %d\n", ret); > > + return ret; > > + } > > + > > + tpm->chip.dev = &pdev->dev; > > + tpm->chip.ops = &imx_tpm_pwm_ops; > > + tpm->chip.base = -1; > > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; > > This is wrong, as some only have 2 channels? I saw we can get channel number from register, will read register to determine the channel number, but for the channel config and status saved in struct, I will still use the MAX channel number to define the array. > > > + 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); > > + > > + clk_disable_unprepare(tpm->clk); > > + > > + return pwmchip_remove(&tpm->chip); > > Wrong order. Before pwmchip_remove returns the PWM must stay > functional. Will make pwmchip_remove before clk disable. > > > +} > > + > > +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) { > > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev); > > + > > + clk_disable_unprepare(tpm->clk); > > You must not disable the clock if the PWM is in use. I will add the enable counter check for it. > > > + return 0; > > +} > > The time I want to/can spend on community review is over now for this week. > I didn't look at all details yet but I think it is still worth to send this mail out to > not make you bored :-) Also I think further thoughts by me will be eased if > you address my comments here first. Thanks for your time/patience, I will generate V5 after all your comments addressed. 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%7C23 > 698f5992914cbb461208d6a9299e8e%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636882393558954567&sdata=5tuFBU7w%2FAULwJGbn > BlOpxmd4JmQQO8wT2qsMgW5yXs%3D&reserved=0 |
On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote: > Hi,Uwe > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > As this interrupts the output, please only do it if necessary. > > OK, will do it ONLY when it is enabled previously. I think you only need to do that when the value actually changes. > > > + /* set duty counter */ > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD_MASK; > > > > I recommend storing this value in driver data. > > NOT quite understand, as we did NOT use it in other places except the get_state, > just reading the register once should be OK there. I had the impression it is used more than once. Will look again in the next iteration. But also note that shadowing the value might already be beneficial for a single call site as driver data might occupy more RAM than necessary anyhow and reading from RAM is faster than from the hardware's register. Probably this is not a fast path, so not worth the optimisation?! > > I wonder why MSA and MSB are two bits instead of making this a field of > > width 2 with 2b10 meaning PWM mode. But maybe it's just me not > > understanding the independent semantic of these two bits? > > I think making them a field makes more sense, but anyway we just follow > the RM. If it makes the driver easier to understand (which I think it does) feel free to derivate from the RM. Just add a comment to the definition, then it's fine. > > Reading the reference manual I'd say in PWM mode the semantic of ELSA > > and ELSB is: > > > > On counter reload set the output to ELSB > > On counter match set the output to ELSA > > > > Noting that in a comment would ease understanding the code here. > > I added below comment for PWM modes: > > 137 /* > 138 * set polarity (for edge-aligned PWM modes) > 139 * > 140 * CPWMS MSB:MSA ELSB:ELSA Mode Configuration > 141 * 0 10 10 PWM High-true pulse > 142 * 0 10 00 PWM Reserved > 143 * 0 10 01 PWM Low-true pulse > 144 * 0 10 11 PWM Reserved > 145 * > 146 * High-true pulse: clear output on counter match, set output on > 147 * counter reload, set output when counter first enabled or paused. > 148 * > 149 * Low-true pulse: set output on counter match, clear output on > 150 * counter reload, clear output when counter first enabled or paused. > 151 */ I stumbled over "high-true" and "low-true" in the RM, too. In my bubble this is an uncommon wording. I'd write instead: /* * set polarity * * ELSB:ELSA = 2b10 yields normal polarity behaviour, ELSB:ELSA * = 2b01 yields inversed polarity. The other values are * reserved. */ And don't write about CPWM, MSA and MSB which are always used with fixed values anyhow in the driver. > > > + /* disable channel */ > > > + writel(PWM_IMX_TPM_CnSC_CHF, > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > Clearing CHF doens't disable the channel as I read the manual. > > This write clears CHF as well as writing other bits 0, to disable the output. Maybe I > can explicitly clear MSA/MSB/ELSA/ELSB to avoid confusion. Ah, I misinterpreted the value written. > > > +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 pwm_state curstate; > > > + u32 duty_cycle = state->duty_cycle; > > > + int ret; > > > + > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > > + > > > + mutex_lock(&tpm->lock); > > > > What should this lock protect? Does it hurt if the state changes between > > pwm_imx_tpm_get_state releasing the lock and pwm_imx_tpm_apply taking > > it? > > The idea is to protect the share resourced by multiple channels, but I think I can make the mutex_lock > includes get_state and remove the lock in get_state function. You might need it in .get_state to return a consistent state to the caller. In this case just introduce an unlocked variant of .get_state to share code between the two functions. And BTW the question was honest. I'm not entirely sure that you need to hold the lock. > > > + */ > > > + if (tpm->user_count != 1) > > > + return -EBUSY; > > > > Ideally if say period = 37 is requested but currently we have period = > > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY. > > I think here the protection is just for making sure that is there are multiple > users, period can NOT be changed, since all channels will be impacted. I think you misunderstood what I intended to say here. Consider that in the case that there is only a single PWM in use configuring for a period of 37 ns actually yields 36 ns because the hardware cannot provide 37 ns and 36 ns is the best match. Then if a second user comes and requests 37 ns, the result here is, that the second user gets the -EBUSY. This is ridiculous however because the request is denied even though the period is already configured for the length that would be configured if the second user were the only one. > > > + tpm->chip.dev = &pdev->dev; > > > + tpm->chip.ops = &imx_tpm_pwm_ops; > > > + tpm->chip.base = -1; > > > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; > > > > This is wrong, as some only have 2 channels? > > I saw we can get channel number from register, will read register to determine > the channel number, but for the channel config and status saved in struct, I will > still use the MAX channel number to define the array. Yeah, that is sensible. Best regards Uwe
Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: 2019年3月18日 16:08 > 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; otavio@ossystems.com.br; > stefan@agner.ch; 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 V4 2/5] pwm: Add i.MX TPM PWM driver support > > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote: > > Hi,Uwe > > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > > > As this interrupts the output, please only do it if necessary. > > > > OK, will do it ONLY when it is enabled previously. > > I think you only need to do that when the value actually changes. OK, I will save the period/div count and ONLY do it when the value actually changes. > > > > > + /* set duty counter */ > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > > > > +PWM_IMX_TPM_MOD_MOD_MASK; > > > > > > I recommend storing this value in driver data. > > > > NOT quite understand, as we did NOT use it in other places except the > > get_state, just reading the register once should be OK there. > > I had the impression it is used more than once. Will look again in the next > iteration. But also note that shadowing the value might already be beneficial > for a single call site as driver data might occupy more RAM than necessary > anyhow and reading from RAM is faster than from the hardware's register. > Probably this is not a fast path, so not worth the optimisation?! OK, will save it in driver data and avoid accessing register again. > > > > I wonder why MSA and MSB are two bits instead of making this a field > > > of width 2 with 2b10 meaning PWM mode. But maybe it's just me not > > > understanding the independent semantic of these two bits? > > > > I think making them a field makes more sense, but anyway we just > > follow the RM. > > If it makes the driver easier to understand (which I think it does) feel free to > derivate from the RM. Just add a comment to the definition, then it's fine. OK, I will change the register definition and a comment for it. > > > > Reading the reference manual I'd say in PWM mode the semantic of > > > ELSA and ELSB is: > > > > > > On counter reload set the output to ELSB > > > On counter match set the output to ELSA > > > > > > Noting that in a comment would ease understanding the code here. > > > > I added below comment for PWM modes: > > > > 137 /* > > 138 * set polarity (for edge-aligned PWM modes) > > 139 * > > 140 * CPWMS MSB:MSA ELSB:ELSA Mode Configuration > > 141 * 0 10 10 PWM High-true pulse > > 142 * 0 10 00 PWM Reserved > > 143 * 0 10 01 PWM Low-true pulse > > 144 * 0 10 11 PWM Reserved > > 145 * > > 146 * High-true pulse: clear output on counter match, set output on > > 147 * counter reload, set output when counter first enabled or paused. > > 148 * > > 149 * Low-true pulse: set output on counter match, clear output on > > 150 * counter reload, clear output when counter first enabled or > paused. > > 151 */ > > I stumbled over "high-true" and "low-true" in the RM, too. In my bubble this > is an uncommon wording. I'd write instead: > > /* > * set polarity > * > * ELSB:ELSA = 2b10 yields normal polarity behaviour, ELSB:ELSA > * = 2b01 yields inversed polarity. The other values are > * reserved. > */ > > And don't write about CPWM, MSA and MSB which are always used with > fixed values anyhow in the driver. > OK. > > > > + /* disable channel */ > > > > + writel(PWM_IMX_TPM_CnSC_CHF, > > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > > > Clearing CHF doens't disable the channel as I read the manual. > > > > This write clears CHF as well as writing other bits 0, to disable the > > output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid > confusion. > > Ah, I misinterpreted the value written. > > > > > +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 pwm_state curstate; > > > > + u32 duty_cycle = state->duty_cycle; > > > > + int ret; > > > > + > > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > > > + > > > > + mutex_lock(&tpm->lock); > > > > > > What should this lock protect? Does it hurt if the state changes > > > between pwm_imx_tpm_get_state releasing the lock and > > > pwm_imx_tpm_apply taking it? > > > > The idea is to protect the share resourced by multiple channels, but I > > think I can make the mutex_lock includes get_state and remove the lock in > get_state function. > A > You might need it in .get_state to return a consistent state to the caller. In > this case just introduce an unlocked variant of .get_state to share code > between the two functions. > > And BTW the question was honest. I'm not entirely sure that you need to > hold the lock. Agreed, if the different channel configuration ONLY access its own register, NOT any shared registers, then I think this lock is unnecessary. > > > > > + */ > > > > + if (tpm->user_count != 1) > > > > + return -EBUSY; > > > > > > Ideally if say period = 37 is requested but currently we have period > > > = > > > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY. > > > > I think here the protection is just for making sure that is there are > > multiple users, period can NOT be changed, since all channels will be > impacted. > > I think you misunderstood what I intended to say here. > > Consider that in the case that there is only a single PWM in use configuring > for a period of 37 ns actually yields 36 ns because the hardware cannot > provide 37 ns and 36 ns is the best match. > > Then if a second user comes and requests 37 ns, the result here is, that the > second user gets the -EBUSY. This is ridiculous however because the request > is denied even though the period is already configured for the length that > would be configured if the second user were the only one. Ah, if I understand correctly, I think I can change it to, if second user try to set a period different from first user, then return -EBUSY, otherwise, return success. > > > > > + tpm->chip.dev = &pdev->dev; > > > > + tpm->chip.ops = &imx_tpm_pwm_ops; > > > > + tpm->chip.base = -1; > > > > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; > > > > > > This is wrong, as some only have 2 channels? > > > > I saw we can get channel number from register, will read register to > > determine the channel number, but for the channel config and status > > saved in struct, I will still use the MAX channel number to define the array. > > Yeah, that is sensible. I will resend V5 patch set, since there are some mis-understanding for previous comments. 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%7C31 > d10d3f4a7c46eea9d708d6ab78d965%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636884932859808816&sdata=LD2D%2BwEhNKFllK2FaI > Wvjttmre0YPV%2BXwv7sdvkZSpo%3D&reserved=0 |
Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Anson Huang > Sent: 2019年3月18日 18:08 > To: 'Uwe Kleine-König' <u.kleine-koenig@pengutronix.de> > 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; otavio@ossystems.com.br; > stefan@agner.ch; 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 V4 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@pengutronix.de] > > Sent: 2019年3月18日 16:08 > > 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; > > otavio@ossystems.com.br; stefan@agner.ch; 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 V4 2/5] pwm: Add i.MX TPM PWM driver support > > > > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote: > > > Hi,Uwe > > > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > > > > > As this interrupts the output, please only do it if necessary. > > > > > > OK, will do it ONLY when it is enabled previously. > > > > I think you only need to do that when the value actually changes. > > OK, I will save the period/div count and ONLY do it when the value actually > changes. After further think, I added below tpm->period to save the current period settings, ONLY when the new period to be set is different from the current period, then the pwm_imx_tpm_config_counter() is called, so I think no need to care about the value changes, the value is always changed when pwm_imx_tpm_config_counter() is called. if (tpm->user_count != 1 && state->period != tpm->period) return -EBUSY; ret = pwm_imx_tpm_config_counter(chip, state->period); if (ret) return ret; > > > > > > > > + /* set duty counter */ > > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > > > > > +PWM_IMX_TPM_MOD_MOD_MASK; > > > > > > > > I recommend storing this value in driver data. > > > > > > NOT quite understand, as we did NOT use it in other places except > > > the get_state, just reading the register once should be OK there. > > > > I had the impression it is used more than once. Will look again in the > > next iteration. But also note that shadowing the value might already > > be beneficial for a single call site as driver data might occupy more > > RAM than necessary anyhow and reading from RAM is faster than from the > hardware's register. > > Probably this is not a fast path, so not worth the optimisation?! > > OK, will save it in driver data and avoid accessing register again. > > > > > > > I wonder why MSA and MSB are two bits instead of making this a > > > > field of width 2 with 2b10 meaning PWM mode. But maybe it's just > > > > me not understanding the independent semantic of these two bits? > > > > > > I think making them a field makes more sense, but anyway we just > > > follow the RM. > > > > If it makes the driver easier to understand (which I think it does) > > feel free to derivate from the RM. Just add a comment to the definition, > then it's fine. > > OK, I will change the register definition and a comment for it. > > > > > > > Reading the reference manual I'd say in PWM mode the semantic of > > > > ELSA and ELSB is: > > > > > > > > On counter reload set the output to ELSB > > > > On counter match set the output to ELSA > > > > > > > > Noting that in a comment would ease understanding the code here. > > > > > > I added below comment for PWM modes: > > > > > > 137 /* > > > 138 * set polarity (for edge-aligned PWM modes) > > > 139 * > > > 140 * CPWMS MSB:MSA ELSB:ELSA Mode Configuration > > > 141 * 0 10 10 PWM High-true pulse > > > 142 * 0 10 00 PWM Reserved > > > 143 * 0 10 01 PWM Low-true pulse > > > 144 * 0 10 11 PWM Reserved > > > 145 * > > > 146 * High-true pulse: clear output on counter match, set output on > > > 147 * counter reload, set output when counter first enabled or > paused. > > > 148 * > > > 149 * Low-true pulse: set output on counter match, clear output on > > > 150 * counter reload, clear output when counter first enabled or > > paused. > > > 151 */ > > > > I stumbled over "high-true" and "low-true" in the RM, too. In my > > bubble this is an uncommon wording. I'd write instead: > > > > /* > > * set polarity > > * > > * ELSB:ELSA = 2b10 yields normal polarity behaviour, ELSB:ELSA > > * = 2b01 yields inversed polarity. The other values are > > * reserved. > > */ > > > > And don't write about CPWM, MSA and MSB which are always used with > > fixed values anyhow in the driver. > > > > OK. > > > > > > + /* disable channel */ > > > > > + writel(PWM_IMX_TPM_CnSC_CHF, > > > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm- > >hwpwm)); > > > > > > > > Clearing CHF doens't disable the channel as I read the manual. > > > > > > This write clears CHF as well as writing other bits 0, to disable > > > the output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid > > confusion. > > > > Ah, I misinterpreted the value written. > > > > > > > +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 pwm_state curstate; > > > > > + u32 duty_cycle = state->duty_cycle; > > > > > + int ret; > > > > > + > > > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > > > > + > > > > > + mutex_lock(&tpm->lock); > > > > > > > > What should this lock protect? Does it hurt if the state changes > > > > between pwm_imx_tpm_get_state releasing the lock and > > > > pwm_imx_tpm_apply taking it? > > > > > > The idea is to protect the share resourced by multiple channels, but > > > I think I can make the mutex_lock includes get_state and remove the > > > lock in > > get_state function. > > A > > You might need it in .get_state to return a consistent state to the > > caller. In this case just introduce an unlocked variant of .get_state > > to share code between the two functions. > > > > And BTW the question was honest. I'm not entirely sure that you need > > to hold the lock. > > Agreed, if the different channel configuration ONLY access its own register, > NOT any shared registers, then I think this lock is unnecessary. Since all the functions in .apply function will need to access registers and these registers are shared by different channels, so I think the lock is necessary. Anson. > > > > > > > > + */ > > > > > + if (tpm->user_count != 1) > > > > > + return -EBUSY; > > > > > > > > Ideally if say period = 37 is requested but currently we have > > > > period = > > > > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY. > > > > > > I think here the protection is just for making sure that is there > > > are multiple users, period can NOT be changed, since all channels > > > will be > > impacted. > > > > I think you misunderstood what I intended to say here. > > > > Consider that in the case that there is only a single PWM in use > > configuring for a period of 37 ns actually yields 36 ns because the > > hardware cannot provide 37 ns and 36 ns is the best match. > > > > Then if a second user comes and requests 37 ns, the result here is, > > that the second user gets the -EBUSY. This is ridiculous however > > because the request is denied even though the period is already > > configured for the length that would be configured if the second user were > the only one. > > Ah, if I understand correctly, I think I can change it to, if second user try to set > a period different from first user, then return -EBUSY, otherwise, return > success. > > > > > > > > + tpm->chip.dev = &pdev->dev; > > > > > + tpm->chip.ops = &imx_tpm_pwm_ops; > > > > > + tpm->chip.base = -1; > > > > > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; > > > > > > > > This is wrong, as some only have 2 channels? > > > > > > I saw we can get channel number from register, will read register to > > > determine the channel number, but for the channel config and status > > > saved in struct, I will still use the MAX channel number to define the array. > > > > Yeah, that is sensible. > > I will resend V5 patch set, since there are some mis-understanding for > previous comments. > > 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%7C31 > > > d10d3f4a7c46eea9d708d6ab78d965%7C686ea1d3bc2b4c6fa92cd99c5c30163 > > > 5%7C0%7C0%7C636884932859808816&sdata=LD2D%2BwEhNKFllK2FaI > > Wvjttmre0YPV%2BXwv7sdvkZSpo%3D&reserved=0 |
Hello, as you got some feedback for v5 already I assume you will post a v6. I'll skip reviewing v5 then. Best regards Uwe
Hi, Uwe Yes, I will post a V6, but before sending V6, can you help confirm if you agree my 2 answers for your last comments, such as the counter disable for changing prescale and the mutex lock in .apply function? Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: 2019年3月18日 22:11 > 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; otavio@ossystems.com.br; > stefan@agner.ch; 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 V4 2/5] pwm: Add i.MX TPM PWM driver support > > Hello, > > as you got some feedback for v5 already I assume you will post a v6. > I'll skip reviewing v5 then. > > 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%7C79 > 7514afffcb4f799bf408d6abab9188%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636885150706417201&sdata=kVut1X8JtjGdGOM2czyRRc > 5di77I2Hu8dQttf8DJn1A%3D&reserved=0 |
Hello, On Mon, Mar 18, 2019 at 02:04:00PM +0000, Anson Huang wrote: > > > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote: > > > > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > > > > > > > As this interrupts the output, please only do it if necessary. > > > > > > > > OK, will do it ONLY when it is enabled previously. > > > > > > I think you only need to do that when the value actually changes. > > > > OK, I will save the period/div count and ONLY do it when the value actually > > changes. > > After further think, I added below tpm->period to save the current period settings, > ONLY when the new period to be set is different from the current period, then the > pwm_imx_tpm_config_counter() is called, so I think no need to care about the value > changes, the value is always changed when pwm_imx_tpm_config_counter() is called. > > if (tpm->user_count != 1 && state->period != tpm->period) > return -EBUSY; > ret = pwm_imx_tpm_config_counter(chip, state->period); > if (ret) > return ret; This is still not the optimal thing to do. What you really want is: p = round_period_according_to_hw_caps(state->period); if (p != actually_configured_period && tpm->user_count != 1) return -EBUSY; > > > > > > + /* set duty counter */ > > > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > > > > > > +PWM_IMX_TPM_MOD_MOD_MASK; > > > > > > > > > > I recommend storing this value in driver data. > > > > > > > > NOT quite understand, as we did NOT use it in other places except > > > > the get_state, just reading the register once should be OK there. > > > > > > I had the impression it is used more than once. Will look again in the > > > next iteration. But also note that shadowing the value might already > > > be beneficial for a single call site as driver data might occupy more > > > RAM than necessary anyhow and reading from RAM is faster than from the > > > hardware's register. > > > Probably this is not a fast path, so not worth the optimisation?! > > > > OK, will save it in driver data and avoid accessing register again. please apply some thought before following my advices. If this is not a fast path and it hurts readability, don't shadow the register in driver data. > > > > > > + /* disable channel */ > > > > > > + writel(PWM_IMX_TPM_CnSC_CHF, > > > > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > > > > > > > Clearing CHF doens't disable the channel as I read the manual. > > > > > > > > This write clears CHF as well as writing other bits 0, to disable > > > > the output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid > > > > confusion. > > > > > > Ah, I misinterpreted the value written. > > > > > > > > > +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 pwm_state curstate; > > > > > > + u32 duty_cycle = state->duty_cycle; > > > > > > + int ret; > > > > > > + > > > > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > > > > > + > > > > > > + mutex_lock(&tpm->lock); > > > > > > > > > > What should this lock protect? Does it hurt if the state changes > > > > > between pwm_imx_tpm_get_state releasing the lock and > > > > > pwm_imx_tpm_apply taking it? > > > > > > > > The idea is to protect the share resourced by multiple channels, but > > > > I think I can make the mutex_lock includes get_state and remove the > > > > lock in get_state function. > > > > > > You might need it in .get_state to return a consistent state to the > > > caller. In this case just introduce an unlocked variant of .get_state > > > to share code between the two functions. > > > > > > And BTW the question was honest. I'm not entirely sure that you need > > > to hold the lock. > > > > Agreed, if the different channel configuration ONLY access its own register, > > NOT any shared registers, then I think this lock is unnecessary. > > Since all the functions in .apply function will need to access registers and these > registers are shared by different channels, so I think the lock is necessary. I think so, too, but didn't thought it to the end. I can invest some time here in the next review round. BTW, one thing that I think is missing is, that .apply must only return when the newly configured setting is already active. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a8f47df..6117fe6 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -201,6 +201,18 @@ 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 || 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 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..f108f75 --- /dev/null +++ b/drivers/pwm/pwm-imx-tpm.c @@ -0,0 +1,396 @@ +// 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/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_GLOBAL 0x8 +#define PWM_IMX_TPM_SC 0x10 +#define PWM_IMX_TPM_CNT 0x14 +#define PWM_IMX_TPM_MOD 0x18 +#define PWM_IMX_TPM_C0SC 0x20 +#define PWM_IMX_TPM_C0V 0x24 + +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3) +#define PWM_IMX_TPM_SC_CPWMS BIT(5) + +#define PWM_IMX_TPM_CnSC_CHF BIT(7) +#define PWM_IMX_TPM_CnSC_MSnB BIT(5) +#define PWM_IMX_TPM_CnSC_MSnA BIT(4) +#define PWM_IMX_TPM_CnSC_ELSnB BIT(3) +#define PWM_IMX_TPM_CnSC_ELSnA BIT(2) + +#define PWM_IMX_TPM_SC_PS_MASK 0x7 +#define PWM_IMX_TPM_MOD_MOD_MASK 0xffff + +#define PWM_IMX_TPM_MAX_COUNT 0xffff + +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6 + +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + n * 0x8) +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + n * 0x8) + +struct imx_tpm_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + void __iomem *base; + struct mutex lock; + u32 user_count; + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM]; + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM]; +}; + +#define to_imx_tpm_pwm_chip(_chip) \ + container_of(_chip, struct imx_tpm_pwm_chip, chip) + +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 period) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + u32 period_cnt, val, div, saved_cmod; + u64 tmp; + + tmp = clk_get_rate(tpm->clk); + tmp *= period; + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); + if (val < PWM_IMX_TPM_MAX_COUNT) + div = 0; + else + div = ilog2(roundup_pow_of_two(val / + (PWM_IMX_TPM_MAX_COUNT + 1))); + if (div > PWM_IMX_TPM_SC_PS_MASK) { + dev_err(chip->dev, + "failed to find valid prescale value!\n"); + return -EINVAL; + } + + /* make sure counter is disabled for programming prescale */ + val = readl(tpm->base + PWM_IMX_TPM_SC); + saved_cmod = val & PWM_IMX_TPM_SC_CMOD; + val &= ~PWM_IMX_TPM_SC_CMOD; + writel(val, tpm->base + PWM_IMX_TPM_SC); + + /* set TPM counter prescale */ + val = readl(tpm->base + PWM_IMX_TPM_SC); + val &= ~PWM_IMX_TPM_SC_PS_MASK; + val |= div; + writel(val, tpm->base + PWM_IMX_TPM_SC); + + /* + * set period counter: according to RM, the MOD register is + * updated immediately when CMOD[1:0] = 2b'00 (counter disabled). + */ + do_div(tmp, NSEC_PER_SEC); + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div) + & PWM_IMX_TPM_MOD_MOD_MASK; + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD); + + /* restore the clock mode */ + val = readl(tpm->base + PWM_IMX_TPM_SC); + val |= saved_cmod; + writel(val, tpm->base + PWM_IMX_TPM_SC); + + return 0; +} + +static void pwm_imx_tpm_config(struct pwm_chip *chip, + struct pwm_device *pwm, + u32 period, + u32 duty_cycle, + enum pwm_polarity polarity) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + u32 duty_cnt, val; + u64 tmp; + + /* set duty counter */ + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD_MASK; + tmp *= duty_cycle; + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period); + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD_MASK, + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); + + /* set polarity */ + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + val &= ~(PWM_IMX_TPM_CnSC_ELSnB | PWM_IMX_TPM_CnSC_ELSnA | + PWM_IMX_TPM_CnSC_MSnA); + val |= PWM_IMX_TPM_CnSC_MSnB; + val |= polarity ? PWM_IMX_TPM_CnSC_ELSnA : PWM_IMX_TPM_CnSC_ELSnB; + /* + * polarity settings will enabled/disable output statue + * immediately, so here ONLY save the config and will be + * written into register when channel is enabled/disabled. + */ + tpm->chn_config[pwm->hwpwm] = val; +} + +static void pwm_imx_tpm_enable(struct pwm_chip *chip, + struct pwm_device *pwm, + bool enable) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + u32 val, i; + + val = readl(tpm->base + PWM_IMX_TPM_SC); + if (enable) { + /* restore channel config */ + writel(tpm->chn_config[pwm->hwpwm], + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + + /* start TPM counter anyway */ + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK; + writel(val, tpm->base + PWM_IMX_TPM_SC); + } else { + /* + * When a channel is disabled, its polarity settings will be + * saved and its output will be disabled by clearing polarity + * setting, when channel is enabled, polarity settings will be + * restored and output will be enabled again. + */ + /* save channel config */ + tpm->chn_config[pwm->hwpwm] = readl(tpm->base + + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + /* disable channel */ + writel(PWM_IMX_TPM_CnSC_CHF, + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + + for (i = 0; i < chip->npwm; i++) + if (i != pwm->hwpwm && tpm->chn_status[i]) + break; + if (i == chip->npwm) { + /* stop TPM counter since all channels are disabled */ + val &= ~PWM_IMX_TPM_SC_CMOD; + writel(val, tpm->base + PWM_IMX_TPM_SC); + } + } + + /* update channel statue */ + tpm->chn_status[pwm->hwpwm] = enable; +} + +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); + u64 tmp; + u32 val, rate; + + mutex_lock(&tpm->lock); + + /* get period */ + rate = clk_get_rate(tpm->clk); + tmp = readl(tpm->base + PWM_IMX_TPM_MOD); + val = readl(tpm->base + PWM_IMX_TPM_SC); + val &= PWM_IMX_TPM_SC_PS_MASK; + tmp *= (1 << val) * NSEC_PER_SEC; + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); + + /* get duty cycle */ + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); + tmp *= (1 << val) * 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 (val & PWM_IMX_TPM_CnSC_ELSnA) + state->polarity = PWM_POLARITY_INVERSED; + else + state->polarity = PWM_POLARITY_NORMAL; + + /* get channel status */ + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; + + mutex_unlock(&tpm->lock); +} + +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 pwm_state curstate; + u32 duty_cycle = state->duty_cycle; + int ret; + + pwm_imx_tpm_get_state(chip, pwm, &curstate); + + mutex_lock(&tpm->lock); + + if (state->period != curstate.period) { + /* + * TPM counter is shared by multiple channels, so + * the prescale and period can NOT be modified when + * there are multiple channels used. + */ + if (tpm->user_count != 1) + return -EBUSY; + ret = pwm_imx_tpm_config_counter(chip, state->period); + if (ret) + return ret; + } + + if (!state->enabled) + duty_cycle = 0; + + if (state->duty_cycle != curstate.duty_cycle || + state->polarity != curstate.polarity) + pwm_imx_tpm_config(chip, pwm, + state->period, duty_cycle, state->polarity); + + if (state->enabled != curstate.enabled) + pwm_imx_tpm_enable(chip, pwm, state->enabled); + + mutex_unlock(&tpm->lock); + + return 0; +} + +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *dev) +{ + 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 *dev) +{ + 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 = { + .get_state = pwm_imx_tpm_get_state, + .request = pwm_imx_tpm_request, + .apply = pwm_imx_tpm_apply, + .free = pwm_imx_tpm_free, + .owner = THIS_MODULE, +}; + +static int pwm_imx_tpm_probe(struct platform_device *pdev) +{ + struct imx_tpm_pwm_chip *tpm; + struct resource *res; + int ret; + + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL); + if (!tpm) + return -ENOMEM; + + platform_set_drvdata(pdev, tpm); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + tpm->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(tpm->base)) { + ret = PTR_ERR(tpm->base); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "pwm ioremap failed %d\n", ret); + return ret; + } + + 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 clk %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(tpm->clk); + if (ret) { + dev_err(&pdev->dev, + "failed to prepare or enable clk %d\n", ret); + return ret; + } + + tpm->chip.dev = &pdev->dev; + tpm->chip.ops = &imx_tpm_pwm_ops; + tpm->chip.base = -1; + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; + + 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); + + clk_disable_unprepare(tpm->clk); + + return pwmchip_remove(&tpm->chip); +} + +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) +{ + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev); + + 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 = clk_prepare_enable(tpm->clk); + + if (ret) + dev_err(dev, + "failed to prepare or enable clk %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-pwm", }, + { /* 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");
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 V3: - use "PWM_IMX_" as macro definition prefix and "pwm_imx_" as function prefix; - improve the limitation txt; - return error for configuring period/prescale fail; - disable clock when driver probe failed and remove; - improve module build dependency; - introduce user_count to determine whether configuing period is allowed; - some logic improvement for setting duty/status etc.; --- drivers/pwm/Kconfig | 12 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-imx-tpm.c | 396 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 409 insertions(+) create mode 100644 drivers/pwm/pwm-imx-tpm.c