From: David Rivshin Allworx <hidden> Date: 2016-01-30 04:26:50
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
--
2.5.0
From: David Rivshin Allworx <hidden> Date: 2016-01-30 04:26:51
From: David Rivshin <redacted>
Fix the calculation of load_value and match_value. Currently they
are slightly too low, which produces a noticeably wrong PWM rate with
sufficiently short periods (i.e. when 1/period approaches clk_rate/2).
Example:
clk_rate=32768Hz, period=122070ns, duty_cycle=61035ns (8192Hz/50% PWM)
Correct values: load = 0xfffffffc, match = 0xfffffffd
Current values: load = 0xfffffffa, match = 0xfffffffc
effective PWM: period=183105ns, duty_cycle=91553ns (5461Hz/50% PWM)
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
---
drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
From: David Rivshin Allworx <hidden> Date: 2016-01-30 04:26:52
From: David Rivshin <redacted>
Add sanity checking to ensure that we do not program load or match values
that are out of range if a user requests period or duty_cycle values which
are not achievable. The match value cannot be less than the load value (but
can be equal), and neither can be 0xffffffff. This means that there must be
at least one fclk cycle between load and match, and another between match
and overflow.
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
---
drivers/pwm/pwm-omap-dmtimer.c | 31 +++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
@@ -142,6 +140,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,*Thenon-activetimeistheremainder:(DM_TIMER_MAX-match_value)*clockcycles.*+*NOTE:Itisrequiredthat:load_value<=match_value<DM_TIMER_MAX+**References:*OMAP4430/60/70TRMsections22.2.4.10and22.2.4.11*AM335xSitaraTRMsections20.1.3.5and20.1.3.6
@@ -149,6 +149,24 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,period_cycles=pwm_omap_dmtimer_get_clock_cycles(clk_rate,period_ns);duty_cycles=pwm_omap_dmtimer_get_clock_cycles(clk_rate,duty_ns);+if(period_cycles<2){+dev_info(chip->dev,+"period %dns is too short for clock rate %luHz\n",+period_ns,clk_rate);+gotoerr_einval;+}+if(duty_cycles<1){+dev_dbg(chip->dev,+"duty cycle %dns is too short for clock rate %luHz, using minimum of 1 clock cycle\n",+duty_ns,clk_rate);+duty_cycles=1;+}elseif(duty_cycles>=period_cycles){+dev_dbg(chip->dev,+"duty cycle %dns is too long for period %dns at clock rate %luHz, using maximum of 1 clock cycle less than period\n",+duty_ns,period_ns,clk_rate);+duty_cycles=period_cycles-1;+}+load_value=(DM_TIMER_MAX-period_cycles)+1;match_value=load_value+duty_cycles-1;
@@ -179,6 +197,11 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,mutex_unlock(&omap->mutex);return0;++err_einval:+mutex_unlock(&omap->mutex);++return-EINVAL;}staticintpwm_omap_dmtimer_set_polarity(structpwm_chip*chip,
From: David Rivshin Allworx <hidden> Date: 2016-01-30 04:26:53
From: David Rivshin <redacted>
When converting period and duty_cycle from nanoseconds to fclk cycles,
the error introduced by the integer division can be appreciable, especially
in the case of slow fclk or short period. Use DIV_ROUND_CLOSEST_ULL() so
that the error is kept to +/- 0.5 clock cycles.
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
---
drivers/pwm/pwm-omap-dmtimer.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
From: David Rivshin Allworx <hidden> Date: 2016-01-30 04:26:54
From: David Rivshin <redacted>
After going through the math and constraints checking to compute load and
match values, it is helpful to know what the resultant period and duty
cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I included it in
case it may be of use to someone in the future. I would have no issues with
dropping this if it's not considered worthwhile.
drivers/pwm/pwm-omap-dmtimer.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: Neil Armstrong <hidden> Date: 2016-01-30 14:51:06
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) [off-list ref]:
From: David Rivshin <redacted>
After going through the math and constraints checking to compute load and
match values, it is helpful to know what the resultant period and duty
cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I included it in
case it may be of use to someone in the future. I would have no issues with
dropping this if it's not considered worthwhile.
It's useful, but converting it as a sysfs attribute would be great !
Neil
From: Neil Armstrong <hidden> Date: 2016-01-30 14:52:12
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) [off-list ref]:
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
Hi David,
Thanks for the work, it seems all good, I'll test them on my side the
next days, but this work needed to be done for sure !
The only OMAP4 HW I have is a Pandaboard and it's very difficult to
get the timer outputs...
Neil
From: David Rivshin Allworx <hidden> Date: 2016-02-01 18:22:28
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to compute
load and match values, it is helpful to know what the resultant
period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I included
it in case it may be of use to someone in the future. I would have
no issues with dropping this if it's not considered worthwhile.
It's useful, but converting it as a sysfs attribute would be great !
Hrm, yes that is an interesting thought. I imagine that many PWM
devices have similar constraints, so perhaps that would be best
handled generically in the pwm core? Maybe as new optional get_*()
ops, a modification to the config() op to add output params, or just
updating new fields in the struct pwm directly?
From: David Rivshin Allworx <hidden> Date: 2016-02-01 18:35:03
On Fri, 29 Jan 2016 23:26:52 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
From: David Rivshin <redacted>
Add sanity checking to ensure that we do not program load or match
values that are out of range if a user requests period or duty_cycle
values which are not achievable. The match value cannot be less than
the load value (but can be equal), and neither can be 0xffffffff.
This means that there must be at least one fclk cycle between load
and match, and another between match and overflow.
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode
timers") Signed-off-by: David Rivshin [off-list ref]
---
[...]
quoted hunk
@@ -149,6 +149,24 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
period_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, period_ns);
duty_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, duty_ns);
+ if (period_cycles < 2) {
+ dev_info(chip->dev,
+ "period %dns is too short for clock rate %luHz\n",
+ period_ns, clk_rate);
+ goto err_einval;
+ }
[...]
I had some second thoughts on this over the weekend:
1) Perhaps the return should be -ERANGE instead of -EINVAL for this case?
2) Is dev_info() too severe for this? Perhaps dev_dbg() would be better?
Any preferences?
Hi,
* David Rivshin (Allworx) [off-list ref] [160201 10:23]:
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
quoted
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to compute
load and match values, it is helpful to know what the resultant
period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I included
it in case it may be of use to someone in the future. I would have
no issues with dropping this if it's not considered worthwhile.
It's useful, but converting it as a sysfs attribute would be great !
Hrm, yes that is an interesting thought. I imagine that many PWM
devices have similar constraints, so perhaps that would be best
handled generically in the pwm core? Maybe as new optional get_*()
ops, a modification to the config() op to add output params, or just
updating new fields in the struct pwm directly?
Yeah for /sys entry it should be Linux generic. The other option
is to use debugfs for driver specific things.
Regards,
Tony
From: David Rivshin Allworx <hidden> Date: 2016-02-01 20:14:09
On Sat, 30 Jan 2016 15:52:12 +0100
Neil Armstrong [off-list ref] wrote:
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst
case a requested period of 2/clk_rate would result in a real period
of 4/clk_rate instead. This is a series includes a fix for that
problem, as well as other related improvements, and is based on the
current linux-pwm/for-next tip.
I have tested on a Sitara AM335x platform, using a scope to verify
the output with a variety of periods and duty cycles. This includes
a PWM rate up clk_rate/2 with 50% duty cycle (e.g. generating
fclk/2) with both 32768Hz and 24MHz fclks. I do not have an OMAP4
board to test with, although appropriate sections in the the
reference manuals appear substantially the same, so I believe the
changes are equally correct there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not
be as reliable as I observed with Sitara. Although I suspect that
it's the same module and will also work, at least under some
circumstances. If anyone with OMAP4 hardware and a scope is so
inclined, I would be curious to know the results.
Hi David,
Thanks for the work, it seems all good, I'll test them on my side the
next days, but this work needed to be done for sure !
Thanks to you (and everyone else involved) for doing the heavy lifting
on the driver to begin with. It came just as I had a need for the PWM
output of a dmtimer, and came to the conclusion that just such a
timer-to-PWM adapter was needed.
The only OMAP4 HW I have is a Pandaboard and it's very difficult to
get the timer outputs...
For what its worth, I took a quick look at the Pandaboard Rev EA1
schematic, and I can see how it doesn't make using a timer PWM output
easy. Easiest option I found might be to change the pinmux for AG24
(h_GPIO_121) to be DMTIMER11_PWM_EVT, and probe it at S2 or J13.
On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
Hi,
* David Rivshin (Allworx) [off-list ref] [160201 10:23]:
quoted
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
quoted
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to compute
load and match values, it is helpful to know what the resultant
period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I included
it in case it may be of use to someone in the future. I would have
no issues with dropping this if it's not considered worthwhile.
It's useful, but converting it as a sysfs attribute would be great !
Hrm, yes that is an interesting thought. I imagine that many PWM
devices have similar constraints, so perhaps that would be best
handled generically in the pwm core? Maybe as new optional get_*()
ops, a modification to the config() op to add output params, or just
updating new fields in the struct pwm directly?
Yeah for /sys entry it should be Linux generic. The other option
is to use debugfs for driver specific things.
Let's go with debugfs for this one. This is used for diagnostic purposes
and not typically needed when configuring a PWM. While other drivers may
compute similar hardware-specific values, there's nothing generic to
them. The period and duty cycle are the generic input values and those
are already exposed via sysfs.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160202/9e212df5/attachment.sig>
From: David Rivshin Allworx <hidden> Date: 2016-02-02 23:44:43
On Tue, 2 Feb 2016 17:23:30 +0100
Thierry Reding [off-list ref] wrote:
On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
quoted
Hi,
* David Rivshin (Allworx) [off-list ref] [160201
10:23]:
quoted
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
quoted
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to
compute load and match values, it is helpful to know what the
resultant period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I
included it in case it may be of use to someone in the
future. I would have no issues with dropping this if it's not
considered worthwhile.
It's useful, but converting it as a sysfs attribute would be
great !
Hrm, yes that is an interesting thought. I imagine that many PWM
devices have similar constraints, so perhaps that would be best
handled generically in the pwm core? Maybe as new optional
get_*() ops, a modification to the config() op to add output
params, or just updating new fields in the struct pwm directly?
Yeah for /sys entry it should be Linux generic. The other option
is to use debugfs for driver specific things.
Let's go with debugfs for this one. This is used for diagnostic
I had looked at using the dbg_show() op to add some additional data.
One thing that discouraged me from going down that path was that
it replaced the call to pwm_dbg_show() for that chip. I wouldn't
want to loose the existing output, as it is useful, but also didn't
like the thought of duplicating the logic/formatting. I think some
way to have both the standard output and add extra output somewhere
(e.g. same line, line after each pwm_device, block after all
pwm_devices on the chip) would make that path more attractive.
Or were you thinking of a separate (per-chip) debugfs file for this
type of information?
purposes and not typically needed when configuring a PWM. While other
drivers may compute similar hardware-specific values, there's nothing
generic to them. The period and duty cycle are the generic input
values and those are already exposed via sysfs.
Just to clarify, what I was thinking of as generic was the concept
that "period/duty I asked for" and "period/duty I got" may be
different, sometimes to a substantial degree. I could imagine
userspace wanting to know the latter, which is what I think Neil
was suggesting.
For the sake of an example, the input clock for a dmtimer is sometimes
(often?) 32768Hz, which means that the real period and duty_cycle output
are limited to being a multiple of ~61035.2ns (16384Hz). So an attempt
to set a period of 100000ns (10KHz) would result in either 61035.2ns, or
122070.4ns (8192Hz), depending on the implementation of the driver (and
patch 2 changes behavior from the former to the latter). You can also
have cases where you desired a 33% duty cycle, but got a 25% duty cycle
instead.
In a quick look, I see substantially similar calculations (i.e.
clk_rate*period_ns/10^9) in most of the other pwm drivers, which makes
sense for any that are programmed in terms of some input clock. This
type of calculation almost guarantees that requested and actual period/
duty_cycle are going to be different to some degree. Some drivers look
like they have even more complicated behavior, apparently due to other
hardware constraints. For instance, fsl-ftm (among others) looks to be
using a power-of-2 prescaler to fit the period_cycles into a 16bit field.
Of course if the input clock rate for these types of devices is
sufficiently high (enough that the desired period is many input clock
cycles), then the difference between "desired" and "actual" is probably
small enough that noone cares. I'm not sure how common it is that
a) there is a substantial difference, and
b) userspace cares about it, and
c) userspace didn't carefully select an achievable value already
If noone has brought it up before, then I'd guess the answer is "not very".
From: Neil Armstrong <hidden> Date: 2016-02-03 10:23:43
On 01/30/2016 05:26 AM, David Rivshin (Allworx) wrote:
From: David Rivshin <redacted>
Fix the calculation of load_value and match_value. Currently they
are slightly too low, which produces a noticeably wrong PWM rate with
sufficiently short periods (i.e. when 1/period approaches clk_rate/2).
Example:
clk_rate=32768Hz, period=122070ns, duty_cycle=61035ns (8192Hz/50% PWM)
Correct values: load = 0xfffffffc, match = 0xfffffffd
Current values: load = 0xfffffffa, match = 0xfffffffc
effective PWM: period=183105ns, duty_cycle=91553ns (5461Hz/50% PWM)
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
OK for me.
Acked-by: Neil Armstrong <redacted>
Thanks !
From: Neil Armstrong <hidden> Date: 2016-02-03 10:24:23
On 02/01/2016 07:35 PM, David Rivshin (Allworx) wrote:
On Fri, 29 Jan 2016 23:26:52 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
quoted
From: David Rivshin <redacted>
Add sanity checking to ensure that we do not program load or match
values that are out of range if a user requests period or duty_cycle
values which are not achievable. The match value cannot be less than
the load value (but can be equal), and neither can be 0xffffffff.
This means that there must be at least one fclk cycle between load
and match, and another between match and overflow.
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode
timers") Signed-off-by: David Rivshin [off-list ref]
---
[...]
quoted
@@ -149,6 +149,24 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
period_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, period_ns);
duty_cycles = pwm_omap_dmtimer_get_clock_cycles(clk_rate, duty_ns);
+ if (period_cycles < 2) {
+ dev_info(chip->dev,
+ "period %dns is too short for clock rate %luHz\n",
+ period_ns, clk_rate);
+ goto err_einval;
+ }
[...]
I had some second thoughts on this over the weekend:
1) Perhaps the return should be -ERANGE instead of -EINVAL for this case?
2) Is dev_info() too severe for this? Perhaps dev_dbg() would be better?
Any preferences?
The current management is OK for me.
Acked-by: Neil Armstrong <redacted>
Thanks !
From: Neil Armstrong <hidden> Date: 2016-02-03 10:24:55
On 01/30/2016 05:26 AM, David Rivshin (Allworx) wrote:
From: David Rivshin <redacted>
When converting period and duty_cycle from nanoseconds to fclk cycles,
the error introduced by the integer division can be appreciable, especially
in the case of slow fclk or short period. Use DIV_ROUND_CLOSEST_ULL() so
that the error is kept to +/- 0.5 clock cycles.
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
OK for me.
Acked-by: Neil Armstrong <redacted>
Thanks !
On Tue, Feb 02, 2016 at 06:44:43PM -0500, David Rivshin (Allworx) wrote:
On Tue, 2 Feb 2016 17:23:30 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
quoted
Hi,
* David Rivshin (Allworx) [off-list ref] [160201
10:23]:
quoted
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
quoted
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to
compute load and match values, it is helpful to know what the
resultant period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I
included it in case it may be of use to someone in the
future. I would have no issues with dropping this if it's not
considered worthwhile.
It's useful, but converting it as a sysfs attribute would be
great !
Hrm, yes that is an interesting thought. I imagine that many PWM
devices have similar constraints, so perhaps that would be best
handled generically in the pwm core? Maybe as new optional
get_*() ops, a modification to the config() op to add output
params, or just updating new fields in the struct pwm directly?
Yeah for /sys entry it should be Linux generic. The other option
is to use debugfs for driver specific things.
Let's go with debugfs for this one. This is used for diagnostic
I had looked at using the dbg_show() op to add some additional data.
One thing that discouraged me from going down that path was that
it replaced the call to pwm_dbg_show() for that chip. I wouldn't
want to loose the existing output, as it is useful, but also didn't
like the thought of duplicating the logic/formatting. I think some
way to have both the standard output and add extra output somewhere
(e.g. same line, line after each pwm_device, block after all
pwm_devices on the chip) would make that path more attractive.
Or were you thinking of a separate (per-chip) debugfs file for this
type of information?
Yes, generally I'd prefer a separate, chip-specific debugfs file for
extra information. To be honest I'm not entirely sure of the usefulness
of the pwm_dbg_show() or letting drivers override it. But that's
slightly off-topic. However...
quoted
purposes and not typically needed when configuring a PWM. While other
drivers may compute similar hardware-specific values, there's nothing
generic to them. The period and duty cycle are the generic input
values and those are already exposed via sysfs.
Just to clarify, what I was thinking of as generic was the concept
that "period/duty I asked for" and "period/duty I got" may be
different, sometimes to a substantial degree. I could imagine
userspace wanting to know the latter, which is what I think Neil
was suggesting.
For the sake of an example, the input clock for a dmtimer is sometimes
(often?) 32768Hz, which means that the real period and duty_cycle output
are limited to being a multiple of ~61035.2ns (16384Hz). So an attempt
to set a period of 100000ns (10KHz) would result in either 61035.2ns, or
122070.4ns (8192Hz), depending on the implementation of the driver (and
patch 2 changes behavior from the former to the latter). You can also
have cases where you desired a 33% duty cycle, but got a 25% duty cycle
instead.
In a quick look, I see substantially similar calculations (i.e.
clk_rate*period_ns/10^9) in most of the other pwm drivers, which makes
sense for any that are programmed in terms of some input clock. This
type of calculation almost guarantees that requested and actual period/
duty_cycle are going to be different to some degree. Some drivers look
like they have even more complicated behavior, apparently due to other
hardware constraints. For instance, fsl-ftm (among others) looks to be
using a power-of-2 prescaler to fit the period_cycles into a 16bit field.
Of course if the input clock rate for these types of devices is
sufficiently high (enough that the desired period is many input clock
cycles), then the difference between "desired" and "actual" is probably
small enough that noone cares. I'm not sure how common it is that
a) there is a substantial difference, and
b) userspace cares about it, and
c) userspace didn't carefully select an achievable value already
If noone has brought it up before, then I'd guess the answer is "not very".
... I think perhaps a better way yet would be to have drivers update the
period and duty cycle to the actual values. That way there won't be a
delta between what's really being programmed to hardware and what shows
up in sysfs. It would also give users a chance to check if what's been
programmed is within an acceptable range or not.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160203/874a22d4/attachment-0001.sig>
From: David Rivshin Allworx <hidden> Date: 2016-02-05 19:51:46
On Wed, 3 Feb 2016 15:14:54 +0100
Thierry Reding [off-list ref] wrote:
On Tue, Feb 02, 2016 at 06:44:43PM -0500, David Rivshin (Allworx)
wrote:
quoted
On Tue, 2 Feb 2016 17:23:30 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
quoted
Hi,
* David Rivshin (Allworx) [off-list ref] [160201
10:23]:
quoted
On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong [off-list ref] wrote:
quoted
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
[off-list ref]:
quoted
From: David Rivshin <redacted>
After going through the math and constraints checking to
compute load and match values, it is helpful to know what
the resultant period and duty cycle are.
Signed-off-by: David Rivshin <redacted>
---
I found this helpful while testing the other changes, so I
included it in case it may be of use to someone in the
future. I would have no issues with dropping this if it's
not considered worthwhile.
It's useful, but converting it as a sysfs attribute would be
great !
Hrm, yes that is an interesting thought. I imagine that many
PWM devices have similar constraints, so perhaps that would
be best handled generically in the pwm core? Maybe as new
optional get_*() ops, a modification to the config() op to
add output params, or just updating new fields in the struct
pwm directly?
Yeah for /sys entry it should be Linux generic. The other option
is to use debugfs for driver specific things.
Let's go with debugfs for this one. This is used for diagnostic
I had looked at using the dbg_show() op to add some additional
data. One thing that discouraged me from going down that path was
that it replaced the call to pwm_dbg_show() for that chip. I
wouldn't want to loose the existing output, as it is useful, but
also didn't like the thought of duplicating the logic/formatting. I
think some way to have both the standard output and add extra
output somewhere (e.g. same line, line after each pwm_device, block
after all pwm_devices on the chip) would make that path more
attractive.
Or were you thinking of a separate (per-chip) debugfs file for this
type of information?
Yes, generally I'd prefer a separate, chip-specific debugfs file for
Makes sense.
extra information. To be honest I'm not entirely sure of the
usefulness of the pwm_dbg_show() or letting drivers override it. But
I did notice that no (in-tree) drivers were currently using
pwm_dbg_show()...
that's slightly off-topic. However...
quoted
quoted
purposes and not typically needed when configuring a PWM. While
other drivers may compute similar hardware-specific values,
there's nothing generic to them. The period and duty cycle are
the generic input values and those are already exposed via
sysfs.
Just to clarify, what I was thinking of as generic was the concept
that "period/duty I asked for" and "period/duty I got" may be
different, sometimes to a substantial degree. I could imagine
userspace wanting to know the latter, which is what I think Neil
was suggesting.
For the sake of an example, the input clock for a dmtimer is
sometimes (often?) 32768Hz, which means that the real period and
duty_cycle output are limited to being a multiple of ~61035.2ns
(16384Hz). So an attempt to set a period of 100000ns (10KHz) would
result in either 61035.2ns, or 122070.4ns (8192Hz), depending on
the implementation of the driver (and patch 2 changes behavior from
the former to the latter). You can also have cases where you
desired a 33% duty cycle, but got a 25% duty cycle instead.
In a quick look, I see substantially similar calculations (i.e.
clk_rate*period_ns/10^9) in most of the other pwm drivers, which
makes sense for any that are programmed in terms of some input
clock. This type of calculation almost guarantees that requested
and actual period/ duty_cycle are going to be different to some
degree. Some drivers look like they have even more complicated
behavior, apparently due to other hardware constraints. For
instance, fsl-ftm (among others) looks to be using a power-of-2
prescaler to fit the period_cycles into a 16bit field.
Of course if the input clock rate for these types of devices is
sufficiently high (enough that the desired period is many input
clock cycles), then the difference between "desired" and "actual"
is probably small enough that noone cares. I'm not sure how common
it is that
a) there is a substantial difference, and
b) userspace cares about it, and
c) userspace didn't carefully select an achievable value already
If noone has brought it up before, then I'd guess the answer is
"not very".
... I think perhaps a better way yet would be to have drivers update
the period and duty cycle to the actual values. That way there won't
be a delta between what's really being programmed to hardware and
what shows up in sysfs. It would also give users a chance to check if
what's been programmed is within an acceptable range or not.
From a userspace ABI point of view, I was unsure whether it was
considered OK for a R/W sysfs attribute of this nature to read back
differently than it was set. To follow the previous example:
# echo 100000 > period
# cat period
122070
I didn't find any obvious precedents for that. Would it potentially
confuse userspace? Is the answer different for new drivers vs
existing ones? The obvious alternative would be a separate file (e.g.
"actual_period", or somesuch name), which avoids those questions, but
I can see how that could be considered less clean. I don't have any
preference either way.
If just updating the existing period/duty_cycle files with the
actual values is the way to go, then there is a second question:
should the struct pwm_device track both requested and actual
values in separate fields for kernel-internal users? Or should
the existing period/duty_cycle fields just be modified to hold
the actual values?
Either way, I think any changes along these lines would probably be
cleaner ontop of the atomic update work (assuming some form of that
gets merged). And since it would also be outside the scope of this
patchset, you may still want to consider the simple debug message
in this patch as at least an interim state.
From: Neil Armstrong <hidden> Date: 2016-02-09 12:49:13
On 02/05/2016 08:51 PM, David Rivshin (Allworx) wrote:
Either way, I think any changes along these lines would probably be
cleaner ontop of the atomic update work (assuming some form of that
gets merged). And since it would also be outside the scope of this
patchset, you may still want to consider the simple debug message
in this patch as at least an interim state.
Sure, we should push these patches and rework this one later.
Acked-by: Neil Armstrong <redacted>
Thanks,
Neil
From: Adam Ford <hidden> Date: 2016-02-15 20:24:17
I tested this on a DM3730, Timer 10
Tested-by: Adam Ford <redacted>
On Wed, Feb 3, 2016 at 4:23 AM, Neil Armstrong [off-list ref] wrote:
On 01/30/2016 05:26 AM, David Rivshin (Allworx) wrote:
quoted
From: David Rivshin <redacted>
Fix the calculation of load_value and match_value. Currently they
are slightly too low, which produces a noticeably wrong PWM rate with
sufficiently short periods (i.e. when 1/period approaches clk_rate/2).
Example:
clk_rate=32768Hz, period=122070ns, duty_cycle=61035ns (8192Hz/50% PWM)
Correct values: load = 0xfffffffc, match = 0xfffffffd
Current values: load = 0xfffffffa, match = 0xfffffffc
effective PWM: period=183105ns, duty_cycle=91553ns (5461Hz/50% PWM)
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
OK for me.
Acked-by: Neil Armstrong <redacted>
Thanks !
@@ -99,7 +100,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,intduty_ns,intperiod_ns){structpwm_omap_dmtimer_chip*omap=to_pwm_omap_dmtimer_chip(chip);-intload_value,match_value;+u32period_cycles,duty_cycles;+u32load_value,match_value;structclk*fclk;unsignedlongclk_rate;booltimer_active;
@@ -133,11 +135,22 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,/**Calculatetheappropriateloadandmatchvaluesbasedonthe*specifiedperiodanddutycycle.Theloadvaluedeterminesthe-*cycletimeandthematchvaluedeterminesthedutycycle.+*periodtimeandthematchvaluedeterminesthedutytime.+*+*Theperiodlastsfor(DM_TIMER_MAX-load_value+1)clockcycles.+*Similarly,theactivetimelasts(match_value-load_value+1)cycles.+*Thenon-activetimeistheremainder:(DM_TIMER_MAX-match_value)+*clockcycles.+*+*References:+*OMAP4430/60/70TRMsections22.2.4.10and22.2.4.11+*AM335xSitaraTRMsections20.1.3.5and20.1.3.6*/-load_value=pwm_omap_dmtimer_calc_value(clk_rate,period_ns);-match_value=pwm_omap_dmtimer_calc_value(clk_rate,-period_ns-duty_ns);+period_cycles=pwm_omap_dmtimer_get_clock_cycles(clk_rate,period_ns);+duty_cycles=pwm_omap_dmtimer_get_clock_cycles(clk_rate,duty_ns);++load_value=(DM_TIMER_MAX-period_cycles)+1;+match_value=load_value+duty_cycles-1;/**WeMUSTstoptheassociateddual-modetimerbeforeattemptingto
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Rivshin Allworx <hidden> Date: 2016-02-27 01:31:00
On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
Hi Thierry,
Gentle ping. It does not look like you've taken this series, and I
wanted to make sure you're not waiting on something from me. It would
be nice to get at least the first patch into 4.5, if possible.
Thanks!
On Fri, Jan 29, 2016 at 11:26:51PM -0500, David Rivshin (Allworx) wrote:
From: David Rivshin <redacted>
Fix the calculation of load_value and match_value. Currently they
are slightly too low, which produces a noticeably wrong PWM rate with
sufficiently short periods (i.e. when 1/period approaches clk_rate/2).
Example:
clk_rate=32768Hz, period=122070ns, duty_cycle=61035ns (8192Hz/50% PWM)
Correct values: load = 0xfffffffc, match = 0xfffffffd
Current values: load = 0xfffffffa, match = 0xfffffffc
effective PWM: period=183105ns, duty_cycle=91553ns (5461Hz/50% PWM)
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
---
drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
On Fri, Jan 29, 2016 at 11:26:53PM -0500, David Rivshin (Allworx) wrote:
From: David Rivshin <redacted>
When converting period and duty_cycle from nanoseconds to fclk cycles,
the error introduced by the integer division can be appreciable, especially
in the case of slow fclk or short period. Use DIV_ROUND_CLOSEST_ULL() so
that the error is kept to +/- 0.5 clock cycles.
Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")
Signed-off-by: David Rivshin <redacted>
---
drivers/pwm/pwm-omap-dmtimer.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
On Fri, Feb 26, 2016 at 08:31:00PM -0500, David Rivshin (Allworx) wrote:
On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
quoted
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
Hi Thierry,
Gentle ping. It does not look like you've taken this series, and I
wanted to make sure you're not waiting on something from me. It would
be nice to get at least the first patch into 4.5, if possible.
I've applied patches 1 and 3, and I'm planning on sending out a pull
request for inclusion in v4.5-rc7 later on.
Patches 2 and 4 didn't seem ready/critical, so let's finish those up
for v4.6-rc1.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160304/9075a63b/attachment.sig>
From: David Rivshin Allworx <hidden> Date: 2016-03-04 16:27:36
On Fri, 4 Mar 2016 16:19:48 +0100
Thierry Reding [off-list ref] wrote:
On Fri, Feb 26, 2016 at 08:31:00PM -0500, David Rivshin (Allworx) wrote:
quoted
On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
quoted
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
Hi Thierry,
Gentle ping. It does not look like you've taken this series, and I
wanted to make sure you're not waiting on something from me. It would
be nice to get at least the first patch into 4.5, if possible.
I've applied patches 1 and 3, and I'm planning on sending out a pull
request for inclusion in v4.5-rc7 later on.
Thanks!
Patches 2 and 4 didn't seem ready/critical, so let's finish those up
for v4.6-rc1.
I know there was a lot of discussion on 4, but I'm not sure what the
concern is on patch 2. Is there something specific you're thinking of?
FYI, I know that Adam Ford is using this driver as the backend for
a pwm-backlight control. Without patch 2 this driver will not configure
the HW in a legal way at 0 or 100% duty cycle. However, I forget what
the practical effect of that is, and Adam seemed to indicate it was OK
for his purposes.
On Fri, Mar 04, 2016 at 11:27:36AM -0500, David Rivshin (Allworx) wrote:
On Fri, 4 Mar 2016 16:19:48 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Fri, Feb 26, 2016 at 08:31:00PM -0500, David Rivshin (Allworx) wrote:
quoted
On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
quoted
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
Hi Thierry,
Gentle ping. It does not look like you've taken this series, and I
wanted to make sure you're not waiting on something from me. It would
be nice to get at least the first patch into 4.5, if possible.
I've applied patches 1 and 3, and I'm planning on sending out a pull
request for inclusion in v4.5-rc7 later on.
Thanks!
quoted
Patches 2 and 4 didn't seem ready/critical, so let's finish those up
for v4.6-rc1.
I know there was a lot of discussion on 4, but I'm not sure what the
concern is on patch 2. Is there something specific you're thinking of?
Patch 2 sounded like some optional sanity checking which we didn't hit
anyway in the current code. Hence I didn't consider it a fix.
FYI, I know that Adam Ford is using this driver as the backend for
a pwm-backlight control. Without patch 2 this driver will not configure
the HW in a legal way at 0 or 100% duty cycle. However, I forget what
the practical effect of that is, and Adam seemed to indicate it was OK
for his purposes.
From: David Rivshin Allworx <hidden> Date: 2016-03-04 23:20:13
On Fri, 4 Mar 2016 22:18:38 +0100
Thierry Reding [off-list ref] wrote:
On Fri, Mar 04, 2016 at 11:27:36AM -0500, David Rivshin (Allworx) wrote:
quoted
On Fri, 4 Mar 2016 16:19:48 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Fri, Feb 26, 2016 at 08:31:00PM -0500, David Rivshin (Allworx) wrote:
quoted
On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
quoted
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
Hi Thierry,
Gentle ping. It does not look like you've taken this series, and I
wanted to make sure you're not waiting on something from me. It would
be nice to get at least the first patch into 4.5, if possible.
I've applied patches 1 and 3, and I'm planning on sending out a pull
request for inclusion in v4.5-rc7 later on.
Thanks!
quoted
Patches 2 and 4 didn't seem ready/critical, so let's finish those up
for v4.6-rc1.
I know there was a lot of discussion on 4, but I'm not sure what the
concern is on patch 2. Is there something specific you're thinking of?
Patch 2 sounded like some optional sanity checking which we didn't hit
anyway in the current code. Hence I didn't consider it a fix.
Hrm, perhaps there is something I should have added/changed in the
patch description to clarify?
For further background, patch 2 protects against things that are legal
in the PWM API itself, but not possible for this hardware to actually do.
Specifically a very short period, or duty_cycle too close to either 0 or
100%. Without the checking, the natural result of the normal math results
in setting up the HW in non-sensical ways.
As long as noone attempts to configure the PWM in those ways, then I'd
agree it's not critical. What made me think it was more important was
when I saw that Adam (and so probably others) used it for a PWM-backlight,
which will naturally try to set 0 and 100% duty cycle.
quoted
FYI, I know that Adam Ford is using this driver as the backend for
a pwm-backlight control. Without patch 2 this driver will not configure
the HW in a legal way at 0 or 100% duty cycle. However, I forget what
the practical effect of that is, and Adam seemed to indicate it was OK
for his purposes.
Okay, I'll hold back a little longer to give you some time to test.
FYI, I just went and retested what the effect is without those checks.
If the duty_cycle is set too low, the result is that the output is constant
high. If the duty_cycle is set too high, the result is that the output is
constant low. IOW, the result is the reverse of what the user requested.
If the period is too low, then you also get one of those two results,
depending on what the duty_cycle is (essentially the duty_cycle is always
too close to 0/100%).
With patch 2, it will clamp the duty_cycle to the lowest/highest possible
value. So while imperfect, at least isn't the reverse of the requested
behavior.
From: Adam Ford <hidden> Date: 2016-03-08 23:23:53
On Fri, Mar 4, 2016 at 5:20 PM, David Rivshin (Allworx)
[off-list ref] wrote:
On Fri, 4 Mar 2016 22:18:38 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Fri, Mar 04, 2016 at 11:27:36AM -0500, David Rivshin (Allworx) wrote:
quoted
On Fri, 4 Mar 2016 16:19:48 +0100
Thierry Reding [off-list ref] wrote:
quoted
On Fri, Feb 26, 2016 at 08:31:00PM -0500, David Rivshin (Allworx) wrote:
quoted
On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" [off-list ref] wrote:
quoted
From: David Rivshin <redacted>
When using a short PWM period (approaching the min of 2/clk_rate),
pwm-omap-dmtimer does not produce accurate results. In the worst case a
requested period of 2/clk_rate would result in a real period of 4/clk_rate
instead. This is a series includes a fix for that problem, as well as
other related improvements, and is based on the current linux-pwm/for-next
tip.
I have tested on a Sitara AM335x platform, using a scope to verify the
output with a variety of periods and duty cycles. This includes a PWM
rate up clk_rate/2 with 50% duty cycle (e.g. generating fclk/2) with
both 32768Hz and 24MHz fclks. I do not have an OMAP4 board to test with,
although appropriate sections in the the reference manuals appear
substantially the same, so I believe the changes are equally correct
there.
Note that the OMAP4 TRMs do effectively state that the maximum PWM
rate is clk_rate/4, so at very fast PWM rates the behavior may not be
as reliable as I observed with Sitara. Although I suspect that it's
the same module and will also work, at least under some circumstances.
If anyone with OMAP4 hardware and a scope is so inclined, I would be
curious to know the results.
David Rivshin (4):
pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
pwm: omap-dmtimer: add sanity checking for load and match values
pwm: omap-dmtimer: round load and match values rather than truncate
pwm: omap-dmtimer: add dev_dbg() message for effective period and duty
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++----------
1 file changed, 55 insertions(+), 16 deletions(-)
Hi Thierry,
Gentle ping. It does not look like you've taken this series, and I
wanted to make sure you're not waiting on something from me. It would
be nice to get at least the first patch into 4.5, if possible.
I've applied patches 1 and 3, and I'm planning on sending out a pull
request for inclusion in v4.5-rc7 later on.
Thanks!
quoted
Patches 2 and 4 didn't seem ready/critical, so let's finish those up
for v4.6-rc1.
I know there was a lot of discussion on 4, but I'm not sure what the
concern is on patch 2. Is there something specific you're thinking of?
Patch 2 sounded like some optional sanity checking which we didn't hit
anyway in the current code. Hence I didn't consider it a fix.
Hrm, perhaps there is something I should have added/changed in the
patch description to clarify?
For further background, patch 2 protects against things that are legal
in the PWM API itself, but not possible for this hardware to actually do.
Specifically a very short period, or duty_cycle too close to either 0 or
100%. Without the checking, the natural result of the normal math results
in setting up the HW in non-sensical ways.
As long as noone attempts to configure the PWM in those ways, then I'd
agree it's not critical. What made me think it was more important was
when I saw that Adam (and so probably others) used it for a PWM-backlight,
which will naturally try to set 0 and 100% duty cycle.
quoted
quoted
FYI, I know that Adam Ford is using this driver as the backend for
a pwm-backlight control. Without patch 2 this driver will not configure
the HW in a legal way at 0 or 100% duty cycle. However, I forget what
the practical effect of that is, and Adam seemed to indicate it was OK
for his purposes.
Okay, I'll hold back a little longer to give you some time to test.
FYI, I just went and retested what the effect is without those checks.
If the duty_cycle is set too low, the result is that the output is constant
high. If the duty_cycle is set too high, the result is that the output is
constant low. IOW, the result is the reverse of what the user requested.
If the period is too low, then you also get one of those two results,
depending on what the duty_cycle is (essentially the duty_cycle is always
too close to 0/100%).
With patch 2, it will clamp the duty_cycle to the lowest/highest possible
value. So while imperfect, at least isn't the reverse of the requested
behavior.
Sorry it took so long to test this, I had some construction at my
house so I was without a computer for a few days, then I put it on the
market and some showings, so I wasn't allowed to be home much over the
weekend.
I have finally tested this today. Applying all 4 patches appears have
no negative impact on what I'm doing. The perceived brightness
appears to work just fine, but I am not using an oscilloscope, so I
can't see the exact duty cycle or frequency.
Tested-By: Adam Ford <redacted>