[PATCH V7 0/5] Add i.MX7ULP EVK PWM backlight support

[PATCH V7 0/5] Add i.MX7ULP EVK PWM backlight support

[Anson Huang]

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 V6:
	- 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        |  19 +
 arch/arm/boot/dts/imx7ulp-evk.dts                  |  20 +
 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                          | 436 +++++++++++++++++++++
 7 files changed, 498 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/imx-tpm-pwm.txt
 create mode 100644 drivers/pwm/pwm-imx-tpm.c

-- 
2.7.4

[PATCH V7 1/5] dt-bindings: pwm: Add i.MX TPM PWM binding

[Anson Huang]

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

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

[Anson Huang]

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 V6:
	- merge "config" and "enable" functions into ONE function pwm_imx_tpm_apply_hw;
	- save computation for confiuring counter, the "round_state" function will return
	  the registers value directly;
	- improve the logic in .apply;
	- return error when there is still PWM active during suspend callback.
---
 drivers/pwm/Kconfig       |  11 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-imx-tpm.c | 428 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 440 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..02403d0
--- /dev/null
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -0,0 +1,428 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018-2019 NXP.
+ *
+ * Limitations:
+ * - The TPM counter and period counter are shared between
+ *   multiple channels, so all channels should use same period
+ *   settings.
+ */
+
+#include <linux/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	BIT(3)
+#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_MOD_MOD	GENMASK(15, 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;
+};
+
+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 = roundup_pow_of_two(clock_unit);
+		prescale = ilog2(prescale) - 16;
+	}
+
+	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
+		return -ERANGE;
+	p->prescale = prescale;
+
+	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
+	if (period_count > PWM_IMX_TPM_MOD_MOD)
+		return -ERANGE;
+	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 LOW, either
+	 * duty cycle is 0 or status is disabled, need
+	 * to make sure the output pin is LOW.
+	 */
+	if (!state->enabled)
+		real_state->duty_cycle = 0;
+	else
+		real_state->duty_cycle = state->duty_cycle;
+
+	real_state->polarity = state->polarity;
+	real_state->enabled = state->enabled;
+
+	return 0;
+}
+
+static void pwm_imx_tpm_config_counter(struct pwm_chip *chip,
+				       struct imx_tpm_pwm_param p)
+{
+	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+	u32 val, saved_cmod;
+
+	/* make sure counter is disabled for programming prescale */
+	val = readl(tpm->base + PWM_IMX_TPM_SC);
+	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
+	if (saved_cmod) {
+		val &= ~PWM_IMX_TPM_SC_CMOD;
+		writel(val, tpm->base + PWM_IMX_TPM_SC);
+	}
+
+	/* set TPM counter prescale */
+	val = readl(tpm->base + PWM_IMX_TPM_SC);
+	val &= ~PWM_IMX_TPM_SC_PS;
+	val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p.prescale);
+	writel(val, tpm->base + PWM_IMX_TPM_SC);
+
+	/*
+	 * set period count: according to RM, the MOD register is
+	 * updated immediately after CMOD[1:0] = 2b'00 above
+	 */
+	writel(p.mod, tpm->base + PWM_IMX_TPM_MOD);
+
+	/* restore the clock mode if necessary */
+	if (saved_cmod) {
+		val = readl(tpm->base + PWM_IMX_TPM_SC);
+		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
+		writel(val, tpm->base + PWM_IMX_TPM_SC);
+	}
+}
+
+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);
+	u32 rate, val;
+	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);
+	val = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
+	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
+	tmp *= (1 << val) * NSEC_PER_SEC;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+	/* get polarity */
+	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+	if (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;
+}
+
+static void 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 pwm_state c;
+	u32 val, sc_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));
+	}
+
+	if (state.enabled != c.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 = 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);
+		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
+		if (state.enabled) {
+			val |= PWM_IMX_TPM_CnSC_MSB;
+			val |= (state.polarity == PWM_POLARITY_NORMAL) ?
+				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
+				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
+			if (++tpm->enable_count == 1) {
+				/* start TPM counter */
+				sc_val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
+				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
+			}
+		} else {
+			if (--tpm->enable_count == 0) {
+				/* stop TPM counter */
+				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
+				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
+			}
+		}
+		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+	}
+}
+
+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, &param, 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;
+		}
+
+		pwm_imx_tpm_config_counter(chip, param);
+		tpm->real_period = real_state.period;
+	}
+
+	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);
+
+	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);
+}
+
+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;
+
+	if (tpm->enable_count == 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

[PATCH V7 3/5] ARM: imx_v6_v7_defconfig: Add TPM PWM support by default

[Anson Huang]

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 5586a50..57862c6 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -399,6 +399,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

[PATCH V7 4/5] ARM: dts: imx7ulp: Add pwm0 support

[Anson Huang]

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

[PATCH V7 5/5] ARM: dts: imx7ulp-evk: Add backlight support

[Anson Huang]

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

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

[Uwe Kleine-König]

Hello,

On Wed, Mar 20, 2019 at 05:06:21AM +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 V6:
> 	- merge "config" and "enable" functions into ONE function pwm_imx_tpm_apply_hw;
> 	- save computation for confiuring counter, the "round_state" function will return
> 	  the registers value directly;
> 	- improve the logic in .apply;
> 	- return error when there is still PWM active during suspend callback.
> ---
>  drivers/pwm/Kconfig       |  11 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-imx-tpm.c | 428 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 440 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..02403d0
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,428 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018-2019 NXP.
> + *
> + * Limitations:
> + * - The TPM counter and period counter are shared between
> + *   multiple channels, so all channels should use same period
> + *   settings.
> + */
> +
> +#include <linux/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	BIT(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_MOD_MOD	GENMASK(15, 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;
> +};
> +
> +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 = roundup_pow_of_two(clock_unit);
> +		prescale = ilog2(prescale) - 16;

This 16 should be a define to make it obvious where it comes from.

Maybe use:

	#define PWM_IMX_TPM_MOD_WIDTH	16
	#define PWM_IMX_TPM_MOD_MOD	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)

?

If clock_unit happens to be 0x10000, we end up with prescale = 0 which
is wrong. I think we need:

	} else {
		prescale = ilog2(clock_unit) + 1 - PWM_IMX_TPM_MOD_WIDTH;

without the roundup.

> +	}
> +
> +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> +		return -ERANGE;
> +	p->prescale = prescale;
> +
> +	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
> +	if (period_count > PWM_IMX_TPM_MOD_MOD)
> +		return -ERANGE;

This check (together with the right way to calculate prescale) is
theoretically superflous. period_count cannot be bigger than
PWM_IMX_TPM_MOD_MOD. I'd either drop that check or add a scary warning
to make people hitting this report the respective parameters.

> +	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 LOW, either
> +	 * duty cycle is 0 or status is disabled, need
> +	 * to make sure the output pin is LOW.

s/LOW/inactive/, then it's also true for inversed polarity.

> +	 */
> +	if (!state->enabled)
> +		real_state->duty_cycle = 0;
> +	else
> +		real_state->duty_cycle = state->duty_cycle;
> +
> +	real_state->polarity = state->polarity;
> +	real_state->enabled = state->enabled;
> +
> +	return 0;
> +}
> +
> +static void pwm_imx_tpm_config_counter(struct pwm_chip *chip,
> +				       struct imx_tpm_pwm_param p)

Please pass p as a pointer. I think pwm_imx_tpm_setup_period is a better
name here.

> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	u32 val, saved_cmod;
> +
> +	/* make sure counter is disabled for programming prescale */
> +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> +	if (saved_cmod) {
> +		val &= ~PWM_IMX_TPM_SC_CMOD;
> +		writel(val, tpm->base + PWM_IMX_TPM_SC);

I thought we agreed on not stopping the counter if the PS field isn't
changed?

> +	}
> +
> +	/* set TPM counter prescale */
> +	val = readl(tpm->base + PWM_IMX_TPM_SC);

val already holds this register value, no need to reread.

> +	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: according to RM, the MOD register is
> +	 * updated immediately after CMOD[1:0] = 2b'00 above
> +	 */

So the current period isn't completed? That's wrong.

> +	writel(p.mod, tpm->base + PWM_IMX_TPM_MOD);
> +
> +	/* restore the clock mode if necessary */
> +	if (saved_cmod) {
> +		val = readl(tpm->base + PWM_IMX_TPM_SC);
> +		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> +	}
> +}
> +
> +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);
> +	u32 rate, val;
> +	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);
> +	val = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> +	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> +	tmp *= (1 << val) * NSEC_PER_SEC;

"prescale" would be a better name here than "val". As in the previous
review round: Don't multiply by (1 << something).

> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> +	/* get polarity */
> +	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;
> +}
> +
> +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> +				 struct pwm_device *pwm,
> +				 struct pwm_state state)

Please pass a the struct pwm_state as a pointer, not as a value.

> +{
> +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> +	struct pwm_state c;
> +	u32 val, sc_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));
> +	}
> +
> +	if (state.enabled != c.enabled) {

This is wrong. If the PWM was running (c.enabled == true) and you are
supposed to disable (state.enabled == false) you enable the hardware
once more.

> +		/*
> +		 * 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 = 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);
> +		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
> +		if (state.enabled) {
> +			val |= PWM_IMX_TPM_CnSC_MSB;
> +			val |= (state.polarity == PWM_POLARITY_NORMAL) ?
> +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);

Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it together with
PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you put the FIELD_PREP
into the definition the line doesn't get excessively long.

Maybe also add

	#define PWM_IMX_TPM_CnSC_ELS_INACTIVE FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0)

?

> +			if (++tpm->enable_count == 1) {
> +				/* start TPM counter */
> +				sc_val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> +				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
> +			}
> +		} else {
> +			if (--tpm->enable_count == 0) {
> +				/* stop TPM counter */
> +				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
> +				writel(sc_val, tpm->base + PWM_IMX_TPM_SC);
> +			}
> +		}
> +		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +	}
> +}
> +
> +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, &param, 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;
> +		}
> +
> +		pwm_imx_tpm_config_counter(chip, param);
> +		tpm->real_period = real_state.period;
> +	}

Maybe add a comment that this could still be optimized. For example if
pwm_imx_tpm_round_state returned prescale = 5 but prescale is currently
6, you might still be able to configure

> +
> +	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);
> +
> +	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);
> +}
> +
> +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;
> +
> +	if (tpm->enable_count == 0) {

tpm->enable_count cannot be different from 0 here.

> +		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");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Anson Huang]

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月20日 16:19
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux@armlinux.org.uk; otavio@ossystems.com.br;
> stefan@agner.ch; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; Robin Gong <yibin.gong@nxp.com>; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hello,
> 
> On Wed, Mar 20, 2019 at 05:06:21AM +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 V6:
> > 	- merge "config" and "enable" functions into ONE function
> pwm_imx_tpm_apply_hw;
> > 	- save computation for confiuring counter, the "round_state"
> function will return
> > 	  the registers value directly;
> > 	- improve the logic in .apply;
> > 	- return error when there is still PWM active during suspend callback.
> > ---
> >  drivers/pwm/Kconfig       |  11 ++
> >  drivers/pwm/Makefile      |   1 +
> >  drivers/pwm/pwm-imx-tpm.c | 428
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 440 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..02403d0
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,428 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018-2019 NXP.
> > + *
> > + * Limitations:
> > + * - The TPM counter and period counter are shared between
> > + *   multiple channels, so all channels should use same period
> > + *   settings.
> > + */
> > +
> > +#include <linux/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	BIT(3)
> 
> #define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK
> 	FIELD_PREP(PWM_IMX_TPM_SC_CMOD, 1)
> 
> ?

Yes, it makes more sense.

> 
> > +#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_MOD_MOD	GENMASK(15, 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;
> > +};
> > +
> > +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 = roundup_pow_of_two(clock_unit);
> > +		prescale = ilog2(prescale) - 16;
> 
> This 16 should be a define to make it obvious where it comes from.
> 
> Maybe use:
> 
> 	#define PWM_IMX_TPM_MOD_WIDTH	16
> 	#define PWM_IMX_TPM_MOD_MOD
> 	GENMASK(PWM_IMX_TPM_MOD_WIDTH - 1, 0)
> 
> ?
> 
> If clock_unit happens to be 0x10000, we end up with prescale = 0 which is
> wrong. I think we need:
> 
> 	} else {
> 		prescale = ilog2(clock_unit) + 1 -
> PWM_IMX_TPM_MOD_WIDTH;
> 
> without the roundup.


OK.

> 
> > +	}
> > +
> > +	if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > +		return -ERANGE;
> > +	p->prescale = prescale;
> > +
> > +	period_count = (clock_unit + ((1 << prescale) >> 1)) >> prescale;
> > +	if (period_count > PWM_IMX_TPM_MOD_MOD)
> > +		return -ERANGE;
> 
> This check (together with the right way to calculate prescale) is theoretically
> superflous. period_count cannot be bigger than
> PWM_IMX_TPM_MOD_MOD. I'd either drop that check or add a scary
> warning to make people hitting this report the respective parameters.

Yeah, the check for the prescale is enough, I will drop this check.

> 
> > +	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 LOW, either
> > +	 * duty cycle is 0 or status is disabled, need
> > +	 * to make sure the output pin is LOW.
> 
> s/LOW/inactive/, then it's also true for inversed polarity.

OK.

> 
> > +	 */
> > +	if (!state->enabled)
> > +		real_state->duty_cycle = 0;
> > +	else
> > +		real_state->duty_cycle = state->duty_cycle;
> > +
> > +	real_state->polarity = state->polarity;
> > +	real_state->enabled = state->enabled;
> > +
> > +	return 0;
> > +}
> > +
> > +static void pwm_imx_tpm_config_counter(struct pwm_chip *chip,
> > +				       struct imx_tpm_pwm_param p)
> 
> Please pass p as a pointer. I think pwm_imx_tpm_setup_period is a better
> name here.

OK.


> 
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	u32 val, saved_cmod;
> > +
> > +	/* make sure counter is disabled for programming prescale */
> > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > +	if (saved_cmod) {
> > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> 
> I thought we agreed on not stopping the counter if the PS field isn't changed?

If the PS field no need to change, the round state should already return the period
equal to current period settings, so this function will NOT be called, right?

         if (real_state.period != tpm->real_period) {
                 if (tpm->user_count > 1) {
                         ret = -EBUSY;
                         goto exit;
                 }

	pwm_imx_tpm_setup_period(chip, param);
                 tpm->real_period = real_state.period;
        } 

> 
> > +	}
> > +
> > +	/* set TPM counter prescale */
> > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> 
> val already holds this register value, no need to reread.

OK.

> 
> > +	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: according to RM, the MOD register is
> > +	 * updated immediately after CMOD[1:0] = 2b'00 above
> > +	 */
> 
> So the current period isn't completed? That's wrong.

So you mean we have to wait for the current period to finish here?
I did NOT know this, so we have to compare the counter value with
the MOD value until they match then proceed the period change?

> 
> > +	writel(p.mod, tpm->base + PWM_IMX_TPM_MOD);
> > +
> > +	/* restore the clock mode if necessary */
> > +	if (saved_cmod) {
> > +		val = readl(tpm->base + PWM_IMX_TPM_SC);
> > +		val |= FIELD_PREP(PWM_IMX_TPM_SC_CMOD, saved_cmod);
> > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > +	}
> > +}
> > +
> > +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);
> > +	u32 rate, val;
> > +	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);
> > +	val = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > +	tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > +	tmp *= (1 << val) * NSEC_PER_SEC;
> 
> "prescale" would be a better name here than "val". As in the previous review
> round: Don't multiply by (1 << something).

OK, I missed it here. Will use below:

	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 */
> > +	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; }
> > +
> > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > +				 struct pwm_device *pwm,
> > +				 struct pwm_state state)
> 
> Please pass a the struct pwm_state as a pointer, not as a value.

OK.

> 
> > +{
> > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > +	struct pwm_state c;
> > +	u32 val, sc_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));
> > +	}
> > +
> > +	if (state.enabled != c.enabled) {
> 
> This is wrong. If the PWM was running (c.enabled == true) and you are
> supposed to disable (state.enabled == false) you enable the hardware once
> more.

A little confused here, as the case you assume, inside this block, there is another
check for state.enabled, if it is false, the polarity will be set to channel disabled mode,
the polarity setting is combined together with the enable status here, am I wrong?  


> 
> > +		/*
> > +		 * 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 = 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);
> > +		sc_val = readl(tpm->base + PWM_IMX_TPM_SC);
> > +		if (state.enabled) {
> > +			val |= PWM_IMX_TPM_CnSC_MSB;
> > +			val |= (state.polarity == PWM_POLARITY_NORMAL) ?
> > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> 
> Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it
> together with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you
> put the FIELD_PREP into the definition the line doesn't get excessively long.
> 

I put the FIELD_PREP into definition, the line still long, but I agree using definition is better.

#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)

                         val |= (state->polarity == PWM_POLARITY_NORMAL) ?
                                 PWM_IMX_TPM_CnSC_ELS_NORMAL :
                                 PWM_IMX_TPM_CnSC_ELS_INVERSED;

> Maybe also add
> 
> 	#define PWM_IMX_TPM_CnSC_ELS_INACTIVE
> FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0)
> 

I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why add it?
I don't quite understand.

> ?
> 
> > +			if (++tpm->enable_count == 1) {
> > +				/* start TPM counter */
> > +				sc_val |=
> PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > +				writel(sc_val, tpm->base +
> PWM_IMX_TPM_SC);
> > +			}
> > +		} else {
> > +			if (--tpm->enable_count == 0) {
> > +				/* stop TPM counter */
> > +				sc_val &= ~PWM_IMX_TPM_SC_CMOD;
> > +				writel(sc_val, tpm->base +
> PWM_IMX_TPM_SC);
> > +			}
> > +		}
> > +		writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +	}
> > +}
> > +
> > +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, &param, 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;
> > +		}
> > +
> > +		pwm_imx_tpm_config_counter(chip, param);
> > +		tpm->real_period = real_state.period;
> > +	}
> 
> Maybe add a comment that this could still be optimized. For example if
> pwm_imx_tpm_round_state returned prescale = 5 but prescale is currently 6,
> you might still be able to configure

You meant for multiple users request different period case? In this block, if there is
ONLY one user and the requested period can be met by HW, it will anyway re-configure
the HW for the prescale and period I think, or I did NOT get your point?

> 
> > +
> > +	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);
> > +
> > +	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);
> > +}
> > +
> > +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;
> > +
> > +	if (tpm->enable_count == 0) {
> 
> tpm->enable_count cannot be different from 0 here.

OK, I will remove the check.

> 
> > +		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");
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C54
> a41e6491e947e92bc108d6ad0cbd6e%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636886667561952278&amp;sdata=Mm8k5iYRHRCNLgDtZZIM
> O7sssx71YUaOAbE9JEp7zgE%3D&amp;reserved=0  |

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

[Uwe Kleine-König]

Hello Anson,

On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote:
> > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote:
> > > +	/* make sure counter is disabled for programming prescale */
> > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > +	if (saved_cmod) {
> > > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > 
> > I thought we agreed on not stopping the counter if the PS field isn't changed?
> 
> If the PS field no need to change, the round state should already return the period
> equal to current period settings, so this function will NOT be called, right?
> 
>          if (real_state.period != tpm->real_period) {
>                  if (tpm->user_count > 1) {
>                          ret = -EBUSY;
>                          goto exit;
>                  }
> 
> 	pwm_imx_tpm_setup_period(chip, param);
>                  tpm->real_period = real_state.period;
>         } 

If the PS field is already right .period might still not match as its
value depends on SC.PS and MOD.MOD.

> > > +	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: according to RM, the MOD register is
> > > +	 * updated immediately after CMOD[1:0] = 2b'00 above
> > > +	 */
> > 
> > So the current period isn't completed? That's wrong.
> 
> So you mean we have to wait for the current period to finish here?
> I did NOT know this, so we have to compare the counter value with

Yeah, see https://patchwork.ozlabs.org/patch/1021566/ which documents
this but waits for feedback by Thierry since some time.

> the MOD value until they match then proceed the period change?

If the hardware doesn't support you here (usually it does) I think it's
not reliable enough to try to sync in software. I assume you can get the
right wave form if you don't change SC.PS but only MOD.MOD? So maybe the
sanest approach is to refuse changing SC.PS if the PWM is running.

Or disable (which also should wait for the current period to finish),
poll for the end (or use an irq?) and then setup the new configuration.

> > > +{
> > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > +	struct pwm_state c;
> > > +	u32 val, sc_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));
> > > +	}
> > > +
> > > +	if (state.enabled != c.enabled) {
> > 
> > This is wrong. If the PWM was running (c.enabled == true) and you are
> > supposed to disable (state.enabled == false) you enable the hardware once
> > more.
> 
> A little confused here, as the case you assume, inside this block, there is another
> check for state.enabled, if it is false, the polarity will be set to channel disabled mode,
> the polarity setting is combined together with the enable status here, am I wrong?  

Ah, you're right. I missed that probably because I saw register accesses
after the state.enabled != c.enabled check.

> > > +			val |= (state.polarity == PWM_POLARITY_NORMAL) ?
> > > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > 
> > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it
> > together with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you
> > put the FIELD_PREP into the definition the line doesn't get excessively long.
> > 
> 
> I put the FIELD_PREP into definition, the line still long, but I agree using definition is better.
> 
> #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)
> 
>                          val |= (state->polarity == PWM_POLARITY_NORMAL) ?
>                                  PWM_IMX_TPM_CnSC_ELS_NORMAL :
>                                  PWM_IMX_TPM_CnSC_ELS_INVERSED;
> 
> > Maybe also add
> > 
> > 	#define PWM_IMX_TPM_CnSC_ELS_INACTIVE FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0)
> > 
> 
> I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why add it?
> I don't quite understand.

You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled ==
false and c.enabled == true:

	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
	val &= ~(PWM_IMX_TPM_CnSC_ELS | ...);
	...
	writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));

> > > +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, &param, 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;
> > > +		}
> > > +
> > > +		pwm_imx_tpm_config_counter(chip, param);
> > > +		tpm->real_period = real_state.period;
> > > +	}
> > 
> > Maybe add a comment that this could still be optimized. For example if
> > pwm_imx_tpm_round_state returned prescale = 5 but prescale is currently 6,
> > you might still be able to configure
> 
> You meant for multiple users request different period case? In this block, if there is
> ONLY one user and the requested period can be met by HW, it will anyway re-configure
> the HW for the prescale and period I think, or I did NOT get your point?

My idea has a flaw. I thought that if there is another user, the
duty_cycle can still be represented if the actually used prescale value
is slightly higher. But then there is still a problem with the period
length that I missed. So my remark was wrong, sorry for that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Anson Huang]

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2019年3月20日 18:58
> To: Anson Huang <anson.huang@nxp.com>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux@armlinux.org.uk; otavio@ossystems.com.br;
> stefan@agner.ch; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; Robin Gong <yibin.gong@nxp.com>; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support
> 
> Hello Anson,
> 
> On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote:
> > > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote:
> > > > +	/* make sure counter is disabled for programming prescale */
> > > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > > +	if (saved_cmod) {
> > > > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > >
> > > I thought we agreed on not stopping the counter if the PS field isn't
> changed?
> >
> > If the PS field no need to change, the round state should already
> > return the period equal to current period settings, so this function will NOT
> be called, right?
> >
> >          if (real_state.period != tpm->real_period) {
> >                  if (tpm->user_count > 1) {
> >                          ret = -EBUSY;
> >                          goto exit;
> >                  }
> >
> > 	pwm_imx_tpm_setup_period(chip, param);
> >                  tpm->real_period = real_state.period;
> >         }
> 
> If the PS field is already right .period might still not match as its value
> depends on SC.PS and MOD.MOD.

Ah, my bad... I did NOT know what I was thinking...
Yes, I will add the PS check to decide whether disabling counter..


> 
> > > > +	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: according to RM, the MOD register is
> > > > +	 * updated immediately after CMOD[1:0] = 2b'00 above
> > > > +	 */
> > >
> > > So the current period isn't completed? That's wrong.
> >
> > So you mean we have to wait for the current period to finish here?
> > I did NOT know this, so we have to compare the counter value with
> 
> Yeah, see
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fpatch%2F1021566%2F&amp;data=02%7C01%7Canson.h
> uang%40nxp.com%7C626a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636886762757876916&amp;sdata=r
> wu%2BUo3GlRX8j4lXSOVuAs7n1yEuP5P2W6vhY%2BjiXdQ%3D&amp;reserve
> d=0 which documents this but waits for feedback by Thierry since some time.
> 
> > the MOD value until they match then proceed the period change?
> 
> If the hardware doesn't support you here (usually it does) I think it's not
> reliable enough to try to sync in software. I assume you can get the right
> wave form if you don't change SC.PS but only MOD.MOD? So maybe the
> sanest approach is to refuse changing SC.PS if the PWM is running.
> 
> Or disable (which also should wait for the current period to finish), poll for
> the end (or use an irq?) and then setup the new configuration.

Let me try to poll the TOF (timer overflow) before setup the new configuration.
And will also need to add timeout for the polling, what shoud the timeout value
be, 100ms? As ideally the max period can be very large, several seconds or even
large, so is the 100mS good here? 

> 
> > > > +{
> > > > +	struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > +	struct pwm_state c;
> > > > +	u32 val, sc_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));
> > > > +	}
> > > > +
> > > > +	if (state.enabled != c.enabled) {
> > >
> > > This is wrong. If the PWM was running (c.enabled == true) and you
> > > are supposed to disable (state.enabled == false) you enable the
> > > hardware once more.
> >
> > A little confused here, as the case you assume, inside this block,
> > there is another check for state.enabled, if it is false, the polarity
> > will be set to channel disabled mode, the polarity setting is combined
> together with the enable status here, am I wrong?
> 
> Ah, you're right. I missed that probably because I saw register accesses after
> the state.enabled != c.enabled check.
> 
> > > > +			val |= (state.polarity == PWM_POLARITY_NORMAL) ?
> > > > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > > > +				FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > >
> > > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it
> together
> > > with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you put
> the
> > > FIELD_PREP into the definition the line doesn't get excessively long.
> > >
> >
> > I put the FIELD_PREP into definition, the line still long, but I agree using
> definition is better.
> >
> > #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)
> >
> >                          val |= (state->polarity == PWM_POLARITY_NORMAL) ?
> >                                  PWM_IMX_TPM_CnSC_ELS_NORMAL :
> >                                  PWM_IMX_TPM_CnSC_ELS_INVERSED;
> >
> > > Maybe also add
> > >
> > > 	#define PWM_IMX_TPM_CnSC_ELS_INACTIVE
> > > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0)
> > >
> >
> > I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why
> add it?
> > I don't quite understand.
> 
> You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled == false
> and c.enabled == true:
> 
> 	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> 	val &= ~(PWM_IMX_TPM_CnSC_ELS | ...);
> 	...
> 	writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));

Ah, OK, I can replace the register field clear with the field prepare definition.

> 
> > > > +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, &param, 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;
> > > > +		}
> > > > +
> > > > +		pwm_imx_tpm_config_counter(chip, param);
> > > > +		tpm->real_period = real_state.period;
> > > > +	}
> > >
> > > Maybe add a comment that this could still be optimized. For example
> > > if pwm_imx_tpm_round_state returned prescale = 5 but prescale is
> > > currently 6, you might still be able to configure
> >
> > You meant for multiple users request different period case? In this
> > block, if there is ONLY one user and the requested period can be met
> > by HW, it will anyway re-configure the HW for the prescale and period I
> think, or I did NOT get your point?
> 
> My idea has a flaw. I thought that if there is another user, the duty_cycle can
> still be represented if the actually used prescale value is slightly higher. But
> then there is still a problem with the period length that I missed. So my
> remark was wrong, sorry for that.

Thanks,
Anson

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

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

[Anson Huang]

Hi, Uwe

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2019年3月20日 19:21
> To: 'Uwe Kleine-König' <u.kleine-koenig@pengutronix.de>
> Cc: thierry.reding@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; linux@armlinux.org.uk; otavio@ossystems.com.br;
> stefan@agner.ch; Leonard Crestez <leonard.crestez@nxp.com>;
> schnitzeltony@gmail.com; Robin Gong <yibin.gong@nxp.com>; linux-
> pwm@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH V7 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月20日 18:58
> > To: Anson Huang <anson.huang@nxp.com>
> > Cc: thierry.reding@gmail.com; robh+dt@kernel.org;
> > mark.rutland@arm.com; shawnguo@kernel.org; s.hauer@pengutronix.de;
> > kernel@pengutronix.de; festevam@gmail.com; linux@armlinux.org.uk;
> > otavio@ossystems.com.br; stefan@agner.ch; Leonard Crestez
> > <leonard.crestez@nxp.com>; schnitzeltony@gmail.com; Robin Gong
> > <yibin.gong@nxp.com>; linux- pwm@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support
> >
> > Hello Anson,
> >
> > On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote:
> > > > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote:
> > > > > +	/* make sure counter is disabled for programming prescale
> */
> > > > > +	val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > > > +	saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > > > +	if (saved_cmod) {
> > > > > +		val &= ~PWM_IMX_TPM_SC_CMOD;
> > > > > +		writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > >
> > > > I thought we agreed on not stopping the counter if the PS field
> > > > isn't
> > changed?
> > >
> > > If the PS field no need to change, the round state should already
> > > return the period equal to current period settings, so this function
> > > will NOT
> > be called, right?
> > >
> > >          if (real_state.period != tpm->real_period) {
> > >                  if (tpm->user_count > 1) {
> > >                          ret = -EBUSY;
> > >                          goto exit;
> > >                  }
> > >
> > > 	pwm_imx_tpm_setup_period(chip, param);
> > >                  tpm->real_period = real_state.period;
> > >         }
> >
> > If the PS field is already right .period might still not match as its
> > value depends on SC.PS and MOD.MOD.
> 
> Ah, my bad... I did NOT know what I was thinking...
> Yes, I will add the PS check to decide whether disabling counter..

I added below additional check for current PS and the new PS

         cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
         if (saved_cmod && cur_prescale != p->prescale) {
                 val &= ~PWM_IMX_TPM_SC_CMOD;
                 writel(val, tpm->base + PWM_IMX_TPM_SC);
         }


> 
> 
> >
> > > > > +	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: according to RM, the MOD register is
> > > > > +	 * updated immediately after CMOD[1:0] = 2b'00 above
> > > > > +	 */
> > > >
> > > > So the current period isn't completed? That's wrong.
> > >
> > > So you mean we have to wait for the current period to finish here?
> > > I did NOT know this, so we have to compare the counter value with
> >
> > Yeah, see
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > h
> work.ozlabs.org%2Fpatch%2F1021566%2F&amp;data=02%7C01%7Canson.h
> >
> uang%40nxp.com%7C626a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc
> >
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636886762757876916&amp;sdata=r
> >
> wu%2BUo3GlRX8j4lXSOVuAs7n1yEuP5P2W6vhY%2BjiXdQ%3D&amp;reserve
> > d=0 which documents this but waits for feedback by Thierry since some
> time.
> >
> > > the MOD value until they match then proceed the period change?
> >
> > If the hardware doesn't support you here (usually it does) I think
> > it's not reliable enough to try to sync in software. I assume you can
> > get the right wave form if you don't change SC.PS but only MOD.MOD? So
> > maybe the sanest approach is to refuse changing SC.PS if the PWM is
> running.
> >
> > Or disable (which also should wait for the current period to finish),
> > poll for the end (or use an irq?) and then setup the new configuration.
> 
> Let me try to poll the TOF (timer overflow) before setup the new
> configuration.
> And will also need to add timeout for the polling, what shoud the timeout
> value be, 100ms? As ideally the max period can be very large, several
> seconds or even large, so is the 100mS good here?

After further check, the reference manual has below statement, so I think we no
need to care about it, the hardware make sure of that, I added below comment
before programming the MOD register, if counter is disabled, the MOD register
will be updated immediately, if counter is enabled, the CPWM bit is fixed as 0 in
our driver, so the MOD register will be updated when counter changes from MOD
to zero.

         /*
          * set period count: according to RM, the MOD register is
          * updated immediately if CMOD[1:0] = 2b'00.
          * if CMOD[1:0] != 2b'00, 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).
          */



> 
> >
> > > > > +{
> > > > > +	struct imx_tpm_pwm_chip *tpm =
> to_imx_tpm_pwm_chip(chip);
> > > > > +	struct pwm_state c;
> > > > > +	u32 val, sc_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));
> > > > > +	}
> > > > > +
> > > > > +	if (state.enabled != c.enabled) {
> > > >
> > > > This is wrong. If the PWM was running (c.enabled == true) and you
> > > > are supposed to disable (state.enabled == false) you enable the
> > > > hardware once more.
> > >
> > > A little confused here, as the case you assume, inside this block,
> > > there is another check for state.enabled, if it is false, the
> > > polarity will be set to channel disabled mode, the polarity setting
> > > is combined
> > together with the enable status here, am I wrong?
> >
> > Ah, you're right. I missed that probably because I saw register
> > accesses after the state.enabled != c.enabled check.
> >
> > > > > +			val |= (state.polarity ==
> PWM_POLARITY_NORMAL) ?
> > > > > +
> 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > > > > +
> 	FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > > >
> > > > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it
> > together
> > > > with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you put
> > the
> > > > FIELD_PREP into the definition the line doesn't get excessively long.
> > > >
> > >
> > > I put the FIELD_PREP into definition, the line still long, but I
> > > agree using
> > definition is better.
> > >
> > > #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)
> > >
> > >                          val |= (state->polarity == PWM_POLARITY_NORMAL) ?
> > >                                  PWM_IMX_TPM_CnSC_ELS_NORMAL :
> > >                                  PWM_IMX_TPM_CnSC_ELS_INVERSED;
> > >
> > > > Maybe also add
> > > >
> > > > 	#define PWM_IMX_TPM_CnSC_ELS_INACTIVE
> > > > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0)
> > > >
> > >
> > > I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so
> why
> > add it?
> > > I don't quite understand.
> >
> > You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled ==
> > false and c.enabled == true:

But the place I used is just to clear the PWM_IMX_TPM_CnSC_ELS field, so
just the MASK is enough for me, if you don't mind, I will leave it as what it is now.

Anson.

> >
> > 	val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > 	val &= ~(PWM_IMX_TPM_CnSC_ELS | ...);
> > 	...
> > 	writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> 
> Ah, OK, I can replace the register field clear with the field prepare definition.
> 
> >
> > > > > +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, &param, 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;
> > > > > +		}
> > > > > +
> > > > > +		pwm_imx_tpm_config_counter(chip, param);
> > > > > +		tpm->real_period = real_state.period;
> > > > > +	}
> > > >
> > > > Maybe add a comment that this could still be optimized. For
> > > > example if pwm_imx_tpm_round_state returned prescale = 5 but
> > > > prescale is currently 6, you might still be able to configure
> > >
> > > You meant for multiple users request different period case? In this
> > > block, if there is ONLY one user and the requested period can be met
> > > by HW, it will anyway re-configure the HW for the prescale and
> > > period I
> > think, or I did NOT get your point?
> >
> > My idea has a flaw. I thought that if there is another user, the
> > duty_cycle can still be represented if the actually used prescale
> > value is slightly higher. But then there is still a problem with the
> > period length that I missed. So my remark was wrong, sorry for that.
> 
> Thanks,
> Anson
> 
> >
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 |
> >
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> >
> engutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C62
> >
> 6a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc2b4c6fa92cd99c5c30163
> >
> 5%7C0%7C0%7C636886762757876916&amp;sdata=JsNRa8DuGYizE7FCyHVuY
> > QSUu4eUu5qTh6Edpf3Azm8%3D&amp;reserved=0  |