* [PATCH 0/2] Add support for NXP LPC18xx PWM/SCT
@ 2015-07-27 4:45 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-27 4:45 UTC (permalink / raw)
To: linux-pwm
Cc: linux-arm-kernel, manabian, ezequiel, thierry.reding,
Ariel D'Alessandro
This patch series adds support for NXP LPC18xx PWM/SCT.
NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
Other SoCs in that family may share the same hardware.
Patchset is based on tag next-20150723 of the linux-next repository.
It has been successfully tested on a Hitex LPC4350 Evaluation Board.
Ariel D'Alessandro (2):
pwm: NXP LPC18xx PWM/SCT driver
DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
.../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 +
drivers/pwm/Kconfig | 12 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-lpc18xx.c | 453 +++++++++++++++++++++
4 files changed, 486 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
create mode 100644 drivers/pwm/pwm-lpc18xx.c
--
1.9.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/2] Add support for NXP LPC18xx PWM/SCT
@ 2015-07-27 4:45 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-27 4:45 UTC (permalink / raw)
To: linux-arm-kernel
This patch series adds support for NXP LPC18xx PWM/SCT.
NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
Other SoCs in that family may share the same hardware.
Patchset is based on tag next-20150723 of the linux-next repository.
It has been successfully tested on a Hitex LPC4350 Evaluation Board.
Ariel D'Alessandro (2):
pwm: NXP LPC18xx PWM/SCT driver
DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
.../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 +
drivers/pwm/Kconfig | 12 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-lpc18xx.c | 453 +++++++++++++++++++++
4 files changed, 486 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
create mode 100644 drivers/pwm/pwm-lpc18xx.c
--
1.9.1
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver
2015-07-27 4:45 ` Ariel D'Alessandro
@ 2015-07-27 4:45 ` Ariel D'Alessandro
-1 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-27 4:45 UTC (permalink / raw)
To: linux-pwm
Cc: linux-arm-kernel, manabian, ezequiel, thierry.reding,
Ariel D'Alessandro
This commit adds support for NXP LPC18xx PWM/SCT.
NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
Other SoCs in that family may share the same hardware.
The PWM supports a total of 16 channels, but only 15 can be simultaneously
requested. There's only one period, global to all the channels, thus PWM driver
will refuse setting different values to it.
Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
drivers/pwm/Kconfig | 12 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-lpc18xx.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 472 insertions(+)
create mode 100644 drivers/pwm/pwm-lpc18xx.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..2c7635c 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -173,6 +173,18 @@ config PWM_LP3943
To compile this driver as a module, choose M here: the module
will be called pwm-lp3943.
+config PWM_LPC18XX
+ tristate "LPC18xx/43xx PWM support"
+ depends on ARCH_LPC18XX
+ help
+ Generic PWM framework driver for NXP LPC18xx PWM/SCT which
+ supports 16 channels.
+ A maximum of 15 channels can be requested simultaneously and
+ must have the same period.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-lpc18xx.
+
config PWM_LPC32XX
tristate "LPC32XX PWM support"
depends on ARCH_LPC32XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5..986b286 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
+obj-$(CONFIG_PWM_LPC18XX) += pwm-lpc18xx.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
diff --git a/drivers/pwm/pwm-lpc18xx.c b/drivers/pwm/pwm-lpc18xx.c
new file mode 100644
index 0000000..77b5e74
--- /dev/null
+++ b/drivers/pwm/pwm-lpc18xx.c
@@ -0,0 +1,459 @@
+/*
+ * NXP LPC18xx Pulse Width Modulator driver
+ *
+ * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * Notes
+ * =====
+ * NXP LPC18xx provides a State Configurable Timer (SCT) which can be configured
+ * as a Pulse Width Modulator.
+ *
+ * SCT supports 16 outputs, 16 events and 16 registers. Each event will be
+ * triggered when its related register matches the SCT counter value, and it
+ * will set or clear a selected output.
+ *
+ * One of the events is preselected to generate the period, thus the maximum
+ * number of simultaneous channels is limited to 15. Notice that period is
+ * global to all the channels, thus PWM driver will refuse setting different
+ * values to it.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/* LPC18xx SCT registers */
+#define LPC18XX_PWM_CONFIG 0x000
+#define LPC18XX_PWM_CONFIG_UNIFY BIT(0)
+#define LPC18XX_PWM_CONFIG_NORELOAD BIT(7)
+
+#define LPC18XX_PWM_CTRL 0x004
+#define LPC18XX_PWM_CTRL_HALT BIT(2)
+#define LPC18XX_PWM_BIDIR BIT(4)
+#define LPC18XX_PWM_PRE_SHIFT 5
+#define LPC18XX_PWM_PRE_MASK (0xff << LPC18XX_PWM_PRE_SHIFT)
+#define LPC18XX_PWM_PRE(x) (x << LPC18XX_PWM_PRE_SHIFT)
+
+#define LPC18XX_PWM_LIMIT 0x008
+
+#define LPC18XX_PWM_RES_BASE 0x058
+#define LPC18XX_PWM_RES_SHIFT(_ch) (_ch * 2)
+#define LPC18XX_PWM_RES(_ch, _action) (_action << LPC18XX_PWM_RES_SHIFT(_ch))
+#define LPC18XX_PWM_RES_MASK(_ch) (0x3 << LPC18XX_PWM_RES_SHIFT(_ch))
+
+#define LPC18XX_PWM_MATCH_BASE 0x100
+#define LPC18XX_PWM_MATCH(_ch) (LPC18XX_PWM_MATCH_BASE + _ch * 4)
+
+#define LPC18XX_PWM_MATCHREL_BASE 0x200
+#define LPC18XX_PWM_MATCHREL(_ch) (LPC18XX_PWM_MATCHREL_BASE + _ch * 4)
+
+#define LPC18XX_PWM_EVSTATEMSK_BASE 0x300
+#define LPC18XX_PWM_EVSTATEMSK(_ch) (LPC18XX_PWM_EVSTATEMSK_BASE + _ch * 8)
+#define LPC18XX_PWM_EVSTATEMSK_ALL 0xffffffff
+
+#define LPC18XX_PWM_EVCTRL_BASE 0x304
+#define LPC18XX_PWM_EVCTRL(_ch) (LPC18XX_PWM_EVCTRL_BASE + _ch * 8)
+
+#define LPC18XX_PWM_EVCTRL_MATCH(_ch) _ch
+
+#define LPC18XX_PWM_EVCTRL_COMB_SHIFT 12
+#define LPC18XX_PWM_EVCTRL_COMB_MATCH (0x1 << LPC18XX_PWM_EVCTRL_COMB_SHIFT)
+
+#define LPC18XX_PWM_OUTPUTSET_BASE 0x500
+#define LPC18XX_PWM_OUTPUTSET(_ch) (LPC18XX_PWM_OUTPUTSET_BASE + _ch * 8)
+
+#define LPC18XX_PWM_OUTPUTCL_BASE 0x504
+#define LPC18XX_PWM_OUTPUTCL(_ch) (LPC18XX_PWM_OUTPUTCL_BASE + _ch * 8)
+
+/* LPC18xx SCT unified counter */
+#define LPC18XX_PWM_TIMER_MAX 0xffffffff
+
+/* LPC18xx SCT events */
+#define LPC18XX_PWM_EVENT_PERIOD 0
+#define LPC18XX_PWM_EVENT_MAX 16
+
+/* SCT conflict resolution */
+enum lpc18xx_pwm_res_action {
+ LPC18XX_PWM_RES_NONE = 0x0,
+ LPC18XX_PWM_RES_SET = 0x1,
+ LPC18XX_PWM_RES_CLEAR = 0x2,
+ LPC18XX_PWM_RES_TOGGLE = 0x3,
+};
+
+struct lpc18xx_pwm_data {
+ unsigned int duty_event;
+};
+
+struct lpc18xx_pwm_chip {
+ struct device *dev;
+ struct pwm_chip chip;
+ void __iomem *base;
+ struct clk *pwm_clk;
+ unsigned long clk_rate;
+ unsigned int period_ns;
+ unsigned int min_period_ns;
+ unsigned int max_period_ns;
+ unsigned int period_event;
+ unsigned long event_map;
+ struct mutex res_lock;
+ struct mutex period_lock;
+};
+
+static inline struct lpc18xx_pwm_chip *to_lpc18xx_pwm_chip(
+ struct pwm_chip *chip)
+{
+ return container_of(chip, struct lpc18xx_pwm_chip, chip);
+}
+
+static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
+ u32 reg, u32 val)
+{
+ writel(val, lpc18xx_pwm->base + reg);
+}
+
+static inline u32 lpc18xx_pwm_readl(struct lpc18xx_pwm_chip *lpc18xx_pwm,
+ u32 reg)
+{
+ return readl(lpc18xx_pwm->base + reg);
+}
+
+static void lpc18xx_pwm_set_conflict_res(struct lpc18xx_pwm_chip *lpc18xx_pwm,
+ struct pwm_device *pwm,
+ enum lpc18xx_pwm_res_action action)
+{
+ u32 val;
+
+ mutex_lock(&lpc18xx_pwm->res_lock);
+
+ /*
+ * Simultaneous set and clear may happen on an output, that is the case
+ * when duty_ns == period_ns. LPC18xx SCT allows to set a conflict
+ * resolution action to be taken in such a case.
+ */
+ val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_RES_BASE);
+ val &= ~LPC18XX_PWM_RES_MASK(pwm->hwpwm);
+ val |= LPC18XX_PWM_RES(pwm->hwpwm, action);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_RES_BASE, val);
+
+ mutex_unlock(&lpc18xx_pwm->res_lock);
+}
+
+static void lpc18xx_pwm_config_period(struct pwm_chip *chip, int period_ns)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ u64 val;
+
+ val = (u64)period_ns * lpc18xx_pwm->clk_rate;
+ do_div(val, NSEC_PER_SEC);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCH(lpc18xx_pwm->period_event),
+ (u32)val);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCHREL(lpc18xx_pwm->period_event),
+ (u32)val);
+}
+
+static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
+ struct pwm_device *pwm, int duty_ns)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+ u64 val;
+
+ val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
+ do_div(val, NSEC_PER_SEC);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCH(lpc18xx_data->duty_event),
+ (u32)val);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCHREL(lpc18xx_data->duty_event),
+ (u32)val);
+}
+
+static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ int i;
+
+ if (period_ns < lpc18xx_pwm->min_period_ns ||
+ period_ns > lpc18xx_pwm->max_period_ns) {
+ dev_err(chip->dev, "period %d not in range\n", period_ns);
+ return -ERANGE;
+ }
+
+ mutex_lock(&lpc18xx_pwm->period_lock);
+
+ /*
+ * The PWM supports only a single period for all PWM channels, therefore
+ * incompatible changes need to be refused.
+ */
+ if (lpc18xx_pwm->period_ns && lpc18xx_pwm->period_ns != period_ns) {
+ dev_err(chip->dev, "conflicting period requested for PWM %u\n",
+ pwm->hwpwm);
+ mutex_unlock(&lpc18xx_pwm->period_lock);
+ return -EBUSY;
+ }
+
+ if (!lpc18xx_pwm->period_ns) {
+ lpc18xx_pwm->period_ns = period_ns;
+ for (i = 0; i < chip->npwm; i++) {
+ pwm_set_period(&chip->pwms[i], period_ns);
+ }
+ lpc18xx_pwm_config_period(chip, period_ns);
+ }
+
+ mutex_unlock(&lpc18xx_pwm->period_lock);
+
+ lpc18xx_pwm_config_duty(chip, pwm, duty_ns);
+
+ return 0;
+}
+
+static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ return 0;
+}
+
+static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+ enum lpc18xx_pwm_res_action res_action;
+ unsigned int set_event, clear_event;
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event),
+ LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_data->duty_event) |
+ LPC18XX_PWM_EVCTRL_COMB_MATCH);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
+ LPC18XX_PWM_EVSTATEMSK_ALL);
+
+ if (pwm->polarity == PWM_POLARITY_NORMAL) {
+ set_event = lpc18xx_pwm->period_event;
+ clear_event = lpc18xx_data->duty_event;
+ res_action = LPC18XX_PWM_RES_SET;
+ } else {
+ set_event = lpc18xx_data->duty_event;
+ clear_event = lpc18xx_pwm->period_event;
+ res_action = LPC18XX_PWM_RES_CLEAR;
+ }
+
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm),
+ BIT(set_event));
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm),
+ BIT(clear_event));
+ lpc18xx_pwm_set_conflict_res(lpc18xx_pwm, pwm, res_action);
+
+ return 0;
+}
+
+static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm), 0);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm), 0);
+}
+
+static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+ unsigned long event;
+
+ event = find_first_zero_bit(&lpc18xx_pwm->event_map,
+ LPC18XX_PWM_EVENT_MAX);
+
+ if (event >= LPC18XX_PWM_EVENT_MAX) {
+ dev_err(lpc18xx_pwm->dev,
+ "maximum number of simultaneous channels reached\n");
+ return -EBUSY;
+ };
+
+ set_bit(event, &lpc18xx_pwm->event_map);
+ lpc18xx_data->duty_event = event;
+
+ return 0;
+}
+
+static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+
+ lpc18xx_pwm_disable(chip, pwm);
+ clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
+}
+
+static const struct pwm_ops lpc18xx_pwm_ops = {
+ .config = lpc18xx_pwm_config,
+ .set_polarity = lpc18xx_pwm_set_polarity,
+ .enable = lpc18xx_pwm_enable,
+ .disable = lpc18xx_pwm_disable,
+ .request = lpc18xx_pwm_request,
+ .free = lpc18xx_pwm_free,
+ .owner = THIS_MODULE,
+};
+
+static const struct of_device_id lpc18xx_pwm_of_match[] = {
+ { .compatible = "nxp,lpc1850-pwm" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
+
+static int lpc18xx_pwm_probe(struct platform_device *pdev)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm;
+ struct pwm_device *pwm;
+ struct resource *res;
+ int ret, i;
+ u64 val;
+
+ lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
+ GFP_KERNEL);
+ if (!lpc18xx_pwm)
+ return -ENOMEM;
+
+ lpc18xx_pwm->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ lpc18xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(lpc18xx_pwm->base))
+ return PTR_ERR(lpc18xx_pwm->base);
+
+ lpc18xx_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
+ if (IS_ERR(lpc18xx_pwm->pwm_clk)) {
+ dev_err(&pdev->dev, "failed to get pwm clock\n");
+ return PTR_ERR(lpc18xx_pwm->pwm_clk);
+ }
+
+ ret = clk_prepare_enable(lpc18xx_pwm->pwm_clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "could not prepare or enable pwm clock\n");
+ return ret;
+ }
+
+ lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
+
+ mutex_init(&lpc18xx_pwm->res_lock);
+ mutex_init(&lpc18xx_pwm->period_lock);
+
+ val = (u64)NSEC_PER_SEC * LPC18XX_PWM_TIMER_MAX;
+ do_div(val, lpc18xx_pwm->clk_rate);
+ lpc18xx_pwm->max_period_ns = val;
+
+ val = (u64)NSEC_PER_SEC;
+ do_div(val, lpc18xx_pwm->clk_rate);
+ lpc18xx_pwm->min_period_ns = val;
+
+ lpc18xx_pwm->chip.dev = &pdev->dev;
+ lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
+ lpc18xx_pwm->chip.base = -1;
+ lpc18xx_pwm->chip.npwm = 16;
+ lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+ lpc18xx_pwm->chip.of_pwm_n_cells = 3;
+
+ /* SCT counter must be in unify (32 bit) mode */
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
+ LPC18XX_PWM_CONFIG_UNIFY);
+
+ /*
+ * Everytime the timer counter reaches the period value, the related
+ * event will be triggered and the counter reset to 0.
+ */
+ set_bit(LPC18XX_PWM_EVENT_PERIOD, &lpc18xx_pwm->event_map);
+ lpc18xx_pwm->period_event = LPC18XX_PWM_EVENT_PERIOD;
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVSTATEMSK(lpc18xx_pwm->period_event),
+ LPC18XX_PWM_EVSTATEMSK_ALL);
+
+ val = LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_pwm->period_event) |
+ LPC18XX_PWM_EVCTRL_COMB_MATCH;
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVCTRL(lpc18xx_pwm->period_event), val);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
+ BIT(lpc18xx_pwm->period_event));
+
+ ret = pwmchip_add(&lpc18xx_pwm->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
+ goto disable_pwmclk;
+ }
+
+ for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
+ pwm = &lpc18xx_pwm->chip.pwms[i];
+ pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
+ sizeof(struct lpc18xx_pwm_data),
+ GFP_KERNEL);
+ if (!pwm->chip_data) {
+ ret = -ENOMEM;
+ goto remove_pwmchip;
+ }
+ }
+
+ platform_set_drvdata(pdev, lpc18xx_pwm);
+
+ val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
+ val &= ~LPC18XX_PWM_BIDIR;
+ val &= ~LPC18XX_PWM_CTRL_HALT;
+ val &= ~LPC18XX_PWM_PRE_MASK;
+ val |= LPC18XX_PWM_PRE(0);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
+
+ return 0;
+
+remove_pwmchip:
+ pwmchip_remove(&lpc18xx_pwm->chip);
+disable_pwmclk:
+ clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
+ return ret;
+}
+
+static int lpc18xx_pwm_remove(struct platform_device *pdev)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
+ u32 val;
+
+ val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
+ val | LPC18XX_PWM_CTRL_HALT);
+
+ clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
+
+ return pwmchip_remove(&lpc18xx_pwm->chip);
+}
+
+static struct platform_driver lpc18xx_pwm_driver = {
+ .driver = {
+ .name = "lpc18xx-pwm",
+ .of_match_table = lpc18xx_pwm_of_match,
+ },
+ .probe = lpc18xx_pwm_probe,
+ .remove = lpc18xx_pwm_remove,
+};
+module_platform_driver(lpc18xx_pwm_driver);
+
+MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
+MODULE_DESCRIPTION("NXP LPC18xx PWM driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver
@ 2015-07-27 4:45 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-27 4:45 UTC (permalink / raw)
To: linux-arm-kernel
This commit adds support for NXP LPC18xx PWM/SCT.
NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
Other SoCs in that family may share the same hardware.
The PWM supports a total of 16 channels, but only 15 can be simultaneously
requested. There's only one period, global to all the channels, thus PWM driver
will refuse setting different values to it.
Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
drivers/pwm/Kconfig | 12 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-lpc18xx.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 472 insertions(+)
create mode 100644 drivers/pwm/pwm-lpc18xx.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..2c7635c 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -173,6 +173,18 @@ config PWM_LP3943
To compile this driver as a module, choose M here: the module
will be called pwm-lp3943.
+config PWM_LPC18XX
+ tristate "LPC18xx/43xx PWM support"
+ depends on ARCH_LPC18XX
+ help
+ Generic PWM framework driver for NXP LPC18xx PWM/SCT which
+ supports 16 channels.
+ A maximum of 15 channels can be requested simultaneously and
+ must have the same period.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-lpc18xx.
+
config PWM_LPC32XX
tristate "LPC32XX PWM support"
depends on ARCH_LPC32XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5..986b286 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
+obj-$(CONFIG_PWM_LPC18XX) += pwm-lpc18xx.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
diff --git a/drivers/pwm/pwm-lpc18xx.c b/drivers/pwm/pwm-lpc18xx.c
new file mode 100644
index 0000000..77b5e74
--- /dev/null
+++ b/drivers/pwm/pwm-lpc18xx.c
@@ -0,0 +1,459 @@
+/*
+ * NXP LPC18xx Pulse Width Modulator driver
+ *
+ * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * Notes
+ * =====
+ * NXP LPC18xx provides a State Configurable Timer (SCT) which can be configured
+ * as a Pulse Width Modulator.
+ *
+ * SCT supports 16 outputs, 16 events and 16 registers. Each event will be
+ * triggered when its related register matches the SCT counter value, and it
+ * will set or clear a selected output.
+ *
+ * One of the events is preselected to generate the period, thus the maximum
+ * number of simultaneous channels is limited to 15. Notice that period is
+ * global to all the channels, thus PWM driver will refuse setting different
+ * values to it.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/* LPC18xx SCT registers */
+#define LPC18XX_PWM_CONFIG 0x000
+#define LPC18XX_PWM_CONFIG_UNIFY BIT(0)
+#define LPC18XX_PWM_CONFIG_NORELOAD BIT(7)
+
+#define LPC18XX_PWM_CTRL 0x004
+#define LPC18XX_PWM_CTRL_HALT BIT(2)
+#define LPC18XX_PWM_BIDIR BIT(4)
+#define LPC18XX_PWM_PRE_SHIFT 5
+#define LPC18XX_PWM_PRE_MASK (0xff << LPC18XX_PWM_PRE_SHIFT)
+#define LPC18XX_PWM_PRE(x) (x << LPC18XX_PWM_PRE_SHIFT)
+
+#define LPC18XX_PWM_LIMIT 0x008
+
+#define LPC18XX_PWM_RES_BASE 0x058
+#define LPC18XX_PWM_RES_SHIFT(_ch) (_ch * 2)
+#define LPC18XX_PWM_RES(_ch, _action) (_action << LPC18XX_PWM_RES_SHIFT(_ch))
+#define LPC18XX_PWM_RES_MASK(_ch) (0x3 << LPC18XX_PWM_RES_SHIFT(_ch))
+
+#define LPC18XX_PWM_MATCH_BASE 0x100
+#define LPC18XX_PWM_MATCH(_ch) (LPC18XX_PWM_MATCH_BASE + _ch * 4)
+
+#define LPC18XX_PWM_MATCHREL_BASE 0x200
+#define LPC18XX_PWM_MATCHREL(_ch) (LPC18XX_PWM_MATCHREL_BASE + _ch * 4)
+
+#define LPC18XX_PWM_EVSTATEMSK_BASE 0x300
+#define LPC18XX_PWM_EVSTATEMSK(_ch) (LPC18XX_PWM_EVSTATEMSK_BASE + _ch * 8)
+#define LPC18XX_PWM_EVSTATEMSK_ALL 0xffffffff
+
+#define LPC18XX_PWM_EVCTRL_BASE 0x304
+#define LPC18XX_PWM_EVCTRL(_ch) (LPC18XX_PWM_EVCTRL_BASE + _ch * 8)
+
+#define LPC18XX_PWM_EVCTRL_MATCH(_ch) _ch
+
+#define LPC18XX_PWM_EVCTRL_COMB_SHIFT 12
+#define LPC18XX_PWM_EVCTRL_COMB_MATCH (0x1 << LPC18XX_PWM_EVCTRL_COMB_SHIFT)
+
+#define LPC18XX_PWM_OUTPUTSET_BASE 0x500
+#define LPC18XX_PWM_OUTPUTSET(_ch) (LPC18XX_PWM_OUTPUTSET_BASE + _ch * 8)
+
+#define LPC18XX_PWM_OUTPUTCL_BASE 0x504
+#define LPC18XX_PWM_OUTPUTCL(_ch) (LPC18XX_PWM_OUTPUTCL_BASE + _ch * 8)
+
+/* LPC18xx SCT unified counter */
+#define LPC18XX_PWM_TIMER_MAX 0xffffffff
+
+/* LPC18xx SCT events */
+#define LPC18XX_PWM_EVENT_PERIOD 0
+#define LPC18XX_PWM_EVENT_MAX 16
+
+/* SCT conflict resolution */
+enum lpc18xx_pwm_res_action {
+ LPC18XX_PWM_RES_NONE = 0x0,
+ LPC18XX_PWM_RES_SET = 0x1,
+ LPC18XX_PWM_RES_CLEAR = 0x2,
+ LPC18XX_PWM_RES_TOGGLE = 0x3,
+};
+
+struct lpc18xx_pwm_data {
+ unsigned int duty_event;
+};
+
+struct lpc18xx_pwm_chip {
+ struct device *dev;
+ struct pwm_chip chip;
+ void __iomem *base;
+ struct clk *pwm_clk;
+ unsigned long clk_rate;
+ unsigned int period_ns;
+ unsigned int min_period_ns;
+ unsigned int max_period_ns;
+ unsigned int period_event;
+ unsigned long event_map;
+ struct mutex res_lock;
+ struct mutex period_lock;
+};
+
+static inline struct lpc18xx_pwm_chip *to_lpc18xx_pwm_chip(
+ struct pwm_chip *chip)
+{
+ return container_of(chip, struct lpc18xx_pwm_chip, chip);
+}
+
+static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
+ u32 reg, u32 val)
+{
+ writel(val, lpc18xx_pwm->base + reg);
+}
+
+static inline u32 lpc18xx_pwm_readl(struct lpc18xx_pwm_chip *lpc18xx_pwm,
+ u32 reg)
+{
+ return readl(lpc18xx_pwm->base + reg);
+}
+
+static void lpc18xx_pwm_set_conflict_res(struct lpc18xx_pwm_chip *lpc18xx_pwm,
+ struct pwm_device *pwm,
+ enum lpc18xx_pwm_res_action action)
+{
+ u32 val;
+
+ mutex_lock(&lpc18xx_pwm->res_lock);
+
+ /*
+ * Simultaneous set and clear may happen on an output, that is the case
+ * when duty_ns == period_ns. LPC18xx SCT allows to set a conflict
+ * resolution action to be taken in such a case.
+ */
+ val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_RES_BASE);
+ val &= ~LPC18XX_PWM_RES_MASK(pwm->hwpwm);
+ val |= LPC18XX_PWM_RES(pwm->hwpwm, action);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_RES_BASE, val);
+
+ mutex_unlock(&lpc18xx_pwm->res_lock);
+}
+
+static void lpc18xx_pwm_config_period(struct pwm_chip *chip, int period_ns)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ u64 val;
+
+ val = (u64)period_ns * lpc18xx_pwm->clk_rate;
+ do_div(val, NSEC_PER_SEC);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCH(lpc18xx_pwm->period_event),
+ (u32)val);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCHREL(lpc18xx_pwm->period_event),
+ (u32)val);
+}
+
+static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
+ struct pwm_device *pwm, int duty_ns)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+ u64 val;
+
+ val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
+ do_div(val, NSEC_PER_SEC);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCH(lpc18xx_data->duty_event),
+ (u32)val);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_MATCHREL(lpc18xx_data->duty_event),
+ (u32)val);
+}
+
+static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ int i;
+
+ if (period_ns < lpc18xx_pwm->min_period_ns ||
+ period_ns > lpc18xx_pwm->max_period_ns) {
+ dev_err(chip->dev, "period %d not in range\n", period_ns);
+ return -ERANGE;
+ }
+
+ mutex_lock(&lpc18xx_pwm->period_lock);
+
+ /*
+ * The PWM supports only a single period for all PWM channels, therefore
+ * incompatible changes need to be refused.
+ */
+ if (lpc18xx_pwm->period_ns && lpc18xx_pwm->period_ns != period_ns) {
+ dev_err(chip->dev, "conflicting period requested for PWM %u\n",
+ pwm->hwpwm);
+ mutex_unlock(&lpc18xx_pwm->period_lock);
+ return -EBUSY;
+ }
+
+ if (!lpc18xx_pwm->period_ns) {
+ lpc18xx_pwm->period_ns = period_ns;
+ for (i = 0; i < chip->npwm; i++) {
+ pwm_set_period(&chip->pwms[i], period_ns);
+ }
+ lpc18xx_pwm_config_period(chip, period_ns);
+ }
+
+ mutex_unlock(&lpc18xx_pwm->period_lock);
+
+ lpc18xx_pwm_config_duty(chip, pwm, duty_ns);
+
+ return 0;
+}
+
+static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ return 0;
+}
+
+static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+ enum lpc18xx_pwm_res_action res_action;
+ unsigned int set_event, clear_event;
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event),
+ LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_data->duty_event) |
+ LPC18XX_PWM_EVCTRL_COMB_MATCH);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
+ LPC18XX_PWM_EVSTATEMSK_ALL);
+
+ if (pwm->polarity == PWM_POLARITY_NORMAL) {
+ set_event = lpc18xx_pwm->period_event;
+ clear_event = lpc18xx_data->duty_event;
+ res_action = LPC18XX_PWM_RES_SET;
+ } else {
+ set_event = lpc18xx_data->duty_event;
+ clear_event = lpc18xx_pwm->period_event;
+ res_action = LPC18XX_PWM_RES_CLEAR;
+ }
+
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm),
+ BIT(set_event));
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm),
+ BIT(clear_event));
+ lpc18xx_pwm_set_conflict_res(lpc18xx_pwm, pwm, res_action);
+
+ return 0;
+}
+
+static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm), 0);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm), 0);
+}
+
+static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+ unsigned long event;
+
+ event = find_first_zero_bit(&lpc18xx_pwm->event_map,
+ LPC18XX_PWM_EVENT_MAX);
+
+ if (event >= LPC18XX_PWM_EVENT_MAX) {
+ dev_err(lpc18xx_pwm->dev,
+ "maximum number of simultaneous channels reached\n");
+ return -EBUSY;
+ };
+
+ set_bit(event, &lpc18xx_pwm->event_map);
+ lpc18xx_data->duty_event = event;
+
+ return 0;
+}
+
+static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
+ struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+
+ lpc18xx_pwm_disable(chip, pwm);
+ clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
+}
+
+static const struct pwm_ops lpc18xx_pwm_ops = {
+ .config = lpc18xx_pwm_config,
+ .set_polarity = lpc18xx_pwm_set_polarity,
+ .enable = lpc18xx_pwm_enable,
+ .disable = lpc18xx_pwm_disable,
+ .request = lpc18xx_pwm_request,
+ .free = lpc18xx_pwm_free,
+ .owner = THIS_MODULE,
+};
+
+static const struct of_device_id lpc18xx_pwm_of_match[] = {
+ { .compatible = "nxp,lpc1850-pwm" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
+
+static int lpc18xx_pwm_probe(struct platform_device *pdev)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm;
+ struct pwm_device *pwm;
+ struct resource *res;
+ int ret, i;
+ u64 val;
+
+ lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
+ GFP_KERNEL);
+ if (!lpc18xx_pwm)
+ return -ENOMEM;
+
+ lpc18xx_pwm->dev = &pdev->dev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ lpc18xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(lpc18xx_pwm->base))
+ return PTR_ERR(lpc18xx_pwm->base);
+
+ lpc18xx_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
+ if (IS_ERR(lpc18xx_pwm->pwm_clk)) {
+ dev_err(&pdev->dev, "failed to get pwm clock\n");
+ return PTR_ERR(lpc18xx_pwm->pwm_clk);
+ }
+
+ ret = clk_prepare_enable(lpc18xx_pwm->pwm_clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "could not prepare or enable pwm clock\n");
+ return ret;
+ }
+
+ lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
+
+ mutex_init(&lpc18xx_pwm->res_lock);
+ mutex_init(&lpc18xx_pwm->period_lock);
+
+ val = (u64)NSEC_PER_SEC * LPC18XX_PWM_TIMER_MAX;
+ do_div(val, lpc18xx_pwm->clk_rate);
+ lpc18xx_pwm->max_period_ns = val;
+
+ val = (u64)NSEC_PER_SEC;
+ do_div(val, lpc18xx_pwm->clk_rate);
+ lpc18xx_pwm->min_period_ns = val;
+
+ lpc18xx_pwm->chip.dev = &pdev->dev;
+ lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
+ lpc18xx_pwm->chip.base = -1;
+ lpc18xx_pwm->chip.npwm = 16;
+ lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+ lpc18xx_pwm->chip.of_pwm_n_cells = 3;
+
+ /* SCT counter must be in unify (32 bit) mode */
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
+ LPC18XX_PWM_CONFIG_UNIFY);
+
+ /*
+ * Everytime the timer counter reaches the period value, the related
+ * event will be triggered and the counter reset to 0.
+ */
+ set_bit(LPC18XX_PWM_EVENT_PERIOD, &lpc18xx_pwm->event_map);
+ lpc18xx_pwm->period_event = LPC18XX_PWM_EVENT_PERIOD;
+
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVSTATEMSK(lpc18xx_pwm->period_event),
+ LPC18XX_PWM_EVSTATEMSK_ALL);
+
+ val = LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_pwm->period_event) |
+ LPC18XX_PWM_EVCTRL_COMB_MATCH;
+ lpc18xx_pwm_writel(lpc18xx_pwm,
+ LPC18XX_PWM_EVCTRL(lpc18xx_pwm->period_event), val);
+
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
+ BIT(lpc18xx_pwm->period_event));
+
+ ret = pwmchip_add(&lpc18xx_pwm->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
+ goto disable_pwmclk;
+ }
+
+ for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
+ pwm = &lpc18xx_pwm->chip.pwms[i];
+ pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
+ sizeof(struct lpc18xx_pwm_data),
+ GFP_KERNEL);
+ if (!pwm->chip_data) {
+ ret = -ENOMEM;
+ goto remove_pwmchip;
+ }
+ }
+
+ platform_set_drvdata(pdev, lpc18xx_pwm);
+
+ val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
+ val &= ~LPC18XX_PWM_BIDIR;
+ val &= ~LPC18XX_PWM_CTRL_HALT;
+ val &= ~LPC18XX_PWM_PRE_MASK;
+ val |= LPC18XX_PWM_PRE(0);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
+
+ return 0;
+
+remove_pwmchip:
+ pwmchip_remove(&lpc18xx_pwm->chip);
+disable_pwmclk:
+ clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
+ return ret;
+}
+
+static int lpc18xx_pwm_remove(struct platform_device *pdev)
+{
+ struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
+ u32 val;
+
+ val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
+ lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
+ val | LPC18XX_PWM_CTRL_HALT);
+
+ clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
+
+ return pwmchip_remove(&lpc18xx_pwm->chip);
+}
+
+static struct platform_driver lpc18xx_pwm_driver = {
+ .driver = {
+ .name = "lpc18xx-pwm",
+ .of_match_table = lpc18xx_pwm_of_match,
+ },
+ .probe = lpc18xx_pwm_probe,
+ .remove = lpc18xx_pwm_remove,
+};
+module_platform_driver(lpc18xx_pwm_driver);
+
+MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
+MODULE_DESCRIPTION("NXP LPC18xx PWM driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-27 4:45 ` Ariel D'Alessandro
@ 2015-07-27 4:45 ` Ariel D'Alessandro
-1 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-27 4:45 UTC (permalink / raw)
To: linux-pwm
Cc: linux-arm-kernel, manabian, ezequiel, thierry.reding,
Ariel D'Alessandro
Add the devicetree binding document for NXP LPC18xx PWM/SCT.
Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
.../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
new file mode 100644
index 0000000..3055429
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
@@ -0,0 +1,20 @@
+* NXP LPC18xx Pulse Width Modulator driver
+
+Required properties:
+ - compatible: Should be "nxp,lpc1850-pwm"
+ - reg: Should contain physical base address and length of pwm registers.
+ - clocks: Must contain an entry for each entry in clock-names.
+ See ../clock/clock-bindings.txt for details.
+ - clock-names: Must include the following entries.
+ - pwm: PWM operating clock.
+ - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
+ of the cells format.
+
+Example:
+ pwm: pwm@40000000 {
+ compatible = "nxp,lpc1850-pwm";
+ reg = <0x40000000 0x580>;
+ clocks =<&ccu1 CLK_CPU_SCT>;
+ clock-names = "pwm";
+ #pwm-cells = <3>;
+ };
--
1.9.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-27 4:45 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-27 4:45 UTC (permalink / raw)
To: linux-arm-kernel
Add the devicetree binding document for NXP LPC18xx PWM/SCT.
Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
---
.../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
new file mode 100644
index 0000000..3055429
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
@@ -0,0 +1,20 @@
+* NXP LPC18xx Pulse Width Modulator driver
+
+Required properties:
+ - compatible: Should be "nxp,lpc1850-pwm"
+ - reg: Should contain physical base address and length of pwm registers.
+ - clocks: Must contain an entry for each entry in clock-names.
+ See ../clock/clock-bindings.txt for details.
+ - clock-names: Must include the following entries.
+ - pwm: PWM operating clock.
+ - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
+ of the cells format.
+
+Example:
+ pwm: pwm at 40000000 {
+ compatible = "nxp,lpc1850-pwm";
+ reg = <0x40000000 0x580>;
+ clocks =<&ccu1 CLK_CPU_SCT>;
+ clock-names = "pwm";
+ #pwm-cells = <3>;
+ };
--
1.9.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver
2015-07-27 4:45 ` Ariel D'Alessandro
@ 2015-07-28 19:48 ` Ezequiel Garcia
-1 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-28 19:48 UTC (permalink / raw)
To: Ariel D'Alessandro
Cc: linux-pwm, linux-arm-kernel, Joachim Eastwood, Thierry Reding
Ariel,
On 27 July 2015 at 01:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
> This commit adds support for NXP LPC18xx PWM/SCT.
>
> NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
> Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
> Other SoCs in that family may share the same hardware.
>
> The PWM supports a total of 16 channels, but only 15 can be simultaneously
> requested. There's only one period, global to all the channels, thus PWM driver
> will refuse setting different values to it.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
> drivers/pwm/Kconfig | 12 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-lpc18xx.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 472 insertions(+)
> create mode 100644 drivers/pwm/pwm-lpc18xx.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..2c7635c 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -173,6 +173,18 @@ config PWM_LP3943
> To compile this driver as a module, choose M here: the module
> will be called pwm-lp3943.
>
> +config PWM_LPC18XX
> + tristate "LPC18xx/43xx PWM support"
> + depends on ARCH_LPC18XX
> + help
> + Generic PWM framework driver for NXP LPC18xx PWM/SCT which
> + supports 16 channels.
> + A maximum of 15 channels can be requested simultaneously and
> + must have the same period.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-lpc18xx.
> +
> config PWM_LPC32XX
> tristate "LPC32XX PWM support"
> depends on ARCH_LPC32XX
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5..986b286 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> +obj-$(CONFIG_PWM_LPC18XX) += pwm-lpc18xx.o
> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
> obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
> diff --git a/drivers/pwm/pwm-lpc18xx.c b/drivers/pwm/pwm-lpc18xx.c
> new file mode 100644
> index 0000000..77b5e74
> --- /dev/null
> +++ b/drivers/pwm/pwm-lpc18xx.c
> @@ -0,0 +1,459 @@
> +/*
> + * NXP LPC18xx Pulse Width Modulator driver
> + *
> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * Notes
> + * =====
> + * NXP LPC18xx provides a State Configurable Timer (SCT) which can be configured
> + * as a Pulse Width Modulator.
> + *
> + * SCT supports 16 outputs, 16 events and 16 registers. Each event will be
> + * triggered when its related register matches the SCT counter value, and it
> + * will set or clear a selected output.
> + *
> + * One of the events is preselected to generate the period, thus the maximum
> + * number of simultaneous channels is limited to 15. Notice that period is
> + * global to all the channels, thus PWM driver will refuse setting different
> + * values to it.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +/* LPC18xx SCT registers */
> +#define LPC18XX_PWM_CONFIG 0x000
> +#define LPC18XX_PWM_CONFIG_UNIFY BIT(0)
> +#define LPC18XX_PWM_CONFIG_NORELOAD BIT(7)
> +
> +#define LPC18XX_PWM_CTRL 0x004
> +#define LPC18XX_PWM_CTRL_HALT BIT(2)
> +#define LPC18XX_PWM_BIDIR BIT(4)
> +#define LPC18XX_PWM_PRE_SHIFT 5
> +#define LPC18XX_PWM_PRE_MASK (0xff << LPC18XX_PWM_PRE_SHIFT)
> +#define LPC18XX_PWM_PRE(x) (x << LPC18XX_PWM_PRE_SHIFT)
> +
> +#define LPC18XX_PWM_LIMIT 0x008
> +
> +#define LPC18XX_PWM_RES_BASE 0x058
> +#define LPC18XX_PWM_RES_SHIFT(_ch) (_ch * 2)
> +#define LPC18XX_PWM_RES(_ch, _action) (_action << LPC18XX_PWM_RES_SHIFT(_ch))
> +#define LPC18XX_PWM_RES_MASK(_ch) (0x3 << LPC18XX_PWM_RES_SHIFT(_ch))
> +
> +#define LPC18XX_PWM_MATCH_BASE 0x100
> +#define LPC18XX_PWM_MATCH(_ch) (LPC18XX_PWM_MATCH_BASE + _ch * 4)
> +
> +#define LPC18XX_PWM_MATCHREL_BASE 0x200
> +#define LPC18XX_PWM_MATCHREL(_ch) (LPC18XX_PWM_MATCHREL_BASE + _ch * 4)
> +
> +#define LPC18XX_PWM_EVSTATEMSK_BASE 0x300
> +#define LPC18XX_PWM_EVSTATEMSK(_ch) (LPC18XX_PWM_EVSTATEMSK_BASE + _ch * 8)
> +#define LPC18XX_PWM_EVSTATEMSK_ALL 0xffffffff
> +
> +#define LPC18XX_PWM_EVCTRL_BASE 0x304
> +#define LPC18XX_PWM_EVCTRL(_ch) (LPC18XX_PWM_EVCTRL_BASE + _ch * 8)
> +
I think some of these are not channels (you are passing duty_event
to them) and so you shouldn't call the argument "_ch".
> +#define LPC18XX_PWM_EVCTRL_MATCH(_ch) _ch
> +
> +#define LPC18XX_PWM_EVCTRL_COMB_SHIFT 12
> +#define LPC18XX_PWM_EVCTRL_COMB_MATCH (0x1 << LPC18XX_PWM_EVCTRL_COMB_SHIFT)
> +
> +#define LPC18XX_PWM_OUTPUTSET_BASE 0x500
> +#define LPC18XX_PWM_OUTPUTSET(_ch) (LPC18XX_PWM_OUTPUTSET_BASE + _ch * 8)
> +
> +#define LPC18XX_PWM_OUTPUTCL_BASE 0x504
> +#define LPC18XX_PWM_OUTPUTCL(_ch) (LPC18XX_PWM_OUTPUTCL_BASE + _ch * 8)
> +
> +/* LPC18xx SCT unified counter */
> +#define LPC18XX_PWM_TIMER_MAX 0xffffffff
> +
> +/* LPC18xx SCT events */
> +#define LPC18XX_PWM_EVENT_PERIOD 0
> +#define LPC18XX_PWM_EVENT_MAX 16
> +
> +/* SCT conflict resolution */
> +enum lpc18xx_pwm_res_action {
> + LPC18XX_PWM_RES_NONE = 0x0,
> + LPC18XX_PWM_RES_SET = 0x1,
> + LPC18XX_PWM_RES_CLEAR = 0x2,
> + LPC18XX_PWM_RES_TOGGLE = 0x3,
> +};
> +
> +struct lpc18xx_pwm_data {
> + unsigned int duty_event;
> +};
> +
> +struct lpc18xx_pwm_chip {
> + struct device *dev;
> + struct pwm_chip chip;
> + void __iomem *base;
> + struct clk *pwm_clk;
> + unsigned long clk_rate;
> + unsigned int period_ns;
> + unsigned int min_period_ns;
> + unsigned int max_period_ns;
> + unsigned int period_event;
> + unsigned long event_map;
> + struct mutex res_lock;
> + struct mutex period_lock;
> +};
> +
> +static inline struct lpc18xx_pwm_chip *to_lpc18xx_pwm_chip(
> + struct pwm_chip *chip)
> +{
> + return container_of(chip, struct lpc18xx_pwm_chip, chip);
> +}
> +
> +static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
> + u32 reg, u32 val)
> +{
> + writel(val, lpc18xx_pwm->base + reg);
> +}
> +
> +static inline u32 lpc18xx_pwm_readl(struct lpc18xx_pwm_chip *lpc18xx_pwm,
> + u32 reg)
> +{
> + return readl(lpc18xx_pwm->base + reg);
> +}
> +
> +static void lpc18xx_pwm_set_conflict_res(struct lpc18xx_pwm_chip *lpc18xx_pwm,
> + struct pwm_device *pwm,
> + enum lpc18xx_pwm_res_action action)
> +{
> + u32 val;
> +
> + mutex_lock(&lpc18xx_pwm->res_lock);
> +
> + /*
> + * Simultaneous set and clear may happen on an output, that is the case
> + * when duty_ns == period_ns. LPC18xx SCT allows to set a conflict
> + * resolution action to be taken in such a case.
> + */
> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_RES_BASE);
> + val &= ~LPC18XX_PWM_RES_MASK(pwm->hwpwm);
> + val |= LPC18XX_PWM_RES(pwm->hwpwm, action);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_RES_BASE, val);
> +
> + mutex_unlock(&lpc18xx_pwm->res_lock);
> +}
> +
> +static void lpc18xx_pwm_config_period(struct pwm_chip *chip, int period_ns)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + u64 val;
> +
> + val = (u64)period_ns * lpc18xx_pwm->clk_rate;
> + do_div(val, NSEC_PER_SEC);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCH(lpc18xx_pwm->period_event),
> + (u32)val);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCHREL(lpc18xx_pwm->period_event),
> + (u32)val);
> +}
> +
> +static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
> + struct pwm_device *pwm, int duty_ns)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> + u64 val;
> +
> + val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
> + do_div(val, NSEC_PER_SEC);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCH(lpc18xx_data->duty_event),
> + (u32)val);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCHREL(lpc18xx_data->duty_event),
> + (u32)val);
> +}
> +
> +static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + int i;
> +
> + if (period_ns < lpc18xx_pwm->min_period_ns ||
> + period_ns > lpc18xx_pwm->max_period_ns) {
> + dev_err(chip->dev, "period %d not in range\n", period_ns);
> + return -ERANGE;
> + }
> +
> + mutex_lock(&lpc18xx_pwm->period_lock);
> +
> + /*
> + * The PWM supports only a single period for all PWM channels, therefore
> + * incompatible changes need to be refused.
> + */
> + if (lpc18xx_pwm->period_ns && lpc18xx_pwm->period_ns != period_ns) {
> + dev_err(chip->dev, "conflicting period requested for PWM %u\n",
> + pwm->hwpwm);
> + mutex_unlock(&lpc18xx_pwm->period_lock);
> + return -EBUSY;
> + }
> +
> + if (!lpc18xx_pwm->period_ns) {
> + lpc18xx_pwm->period_ns = period_ns;
You set the period_ns, which is constrained to be a global period
for all channels.
However, you are not clearing this. So there's no way to change the period.
I believe you should allow to change the period if there's only a single
channel enabled.
Thus, only forbid when changing the period of a channel that hasn't
requested period change.
> + for (i = 0; i < chip->npwm; i++) {
> + pwm_set_period(&chip->pwms[i], period_ns);
> + }
> + lpc18xx_pwm_config_period(chip, period_ns);
> + }
> +
> + mutex_unlock(&lpc18xx_pwm->period_lock);
> +
> + lpc18xx_pwm_config_duty(chip, pwm, duty_ns);
> +
> + return 0;
> +}
> +
> +static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + return 0;
> +}
> +
> +static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> + enum lpc18xx_pwm_res_action res_action;
> + unsigned int set_event, clear_event;
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event),
> + LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_data->duty_event) |
> + LPC18XX_PWM_EVCTRL_COMB_MATCH);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
> + LPC18XX_PWM_EVSTATEMSK_ALL);
> +
> + if (pwm->polarity == PWM_POLARITY_NORMAL) {
> + set_event = lpc18xx_pwm->period_event;
> + clear_event = lpc18xx_data->duty_event;
> + res_action = LPC18XX_PWM_RES_SET;
> + } else {
> + set_event = lpc18xx_data->duty_event;
> + clear_event = lpc18xx_pwm->period_event;
> + res_action = LPC18XX_PWM_RES_CLEAR;
> + }
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm),
> + BIT(set_event));
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm),
> + BIT(clear_event));
> + lpc18xx_pwm_set_conflict_res(lpc18xx_pwm, pwm, res_action);
> +
> + return 0;
> +}
> +
> +static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm), 0);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm), 0);
> +}
> +
> +static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> + unsigned long event;
> +
> + event = find_first_zero_bit(&lpc18xx_pwm->event_map,
> + LPC18XX_PWM_EVENT_MAX);
> +
> + if (event >= LPC18XX_PWM_EVENT_MAX) {
> + dev_err(lpc18xx_pwm->dev,
> + "maximum number of simultaneous channels reached\n");
> + return -EBUSY;
> + };
> +
> + set_bit(event, &lpc18xx_pwm->event_map);
> + lpc18xx_data->duty_event = event;
> +
> + return 0;
> +}
> +
> +static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> +
> + lpc18xx_pwm_disable(chip, pwm);
> + clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
> +}
> +
> +static const struct pwm_ops lpc18xx_pwm_ops = {
> + .config = lpc18xx_pwm_config,
> + .set_polarity = lpc18xx_pwm_set_polarity,
> + .enable = lpc18xx_pwm_enable,
> + .disable = lpc18xx_pwm_disable,
> + .request = lpc18xx_pwm_request,
> + .free = lpc18xx_pwm_free,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id lpc18xx_pwm_of_match[] = {
> + { .compatible = "nxp,lpc1850-pwm" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
> +
> +static int lpc18xx_pwm_probe(struct platform_device *pdev)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm;
> + struct pwm_device *pwm;
> + struct resource *res;
> + int ret, i;
> + u64 val;
> +
> + lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
> + GFP_KERNEL);
> + if (!lpc18xx_pwm)
> + return -ENOMEM;
> +
> + lpc18xx_pwm->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + lpc18xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(lpc18xx_pwm->base))
> + return PTR_ERR(lpc18xx_pwm->base);
> +
> + lpc18xx_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(lpc18xx_pwm->pwm_clk)) {
> + dev_err(&pdev->dev, "failed to get pwm clock\n");
> + return PTR_ERR(lpc18xx_pwm->pwm_clk);
> + }
> +
> + ret = clk_prepare_enable(lpc18xx_pwm->pwm_clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "could not prepare or enable pwm clock\n");
> + return ret;
> + }
> +
> + lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
> +
> + mutex_init(&lpc18xx_pwm->res_lock);
> + mutex_init(&lpc18xx_pwm->period_lock);
> +
> + val = (u64)NSEC_PER_SEC * LPC18XX_PWM_TIMER_MAX;
> + do_div(val, lpc18xx_pwm->clk_rate);
> + lpc18xx_pwm->max_period_ns = val;
> +
> + val = (u64)NSEC_PER_SEC;
> + do_div(val, lpc18xx_pwm->clk_rate);
> + lpc18xx_pwm->min_period_ns = val;
> +
> + lpc18xx_pwm->chip.dev = &pdev->dev;
> + lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
> + lpc18xx_pwm->chip.base = -1;
> + lpc18xx_pwm->chip.npwm = 16;
> + lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> + lpc18xx_pwm->chip.of_pwm_n_cells = 3;
> +
> + /* SCT counter must be in unify (32 bit) mode */
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
> + LPC18XX_PWM_CONFIG_UNIFY);
> +
> + /*
> + * Everytime the timer counter reaches the period value, the related
> + * event will be triggered and the counter reset to 0.
> + */
> + set_bit(LPC18XX_PWM_EVENT_PERIOD, &lpc18xx_pwm->event_map);
> + lpc18xx_pwm->period_event = LPC18XX_PWM_EVENT_PERIOD;
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_pwm->period_event),
> + LPC18XX_PWM_EVSTATEMSK_ALL);
> +
> + val = LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_pwm->period_event) |
> + LPC18XX_PWM_EVCTRL_COMB_MATCH;
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVCTRL(lpc18xx_pwm->period_event), val);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
> + BIT(lpc18xx_pwm->period_event));
> +
> + ret = pwmchip_add(&lpc18xx_pwm->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
> + goto disable_pwmclk;
> + }
> +
> + for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
> + pwm = &lpc18xx_pwm->chip.pwms[i];
> + pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
> + sizeof(struct lpc18xx_pwm_data),
> + GFP_KERNEL);
> + if (!pwm->chip_data) {
> + ret = -ENOMEM;
> + goto remove_pwmchip;
> + }
> + }
> +
> + platform_set_drvdata(pdev, lpc18xx_pwm);
> +
> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
> + val &= ~LPC18XX_PWM_BIDIR;
> + val &= ~LPC18XX_PWM_CTRL_HALT;
> + val &= ~LPC18XX_PWM_PRE_MASK;
> + val |= LPC18XX_PWM_PRE(0);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
> +
> + return 0;
> +
> +remove_pwmchip:
> + pwmchip_remove(&lpc18xx_pwm->chip);
> +disable_pwmclk:
> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
> + return ret;
> +}
> +
> +static int lpc18xx_pwm_remove(struct platform_device *pdev)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
> + u32 val;
> +
> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
> + val | LPC18XX_PWM_CTRL_HALT);
> +
> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
> +
> + return pwmchip_remove(&lpc18xx_pwm->chip);
> +}
> +
> +static struct platform_driver lpc18xx_pwm_driver = {
> + .driver = {
> + .name = "lpc18xx-pwm",
> + .of_match_table = lpc18xx_pwm_of_match,
> + },
> + .probe = lpc18xx_pwm_probe,
> + .remove = lpc18xx_pwm_remove,
> +};
> +module_platform_driver(lpc18xx_pwm_driver);
> +
> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
> +MODULE_DESCRIPTION("NXP LPC18xx PWM driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver
@ 2015-07-28 19:48 ` Ezequiel Garcia
0 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-28 19:48 UTC (permalink / raw)
To: linux-arm-kernel
Ariel,
On 27 July 2015 at 01:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
> This commit adds support for NXP LPC18xx PWM/SCT.
>
> NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
> Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
> Other SoCs in that family may share the same hardware.
>
> The PWM supports a total of 16 channels, but only 15 can be simultaneously
> requested. There's only one period, global to all the channels, thus PWM driver
> will refuse setting different values to it.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
> drivers/pwm/Kconfig | 12 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-lpc18xx.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 472 insertions(+)
> create mode 100644 drivers/pwm/pwm-lpc18xx.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..2c7635c 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -173,6 +173,18 @@ config PWM_LP3943
> To compile this driver as a module, choose M here: the module
> will be called pwm-lp3943.
>
> +config PWM_LPC18XX
> + tristate "LPC18xx/43xx PWM support"
> + depends on ARCH_LPC18XX
> + help
> + Generic PWM framework driver for NXP LPC18xx PWM/SCT which
> + supports 16 channels.
> + A maximum of 15 channels can be requested simultaneously and
> + must have the same period.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-lpc18xx.
> +
> config PWM_LPC32XX
> tristate "LPC32XX PWM support"
> depends on ARCH_LPC32XX
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5..986b286 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
> obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> +obj-$(CONFIG_PWM_LPC18XX) += pwm-lpc18xx.o
> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
> obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
> diff --git a/drivers/pwm/pwm-lpc18xx.c b/drivers/pwm/pwm-lpc18xx.c
> new file mode 100644
> index 0000000..77b5e74
> --- /dev/null
> +++ b/drivers/pwm/pwm-lpc18xx.c
> @@ -0,0 +1,459 @@
> +/*
> + * NXP LPC18xx Pulse Width Modulator driver
> + *
> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License.
> + *
> + * Notes
> + * =====
> + * NXP LPC18xx provides a State Configurable Timer (SCT) which can be configured
> + * as a Pulse Width Modulator.
> + *
> + * SCT supports 16 outputs, 16 events and 16 registers. Each event will be
> + * triggered when its related register matches the SCT counter value, and it
> + * will set or clear a selected output.
> + *
> + * One of the events is preselected to generate the period, thus the maximum
> + * number of simultaneous channels is limited to 15. Notice that period is
> + * global to all the channels, thus PWM driver will refuse setting different
> + * values to it.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +/* LPC18xx SCT registers */
> +#define LPC18XX_PWM_CONFIG 0x000
> +#define LPC18XX_PWM_CONFIG_UNIFY BIT(0)
> +#define LPC18XX_PWM_CONFIG_NORELOAD BIT(7)
> +
> +#define LPC18XX_PWM_CTRL 0x004
> +#define LPC18XX_PWM_CTRL_HALT BIT(2)
> +#define LPC18XX_PWM_BIDIR BIT(4)
> +#define LPC18XX_PWM_PRE_SHIFT 5
> +#define LPC18XX_PWM_PRE_MASK (0xff << LPC18XX_PWM_PRE_SHIFT)
> +#define LPC18XX_PWM_PRE(x) (x << LPC18XX_PWM_PRE_SHIFT)
> +
> +#define LPC18XX_PWM_LIMIT 0x008
> +
> +#define LPC18XX_PWM_RES_BASE 0x058
> +#define LPC18XX_PWM_RES_SHIFT(_ch) (_ch * 2)
> +#define LPC18XX_PWM_RES(_ch, _action) (_action << LPC18XX_PWM_RES_SHIFT(_ch))
> +#define LPC18XX_PWM_RES_MASK(_ch) (0x3 << LPC18XX_PWM_RES_SHIFT(_ch))
> +
> +#define LPC18XX_PWM_MATCH_BASE 0x100
> +#define LPC18XX_PWM_MATCH(_ch) (LPC18XX_PWM_MATCH_BASE + _ch * 4)
> +
> +#define LPC18XX_PWM_MATCHREL_BASE 0x200
> +#define LPC18XX_PWM_MATCHREL(_ch) (LPC18XX_PWM_MATCHREL_BASE + _ch * 4)
> +
> +#define LPC18XX_PWM_EVSTATEMSK_BASE 0x300
> +#define LPC18XX_PWM_EVSTATEMSK(_ch) (LPC18XX_PWM_EVSTATEMSK_BASE + _ch * 8)
> +#define LPC18XX_PWM_EVSTATEMSK_ALL 0xffffffff
> +
> +#define LPC18XX_PWM_EVCTRL_BASE 0x304
> +#define LPC18XX_PWM_EVCTRL(_ch) (LPC18XX_PWM_EVCTRL_BASE + _ch * 8)
> +
I think some of these are not channels (you are passing duty_event
to them) and so you shouldn't call the argument "_ch".
> +#define LPC18XX_PWM_EVCTRL_MATCH(_ch) _ch
> +
> +#define LPC18XX_PWM_EVCTRL_COMB_SHIFT 12
> +#define LPC18XX_PWM_EVCTRL_COMB_MATCH (0x1 << LPC18XX_PWM_EVCTRL_COMB_SHIFT)
> +
> +#define LPC18XX_PWM_OUTPUTSET_BASE 0x500
> +#define LPC18XX_PWM_OUTPUTSET(_ch) (LPC18XX_PWM_OUTPUTSET_BASE + _ch * 8)
> +
> +#define LPC18XX_PWM_OUTPUTCL_BASE 0x504
> +#define LPC18XX_PWM_OUTPUTCL(_ch) (LPC18XX_PWM_OUTPUTCL_BASE + _ch * 8)
> +
> +/* LPC18xx SCT unified counter */
> +#define LPC18XX_PWM_TIMER_MAX 0xffffffff
> +
> +/* LPC18xx SCT events */
> +#define LPC18XX_PWM_EVENT_PERIOD 0
> +#define LPC18XX_PWM_EVENT_MAX 16
> +
> +/* SCT conflict resolution */
> +enum lpc18xx_pwm_res_action {
> + LPC18XX_PWM_RES_NONE = 0x0,
> + LPC18XX_PWM_RES_SET = 0x1,
> + LPC18XX_PWM_RES_CLEAR = 0x2,
> + LPC18XX_PWM_RES_TOGGLE = 0x3,
> +};
> +
> +struct lpc18xx_pwm_data {
> + unsigned int duty_event;
> +};
> +
> +struct lpc18xx_pwm_chip {
> + struct device *dev;
> + struct pwm_chip chip;
> + void __iomem *base;
> + struct clk *pwm_clk;
> + unsigned long clk_rate;
> + unsigned int period_ns;
> + unsigned int min_period_ns;
> + unsigned int max_period_ns;
> + unsigned int period_event;
> + unsigned long event_map;
> + struct mutex res_lock;
> + struct mutex period_lock;
> +};
> +
> +static inline struct lpc18xx_pwm_chip *to_lpc18xx_pwm_chip(
> + struct pwm_chip *chip)
> +{
> + return container_of(chip, struct lpc18xx_pwm_chip, chip);
> +}
> +
> +static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
> + u32 reg, u32 val)
> +{
> + writel(val, lpc18xx_pwm->base + reg);
> +}
> +
> +static inline u32 lpc18xx_pwm_readl(struct lpc18xx_pwm_chip *lpc18xx_pwm,
> + u32 reg)
> +{
> + return readl(lpc18xx_pwm->base + reg);
> +}
> +
> +static void lpc18xx_pwm_set_conflict_res(struct lpc18xx_pwm_chip *lpc18xx_pwm,
> + struct pwm_device *pwm,
> + enum lpc18xx_pwm_res_action action)
> +{
> + u32 val;
> +
> + mutex_lock(&lpc18xx_pwm->res_lock);
> +
> + /*
> + * Simultaneous set and clear may happen on an output, that is the case
> + * when duty_ns == period_ns. LPC18xx SCT allows to set a conflict
> + * resolution action to be taken in such a case.
> + */
> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_RES_BASE);
> + val &= ~LPC18XX_PWM_RES_MASK(pwm->hwpwm);
> + val |= LPC18XX_PWM_RES(pwm->hwpwm, action);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_RES_BASE, val);
> +
> + mutex_unlock(&lpc18xx_pwm->res_lock);
> +}
> +
> +static void lpc18xx_pwm_config_period(struct pwm_chip *chip, int period_ns)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + u64 val;
> +
> + val = (u64)period_ns * lpc18xx_pwm->clk_rate;
> + do_div(val, NSEC_PER_SEC);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCH(lpc18xx_pwm->period_event),
> + (u32)val);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCHREL(lpc18xx_pwm->period_event),
> + (u32)val);
> +}
> +
> +static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
> + struct pwm_device *pwm, int duty_ns)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> + u64 val;
> +
> + val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
> + do_div(val, NSEC_PER_SEC);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCH(lpc18xx_data->duty_event),
> + (u32)val);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_MATCHREL(lpc18xx_data->duty_event),
> + (u32)val);
> +}
> +
> +static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + int i;
> +
> + if (period_ns < lpc18xx_pwm->min_period_ns ||
> + period_ns > lpc18xx_pwm->max_period_ns) {
> + dev_err(chip->dev, "period %d not in range\n", period_ns);
> + return -ERANGE;
> + }
> +
> + mutex_lock(&lpc18xx_pwm->period_lock);
> +
> + /*
> + * The PWM supports only a single period for all PWM channels, therefore
> + * incompatible changes need to be refused.
> + */
> + if (lpc18xx_pwm->period_ns && lpc18xx_pwm->period_ns != period_ns) {
> + dev_err(chip->dev, "conflicting period requested for PWM %u\n",
> + pwm->hwpwm);
> + mutex_unlock(&lpc18xx_pwm->period_lock);
> + return -EBUSY;
> + }
> +
> + if (!lpc18xx_pwm->period_ns) {
> + lpc18xx_pwm->period_ns = period_ns;
You set the period_ns, which is constrained to be a global period
for all channels.
However, you are not clearing this. So there's no way to change the period.
I believe you should allow to change the period if there's only a single
channel enabled.
Thus, only forbid when changing the period of a channel that hasn't
requested period change.
> + for (i = 0; i < chip->npwm; i++) {
> + pwm_set_period(&chip->pwms[i], period_ns);
> + }
> + lpc18xx_pwm_config_period(chip, period_ns);
> + }
> +
> + mutex_unlock(&lpc18xx_pwm->period_lock);
> +
> + lpc18xx_pwm_config_duty(chip, pwm, duty_ns);
> +
> + return 0;
> +}
> +
> +static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + return 0;
> +}
> +
> +static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> + enum lpc18xx_pwm_res_action res_action;
> + unsigned int set_event, clear_event;
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event),
> + LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_data->duty_event) |
> + LPC18XX_PWM_EVCTRL_COMB_MATCH);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
> + LPC18XX_PWM_EVSTATEMSK_ALL);
> +
> + if (pwm->polarity == PWM_POLARITY_NORMAL) {
> + set_event = lpc18xx_pwm->period_event;
> + clear_event = lpc18xx_data->duty_event;
> + res_action = LPC18XX_PWM_RES_SET;
> + } else {
> + set_event = lpc18xx_data->duty_event;
> + clear_event = lpc18xx_pwm->period_event;
> + res_action = LPC18XX_PWM_RES_CLEAR;
> + }
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm),
> + BIT(set_event));
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm),
> + BIT(clear_event));
> + lpc18xx_pwm_set_conflict_res(lpc18xx_pwm, pwm, res_action);
> +
> + return 0;
> +}
> +
> +static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm), 0);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm), 0);
> +}
> +
> +static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> + unsigned long event;
> +
> + event = find_first_zero_bit(&lpc18xx_pwm->event_map,
> + LPC18XX_PWM_EVENT_MAX);
> +
> + if (event >= LPC18XX_PWM_EVENT_MAX) {
> + dev_err(lpc18xx_pwm->dev,
> + "maximum number of simultaneous channels reached\n");
> + return -EBUSY;
> + };
> +
> + set_bit(event, &lpc18xx_pwm->event_map);
> + lpc18xx_data->duty_event = event;
> +
> + return 0;
> +}
> +
> +static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
> +
> + lpc18xx_pwm_disable(chip, pwm);
> + clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
> +}
> +
> +static const struct pwm_ops lpc18xx_pwm_ops = {
> + .config = lpc18xx_pwm_config,
> + .set_polarity = lpc18xx_pwm_set_polarity,
> + .enable = lpc18xx_pwm_enable,
> + .disable = lpc18xx_pwm_disable,
> + .request = lpc18xx_pwm_request,
> + .free = lpc18xx_pwm_free,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id lpc18xx_pwm_of_match[] = {
> + { .compatible = "nxp,lpc1850-pwm" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
> +
> +static int lpc18xx_pwm_probe(struct platform_device *pdev)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm;
> + struct pwm_device *pwm;
> + struct resource *res;
> + int ret, i;
> + u64 val;
> +
> + lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
> + GFP_KERNEL);
> + if (!lpc18xx_pwm)
> + return -ENOMEM;
> +
> + lpc18xx_pwm->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + lpc18xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(lpc18xx_pwm->base))
> + return PTR_ERR(lpc18xx_pwm->base);
> +
> + lpc18xx_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(lpc18xx_pwm->pwm_clk)) {
> + dev_err(&pdev->dev, "failed to get pwm clock\n");
> + return PTR_ERR(lpc18xx_pwm->pwm_clk);
> + }
> +
> + ret = clk_prepare_enable(lpc18xx_pwm->pwm_clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "could not prepare or enable pwm clock\n");
> + return ret;
> + }
> +
> + lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
> +
> + mutex_init(&lpc18xx_pwm->res_lock);
> + mutex_init(&lpc18xx_pwm->period_lock);
> +
> + val = (u64)NSEC_PER_SEC * LPC18XX_PWM_TIMER_MAX;
> + do_div(val, lpc18xx_pwm->clk_rate);
> + lpc18xx_pwm->max_period_ns = val;
> +
> + val = (u64)NSEC_PER_SEC;
> + do_div(val, lpc18xx_pwm->clk_rate);
> + lpc18xx_pwm->min_period_ns = val;
> +
> + lpc18xx_pwm->chip.dev = &pdev->dev;
> + lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
> + lpc18xx_pwm->chip.base = -1;
> + lpc18xx_pwm->chip.npwm = 16;
> + lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> + lpc18xx_pwm->chip.of_pwm_n_cells = 3;
> +
> + /* SCT counter must be in unify (32 bit) mode */
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
> + LPC18XX_PWM_CONFIG_UNIFY);
> +
> + /*
> + * Everytime the timer counter reaches the period value, the related
> + * event will be triggered and the counter reset to 0.
> + */
> + set_bit(LPC18XX_PWM_EVENT_PERIOD, &lpc18xx_pwm->event_map);
> + lpc18xx_pwm->period_event = LPC18XX_PWM_EVENT_PERIOD;
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_pwm->period_event),
> + LPC18XX_PWM_EVSTATEMSK_ALL);
> +
> + val = LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_pwm->period_event) |
> + LPC18XX_PWM_EVCTRL_COMB_MATCH;
> + lpc18xx_pwm_writel(lpc18xx_pwm,
> + LPC18XX_PWM_EVCTRL(lpc18xx_pwm->period_event), val);
> +
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
> + BIT(lpc18xx_pwm->period_event));
> +
> + ret = pwmchip_add(&lpc18xx_pwm->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
> + goto disable_pwmclk;
> + }
> +
> + for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
> + pwm = &lpc18xx_pwm->chip.pwms[i];
> + pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
> + sizeof(struct lpc18xx_pwm_data),
> + GFP_KERNEL);
> + if (!pwm->chip_data) {
> + ret = -ENOMEM;
> + goto remove_pwmchip;
> + }
> + }
> +
> + platform_set_drvdata(pdev, lpc18xx_pwm);
> +
> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
> + val &= ~LPC18XX_PWM_BIDIR;
> + val &= ~LPC18XX_PWM_CTRL_HALT;
> + val &= ~LPC18XX_PWM_PRE_MASK;
> + val |= LPC18XX_PWM_PRE(0);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
> +
> + return 0;
> +
> +remove_pwmchip:
> + pwmchip_remove(&lpc18xx_pwm->chip);
> +disable_pwmclk:
> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
> + return ret;
> +}
> +
> +static int lpc18xx_pwm_remove(struct platform_device *pdev)
> +{
> + struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
> + u32 val;
> +
> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
> + val | LPC18XX_PWM_CTRL_HALT);
> +
> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
> +
> + return pwmchip_remove(&lpc18xx_pwm->chip);
> +}
> +
> +static struct platform_driver lpc18xx_pwm_driver = {
> + .driver = {
> + .name = "lpc18xx-pwm",
> + .of_match_table = lpc18xx_pwm_of_match,
> + },
> + .probe = lpc18xx_pwm_probe,
> + .remove = lpc18xx_pwm_remove,
> +};
> +module_platform_driver(lpc18xx_pwm_driver);
> +
> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
> +MODULE_DESCRIPTION("NXP LPC18xx PWM driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
--
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-27 4:45 ` Ariel D'Alessandro
@ 2015-07-28 22:37 ` Joachim Eastwood
-1 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-07-28 22:37 UTC (permalink / raw)
To: Ariel D'Alessandro
Cc: linux-pwm, linux-arm-kernel, Ezequiel Garcia, Thierry Reding
On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
> new file mode 100644
> index 0000000..3055429
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
> @@ -0,0 +1,20 @@
> +* NXP LPC18xx Pulse Width Modulator driver
> +
> +Required properties:
> + - compatible: Should be "nxp,lpc1850-pwm"
> + - reg: Should contain physical base address and length of pwm registers.
> + - clocks: Must contain an entry for each entry in clock-names.
> + See ../clock/clock-bindings.txt for details.
> + - clock-names: Must include the following entries.
> + - pwm: PWM operating clock.
> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
> + of the cells format.
> +
> +Example:
> + pwm: pwm@40000000 {
> + compatible = "nxp,lpc1850-pwm";
I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
name of hardware block as described in the user manual and while PWM
is the most obvious usage for this block on Linux, the hardware is not
limited to just doing that. So as a bit of future proofing if someone
wants to use this block for more than PWM I would prefer SCT.
> + reg = <0x40000000 0x580>;
Please map the entire memory block as noted in the user manual, ie. 0x1000.
regards,
Joachim Eastwood
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-28 22:37 ` Joachim Eastwood
0 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-07-28 22:37 UTC (permalink / raw)
To: linux-arm-kernel
On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> ---
> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
> new file mode 100644
> index 0000000..3055429
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
> @@ -0,0 +1,20 @@
> +* NXP LPC18xx Pulse Width Modulator driver
> +
> +Required properties:
> + - compatible: Should be "nxp,lpc1850-pwm"
> + - reg: Should contain physical base address and length of pwm registers.
> + - clocks: Must contain an entry for each entry in clock-names.
> + See ../clock/clock-bindings.txt for details.
> + - clock-names: Must include the following entries.
> + - pwm: PWM operating clock.
> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
> + of the cells format.
> +
> +Example:
> + pwm: pwm at 40000000 {
> + compatible = "nxp,lpc1850-pwm";
I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
name of hardware block as described in the user manual and while PWM
is the most obvious usage for this block on Linux, the hardware is not
limited to just doing that. So as a bit of future proofing if someone
wants to use this block for more than PWM I would prefer SCT.
> + reg = <0x40000000 0x580>;
Please map the entire memory block as noted in the user manual, ie. 0x1000.
regards,
Joachim Eastwood
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-28 22:37 ` Joachim Eastwood
@ 2015-07-28 22:45 ` Ezequiel Garcia
-1 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-28 22:45 UTC (permalink / raw)
To: Joachim Eastwood
Cc: Ariel D'Alessandro, linux-pwm, linux-arm-kernel, Thierry Reding
On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> new file mode 100644
>> index 0000000..3055429
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> @@ -0,0 +1,20 @@
>> +* NXP LPC18xx Pulse Width Modulator driver
>> +
>> +Required properties:
>> + - compatible: Should be "nxp,lpc1850-pwm"
>> + - reg: Should contain physical base address and length of pwm registers.
>> + - clocks: Must contain an entry for each entry in clock-names.
>> + See ../clock/clock-bindings.txt for details.
>> + - clock-names: Must include the following entries.
>> + - pwm: PWM operating clock.
>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>> + of the cells format.
>> +
>> +Example:
>> + pwm: pwm@40000000 {
>> + compatible = "nxp,lpc1850-pwm";
>
> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
> name of hardware block as described in the user manual and while PWM
> is the most obvious usage for this block on Linux, the hardware is not
> limited to just doing that. So as a bit of future proofing if someone
> wants to use this block for more than PWM I would prefer SCT.
>
Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
Sounds like the word PWM should be in the compatible as it describes
not only the
device, but the device used in a certain way.
Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
e.g. a clocksource/clockevents (we can also use SCT for that)?
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-28 22:45 ` Ezequiel Garcia
0 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-28 22:45 UTC (permalink / raw)
To: linux-arm-kernel
On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> new file mode 100644
>> index 0000000..3055429
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> @@ -0,0 +1,20 @@
>> +* NXP LPC18xx Pulse Width Modulator driver
>> +
>> +Required properties:
>> + - compatible: Should be "nxp,lpc1850-pwm"
>> + - reg: Should contain physical base address and length of pwm registers.
>> + - clocks: Must contain an entry for each entry in clock-names.
>> + See ../clock/clock-bindings.txt for details.
>> + - clock-names: Must include the following entries.
>> + - pwm: PWM operating clock.
>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>> + of the cells format.
>> +
>> +Example:
>> + pwm: pwm at 40000000 {
>> + compatible = "nxp,lpc1850-pwm";
>
> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
> name of hardware block as described in the user manual and while PWM
> is the most obvious usage for this block on Linux, the hardware is not
> limited to just doing that. So as a bit of future proofing if someone
> wants to use this block for more than PWM I would prefer SCT.
>
Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
Sounds like the word PWM should be in the compatible as it describes
not only the
device, but the device used in a certain way.
Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
e.g. a clocksource/clockevents (we can also use SCT for that)?
--
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-28 22:45 ` Ezequiel Garcia
@ 2015-07-28 23:22 ` Joachim Eastwood
-1 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-07-28 23:22 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Ariel D'Alessandro, linux-pwm, linux-arm-kernel, Thierry Reding
On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>
>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>> ---
>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>> new file mode 100644
>>> index 0000000..3055429
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>> @@ -0,0 +1,20 @@
>>> +* NXP LPC18xx Pulse Width Modulator driver
>>> +
>>> +Required properties:
>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>> + - reg: Should contain physical base address and length of pwm registers.
>>> + - clocks: Must contain an entry for each entry in clock-names.
>>> + See ../clock/clock-bindings.txt for details.
>>> + - clock-names: Must include the following entries.
>>> + - pwm: PWM operating clock.
>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>> + of the cells format.
>>> +
>>> +Example:
>>> + pwm: pwm@40000000 {
>>> + compatible = "nxp,lpc1850-pwm";
>>
>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>> name of hardware block as described in the user manual and while PWM
>> is the most obvious usage for this block on Linux, the hardware is not
>> limited to just doing that. So as a bit of future proofing if someone
>> wants to use this block for more than PWM I would prefer SCT.
>>
>
> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>
> Sounds like the word PWM should be in the compatible as it describes
> not only the device, but the device used in a certain way.
>
> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
> e.g. a clocksource/clockevents (we can also use SCT for that)?
I not sure how to best handle dt bindings with such flexible hardware blocks.
But are you suggestion to have multiple drivers for the same hw block?
ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
"nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
or the other.
So I am not sure how to best handle this, but I think we should at
least consider more than just PWM usage for this hw block. Note that
not against calling it "lpc1850-sct-pwm", I like that better than just
"lpc1850-pwm".
regards,
Joachim Eastwood
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-28 23:22 ` Joachim Eastwood
0 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-07-28 23:22 UTC (permalink / raw)
To: linux-arm-kernel
On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>
>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>> ---
>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>> new file mode 100644
>>> index 0000000..3055429
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>> @@ -0,0 +1,20 @@
>>> +* NXP LPC18xx Pulse Width Modulator driver
>>> +
>>> +Required properties:
>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>> + - reg: Should contain physical base address and length of pwm registers.
>>> + - clocks: Must contain an entry for each entry in clock-names.
>>> + See ../clock/clock-bindings.txt for details.
>>> + - clock-names: Must include the following entries.
>>> + - pwm: PWM operating clock.
>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>> + of the cells format.
>>> +
>>> +Example:
>>> + pwm: pwm at 40000000 {
>>> + compatible = "nxp,lpc1850-pwm";
>>
>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>> name of hardware block as described in the user manual and while PWM
>> is the most obvious usage for this block on Linux, the hardware is not
>> limited to just doing that. So as a bit of future proofing if someone
>> wants to use this block for more than PWM I would prefer SCT.
>>
>
> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>
> Sounds like the word PWM should be in the compatible as it describes
> not only the device, but the device used in a certain way.
>
> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
> e.g. a clocksource/clockevents (we can also use SCT for that)?
I not sure how to best handle dt bindings with such flexible hardware blocks.
But are you suggestion to have multiple drivers for the same hw block?
ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
"nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
or the other.
So I am not sure how to best handle this, but I think we should at
least consider more than just PWM usage for this hw block. Note that
not against calling it "lpc1850-sct-pwm", I like that better than just
"lpc1850-pwm".
regards,
Joachim Eastwood
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-28 22:37 ` Joachim Eastwood
@ 2015-07-29 13:19 ` Ariel D'Alessandro
-1 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-29 13:19 UTC (permalink / raw)
To: Joachim Eastwood
Cc: linux-pwm, linux-arm-kernel, Ezequiel Garcia, Thierry Reding
Joachim,
El 28/07/15 a las 19:37, Joachim Eastwood escibió:
> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> new file mode 100644
>> index 0000000..3055429
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> @@ -0,0 +1,20 @@
>> +* NXP LPC18xx Pulse Width Modulator driver
>> +
>> +Required properties:
>> + - compatible: Should be "nxp,lpc1850-pwm"
>> + - reg: Should contain physical base address and length of pwm registers.
>> + - clocks: Must contain an entry for each entry in clock-names.
>> + See ../clock/clock-bindings.txt for details.
>> + - clock-names: Must include the following entries.
>> + - pwm: PWM operating clock.
>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>> + of the cells format.
>> +
>> +Example:
>> + pwm: pwm@40000000 {
>> + compatible = "nxp,lpc1850-pwm";
[...]
>> + reg = <0x40000000 0x580>;
>
> Please map the entire memory block as noted in the user manual, ie. 0x1000.
OK, I'll change that in v2.
>
>
> regards,
> Joachim Eastwood
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-29 13:19 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-07-29 13:19 UTC (permalink / raw)
To: linux-arm-kernel
Joachim,
El 28/07/15 a las 19:37, Joachim Eastwood escibi?:
> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> new file mode 100644
>> index 0000000..3055429
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>> @@ -0,0 +1,20 @@
>> +* NXP LPC18xx Pulse Width Modulator driver
>> +
>> +Required properties:
>> + - compatible: Should be "nxp,lpc1850-pwm"
>> + - reg: Should contain physical base address and length of pwm registers.
>> + - clocks: Must contain an entry for each entry in clock-names.
>> + See ../clock/clock-bindings.txt for details.
>> + - clock-names: Must include the following entries.
>> + - pwm: PWM operating clock.
>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>> + of the cells format.
>> +
>> +Example:
>> + pwm: pwm at 40000000 {
>> + compatible = "nxp,lpc1850-pwm";
[...]
>> + reg = <0x40000000 0x580>;
>
> Please map the entire memory block as noted in the user manual, ie. 0x1000.
OK, I'll change that in v2.
>
>
> regards,
> Joachim Eastwood
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-28 23:22 ` Joachim Eastwood
@ 2015-07-29 13:47 ` Ezequiel Garcia
-1 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-29 13:47 UTC (permalink / raw)
To: Joachim Eastwood
Cc: Ariel D'Alessandro, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Thierry Reding, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala
+devicetree guys
On 28 July 2015 at 20:22, Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgC/G2K4zDHf@public.gmane.org.ar> wrote:
>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>
>>>> Signed-off-by: Ariel D'Alessandro <ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
>>>> ---
>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>> new file mode 100644
>>>> index 0000000..3055429
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>> @@ -0,0 +1,20 @@
>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>> +
>>>> +Required properties:
>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>> + See ../clock/clock-bindings.txt for details.
>>>> + - clock-names: Must include the following entries.
>>>> + - pwm: PWM operating clock.
>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>> + of the cells format.
>>>> +
>>>> +Example:
>>>> + pwm: pwm@40000000 {
>>>> + compatible = "nxp,lpc1850-pwm";
>>>
>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>> name of hardware block as described in the user manual and while PWM
>>> is the most obvious usage for this block on Linux, the hardware is not
>>> limited to just doing that. So as a bit of future proofing if someone
>>> wants to use this block for more than PWM I would prefer SCT.
>>>
>>
>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>
>> Sounds like the word PWM should be in the compatible as it describes
>> not only the device, but the device used in a certain way.
>>
>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>
> I not sure how to best handle dt bindings with such flexible hardware blocks.
>
> But are you suggestion to have multiple drivers for the same hw block?
> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
> or the other.
>
Yup, I'm suggesting just that. If the hardware is capable, I can't see
why we can't have different drivers for it.
> So I am not sure how to best handle this, but I think we should at
> least consider more than just PWM usage for this hw block. Note that
> not against calling it "lpc1850-sct-pwm", I like that better than just
> "lpc1850-pwm".
>
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-29 13:47 ` Ezequiel Garcia
0 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-29 13:47 UTC (permalink / raw)
To: linux-arm-kernel
+devicetree guys
On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>
>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>> ---
>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>> new file mode 100644
>>>> index 0000000..3055429
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>> @@ -0,0 +1,20 @@
>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>> +
>>>> +Required properties:
>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>> + See ../clock/clock-bindings.txt for details.
>>>> + - clock-names: Must include the following entries.
>>>> + - pwm: PWM operating clock.
>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>> + of the cells format.
>>>> +
>>>> +Example:
>>>> + pwm: pwm at 40000000 {
>>>> + compatible = "nxp,lpc1850-pwm";
>>>
>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>> name of hardware block as described in the user manual and while PWM
>>> is the most obvious usage for this block on Linux, the hardware is not
>>> limited to just doing that. So as a bit of future proofing if someone
>>> wants to use this block for more than PWM I would prefer SCT.
>>>
>>
>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>
>> Sounds like the word PWM should be in the compatible as it describes
>> not only the device, but the device used in a certain way.
>>
>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>
> I not sure how to best handle dt bindings with such flexible hardware blocks.
>
> But are you suggestion to have multiple drivers for the same hw block?
> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
> or the other.
>
Yup, I'm suggesting just that. If the hardware is capable, I can't see
why we can't have different drivers for it.
> So I am not sure how to best handle this, but I think we should at
> least consider more than just PWM usage for this hw block. Note that
> not against calling it "lpc1850-sct-pwm", I like that better than just
> "lpc1850-pwm".
>
--
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-29 13:47 ` Ezequiel Garcia
@ 2015-07-30 23:30 ` Joachim Eastwood
-1 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-07-30 23:30 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Ariel D'Alessandro, linux-pwm-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Thierry Reding, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala
On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjnB6l0xXb4Jt@public.gmane.orgr> wrote:
> +devicetree guys
>
> On 28 July 2015 at 20:22, Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjg@public.gmane.orgm.ar> wrote:
>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>
>>>>> Signed-off-by: Ariel D'Alessandro <ariel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
>>>>> ---
>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>> 1 file changed, 20 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>> new file mode 100644
>>>>> index 0000000..3055429
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>> @@ -0,0 +1,20 @@
>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>> +
>>>>> +Required properties:
>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>> + See ../clock/clock-bindings.txt for details.
>>>>> + - clock-names: Must include the following entries.
>>>>> + - pwm: PWM operating clock.
>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>> + of the cells format.
>>>>> +
>>>>> +Example:
>>>>> + pwm: pwm@40000000 {
>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>
>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>> name of hardware block as described in the user manual and while PWM
>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>> limited to just doing that. So as a bit of future proofing if someone
>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>
>>>
>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>
>>> Sounds like the word PWM should be in the compatible as it describes
>>> not only the device, but the device used in a certain way.
>>>
>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>
>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>
>> But are you suggestion to have multiple drivers for the same hw block?
>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>> or the other.
>>
>
> Yup, I'm suggesting just that. If the hardware is capable, I can't see
> why we can't have different drivers for it.
Well, I think it would be nice if we could have one driver per hw
block. But then you need a way to select whether you want to use the
counter in the SCT block for PWM or, for example, clocksource. Sort of
mode selection I guess.
But then again if someone has some very custom and application
specific usage for the SCT block that maybe better served by a
standalone driver anyway.
So for now I am fine with "nxp,lpc1850-sct-pwm". I shouldn't be too
hard to change the bindings if we see other users of this block, I
believe.
regards,
Joachim Eastwood
>> So I am not sure how to best handle this, but I think we should at
>> least consider more than just PWM usage for this hw block. Note that
>> not against calling it "lpc1850-sct-pwm", I like that better than just
>> "lpc1850-pwm".
>>
>
>
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-30 23:30 ` Joachim Eastwood
0 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-07-30 23:30 UTC (permalink / raw)
To: linux-arm-kernel
On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> +devicetree guys
>
> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>
>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>> ---
>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>> 1 file changed, 20 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>> new file mode 100644
>>>>> index 0000000..3055429
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>> @@ -0,0 +1,20 @@
>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>> +
>>>>> +Required properties:
>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>> + See ../clock/clock-bindings.txt for details.
>>>>> + - clock-names: Must include the following entries.
>>>>> + - pwm: PWM operating clock.
>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>> + of the cells format.
>>>>> +
>>>>> +Example:
>>>>> + pwm: pwm at 40000000 {
>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>
>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>> name of hardware block as described in the user manual and while PWM
>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>> limited to just doing that. So as a bit of future proofing if someone
>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>
>>>
>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>
>>> Sounds like the word PWM should be in the compatible as it describes
>>> not only the device, but the device used in a certain way.
>>>
>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>
>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>
>> But are you suggestion to have multiple drivers for the same hw block?
>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>> or the other.
>>
>
> Yup, I'm suggesting just that. If the hardware is capable, I can't see
> why we can't have different drivers for it.
Well, I think it would be nice if we could have one driver per hw
block. But then you need a way to select whether you want to use the
counter in the SCT block for PWM or, for example, clocksource. Sort of
mode selection I guess.
But then again if someone has some very custom and application
specific usage for the SCT block that maybe better served by a
standalone driver anyway.
So for now I am fine with "nxp,lpc1850-sct-pwm". I shouldn't be too
hard to change the bindings if we see other users of this block, I
believe.
regards,
Joachim Eastwood
>> So I am not sure how to best handle this, but I think we should at
>> least consider more than just PWM usage for this hw block. Note that
>> not against calling it "lpc1850-sct-pwm", I like that better than just
>> "lpc1850-pwm".
>>
>
>
> --
> Ezequiel Garc?a, VanguardiaSur
> www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-30 23:30 ` Joachim Eastwood
@ 2015-07-31 18:09 ` Ezequiel Garcia
-1 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-31 18:09 UTC (permalink / raw)
To: Joachim Eastwood
Cc: Ariel D'Alessandro, linux-pwm, linux-arm-kernel,
Thierry Reding, devicetree, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala
Joachim,
On 30 July 2015 at 20:30, Joachim Eastwood <manabian@gmail.com> wrote:
> On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>> +devicetree guys
>>
>> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>>
>>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>>> ---
>>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>>> 1 file changed, 20 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..3055429
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>>> +
>>>>>> +Required properties:
>>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>>> + See ../clock/clock-bindings.txt for details.
>>>>>> + - clock-names: Must include the following entries.
>>>>>> + - pwm: PWM operating clock.
>>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>>> + of the cells format.
>>>>>> +
>>>>>> +Example:
>>>>>> + pwm: pwm@40000000 {
>>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>>
>>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>>> name of hardware block as described in the user manual and while PWM
>>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>>> limited to just doing that. So as a bit of future proofing if someone
>>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>>
>>>>
>>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>>
>>>> Sounds like the word PWM should be in the compatible as it describes
>>>> not only the device, but the device used in a certain way.
>>>>
>>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>>
>>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>>
>>> But are you suggestion to have multiple drivers for the same hw block?
>>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>>> or the other.
>>>
>>
>> Yup, I'm suggesting just that. If the hardware is capable, I can't see
>> why we can't have different drivers for it.
>
> Well, I think it would be nice if we could have one driver per hw
> block. But then you need a way to select whether you want to use the
> counter in the SCT block for PWM or, for example, clocksource. Sort of
> mode selection I guess.
>
> But then again if someone has some very custom and application
> specific usage for the SCT block that maybe better served by a
> standalone driver anyway.
>
Technically speaking, I believe we can use "nxp,lpc1850-sct" to expose
both a PWM interface and a clocksource in a single driver. Not too
clean, though.
Alternatively, we should be able to have a multifunction device (MFD) to mux
the SCT resources to be consumed by different kind of drivers.
Given there's no counter API to feed SCT, and given we use the other
timer hw block to feed clocksource and clockevents for the LPC18/43xx,
I think currently the simplest choice is to have "nxp,lpc1850-sct-pwm"
describing this PWM SCT driver.
If we later want to change completely the PWM SCT driver, we can do
that without any bindings change.
> So for now I am fine with "nxp,lpc1850-sct-pwm". I shouldn't be too
> hard to change the bindings if we see other users of this block, I
> believe.
>
Well, that would break DT backward compatibility, so it's not an ideal.
Like I said, "nxp,lpc1850-sct-pwm" seems to be specific enough to
describe this driver.
I'd like to know what Thierry thinks here.
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-07-31 18:09 ` Ezequiel Garcia
0 siblings, 0 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2015-07-31 18:09 UTC (permalink / raw)
To: linux-arm-kernel
Joachim,
On 30 July 2015 at 20:30, Joachim Eastwood <manabian@gmail.com> wrote:
> On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>> +devicetree guys
>>
>> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>>
>>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>>> ---
>>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>>> 1 file changed, 20 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..3055429
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>>> +
>>>>>> +Required properties:
>>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>>> + See ../clock/clock-bindings.txt for details.
>>>>>> + - clock-names: Must include the following entries.
>>>>>> + - pwm: PWM operating clock.
>>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>>> + of the cells format.
>>>>>> +
>>>>>> +Example:
>>>>>> + pwm: pwm at 40000000 {
>>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>>
>>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>>> name of hardware block as described in the user manual and while PWM
>>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>>> limited to just doing that. So as a bit of future proofing if someone
>>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>>
>>>>
>>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>>
>>>> Sounds like the word PWM should be in the compatible as it describes
>>>> not only the device, but the device used in a certain way.
>>>>
>>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>>
>>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>>
>>> But are you suggestion to have multiple drivers for the same hw block?
>>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>>> or the other.
>>>
>>
>> Yup, I'm suggesting just that. If the hardware is capable, I can't see
>> why we can't have different drivers for it.
>
> Well, I think it would be nice if we could have one driver per hw
> block. But then you need a way to select whether you want to use the
> counter in the SCT block for PWM or, for example, clocksource. Sort of
> mode selection I guess.
>
> But then again if someone has some very custom and application
> specific usage for the SCT block that maybe better served by a
> standalone driver anyway.
>
Technically speaking, I believe we can use "nxp,lpc1850-sct" to expose
both a PWM interface and a clocksource in a single driver. Not too
clean, though.
Alternatively, we should be able to have a multifunction device (MFD) to mux
the SCT resources to be consumed by different kind of drivers.
Given there's no counter API to feed SCT, and given we use the other
timer hw block to feed clocksource and clockevents for the LPC18/43xx,
I think currently the simplest choice is to have "nxp,lpc1850-sct-pwm"
describing this PWM SCT driver.
If we later want to change completely the PWM SCT driver, we can do
that without any bindings change.
> So for now I am fine with "nxp,lpc1850-sct-pwm". I shouldn't be too
> hard to change the bindings if we see other users of this block, I
> believe.
>
Well, that would break DT backward compatibility, so it's not an ideal.
Like I said, "nxp,lpc1850-sct-pwm" seems to be specific enough to
describe this driver.
I'd like to know what Thierry thinks here.
--
Ezequiel Garc?a, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver
2015-07-28 19:48 ` Ezequiel Garcia
@ 2015-08-05 23:03 ` Ariel D'Alessandro
-1 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-08-05 23:03 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: linux-pwm, linux-arm-kernel, Joachim Eastwood, Thierry Reding
El 28/07/15 a las 16:48, Ezequiel Garcia escibió:
> Ariel,
>
> On 27 July 2015 at 01:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> This commit adds support for NXP LPC18xx PWM/SCT.
>>
>> NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
>> Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
>> Other SoCs in that family may share the same hardware.
>>
>> The PWM supports a total of 16 channels, but only 15 can be simultaneously
>> requested. There's only one period, global to all the channels, thus PWM driver
>> will refuse setting different values to it.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>> drivers/pwm/Kconfig | 12 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-lpc18xx.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 472 insertions(+)
>> create mode 100644 drivers/pwm/pwm-lpc18xx.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index b1541f4..2c7635c 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -173,6 +173,18 @@ config PWM_LP3943
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-lp3943.
>>
>> +config PWM_LPC18XX
>> + tristate "LPC18xx/43xx PWM support"
>> + depends on ARCH_LPC18XX
>> + help
>> + Generic PWM framework driver for NXP LPC18xx PWM/SCT which
>> + supports 16 channels.
>> + A maximum of 15 channels can be requested simultaneously and
>> + must have the same period.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-lpc18xx.
>> +
>> config PWM_LPC32XX
>> tristate "LPC32XX PWM support"
>> depends on ARCH_LPC32XX
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index ec50eb5..986b286 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
>> obj-$(CONFIG_PWM_IMX) += pwm-imx.o
>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
>> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
>> +obj-$(CONFIG_PWM_LPC18XX) += pwm-lpc18xx.o
>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
>> obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
>> obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
>> diff --git a/drivers/pwm/pwm-lpc18xx.c b/drivers/pwm/pwm-lpc18xx.c
>> new file mode 100644
>> index 0000000..77b5e74
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-lpc18xx.c
>> @@ -0,0 +1,459 @@
>> +/*
>> + * NXP LPC18xx Pulse Width Modulator driver
>> + *
>> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License.
>> + *
>> + * Notes
>> + * =====
>> + * NXP LPC18xx provides a State Configurable Timer (SCT) which can be configured
>> + * as a Pulse Width Modulator.
>> + *
>> + * SCT supports 16 outputs, 16 events and 16 registers. Each event will be
>> + * triggered when its related register matches the SCT counter value, and it
>> + * will set or clear a selected output.
>> + *
>> + * One of the events is preselected to generate the period, thus the maximum
>> + * number of simultaneous channels is limited to 15. Notice that period is
>> + * global to all the channels, thus PWM driver will refuse setting different
>> + * values to it.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +
>> +/* LPC18xx SCT registers */
>> +#define LPC18XX_PWM_CONFIG 0x000
>> +#define LPC18XX_PWM_CONFIG_UNIFY BIT(0)
>> +#define LPC18XX_PWM_CONFIG_NORELOAD BIT(7)
>> +
>> +#define LPC18XX_PWM_CTRL 0x004
>> +#define LPC18XX_PWM_CTRL_HALT BIT(2)
>> +#define LPC18XX_PWM_BIDIR BIT(4)
>> +#define LPC18XX_PWM_PRE_SHIFT 5
>> +#define LPC18XX_PWM_PRE_MASK (0xff << LPC18XX_PWM_PRE_SHIFT)
>> +#define LPC18XX_PWM_PRE(x) (x << LPC18XX_PWM_PRE_SHIFT)
>> +
>> +#define LPC18XX_PWM_LIMIT 0x008
>> +
>> +#define LPC18XX_PWM_RES_BASE 0x058
>> +#define LPC18XX_PWM_RES_SHIFT(_ch) (_ch * 2)
>> +#define LPC18XX_PWM_RES(_ch, _action) (_action << LPC18XX_PWM_RES_SHIFT(_ch))
>> +#define LPC18XX_PWM_RES_MASK(_ch) (0x3 << LPC18XX_PWM_RES_SHIFT(_ch))
>> +
>> +#define LPC18XX_PWM_MATCH_BASE 0x100
>> +#define LPC18XX_PWM_MATCH(_ch) (LPC18XX_PWM_MATCH_BASE + _ch * 4)
>> +
>> +#define LPC18XX_PWM_MATCHREL_BASE 0x200
>> +#define LPC18XX_PWM_MATCHREL(_ch) (LPC18XX_PWM_MATCHREL_BASE + _ch * 4)
>> +
>> +#define LPC18XX_PWM_EVSTATEMSK_BASE 0x300
>> +#define LPC18XX_PWM_EVSTATEMSK(_ch) (LPC18XX_PWM_EVSTATEMSK_BASE + _ch * 8)
>> +#define LPC18XX_PWM_EVSTATEMSK_ALL 0xffffffff
>> +
>> +#define LPC18XX_PWM_EVCTRL_BASE 0x304
>> +#define LPC18XX_PWM_EVCTRL(_ch) (LPC18XX_PWM_EVCTRL_BASE + _ch * 8)
>> +
>
> I think some of these are not channels (you are passing duty_event
> to them) and so you shouldn't call the argument "_ch".
Right.
>
>> +#define LPC18XX_PWM_EVCTRL_MATCH(_ch) _ch
>> +
>> +#define LPC18XX_PWM_EVCTRL_COMB_SHIFT 12
>> +#define LPC18XX_PWM_EVCTRL_COMB_MATCH (0x1 << LPC18XX_PWM_EVCTRL_COMB_SHIFT)
>> +
>> +#define LPC18XX_PWM_OUTPUTSET_BASE 0x500
>> +#define LPC18XX_PWM_OUTPUTSET(_ch) (LPC18XX_PWM_OUTPUTSET_BASE + _ch * 8)
>> +
>> +#define LPC18XX_PWM_OUTPUTCL_BASE 0x504
>> +#define LPC18XX_PWM_OUTPUTCL(_ch) (LPC18XX_PWM_OUTPUTCL_BASE + _ch * 8)
>> +
>> +/* LPC18xx SCT unified counter */
>> +#define LPC18XX_PWM_TIMER_MAX 0xffffffff
>> +
>> +/* LPC18xx SCT events */
>> +#define LPC18XX_PWM_EVENT_PERIOD 0
>> +#define LPC18XX_PWM_EVENT_MAX 16
>> +
>> +/* SCT conflict resolution */
>> +enum lpc18xx_pwm_res_action {
>> + LPC18XX_PWM_RES_NONE = 0x0,
>> + LPC18XX_PWM_RES_SET = 0x1,
>> + LPC18XX_PWM_RES_CLEAR = 0x2,
>> + LPC18XX_PWM_RES_TOGGLE = 0x3,
>> +};
>> +
>> +struct lpc18xx_pwm_data {
>> + unsigned int duty_event;
>> +};
>> +
>> +struct lpc18xx_pwm_chip {
>> + struct device *dev;
>> + struct pwm_chip chip;
>> + void __iomem *base;
>> + struct clk *pwm_clk;
>> + unsigned long clk_rate;
>> + unsigned int period_ns;
>> + unsigned int min_period_ns;
>> + unsigned int max_period_ns;
>> + unsigned int period_event;
>> + unsigned long event_map;
>> + struct mutex res_lock;
>> + struct mutex period_lock;
>> +};
>> +
>> +static inline struct lpc18xx_pwm_chip *to_lpc18xx_pwm_chip(
>> + struct pwm_chip *chip)
>> +{
>> + return container_of(chip, struct lpc18xx_pwm_chip, chip);
>> +}
>> +
>> +static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
>> + u32 reg, u32 val)
>> +{
>> + writel(val, lpc18xx_pwm->base + reg);
>> +}
>> +
>> +static inline u32 lpc18xx_pwm_readl(struct lpc18xx_pwm_chip *lpc18xx_pwm,
>> + u32 reg)
>> +{
>> + return readl(lpc18xx_pwm->base + reg);
>> +}
>> +
>> +static void lpc18xx_pwm_set_conflict_res(struct lpc18xx_pwm_chip *lpc18xx_pwm,
>> + struct pwm_device *pwm,
>> + enum lpc18xx_pwm_res_action action)
>> +{
>> + u32 val;
>> +
>> + mutex_lock(&lpc18xx_pwm->res_lock);
>> +
>> + /*
>> + * Simultaneous set and clear may happen on an output, that is the case
>> + * when duty_ns == period_ns. LPC18xx SCT allows to set a conflict
>> + * resolution action to be taken in such a case.
>> + */
>> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_RES_BASE);
>> + val &= ~LPC18XX_PWM_RES_MASK(pwm->hwpwm);
>> + val |= LPC18XX_PWM_RES(pwm->hwpwm, action);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_RES_BASE, val);
>> +
>> + mutex_unlock(&lpc18xx_pwm->res_lock);
>> +}
>> +
>> +static void lpc18xx_pwm_config_period(struct pwm_chip *chip, int period_ns)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + u64 val;
>> +
>> + val = (u64)period_ns * lpc18xx_pwm->clk_rate;
>> + do_div(val, NSEC_PER_SEC);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCH(lpc18xx_pwm->period_event),
>> + (u32)val);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCHREL(lpc18xx_pwm->period_event),
>> + (u32)val);
>> +}
>> +
>> +static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
>> + struct pwm_device *pwm, int duty_ns)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> + u64 val;
>> +
>> + val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
>> + do_div(val, NSEC_PER_SEC);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCH(lpc18xx_data->duty_event),
>> + (u32)val);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCHREL(lpc18xx_data->duty_event),
>> + (u32)val);
>> +}
>> +
>> +static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> + int duty_ns, int period_ns)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + int i;
>> +
>> + if (period_ns < lpc18xx_pwm->min_period_ns ||
>> + period_ns > lpc18xx_pwm->max_period_ns) {
>> + dev_err(chip->dev, "period %d not in range\n", period_ns);
>> + return -ERANGE;
>> + }
>> +
>> + mutex_lock(&lpc18xx_pwm->period_lock);
>> +
>> + /*
>> + * The PWM supports only a single period for all PWM channels, therefore
>> + * incompatible changes need to be refused.
>> + */
>> + if (lpc18xx_pwm->period_ns && lpc18xx_pwm->period_ns != period_ns) {
>> + dev_err(chip->dev, "conflicting period requested for PWM %u\n",
>> + pwm->hwpwm);
>> + mutex_unlock(&lpc18xx_pwm->period_lock);
>> + return -EBUSY;
>> + }
>> +
>> + if (!lpc18xx_pwm->period_ns) {
>> + lpc18xx_pwm->period_ns = period_ns;
>
> You set the period_ns, which is constrained to be a global period
> for all channels.
>
> However, you are not clearing this. So there's no way to change the period.
> I believe you should allow to change the period if there's only a single
> channel enabled.
>
> Thus, only forbid when changing the period of a channel that hasn't
> requested period change.
OK. I'm sending patchset v2 with these modifications.
>
>> + for (i = 0; i < chip->npwm; i++) {
>> + pwm_set_period(&chip->pwms[i], period_ns);
>> + }
>> + lpc18xx_pwm_config_period(chip, period_ns);
>> + }
>> +
>> + mutex_unlock(&lpc18xx_pwm->period_lock);
>> +
>> + lpc18xx_pwm_config_duty(chip, pwm, duty_ns);
>> +
>> + return 0;
>> +}
>> +
>> +static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
>> + struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>> +{
>> + return 0;
>> +}
>> +
>> +static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> + enum lpc18xx_pwm_res_action res_action;
>> + unsigned int set_event, clear_event;
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event),
>> + LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_data->duty_event) |
>> + LPC18XX_PWM_EVCTRL_COMB_MATCH);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
>> + LPC18XX_PWM_EVSTATEMSK_ALL);
>> +
>> + if (pwm->polarity == PWM_POLARITY_NORMAL) {
>> + set_event = lpc18xx_pwm->period_event;
>> + clear_event = lpc18xx_data->duty_event;
>> + res_action = LPC18XX_PWM_RES_SET;
>> + } else {
>> + set_event = lpc18xx_data->duty_event;
>> + clear_event = lpc18xx_pwm->period_event;
>> + res_action = LPC18XX_PWM_RES_CLEAR;
>> + }
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm),
>> + BIT(set_event));
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm),
>> + BIT(clear_event));
>> + lpc18xx_pwm_set_conflict_res(lpc18xx_pwm, pwm, res_action);
>> +
>> + return 0;
>> +}
>> +
>> +static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm), 0);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm), 0);
>> +}
>> +
>> +static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> + unsigned long event;
>> +
>> + event = find_first_zero_bit(&lpc18xx_pwm->event_map,
>> + LPC18XX_PWM_EVENT_MAX);
>> +
>> + if (event >= LPC18XX_PWM_EVENT_MAX) {
>> + dev_err(lpc18xx_pwm->dev,
>> + "maximum number of simultaneous channels reached\n");
>> + return -EBUSY;
>> + };
>> +
>> + set_bit(event, &lpc18xx_pwm->event_map);
>> + lpc18xx_data->duty_event = event;
>> +
>> + return 0;
>> +}
>> +
>> +static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> +
>> + lpc18xx_pwm_disable(chip, pwm);
>> + clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
>> +}
>> +
>> +static const struct pwm_ops lpc18xx_pwm_ops = {
>> + .config = lpc18xx_pwm_config,
>> + .set_polarity = lpc18xx_pwm_set_polarity,
>> + .enable = lpc18xx_pwm_enable,
>> + .disable = lpc18xx_pwm_disable,
>> + .request = lpc18xx_pwm_request,
>> + .free = lpc18xx_pwm_free,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static const struct of_device_id lpc18xx_pwm_of_match[] = {
>> + { .compatible = "nxp,lpc1850-pwm" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
>> +
>> +static int lpc18xx_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm;
>> + struct pwm_device *pwm;
>> + struct resource *res;
>> + int ret, i;
>> + u64 val;
>> +
>> + lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
>> + GFP_KERNEL);
>> + if (!lpc18xx_pwm)
>> + return -ENOMEM;
>> +
>> + lpc18xx_pwm->dev = &pdev->dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + lpc18xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(lpc18xx_pwm->base))
>> + return PTR_ERR(lpc18xx_pwm->base);
>> +
>> + lpc18xx_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
>> + if (IS_ERR(lpc18xx_pwm->pwm_clk)) {
>> + dev_err(&pdev->dev, "failed to get pwm clock\n");
>> + return PTR_ERR(lpc18xx_pwm->pwm_clk);
>> + }
>> +
>> + ret = clk_prepare_enable(lpc18xx_pwm->pwm_clk);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "could not prepare or enable pwm clock\n");
>> + return ret;
>> + }
>> +
>> + lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
>> +
>> + mutex_init(&lpc18xx_pwm->res_lock);
>> + mutex_init(&lpc18xx_pwm->period_lock);
>> +
>> + val = (u64)NSEC_PER_SEC * LPC18XX_PWM_TIMER_MAX;
>> + do_div(val, lpc18xx_pwm->clk_rate);
>> + lpc18xx_pwm->max_period_ns = val;
>> +
>> + val = (u64)NSEC_PER_SEC;
>> + do_div(val, lpc18xx_pwm->clk_rate);
>> + lpc18xx_pwm->min_period_ns = val;
>> +
>> + lpc18xx_pwm->chip.dev = &pdev->dev;
>> + lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
>> + lpc18xx_pwm->chip.base = -1;
>> + lpc18xx_pwm->chip.npwm = 16;
>> + lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
>> + lpc18xx_pwm->chip.of_pwm_n_cells = 3;
>> +
>> + /* SCT counter must be in unify (32 bit) mode */
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
>> + LPC18XX_PWM_CONFIG_UNIFY);
>> +
>> + /*
>> + * Everytime the timer counter reaches the period value, the related
>> + * event will be triggered and the counter reset to 0.
>> + */
>> + set_bit(LPC18XX_PWM_EVENT_PERIOD, &lpc18xx_pwm->event_map);
>> + lpc18xx_pwm->period_event = LPC18XX_PWM_EVENT_PERIOD;
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_pwm->period_event),
>> + LPC18XX_PWM_EVSTATEMSK_ALL);
>> +
>> + val = LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_pwm->period_event) |
>> + LPC18XX_PWM_EVCTRL_COMB_MATCH;
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVCTRL(lpc18xx_pwm->period_event), val);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
>> + BIT(lpc18xx_pwm->period_event));
>> +
>> + ret = pwmchip_add(&lpc18xx_pwm->chip);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
>> + goto disable_pwmclk;
>> + }
>> +
>> + for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
>> + pwm = &lpc18xx_pwm->chip.pwms[i];
>> + pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
>> + sizeof(struct lpc18xx_pwm_data),
>> + GFP_KERNEL);
>> + if (!pwm->chip_data) {
>> + ret = -ENOMEM;
>> + goto remove_pwmchip;
>> + }
>> + }
>> +
>> + platform_set_drvdata(pdev, lpc18xx_pwm);
>> +
>> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
>> + val &= ~LPC18XX_PWM_BIDIR;
>> + val &= ~LPC18XX_PWM_CTRL_HALT;
>> + val &= ~LPC18XX_PWM_PRE_MASK;
>> + val |= LPC18XX_PWM_PRE(0);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
>> +
>> + return 0;
>> +
>> +remove_pwmchip:
>> + pwmchip_remove(&lpc18xx_pwm->chip);
>> +disable_pwmclk:
>> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
>> + return ret;
>> +}
>> +
>> +static int lpc18xx_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
>> + u32 val;
>> +
>> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
>> + val | LPC18XX_PWM_CTRL_HALT);
>> +
>> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
>> +
>> + return pwmchip_remove(&lpc18xx_pwm->chip);
>> +}
>> +
>> +static struct platform_driver lpc18xx_pwm_driver = {
>> + .driver = {
>> + .name = "lpc18xx-pwm",
>> + .of_match_table = lpc18xx_pwm_of_match,
>> + },
>> + .probe = lpc18xx_pwm_probe,
>> + .remove = lpc18xx_pwm_remove,
>> +};
>> +module_platform_driver(lpc18xx_pwm_driver);
>> +
>> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
>> +MODULE_DESCRIPTION("NXP LPC18xx PWM driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver
@ 2015-08-05 23:03 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-08-05 23:03 UTC (permalink / raw)
To: linux-arm-kernel
El 28/07/15 a las 16:48, Ezequiel Garcia escibi?:
> Ariel,
>
> On 27 July 2015 at 01:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>> This commit adds support for NXP LPC18xx PWM/SCT.
>>
>> NXP LPC SoCs family, which includes LPC18xx/LPC43xx, provides a State
>> Configurable Timer (SCT) which can be configured as a Pulse Width Modulator.
>> Other SoCs in that family may share the same hardware.
>>
>> The PWM supports a total of 16 channels, but only 15 can be simultaneously
>> requested. There's only one period, global to all the channels, thus PWM driver
>> will refuse setting different values to it.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> ---
>> drivers/pwm/Kconfig | 12 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-lpc18xx.c | 459 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 472 insertions(+)
>> create mode 100644 drivers/pwm/pwm-lpc18xx.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index b1541f4..2c7635c 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -173,6 +173,18 @@ config PWM_LP3943
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-lp3943.
>>
>> +config PWM_LPC18XX
>> + tristate "LPC18xx/43xx PWM support"
>> + depends on ARCH_LPC18XX
>> + help
>> + Generic PWM framework driver for NXP LPC18xx PWM/SCT which
>> + supports 16 channels.
>> + A maximum of 15 channels can be requested simultaneously and
>> + must have the same period.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-lpc18xx.
>> +
>> config PWM_LPC32XX
>> tristate "LPC32XX PWM support"
>> depends on ARCH_LPC32XX
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index ec50eb5..986b286 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o
>> obj-$(CONFIG_PWM_IMX) += pwm-imx.o
>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
>> obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
>> +obj-$(CONFIG_PWM_LPC18XX) += pwm-lpc18xx.o
>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
>> obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
>> obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
>> diff --git a/drivers/pwm/pwm-lpc18xx.c b/drivers/pwm/pwm-lpc18xx.c
>> new file mode 100644
>> index 0000000..77b5e74
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-lpc18xx.c
>> @@ -0,0 +1,459 @@
>> +/*
>> + * NXP LPC18xx Pulse Width Modulator driver
>> + *
>> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License.
>> + *
>> + * Notes
>> + * =====
>> + * NXP LPC18xx provides a State Configurable Timer (SCT) which can be configured
>> + * as a Pulse Width Modulator.
>> + *
>> + * SCT supports 16 outputs, 16 events and 16 registers. Each event will be
>> + * triggered when its related register matches the SCT counter value, and it
>> + * will set or clear a selected output.
>> + *
>> + * One of the events is preselected to generate the period, thus the maximum
>> + * number of simultaneous channels is limited to 15. Notice that period is
>> + * global to all the channels, thus PWM driver will refuse setting different
>> + * values to it.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +
>> +/* LPC18xx SCT registers */
>> +#define LPC18XX_PWM_CONFIG 0x000
>> +#define LPC18XX_PWM_CONFIG_UNIFY BIT(0)
>> +#define LPC18XX_PWM_CONFIG_NORELOAD BIT(7)
>> +
>> +#define LPC18XX_PWM_CTRL 0x004
>> +#define LPC18XX_PWM_CTRL_HALT BIT(2)
>> +#define LPC18XX_PWM_BIDIR BIT(4)
>> +#define LPC18XX_PWM_PRE_SHIFT 5
>> +#define LPC18XX_PWM_PRE_MASK (0xff << LPC18XX_PWM_PRE_SHIFT)
>> +#define LPC18XX_PWM_PRE(x) (x << LPC18XX_PWM_PRE_SHIFT)
>> +
>> +#define LPC18XX_PWM_LIMIT 0x008
>> +
>> +#define LPC18XX_PWM_RES_BASE 0x058
>> +#define LPC18XX_PWM_RES_SHIFT(_ch) (_ch * 2)
>> +#define LPC18XX_PWM_RES(_ch, _action) (_action << LPC18XX_PWM_RES_SHIFT(_ch))
>> +#define LPC18XX_PWM_RES_MASK(_ch) (0x3 << LPC18XX_PWM_RES_SHIFT(_ch))
>> +
>> +#define LPC18XX_PWM_MATCH_BASE 0x100
>> +#define LPC18XX_PWM_MATCH(_ch) (LPC18XX_PWM_MATCH_BASE + _ch * 4)
>> +
>> +#define LPC18XX_PWM_MATCHREL_BASE 0x200
>> +#define LPC18XX_PWM_MATCHREL(_ch) (LPC18XX_PWM_MATCHREL_BASE + _ch * 4)
>> +
>> +#define LPC18XX_PWM_EVSTATEMSK_BASE 0x300
>> +#define LPC18XX_PWM_EVSTATEMSK(_ch) (LPC18XX_PWM_EVSTATEMSK_BASE + _ch * 8)
>> +#define LPC18XX_PWM_EVSTATEMSK_ALL 0xffffffff
>> +
>> +#define LPC18XX_PWM_EVCTRL_BASE 0x304
>> +#define LPC18XX_PWM_EVCTRL(_ch) (LPC18XX_PWM_EVCTRL_BASE + _ch * 8)
>> +
>
> I think some of these are not channels (you are passing duty_event
> to them) and so you shouldn't call the argument "_ch".
Right.
>
>> +#define LPC18XX_PWM_EVCTRL_MATCH(_ch) _ch
>> +
>> +#define LPC18XX_PWM_EVCTRL_COMB_SHIFT 12
>> +#define LPC18XX_PWM_EVCTRL_COMB_MATCH (0x1 << LPC18XX_PWM_EVCTRL_COMB_SHIFT)
>> +
>> +#define LPC18XX_PWM_OUTPUTSET_BASE 0x500
>> +#define LPC18XX_PWM_OUTPUTSET(_ch) (LPC18XX_PWM_OUTPUTSET_BASE + _ch * 8)
>> +
>> +#define LPC18XX_PWM_OUTPUTCL_BASE 0x504
>> +#define LPC18XX_PWM_OUTPUTCL(_ch) (LPC18XX_PWM_OUTPUTCL_BASE + _ch * 8)
>> +
>> +/* LPC18xx SCT unified counter */
>> +#define LPC18XX_PWM_TIMER_MAX 0xffffffff
>> +
>> +/* LPC18xx SCT events */
>> +#define LPC18XX_PWM_EVENT_PERIOD 0
>> +#define LPC18XX_PWM_EVENT_MAX 16
>> +
>> +/* SCT conflict resolution */
>> +enum lpc18xx_pwm_res_action {
>> + LPC18XX_PWM_RES_NONE = 0x0,
>> + LPC18XX_PWM_RES_SET = 0x1,
>> + LPC18XX_PWM_RES_CLEAR = 0x2,
>> + LPC18XX_PWM_RES_TOGGLE = 0x3,
>> +};
>> +
>> +struct lpc18xx_pwm_data {
>> + unsigned int duty_event;
>> +};
>> +
>> +struct lpc18xx_pwm_chip {
>> + struct device *dev;
>> + struct pwm_chip chip;
>> + void __iomem *base;
>> + struct clk *pwm_clk;
>> + unsigned long clk_rate;
>> + unsigned int period_ns;
>> + unsigned int min_period_ns;
>> + unsigned int max_period_ns;
>> + unsigned int period_event;
>> + unsigned long event_map;
>> + struct mutex res_lock;
>> + struct mutex period_lock;
>> +};
>> +
>> +static inline struct lpc18xx_pwm_chip *to_lpc18xx_pwm_chip(
>> + struct pwm_chip *chip)
>> +{
>> + return container_of(chip, struct lpc18xx_pwm_chip, chip);
>> +}
>> +
>> +static inline void lpc18xx_pwm_writel(struct lpc18xx_pwm_chip *lpc18xx_pwm,
>> + u32 reg, u32 val)
>> +{
>> + writel(val, lpc18xx_pwm->base + reg);
>> +}
>> +
>> +static inline u32 lpc18xx_pwm_readl(struct lpc18xx_pwm_chip *lpc18xx_pwm,
>> + u32 reg)
>> +{
>> + return readl(lpc18xx_pwm->base + reg);
>> +}
>> +
>> +static void lpc18xx_pwm_set_conflict_res(struct lpc18xx_pwm_chip *lpc18xx_pwm,
>> + struct pwm_device *pwm,
>> + enum lpc18xx_pwm_res_action action)
>> +{
>> + u32 val;
>> +
>> + mutex_lock(&lpc18xx_pwm->res_lock);
>> +
>> + /*
>> + * Simultaneous set and clear may happen on an output, that is the case
>> + * when duty_ns == period_ns. LPC18xx SCT allows to set a conflict
>> + * resolution action to be taken in such a case.
>> + */
>> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_RES_BASE);
>> + val &= ~LPC18XX_PWM_RES_MASK(pwm->hwpwm);
>> + val |= LPC18XX_PWM_RES(pwm->hwpwm, action);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_RES_BASE, val);
>> +
>> + mutex_unlock(&lpc18xx_pwm->res_lock);
>> +}
>> +
>> +static void lpc18xx_pwm_config_period(struct pwm_chip *chip, int period_ns)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + u64 val;
>> +
>> + val = (u64)period_ns * lpc18xx_pwm->clk_rate;
>> + do_div(val, NSEC_PER_SEC);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCH(lpc18xx_pwm->period_event),
>> + (u32)val);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCHREL(lpc18xx_pwm->period_event),
>> + (u32)val);
>> +}
>> +
>> +static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
>> + struct pwm_device *pwm, int duty_ns)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> + u64 val;
>> +
>> + val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
>> + do_div(val, NSEC_PER_SEC);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCH(lpc18xx_data->duty_event),
>> + (u32)val);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_MATCHREL(lpc18xx_data->duty_event),
>> + (u32)val);
>> +}
>> +
>> +static int lpc18xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> + int duty_ns, int period_ns)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + int i;
>> +
>> + if (period_ns < lpc18xx_pwm->min_period_ns ||
>> + period_ns > lpc18xx_pwm->max_period_ns) {
>> + dev_err(chip->dev, "period %d not in range\n", period_ns);
>> + return -ERANGE;
>> + }
>> +
>> + mutex_lock(&lpc18xx_pwm->period_lock);
>> +
>> + /*
>> + * The PWM supports only a single period for all PWM channels, therefore
>> + * incompatible changes need to be refused.
>> + */
>> + if (lpc18xx_pwm->period_ns && lpc18xx_pwm->period_ns != period_ns) {
>> + dev_err(chip->dev, "conflicting period requested for PWM %u\n",
>> + pwm->hwpwm);
>> + mutex_unlock(&lpc18xx_pwm->period_lock);
>> + return -EBUSY;
>> + }
>> +
>> + if (!lpc18xx_pwm->period_ns) {
>> + lpc18xx_pwm->period_ns = period_ns;
>
> You set the period_ns, which is constrained to be a global period
> for all channels.
>
> However, you are not clearing this. So there's no way to change the period.
> I believe you should allow to change the period if there's only a single
> channel enabled.
>
> Thus, only forbid when changing the period of a channel that hasn't
> requested period change.
OK. I'm sending patchset v2 with these modifications.
>
>> + for (i = 0; i < chip->npwm; i++) {
>> + pwm_set_period(&chip->pwms[i], period_ns);
>> + }
>> + lpc18xx_pwm_config_period(chip, period_ns);
>> + }
>> +
>> + mutex_unlock(&lpc18xx_pwm->period_lock);
>> +
>> + lpc18xx_pwm_config_duty(chip, pwm, duty_ns);
>> +
>> + return 0;
>> +}
>> +
>> +static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
>> + struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>> +{
>> + return 0;
>> +}
>> +
>> +static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> + enum lpc18xx_pwm_res_action res_action;
>> + unsigned int set_event, clear_event;
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event),
>> + LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_data->duty_event) |
>> + LPC18XX_PWM_EVCTRL_COMB_MATCH);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_data->duty_event),
>> + LPC18XX_PWM_EVSTATEMSK_ALL);
>> +
>> + if (pwm->polarity == PWM_POLARITY_NORMAL) {
>> + set_event = lpc18xx_pwm->period_event;
>> + clear_event = lpc18xx_data->duty_event;
>> + res_action = LPC18XX_PWM_RES_SET;
>> + } else {
>> + set_event = lpc18xx_data->duty_event;
>> + clear_event = lpc18xx_pwm->period_event;
>> + res_action = LPC18XX_PWM_RES_CLEAR;
>> + }
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm),
>> + BIT(set_event));
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm),
>> + BIT(clear_event));
>> + lpc18xx_pwm_set_conflict_res(lpc18xx_pwm, pwm, res_action);
>> +
>> + return 0;
>> +}
>> +
>> +static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTSET(pwm->hwpwm), 0);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_OUTPUTCL(pwm->hwpwm), 0);
>> +}
>> +
>> +static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> + unsigned long event;
>> +
>> + event = find_first_zero_bit(&lpc18xx_pwm->event_map,
>> + LPC18XX_PWM_EVENT_MAX);
>> +
>> + if (event >= LPC18XX_PWM_EVENT_MAX) {
>> + dev_err(lpc18xx_pwm->dev,
>> + "maximum number of simultaneous channels reached\n");
>> + return -EBUSY;
>> + };
>> +
>> + set_bit(event, &lpc18xx_pwm->event_map);
>> + lpc18xx_data->duty_event = event;
>> +
>> + return 0;
>> +}
>> +
>> +static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
>> + struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
>> +
>> + lpc18xx_pwm_disable(chip, pwm);
>> + clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
>> +}
>> +
>> +static const struct pwm_ops lpc18xx_pwm_ops = {
>> + .config = lpc18xx_pwm_config,
>> + .set_polarity = lpc18xx_pwm_set_polarity,
>> + .enable = lpc18xx_pwm_enable,
>> + .disable = lpc18xx_pwm_disable,
>> + .request = lpc18xx_pwm_request,
>> + .free = lpc18xx_pwm_free,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static const struct of_device_id lpc18xx_pwm_of_match[] = {
>> + { .compatible = "nxp,lpc1850-pwm" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
>> +
>> +static int lpc18xx_pwm_probe(struct platform_device *pdev)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm;
>> + struct pwm_device *pwm;
>> + struct resource *res;
>> + int ret, i;
>> + u64 val;
>> +
>> + lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
>> + GFP_KERNEL);
>> + if (!lpc18xx_pwm)
>> + return -ENOMEM;
>> +
>> + lpc18xx_pwm->dev = &pdev->dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + lpc18xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(lpc18xx_pwm->base))
>> + return PTR_ERR(lpc18xx_pwm->base);
>> +
>> + lpc18xx_pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
>> + if (IS_ERR(lpc18xx_pwm->pwm_clk)) {
>> + dev_err(&pdev->dev, "failed to get pwm clock\n");
>> + return PTR_ERR(lpc18xx_pwm->pwm_clk);
>> + }
>> +
>> + ret = clk_prepare_enable(lpc18xx_pwm->pwm_clk);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "could not prepare or enable pwm clock\n");
>> + return ret;
>> + }
>> +
>> + lpc18xx_pwm->clk_rate = clk_get_rate(lpc18xx_pwm->pwm_clk);
>> +
>> + mutex_init(&lpc18xx_pwm->res_lock);
>> + mutex_init(&lpc18xx_pwm->period_lock);
>> +
>> + val = (u64)NSEC_PER_SEC * LPC18XX_PWM_TIMER_MAX;
>> + do_div(val, lpc18xx_pwm->clk_rate);
>> + lpc18xx_pwm->max_period_ns = val;
>> +
>> + val = (u64)NSEC_PER_SEC;
>> + do_div(val, lpc18xx_pwm->clk_rate);
>> + lpc18xx_pwm->min_period_ns = val;
>> +
>> + lpc18xx_pwm->chip.dev = &pdev->dev;
>> + lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
>> + lpc18xx_pwm->chip.base = -1;
>> + lpc18xx_pwm->chip.npwm = 16;
>> + lpc18xx_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
>> + lpc18xx_pwm->chip.of_pwm_n_cells = 3;
>> +
>> + /* SCT counter must be in unify (32 bit) mode */
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
>> + LPC18XX_PWM_CONFIG_UNIFY);
>> +
>> + /*
>> + * Everytime the timer counter reaches the period value, the related
>> + * event will be triggered and the counter reset to 0.
>> + */
>> + set_bit(LPC18XX_PWM_EVENT_PERIOD, &lpc18xx_pwm->event_map);
>> + lpc18xx_pwm->period_event = LPC18XX_PWM_EVENT_PERIOD;
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVSTATEMSK(lpc18xx_pwm->period_event),
>> + LPC18XX_PWM_EVSTATEMSK_ALL);
>> +
>> + val = LPC18XX_PWM_EVCTRL_MATCH(lpc18xx_pwm->period_event) |
>> + LPC18XX_PWM_EVCTRL_COMB_MATCH;
>> + lpc18xx_pwm_writel(lpc18xx_pwm,
>> + LPC18XX_PWM_EVCTRL(lpc18xx_pwm->period_event), val);
>> +
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
>> + BIT(lpc18xx_pwm->period_event));
>> +
>> + ret = pwmchip_add(&lpc18xx_pwm->chip);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
>> + goto disable_pwmclk;
>> + }
>> +
>> + for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
>> + pwm = &lpc18xx_pwm->chip.pwms[i];
>> + pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
>> + sizeof(struct lpc18xx_pwm_data),
>> + GFP_KERNEL);
>> + if (!pwm->chip_data) {
>> + ret = -ENOMEM;
>> + goto remove_pwmchip;
>> + }
>> + }
>> +
>> + platform_set_drvdata(pdev, lpc18xx_pwm);
>> +
>> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
>> + val &= ~LPC18XX_PWM_BIDIR;
>> + val &= ~LPC18XX_PWM_CTRL_HALT;
>> + val &= ~LPC18XX_PWM_PRE_MASK;
>> + val |= LPC18XX_PWM_PRE(0);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
>> +
>> + return 0;
>> +
>> +remove_pwmchip:
>> + pwmchip_remove(&lpc18xx_pwm->chip);
>> +disable_pwmclk:
>> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
>> + return ret;
>> +}
>> +
>> +static int lpc18xx_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct lpc18xx_pwm_chip *lpc18xx_pwm = platform_get_drvdata(pdev);
>> + u32 val;
>> +
>> + val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
>> + lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL,
>> + val | LPC18XX_PWM_CTRL_HALT);
>> +
>> + clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
>> +
>> + return pwmchip_remove(&lpc18xx_pwm->chip);
>> +}
>> +
>> +static struct platform_driver lpc18xx_pwm_driver = {
>> + .driver = {
>> + .name = "lpc18xx-pwm",
>> + .of_match_table = lpc18xx_pwm_of_match,
>> + },
>> + .probe = lpc18xx_pwm_probe,
>> + .remove = lpc18xx_pwm_remove,
>> +};
>> +module_platform_driver(lpc18xx_pwm_driver);
>> +
>> +MODULE_AUTHOR("Ariel D'Alessandro <ariel@vanguardiasur.com.ar>");
>> +MODULE_DESCRIPTION("NXP LPC18xx PWM driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>
>
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-31 18:09 ` Ezequiel Garcia
@ 2015-08-05 23:13 ` Ariel D'Alessandro
-1 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-08-05 23:13 UTC (permalink / raw)
To: Ezequiel Garcia, Joachim Eastwood
Cc: linux-pwm, linux-arm-kernel, Thierry Reding, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala
Ezequiel, Joachim:
El 31/07/15 a las 15:09, Ezequiel Garcia escibió:
> Joachim,
>
> On 30 July 2015 at 20:30, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>> +devicetree guys
>>>
>>> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>>>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>>>
>>>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>>>> ---
>>>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>>>> 1 file changed, 20 insertions(+)
>>>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3055429
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>>>> + See ../clock/clock-bindings.txt for details.
>>>>>>> + - clock-names: Must include the following entries.
>>>>>>> + - pwm: PWM operating clock.
>>>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>>>> + of the cells format.
>>>>>>> +
>>>>>>> +Example:
>>>>>>> + pwm: pwm@40000000 {
>>>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>>>
>>>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>>>> name of hardware block as described in the user manual and while PWM
>>>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>>>> limited to just doing that. So as a bit of future proofing if someone
>>>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>>>
>>>>>
>>>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>>>
>>>>> Sounds like the word PWM should be in the compatible as it describes
>>>>> not only the device, but the device used in a certain way.
>>>>>
>>>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>>>
>>>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>>>
>>>> But are you suggestion to have multiple drivers for the same hw block?
>>>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>>>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>>>> or the other.
>>>>
>>>
>>> Yup, I'm suggesting just that. If the hardware is capable, I can't see
>>> why we can't have different drivers for it.
>>
>> Well, I think it would be nice if we could have one driver per hw
>> block. But then you need a way to select whether you want to use the
>> counter in the SCT block for PWM or, for example, clocksource. Sort of
>> mode selection I guess.
>>
>> But then again if someone has some very custom and application
>> specific usage for the SCT block that maybe better served by a
>> standalone driver anyway.
>>
>
> Technically speaking, I believe we can use "nxp,lpc1850-sct" to expose
> both a PWM interface and a clocksource in a single driver. Not too
> clean, though.
>
> Alternatively, we should be able to have a multifunction device (MFD) to mux
> the SCT resources to be consumed by different kind of drivers.
>
> Given there's no counter API to feed SCT, and given we use the other
> timer hw block to feed clocksource and clockevents for the LPC18/43xx,
> I think currently the simplest choice is to have "nxp,lpc1850-sct-pwm"
> describing this PWM SCT driver.
>
> If we later want to change completely the PWM SCT driver, we can do
> that without any bindings change.
>
>> So for now I am fine with "nxp,lpc1850-sct-pwm". I shouldn't be too
>> hard to change the bindings if we see other users of this block, I
>> believe.
>>
>
> Well, that would break DT backward compatibility, so it's not an ideal.
> Like I said, "nxp,lpc1850-sct-pwm" seems to be specific enough to
> describe this driver.
>
> I'd like to know what Thierry thinks here.
>
OK then. Until we have a better idea, I'll set the compatible string to
"nxp,lpc1850-sct-pwm".
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-08-05 23:13 ` Ariel D'Alessandro
0 siblings, 0 replies; 28+ messages in thread
From: Ariel D'Alessandro @ 2015-08-05 23:13 UTC (permalink / raw)
To: linux-arm-kernel
Ezequiel, Joachim:
El 31/07/15 a las 15:09, Ezequiel Garcia escibi?:
> Joachim,
>
> On 30 July 2015 at 20:30, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>> +devicetree guys
>>>
>>> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>>>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>>>
>>>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>>>> ---
>>>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>>>> 1 file changed, 20 insertions(+)
>>>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3055429
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>>>> + See ../clock/clock-bindings.txt for details.
>>>>>>> + - clock-names: Must include the following entries.
>>>>>>> + - pwm: PWM operating clock.
>>>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>>>> + of the cells format.
>>>>>>> +
>>>>>>> +Example:
>>>>>>> + pwm: pwm at 40000000 {
>>>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>>>
>>>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>>>> name of hardware block as described in the user manual and while PWM
>>>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>>>> limited to just doing that. So as a bit of future proofing if someone
>>>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>>>
>>>>>
>>>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>>>
>>>>> Sounds like the word PWM should be in the compatible as it describes
>>>>> not only the device, but the device used in a certain way.
>>>>>
>>>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>>>
>>>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>>>
>>>> But are you suggestion to have multiple drivers for the same hw block?
>>>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>>>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>>>> or the other.
>>>>
>>>
>>> Yup, I'm suggesting just that. If the hardware is capable, I can't see
>>> why we can't have different drivers for it.
>>
>> Well, I think it would be nice if we could have one driver per hw
>> block. But then you need a way to select whether you want to use the
>> counter in the SCT block for PWM or, for example, clocksource. Sort of
>> mode selection I guess.
>>
>> But then again if someone has some very custom and application
>> specific usage for the SCT block that maybe better served by a
>> standalone driver anyway.
>>
>
> Technically speaking, I believe we can use "nxp,lpc1850-sct" to expose
> both a PWM interface and a clocksource in a single driver. Not too
> clean, though.
>
> Alternatively, we should be able to have a multifunction device (MFD) to mux
> the SCT resources to be consumed by different kind of drivers.
>
> Given there's no counter API to feed SCT, and given we use the other
> timer hw block to feed clocksource and clockevents for the LPC18/43xx,
> I think currently the simplest choice is to have "nxp,lpc1850-sct-pwm"
> describing this PWM SCT driver.
>
> If we later want to change completely the PWM SCT driver, we can do
> that without any bindings change.
>
>> So for now I am fine with "nxp,lpc1850-sct-pwm". I shouldn't be too
>> hard to change the bindings if we see other users of this block, I
>> believe.
>>
>
> Well, that would break DT backward compatibility, so it's not an ideal.
> Like I said, "nxp,lpc1850-sct-pwm" seems to be specific enough to
> describe this driver.
>
> I'd like to know what Thierry thinks here.
>
OK then. Until we have a better idea, I'll set the compatible string to
"nxp,lpc1850-sct-pwm".
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
2015-07-31 18:09 ` Ezequiel Garcia
@ 2015-08-07 15:44 ` Joachim Eastwood
-1 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-08-07 15:44 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Ariel D'Alessandro, linux-pwm, linux-arm-kernel,
Thierry Reding, devicetree, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala
On 31 July 2015 at 20:09, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> Joachim,
>
> On 30 July 2015 at 20:30, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>> +devicetree guys
>>>
>>> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>>>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>>>
>>>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>>>> ---
>>>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>>>> 1 file changed, 20 insertions(+)
>>>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3055429
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>>>> + See ../clock/clock-bindings.txt for details.
>>>>>>> + - clock-names: Must include the following entries.
>>>>>>> + - pwm: PWM operating clock.
>>>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>>>> + of the cells format.
>>>>>>> +
>>>>>>> +Example:
>>>>>>> + pwm: pwm@40000000 {
>>>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>>>
>>>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>>>> name of hardware block as described in the user manual and while PWM
>>>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>>>> limited to just doing that. So as a bit of future proofing if someone
>>>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>>>
>>>>>
>>>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>>>
>>>>> Sounds like the word PWM should be in the compatible as it describes
>>>>> not only the device, but the device used in a certain way.
>>>>>
>>>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>>>
>>>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>>>
>>>> But are you suggestion to have multiple drivers for the same hw block?
>>>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>>>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>>>> or the other.
>>>>
>>>
>>> Yup, I'm suggesting just that. If the hardware is capable, I can't see
>>> why we can't have different drivers for it.
>>
>> Well, I think it would be nice if we could have one driver per hw
>> block. But then you need a way to select whether you want to use the
>> counter in the SCT block for PWM or, for example, clocksource. Sort of
>> mode selection I guess.
>>
>> But then again if someone has some very custom and application
>> specific usage for the SCT block that maybe better served by a
>> standalone driver anyway.
>>
>
> Technically speaking, I believe we can use "nxp,lpc1850-sct" to expose
> both a PWM interface and a clocksource in a single driver. Not too
> clean, though.
Well, I don't think that is unclean way of doing it. But it's up to
the different subsystem maintainers if they want users outside their
normal directory as we would, for example, end of with a clocksource
under the pwm dir.
> Alternatively, we should be able to have a multifunction device (MFD) to mux
> the SCT resources to be consumed by different kind of drivers.
That is an other way of doing it.
But you would still need to a way to chose between either PWM or
clocksource since the SCT has a limited number of counters. (SCT can
be configured to have either two 16-bit timers or one 32-bit)
> Given there's no counter API to feed SCT, and given we use the other
> timer hw block to feed clocksource and clockevents for the LPC18/43xx,
> I think currently the simplest choice is to have "nxp,lpc1850-sct-pwm"
> describing this PWM SCT driver.
>
> If we later want to change completely the PWM SCT driver, we can do
> that without any bindings change.
Agreed. Let's keep it simple now and deal with it again if another
user for the SCT block turns up.
regards,
Joachim Eastwood
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation
@ 2015-08-07 15:44 ` Joachim Eastwood
0 siblings, 0 replies; 28+ messages in thread
From: Joachim Eastwood @ 2015-08-07 15:44 UTC (permalink / raw)
To: linux-arm-kernel
On 31 July 2015 at 20:09, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
> Joachim,
>
> On 30 July 2015 at 20:30, Joachim Eastwood <manabian@gmail.com> wrote:
>> On 29 July 2015 at 15:47, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>> +devicetree guys
>>>
>>> On 28 July 2015 at 20:22, Joachim Eastwood <manabian@gmail.com> wrote:
>>>> On 29 July 2015 at 00:45, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>>>>> On 28 July 2015 at 19:37, Joachim Eastwood <manabian@gmail.com> wrote:
>>>>>> On 27 July 2015 at 06:45, Ariel D'Alessandro <ariel@vanguardiasur.com.ar> wrote:
>>>>>>> Add the devicetree binding document for NXP LPC18xx PWM/SCT.
>>>>>>>
>>>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>>>> ---
>>>>>>> .../devicetree/bindings/pwm/lpc1850-pwm.txt | 20 ++++++++++++++++++++
>>>>>>> 1 file changed, 20 insertions(+)
>>>>>>> create mode 100644 Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..3055429
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/lpc1850-pwm.txt
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +* NXP LPC18xx Pulse Width Modulator driver
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> + - compatible: Should be "nxp,lpc1850-pwm"
>>>>>>> + - reg: Should contain physical base address and length of pwm registers.
>>>>>>> + - clocks: Must contain an entry for each entry in clock-names.
>>>>>>> + See ../clock/clock-bindings.txt for details.
>>>>>>> + - clock-names: Must include the following entries.
>>>>>>> + - pwm: PWM operating clock.
>>>>>>> + - #pwm-cells: Should be 3. See pwm.txt in this directory for the description
>>>>>>> + of the cells format.
>>>>>>> +
>>>>>>> +Example:
>>>>>>> + pwm: pwm at 40000000 {
>>>>>>> + compatible = "nxp,lpc1850-pwm";
>>>>>>
>>>>>> I would prefer "nxp,lpc1850-sct" for a couple of reasons. SCT is the
>>>>>> name of hardware block as described in the user manual and while PWM
>>>>>> is the most obvious usage for this block on Linux, the hardware is not
>>>>>> limited to just doing that. So as a bit of future proofing if someone
>>>>>> wants to use this block for more than PWM I would prefer SCT.
>>>>>>
>>>>>
>>>>> Shouldn't we use something like "nxp,lpc1850-sct-pwm"?
>>>>>
>>>>> Sounds like the word PWM should be in the compatible as it describes
>>>>> not only the device, but the device used in a certain way.
>>>>>
>>>>> Otherwise, how would we use "nxp,lpc1850-sct" to distinguish a PWM from
>>>>> e.g. a clocksource/clockevents (we can also use SCT for that)?
>>>>
>>>> I not sure how to best handle dt bindings with such flexible hardware blocks.
>>>>
>>>> But are you suggestion to have multiple drivers for the same hw block?
>>>> ie.: "nxp,lpc1850-sct-pwm" for PWM and for example
>>>> "nxp,lpc1850-sct-clocksource" for clocksource and then use/enable one
>>>> or the other.
>>>>
>>>
>>> Yup, I'm suggesting just that. If the hardware is capable, I can't see
>>> why we can't have different drivers for it.
>>
>> Well, I think it would be nice if we could have one driver per hw
>> block. But then you need a way to select whether you want to use the
>> counter in the SCT block for PWM or, for example, clocksource. Sort of
>> mode selection I guess.
>>
>> But then again if someone has some very custom and application
>> specific usage for the SCT block that maybe better served by a
>> standalone driver anyway.
>>
>
> Technically speaking, I believe we can use "nxp,lpc1850-sct" to expose
> both a PWM interface and a clocksource in a single driver. Not too
> clean, though.
Well, I don't think that is unclean way of doing it. But it's up to
the different subsystem maintainers if they want users outside their
normal directory as we would, for example, end of with a clocksource
under the pwm dir.
> Alternatively, we should be able to have a multifunction device (MFD) to mux
> the SCT resources to be consumed by different kind of drivers.
That is an other way of doing it.
But you would still need to a way to chose between either PWM or
clocksource since the SCT has a limited number of counters. (SCT can
be configured to have either two 16-bit timers or one 32-bit)
> Given there's no counter API to feed SCT, and given we use the other
> timer hw block to feed clocksource and clockevents for the LPC18/43xx,
> I think currently the simplest choice is to have "nxp,lpc1850-sct-pwm"
> describing this PWM SCT driver.
>
> If we later want to change completely the PWM SCT driver, we can do
> that without any bindings change.
Agreed. Let's keep it simple now and deal with it again if another
user for the SCT block turns up.
regards,
Joachim Eastwood
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-08-07 15:44 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-27 4:45 [PATCH 0/2] Add support for NXP LPC18xx PWM/SCT Ariel D'Alessandro
2015-07-27 4:45 ` Ariel D'Alessandro
2015-07-27 4:45 ` [PATCH 1/2] pwm: NXP LPC18xx PWM/SCT driver Ariel D'Alessandro
2015-07-27 4:45 ` Ariel D'Alessandro
2015-07-28 19:48 ` Ezequiel Garcia
2015-07-28 19:48 ` Ezequiel Garcia
2015-08-05 23:03 ` Ariel D'Alessandro
2015-08-05 23:03 ` Ariel D'Alessandro
2015-07-27 4:45 ` [PATCH 2/2] DT: pwm: Add NXP LPC18xx PWM/SCT binding documentation Ariel D'Alessandro
2015-07-27 4:45 ` Ariel D'Alessandro
2015-07-28 22:37 ` Joachim Eastwood
2015-07-28 22:37 ` Joachim Eastwood
2015-07-28 22:45 ` Ezequiel Garcia
2015-07-28 22:45 ` Ezequiel Garcia
2015-07-28 23:22 ` Joachim Eastwood
2015-07-28 23:22 ` Joachim Eastwood
[not found] ` <CAGhQ9VwudkK=bo5Qk79O7fmdEEm1KEWxUhLO14ucCJ+FbhqQqw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-07-29 13:47 ` Ezequiel Garcia
2015-07-29 13:47 ` Ezequiel Garcia
[not found] ` <CAAEAJfA22JA0AWB1FCiDVYEy5BqasDror_Ejpd=b3s14BWpORw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-07-30 23:30 ` Joachim Eastwood
2015-07-30 23:30 ` Joachim Eastwood
2015-07-31 18:09 ` Ezequiel Garcia
2015-07-31 18:09 ` Ezequiel Garcia
2015-08-05 23:13 ` Ariel D'Alessandro
2015-08-05 23:13 ` Ariel D'Alessandro
2015-08-07 15:44 ` Joachim Eastwood
2015-08-07 15:44 ` Joachim Eastwood
2015-07-29 13:19 ` Ariel D'Alessandro
2015-07-29 13:19 ` Ariel D'Alessandro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.