linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates
@ 2020-02-28  9:56 Lokesh Vutla
  2020-02-28  9:56 ` [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file Lokesh Vutla
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

This series fixes minor issues in config callback and allows for on the
fly updates for pwm period and duty cycle. This is mainly intended to
allow pwm omap dmtimer to be used for generating a 1PPS signal that can be
syncronized to PTP clock in CPTS module available in AM335x and AM57xx SoCs.

Series depends on the following series:
- https://patchwork.kernel.org/patch/11379875/
- https://patchwork.kernel.org/project/linux-omap/list/?series=248929 

Full dependencies can be seen here:
https://github.com/lokeshvutla/linux/tree/devel/pwm-1pps-generation-v2

Changes since v1:
- Updated commit description in PATCH 1
- Added a brief about PWM generation using OMAP DM timer.
- Updated the pwm stop callback to allow for completing the current pwm
  cycle.
- Added the limitaitons of hardware.
- Used hw status instead of relying on pwm framework for state update.

Lokesh Vutla (6):
  pwm: omap-dmtimer: Drop unused header file
  pwm: omap-dmtimer: Update description for pwm omap dm timer
  pwm: omap-dmtimer: Fix pwm enabling sequence
  pwm: omap-dmtimer: Fix pwm disabling sequence
  pwm: omap-dmtimer: Do not disable pwm before changing
    period/duty_cycle
  pwm: omap-dmtimer: Implement .apply callback

 drivers/pwm/pwm-omap-dmtimer.c                | 248 +++++++++++++-----
 include/clocksource/timer-ti-dm.h             |   3 +-
 .../linux/platform_data/pwm_omap_dmtimer.h    |  90 -------
 3 files changed, 178 insertions(+), 163 deletions(-)
 delete mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h

-- 
2.23.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file
  2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
@ 2020-02-28  9:56 ` Lokesh Vutla
  2020-03-06 18:03   ` Tony Lindgren
  2020-02-28  9:56 ` [PATCH v2 2/6] pwm: omap-dmtimer: Update description for pwm omap dm timer Lokesh Vutla
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

pwm_omap_dmtimer.h is used only:
- to typedef struct omap_dm_timer to pwm_omap_dmtimer
- for macro PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE
Rest of the file is pretty mush unsed. So reuse omap_dm_timer
and OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE in pwm-omap-dmtimer.c
and delete the header file.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c                | 20 ++---
 include/clocksource/timer-ti-dm.h             |  3 +-
 .../linux/platform_data/pwm_omap_dmtimer.h    | 90 -------------------
 3 files changed, 10 insertions(+), 103 deletions(-)
 delete mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 9e4378dc6897..e4f5f710bfaa 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -20,8 +20,8 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <clocksource/timer-ti-dm.h>
 #include <linux/platform_data/dmtimer-omap.h>
-#include <linux/platform_data/pwm_omap_dmtimer.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
@@ -34,7 +34,7 @@
 struct pwm_omap_dmtimer_chip {
 	struct pwm_chip chip;
 	struct mutex mutex;
-	pwm_omap_dmtimer *dm_timer;
+	struct omap_dm_timer *dm_timer;
 	const struct omap_dm_timer_ops *pdata;
 	struct platform_device *dm_timer_pdev;
 };
@@ -190,10 +190,9 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 		load_value, load_value,	match_value, match_value);
 
 	omap->pdata->set_pwm(omap->dm_timer,
-			      pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			      true,
-			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE,
-			      true);
+			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
+			     true);
 
 	/* If config was called while timer was running it must be reenabled. */
 	if (timer_active)
@@ -221,10 +220,9 @@ static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
 	 */
 	mutex_lock(&omap->mutex);
 	omap->pdata->set_pwm(omap->dm_timer,
-			      polarity == PWM_POLARITY_INVERSED,
-			      true,
-			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE,
-			      true);
+			     polarity == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
+			     true);
 	mutex_unlock(&omap->mutex);
 
 	return 0;
@@ -246,7 +244,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	struct pwm_omap_dmtimer_chip *omap;
 	struct dmtimer_platform_data *timer_pdata;
 	const struct omap_dm_timer_ops *pdata;
-	pwm_omap_dmtimer *dm_timer;
+	struct omap_dm_timer *dm_timer;
 	u32 v;
 	int ret = 0;
 
diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h
index c2b172b28652..21b732a2368b 100644
--- a/include/clocksource/timer-ti-dm.h
+++ b/include/clocksource/timer-ti-dm.h
@@ -247,8 +247,7 @@ int omap_dm_timers_active(void);
 
 /*
  * The below are inlined to optimize code size for system timers. Other code
- * should not need these at all, see
- * include/linux/platform_data/pwm_omap_dmtimer.h
+ * should not need these at all.
  */
 #if defined(CONFIG_ARCH_OMAP1) || defined(CONFIG_ARCH_OMAP2PLUS)
 static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
diff --git a/include/linux/platform_data/pwm_omap_dmtimer.h b/include/linux/platform_data/pwm_omap_dmtimer.h
deleted file mode 100644
index e7d521e48855..000000000000
--- a/include/linux/platform_data/pwm_omap_dmtimer.h
+++ /dev/null
@@ -1,90 +0,0 @@
-/*
- * include/linux/platform_data/pwm_omap_dmtimer.h
- *
- * OMAP Dual-Mode Timer PWM platform data
- *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com/
- * Tarun Kanti DebBarma <tarun.kanti@ti.com>
- * Thara Gopinath <thara@ti.com>
- *
- * Platform device conversion and hwmod support.
- *
- * Copyright (C) 2005 Nokia Corporation
- * Author: Lauri Leukkunen <lauri.leukkunen@nokia.com>
- * PWM and clock framework support by Timo Teras.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- *
- * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
- * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN
- * NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
- * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
- * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
- * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
- * You should have received a copy of the  GNU General Public License along
- * with this program; if not, write  to the Free Software Foundation, Inc.,
- * 675 Mass Ave, Cambridge, MA 02139, USA.
- */
-
-#ifndef __PWM_OMAP_DMTIMER_PDATA_H
-#define __PWM_OMAP_DMTIMER_PDATA_H
-
-/* clock sources */
-#define PWM_OMAP_DMTIMER_SRC_SYS_CLK			0x00
-#define PWM_OMAP_DMTIMER_SRC_32_KHZ			0x01
-#define PWM_OMAP_DMTIMER_SRC_EXT_CLK			0x02
-
-/* timer interrupt enable bits */
-#define PWM_OMAP_DMTIMER_INT_CAPTURE			(1 << 2)
-#define PWM_OMAP_DMTIMER_INT_OVERFLOW			(1 << 1)
-#define PWM_OMAP_DMTIMER_INT_MATCH			(1 << 0)
-
-/* trigger types */
-#define PWM_OMAP_DMTIMER_TRIGGER_NONE			0x00
-#define PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW		0x01
-#define PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE	0x02
-
-struct omap_dm_timer;
-typedef struct omap_dm_timer pwm_omap_dmtimer;
-
-struct pwm_omap_dmtimer_pdata {
-	pwm_omap_dmtimer *(*request_by_node)(struct device_node *np);
-	pwm_omap_dmtimer *(*request_specific)(int timer_id);
-	pwm_omap_dmtimer *(*request)(void);
-
-	int	(*free)(pwm_omap_dmtimer *timer);
-
-	void	(*enable)(pwm_omap_dmtimer *timer);
-	void	(*disable)(pwm_omap_dmtimer *timer);
-
-	int	(*get_irq)(pwm_omap_dmtimer *timer);
-	int	(*set_int_enable)(pwm_omap_dmtimer *timer, unsigned int value);
-	int	(*set_int_disable)(pwm_omap_dmtimer *timer, u32 mask);
-
-	struct clk *(*get_fclk)(pwm_omap_dmtimer *timer);
-
-	int	(*start)(pwm_omap_dmtimer *timer);
-	int	(*stop)(pwm_omap_dmtimer *timer);
-	int	(*set_source)(pwm_omap_dmtimer *timer, int source);
-
-	int	(*set_load)(pwm_omap_dmtimer *timer, int autoreload,
-			unsigned int value);
-	int	(*set_match)(pwm_omap_dmtimer *timer, int enable,
-			unsigned int match);
-	int	(*set_pwm)(pwm_omap_dmtimer *timer, int def_on,
-			int toggle, int trigger);
-	int	(*set_prescaler)(pwm_omap_dmtimer *timer, int prescaler);
-
-	unsigned int (*read_counter)(pwm_omap_dmtimer *timer);
-	int	(*write_counter)(pwm_omap_dmtimer *timer, unsigned int value);
-	unsigned int (*read_status)(pwm_omap_dmtimer *timer);
-	int	(*write_status)(pwm_omap_dmtimer *timer, unsigned int value);
-};
-
-#endif /* __PWM_OMAP_DMTIMER_PDATA_H */
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 2/6] pwm: omap-dmtimer: Update description for pwm omap dm timer
  2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
  2020-02-28  9:56 ` [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file Lokesh Vutla
@ 2020-02-28  9:56 ` Lokesh Vutla
  2020-03-06 18:03   ` Tony Lindgren
  2020-02-28  9:56 ` [PATCH v2 3/6] pwm: omap-dmtimer: Fix pwm enabling sequence Lokesh Vutla
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

Update the description with a brief about how pwm is generated
using OMAP DM timer. Also add link to the Reference Manual.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index e4f5f710bfaa..a24a630ccdb9 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -10,7 +10,11 @@
  *
  * Description:
  *   This file is the core OMAP support for the generic, Linux
- *   PWM driver / controller, using the OMAP's dual-mode timers.
+ *   PWM driver / controller, using the OMAP's dual-mode timers
+ *   with a timer counter that goes up. When it overflows it gets
+ *   reloaded with the load value and the pwm output goes up.
+ *   When counter matches with match register, the output goes down.
+ *   Reference Manual: http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
  */
 
 #include <linux/clk.h>
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 3/6] pwm: omap-dmtimer: Fix pwm enabling sequence
  2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
  2020-02-28  9:56 ` [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file Lokesh Vutla
  2020-02-28  9:56 ` [PATCH v2 2/6] pwm: omap-dmtimer: Update description for pwm omap dm timer Lokesh Vutla
@ 2020-02-28  9:56 ` Lokesh Vutla
  2020-03-06 18:04   ` Tony Lindgren
  2020-02-28  9:56 ` [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence Lokesh Vutla
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

To configure DM timer in pwm mode the following needs to be set in
OMAP_TIMER_CTRL_REG using set_pwm callback:
- Set toggle mode on PORTIMERPWM output pin
- Set trigger on overflow and match on PORTIMERPWM output pin.
- Set auto reload

This is a one time configuration and needs to be set before the start of
the dm timer. But the current driver tries to set the same configuration
for every period/duty cycle update, which is not needed. So move the pwm
setup before enabling timer and do not update it in pwm_omap_dmtimer_config.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index a24a630ccdb9..bc338619232d 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -77,6 +77,11 @@ static int pwm_omap_dmtimer_enable(struct pwm_chip *chip,
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
 
 	mutex_lock(&omap->mutex);
+	omap->pdata->set_pwm(omap->dm_timer,
+			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
+			     true);
+
 	pwm_omap_dmtimer_start(omap);
 	mutex_unlock(&omap->mutex);
 
@@ -193,11 +198,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	omap->pdata->set_pwm(omap->dm_timer,
-			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
-			     true);
-
 	/* If config was called while timer was running it must be reenabled. */
 	if (timer_active)
 		pwm_omap_dmtimer_start(omap);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
                   ` (2 preceding siblings ...)
  2020-02-28  9:56 ` [PATCH v2 3/6] pwm: omap-dmtimer: Fix pwm enabling sequence Lokesh Vutla
@ 2020-02-28  9:56 ` Lokesh Vutla
  2020-03-06 18:14   ` Tony Lindgren
  2020-02-28  9:56 ` [PATCH v2 5/6] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle Lokesh Vutla
  2020-02-28  9:56 ` [PATCH v2 6/6] pwm: omap-dmtimer: Implement .apply callback Lokesh Vutla
  5 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

pwm_omap_dmtimer_disable() calls .stop callback which abruptly stops the
timer counter. This doesn't complete the current pwm cycle and
immediately disables the pwm. Instead disable the auto reload
functionality which allows to complete the current pwm cycle and then
disables the timer.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index bc338619232d..89b3c25d02b8 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -93,8 +93,16 @@ static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
 {
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
 
+	/*
+	 * Disable auto reload so that the current cycle gets completed and
+	 * then the counter stops.
+	 */
 	mutex_lock(&omap->mutex);
-	omap->pdata->stop(omap->dm_timer);
+	omap->pdata->set_pwm(omap->dm_timer,
+			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
+			     false);
+
 	mutex_unlock(&omap->mutex);
 }
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 5/6] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle
  2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
                   ` (3 preceding siblings ...)
  2020-02-28  9:56 ` [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence Lokesh Vutla
@ 2020-02-28  9:56 ` Lokesh Vutla
  2020-03-06 18:17   ` Tony Lindgren
  2020-02-28  9:56 ` [PATCH v2 6/6] pwm: omap-dmtimer: Implement .apply callback Lokesh Vutla
  5 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

Only the Timer control register(TCLR) cannot be updated when the timer
is running. Registers like Counter register(TCRR), loader register(TLDR),
match register(TMAR) can be updated when the counter is running. Since
TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
timer for period/duty_cycle update.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 89b3c25d02b8..e7487ceed0a3 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -15,6 +15,15 @@
  *   reloaded with the load value and the pwm output goes up.
  *   When counter matches with match register, the output goes down.
  *   Reference Manual: http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
+ *
+ * Limitations:
+ * - When PWM is running and changing both duty cycle and period,
+ *   we cannot prevent in software that the output might produce
+ *   a period with mixed settings. Especially when period/duty_cyle
+ *   is updated while the pwm pin is high, current pwm period/duty_cycle
+ *   can get updated as below based on the current timer counter:
+ *   	- period for current cycle =  current_period + new period
+ *   	- duty_cycle for current period = current period + new duty_cycle.
  */
 
 #include <linux/clk.h>
@@ -115,7 +124,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	u32 load_value, match_value;
 	struct clk *fclk;
 	unsigned long clk_rate;
-	bool timer_active;
 
 	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
 		duty_ns, period_ns);
@@ -191,25 +199,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	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
-	 * write its registers, but calls to omap_dm_timer_start/stop must
-	 * be balanced so check if timer is active before calling timer_stop.
-	 */
-	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
-	if (timer_active)
-		omap->pdata->stop(omap->dm_timer);
-
 	omap->pdata->set_load(omap->dm_timer, load_value);
 	omap->pdata->set_match(omap->dm_timer, true, match_value);
 
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	/* If config was called while timer was running it must be reenabled. */
-	if (timer_active)
-		pwm_omap_dmtimer_start(omap);
-
 	mutex_unlock(&omap->mutex);
 
 	return 0;
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v2 6/6] pwm: omap-dmtimer: Implement .apply callback
  2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
                   ` (4 preceding siblings ...)
  2020-02-28  9:56 ` [PATCH v2 5/6] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle Lokesh Vutla
@ 2020-02-28  9:56 ` Lokesh Vutla
  5 siblings, 0 replies; 21+ messages in thread
From: Lokesh Vutla @ 2020-02-28  9:56 UTC (permalink / raw)
  To: Thierry Reding, Uwe Kleine-König
  Cc: Tony Lindgren, Linux OMAP Mailing List, linux-kernel, linux-pwm,
	Sekhar Nori, Vignesh R, Lokesh Vutla

Implement .apply callback and drop the legacy callbacks(enable, disable,
config, set_polarity). In .apply() check for the current hardware status
before changing the pwm configuration.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 215 ++++++++++++++++++++++++---------
 1 file changed, 158 insertions(+), 57 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index e7487ceed0a3..5c86b1e02209 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -24,6 +24,11 @@
  *   can get updated as below based on the current timer counter:
  *   	- period for current cycle =  current_period + new period
  *   	- duty_cycle for current period = current period + new duty_cycle.
+ * - PWM OMAP DM timer cannot change the polarity when pwm is active. When
+ *   user requests a change in polarity when in active state:
+ *	- PWM is stopped abruptly(without completing the current cycle)
+ *	- Polarity is changed
+ *	- A fresh cycle is started.
  */
 
 #include <linux/clk.h>
@@ -44,8 +49,18 @@
 #define DM_TIMER_LOAD_MIN 0xfffffffe
 #define DM_TIMER_MAX      0xffffffff
 
+/**
+ * struct pwm_omap_dmtimer_chip - Structure representing a pwm chip
+ *				  corresponding to omap dmtimer.
+ * @chip:		PWM chip structure representing PWM controller
+ * @mutex:		Mutex to protect pwm apply state
+ * @dm_timer:		Pointer to omap dm timer.
+ * @pdata:		Pointer to omap dm timer ops.
+ * dm_timer_pdev:	Pointer to omap dm timer platform device
+ */
 struct pwm_omap_dmtimer_chip {
 	struct pwm_chip chip;
+	/* Mutex to protect pwm apply state */
 	struct mutex mutex;
 	struct omap_dm_timer *dm_timer;
 	const struct omap_dm_timer_ops *pdata;
@@ -58,11 +73,22 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)
 	return container_of(chip, struct pwm_omap_dmtimer_chip, chip);
 }
 
+/**
+ * pwm_omap_dmtimer_get_clock_cycles() - Get clock cycles in a time frame
+ * @clk_rate:	pwm timer clock rate
+ * @ns:		time frame in nano seconds.
+ *
+ * Return number of clock cycles in a given period(ins ns).
+ */
 static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns)
 {
 	return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC);
 }
 
+/**
+ * pwm_omap_dmtimer_start() - Start the pwm omap dm timer in pwm mode
+ * @omap:	Pointer to pwm omap dm timer chip
+ */
 static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
 {
 	/*
@@ -80,41 +106,70 @@ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
 	omap->pdata->start(omap->dm_timer);
 }
 
-static int pwm_omap_dmtimer_enable(struct pwm_chip *chip,
-				   struct pwm_device *pwm)
+/**
+ * pwm_omap_dmtimer_is_enabled() -  Detect if the pwm is enabled.
+ * @omap:	Pointer to pwm omap dm timer chip
+ *
+ * Note: PWM is considered as enabled only when timer is counting and
+ *	 auto-reload feature is enabled.
+ *
+ * Return true if pwm is enabled else false.
+ */
+static bool pwm_omap_dmtimer_is_enabled(struct pwm_omap_dmtimer_chip *omap)
 {
-	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
+	u32 status;
 
-	mutex_lock(&omap->mutex);
-	omap->pdata->set_pwm(omap->dm_timer,
-			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
-			     true);
+	status = omap->pdata->get_pwm_status(omap->dm_timer);
 
-	pwm_omap_dmtimer_start(omap);
-	mutex_unlock(&omap->mutex);
+	return !!(status & OMAP_TIMER_CTRL_ST) &&
+	       !!(status & OMAP_TIMER_CTRL_AR);
+}
 
-	return 0;
+/**
+ * pwm_omap_dmtimer_is_completing() -  Detect if the pwm is completing the
+ *				       last cycle.
+ * @omap:	Pointer to pwm omap dm timer chip
+ *
+ * Note: PWM is considered as completing only when timer is counting and
+ *	 auto-reload feature is disabled.
+ *
+ * Return true if pwm is completing the last cycle else false.
+ */
+static bool pwm_omap_dmtimer_is_completing(struct pwm_omap_dmtimer_chip *omap)
+{
+	u32 status;
+
+	status = omap->pdata->get_pwm_status(omap->dm_timer);
+
+	return !!(status & OMAP_TIMER_CTRL_ST) &&
+	       !(status & OMAP_TIMER_CTRL_AR);
 }
 
-static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
-				     struct pwm_device *pwm)
+/**
+ * pwm_omap_dmtimer_polarity() -  Detect the polarity of pwm.
+ * @omap:	Pointer to pwm omap dm timer chip
+ *
+ * Return the polarity of pwm.
+ */
+static int pwm_omap_dmtimer_polarity(struct pwm_omap_dmtimer_chip *omap)
 {
-	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
+	u32 status;
 
-	/*
-	 * Disable auto reload so that the current cycle gets completed and
-	 * then the counter stops.
-	 */
-	mutex_lock(&omap->mutex);
-	omap->pdata->set_pwm(omap->dm_timer,
-			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
-			     false);
+	status = omap->pdata->get_pwm_status(omap->dm_timer);
 
-	mutex_unlock(&omap->mutex);
+	return !!(status & OMAP_TIMER_CTRL_SCPWM);
 }
 
+/**
+ * pwm_omap_dmtimer_config() - Update the configuration of pwm omap dm timer
+ * @chip:	Pointer to PWM controller
+ * @pwm:	Pointer to PWM channel
+ * @duty_ns:	New duty cycle in nano seconds
+ * @period_ns:	New period in nano seconds
+ *
+ * Return 0 if successfully changed the period/duty_cycle else appropriate
+ * error.
+ */
 static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 				   struct pwm_device *pwm,
 				   int duty_ns, int period_ns)
@@ -122,30 +177,26 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
 	u32 period_cycles, duty_cycles;
 	u32 load_value, match_value;
-	struct clk *fclk;
 	unsigned long clk_rate;
+	struct clk *fclk;
 
 	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
 		duty_ns, period_ns);
 
-	mutex_lock(&omap->mutex);
 	if (duty_ns == pwm_get_duty_cycle(pwm) &&
-	    period_ns == pwm_get_period(pwm)) {
-		/* No change - don't cause any transients. */
-		mutex_unlock(&omap->mutex);
+	    period_ns == pwm_get_period(pwm))
 		return 0;
-	}
 
 	fclk = omap->pdata->get_fclk(omap->dm_timer);
 	if (!fclk) {
 		dev_err(chip->dev, "invalid pmtimer fclk\n");
-		goto err_einval;
+		return -EINVAL;
 	}
 
 	clk_rate = clk_get_rate(fclk);
 	if (!clk_rate) {
 		dev_err(chip->dev, "invalid pmtimer fclk rate\n");
-		goto err_einval;
+		return -EINVAL;
 	}
 
 	dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate);
@@ -173,7 +224,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 		dev_info(chip->dev,
 			 "period %d ns too short for clock rate %lu Hz\n",
 			 period_ns, clk_rate);
-		goto err_einval;
+		return -EINVAL;
 	}
 
 	if (duty_cycles < 1) {
@@ -205,55 +256,104 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	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,
-					 struct pwm_device *pwm,
-					 enum pwm_polarity polarity)
+/**
+ * pwm_omap_dmtimer_set_polarity() - Changes the polarity of the pwm dm timer.
+ * @chip:	Pointer to PWM controller
+ * @pwm:	Pointer to PWM channel
+ * @polarity:	New pwm polarity to be set
+ */
+static void pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
+					  struct pwm_device *pwm,
+					  enum pwm_polarity polarity)
 {
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
+	bool enabled;
+
+	/* Disable the PWM before changing the polarity. */
+	enabled = pwm_omap_dmtimer_is_enabled(omap);
+	if (enabled || pwm_omap_dmtimer_is_completing(omap))
+		omap->pdata->stop(omap->dm_timer);
 
-	/*
-	 * PWM core will not call set_polarity while PWM is enabled so it's
-	 * safe to reconfigure the timer here without stopping it first.
-	 */
-	mutex_lock(&omap->mutex);
 	omap->pdata->set_pwm(omap->dm_timer,
 			     polarity == PWM_POLARITY_INVERSED,
 			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
 			     true);
+
+	if (enabled)
+		pwm_omap_dmtimer_start(omap);
+}
+
+/**
+ * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
+ * @chip:	Pointer to PWM controller
+ * @pwm:	Pointer to PWM channel
+ * @state:	New state to apply
+ *
+ * Return 0 if successfully changed the state else appropriate error.
+ */
+static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const struct pwm_state *state)
+{
+	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
+	int ret = 0;
+
+	mutex_lock(&omap->mutex);
+
+	if (pwm_omap_dmtimer_is_enabled(omap) && !state->enabled) {
+		/*
+		 * Disable auto reload so that the current cycle gets completed
+		 * and then the counter stops.
+		 */
+		omap->pdata->set_pwm(omap->dm_timer,
+				pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
+				true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
+				false);
+		goto unlock_mutex;
+	}
+
+	if (pwm_omap_dmtimer_polarity(omap) != state->polarity)
+		pwm_omap_dmtimer_set_polarity(chip, pwm, state->polarity);
+
+	ret = pwm_omap_dmtimer_config(chip, pwm, state->duty_cycle,
+				      state->period);
+	if (ret)
+		goto unlock_mutex;
+
+	if (!pwm_omap_dmtimer_is_enabled(omap) && state->enabled) {
+		omap->pdata->set_pwm(omap->dm_timer,
+				     state->polarity == PWM_POLARITY_INVERSED,
+				     true,
+				     OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
+				     true);
+		pwm_omap_dmtimer_start(omap);
+	}
+
+unlock_mutex:
 	mutex_unlock(&omap->mutex);
 
-	return 0;
+	return ret;
 }
 
 static const struct pwm_ops pwm_omap_dmtimer_ops = {
-	.enable	= pwm_omap_dmtimer_enable,
-	.disable = pwm_omap_dmtimer_disable,
-	.config	= pwm_omap_dmtimer_config,
-	.set_polarity = pwm_omap_dmtimer_set_polarity,
+	.apply = pwm_omap_dmtimer_apply,
 	.owner = THIS_MODULE,
 };
 
 static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *timer;
-	struct platform_device *timer_pdev;
-	struct pwm_omap_dmtimer_chip *omap;
 	struct dmtimer_platform_data *timer_pdata;
 	const struct omap_dm_timer_ops *pdata;
+	struct platform_device *timer_pdev;
+	struct pwm_omap_dmtimer_chip *omap;
 	struct omap_dm_timer *dm_timer;
-	u32 v;
+	struct device_node *timer;
 	int ret = 0;
+	u32 v;
 
 	timer = of_parse_phandle(np, "ti,timers", 0);
 	if (!timer)
@@ -286,6 +386,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	    !pdata->set_load ||
 	    !pdata->set_match ||
 	    !pdata->set_pwm ||
+	    !pdata->get_pwm_status ||
 	    !pdata->set_prescaler ||
 	    !pdata->write_counter) {
 		dev_err(&pdev->dev, "Incomplete dmtimer pdata structure\n");
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file
  2020-02-28  9:56 ` [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file Lokesh Vutla
@ 2020-03-06 18:03   ` Tony Lindgren
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-03-06 18:03 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R

* Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
> pwm_omap_dmtimer.h is used only:
> - to typedef struct omap_dm_timer to pwm_omap_dmtimer
> - for macro PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE
> Rest of the file is pretty mush unsed. So reuse omap_dm_timer
> and OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE in pwm-omap-dmtimer.c
> and delete the header file.

Acked-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/6] pwm: omap-dmtimer: Update description for pwm omap dm timer
  2020-02-28  9:56 ` [PATCH v2 2/6] pwm: omap-dmtimer: Update description for pwm omap dm timer Lokesh Vutla
@ 2020-03-06 18:03   ` Tony Lindgren
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-03-06 18:03 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R

* Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
> Update the description with a brief about how pwm is generated
> using OMAP DM timer. Also add link to the Reference Manual.

Acked-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/6] pwm: omap-dmtimer: Fix pwm enabling sequence
  2020-02-28  9:56 ` [PATCH v2 3/6] pwm: omap-dmtimer: Fix pwm enabling sequence Lokesh Vutla
@ 2020-03-06 18:04   ` Tony Lindgren
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-03-06 18:04 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R

* Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
> To configure DM timer in pwm mode the following needs to be set in
> OMAP_TIMER_CTRL_REG using set_pwm callback:
> - Set toggle mode on PORTIMERPWM output pin
> - Set trigger on overflow and match on PORTIMERPWM output pin.
> - Set auto reload
> 
> This is a one time configuration and needs to be set before the start of
> the dm timer. But the current driver tries to set the same configuration
> for every period/duty cycle update, which is not needed. So move the pwm
> setup before enabling timer and do not update it in pwm_omap_dmtimer_config.

Still works for me:

Tested-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-02-28  9:56 ` [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence Lokesh Vutla
@ 2020-03-06 18:14   ` Tony Lindgren
  2020-03-09  4:51     ` Lokesh Vutla
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-03-06 18:14 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

* Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
> pwm_omap_dmtimer_disable() calls .stop callback which abruptly stops the
> timer counter. This doesn't complete the current pwm cycle and
> immediately disables the pwm. Instead disable the auto reload
> functionality which allows to complete the current pwm cycle and then
> disables the timer.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index bc338619232d..89b3c25d02b8 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -93,8 +93,16 @@ static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
>  {
>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>  
> +	/*
> +	 * Disable auto reload so that the current cycle gets completed and
> +	 * then the counter stops.
> +	 */
>  	mutex_lock(&omap->mutex);
> -	omap->pdata->stop(omap->dm_timer);
> +	omap->pdata->set_pwm(omap->dm_timer,
> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
> +			     false);
> +
>  	mutex_unlock(&omap->mutex);
>  }

I'm seeing an issue with this patch where after use something is
left on and power consumption stays higher by about 30 mW after
use.

I can reproduce this easily on droid4 with Sebastian's rumble-test
app[0]. After use, I sometimes also hear the vibrator keep chirping
quietly, so there seems to be some pwm still happening after disable :)

Reloading modules for pwm-vibra and pwm-omap-dmtimer make the power
consumption go back down again.

If you have a scope set up, maybe check the lines are quiet after
disable after this patch?

Regards,

Tony


[0] https://git.collabora.com/cgit/user/sre/rumble-test.git/plain/rumble-test.c

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/6] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle
  2020-02-28  9:56 ` [PATCH v2 5/6] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle Lokesh Vutla
@ 2020-03-06 18:17   ` Tony Lindgren
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-03-06 18:17 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R

* Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
> Only the Timer control register(TCLR) cannot be updated when the timer
> is running. Registers like Counter register(TCRR), loader register(TLDR),
> match register(TMAR) can be updated when the counter is running. Since
> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> timer for period/duty_cycle update.

Still works for me with "pwm: omap-dmtimer: Fix pwm disabling sequence"
patch left out:

Tested-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-06 18:14   ` Tony Lindgren
@ 2020-03-09  4:51     ` Lokesh Vutla
  2020-03-09 12:30       ` Sebastian Reichel
  2020-03-09 18:01       ` Tony Lindgren
  0 siblings, 2 replies; 21+ messages in thread
From: Lokesh Vutla @ 2020-03-09  4:51 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

Hi Tony,

On 06/03/20 11:44 PM, Tony Lindgren wrote:
> * Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
>> pwm_omap_dmtimer_disable() calls .stop callback which abruptly stops the
>> timer counter. This doesn't complete the current pwm cycle and
>> immediately disables the pwm. Instead disable the auto reload
>> functionality which allows to complete the current pwm cycle and then
>> disables the timer.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>> index bc338619232d..89b3c25d02b8 100644
>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> @@ -93,8 +93,16 @@ static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
>>  {
>>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>>  
>> +	/*
>> +	 * Disable auto reload so that the current cycle gets completed and
>> +	 * then the counter stops.
>> +	 */
>>  	mutex_lock(&omap->mutex);
>> -	omap->pdata->stop(omap->dm_timer);
>> +	omap->pdata->set_pwm(omap->dm_timer,
>> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
>> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
>> +			     false);
>> +
>>  	mutex_unlock(&omap->mutex);
>>  }
> 
> I'm seeing an issue with this patch where after use something is
> left on and power consumption stays higher by about 30 mW after
> use.

Interesting...What is the PWM period and duty cycle in the test case?
Can you dump the following registers before and after disabling:
- TLDR
- TMAR
- TCLR

> 
> I can reproduce this easily on droid4 with Sebastian's rumble-test
> app[0]. After use, I sometimes also hear the vibrator keep chirping
> quietly, so there seems to be some pwm still happening after disable :)

hmm..The line clearly goes down on the scope after the current pwm duty cycle is
done and never comes back.

> 
> Reloading modules for pwm-vibra and pwm-omap-dmtimer make the power
> consumption go back down again.
> 
> If you have a scope set up, maybe check the lines are quiet after
> disable after this patch?
> 
> Regards,
> 
> Tony
> 
> 
> [0] https://git.collabora.com/cgit/user/sre/rumble-test.git/plain/rumble-test.c

This is redirecting to collabora.com. Is this code available in github or some
public repo?

Thanks and regards,
Lokesh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-09  4:51     ` Lokesh Vutla
@ 2020-03-09 12:30       ` Sebastian Reichel
  2020-03-09 18:01       ` Tony Lindgren
  1 sibling, 0 replies; 21+ messages in thread
From: Sebastian Reichel @ 2020-03-09 12:30 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Tony Lindgren, Thierry Reding, Uwe Kleine-König,
	Linux OMAP Mailing List, linux-kernel, linux-pwm, Sekhar Nori,
	Vignesh R

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

Hi,

On Mon, Mar 09, 2020 at 10:21:59AM +0530, Lokesh Vutla wrote:
> > [0] https://git.collabora.com/cgit/user/sre/rumble-test.git/plain/rumble-test.c
> 
> This is redirecting to collabora.com. Is this code available in github or some
> public repo?
> 
> Thanks and regards,
> Lokesh

Sorry, we migrated to gitlab under different domain. I pushed it
here now: https://github.com/sre/rumble-test

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-09  4:51     ` Lokesh Vutla
  2020-03-09 12:30       ` Sebastian Reichel
@ 2020-03-09 18:01       ` Tony Lindgren
  2020-03-10  7:04         ` Lokesh Vutla
  1 sibling, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-03-09 18:01 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

* Lokesh Vutla <lokeshvutla@ti.com> [200309 04:53]:
> Hi Tony,
> 
> On 06/03/20 11:44 PM, Tony Lindgren wrote:
> > * Lokesh Vutla <lokeshvutla@ti.com> [200228 09:58]:
> >> pwm_omap_dmtimer_disable() calls .stop callback which abruptly stops the
> >> timer counter. This doesn't complete the current pwm cycle and
> >> immediately disables the pwm. Instead disable the auto reload
> >> functionality which allows to complete the current pwm cycle and then
> >> disables the timer.
> >>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> ---
> >>  drivers/pwm/pwm-omap-dmtimer.c | 10 +++++++++-
> >>  1 file changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> >> index bc338619232d..89b3c25d02b8 100644
> >> --- a/drivers/pwm/pwm-omap-dmtimer.c
> >> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> >> @@ -93,8 +93,16 @@ static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
> >>  {
> >>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> >>  
> >> +	/*
> >> +	 * Disable auto reload so that the current cycle gets completed and
> >> +	 * then the counter stops.
> >> +	 */
> >>  	mutex_lock(&omap->mutex);
> >> -	omap->pdata->stop(omap->dm_timer);
> >> +	omap->pdata->set_pwm(omap->dm_timer,
> >> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> >> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
> >> +			     false);
> >> +
> >>  	mutex_unlock(&omap->mutex);
> >>  }
> > 
> > I'm seeing an issue with this patch where after use something is
> > left on and power consumption stays higher by about 30 mW after
> > use.
> 
> Interesting...What is the PWM period and duty cycle in the test case?
> Can you dump the following registers before and after disabling:
> - TLDR
> - TMAR
> - TCLR

Here's the state dumped before and after in omap_dm_timer_set_pwm():

omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841

So looks like the start bit is still enabled after use?

I think the duty cycle depends on the strength set for rumble-test.c.

> > I can reproduce this easily on droid4 with Sebastian's rumble-test
> > app[0]. After use, I sometimes also hear the vibrator keep chirping
> > quietly, so there seems to be some pwm still happening after disable :)
> 
> hmm..The line clearly goes down on the scope after the current pwm duty cycle is
> done and never comes back.

OK

Regards,

Tony

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-09 18:01       ` Tony Lindgren
@ 2020-03-10  7:04         ` Lokesh Vutla
  2020-03-10 15:52           ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-03-10  7:04 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

Hi Tony,

[...snip...]

>>>>  
>>>> +	/*
>>>> +	 * Disable auto reload so that the current cycle gets completed and
>>>> +	 * then the counter stops.
>>>> +	 */
>>>>  	mutex_lock(&omap->mutex);
>>>> -	omap->pdata->stop(omap->dm_timer);
>>>> +	omap->pdata->set_pwm(omap->dm_timer,
>>>> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
>>>> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
>>>> +			     false);
>>>> +
>>>>  	mutex_unlock(&omap->mutex);
>>>>  }
>>>
>>> I'm seeing an issue with this patch where after use something is
>>> left on and power consumption stays higher by about 30 mW after
>>> use.
>>
>> Interesting...What is the PWM period and duty cycle in the test case?
>> Can you dump the following registers before and after disabling:
>> - TLDR
>> - TMAR
>> - TCLR
> 
> Here's the state dumped before and after in omap_dm_timer_set_pwm():
> 
> omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
> omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
> omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
> omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
> omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
> omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
> omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
> omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
> 

Looking at the registers:
period = 327 *(1000/clk_freq in MHz) ns
duty_cycle =  perioid.

I did simulate this behavior on BeagleBoneBlack on timer7. PWM is going down
after disabling.

> So looks like the start bit is still enabled after use?

Right, that is expected. The start bit gets disabled automatically once the pwm
period completes. This is because auto reload bit is off. That's the main idea
of this patch so that PWM period is completed after disabling, else PWM is
stopped abruptly.

Not sure why it is not happening in your case. If you think it is not needed, I
can drop this patch and add a limitation saying that PWM gets disabled
immediately without completing the current cycle.

Thanks and regards,
Lokesh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-10  7:04         ` Lokesh Vutla
@ 2020-03-10 15:52           ` Tony Lindgren
  2020-03-11  4:12             ` Lokesh Vutla
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-03-10 15:52 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

* Lokesh Vutla <lokeshvutla@ti.com> [200310 07:06]:
> Hi Tony,
> 
> [...snip...]
> 
> >>>>  
> >>>> +	/*
> >>>> +	 * Disable auto reload so that the current cycle gets completed and
> >>>> +	 * then the counter stops.
> >>>> +	 */
> >>>>  	mutex_lock(&omap->mutex);
> >>>> -	omap->pdata->stop(omap->dm_timer);
> >>>> +	omap->pdata->set_pwm(omap->dm_timer,
> >>>> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> >>>> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
> >>>> +			     false);
> >>>> +
> >>>>  	mutex_unlock(&omap->mutex);
> >>>>  }
> >>>
> >>> I'm seeing an issue with this patch where after use something is
> >>> left on and power consumption stays higher by about 30 mW after
> >>> use.
> >>
> >> Interesting...What is the PWM period and duty cycle in the test case?
> >> Can you dump the following registers before and after disabling:
> >> - TLDR
> >> - TMAR
> >> - TCLR
> > 
> > Here's the state dumped before and after in omap_dm_timer_set_pwm():
> > 
> > omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
> > omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
> > omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
> > omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
> > omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
> > omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
> > omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
> > omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
> > 
> 
> Looking at the registers:
> period = 327 *(1000/clk_freq in MHz) ns
> duty_cycle =  perioid.
> 
> I did simulate this behavior on BeagleBoneBlack on timer7. PWM is going down
> after disabling.
> 
> > So looks like the start bit is still enabled after use?
> 
> Right, that is expected. The start bit gets disabled automatically once the pwm
> period completes. This is because auto reload bit is off. That's the main idea
> of this patch so that PWM period is completed after disabling, else PWM is
> stopped abruptly.

OK

> Not sure why it is not happening in your case. If you think it is not needed, I
> can drop this patch and add a limitation saying that PWM gets disabled
> immediately without completing the current cycle.

Could it be that we now have the cpu_pm notifier restore something
invalid after exiting idle that restarts the counter?

Regards,

Tony

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-10 15:52           ` Tony Lindgren
@ 2020-03-11  4:12             ` Lokesh Vutla
  2020-03-12  0:58               ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Lokesh Vutla @ 2020-03-11  4:12 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel



On 10/03/20 9:22 PM, Tony Lindgren wrote:
> * Lokesh Vutla <lokeshvutla@ti.com> [200310 07:06]:
>> Hi Tony,
>>
>> [...snip...]
>>
>>>>>>  
>>>>>> +	/*
>>>>>> +	 * Disable auto reload so that the current cycle gets completed and
>>>>>> +	 * then the counter stops.
>>>>>> +	 */
>>>>>>  	mutex_lock(&omap->mutex);
>>>>>> -	omap->pdata->stop(omap->dm_timer);
>>>>>> +	omap->pdata->set_pwm(omap->dm_timer,
>>>>>> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
>>>>>> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
>>>>>> +			     false);
>>>>>> +
>>>>>>  	mutex_unlock(&omap->mutex);
>>>>>>  }
>>>>>
>>>>> I'm seeing an issue with this patch where after use something is
>>>>> left on and power consumption stays higher by about 30 mW after
>>>>> use.
>>>>
>>>> Interesting...What is the PWM period and duty cycle in the test case?
>>>> Can you dump the following registers before and after disabling:
>>>> - TLDR
>>>> - TMAR
>>>> - TCLR
>>>
>>> Here's the state dumped before and after in omap_dm_timer_set_pwm():
>>>
>>> omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
>>> omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
>>> omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
>>> omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
>>> omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
>>> omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
>>> omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
>>> omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
>>>
>>
>> Looking at the registers:
>> period = 327 *(1000/clk_freq in MHz) ns
>> duty_cycle =  perioid.
>>
>> I did simulate this behavior on BeagleBoneBlack on timer7. PWM is going down
>> after disabling.
>>
>>> So looks like the start bit is still enabled after use?
>>
>> Right, that is expected. The start bit gets disabled automatically once the pwm
>> period completes. This is because auto reload bit is off. That's the main idea
>> of this patch so that PWM period is completed after disabling, else PWM is
>> stopped abruptly.
> 
> OK
> 
>> Not sure why it is not happening in your case. If you think it is not needed, I
>> can drop this patch and add a limitation saying that PWM gets disabled
>> immediately without completing the current cycle.
> 
> Could it be that we now have the cpu_pm notifier restore something
> invalid after exiting idle that restarts the counter?

If that's the case, mis behavior should have happened without this patch as well.

Is it possible for you to dump the registers when you are observing higher power
consumption after the use?

However, I see an issue with the patch itself as pm_runtime is not disabled
after the pwm is stopped. Not sure how that could be nullified with this approach.


Thanks and regards,
Lokesh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-11  4:12             ` Lokesh Vutla
@ 2020-03-12  0:58               ` Tony Lindgren
  2020-03-13 15:34                 ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-03-12  0:58 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

* Lokesh Vutla <lokeshvutla@ti.com> [200311 04:14]:
> 
> 
> On 10/03/20 9:22 PM, Tony Lindgren wrote:
> > * Lokesh Vutla <lokeshvutla@ti.com> [200310 07:06]:
> >> Hi Tony,
> >>
> >> [...snip...]
> >>
> >>>>>>  
> >>>>>> +	/*
> >>>>>> +	 * Disable auto reload so that the current cycle gets completed and
> >>>>>> +	 * then the counter stops.
> >>>>>> +	 */
> >>>>>>  	mutex_lock(&omap->mutex);
> >>>>>> -	omap->pdata->stop(omap->dm_timer);
> >>>>>> +	omap->pdata->set_pwm(omap->dm_timer,
> >>>>>> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> >>>>>> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE,
> >>>>>> +			     false);
> >>>>>> +
> >>>>>>  	mutex_unlock(&omap->mutex);
> >>>>>>  }
> >>>>>
> >>>>> I'm seeing an issue with this patch where after use something is
> >>>>> left on and power consumption stays higher by about 30 mW after
> >>>>> use.
> >>>>
> >>>> Interesting...What is the PWM period and duty cycle in the test case?
> >>>> Can you dump the following registers before and after disabling:
> >>>> - TLDR
> >>>> - TMAR
> >>>> - TCLR
> >>>
> >>> Here's the state dumped before and after in omap_dm_timer_set_pwm():
> >>>
> >>> omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
> >>> omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
> >>> omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00000040
> >>> omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001842
> >>> omap_timer 4013e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
> >>> omap_timer 4013e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
> >>> omap_timer 4803e000.timer: XXX set_pwm before: tldr: fffffeb8 tmar: fffffffe tclr: 00001843
> >>> omap_timer 4803e000.timer: XXX set_pwm after: tldr: fffffeb8 tmar: fffffffe tclr: 00001841
> >>>
> >>
> >> Looking at the registers:
> >> period = 327 *(1000/clk_freq in MHz) ns
> >> duty_cycle =  perioid.
> >>
> >> I did simulate this behavior on BeagleBoneBlack on timer7. PWM is going down
> >> after disabling.
> >>
> >>> So looks like the start bit is still enabled after use?
> >>
> >> Right, that is expected. The start bit gets disabled automatically once the pwm
> >> period completes. This is because auto reload bit is off. That's the main idea
> >> of this patch so that PWM period is completed after disabling, else PWM is
> >> stopped abruptly.
> > 
> > OK
> > 
> >> Not sure why it is not happening in your case. If you think it is not needed, I
> >> can drop this patch and add a limitation saying that PWM gets disabled
> >> immediately without completing the current cycle.
> > 
> > Could it be that we now have the cpu_pm notifier restore something
> > invalid after exiting idle that restarts the counter?
> 
> If that's the case, mis behavior should have happened without this patch as well.

Hmm but without this patch we stop the timer so enable bit is
cleared before we ever save context. I think now we can have
enable bit still on, save the context, and keep restoring the
enable bit that never has a chance to clear if we do this at
a fast enough rate.

> Is it possible for you to dump the registers when you are observing higher power
> consumption after the use?

Well they seem to be the same as in the dump above, here are the regs
dumped after use with chirping happening:

$ sudo rwmem 0x4803e000+0x60
0x4803e000 = 0x4fff1301
0x4803e004 = 0000000000
0x4803e008 = 0000000000
0x4803e00c = 0000000000
0x4803e010 = 0x0000000c
0x4803e014 = 0000000000
0x4803e018 = 0000000000
0x4803e01c = 0000000000
0x4803e020 = 0000000000
0x4803e024 = 0x00000003
0x4803e028 = 0000000000
0x4803e02c = 0000000000
0x4803e030 = 0000000000
0x4803e034 = 0000000000
0x4803e038 = 0x00001841
0x4803e03c = 0x00000402
0x4803e040 = 0xfffffeb8
0x4803e044 = 0xffffffff
0x4803e048 = 0000000000
0x4803e04c = 0xfffffffe
0x4803e050 = 0000000000
0x4803e054 = 0000000000
0x4803e058 = 0000000000
0x4803e05c = 0000000000

$ sudo rwmem 0x4013e000+0x60
0x4013e000 = 0x4fff0301
0x4013e004 = 0000000000
0x4013e008 = 0000000000
0x4013e00c = 0000000000
0x4013e010 = 0000000000
0x4013e014 = 0000000000
0x4013e018 = 0000000000
0x4013e01c = 0000000000
0x4013e020 = 0000000000
0x4013e024 = 0x00000003
0x4013e028 = 0000000000
0x4013e02c = 0000000000
0x4013e030 = 0000000000
0x4013e034 = 0000000000
0x4013e038 = 0x00001841
0x4013e03c = 0x00000407
0x4013e040 = 0xfffffeb8
0x4013e044 = 0xffffffff
0x4013e048 = 0000000000
0x4013e04c = 0xfffffffe
0x4013e050 = 0000000000
0x4013e054 = 0000000000
0x4013e058 = 0000000000
0x4013e05c = 0000000000

So looks like the enable bit is never cleared now.

> However, I see an issue with the patch itself as pm_runtime is not disabled
> after the pwm is stopped. Not sure how that could be nullified with this approach.

Hmm yeah not sure what could be used to clear things
when the current cycle is completed unless there's
some interrupt for it.

Regards,

Tony


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-12  0:58               ` Tony Lindgren
@ 2020-03-13 15:34                 ` Tony Lindgren
  2020-03-13 15:50                   ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2020-03-13 15:34 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

* Tony Lindgren <tony@atomide.com> [200312 00:59]:
> * Lokesh Vutla <lokeshvutla@ti.com> [200311 04:14]:
> > However, I see an issue with the patch itself as pm_runtime is not disabled
> > after the pwm is stopped. Not sure how that could be nullified with this approach.
> 
> Hmm yeah not sure what could be used to clear things
> when the current cycle is completed unless there's
> some interrupt for it.

You could enable pm_runtime_use_autosuspend() for pwm use,
then set the timeout to the cycle length, then in the
runtime_suspend make sure the enable bit is cleared if
requested.

But this too seems inaccurate, it would be best to clear
the enable bit on some cycle completion interrupt if
such thing is available.

Regards,

Tony

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence
  2020-03-13 15:34                 ` Tony Lindgren
@ 2020-03-13 15:50                   ` Tony Lindgren
  0 siblings, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2020-03-13 15:50 UTC (permalink / raw)
  To: Lokesh Vutla
  Cc: Thierry Reding, Uwe Kleine-König, Linux OMAP Mailing List,
	linux-kernel, linux-pwm, Sekhar Nori, Vignesh R,
	Sebastian Reichel

* Tony Lindgren <tony@atomide.com> [200313 15:35]:
> * Tony Lindgren <tony@atomide.com> [200312 00:59]:
> > * Lokesh Vutla <lokeshvutla@ti.com> [200311 04:14]:
> > > However, I see an issue with the patch itself as pm_runtime is not disabled
> > > after the pwm is stopped. Not sure how that could be nullified with this approach.
> > 
> > Hmm yeah not sure what could be used to clear things
> > when the current cycle is completed unless there's
> > some interrupt for it.
> 
> You could enable pm_runtime_use_autosuspend() for pwm use,
> then set the timeout to the cycle length, then in the
> runtime_suspend make sure the enable bit is cleared if
> requested.
> 
> But this too seems inaccurate, it would be best to clear
> the enable bit on some cycle completion interrupt if
> such thing is available.

I think enabling pm_runtime_use_autosuspend() for pwm,
adding a flag for pwm_enabled, and blocking cpu_pm if
pwm_enabled is set might do the trick though. Then clear
pwm_enabled flag in runtime_suspend if set.

This depend on cpuidle respecting NOTIFY_BAD that I
fixed in the recent thread:

[PATCH 0/3] Block idle in gpio-omap with cpu_pm

Regards,

Tony

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-03-13 15:50 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28  9:56 [PATCH v2 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates Lokesh Vutla
2020-02-28  9:56 ` [PATCH v2 1/6] pwm: omap-dmtimer: Drop unused header file Lokesh Vutla
2020-03-06 18:03   ` Tony Lindgren
2020-02-28  9:56 ` [PATCH v2 2/6] pwm: omap-dmtimer: Update description for pwm omap dm timer Lokesh Vutla
2020-03-06 18:03   ` Tony Lindgren
2020-02-28  9:56 ` [PATCH v2 3/6] pwm: omap-dmtimer: Fix pwm enabling sequence Lokesh Vutla
2020-03-06 18:04   ` Tony Lindgren
2020-02-28  9:56 ` [PATCH v2 4/6] pwm: omap-dmtimer: Fix pwm disabling sequence Lokesh Vutla
2020-03-06 18:14   ` Tony Lindgren
2020-03-09  4:51     ` Lokesh Vutla
2020-03-09 12:30       ` Sebastian Reichel
2020-03-09 18:01       ` Tony Lindgren
2020-03-10  7:04         ` Lokesh Vutla
2020-03-10 15:52           ` Tony Lindgren
2020-03-11  4:12             ` Lokesh Vutla
2020-03-12  0:58               ` Tony Lindgren
2020-03-13 15:34                 ` Tony Lindgren
2020-03-13 15:50                   ` Tony Lindgren
2020-02-28  9:56 ` [PATCH v2 5/6] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle Lokesh Vutla
2020-03-06 18:17   ` Tony Lindgren
2020-02-28  9:56 ` [PATCH v2 6/6] pwm: omap-dmtimer: Implement .apply callback Lokesh Vutla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).