[PATCH 0/2] pwm: Add support for R-Car PWM Timer

[PATCH 0/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

R-Car SoCs have a seven-channel pulse width modulation (PWM) timer.
This driver adds support for the PWM Timer as a single PWM chip and
seven PWM devices.

This patch set is based on the linux-pwm.git / for-next branch.
(commit id = efb0de55b6a2ec15fc424e660601f22ae2fa487a)

Yoshihiro Shimoda (2):
  pwm: Add device tree binding document for R-Car PWM Timer
  pwm: Add support for R-Car PWM Timer

 .../devicetree/bindings/pwm/renesas,pwm-rcar.txt   |  21 ++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-rcar.c                             | 282 +++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
 create mode 100644 drivers/pwm/pwm-rcar.c

-- 
1.9.1

[PATCH 1/2] pwm: Add device tree binding document for R-Car PWM Timer

[Yoshihiro Shimoda]

Add binding document for Renesas PWM Timer on R-Car SoCs.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 .../devicetree/bindings/pwm/renesas,pwm-rcar.txt    | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt

diff --git a/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
new file mode 100644
index 0000000..38123f9
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
@@ -0,0 +1,21 @@
+* Renesas R-Car PWM Timer Controller
+
+Required Properties:
+- compatible: must contain "renesas,pwm-rcar"
+- reg: base address and length of the registers block for the PWM
+- #pwm-cells: should be 7. See pwm.txt in this directory for a description of
+  the cells format.
+- clocks: clock phandle and specifier pair.
+- pinctrl-0: phandle, referring to a default pin configuration node.
+- pinctrl-names: Set to "default"
+
+Example: R8A7790 (R-Car H2) PWM Timer node
+
+	pwm: pwm@e6e30000 {
+		compatible = "renesas,pwm-rcar";
+		reg = <0 0xe6e30000 0 0x7000>;
+		#pwm-cells = <7>;
+		clocks = <&mstp5_clks R8A7790_CLK_PWM>;
+		pinctrl-0 = <&pwm3_pins>;
+		pinctrl-names = "default";
+	};
-- 
1.9.1

Re: [PATCH 1/2] pwm: Add device tree binding document for R-Car PWM Timer

[Geert Uytterhoeven]

Hi Shimoda-san,

On Wed, May 13, 2015 at 11:27 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Add binding document for Renesas PWM Timer on R-Car SoCs.

Thanks for your patch!

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  .../devicetree/bindings/pwm/renesas,pwm-rcar.txt    | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> new file mode 100644
> index 0000000..38123f9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> @@ -0,0 +1,21 @@
> +* Renesas R-Car PWM Timer Controller
> +
> +Required Properties:
> +- compatible: must contain "renesas,pwm-rcar"

Can you please add SoC-specific compatible values, too? That way we can handle
 SoC-specific differences if we ever encounter them.
If I'm not mistaken, this PWM IP core is present on both R-Car Gen1 and Gen2
SoCs?

> +- reg: base address and length of the registers block for the PWM
> +- #pwm-cells: should be 7. See pwm.txt in this directory for a description of
> +  the cells format.

Why 7? This is not the number of channels, cfr. pwm.txt you reference above:

    "pwm-specifier typically encodes the chip-relative PWM number and the PWM
     period in nanoseconds.

     Optionally, the pwm-specifier can encode a number of flags (defined in
     <dt-bindings/pwm/pwm.h>) in a third cell:
     - PWM_POLARITY_INVERTED: invert the PWM signal polarity"

So 2 or 3 makes more sense to me.

> +- clocks: clock phandle and specifier pair.
> +- pinctrl-0: phandle, referring to a default pin configuration node.
> +- pinctrl-names: Set to "default"
> +
> +Example: R8A7790 (R-Car H2) PWM Timer node
> +
> +       pwm: pwm@e6e30000 {
> +               compatible = "renesas,pwm-rcar";
> +               reg = <0 0xe6e30000 0 0x7000>;
> +               #pwm-cells = <7>;
> +               clocks = <&mstp5_clks R8A7790_CLK_PWM>;
> +               pinctrl-0 = <&pwm3_pins>;
> +               pinctrl-names = "default";
> +       };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 1/2] pwm: Add device tree binding document for R-Car PWM Timer

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for the review!

> Sent: Friday, May 15, 2015 12:48 AM
> 
> Hi Shimoda-san,
> 
> On Wed, May 13, 2015 at 11:27 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Add binding document for Renesas PWM Timer on R-Car SoCs.
> 
> Thanks for your patch!
> 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  .../devicetree/bindings/pwm/renesas,pwm-rcar.txt    | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> > new file mode 100644
> > index 0000000..38123f9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
> > @@ -0,0 +1,21 @@
> > +* Renesas R-Car PWM Timer Controller
> > +
> > +Required Properties:
> > +- compatible: must contain "renesas,pwm-rcar"
> 
> Can you please add SoC-specific compatible values, too? That way we can handle
>  SoC-specific differences if we ever encounter them.
> If I'm not mistaken, this PWM IP core is present on both R-Car Gen1 and Gen2
> SoCs?

According to the manuals of both R-Car Gen1 (H1, M1, E1) and Gen2,
this PWM IP core is present on them. Also this IP is present on R-Car next generation.

Of course, I can add SoC-specific compatible values, like "renesas,pwm-r7a7790" or
something. So, I will change the compatible values in v2 patch.

> > +- reg: base address and length of the registers block for the PWM
> > +- #pwm-cells: should be 7. See pwm.txt in this directory for a description of
> > +  the cells format.
> 
> Why 7? This is not the number of channels, cfr. pwm.txt you reference above:
> 
>     "pwm-specifier typically encodes the chip-relative PWM number and the PWM
>      period in nanoseconds.
> 
>      Optionally, the pwm-specifier can encode a number of flags (defined in
>      <dt-bindings/pwm/pwm.h>) in a third cell:
>      - PWM_POLARITY_INVERTED: invert the PWM signal polarity"
> 
> So 2 or 3 makes more sense to me.

Thank you very much for the point. I completely misunderstood this specification.
Since this PWM IP cannot control polarity, this value should be 2.
So, I will fix this in v2 patch.

Best regards,
Yoshihiro Shimoda

[PATCH 2/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

R-Car SoCs have a seven-channel pulse width modulation (PWM) timer.
This driver adds support for the PWM Timer as a single PWM chip and
seven PWM devices.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/Kconfig    |  11 ++
 drivers/pwm/Makefile   |   1 +
 drivers/pwm/pwm-rcar.c | 276 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/pwm/pwm-rcar.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..7e98175 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -249,6 +249,17 @@ config PWM_PXA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-pxa.
 
+config PWM_RCAR
+	tristate "Renesas R-Car PWM support"
+	depends on ARCH_SHMOBILE || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This driver exposes the PWM Timer controller found in Renesas
+	  chips through the PWM API.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-rcar.
+
 config PWM_RENESAS_TPU
 	tristate "Renesas TPU PWM support"
 	depends on ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5..79d3dc3 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
+obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
new file mode 100644
index 0000000..29011e8
--- /dev/null
+++ b/drivers/pwm/pwm-rcar.c
@@ -0,0 +1,276 @@
+/*
+ * R-Car PWM Timer driver
+ *
+ * Copyright (C) 2015 Renesas Electronics Corporation
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#define NUM_RCAR_PWM_CHANNELS	7
+#define RCAR_PWM_MAX_DIVISION	24
+#define RCAR_PWM_MAX_CYCLE	1023
+
+#define RCAR_PWMCR		0x00
+#define RCAR_PWMCNT		0x04
+#define RCAR_PWM_CH_OFFSET	0x1000
+
+#define RCAR_PWMCR_CC0_MASK	0x000f0000
+#define RCAR_PWMCR_CC0_SHIFT	16
+#define RCAR_PWMCR_CCMD		BIT(15)
+#define RCAR_PWMCR_SYNC		BIT(11)
+#define RCAR_PWMCR_SS0		BIT(4)
+#define RCAR_PWMCR_EN0		BIT(0)
+
+#define RCAR_PWMCNT_CYC0_MASK	0x03ff0000
+#define RCAR_PWMCNT_CYC0_SHIFT	16
+#define RCAR_PWMCNT_PH0_MASK	0x000003ff
+#define RCAR_PWMCNT_PH0_SHIFT	0
+
+struct rcar_pwm_chip {
+	struct platform_device *pdev;
+	struct pwm_chip chip;
+
+	void __iomem *base;
+	struct clk *clk;
+};
+
+#define to_rcar_pwm_chip(chip)	container_of(chip, struct rcar_pwm_chip, chip)
+
+static u32 rcar_pwm_get_reg_offset(unsigned int channel)
+{
+	return channel * RCAR_PWM_CH_OFFSET;
+}
+
+static void rcar_pwm_write(struct rcar_pwm_chip *rp, unsigned int channel,
+			   u32 data, u32 reg)
+{
+	iowrite32(data, rp->base + rcar_pwm_get_reg_offset(channel) + reg);
+}
+
+static u32 rcar_pwm_read(struct rcar_pwm_chip *rp, unsigned int channel,
+			 u32 reg)
+{
+	return ioread32(rp->base + rcar_pwm_get_reg_offset(channel) + reg);
+}
+
+static void rcar_pwm_bit_modify(struct rcar_pwm_chip *rp, unsigned int channel,
+				u32 mask, u32 data, u32 reg)
+{
+	u32 val = rcar_pwm_read(rp, channel, reg);
+
+	val &= ~mask;
+	val |= data & mask;
+	rcar_pwm_write(rp, channel, val, reg);
+}
+
+static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,
+				       int period_ns)
+{
+	int div;
+	unsigned long clk_rate = clk_get_rate(rp->clk);
+	unsigned long long max;	/* max cycle / nanoseconds */
+
+	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
+		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
+		do_div(max, clk_rate / (1 << div));
+		if (period_ns < max)
+			break;
+	}
+
+	return div;
+}
+
+static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
+				       int channel, int div)
+{
+	u32 val = rcar_pwm_read(rp, channel, RCAR_PWMCR);
+
+	if (div > RCAR_PWM_MAX_DIVISION)
+		return;
+
+	val &= ~(RCAR_PWMCR_CCMD | RCAR_PWMCR_CC0_MASK);
+	if (div & 1)
+		val |= RCAR_PWMCR_CCMD;
+	div >>= 1;
+	val |= div << RCAR_PWMCR_CC0_SHIFT;
+	rcar_pwm_write(rp, channel, val, RCAR_PWMCR);
+}
+
+static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int channel,
+				 int div, int duty_ns, int period_ns)
+{
+	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
+	unsigned long clk_rate = clk_get_rate(rp->clk);
+	u32 cyc, ph;
+
+	one_cycle = (unsigned long long)NSEC_PER_SEC * 100;
+	do_div(one_cycle, clk_rate / (1 << div));
+
+	tmp = period_ns * 100;
+	do_div(tmp, one_cycle);
+	cyc = ((u32)tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
+
+	tmp = duty_ns * 100;
+	do_div(tmp, one_cycle);
+	ph = (u32)tmp & RCAR_PWMCNT_PH0_MASK;
+
+	/* Avoid prohibited setting */
+	if (cyc && ph)
+		rcar_pwm_write(rp, channel, cyc | ph, RCAR_PWMCNT);
+}
+
+static int rcar_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+
+	return clk_prepare_enable(rp->clk);
+}
+
+static void rcar_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+
+	clk_disable_unprepare(rp->clk);
+}
+
+static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+	int div;
+
+	div = rcar_pwn_get_clock_division(rp, period_ns);
+
+	rcar_pwm_bit_modify(rp, pwm->hwpwm, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC,
+			    RCAR_PWMCR);
+	rcar_pwm_set_counter(rp, pwm->hwpwm, div, duty_ns, period_ns);
+	rcar_pwm_set_clock_control(rp, pwm->hwpwm, div);
+	rcar_pwm_bit_modify(rp, pwm->hwpwm, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
+
+	return 0;
+}
+
+static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+	u32 pwmcnt;
+
+	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
+	pwmcnt = rcar_pwm_read(rp, pwm->hwpwm, RCAR_PWMCNT);
+	if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
+	    !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
+		return -EINVAL;
+
+	rcar_pwm_bit_modify(rp, pwm->hwpwm, RCAR_PWMCR_EN0, RCAR_PWMCR_EN0,
+			    RCAR_PWMCR);
+
+	return 0;
+}
+
+static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+
+	rcar_pwm_bit_modify(rp, pwm->hwpwm, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
+}
+
+static const struct pwm_ops rcar_pwm_ops = {
+	.request	= rcar_pwm_request,
+	.free		= rcar_pwm_free,
+	.config		= rcar_pwm_config,
+	.enable		= rcar_pwm_enable,
+	.disable	= rcar_pwm_disable,
+	.owner		= THIS_MODULE,
+};
+
+static int rcar_pwm_probe(struct platform_device *pdev)
+{
+	struct rcar_pwm_chip *rcar_pwm;
+	struct resource *res;
+	int ret;
+
+	rcar_pwm = devm_kzalloc(&pdev->dev, sizeof(*rcar_pwm), GFP_KERNEL);
+	if (rcar_pwm == NULL)
+		return -ENOMEM;
+
+	rcar_pwm->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	rcar_pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rcar_pwm->base))
+		return PTR_ERR(rcar_pwm->base);
+
+	rcar_pwm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(rcar_pwm->clk)) {
+		dev_err(&pdev->dev, "cannot get clock\n");
+		return PTR_ERR(rcar_pwm->clk);
+	}
+
+	platform_set_drvdata(pdev, rcar_pwm);
+
+	rcar_pwm->chip.dev = &pdev->dev;
+	rcar_pwm->chip.ops = &rcar_pwm_ops;
+	rcar_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+	rcar_pwm->chip.base = -1;
+	rcar_pwm->chip.npwm = NUM_RCAR_PWM_CHANNELS;
+
+	ret = pwmchip_add(&rcar_pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register PWM chip\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "R-Car PWM Timer registered\n");
+
+	pm_runtime_enable(&pdev->dev);
+
+	return 0;
+}
+
+static int rcar_pwm_remove(struct platform_device *pdev)
+{
+	struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&rcar_pwm->chip);
+	if (ret)
+		return ret;
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id rcar_pwm_of_table[] = {
+	{ .compatible = "renesas,pwm-rcar", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, rcar_pwm_of_table);
+
+static struct platform_driver rcar_pwm_driver = {
+	.probe		= rcar_pwm_probe,
+	.remove		= rcar_pwm_remove,
+	.driver		= {
+		.name	= "pwm-rcar",
+		.of_match_table = of_match_ptr(rcar_pwm_of_table),
+	}
+};
+module_platform_driver(rcar_pwm_driver);
+
+MODULE_AUTHOR("Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>");
+MODULE_DESCRIPTION("Renesas PWM Timer Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:pwm-rcar");
-- 
1.9.1

Re: [PATCH 2/2] pwm: Add support for R-Car PWM Timer

[Geert Uytterhoeven]

Hi Shimoda-san,

On Wed, May 13, 2015 at 11:27 AM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> R-Car SoCs have a seven-channel pulse width modulation (PWM) timer.
> This driver adds support for the PWM Timer as a single PWM chip and
> seven PWM devices.

Thanks for your patch!

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index b1541f4..7e98175 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -249,6 +249,17 @@ config PWM_PXA
>           To compile this driver as a module, choose M here: the module
>           will be called pwm-pxa.
>
> +config PWM_RCAR
> +       tristate "Renesas R-Car PWM support"
> +       depends on ARCH_SHMOBILE || COMPILE_TEST

Could be instead

depends on ARCH_RCAR_GEN1 ||  ARCH_RCAR_GEN2 || COMPILE_TEST

> +       depends on HAS_IOMEM
> +       help
> +         This driver exposes the PWM Timer controller found in Renesas
> +         chips through the PWM API.

"Renesas R-Car chips" (e.g. R-Mobile uses pwm-renesas-tpu).

> --- /dev/null
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -0,0 +1,276 @@
> +/*
> + * R-Car PWM Timer driver
> + *
> + * Copyright (C) 2015 Renesas Electronics Corporation
> + *
> + * This is free software; you can redistribute it and/or modify
> + * it under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +
> +#define NUM_RCAR_PWM_CHANNELS  7

I'm wondering whether this should be a DT property, to prepare for
future expansion.
Alternatively, as these are really 7 individual channels, with 7 individual
register blocks spaced 0x1000 apart, you could have 7 individual device nodes,
each driving one pwm output. That would probably incur more overhead, as
there would be 7 individual pwm controllers. But it does allow for future
expansion, and allows to cope when the individual register blocks are moved
in the SoC address space (I can imagine that on old SoCs e.g. all SCIF
blocks were nicely lay out in the address space, while now they have arbitrary
base addresses).

What do other people think?

> +static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,
> +                                      int period_ns)
> +{
> +       int div;
> +       unsigned long clk_rate = clk_get_rate(rp->clk);
> +       unsigned long long max; /* max cycle / nanoseconds */
> +
> +       for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> +               max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> +               do_div(max, clk_rate / (1 << div));

Dividing clk_rate by "1 << div" reduces accuracy a lot for large values of div.
"NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE * (1 << div)" just fits in 64-bit,
so you can use

        max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
              (1 << div);
        do_div(max, clk_rate);

> +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int channel,
> +                                int div, int duty_ns, int period_ns)
> +{
> +       unsigned long long one_cycle, tmp;      /* 0.01 nanoseconds */
> +       unsigned long clk_rate = clk_get_rate(rp->clk);
> +       u32 cyc, ph;
> +
> +       one_cycle = (unsigned long long)NSEC_PER_SEC * 100;
> +       do_div(one_cycle, clk_rate / (1 << div));

Same here:

        one_cycle = (unsigned long long)NSEC_PER_SEC * 100 * (1 << div)
        do_div(one_cycle, clk_rate);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 2/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for your review!

> Sent: Friday, May 15, 2015 1:07 AM
> 
> Hi Shimoda-san,
> 
> On Wed, May 13, 2015 at 11:27 AM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > R-Car SoCs have a seven-channel pulse width modulation (PWM) timer.
> > This driver adds support for the PWM Timer as a single PWM chip and
> > seven PWM devices.
> 
> Thanks for your patch!
> 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index b1541f4..7e98175 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -249,6 +249,17 @@ config PWM_PXA
> >           To compile this driver as a module, choose M here: the module
> >           will be called pwm-pxa.
> >
> > +config PWM_RCAR
> > +       tristate "Renesas R-Car PWM support"
> > +       depends on ARCH_SHMOBILE || COMPILE_TEST
> 
> Could be instead
> 
> depends on ARCH_RCAR_GEN1 ||  ARCH_RCAR_GEN2 || COMPILE_TEST

I will fix this.

> > +       depends on HAS_IOMEM
> > +       help
> > +         This driver exposes the PWM Timer controller found in Renesas
> > +         chips through the PWM API.
> 
> "Renesas R-Car chips" (e.g. R-Mobile uses pwm-renesas-tpu).

I will fix this.

> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rcar.c
< snip >
> > +#define NUM_RCAR_PWM_CHANNELS  7
> 
> I'm wondering whether this should be a DT property, to prepare for
> future expansion.
> Alternatively, as these are really 7 individual channels, with 7 individual
> register blocks spaced 0x1000 apart, you could have 7 individual device nodes,
> each driving one pwm output. That would probably incur more overhead, as
> there would be 7 individual pwm controllers. But it does allow for future
> expansion, and allows to cope when the individual register blocks are moved
> in the SoC address space (I can imagine that on old SoCs e.g. all SCIF
> blocks were nicely lay out in the address space, while now they have arbitrary
> base addresses).
> 
> What do other people think?

My initial development, I wrote 7 individual device nodes. However, after the driver
was proved, the sysfs for pwm is:
 /sys/class/pwm/pwmchip0/pwm0
 /sys/class/pwm/pwmchip1/pwm0
 ...
 /sys/class/pwm/pwmchip6/pwm0

I felt this was strange numbering a little.
So, I changed the style to one pwm chip and 7 pwm channels:
 /sys/class/pwm/pwmchip0/pwm0
 /sys/class/pwm/pwmchip0/pwm1
 ...
 /sys/class/pwm/pwmchip0/pwm6

I'm not sure which style is good for the PWM IP.

> > +static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,
> > +                                      int period_ns)
> > +{
> > +       int div;
> > +       unsigned long clk_rate = clk_get_rate(rp->clk);
> > +       unsigned long long max; /* max cycle / nanoseconds */
> > +
> > +       for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> > +               max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> > +               do_div(max, clk_rate / (1 << div));
> 
> Dividing clk_rate by "1 << div" reduces accuracy a lot for large values of div.
> "NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE * (1 << div)" just fits in 64-bit,
> so you can use
> 
>         max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
>               (1 << div);
>         do_div(max, clk_rate);

Thank you very much for the suggestion. I will fix this in v2.

> > +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int channel,
> > +                                int div, int duty_ns, int period_ns)
> > +{
> > +       unsigned long long one_cycle, tmp;      /* 0.01 nanoseconds */
> > +       unsigned long clk_rate = clk_get_rate(rp->clk);
> > +       u32 cyc, ph;
> > +
> > +       one_cycle = (unsigned long long)NSEC_PER_SEC * 100;
> > +       do_div(one_cycle, clk_rate / (1 << div));
> 
> Same here:
> 
>         one_cycle = (unsigned long long)NSEC_PER_SEC * 100 * (1 << div)
>         do_div(one_cycle, clk_rate);

I will fix this in v2.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds