* [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support @ 2019-03-22 1:47 Anson Huang 2019-03-22 1:48 ` [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding Anson Huang ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Anson Huang @ 2019-03-22 1:47 UTC (permalink / raw) To: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam, linux, stefan, otavio, Leonard Crestez, Robin Gong, jan.tuerk, linux-pwm, devicetree, linux-arm-kernel, linux-kernel, u.kleine-koenig Cc: dl-linux-imx i.MX7ULP EVK board has MIPI-DSI display, its backlight is supplied by TPM PWM module, this patch set enables i.MX7ULP TPM PWM driver support and also add backlight support for MIPI-DSI display. Changes since V8: - ONLY change the pwm driver patch. Anson Huang (5): dt-bindings: pwm: Add i.MX TPM PWM binding pwm: Add i.MX TPM PWM driver support ARM: imx_v6_v7_defconfig: Add TPM PWM support by default ARM: dts: imx7ulp: Add pwm0 support ARM: dts: imx7ulp-evk: Add backlight support .../devicetree/bindings/pwm/imx-tpm-pwm.txt | 22 + arch/arm/boot/dts/imx7ulp-evk.dts | 21 + arch/arm/boot/dts/imx7ulp.dtsi | 10 + arch/arm/configs/imx_v6_v7_defconfig | 1 + drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-imx-tpm.c | 518 +++++++++++++++++++++ 7 files changed, 584 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt create mode 100644 drivers/pwm/pwm-imx-tpm.c -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding 2019-03-22 1:47 [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support Anson Huang @ 2019-03-22 1:48 ` Anson Huang 2019-03-25 20:41 ` Rob Herring 2019-03-22 1:48 ` [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support Anson Huang ` (3 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Anson Huang @ 2019-03-22 1:48 UTC (permalink / raw) To: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam, linux, stefan, otavio, Leonard Crestez, Robin Gong, jan.tuerk, linux-pwm, devicetree, linux-arm-kernel, linux-kernel, u.kleine-koenig Cc: dl-linux-imx Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM binding. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- No changes. --- .../devicetree/bindings/pwm/imx-tpm-pwm.txt | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt diff --git a/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt new file mode 100644 index 0000000..94f1ad5 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt @@ -0,0 +1,22 @@ +Freescale i.MX TPM PWM controller + +Required properties: +- compatible : Should be "fsl,imx-tpm". +- reg: Physical base address and length of the controller's registers. +- #pwm-cells: Should be 3. See pwm.txt in this directory for a description of the cells format. +- clocks : The clock provided by the SoC to drive the PWM. +- interrupts: The interrupt for the PWM controller. + +Note: The TPM counter and period counter are shared between multiple channels, so all channels +should use same period setting. + +Example: + +pwm0: tpm@40250000 { + compatible = "fsl,imx-tpm"; + reg = <0x40250000 0x1000>; + assigned-clocks = <&clks IMX7ULP_CLK_LPTPM4>; + assigned-clock-parents = <&clks IMX7ULP_CLK_SOSC_BUS_CLK>; + clocks = <&clks IMX7ULP_CLK_LPTPM4>; + #pwm-cells = <3>; +}; -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding 2019-03-22 1:48 ` [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding Anson Huang @ 2019-03-25 20:41 ` Rob Herring 2019-03-25 20:59 ` Uwe Kleine-König 2019-03-26 0:56 ` Anson Huang 0 siblings, 2 replies; 14+ messages in thread From: Rob Herring @ 2019-03-25 20:41 UTC (permalink / raw) To: Anson Huang Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel, thierry.reding, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez, festevam, linux-arm-kernel On Fri, Mar 22, 2019 at 01:48:05AM +0000, Anson Huang wrote: > Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM binding. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > No changes. v9? I don't recall seeing any previous versions. > --- > .../devicetree/bindings/pwm/imx-tpm-pwm.txt | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > diff --git a/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > new file mode 100644 > index 0000000..94f1ad5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > @@ -0,0 +1,22 @@ > +Freescale i.MX TPM PWM controller > + > +Required properties: > +- compatible : Should be "fsl,imx-tpm". Needs to be SoC specific. > +- reg: Physical base address and length of the controller's registers. > +- #pwm-cells: Should be 3. See pwm.txt in this directory for a description of the cells format. > +- clocks : The clock provided by the SoC to drive the PWM. > +- interrupts: The interrupt for the PWM controller. > + > +Note: The TPM counter and period counter are shared between multiple channels, so all channels > +should use same period setting. > + > +Example: > + > +pwm0: tpm@40250000 { pwm@... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding 2019-03-25 20:41 ` Rob Herring @ 2019-03-25 20:59 ` Uwe Kleine-König 2019-03-26 18:51 ` Rob Herring 2019-03-26 0:56 ` Anson Huang 1 sibling, 1 reply; 14+ messages in thread From: Uwe Kleine-König @ 2019-03-25 20:59 UTC (permalink / raw) To: Rob Herring Cc: mark.rutland, linux-pwm, Robin Gong, otavio, Anson Huang, devicetree, festevam, s.hauer, jan.tuerk, linux, stefan, linux-kernel, thierry.reding, dl-linux-imx, kernel, Leonard Crestez, shawnguo, linux-arm-kernel On Mon, Mar 25, 2019 at 03:41:37PM -0500, Rob Herring wrote: > On Fri, Mar 22, 2019 at 01:48:05AM +0000, Anson Huang wrote: > > Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM binding. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > No changes. > > v9? I don't recall seeing any previous versions. Not sure it actually matters, but you were on Cc: for v7 and v8 at least: https://www.spinics.net/lists/linux-pwm/msg09300.html https://www.spinics.net/lists/linux-pwm/msg09321.html Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding 2019-03-25 20:59 ` Uwe Kleine-König @ 2019-03-26 18:51 ` Rob Herring 0 siblings, 0 replies; 14+ messages in thread From: Rob Herring @ 2019-03-26 18:51 UTC (permalink / raw) To: Uwe Kleine-König, Anson Huang Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel, thierry.reding, dl-linux-imx, kernel, Leonard Crestez, festevam, linux-arm-kernel On Mon, Mar 25, 2019 at 3:59 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Mar 25, 2019 at 03:41:37PM -0500, Rob Herring wrote: > > On Fri, Mar 22, 2019 at 01:48:05AM +0000, Anson Huang wrote: > > > Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM binding. > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > --- > > > No changes. > > > > v9? I don't recall seeing any previous versions. > > Not sure it actually matters, but you were on Cc: for v7 and v8 at > least: > > https://www.spinics.net/lists/linux-pwm/msg09300.html > https://www.spinics.net/lists/linux-pwm/msg09321.html 10 versions now in 2 weeks is why. Reviewers do get busy and/or go on vacation. Please slow down your pace and give people time to review. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding 2019-03-25 20:41 ` Rob Herring 2019-03-25 20:59 ` Uwe Kleine-König @ 2019-03-26 0:56 ` Anson Huang 2019-03-26 6:53 ` Anson Huang 1 sibling, 1 reply; 14+ messages in thread From: Anson Huang @ 2019-03-26 0:56 UTC (permalink / raw) To: Rob Herring Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel, thierry.reding, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez, festevam, linux-arm-kernel Hi, Rob Best Regards! Anson Huang > -----Original Message----- > From: Rob Herring [mailto:robh@kernel.org] > Sent: 2019年3月26日 4:42 > To: Anson Huang <anson.huang@nxp.com> > Cc: thierry.reding@gmail.com; mark.rutland@arm.com; > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; linux@armlinux.org.uk; stefan@agner.ch; > otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>; > Robin Gong <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux- > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine- > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding > > On Fri, Mar 22, 2019 at 01:48:05AM +0000, Anson Huang wrote: > > Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM > binding. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > No changes. > > v9? I don't recall seeing any previous versions. I checked my mailbox, you are in the "to" list in every version. The first version's subject is: [PATCH 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding... > > > --- > > .../devicetree/bindings/pwm/imx-tpm-pwm.txt | 22 > ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > > > diff --git a/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > new file mode 100644 > > index 0000000..94f1ad5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > @@ -0,0 +1,22 @@ > > +Freescale i.MX TPM PWM controller > > + > > +Required properties: > > +- compatible : Should be "fsl,imx-tpm". > > Needs to be SoC specific. OK, will change it to "fsl,imx7ulp-tpm" in next verson. > > > +- reg: Physical base address and length of the controller's registers. > > +- #pwm-cells: Should be 3. See pwm.txt in this directory for a description > of the cells format. > > +- clocks : The clock provided by the SoC to drive the PWM. > > +- interrupts: The interrupt for the PWM controller. > > + > > +Note: The TPM counter and period counter are shared between multiple > > +channels, so all channels should use same period setting. > > + > > +Example: > > + > > +pwm0: tpm@40250000 { > > pwm@... I forgot to update the example in DT binding, sorry for that, will update it according to below dts node: pwm0: pwm@40250000 { Anson. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding 2019-03-26 0:56 ` Anson Huang @ 2019-03-26 6:53 ` Anson Huang 0 siblings, 0 replies; 14+ messages in thread From: Anson Huang @ 2019-03-26 6:53 UTC (permalink / raw) To: Rob Herring Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, shawnguo, s.hauer, jan.tuerk, linux, stefan, linux-kernel, thierry.reding, dl-linux-imx, kernel, u.kleine-koenig, Leonard Crestez, festevam, linux-arm-kernel Hi, Rob Best Regards! Anson Huang > -----Original Message----- > From: Anson Huang > Sent: 2019年3月26日 8:56 > To: 'Rob Herring' <robh@kernel.org> > Cc: thierry.reding@gmail.com; mark.rutland@arm.com; > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > festevam@gmail.com; linux@armlinux.org.uk; stefan@agner.ch; > otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>; > Robin Gong <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux- > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine- > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com> > Subject: RE: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding > > Hi, Rob > > Best Regards! > Anson Huang > > > -----Original Message----- > > From: Rob Herring [mailto:robh@kernel.org] > > Sent: 2019年3月26日 4:42 > > To: Anson Huang <anson.huang@nxp.com> > > Cc: thierry.reding@gmail.com; mark.rutland@arm.com; > > shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de; > > festevam@gmail.com; linux@armlinux.org.uk; stefan@agner.ch; > > otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>; > > Robin Gong <yibin.gong@nxp.com>; jan.tuerk@emtrion.com; linux- > > pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; u.kleine- > > koenig@pengutronix.de; dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding > > > > On Fri, Mar 22, 2019 at 01:48:05AM +0000, Anson Huang wrote: > > > Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM > > binding. > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > --- > > > No changes. > > > > v9? I don't recall seeing any previous versions. > > I checked my mailbox, you are in the "to" list in every version. The first > version's subject is: > [PATCH 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding... > > > > > > --- > > > .../devicetree/bindings/pwm/imx-tpm-pwm.txt | 22 > > ++++++++++++++++++++++ > > > 1 file changed, 22 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > > b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > > new file mode 100644 > > > index 0000000..94f1ad5 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt > > > @@ -0,0 +1,22 @@ > > > +Freescale i.MX TPM PWM controller > > > + > > > +Required properties: > > > +- compatible : Should be "fsl,imx-tpm". > > > > Needs to be SoC specific. > > OK, will change it to "fsl,imx7ulp-tpm" in next verson. As " fsl,imx7ulp-tpm " is already used for timer, TPM can work as a timer or PWM, so I have to use " fsl,imx7ulp-pwm" in the V10 patch I just sent. Anson. > > > > > > +- reg: Physical base address and length of the controller's registers. > > > +- #pwm-cells: Should be 3. See pwm.txt in this directory for a > > > +description > > of the cells format. > > > +- clocks : The clock provided by the SoC to drive the PWM. > > > +- interrupts: The interrupt for the PWM controller. > > > + > > > +Note: The TPM counter and period counter are shared between > > > +multiple channels, so all channels should use same period setting. > > > + > > > +Example: > > > + > > > +pwm0: tpm@40250000 { > > > > pwm@... > > I forgot to update the example in DT binding, sorry for that, will update it > according to below dts node: > > pwm0: pwm@40250000 { > > Anson. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support 2019-03-22 1:47 [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support Anson Huang 2019-03-22 1:48 ` [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding Anson Huang @ 2019-03-22 1:48 ` Anson Huang 2019-03-25 9:30 ` Uwe Kleine-König 2019-03-22 1:48 ` [PATCH V9 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default Anson Huang ` (2 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Anson Huang @ 2019-03-22 1:48 UTC (permalink / raw) To: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam, linux, stefan, otavio, Leonard Crestez, Robin Gong, jan.tuerk, linux-pwm, devicetree, linux-arm-kernel, linux-kernel, u.kleine-koenig Cc: dl-linux-imx 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@nxp.com> --- Changes since V8: - add more limitation notes for period/duty update un-atomic limitations; - add waiting for period/duty update actually applied to HW; - move the duty update into period update function to make them to be updated as together as possiable; - don't allow PS change if counter is running; - save channel polarity settings and return it directly when .get_state is called, as the HW polarity setting could be impacted by enable status. --- drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-imx-tpm.c | 518 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 530 insertions(+) create mode 100644 drivers/pwm/pwm-imx-tpm.c diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 54f8238..3ea0391 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -210,6 +210,17 @@ config PWM_IMX27 To compile this driver as a module, choose M here: the module will be called pwm-imx27. +config PWM_IMX_TPM + tristate "i.MX TPM PWM support" + depends on ARCH_MXC || COMPILE_TEST + depends on HAVE_CLK && HAS_IOMEM + help + Generic PWM framework driver for i.MX7ULP TPM module, TPM's full + name is Low Power Timer/Pulse Width Modulation Module. + + To compile this driver as a module, choose M here: the module + will be called pwm-imx-tpm. + config PWM_JZ4740 tristate "Ingenic JZ47xx PWM support" depends on MACH_INGENIC diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 448825e..c368599 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o obj-$(CONFIG_PWM_IMG) += pwm-img.o obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c new file mode 100644 index 0000000..58af0915 --- /dev/null +++ b/drivers/pwm/pwm-imx-tpm.c @@ -0,0 +1,518 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2018-2019 NXP. + * + * Limitations: + * - The TPM counter and period counter are shared between + * multiple channels, so all channels should use same period + * settings. + * - Changes to polarity cannot be latched at the time of the + * next period start. + * - The period and duty changes are NOT atomic, if new period and + * new duty are requested simultaneously when counter is running, + * there could be a small window of running old duty with new + * period, as the period is updated before duty in this driver, the + * probability is very low, ONLY happen when the TPM counter changes + * from MOD to zero between the consecutive update of period and + * duty. + */ + +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/log2.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> + +#define PWM_IMX_TPM_PARAM 0x4 +#define PWM_IMX_TPM_GLOBAL 0x8 +#define PWM_IMX_TPM_SC 0x10 +#define PWM_IMX_TPM_CNT 0x14 +#define PWM_IMX_TPM_MOD 0x18 +#define PWM_IMX_TPM_CnSC(n) (0x20 + (n) * 0x8) +#define PWM_IMX_TPM_CnV(n) (0x24 + (n) * 0x8) + +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7, 0) + +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0) +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1) +#define PWM_IMX_TPM_SC_CPWMS BIT(5) + +#define PWM_IMX_TPM_CnSC_CHF BIT(7) +#define PWM_IMX_TPM_CnSC_MSB BIT(5) +#define PWM_IMX_TPM_CnSC_MSA BIT(4) + +/* + * The reference manual describes this field as two separate bits. The + * samantic of the two bits isn't orthogonal though, so they are treated + * together as a 2-bit field here. + */ +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED 0x1 +#define PWM_IMX_TPM_CnSC_ELS_INVERSED FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) +#define PWM_IMX_TPM_CnSC_ELS_NORMAL FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) + + +#define PWM_IMX_TPM_MOD_WIDTH 16 +#define PWM_IMX_TPM_MOD_MOD GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0) + +struct imx_tpm_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + void __iomem *base; + struct mutex lock; + u32 user_count; + u32 enable_count; + u32 real_period; +}; + +struct imx_tpm_pwm_param { + u8 prescale; + u32 mod; + u32 val; +}; + +struct imx_tpm_pwm_channel { + enum pwm_polarity polarity; +}; + +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct imx_tpm_pwm_chip, chip); +} + +static int pwm_imx_tpm_round_state(struct pwm_chip *chip, + struct imx_tpm_pwm_param *p, + struct pwm_state *state, + struct pwm_state *real_state) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + u32 rate, prescale, period_count, clock_unit; + u64 tmp; + + rate = clk_get_rate(tpm->clk); + tmp = (u64)state->period * rate; + clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); + if (clock_unit <= PWM_IMX_TPM_MOD_MOD) + prescale = 0; + else + prescale = ilog2(clock_unit) + 1 - PWM_IMX_TPM_MOD_WIDTH; + + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale))) + return -ERANGE; + p->prescale = prescale; + + period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale; + p->mod = period_count; + + /* calculate real period HW can support */ + tmp = (u64)period_count << prescale; + tmp *= NSEC_PER_SEC; + real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); + + /* + * if eventually the PWM output is inactive, either + * duty cycle is 0 or status is disabled, need to + * make sure the output pin is inactive. + */ + if (!state->enabled) + real_state->duty_cycle = 0; + else + real_state->duty_cycle = state->duty_cycle; + + tmp = (u64)p->mod * real_state->duty_cycle; + p->val = DIV_ROUND_CLOSEST_ULL(tmp, real_state->period); + + real_state->polarity = state->polarity; + real_state->enabled = state->enabled; + + return 0; +} + +/* this function is supposed to be called with mutex hold */ +static int pwm_imx_tpm_setup_period_duty(struct pwm_chip *chip, + struct pwm_device *pwm, + struct imx_tpm_pwm_param *p) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + unsigned long timeout; + u32 val, cmod, cur_prescale; + + val = readl(tpm->base + PWM_IMX_TPM_SC); + cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); + cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val); + if (cmod && cur_prescale != p->prescale) + return -EBUSY; + + /* set TPM counter prescale */ + val &= ~PWM_IMX_TPM_SC_PS; + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale); + writel(val, tpm->base + PWM_IMX_TPM_SC); + + /* + * set period count: + * if (CMOD[1:0] = 0:0) then MOD register is updated when MOD + * register is written. + * + * if (CMOD[1:0] ≠ 0:0), then MOD register is updated according + * to the CPWMS bit, that is: + * + * if the selected mode is not CPWM then MOD register is updated + * after MOD register was written and the TPM counter changes from + * MOD to zero. + * + * if the selected mode is CPWM then MOD register is updated after + * MOD register was written and the TPM counter changes from MOD + * to (MOD – 1). + */ + writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); + + /* + * set channel value: + * if (CMOD[1:0] = 0:0) then CnV register is updated when CnV + * register is written. + * + * if (CMOD[1:0] ≠ 0:0), then CnV register is updated according + * to the selected mode, that is: + * + * if the selected mode is output compare then CnV register is + * updated on the next TPM counter increment (end of the prescaler + * counting) after CnV register was written. + * + * if the selected mode is EPWM then CnV register is updated after + * CnV register was written and the TPM counter changes from MOD + * to zero. + * + * if the selected mode is CPWM then CnV register is updated after + * CnV register was written and the TPM counter changes from MOD + * to (MOD – 1). + */ + writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); + + /* make sure MOD & CnV registers are updated */ + timeout = jiffies + msecs_to_jiffies(tpm->real_period / + NSEC_PER_MSEC + 1); + while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod + || readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) + != p->val) { + if (time_after(jiffies, timeout)) + return -ETIME; + cpu_relax(); + } + + return 0; +} + +static 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); + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm); + u32 rate, val, prescale; + u64 tmp; + + /* get period */ + state->period = tpm->real_period; + + /* get duty cycle */ + rate = clk_get_rate(tpm->clk); + val = readl(tpm->base + PWM_IMX_TPM_SC); + prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val); + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); + tmp = (tmp << prescale) * NSEC_PER_SEC; + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate); + + /* get polarity */ + if (chan) { + state->polarity = chan->polarity; + } else { + /* in case no channel requested yet, return HW status */ + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == + PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED) + state->polarity = PWM_POLARITY_INVERSED; + else + /* + * Assume reserved values (2b00 and 2b11) to yield + * normal polarity. + */ + state->polarity = PWM_POLARITY_NORMAL; + } + + /* get channel status */ + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false; +} + +/* this function is supposed to be called with mutex hold */ +static int pwm_imx_tpm_apply_hw(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm); + unsigned long timeout; + struct pwm_state c; + u32 val; + u64 tmp; + + pwm_imx_tpm_get_state(chip, pwm, &c); + + if (state->duty_cycle != c.duty_cycle) { + /* set duty counter */ + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD; + tmp *= state->duty_cycle; + val = DIV_ROUND_CLOSEST_ULL(tmp, state->period); + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); + + /* make sure CnV register is updated */ + timeout = jiffies + msecs_to_jiffies(tpm->real_period / + NSEC_PER_MSEC + 1); + while (readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) != val) { + if (time_after(jiffies, timeout)) + return -ETIME; + cpu_relax(); + } + } + + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA | + PWM_IMX_TPM_CnSC_MSB); + if (state->enabled) { + /* + * set polarity (for edge-aligned PWM modes) + * + * ELS[1:0] = 2b10 yields normal polarity behaviour, + * ELS[1:0] = 2b01 yields inversed polarity. + * The other values are reserved. + * + * polarity settings will enabled/disable output status + * immediately, so if the channel is disabled, need to + * make sure MSA/MSB/ELS are set to 0 which means channel + * disabled. + */ + val |= PWM_IMX_TPM_CnSC_MSB; + val |= (state->polarity == PWM_POLARITY_NORMAL) ? + PWM_IMX_TPM_CnSC_ELS_NORMAL : + PWM_IMX_TPM_CnSC_ELS_INVERSED; + } + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); + + /* control the counter status */ + if (state->enabled != c.enabled) { + val = readl(tpm->base + PWM_IMX_TPM_SC); + if (state->enabled) { + if (++tpm->enable_count == 1) + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK; + } else { + if (--tpm->enable_count == 0) + val &= ~PWM_IMX_TPM_SC_CMOD; + } + writel(val, tpm->base + PWM_IMX_TPM_SC); + } + + /* save last polarity setting */ + chan->polarity = state->polarity; + + return 0; +} + +static int pwm_imx_tpm_apply(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + struct imx_tpm_pwm_param param; + struct pwm_state real_state; + int ret; + + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); + if (ret) + return -EINVAL; + + mutex_lock(&tpm->lock); + + /* + * TPM counter is shared by multiple channels, so + * prescale and period can NOT be modified when + * there are multiple channels in use with different + * period settings. + */ + if (real_state.period != tpm->real_period) { + if (tpm->user_count > 1) { + ret = -EBUSY; + goto exit; + } + + ret = pwm_imx_tpm_setup_period_duty(chip, pwm, ¶m); + if (ret) + goto exit; + + tpm->real_period = real_state.period; + } + + ret = pwm_imx_tpm_apply_hw(chip, pwm, &real_state); + +exit: + mutex_unlock(&tpm->lock); + + return ret; +} + +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + struct imx_tpm_pwm_channel *chan; + + chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL); + if (!chan) + return -ENOMEM; + + pwm_set_chip_data(pwm, chan); + + mutex_lock(&tpm->lock); + tpm->user_count++; + mutex_unlock(&tpm->lock); + + return 0; +} + +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); + + mutex_lock(&tpm->lock); + tpm->user_count--; + mutex_unlock(&tpm->lock); + + devm_kfree(chip->dev, pwm_get_chip_data(pwm)); + pwm_set_chip_data(pwm, NULL); +} + +static const struct pwm_ops imx_tpm_pwm_ops = { + .request = pwm_imx_tpm_request, + .free = pwm_imx_tpm_free, + .get_state = pwm_imx_tpm_get_state, + .apply = pwm_imx_tpm_apply, + .owner = THIS_MODULE, +}; + +static int pwm_imx_tpm_probe(struct platform_device *pdev) +{ + struct imx_tpm_pwm_chip *tpm; + int ret; + u32 val; + + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL); + if (!tpm) + return -ENOMEM; + + platform_set_drvdata(pdev, tpm); + + tpm->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(tpm->base)) + return PTR_ERR(tpm->base); + + 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 clock: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(tpm->clk); + if (ret) { + dev_err(&pdev->dev, + "failed to prepare or enable clock: %d\n", ret); + return ret; + } + + tpm->chip.dev = &pdev->dev; + tpm->chip.ops = &imx_tpm_pwm_ops; + tpm->chip.base = -1; + tpm->chip.of_xlate = of_pwm_xlate_with_flags; + tpm->chip.of_pwm_n_cells = 3; + + /* get number of channels */ + val = readl(tpm->base + PWM_IMX_TPM_PARAM); + tpm->chip.npwm = FIELD_GET(PWM_IMX_TPM_PARAM_CHAN, val); + + 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); + int ret = pwmchip_remove(&tpm->chip); + + clk_disable_unprepare(tpm->clk); + + return ret; +} + +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) +{ + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev); + + if (tpm->enable_count > 0) + return -EBUSY; + + 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 = 0; + + ret = clk_prepare_enable(tpm->clk); + if (ret) + dev_err(dev, + "failed to prepare or enable clock: %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", }, + { /* 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"); -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support 2019-03-22 1:48 ` [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support Anson Huang @ 2019-03-25 9:30 ` Uwe Kleine-König 2019-03-25 11:58 ` Anson Huang 0 siblings, 1 reply; 14+ messages in thread From: Uwe Kleine-König @ 2019-03-25 9:30 UTC (permalink / raw) To: Anson Huang Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt, linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez, shawnguo, linux-arm-kernel, dl-linux-imx On Fri, Mar 22, 2019 at 01:48:11AM +0000, Anson Huang wrote: > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module) > inside, it can support multiple PWM channels, all the channels > share same counter and period setting, but each channel can > configure its duty and polarity independently. > > There are several TPM modules in i.MX7ULP, the number of channels > in TPM modules are different, it can be read from each TPM module's > PARAM register. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > Changes since V8: > - add more limitation notes for period/duty update un-atomic limitations; > - add waiting for period/duty update actually applied to HW; > - move the duty update into period update function to make them to be updated > as together as possiable; > - don't allow PS change if counter is running; > - save channel polarity settings and return it directly when .get_state is called, > as the HW polarity setting could be impacted by enable status. > --- > drivers/pwm/Kconfig | 11 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-imx-tpm.c | 518 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 530 insertions(+) > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 54f8238..3ea0391 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -210,6 +210,17 @@ config PWM_IMX27 > To compile this driver as a module, choose M here: the module > will be called pwm-imx27. > > +config PWM_IMX_TPM > + tristate "i.MX TPM PWM support" > + depends on ARCH_MXC || COMPILE_TEST > + depends on HAVE_CLK && HAS_IOMEM > + help > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's full > + name is Low Power Timer/Pulse Width Modulation Module. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-imx-tpm. > + > config PWM_JZ4740 > tristate "Ingenic JZ47xx PWM support" > depends on MACH_INGENIC > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 448825e..c368599 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o > obj-$(CONFIG_PWM_IMG) += pwm-img.o > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c > new file mode 100644 > index 0000000..58af0915 > --- /dev/null > +++ b/drivers/pwm/pwm-imx-tpm.c > @@ -0,0 +1,518 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2018-2019 NXP. > + * > + * Limitations: > + * - The TPM counter and period counter are shared between > + * multiple channels, so all channels should use same period > + * settings. > + * - Changes to polarity cannot be latched at the time of the > + * next period start. > + * - The period and duty changes are NOT atomic, if new period and > + * new duty are requested simultaneously when counter is running, > + * there could be a small window of running old duty with new > + * period, as the period is updated before duty in this driver, the > + * probability is very low, ONLY happen when the TPM counter changes > + * from MOD to zero between the consecutive update of period and > + * duty. The window that this bug triggers is small, but if it does, the window where the invalid combination is applied, isn't small---it's one complete period if I'm not mistaken. So I'd write: - Changing period and duty cycle together isn't atomic. With the wrong timing it might happen that a period is produced with old duty cycle but new period settings. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/bitops.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/log2.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/slab.h> > + > +#define PWM_IMX_TPM_PARAM 0x4 > +#define PWM_IMX_TPM_GLOBAL 0x8 > +#define PWM_IMX_TPM_SC 0x10 > +#define PWM_IMX_TPM_CNT 0x14 > +#define PWM_IMX_TPM_MOD 0x18 > +#define PWM_IMX_TPM_CnSC(n) (0x20 + (n) * 0x8) > +#define PWM_IMX_TPM_CnV(n) (0x24 + (n) * 0x8) > + > +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7, 0) > + > +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0) > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1) > +#define PWM_IMX_TPM_SC_CPWMS BIT(5) > + > +#define PWM_IMX_TPM_CnSC_CHF BIT(7) > +#define PWM_IMX_TPM_CnSC_MSB BIT(5) > +#define PWM_IMX_TPM_CnSC_MSA BIT(4) > + > +/* > + * The reference manual describes this field as two separate bits. The > + * samantic of the two bits isn't orthogonal though, so they are treated s/samantic/semantic/ > + * together as a 2-bit field here. > + */ > +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) > +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED 0x1 > +#define PWM_IMX_TPM_CnSC_ELS_INVERSED FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) > +#define PWM_IMX_TPM_CnSC_ELS_NORMAL FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) > + > + > +#define PWM_IMX_TPM_MOD_WIDTH 16 > +#define PWM_IMX_TPM_MOD_MOD GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0) > + > +struct imx_tpm_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *base; > + struct mutex lock; > + u32 user_count; > + u32 enable_count; > + u32 real_period; > +}; > + > +struct imx_tpm_pwm_param { > + u8 prescale; > + u32 mod; > + u32 val; > +}; > + > +struct imx_tpm_pwm_channel { > + enum pwm_polarity polarity; > +}; > + > +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct pwm_chip *chip) > +{ > + return container_of(chip, struct imx_tpm_pwm_chip, chip); > +} > + > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip, > + struct imx_tpm_pwm_param *p, > + struct pwm_state *state, > + struct pwm_state *real_state) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + u32 rate, prescale, period_count, clock_unit; > + u64 tmp; > + > + rate = clk_get_rate(tpm->clk); > + tmp = (u64)state->period * rate; > + clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > + if (clock_unit <= PWM_IMX_TPM_MOD_MOD) > + prescale = 0; > + else > + prescale = ilog2(clock_unit) + 1 - PWM_IMX_TPM_MOD_WIDTH; > + > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale))) > + return -ERANGE; > + p->prescale = prescale; > + > + period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale; > + p->mod = period_count; > + > + /* calculate real period HW can support */ > + tmp = (u64)period_count << prescale; > + tmp *= NSEC_PER_SEC; > + real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > + > + /* > + * if eventually the PWM output is inactive, either > + * duty cycle is 0 or status is disabled, need to > + * make sure the output pin is inactive. > + */ > + if (!state->enabled) > + real_state->duty_cycle = 0; > + else > + real_state->duty_cycle = state->duty_cycle; > + > + tmp = (u64)p->mod * real_state->duty_cycle; > + p->val = DIV_ROUND_CLOSEST_ULL(tmp, real_state->period); > + > + real_state->polarity = state->polarity; > + real_state->enabled = state->enabled; > + > + return 0; > +} > + > +/* this function is supposed to be called with mutex hold */ > +static int pwm_imx_tpm_setup_period_duty(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct imx_tpm_pwm_param *p) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + unsigned long timeout; > + u32 val, cmod, cur_prescale; > + > + val = readl(tpm->base + PWM_IMX_TPM_SC); > + cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > + cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val); > + if (cmod && cur_prescale != p->prescale) > + return -EBUSY; > + > + /* set TPM counter prescale */ > + val &= ~PWM_IMX_TPM_SC_PS; > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale); > + writel(val, tpm->base + PWM_IMX_TPM_SC); > + > + /* > + * set period count: > + * if (CMOD[1:0] = 0:0) then MOD register is updated when MOD > + * register is written. > + * > + * if (CMOD[1:0] ≠ 0:0), then MOD register is updated according > + * to the CPWMS bit, that is: > + * > + * if the selected mode is not CPWM then MOD register is updated > + * after MOD register was written and the TPM counter changes from > + * MOD to zero. > + * > + * if the selected mode is CPWM then MOD register is updated after > + * MOD register was written and the TPM counter changes from MOD > + * to (MOD – 1). Given that the driver doesn't make use of CPWM, this comment could be simplified. I'd write: /* * If the PWM is enabled (CMOD[1:0] ≠ 2b00), the period length * is latched into hardware when the next period starts. */ This is even true for the (here unused) CPWM mode. (The reference manual isn't entirely clear here IMHO. I assume if MOD == 4000 and CNT == 2001 then MOD is then changed to 2000, the currently running period is completed with a length of 4000 prescaled clk cycles?!) > + */ > + writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); > + > + /* > + * set channel value: > + * if (CMOD[1:0] = 0:0) then CnV register is updated when CnV > + * register is written. > + * > + * if (CMOD[1:0] ≠ 0:0), then CnV register is updated according > + * to the selected mode, that is: > + * > + * if the selected mode is output compare then CnV register is > + * updated on the next TPM counter increment (end of the prescaler > + * counting) after CnV register was written. > + * > + * if the selected mode is EPWM then CnV register is updated after > + * CnV register was written and the TPM counter changes from MOD > + * to zero. > + * > + * if the selected mode is CPWM then CnV register is updated after > + * CnV register was written and the TPM counter changes from MOD > + * to (MOD – 1). This is similar to the above too verbose and covers stuff that is not relevant for this driver. Also the used wording is not obvious if you don't look into the reference manual. > + */ > + writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > + > + /* make sure MOD & CnV registers are updated */ > + timeout = jiffies + msecs_to_jiffies(tpm->real_period / > + NSEC_PER_MSEC + 1); > + while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod > + || readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) > + != p->val) { > + if (time_after(jiffies, timeout)) > + return -ETIME; > + cpu_relax(); > + } > + > + return 0; > +} > [...] > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + struct imx_tpm_pwm_param param; > + struct pwm_state real_state; > + int ret; > + > + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > + if (ret) > + return -EINVAL; > + > + mutex_lock(&tpm->lock); > + > + /* > + * TPM counter is shared by multiple channels, so > + * prescale and period can NOT be modified when > + * there are multiple channels in use with different > + * period settings. > + */ > + if (real_state.period != tpm->real_period) { > + if (tpm->user_count > 1) { > + ret = -EBUSY; > + goto exit; > + } > + > + ret = pwm_imx_tpm_setup_period_duty(chip, pwm, ¶m); > + if (ret) > + goto exit; > + > + tpm->real_period = real_state.period; > + } > + > + ret = pwm_imx_tpm_apply_hw(chip, pwm, &real_state); It's unintuitive here that both pwm_imx_tpm_setup_period_duty and pwm_imx_tpm_apply_hw (potentially) configure the duty cycle. I didn't thought it to an end, but maybe this could be optimised? > +exit: > + mutex_unlock(&tpm->lock); > + > + return ret; > +} > + > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + struct imx_tpm_pwm_channel *chan; > + > + chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL); There is no advantage in using the devm variant here as the requested memory is freed in .free anyhow. So this only adds additional memory foodprint and runtime overhead. > + if (!chan) > + return -ENOMEM; > + > + pwm_set_chip_data(pwm, chan); > + > + mutex_lock(&tpm->lock); > + tpm->user_count++; > + mutex_unlock(&tpm->lock); > + > + return 0; > +} > + > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > + > + mutex_lock(&tpm->lock); > + tpm->user_count--; > + mutex_unlock(&tpm->lock); > + > + devm_kfree(chip->dev, pwm_get_chip_data(pwm)); > + pwm_set_chip_data(pwm, NULL); The call to pwm_set_chip_data could better be done in the PWM core. > +} -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support 2019-03-25 9:30 ` Uwe Kleine-König @ 2019-03-25 11:58 ` Anson Huang 2019-03-26 6:56 ` Anson Huang 0 siblings, 1 reply; 14+ messages in thread From: Anson Huang @ 2019-03-25 11:58 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt, linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez, shawnguo, linux-arm-kernel, dl-linux-imx Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > Sent: 2019年3月25日 17:30 > 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; stefan@agner.ch; > otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>; > Robin Gong <yibin.gong@nxp.com>; jan.tuerk@emtrion.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 V9 2/5] pwm: Add i.MX TPM PWM driver support > > On Fri, Mar 22, 2019 at 01:48:11AM +0000, Anson Huang wrote: > > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module) > > inside, it can support multiple PWM channels, all the channels share > > same counter and period setting, but each channel can configure its > > duty and polarity independently. > > > > There are several TPM modules in i.MX7ULP, the number of channels in > > TPM modules are different, it can be read from each TPM module's PARAM > > register. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > Changes since V8: > > - add more limitation notes for period/duty update un-atomic > limitations; > > - add waiting for period/duty update actually applied to HW; > > - move the duty update into period update function to make them to > be updated > > as together as possiable; > > - don't allow PS change if counter is running; > > - save channel polarity settings and return it directly when .get_state > is called, > > as the HW polarity setting could be impacted by enable status. > > --- > > drivers/pwm/Kconfig | 11 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-imx-tpm.c | 518 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 530 insertions(+) > > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > 54f8238..3ea0391 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -210,6 +210,17 @@ config PWM_IMX27 > > To compile this driver as a module, choose M here: the module > > will be called pwm-imx27. > > > > +config PWM_IMX_TPM > > + tristate "i.MX TPM PWM support" > > + depends on ARCH_MXC || COMPILE_TEST > > + depends on HAVE_CLK && HAS_IOMEM > > + help > > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's > full > > + name is Low Power Timer/Pulse Width Modulation Module. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-imx-tpm. > > + > > config PWM_JZ4740 > > tristate "Ingenic JZ47xx PWM support" > > depends on MACH_INGENIC > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > > 448825e..c368599 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm- > hibvt.o > > obj-$(CONFIG_PWM_IMG) += pwm-img.o > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c > new > > file mode 100644 index 0000000..58af0915 > > --- /dev/null > > +++ b/drivers/pwm/pwm-imx-tpm.c > > @@ -0,0 +1,518 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright 2018-2019 NXP. > > + * > > + * Limitations: > > + * - The TPM counter and period counter are shared between > > + * multiple channels, so all channels should use same period > > + * settings. > > + * - Changes to polarity cannot be latched at the time of the > > + * next period start. > > + * - The period and duty changes are NOT atomic, if new period and > > + * new duty are requested simultaneously when counter is running, > > + * there could be a small window of running old duty with new > > + * period, as the period is updated before duty in this driver, the > > + * probability is very low, ONLY happen when the TPM counter changes > > + * from MOD to zero between the consecutive update of period and > > + * duty. > > The window that this bug triggers is small, but if it does, the window where > the invalid combination is applied, isn't small---it's one complete period if I'm > not mistaken. So I'd write: > > - Changing period and duty cycle together isn't atomic. With the wrong > timing it might happen that a period is produced with old duty cycle > but new period settings. > OK, thanks. > > + */ > > + > > +#include <linux/bitfield.h> > > +#include <linux/bitops.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/log2.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/slab.h> > > + > > +#define PWM_IMX_TPM_PARAM 0x4 > > +#define PWM_IMX_TPM_GLOBAL 0x8 > > +#define PWM_IMX_TPM_SC 0x10 > > +#define PWM_IMX_TPM_CNT 0x14 > > +#define PWM_IMX_TPM_MOD 0x18 > > +#define PWM_IMX_TPM_CnSC(n) (0x20 + (n) * 0x8) > > +#define PWM_IMX_TPM_CnV(n) (0x24 + (n) * 0x8) > > + > > +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7, > 0) > > + > > +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0) > > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) > > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK > FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1) > > +#define PWM_IMX_TPM_SC_CPWMS BIT(5) > > + > > +#define PWM_IMX_TPM_CnSC_CHF BIT(7) > > +#define PWM_IMX_TPM_CnSC_MSB BIT(5) > > +#define PWM_IMX_TPM_CnSC_MSA BIT(4) > > + > > +/* > > + * The reference manual describes this field as two separate bits. > > +The > > + * samantic of the two bits isn't orthogonal though, so they are > > +treated > > s/samantic/semantic/ > > > + * together as a 2-bit field here. > > + */ > > +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) > > +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED 0x1 > > +#define PWM_IMX_TPM_CnSC_ELS_INVERSED > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) > > +#define PWM_IMX_TPM_CnSC_ELS_NORMAL > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) > > + > > + > > +#define PWM_IMX_TPM_MOD_WIDTH 16 > > +#define PWM_IMX_TPM_MOD_MOD > GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0) > > + > > +struct imx_tpm_pwm_chip { > > + struct pwm_chip chip; > > + struct clk *clk; > > + void __iomem *base; > > + struct mutex lock; > > + u32 user_count; > > + u32 enable_count; > > + u32 real_period; > > +}; > > + > > +struct imx_tpm_pwm_param { > > + u8 prescale; > > + u32 mod; > > + u32 val; > > +}; > > + > > +struct imx_tpm_pwm_channel { > > + enum pwm_polarity polarity; > > +}; > > + > > +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct > > +pwm_chip *chip) { > > + return container_of(chip, struct imx_tpm_pwm_chip, chip); } > > + > > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip, > > + struct imx_tpm_pwm_param *p, > > + struct pwm_state *state, > > + struct pwm_state *real_state) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 rate, prescale, period_count, clock_unit; > > + u64 tmp; > > + > > + rate = clk_get_rate(tpm->clk); > > + tmp = (u64)state->period * rate; > > + clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > > + if (clock_unit <= PWM_IMX_TPM_MOD_MOD) > > + prescale = 0; > > + else > > + prescale = ilog2(clock_unit) + 1 - > PWM_IMX_TPM_MOD_WIDTH; > > + > > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale))) > > + return -ERANGE; > > + p->prescale = prescale; > > + > > + period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale; > > + p->mod = period_count; > > + > > + /* calculate real period HW can support */ > > + tmp = (u64)period_count << prescale; > > + tmp *= NSEC_PER_SEC; > > + real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > + > > + /* > > + * if eventually the PWM output is inactive, either > > + * duty cycle is 0 or status is disabled, need to > > + * make sure the output pin is inactive. > > + */ > > + if (!state->enabled) > > + real_state->duty_cycle = 0; > > + else > > + real_state->duty_cycle = state->duty_cycle; > > + > > + tmp = (u64)p->mod * real_state->duty_cycle; > > + p->val = DIV_ROUND_CLOSEST_ULL(tmp, real_state->period); > > + > > + real_state->polarity = state->polarity; > > + real_state->enabled = state->enabled; > > + > > + return 0; > > +} > > + > > +/* this function is supposed to be called with mutex hold */ static > > +int pwm_imx_tpm_setup_period_duty(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + struct imx_tpm_pwm_param *p) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + unsigned long timeout; > > + u32 val, cmod, cur_prescale; > > + > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > > + cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val); > > + if (cmod && cur_prescale != p->prescale) > > + return -EBUSY; > > + > > + /* set TPM counter prescale */ > > + val &= ~PWM_IMX_TPM_SC_PS; > > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale); > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + > > + /* > > + * set period count: > > + * if (CMOD[1:0] = 0:0) then MOD register is updated when MOD > > + * register is written. > > + * > > + * if (CMOD[1:0] ≠ 0:0), then MOD register is updated according > > + * to the CPWMS bit, that is: > > + * > > + * if the selected mode is not CPWM then MOD register is updated > > + * after MOD register was written and the TPM counter changes from > > + * MOD to zero. > > + * > > + * if the selected mode is CPWM then MOD register is updated after > > + * MOD register was written and the TPM counter changes from > MOD > > + * to (MOD – 1). > > Given that the driver doesn't make use of CPWM, this comment could be > simplified. I'd write: > > /* > * If the PWM is enabled (CMOD[1:0] ≠ 2b00), the period length > * is latched into hardware when the next period starts. > */ > OK, thanks. > This is even true for the (here unused) CPWM mode. (The reference manual > isn't entirely clear here IMHO. I assume if MOD == 4000 and CNT == 2001 > then MOD is then changed to 2000, the currently running period is > completed with a length of 4000 prescaled clk cycles?!) Based on my understanding from RM, I think so. > > > + */ > > + writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); > > + > > + /* > > + * set channel value: > > + * if (CMOD[1:0] = 0:0) then CnV register is updated when CnV > > + * register is written. > > + * > > + * if (CMOD[1:0] ≠ 0:0), then CnV register is updated according > > + * to the selected mode, that is: > > + * > > + * if the selected mode is output compare then CnV register is > > + * updated on the next TPM counter increment (end of the prescaler > > + * counting) after CnV register was written. > > + * > > + * if the selected mode is EPWM then CnV register is updated after > > + * CnV register was written and the TPM counter changes from MOD > > + * to zero. > > + * > > + * if the selected mode is CPWM then CnV register is updated after > > + * CnV register was written and the TPM counter changes from MOD > > + * to (MOD – 1). > > This is similar to the above too verbose and covers stuff that is not relevant > for this driver. Also the used wording is not obvious if you don't look into the > reference manual. OK, thanks. > > > + */ > > + writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > + > > + /* make sure MOD & CnV registers are updated */ > > + timeout = jiffies + msecs_to_jiffies(tpm->real_period / > > + NSEC_PER_MSEC + 1); > > + while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod > > + || readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) > > + != p->val) { > > + if (time_after(jiffies, timeout)) > > + return -ETIME; > > + cpu_relax(); > > + } > > + > > + return 0; > > +} > > [...] > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + struct imx_tpm_pwm_param param; > > + struct pwm_state real_state; > > + int ret; > > + > > + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > > + if (ret) > > + return -EINVAL; > > + > > + mutex_lock(&tpm->lock); > > + > > + /* > > + * TPM counter is shared by multiple channels, so > > + * prescale and period can NOT be modified when > > + * there are multiple channels in use with different > > + * period settings. > > + */ > > + if (real_state.period != tpm->real_period) { > > + if (tpm->user_count > 1) { > > + ret = -EBUSY; > > + goto exit; > > + } > > + > > + ret = pwm_imx_tpm_setup_period_duty(chip, pwm, > ¶m); > > + if (ret) > > + goto exit; > > + > > + tpm->real_period = real_state.period; > > + } > > + > > + ret = pwm_imx_tpm_apply_hw(chip, pwm, &real_state); > > It's unintuitive here that both pwm_imx_tpm_setup_period_duty and > pwm_imx_tpm_apply_hw (potentially) configure the duty cycle. I didn't > thought it to an end, but maybe this could be optimised? This is also my concern when implementing it, since period needs to be configured before duty, and we want to put these 2 configurations as close as possible, so for the period change, I also configure the duty together, but for normal use cases, period does NOT change, so we also need to consider duty change ONLY case, that is why I put a current duty and requested duty check in the pwm_imx_tpm_apply_hw() function. if (state->duty_cycle != c.duty_cycle) { /* set duty counter */ tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD; tmp *= state->duty_cycle; val = DIV_ROUND_CLOSEST_ULL(tmp, state->period); writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); We can also put all the configurations together in 1 function, but that also introduce some confusion I think, current implementation separate the period settings (for all channels) and other settings (for each channel) in 2 function, it is just because that the duty change is better to be put as close as period change, so I did it this way. Maybe add some comments for it is acceptable? > > > +exit: > > + mutex_unlock(&tpm->lock); > > + > > + return ret; > > +} > > + > > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct > > +pwm_device *pwm) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + struct imx_tpm_pwm_channel *chan; > > + > > + chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL); > > There is no advantage in using the devm variant here as the requested > memory is freed in .free anyhow. So this only adds additional memory > foodprint and runtime overhead. Makes sense, I will use kzalloc directly, looks like other pwm driver(drivers/pwm/pwm-samsung.c) also has such "issue". > > > + if (!chan) > > + return -ENOMEM; > > + > > + pwm_set_chip_data(pwm, chan); > > + > > + mutex_lock(&tpm->lock); > > + tpm->user_count++; > > + mutex_unlock(&tpm->lock); > > + > > + return 0; > > +} > > + > > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct > pwm_device > > +*pwm) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + > > + mutex_lock(&tpm->lock); > > + tpm->user_count--; > > + mutex_unlock(&tpm->lock); > > + > > + devm_kfree(chip->dev, pwm_get_chip_data(pwm)); > > + pwm_set_chip_data(pwm, NULL); > > The call to pwm_set_chip_data could better be done in the PWM core. You meant doing a new patch to add it in PWM core.c after ->free call? If yes, then I think this should be another topic, as many other pwm drivers also call it in their own driver, maybe it can be improved by another patch? Thanks, Anson. > > > +} > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7Ca7 > 1937c9d5e84033309008d6b1048c3a%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636891030423087284&sdata=a3xsu9iaAGvfUYv%2FNo6 > T5Uvw6k%2F5VbyI2cFzsrnA4IM%3D&reserved=0 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support 2019-03-25 11:58 ` Anson Huang @ 2019-03-26 6:56 ` Anson Huang 0 siblings, 0 replies; 14+ messages in thread From: Anson Huang @ 2019-03-26 6:56 UTC (permalink / raw) To: Uwe Kleine-König Cc: mark.rutland, linux-pwm, Robin Gong, otavio, devicetree, festevam, s.hauer, jan.tuerk, linux, robh+dt, linux-kernel, thierry.reding, stefan, kernel, Leonard Crestez, shawnguo, linux-arm-kernel, dl-linux-imx Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Anson Huang > Sent: 2019年3月25日 19:59 > To: 'Uwe Kleine-König' <u.kleine-koenig@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; stefan@agner.ch; > otavio@ossystems.com.br; Leonard Crestez <leonard.crestez@nxp.com>; > Robin Gong <yibin.gong@nxp.com>; jan.tuerk@emtrion.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 V9 2/5] pwm: Add i.MX TPM PWM driver support > > Hi, Uwe > > Best Regards! > Anson Huang > > > -----Original Message----- > > From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de] > > Sent: 2019年3月25日 17:30 > > 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; > > stefan@agner.ch; otavio@ossystems.com.br; Leonard Crestez > > <leonard.crestez@nxp.com>; Robin Gong <yibin.gong@nxp.com>; > > jan.tuerk@emtrion.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 V9 2/5] pwm: Add i.MX TPM PWM driver support > > > > On Fri, Mar 22, 2019 at 01:48:11AM +0000, Anson Huang wrote: > > > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module) > > > inside, it can support multiple PWM channels, all the channels share > > > same counter and period setting, but each channel can configure its > > > duty and polarity independently. > > > > > > There are several TPM modules in i.MX7ULP, the number of channels in > > > TPM modules are different, it can be read from each TPM module's > > > PARAM register. > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > --- > > > Changes since V8: > > > - add more limitation notes for period/duty update un-atomic > > limitations; > > > - add waiting for period/duty update actually applied to HW; > > > - move the duty update into period update function to make them to > > be updated > > > as together as possiable; > > > - don't allow PS change if counter is running; > > > - save channel polarity settings and return it directly when > > > .get_state > > is called, > > > as the HW polarity setting could be impacted by enable status. > > > --- > > > drivers/pwm/Kconfig | 11 + > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-imx-tpm.c | 518 > > > ++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 530 insertions(+) > > > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > > 54f8238..3ea0391 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -210,6 +210,17 @@ config PWM_IMX27 > > > To compile this driver as a module, choose M here: the module > > > will be called pwm-imx27. > > > > > > +config PWM_IMX_TPM > > > + tristate "i.MX TPM PWM support" > > > + depends on ARCH_MXC || COMPILE_TEST > > > + depends on HAVE_CLK && HAS_IOMEM > > > + help > > > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's > > full > > > + name is Low Power Timer/Pulse Width Modulation Module. > > > + > > > + To compile this driver as a module, choose M here: the module > > > + will be called pwm-imx-tpm. > > > + > > > config PWM_JZ4740 > > > tristate "Ingenic JZ47xx PWM support" > > > depends on MACH_INGENIC > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > > > 448825e..c368599 100644 > > > --- a/drivers/pwm/Makefile > > > +++ b/drivers/pwm/Makefile > > > @@ -19,6 +19,7 @@ obj-$(CONFIG_PWM_HIBVT) += pwm- > > hibvt.o > > > obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c > > new > > > file mode 100644 index 0000000..58af0915 > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-imx-tpm.c > > > @@ -0,0 +1,518 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright 2018-2019 NXP. > > > + * > > > + * Limitations: > > > + * - The TPM counter and period counter are shared between > > > + * multiple channels, so all channels should use same period > > > + * settings. > > > + * - Changes to polarity cannot be latched at the time of the > > > + * next period start. > > > + * - The period and duty changes are NOT atomic, if new period and > > > + * new duty are requested simultaneously when counter is running, > > > + * there could be a small window of running old duty with new > > > + * period, as the period is updated before duty in this driver, the > > > + * probability is very low, ONLY happen when the TPM counter changes > > > + * from MOD to zero between the consecutive update of period and > > > + * duty. > > > > The window that this bug triggers is small, but if it does, the window > > where the invalid combination is applied, isn't small---it's one > > complete period if I'm not mistaken. So I'd write: > > > > - Changing period and duty cycle together isn't atomic. With the wrong > > timing it might happen that a period is produced with old duty cycle > > but new period settings. > > > > OK, thanks. > > > > + */ > > > + > > > +#include <linux/bitfield.h> > > > +#include <linux/bitops.h> > > > +#include <linux/clk.h> > > > +#include <linux/err.h> > > > +#include <linux/io.h> > > > +#include <linux/log2.h> > > > +#include <linux/module.h> > > > +#include <linux/of.h> > > > +#include <linux/of_address.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/pwm.h> > > > +#include <linux/slab.h> > > > + > > > +#define PWM_IMX_TPM_PARAM 0x4 > > > +#define PWM_IMX_TPM_GLOBAL 0x8 > > > +#define PWM_IMX_TPM_SC 0x10 > > > +#define PWM_IMX_TPM_CNT 0x14 > > > +#define PWM_IMX_TPM_MOD 0x18 > > > +#define PWM_IMX_TPM_CnSC(n) (0x20 + (n) * 0x8) > > > +#define PWM_IMX_TPM_CnV(n) (0x24 + (n) * 0x8) > > > + > > > +#define PWM_IMX_TPM_PARAM_CHAN GENMASK(7, > > 0) > > > + > > > +#define PWM_IMX_TPM_SC_PS GENMASK(2, 0) > > > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, > 3) > > > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK > > FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1) > > > +#define PWM_IMX_TPM_SC_CPWMS BIT(5) > > > + > > > +#define PWM_IMX_TPM_CnSC_CHF BIT(7) > > > +#define PWM_IMX_TPM_CnSC_MSB BIT(5) > > > +#define PWM_IMX_TPM_CnSC_MSA BIT(4) > > > + > > > +/* > > > + * The reference manual describes this field as two separate bits. > > > +The > > > + * samantic of the two bits isn't orthogonal though, so they are > > > +treated > > > > s/samantic/semantic/ > > > > > + * together as a 2-bit field here. > > > + */ > > > +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) > > > +#define PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED 0x1 > > > +#define PWM_IMX_TPM_CnSC_ELS_INVERSED > > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) > > > +#define PWM_IMX_TPM_CnSC_ELS_NORMAL > > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) > > > + > > > + > > > +#define PWM_IMX_TPM_MOD_WIDTH 16 > > > +#define PWM_IMX_TPM_MOD_MOD > > GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0) > > > + > > > +struct imx_tpm_pwm_chip { > > > + struct pwm_chip chip; > > > + struct clk *clk; > > > + void __iomem *base; > > > + struct mutex lock; > > > + u32 user_count; > > > + u32 enable_count; > > > + u32 real_period; > > > +}; > > > + > > > +struct imx_tpm_pwm_param { > > > + u8 prescale; > > > + u32 mod; > > > + u32 val; > > > +}; > > > + > > > +struct imx_tpm_pwm_channel { > > > + enum pwm_polarity polarity; > > > +}; > > > + > > > +static inline struct imx_tpm_pwm_chip *to_imx_tpm_pwm_chip(struct > > > +pwm_chip *chip) { > > > + return container_of(chip, struct imx_tpm_pwm_chip, chip); } > > > + > > > +static int pwm_imx_tpm_round_state(struct pwm_chip *chip, > > > + struct imx_tpm_pwm_param *p, > > > + struct pwm_state *state, > > > + struct pwm_state *real_state) { > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + u32 rate, prescale, period_count, clock_unit; > > > + u64 tmp; > > > + > > > + rate = clk_get_rate(tpm->clk); > > > + tmp = (u64)state->period * rate; > > > + clock_unit = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > > > + if (clock_unit <= PWM_IMX_TPM_MOD_MOD) > > > + prescale = 0; > > > + else > > > + prescale = ilog2(clock_unit) + 1 - > > PWM_IMX_TPM_MOD_WIDTH; > > > + > > > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale))) > > > + return -ERANGE; > > > + p->prescale = prescale; > > > + > > > + period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale; > > > + p->mod = period_count; > > > + > > > + /* calculate real period HW can support */ > > > + tmp = (u64)period_count << prescale; > > > + tmp *= NSEC_PER_SEC; > > > + real_state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > > + > > > + /* > > > + * if eventually the PWM output is inactive, either > > > + * duty cycle is 0 or status is disabled, need to > > > + * make sure the output pin is inactive. > > > + */ > > > + if (!state->enabled) > > > + real_state->duty_cycle = 0; > > > + else > > > + real_state->duty_cycle = state->duty_cycle; > > > + > > > + tmp = (u64)p->mod * real_state->duty_cycle; > > > + p->val = DIV_ROUND_CLOSEST_ULL(tmp, real_state->period); > > > + > > > + real_state->polarity = state->polarity; > > > + real_state->enabled = state->enabled; > > > + > > > + return 0; > > > +} > > > + > > > +/* this function is supposed to be called with mutex hold */ static > > > +int pwm_imx_tpm_setup_period_duty(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct imx_tpm_pwm_param *p) > > > +{ > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + unsigned long timeout; > > > + u32 val, cmod, cur_prescale; > > > + > > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > > + cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > > > + cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val); > > > + if (cmod && cur_prescale != p->prescale) > > > + return -EBUSY; > > > + > > > + /* set TPM counter prescale */ > > > + val &= ~PWM_IMX_TPM_SC_PS; > > > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale); > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > + > > > + /* > > > + * set period count: > > > + * if (CMOD[1:0] = 0:0) then MOD register is updated when MOD > > > + * register is written. > > > + * > > > + * if (CMOD[1:0] ≠ 0:0), then MOD register is updated according > > > + * to the CPWMS bit, that is: > > > + * > > > + * if the selected mode is not CPWM then MOD register is updated > > > + * after MOD register was written and the TPM counter changes from > > > + * MOD to zero. > > > + * > > > + * if the selected mode is CPWM then MOD register is updated after > > > + * MOD register was written and the TPM counter changes from > > MOD > > > + * to (MOD – 1). > > > > Given that the driver doesn't make use of CPWM, this comment could be > > simplified. I'd write: > > > > /* > > * If the PWM is enabled (CMOD[1:0] ≠ 2b00), the period length > > * is latched into hardware when the next period starts. > > */ > > > > OK, thanks. > > > This is even true for the (here unused) CPWM mode. (The reference > > manual isn't entirely clear here IMHO. I assume if MOD == 4000 and CNT > > == 2001 then MOD is then changed to 2000, the currently running period > > is completed with a length of 4000 prescaled clk cycles?!) > > Based on my understanding from RM, I think so. > > > > > > + */ > > > + writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); > > > + > > > + /* > > > + * set channel value: > > > + * if (CMOD[1:0] = 0:0) then CnV register is updated when CnV > > > + * register is written. > > > + * > > > + * if (CMOD[1:0] ≠ 0:0), then CnV register is updated according > > > + * to the selected mode, that is: > > > + * > > > + * if the selected mode is output compare then CnV register is > > > + * updated on the next TPM counter increment (end of the prescaler > > > + * counting) after CnV register was written. > > > + * > > > + * if the selected mode is EPWM then CnV register is updated after > > > + * CnV register was written and the TPM counter changes from MOD > > > + * to zero. > > > + * > > > + * if the selected mode is CPWM then CnV register is updated after > > > + * CnV register was written and the TPM counter changes from MOD > > > + * to (MOD – 1). > > > > This is similar to the above too verbose and covers stuff that is not > > relevant for this driver. Also the used wording is not obvious if you > > don't look into the reference manual. > > > OK, thanks. > > > > > > + */ > > > + writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > + > > > + /* make sure MOD & CnV registers are updated */ > > > + timeout = jiffies + msecs_to_jiffies(tpm->real_period / > > > + NSEC_PER_MSEC + 1); > > > + while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod > > > + || readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) > > > + != p->val) { > > > + if (time_after(jiffies, timeout)) > > > + return -ETIME; > > > + cpu_relax(); > > > + } > > > + > > > + return 0; > > > +} > > > [...] > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + struct imx_tpm_pwm_param param; > > > + struct pwm_state real_state; > > > + int ret; > > > + > > > + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > > > + if (ret) > > > + return -EINVAL; > > > + > > > + mutex_lock(&tpm->lock); > > > + > > > + /* > > > + * TPM counter is shared by multiple channels, so > > > + * prescale and period can NOT be modified when > > > + * there are multiple channels in use with different > > > + * period settings. > > > + */ > > > + if (real_state.period != tpm->real_period) { > > > + if (tpm->user_count > 1) { > > > + ret = -EBUSY; > > > + goto exit; > > > + } > > > + > > > + ret = pwm_imx_tpm_setup_period_duty(chip, pwm, > > ¶m); > > > + if (ret) > > > + goto exit; > > > + > > > + tpm->real_period = real_state.period; > > > + } > > > + > > > + ret = pwm_imx_tpm_apply_hw(chip, pwm, &real_state); > > > > It's unintuitive here that both pwm_imx_tpm_setup_period_duty and > > pwm_imx_tpm_apply_hw (potentially) configure the duty cycle. I didn't > > thought it to an end, but maybe this could be optimised? > > > This is also my concern when implementing it, since period needs to be > configured before duty, and we want to put these 2 configurations as close > as possible, so for the period change, I also configure the duty together, but > for normal use cases, period does NOT change, so we also need to consider > duty change ONLY case, that is why I put a current duty and requested duty > check in the pwm_imx_tpm_apply_hw() function. > > if (state->duty_cycle != c.duty_cycle) { > /* set duty counter */ > tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > PWM_IMX_TPM_MOD_MOD; > tmp *= state->duty_cycle; > val = DIV_ROUND_CLOSEST_ULL(tmp, state->period); > writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > We can also put all the configurations together in 1 function, but that also > introduce some confusion I think, current implementation separate the > period settings (for all channels) and other settings (for each channel) in 2 > function, it is just because that the duty change is better to be put as close as > period change, so I did it this way. Maybe add some comments for it is > acceptable? I just sent out V10 patch set to remove the pwm_imx_tpm_setup_period_duty() function, and put all the configurations in pwm_imx_tpm_apply_hw() function, the defect introduced would be a slightly latency between period update and duty update, it is trivial I think, but it can avoid the duplicated code/function of setting duty. Thanks. Anson. > > > > > > > +exit: > > > + mutex_unlock(&tpm->lock); > > > + > > > + return ret; > > > +} > > > + > > > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct > > > +pwm_device *pwm) { > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + struct imx_tpm_pwm_channel *chan; > > > + > > > + chan = devm_kzalloc(chip->dev, sizeof(*chan), GFP_KERNEL); > > > > There is no advantage in using the devm variant here as the requested > > memory is freed in .free anyhow. So this only adds additional memory > > foodprint and runtime overhead. > > Makes sense, I will use kzalloc directly, looks like other pwm > driver(drivers/pwm/pwm-samsung.c) also has such "issue". > > > > > > + if (!chan) > > > + return -ENOMEM; > > > + > > > + pwm_set_chip_data(pwm, chan); > > > + > > > + mutex_lock(&tpm->lock); > > > + tpm->user_count++; > > > + mutex_unlock(&tpm->lock); > > > + > > > + return 0; > > > +} > > > + > > > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct > > pwm_device > > > +*pwm) { > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > + > > > + mutex_lock(&tpm->lock); > > > + tpm->user_count--; > > > + mutex_unlock(&tpm->lock); > > > + > > > + devm_kfree(chip->dev, pwm_get_chip_data(pwm)); > > > + pwm_set_chip_data(pwm, NULL); > > > > The call to pwm_set_chip_data could better be done in the PWM core. > > You meant doing a new patch to add it in PWM core.c after ->free call? > If yes, then I think this should be another topic, as many other pwm drivers > also call it in their own driver, maybe it can be improved by another patch? > > Thanks, > Anson. > > > > > > +} > > > > -- > > Pengutronix e.K. | Uwe Kleine-König | > > Industrial Linux Solutions | > > > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > > > engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7Ca7 > > > 1937c9d5e84033309008d6b1048c3a%7C686ea1d3bc2b4c6fa92cd99c5c30163 > > > 5%7C0%7C0%7C636891030423087284&sdata=a3xsu9iaAGvfUYv%2FNo6 > > T5Uvw6k%2F5VbyI2cFzsrnA4IM%3D&reserved=0 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V9 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default 2019-03-22 1:47 [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support Anson Huang 2019-03-22 1:48 ` [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding Anson Huang 2019-03-22 1:48 ` [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support Anson Huang @ 2019-03-22 1:48 ` Anson Huang 2019-03-22 1:48 ` [PATCH V9 4/5] ARM: dts: imx7ulp: Add pwm0 support Anson Huang 2019-03-22 1:48 ` [PATCH V9 5/5] ARM: dts: imx7ulp-evk: Add backlight support Anson Huang 4 siblings, 0 replies; 14+ messages in thread From: Anson Huang @ 2019-03-22 1:48 UTC (permalink / raw) To: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam, linux, stefan, otavio, Leonard Crestez, Robin Gong, jan.tuerk, linux-pwm, devicetree, linux-arm-kernel, linux-kernel, u.kleine-koenig Cc: dl-linux-imx Select CONFIG_PWM_IMX_TPM by default to support i.MX7ULP TPM PWM. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- No changes. --- arch/arm/configs/imx_v6_v7_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig index 1872dac..2a35806 100644 --- a/arch/arm/configs/imx_v6_v7_defconfig +++ b/arch/arm/configs/imx_v6_v7_defconfig @@ -400,6 +400,7 @@ CONFIG_MPL3115=y CONFIG_PWM=y CONFIG_PWM_FSL_FTM=y CONFIG_PWM_IMX=y +CONFIG_PWM_IMX_TPM=y CONFIG_NVMEM_IMX_OCOTP=y CONFIG_NVMEM_VF610_OCOTP=y CONFIG_TEE=y -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V9 4/5] ARM: dts: imx7ulp: Add pwm0 support 2019-03-22 1:47 [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support Anson Huang ` (2 preceding siblings ...) 2019-03-22 1:48 ` [PATCH V9 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default Anson Huang @ 2019-03-22 1:48 ` Anson Huang 2019-03-22 1:48 ` [PATCH V9 5/5] ARM: dts: imx7ulp-evk: Add backlight support Anson Huang 4 siblings, 0 replies; 14+ messages in thread From: Anson Huang @ 2019-03-22 1:48 UTC (permalink / raw) To: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam, linux, stefan, otavio, Leonard Crestez, Robin Gong, jan.tuerk, linux-pwm, devicetree, linux-arm-kernel, linux-kernel, u.kleine-koenig Cc: dl-linux-imx Add i.MX7ULP EVK board PWM0 support. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- No changes; --- arch/arm/boot/dts/imx7ulp.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/boot/dts/imx7ulp.dtsi b/arch/arm/boot/dts/imx7ulp.dtsi index eb349fd..15d04fb 100644 --- a/arch/arm/boot/dts/imx7ulp.dtsi +++ b/arch/arm/boot/dts/imx7ulp.dtsi @@ -124,6 +124,16 @@ status = "disabled"; }; + pwm0: pwm@40250000 { + compatible = "fsl,imx-tpm"; + reg = <0x40250000 0x1000>; + assigned-clocks = <&pcc2 IMX7ULP_CLK_LPTPM4>; + assigned-clock-parents = <&scg1 IMX7ULP_CLK_SOSC_BUS_CLK>; + clocks = <&pcc2 IMX7ULP_CLK_LPTPM4>; + #pwm-cells = <3>; + status = "disabled"; + }; + tpm5: tpm@40260000 { compatible = "fsl,imx7ulp-tpm"; reg = <0x40260000 0x1000>; -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V9 5/5] ARM: dts: imx7ulp-evk: Add backlight support 2019-03-22 1:47 [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support Anson Huang ` (3 preceding siblings ...) 2019-03-22 1:48 ` [PATCH V9 4/5] ARM: dts: imx7ulp: Add pwm0 support Anson Huang @ 2019-03-22 1:48 ` Anson Huang 4 siblings, 0 replies; 14+ messages in thread From: Anson Huang @ 2019-03-22 1:48 UTC (permalink / raw) To: thierry.reding, robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam, linux, stefan, otavio, Leonard Crestez, Robin Gong, jan.tuerk, linux-pwm, devicetree, linux-arm-kernel, linux-kernel, u.kleine-koenig Cc: dl-linux-imx This patch adds i.MX7ULP EVK board MIPI-DSI backlight support. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- No changes. --- arch/arm/boot/dts/imx7ulp-evk.dts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/arch/arm/boot/dts/imx7ulp-evk.dts b/arch/arm/boot/dts/imx7ulp-evk.dts index a09026a..7c44ffa 100644 --- a/arch/arm/boot/dts/imx7ulp-evk.dts +++ b/arch/arm/boot/dts/imx7ulp-evk.dts @@ -8,6 +8,7 @@ /dts-v1/; #include "imx7ulp.dtsi" +#include <dt-bindings/pwm/pwm.h> / { model = "NXP i.MX7ULP EVK"; @@ -22,6 +23,14 @@ reg = <0x60000000 0x40000000>; }; + backlight { + compatible = "pwm-backlight"; + pwms = <&pwm0 1 50000 0>; + brightness-levels = <0 20 25 30 35 40 100>; + default-brightness-level = <6>; + status = "okay"; + }; + reg_vsd_3v3: regulator-vsd-3v3 { compatible = "regulator-fixed"; regulator-name = "VSD_3V3"; @@ -40,6 +49,12 @@ status = "okay"; }; +&pwm0 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_pwm0>; + status = "okay"; +}; + &usdhc0 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_usdhc0>; @@ -57,6 +72,12 @@ bias-pull-up; }; + pinctrl_pwm0: pwm0grp { + fsl,pins = < + IMX7ULP_PAD_PTF2__TPM4_CH1 0x2 + >; + }; + pinctrl_usdhc0: usdhc0grp { fsl,pins = < IMX7ULP_PAD_PTD1__SDHC0_CMD 0x43 -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-03-26 18:51 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-22 1:47 [PATCH V9 0/5] Add i.MX7ULP EVK PWM backlight support Anson Huang 2019-03-22 1:48 ` [PATCH V9 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding Anson Huang 2019-03-25 20:41 ` Rob Herring 2019-03-25 20:59 ` Uwe Kleine-König 2019-03-26 18:51 ` Rob Herring 2019-03-26 0:56 ` Anson Huang 2019-03-26 6:53 ` Anson Huang 2019-03-22 1:48 ` [PATCH V9 2/5] pwm: Add i.MX TPM PWM driver support Anson Huang 2019-03-25 9:30 ` Uwe Kleine-König 2019-03-25 11:58 ` Anson Huang 2019-03-26 6:56 ` Anson Huang 2019-03-22 1:48 ` [PATCH V9 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default Anson Huang 2019-03-22 1:48 ` [PATCH V9 4/5] ARM: dts: imx7ulp: Add pwm0 support Anson Huang 2019-03-22 1:48 ` [PATCH V9 5/5] ARM: dts: imx7ulp-evk: Add backlight support Anson Huang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).