[v4,06/10] pwm: Add NVIDIA Tegra SoC support

This commit adds a generic PWM framework driver for the PWFM controller
found on NVIDIA Tegra SoCs. The driver is based on code from the
Chromium kernel tree and was originally written by Gary King (NVIDIA)
and later modified by Simon Que (Chromium).

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
Changes in v4:
  - merge patch from ChromiumOS kernel to fix a rounding issue
  - move DT code to patch 7

Changes in v3:
  - use pwm_device.hwpwm instead of recomputing it
  - update pwm_ops for changes in patch 2

Changes in v2:
  - set pwm_chip.dev field
  - rename clk_enb to enable
  - introduce NUM_PWM macro instead of hard-coding the number of PWM
    devices
  - update comment in tegra_pwm_config
  - fix coding-style for multi-line comments
  - use module_platform_driver
  - change license to GPL

 drivers/pwm/Kconfig     |   10 ++
 drivers/pwm/Makefile    |    1 +
 drivers/pwm/pwm-tegra.c |  256 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 267 insertions(+)
 create mode 100644 drivers/pwm/pwm-tegra.c

On Wed, Mar 14, 2012 at 04:56:29PM +0100, Thierry Reding wrote:
> This commit adds a generic PWM framework driver for the PWFM controller
> found on NVIDIA Tegra SoCs. The driver is based on code from the
> Chromium kernel tree and was originally written by Gary King (NVIDIA)
> and later modified by Simon Que (Chromium).
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
> Changes in v4:
>   - merge patch from ChromiumOS kernel to fix a rounding issue
>   - move DT code to patch 7
> 
> Changes in v3:
>   - use pwm_device.hwpwm instead of recomputing it
>   - update pwm_ops for changes in patch 2
> 
> Changes in v2:
>   - set pwm_chip.dev field
>   - rename clk_enb to enable
>   - introduce NUM_PWM macro instead of hard-coding the number of PWM
>     devices
>   - update comment in tegra_pwm_config
>   - fix coding-style for multi-line comments
>   - use module_platform_driver
>   - change license to GPL
> 
>  drivers/pwm/Kconfig     |   10 ++
>  drivers/pwm/Makefile    |    1 +
>  drivers/pwm/pwm-tegra.c |  256 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 267 insertions(+)
>  create mode 100644 drivers/pwm/pwm-tegra.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 93c1052..bda6f23 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -9,4 +9,14 @@ menuconfig PWM
>  
>  if PWM
>  
> +config PWM_TEGRA
> +	tristate "NVIDIA Tegra PWM support"
> +	depends on ARCH_TEGRA
> +	help
> +	  Generic PWM framework driver for the PWFM controller found on NVIDIA
> +	  Tegra SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-tegra.
> +
>  endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 3469c3d..12300f5 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_PWM)		+= core.o
> +obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> new file mode 100644
> index 0000000..19540fc
> --- /dev/null
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -0,0 +1,256 @@
> +/*
> + * drivers/pwm/pwm-tegra.c
> + *
> + * Tegra pulse-width-modulation controller driver
> + *
> + * Copyright (c) 2010, NVIDIA Corporation.
> + * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * 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, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pwm.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define PWM_ENABLE	(1 << 31)
> +#define PWM_DUTY_WIDTH	8
> +#define PWM_DUTY_SHIFT	16
> +#define PWM_SCALE_WIDTH	13
> +#define PWM_SCALE_SHIFT	0
> +
> +#define NUM_PWM 4
> +
> +struct tegra_pwm_chip {
> +	struct pwm_chip		chip;
> +	struct device		*dev;
> +
> +	struct clk		*clk;
> +
> +	int			enable[NUM_PWM];
> +	void __iomem		*mmio_base;
> +};
> +
> +static inline struct tegra_pwm_chip *to_tegra_pwm_chip(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct tegra_pwm_chip, chip);
> +}
> +
> +static inline int pwm_writel(struct tegra_pwm_chip *chip, unsigned int num,
> +		unsigned long val)
> +{
> +	unsigned long offset = num << 4;
> +	int rc;
> +
> +	rc = clk_enable(chip->clk);
> +	if (WARN_ON(rc))
> +		return rc;
> +
> +	writel(val, chip->mmio_base + offset);
> +	clk_disable(chip->clk);
> +
> +	return 0;
> +}
> +
> +static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +		int duty_ns, int period_ns)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	unsigned long long c;
> +	unsigned long rate, hz;
> +	u32 val = 0;
> +

Every pwm driver I have been looking at has some validation on
parameters here, something like:

	if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
		return -EINVAL;

It's not needed for tegra, or the check on those platforms is
unnecessary?

> +	/*
> +	 * Convert from duty_ns / period_ns to a fixed number of duty ticks
> +	 * per (1 << PWM_DUTY_WIDTH) cycles and make sure to round to the
> +	 * nearest integer during division.
> +	 */
> +	c = duty_ns * ((1 << PWM_DUTY_WIDTH) - 1) + period_ns / 2;
> +	do_div(c, period_ns);
> +
> +	val = (u32)c << PWM_DUTY_SHIFT;
> +
> +	/*
> +	 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
> +	 * cycles at the PWM clock rate will take period_ns nanoseconds.
> +	 */
> +	rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
> +	hz = 1000000000ul / period_ns;
> +
> +	rate = (rate + (hz / 2)) / hz;
> +
> +	/*
> +	 * Since the actual PWM divider is the register's frequency divider
> +	 * field minus 1, we need to decrement to get the correct value to
> +	 * write to the register.
> +	 */
> +	if (rate > 0)
> +		rate--;
> +
> +	/*
> +	 * Make sure that the rate will fit in the register's frequency
> +	 * divider field.
> +	 */
> +	if (rate >> PWM_SCALE_WIDTH)
> +		return -EINVAL;
> +
> +	val |= (rate << PWM_SCALE_SHIFT);
> +
> +	/* If the PWM channel is enabled, keep it enabled */
> +	if (pc->enable[pwm->hwpwm])
> +		val |= PWM_ENABLE;
> +
> +	return pwm_writel(pc, pwm->hwpwm, val);
> +}
> +
> +static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	int rc = 0;
> +
> +	if (!pc->enable[pwm->hwpwm]) {
> +		rc = clk_enable(pc->clk);
> +		if (!rc) {
> +			unsigned long offset = pwm->hwpwm << 4;
> +			u32 val;
> +
> +			val = readl(pc->mmio_base + offset);
> +			val |= PWM_ENABLE;
> +			writel(val, pc->mmio_base + offset);
> +
> +			pc->enable[pwm->hwpwm] = 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void tegra_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +
> +	if (pc->enable[pwm->hwpwm]) {
> +		unsigned long offset = pwm->hwpwm << 4;
> +		u32 val;
> +
> +		val = readl(pc->mmio_base + offset);
> +		val &= ~PWM_ENABLE;
> +		writel(val, pc->mmio_base + offset);
> +
> +		clk_disable(pc->clk);
> +		pc->enable[pwm->hwpwm] = 0;
> +	}
> +}
> +
> +static struct pwm_ops tegra_pwm_ops = {
> +	.config = tegra_pwm_config,
> +	.enable = tegra_pwm_enable,
> +	.disable = tegra_pwm_disable,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int tegra_pwm_probe(struct platform_device *pdev)
> +{
> +	struct tegra_pwm_chip *pwm;
> +	struct resource *r;
> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	pwm->dev = &pdev->dev;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r) {
> +		dev_err(&pdev->dev, "no memory resources defined\n");
> +		return -ENODEV;
> +	}
> +
> +	r = devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
> +			pdev->name);
> +	if (!r) {
> +		dev_err(&pdev->dev, "failed to request memory\n");
> +		return -EBUSY;
> +	}
> +
> +	pwm->mmio_base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> +	if (!pwm->mmio_base) {
> +		dev_err(&pdev->dev, "failed to ioremap() region\n");
> +		return -ENODEV;
> +	}
> +

The helper devm_request_and_ioremap() can help here?

> +	platform_set_drvdata(pdev, pwm);
> +
> +	pwm->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return PTR_ERR(pwm->clk);
> +
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &tegra_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = NUM_PWM;
> +
> +	ret = pwmchip_add(&pwm->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +		clk_put(pwm->clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devexit tegra_pwm_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pwm_chip *pwm = platform_get_drvdata(pdev);
> +	int i;
> +
> +	if (WARN_ON(!pwm))
> +		return -ENODEV;
> +
> +	pwmchip_remove(&pwm->chip);
> +
> +	for (i = 0; i < NUM_PWM; i++) {
> +		pwm_writel(pwm, i, 0);
> +
> +		if (pwm->enable[i])
> +			clk_disable(pwm->clk);
> +	}
> +
> +	clk_put(pwm->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver tegra_pwm_driver = {
> +	.driver = {
> +		.name = "tegra-pwm",
> +	},
> +	.probe = tegra_pwm_probe,
> +	.remove = __devexit_p(tegra_pwm_remove),
> +};
> +
I would remove this blank line.

> +module_platform_driver(tegra_pwm_driver);

Regards,
Shawn

> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("NVIDIA Corporation");
> -- 
> 1.7.9.4
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

* Shawn Guo wrote:
> On Wed, Mar 14, 2012 at 04:56:29PM +0100, Thierry Reding wrote:
> > This commit adds a generic PWM framework driver for the PWFM controller
> > found on NVIDIA Tegra SoCs. The driver is based on code from the
> > Chromium kernel tree and was originally written by Gary King (NVIDIA)
> > and later modified by Simon Que (Chromium).
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> > Changes in v4:
> >   - merge patch from ChromiumOS kernel to fix a rounding issue
> >   - move DT code to patch 7
> > 
> > Changes in v3:
> >   - use pwm_device.hwpwm instead of recomputing it
> >   - update pwm_ops for changes in patch 2
> > 
> > Changes in v2:
> >   - set pwm_chip.dev field
> >   - rename clk_enb to enable
> >   - introduce NUM_PWM macro instead of hard-coding the number of PWM
> >     devices
> >   - update comment in tegra_pwm_config
> >   - fix coding-style for multi-line comments
> >   - use module_platform_driver
> >   - change license to GPL
> > 
> >  drivers/pwm/Kconfig     |   10 ++
> >  drivers/pwm/Makefile    |    1 +
> >  drivers/pwm/pwm-tegra.c |  256 +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 267 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-tegra.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 93c1052..bda6f23 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -9,4 +9,14 @@ menuconfig PWM
> >  
> >  if PWM
> >  
> > +config PWM_TEGRA
> > +	tristate "NVIDIA Tegra PWM support"
> > +	depends on ARCH_TEGRA
> > +	help
> > +	  Generic PWM framework driver for the PWFM controller found on NVIDIA
> > +	  Tegra SoCs.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called pwm-tegra.
> > +
> >  endif
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 3469c3d..12300f5 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -1 +1,2 @@
> >  obj-$(CONFIG_PWM)		+= core.o
> > +obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > new file mode 100644
> > index 0000000..19540fc
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -0,0 +1,256 @@
> > +/*
> > + * drivers/pwm/pwm-tegra.c
> > + *
> > + * Tegra pulse-width-modulation controller driver
> > + *
> > + * Copyright (c) 2010, NVIDIA Corporation.
> > + * Based on arch/arm/plat-mxc/pwm.c by Sascha Hauer <s.hauer@pengutronix.de>
> > + *
> > + * 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, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write to the Free Software Foundation, Inc.,
> > + * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/pwm.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define PWM_ENABLE	(1 << 31)
> > +#define PWM_DUTY_WIDTH	8
> > +#define PWM_DUTY_SHIFT	16
> > +#define PWM_SCALE_WIDTH	13
> > +#define PWM_SCALE_SHIFT	0
> > +
> > +#define NUM_PWM 4
> > +
> > +struct tegra_pwm_chip {
> > +	struct pwm_chip		chip;
> > +	struct device		*dev;
> > +
> > +	struct clk		*clk;
> > +
> > +	int			enable[NUM_PWM];
> > +	void __iomem		*mmio_base;
> > +};
> > +
> > +static inline struct tegra_pwm_chip *to_tegra_pwm_chip(struct pwm_chip *chip)
> > +{
> > +	return container_of(chip, struct tegra_pwm_chip, chip);
> > +}
> > +
> > +static inline int pwm_writel(struct tegra_pwm_chip *chip, unsigned int num,
> > +		unsigned long val)
> > +{
> > +	unsigned long offset = num << 4;
> > +	int rc;
> > +
> > +	rc = clk_enable(chip->clk);
> > +	if (WARN_ON(rc))
> > +		return rc;
> > +
> > +	writel(val, chip->mmio_base + offset);
> > +	clk_disable(chip->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +		int duty_ns, int period_ns)
> > +{
> > +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> > +	unsigned long long c;
> > +	unsigned long rate, hz;
> > +	u32 val = 0;
> > +
> 
> Every pwm driver I have been looking at has some validation on
> parameters here, something like:
> 
> 	if (pwm == NULL || period_ns == 0 || duty_ns > period_ns)
> 		return -EINVAL;
> 
> It's not needed for tegra, or the check on those platforms is
> unnecessary?

Yes, I should add similar checks for the Tegra driver. On the other hand
maybe the checks should be pushed into the core. At least those checks that
are truly general sanity checks. Off the top of my head, I can think of the
following general preconditions:

	pwm != NULL
	period_ns > 0
	duty_ns >= 0
	duty_ns <= period_ns

Of course duty_ns >= 0 could be done away with by just making the duty_ns and
period_ns parameters unsigned. But such changes were actually supposed to be
added incrementally once the framework had been merged.

> > +	r = devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
> > +			pdev->name);
> > +	if (!r) {
> > +		dev_err(&pdev->dev, "failed to request memory\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	pwm->mmio_base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> > +	if (!pwm->mmio_base) {
> > +		dev_err(&pdev->dev, "failed to ioremap() region\n");
> > +		return -ENODEV;
> > +	}
> > +
> 
> The helper devm_request_and_ioremap() can help here?

Yes, absolutely.

> > +static struct platform_driver tegra_pwm_driver = {
> > +	.driver = {
> > +		.name = "tegra-pwm",
> > +	},
> > +	.probe = tegra_pwm_probe,
> > +	.remove = __devexit_p(tegra_pwm_remove),
> > +};
> > +
> I would remove this blank line.
> 
> > +module_platform_driver(tegra_pwm_driver);

I don't know; I think it makes the code cluttered.

Thierry

On 03/14/2012 09:56 AM, Thierry Reding wrote:
> This commit adds a generic PWM framework driver for the PWFM controller
> found on NVIDIA Tegra SoCs. The driver is based on code from the
> Chromium kernel tree and was originally written by Gary King (NVIDIA)
...
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
...
> +static int tegra_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	int rc = 0;
> +
> +	if (!pc->enable[pwm->hwpwm]) {

IIRC, the new PWM core only calls the enable() op for a disabled ->
enabled transition, so this driver probably doesn't need to check the
same thing, and you can get rid of tegra_pwm_chip.enable[] and have
tegra_pwm_config() read the enable flag from the core pwm device instead.

> +		rc = clk_enable(pc->clk);
> +		if (!rc) {
> +			unsigned long offset = pwm->hwpwm << 4;
> +			u32 val;
> +
> +			val = readl(pc->mmio_base + offset);
> +			val |= PWM_ENABLE;
> +			writel(val, pc->mmio_base + offset);

It seems a little of for the driver to define a pwm_writel() function
but only use it in some places but not others. I guess it's because in
some places the code knows the clock is on, so doesn't need the
clk_enable()/disable() helper in pwm_writel(), but I'd tend towards
always using pwm_readl()/pwm_writel() throughout the driver, and lifting
the clock management out of those low-level functions myself.

> +static int __devexit tegra_pwm_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pwm_chip *pwm = platform_get_drvdata(pdev);
> +	int i;
> +
> +	if (WARN_ON(!pwm))
> +		return -ENODEV;
> +
> +	pwmchip_remove(&pwm->chip);
> +
> +	for (i = 0; i < NUM_PWM; i++) {
> +		pwm_writel(pwm, i, 0);
> +
> +		if (pwm->enable[i])
> +			clk_disable(pwm->clk);

Should the core call the disable() op before the remove() op so drivers
don't need to check this? I'm a little in two minds about this; if this
decision is deferred to drivers (as the code above assumes), then I
suppose the whole remove() path might be marginally more efficient.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html