[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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

[PATCH 1/4] pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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 <drivshin@allworx.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 826634e..0083e75 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -31,6 +31,7 @@
 #include <linux/time.h>
 
 #define DM_TIMER_LOAD_MIN 0xfffffffe
+#define DM_TIMER_MAX      0xffffffff
 
 struct 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
-- 
2.5.0

[PATCH 1/4] pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation

[Neil Armstrong]

On 01/30/2016 05:26 AM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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 <drivshin@allworx.com>

OK for me.

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks !

> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 826634e..0083e75 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -31,6 +31,7 @@
>  #include <linux/time.h>
>  
>  #define DM_TIMER_LOAD_MIN 0xfffffffe
> +#define DM_TIMER_MAX      0xffffffff
>  
>  struct 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

[Adam Ford]

I tested this on a DM3730, Timer 10

Tested-by: Adam Ford <aford173@gmail.com>

On Wed, Feb 3, 2016 at 4:23 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 01/30/2016 05:26 AM, David Rivshin (Allworx) wrote:
>> From: David Rivshin <drivshin@allworx.com>
>>
>> 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 <drivshin@allworx.com>
>
> OK for me.
>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
>
> Thanks !
>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 27 ++++++++++++++++++++-------
>>  1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>> index 826634e..0083e75 100644
>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/time.h>
>>
>>  #define DM_TIMER_LOAD_MIN 0xfffffffe
>> +#define DM_TIMER_MAX      0xffffffff
>>
>>  struct 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
>>
>
> --
> 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

[PATCH 1/4] pwm: omap-dmtimer: fix inaccurate period/duty_cycle calculation

[Thierry Reding]

On Fri, Jan 29, 2016 at 11:26:51PM -0500, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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 <drivshin@allworx.com>
> ---
>  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

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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 <drivshin@allworx.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 0083e75..103d729 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -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,
-- 
2.5.0

[PATCH 2/4] pwm: omap-dmtimer: add sanity checking for load and match values

[David Rivshin (Allworx)]

On Fri, 29 Jan 2016 23:26:52 -0500
"David Rivshin (Allworx)" <drivshin.allworx@gmail.com> wrote:

> From: David Rivshin <drivshin@allworx.com>
> 
> 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 <drivshin@allworx.com>
> ---
 [...]
> @@ -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

[Neil Armstrong]

On 02/01/2016 07:35 PM, David Rivshin (Allworx) wrote:
> On Fri, 29 Jan 2016 23:26:52 -0500
> "David Rivshin (Allworx)" <drivshin.allworx@gmail.com> wrote:
> 
>> From: David Rivshin <drivshin@allworx.com>
>>
>> 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 <drivshin@allworx.com>
>> ---
>  [...]
>> @@ -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 <narmstrong@baylibre.com>

Thanks !

[PATCH 3/4] pwm: omap-dmtimer: round load and match values rather than truncate

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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 <drivshin@allworx.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 103d729..8c9953c 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -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)
-- 
2.5.0

[PATCH 3/4] pwm: omap-dmtimer: round load and match values rather than truncate

[Neil Armstrong]

On 01/30/2016 05:26 AM, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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 <drivshin@allworx.com>

OK for me.

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks !

> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 103d729..8c9953c 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -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)
> 

[PATCH 3/4] pwm: omap-dmtimer: round load and match values rather than truncate

[Thierry Reding]

On Fri, Jan 29, 2016 at 11:26:53PM -0500, David Rivshin (Allworx) wrote:
> From: David Rivshin <drivshin@allworx.com>
> 
> 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 <drivshin@allworx.com>
> ---
>  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

[David Rivshin (Allworx)]

From: David Rivshin <drivshin@allworx.com>

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 <drivshin@allworx.com>
---

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(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 8c9953c..44e3dca 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -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;
 
-- 
2.5.0

[PATCH 4/4] pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle

[Neil Armstrong]

2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) <drivshin.allworx@gmail.com>:
> From: David Rivshin <drivshin@allworx.com>
>
> 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 <drivshin@allworx.com>
> ---
>
> 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

[David Rivshin (Allworx)]

On Sat, 30 Jan 2016 15:51:06 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> <drivshin.allworx@gmail.com>:
> > From: David Rivshin <drivshin@allworx.com>
> >
> > 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 <drivshin@allworx.com>
> > ---
> >
> > 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

[Tony Lindgren]

Hi,

* David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 10:23]:
> On Sat, 30 Jan 2016 15:51:06 +0100
> Neil Armstrong <narmstrong@baylibre.com> wrote:
> 
> > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> > <drivshin.allworx@gmail.com>:
> > > From: David Rivshin <drivshin@allworx.com>
> > >
> > > 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 <drivshin@allworx.com>
> > > ---
> > >
> > > 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

[PATCH 4/4] pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle

[Thierry Reding]

On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 10:23]:
> > On Sat, 30 Jan 2016 15:51:06 +0100
> > Neil Armstrong <narmstrong@baylibre.com> wrote:
> > 
> > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> > > <drivshin.allworx@gmail.com>:
> > > > From: David Rivshin <drivshin@allworx.com>
> > > >
> > > > 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 <drivshin@allworx.com>
> > > > ---
> > > >
> > > > 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>

[PATCH 4/4] pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle

[David Rivshin (Allworx)]

On Tue, 2 Feb 2016 17:23:30 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
> > Hi,
> > 
> > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201
> > 10:23]:  
> > > On Sat, 30 Jan 2016 15:51:06 +0100
> > > Neil Armstrong <narmstrong@baylibre.com> wrote:
> > >   
> > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> > > > <drivshin.allworx@gmail.com>:  
> > > > > From: David Rivshin <drivshin@allworx.com>
> > > > >
> > > > > 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 <drivshin@allworx.com>
> > > > > ---
> > > > >
> > > > > 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".

[PATCH 4/4] pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle

[Thierry Reding]

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 <thierry.reding@gmail.com> wrote:
> 
> > On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201
> > > 10:23]:  
> > > > On Sat, 30 Jan 2016 15:51:06 +0100
> > > > Neil Armstrong <narmstrong@baylibre.com> wrote:
> > > >   
> > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> > > > > <drivshin.allworx@gmail.com>:  
> > > > > > From: David Rivshin <drivshin@allworx.com>
> > > > > >
> > > > > > 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 <drivshin@allworx.com>
> > > > > > ---
> > > > > >
> > > > > > 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...

> > 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>

[PATCH 4/4] pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle

[David Rivshin (Allworx)]

On Wed, 3 Feb 2016 15:14:54 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> 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 <thierry.reding@gmail.com> wrote:
> >   
> > > On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote:  
> > > > Hi,
> > > > 
> > > > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201
> > > > 10:23]:    
> > > > > On Sat, 30 Jan 2016 15:51:06 +0100
> > > > > Neil Armstrong <narmstrong@baylibre.com> wrote:
> > > > >     
> > > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> > > > > > <drivshin.allworx@gmail.com>:    
> > > > > > > From: David Rivshin <drivshin@allworx.com>
> > > > > > >
> > > > > > > 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 <drivshin@allworx.com>
> > > > > > > ---
> > > > > > >
> > > > > > > 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...
> 
> > > 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.

[PATCH 4/4] pwm: omap-dmtimer: add dev_dbg() message for effective period and duty cycle

[Neil Armstrong]

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 <narmstrong@baylibre.com>

Thanks,
Neil

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[Neil Armstrong]

2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) <drivshin.allworx@gmail.com>:
> From: David Rivshin <drivshin@allworx.com>
>
> 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

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[David Rivshin (Allworx)]

On Sat, 30 Jan 2016 15:52:12 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx)
> <drivshin.allworx@gmail.com>:
> > From: David Rivshin <drivshin@allworx.com>
> >
> > 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.

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[David Rivshin (Allworx)]

On Fri, 29 Jan 2016 23:26:50 -0500
"David Rivshin (Allworx)" <drivshin.allworx@gmail.com> wrote:

> From: David Rivshin <drivshin@allworx.com>
> 
> 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!

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[Thierry Reding]

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)" <drivshin.allworx@gmail.com> wrote:
> 
> > From: David Rivshin <drivshin@allworx.com>
> > 
> > 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>

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 16:19:48 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> 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)" <drivshin.allworx@gmail.com> wrote:
> >   
> > > From: David Rivshin <drivshin@allworx.com>
> > > 
> > > 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.

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 10:29:07 -0600
Adam Ford <aford173@gmail.com> 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. 

> 
> Thanks!
> 
> Adam
> On Mar 4, 2016 10:27 AM, "David Rivshin (Allworx)" <
> drivshin.allworx at gmail.com> wrote:  
> 
> > On Fri, 4 Mar 2016 16:19:48 +0100
> > Thierry Reding <thierry.reding@gmail.com> wrote:
> >  
> > > 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)" <drivshin.allworx@gmail.com> wrote:
> > > >  
> > > > > From: David Rivshin <drivshin@allworx.com>
> > > > >
> > > > > 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.
> >
> >  

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[Adam Ford]

On Fri, Mar 4, 2016 at 2:01 PM, David Rivshin (Allworx)
<drivshin.allworx@gmail.com> wrote:
> On Fri, 4 Mar 2016 10:29:07 -0600
> Adam Ford <aford173@gmail.com> 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

>>
>> Thanks!
>>
>> Adam
>> On Mar 4, 2016 10:27 AM, "David Rivshin (Allworx)" <
>> drivshin.allworx at gmail.com> wrote:
>>
>> > On Fri, 4 Mar 2016 16:19:48 +0100
>> > Thierry Reding <thierry.reding@gmail.com> wrote:
>> >
>> > > 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)" <drivshin.allworx@gmail.com> wrote:
>> > > >
>> > > > > From: David Rivshin <drivshin@allworx.com>
>> > > > >
>> > > > > 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.
>> >
>> >
>

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[Thierry Reding]

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 <thierry.reding@gmail.com> wrote:
> 
> > 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)" <drivshin.allworx@gmail.com> wrote:
> > >   
> > > > From: David Rivshin <drivshin@allworx.com>
> > > > 
> > > > 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?

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.

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>

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[David Rivshin (Allworx)]

On Fri, 4 Mar 2016 22:18:38 +0100
Thierry Reding <thierry.reding@gmail.com> wrote:

> 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 <thierry.reding@gmail.com> wrote:
> >   
> > > 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)" <drivshin.allworx@gmail.com> wrote:
> > > >     
> > > > > From: David Rivshin <drivshin@allworx.com>
> > > > > 
> > > > > 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?  
> 
> 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.

> > 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.

[PATCH 0/4] pwm: omap-dmtimer: fix period/duty_cycle calculation

[Adam Ford]

On Fri, Mar 4, 2016 at 5:20 PM, David Rivshin (Allworx)
<drivshin.allworx@gmail.com> wrote:
> On Fri, 4 Mar 2016 22:18:38 +0100
> Thierry Reding <thierry.reding@gmail.com> wrote:
>
>> 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 <thierry.reding@gmail.com> wrote:
>> >
>> > > 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)" <drivshin.allworx@gmail.com> wrote:
>> > > >
>> > > > > From: David Rivshin <drivshin@allworx.com>
>> > > > >
>> > > > > 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?
>>
>> 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.
>
>> > 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 <aford173@gmail.com>