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

From: Anson Huang
Date: Thu May 09 2019 - 09:13:18 EST

Hi, Uwe

> -----Original Message-----
> From: Uwe Kleine-KÃnig [mailto:u.kleine-koenig@xxxxxxxxxxxxxx]
> Sent: Thursday, May 9, 2019 3:20 PM
> 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>; linux-pwm@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH V11 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Wed, Apr 10, 2019 at 01:47:40AM +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 V10:
> > - remove channel private data which is ONLY for storing polarity, just
> read it from HW register;
> > - improve pwm_imx_tpm_round_state() and
> pwm_imx_tpm_apply_hw() parameters sequence;
> > - improve comments for polarity setting;
> > - refuse polarity change if PWM is active.
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 442
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 454 insertions(+)
> > create mode 100644 drivers/pwm/pwm-imx-tpm.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > c054bd1..1311b540 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..9349f4f
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,442 @@
> > +// 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.
> > + * - 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.
> > + */
> > +
> > +#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
> > + * semantic of the two bits isn't orthogonal though, so they are
> > +treated
> > + * together as a 2-bit field here.
> > + */
> > +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2)
> > +#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;
> > +};
> > +
> > +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); }
> > +
>
> Maybe add a comment here describing the purpose of this function.
> Something like:
>
> /*
> * This function determines for a given pwm_state *state that a consumer
> * might request the pwm_state *realstate that eventually is implemented
> * by the hardware and the necessary register values (in *p) to achive
> * this.
> */
>
> I didn't revalidate all the maths in this driver but assume they are still right
> from the previous rounds. If you add the comment I suggested above, feel
> free to also add
>
> Reviewed-by: Uwe Kleine-KÃnig <u.kleine-koenig@xxxxxxxxxxxxxx>
>

OK, I will add this comment in V12, thanks!

> > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip,
> > + struct imx_tpm_pwm_param *p,
> > + struct pwm_state *real_state,
> > + struct pwm_state *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;
>
> It's a bit sad that my ideas for the core concerning a round_rate callback
> don't go down particularly well on Thierry's side.
>
> With the way I suggested we'd continue with prescale = 7 in this case.
>
> As of now there is no rule which kind of deviation to accept and which not. :-
> | (Nothing this patch can change of course.)
>

Thanks,

Anson.

> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-KÃnig |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C87
> 2ade014b314f5edf6d08d6d44ed41e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636929832363393461&amp;sdata=QTSwOn0288HYlBCus%2
> FSlw9BDljPqMJ1WJD2KbEiJhuM%3D&amp;reserved=0 |