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&data=02%7C01%7Canson.huang%40nxp.com%7C87 > 2ade014b314f5edf6d08d6d44ed41e%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636929832363393461&sdata=QTSwOn0288HYlBCus%2 > FSlw9BDljPqMJ1WJD2KbEiJhuM%3D&reserved=0 |
