* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2012-12-13 3:06 ` NeilBrown
0 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2012-12-13 3:06 UTC (permalink / raw)
To: Jon Hunter; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 14876 bytes --]
[Thierry: question for you near the end - thanks]
On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> Hi Neil,
>
> On 12/12/2012 02:24 AM, NeilBrown wrote:
> >
> >
> > This patch is based on an earlier patch by Grant Erickson
> > which provided pwm devices using the 'legacy' interface.
> >
> > This driver instead uses the new framework interface.
> >
> > Cc: Grant Erickson <marathon96@gmail.com>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..7df573a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -85,6 +85,15 @@ config PWM_MXS
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-mxs.
> >
> > +config PWM_OMAP
> > + tristate "OMAP pwm support"
> > + depends on ARCH_OMAP
>
> We should probably have depends on or selects OMAP_DM_TIMER here too.
Sounds sensible - fixed.
>
> > + help
> > + Generic PWM framework driver for OMAP
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-omap
> > +
> > config PWM_PUV3
> > tristate "PKUnity NetBook-0916 PWM support"
> > depends on ARCH_PUV3
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..f5d200d 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> > obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
> > +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
> > obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> > new file mode 100644
> > index 0000000..e3dbce3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-omap.c
> > @@ -0,0 +1,318 @@
> > +/*
> > + * Copyright (c) 2012 NeilBrown <neilb@suse.de>
> > + * Heavily based on earlier code which is:
> > + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
> > + *
> > + * Also based on pwm-samsung.c
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * Description:
> > + * This file is the core OMAP2/3 support for the generic, Linux
>
> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
> work for OMAP1-5.
>
Done.
> > + * PWM driver / controller, using the OMAP's dual-mode timers.
> > + *
> > + * The 'id' number for the device encodes the number of the dm timer
> > + * to use, and the polarity of the output.
> > + * lsb is '1' of active-high, and '0' for active low
> > + * remaining bit a timer number and need to be shifted down before use.
> > + */
> > +
> > +#define pr_fmt(fmt) "pwm-omap: " fmt
> > +
> > +#include <linux/export.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/pwm.h>
> > +#include <linux/module.h>
> > +
> > +#include <plat/dmtimer.h>
>
> This is going to be a problem for the single zImage work, because we
> cannot include any plat headers in driver code any more. Therefore,
> although it is not ideal, one way to handle this is pass function
> pointers to the various dmtimer APIs that are needed via the platform
> data. Painful I know ...
But that doesn't work with devicetree does it?
Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
It only included other things from include/linux, so it should be safe.
>
> > +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE
> > +
> > +struct omap_chip {
> > + struct platform_device *pdev;
> > +
> > + struct omap_dm_timer *dm_timer;
> > + unsigned int polarity;
> > + const char *label;
> > +
> > + unsigned int duty_ns, period_ns;
> > + struct pwm_chip chip;
> > +};
> > +
> > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
> > +
> > +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> > +
> > +/**
> > + * pwm_calc_value - determines the counter value for a clock rate and period.
> > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
> > + * counter value for.
> > + * @ns: The period, in nanoseconds, to computer the counter value for.
> > + *
> > + * Returns the PWM counter value for the specified clock rate and period.
> > + */
> > +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> > +{
> > + const unsigned long nanoseconds_per_second = 1000000000;
> > + int cycles;
> > + __u64 c;
> > +
> > + c = (__u64)clk_rate * ns;
> > + do_div(c, nanoseconds_per_second);
> > + cycles = c;
> > +
> > + return DM_TIMER_LOAD_MIN - cycles;
> > +}
> > +
> > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct omap_chip *omap = to_omap_chip(chip);
> > + int status = 0;
> > +
> > + /* Enable the counter--always--before attempting to write its
> > + * registers and then set the timer to its minimum load value to
> > + * ensure we get an overflow event right away once we start it.
> > + */
> > +
> > + omap_dm_timer_enable(omap->dm_timer);
> > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > + omap_dm_timer_start(omap->dm_timer);
> > + omap_dm_timer_disable(omap->dm_timer);
>
> Why not just use omap_dm_timer_load_start() here instead of the above 4
> APIs?
Because I didn't know about it. I do now :-)
>
> > +
> > + return status;
> > +}
> > +
> > +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct omap_chip *omap = to_omap_chip(chip);
> > +
> > + omap_dm_timer_stop(omap->dm_timer);
> > +}
> > +
> > +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > + int duty_ns, int period_ns)
> > +{
> > + struct omap_chip *omap = to_omap_chip(chip);
> > + int status = 0;
> > + const bool enable = true;
> > + const bool autoreload = true;
> > + const bool toggle = true;
> > + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
> > + int load_value, match_value;
> > + unsigned long clk_rate;
> > +
> > + dev_dbg(chip->dev,
> > + "duty cycle: %d, period %d\n",
> > + duty_ns, period_ns);
> > +
> > + if (omap->duty_ns == duty_ns &&
> > + omap->period_ns == period_ns)
> > + /* No change - don't cause any transients */
> > + return 0;
> > +
> > + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));
> > +
> > + /* Calculate the appropriate load and match values based on the
> > + * specified period and duty cycle. The load value determines the
> > + * cycle time and the match value determines the duty cycle.
> > + */
> > +
> > + load_value = pwm_calc_value(clk_rate, period_ns);
> > + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
> > +
> > + /* We MUST enable yet stop the associated dual-mode timer before
> > + * attempting to write its registers. Hopefully it is already
> > + * disabled, but call the (idempotent) pwm_disable just in case
> > + */
> > +
> > + pwm_disable(pwm);
> > +
> > + omap_dm_timer_enable(omap->dm_timer);
>
> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> will enable the timer. So this should not be necessary.
True. That is what you get for copying someone else's code and not
understanding it fully.
>
> > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> > +
> > + dev_dbg(chip->dev,
> > + "load value: %#08x (%d), "
> > + "match value: %#08x (%d)\n",
> > + load_value, load_value,
> > + match_value, match_value);
> > +
> > + omap_dm_timer_set_pwm(omap->dm_timer,
> > + !omap->polarity,
> > + toggle,
> > + trigger);
> > +
> > + /* Set the counter to generate an overflow event immediately. */
> > +
> > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > +
> > + /* Now that we're done configuring the dual-mode timer, disable it
> > + * again. We'll enable and start it later, when requested.
> > + */
> > +
> > + omap_dm_timer_disable(omap->dm_timer);
>
> Similarly the disable should not be needed here either.
>
> > + omap->duty_ns = duty_ns;
> > + omap->period_ns = period_ns;
> > +
> > + return status;
> > +}
> > +
> > +
> > +static struct pwm_ops omap_pwm_ops = {
> > + .enable = omap_pwm_enable,
> > + .disable= omap_pwm_disable,
> > + .config = omap_pwm_config,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +/**
> > + * omap_pwm_probe - check for the PWM and bind it to the driver.
> > + * @pdev: A pointer to the platform device node associated with the
> > + * PWM instance to be probed for driver binding.
> > + *
> > + * Returns 0 if the PWM instance was successfully bound to the driver;
> > + * otherwise, < 0 on error.
> > + */
> > +static int __devinit omap_pwm_probe(struct platform_device *pdev)
>
> I believe that __devinit is no longer required.
>
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct omap_chip *omap;
> > + int status = 0;
> > + unsigned int id = pdev->id;
> > + unsigned int timer = id >> 1; /* lsb is polarity */
> > +
> > + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
> > +
> > + if (omap == NULL) {
> > + dev_err(dev, "Could not allocate memory.\n");
> > + status = -ENOMEM;
> > + goto done;
> > + }
> > +
> > + /* Request the OMAP dual-mode timer that will be bound to and
> > + * associated with this generic PWM.
> > + */
> > +
> > + omap->dm_timer = omap_dm_timer_request_specific(timer);
>
> I would recommend that you use omap_dm_timer_request_by_cap() (new for
> v3.8 so you should be able to use once v3.8-rc1 is out) here to request
> a timer that supports the PWM output. The above function will not be
> supported when booting with device-tree.
I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
this now.
Would you object to the patch being submitted with the current call, then an
update when I do move on to 3.8?
However.... I may be misunderstanding something, but I want a timer to drive
a particular output pin - GPIO-57. And I thought that it could only be
driver by GPT11. So I need to explicitly request number 11 don't I?
>
> > +
> > + if (omap->dm_timer == NULL) {
> > + status = -EPROBE_DEFER;
> > + goto err_free;
> > + }
> > +
> > + /* Configure the source for the dual-mode timer backing this
> > + * generic PWM device. The clock source will ultimately determine
> > + * how small or large the PWM frequency can be.
> > + *
> > + * At some point, it's probably worth revisiting moving this to
> > + * the configure method and choosing either the slow- or
> > + * system-clock source as appropriate for the desired PWM period.
> > + */
> > +
> > + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
> > +
> > + /* Cache away other miscellaneous driver-private data and state
> > + * information and add the driver-private data to the platform
> > + * device.
> > + */
> > +
> > + omap->chip.dev = dev;
> > + omap->chip.ops = &omap_pwm_ops;
> > + omap->chip.base = -1;
> > + omap->chip.npwm = 1;
> > + omap->polarity = id & 1;
> > +
> > + status = pwmchip_add(&omap->chip);
> > + if (status < 0) {
> > + dev_err(dev, "failed to register pwm\n");
> > + omap_dm_timer_free(omap->dm_timer);
> > + goto err_free;
> > + }
> > +
> > + platform_set_drvdata(pdev, omap);
> > +
> > + status = 0;
> > + goto done;
> > +
> > + err_free:
> > + kfree(omap);
> > + done:
> > + return status;
> > +}
> > +
> > +/**
> > + * omap_pwm_remove - unbind the specified PWM platform device from the driver.
> > + * @pdev: A pointer to the platform device node associated with the
> > + * PWM instance to be unbound/removed.
> > + *
> > + * Returns 0 if the PWM was successfully removed as a platform device;
> > + * otherwise, < 0 on error.
> > + */
> > +static int __devexit omap_pwm_remove(struct platform_device *pdev)
>
> I believe that __devexit is no longer required.
>
> > +{
> > + struct omap_chip *omap = platform_get_drvdata(pdev);
> > + int status = 0;
> > +
> > + status = pwmchip_remove(&omap->chip);
> > + if (status < 0)
> > + goto done;
> > +
> > + omap_dm_timer_free(omap->dm_timer);
>
> Is it guaranteed that the timer will be disabled at this point?
Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
might not be disabled.
Thierry: should pwm_put do that, or do I need a 'free' function in my chip
ops to do that?
>
> > +
> > + kfree(omap);
> > +
> > + done:
> > + return status;
> > +}
> > +
> > +#if CONFIG_PM
> > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > + struct omap_chip *omap = platform_get_drvdata(pdev);
> > + /* No one preserve these values during suspend so reset them
> > + * Otherwise driver leaves PWM unconfigured if same values
> > + * passed to pwm_config
> > + */
> > + omap->period_ns = 0;
> > + omap->duty_ns = 0;
>
>
> Hmmm, looks like you are trying to force a reconfiguration after suspend
> if the same values are used. Is there an underlying problem here that
> you are trying to workaround?
I copied that from pwm-samsung.c.
The key question is: does a dmtimer preserve all register values over suspend.
If so, then I guess we don't need this.
If not, we do (because omap_pwm_config short circuits if it thinks the config
hasn't changed).
Maybe I should test and see - though as my backlight always blanks before
suspend that might not be straight forward...
>
> Please note that I am not familiar with the PWM sub-system to know how
> suspend-resume is typically handled and if this is normal or not.
>
> > + return 0;
> > +}
> > +#else
> > +#define omap_pwm_suspend NULL
> > +#endif
> > +
> > +static struct platform_driver omap_pwm_driver = {
> > + .driver.name = "omap-pwm",
> > + .driver.owner = THIS_MODULE,
> > + .probe = omap_pwm_probe,
> > + .remove = __devexit_p(omap_pwm_remove),
>
> I believe that __devexit_p is no longer required.
>
> Otherwise it looks good to me. Thanks for sending!
And thanks a lot for reading and reviewing!
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 3:06 ` NeilBrown
@ 2012-12-13 4:33 ` NeilBrown
-1 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2012-12-13 4:33 UTC (permalink / raw)
To: NeilBrown; +Cc: Jon Hunter, Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]
On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
> > > + omap_dm_timer_enable(omap->dm_timer);
> >
> > Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> > will enable the timer. So this should not be necessary.
>
> True. That is what you get for copying someone else's code and not
> understanding it fully.
However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
explicitly checks that it is already runtime-enabled.
Does that mean I don't need to call omap_dm_timer_write_counter here? Or
does it mean that I do need the enable/disable pair?
>
> >
> > > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> > > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> > > +
> > > + dev_dbg(chip->dev,
> > > + "load value: %#08x (%d), "
> > > + "match value: %#08x (%d)\n",
> > > + load_value, load_value,
> > > + match_value, match_value);
> > > +
> > > + omap_dm_timer_set_pwm(omap->dm_timer,
> > > + !omap->polarity,
> > > + toggle,
> > > + trigger);
> > > +
> > > + /* Set the counter to generate an overflow event immediately. */
> > > +
> > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > > +
> > > + /* Now that we're done configuring the dual-mode timer, disable it
> > > + * again. We'll enable and start it later, when requested.
> > > + */
> > > +
> > > + omap_dm_timer_disable(omap->dm_timer);
> >
> > Similarly the disable should not be needed here either.
> >
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2012-12-13 4:33 ` NeilBrown
0 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2012-12-13 4:33 UTC (permalink / raw)
To: NeilBrown; +Cc: Jon Hunter, Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]
On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
> > > + omap_dm_timer_enable(omap->dm_timer);
> >
> > Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> > will enable the timer. So this should not be necessary.
>
> True. That is what you get for copying someone else's code and not
> understanding it fully.
However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
explicitly checks that it is already runtime-enabled.
Does that mean I don't need to call omap_dm_timer_write_counter here? Or
does it mean that I do need the enable/disable pair?
>
> >
> > > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
> > > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
> > > +
> > > + dev_dbg(chip->dev,
> > > + "load value: %#08x (%d), "
> > > + "match value: %#08x (%d)\n",
> > > + load_value, load_value,
> > > + match_value, match_value);
> > > +
> > > + omap_dm_timer_set_pwm(omap->dm_timer,
> > > + !omap->polarity,
> > > + toggle,
> > > + trigger);
> > > +
> > > + /* Set the counter to generate an overflow event immediately. */
> > > +
> > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> > > +
> > > + /* Now that we're done configuring the dual-mode timer, disable it
> > > + * again. We'll enable and start it later, when requested.
> > > + */
> > > +
> > > + omap_dm_timer_disable(omap->dm_timer);
> >
> > Similarly the disable should not be needed here either.
> >
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 4:33 ` NeilBrown
@ 2012-12-13 17:42 ` Jon Hunter
-1 siblings, 0 replies; 27+ messages in thread
From: Jon Hunter @ 2012-12-13 17:42 UTC (permalink / raw)
To: NeilBrown; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
On 12/12/2012 10:33 PM, NeilBrown wrote:
> On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
>
>>>> + omap_dm_timer_enable(omap->dm_timer);
>>>
>>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
>>> will enable the timer. So this should not be necessary.
>>
>> True. That is what you get for copying someone else's code and not
>> understanding it fully.
>
> However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
> explicitly checks that it is already runtime-enabled.
>
> Does that mean I don't need to call omap_dm_timer_write_counter here? Or
> does it mean that I do need the enable/disable pair?
Typically, omap_dm_timer_write_counter() is used to update the counter
value while the counter is running and hence is enabled.
Looking at the code, some more I now see what they are trying to do. It
seems that they are trying to force an overflow to occur as soon as they
enable the timer. This will cause the timer to load the count value from
the timer load register into the timer counter register. So that does
make sense to me. However, this should not be necessary as
omap_dm_timer_set_load should do this for you. Therefore, I think that
you could accomplish the same thing by doing ...
omap_pwm_config
--> omap_dm_timer_set_load()
--> omap_dm_timer_set_match()
--> omap_dm_timer_set_pwm()
omap_pwm_enable
--> omap_dm_timer_start()
If we call _set_load in config then we don't need to call _load_start in
the enable, we can just call _start.
Can you try this and see if this is working ok?
Cheers
Jon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2012-12-13 17:42 ` Jon Hunter
0 siblings, 0 replies; 27+ messages in thread
From: Jon Hunter @ 2012-12-13 17:42 UTC (permalink / raw)
To: NeilBrown; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
On 12/12/2012 10:33 PM, NeilBrown wrote:
> On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
>
>>>> + omap_dm_timer_enable(omap->dm_timer);
>>>
>>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
>>> will enable the timer. So this should not be necessary.
>>
>> True. That is what you get for copying someone else's code and not
>> understanding it fully.
>
> However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
> explicitly checks that it is already runtime-enabled.
>
> Does that mean I don't need to call omap_dm_timer_write_counter here? Or
> does it mean that I do need the enable/disable pair?
Typically, omap_dm_timer_write_counter() is used to update the counter
value while the counter is running and hence is enabled.
Looking at the code, some more I now see what they are trying to do. It
seems that they are trying to force an overflow to occur as soon as they
enable the timer. This will cause the timer to load the count value from
the timer load register into the timer counter register. So that does
make sense to me. However, this should not be necessary as
omap_dm_timer_set_load should do this for you. Therefore, I think that
you could accomplish the same thing by doing ...
omap_pwm_config
--> omap_dm_timer_set_load()
--> omap_dm_timer_set_match()
--> omap_dm_timer_set_pwm()
omap_pwm_enable
--> omap_dm_timer_start()
If we call _set_load in config then we don't need to call _load_start in
the enable, we can just call _start.
Can you try this and see if this is working ok?
Cheers
Jon
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 17:42 ` Jon Hunter
@ 2012-12-15 0:16 ` NeilBrown
-1 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2012-12-15 0:16 UTC (permalink / raw)
To: Jon Hunter; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 12175 bytes --]
On Thu, 13 Dec 2012 11:42:18 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 12/12/2012 10:33 PM, NeilBrown wrote:
> > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
> >
> >>>> + omap_dm_timer_enable(omap->dm_timer);
> >>>
> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> >>> will enable the timer. So this should not be necessary.
> >>
> >> True. That is what you get for copying someone else's code and not
> >> understanding it fully.
> >
> > However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
> > explicitly checks that it is already runtime-enabled.
> >
> > Does that mean I don't need to call omap_dm_timer_write_counter here? Or
> > does it mean that I do need the enable/disable pair?
>
> Typically, omap_dm_timer_write_counter() is used to update the counter
> value while the counter is running and hence is enabled.
>
> Looking at the code, some more I now see what they are trying to do. It
> seems that they are trying to force an overflow to occur as soon as they
> enable the timer. This will cause the timer to load the count value from
> the timer load register into the timer counter register. So that does
> make sense to me. However, this should not be necessary as
> omap_dm_timer_set_load should do this for you. Therefore, I think that
> you could accomplish the same thing by doing ...
>
> omap_pwm_config
> --> omap_dm_timer_set_load()
> --> omap_dm_timer_set_match()
> --> omap_dm_timer_set_pwm()
>
> omap_pwm_enable
> --> omap_dm_timer_start()
>
> If we call _set_load in config then we don't need to call _load_start in
> the enable, we can just call _start.
>
> Can you try this and see if this is working ok?
Seems to work, and is much neater. Thanks.
Below is my current patch.
Unresolved issues are:
- it uses
omap_dm_timer_request_specific()
which apparently isn't ideal.
- It still zeros things in the suspend routine. I haven't explored this at
all yet
Thanks,
NeilBrown
From 69ed735d1bc377e8e65345792997f809e60b5fbf Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Sun, 2 Dec 2012 14:53:20 +1100
Subject: [PATCH] pwm: omap: Add PWM support using dual-mode timers
This patch is based on an earlier patch by Grant Erickson
which provided PWM devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Platform data must be provided to identify which dmtimer to use.
Lots of cleanup and inprovements thanks to Thierry Reding
and Jon Hunter.
Cc: Grant Erickson <marathon96@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..32c1253 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate "OMAP PWM support"
+ depends on ARCH_OMAP && OMAP_DM_TIMER
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate "PKUnity NetBook-0916 PWM support"
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 0000000..344072c
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,271 @@
+/*
+ * Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ * Heavily based on earlier code which is:
+ * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
+ *
+ * Also based on pwm-samsung.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * Description:
+ * This file is the core OMAP support for the generic, Linux
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ */
+
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <linux/module.h>
+#include <linux/platform_data/omap-pwm.h>
+
+#include <plat/dmtimer.h>
+
+#define DM_TIMER_LOAD_MIN 0xFFFFFFFE
+
+struct omap_chip {
+ struct omap_dm_timer *dm_timer;
+ enum pwm_polarity polarity;
+ unsigned int duty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+/**
+ * pwm_calc_value - Determine the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ * counter value for.
+ * @ns: The period, in nanoseconds, to compute the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ u64 c;
+
+ c = (u64)clk_rate * ns;
+ do_div(c, NSEC_PER_SEC);
+
+ return DM_TIMER_LOAD_MIN - c;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ omap_dm_timer_start(omap->dm_timer);
+
+ return 0;
+}
+
+static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ omap_dm_timer_stop(omap->dm_timer);
+}
+
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int load_value, match_value;
+ unsigned long clk_rate;
+
+ dev_dbg(chip->dev, "duty cycle: %d, period %d\n", duty_ns, period_ns);
+
+ if (omap->duty_ns == duty_ns &&
+ omap->period_ns == period_ns)
+ /* No change - don't cause any transients. */
+ return 0;
+
+ clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));
+
+ /*
+ * Calculate the appropriate load and match values based on the
+ * specified period and duty cycle. The load value determines the
+ * cycle time and the match value determines the duty cycle.
+ */
+
+ load_value = pwm_calc_value(clk_rate, period_ns);
+ match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
+
+ /*
+ * We MUST enable yet stop the associated dual-mode timer before
+ * attempting to write its registers. Hopefully it is already
+ * disabled, but call the (idempotent) pwm_disable just in case.
+ */
+
+ pwm_disable(pwm);
+
+ omap_dm_timer_set_load(omap->dm_timer, true, load_value);
+ omap_dm_timer_set_match(omap->dm_timer, true, match_value);
+
+ dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
+ load_value, load_value, match_value, match_value);
+
+ omap_dm_timer_set_pwm(omap->dm_timer,
+ omap->polarity == PWM_POLARITY_INVERSED,
+ true,
+ OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+
+ omap->duty_ns = duty_ns;
+ omap->period_ns = period_ns;
+
+ return 0;
+}
+
+static int omap_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ if (omap->polarity == polarity)
+ return 0;
+
+ omap->polarity = polarity;
+
+ omap_dm_timer_set_pwm(omap->dm_timer,
+ omap->polarity == PWM_POLARITY_INVERSED,
+ true,
+ OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+ return 0;
+}
+
+static struct pwm_ops omap_pwm_ops = {
+ .enable = omap_pwm_enable,
+ .disable = omap_pwm_disable,
+ .config = omap_pwm_config,
+ .set_polarity = omap_pwm_set_polarity,
+ .owner = THIS_MODULE,
+};
+
+static int omap_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct omap_chip *omap;
+ int status = 0;
+ struct omap_pwm_pdata *pdata = dev->platform_data;
+
+ if (!pdata) {
+ dev_err(dev, "No platform data provided\n");
+ return -ENODEV;
+ }
+
+ omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
+ if (omap == NULL) {
+ dev_err(dev, "Could not allocate memory.\n");
+ return -ENOMEM;
+ }
+
+ /*
+ * Request the OMAP dual-mode timer that will be bound to and
+ * associated with this generic PWM.
+ */
+
+ omap->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
+ if (omap->dm_timer == NULL) {
+ status = -EPROBE_DEFER;
+ goto err_free;
+ }
+
+ /*
+ * Configure the source for the dual-mode timer backing this
+ * generic PWM device. The clock source will ultimately determine
+ * how small or large the PWM frequency can be.
+ *
+ * At some point, it's probably worth revisiting moving this to
+ * the configure method and choosing either the slow- or
+ * system-clock source as appropriate for the desired PWM period.
+ */
+
+ omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
+
+ /*
+ * Cache away other miscellaneous driver-private data and state
+ * information and add the driver-private data to the platform
+ * device.
+ */
+
+ omap->chip.dev = dev;
+ omap->chip.ops = &omap_pwm_ops;
+ omap->chip.base = -1;
+ omap->chip.npwm = 1;
+ omap->polarity = PWM_POLARITY_NORMAL;
+
+ status = pwmchip_add(&omap->chip);
+ if (status < 0) {
+ dev_err(dev, "failed to register PWM\n");
+ omap_dm_timer_free(omap->dm_timer);
+ goto err_free;
+ }
+
+ platform_set_drvdata(pdev, omap);
+
+ return 0;
+
+ err_free:
+ kfree(omap);
+ return status;
+}
+
+static int omap_pwm_remove(struct platform_device *pdev)
+{
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ int status;
+
+ omap_dm_timer_stop(omap->dm_timer);
+ status = pwmchip_remove(&omap->chip);
+ if (status < 0)
+ return status;
+
+ omap_dm_timer_free(omap->dm_timer);
+ kfree(omap);
+
+ return 0;
+}
+
+#if CONFIG_PM
+static int omap_pwm_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ /*
+ * No one preserve these values during suspend so reset them,
+ * otherwise driver leaves PWM unconfigured if same values
+ * passed to pwm_config.
+ */
+ omap->period_ns = 0;
+ omap->duty_ns = 0;
+
+ return 0;
+}
+#else
+#define omap_pwm_suspend NULL
+#endif
+
+static SIMPLE_DEV_PM_OPS(omap_pwm_pm, omap_pwm_suspend, NULL);
+static struct platform_driver omap_pwm_driver = {
+ .driver = {
+ .name = "omap-pwm",
+ .owner = THIS_MODULE,
+ .pm = &omap_pwm_pm,
+ },
+ .probe = omap_pwm_probe,
+ .remove = omap_pwm_remove,
+};
+module_platform_driver(omap_pwm_driver);
+
+MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("2012-12-01");
+MODULE_DESCRIPTION("OMAP PWM Driver using Dual-mode Timers");
diff --git a/include/linux/platform_data/omap-pwm.h b/include/linux/platform_data/omap-pwm.h
new file mode 100644
index 0000000..169fc3c
--- /dev/null
+++ b/include/linux/platform_data/omap-pwm.h
@@ -0,0 +1,20 @@
+/*
+ * omap-pwm.h
+ *
+ * Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * Set the timer id to use for a PWM
+ */
+
+#ifndef _OMAP_PWM_H_
+#define _OMAP_PWM_H_
+
+struct omap_pwm_pdata {
+ int timer_id;
+};
+
+#endif /* _OMAP_PWM_H_ */
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2012-12-15 0:16 ` NeilBrown
0 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2012-12-15 0:16 UTC (permalink / raw)
To: Jon Hunter; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 12175 bytes --]
On Thu, 13 Dec 2012 11:42:18 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 12/12/2012 10:33 PM, NeilBrown wrote:
> > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown <neilb@suse.de> wrote:
> >
> >>>> + omap_dm_timer_enable(omap->dm_timer);
> >>>
> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> >>> will enable the timer. So this should not be necessary.
> >>
> >> True. That is what you get for copying someone else's code and not
> >> understanding it fully.
> >
> > However .... omap_dm_timer_write_counter *doesn't* enable the timer, and
> > explicitly checks that it is already runtime-enabled.
> >
> > Does that mean I don't need to call omap_dm_timer_write_counter here? Or
> > does it mean that I do need the enable/disable pair?
>
> Typically, omap_dm_timer_write_counter() is used to update the counter
> value while the counter is running and hence is enabled.
>
> Looking at the code, some more I now see what they are trying to do. It
> seems that they are trying to force an overflow to occur as soon as they
> enable the timer. This will cause the timer to load the count value from
> the timer load register into the timer counter register. So that does
> make sense to me. However, this should not be necessary as
> omap_dm_timer_set_load should do this for you. Therefore, I think that
> you could accomplish the same thing by doing ...
>
> omap_pwm_config
> --> omap_dm_timer_set_load()
> --> omap_dm_timer_set_match()
> --> omap_dm_timer_set_pwm()
>
> omap_pwm_enable
> --> omap_dm_timer_start()
>
> If we call _set_load in config then we don't need to call _load_start in
> the enable, we can just call _start.
>
> Can you try this and see if this is working ok?
Seems to work, and is much neater. Thanks.
Below is my current patch.
Unresolved issues are:
- it uses
omap_dm_timer_request_specific()
which apparently isn't ideal.
- It still zeros things in the suspend routine. I haven't explored this at
all yet
Thanks,
NeilBrown
From 69ed735d1bc377e8e65345792997f809e60b5fbf Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Sun, 2 Dec 2012 14:53:20 +1100
Subject: [PATCH] pwm: omap: Add PWM support using dual-mode timers
This patch is based on an earlier patch by Grant Erickson
which provided PWM devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Platform data must be provided to identify which dmtimer to use.
Lots of cleanup and inprovements thanks to Thierry Reding
and Jon Hunter.
Cc: Grant Erickson <marathon96@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..32c1253 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate "OMAP PWM support"
+ depends on ARCH_OMAP && OMAP_DM_TIMER
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate "PKUnity NetBook-0916 PWM support"
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 0000000..344072c
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,271 @@
+/*
+ * Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ * Heavily based on earlier code which is:
+ * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
+ *
+ * Also based on pwm-samsung.c
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * Description:
+ * This file is the core OMAP support for the generic, Linux
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ */
+
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+#include <linux/module.h>
+#include <linux/platform_data/omap-pwm.h>
+
+#include <plat/dmtimer.h>
+
+#define DM_TIMER_LOAD_MIN 0xFFFFFFFE
+
+struct omap_chip {
+ struct omap_dm_timer *dm_timer;
+ enum pwm_polarity polarity;
+ unsigned int duty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+/**
+ * pwm_calc_value - Determine the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ * counter value for.
+ * @ns: The period, in nanoseconds, to compute the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ u64 c;
+
+ c = (u64)clk_rate * ns;
+ do_div(c, NSEC_PER_SEC);
+
+ return DM_TIMER_LOAD_MIN - c;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ omap_dm_timer_start(omap->dm_timer);
+
+ return 0;
+}
+
+static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ omap_dm_timer_stop(omap->dm_timer);
+}
+
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int load_value, match_value;
+ unsigned long clk_rate;
+
+ dev_dbg(chip->dev, "duty cycle: %d, period %d\n", duty_ns, period_ns);
+
+ if (omap->duty_ns == duty_ns &&
+ omap->period_ns == period_ns)
+ /* No change - don't cause any transients. */
+ return 0;
+
+ clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));
+
+ /*
+ * Calculate the appropriate load and match values based on the
+ * specified period and duty cycle. The load value determines the
+ * cycle time and the match value determines the duty cycle.
+ */
+
+ load_value = pwm_calc_value(clk_rate, period_ns);
+ match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
+
+ /*
+ * We MUST enable yet stop the associated dual-mode timer before
+ * attempting to write its registers. Hopefully it is already
+ * disabled, but call the (idempotent) pwm_disable just in case.
+ */
+
+ pwm_disable(pwm);
+
+ omap_dm_timer_set_load(omap->dm_timer, true, load_value);
+ omap_dm_timer_set_match(omap->dm_timer, true, match_value);
+
+ dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
+ load_value, load_value, match_value, match_value);
+
+ omap_dm_timer_set_pwm(omap->dm_timer,
+ omap->polarity == PWM_POLARITY_INVERSED,
+ true,
+ OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+
+ omap->duty_ns = duty_ns;
+ omap->period_ns = period_ns;
+
+ return 0;
+}
+
+static int omap_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ if (omap->polarity == polarity)
+ return 0;
+
+ omap->polarity = polarity;
+
+ omap_dm_timer_set_pwm(omap->dm_timer,
+ omap->polarity == PWM_POLARITY_INVERSED,
+ true,
+ OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+ return 0;
+}
+
+static struct pwm_ops omap_pwm_ops = {
+ .enable = omap_pwm_enable,
+ .disable = omap_pwm_disable,
+ .config = omap_pwm_config,
+ .set_polarity = omap_pwm_set_polarity,
+ .owner = THIS_MODULE,
+};
+
+static int omap_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct omap_chip *omap;
+ int status = 0;
+ struct omap_pwm_pdata *pdata = dev->platform_data;
+
+ if (!pdata) {
+ dev_err(dev, "No platform data provided\n");
+ return -ENODEV;
+ }
+
+ omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
+ if (omap == NULL) {
+ dev_err(dev, "Could not allocate memory.\n");
+ return -ENOMEM;
+ }
+
+ /*
+ * Request the OMAP dual-mode timer that will be bound to and
+ * associated with this generic PWM.
+ */
+
+ omap->dm_timer = omap_dm_timer_request_specific(pdata->timer_id);
+ if (omap->dm_timer == NULL) {
+ status = -EPROBE_DEFER;
+ goto err_free;
+ }
+
+ /*
+ * Configure the source for the dual-mode timer backing this
+ * generic PWM device. The clock source will ultimately determine
+ * how small or large the PWM frequency can be.
+ *
+ * At some point, it's probably worth revisiting moving this to
+ * the configure method and choosing either the slow- or
+ * system-clock source as appropriate for the desired PWM period.
+ */
+
+ omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
+
+ /*
+ * Cache away other miscellaneous driver-private data and state
+ * information and add the driver-private data to the platform
+ * device.
+ */
+
+ omap->chip.dev = dev;
+ omap->chip.ops = &omap_pwm_ops;
+ omap->chip.base = -1;
+ omap->chip.npwm = 1;
+ omap->polarity = PWM_POLARITY_NORMAL;
+
+ status = pwmchip_add(&omap->chip);
+ if (status < 0) {
+ dev_err(dev, "failed to register PWM\n");
+ omap_dm_timer_free(omap->dm_timer);
+ goto err_free;
+ }
+
+ platform_set_drvdata(pdev, omap);
+
+ return 0;
+
+ err_free:
+ kfree(omap);
+ return status;
+}
+
+static int omap_pwm_remove(struct platform_device *pdev)
+{
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ int status;
+
+ omap_dm_timer_stop(omap->dm_timer);
+ status = pwmchip_remove(&omap->chip);
+ if (status < 0)
+ return status;
+
+ omap_dm_timer_free(omap->dm_timer);
+ kfree(omap);
+
+ return 0;
+}
+
+#if CONFIG_PM
+static int omap_pwm_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ /*
+ * No one preserve these values during suspend so reset them,
+ * otherwise driver leaves PWM unconfigured if same values
+ * passed to pwm_config.
+ */
+ omap->period_ns = 0;
+ omap->duty_ns = 0;
+
+ return 0;
+}
+#else
+#define omap_pwm_suspend NULL
+#endif
+
+static SIMPLE_DEV_PM_OPS(omap_pwm_pm, omap_pwm_suspend, NULL);
+static struct platform_driver omap_pwm_driver = {
+ .driver = {
+ .name = "omap-pwm",
+ .owner = THIS_MODULE,
+ .pm = &omap_pwm_pm,
+ },
+ .probe = omap_pwm_probe,
+ .remove = omap_pwm_remove,
+};
+module_platform_driver(omap_pwm_driver);
+
+MODULE_AUTHOR("Grant Erickson <marathon96@gmail.com>");
+MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("2012-12-01");
+MODULE_DESCRIPTION("OMAP PWM Driver using Dual-mode Timers");
diff --git a/include/linux/platform_data/omap-pwm.h b/include/linux/platform_data/omap-pwm.h
new file mode 100644
index 0000000..169fc3c
--- /dev/null
+++ b/include/linux/platform_data/omap-pwm.h
@@ -0,0 +1,20 @@
+/*
+ * omap-pwm.h
+ *
+ * Copyright (c) 2012 NeilBrown <neilb@suse.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * Set the timer id to use for a PWM
+ */
+
+#ifndef _OMAP_PWM_H_
+#define _OMAP_PWM_H_
+
+struct omap_pwm_pdata {
+ int timer_id;
+};
+
+#endif /* _OMAP_PWM_H_ */
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 3:06 ` NeilBrown
(?)
(?)
@ 2012-12-13 7:11 ` Thierry Reding
-1 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-12-13 7:11 UTC (permalink / raw)
To: NeilBrown; +Cc: Jon Hunter, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]
On Thu, Dec 13, 2012 at 02:06:35PM +1100, NeilBrown wrote:
>
> [Thierry: question for you near the end - thanks]
>
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
> > Hi Neil,
> >
> > On 12/12/2012 02:24 AM, NeilBrown wrote:
[...]
> > > +{
> > > + struct omap_chip *omap = platform_get_drvdata(pdev);
> > > + int status = 0;
> > > +
> > > + status = pwmchip_remove(&omap->chip);
> > > + if (status < 0)
> > > + goto done;
> > > +
> > > + omap_dm_timer_free(omap->dm_timer);
> >
> > Is it guaranteed that the timer will be disabled at this point?
>
> Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
> might not be disabled.
> Thierry: should pwm_put do that, or do I need a 'free' function in my chip
> ops to do that?
To be honest, I haven't decided yet. =) There have been discussions that
resulted in a request to run pwm_disable() from pwmchip_remove() on all
PWM devices a chip provides.
This isn't implemented yet and I'm not sure about all the side-effects.
I think for now the best way would be to implement .free() within this
driver, or even do an explicit pwm_disable() in the driver's .remove()
function to do this. When I've come to a decision I'll refactor all of
that in one patch across the whole subsystem.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 3:06 ` NeilBrown
@ 2012-12-13 17:07 ` Jon Hunter
-1 siblings, 0 replies; 27+ messages in thread
From: Jon Hunter @ 2012-12-13 17:07 UTC (permalink / raw)
To: NeilBrown; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
On 12/12/2012 09:06 PM, NeilBrown wrote:
>
> [Thierry: question for you near the end - thanks]
>
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
>> Hi Neil,
>>
>> On 12/12/2012 02:24 AM, NeilBrown wrote:
>>>
>>>
>>> This patch is based on an earlier patch by Grant Erickson
>>> which provided pwm devices using the 'legacy' interface.
>>>
>>> This driver instead uses the new framework interface.
>>>
>>> Cc: Grant Erickson <marathon96@gmail.com>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index ed81720..7df573a 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -85,6 +85,15 @@ config PWM_MXS
>>> To compile this driver as a module, choose M here: the module
>>> will be called pwm-mxs.
>>>
>>> +config PWM_OMAP
>>> + tristate "OMAP pwm support"
>>> + depends on ARCH_OMAP
>>
>> We should probably have depends on or selects OMAP_DM_TIMER here too.
>
> Sounds sensible - fixed.
>
>>
>>> + help
>>> + Generic PWM framework driver for OMAP
>>> +
>>> + To compile this driver as a module, choose M here: the module
>>> + will be called pwm-omap
>>> +
>>> config PWM_PUV3
>>> tristate "PKUnity NetBook-0916 PWM support"
>>> depends on ARCH_PUV3
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index acfe482..f5d200d 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
>>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
>>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
>>> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
>>> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
>>> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
>>> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
>>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>>> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
>>> new file mode 100644
>>> index 0000000..e3dbce3
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-omap.c
>>> @@ -0,0 +1,318 @@
>>> +/*
>>> + * Copyright (c) 2012 NeilBrown <neilb@suse.de>
>>> + * Heavily based on earlier code which is:
>>> + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
>>> + *
>>> + * Also based on pwm-samsung.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * Description:
>>> + * This file is the core OMAP2/3 support for the generic, Linux
>>
>> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
>> work for OMAP1-5.
>>
>
> Done.
>
>
>>> + * PWM driver / controller, using the OMAP's dual-mode timers.
>>> + *
>>> + * The 'id' number for the device encodes the number of the dm timer
>>> + * to use, and the polarity of the output.
>>> + * lsb is '1' of active-high, and '0' for active low
>>> + * remaining bit a timer number and need to be shifted down before use.
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "pwm-omap: " fmt
>>> +
>>> +#include <linux/export.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/err.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/pwm.h>
>>> +#include <linux/module.h>
>>> +
>>> +#include <plat/dmtimer.h>
>>
>> This is going to be a problem for the single zImage work, because we
>> cannot include any plat headers in driver code any more. Therefore,
>> although it is not ideal, one way to handle this is pass function
>> pointers to the various dmtimer APIs that are needed via the platform
>> data. Painful I know ...
>
> But that doesn't work with devicetree does it?
Ugh, you are right! This is becoming an increasing problem.
> Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
I can ask Tony if he thinks we could do that.
> It only included other things from include/linux, so it should be safe.
>
>>
>>> +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE
>>> +
>>> +struct omap_chip {
>>> + struct platform_device *pdev;
>>> +
>>> + struct omap_dm_timer *dm_timer;
>>> + unsigned int polarity;
>>> + const char *label;
>>> +
>>> + unsigned int duty_ns, period_ns;
>>> + struct pwm_chip chip;
>>> +};
>>> +
>>> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
>>> +
>>> +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
>>> +
>>> +/**
>>> + * pwm_calc_value - determines the counter value for a clock rate and period.
>>> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
>>> + * counter value for.
>>> + * @ns: The period, in nanoseconds, to computer the counter value for.
>>> + *
>>> + * Returns the PWM counter value for the specified clock rate and period.
>>> + */
>>> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
>>> +{
>>> + const unsigned long nanoseconds_per_second = 1000000000;
>>> + int cycles;
>>> + __u64 c;
>>> +
>>> + c = (__u64)clk_rate * ns;
>>> + do_div(c, nanoseconds_per_second);
>>> + cycles = c;
>>> +
>>> + return DM_TIMER_LOAD_MIN - cycles;
>>> +}
>>> +
>>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> + struct omap_chip *omap = to_omap_chip(chip);
>>> + int status = 0;
>>> +
>>> + /* Enable the counter--always--before attempting to write its
>>> + * registers and then set the timer to its minimum load value to
>>> + * ensure we get an overflow event right away once we start it.
>>> + */
>>> +
>>> + omap_dm_timer_enable(omap->dm_timer);
>>> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>>> + omap_dm_timer_start(omap->dm_timer);
>>> + omap_dm_timer_disable(omap->dm_timer);
>>
>> Why not just use omap_dm_timer_load_start() here instead of the above 4
>> APIs?
>
> Because I didn't know about it. I do now :-)
No problem.
>>
>>> +
>>> + return status;
>>> +}
>>> +
>>> +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> + struct omap_chip *omap = to_omap_chip(chip);
>>> +
>>> + omap_dm_timer_stop(omap->dm_timer);
>>> +}
>>> +
>>> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>> + int duty_ns, int period_ns)
>>> +{
>>> + struct omap_chip *omap = to_omap_chip(chip);
>>> + int status = 0;
>>> + const bool enable = true;
>>> + const bool autoreload = true;
>>> + const bool toggle = true;
>>> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
>>> + int load_value, match_value;
>>> + unsigned long clk_rate;
>>> +
>>> + dev_dbg(chip->dev,
>>> + "duty cycle: %d, period %d\n",
>>> + duty_ns, period_ns);
>>> +
>>> + if (omap->duty_ns == duty_ns &&
>>> + omap->period_ns == period_ns)
>>> + /* No change - don't cause any transients */
>>> + return 0;
>>> +
>>> + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));
>>> +
>>> + /* Calculate the appropriate load and match values based on the
>>> + * specified period and duty cycle. The load value determines the
>>> + * cycle time and the match value determines the duty cycle.
>>> + */
>>> +
>>> + load_value = pwm_calc_value(clk_rate, period_ns);
>>> + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
>>> +
>>> + /* We MUST enable yet stop the associated dual-mode timer before
>>> + * attempting to write its registers. Hopefully it is already
>>> + * disabled, but call the (idempotent) pwm_disable just in case
>>> + */
>>> +
>>> + pwm_disable(pwm);
>>> +
>>> + omap_dm_timer_enable(omap->dm_timer);
>>
>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
>> will enable the timer. So this should not be necessary.
>
> True. That is what you get for copying someone else's code and not
> understanding it fully.
>
>>
>>> + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
>>> + omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
>>> +
>>> + dev_dbg(chip->dev,
>>> + "load value: %#08x (%d), "
>>> + "match value: %#08x (%d)\n",
>>> + load_value, load_value,
>>> + match_value, match_value);
>>> +
>>> + omap_dm_timer_set_pwm(omap->dm_timer,
>>> + !omap->polarity,
>>> + toggle,
>>> + trigger);
>>> +
>>> + /* Set the counter to generate an overflow event immediately. */
>>> +
>>> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>>> +
>>> + /* Now that we're done configuring the dual-mode timer, disable it
>>> + * again. We'll enable and start it later, when requested.
>>> + */
>>> +
>>> + omap_dm_timer_disable(omap->dm_timer);
>>
>> Similarly the disable should not be needed here either.
>>
>>> + omap->duty_ns = duty_ns;
>>> + omap->period_ns = period_ns;
>>> +
>>> + return status;
>>> +}
>>> +
>>> +
>>> +static struct pwm_ops omap_pwm_ops = {
>>> + .enable = omap_pwm_enable,
>>> + .disable= omap_pwm_disable,
>>> + .config = omap_pwm_config,
>>> + .owner = THIS_MODULE,
>>> +};
>>> +
>>> +/**
>>> + * omap_pwm_probe - check for the PWM and bind it to the driver.
>>> + * @pdev: A pointer to the platform device node associated with the
>>> + * PWM instance to be probed for driver binding.
>>> + *
>>> + * Returns 0 if the PWM instance was successfully bound to the driver;
>>> + * otherwise, < 0 on error.
>>> + */
>>> +static int __devinit omap_pwm_probe(struct platform_device *pdev)
>>
>> I believe that __devinit is no longer required.
>>
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct omap_chip *omap;
>>> + int status = 0;
>>> + unsigned int id = pdev->id;
>>> + unsigned int timer = id >> 1; /* lsb is polarity */
>>> +
>>> + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
>>> +
>>> + if (omap == NULL) {
>>> + dev_err(dev, "Could not allocate memory.\n");
>>> + status = -ENOMEM;
>>> + goto done;
>>> + }
>>> +
>>> + /* Request the OMAP dual-mode timer that will be bound to and
>>> + * associated with this generic PWM.
>>> + */
>>> +
>>> + omap->dm_timer = omap_dm_timer_request_specific(timer);
>>
>> I would recommend that you use omap_dm_timer_request_by_cap() (new for
>> v3.8 so you should be able to use once v3.8-rc1 is out) here to request
>> a timer that supports the PWM output. The above function will not be
>> supported when booting with device-tree.
>
> I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
> this now.
> Would you object to the patch being submitted with the current call, then an
> update when I do move on to 3.8?
I don't have strong objections, but I did not think Thierry liked the
use of the id.
> However.... I may be misunderstanding something, but I want a timer to drive
> a particular output pin - GPIO-57. And I thought that it could only be
> driver by GPT11. So I need to explicitly request number 11 don't I?
Ugh, good point. Looks like for dev-tree we will need a way to request a
timer by IO and this is missing today!
>
>>
>>> +
>>> + if (omap->dm_timer == NULL) {
>>> + status = -EPROBE_DEFER;
>>> + goto err_free;
>>> + }
>>> +
>>> + /* Configure the source for the dual-mode timer backing this
>>> + * generic PWM device. The clock source will ultimately determine
>>> + * how small or large the PWM frequency can be.
>>> + *
>>> + * At some point, it's probably worth revisiting moving this to
>>> + * the configure method and choosing either the slow- or
>>> + * system-clock source as appropriate for the desired PWM period.
>>> + */
>>> +
>>> + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
>>> +
>>> + /* Cache away other miscellaneous driver-private data and state
>>> + * information and add the driver-private data to the platform
>>> + * device.
>>> + */
>>> +
>>> + omap->chip.dev = dev;
>>> + omap->chip.ops = &omap_pwm_ops;
>>> + omap->chip.base = -1;
>>> + omap->chip.npwm = 1;
>>> + omap->polarity = id & 1;
>>> +
>>> + status = pwmchip_add(&omap->chip);
>>> + if (status < 0) {
>>> + dev_err(dev, "failed to register pwm\n");
>>> + omap_dm_timer_free(omap->dm_timer);
>>> + goto err_free;
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, omap);
>>> +
>>> + status = 0;
>>> + goto done;
>>> +
>>> + err_free:
>>> + kfree(omap);
>>> + done:
>>> + return status;
>>> +}
>>> +
>>> +/**
>>> + * omap_pwm_remove - unbind the specified PWM platform device from the driver.
>>> + * @pdev: A pointer to the platform device node associated with the
>>> + * PWM instance to be unbound/removed.
>>> + *
>>> + * Returns 0 if the PWM was successfully removed as a platform device;
>>> + * otherwise, < 0 on error.
>>> + */
>>> +static int __devexit omap_pwm_remove(struct platform_device *pdev)
>>
>> I believe that __devexit is no longer required.
>>
>>> +{
>>> + struct omap_chip *omap = platform_get_drvdata(pdev);
>>> + int status = 0;
>>> +
>>> + status = pwmchip_remove(&omap->chip);
>>> + if (status < 0)
>>> + goto done;
>>> +
>>> + omap_dm_timer_free(omap->dm_timer);
>>
>> Is it guaranteed that the timer will be disabled at this point?
>
> Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
> might not be disabled.
> Thierry: should pwm_put do that, or do I need a 'free' function in my chip
> ops to do that?
>
>
>>
>>> +
>>> + kfree(omap);
>>> +
>>> + done:
>>> + return status;
>>> +}
>>> +
>>> +#if CONFIG_PM
>>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
>>> +{
>>> + struct omap_chip *omap = platform_get_drvdata(pdev);
>>> + /* No one preserve these values during suspend so reset them
>>> + * Otherwise driver leaves PWM unconfigured if same values
>>> + * passed to pwm_config
>>> + */
>>> + omap->period_ns = 0;
>>> + omap->duty_ns = 0;
>>
>>
>> Hmmm, looks like you are trying to force a reconfiguration after suspend
>> if the same values are used. Is there an underlying problem here that
>> you are trying to workaround?
>
> I copied that from pwm-samsung.c.
>
> The key question is: does a dmtimer preserve all register values over suspend.
> If so, then I guess we don't need this.
> If not, we do (because omap_pwm_config short circuits if it thinks the config
> hasn't changed).
I gave it a quick test on omap3/4 when just operating the timer as a
counter (not driving a pwm output) and suspend/resume works fine.
However, it does not work if I enable off mode (via the debugfs). This
is not enabled by default and may be I should put that on my to-do list
as well.
> Maybe I should test and see - though as my backlight always blanks before
> suspend that might not be straight forward...
Yes it would be great if you can test too. I have been finding that
certain features of the timer are not that well tested.
>>
>> Please note that I am not familiar with the PWM sub-system to know how
>> suspend-resume is typically handled and if this is normal or not.
>>
>>> + return 0;
>>> +}
>>> +#else
>>> +#define omap_pwm_suspend NULL
>>> +#endif
>>> +
>>> +static struct platform_driver omap_pwm_driver = {
>>> + .driver.name = "omap-pwm",
>>> + .driver.owner = THIS_MODULE,
>>> + .probe = omap_pwm_probe,
>>> + .remove = __devexit_p(omap_pwm_remove),
>>
>> I believe that __devexit_p is no longer required.
>>
>> Otherwise it looks good to me. Thanks for sending!
>
> And thanks a lot for reading and reviewing!
No problem.
Cheers
Jon
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2012-12-13 17:07 ` Jon Hunter
0 siblings, 0 replies; 27+ messages in thread
From: Jon Hunter @ 2012-12-13 17:07 UTC (permalink / raw)
To: NeilBrown; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
On 12/12/2012 09:06 PM, NeilBrown wrote:
>
> [Thierry: question for you near the end - thanks]
>
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
>> Hi Neil,
>>
>> On 12/12/2012 02:24 AM, NeilBrown wrote:
>>>
>>>
>>> This patch is based on an earlier patch by Grant Erickson
>>> which provided pwm devices using the 'legacy' interface.
>>>
>>> This driver instead uses the new framework interface.
>>>
>>> Cc: Grant Erickson <marathon96@gmail.com>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index ed81720..7df573a 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -85,6 +85,15 @@ config PWM_MXS
>>> To compile this driver as a module, choose M here: the module
>>> will be called pwm-mxs.
>>>
>>> +config PWM_OMAP
>>> + tristate "OMAP pwm support"
>>> + depends on ARCH_OMAP
>>
>> We should probably have depends on or selects OMAP_DM_TIMER here too.
>
> Sounds sensible - fixed.
>
>>
>>> + help
>>> + Generic PWM framework driver for OMAP
>>> +
>>> + To compile this driver as a module, choose M here: the module
>>> + will be called pwm-omap
>>> +
>>> config PWM_PUV3
>>> tristate "PKUnity NetBook-0916 PWM support"
>>> depends on ARCH_PUV3
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index acfe482..f5d200d 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
>>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
>>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
>>> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
>>> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
>>> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
>>> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
>>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>>> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
>>> new file mode 100644
>>> index 0000000..e3dbce3
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-omap.c
>>> @@ -0,0 +1,318 @@
>>> +/*
>>> + * Copyright (c) 2012 NeilBrown <neilb@suse.de>
>>> + * Heavily based on earlier code which is:
>>> + * Copyright (c) 2010 Grant Erickson <marathon96@gmail.com>
>>> + *
>>> + * Also based on pwm-samsung.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * Description:
>>> + * This file is the core OMAP2/3 support for the generic, Linux
>>
>> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
>> work for OMAP1-5.
>>
>
> Done.
>
>
>>> + * PWM driver / controller, using the OMAP's dual-mode timers.
>>> + *
>>> + * The 'id' number for the device encodes the number of the dm timer
>>> + * to use, and the polarity of the output.
>>> + * lsb is '1' of active-high, and '0' for active low
>>> + * remaining bit a timer number and need to be shifted down before use.
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "pwm-omap: " fmt
>>> +
>>> +#include <linux/export.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/err.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/pwm.h>
>>> +#include <linux/module.h>
>>> +
>>> +#include <plat/dmtimer.h>
>>
>> This is going to be a problem for the single zImage work, because we
>> cannot include any plat headers in driver code any more. Therefore,
>> although it is not ideal, one way to handle this is pass function
>> pointers to the various dmtimer APIs that are needed via the platform
>> data. Painful I know ...
>
> But that doesn't work with devicetree does it?
Ugh, you are right! This is becoming an increasing problem.
> Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
I can ask Tony if he thinks we could do that.
> It only included other things from include/linux, so it should be safe.
>
>>
>>> +#define DM_TIMER_LOAD_MIN 0xFFFFFFFE
>>> +
>>> +struct omap_chip {
>>> + struct platform_device *pdev;
>>> +
>>> + struct omap_dm_timer *dm_timer;
>>> + unsigned int polarity;
>>> + const char *label;
>>> +
>>> + unsigned int duty_ns, period_ns;
>>> + struct pwm_chip chip;
>>> +};
>>> +
>>> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
>>> +
>>> +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
>>> +
>>> +/**
>>> + * pwm_calc_value - determines the counter value for a clock rate and period.
>>> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
>>> + * counter value for.
>>> + * @ns: The period, in nanoseconds, to computer the counter value for.
>>> + *
>>> + * Returns the PWM counter value for the specified clock rate and period.
>>> + */
>>> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
>>> +{
>>> + const unsigned long nanoseconds_per_second = 1000000000;
>>> + int cycles;
>>> + __u64 c;
>>> +
>>> + c = (__u64)clk_rate * ns;
>>> + do_div(c, nanoseconds_per_second);
>>> + cycles = c;
>>> +
>>> + return DM_TIMER_LOAD_MIN - cycles;
>>> +}
>>> +
>>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> + struct omap_chip *omap = to_omap_chip(chip);
>>> + int status = 0;
>>> +
>>> + /* Enable the counter--always--before attempting to write its
>>> + * registers and then set the timer to its minimum load value to
>>> + * ensure we get an overflow event right away once we start it.
>>> + */
>>> +
>>> + omap_dm_timer_enable(omap->dm_timer);
>>> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>>> + omap_dm_timer_start(omap->dm_timer);
>>> + omap_dm_timer_disable(omap->dm_timer);
>>
>> Why not just use omap_dm_timer_load_start() here instead of the above 4
>> APIs?
>
> Because I didn't know about it. I do now :-)
No problem.
>>
>>> +
>>> + return status;
>>> +}
>>> +
>>> +static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>> +{
>>> + struct omap_chip *omap = to_omap_chip(chip);
>>> +
>>> + omap_dm_timer_stop(omap->dm_timer);
>>> +}
>>> +
>>> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>> + int duty_ns, int period_ns)
>>> +{
>>> + struct omap_chip *omap = to_omap_chip(chip);
>>> + int status = 0;
>>> + const bool enable = true;
>>> + const bool autoreload = true;
>>> + const bool toggle = true;
>>> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
>>> + int load_value, match_value;
>>> + unsigned long clk_rate;
>>> +
>>> + dev_dbg(chip->dev,
>>> + "duty cycle: %d, period %d\n",
>>> + duty_ns, period_ns);
>>> +
>>> + if (omap->duty_ns == duty_ns &&
>>> + omap->period_ns == period_ns)
>>> + /* No change - don't cause any transients */
>>> + return 0;
>>> +
>>> + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));
>>> +
>>> + /* Calculate the appropriate load and match values based on the
>>> + * specified period and duty cycle. The load value determines the
>>> + * cycle time and the match value determines the duty cycle.
>>> + */
>>> +
>>> + load_value = pwm_calc_value(clk_rate, period_ns);
>>> + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
>>> +
>>> + /* We MUST enable yet stop the associated dual-mode timer before
>>> + * attempting to write its registers. Hopefully it is already
>>> + * disabled, but call the (idempotent) pwm_disable just in case
>>> + */
>>> +
>>> + pwm_disable(pwm);
>>> +
>>> + omap_dm_timer_enable(omap->dm_timer);
>>
>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
>> will enable the timer. So this should not be necessary.
>
> True. That is what you get for copying someone else's code and not
> understanding it fully.
>
>>
>>> + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value);
>>> + omap_dm_timer_set_match(omap->dm_timer, enable, match_value);
>>> +
>>> + dev_dbg(chip->dev,
>>> + "load value: %#08x (%d), "
>>> + "match value: %#08x (%d)\n",
>>> + load_value, load_value,
>>> + match_value, match_value);
>>> +
>>> + omap_dm_timer_set_pwm(omap->dm_timer,
>>> + !omap->polarity,
>>> + toggle,
>>> + trigger);
>>> +
>>> + /* Set the counter to generate an overflow event immediately. */
>>> +
>>> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>>> +
>>> + /* Now that we're done configuring the dual-mode timer, disable it
>>> + * again. We'll enable and start it later, when requested.
>>> + */
>>> +
>>> + omap_dm_timer_disable(omap->dm_timer);
>>
>> Similarly the disable should not be needed here either.
>>
>>> + omap->duty_ns = duty_ns;
>>> + omap->period_ns = period_ns;
>>> +
>>> + return status;
>>> +}
>>> +
>>> +
>>> +static struct pwm_ops omap_pwm_ops = {
>>> + .enable = omap_pwm_enable,
>>> + .disable= omap_pwm_disable,
>>> + .config = omap_pwm_config,
>>> + .owner = THIS_MODULE,
>>> +};
>>> +
>>> +/**
>>> + * omap_pwm_probe - check for the PWM and bind it to the driver.
>>> + * @pdev: A pointer to the platform device node associated with the
>>> + * PWM instance to be probed for driver binding.
>>> + *
>>> + * Returns 0 if the PWM instance was successfully bound to the driver;
>>> + * otherwise, < 0 on error.
>>> + */
>>> +static int __devinit omap_pwm_probe(struct platform_device *pdev)
>>
>> I believe that __devinit is no longer required.
>>
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct omap_chip *omap;
>>> + int status = 0;
>>> + unsigned int id = pdev->id;
>>> + unsigned int timer = id >> 1; /* lsb is polarity */
>>> +
>>> + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
>>> +
>>> + if (omap == NULL) {
>>> + dev_err(dev, "Could not allocate memory.\n");
>>> + status = -ENOMEM;
>>> + goto done;
>>> + }
>>> +
>>> + /* Request the OMAP dual-mode timer that will be bound to and
>>> + * associated with this generic PWM.
>>> + */
>>> +
>>> + omap->dm_timer = omap_dm_timer_request_specific(timer);
>>
>> I would recommend that you use omap_dm_timer_request_by_cap() (new for
>> v3.8 so you should be able to use once v3.8-rc1 is out) here to request
>> a timer that supports the PWM output. The above function will not be
>> supported when booting with device-tree.
>
> I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
> this now.
> Would you object to the patch being submitted with the current call, then an
> update when I do move on to 3.8?
I don't have strong objections, but I did not think Thierry liked the
use of the id.
> However.... I may be misunderstanding something, but I want a timer to drive
> a particular output pin - GPIO-57. And I thought that it could only be
> driver by GPT11. So I need to explicitly request number 11 don't I?
Ugh, good point. Looks like for dev-tree we will need a way to request a
timer by IO and this is missing today!
>
>>
>>> +
>>> + if (omap->dm_timer == NULL) {
>>> + status = -EPROBE_DEFER;
>>> + goto err_free;
>>> + }
>>> +
>>> + /* Configure the source for the dual-mode timer backing this
>>> + * generic PWM device. The clock source will ultimately determine
>>> + * how small or large the PWM frequency can be.
>>> + *
>>> + * At some point, it's probably worth revisiting moving this to
>>> + * the configure method and choosing either the slow- or
>>> + * system-clock source as appropriate for the desired PWM period.
>>> + */
>>> +
>>> + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
>>> +
>>> + /* Cache away other miscellaneous driver-private data and state
>>> + * information and add the driver-private data to the platform
>>> + * device.
>>> + */
>>> +
>>> + omap->chip.dev = dev;
>>> + omap->chip.ops = &omap_pwm_ops;
>>> + omap->chip.base = -1;
>>> + omap->chip.npwm = 1;
>>> + omap->polarity = id & 1;
>>> +
>>> + status = pwmchip_add(&omap->chip);
>>> + if (status < 0) {
>>> + dev_err(dev, "failed to register pwm\n");
>>> + omap_dm_timer_free(omap->dm_timer);
>>> + goto err_free;
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, omap);
>>> +
>>> + status = 0;
>>> + goto done;
>>> +
>>> + err_free:
>>> + kfree(omap);
>>> + done:
>>> + return status;
>>> +}
>>> +
>>> +/**
>>> + * omap_pwm_remove - unbind the specified PWM platform device from the driver.
>>> + * @pdev: A pointer to the platform device node associated with the
>>> + * PWM instance to be unbound/removed.
>>> + *
>>> + * Returns 0 if the PWM was successfully removed as a platform device;
>>> + * otherwise, < 0 on error.
>>> + */
>>> +static int __devexit omap_pwm_remove(struct platform_device *pdev)
>>
>> I believe that __devexit is no longer required.
>>
>>> +{
>>> + struct omap_chip *omap = platform_get_drvdata(pdev);
>>> + int status = 0;
>>> +
>>> + status = pwmchip_remove(&omap->chip);
>>> + if (status < 0)
>>> + goto done;
>>> +
>>> + omap_dm_timer_free(omap->dm_timer);
>>
>> Is it guaranteed that the timer will be disabled at this point?
>
> Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
> might not be disabled.
> Thierry: should pwm_put do that, or do I need a 'free' function in my chip
> ops to do that?
>
>
>>
>>> +
>>> + kfree(omap);
>>> +
>>> + done:
>>> + return status;
>>> +}
>>> +
>>> +#if CONFIG_PM
>>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
>>> +{
>>> + struct omap_chip *omap = platform_get_drvdata(pdev);
>>> + /* No one preserve these values during suspend so reset them
>>> + * Otherwise driver leaves PWM unconfigured if same values
>>> + * passed to pwm_config
>>> + */
>>> + omap->period_ns = 0;
>>> + omap->duty_ns = 0;
>>
>>
>> Hmmm, looks like you are trying to force a reconfiguration after suspend
>> if the same values are used. Is there an underlying problem here that
>> you are trying to workaround?
>
> I copied that from pwm-samsung.c.
>
> The key question is: does a dmtimer preserve all register values over suspend.
> If so, then I guess we don't need this.
> If not, we do (because omap_pwm_config short circuits if it thinks the config
> hasn't changed).
I gave it a quick test on omap3/4 when just operating the timer as a
counter (not driving a pwm output) and suspend/resume works fine.
However, it does not work if I enable off mode (via the debugfs). This
is not enabled by default and may be I should put that on my to-do list
as well.
> Maybe I should test and see - though as my backlight always blanks before
> suspend that might not be straight forward...
Yes it would be great if you can test too. I have been finding that
certain features of the timer are not that well tested.
>>
>> Please note that I am not familiar with the PWM sub-system to know how
>> suspend-resume is typically handled and if this is normal or not.
>>
>>> + return 0;
>>> +}
>>> +#else
>>> +#define omap_pwm_suspend NULL
>>> +#endif
>>> +
>>> +static struct platform_driver omap_pwm_driver = {
>>> + .driver.name = "omap-pwm",
>>> + .driver.owner = THIS_MODULE,
>>> + .probe = omap_pwm_probe,
>>> + .remove = __devexit_p(omap_pwm_remove),
>>
>> I believe that __devexit_p is no longer required.
>>
>> Otherwise it looks good to me. Thanks for sending!
>
> And thanks a lot for reading and reviewing!
No problem.
Cheers
Jon
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 17:07 ` Jon Hunter
(?)
@ 2012-12-13 17:34 ` Tony Lindgren
-1 siblings, 0 replies; 27+ messages in thread
From: Tony Lindgren @ 2012-12-13 17:34 UTC (permalink / raw)
To: Jon Hunter; +Cc: NeilBrown, Thierry Reding, Grant Erickson, linux-omap, lkml
* Jon Hunter <jon-hunter@ti.com> [121213 09:11]:
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> >> On 12/12/2012 02:24 AM, NeilBrown wrote:
> >>> +
> >>> +#include <plat/dmtimer.h>
> >>
> >> This is going to be a problem for the single zImage work, because we
> >> cannot include any plat headers in driver code any more. Therefore,
> >> although it is not ideal, one way to handle this is pass function
> >> pointers to the various dmtimer APIs that are needed via the platform
> >> data. Painful I know ...
> >
> > But that doesn't work with devicetree does it?
>
> Ugh, you are right! This is becoming an increasing problem.
>
> > Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
>
> I can ask Tony if he thinks we could do that.
Yeah we need to fix this somehow. First we need to limit that header
to the minimum and have most of it in a local header file for the
clocksource and clockevent. Then let's move the minimal header to
include/linux/omap-dmtimer.h until we have something Linux generic
available for doing things like this.
Regards,
Tony
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 17:07 ` Jon Hunter
@ 2013-01-06 21:12 ` NeilBrown
-1 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2013-01-06 21:12 UTC (permalink / raw)
To: Jon Hunter; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]
On Thu, 13 Dec 2012 11:07:25 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> >
> >>> +
> >>> +#if CONFIG_PM
> >>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> >>> +{
> >>> + struct omap_chip *omap = platform_get_drvdata(pdev);
> >>> + /* No one preserve these values during suspend so reset them
> >>> + * Otherwise driver leaves PWM unconfigured if same values
> >>> + * passed to pwm_config
> >>> + */
> >>> + omap->period_ns = 0;
> >>> + omap->duty_ns = 0;
> >>
> >>
> >> Hmmm, looks like you are trying to force a reconfiguration after suspend
> >> if the same values are used. Is there an underlying problem here that
> >> you are trying to workaround?
> >
> > I copied that from pwm-samsung.c.
> >
> > The key question is: does a dmtimer preserve all register values over suspend.
> > If so, then I guess we don't need this.
> > If not, we do (because omap_pwm_config short circuits if it thinks the config
> > hasn't changed).
>
> I gave it a quick test on omap3/4 when just operating the timer as a
> counter (not driving a pwm output) and suspend/resume works fine.
> However, it does not work if I enable off mode (via the debugfs). This
> is not enabled by default and may be I should put that on my to-do list
> as well.
I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).
NeilBrown
From: NeilBrown <neilb@suse.de>
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
The context loss handling in dmtimer appears to assume that
omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
omap_dm_timer_stop()
bracket all interactions. Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.
So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).
Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.
Signed-off-by: NeilBrown <neilb@suse.de>
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(&timer->pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ int loss_count =
+ omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
+ if (loss_count != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = loss_count;
+ }
+ }
}
EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
@@ -347,12 +356,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
omap_dm_timer_enable(timer);
- if (!(timer->capability & OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
- timer->ctx_loss_count)
- omap_timer_restore_context(timer);
- }
-
l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
if (!(l & OMAP_TIMER_CTRL_ST)) {
l |= OMAP_TIMER_CTRL_ST;
@@ -377,10 +380,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
__omap_dm_timer_stop(timer, timer->posted, rate);
- if (!(timer->capability & OMAP_TIMER_ALWON))
- timer->ctx_loss_count =
- omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
-
/*
* Since the register values are computed and written within
* __omap_dm_timer_stop, we need to use read to retrieve the
@@ -494,12 +493,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
omap_dm_timer_enable(timer);
- if (!(timer->capability & OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
- timer->ctx_loss_count)
- omap_timer_restore_context(timer);
- }
-
l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
if (autoreload) {
l |= OMAP_TIMER_CTRL_AR;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2013-01-06 21:12 ` NeilBrown
0 siblings, 0 replies; 27+ messages in thread
From: NeilBrown @ 2013-01-06 21:12 UTC (permalink / raw)
To: Jon Hunter; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]
On Thu, 13 Dec 2012 11:07:25 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
>
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> >
> >>> +
> >>> +#if CONFIG_PM
> >>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> >>> +{
> >>> + struct omap_chip *omap = platform_get_drvdata(pdev);
> >>> + /* No one preserve these values during suspend so reset them
> >>> + * Otherwise driver leaves PWM unconfigured if same values
> >>> + * passed to pwm_config
> >>> + */
> >>> + omap->period_ns = 0;
> >>> + omap->duty_ns = 0;
> >>
> >>
> >> Hmmm, looks like you are trying to force a reconfiguration after suspend
> >> if the same values are used. Is there an underlying problem here that
> >> you are trying to workaround?
> >
> > I copied that from pwm-samsung.c.
> >
> > The key question is: does a dmtimer preserve all register values over suspend.
> > If so, then I guess we don't need this.
> > If not, we do (because omap_pwm_config short circuits if it thinks the config
> > hasn't changed).
>
> I gave it a quick test on omap3/4 when just operating the timer as a
> counter (not driving a pwm output) and suspend/resume works fine.
> However, it does not work if I enable off mode (via the debugfs). This
> is not enabled by default and may be I should put that on my to-do list
> as well.
I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).
NeilBrown
From: NeilBrown <neilb@suse.de>
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
The context loss handling in dmtimer appears to assume that
omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
omap_dm_timer_stop()
bracket all interactions. Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.
So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).
Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.
Signed-off-by: NeilBrown <neilb@suse.de>
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(&timer->pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ int loss_count =
+ omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
+ if (loss_count != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = loss_count;
+ }
+ }
}
EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
@@ -347,12 +356,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
omap_dm_timer_enable(timer);
- if (!(timer->capability & OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
- timer->ctx_loss_count)
- omap_timer_restore_context(timer);
- }
-
l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
if (!(l & OMAP_TIMER_CTRL_ST)) {
l |= OMAP_TIMER_CTRL_ST;
@@ -377,10 +380,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
__omap_dm_timer_stop(timer, timer->posted, rate);
- if (!(timer->capability & OMAP_TIMER_ALWON))
- timer->ctx_loss_count =
- omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
-
/*
* Since the register values are computed and written within
* __omap_dm_timer_stop, we need to use read to retrieve the
@@ -494,12 +493,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
omap_dm_timer_enable(timer);
- if (!(timer->capability & OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
- timer->ctx_loss_count)
- omap_timer_restore_context(timer);
- }
-
l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
if (autoreload) {
l |= OMAP_TIMER_CTRL_AR;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2013-01-06 21:12 ` NeilBrown
@ 2013-01-07 22:24 ` Jon Hunter
-1 siblings, 0 replies; 27+ messages in thread
From: Jon Hunter @ 2013-01-07 22:24 UTC (permalink / raw)
To: NeilBrown; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
On 01/06/2013 03:12 PM, NeilBrown wrote:
[snip]
> I've been playing with off-mode and discovered that the first attempt to set
> the PWM after resume didn't work, but subsequent ones did.
> I did some digging and came up with the following patch.
> With this in place, the omap_pwm_suspend() above is definitely pointless (was
> wasn't really useful even without it).
Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(
Some minor comments below ...
> NeilBrown
>
>
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 7 Jan 2013 07:53:03 +1100
> Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
Nit, subject should formatted "ARM: OMAP: blah blah blah"
Also, may be worth calling this "fix context-loss" as this is really
fixing something that is broken.
> The context loss handling in dmtimer appears to assume that
> omap_dm_timer_set_load_start() or omap_dm_timer_start()
> and
> omap_dm_timer_stop()
>
> bracket all interactions. Only the first two restore the context and
> the last updates the context loss counter.
> However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
> reasonably be called outside this bracketing, and the fact that they
> call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
> is expected.
>
> So if, after a transition into and out of off-mode which would cause
> the dm timer to loose all state, omap_dm_timer_set_match() is called
> before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
> will be 'wrong' and this wrong value will be stored context.tclr so
> a subsequent omap_dm_timer_start() can fail (As the control register
> is wrong).
>
> Simplify this be doing the restore-from-context in
> omap_dm_timer_enable() so that whenever the timer is enabled, the
> context is correct.
> Also update the ctx_loss_count at the same time as we notice it is
> wrong - these is no value in delaying this until the
> omap_dm_timer_disable() as it cannot change while the timer is
> enabled.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 938b50a..c216696 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
> void omap_dm_timer_enable(struct omap_dm_timer *timer)
> {
> pm_runtime_get_sync(&timer->pdev->dev);
> +
> + if (!(timer->capability & OMAP_TIMER_ALWON)) {
> + int loss_count =
> + omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
> + if (loss_count != timer->ctx_loss_count) {
> + omap_timer_restore_context(timer);
> + timer->ctx_loss_count = loss_count;
> + }
> + }
> }
Can you rebase on v3.8-rc2? We no longer call
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ int c;
+
pm_runtime_get_sync(&timer->pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ if (timer->get_context_loss_count) {
+ c = timer->get_context_loss_count(&timer->pdev->dev);
+ if (c != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = c;
+ }
+ }
+ }
Cheers
Jon
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH] OMAP: add pwm driver using dmtimers.
@ 2013-01-07 22:24 ` Jon Hunter
0 siblings, 0 replies; 27+ messages in thread
From: Jon Hunter @ 2013-01-07 22:24 UTC (permalink / raw)
To: NeilBrown; +Cc: Thierry Reding, Grant Erickson, linux-omap, lkml
On 01/06/2013 03:12 PM, NeilBrown wrote:
[snip]
> I've been playing with off-mode and discovered that the first attempt to set
> the PWM after resume didn't work, but subsequent ones did.
> I did some digging and came up with the following patch.
> With this in place, the omap_pwm_suspend() above is definitely pointless (was
> wasn't really useful even without it).
Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(
Some minor comments below ...
> NeilBrown
>
>
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 7 Jan 2013 07:53:03 +1100
> Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
Nit, subject should formatted "ARM: OMAP: blah blah blah"
Also, may be worth calling this "fix context-loss" as this is really
fixing something that is broken.
> The context loss handling in dmtimer appears to assume that
> omap_dm_timer_set_load_start() or omap_dm_timer_start()
> and
> omap_dm_timer_stop()
>
> bracket all interactions. Only the first two restore the context and
> the last updates the context loss counter.
> However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
> reasonably be called outside this bracketing, and the fact that they
> call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
> is expected.
>
> So if, after a transition into and out of off-mode which would cause
> the dm timer to loose all state, omap_dm_timer_set_match() is called
> before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
> will be 'wrong' and this wrong value will be stored context.tclr so
> a subsequent omap_dm_timer_start() can fail (As the control register
> is wrong).
>
> Simplify this be doing the restore-from-context in
> omap_dm_timer_enable() so that whenever the timer is enabled, the
> context is correct.
> Also update the ctx_loss_count at the same time as we notice it is
> wrong - these is no value in delaying this until the
> omap_dm_timer_disable() as it cannot change while the timer is
> enabled.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
>
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 938b50a..c216696 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
> void omap_dm_timer_enable(struct omap_dm_timer *timer)
> {
> pm_runtime_get_sync(&timer->pdev->dev);
> +
> + if (!(timer->capability & OMAP_TIMER_ALWON)) {
> + int loss_count =
> + omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
> + if (loss_count != timer->ctx_loss_count) {
> + omap_timer_restore_context(timer);
> + timer->ctx_loss_count = loss_count;
> + }
> + }
> }
Can you rebase on v3.8-rc2? We no longer call
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ int c;
+
pm_runtime_get_sync(&timer->pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ if (timer->get_context_loss_count) {
+ c = timer->get_context_loss_count(&timer->pdev->dev);
+ if (c != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = c;
+ }
+ }
+ }
Cheers
Jon
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] OMAP: add pwm driver using dmtimers.
2012-12-13 3:06 ` NeilBrown
` (3 preceding siblings ...)
(?)
@ 2012-12-13 17:41 ` Tony Lindgren
-1 siblings, 0 replies; 27+ messages in thread
From: Tony Lindgren @ 2012-12-13 17:41 UTC (permalink / raw)
To: NeilBrown; +Cc: Jon Hunter, Thierry Reding, Grant Erickson, linux-omap, lkml
* NeilBrown <neilb@suse.de> [121212 19:09]:
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter <jon-hunter@ti.com> wrote:
> > On 12/12/2012 02:24 AM, NeilBrown wrote:
> > > +
> > > + /* Request the OMAP dual-mode timer that will be bound to and
> > > + * associated with this generic PWM.
> > > + */
> > > +
> > > + omap->dm_timer = omap_dm_timer_request_specific(timer);
> >
> > I would recommend that you use omap_dm_timer_request_by_cap() (new for
> > v3.8 so you should be able to use once v3.8-rc1 is out) here to request
> > a timer that supports the PWM output. The above function will not be
> > supported when booting with device-tree.
>
> I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do
> this now.
> Would you object to the patch being submitted with the current call, then an
> update when I do move on to 3.8?
>
> However.... I may be misunderstanding something, but I want a timer to drive
> a particular output pin - GPIO-57. And I thought that it could only be
> driver by GPT11. So I need to explicitly request number 11 don't I?
Yes I believe it needs to tied to a specific GPT instance to have access
to the pin. We should not export omap_dm_timer_request_specific(), that
information should become from platform_data and device tree.
Regards,
Tony
^ permalink raw reply [flat|nested] 27+ messages in thread