* [PATCH 0/2] PWM driver support for EHRPWM & ECAP @ 2012-07-13 9:35 Philip, Avinash 2012-07-13 9:35 ` [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM Philip, Avinash 2012-07-13 9:35 ` [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM Philip, Avinash 0 siblings, 2 replies; 14+ messages in thread From: Philip, Avinash @ 2012-07-13 9:35 UTC (permalink / raw) To: linux-arm-kernel This patch series adds support for PWM driver support for EHRPWM and ECAP modules which has been present on AM335x SOC. AM335X SOC has 3 instances of ECAP & EHRPWM. EHRPWM device can be used to generate PWM waveforms. It has 2 channels of PWM output and each can be configured for different duty cycle and polarity. ECAP device can be used as either Capture or APWM mode. In APWM mode, ECAP can generate PWM waveform. Patch #1 adds Generic PWM driver support for simple APWM functionality of ECAP ECAP hardware. Patch #2 adds PWM driver support for EHRPWM device. This patch series is based on new PWM framework maintained at [1]. 1. https://gitorious.org/linux-pwm/linux-pwm/trees/for-next 2. http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104483.html Philip, Avinash (2): PWM: ECAP: PWM driver support for ECAP APWM. PWM: EHRPWM: PWM driver support for EHRPWM. drivers/pwm/Kconfig | 22 ++ drivers/pwm/Makefile | 2 + drivers/pwm/pwm-ecap.c | 255 +++++++++++++++++++++++++ drivers/pwm/pwm-ehrpwm.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 755 insertions(+), 0 deletions(-) create mode 100644 drivers/pwm/pwm-ecap.c create mode 100644 drivers/pwm/pwm-ehrpwm.c ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-13 9:35 [PATCH 0/2] PWM driver support for EHRPWM & ECAP Philip, Avinash @ 2012-07-13 9:35 ` Philip, Avinash 2012-07-23 6:52 ` Thierry Reding 2012-07-13 9:35 ` [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM Philip, Avinash 1 sibling, 1 reply; 14+ messages in thread From: Philip, Avinash @ 2012-07-13 9:35 UTC (permalink / raw) To: linux-arm-kernel ECAP hardware on AM33XX SOC supports auxiliary PWM (APWM) feature. This commit adds PWM driver support for ECAP hardware on AM33XX SOC. In the ECAP hardware, each PWM pin can also be configured to be in capture mode. Current implementation only supports PWM mode of operation. Also, hardware supports sync between multiple PWM pins but the driver supports simple independent PWM functionality. Signed-off-by: Philip, Avinash <avinashphilip@ti.com> Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com> --- :100644 100644 94e176e... f20b8f2... M drivers/pwm/Kconfig :100644 100644 5459702... 7dd90ec... M drivers/pwm/Makefile :000000 100644 0000000... 81efc9e... A drivers/pwm/pwm-ecap.c drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-ecap.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 266 insertions(+), 0 deletions(-) diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 94e176e..f20b8f2 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -85,4 +85,14 @@ config PWM_VT8500 To compile this driver as a module, choose M here: the module will be called pwm-vt8500. +config PWM_ECAP + tristate "ECAP PWM support" + depends on SOC_AM33XX + help + PWM driver support for the ECAP APWM controller found on AM33XX + TI SOC + + To compile this driver as a module, choose M here: the module + will be called pwm_ecap. + endif diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 5459702..7dd90ec 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -7,3 +7,4 @@ obj-$(CONFIG_PWM_PXA) += pwm-pxa.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o +obj-$(CONFIG_PWM_ECAP) += pwm-ecap.o diff --git a/drivers/pwm/pwm-ecap.c b/drivers/pwm/pwm-ecap.c new file mode 100644 index 0000000..81efc9e --- /dev/null +++ b/drivers/pwm/pwm-ecap.c @@ -0,0 +1,255 @@ +/* + * ECAP PWM driver + * + * Copyright (C) 2012 Texas Instruments, Inc. - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, 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., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/pm_runtime.h> +#include <linux/pwm.h> + +/* ECAP registers and bits definitions */ +#define TSCTR 0x00 +#define CTRPHS 0x04 +#define CAP1 0x08 +#define CAP2 0x0C +#define CAP3 0x10 +#define CAP4 0x14 +#define ECCTL1 0x28 +#define ECCTL2 0x2A +#define ECCTL2_APWM_POL_LOW BIT(10) +#define ECCTL2_APWM_MODE BIT(9) +#define ECCTL2_SYNC_SEL_DISA (BIT(7) | BIT(6)) +#define ECCTL2_TSCTR_FREERUN BIT(4) + +#define ECEINT 0x2C +#define ECFLG 0x2E +#define ECCLR 0x30 +#define REVID 0x5c + +#define DRIVER_NAME "ecap" + +struct ecap_pwm_chip { + struct pwm_chip chip; + unsigned int clk_rate; + void __iomem *mmio_base; + int pwm_period_ns; + int pwm_duty_ns; +}; + +static inline struct ecap_pwm_chip *to_ecap_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct ecap_pwm_chip, chip); +} + +/* + * period_ns = 10^9 * period_cycles / PWM_CLK_RATE + * duty_ns = 10^9 * duty_cycles / PWM_CLK_RATE + */ +static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip); + unsigned long long c; + unsigned long period_cycles, duty_cycles; + unsigned int reg_val; + + if (period_ns < 0 || duty_ns < 0 || period_ns > NSEC_PER_SEC) + return -ERANGE; + + c = pc->clk_rate; + c = c * period_ns; + do_div(c, NSEC_PER_SEC); + period_cycles = (unsigned long)c; + + if (period_cycles < 1) { + period_cycles = 1; + duty_cycles = 1; + } else { + c = pc->clk_rate; + c = c * duty_ns; + do_div(c, NSEC_PER_SEC); + duty_cycles = (unsigned long)c; + } + + pc->pwm_duty_ns = duty_ns; + pc->pwm_period_ns = period_ns; + + pm_runtime_get_sync(pc->chip.dev); + + reg_val = readw(pc->mmio_base + ECCTL2); + + /* Configure PWM mode & disable sync option */ + reg_val |= ECCTL2_APWM_MODE | ECCTL2_SYNC_SEL_DISA; + + writew(reg_val, pc->mmio_base + ECCTL2); + + if (!test_bit(PWMF_ENABLED, &pwm->flags)) { + /* Update active registers if not running */ + writel(duty_cycles, pc->mmio_base + CAP2); + writel(period_cycles, pc->mmio_base + CAP1); + } else { + /* + * Update shadow registers to configure period and + * compare values. This helps current PWM period to + * complete on reconfiguring + */ + writel(duty_cycles, pc->mmio_base + CAP4); + writel(period_cycles, pc->mmio_base + CAP3); + } + + pm_runtime_put_sync(pc->chip.dev); + return 0; +} + +static int ecap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip); + unsigned int reg_val; + + /* Leave clock enabled on enabling PWM */ + pm_runtime_get_sync(pc->chip.dev); + + /* + * Enable 'Free run Time stamp counter mode' to start counter + * and 'APWM mode' to enable APWM output + */ + reg_val = readw(pc->mmio_base + ECCTL2); + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; + writew(reg_val, pc->mmio_base + ECCTL2); + return 0; +} + +static void ecap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip); + unsigned int reg_val; + + /* + * Disable 'Free run Time stamp counter mode' to stop counter + * and 'APWM mode' to put APWM output to low + */ + reg_val = readw(pc->mmio_base + ECCTL2); + reg_val &= ~(ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE); + writew(reg_val, pc->mmio_base + ECCTL2); + + /* Disable clock on PWM disable */ + pm_runtime_put_sync(pc->chip.dev); +} + +static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + if (test_bit(PWMF_ENABLED, &pwm->flags)) { + dev_warn(chip->dev, "Removing PWM device without disabling\n"); + pm_runtime_put_sync(chip->dev); + } +} + +static struct pwm_ops ecap_pwm_ops = { + .free = ecap_pwm_free, + .config = ecap_pwm_config, + .enable = ecap_pwm_enable, + .disable = ecap_pwm_disable, + .owner = THIS_MODULE, +}; + +static int __devinit ecap_pwm_probe(struct platform_device *pdev) +{ + int ret; + struct resource *r; + struct clk *clk; + struct ecap_pwm_chip *pc; + + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); + if (!pc) { + dev_err(&pdev->dev, "failed to allocate memory\n"); + return -ENOMEM; + } + + clk = devm_clk_get(&pdev->dev, "fck"); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "failed to get clock\n"); + return PTR_ERR(clk); + } + + pc->clk_rate = clk_get_rate(clk); + if (!pc->clk_rate) { + dev_err(&pdev->dev, "failed to get clock rate\n"); + return -EINVAL; + } + + pc->chip.dev = &pdev->dev; + pc->chip.ops = &ecap_pwm_ops; + pc->chip.base = -1; + pc->chip.npwm = 1; + + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); + if (r == NULL) { + dev_err(&pdev->dev, "no memory resource defined\n"); + return -ENODEV; + } + + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r); + if (!pc->mmio_base) { + dev_err(&pdev->dev, "failed to ioremap() registers\n"); + return -EADDRNOTAVAIL; + } + + ret = pwmchip_add(&pc->chip); + if (ret < 0) { + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); + return ret; + } + + pm_runtime_enable(&pdev->dev); + platform_set_drvdata(pdev, pc); + dev_info(&pdev->dev, "PWM device initialized\n"); + return 0; +} + +static int __devexit ecap_pwm_remove(struct platform_device *pdev) +{ + struct ecap_pwm_chip *pc = platform_get_drvdata(pdev); + struct pwm_device *pwm; + + if (WARN_ON(!pc)) + return -ENODEV; + + pwm = &pc->chip.pwms[0]; + pm_runtime_put_sync(&pdev->dev); + pm_runtime_disable(&pdev->dev); + pwmchip_remove(&pc->chip); + return 0; +} + +static struct platform_driver ecap_pwm_driver = { + .driver = { + .name = DRIVER_NAME, + }, + .probe = ecap_pwm_probe, + .remove = __devexit_p(ecap_pwm_remove), +}; + +module_platform_driver(ecap_pwm_driver); + +MODULE_DESCRIPTION("ECAP PWM driver"); +MODULE_AUTHOR("Texas Instruments"); +MODULE_LICENSE("GPL"); -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-13 9:35 ` [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM Philip, Avinash @ 2012-07-23 6:52 ` Thierry Reding 2012-07-23 9:10 ` Philip, Avinash 0 siblings, 1 reply; 14+ messages in thread From: Thierry Reding @ 2012-07-23 6:52 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > ECAP hardware on AM33XX SOC supports auxiliary PWM (APWM) feature. This > commit adds PWM driver support for ECAP hardware on AM33XX SOC. > > In the ECAP hardware, each PWM pin can also be configured to be in > capture mode. Current implementation only supports PWM mode of > operation. Also, hardware supports sync between multiple PWM pins but > the driver supports simple independent PWM functionality. > > Signed-off-by: Philip, Avinash <avinashphilip@ti.com> > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com> > --- > :100644 100644 94e176e... f20b8f2... M drivers/pwm/Kconfig > :100644 100644 5459702... 7dd90ec... M drivers/pwm/Makefile > :000000 100644 0000000... 81efc9e... A drivers/pwm/pwm-ecap.c > drivers/pwm/Kconfig | 10 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-ecap.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 266 insertions(+), 0 deletions(-) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 94e176e..f20b8f2 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -85,4 +85,14 @@ config PWM_VT8500 > To compile this driver as a module, choose M here: the module > will be called pwm-vt8500. > > +config PWM_ECAP > + tristate "ECAP PWM support" > + depends on SOC_AM33XX > + help > + PWM driver support for the ECAP APWM controller found on AM33XX > + TI SOC > + > + To compile this driver as a module, choose M here: the module > + will be called pwm_ecap. > + > endif > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 5459702..7dd90ec 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -7,3 +7,4 @@ obj-$(CONFIG_PWM_PXA) += pwm-pxa.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > +obj-$(CONFIG_PWM_ECAP) += pwm-ecap.o Both the Kconfig and Makefile should have the entries ordered alphabetically. > diff --git a/drivers/pwm/pwm-ecap.c b/drivers/pwm/pwm-ecap.c > new file mode 100644 > index 0000000..81efc9e > --- /dev/null > +++ b/drivers/pwm/pwm-ecap.c > @@ -0,0 +1,255 @@ > +/* > + * ECAP PWM driver > + * > + * Copyright (C) 2012 Texas Instruments, Inc. - http://www.ti.com/ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, 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., 675 Mass Ave, Cambridge, MA 02139, USA. > + */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > +#include <linux/err.h> > +#include <linux/clk.h> > +#include <linux/pm_runtime.h> > +#include <linux/pwm.h> > + > +/* ECAP registers and bits definitions */ > +#define TSCTR 0x00 > +#define CTRPHS 0x04 > +#define CAP1 0x08 > +#define CAP2 0x0C > +#define CAP3 0x10 > +#define CAP4 0x14 > +#define ECCTL1 0x28 These registers are not used. I guess there is some use to list all registers here but on the other hand the majority are unused so they just clutter the driver. > +#define ECCTL2_APWM_POL_LOW BIT(10) This bit is never used. > +#define ECEINT 0x2C > +#define ECFLG 0x2E > +#define ECCLR 0x30 > +#define REVID 0x5c These are unused as well. > + > +#define DRIVER_NAME "ecap" You only use this once when defining the struct platform_driver, so maybe you can just drop it. > + > +struct ecap_pwm_chip { > + struct pwm_chip chip; > + unsigned int clk_rate; > + void __iomem *mmio_base; > + int pwm_period_ns; > + int pwm_duty_ns; > +}; The pwm_period_ns and pwm_duty_ns don't seem to be used at all. Can they be dropped? > + > +static inline struct ecap_pwm_chip *to_ecap_pwm_chip(struct pwm_chip *chip) > +{ > + return container_of(chip, struct ecap_pwm_chip, chip); > +} > + > +/* > + * period_ns = 10^9 * period_cycles / PWM_CLK_RATE > + * duty_ns = 10^9 * duty_cycles / PWM_CLK_RATE > + */ > +static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip); > + unsigned long long c; > + unsigned long period_cycles, duty_cycles; > + unsigned int reg_val; > + > + if (period_ns < 0 || duty_ns < 0 || period_ns > NSEC_PER_SEC) > + return -ERANGE; > + > + c = pc->clk_rate; > + c = c * period_ns; > + do_div(c, NSEC_PER_SEC); > + period_cycles = (unsigned long)c; > + > + if (period_cycles < 1) { > + period_cycles = 1; > + duty_cycles = 1; > + } else { > + c = pc->clk_rate; > + c = c * duty_ns; > + do_div(c, NSEC_PER_SEC); > + duty_cycles = (unsigned long)c; > + } > + > + pc->pwm_duty_ns = duty_ns; > + pc->pwm_period_ns = period_ns; > + > + pm_runtime_get_sync(pc->chip.dev); > + > + reg_val = readw(pc->mmio_base + ECCTL2); > + > + /* Configure PWM mode & disable sync option */ > + reg_val |= ECCTL2_APWM_MODE | ECCTL2_SYNC_SEL_DISA; > + > + writew(reg_val, pc->mmio_base + ECCTL2); > + > + if (!test_bit(PWMF_ENABLED, &pwm->flags)) { > + /* Update active registers if not running */ > + writel(duty_cycles, pc->mmio_base + CAP2); > + writel(period_cycles, pc->mmio_base + CAP1); > + } else { > + /* > + * Update shadow registers to configure period and > + * compare values. This helps current PWM period to > + * complete on reconfiguring > + */ > + writel(duty_cycles, pc->mmio_base + CAP4); > + writel(period_cycles, pc->mmio_base + CAP3); > + } > + > + pm_runtime_put_sync(pc->chip.dev); > + return 0; > +} > + > +static int ecap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip); > + unsigned int reg_val; > + > + /* Leave clock enabled on enabling PWM */ > + pm_runtime_get_sync(pc->chip.dev); > + > + /* > + * Enable 'Free run Time stamp counter mode' to start counter > + * and 'APWM mode' to enable APWM output > + */ > + reg_val = readw(pc->mmio_base + ECCTL2); > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; You already set the APWM_MODE bit in .config(), why is it needed here again? Seeing that .disable() clears the bit as well, perhaps leaving it clear in .config() is the better option. > + writew(reg_val, pc->mmio_base + ECCTL2); > + return 0; > +} > + > +static void ecap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip); > + unsigned int reg_val; > + > + /* > + * Disable 'Free run Time stamp counter mode' to stop counter > + * and 'APWM mode' to put APWM output to low > + */ > + reg_val = readw(pc->mmio_base + ECCTL2); > + reg_val &= ~(ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE); > + writew(reg_val, pc->mmio_base + ECCTL2); > + > + /* Disable clock on PWM disable */ > + pm_runtime_put_sync(pc->chip.dev); > +} > + > +static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > + dev_warn(chip->dev, "Removing PWM device without disabling\n"); > + pm_runtime_put_sync(chip->dev); > + } > +} I wonder whether we want to have something like this in the PWM core at some point. Maybe we should call .disable() automatically when they are freed, or alternatively return -EBUSY if a PWM is freed but still enabled. I think I prefer the latter. For now we can leave this in, because I don't want to make any such core changes for 3.6 now that the merge window is open. > + > +static struct pwm_ops ecap_pwm_ops = { > + .free = ecap_pwm_free, > + .config = ecap_pwm_config, > + .enable = ecap_pwm_enable, > + .disable = ecap_pwm_disable, > + .owner = THIS_MODULE, > +}; This should be "static const pwm_ops ...". > + > +static int __devinit ecap_pwm_probe(struct platform_device *pdev) > +{ > + int ret; > + struct resource *r; > + struct clk *clk; > + struct ecap_pwm_chip *pc; > + > + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > + if (!pc) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); > + return -ENOMEM; > + } > + > + clk = devm_clk_get(&pdev->dev, "fck"); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "failed to get clock\n"); > + return PTR_ERR(clk); > + } > + > + pc->clk_rate = clk_get_rate(clk); > + if (!pc->clk_rate) { > + dev_err(&pdev->dev, "failed to get clock rate\n"); > + return -EINVAL; > + } > + > + pc->chip.dev = &pdev->dev; > + pc->chip.ops = &ecap_pwm_ops; > + pc->chip.base = -1; > + pc->chip.npwm = 1; The cover letter mentions that the AM335x has 3 instances of the ECAP. Is there any chance that they can be unified in one driver instance (i.e. .npwm = 3?). > + > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); > + if (r == NULL) { You use "if (!ptr)" everywhere else, maybe you should make this consistent? Also the platform_get_resource_byname() is really only useful for devices that have several register regions associated with them. I don't know your hardware in detail, but I doubt that a PWM device has more than one register region. > + dev_err(&pdev->dev, "no memory resource defined\n"); > + return -ENODEV; > + } > + > + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r); > + if (!pc->mmio_base) { > + dev_err(&pdev->dev, "failed to ioremap() registers\n"); > + return -EADDRNOTAVAIL; > + } > + > + ret = pwmchip_add(&pc->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); > + return ret; > + } > + > + pm_runtime_enable(&pdev->dev); > + platform_set_drvdata(pdev, pc); > + dev_info(&pdev->dev, "PWM device initialized\n"); I think this can go away. If none of the above errors is shown you can just assume that the PWM was initialized. > + return 0; > +} > + > +static int __devexit ecap_pwm_remove(struct platform_device *pdev) > +{ > + struct ecap_pwm_chip *pc = platform_get_drvdata(pdev); > + struct pwm_device *pwm; > + > + if (WARN_ON(!pc)) > + return -ENODEV; Do you really want to be this verbose? This kind of check is very rare anyway, but explicitly warning about it doesn't seems overkill. I think the check can even be left out, since every possible error that would lead to the driver data not being checked would already cause .probe() to return < 0 and therefore .remove() won't ever be called anyway. > + > + pwm = &pc->chip.pwms[0]; You don't use this variable. Thierry > + pm_runtime_put_sync(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > + pwmchip_remove(&pc->chip); > + return 0; > +} > + > +static struct platform_driver ecap_pwm_driver = { > + .driver = { > + .name = DRIVER_NAME, > + }, > + .probe = ecap_pwm_probe, > + .remove = __devexit_p(ecap_pwm_remove), > +}; > + > +module_platform_driver(ecap_pwm_driver); > + > +MODULE_DESCRIPTION("ECAP PWM driver"); > +MODULE_AUTHOR("Texas Instruments"); > +MODULE_LICENSE("GPL"); > -- > 1.7.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/75e99371/attachment-0001.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-23 6:52 ` Thierry Reding @ 2012-07-23 9:10 ` Philip, Avinash 2012-07-23 9:22 ` Thierry Reding 0 siblings, 1 reply; 14+ messages in thread From: Philip, Avinash @ 2012-07-23 9:10 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: [snip] > > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > > +obj-$(CONFIG_PWM_ECAP) += pwm-ecap.o > > Both the Kconfig and Makefile should have the entries ordered > alphabetically. Ok. I will correct it. > [snip] > > +/* ECAP registers and bits definitions */ > > +#define TSCTR 0x00 > > +#define CTRPHS 0x04 > > +#define CAP1 0x08 > > +#define CAP2 0x0C > > +#define CAP3 0x10 > > +#define CAP4 0x14 > > +#define ECCTL1 0x28 > > These registers are not used. I guess there is some use to list all > registers here but on the other hand the majority are unused so they > just clutter the driver. > > > +#define ECCTL2_APWM_POL_LOW BIT(10) > > This bit is never used. > > > +#define ECEINT 0x2C > > +#define ECFLG 0x2E > > +#define ECCLR 0x30 > > +#define REVID 0x5c > > These are unused as well. Ok. I will remove it. These are been placed in future enhancement. > > > + > > +#define DRIVER_NAME "ecap" > > You only use this once when defining the struct platform_driver, so > maybe you can just drop it. In the v2 patch, I am planning to use same in platform_get_resource_byname(). Here I will use this define to search resources. > > > + > > +struct ecap_pwm_chip { > > + struct pwm_chip chip; > > + unsigned int clk_rate; > > + void __iomem *mmio_base; > > + int pwm_period_ns; > > + int pwm_duty_ns; > > +}; > > The pwm_period_ns and pwm_duty_ns don't seem to be used at all. Can they > be dropped? Ok. I will remove it. > > > + /* > > + * Enable 'Free run Time stamp counter mode' to start counter > > + * and 'APWM mode' to enable APWM output > > + */ > > + reg_val = readw(pc->mmio_base + ECCTL2); > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > You already set the APWM_MODE bit in .config(), why is it needed here > again? Seeing that .disable() clears the bit as well, perhaps leaving it > clear in .config() is the better option. Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low in idle state). The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) During PWM configuration. To enable loading from Shadow registers, APWM mode should be set. The same is done in .config(). > > > +} > > + > > +static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + if (test_bit(PWMF_ENABLED, &pwm->flags)) { > > + dev_warn(chip->dev, "Removing PWM device without disabling\n"); > > + pm_runtime_put_sync(chip->dev); > > + } > > +} > > I wonder whether we want to have something like this in the PWM core at > some point. Maybe we should call .disable() automatically when they are > freed, or alternatively return -EBUSY if a PWM is freed but still > enabled. I think I prefer the latter. For now we can leave this in, > because I don't want to make any such core changes for 3.6 now that the > merge window is open. OK Thanks. > > > + > > +static struct pwm_ops ecap_pwm_ops = { > > + .free = ecap_pwm_free, > > + .config = ecap_pwm_config, > > + .enable = ecap_pwm_enable, > > + .disable = ecap_pwm_disable, > > + .owner = THIS_MODULE, > > +}; > > This should be "static const pwm_ops ...". Ok I will correct it. > > > + > > +static int __devinit ecap_pwm_probe(struct platform_device *pdev) > > +{ > > + int ret; > > + struct resource *r; > > + struct clk *clk; > > + struct ecap_pwm_chip *pc; > > + > > + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > > + if (!pc) { > > + dev_err(&pdev->dev, "failed to allocate memory\n"); > > + return -ENOMEM; > > + } > > + > > + clk = devm_clk_get(&pdev->dev, "fck"); > > + if (IS_ERR(clk)) { > > + dev_err(&pdev->dev, "failed to get clock\n"); > > + return PTR_ERR(clk); > > + } > > + > > + pc->clk_rate = clk_get_rate(clk); > > + if (!pc->clk_rate) { > > + dev_err(&pdev->dev, "failed to get clock rate\n"); > > + return -EINVAL; > > + } > > + > > + pc->chip.dev = &pdev->dev; > > + pc->chip.ops = &ecap_pwm_ops; > > + pc->chip.base = -1; > > + pc->chip.npwm = 1; > > The cover letter mentions that the AM335x has 3 instances of the ECAP. > Is there any chance that they can be unified in one driver instance > (i.e. .npwm = 3?). No. All instances have separate resources (clocks, memory ..). > > > + > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); > > + if (r == NULL) { > > You use "if (!ptr)" everywhere else, maybe you should make this > consistent? Ok > Also the platform_get_resource_byname() is really only > useful for devices that have several register regions associated with > them. I don't know your hardware in detail, but I doubt that a PWM > device has more than one register region. In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & Common Config space). So I need to use platform_get_resource_byname() > > > + dev_err(&pdev->dev, "no memory resource defined\n"); > > + return -ENODEV; > > + } > > + > > + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r); > > + if (!pc->mmio_base) { > > + dev_err(&pdev->dev, "failed to ioremap() registers\n"); > > + return -EADDRNOTAVAIL; > > + } > > + > > + ret = pwmchip_add(&pc->chip); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); > > + return ret; > > + } > > + > > + pm_runtime_enable(&pdev->dev); > > + platform_set_drvdata(pdev, pc); > > + dev_info(&pdev->dev, "PWM device initialized\n"); > > I think this can go away. If none of the above errors is shown you can > just assume that the PWM was initialized. Ok. I will remove. > > > + return 0; > > +} > > + > > +static int __devexit ecap_pwm_remove(struct platform_device *pdev) > > +{ > > + struct ecap_pwm_chip *pc = platform_get_drvdata(pdev); > > + struct pwm_device *pwm; > > + > > + if (WARN_ON(!pc)) > > + return -ENODEV; > > Do you really want to be this verbose? This kind of check is very rare > anyway, but explicitly warning about it doesn't seems overkill. I think > the check can even be left out, since every possible error that would > lead to the driver data not being checked would already cause .probe() > to return < 0 and therefore .remove() won't ever be called anyway. Point taken, I will remove. > > > + > > + pwm = &pc->chip.pwms[0]; > > You don't use this variable. Ok Thanks Avinash > > Thierry > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-23 9:10 ` Philip, Avinash @ 2012-07-23 9:22 ` Thierry Reding 2012-07-23 12:44 ` Philip, Avinash 0 siblings, 1 reply; 14+ messages in thread From: Thierry Reding @ 2012-07-23 9:22 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > + /* > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > + * and 'APWM mode' to enable APWM output > > > + */ > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > clear in .config() is the better option. > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > in idle state). > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > During PWM configuration. To enable loading from Shadow registers, APWM mode > should be set. > The same is done in .config(). My point was that if you do it in .enable(), then why do you still set it in .config()? Since the sequence is typically .config() followed by .enable(), setting the bit in both seems redundant. It should be enough to load the registers during .enable(), right? > > > + pc->chip.dev = &pdev->dev; > > > + pc->chip.ops = &ecap_pwm_ops; > > > + pc->chip.base = -1; > > > + pc->chip.npwm = 1; > > > > The cover letter mentions that the AM335x has 3 instances of the ECAP. > > Is there any chance that they can be unified in one driver instance > > (i.e. .npwm = 3?). > > No. All instances have separate resources (clocks, memory ..). > > > > > > + > > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); > > > + if (r == NULL) { > > > > You use "if (!ptr)" everywhere else, maybe you should make this > > consistent? > Ok > > Also the platform_get_resource_byname() is really only > > useful for devices that have several register regions associated with > > them. I don't know your hardware in detail, but I doubt that a PWM > > device has more than one register region. > > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & > Common Config space). So I need to use platform_get_resource_byname() Above you say that all instances have separate resources, so how come you need to specify 4 register spaces? The eCAP registers should clearly be passed to the eCAP device, while the eHRPWM registers should go to the eHRPWM device. My point is that if you need to refer to the register region by name, then the driver should clearly be using more than a single region. Neither the eCAP nor eHRPWM do that. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/768d1fc6/attachment.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-23 9:22 ` Thierry Reding @ 2012-07-23 12:44 ` Philip, Avinash 2012-07-23 13:37 ` Thierry Reding 0 siblings, 1 reply; 14+ messages in thread From: Philip, Avinash @ 2012-07-23 12:44 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > + /* > > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > > + * and 'APWM mode' to enable APWM output > > > > + */ > > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > > clear in .config() is the better option. > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > in idle state). > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > should be set. > > The same is done in .config(). > > My point was that if you do it in .enable(), then why do you still set > it in .config()? Since the sequence is typically .config() followed by > .enable(), setting the bit in both seems redundant. It should be enough > to load the registers during .enable(), right? Consider scenarios where .enable() can be called without calling .config(). Example I just need to stop the pwm signal momentarily and re-enable. In this case, .config() need not be called. So, APWM mode bit needs to be set in .enable() Now, considering .config() followed by .enable(). Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM mode bit to be set. The same can be done in .enable() also. However, we again need to pass the pwm parameters (period & duty cycle values) to the .enable(). We don't want to duplicate this parameter passing. To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > > + pc->chip.dev = &pdev->dev; > > > > + pc->chip.ops = &ecap_pwm_ops; > > > > + pc->chip.base = -1; > > > > + pc->chip.npwm = 1; > > > > > > The cover letter mentions that the AM335x has 3 instances of the ECAP. > > > Is there any chance that they can be unified in one driver instance > > > (i.e. .npwm = 3?). > > > > No. All instances have separate resources (clocks, memory ..). > > > > > > > > > + > > > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); > > > > + if (r == NULL) { > > > > > > You use "if (!ptr)" everywhere else, maybe you should make this > > > consistent? > > Ok > > > Also the platform_get_resource_byname() is really only > > > useful for devices that have several register regions associated with > > > them. I don't know your hardware in detail, but I doubt that a PWM > > > device has more than one register region. > > > > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & > > Common Config space). So I need to use platform_get_resource_byname() > > Above you say that all instances have separate resources, so how come > you need to specify 4 register spaces? The eCAP registers should clearly > be passed to the eCAP device, while the eHRPWM registers should go to > the eHRPWM device. > > My point is that if you need to refer to the register region by name, > then the driver should clearly be using more than a single region. > Neither the eCAP nor eHRPWM do that. On AM335x SoC, this common config space is shared by the other 3 modules (eCAP, eHRPWM, eQEP). However, on Davinci platform don't have any common config space. So from driver reusability view, usage of platform_get_resource_byname() is better choice. Thanks Avinash > > Thierry > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-23 12:44 ` Philip, Avinash @ 2012-07-23 13:37 ` Thierry Reding 2012-07-24 7:52 ` Philip, Avinash 0 siblings, 1 reply; 14+ messages in thread From: Thierry Reding @ 2012-07-23 13:37 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > + /* > > > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > > > + * and 'APWM mode' to enable APWM output > > > > > + */ > > > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > > > clear in .config() is the better option. > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > in idle state). > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > should be set. > > > The same is done in .config(). > > > > My point was that if you do it in .enable(), then why do you still set > > it in .config()? Since the sequence is typically .config() followed by > > .enable(), setting the bit in both seems redundant. It should be enough > > to load the registers during .enable(), right? > > Consider scenarios where .enable() can be called without calling .config(). > Example I just need to stop the pwm signal momentarily and re-enable. > In this case, .config() need not be called. So, APWM mode bit needs to be > set in .enable() > > Now, considering .config() followed by .enable(). > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > mode bit to be set. > > The same can be done in .enable() also. However, we again need to pass > the pwm parameters (period & duty cycle values) to the .enable(). > We don't want to duplicate this parameter passing. > To avoid this we enable the APWM mode bit in both .config() & .enable(). That's weird. I assumed that the values written to the shadow registers would automatically be copied to the active registers on each new PWM period. If I understand correctly what you're saying, however, the eCAP requires the APWM bit to be set in order to write the shadow registers at all. > > > > > + pc->chip.dev = &pdev->dev; > > > > > + pc->chip.ops = &ecap_pwm_ops; > > > > > + pc->chip.base = -1; > > > > > + pc->chip.npwm = 1; > > > > > > > > The cover letter mentions that the AM335x has 3 instances of the ECAP. > > > > Is there any chance that they can be unified in one driver instance > > > > (i.e. .npwm = 3?). > > > > > > No. All instances have separate resources (clocks, memory ..). > > > > > > > > > > > > + > > > > > + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecap_reg"); > > > > > + if (r == NULL) { > > > > > > > > You use "if (!ptr)" everywhere else, maybe you should make this > > > > consistent? > > > Ok > > > > Also the platform_get_resource_byname() is really only > > > > useful for devices that have several register regions associated with > > > > them. I don't know your hardware in detail, but I doubt that a PWM > > > > device has more than one register region. > > > > > > In fact PWM Module has 4 different register space (eCAP, eHRPWM, eQEP & > > > Common Config space). So I need to use platform_get_resource_byname() > > > > Above you say that all instances have separate resources, so how come > > you need to specify 4 register spaces? The eCAP registers should clearly > > be passed to the eCAP device, while the eHRPWM registers should go to > > the eHRPWM device. > > > > My point is that if you need to refer to the register region by name, > > then the driver should clearly be using more than a single region. > > Neither the eCAP nor eHRPWM do that. > > On AM335x SoC, this common config space is shared by the other 3 > modules (eCAP, eHRPWM, eQEP). > However, on Davinci platform don't have any common config space. > > So from driver reusability view, usage of platform_get_resource_byname() > is better choice. I don't quite see how you would be able to reuse the driver in that case. The driver that you posted uses only one memory region, so the platform device that you instantiate for the driver to bind to only gets the one region through the .resource and .num_resources fields. How is that going to work? Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/62e6b9c8/attachment.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-23 13:37 ` Thierry Reding @ 2012-07-24 7:52 ` Philip, Avinash 2012-07-24 8:07 ` Thierry Reding 0 siblings, 1 reply; 14+ messages in thread From: Philip, Avinash @ 2012-07-24 7:52 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > + /* > > > > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > > > > + * and 'APWM mode' to enable APWM output > > > > > > + */ > > > > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > > > > clear in .config() is the better option. > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > in idle state). > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > should be set. > > > > The same is done in .config(). > > > > > > My point was that if you do it in .enable(), then why do you still set > > > it in .config()? Since the sequence is typically .config() followed by > > > .enable(), setting the bit in both seems redundant. It should be enough > > > to load the registers during .enable(), right? > > > > Consider scenarios where .enable() can be called without calling .config(). > > Example I just need to stop the pwm signal momentarily and re-enable. > > In this case, .config() need not be called. So, APWM mode bit needs to be > > set in .enable() > > > > Now, considering .config() followed by .enable(). > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > mode bit to be set. > > > > The same can be done in .enable() also. However, we again need to pass > > the pwm parameters (period & duty cycle values) to the .enable(). > > We don't want to duplicate this parameter passing. > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > That's weird. I assumed that the values written to the shadow registers > would automatically be copied to the active registers on each new PWM > period. This is correct in case if PWM is running. > If I understand correctly what you're saying, however, the eCAP > requires the APWM bit to be set in order to write the shadow registers > at all. In APWM mode, writing to CAP1/CAP2 (active registers) will also write the same value to the corresponding CAP3/CAP4 (shadow registers). This emulates immediate mode. Writing directly to the shadow registers CAP3/CAP4 will invoke the shadow mode. If PWM is not running, cycle & duty values written to active registers, not to shadow registers. To copy these value to shadow registers APWM mode to be set. This way reloading from shadow registers can be ensured on next PWM period/cycle. If PWM is running, cycle & duty values written to shadow registers. So that it won't disturb current PWM period. On the next PWM period, new values will be reloaded from shadow registers to active registers. So APWM mode to be set to copy shadow registers from active registers (immediate mode). > [snip] > > > My point is that if you need to refer to the register region by name, > > > then the driver should clearly be using more than a single region. > > > Neither the eCAP nor eHRPWM do that. > > > > On AM335x SoC, this common config space is shared by the other 3 > > modules (eCAP, eHRPWM, eQEP). > > However, on Davinci platform don't have any common config space. > > > > So from driver reusability view, usage of platform_get_resource_byname() > > is better choice. > > I don't quite see how you would be able to reuse the driver in that > case. The driver that you posted uses only one memory region, so the > platform device that you instantiate for the driver to bind to only > gets the one region through the .resource and .num_resources fields. > > How is that going to work? I understand now. I will use platform_get_resource(). Thanks Avinash > > Thierry > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-24 7:52 ` Philip, Avinash @ 2012-07-24 8:07 ` Thierry Reding 2012-07-24 8:36 ` Philip, Avinash 0 siblings, 1 reply; 14+ messages in thread From: Thierry Reding @ 2012-07-24 8:07 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote: > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > > + /* > > > > > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > > > > > + * and 'APWM mode' to enable APWM output > > > > > > > + */ > > > > > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > > > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > > > > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > > > > > clear in .config() is the better option. > > > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > > in idle state). > > > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > > should be set. > > > > > The same is done in .config(). > > > > > > > > My point was that if you do it in .enable(), then why do you still set > > > > it in .config()? Since the sequence is typically .config() followed by > > > > .enable(), setting the bit in both seems redundant. It should be enough > > > > to load the registers during .enable(), right? > > > > > > Consider scenarios where .enable() can be called without calling .config(). > > > Example I just need to stop the pwm signal momentarily and re-enable. > > > In this case, .config() need not be called. So, APWM mode bit needs to be > > > set in .enable() > > > > > > Now, considering .config() followed by .enable(). > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > > mode bit to be set. > > > > > > The same can be done in .enable() also. However, we again need to pass > > > the pwm parameters (period & duty cycle values) to the .enable(). > > > We don't want to duplicate this parameter passing. > > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > That's weird. I assumed that the values written to the shadow registers > > would automatically be copied to the active registers on each new PWM > > period. > > This is correct in case if PWM is running. > > > If I understand correctly what you're saying, however, the eCAP > > requires the APWM bit to be set in order to write the shadow registers > > at all. > > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates > immediate mode. > > Writing directly to the shadow registers CAP3/CAP4 will invoke the > shadow mode. > > If PWM is not running, cycle & duty values written to active registers, not > to shadow registers. To copy these value to shadow registers APWM mode to be > set. This way reloading from shadow registers can be ensured on next PWM > period/cycle. I think this is the part I don't understand. If you wrote cycle and duty values to the active registers already, then the shadow registers should be ignored by the hardware. There should be no need to reload the active registers. > If PWM is running, cycle & duty values written to shadow registers. So that > it won't disturb current PWM period. On the next PWM period, new values will > be reloaded from shadow registers to active registers. > > So APWM mode to be set to copy shadow registers from active registers > (immediate mode). I realize that I'm just talking from a general understanding of shadow registers and maybe you're hardware doesn't work quite that way. If this is really the only way that you can make the hardware work then I will no longer object. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120724/41985e23/attachment-0001.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-24 8:07 ` Thierry Reding @ 2012-07-24 8:36 ` Philip, Avinash 2012-07-24 8:48 ` Thierry Reding 0 siblings, 1 reply; 14+ messages in thread From: Philip, Avinash @ 2012-07-24 8:36 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 24, 2012 at 13:37:24, Thierry Reding wrote: > On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote: > > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > > > + /* > > > > > > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > > > > > > + * and 'APWM mode' to enable APWM output > > > > > > > > + */ > > > > > > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > > > > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > > > > > > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > > > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > > > > > > clear in .config() is the better option. > > > > > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > > > in idle state). > > > > > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > > > should be set. > > > > > > The same is done in .config(). > > > > > > > > > > My point was that if you do it in .enable(), then why do you still set > > > > > it in .config()? Since the sequence is typically .config() followed by > > > > > .enable(), setting the bit in both seems redundant. It should be enough > > > > > to load the registers during .enable(), right? > > > > > > > > Consider scenarios where .enable() can be called without calling .config(). > > > > Example I just need to stop the pwm signal momentarily and re-enable. > > > > In this case, .config() need not be called. So, APWM mode bit needs to be > > > > set in .enable() > > > > > > > > Now, considering .config() followed by .enable(). > > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > > > mode bit to be set. > > > > > > > > The same can be done in .enable() also. However, we again need to pass > > > > the pwm parameters (period & duty cycle values) to the .enable(). > > > > We don't want to duplicate this parameter passing. > > > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > > > That's weird. I assumed that the values written to the shadow registers > > > would automatically be copied to the active registers on each new PWM > > > period. > > > > This is correct in case if PWM is running. > > > > > If I understand correctly what you're saying, however, the eCAP > > > requires the APWM bit to be set in order to write the shadow registers > > > at all. > > > > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the > > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates > > immediate mode. > > > > Writing directly to the shadow registers CAP3/CAP4 will invoke the > > shadow mode. > > > > If PWM is not running, cycle & duty values written to active registers, not > > to shadow registers. To copy these value to shadow registers APWM mode to be > > set. This way reloading from shadow registers can be ensured on next PWM > > period/cycle. > > I think this is the part I don't understand. If you wrote cycle and duty > values to the active registers already, then the shadow registers should > be ignored by the hardware. There should be no need to reload the active > registers. ECAP PWM hardware always loads active registers from shadow registers at counter = period value (starting of next period). So on every next cycle active registers being updated from shadow registers. So shadow registers acting as a backup. > > > If PWM is running, cycle & duty values written to shadow registers. So that > > it won't disturb current PWM period. On the next PWM period, new values will > > be reloaded from shadow registers to active registers. > > > > So APWM mode to be set to copy shadow registers from active registers > > (immediate mode). > > I realize that I'm just talking from a general understanding of shadow > registers and maybe you're hardware doesn't work quite that way. If this > is really the only way that you can make the hardware work then I will > no longer object. Ok. Thanks Avinash > > Thierry > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM. 2012-07-24 8:36 ` Philip, Avinash @ 2012-07-24 8:48 ` Thierry Reding 0 siblings, 0 replies; 14+ messages in thread From: Thierry Reding @ 2012-07-24 8:48 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jul 24, 2012 at 08:36:08AM +0000, Philip, Avinash wrote: > On Tue, Jul 24, 2012 at 13:37:24, Thierry Reding wrote: > > On Tue, Jul 24, 2012 at 07:52:06AM +0000, Philip, Avinash wrote: > > > On Mon, Jul 23, 2012 at 19:07:04, Thierry Reding wrote: > > > > On Mon, Jul 23, 2012 at 12:44:38PM +0000, Philip, Avinash wrote: > > > > > On Mon, Jul 23, 2012 at 14:52:04, Thierry Reding wrote: > > > > > > On Mon, Jul 23, 2012 at 09:10:09AM +0000, Philip, Avinash wrote: > > > > > > > On Mon, Jul 23, 2012 at 12:22:35, Thierry Reding wrote: > > > > > > > > On Fri, Jul 13, 2012 at 03:05:01PM +0530, Philip, Avinash wrote: > > > > > > > > > + /* > > > > > > > > > + * Enable 'Free run Time stamp counter mode' to start counter > > > > > > > > > + * and 'APWM mode' to enable APWM output > > > > > > > > > + */ > > > > > > > > > + reg_val = readw(pc->mmio_base + ECCTL2); > > > > > > > > > + reg_val |= ECCTL2_TSCTR_FREERUN | ECCTL2_APWM_MODE; > > > > > > > > > > > > > > > > You already set the APWM_MODE bit in .config(), why is it needed here > > > > > > > > again? Seeing that .disable() clears the bit as well, perhaps leaving it > > > > > > > > clear in .config() is the better option. > > > > > > > > > > > > > > Clearing APWM mode in .disable() ensures PWM low output (i.e., PWM pin = low > > > > > > > in idle state). > > > > > > > > > > > > > > The Period & Compare registers are updated from Shadow registers (CAP1 & CAP2) > > > > > > > During PWM configuration. To enable loading from Shadow registers, APWM mode > > > > > > > should be set. > > > > > > > The same is done in .config(). > > > > > > > > > > > > My point was that if you do it in .enable(), then why do you still set > > > > > > it in .config()? Since the sequence is typically .config() followed by > > > > > > .enable(), setting the bit in both seems redundant. It should be enough > > > > > > to load the registers during .enable(), right? > > > > > > > > > > Consider scenarios where .enable() can be called without calling .config(). > > > > > Example I just need to stop the pwm signal momentarily and re-enable. > > > > > In this case, .config() need not be called. So, APWM mode bit needs to be > > > > > set in .enable() > > > > > > > > > > Now, considering .config() followed by .enable(). > > > > > Currently in PWM driver, the CAP1 & CAP2 registers is copied to shadow > > > > > Registers (CAP3 & CAP4) by hardwaare in .config(). This requires APWM > > > > > mode bit to be set. > > > > > > > > > > The same can be done in .enable() also. However, we again need to pass > > > > > the pwm parameters (period & duty cycle values) to the .enable(). > > > > > We don't want to duplicate this parameter passing. > > > > > To avoid this we enable the APWM mode bit in both .config() & .enable(). > > > > > > > > That's weird. I assumed that the values written to the shadow registers > > > > would automatically be copied to the active registers on each new PWM > > > > period. > > > > > > This is correct in case if PWM is running. > > > > > > > If I understand correctly what you're saying, however, the eCAP > > > > requires the APWM bit to be set in order to write the shadow registers > > > > at all. > > > > > > In APWM mode, writing to CAP1/CAP2 (active registers) will also write the > > > same value to the corresponding CAP3/CAP4 (shadow registers). This emulates > > > immediate mode. > > > > > > Writing directly to the shadow registers CAP3/CAP4 will invoke the > > > shadow mode. > > > > > > If PWM is not running, cycle & duty values written to active registers, not > > > to shadow registers. To copy these value to shadow registers APWM mode to be > > > set. This way reloading from shadow registers can be ensured on next PWM > > > period/cycle. > > > > I think this is the part I don't understand. If you wrote cycle and duty > > values to the active registers already, then the shadow registers should > > be ignored by the hardware. There should be no need to reload the active > > registers. > > ECAP PWM hardware always loads active registers from shadow registers at > counter = period value (starting of next period). So on every next cycle > active registers being updated from shadow registers. So shadow registers > acting as a backup. Okay, in that case I don't see any other option. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120724/55639ece/attachment.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM. 2012-07-13 9:35 [PATCH 0/2] PWM driver support for EHRPWM & ECAP Philip, Avinash 2012-07-13 9:35 ` [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM Philip, Avinash @ 2012-07-13 9:35 ` Philip, Avinash 2012-07-23 7:42 ` Thierry Reding 1 sibling, 1 reply; 14+ messages in thread From: Philip, Avinash @ 2012-07-13 9:35 UTC (permalink / raw) To: linux-arm-kernel Enhanced high resolution PWM module (EHRPWM) hardware can be used to generate PWM output over 2 channels. This commit adds PWM driver support for EHRPWM device present on AM33XX SOC. Current implementation supports simple PWM functionality. Signed-off-by: Philip, Avinash <avinashphilip@ti.com> Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com> --- :100644 100644 f20b8f2... ad62d7a... M drivers/pwm/Kconfig :100644 100644 7dd90ec... 636a1d6... M drivers/pwm/Makefile :000000 100644 0000000... 985d334... A drivers/pwm/pwm-ehrpwm.c drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-ehrpwm.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 487 insertions(+), 0 deletions(-) diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index f20b8f2..ad62d7a 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -95,4 +95,14 @@ config PWM_ECAP To compile this driver as a module, choose M here: the module will be called pwm_ecap. +config PWM_EHRPWM + tristate "EHRPWM PWM support" + depends on SOC_AM33XX + help + PWM driver support for the EHRPWM controller found on AM33XX + TI SOC + + To compile this driver as a module, choose M here: the module + will be called pwm-ehrpwm. + endif diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 7dd90ec..636a1d6 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -8,3 +8,4 @@ obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o obj-$(CONFIG_PWM_ECAP) += pwm-ecap.o +obj-$(CONFIG_PWM_EHRPWM) += pwm-ehrpwm.o diff --git a/drivers/pwm/pwm-ehrpwm.c b/drivers/pwm/pwm-ehrpwm.c new file mode 100644 index 0000000..985d334 --- /dev/null +++ b/drivers/pwm/pwm-ehrpwm.c @@ -0,0 +1,476 @@ +/* + * EHRPWM PWM driver + * + * Copyright (C) 2012 Texas Instruments, Inc. - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, 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., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/io.h> +#include <linux/err.h> +#include <linux/clk.h> +#include <linux/pm_runtime.h> + +/* EHRPWM registers and bits definitions */ + +/* Time base module registers */ +#define TBCTL 0x00 +#define TBSTS 0x02 +#define TBPHS 0x06 +#define TBCNT 0x08 +#define TBPRD 0x0A + +#define TBCTL_RUN_MASK (BIT(15) | BIT(14)) +#define TBCTL_STOP_NEXT 0 +#define TBCTL_STOP_ON_CYCLE BIT(14) +#define TBCTL_FREE_RUN (BIT(15) | BIT(14)) +#define TBCTL_PRDLD_MASK BIT(3) +#define TBCTL_PRDLD_SHDW 0 +#define TBCTL_PRDLD_IMDT BIT(3) +#define TBCTL_CTRMODE_MASK (BIT(1) | BIT(0)) +#define TBCTL_CTRMODE_UP 0 +#define TBCTL_CTRMODE_DOWN BIT(0) +#define TBCTL_CTRMODE_UPDOWN BIT(1) +#define TBCTL_CLKDIV_MASK (BIT(12) | BIT(11) | BIT(10) | BIT(9) | \ + BIT(8) | BIT(7)) +#define TBCTL_CTRMODE_DOWN BIT(0) +#define TBCTL_CTRMODE_UPDOWN BIT(1) +#define TBCTL_CTRMODE_FREEZE (BIT(1) | BIT(0)) +#define TBCTL_HSPCLKDIV_SHIFT 7 +#define TBCTL_CLKDIV_SHIFT 10 + +#define CLKDIV_MAX 7 +#define HSPCLKDIV_MAX 7 +#define PERIOD_MAX 0xFFFF + +/* Compare module registers */ +#define CMPCTL 0x0E +#define CMPA 0x12 +#define CMPB 0x14 + +/* Action qualifier module registers */ +#define AQCTLA 0x16 +#define AQCTLB 0x18 +#define AQSFRC 0x1A +#define AQCSFRC 0x1C + +#define AQCTL_CBD_MASK (BIT(11) | BIT(10)) +#define AQCTL_CBD_FRCLOW BIT(10) +#define AQCTL_CBD_FRCHIGH BIT(11) +#define AQCTL_CBD_FRCTOGGLE (BIT(11) | BIT(10)) +#define AQCTL_CBU_MASK (BIT(9) | BIT(8)) +#define AQCTL_CBU_FRCLOW BIT(8) +#define AQCTL_CBU_FRCHIGH BIT(9) +#define AQCTL_CBU_FRCTOGGLE (BIT(9) | BIT(8)) +#define AQCTL_CAD_MASK (BIT(7) | BIT(6)) +#define AQCTL_CAD_FRCLOW BIT(6) +#define AQCTL_CAD_FRCHIGH BIT(7) +#define AQCTL_CAD_FRCTOGGLE (BIT(7) | BIT(6)) +#define AQCTL_CAU_MASK (BIT(5) | BIT(4)) +#define AQCTL_CAU_FRCLOW BIT(4) +#define AQCTL_CAU_FRCHIGH BIT(5) +#define AQCTL_CAU_FRCTOGGLE (BIT(5) | BIT(4)) +#define AQCTL_PRD_MASK (BIT(3) | BIT(2)) +#define AQCTL_PRD_FRCLOW BIT(2) +#define AQCTL_PRD_FRCHIGH BIT(3) +#define AQCTL_PRD_FRCTOGGLE (BIT(3) | BIT(2)) +#define AQCTL_ZRO_MASK (BIT(1) | BIT(0)) +#define AQCTL_ZRO_FRCLOW BIT(0) +#define AQCTL_ZRO_FRCHIGH BIT(1) +#define AQCTL_ZRO_FRCTOGGLE (BIT(1) | BIT(0)) + +#define AQSFRC_RLDCSF_MASK (BIT(7) | BIT(6)) +#define AQSFRC_RLDCSF_ZRO 0 +#define AQSFRC_RLDCSF_PRD BIT(6) +#define AQSFRC_RLDCSF_ZROPRD BIT(7) +#define AQSFRC_RLDCSF_IMDT (BIT(7) | BIT(6)) +#define AQSFRC_OTSFB_MASK BIT(5) +#define AQSFRC_OTSFB_FRC1 BIT(5) +#define AQSFRC_ACTSFB_MASK (BIT(3) | BIT(4)) +#define AQSFRC_ACTSFB_NOTH 0 +#define AQSFRC_ACTSFB_CLR BIT(3) +#define AQSFRC_ACTSFB_SET BIT(4) +#define AQSFRC_ACTSFB_TGL (BIT(4) | BIT(3)) +#define AQSFRC_OTSFA_MASK BIT(2) +#define AQSFRC_OTSFA_FRC1 BIT(2) +#define AQSFRC_ACTSFA_MASK (BIT(1) | BIT(0)) +#define AQSFRC_ACTSFA_NOTH 0 +#define AQSFRC_ACTSFA_CLR BIT(0) +#define AQSFRC_ACTSFA_SET BIT(1) +#define AQSFRC_ACTSFA_TGL (BIT(1) | BIT(0)) + +#define AQCSFRC_CSFB_MASK (BIT(3) | BIT(2)) +#define AQCSFRC_CSFB_FRCDIS 0 +#define AQCSFRC_CSFB_FRCLOW BIT(2) +#define AQCSFRC_CSFB_FRCHIGH BIT(3) +#define AQCSFRC_CSFB_DISSWFRC (BIT(3) | BIT(2)) +#define AQCSFRC_CSFA_MASK (BIT(1) | BIT(0)) +#define AQCSFRC_CSFA_FRCDIS 0 +#define AQCSFRC_CSFA_FRCLOW BIT(0) +#define AQCSFRC_CSFA_FRCHIGH BIT(1) +#define AQCSFRC_CSFA_DISSWFRC (BIT(1) | BIT(0)) + +/* Dead band module registers */ +#define DBCTL 0x1E +#define DBRED 0x20 +#define DBFED 0x22 + +/* Trip zone module registers */ +#define TZSEL 0x24 +#define TZCTL 0x28 +#define TZEINT 0x2A +#define TZFLG 0x2C +#define TZCLR 0x2E +#define TZFRC 0x30 + +/* Event trigger module registers */ +#define ETSEL 0x32 +#define ETPS 0x34 +#define ETFLG 0x36 +#define ETCLR 0x38 +#define ETFRC 0x3A +#define PCCTL 0x3C + +/* High resolution module registers */ +#define TBPHSHR 0x04 +#define CMPAHR 0x10 +#define HRCTL 0x40 + +#define PWM_CHANNEL 2 /* EHRPWM channels */ + +#define DRIVER_NAME "ehrpwm" + +struct ehrpwm_pwm_chip { + struct pwm_chip chip; + unsigned int clk_rate; + void __iomem *mmio_base; + int pwm_period_ns; + int pwm_duty_ns; +}; + +static inline struct ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip) +{ + return container_of(chip, struct ehrpwm_pwm_chip, chip); +} + +static void ehrpwm_write(void *base, int offset, unsigned long val) +{ + writew(val & 0xffff, base + offset); +} + +static void ehrpwm_modify(void *base, int offset, unsigned short mask, + unsigned short val) +{ + unsigned short regval; + + regval = readw(base + offset); + regval &= ~mask; + regval |= val & mask; + writew(regval, base + offset); +} + +/** + * set_prescale_div - Set up the prescaler divider function + * @rqst_prescaler: prescaler value min + * @prescale_div: prescaler value set + * @tb_clk_div: Time Base Control prescaler bits + */ +static int set_prescale_div(unsigned long rqst_prescaler, + unsigned short *prescale_div, unsigned short *tb_clk_div) +{ + unsigned int clkdiv, hspclkdiv; + + for (clkdiv = 0; clkdiv <= CLKDIV_MAX; clkdiv++) { + for (hspclkdiv = 0; hspclkdiv <= HSPCLKDIV_MAX; hspclkdiv++) { + + /* + * calculations for prescaler value : + * prescale_div = HSPCLKDIVIDER * CLKDIVIDER. + * HSPCLKDIVIDER = 2 ** hspclkdiv + * CLKDIVIDER = (1), if clkdiv == 0 *OR* + * (2 * clkdiv), if clkdiv != 0 + * + * Configure prescale_div value such that period + * register value is less than 65535. + */ + + *prescale_div = (1 << clkdiv) * + (hspclkdiv ? (hspclkdiv * 2) : 1); + if (*prescale_div > rqst_prescaler) { + *tb_clk_div = (clkdiv << TBCTL_CLKDIV_SHIFT) | + (hspclkdiv << TBCTL_HSPCLKDIV_SHIFT); + return 0; + } + } + } + return 1; +} + +static void configure_chans(struct ehrpwm_pwm_chip *pc, int chan, + unsigned long duty_cycles) +{ + int cmp_reg, aqctl_reg; + unsigned short aqctl_val, aqctl_mask; + + /* + * Channels can be configured from action qualifier module. + * Channel 0 configured with compare A register and for + * up-counter mode. + * Channel 1 configured with compare B register and for + * up-counter mode. + */ + if (chan == 1) { + aqctl_reg = AQCTLB; + cmp_reg = CMPB; + /* Configure PWM Low from compare B value */ + aqctl_val = AQCTL_CBU_FRCLOW; + aqctl_mask = AQCTL_CBU_MASK; + } else { + cmp_reg = CMPA; + aqctl_reg = AQCTLA; + /* Configure PWM Low from compare A value */ + aqctl_val = AQCTL_CAU_FRCLOW; + aqctl_mask = AQCTL_CAU_MASK; + } + + /* Configure PWM High from period value and zero value */ + aqctl_val |= AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH; + aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK; + ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val); + + ehrpwm_write(pc->mmio_base, cmp_reg, duty_cycles); +} + +/* + * period_ns = 10^9 * (ps_divval * period_cycles) / PWM_CLK_RATE + * duty_ns = 10^9 * (ps_divval * duty_cycles) / PWM_CLK_RATE + */ +static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip); + unsigned long long c; + unsigned long period_cycles, duty_cycles; + unsigned short ps_divval, tb_divval, chan; + + if (period_ns < 0 || duty_ns < 0 || period_ns > NSEC_PER_SEC) + return -ERANGE; + + c = pc->clk_rate; + c = c * period_ns; + do_div(c, NSEC_PER_SEC); + period_cycles = (unsigned long)c; + + if (period_cycles < 1) { + period_cycles = 1; + duty_cycles = 1; + } else { + c = pc->clk_rate; + c = c * duty_ns; + do_div(c, NSEC_PER_SEC); + duty_cycles = (unsigned long)c; + } + + pc->pwm_duty_ns = duty_ns; + pc->pwm_period_ns = period_ns; + + /* Configure clock prescaler to support Low frequency PWM wave */ + if (set_prescale_div(period_cycles/PERIOD_MAX, &ps_divval, + &tb_divval)) { + dev_err(chip->dev, "Unsupported values\n"); + return -EINVAL; + } + + pm_runtime_get_sync(chip->dev); + + /* Configure shadow loading on Period register */ + ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_PRDLD_MASK, TBCTL_PRDLD_SHDW); + + /* Update clock prescaler values */ + ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CLKDIV_MASK, tb_divval); + + /* Update period & duty cycle with presacler division */ + period_cycles = period_cycles / ps_divval; + duty_cycles = duty_cycles / ps_divval; + + ehrpwm_write(pc->mmio_base, TBPRD, period_cycles); + + /* Configure ehrpwm counter for up-count mode */ + ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK, + TBCTL_CTRMODE_UP); + + /* Configure the channel for duty cycle */ + chan = pwm->hwpwm; + configure_chans(pc, chan, duty_cycles); + pm_runtime_put_sync(chip->dev); + return 0; +} + +static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip); + unsigned short aqcsfrc_val, aqcsfrc_mask; + + /* Leave clock enabled on enabling PWM */ + pm_runtime_get_sync(chip->dev); + + /* Disabling Action Qualifier on PWM output */ + if (pwm->hwpwm) { + aqcsfrc_val = AQCSFRC_CSFB_FRCDIS; + aqcsfrc_mask = AQCSFRC_CSFB_MASK; + } else { + aqcsfrc_val = AQCSFRC_CSFA_FRCDIS; + aqcsfrc_mask = AQCSFRC_CSFA_MASK; + } + + /* Changes to shadow mode */ + ehrpwm_modify(pc->mmio_base, AQSFRC, AQSFRC_RLDCSF_MASK, + AQSFRC_RLDCSF_ZRO); + + ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val); + + /* Enable time counter for free_run */ + ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_FREE_RUN); + return 0; +} + +static void ehrpwm_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip); + unsigned short aqcsfrc_val, aqcsfrc_mask; + + /* Action Qualifier puts PWM output low forcefully */ + if (pwm->hwpwm) { + aqcsfrc_val = AQCSFRC_CSFB_FRCLOW; + aqcsfrc_mask = AQCSFRC_CSFB_MASK; + } else { + aqcsfrc_val = AQCSFRC_CSFA_FRCLOW; + aqcsfrc_mask = AQCSFRC_CSFA_MASK; + } + + /* + * Changes to immediate action on Action Qualifier. This puts + * Action Qualifier control on PWM output from next TBCLK + */ + ehrpwm_modify(pc->mmio_base, AQSFRC, AQSFRC_RLDCSF_MASK, + AQSFRC_RLDCSF_IMDT); + + ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val); + + /* Stop Time base counter */ + ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_STOP_NEXT); + + /* Disable clock on PWM disable */ + pm_runtime_put_sync(chip->dev); +} + +static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + if (test_bit(PWMF_ENABLED, &pwm->flags)) { + dev_warn(chip->dev, "Removing PWM device without disabling\n"); + pm_runtime_put_sync(chip->dev); + } +} + +static struct pwm_ops ehrpwm_pwm_ops = { + .free = ehrpwm_pwm_free, + .config = ehrpwm_pwm_config, + .enable = ehrpwm_pwm_enable, + .disable = ehrpwm_pwm_disable, + .owner = THIS_MODULE, +}; + +static int __devinit ehrpwm_pwm_probe(struct platform_device *pdev) +{ + int ret; + struct resource *r; + struct clk *clk; + struct ehrpwm_pwm_chip *pc; + + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); + if (!pc) { + dev_err(&pdev->dev, "failed to allocate memory\n"); + return -ENOMEM; + } + + clk = devm_clk_get(&pdev->dev, "fck"); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "failed to get clock\n"); + return PTR_ERR(clk); + } + + pc->clk_rate = clk_get_rate(clk); + if (!pc->clk_rate) { + dev_err(&pdev->dev, "failed to get clock rate\n"); + return -EINVAL; + } + + pc->chip.dev = &pdev->dev; + pc->chip.ops = &ehrpwm_pwm_ops; + pc->chip.base = -1; + pc->chip.npwm = PWM_CHANNEL; + + r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ehrpwm_reg"); + if (r == NULL) { + dev_err(&pdev->dev, "no memory resource defined\n"); + return -ENODEV; + } + + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r); + if (!pc->mmio_base) { + dev_err(&pdev->dev, "failed to ioremap() registers\n"); + return -EADDRNOTAVAIL; + } + + ret = pwmchip_add(&pc->chip); + if (ret < 0) { + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); + return ret; + } + + pm_runtime_enable(&pdev->dev); + platform_set_drvdata(pdev, pc); + dev_info(&pdev->dev, "PWM device initialized\n"); + return 0; +} + +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev) +{ + struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev); + + if (WARN_ON(!pc)) + return -ENODEV; + + pm_runtime_disable(&pdev->dev); + pwmchip_remove(&pc->chip); + return 0; +} + +static struct platform_driver ehrpwm_pwm_driver = { + .driver = { + .name = DRIVER_NAME, + }, + .probe = ehrpwm_pwm_probe, + .remove = __devexit_p(ehrpwm_pwm_remove), +}; + +module_platform_driver(ehrpwm_pwm_driver); + +MODULE_DESCRIPTION("EHRPWM PWM driver"); +MODULE_AUTHOR("Texas Instruments"); +MODULE_LICENSE("GPL"); -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM. 2012-07-13 9:35 ` [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM Philip, Avinash @ 2012-07-23 7:42 ` Thierry Reding 2012-07-23 9:16 ` Philip, Avinash 0 siblings, 1 reply; 14+ messages in thread From: Thierry Reding @ 2012-07-23 7:42 UTC (permalink / raw) To: linux-arm-kernel On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote: > Enhanced high resolution PWM module (EHRPWM) hardware can be used to > generate PWM output over 2 channels. This commit adds PWM driver support > for EHRPWM device present on AM33XX SOC. Current implementation supports > simple PWM functionality. > > Signed-off-by: Philip, Avinash <avinashphilip@ti.com> > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com> So this driver is very similar to the ECAP one and pretty much all the comments apply to this as well. Some additional comments below. > --- > :100644 100644 f20b8f2... ad62d7a... M drivers/pwm/Kconfig > :100644 100644 7dd90ec... 636a1d6... M drivers/pwm/Makefile > :000000 100644 0000000... 985d334... A drivers/pwm/pwm-ehrpwm.c > drivers/pwm/Kconfig | 10 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-ehrpwm.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 487 insertions(+), 0 deletions(-) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index f20b8f2..ad62d7a 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -95,4 +95,14 @@ config PWM_ECAP > To compile this driver as a module, choose M here: the module > will be called pwm_ecap. > > +config PWM_EHRPWM > + tristate "EHRPWM PWM support" > + depends on SOC_AM33XX > + help > + PWM driver support for the EHRPWM controller found on AM33XX > + TI SOC > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-ehrpwm. > + Maybe it would be useful to prefix the names with AM33XX here. ECAP and EHRPWM are sort of generic and may have name clashes in the future. > +#define PWM_CHANNEL 2 /* EHRPWM channels */ I'd say you can just replace the one occurrence of this with the literal 2. If you still want to have the symbolic name, then I'd suggest to call it something like NUM_PWM_CHANNELS to make its meaning more obvious. > +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev) > +{ > + struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev); > + > + if (WARN_ON(!pc)) > + return -ENODEV; > + > + pm_runtime_disable(&pdev->dev); > + pwmchip_remove(&pc->chip); > + return 0; > +} I forgot to mention this for ECAP, but you need to check the return value of pwmchip_remove() because there are situations where it can actually fail. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120723/9ecdce34/attachment.sig> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM. 2012-07-23 7:42 ` Thierry Reding @ 2012-07-23 9:16 ` Philip, Avinash 0 siblings, 0 replies; 14+ messages in thread From: Philip, Avinash @ 2012-07-23 9:16 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 23, 2012 at 13:12:21, Thierry Reding wrote: > On Fri, Jul 13, 2012 at 03:05:02PM +0530, Philip, Avinash wrote: > > Enhanced high resolution PWM module (EHRPWM) hardware can be used to > > generate PWM output over 2 channels. This commit adds PWM driver support > > for EHRPWM device present on AM33XX SOC. Current implementation supports > > simple PWM functionality. > > > > Signed-off-by: Philip, Avinash <avinashphilip@ti.com> > > Reviewed-by: Vaibhav Bedia <vaibhav.bedia@ti.com> > > So this driver is very similar to the ECAP one and pretty much all the > comments apply to this as well. Some additional comments below. Ok I will correct it for all the comments. > > > --- [snip] > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-ehrpwm. > > + > > Maybe it would be useful to prefix the names with AM33XX here. ECAP and > EHRPWM are sort of generic and may have name clashes in the future. Ok, I will make us TI prefix as davinci platform also uses same modules. > > > +#define PWM_CHANNEL 2 /* EHRPWM channels */ > > I'd say you can just replace the one occurrence of this with the literal > 2. If you still want to have the symbolic name, then I'd suggest to call > it something like NUM_PWM_CHANNELS to make its meaning more obvious. I will correct it as NUM_PWM_CHANNELS. > > > +static int __devexit ehrpwm_pwm_remove(struct platform_device *pdev) > > +{ > > + struct ehrpwm_pwm_chip *pc = platform_get_drvdata(pdev); > > + > > + if (WARN_ON(!pc)) > > + return -ENODEV; > > + > > + pm_runtime_disable(&pdev->dev); > > + pwmchip_remove(&pc->chip); > > + return 0; > > +} > > I forgot to mention this for ECAP, but you need to check the return > value of pwmchip_remove() because there are situations where it can > actually fail. I will correct it. Thanks Avinash > > Thierry > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-07-24 8:48 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-13 9:35 [PATCH 0/2] PWM driver support for EHRPWM & ECAP Philip, Avinash 2012-07-13 9:35 ` [PATCH 1/2] PWM: ECAP: PWM driver support for ECAP APWM Philip, Avinash 2012-07-23 6:52 ` Thierry Reding 2012-07-23 9:10 ` Philip, Avinash 2012-07-23 9:22 ` Thierry Reding 2012-07-23 12:44 ` Philip, Avinash 2012-07-23 13:37 ` Thierry Reding 2012-07-24 7:52 ` Philip, Avinash 2012-07-24 8:07 ` Thierry Reding 2012-07-24 8:36 ` Philip, Avinash 2012-07-24 8:48 ` Thierry Reding 2012-07-13 9:35 ` [PATCH 2/2] PWM: EHRPWM: PWM driver support for EHRPWM Philip, Avinash 2012-07-23 7:42 ` Thierry Reding 2012-07-23 9:16 ` Philip, Avinash
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).