[PATCH v2 0/5] ARM: berlin: PWM support

[PATCH v2 0/5] ARM: berlin: PWM support

[Antoine Tenart]

Hi all,

This series adds a driver for the Marvell Berlin PWM controller, which
has 4 channels.

This has been tested on a BG2Q DMP.

Thanks,

Antoine

Changes since v1:
	- Added a sentinel in berlin_pwm_match[]

Antoine Tenart (5):
  pwm: add the Berlin pwm controller driver
  Documentation: bindings: document the Berlin PWM driver
  ARM: berlin: add a PWM node on the BG2Q
  ARM: berlin: add a PWM node on the BG2
  ARM: berlin: add a PWM node on the BG2CD

 .../devicetree/bindings/pwm/pwm-berlin.txt         |  15 ++
 arch/arm/boot/dts/berlin2.dtsi                     |   6 +
 arch/arm/boot/dts/berlin2cd.dtsi                   |   6 +
 arch/arm/boot/dts/berlin2q.dtsi                    |   6 +
 drivers/pwm/Kconfig                                |   9 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-berlin.c                           | 207 +++++++++++++++++++++
 7 files changed, 250 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-berlin.txt
 create mode 100644 drivers/pwm/pwm-berlin.c

-- 
2.5.0

[PATCH v2 1/5] pwm: add the Berlin pwm controller driver

[Antoine Tenart]

Add a PWM controller driver for the Marvell Berlin SoCs. This PWM
controller has 4 channels.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pwm/Kconfig      |   9 +++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-berlin.c | 207 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 217 insertions(+)
 create mode 100644 drivers/pwm/pwm-berlin.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f40fd8d..1773da8145b8 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -92,6 +92,15 @@ config PWM_BCM2835
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-bcm2835.
 
+config PWM_BERLIN
+	tristate "Berlin PWM support"
+	depends on ARCH_BERLIN
+	help
+	  PWM framework driver for Berlin.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-berlin.
+
 config PWM_BFIN
 	tristate "Blackfin PWM support"
 	depends on BFIN_GPTIMERS
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5b5a8f..670c5fce8bbb 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
+obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
new file mode 100644
index 000000000000..786990c1c750
--- /dev/null
+++ b/drivers/pwm/pwm-berlin.c
@@ -0,0 +1,207 @@
+/*
+ * Marvell Berlin PWM driver
+ *
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ *
+ * Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/spinlock.h>
+
+#define BERLIN_PWM_EN			0x0
+#define BERLIN_PWM_CONTROL		0x4
+#define BERLIN_PWM_DUTY			0x8
+#define BERLIN_PWM_TCNT			0xc
+
+#define BERLIN_PWM_CLK_RATE		100000000
+#define BERLIN_PWM_PRESCALE_MASK	0x7
+#define BERLIN_PWM_PRESCALE_MAX		4096
+
+struct berlin_pwm_chip {
+	struct pwm_chip chip;
+	void __iomem *base;
+	spinlock_t lock;
+};
+
+#define to_berlin_pwm_chip(chip)	\
+	container_of((chip), struct berlin_pwm_chip, chip)
+
+#define berlin_pwm_readl(chip, channel, offset)		\
+	readl((chip)->base + (channel) * 0x10 + offset)
+#define berlin_pwm_writel(val, chip, channel, offset)	\
+	writel(val, (chip)->base + (channel) * 0x10 + offset)
+
+static const u32 prescaler_table[] = {
+	1,
+	4,
+	8,
+	16,
+	64,
+	256,
+	1024,
+	4096,
+};
+
+static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			     int duty_ns, int period_ns)
+{
+	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
+	int prescale;
+	u32 val, duty, period;
+	u64 tmp;
+
+	for (prescale = 0; prescale < ARRAY_SIZE(prescaler_table); prescale++) {
+		tmp = BERLIN_PWM_CLK_RATE;
+		do_div(tmp, prescaler_table[prescale]);
+		tmp *= period_ns;
+		do_div(tmp, NSEC_PER_SEC);
+
+		if (tmp - 1 <= BERLIN_PWM_PRESCALE_MAX)
+			break;
+	}
+
+	if (tmp - 1 > BERLIN_PWM_PRESCALE_MAX)
+		return -EINVAL;
+
+	period = tmp;
+	tmp *= duty_ns;
+	do_div(tmp, period_ns);
+	duty = tmp;
+
+	spin_lock(&berlin_chip->lock);
+
+	val = berlin_pwm_readl(berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
+	val &= ~BERLIN_PWM_PRESCALE_MASK;
+	val |= prescale;
+	berlin_pwm_writel(val, berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
+
+	berlin_pwm_writel(duty, berlin_chip, pwm->hwpwm, BERLIN_PWM_DUTY);
+	berlin_pwm_writel(period, berlin_chip, pwm->hwpwm, BERLIN_PWM_TCNT);
+
+	spin_unlock(&berlin_chip->lock);
+
+	return 0;
+}
+
+static int berlin_pwm_set_polarity(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   enum pwm_polarity polarity)
+{
+	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
+	u32 val;
+
+	spin_lock(&berlin_chip->lock);
+
+	val = berlin_pwm_readl(berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		val &= ~BIT(3);
+	else
+		val |= BIT(3);
+
+	berlin_pwm_writel(val, berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
+
+	spin_unlock(&berlin_chip->lock);
+
+	return 0;
+}
+
+static int berlin_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
+
+	spin_lock(&berlin_chip->lock);
+	berlin_pwm_writel(0x1, berlin_chip, pwm->hwpwm, BERLIN_PWM_EN);
+	spin_unlock(&berlin_chip->lock);
+
+	return 0;
+}
+
+static void berlin_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
+
+	spin_lock(&berlin_chip->lock);
+	berlin_pwm_writel(0x0, berlin_chip, pwm->hwpwm, BERLIN_PWM_EN);
+	spin_unlock(&berlin_chip->lock);
+}
+
+static const struct pwm_ops berlin_pwm_ops = {
+	.config = berlin_pwm_config,
+	.set_polarity = berlin_pwm_set_polarity,
+	.enable = berlin_pwm_enable,
+	.disable = berlin_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static const struct of_device_id berlin_pwm_match[] = {
+	{ .compatible = "marvell,berlin-pwm" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, berlin_pwm_match);
+
+static int berlin_pwm_probe(struct platform_device *pdev)
+{
+	struct berlin_pwm_chip *pwm;
+	struct resource *res;
+	int ret;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pwm->base))
+		return PTR_ERR(pwm->base);
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &berlin_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = 4;
+	pwm->chip.can_sleep = true;
+	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+	pwm->chip.of_pwm_n_cells = 3;
+
+	spin_lock_init(&pwm->lock);
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to add PWM chip: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int berlin_pwm_remove(struct platform_device *pdev)
+{
+	struct berlin_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&pwm->chip);
+}
+
+static struct platform_driver berlin_pwm_driver = {
+	.probe	= berlin_pwm_probe,
+	.remove	= berlin_pwm_remove,
+	.driver	= {
+		.name	= "berlin-pwm",
+		.of_match_table = berlin_pwm_match,
+	},
+};
+module_platform_driver(berlin_pwm_driver);
+
+MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Berlin PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

[PATCH v2 2/5] Documentation: bindings: document the Berlin PWM driver

[Antoine Tenart]

Following the addition of a Berlin PWM driver, this patch adds the
corresponding documentation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 Documentation/devicetree/bindings/pwm/pwm-berlin.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-berlin.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-berlin.txt b/Documentation/devicetree/bindings/pwm/pwm-berlin.txt
new file mode 100644
index 000000000000..3d7ab7bfbf54
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-berlin.txt
@@ -0,0 +1,15 @@
+Berlin PWM controller
+
+Required properties:
+- compatible: should be "marvell,berlin-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: should be 3. See pwm.txt in this directory for a description of
+  the cells format.
+
+Example:
+
+pwm: pwm@f7f20000 {
+	compatible = "marvell,berlin-pwm";
+	reg = <0xf7f20000 0x40>;
+	#pwm-cells = <3>;
+}
-- 
2.5.0

[PATCH v2 3/5] ARM: berlin: add a PWM node on the BG2Q

[Antoine Tenart]

This patch adds a PWM node in the Berlin BG2Q device tree, using the
newly added Berlin PWM driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index 63a48490e2f9..41491a6bfcd0 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -609,5 +609,11 @@
 				interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			};
 		};
+
+		pwm: pwm@f20000 {
+			compatible = "marvell,berlin-pwm";
+			reg = <0xf20000 0x40>;
+			#pwm-cells = <3>;
+		};
 	};
 };
-- 
2.5.0

[PATCH v2 4/5] ARM: berlin: add a PWM node on the BG2

[Antoine Tenart]

This patch adds a PWM node in the Berlin BG2 device tree, using the
newly added Berlin PWM driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
index ef811de09908..e17bd5faed27 100644
--- a/arch/arm/boot/dts/berlin2.dtsi
+++ b/arch/arm/boot/dts/berlin2.dtsi
@@ -512,5 +512,11 @@
 				interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			};
 		};
+
+		pwm: pwm@f20000 {
+			compatible = "marvell,berlin-pwm";
+			reg = <0xf20000 0x40>;
+			#pwm-cells = <3>;
+		};
 	};
 };
-- 
2.5.0

[PATCH v2 5/5] ARM: berlin: add a PWM node on the BG2CD

[Antoine Tenart]

This patch adds a PWM node in the Berlin BG2CD device tree, using the
newly added Berlin PWM driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2cd.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2cd.dtsi b/arch/arm/boot/dts/berlin2cd.dtsi
index 900213d78a32..0503ca095acb 100644
--- a/arch/arm/boot/dts/berlin2cd.dtsi
+++ b/arch/arm/boot/dts/berlin2cd.dtsi
@@ -446,5 +446,11 @@
 				interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			};
 		};
+
+		pwm: pwm@f20000 {
+			compatible = "marvell,berlin-pwm";
+			reg = <0xf20000 0x40>;
+			#pwm-cells = <3>;
+		};
 	};
 };
-- 
2.5.0

Re: [PATCH v2 1/5] pwm: add the Berlin pwm controller driver

[Jisheng Zhang]

Hi,

On Thu, 30 Jul 2015 11:23:17 +0200
Antoine Tenart <antoine.tenart@free-electrons.com> wrote:

> Add a PWM controller driver for the Marvell Berlin SoCs. This PWM
> controller has 4 channels.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/pwm/Kconfig      |   9 +++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-berlin.c | 207 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 217 insertions(+)
>  create mode 100644 drivers/pwm/pwm-berlin.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f40fd8d..1773da8145b8 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -92,6 +92,15 @@ config PWM_BCM2835
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-bcm2835.
>  
> +config PWM_BERLIN
> +	tristate "Berlin PWM support"
> +	depends on ARCH_BERLIN
> +	help
> +	  PWM framework driver for Berlin.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-berlin.
> +
>  config PWM_BFIN
>  	tristate "Blackfin PWM support"
>  	depends on BFIN_GPTIMERS
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5b5a8f..670c5fce8bbb 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
>  obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
>  obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
>  obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
> +obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
>  obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
>  obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
> diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
> new file mode 100644
> index 000000000000..786990c1c750
> --- /dev/null
> +++ b/drivers/pwm/pwm-berlin.c
> @@ -0,0 +1,207 @@
> +/*
> + * Marvell Berlin PWM driver
> + *
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + *
> + * Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/spinlock.h>
> +
> +#define BERLIN_PWM_EN			0x0
> +#define BERLIN_PWM_CONTROL		0x4
> +#define BERLIN_PWM_DUTY			0x8
> +#define BERLIN_PWM_TCNT			0xc
> +
> +#define BERLIN_PWM_CLK_RATE		100000000

All PWMs in SoC use cfgclk as clock source, this is missed in BSP we provided
to you. So We could get clk rate rather than hardcoding as this. And we need to
clk_prepare_enable cfgclk in proper point.

> +#define BERLIN_PWM_PRESCALE_MASK	0x7
> +#define BERLIN_PWM_PRESCALE_MAX		4096
> +
> +struct berlin_pwm_chip {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +	spinlock_t lock;
> +};
> +
> +#define to_berlin_pwm_chip(chip)	\
> +	container_of((chip), struct berlin_pwm_chip, chip)
> +
> +#define berlin_pwm_readl(chip, channel, offset)		\
> +	readl((chip)->base + (channel) * 0x10 + offset)

could we use relaxed version?

> +#define berlin_pwm_writel(val, chip, channel, offset)	\
> +	writel(val, (chip)->base + (channel) * 0x10 + offset)

ditto

> +
> +static const u32 prescaler_table[] = {
> +	1,
> +	4,
> +	8,
> +	16,
> +	64,
> +	256,
> +	1024,
> +	4096,
> +};
> +
> +static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     int duty_ns, int period_ns)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +	int prescale;
> +	u32 val, duty, period;
> +	u64 tmp;
> +
> +	for (prescale = 0; prescale < ARRAY_SIZE(prescaler_table); prescale++) {
> +		tmp = BERLIN_PWM_CLK_RATE;
> +		do_div(tmp, prescaler_table[prescale]);
> +		tmp *= period_ns;
> +		do_div(tmp, NSEC_PER_SEC);
> +
> +		if (tmp - 1 <= BERLIN_PWM_PRESCALE_MAX)
> +			break;
> +	}
> +
> +	if (tmp - 1 > BERLIN_PWM_PRESCALE_MAX)
> +		return -EINVAL;
> +
> +	period = tmp;
> +	tmp *= duty_ns;
> +	do_div(tmp, period_ns);
> +	duty = tmp;
> +
> +	spin_lock(&berlin_chip->lock);

Any reason to take spinlock?

> +
> +	val = berlin_pwm_readl(berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +	val &= ~BERLIN_PWM_PRESCALE_MASK;
> +	val |= prescale;
> +	berlin_pwm_writel(val, berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +
> +	berlin_pwm_writel(duty, berlin_chip, pwm->hwpwm, BERLIN_PWM_DUTY);
> +	berlin_pwm_writel(period, berlin_chip, pwm->hwpwm, BERLIN_PWM_TCNT);
> +
> +	spin_unlock(&berlin_chip->lock);
> +
> +	return 0;
> +}
> +
> +static int berlin_pwm_set_polarity(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   enum pwm_polarity polarity)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +	u32 val;
> +
> +	spin_lock(&berlin_chip->lock);
> +
> +	val = berlin_pwm_readl(berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		val &= ~BIT(3);
> +	else
> +		val |= BIT(3);
> +
> +	berlin_pwm_writel(val, berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +
> +	spin_unlock(&berlin_chip->lock);
> +
> +	return 0;
> +}
> +
> +static int berlin_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +
> +	spin_lock(&berlin_chip->lock);
> +	berlin_pwm_writel(0x1, berlin_chip, pwm->hwpwm, BERLIN_PWM_EN);
> +	spin_unlock(&berlin_chip->lock);
> +
> +	return 0;
> +}
> +
> +static void berlin_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +
> +	spin_lock(&berlin_chip->lock);
> +	berlin_pwm_writel(0x0, berlin_chip, pwm->hwpwm, BERLIN_PWM_EN);
> +	spin_unlock(&berlin_chip->lock);
> +}
> +
> +static const struct pwm_ops berlin_pwm_ops = {
> +	.config = berlin_pwm_config,
> +	.set_polarity = berlin_pwm_set_polarity,
> +	.enable = berlin_pwm_enable,
> +	.disable = berlin_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id berlin_pwm_match[] = {
> +	{ .compatible = "marvell,berlin-pwm" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, berlin_pwm_match);
> +
> +static int berlin_pwm_probe(struct platform_device *pdev)
> +{
> +	struct berlin_pwm_chip *pwm;
> +	struct resource *res;
> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pwm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pwm->base))
> +		return PTR_ERR(pwm->base);
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &berlin_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = 4;
> +	pwm->chip.can_sleep = true;
> +	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	pwm->chip.of_pwm_n_cells = 3;
> +
> +	spin_lock_init(&pwm->lock);
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to add PWM chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	return 0;
> +}
> +
> +static int berlin_pwm_remove(struct platform_device *pdev)
> +{
> +	struct berlin_pwm_chip *pwm = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&pwm->chip);
> +}
> +
> +static struct platform_driver berlin_pwm_driver = {
> +	.probe	= berlin_pwm_probe,
> +	.remove	= berlin_pwm_remove,
> +	.driver	= {
> +		.name	= "berlin-pwm",
> +		.of_match_table = berlin_pwm_match,
> +	},
> +};
> +module_platform_driver(berlin_pwm_driver);
> +
> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
> +MODULE_DESCRIPTION("Marvell Berlin PWM driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/5] pwm: add the Berlin pwm controller driver

[Sebastian Hesselbarth]

On 30.07.2015 11:23, Antoine Tenart wrote:
> Add a PWM controller driver for the Marvell Berlin SoCs. This PWM
> controller has 4 channels.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Antoine,

thanks for providing these patches, some comments below.

> ---
>   drivers/pwm/Kconfig      |   9 +++
>   drivers/pwm/Makefile     |   1 +
>   drivers/pwm/pwm-berlin.c | 207 +++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 217 insertions(+)
>   create mode 100644 drivers/pwm/pwm-berlin.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f40fd8d..1773da8145b8 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -92,6 +92,15 @@ config PWM_BCM2835
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called pwm-bcm2835.
>
> +config PWM_BERLIN
> +	tristate "Berlin PWM support"
> +	depends on ARCH_BERLIN
> +	help
> +	  PWM framework driver for Berlin.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-berlin.
> +
>   config PWM_BFIN
>   	tristate "Blackfin PWM support"
>   	depends on BFIN_GPTIMERS
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index ec50eb5b5a8f..670c5fce8bbb 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
>   obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
>   obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
>   obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
> +obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
>   obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
>   obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>   obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
> diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
> new file mode 100644
> index 000000000000..786990c1c750
> --- /dev/null
> +++ b/drivers/pwm/pwm-berlin.c
> @@ -0,0 +1,207 @@
> +/*
> + * Marvell Berlin PWM driver
> + *
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + *
> + * Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/spinlock.h>
> +
> +#define BERLIN_PWM_EN			0x0
> +#define BERLIN_PWM_CONTROL		0x4
> +#define BERLIN_PWM_DUTY			0x8
> +#define BERLIN_PWM_TCNT			0xc
> +
> +#define BERLIN_PWM_CLK_RATE		100000000
> +#define BERLIN_PWM_PRESCALE_MASK	0x7
> +#define BERLIN_PWM_PRESCALE_MAX		4096

As Jisheng already pointed out: please use a DT provided
input clock to the PWM driver.

> +
> +struct berlin_pwm_chip {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +	spinlock_t lock;
> +};
> +
> +#define to_berlin_pwm_chip(chip)	\
> +	container_of((chip), struct berlin_pwm_chip, chip)
> +
> +#define berlin_pwm_readl(chip, channel, offset)		\
> +	readl((chip)->base + (channel) * 0x10 + offset)
> +#define berlin_pwm_writel(val, chip, channel, offset)	\
> +	writel(val, (chip)->base + (channel) * 0x10 + offset)

In contrast to Jisheng's comment about _relaxed() versions of
readl/writel, I don't really care much. Writing the PWM registers
is everything but timing critical, so I'd also accept the non-relaxed
versions here.

> +static const u32 prescaler_table[] = {
> +	1,
> +	4,
> +	8,
> +	16,
> +	64,
> +	256,
> +	1024,
> +	4096,
> +};
> +
> +static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     int duty_ns, int period_ns)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +	int prescale;
> +	u32 val, duty, period;
> +	u64 tmp;
> +
> +	for (prescale = 0; prescale < ARRAY_SIZE(prescaler_table); prescale++) {
> +		tmp = BERLIN_PWM_CLK_RATE;
> +		do_div(tmp, prescaler_table[prescale]);
> +		tmp *= period_ns;
> +		do_div(tmp, NSEC_PER_SEC);
> +
> +		if (tmp - 1 <= BERLIN_PWM_PRESCALE_MAX)
> +			break;
> +	}
> +
> +	if (tmp - 1 > BERLIN_PWM_PRESCALE_MAX)
> +		return -EINVAL;

Something about the </> BERLIN_PWM_PRESCALE_MAX above looks odd to me.
As soon as you multiply with period_ns you cannot compare against
BERLIN_PWM_PRESCALE_MAX anymore as you ignore that there also is a
TCNT register.

How about you calculate _absolute_ (i.e. w/ prescaler and tcnt) min
and max frequencies on probe or here in _config and bail out early
if requested rate is beyond that range. Then you can find the best
prescaler/tcnt combination with (open-coded):

static const u32 prescaler_diff_table[] = {
/*	1, 4, 8, 16, 64, 256, 1024, 4096 */
	1, 4, 2,  2,  4,   4,    4,    4,
};


u64 cycles;

// assume period_ns = 2500000 (2.5ms for 400Hz)
cycles = clk_get_rate(clk);   // cycles = 100000000 (100MHz)
cycles *= ns_period;          // cycles = 100000000 * 2500000
                               //        = 250 * 10^12
do_div(cycles, NSEC_PER_SEC)  // cycles = 250000

prescale = 0;
while (cycles > BERLIN_PWM_MAX_TCNT)
	do_div(cycles, prescaler_diff_table[++prescale]);

And so on...

Also, it would be nice to have the PWM output frequency
function written down in a comment, e.g.

/*
  * fOUT = fREF / PRESCALE / (TCNT+1)
  * or
  * TCNT = (fOUT * PRESCALE / fREF) - 1
  * and
  * DUTY = duty * TCNT
  */

or something more appropriate.

> +	period = tmp;
> +	tmp *= duty_ns;
> +	do_div(tmp, period_ns);
> +	duty = tmp;
> +
> +	spin_lock(&berlin_chip->lock);

AFAIKS, Jisheng is right about the spin_lock - if the pwm registers
are separate for the different channels you shouldn't need any spinlock
at all - BUT from Documentation/pwm.txt, I read:

"""
Currently the PWM core does not enforce any locking to pwm_enable(),
pwm_disable() and pwm_config(), so the calling context is currently
driver specific. This is an issue derived from the former barebone API
and should be fixed soon.
"""

So, as long as this is not fixed, I suggest to keep the spinlocks - or
wait for Thierry to rule about it.

> +	val = berlin_pwm_readl(berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +	val &= ~BERLIN_PWM_PRESCALE_MASK;
> +	val |= prescale;
> +	berlin_pwm_writel(val, berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +
> +	berlin_pwm_writel(duty, berlin_chip, pwm->hwpwm, BERLIN_PWM_DUTY);
> +	berlin_pwm_writel(period, berlin_chip, pwm->hwpwm, BERLIN_PWM_TCNT);

For the above calculations to make sense, you'll need the max allowed
TCNT/DUTY values. If you cannot derive them from any DS/BSP, you can try
to writel((u32)-1, TCNT) and read-back the result. Usually that should
give you the max allowed value for that register.

> +	spin_unlock(&berlin_chip->lock);
> +
> +	return 0;
> +}
> +
> +static int berlin_pwm_set_polarity(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   enum pwm_polarity polarity)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +	u32 val;
> +
> +	spin_lock(&berlin_chip->lock);
> +
> +	val = berlin_pwm_readl(berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +
> +	if (polarity == PWM_POLARITY_NORMAL)
> +		val &= ~BIT(3);
> +	else
> +		val |= BIT(3);

Please use some meaningful name for BIT(3).

> +
> +	berlin_pwm_writel(val, berlin_chip, pwm->hwpwm, BERLIN_PWM_CONTROL);
> +
> +	spin_unlock(&berlin_chip->lock);
> +
> +	return 0;
> +}
> +
> +static int berlin_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +
> +	spin_lock(&berlin_chip->lock);
> +	berlin_pwm_writel(0x1, berlin_chip, pwm->hwpwm, BERLIN_PWM_EN);

ditto for 0x1 or BIT(0)

> +	spin_unlock(&berlin_chip->lock);
> +
> +	return 0;
> +}
> +
> +static void berlin_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct berlin_pwm_chip *berlin_chip = to_berlin_pwm_chip(chip);
> +
> +	spin_lock(&berlin_chip->lock);
> +	berlin_pwm_writel(0x0, berlin_chip, pwm->hwpwm, BERLIN_PWM_EN);

ditto.

The rest looks fine to me.

Sebastian

> +	spin_unlock(&berlin_chip->lock);
> +}
> +
> +static const struct pwm_ops berlin_pwm_ops = {
> +	.config = berlin_pwm_config,
> +	.set_polarity = berlin_pwm_set_polarity,
> +	.enable = berlin_pwm_enable,
> +	.disable = berlin_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id berlin_pwm_match[] = {
> +	{ .compatible = "marvell,berlin-pwm" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, berlin_pwm_match);
> +
> +static int berlin_pwm_probe(struct platform_device *pdev)
> +{
> +	struct berlin_pwm_chip *pwm;
> +	struct resource *res;
> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pwm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pwm->base))
> +		return PTR_ERR(pwm->base);
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &berlin_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = 4;
> +	pwm->chip.can_sleep = true;
> +	pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	pwm->chip.of_pwm_n_cells = 3;
> +
> +	spin_lock_init(&pwm->lock);
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to add PWM chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	return 0;
> +}
> +
> +static int berlin_pwm_remove(struct platform_device *pdev)
> +{
> +	struct berlin_pwm_chip *pwm = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&pwm->chip);
> +}
> +
> +static struct platform_driver berlin_pwm_driver = {
> +	.probe	= berlin_pwm_probe,
> +	.remove	= berlin_pwm_remove,
> +	.driver	= {
> +		.name	= "berlin-pwm",
> +		.of_match_table = berlin_pwm_match,
> +	},
> +};
> +module_platform_driver(berlin_pwm_driver);
> +
> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
> +MODULE_DESCRIPTION("Marvell Berlin PWM driver");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH v2 2/5] Documentation: bindings: document the Berlin PWM driver

[Sebastian Hesselbarth]

On 30.07.2015 11:23, Antoine Tenart wrote:
> Following the addition of a Berlin PWM driver, this patch adds the
> corresponding documentation.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>   Documentation/devicetree/bindings/pwm/pwm-berlin.txt | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pwm/pwm-berlin.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-berlin.txt b/Documentation/devicetree/bindings/pwm/pwm-berlin.txt
> new file mode 100644
> index 000000000000..3d7ab7bfbf54
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-berlin.txt
> @@ -0,0 +1,15 @@
> +Berlin PWM controller

Just a little sentence about "PWM IP found in Marvell Berlin SoCs" ?
Besides that, this looks pretty straight-forward to me. Feel free
to add my

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

for the Berlin part of it.

Sebastian

> +Required properties:
> +- compatible: should be "marvell,berlin-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: should be 3. See pwm.txt in this directory for a description of
> +  the cells format.
> +
> +Example:
> +
> +pwm: pwm@f7f20000 {
> +	compatible = "marvell,berlin-pwm";
> +	reg = <0xf7f20000 0x40>;
> +	#pwm-cells = <3>;
> +}
>

Re: [PATCH v2 4/5] ARM: berlin: add a PWM node on the BG2

[Sebastian Hesselbarth]

On 30.07.2015 11:23, Antoine Tenart wrote:
> This patch adds a PWM node in the Berlin BG2 device tree, using the
> newly added Berlin PWM driver.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>   arch/arm/boot/dts/berlin2.dtsi | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
> index ef811de09908..e17bd5faed27 100644
> --- a/arch/arm/boot/dts/berlin2.dtsi
> +++ b/arch/arm/boot/dts/berlin2.dtsi
> @@ -512,5 +512,11 @@
>   				interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
>   			};
>   		};
> +
> +		pwm: pwm@f20000 {
> +			compatible = "marvell,berlin-pwm";
> +			reg = <0xf20000 0x40>;
> +			#pwm-cells = <3>;
> +		};

I only checked berlin2.dtsi:
The top most line with <GIC_SPI 15 ...> belongs to a sub-node of
apb@fc0000 - which means that the pwm node isn't sorted in by
address. Please sort the nodes by address in all three DT patches.

BTW, is there any IRQ from the PWM IP routed to any intc upstream?
A quick look into some BSP code does not reveal any hints maybe
one of the Marvell guys can comment on it.

Sebastian

>   	};
>   };
>

Re: [PATCH v2 4/5] ARM: berlin: add a PWM node on the BG2

[Jisheng Zhang]

Dear Sebastian,

On Mon, 10 Aug 2015 21:51:44 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 30.07.2015 11:23, Antoine Tenart wrote:
> > This patch adds a PWM node in the Berlin BG2 device tree, using the
> > newly added Berlin PWM driver.
> >
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> >   arch/arm/boot/dts/berlin2.dtsi | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
> > index ef811de09908..e17bd5faed27 100644
> > --- a/arch/arm/boot/dts/berlin2.dtsi
> > +++ b/arch/arm/boot/dts/berlin2.dtsi
> > @@ -512,5 +512,11 @@
> >   				interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> >   			};
> >   		};
> > +
> > +		pwm: pwm@f20000 {
> > +			compatible = "marvell,berlin-pwm";
> > +			reg = <0xf20000 0x40>;
> > +			#pwm-cells = <3>;
> > +		};
> 
> I only checked berlin2.dtsi:
> The top most line with <GIC_SPI 15 ...> belongs to a sub-node of
> apb@fc0000 - which means that the pwm node isn't sorted in by
> address. Please sort the nodes by address in all three DT patches.
> 
> BTW, is there any IRQ from the PWM IP routed to any intc upstream?
> A quick look into some BSP code does not reveal any hints maybe
> one of the Marvell guys can comment on it.

There's no interrupt for the PWM in all berlin SoCs ;)

Thanks,
Jisheng