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

From: Anson Huang
Date: Thu Mar 14 2019 - 10:42:17 EST

Next message: Thomas Renninger: "[PATCH] [RESEND] Do not modify perf bias performance setting by default at boot"
Previous message: William Breathitt Gray: "Re: [PATCH v10 10/10] thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro"
In reply to: Anson Huang: "RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support"
Next in thread: Anson Huang: "[PATCH V2 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Uwe

Best Regards!
Anson Huang

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

Will fix it in V4.

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

Will fix it in V4.

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

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

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

Will use below in V4:
#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6

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

Fix it in V4.

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

I will use below for V4:

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

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

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

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

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

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

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

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

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

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

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

Thanks,
Anson.

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

Next message: Thomas Renninger: "[PATCH] [RESEND] Do not modify perf bias performance setting by default at boot"
Previous message: William Breathitt Gray: "Re: [PATCH v10 10/10] thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro"
In reply to: Anson Huang: "RE: [PATCH V2 2/5] pwm: Add i.MX TPM PWM driver support"
Next in thread: Anson Huang: "[PATCH V2 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]