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.
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
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 <[email protected]>
---
Changes since V5:
- improve commit message body;
- add period round function;
- use per channel data for saving channel's private data;
- improve error message output during probe;
- improve different period settings by different channels' handling;
- support #pwm-cells 3 cases.
---
drivers/pwm/Kconfig | 11 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-imx-tpm.c | 463 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 475 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..bb6b27e
--- /dev/null
+++ b/drivers/pwm/pwm-imx-tpm.c
@@ -0,0 +1,463 @@
+// 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)
+#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) /* combine ELSA and ELSB as a field */
+
+#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
+
+#define PWM_IMX_TPM_MAX_COUNT 0xffff
+
+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_channel {
+ u32 config;
+ bool status;
+};
+
+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 unsigned int pwm_imx_tpm_round_period(struct pwm_chip *chip,
+ u32 period)
+{
+ struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+ u32 rate, real_period, prescale, period_count;
+ u64 tmp;
+
+ rate = clk_get_rate(tpm->clk);
+ tmp = period;
+ tmp *= rate;
+ tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
+ if (tmp <= PWM_IMX_TPM_MAX_COUNT) {
+ prescale = 0;
+ } else {
+ prescale = roundup_pow_of_two(tmp /
+ (PWM_IMX_TPM_MAX_COUNT + 1));
+ prescale = ilog2(prescale);
+ }
+
+ /* if no valid prescale found, use MAX instead */
+ if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
+ prescale = PWM_IMX_TPM_SC_PS >> __bf_shf(PWM_IMX_TPM_SC_PS);
+
+ /* if no valid period count found, use MAX instead */
+ period_count = (tmp + ((1 << prescale) >> 1)) >> prescale;
+ if (period_count > PWM_IMX_TPM_MOD_MOD)
+ period_count = PWM_IMX_TPM_MOD_MOD;
+
+ /* calculate real period HW can support */
+ tmp = period_count;
+ tmp *= (1 << prescale) * NSEC_PER_SEC;
+ real_period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+ return real_period;
+}
+
+static void pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 period)
+{
+ struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+ u32 val, rate, prescale, saved_cmod;
+ u64 tmp;
+
+ rate = clk_get_rate(tpm->clk);
+ tmp = period;
+ tmp *= rate;
+ tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
+ if (tmp <= PWM_IMX_TPM_MAX_COUNT) {
+ prescale = 0;
+ } else {
+ prescale = roundup_pow_of_two(tmp /
+ (PWM_IMX_TPM_MAX_COUNT + 1));
+ prescale = ilog2(prescale);
+ }
+
+ /* 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, 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
+ */
+ val = (tmp + ((1 << prescale) >> 1)) >> prescale;
+ writel(val, 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);
+ }
+
+ tpm->real_period = period;
+}
+
+static void pwm_imx_tpm_config(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ u32 period,
+ u32 duty_cycle,
+ enum pwm_polarity polarity)
+{
+ struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+ struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
+ u32 val;
+ u64 tmp;
+
+ /* set duty counter */
+ tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
+ tmp *= duty_cycle;
+ val = DIV_ROUND_CLOSEST_ULL(tmp, period);
+ writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
+
+ /*
+ * 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.
+ */
+ val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+ val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA);
+ val |= PWM_IMX_TPM_CnSC_MSB;
+ val |= (polarity == PWM_POLARITY_NORMAL) ?
+ FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
+ FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
+ /*
+ * polarity settings will enabled/disable output status
+ * immediately, so here ONLY save the config and write
+ * it into register when channel is enabled/disabled.
+ */
+ chan->config = val;
+}
+
+/*
+ * When a channel's polarity is configured, the polarity settings
+ * will be saved and ONLY write into the register when the channel
+ * is enabled.
+ *
+ * When a channel is disabled, its polarity settings will be saved
+ * and its output will be disabled by clearing polarity settings.
+ *
+ * When a channel is enabled, its polarity settings will be restored
+ * and output will be enabled again.
+ */
+static void pwm_imx_tpm_enable(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ bool enable)
+{
+ struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
+ struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
+ u32 val;
+
+ val = readl(tpm->base + PWM_IMX_TPM_SC);
+ if (enable) {
+ /* restore channel config */
+ writel(chan->config,
+ tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+
+ if (++tpm->enable_count == 1) {
+ /* start TPM counter */
+ val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
+ writel(val, tpm->base + PWM_IMX_TPM_SC);
+ }
+ } else {
+ /* disable channel */
+ val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+ val &= ~(PWM_IMX_TPM_CnSC_MSA | PWM_IMX_TPM_CnSC_MSB |
+ PWM_IMX_TPM_CnSC_ELS);
+ writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
+
+ if (--tpm->enable_count == 0) {
+ /* stop TPM counter since all channels are disabled */
+ val &= ~PWM_IMX_TPM_SC_CMOD;
+ writel(val, tpm->base + PWM_IMX_TPM_SC);
+ }
+ }
+
+ /* update channel status */
+ chan->status = enable;
+}
+
+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) == 0x1)
+ state->polarity = PWM_POLARITY_INVERSED;
+ else if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x2)
+ state->polarity = PWM_POLARITY_NORMAL;
+
+ /* get channel status */
+ state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
+}
+
+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_channel *chan = pwm_get_chip_data(pwm);
+ u32 p;
+
+ mutex_lock(&tpm->lock);
+
+ if (state->period != tpm->real_period) {
+ /*
+ * 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.
+ */
+ p = pwm_imx_tpm_round_period(chip, state->period);
+ if (p != tpm->real_period && tpm->user_count != 1)
+ return -EBUSY;
+ else if (p != tpm->real_period)
+ pwm_imx_tpm_config_counter(chip, p);
+ }
+
+ if (state->enabled == false) {
+ /*
+ * 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.
+ */
+ pwm_imx_tpm_config(chip, pwm, state->period,
+ 0, state->polarity);
+ if (chan->status == true)
+ pwm_imx_tpm_enable(chip, pwm, false);
+ } else {
+ pwm_imx_tpm_config(chip, pwm, state->period,
+ state->duty_cycle, state->polarity);
+ if (chan->status == false)
+ pwm_imx_tpm_enable(chip, pwm, true);
+ }
+
+ mutex_unlock(&tpm->lock);
+
+ return 0;
+}
+
+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)
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("i.MX TPM PWM Driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
Add i.MX7ULP EVK board PWM0 support.
Signed-off-by: Anson Huang <[email protected]>
---
Changes since V5:
- change #pwm-cells to 3.
---
arch/arm/boot/dts/imx7ulp-evk.dts | 12 ++++++++++++
arch/arm/boot/dts/imx7ulp.dtsi | 10 ++++++++++
2 files changed, 22 insertions(+)
diff --git a/arch/arm/boot/dts/imx7ulp-evk.dts b/arch/arm/boot/dts/imx7ulp-evk.dts
index a09026a..3f5ea18 100644
--- a/arch/arm/boot/dts/imx7ulp-evk.dts
+++ b/arch/arm/boot/dts/imx7ulp-evk.dts
@@ -40,6 +40,12 @@
status = "okay";
};
+&pwm0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_pwm0>;
+ status = "okay";
+};
+
&usdhc0 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_usdhc0>;
@@ -57,6 +63,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
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
This patch adds i.MX7ULP EVK board MIPI-DSI backlight
support.
Signed-off-by: Anson Huang <[email protected]>
---
Changes since V5:
- change #pwm-cells to 3, add polarity settings.
---
arch/arm/boot/dts/imx7ulp-evk.dts | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/imx7ulp-evk.dts b/arch/arm/boot/dts/imx7ulp-evk.dts
index 3f5ea18..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";
--
2.7.4
Add i.MX TPM(Low Power Timer/Pulse Width Modulation Module) PWM binding.
Signed-off-by: Anson Huang <[email protected]>
---
Changes since V5:
- improve compatible string;
- change #pwm-cells to 3;
- "pwm" -> "PWM";
---
.../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
Select CONFIG_PWM_IMX_TPM by default to support i.MX7ULP
TPM PWM.
Signed-off-by: Anson Huang <[email protected]>
---
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
On Tue, Mar 19, 2019 at 06:50:26AM +0000, Anson Huang wrote:
> Add i.MX7ULP EVK board PWM0 support.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> Changes since V5:
> - change #pwm-cells to 3.
> ---
> arch/arm/boot/dts/imx7ulp-evk.dts | 12 ++++++++++++
> arch/arm/boot/dts/imx7ulp.dtsi | 10 ++++++++++
IMHO the change to the evk.dts belongs into the last patch.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:[email protected]]
> Sent: 2019年3月19日 14:55
> To: Anson Huang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Leonard Crestez <[email protected]>;
> [email protected]; [email protected]; Robin Gong
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH V6 4/5] ARM: dts: imx7ulp: Add pwm0 support
>
> On Tue, Mar 19, 2019 at 06:50:26AM +0000, Anson Huang wrote:
> > Add i.MX7ULP EVK board PWM0 support.
> >
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > Changes since V5:
> > - change #pwm-cells to 3.
> > ---
> > arch/arm/boot/dts/imx7ulp-evk.dts | 12 ++++++++++++
> > arch/arm/boot/dts/imx7ulp.dtsi | 10 ++++++++++
>
> IMHO the change to the evk.dts belongs into the last patch.
Ah, yes, the PWM pin settings in board dts should belongs into the backlight patch,
I will resend V6, thanks.
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7Ce3
> 645a6d4d6643a1f80f08d6ac37c725%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636885752897208572&sdata=N67sms9lX6asXpYaq%2Ftf
> TNpNz%2BeDIKNx89%2F57CYQAwA%3D&reserved=0 |
On Tue, Mar 19, 2019 at 06:50:12AM +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 <[email protected]>
> ---
> Changes since V5:
> - improve commit message body;
> - add period round function;
> - use per channel data for saving channel's private data;
> - improve error message output during probe;
> - improve different period settings by different channels' handling;
> - support #pwm-cells 3 cases.
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-imx-tpm.c | 463 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 475 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..bb6b27e
> --- /dev/null
> +++ b/drivers/pwm/pwm-imx-tpm.c
> @@ -0,0 +1,463 @@
> +// 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)
> +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) /* combine ELSA and ELSB as a field */
I'd write a more verbose comment here to make this understandable.
Something like:
/*
* 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_MOD_MOD GENMASK(15, 0)
> +
> +#define PWM_IMX_TPM_MAX_COUNT 0xffff
This is just the maximal value for PWM_IMX_TPM_MOD_MOD? If so, this
should be reflected in the name. Other than that, PWM_IMX_TPM_MOD_MOD
could be used for that, too.
> +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_channel {
> + u32 config;
> + bool status;
> +};
> +
> +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 unsigned int pwm_imx_tpm_round_period(struct pwm_chip *chip,
> + u32 period)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 rate, real_period, prescale, period_count;
> + u64 tmp;
> +
> + rate = clk_get_rate(tpm->clk);
> + tmp = period;
> + tmp *= rate;
> + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
The value tmp holds is the time a single counter step takes, right?
Please give that a better name. Something like clock_unit.
> + if (tmp <= PWM_IMX_TPM_MAX_COUNT) {
> + prescale = 0;
> + } else {
> + prescale = roundup_pow_of_two(tmp /
> + (PWM_IMX_TPM_MAX_COUNT + 1));
Do divide a u64 you must not use a plain division. You need div64_ul()
here.
> + prescale = ilog2(prescale);
This is equivalent to:
prescale = roundup_pow_of_two(tmp);
prescale = ilog2(prescale) - 16;
which safes a division. (Of course don't use a plain 16 in your code.)
> + }
> +
> + /* if no valid prescale found, use MAX instead */
> + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> + prescale = PWM_IMX_TPM_SC_PS >> __bf_shf(PWM_IMX_TPM_SC_PS);
prescale = FIELD_PREP(PWM_IMX_TPM_SC_PS, PWM_IMX_TPM_SC_PS)
What is the consequence if the calculated prescale isn't valid? I assume
this yields a greatly different period? If yes, this should result in an
error.
> + /* if no valid period count found, use MAX instead */
> + period_count = (tmp + ((1 << prescale) >> 1)) >> prescale;
> + if (period_count > PWM_IMX_TPM_MOD_MOD)
> + period_count = PWM_IMX_TPM_MOD_MOD;
Ditto.
> + /* calculate real period HW can support */
> + tmp = period_count;
> + tmp *= (1 << prescale) * NSEC_PER_SEC;
I don't know what the compiler does here, I guess it is a bit easier for
it to optimise here if you write:
tmp = (u64)period_count << prescale;
tmp *= NSEC_PER_SEC;
> + real_period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> + return real_period;
> +}
> +
> +static void pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 period)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + u32 val, rate, prescale, saved_cmod;
> + u64 tmp;
> +
> + rate = clk_get_rate(tpm->clk);
> + tmp = period;
> + tmp *= rate;
> + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> + if (tmp <= PWM_IMX_TPM_MAX_COUNT) {
> + prescale = 0;
> + } else {
> + prescale = roundup_pow_of_two(tmp /
> + (PWM_IMX_TPM_MAX_COUNT + 1));
> + prescale = ilog2(prescale);
> + }
> +
> + /* 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, 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
> + */
> + val = (tmp + ((1 << prescale) >> 1)) >> prescale;
> + writel(val, 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);
> + }
> +
> + tpm->real_period = period;
> +}
> +
> +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + u32 period,
> + u32 duty_cycle,
> + enum pwm_polarity polarity)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> + u32 val;
> + u64 tmp;
> +
> + /* set duty counter */
> + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
> + tmp *= duty_cycle;
> + val = DIV_ROUND_CLOSEST_ULL(tmp, period);
> + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> +
> + /*
> + * 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.
> + */
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA);
> + val |= PWM_IMX_TPM_CnSC_MSB;
> + val |= (polarity == PWM_POLARITY_NORMAL) ?
> + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> + /*
> + * polarity settings will enabled/disable output status
> + * immediately, so here ONLY save the config and write
> + * it into register when channel is enabled/disabled.
> + */
> + chan->config = val;
This function's behaviour is strange. It configures the hardware with
the right the duty_cycle but not the polarity. I cannot imagine that
this not buggy.
> +}
> +
> +/*
> + * When a channel's polarity is configured, the polarity settings
> + * will be saved and ONLY write into the register when the channel
> + * is enabled.
> + *
> + * When a channel is disabled, its polarity settings will be saved
> + * and its output will be disabled by clearing polarity settings.
> + *
> + * When a channel is enabled, its polarity settings will be restored
> + * and output will be enabled again.
> + */
> +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + bool enable)
> +{
> + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> + u32 val;
> +
> + val = readl(tpm->base + PWM_IMX_TPM_SC);
> + if (enable) {
> + /* restore channel config */
> + writel(chan->config,
> + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +
> + if (++tpm->enable_count == 1) {
> + /* start TPM counter */
> + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> + } else {
> + /* disable channel */
> + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> + val &= ~(PWM_IMX_TPM_CnSC_MSA | PWM_IMX_TPM_CnSC_MSB |
> + PWM_IMX_TPM_CnSC_ELS);
> + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> +
> + if (--tpm->enable_count == 0) {
> + /* stop TPM counter since all channels are disabled */
> + val &= ~PWM_IMX_TPM_SC_CMOD;
> + writel(val, tpm->base + PWM_IMX_TPM_SC);
> + }
> + }
> +
> + /* update channel status */
> + chan->status = enable;
> +}
> +
> +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) == 0x1)
> + state->polarity = PWM_POLARITY_INVERSED;
> + else if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x2)
> + state->polarity = PWM_POLARITY_NORMAL;
else ?
> +
> + /* get channel status */
> + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true : false;
> +}
> +
> +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_channel *chan = pwm_get_chip_data(pwm);
> + u32 p;
> +
> + mutex_lock(&tpm->lock);
> +
> + if (state->period != tpm->real_period) {
> + /*
> + * 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.
> + */
> + p = pwm_imx_tpm_round_period(chip, state->period);
> + if (p != tpm->real_period && tpm->user_count != 1)
> + return -EBUSY;
> + else if (p != tpm->real_period)
> + pwm_imx_tpm_config_counter(chip, p);
> + }
This looks more complicated than it should be. What about:
p = pwm_imx_tpm_round_period(chip, state->period);
if (p != tpm->real_period) {
if (tpm->user_count != 1)
return -EBUSY;
pwm_imx_tpm_config_counter(chip, p);
}
Another optimisation is possible here as pwm_imx_tpm_round_period and
pwm_imx_tpm_config_counter do effectively the same calculations: Let
pwm_imx_tpm_round_period return the respective register values that are
then just written to the registers instead of recalculating them once
more.
> + if (state->enabled == false) {
> + /*
> + * 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.
> + */
> + pwm_imx_tpm_config(chip, pwm, state->period,
> + 0, state->polarity);
> + if (chan->status == true)
> + pwm_imx_tpm_enable(chip, pwm, false);
> + } else {
> + pwm_imx_tpm_config(chip, pwm, state->period,
> + state->duty_cycle, state->polarity);
> + if (chan->status == false)
> + pwm_imx_tpm_enable(chip, pwm, true);
> + }
This function is really hard to understand. The factors making this
complicated are:
- Strange semantic of pwm_imx_tpm_config, IMHO the things done in both
pwm_imx_tpm_config and pwm_imx_tpm_enable should be done in a single
function.
- "status" could better be names "enabled"
IMHO it should look like:
int my_apply(chip, pwm, state)
{
ret = my_round_state(state, &hwparams);
if (ret)
return ret;
mutex_lock();
if (usercount > 1 &&
my_hwparams_interfere(current_state, hwparams))
ret = -EBUSY;
goto out_unlock;
my_apply_to_hardware(hwparams);
mutex_unlock();
out_unlock:
return ret;
}
> + mutex_unlock(&tpm->lock);
> +
> + return 0;
> +}
> +
> +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)
> + clk_disable_unprepare(tpm->clk);
> +
> + return 0;
IMHO you should return an error if enable_count isn't 0 to prevent going
to suspend.
> +}
> +
> +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 <[email protected]>");
> +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/ |
Hi,
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:[email protected]]
> Sent: 2019年3月19日 16:04
> To: Anson Huang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Leonard Crestez <[email protected]>;
> [email protected]; [email protected]; Robin Gong
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH V6 2/5] pwm: Add i.MX TPM PWM driver support
>
> On Tue, Mar 19, 2019 at 06:50:12AM +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 <[email protected]>
> > ---
> > Changes since V5:
> > - improve commit message body;
> > - add period round function;
> > - use per channel data for saving channel's private data;
> > - improve error message output during probe;
> > - improve different period settings by different channels' handling;
> > - support #pwm-cells 3 cases.
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-imx-tpm.c | 463
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 475 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..bb6b27e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-imx-tpm.c
> > @@ -0,0 +1,463 @@
> > +// 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)
> > +#define PWM_IMX_TPM_CnSC_ELS GENMASK(3, 2) /* combine ELSA and
> ELSB as a field */
>
> I'd write a more verbose comment here to make this understandable.
> Something like:
>
> /*
> * 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.
> */
OK.
>
> > +
> > +#define PWM_IMX_TPM_MOD_MOD GENMASK(15, 0)
> > +
> > +#define PWM_IMX_TPM_MAX_COUNT 0xffff
>
> This is just the maximal value for PWM_IMX_TPM_MOD_MOD? If so, this
> should be reflected in the name. Other than that,
> PWM_IMX_TPM_MOD_MOD could be used for that, too.
I think I can just use PWM_IMX_TPM_MOD_MOD directly, as this is indicating
MAX period count.
>
> > +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_channel {
> > + u32 config;
> > + bool status;
> > +};
> > +
> > +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 unsigned int pwm_imx_tpm_round_period(struct pwm_chip *chip,
> > + u32 period)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 rate, real_period, prescale, period_count;
> > + u64 tmp;
> > +
> > + rate = clk_get_rate(tpm->clk);
> > + tmp = period;
> > + tmp *= rate;
> > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
>
> The value tmp holds is the time a single counter step takes, right?
> Please give that a better name. Something like clock_unit.
OK.
>
> > + if (tmp <= PWM_IMX_TPM_MAX_COUNT) {
> > + prescale = 0;
> > + } else {
> > + prescale = roundup_pow_of_two(tmp /
> > + (PWM_IMX_TPM_MAX_COUNT +
> 1));
>
> Do divide a u64 you must not use a plain division. You need div64_ul() here.
>
> > + prescale = ilog2(prescale);
>
> This is equivalent to:
>
> prescale = roundup_pow_of_two(tmp);
> prescale = ilog2(prescale) - 16;
>
> which safes a division. (Of course don't use a plain 16 in your code.)
>
OK, I was just using tmp as a temp value which can be used in following,
Now that it is NOT a good way, I will optimize it following your advices.
> > + }
> > +
> > + /* if no valid prescale found, use MAX instead */
> > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > + prescale = PWM_IMX_TPM_SC_PS >>
> __bf_shf(PWM_IMX_TPM_SC_PS);
>
> prescale = FIELD_PREP(PWM_IMX_TPM_SC_PS,
> PWM_IMX_TPM_SC_PS)
>
> What is the consequence if the calculated prescale isn't valid? I assume this
> yields a greatly different period? If yes, this should result in an error.
Yes, that is what I intend to do, if a request period is too large and the prescale
Can NOT meet the requirement, I force it to the largest value that hardware can
Provide, I am NOT sure if it is the correct solution, if no, I can make it return error
If round period returns an invalid result.
>
> > + /* if no valid period count found, use MAX instead */
> > + period_count = (tmp + ((1 << prescale) >> 1)) >> prescale;
> > + if (period_count > PWM_IMX_TPM_MOD_MOD)
> > + period_count = PWM_IMX_TPM_MOD_MOD;
>
> Ditto.
>
> > + /* calculate real period HW can support */
> > + tmp = period_count;
> > + tmp *= (1 << prescale) * NSEC_PER_SEC;
>
> I don't know what the compiler does here, I guess it is a bit easier for it to
> optimise here if you write:
>
> tmp = (u64)period_count << prescale;
> tmp *= NSEC_PER_SEC;
OK, I just don't want to use (u64) force cast, so I did that.
>
> > + real_period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + return real_period;
> > +}
> > +
> > +static void pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32
> > +period) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + u32 val, rate, prescale, saved_cmod;
> > + u64 tmp;
> > +
> > + rate = clk_get_rate(tpm->clk);
> > + tmp = period;
> > + tmp *= rate;
> > + tmp = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC);
> > + if (tmp <= PWM_IMX_TPM_MAX_COUNT) {
> > + prescale = 0;
> > + } else {
> > + prescale = roundup_pow_of_two(tmp /
> > + (PWM_IMX_TPM_MAX_COUNT +
> 1));
> > + prescale = ilog2(prescale);
> > + }
> > +
> > + /* 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, 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
> > + */
> > + val = (tmp + ((1 << prescale) >> 1)) >> prescale;
> > + writel(val, 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);
> > + }
> > +
> > + tpm->real_period = period;
> > +}
> > +
> > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + u32 period,
> > + u32 duty_cycle,
> > + enum pwm_polarity polarity) {
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> > + u32 val;
> > + u64 tmp;
> > +
> > + /* set duty counter */
> > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> PWM_IMX_TPM_MOD_MOD;
> > + tmp *= duty_cycle;
> > + val = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > +
> > + /*
> > + * 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.
> > + */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA);
> > + val |= PWM_IMX_TPM_CnSC_MSB;
> > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > + /*
> > + * polarity settings will enabled/disable output status
> > + * immediately, so here ONLY save the config and write
> > + * it into register when channel is enabled/disabled.
> > + */
> > + chan->config = val;
>
> This function's behaviour is strange. It configures the hardware with the right
> the duty_cycle but not the polarity. I cannot imagine that this not buggy.
I think that is hardware limitation here, I used the polarity setting to enable/disable
the channel, if we set the polarity here directly, the output status may NOT reflect
the real state, if eventually the status is disabled, setting the polarity directly into register
will make the output active, that is NOT expected, right? And that is why I put comments
here.
>
> > +}
> > +
> > +/*
> > + * When a channel's polarity is configured, the polarity settings
> > + * will be saved and ONLY write into the register when the channel
> > + * is enabled.
> > + *
> > + * When a channel is disabled, its polarity settings will be saved
> > + * and its output will be disabled by clearing polarity settings.
> > + *
> > + * When a channel is enabled, its polarity settings will be restored
> > + * and output will be enabled again.
> > + */
> > +static void pwm_imx_tpm_enable(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + bool enable)
> > +{
> > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> > + u32 val;
> > +
> > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > + if (enable) {
> > + /* restore channel config */
> > + writel(chan->config,
> > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > +
> > + if (++tpm->enable_count == 1) {
> > + /* start TPM counter */
> > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + } else {
> > + /* disable channel */
> > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > + val &= ~(PWM_IMX_TPM_CnSC_MSA |
> PWM_IMX_TPM_CnSC_MSB |
> > + PWM_IMX_TPM_CnSC_ELS);
> > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm-
> >hwpwm));
> > +
> > + if (--tpm->enable_count == 0) {
> > + /* stop TPM counter since all channels are disabled
> */
> > + val &= ~PWM_IMX_TPM_SC_CMOD;
> > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > + }
> > + }
> > +
> > + /* update channel status */
> > + chan->status = enable;
> > +}
> > +
> > +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) == 0x1)
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + else if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x2)
> > + state->polarity = PWM_POLARITY_NORMAL;
>
> else ?
OK, maybe I can set other values to a default polarity.
>
> > +
> > + /* get channel status */
> > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > +false; }
> > +
> > +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_channel *chan = pwm_get_chip_data(pwm);
> > + u32 p;
> > +
> > + mutex_lock(&tpm->lock);
> > +
> > + if (state->period != tpm->real_period) {
> > + /*
> > + * 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.
> > + */
> > + p = pwm_imx_tpm_round_period(chip, state->period);
> > + if (p != tpm->real_period && tpm->user_count != 1)
> > + return -EBUSY;
> > + else if (p != tpm->real_period)
> > + pwm_imx_tpm_config_counter(chip, p);
> > + }
>
> This looks more complicated than it should be. What about:
>
> p = pwm_imx_tpm_round_period(chip, state->period);
>
> if (p != tpm->real_period) {
> if (tpm->user_count != 1)
> return -EBUSY;
>
> pwm_imx_tpm_config_counter(chip, p);
> }
>
> Another optimisation is possible here as pwm_imx_tpm_round_period and
> pwm_imx_tpm_config_counter do effectively the same calculations: Let
> pwm_imx_tpm_round_period return the respective register values that are
> then just written to the registers instead of recalculating them once more.
>
This is also my original thoughts, it can avoid computing again in config function, but
I have to find a place to save the prescale and period count, maybe adding them in
driver data? Then in config function, just write the registers directly.
> > + if (state->enabled == false) {
> > + /*
> > + * 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.
> > + */
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + 0, state->polarity);
> > + if (chan->status == true)
> > + pwm_imx_tpm_enable(chip, pwm, false);
> > + } else {
> > + pwm_imx_tpm_config(chip, pwm, state->period,
> > + state->duty_cycle, state->polarity);
> > + if (chan->status == false)
> > + pwm_imx_tpm_enable(chip, pwm, true);
> > + }
>
> This function is really hard to understand. The factors making this
> complicated are:
>
> - Strange semantic of pwm_imx_tpm_config, IMHO the things done in both
> pwm_imx_tpm_config and pwm_imx_tpm_enable should be done in a
> single
> function.
> - "status" could better be names "enabled"
>
> IMHO it should look like:
>
> int my_apply(chip, pwm, state)
> {
> ret = my_round_state(state, &hwparams);
> if (ret)
> return ret;
>
> mutex_lock();
>
> if (usercount > 1 &&
> my_hwparams_interfere(current_state, hwparams))
> ret = -EBUSY;
> goto out_unlock;
>
> my_apply_to_hardware(hwparams);
>
> mutex_unlock();
> out_unlock:
>
> return ret;
> }
>
Agreed, that can also solve the polarity setting question, will give it a try.
> > + mutex_unlock(&tpm->lock);
> > +
> > + return 0;
> > +}
> > +
> > +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)
> > + clk_disable_unprepare(tpm->clk);
> > +
> > + return 0;
>
> IMHO you should return an error if enable_count isn't 0 to prevent going to
> suspend.
OK, although maybe there is other voice that fail to disabling a clock does NOT impact
System suspend function, the clock will be automatically turned off by hardware anyway,
But I agree that adding error return is also making sense here.
Anson.
>
> > +}
> > +
> > +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 <[email protected]>");
> > +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&data=02%7C01%7Canson.huang%40nxp.com%7Cde
> 50da26809943d1f91008d6ac4187bb%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636885794786167747&sdata=EcLpUzIHibxW%2B3rEzPI
> or7d67kzHT73%2BZ2%2F4kZauZ%2BY%3D&reserved=0 |
Hello,
On Tue, Mar 19, 2019 at 09:08:26AM +0000, Anson Huang wrote:
> > On Tue, Mar 19, 2019 at 06:50:12AM +0000, Anson Huang wrote:
> > > + }
> > > +
> > > + /* if no valid prescale found, use MAX instead */
> > > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > > + prescale = PWM_IMX_TPM_SC_PS >>
> > __bf_shf(PWM_IMX_TPM_SC_PS);
> >
> > prescale = FIELD_PREP(PWM_IMX_TPM_SC_PS,
> > PWM_IMX_TPM_SC_PS)
> >
> > What is the consequence if the calculated prescale isn't valid? I assume this
> > yields a greatly different period? If yes, this should result in an error.
>
> Yes, that is what I intend to do, if a request period is too large and the prescale
> Can NOT meet the requirement, I force it to the largest value that hardware can
> Provide, I am NOT sure if it is the correct solution, if no, I can make it return error
> If round period returns an invalid result.
I'd say for sure it is not correct to configure for a period of 3 s if
20 s are requested. Where to draw the line exactly I don't know either.
In my opinion we need a general guideline like:
If the requested period + duty_cycle combo isn't supported use
the biggest available period that is not bigger than requested,
scale duty cycle accordingly and pick the biggest available duty
cycle that is not bigger than the scaled value.
If the requested period is shorter than possible, return
-ERANGE.
I think the above is sensible, but that's something that IMHO must first
be declared to be "official" as it doesn't make sense if a single driver
implements this and the other still do their own stuff.
Additionally I would consider it beneficial to have something like:
int pwm_round_state(pwm, &state)
that modifies state according to the above rules without affecting the
hardware. With the respective driver callback the framework could then
implement stuff like: "If the resulting configuration deviates too much
from the requested state, return an error to the consumer." with having
the definition of "too much" in a single place. It would also allow a
consumer who cares much about the resulting waveform to determine the
effects without testing.
The framework could then add additional checks like:
int pwm_apply_state(pwm, state)
{
struct pwm_state rounded_state;
if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
rounded_state = pwm->ops->round_state(state);
if !was_rounded_according_to_the_rules(round_state):
warn(...)
pwm->ops->apply(pwm, state)
if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
actual_state = pwm->ops->get_state(state);
if actual_state != round_state:
warn(...)
> > > + /* calculate real period HW can support */
> > > + tmp = period_count;
> > > + tmp *= (1 << prescale) * NSEC_PER_SEC;
> >
> > I don't know what the compiler does here, I guess it is a bit easier for it to
> > optimise here if you write:
> >
> > tmp = (u64)period_count << prescale;
> > tmp *= NSEC_PER_SEC;
>
>
> OK, I just don't want to use (u64) force cast, so I did that.
I don't care much about that. If you prefer do:
tmp = period_cound;
tmp <<= prescale;
tmp *= NSEC_PER_SEC;
Just don't multiply by (1 << prescale).
> > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + u32 period,
> > > + u32 duty_cycle,
> > > + enum pwm_polarity polarity) {
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> > > + u32 val;
> > > + u64 tmp;
> > > +
> > > + /* set duty counter */
> > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & PWM_IMX_TPM_MOD_MOD;
> > > + tmp *= duty_cycle;
> > > + val = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > +
> > > + /*
> > > + * 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.
> > > + */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA);
> > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > > + /*
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so here ONLY save the config and write
> > > + * it into register when channel is enabled/disabled.
> > > + */
> > > + chan->config = val;
> >
> > This function's behaviour is strange. It configures the hardware with the right
> > the duty_cycle but not the polarity. I cannot imagine that this not buggy.
>
> I think that is hardware limitation here, I used the polarity setting to enable/disable
> the channel, if we set the polarity here directly, the output status may NOT reflect
> the real state, if eventually the status is disabled, setting the polarity directly into register
> will make the output active, that is NOT expected, right? And that is why I put comments
> here.
The frameworks expectation is that .apply results in the hardware
switches to the new configuration directly after completing a previously
configured period. So if you change the period, get preempted and then
change the polarity three periods later that is a bug. If it's
impossible to fix this in software please do as good as possible
(whatever that means) and add this problem to the list of limitations at
the top of the driver.
> > > +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) == 0x1)
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + else if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x2)
> > > + state->polarity = PWM_POLARITY_NORMAL;
> >
> > else ?
>
> OK, maybe I can set other values to a default polarity.
I'd do:
if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x1)
state->polarity = PWM_POLARITY_INVERSED;
else
/*
* Assume reserved values (0b00 and 0b11) to yield
* normal polarity.
*/
state->polarity = PWM_POLARITY_NORMAL;
(unless it can be determined what the other configurations do. The RM
declares them as reserved, maybe they just yield constant high and
constant low patterns?)
A define for "inversed" (1) and "normal" (2) would be good.
> > > + /* get channel status */
> > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > > +false; }
> > > +
> > > +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_channel *chan = pwm_get_chip_data(pwm);
> > > + u32 p;
> > > +
> > > + mutex_lock(&tpm->lock);
> > > +
> > > + if (state->period != tpm->real_period) {
> > > + /*
> > > + * 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.
> > > + */
> > > + p = pwm_imx_tpm_round_period(chip, state->period);
> > > + if (p != tpm->real_period && tpm->user_count != 1)
> > > + return -EBUSY;
> > > + else if (p != tpm->real_period)
> > > + pwm_imx_tpm_config_counter(chip, p);
> > > + }
> >
> > This looks more complicated than it should be. What about:
> >
> > p = pwm_imx_tpm_round_period(chip, state->period);
> >
> > if (p != tpm->real_period) {
> > if (tpm->user_count != 1)
> > return -EBUSY;
> >
> > pwm_imx_tpm_config_counter(chip, p);
> > }
> >
> > Another optimisation is possible here as pwm_imx_tpm_round_period and
> > pwm_imx_tpm_config_counter do effectively the same calculations: Let
> > pwm_imx_tpm_round_period return the respective register values that are
> > then just written to the registers instead of recalculating them once more.
> >
>
> This is also my original thoughts, it can avoid computing again in config function, but
> I have to find a place to save the prescale and period count, maybe adding them in
> driver data? Then in config function, just write the registers directly.
No, don't hide that in driver data---hiding is bad. Pass a pointer to a
custom struct that lives on the stack instead.
> > > + if (state->enabled == false) {
> > > + /*
> > > + * 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.
> > > + */
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + 0, state->polarity);
> > > + if (chan->status == true)
> > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > + } else {
> > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > + state->duty_cycle, state->polarity);
> > > + if (chan->status == false)
> > > + pwm_imx_tpm_enable(chip, pwm, true);
> > > + }
> >
> > This function is really hard to understand. The factors making this
> > complicated are:
> >
> > - Strange semantic of pwm_imx_tpm_config, IMHO the things done in both
> > pwm_imx_tpm_config and pwm_imx_tpm_enable should be done in a
> > single
> > function.
> > - "status" could better be names "enabled"
> >
> > IMHO it should look like:
> >
> > int my_apply(chip, pwm, state)
> > {
> > ret = my_round_state(state, &hwparams);
> > if (ret)
> > return ret;
> >
> > mutex_lock();
> >
> > if (usercount > 1 &&
> > my_hwparams_interfere(current_state, hwparams))
> > ret = -EBUSY;
> > goto out_unlock;
> >
> > my_apply_to_hardware(hwparams);
> >
> > mutex_unlock();
> > out_unlock:
> >
> > return ret;
> > }
> >
>
> Agreed, that can also solve the polarity setting question, will give it a try.
And it could also simplify to implement the callback for my suggested
pwm_round_state above.
> > > +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)
> > > + clk_disable_unprepare(tpm->clk);
> > > +
> > > + return 0;
> >
> > IMHO you should return an error if enable_count isn't 0 to prevent going to
> > suspend.
>
> OK, although maybe there is other voice that fail to disabling a clock does NOT impact
> System suspend function, the clock will be automatically turned off by hardware anyway,
> But I agree that adding error return is also making sense here.
Not sure you understood what I intended to say. If the enable_count
isn't zero you shouldn't even try to disable the clock. Consider the PWM
is driving a step motor that must not be stopped in an uncontrolled
manner.
There is an effort to ensure that a PWM consumer is suspended before the
PWM. Together with this everybody should be happy.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi, Uwe
Best Regards!
Anson Huang
> -----Original Message-----
> From: Uwe Kleine-König [mailto:[email protected]]
> Sent: 2019年3月19日 18:17
> To: Anson Huang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Leonard Crestez <[email protected]>;
> [email protected]; [email protected]; Robin Gong
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; dl-linux-imx <[email protected]>
> Subject: Re: [PATCH V6 2/5] pwm: Add i.MX TPM PWM driver support
>
> Hello,
>
> On Tue, Mar 19, 2019 at 09:08:26AM +0000, Anson Huang wrote:
> > > On Tue, Mar 19, 2019 at 06:50:12AM +0000, Anson Huang wrote:
> > > > + }
> > > > +
> > > > + /* if no valid prescale found, use MAX instead */
> > > > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > > > + prescale = PWM_IMX_TPM_SC_PS >>
> > > __bf_shf(PWM_IMX_TPM_SC_PS);
> > >
> > > prescale = FIELD_PREP(PWM_IMX_TPM_SC_PS,
> > > PWM_IMX_TPM_SC_PS)
> > >
> > > What is the consequence if the calculated prescale isn't valid? I
> > > assume this yields a greatly different period? If yes, this should result in
> an error.
> >
> > Yes, that is what I intend to do, if a request period is too large and
> > the prescale Can NOT meet the requirement, I force it to the largest
> > value that hardware can Provide, I am NOT sure if it is the correct
> > solution, if no, I can make it return error If round period returns an invalid
> result.
>
> I'd say for sure it is not correct to configure for a period of 3 s if
> 20 s are requested. Where to draw the line exactly I don't know either.
>
> In my opinion we need a general guideline like:
>
> If the requested period + duty_cycle combo isn't supported use
> the biggest available period that is not bigger than requested,
> scale duty cycle accordingly and pick the biggest available duty
> cycle that is not bigger than the scaled value.
> If the requested period is shorter than possible, return
> -ERANGE.
>
> I think the above is sensible, but that's something that IMHO must first be
> declared to be "official" as it doesn't make sense if a single driver implements
> this and the other still do their own stuff.
As for now, do you agree if I make TPM driver ONLY support those period/duty
Setting that HW can support, for other request, just return -ERANGE to make it
Simple until there is a general guideline available?
>
> Additionally I would consider it beneficial to have something like:
>
> int pwm_round_state(pwm, &state)
>
> that modifies state according to the above rules without affecting the
> hardware. With the respective driver callback the framework could then
> implement stuff like: "If the resulting configuration deviates too much from
> the requested state, return an error to the consumer." with having the
> definition of "too much" in a single place. It would also allow a consumer
> who cares much about the resulting waveform to determine the effects
> without testing.
>
> The framework could then add additional checks like:
>
> int pwm_apply_state(pwm, state)
> {
> struct pwm_state rounded_state;
>
> if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
> rounded_state = pwm->ops->round_state(state);
>
> if !was_rounded_according_to_the_rules(round_state):
> warn(...)
>
> pwm->ops->apply(pwm, state)
>
> if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
> actual_state = pwm->ops->get_state(state);
> if actual_state != round_state:
> warn(...)
>
As for now, do you agree if I add the "round state" function to make sure every time
the newly requested state applied, the HW outputs the expected result? I can merge
the "config" and "enable" function into 1 function and make sure of that.
> > > > + /* calculate real period HW can support */
> > > > + tmp = period_count;
> > > > + tmp *= (1 << prescale) * NSEC_PER_SEC;
> > >
> > > I don't know what the compiler does here, I guess it is a bit easier
> > > for it to optimise here if you write:
> > >
> > > tmp = (u64)period_count << prescale;
> > > tmp *= NSEC_PER_SEC;
> >
> >
> > OK, I just don't want to use (u64) force cast, so I did that.
>
> I don't care much about that. If you prefer do:
>
> tmp = period_cound;
> tmp <<= prescale;
> tmp *= NSEC_PER_SEC;
>
> Just don't multiply by (1 << prescale).
OK.
>
> > > > +static void pwm_imx_tpm_config(struct pwm_chip *chip,
> > > > + struct pwm_device *pwm,
> > > > + u32 period,
> > > > + u32 duty_cycle,
> > > > + enum pwm_polarity polarity) {
> > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > > + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> > > > + u32 val;
> > > > + u64 tmp;
> > > > +
> > > > + /* set duty counter */
> > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) &
> PWM_IMX_TPM_MOD_MOD;
> > > > + tmp *= duty_cycle;
> > > > + val = DIV_ROUND_CLOSEST_ULL(tmp, period);
> > > > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > > +
> > > > + /*
> > > > + * 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.
> > > > + */
> > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > > + val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA);
> > > > + val |= PWM_IMX_TPM_CnSC_MSB;
> > > > + val |= (polarity == PWM_POLARITY_NORMAL) ?
> > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) :
> > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1);
> > > > + /*
> > > > + * polarity settings will enabled/disable output status
> > > > + * immediately, so here ONLY save the config and write
> > > > + * it into register when channel is enabled/disabled.
> > > > + */
> > > > + chan->config = val;
> > >
> > > This function's behaviour is strange. It configures the hardware
> > > with the right the duty_cycle but not the polarity. I cannot imagine that
> this not buggy.
> >
> > I think that is hardware limitation here, I used the polarity setting
> > to enable/disable the channel, if we set the polarity here directly,
> > the output status may NOT reflect the real state, if eventually the
> > status is disabled, setting the polarity directly into register will
> > make the output active, that is NOT expected, right? And that is why I put
> comments here.
>
> The frameworks expectation is that .apply results in the hardware switches
> to the new configuration directly after completing a previously configured
> period. So if you change the period, get preempted and then change the
> polarity three periods later that is a bug. If it's impossible to fix this in
> software please do as good as possible (whatever that means) and add this
> problem to the list of limitations at the top of the driver.
I think after merging the "config" and "enable" function into ONE function, the
output result should can be expected and no "polarity setting late" issue, talking
about the preempt between the period setting and polarity settings, should we
use the spin_lock_irqsave() instead of mutex() for .apply?
>
> > > > +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) == 0x1)
> > > > + state->polarity = PWM_POLARITY_INVERSED;
> > > > + else if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x2)
> > > > + state->polarity = PWM_POLARITY_NORMAL;
> > >
> > > else ?
> >
> > OK, maybe I can set other values to a default polarity.
>
> I'd do:
>
> if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) == 0x1)
> state->polarity = PWM_POLARITY_INVERSED;
> else
> /*
> * Assume reserved values (0b00 and 0b11) to yield
> * normal polarity.
> */
> state->polarity = PWM_POLARITY_NORMAL;
>
> (unless it can be determined what the other configurations do. The RM
> declares them as reserved, maybe they just yield constant high and constant
> low patterns?)
>
> A define for "inversed" (1) and "normal" (2) would be good.
OK.
>
> > > > + /* get channel status */
> > > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > > > +false; }
> > > > +
> > > > +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_channel *chan = pwm_get_chip_data(pwm);
> > > > + u32 p;
> > > > +
> > > > + mutex_lock(&tpm->lock);
> > > > +
> > > > + if (state->period != tpm->real_period) {
> > > > + /*
> > > > + * 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.
> > > > + */
> > > > + p = pwm_imx_tpm_round_period(chip, state->period);
> > > > + if (p != tpm->real_period && tpm->user_count != 1)
> > > > + return -EBUSY;
> > > > + else if (p != tpm->real_period)
> > > > + pwm_imx_tpm_config_counter(chip, p);
> > > > + }
> > >
> > > This looks more complicated than it should be. What about:
> > >
> > > p = pwm_imx_tpm_round_period(chip, state->period);
> > >
> > > if (p != tpm->real_period) {
> > > if (tpm->user_count != 1)
> > > return -EBUSY;
> > >
> > > pwm_imx_tpm_config_counter(chip, p);
> > > }
> > >
> > > Another optimisation is possible here as pwm_imx_tpm_round_period
> > > and pwm_imx_tpm_config_counter do effectively the same calculations:
> > > Let pwm_imx_tpm_round_period return the respective register values
> > > that are then just written to the registers instead of recalculating them
> once more.
> > >
> >
> > This is also my original thoughts, it can avoid computing again in
> > config function, but I have to find a place to save the prescale and
> > period count, maybe adding them in driver data? Then in config function,
> just write the registers directly.
>
> No, don't hide that in driver data---hiding is bad. Pass a pointer to a custom
> struct that lives on the stack instead.
OK.
>
> > > > + if (state->enabled == false) {
> > > > + /*
> > > > + * 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.
> > > > + */
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + 0, state->polarity);
> > > > + if (chan->status == true)
> > > > + pwm_imx_tpm_enable(chip, pwm, false);
> > > > + } else {
> > > > + pwm_imx_tpm_config(chip, pwm, state->period,
> > > > + state->duty_cycle, state->polarity);
> > > > + if (chan->status == false)
> > > > + pwm_imx_tpm_enable(chip, pwm, true);
> > > > + }
> > >
> > > This function is really hard to understand. The factors making this
> > > complicated are:
> > >
> > > - Strange semantic of pwm_imx_tpm_config, IMHO the things done in
> both
> > > pwm_imx_tpm_config and pwm_imx_tpm_enable should be done in a
> > > single
> > > function.
> > > - "status" could better be names "enabled"
> > >
> > > IMHO it should look like:
> > >
> > > int my_apply(chip, pwm, state)
> > > {
> > > ret = my_round_state(state, &hwparams);
> > > if (ret)
> > > return ret;
> > >
> > > mutex_lock();
> > >
> > > if (usercount > 1 &&
> > > my_hwparams_interfere(current_state, hwparams))
> > > ret = -EBUSY;
> > > goto out_unlock;
> > >
> > > my_apply_to_hardware(hwparams);
> > >
> > > mutex_unlock();
> > > out_unlock:
> > >
> > > return ret;
> > > }
> > >
> >
> > Agreed, that can also solve the polarity setting question, will give it a try.
>
> And it could also simplify to implement the callback for my suggested
> pwm_round_state above.
OK.
>
> > > > +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)
> > > > + clk_disable_unprepare(tpm->clk);
> > > > +
> > > > + return 0;
> > >
> > > IMHO you should return an error if enable_count isn't 0 to prevent
> > > going to suspend.
> >
> > OK, although maybe there is other voice that fail to disabling a clock
> > does NOT impact System suspend function, the clock will be
> > automatically turned off by hardware anyway, But I agree that adding error
> return is also making sense here.
>
> Not sure you understood what I intended to say. If the enable_count isn't
> zero you shouldn't even try to disable the clock. Consider the PWM is driving
> a step motor that must not be stopped in an uncontrolled manner.
>
> There is an effort to ensure that a PWM consumer is suspended before the
> PWM. Together with this everybody should be happy.
OK, understood now, thanks.
Anson.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7Ce1
> ab7391f8484f9c496d08d6ac540413%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636885874182445059&sdata=6ixF32SzmnBN%2BbOFaut
> vKaerQ7sEgOyqtjMps86yztc%3D&reserved=0 |
Hello,
On Tue, Mar 19, 2019 at 11:49:32AM +0000, Anson Huang wrote:
> > On Tue, Mar 19, 2019 at 09:08:26AM +0000, Anson Huang wrote:
> > > > On Tue, Mar 19, 2019 at 06:50:12AM +0000, Anson Huang wrote:
> > > > > + }
> > > > > +
> > > > > + /* if no valid prescale found, use MAX instead */
> > > > > + if ((!FIELD_FIT(PWM_IMX_TPM_SC_PS, prescale)))
> > > > > + prescale = PWM_IMX_TPM_SC_PS >> __bf_shf(PWM_IMX_TPM_SC_PS);
> > > >
> > > > prescale = FIELD_PREP(PWM_IMX_TPM_SC_PS, PWM_IMX_TPM_SC_PS)
> > > >
> > > > What is the consequence if the calculated prescale isn't valid? I
> > > > assume this yields a greatly different period? If yes, this should result in
> > > > an error.
> > >
> > > Yes, that is what I intend to do, if a request period is too large and
> > > the prescale Can NOT meet the requirement, I force it to the largest
> > > value that hardware can Provide, I am NOT sure if it is the correct
> > > solution, if no, I can make it return error If round period returns an invalid
> > > result.
> >
> > I'd say for sure it is not correct to configure for a period of 3 s if
> > 20 s are requested. Where to draw the line exactly I don't know either.
> >
> > In my opinion we need a general guideline like:
> >
> > If the requested period + duty_cycle combo isn't supported use
> > the biggest available period that is not bigger than requested,
> > scale duty cycle accordingly and pick the biggest available duty
> > cycle that is not bigger than the scaled value.
> > If the requested period is shorter than possible, return
> > -ERANGE.
> >
> > I think the above is sensible, but that's something that IMHO must first be
> > declared to be "official" as it doesn't make sense if a single driver implements
> > this and the other still do their own stuff.
>
> As for now, do you agree if I make TPM driver ONLY support those period/duty
> Setting that HW can support, for other request, just return -ERANGE to make it
> Simple until there is a general guideline available?
I would be surprised if that would result in something that is actually
usable. But feel free to try.
> > Additionally I would consider it beneficial to have something like:
> >
> > int pwm_round_state(pwm, &state)
> >
> > that modifies state according to the above rules without affecting the
> > hardware. With the respective driver callback the framework could then
> > implement stuff like: "If the resulting configuration deviates too much from
> > the requested state, return an error to the consumer." with having the
> > definition of "too much" in a single place. It would also allow a consumer
> > who cares much about the resulting waveform to determine the effects
> > without testing.
> >
> > The framework could then add additional checks like:
> >
> > int pwm_apply_state(pwm, state)
> > {
> > struct pwm_state rounded_state;
> >
> > if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
> > rounded_state = pwm->ops->round_state(state);
> >
> > if !was_rounded_according_to_the_rules(round_state):
> > warn(...)
> >
> > pwm->ops->apply(pwm, state)
> >
> > if IS_ENABLED(CONFIG_PWM_EXTRA_CHECKS):
> > actual_state = pwm->ops->get_state(state);
> > if actual_state != round_state:
> > warn(...)
> >
>
> As for now, do you agree if I add the "round state" function to make sure every time
> the newly requested state applied, the HW outputs the expected result? I can merge
> the "config" and "enable" function into 1 function and make sure of that.
Yeah, that sounds right. Splitting into enable and config is a thing
from the past and makes it hard to provide the needed atomicity.
> > > > > + /*
> > > > > + * polarity settings will enabled/disable output status
> > > > > + * immediately, so here ONLY save the config and write
> > > > > + * it into register when channel is enabled/disabled.
> > > > > + */
> > > > > + chan->config = val;
> > > >
> > > > This function's behaviour is strange. It configures the hardware
> > > > with the right the duty_cycle but not the polarity. I cannot imagine that
> > > > this not buggy.
> > >
> > > I think that is hardware limitation here, I used the polarity setting
> > > to enable/disable the channel, if we set the polarity here directly,
> > > the output status may NOT reflect the real state, if eventually the
> > > status is disabled, setting the polarity directly into register will
> > > make the output active, that is NOT expected, right? And that is why I put
> > > comments here.
> >
> > The frameworks expectation is that .apply results in the hardware switches
> > to the new configuration directly after completing a previously configured
> > period. So if you change the period, get preempted and then change the
> > polarity three periods later that is a bug. If it's impossible to fix this in
> > software please do as good as possible (whatever that means) and add this
> > problem to the list of limitations at the top of the driver.
>
> I think after merging the "config" and "enable" function into ONE function, the
> output result should can be expected and no "polarity setting late" issue, talking
> about the preempt between the period setting and polarity settings, should we
> use the spin_lock_irqsave() instead of mutex() for .apply?
No, I think mutexes are fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |