Final Samsung PWM support cleanup

This patch introduces new Samsung PWM driver, which uses Samsung PWM/timer master driver to control shared parts of the hardware.

Signed-off-by Tomasz Figa <[email protected]>

---
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 528
++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
insertions(+)
 create mode 100644 drivers/pwm/pwm-samsung.c

drivers/pwm/Makefile +0 -0

drivers/pwm/Makefile +57 -0

+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		dev_err(dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	chip->chip.dev = &pdev->dev;
+	chip->chip.ops = &pwm_samsung_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = SAMSUNG_PWM_NUM;
+
+	if (pdev->dev.of_node) {
+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data specified\n");
+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip->variant));
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get mem resource\n");
+		return -ENOMEM;
+	}
+
+	chip->base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!chip->base) {
+		dev_err(&pdev->dev, "failed to request and map registers\n");
+		return -ENOMEM;
+	}
+
+	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
+	if (IS_ERR(chip->base_clk)) {
+		dev_err(dev, "failed to get timer base clk\n");
+		return PTR_ERR(chip->base_clk);
+	}
+	clk_prepare_enable(chip->base_clk);
+
+	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
+	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
+

drivers/pwm/Makefile +10 -0

+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(chip->base_clk);
+
+	return ret;
+}

Am Mittwoch, 5. Juni 2013, 23:18:13 schrieb Tomasz Figa:

Quoted 15 lines

This patch introduces new Samsung PWM driver, which uses Samsung PWM/timer master driver to control shared parts of the hardware.

Signed-off-by Tomasz Figa <[email protected]>

---

 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 528

++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529 insertions(+)

 create mode 100644 drivers/pwm/pwm-samsung.c

drivers/pwm/Makefile +0 -0

[...]

Quoted 20 lines

drivers/pwm/Makefile +20 -0

+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		dev_err(dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	chip->chip.dev = &pdev->dev;
+	chip->chip.ops = &pwm_samsung_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = SAMSUNG_PWM_NUM;
+
+	if (pdev->dev.of_node) {
+		ret = pwm_samsung_parse_dt(chip);

drivers/pwm/Makefile

Quoted 8 lines

drivers/pwm/Makefile +8 -0

+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data 

drivers/pwm/Makefile

Quoted 5 lines

drivers/pwm/Makefile +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip-

variant));

drivers/pwm/Makefile

Quoted 11 lines

drivers/pwm/Makefile +11 -0

+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get mem resource\n");
+		return -ENOMEM;
+	}
+
+	chip->base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!chip->base) {
+		dev_err(&pdev->dev, "failed to request and map 

Quoted 18 lines

drivers/pwm/Makefile +18 -0

+		return -ENOMEM;
+	}
+
+	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
+	if (IS_ERR(chip->base_clk)) {
+		dev_err(dev, "failed to get timer base clk\n");
+		return PTR_ERR(chip->base_clk);
+	}
+	clk_prepare_enable(chip->base_clk);
+
+	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
+	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to register pwm\n");
+		goto err_clk_disable;
+	}

drivers/pwm/Makefile

Quoted 5 lines

drivers/pwm/Makefile +5 -0

+
+	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
+		clk_get_rate(chip->base_clk),
+		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
+		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);

hmm, these values simply tell some internal state of the pwm, so wouldn't a dev_dbg be more appropriate?

Quoted 10 lines

drivers/pwm/Makefile +10 -0

+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(chip->base_clk);
+
+	return ret;
+}

This patch introduces new Samsung PWM driver, which uses Samsung PWM/timer master driver to control shared parts of the hardware.

Signed-off-by Tomasz Figa <[email protected]>

---
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 528
++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
insertions(+)
 create mode 100644 drivers/pwm/pwm-samsung.c

drivers/pwm/Makefile +1 -0

index 229a599..833c3ac 100644
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
+obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o

drivers/pwm/pwm-samsung.c new file +11 -0

new file mode 100644
index 0000000..61bed3d
@@ -0,0 +1,528 @@
+/* drivers/pwm/pwm-samsung.c
+ *
+ * Copyright (c) 2007 Ben Dooks
+ * Copyright (c) 2008 Simtec Electronics
+ *     Ben Dooks <[email protected]>, <[email protected]>
+ * Copyright (c) 2013 Tomasz Figa <[email protected]>
+ *
+ * PWM driver for Samsung SoCs
+ *
+ * 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.

drivers/pwm/pwm-samsung.c new file +91 -0

+*/
+
+#include <linux/clk.h>
+#include <linux/export.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/time.h>
+
+#include <clocksource/samsung_pwm.h>
+
+#define REG_TCFG0			0x00
+#define REG_TCFG1			0x04
+#define REG_TCON			0x08
+
+#define REG_TCNTB(tmr)			(0x0c + ((tmr) * 0xc))
+#define REG_TCMPB(tmr)			(0x10 + ((tmr) * 0xc))
+
+#define TCFG0_PRESCALER_MASK		0xff
+#define TCFG0_PRESCALER1_SHIFT		8
+
+#define TCFG1_MUX_MASK			0xf
+#define TCFG1_SHIFT(x)			((x) * 4)
+
+#define TCON_START(chan)		(1 << (4 * (chan) + 0))
+#define TCON_MANUALUPDATE(chan)		(1 << (4 * (chan) + 1))
+#define TCON_INVERT(chan)		(1 << (4 * (chan) + 2))
+#define TCON_AUTORELOAD(chan)		(1 << (4 * (chan) + 3))
+
+struct samsung_pwm_channel {
+	unsigned long period_ns;
+	unsigned long duty_ns;
+	unsigned long tin_ns;
+};
+
+struct samsung_pwm_chip {
+	struct pwm_chip chip;
+	struct samsung_pwm_variant variant;
+	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];
+
+	void __iomem *base;
+	struct clk *base_clk;
+	struct clk *tclk0;
+	struct clk *tclk1;
+};
+#define to_samsung_pwm_chip(chip)	\
+			container_of(chip, struct samsung_pwm_chip, chip)
+
+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif
+
+static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
+					unsigned int channel, u8 divisor)
+{
+	u8 shift = TCFG1_SHIFT(channel);
+	unsigned long flags;
+	u32 reg;
+	u8 bits;
+
+	bits = (fls(divisor) - 1) - pwm->variant.div_base;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	reg = readl(pwm->base + REG_TCFG1);
+	reg &= ~(TCFG1_MUX_MASK << shift);
+	reg |= bits << shift;
+	writel(reg, pwm->base + REG_TCFG1);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,
+							unsigned int chan)
+{
+	struct samsung_pwm_variant *variant = &chip->variant;
+	u32 reg;
+
+	reg = readl(chip->base + REG_TCFG1);
+	reg >>= TCFG1_SHIFT(chan);
+	reg &= TCFG1_MUX_MASK;
+
+	return ((1 << reg) & variant->tclk_mask) == 0;
+}
+
+static unsigned long pwm_samsung_get_tin_rate(struct samsung_pwm_chip

*chip, + unsigned

drivers/pwm/pwm-samsung.c new file +15 -0

+{
+	unsigned long rate;
+	u32 reg;
+
+	rate = clk_get_rate(chip->base_clk);
+
+	reg = readl(chip->base + REG_TCFG0);
+	if (chan >= 2)
+		reg >>= TCFG0_PRESCALER1_SHIFT;
+	reg &= TCFG0_PRESCALER_MASK;
+
+	return rate / (reg + 1);
+}
+
+static unsigned long pwm_samsung_calc_tin(struct samsung_pwm_chip

*chip, + unsigned int chan,

drivers/pwm/pwm-samsung.c new file +6 -0

+{
+	struct samsung_pwm_variant *variant = &chip->variant;
+	unsigned long rate;
+	unsigned int div;
+	struct clk *clk;
+

drivers/pwm/pwm-samsung.c new file +12 -0

+	 * frequencies lower than requested.
+	 */
+	for (div = variant->div_base; div < 4; ++div)
+		if ((rate >> (variant->bits + div)) < freq)
+			break;
+
+	pwm_samsung_set_divisor(chip, chan, 1 << div);
+
+	return rate >> div;
+}
+
+static int pwm_samsung_request(struct pwm_chip *chip, struct pwm_device

*pwm) +{

drivers/pwm/pwm-samsung.c new file +6 -0

+			pwm->hwpwm);
+
+	return -EINVAL;
+}
+
+static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device

*pwm) +{

drivers/pwm/pwm-samsung.c new file +23 -0

+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long tcon;
+	unsigned long flags;
+
+	if (channel > 0)
+		++channel;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+
+	tcon &= ~TCON_MANUALUPDATE(channel);
+	tcon |= TCON_START(channel) | TCON_AUTORELOAD(channel);
+
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+
+	return 0;
+}
+
+static void pwm_samsung_disable(struct pwm_chip *chip, struct

pwm_device *pwm) +{

drivers/pwm/pwm-samsung.c new file +18 -0

+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long tcon;
+	unsigned long flags;
+
+	if (channel > 0)
+		++channel;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+	tcon &= ~TCON_AUTORELOAD(channel);
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
+static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device

*pwm, + int duty_ns, int period_ns)

drivers/pwm/pwm-samsung.c new file +3 -0

+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	struct samsung_pwm_channel *chan = &our_chip->channels[pwm-

hwpwm];

drivers/pwm/pwm-samsung.c new file +21 -0

+	unsigned long tin_ns = chan->tin_ns;
+	unsigned int tcon_chan = pwm->hwpwm;
+	unsigned long tin_rate;
+	unsigned long period;
+	unsigned long flags;
+	unsigned long tcnt;
+	long tcmp;
+	u32 tcon;
+
+	/* We currently avoid using 64bit arithmetic by using the
+	 * fact that anything faster than 1Hz is easily representable
+	 * by 32bits. */
+	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
+		return -ERANGE;
+
+	if (period_ns == chan->period_ns && duty_ns == chan->duty_ns)
+		return 0;
+
+	/* The TCMP and TCNT can be read without a lock, they're not
+	 * shared between the timers. */
+	tcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));

drivers/pwm/pwm-samsung.c new file +12 -0

+
+		chan->period_ns = period_ns;
+
+		dev_dbg(our_chip->chip.dev, "tin_rate=%lu\n", tin_rate);
+
+		tin_ns = NSEC_PER_SEC / tin_rate;
+		tcnt = period_ns / tin_ns;
+
+		chan->tin_ns = tin_ns;
+	}
+
+	/* Note, counters count down */

drivers/pwm/pwm-samsung.c new file +18 -0

+
+	if (tcmp < 0)
+		tcmp = 0;
+
+	/* Update the PWM register block. */
+	if (tcon_chan > 0)
+		++tcon_chan;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+
+	tcnt--;
+
+	tcon &= ~(TCON_START(tcon_chan) | TCON_AUTORELOAD(tcon_chan));
+	tcon |= TCON_MANUALUPDATE(tcon_chan);
+
+	__raw_writel(tcnt, our_chip->base + REG_TCNTB(pwm->hwpwm));

drivers/pwm/pwm-samsung.c new file +60 -0

+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long flags;
+	u32 tcon;
+
+	if (channel > 0)
+		++channel;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = __raw_readl(our_chip->base + REG_TCON);
+
+	/* Invert in hardware means normal polarity of PWM core */
+	if (polarity == PWM_POLARITY_NORMAL)
+		tcon |= TCON_INVERT(channel);
+	else
+		tcon &= ~TCON_INVERT(channel);
+
+	__raw_writel(tcon, our_chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+
+	return 0;
+}
+
+static struct pwm_ops pwm_samsung_ops = {
+	.request	= pwm_samsung_request,
+	.enable		= pwm_samsung_enable,
+	.disable	= pwm_samsung_disable,
+	.config		= pwm_samsung_config,
+	.set_polarity	= pwm_samsung_set_polarity,
+	.owner		= THIS_MODULE,
+};
+
+#ifdef CONFIG_OF
+static const struct samsung_pwm_variant s3c24xx_variant = {
+	.bits		= 16,
+	.div_base	= 1,
+	.has_tint_cstat	= false,
+	.tclk_mask	= (1 << 4),
+};
+
+static const struct samsung_pwm_variant s3c64xx_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
+};
+
+static const struct samsung_pwm_variant s5p64x0_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= 0,
+};
+
+static const struct samsung_pwm_variant s5p_variant = {
+	.bits		= 32,
+	.div_base	= 0,

drivers/pwm/pwm-samsung.c new file +9 -0

+	{},
+};
+#endif
+
+static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
+{
+	struct device_node *np = chip->chip.dev->of_node;
+	const struct of_device_id *match;
+	struct property *prop;

property\n", +

drivers/pwm/pwm-samsung.c new file +26 -0

+			continue;
+		}
+		chip->variant.output_mask |= 1 << val;
+	}
+
+	return 0;
+}
+
+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		dev_err(dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	chip->chip.dev = &pdev->dev;
+	chip->chip.ops = &pwm_samsung_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = SAMSUNG_PWM_NUM;
+

drivers/pwm/pwm-samsung.c new file +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip-

variant));

drivers/pwm/pwm-samsung.c new file +1 -0

+	}

drivers/pwm/pwm-samsung.c new file +86 -0

+		return -ENOMEM;
+	}
+
+	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
+	if (IS_ERR(chip->base_clk)) {
+		dev_err(dev, "failed to get timer base clk\n");
+		return PTR_ERR(chip->base_clk);
+	}
+	clk_prepare_enable(chip->base_clk);
+
+	chip->tclk0 = devm_clk_get(&pdev->dev, "pwm-tclk0");
+	chip->tclk1 = devm_clk_get(&pdev->dev, "pwm-tclk1");
+
+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to register pwm\n");
+		goto err_clk_disable;
+	}
+
+	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
+		clk_get_rate(chip->base_clk),
+		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
+		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(chip->base_clk);
+
+	return ret;
+}
+
+static int pwm_samsung_remove(struct platform_device *pdev)
+{
+	struct samsung_pwm_chip *chip = platform_get_drvdata(pdev);
+	int err;
+
+	err = pwmchip_remove(&chip->chip);
+	if (err < 0)
+		return err;
+
+	clk_disable_unprepare(chip->base_clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int pwm_samsung_suspend(struct device *dev)
+{
+	struct samsung_pwm_chip *chip = dev_get_drvdata(dev);
+	int i;
+
+	/* No one preserve these values during suspend so reset them
+	 * Otherwise driver leaves PWM unconfigured if same values
+	 * passed to pwm_config
+	 */
+	for (i = 0; i < SAMSUNG_PWM_NUM; ++i) {
+		chip->channels[i].period_ns = 0;
+		chip->channels[i].duty_ns = 0;
+	}
+
+	return 0;
+}
+#endif
+
+static struct dev_pm_ops pwm_samsung_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pwm_samsung_suspend, NULL)
+};
+
+static struct platform_driver pwm_samsung_driver = {
+	.driver		= {
+		.name	= "samsung-pwm",
+		.owner	= THIS_MODULE,
+		.pm	= &pwm_samsung_pm_ops,
+		.of_match_table = of_match_ptr(samsung_pwm_matches),
+	},
+	.probe		= pwm_samsung_probe,
+	.remove		= pwm_samsung_remove,
+};
+module_platform_driver(pwm_samsung_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tomasz Figa <[email protected]>");
+MODULE_ALIAS("platform:samsung-pwm");

Hi Kukjin,

Quoted 10 lines · On Wednesday 05 of June 2013 23:18:13 Tomasz Figa wrote:

This patch introduces new Samsung PWM driver, which uses Samsung PWM/timer master driver to control shared parts of the hardware.

Signed-off-by Tomasz Figa<[email protected]>

---
  drivers/pwm/Makefile      |   1 +
  drivers/pwm/pwm-samsung.c | 528

++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529 insertions(+)
create mode 100644 drivers/pwm/pwm-samsung.c

I you haven't yet merged this patch, please let me send v2 of this series, addressing Heiko's comments and adjustments to my understanding of the PWM hardware after getting access to SMDK6410 board, which uses PWM to control LCD backlight.

This patch introduces new Samsung PWM driver, which uses Samsung PWM/timer master driver to control shared parts of the hardware.

Signed-off-by Tomasz Figa <[email protected]>

---
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 529 insertions(+)
 create mode 100644 drivers/pwm/pwm-samsung.c

drivers/pwm/Makefile +1 -0

index 229a599..833c3ac 100644
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
+obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o

drivers/pwm/pwm-samsung.c new file +0 -0

new file mode 100644
index 0000000..61bed3d

drivers/pwm/pwm-samsung.c new file +50 -0

+#ifdef CONFIG_OF
+static const struct samsung_pwm_variant s3c24xx_variant = {
+	.bits		= 16,
+	.div_base	= 1,
+	.has_tint_cstat	= false,
+	.tclk_mask	= (1 << 4),
+};
+
+static const struct samsung_pwm_variant s3c64xx_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
+};
+
+static const struct samsung_pwm_variant s5p64x0_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= 0,
+};
+
+static const struct samsung_pwm_variant s5p_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 5),
+};
+
+static const struct of_device_id samsung_pwm_matches[] = {
+	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
+	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
+	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
+	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
+	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant },
+	{},
+};
+#endif
+
+static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
+{
+	struct device_node *np = chip->chip.dev->of_node;
+	const struct of_device_id *match;
+	struct property *prop;
+	const __be32 *cur;
+	u32 val;
+
+	match = of_match_node(samsung_pwm_matches, np);
+	if (!match)
+		return -ENODEV;

drivers/pwm/pwm-samsung.c new file +9 -0

+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {

drivers/pwm/pwm-samsung.c new file +5 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;

drivers/pwm/pwm-samsung.c new file +6 -0

+		goto err_clk_disable;
+	}
+
+	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
+		clk_get_rate(chip->base_clk),
+		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,

Quoted 4 lines · Tomasz Figa

This patch introduces new Samsung PWM driver, which uses Samsung PWM/timer master driver to control shared parts of the hardware.

Signed-off-by Tomasz Figa <[email protected]>

Sorry for jumping in so late, I've been busy with other things lately.

Quoted 24 lines

---

 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-samsung.c | 528
 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 529
 insertions(+)
 create mode 100644 drivers/pwm/pwm-samsung.c

drivers/pwm/Makefile +1 -0

index 229a599..833c3ac 100644
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung-legacy.o
 
+obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 

obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o

drivers/pwm/pwm-samsung.c line 1

Quoted 7 lines

drivers/pwm/pwm-samsung.c new file +1 -0

new file mode 100644
index 0000000..61bed3d
@@ -0,0 +1,528 @@
+/* drivers/pwm/pwm-samsung.c

Nit: this line can be dropped. It serves no purpose.

Quoted 9 lines

drivers/pwm/pwm-samsung.c new file +9 -0

+ *
+ * Copyright (c) 2007 Ben Dooks
+ * Copyright (c) 2008 Simtec Electronics
+ *     Ben Dooks <[email protected]>, <[email protected]>
+ * Copyright (c) 2013 Tomasz Figa <[email protected]>
+ *
+ * PWM driver for Samsung SoCs
+ *
+ * This program is free software; you can redistribute it and/or

Quoted 3 lines

modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License. +*/

Nit: the */ should align with the * above.

drivers/pwm/pwm-samsung.c line 56

Quoted 10 lines

drivers/pwm/pwm-samsung.c new file +10 -0

+struct samsung_pwm_channel {
+	unsigned long period_ns;
+	unsigned long duty_ns;
+	unsigned long tin_ns;
+};
+
+struct samsung_pwm_chip {
+	struct pwm_chip chip;
+	struct samsung_pwm_variant variant;
+	struct samsung_pwm_channel channels[SAMSUNG_PWM_NUM];

The new driver for Renesas did something similar, but I want to discourage storing per-channel data within the chip structure.

The PWM framework provides a way to store this information along with the PWM device (see pwm_{set,get}_chip_data()).

drivers/pwm/pwm-samsung.c line 64

Quoted 8 lines

drivers/pwm/pwm-samsung.c new file +8 -0

+
+	void __iomem *base;
+	struct clk *base_clk;
+	struct clk *tclk0;
+	struct clk *tclk1;
+};
+#define to_samsung_pwm_chip(chip)	\
+			container_of(chip, struct samsung_pwm_chip, chip)

Can you turn this into a static inline function please?

drivers/pwm/pwm-samsung.c line 68

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of samsung_pwm_chip?

drivers/pwm/pwm-samsung.c line 71

Quoted 2 lines

drivers/pwm/pwm-samsung.c new file +2 -0

+static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
+					unsigned int channel, u8 divisor)

Nit: please align arguments on subsequent lines with the first argument of the first line. There's many more of these but I haven't mentioned them all explicitly.

Quoted 1 line

+static inline int pwm_samsung_is_tdiv(struct samsung_pwm_chip *chip,

Any particular reason for making this inline?

drivers/pwm/pwm-samsung.c

Quoted 5 lines

+static int pwm_samsung_config(struct pwm_chip *chip, struct
pwm_device *pwm, + int duty_ns, int period_ns)

drivers/pwm/pwm-samsung.c new file +3 -0

+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	struct samsung_pwm_channel *chan = &our_chip->channels[pwm-

hwpwm];

drivers/pwm/pwm-samsung.c line 223

Quoted 6 lines

drivers/pwm/pwm-samsung.c new file +6 -0

+	unsigned long tin_ns = chan->tin_ns;
+	unsigned int tcon_chan = pwm->hwpwm;
+	unsigned long tin_rate;
+	unsigned long period;
+	unsigned long flags;
+	unsigned long tcnt;

Many of these unsigned long variable could be declared on a single line to make the function shorter.

drivers/pwm/pwm-samsung.c line 229

Quoted 6 lines

drivers/pwm/pwm-samsung.c new file +6 -0

+	long tcmp;
+	u32 tcon;
+
+	/* We currently avoid using 64bit arithmetic by using the
+	 * fact that anything faster than 1Hz is easily representable
+	 * by 32bits. */

Can you turn these into proper block-style comments? Like so:

/*
* We currently...
* ...
* by 32 bits.
*/

drivers/pwm/pwm-samsung.c line 231

Quoted 2 lines

drivers/pwm/pwm-samsung.c new file +2 -0

+	if (period_ns > NSEC_PER_SEC || duty_ns > NSEC_PER_SEC)
+		return -ERANGE;

Note that technically you only need to check period_ns because the core already ensures that duty_ns <= period_ns.

drivers/pwm/pwm-samsung.c

Quoted 2 lines

drivers/pwm/pwm-samsung.c new file +2 -0

+static int pwm_samsung_set_polarity(struct pwm_chip *chip,
+			struct pwm_device *pwm, enum pwm_polarity 

Quoted 4 lines

drivers/pwm/pwm-samsung.c new file +4 -0

+{
+	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
+	unsigned int channel = pwm->hwpwm;
+	unsigned long flags;

drivers/pwm/pwm-samsung.c line 306

Quoted 4 lines

drivers/pwm/pwm-samsung.c new file +4 -0

+	u32 tcon;
+
+	if (channel > 0)
+		++channel;

You have to repeat that in quite a few places, so I wonder if it'd make sense to wrap it into a function and add a comment about why the increment is necessary.

Quoted 1 line

+static struct pwm_ops pwm_samsung_ops = {

"static const" please.

Quoted 47 lines

drivers/pwm/pwm-samsung.c new file +35 -0

+#ifdef CONFIG_OF
+static const struct samsung_pwm_variant s3c24xx_variant = {
+	.bits		= 16,
+	.div_base	= 1,
+	.has_tint_cstat	= false,
+	.tclk_mask	= (1 << 4),
+};
+
+static const struct samsung_pwm_variant s3c64xx_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 7) | (1 << 6) | (1 << 5),
+};
+
+static const struct samsung_pwm_variant s5p64x0_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= 0,
+};
+
+static const struct samsung_pwm_variant s5p_variant = {
+	.bits		= 32,
+	.div_base	= 0,
+	.has_tint_cstat	= true,
+	.tclk_mask	= (1 << 5),
+};
+
+static const struct of_device_id samsung_pwm_matches[] = {
+	{ .compatible = "samsung,s3c2410-pwm", .data = &s3c24xx_variant },
+	{ .compatible = "samsung,s3c6400-pwm", .data = &s3c64xx_variant },
+	{ .compatible = "samsung,s5p6440-pwm", .data = &s5p64x0_variant },
+	{ .compatible = "samsung,s5pc100-pwm", .data = &s5p_variant },
+	{ .compatible = "samsung,exynos4210-pwm", .data = &s5p64x0_variant

},

drivers/pwm/pwm-samsung.c new file +11 -0

+	{},
+};
+#endif
+
+static int pwm_samsung_parse_dt(struct samsung_pwm_chip *chip)
+{
+	struct device_node *np = chip->chip.dev->of_node;
+	const struct of_device_id *match;
+	struct property *prop;
+	const __be32 *cur;
+	u32 val;

drivers/pwm/pwm-samsung.c

Quoted 8 lines

drivers/pwm/pwm-samsung.c new file +8 -0

+
+	match = of_match_node(samsung_pwm_matches, np);
+	if (!match)
+		return -ENODEV;
+
+	memcpy(&chip->variant, match->data, sizeof(chip->variant));
+
+	of_property_for_each_u32(np, "samsung,pwm-outputs", prop, cur, 

drivers/pwm/pwm-samsung.c

Quoted 3 lines

{

drivers/pwm/pwm-samsung.c new file +2 -0

+		if (val >= SAMSUNG_PWM_NUM) {
+			pr_warning("%s: invalid channel index in 

Quoted 1 line

property\n", +

drivers/pwm/pwm-samsung.c line 393

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+			continue;
+		}
+		chip->variant.output_mask |= 1 << val;

Could the output_mask be moved to the struct samsung_pwm_chip instead? The reason I ask is because it would allow you to make the variant constant throughout the driver.

Quoted 14 lines

drivers/pwm/pwm-samsung.c new file +14 -0

+static int pwm_samsung_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct samsung_pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (chip == NULL) {
+		dev_err(dev, "failed to allocate driver data\n");
+		return -ENOMEM;
+	}
+
+	chip->chip.dev = &pdev->dev;

drivers/pwm/pwm-samsung.c line 417

Quoted 5 lines

drivers/pwm/pwm-samsung.c new file +5 -0

+	chip->chip.ops = &pwm_samsung_ops;
+	chip->chip.base = -1;
+	chip->chip.npwm = SAMSUNG_PWM_NUM;
+
+	if (pdev->dev.of_node) {

Maybe add an IS_ENABLED(CONFIG_OF) check here? That'd allow all OF- related code to be thrown away if OF isn't selected.

drivers/pwm/pwm-samsung.c

Quoted 9 lines

drivers/pwm/pwm-samsung.c new file +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data 

drivers/pwm/pwm-samsung.c

Quoted 5 lines

drivers/pwm/pwm-samsung.c new file +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip-

variant));

drivers/pwm/pwm-samsung.c

Quoted 1 line

drivers/pwm/pwm-samsung.c new file +1 -0

+	}

Obviously this needs some modification in order for the variant to become constant. But I think you can easily do so by making the driver match using the platform_driver's id_table field, similar to how the matching is done for OF.

drivers/pwm/pwm-samsung.c

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+	chip->base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!chip->base) {
+		dev_err(&pdev->dev, "failed to request and map 

drivers/pwm/pwm-samsung.c

Quoted 2 lines

drivers/pwm/pwm-samsung.c new file +2 -0

+		return -ENOMEM;
+	}

devm_request_and_ioremap() is now deprecated and in the process of being removed. You should use devm_ioremap_resource() instead.

drivers/pwm/pwm-samsung.c line 451

Quoted 7 lines

drivers/pwm/pwm-samsung.c new file +7 -0

+
+	chip->base_clk = devm_clk_get(&pdev->dev, "timers");
+	if (IS_ERR(chip->base_clk)) {
+		dev_err(dev, "failed to get timer base clk\n");
+		return PTR_ERR(chip->base_clk);
+	}
+	clk_prepare_enable(chip->base_clk);

You need to check the return value of clk_prepare_enable(). And if I was very pedantic, there should be a blank line before this one.

drivers/pwm/pwm-samsung.c line 458

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+	ret = pwmchip_add(&chip->chip);
+	if (ret < 0) {
+		dev_err(dev, "failed to register pwm\n");

"failed to register PWM chip" please.

Quoted 12 lines

drivers/pwm/pwm-samsung.c new file +12 -0

+		goto err_clk_disable;
+	}
+
+	dev_info(dev, "base_clk at %lu, tclk0 at %lu, tclk1 at %lu\n",
+		clk_get_rate(chip->base_clk),
+		!IS_ERR(chip->tclk0) ? clk_get_rate(chip->tclk0) : 0,
+		!IS_ERR(chip->tclk1) ? clk_get_rate(chip->tclk1) : 0);
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+

drivers/pwm/pwm-samsung.c line 474

Quoted 4 lines

drivers/pwm/pwm-samsung.c new file +4 -0

+err_clk_disable:
+	clk_disable_unprepare(chip->base_clk);
+
+	return ret;

There's only a single case where this can actually happen, so I don't think you need the label here. Just put the clk_disable_unprepare() call and the return statement where you jump to the label.

Quoted 1 line

+#ifdef CONFIG_PM

I think this should really be CONFIG_PM_SLEEP.

Quoted 1 line

+static struct dev_pm_ops pwm_samsung_pm_ops = {

"static const" please.

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

Quoted 6 lines

drivers/pwm/pwm-samsung.c line 68

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of samsung_pwm_chip?

There are few registers shared with samsung_pwm_timer clocksource driver and so normally the spinlock is exported from it. However on on some platforms (namely Exynos >=4x12) kernel can be compiled without that driver, so the lock must be defined locally, just to synchronize multiple PWM channels, as they share registers as well.

Quoted 9 lines

drivers/pwm/pwm-samsung.c

Quoted 9 lines

drivers/pwm/pwm-samsung.c new file +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data 

specified\n");

Quoted 6 lines

drivers/pwm/pwm-samsung.c

Quoted 5 lines

drivers/pwm/pwm-samsung.c new file +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							sizeof(chip-

variant));

Quoted 6 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Obviously this needs some modification in order for the variant to become constant. But I think you can easily do so by making the driver match using the platform_driver's id_table field, similar to how the matching is done for OF.

Generally output_mask is board-dependent and is passed inside a variant struct using platform_data pointer.

Same platform data is used in samsung_pwm_timer clocksource driver, so I just reused it here without adding the need to rename platform device at runtime (see arch/arm/plat-samsung/devs.c).

Quoted 2 lines · Tomasz Figa

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

[...]

Quoted 7 lines

Quoted 6 lines

drivers/pwm/pwm-samsung.c line 68

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of samsung_pwm_chip?

Quoted 5 lines

There are few registers shared with samsung_pwm_timer clocksource driver and so normally the spinlock is exported from it. However on on some platforms (namely Exynos >=4x12) kernel can be compiled without that driver, so the lock must be defined locally, just to synchronize multiple PWM channels, as they share registers as well.

Okay, I think this needs further explanation. The clocksource driver is used for what exactly? From a quick look it seems to be very much PWM- specific. According to the device tree binding for the PWM driver, the timer blocks can also be used as clock sources and clock event timers. So if I understand correctly you have setups where you use one or more channels as clock source or clock event timer and one or more channels as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize accesses. You need to do much more than that. To properly split this across several drivers there needs to be a mechanism to allocate channels for use either as clock source/event timer or PWM. Otherwise, how do you know that drivers aren't stepping on each other's toes?

Quoted 12 lines

Quoted 9 lines

drivers/pwm/pwm-samsung.c

Quoted 9 lines

drivers/pwm/pwm-samsung.c new file +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data

specified\n");

Quoted 5 lines

Quoted 5 lines

drivers/pwm/pwm-samsung.c line 433

Quoted 5 lines

drivers/pwm/pwm-samsung.c new file +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							

Quoted 11 lines

Quoted 10 lines

variant));

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Obviously this needs some modification in order for the variant to become constant. But I think you can easily do so by making the driver
match using the platform_driver's id_table field, similar to how the matching is done for OF.

Quoted 3 lines

Generally output_mask is board-dependent and is passed inside a variant
struct using platform_data pointer.

That's okay. But output_mask is the only thing that's board-dependent. Everything else in the variant is SoC dependent judging by the OF device table. So really only the output_mask should be part of the platform data.

Quoted 3 lines

Same platform data is used in samsung_pwm_timer clocksource driver, so I just reused it here without adding the need to rename platform device at runtime (see arch/arm/plat-samsung/devs.c).

Looking a bit at git log for the clocksource driver, there's this commit:

a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem

That's an ugly workaround for sharing registers between two drivers. There's a reason why drivers do request_mem_region(), and it is precisely to prevent them from accessing the same registers. As I already said above, I think you need to come up with some sort of API to share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm and pwm-tiecap drivers, which use a shared block of registers. Initially something similar was done as you do here, but eventually we came up with a much better solution that involved introducing a new driver for the shared functionality and an exported API.

The situation seems to be somewhat different here since you actually share the same resources for different functionality instead of sharing one subset of register across multiple drivers, but I think a similar solution can be applied here.

Quoted 44 lines · On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:

Quoted 2 lines · Tomasz Figa

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

[...]

Quoted 7 lines

Quoted 6 lines

drivers/pwm/pwm-samsung.c line 68

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of samsung_pwm_chip?

Quoted 5 lines

There are few registers shared with samsung_pwm_timer clocksource driver and so normally the spinlock is exported from it. However on on some platforms (namely Exynos >=4x12) kernel can be compiled without that driver, so the lock must be defined locally, just to synchronize multiple PWM channels, as they share registers as well.

Okay, I think this needs further explanation. The clocksource driver is used for what exactly? From a quick look it seems to be very much PWM- specific. According to the device tree binding for the PWM driver, the timer blocks can also be used as clock sources and clock event timers. So if I understand correctly you have setups where you use one or more channels as clock source or clock event timer and one or more channels as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize accesses. You need to do much more than that. To properly split this across several drivers there needs to be a mechanism to allocate channels for use either as clock source/event timer or PWM. Otherwise, how do you know that drivers aren't stepping on each other's toes?

Quoted 12 lines

Quoted 9 lines

drivers/pwm/pwm-samsung.c

Quoted 9 lines

drivers/pwm/pwm-samsung.c new file +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data

specified\n");

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/pwm/pwm-samsung.c line 433

Quoted 5 lines

drivers/pwm/pwm-samsung.c new file +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+							

sizeof(chip-

Quoted 20 lines

Quoted 11 lines

Quoted 10 lines

variant));

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Obviously this needs some modification in order for the variant to become constant. But I think you can easily do so by making the driver
match using the platform_driver's id_table field, similar to how the matching is done for OF.

Quoted 3 lines

Generally output_mask is board-dependent and is passed inside a variant
struct using platform_data pointer.

That's okay. But output_mask is the only thing that's board-dependent. Everything else in the variant is SoC dependent judging by the OF device table. So really only the output_mask should be part of the platform data.

Quoted 8 lines

Quoted 3 lines

Same platform data is used in samsung_pwm_timer clocksource driver, so I just reused it here without adding the need to rename platform device at runtime (see arch/arm/plat-samsung/devs.c).

Looking a bit at git log for the clocksource driver, there's this commit:

a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem

region

Quoted 13 lines

That's an ugly workaround for sharing registers between two drivers. There's a reason why drivers do request_mem_region(), and it is precisely to prevent them from accessing the same registers. As I already said above, I think you need to come up with some sort of API to share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm and pwm-tiecap drivers, which use a shared block of registers. Initially something similar was done as you do here, but eventually we came up with a much better solution that involved introducing a new driver for the shared functionality and an exported API.

Quoted 4 lines

The situation seems to be somewhat different here since you actually share the same resources for different functionality instead of sharing one subset of register across multiple drivers, but I think a similar solution can be applied here.

Well, current design, or rather lack of thereof, has been established after a significant amount of discussion, mostly with ARM SoC and MFD maintainers.

It started from a simple reorganization of the clocksource driver to be multiplatform friendly, without any means of synchronizing both drivers other than local_irq_save() and _restore() that used to be there originally, before I started my work on this. This was enough to work correctly, because both drivers at the same time are used only on uniprocessor systems.

Here's the thread with patches:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=17267

Then I opened a Pandora's Box by asking for opinion in this thread:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=16500

After that I started working on a complex framework for sharing this IP block that would solve all the problems with synchronization, channel allocation, device tree parsing, variant management, etc.

Here's an initial version, which started as an MFD master driver, which would let 2 client drivers use the hardware (only the clocksource part was implemented in that series):
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=229250

Still, that version didn't receive too good feedback, with comments pointing me to change the architecture to master-slave, with the clocksource driver being the master and PWM driver being the slave. And so v5 was made:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

This version was generally accepted, but then we discussed on IRC (mostly me and Arnd) whether such complex infrastructure is really needed and concluded that for this platform a simple shared spinlock would be enough, based on following reasons:
- on all SoCs on which the clocksource driver is going to be used there is only one instance of the PWM block, with 5 channels

- on all existing boards there are always at least two channels that don't have outputs

- operations on particular PWM channels must be synchronized anyway, because there are registers shared by all PWM channels (TCON being most important where enable, autoreload and invert bits are located for all channels).

And so we got to current design which is basically a shared spinlock and per-board output_mask, which specifies which channels have outputs on particular boards. If you look to the clocksource driver, you can see that it tries to allocate two output-less channels for timekeeping purposes and if it fails to do so, it simply panics.

IMHO this solution is fine, because it's simple, has little overhead and it just works in case of platforms for which it is needed.

Quoted 144 lines · Tomasz Figa

Quoted 51 lines · On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:

Quoted 2 lines · Tomasz Figa

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

[...]

Quoted 7 lines

Quoted 6 lines

drivers/pwm/pwm-samsung.c line 68

Quoted 3 lines

drivers/pwm/pwm-samsung.c new file +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of samsung_pwm_chip?

Quoted 5 lines

There are few registers shared with samsung_pwm_timer clocksource driver and so normally the spinlock is exported from it. However on on some platforms (namely Exynos >=4x12) kernel can be compiled without that driver, so the lock must be defined locally, just to synchronize multiple PWM channels, as they share registers as well.

Okay, I think this needs further explanation. The clocksource driver is
used for what exactly? From a quick look it seems to be very much PWM-
specific. According to the device tree binding for the PWM driver, the
timer blocks can also be used as clock sources and clock event timers.
So if I understand correctly you have setups where you use one or more
channels as clock source or clock event timer and one or more channels
as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize accesses. You need to do much more than that. To properly split this across several drivers there needs to be a mechanism to allocate channels for use either as clock source/event timer or PWM. Otherwise,
how do you know that drivers aren't stepping on each other's toes?

Quoted 12 lines

Quoted 9 lines

drivers/pwm/pwm-samsung.c

Quoted 9 lines

drivers/pwm/pwm-samsung.c new file +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data

specified\n");

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/pwm/pwm-samsung.c line 433

Quoted 5 lines

drivers/pwm/pwm-samsung.c new file +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+

sizeof(chip-

Quoted 28 lines

Quoted 12 lines

Quoted 11 lines

variant));

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Obviously this needs some modification in order for the variant to
become constant. But I think you can easily do so by making the driver
match using the platform_driver's id_table field, similar to how the
matching is done for OF.

Quoted 3 lines

Generally output_mask is board-dependent and is passed inside a variant
struct using platform_data pointer.

That's okay. But output_mask is the only thing that's board-dependent.
Everything else in the variant is SoC dependent judging by the OF device
table. So really only the output_mask should be part of the platform data.

Quoted 4 lines

Same platform data is used in samsung_pwm_timer clocksource driver, so
I just reused it here without adding the need to rename platform device at runtime (see arch/arm/plat-samsung/devs.c).

Quoted 4 lines

Looking a bit at git log for the clocksource driver, there's this

commit:
a3ce54f clocksource: samsung_pwm_timer: Do not request PWM mem

region

Quoted 14 lines

That's an ugly workaround for sharing registers between two drivers. There's a reason why drivers do request_mem_region(), and it is precisely to prevent them from accessing the same registers. As I already said above, I think you need to come up with some sort of API to
share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm and pwm-tiecap drivers, which use a shared block of registers. Initially something similar was done as you do here, but eventually we came up with a much better solution that involved introducing a new driver for
the shared functionality and an exported API.

Quoted 5 lines

The situation seems to be somewhat different here since you actually share the same resources for different functionality instead of sharing
one subset of register across multiple drivers, but I think a similar solution can be applied here.

Well, current design, or rather lack of thereof, has been established after a significant amount of discussion, mostly with ARM SoC and MFD maintainers.

It started from a simple reorganization of the clocksource driver to be multiplatform friendly, without any means of synchronizing both drivers other than local_irq_save() and _restore() that used to be there originally, before I started my work on this. This was enough to work correctly, because both drivers at the same time are used only on uniprocessor systems.

Here's the thread with patches:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1726
7

Then I opened a Pandora's Box by asking for opinion in this thread:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1650
0

After that I started working on a complex framework for sharing this IP block that would solve all the problems with synchronization, channel allocation, device tree parsing, variant management, etc.

Here's an initial version, which started as an MFD master driver, which would let 2 client drivers use the hardware (only the clocksource part was implemented in that series):
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2292
50

Quoted 5 lines

Still, that version didn't receive too good feedback, with comments pointing me to change the architecture to master-slave, with the clocksource driver being the master and PWM driver being the slave. And so v5 was made:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

Yeah, this seemed to be going in the right direction.

Quoted 6 lines

This version was generally accepted, but then we discussed on IRC (mostly me and Arnd) whether such complex infrastructure is really needed and concluded that for this platform a simple shared spinlock would be enough, based on following reasons:
- on all SoCs on which the clocksource driver is going to be used there is only one instance of the PWM block, with 5 channels

You say that now. But then at some point somebody decides that it might be useful to have a second instance.

Quoted 2 lines

- on all existing boards there are always at least two channels that don't have outputs

I don't see how that's relevant here.

Quoted 4 lines

- operations on particular PWM channels must be synchronized anyway, because there are registers shared by all PWM channels (TCON being most important where enable, autoreload and invert bits are located for all channels).

And that's precisely why there should be a more explicit way of synchronizing than just acquiring a lock. Otherwise you have no possibility whatsoever to detect when a board tries to use both the clocksource and PWM drivers in incompatible ways. The lock may prevent concurrent accesses to the shared registers, but that won't help you if the PWM driver suddenly decides to use an output that was originally meant to be used as clock source.

Quoted 7 lines

And so we got to current design which is basically a shared spinlock and
per-board output_mask, which specifies which channels have outputs on particular boards. If you look to the clocksource driver, you can see that it tries to allocate two output-less channels for timekeeping purposes and if it fails to do so, it simply panics.

Quoted 3 lines

IMHO this solution is fine, because it's simple, has little overhead and
it just works in case of platforms for which it is needed.

Okay, I like simple. But I see too much potential for this to break in the future. There are just too many assumptions for my taste. It isn't anything that I'm looking forward to have to maintain.

Quoted 102 lines · On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:

Quoted 101 lines · Tomasz Figa

Quoted 59 lines · On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:

Quoted 2 lines · Tomasz Figa

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

[...]

Quoted 8 lines

Quoted 7 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of
samsung_pwm_chip?

Quoted 9 lines

There are few registers shared with samsung_pwm_timer clocksource
driver and so normally the spinlock is exported from it. However on
on some platforms (namely Exynos >=4x12) kernel can be compiled without that driver, so the lock must be defined locally, just to
synchronize multiple PWM channels, as they share registers as well.

Okay, I think this needs further explanation. The clocksource driver
is
used for what exactly? From a quick look it seems to be very much PWM-
specific. According to the device tree binding for the PWM driver, the
timer blocks can also be used as clock sources and clock event timers.
So if I understand correctly you have setups where you use one or more
channels as clock source or clock event timer and one or more channels
as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize
accesses. You need to do much more than that. To properly split this
across several drivers there needs to be a mechanism to allocate channels for use either as clock source/event timer or PWM. Otherwise,
how do you know that drivers aren't stepping on each other's toes?

Quoted 12 lines

Quoted 9 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data

specified\n");

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+

sizeof(chip-

Quoted 33 lines

Quoted 15 lines

Quoted 14 lines

variant));

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Obviously this needs some modification in order for the variant
to
become constant. But I think you can easily do so by making the
driver
match using the platform_driver's id_table field, similar to how
the
matching is done for OF.

Quoted 3 lines

Generally output_mask is board-dependent and is passed inside a variant
struct using platform_data pointer.

That's okay. But output_mask is the only thing that's board-dependent.
Everything else in the variant is SoC dependent judging by the OF device
table. So really only the output_mask should be part of the platform
data.

Quoted 5 lines

Same platform data is used in samsung_pwm_timer clocksource driver,
so
I just reused it here without adding the need to rename platform device at runtime (see arch/arm/plat-samsung/devs.c).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Looking a bit at git log for the clocksource driver, there's this

commit:
a3ce54f clocksource: samsung_pwm_timer: Do not request PWM

Quoted 88 lines

Quoted 71 lines

region

Quoted 19 lines

That's an ugly workaround for sharing registers between two drivers.
There's a reason why drivers do request_mem_region(), and it is precisely to prevent them from accessing the same registers. As I already said above, I think you need to come up with some sort of API
to
share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm and
pwm-tiecap drivers, which use a shared block of registers. Initially
something similar was done as you do here, but eventually we came up
with a much better solution that involved introducing a new driver for
the shared functionality and an exported API.

Quoted 7 lines

The situation seems to be somewhat different here since you actually
share the same resources for different functionality instead of sharing
one subset of register across multiple drivers, but I think a similar
solution can be applied here.

Well, current design, or rather lack of thereof, has been established
after a significant amount of discussion, mostly with ARM SoC and MFD
maintainers.

It started from a simple reorganization of the clocksource driver to be
multiplatform friendly, without any means of synchronizing both drivers
other than local_irq_save() and _restore() that used to be there originally, before I started my work on this. This was enough to work
correctly, because both drivers at the same time are used only on uniprocessor systems.

Here's the thread with patches:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1
726
7

Then I opened a Pandora's Box by asking for opinion in this thread:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1
650
0

After that I started working on a complex framework for sharing this IP
block that would solve all the problems with synchronization, channel
allocation, device tree parsing, variant management, etc.

Here's an initial version, which started as an MFD master driver, which
would let 2 client drivers use the hardware (only the clocksource part
was implemented in that series):
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2
292
50

Quoted 6 lines

Still, that version didn't receive too good feedback, with comments pointing me to change the architecture to master-slave, with the clocksource driver being the master and PWM driver being the slave. And
so v5 was made:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

Yeah, this seemed to be going in the right direction.

Quoted 7 lines

This version was generally accepted, but then we discussed on IRC (mostly me and Arnd) whether such complex infrastructure is really needed and concluded that for this platform a simple shared spinlock would be enough, based on following reasons:
- on all SoCs on which the clocksource driver is going to be used there
is only one instance of the PWM block, with 5 channels

Quoted 3 lines

You say that now. But then at some point somebody decides that it might
be useful to have a second instance.

No, he won't.

Simply because no new SoC is going to be made which will use the samsung_pwm_timer driver and all existing ones have only a single instance of this IP.

Quoted 4 lines

Quoted 2 lines

- on all existing boards there are always at least two channels that don't have outputs

I don't see how that's relevant here.

This is the information passed through output_mask. Clocksource driver can use _only_ channels without output and PWM driver can use _only_ channels with output.

Both drivers receive the same output_mask value, either from the same variant structure or from device tree.

Quoted 7 lines

Quoted 6 lines

- operations on particular PWM channels must be synchronized anyway, because there are registers shared by all PWM channels (TCON being most
important where enable, autoreload and invert bits are located for all
channels).

Quoted 8 lines

And that's precisely why there should be a more explicit way of synchronizing than just acquiring a lock. Otherwise you have no possibility whatsoever to detect when a board tries to use both the clocksource and PWM drivers in incompatible ways. The lock may prevent concurrent accesses to the shared registers, but that won't help you if
the PWM driver suddenly decides to use an output that was originally meant to be used as clock source.

See above.

Quoted 13 lines

Quoted 12 lines

And so we got to current design which is basically a shared spinlock and
per-board output_mask, which specifies which channels have outputs on
particular boards. If you look to the clocksource driver, you can see
that it tries to allocate two output-less channels for timekeeping purposes and if it fails to do so, it simply panics.

IMHO this solution is fine, because it's simple, has little overhead and
it just works in case of platforms for which it is needed.

Quoted 3 lines

Okay, I like simple. But I see too much potential for this to break in the future. There are just too many assumptions for my taste. It isn't anything that I'm looking forward to have to maintain.

Those are completely safe assumptions for existing platforms. Any new platform with the same PWM IP will use this driver exclusively, without the clocksource driver and so without the problems you mentioned.

Hi Thierry,

Quoted 103 lines · On Wednesday 19 of June 2013 12:23:52 Tomasz Figa wrote:

Quoted 102 lines · On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:

Quoted 101 lines · Tomasz Figa

Quoted 59 lines · On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:

Quoted 2 lines · Tomasz Figa

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

[...]

Quoted 8 lines

Quoted 7 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part of
samsung_pwm_chip?

Quoted 9 lines

There are few registers shared with samsung_pwm_timer clocksource
driver and so normally the spinlock is exported from it. However on
on some platforms (namely Exynos >=4x12) kernel can be compiled without that driver, so the lock must be defined locally, just to
synchronize multiple PWM channels, as they share registers as well.

Okay, I think this needs further explanation. The clocksource driver
is
used for what exactly? From a quick look it seems to be very much PWM-
specific. According to the device tree binding for the PWM driver, the
timer blocks can also be used as clock sources and clock event timers.
So if I understand correctly you have setups where you use one or more
channels as clock source or clock event timer and one or more channels
as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize
accesses. You need to do much more than that. To properly split this
across several drivers there needs to be a mechanism to allocate channels for use either as clock source/event timer or PWM. Otherwise,
how do you know that drivers aren't stepping on each other's toes?

Quoted 12 lines

Quoted 9 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data

specified\n");

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+

sizeof(chip-

Quoted 33 lines

Quoted 15 lines

Quoted 14 lines

variant));

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

Obviously this needs some modification in order for the variant
to
become constant. But I think you can easily do so by making the
driver
match using the platform_driver's id_table field, similar to how
the
matching is done for OF.

Quoted 3 lines

Generally output_mask is board-dependent and is passed inside a variant
struct using platform_data pointer.

That's okay. But output_mask is the only thing that's board-dependent.
Everything else in the variant is SoC dependent judging by the OF device
table. So really only the output_mask should be part of the platform
data.

Quoted 5 lines

Same platform data is used in samsung_pwm_timer clocksource driver,
so
I just reused it here without adding the need to rename platform device at runtime (see arch/arm/plat-samsung/devs.c).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Looking a bit at git log for the clocksource driver, there's this

commit:
a3ce54f clocksource: samsung_pwm_timer: Do not request PWM

mem

Quoted 145 lines

Quoted 88 lines

Quoted 71 lines

region

Quoted 19 lines

That's an ugly workaround for sharing registers between two drivers.
There's a reason why drivers do request_mem_region(), and it is precisely to prevent them from accessing the same registers. As I already said above, I think you need to come up with some sort of API
to
share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm and
pwm-tiecap drivers, which use a shared block of registers. Initially
something similar was done as you do here, but eventually we came up
with a much better solution that involved introducing a new driver for
the shared functionality and an exported API.

Quoted 7 lines

The situation seems to be somewhat different here since you actually
share the same resources for different functionality instead of sharing
one subset of register across multiple drivers, but I think a similar
solution can be applied here.

Well, current design, or rather lack of thereof, has been established
after a significant amount of discussion, mostly with ARM SoC and MFD
maintainers.

It started from a simple reorganization of the clocksource driver to be
multiplatform friendly, without any means of synchronizing both drivers
other than local_irq_save() and _restore() that used to be there originally, before I started my work on this. This was enough to work
correctly, because both drivers at the same time are used only on uniprocessor systems.

Here's the thread with patches:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1
726
7

Then I opened a Pandora's Box by asking for opinion in this thread:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1
650
0

After that I started working on a complex framework for sharing this IP
block that would solve all the problems with synchronization, channel
allocation, device tree parsing, variant management, etc.

Here's an initial version, which started as an MFD master driver, which
would let 2 client drivers use the hardware (only the clocksource part
was implemented in that series):
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2
292
50

Quoted 6 lines

Still, that version didn't receive too good feedback, with comments pointing me to change the architecture to master-slave, with the clocksource driver being the master and PWM driver being the slave. And
so v5 was made:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

Yeah, this seemed to be going in the right direction.

Quoted 7 lines

This version was generally accepted, but then we discussed on IRC (mostly me and Arnd) whether such complex infrastructure is really needed and concluded that for this platform a simple shared spinlock would be enough, based on following reasons:
- on all SoCs on which the clocksource driver is going to be used there
is only one instance of the PWM block, with 5 channels

Quoted 3 lines

You say that now. But then at some point somebody decides that it might
be useful to have a second instance.

No, he won't.

Simply because no new SoC is going to be made which will use the samsung_pwm_timer driver and all existing ones have only a single instance of this IP.

Quoted 4 lines

Quoted 2 lines

- on all existing boards there are always at least two channels that don't have outputs

I don't see how that's relevant here.

This is the information passed through output_mask. Clocksource driver can use _only_ channels without output and PWM driver can use _only_ channels with output.

Both drivers receive the same output_mask value, either from the same variant structure or from device tree.

Quoted 7 lines

Quoted 6 lines

- operations on particular PWM channels must be synchronized anyway, because there are registers shared by all PWM channels (TCON being most
important where enable, autoreload and invert bits are located for all
channels).

Quoted 8 lines

And that's precisely why there should be a more explicit way of synchronizing than just acquiring a lock. Otherwise you have no possibility whatsoever to detect when a board tries to use both the clocksource and PWM drivers in incompatible ways. The lock may prevent concurrent accesses to the shared registers, but that won't help you if
the PWM driver suddenly decides to use an output that was originally meant to be used as clock source.

See above.

Quoted 16 lines

Quoted 9 lines

And so we got to current design which is basically a shared spinlock and
per-board output_mask, which specifies which channels have outputs on
particular boards. If you look to the clocksource driver, you can see
that it tries to allocate two output-less channels for timekeeping purposes and if it fails to do so, it simply panics.

Quoted 3 lines

IMHO this solution is fine, because it's simple, has little overhead and
it just works in case of platforms for which it is needed.

Okay, I like simple. But I see too much potential for this to break in the future. There are just too many assumptions for my taste. It isn't anything that I'm looking forward to have to maintain.

Quoted 3 lines

Those are completely safe assumptions for existing platforms. Any new platform with the same PWM IP will use this driver exclusively, without the clocksource driver and so without the problems you mentioned.

Would you mind taking into account things I pointed in reply above?

Quoted 9 lines · Tomasz Figa

Hi Thierry,

Quoted 6 lines · On Wednesday 19 of June 2013 12:23:52 Tomasz Figa wrote:

Quoted 5 lines · On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:

Quoted 4 lines · Tomasz Figa

Quoted 3 lines · On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:

Quoted 2 lines · Tomasz Figa

Quoted 1 line · On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa

Quoted 55 lines

Quoted 55 lines

Quoted 55 lines

Quoted 55 lines

Quoted 55 lines

[...]

Quoted 9 lines

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part
of
samsung_pwm_chip?

Quoted 13 lines

There are few registers shared with samsung_pwm_timer clocksource
driver and so normally the spinlock is exported from it. However
on
on some platforms (namely Exynos >=4x12) kernel can be compiled
without that driver, so the lock must be defined locally, just
to
synchronize multiple PWM channels, as they share registers as
well.

Okay, I think this needs further explanation. The clocksource driver
is
used for what exactly? From a quick look it seems to be very much
PWM-
specific. According to the device tree binding for the PWM driver,
the
timer blocks can also be used as clock sources and clock event timers.
So if I understand correctly you have setups where you use one or
more
channels as clock source or clock event timer and one or more channels
as PWM outputs.

In that case it's a very bad idea to use a global lock to synchronize
accesses. You need to do much more than that. To properly split
this
across several drivers there needs to be a mechanism to allocate
channels for use either as clock source/event timer or PWM. Otherwise,
how do you know that drivers aren't stepping on each other's toes?

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = 

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform