pwm: omap-dmtimer: fix period/duty_cycle calculation
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
From: David Rivshin <[email protected]>
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(-)
Signature · 1 line
2.5.0
PATCH 1/4 pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
From: David Rivshin <[email protected]>
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)
--- drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
drivers/pwm/pwm-omap-dmtimer.c +20 -7
@@ -31,6 +31,7 @@#include <linux/time.h> #define DM_TIMER_LOAD_MIN 0xfffffffe +#define DM_TIMER_MAX 0xffffffffstruct pwm_omap_dmtimer_chip {struct pwm_chip chip;
@@ -46,13 +47,13 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip) return container_of(chip, struct pwm_omap_dmtimer_chip, chip); } -static int pwm_omap_dmtimer_calc_value (unsigned long clk_rate,int ns) +static u32 pwm_omap_dmtimer_get_clock_cycles (unsigned long clk_rate,int ns) {u64 c = (u64 )clk_rate * ns;do_div (c,NSEC_PER_SEC ); -return DM_TIMER_LOAD_MIN - c; +return c; }static void pwm_omap_dmtimer_start (struct pwm_omap_dmtimer_chip *omap)
@@ -99,7 +100,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, int duty_ns, int period_ns) {struct pwm_omap_dmtimer_chip *omap =to_pwm_omap_dmtimer_chip (chip); -int load_value, match_value; +u32 period_cycles, duty_cycles; +u32 load_value, match_value;struct clk *fclk;unsigned long clk_rate;bool timer_active;
@@ -133,11 +135,22 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,/* * 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. +* period time and the match value determines the duty time. +* +* The period lasts for (DM_TIMER_MAX-load_value+1) clock cycles. +* Similarly, the active time lasts (match_value-load_value+1) cycles. +* The non-active time is the remainder: (DM_TIMER_MAX-match_value) +* clock cycles. +* +* References: +* OMAP4430/60/70 TRM sections 22.2.4.10 and 22.2.4.11 +* AM335x Sitara TRM sections 20.1.3.5 and 20.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 ;/* * We MUST stop the associated dual-mode timer before attempting to
Signature · 1 line
2.5.0
PATCH 1/4 pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
Quoted 6 lines · David Rivshin (Allworx)
From: David Rivshin <[email protected]>
Fix the calculation of
load_valueandmatch_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).
Quoted 8 lines
Example:
clk_rate=32768Hz, period=122070ns, duty_cycle=61035ns (8192Hz/50% PWM) Correct values: load =0xfffffffc, match =0xfffffffdCurrent values: load =0xfffffffa, match =0xfffffffceffective 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 <[email protected]>
OK for me.
Thanks !
Quoted 70 lines
--- drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)drivers/pwm/pwm-omap-dmtimer.c +20
-7@@ -31,6 +31,7 @@#include <linux/time.h> #define DM_TIMER_LOAD_MIN 0xfffffffe +#define DM_TIMER_MAX 0xffffffffstruct pwm_omap_dmtimer_chip {struct pwm_chip chip;@@ -46,13 +47,13 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip) return container_of(chip, struct pwm_omap_dmtimer_chip, chip); } -static int pwm_omap_dmtimer_calc_value (unsigned long clk_rate,int ns) +static u32 pwm_omap_dmtimer_get_clock_cycles (unsigned long clk_rate,int ns) {u64 c = (u64 )clk_rate * ns;do_div (c,NSEC_PER_SEC ); -return DM_TIMER_LOAD_MIN - c; +return c; }static void pwm_omap_dmtimer_start (struct pwm_omap_dmtimer_chip *omap)@@ -99,7 +100,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, int duty_ns, int period_ns) {struct pwm_omap_dmtimer_chip *omap =to_pwm_omap_dmtimer_chip (chip); -int load_value, match_value; +u32 period_cycles, duty_cycles; +u32 load_value, match_value;struct clk *fclk;unsigned long clk_rate;bool timer_active;@@ -133,11 +135,22 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,/* * 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. +* period time and the match value determines the duty time. +* +* The period lasts for (DM_TIMER_MAX-load_value+1) clock cycles. +* Similarly, the active time lasts (match_value-load_value+1) cycles. +* The non-active time is the remainder: (DM_TIMER_MAX-match_value) +* clock cycles. +* +* References: +* OMAP4430/60/70 TRM sections 22.2.4.10 and 22.2.4.11 +* AM335x Sitara TRM sections 20.1.3.5 and 20.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 ;/* * We MUST stop the associated dual-mode timer before attempting to
PATCH 1/4 pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
I tested this on a DM3730, Timer 10
Quoted 97 lines · Neil Armstrong
Quoted 6 lines · David Rivshin (Allworx)
From: David Rivshin <[email protected]>
Fix the calculation of
load_valueandmatch_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).Quoted 8 lines
Example:
clk_rate=32768Hz, period=122070ns, duty_cycle=61035ns (8192Hz/50% PWM) Correct values: load =0xfffffffc, match =0xfffffffdCurrent values: load =0xfffffffa, match =0xfffffffceffective 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 <[email protected]>OK for me.
Acked-by Neil Armstrong <[email protected]>Thanks !
Quoted 60 lines
--- drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)drivers/pwm/pwm-omap-dmtimer.c +15
-5@@ -31,6 +31,7 @@#include <linux/time.h> #define DM_TIMER_LOAD_MIN 0xfffffffe +#define DM_TIMER_MAX 0xffffffffstruct pwm_omap_dmtimer_chip {struct pwm_chip chip;@@ -46,13 +47,13 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip) return container_of(chip, struct pwm_omap_dmtimer_chip, chip); } -static int pwm_omap_dmtimer_calc_value (unsigned long clk_rate,int ns) +static u32 pwm_omap_dmtimer_get_clock_cycles (unsigned long clk_rate,int ns) {u64 c = (u64 )clk_rate * ns;do_div (c,NSEC_PER_SEC ); -return DM_TIMER_LOAD_MIN - c; +return c; }static void pwm_omap_dmtimer_start (struct pwm_omap_dmtimer_chip *omap)@@ -99,7 +100,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, int duty_ns, int period_ns) {struct pwm_omap_dmtimer_chip *omap =to_pwm_omap_dmtimer_chip (chip); -int load_value, match_value; +u32 period_cycles, duty_cycles; +u32 load_value, match_value;struct clk *fclk;unsigned long clk_rate;bool timer_active;@@ -133,11 +135,22 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,/* * 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. +* period time and the match value determines the duty time. +* +* The period lasts for (DM_TIMER_MAX-load_value+1) clock cycles. +* Similarly, the active time lasts (match_value-load_value+1) cycles. +* The non-active time is the remainder: (DM_TIMER_MAX-match_value) +* clock cycles. +* +* References: +* OMAP4430/60/70 TRM sections 22.2.4.10 and 22.2.4.11 +* AM335x Sitara TRM sections 20.1.3.5 and 20.1.3.6 */ - load_value =pwm_omap_dmtimer_calc_value (clk_rate, period_ns);drivers/pwm/pwm-omap-dmtimer.c static int pwm_omap_dmtimer_config(struct pwm_chip *chip, · line 156
Quoted 10 lines
drivers/pwm/pwm-omap-dmtimer.c +5
-2- 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 ;/* * We MUST stop the associated dual-mode timer before attempting to --
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
Thierry Reding
AppliedPATCH 1/4 pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation
In reply to David Rivshin (Allworx)Quoted 12 lines · David Rivshin (Allworx)
From: David Rivshin <[email protected]>
Fix the calculation of
load_valueandmatch_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 =0xfffffffdCurrent values: load =0xfffffffa, match =0xfffffffceffective PWM: period=183105ns, duty_cycle=91553ns (5461Hz/50% PWM)
Quoted 5 lines
Fixes 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")Signed-off-by David Rivshin <[email protected]>--- drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)
Applied, thanks.
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/411a42e6/attachment.sig>
PATCH 2/4 pwm: omap-dmtimer: add sanity checking for load and match values
In reply to David Rivshin (Allworx)From: David Rivshin <[email protected]>
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.
--- drivers/pwm/pwm-omap-dmtimer.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
drivers/pwm/pwm-omap-dmtimer.c +27 -4
@@ -119,15 +119,13 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, fclk = omap->pdata->get_fclk (omap->dm_timer);if (!fclk) {dev_err (chip->dev,"invalid pmtimer fclk\n" ); -mutex_unlock (&omap->mutex); -return -EINVAL ; + goto err_einval; } clk_rate =clk_get_rate (fclk);if (!clk_rate) {dev_err (chip->dev,"invalid pmtimer fclk rate\n" ); -mutex_unlock (&omap->mutex); -return -EINVAL ; + goto err_einval; }dev_dbg (chip->dev,"clk rate: %luHz\n" , clk_rate);
@@ -142,6 +140,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,* The non-active time is the remainder: (DM_TIMER_MAX-match_value) * clock cycles. * +* NOTE: It is required that: load_value <= match_value < DM_TIMER_MAX +* * References: * OMAP4430/60/70 TRM sections 22.2.4.10 and 22.2.4.11 * AM335x Sitara TRM sections 20.1.3.5 and 20.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); + goto err_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 ; + }else if (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);return 0 ; + +err_einval: +mutex_unlock (&omap->mutex); + +return -EINVAL ; } static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
Signature · 1 line
2.5.0
PATCH 2/4 pwm: omap-dmtimer: add sanity checking for load and match values
Quoted 12 lines · On Fri, 29 Jan 2016 23:26:52 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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_cyclevalues which are not achievable. The match value cannot be less than the load value (but can be equal), and neither can be0xffffffff. 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-modetimers") Signed-off-by: David Rivshin <[email protected]>
---
[...]
drivers/pwm/pwm-omap-dmtimer.c static int pwm_omap_dmtimer_config(struct pwm_chip *chip, · line 157
Quoted 10 lines
drivers/pwm/pwm-omap-dmtimer.c +6
-0@@ -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?
PATCH 2/4 pwm: omap-dmtimer: add sanity checking for load and match values
Quoted 28 lines · David Rivshin (Allworx)
Quoted 12 lines · On Fri, 29 Jan 2016 23:26:52 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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_cyclevalues which are not achievable. The match value cannot be less than the load value (but can be equal), and neither can be0xffffffff. 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-modetimers") Signed-off-by: David Rivshin <[email protected]>
---[...]
Quoted 10 lines
drivers/pwm/pwm-omap-dmtimer.c +6
-0@@ -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; + }[...]
Quoted 5 lines
I had some second thoughts on this over the weekend:
1) Perhaps the return should be-ERANGEinstead of-EINVALfor this case?
2) Isdev_info()too severe for this? Perhapsdev_dbg()would be better? Any preferences?
The current management is OK for me.
Thanks !
PATCH 3/4 pwm: omap-dmtimer: round load and match values rather than truncate
In reply to David Rivshin (Allworx)From: David Rivshin <[email protected]>
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.
--- drivers/pwm/pwm-omap-dmtimer.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/pwm/pwm-omap-dmtimer.c +1 -5
@@ -49,11 +49,7 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)static u32 pwm_omap_dmtimer_get_clock_cycles (unsigned long clk_rate,int ns) { -u64 c = (u64 )clk_rate * ns; - -do_div (c,NSEC_PER_SEC ); - -return c; +return DIV_ROUND_CLOSEST_ULL ((u64 )clk_rate * ns,NSEC_PER_SEC ); }static void pwm_omap_dmtimer_start (struct pwm_omap_dmtimer_chip *omap)
Signature · 1 line
2.5.0
PATCH 3/4 pwm: omap-dmtimer: round load and match values rather than truncate
Quoted 9 lines · David Rivshin (Allworx)
From: David Rivshin <[email protected]>
When converting period and
duty_cyclefrom nanoseconds to fclk cycles, the error introduced by the integer division can be appreciable, especially in the case of slow fclk or short period. UseDIV_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 <[email protected]>
OK for me.
Thanks !
Quoted 22 lines
--- drivers/pwm/pwm-omap-dmtimer.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)drivers/pwm/pwm-omap-dmtimer.c +1
-5@@ -49,11 +49,7 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)static u32 pwm_omap_dmtimer_get_clock_cycles (unsigned long clk_rate,int ns) { -u64 c = (u64 )clk_rate * ns; - -do_div (c,NSEC_PER_SEC ); - -return c; +return DIV_ROUND_CLOSEST_ULL ((u64 )clk_rate * ns,NSEC_PER_SEC ); }static void pwm_omap_dmtimer_start (struct pwm_omap_dmtimer_chip *omap)
Thierry Reding
AppliedPATCH 3/4 pwm: omap-dmtimer: round load and match values rather than truncate
In reply to David Rivshin (Allworx)Quoted 7 lines · David Rivshin (Allworx)
From: David Rivshin <[email protected]>
When converting period and
duty_cyclefrom nanoseconds to fclk cycles, the error introduced by the integer division can be appreciable, especially in the case of slow fclk or short period. UseDIV_ROUND_CLOSEST_ULL()so that the error is kept to +/- 0.5 clock cycles.
Quoted 5 lines
Fixes 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers")Signed-off-by David Rivshin <[email protected]>--- drivers/pwm/pwm-omap-dmtimer.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Applied, thanks.
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/999afb7d/attachment.sig>
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
In reply to David Rivshin (Allworx)From: David Rivshin <[email protected]>
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.
---
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(-)
drivers/pwm/pwm-omap-dmtimer.c +8 -1
@@ -102,7 +102,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,unsigned long clk_rate;bool timer_active; -dev_dbg (chip->dev,"duty cycle: %d, period %d\n" , duty_ns, period_ns); +dev_dbg (chip->dev,"requested duty cycle: %dns, period: %dns\n" , + duty_ns, period_ns);mutex_lock (&omap->mutex); if (duty_ns == pwm_get_duty_cycle(pwm) &&
@@ -163,6 +164,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, duty_cycles = period_cycles -1 ; } +dev_dbg (chip->dev,"effective duty cycle: %lldns, period: %lldns\n" , +DIV_ROUND_CLOSEST_ULL ((u64 )NSEC_PER_SEC * duty_cycles, + clk_rate), +DIV_ROUND_CLOSEST_ULL ((u64 )NSEC_PER_SEC * period_cycles, + clk_rate)); + load_value = (DM_TIMER_MAX - period_cycles) +1 ; match_value = load_value + duty_cycles -1 ;
Signature · 1 line
2.5.0
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) <[email protected]>:
Quoted 6 lines
From: David Rivshin <[email protected]>
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.
Quoted 6 lines
Signed-off-by David Rivshin <[email protected]>---
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
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Quoted 8 lines · On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 6 lines
From: David Rivshin <[email protected]>
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.
Quoted 8 lines
Quoted 6 lines
Signed-off-by David Rivshin <[email protected]>---
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?
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Hi,
Quoted 18 lines · David Rivshin (Allworx)
Quoted 15 lines · On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 12 lines
From: David Rivshin <[email protected]>
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 <[email protected]>---
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.
Quoted 7 lines
Quoted 1 line
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
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Quoted 23 lines · Tony Lindgren
Hi,
Quoted 20 lines · David Rivshin (Allworx)
Quoted 16 lines · On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 6 lines
From: David Rivshin <[email protected]>
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.
Quoted 6 lines
Signed-off-by David Rivshin <[email protected]>---
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 !
Quoted 8 lines
Quoted 5 lines
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>
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Quoted 33 lines · On Tue, 2 Feb 2016 17:23:30 +0100 Thierry Reding <[email protected]>…
Quoted 32 lines · Tony Lindgren
Hi,
* David Rivshin (Allworx) <[email protected]> [160201 10:23]:
Quoted 27 lines
Quoted 11 lines · On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 9 lines
From: David Rivshin <[email protected]>
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 <[email protected]>---
Quoted 7 lines
Quoted 4 lines
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?
Quoted 4 lines
Quoted 2 lines
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?
Quoted 4 lines
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".
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Quoted 50 lines · David Rivshin (Allworx)
Quoted 33 lines · On Tue, 2 Feb 2016 17:23:30 +0100 Thierry Reding <[email protected]>…
Quoted 32 lines · Tony Lindgren
Hi,
* David Rivshin (Allworx) <[email protected]> [160201 10:23]:
Quoted 27 lines
Quoted 11 lines · On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 9 lines
From: David Rivshin <[email protected]>
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 <[email protected]>---
Quoted 7 lines
Quoted 4 lines
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?Quoted 4 lines
Quoted 2 lines
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 topwm_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 eachpwm_device, block after allpwm_deviceson the chip) would make that path more attractive.
Quoted 2 lines
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 29 lines
Quoted 4 lines
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_cycleoutput 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_cycleare 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 theperiod_cyclesinto a 16bit field.
Quoted 8 lines
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>
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Quoted 54 lines · On Wed, 3 Feb 2016 15:14:54 +0100 Thierry Reding <[email protected]>…
Quoted 52 lines · David Rivshin (Allworx)
Quoted 34 lines · On Tue, 2 Feb 2016 17:23:30 +0100 Thierry Reding <[email protected]>…
Quoted 33 lines · Tony Lindgren
Hi,
* David Rivshin (Allworx) <[email protected]> [160201 10:23]:
Quoted 28 lines
Quoted 11 lines · On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 9 lines
From: David Rivshin <[email protected]>
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 <[email protected]>---
Quoted 7 lines
Quoted 4 lines
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?Quoted 4 lines
Quoted 2 lines
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 topwm_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 eachpwm_device, block after allpwm_deviceson the chip) would make that path more attractive.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 2 lines
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()...
Quoted 44 lines
that's slightly off-topic. However...
Quoted 31 lines
Quoted 5 lines
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_cycleoutput 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_cycleare 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 theperiod_cyclesinto a 16bit field.Quoted 10 lines
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".
Quoted 7 lines
... 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.
PATCH 4/4 pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle
Quoted 7 lines · David Rivshin (Allworx)
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.
Thanks,
Neil
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
In reply to David Rivshin (Allworx)2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) <[email protected]>:
Quoted 17 lines
From: David Rivshin <[email protected]>
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.
Quoted 6 lines
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
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 26 lines · On Sat, 30 Jan 2016 15:52:12 +0100 Neil Armstrong <[email protected]>…
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
<[email protected]>:Quoted 17 lines
From: David Rivshin <[email protected]>
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.
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 2 lines
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.
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
In reply to David Rivshin (Allworx)Quoted 31 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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: adddev_dbg()message for effective period and duty
cycle
Quoted 3 lines
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!
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 38 lines · David Rivshin (Allworx)
Quoted 31 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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: adddev_dbg()message for effective period and duty
cycleQuoted 3 lines
drivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 16 deletions(-)
Quoted 5 lines
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>
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 39 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 38 lines · David Rivshin (Allworx)
Quoted 34 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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: adddev_dbg()message for effective period and duty
cycledrivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 16 deletions(-)
Quoted 8 lines
Quoted 5 lines
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 2 lines
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.
I am OK with it. 0% vs 1% is not perceivable and neither is 99% vs 100%.
Thanks!
Adam
Quoted 74 lines · David Rivshin (Allworx)
Quoted 8 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 7 lines · David Rivshin (Allworx)
Quoted 4 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
requested period of 2/clk_rate would result in a real period of
4/clk_rate
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
tip.
I have tested on a Sitara AM335x platform, using a scope to verify
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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,
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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: adddev_dbg()message for effective period andduty
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71++++++++++++++++++++++++++++++++----------
Quoted 11 lines
Quoted 8 lines
Quoted 2 lines
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 2 lines
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.
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 1 line · On Fri, 4 Mar 2016 10:29:07 -0600 Adam Ford <[email protected]> wrote:
I am OK with it. 0% vs 1% is not perceivable and neither is 99% vs 100%.
IIRC, you tested with none of these patches, and with all of them. You might want to double check the behavior with just patch 1 and 3, since that's what Thierry applied. I'm just concerned that the behavior now might be worse at one extreme or the other, compared to your previous tests.
Quoted 81 lines
Thanks!
Adam
Quoted 74 lines · David Rivshin (Allworx)
Quoted 8 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 7 lines · David Rivshin (Allworx)
Quoted 4 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
requested period of 2/clk_rate would result in a real period of
4/clk_rate
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
tip.
I have tested on a Sitara AM335x platform, using a scope to verify
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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,
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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: adddev_dbg()message for effective period andduty
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71++++++++++++++++++++++++++++++++----------
Quoted 11 lines
Quoted 8 lines
Quoted 2 lines
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 2 lines
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.
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 11 lines · David Rivshin (Allworx)
Quoted 1 line · On Fri, 4 Mar 2016 10:29:07 -0600 Adam Ford <[email protected]> wrote:
I am OK with it. 0% vs 1% is not perceivable and neither is 99% vs 100%.
IIRC, you tested with none of these patches, and with all of them. You might want to double check the behavior with just patch 1 and 3, since that's what Thierry applied. I'm just concerned that the behavior now might be worse at one extreme or the other, compared to your previous tests.
I will give them a try this afternoon and see how they are perceived. I am offsite, so I don't have an oscilloscope to actually observe the waveform.
adam
Quoted 82 lines
Quoted 81 lines
Thanks!
Adam
Quoted 74 lines · David Rivshin (Allworx)
Quoted 8 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 7 lines · David Rivshin (Allworx)
Quoted 4 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
requested period of 2/clk_rate would result in a real period of
4/clk_rate
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
tip.
I have tested on a Sitara AM335x platform, using a scope to verify
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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,
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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: adddev_dbg()message for effective period andduty
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
cycle
drivers/pwm/pwm-omap-dmtimer.c | 71++++++++++++++++++++++++++++++++----------
Quoted 11 lines
Quoted 8 lines
Quoted 2 lines
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 2 lines
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.
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
In reply to David Rivshin (Allworx)Quoted 51 lines · David Rivshin (Allworx)
Quoted 39 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 38 lines · David Rivshin (Allworx)
Quoted 34 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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: adddev_dbg()message for effective period and duty
cycledrivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 16 deletions(-)Quoted 8 lines
Quoted 5 lines
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.
Quoted 7 lines
Thanks!
Quoted 2 lines
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.
Quoted 5 lines
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.
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/83c0a526/attachment-0001.sig>
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 54 lines · On Fri, 4 Mar 2016 22:18:38 +0100 Thierry Reding <[email protected]>…
Quoted 53 lines · David Rivshin (Allworx)
Quoted 39 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 38 lines · David Rivshin (Allworx)
Quoted 34 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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: adddev_dbg()message for effective period and duty
cycledrivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 16 deletions(-)Quoted 8 lines
Quoted 5 lines
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 8 lines
Quoted 5 lines
Quoted 2 lines
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 7 lines
Quoted 5 lines
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.
cover pwm: omap-dmtimer: fix period/duty_cycle calculation
Quoted 93 lines · David Rivshin (Allworx)
Quoted 54 lines · On Fri, 4 Mar 2016 22:18:38 +0100 Thierry Reding <[email protected]>…
Quoted 53 lines · David Rivshin (Allworx)
Quoted 39 lines · On Fri, 4 Mar 2016 16:19:48 +0100 Thierry Reding <[email protected]>…
Quoted 38 lines · David Rivshin (Allworx)
Quoted 34 lines · On Fri, 29 Jan 2016 23:26:50 -0500 "David Rivshin (Allworx)"…
From: David Rivshin <[email protected]>
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: adddev_dbg()message for effective period and duty
cycledrivers/pwm/pwm-omap-dmtimer.c | 71 ++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 16 deletions(-)Quoted 8 lines
Quoted 5 lines
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 8 lines
Quoted 5 lines
Quoted 2 lines
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_cycletoo 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 7 lines
Quoted 5 lines
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_cycleis set too low, the result is that the output is constant high. If theduty_cycleis set too high, the result is that the output is constant low. IOW, the result is the reverse of what the user requested.
Quoted 7 lines
If the period is too low, then you also get one of those two results, depending on what the
duty_cycleis (essentially theduty_cycleis always too close to 0/100%).With patch 2, it will clamp the
duty_cycleto 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.