[PATCH v3 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[PATCH v3 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[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=251691

Full dependencies can be seen here:
https://github.com/lokeshvutla/linux/tree/devel/pwm-dynamic-period-updates-v3

Changes since v2:
- Restored the existing behavior on pwm stop. PWM stops immediately when
  .stop is called as timer counter stops immediately. If just the
  autoreload is disabled as in v2, there is a possibility that pwm is
  never stopped.
- Added the above limitation in the driver description
- Rebased on top of v5.6-rc5

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 (5):
  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: Do not disable pwm before changing
    period/duty_cycle
  pwm: omap-dmtimer: Implement .apply callback

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

-- 
2.23.0

[PATCH v3 1/5] pwm: omap-dmtimer: Drop unused header file

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

Acked-by: Tony Lindgren <tony@atomide.com>
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 25f05235866e..531ca87fcd08 100644
--- a/include/clocksource/timer-ti-dm.h
+++ b/include/clocksource/timer-ti-dm.h
@@ -248,8 +248,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

[PATCH v3 2/5] pwm: omap-dmtimer: Update description for pwm omap dm timer

[Lokesh Vutla]

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

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Tony Lindgren <tony@atomide.com>
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 e4f5f710bfaa..92aac6c86b95 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -10,7 +10,15 @@
  *
  * 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
+ *
+ * Limitations:
+ * - When PWM is stopped, timer counter gets stopped immediately. This
+ *   doesn't allow the current PWM period to complete and stops abruptly.
  */
 
 #include <linux/clk.h>
-- 
2.23.0

[PATCH v3 3/5] pwm: omap-dmtimer: Fix pwm enabling sequence

[Lokesh Vutla]

To configure DM timer is 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.

Tested-by: Tony Lindgren <tony@atomide.com>
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 92aac6c86b95..85b17b49980b 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -81,6 +81,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);
 
@@ -197,11 +202,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

[PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

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

Tested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 85b17b49980b..c56e7256e923 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -19,6 +19,13 @@
  * Limitations:
  * - When PWM is stopped, timer counter gets stopped immediately. This
  *   doesn't allow the current PWM period to complete and stops abruptly.
+ * - 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>
@@ -111,7 +118,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);
@@ -187,25 +193,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

[PATCH v3 5/5] pwm: omap-dmtimer: Implement .apply callback

[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 | 180 +++++++++++++++++++++++----------
 1 file changed, 129 insertions(+), 51 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index c56e7256e923..0d31833db2e2 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -26,6 +26,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>
@@ -46,8 +51,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;
@@ -60,11 +75,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)
 {
 	/*
@@ -82,33 +108,46 @@ 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
+ *
+ * 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);
-
-	pwm_omap_dmtimer_start(omap);
-	mutex_unlock(&omap->mutex);
+	status = omap->pdata->get_pwm_status(omap->dm_timer);
 
-	return 0;
+	return !!(status & OMAP_TIMER_CTRL_ST);
 }
 
-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;
 
-	mutex_lock(&omap->mutex);
-	omap->pdata->stop(omap->dm_timer);
-	mutex_unlock(&omap->mutex);
+	status = omap->pdata->get_pwm_status(omap->dm_timer);
+
+	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)
@@ -116,30 +155,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);
@@ -167,7 +202,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) {
@@ -199,55 +234,97 @@ 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)
+		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) {
+		omap->pdata->stop(omap->dm_timer);
+		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)
@@ -280,6 +357,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

Re: [PATCH v3 1/5] pwm: omap-dmtimer: Drop unused header file

[Uwe Kleine-König]

On Thu, Mar 12, 2020 at 09:52:06AM +0530, Lokesh Vutla wrote:
> 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>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 2/5] pwm: omap-dmtimer: Update description for pwm omap dm timer

[Uwe Kleine-König]

On Thu, Mar 12, 2020 at 09:52:07AM +0530, Lokesh Vutla wrote:
> Update the description with a brief about how pwm is generated
> using OMAP DM timer and add Limitations for the pwm generations.
> Also add link to the Reference Manual.
> 
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Acked-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> 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.

I'm not sure what is sensible here. Stopping the PWM for a short period
is bad, but maybe emitting a wrong period isn't better. You can however
optimise it if only one of period or duty_cycle changes.

@Thierry, what is your position here? I tend to say a short stop is
preferable.

> Tested-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 85b17b49980b..c56e7256e923 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -19,6 +19,13 @@
>   * Limitations:
>   * - When PWM is stopped, timer counter gets stopped immediately. This
>   *   doesn't allow the current PWM period to complete and stops abruptly.
> + * - 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.

In case we stay with a short stop, adding something like:

 - The PWM has to be stopped for updates of both period and duty_cycle.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 12/03/20 12:10 PM, Uwe Kleine-König wrote:
> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
>> 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.
> 
> I'm not sure what is sensible here. Stopping the PWM for a short period
> is bad, but maybe emitting a wrong period isn't better. You can however
> optimise it if only one of period or duty_cycle changes.
> 
> @Thierry, what is your position here? I tend to say a short stop is
> preferable.

Short stop has side effects especially in the case where 1PPS is generated using
this PWM. In this case where PWM period is continuously synced with PTP clock,
cannot expect any breaks in PWM. This doesn't fall in the above limitations as
well. as duty_cycle is not a worry and only the rising edge is all that matters.

Also any specific reason why you wanted to stop rather than having the mentioned
limitation? it is just a corner anyway and doesn't happen all the time.

Thanks and regards,
Lokesh

> 
>> Tested-by: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 21 +++++++--------------
>>  1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>> index 85b17b49980b..c56e7256e923 100644
>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> @@ -19,6 +19,13 @@
>>   * Limitations:
>>   * - When PWM is stopped, timer counter gets stopped immediately. This
>>   *   doesn't allow the current PWM period to complete and stops abruptly.
>> + * - 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.
> 
> In case we stay with a short stop, adding something like:
> 
>  - The PWM has to be stopped for updates of both period and duty_cycle.
> 
> Best regards
> Uwe
> 

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

On Thu, Mar 12, 2020 at 01:35:32PM +0530, Lokesh Vutla wrote:
> On 12/03/20 12:10 PM, Uwe Kleine-König wrote:
> > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> >> 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.
> > 
> > I'm not sure what is sensible here. Stopping the PWM for a short period
> > is bad, but maybe emitting a wrong period isn't better. You can however
> > optimise it if only one of period or duty_cycle changes.
> > 
> > @Thierry, what is your position here? I tend to say a short stop is
> > preferable.
> 
> Short stop has side effects especially in the case where 1PPS is generated using
> this PWM. In this case where PWM period is continuously synced with PTP clock,
> cannot expect any breaks in PWM. This doesn't fall in the above limitations as
> well. as duty_cycle is not a worry and only the rising edge is all that matters.
> 
> Also any specific reason why you wanted to stop rather than having the mentioned
> limitation? it is just a corner anyway and doesn't happen all the time.

I'm a bit torn here. Which of the two steps out of line is worse depends
on what is driven by the PWM in question. And also I think ignoring
"just corner cases" is a reliable way into trouble.

The usual PWM contributer (understandably) cares mostly about their own
problem they have to solve. If however you take a step back and care
about the PWM framework as a whole to be capable to solve problems in
general, such that any consumer just has to know that there is a PWM and
start requesting specific settings for their work to get done, it gets
obvious that you want some kind of uniform behaviour of each hardware
driver. And then a short inactive break between two periods is more
common and better understandable than a mixed period.

Also being a corner case that only happens (say) once in 100000 cases
isn't a clear upside. This just results in a machine leaving the development
department, passing the production test and then behave unexpected once
per year in the field.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 12/03/20 2:17 PM, Uwe Kleine-König wrote:
> On Thu, Mar 12, 2020 at 01:35:32PM +0530, Lokesh Vutla wrote:
>> On 12/03/20 12:10 PM, Uwe Kleine-König wrote:
>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
>>>> 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.
>>>
>>> I'm not sure what is sensible here. Stopping the PWM for a short period
>>> is bad, but maybe emitting a wrong period isn't better. You can however
>>> optimise it if only one of period or duty_cycle changes.
>>>
>>> @Thierry, what is your position here? I tend to say a short stop is
>>> preferable.
>>
>> Short stop has side effects especially in the case where 1PPS is generated using
>> this PWM. In this case where PWM period is continuously synced with PTP clock,
>> cannot expect any breaks in PWM. This doesn't fall in the above limitations as
>> well. as duty_cycle is not a worry and only the rising edge is all that matters.
>>
>> Also any specific reason why you wanted to stop rather than having the mentioned
>> limitation? it is just a corner anyway and doesn't happen all the time.
> 
> I'm a bit torn here. Which of the two steps out of line is worse depends
> on what is driven by the PWM in question. And also I think ignoring
> "just corner cases" is a reliable way into trouble.

I do agree that corner cases should not be ignored. But in this particular
driver, just trying to explain the effect of this corner case. On dynamic pwm
period update, the current pwm cycle might generate a period with mixed
settings. IMHO, it is okay to live with it and mark it as a limitation as you
pointed out in case of sifive driver[0].


> 
> The usual PWM contributer (understandably) cares mostly about their own
> problem they have to solve. If however you take a step back and care
> about the PWM framework as a whole to be capable to solve problems in
> general, such that any consumer just has to know that there is a PWM and
> start requesting specific settings for their work to get done, it gets
> obvious that you want some kind of uniform behaviour of each hardware
> driver. And then a short inactive break between two periods is more
> common and better understandable than a mixed period.

But the problem here is that inactive breaks between two periods is not desired.
Because the pwm is used to generate a 1PPS signal and is continuously
synchronized with PTP clock.

I am up if this can be solved generically. But updating period is very specific
to hardware implementation. Not sure what generic solution can be brought out of
this. Please correct me if I am wrong.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-sifive.c#n7

Thanks and regards,
Lokesh

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Richard Cochran]

On Thu, Mar 12, 2020 at 04:14:34PM +0530, Lokesh Vutla wrote:
> But the problem here is that inactive breaks between two periods is not desired.
> Because the pwm is used to generate a 1PPS signal and is continuously
> synchronized with PTP clock.

The 1-PPS case is the "easy" one.  If the PWM is adjustable on the
fly, then people will use it with higher frequency signals.
 
> I am up if this can be solved generically. But updating period is very specific
> to hardware implementation. Not sure what generic solution can be brought out of
> this. Please correct me if I am wrong.

What happens today when the PWM frequency or duty cycle are changed
while the signal is enabled?

Do different PWM devices/drivers behave the same way?

Does this series change the behavior of the am335x and friends?

Thanks,
Richard

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Richard,

On 12/03/20 7:51 PM, Richard Cochran wrote:
> On Thu, Mar 12, 2020 at 04:14:34PM +0530, Lokesh Vutla wrote:
>> But the problem here is that inactive breaks between two periods is not desired.
>> Because the pwm is used to generate a 1PPS signal and is continuously
>> synchronized with PTP clock.
> 
> The 1-PPS case is the "easy" one.  If the PWM is adjustable on the
> fly, then people will use it with higher frequency signals.

Yes, PWM can be adjusted on the fly. TRM does specify that corresponding
registers(TLDR, TMAR, TCRR) registers can be updated when timer is active.

>  
>> I am up if this can be solved generically. But updating period is very specific
>> to hardware implementation. Not sure what generic solution can be brought out of
>> this. Please correct me if I am wrong.
> 
> What happens today when the PWM frequency or duty cycle are changed
> while the signal is enabled?

Today, PWM is stopped and then period/duty_cycle are updated.

> 
> Do different PWM devices/drivers behave the same way?
> 
> Does this series change the behavior of the am335x and friends?

Yes, this series  is applicable on all TI OMAP2+ devices with DMTIMER.

[0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf Section 20.1.1.1 DMTIMER
overview Page 4436.

Thanks and regards,
Lokesh

Re: [PATCH v3 5/5] pwm: omap-dmtimer: Implement .apply callback

[Tony Lindgren]

* Lokesh Vutla <lokeshvutla@ti.com> [200312 04:24]:
> 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.

This set works just fine for me thanks, so for this patch too:

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 12/03/20 4:14 PM, Lokesh Vutla wrote:
> Hi Uwe,
> 
> On 12/03/20 2:17 PM, Uwe Kleine-König wrote:
>> On Thu, Mar 12, 2020 at 01:35:32PM +0530, Lokesh Vutla wrote:
>>> On 12/03/20 12:10 PM, Uwe Kleine-König wrote:
>>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
>>>>> 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.
>>>>
>>>> I'm not sure what is sensible here. Stopping the PWM for a short period
>>>> is bad, but maybe emitting a wrong period isn't better. You can however
>>>> optimise it if only one of period or duty_cycle changes.
>>>>
>>>> @Thierry, what is your position here? I tend to say a short stop is
>>>> preferable.
>>>
>>> Short stop has side effects especially in the case where 1PPS is generated using
>>> this PWM. In this case where PWM period is continuously synced with PTP clock,
>>> cannot expect any breaks in PWM. This doesn't fall in the above limitations as
>>> well. as duty_cycle is not a worry and only the rising edge is all that matters.
>>>
>>> Also any specific reason why you wanted to stop rather than having the mentioned
>>> limitation? it is just a corner anyway and doesn't happen all the time.
>>
>> I'm a bit torn here. Which of the two steps out of line is worse depends
>> on what is driven by the PWM in question. And also I think ignoring
>> "just corner cases" is a reliable way into trouble.
> 
> I do agree that corner cases should not be ignored. But in this particular
> driver, just trying to explain the effect of this corner case. On dynamic pwm
> period update, the current pwm cycle might generate a period with mixed
> settings. IMHO, it is okay to live with it and mark it as a limitation as you
> pointed out in case of sifive driver[0].

Not sure what is the conclusion here. If there are no objections on this series,
can it be merged?

Thanks and regards,
Lokesh

> 
> 
>>
>> The usual PWM contributer (understandably) cares mostly about their own
>> problem they have to solve. If however you take a step back and care
>> about the PWM framework as a whole to be capable to solve problems in
>> general, such that any consumer just has to know that there is a PWM and
>> start requesting specific settings for their work to get done, it gets
>> obvious that you want some kind of uniform behaviour of each hardware
>> driver. And then a short inactive break between two periods is more
>> common and better understandable than a mixed period.
> 
> But the problem here is that inactive breaks between two periods is not desired.
> Because the pwm is used to generate a 1PPS signal and is continuously
> synchronized with PTP clock.
> 
> I am up if this can be solved generically. But updating period is very specific
> to hardware implementation. Not sure what generic solution can be brought out of
> this. Please correct me if I am wrong.
> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-sifive.c#n7
> 
> Thanks and regards,
> Lokesh
> 

Re: [PATCH v3 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[Lokesh Vutla]

Hi All,

On 12/03/20 9:52 AM, Lokesh Vutla wrote:
> 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=251691

Gentle ping on this series :) The above dependencies are merged into timer tree
and below is the immutable branch:

https://git.linaro.org/people/dlezcano/linux.git/log/?h=timers/drivers/timer-ti-dm

Thanks and regards,
Lokesh

Re: [PATCH v3 0/5] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[Thierry Reding]

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

On Mon, Mar 23, 2020 at 05:00:13PM +0530, Lokesh Vutla wrote:
> Hi All,
> 
> On 12/03/20 9:52 AM, Lokesh Vutla wrote:
> > 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=251691
> 
> Gentle ping on this series :) The above dependencies are merged into timer tree
> and below is the immutable branch:
> 
> https://git.linaro.org/people/dlezcano/linux.git/log/?h=timers/drivers/timer-ti-dm

I've pulled in the above branch into the PWM tree and applied your
series on top.

Thanks,
Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

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

On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > 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.
> 
> I'm not sure what is sensible here. Stopping the PWM for a short period
> is bad, but maybe emitting a wrong period isn't better. You can however
> optimise it if only one of period or duty_cycle changes.
> 
> @Thierry, what is your position here? I tend to say a short stop is
> preferable.

It's not clear to me from the above description how exactly the device
behaves, but I suspect that it may latch the values in those registers
and only update the actual signal output once a period has finished. I
know of a couple of other devices that do that, so it wouldn't be
surprising.

Even if that was not the case, I think this is just the kind of thing
that we have to live with. Sometimes it just isn't possible to have all
supported devices adhere strictly to an API. So I think the best we can
do is have an API that loosely defines what's supposed to happen and
make a best effort to implement those semantics. If a device deviates
slightly from those expectations, we can always cross fingers and hope
that things still work. And it looks like they are.

So I think if Lokesh and Tony agree that this is the right thing to do
and have verified that things still work after this, that's about as
good as it's going to get.

I know this is perhaps cheating a little, or turning a blind eye, but I
don't know what the alternative would be. Do we want to tell people that
a given PWM controller can't be used if it doesn't work according to our
expectations? That's hard to argue if that controller works just fine
for all known use-cases.

Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello Thierry,

On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > 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.
> > 
> > I'm not sure what is sensible here. Stopping the PWM for a short period
> > is bad, but maybe emitting a wrong period isn't better. You can however
> > optimise it if only one of period or duty_cycle changes.
> > 
> > @Thierry, what is your position here? I tend to say a short stop is
> > preferable.
> 
> It's not clear to me from the above description how exactly the device
> behaves, but I suspect that it may latch the values in those registers
> and only update the actual signal output once a period has finished. I
> know of a couple of other devices that do that, so it wouldn't be
> surprising.
> 
> Even if that was not the case, I think this is just the kind of thing
> that we have to live with. Sometimes it just isn't possible to have all
> supported devices adhere strictly to an API. So I think the best we can
> do is have an API that loosely defines what's supposed to happen and
> make a best effort to implement those semantics. If a device deviates
> slightly from those expectations, we can always cross fingers and hope
> that things still work. And it looks like they are.
> 
> So I think if Lokesh and Tony agree that this is the right thing to do
> and have verified that things still work after this, that's about as
> good as it's going to get.

I'd say this isn't for the platform people to decide. My position here
is that the PWM drivers should behave as uniform as possible to minimize
surprises for consumers. And so it's a "PWM decision" that is to be made
here, not an "omap decision".

> I know this is perhaps cheating a little, or turning a blind eye, but I
> don't know what the alternative would be. Do we want to tell people that
> a given PWM controller can't be used if it doesn't work according to our
> expectations? That's hard to argue if that controller works just fine
> for all known use-cases.

I'd like have some official policy here which of the alternatives is the
preferred cheat.

The situation here is that period and duty_cycle cannot be updated
atomically. So the two options are:

 - stop shortly
 - update with hardware running and maybe emit a broken period

I tend to say "stop shortly" is the better alternative.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Thierry,

On 30/03/20 7:44 PM, Thierry Reding wrote:
> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
>>> 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.
>>
>> I'm not sure what is sensible here. Stopping the PWM for a short period
>> is bad, but maybe emitting a wrong period isn't better. You can however
>> optimise it if only one of period or duty_cycle changes.
>>
>> @Thierry, what is your position here? I tend to say a short stop is
>> preferable.
> 
> It's not clear to me from the above description how exactly the device
> behaves, but I suspect that it may latch the values in those registers
> and only update the actual signal output once a period has finished. I
> know of a couple of other devices that do that, so it wouldn't be
> surprising.
> 
> Even if that was not the case, I think this is just the kind of thing
> that we have to live with. Sometimes it just isn't possible to have all
> supported devices adhere strictly to an API. So I think the best we can
> do is have an API that loosely defines what's supposed to happen and
> make a best effort to implement those semantics. If a device deviates
> slightly from those expectations, we can always cross fingers and hope
> that things still work. And it looks like they are.
> 
> So I think if Lokesh and Tony agree that this is the right thing to do
> and have verified that things still work after this, that's about as
> good as it's going to get.

Yes this is needed especially in the use-case[0] that I am trying to enable
using PWM. In this case PWM cannot be stopped in between and needs to be updated
dynamically. Also hardware doesn't provide any restrictions on updating the
period. So IMHO, this might be the right thing to do.

Tony did provide tested-by and I measured PWM signals on scope with these
changes. Let me know if any thing else is required?

[0] https://sourceforge.net/p/linuxptp/mailman/message/36943248/

Thanks and regards,
Lokesh

> 
> I know this is perhaps cheating a little, or turning a blind eye, but I
> don't know what the alternative would be. Do we want to tell people that
> a given PWM controller can't be used if it doesn't work according to our
> expectations? That's hard to argue if that controller works just fine
> for all known use-cases.
> 
> Thierry
> 

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

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

On Tue, Mar 31, 2020 at 08:59:47PM +0530, Lokesh Vutla wrote:
> Hi Thierry,
> 
> On 30/03/20 7:44 PM, Thierry Reding wrote:
> > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> >> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> >>> 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.
> >>
> >> I'm not sure what is sensible here. Stopping the PWM for a short period
> >> is bad, but maybe emitting a wrong period isn't better. You can however
> >> optimise it if only one of period or duty_cycle changes.
> >>
> >> @Thierry, what is your position here? I tend to say a short stop is
> >> preferable.
> > 
> > It's not clear to me from the above description how exactly the device
> > behaves, but I suspect that it may latch the values in those registers
> > and only update the actual signal output once a period has finished. I
> > know of a couple of other devices that do that, so it wouldn't be
> > surprising.
> > 
> > Even if that was not the case, I think this is just the kind of thing
> > that we have to live with. Sometimes it just isn't possible to have all
> > supported devices adhere strictly to an API. So I think the best we can
> > do is have an API that loosely defines what's supposed to happen and
> > make a best effort to implement those semantics. If a device deviates
> > slightly from those expectations, we can always cross fingers and hope
> > that things still work. And it looks like they are.
> > 
> > So I think if Lokesh and Tony agree that this is the right thing to do
> > and have verified that things still work after this, that's about as
> > good as it's going to get.
> 
> Yes this is needed especially in the use-case[0] that I am trying to enable
> using PWM. In this case PWM cannot be stopped in between and needs to be updated
> dynamically. Also hardware doesn't provide any restrictions on updating the
> period. So IMHO, this might be the right thing to do.
> 
> Tony did provide tested-by and I measured PWM signals on scope with these
> changes. Let me know if any thing else is required?
> 
> [0] https://sourceforge.net/p/linuxptp/mailman/message/36943248/

From you measurements, can you tell whether or not the signal actually
gets updated in the middle of a period, or does it only get updated at
the end of a full period?

Does the reference manual document this?

Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

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

On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > > 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.
> > > 
> > > I'm not sure what is sensible here. Stopping the PWM for a short period
> > > is bad, but maybe emitting a wrong period isn't better. You can however
> > > optimise it if only one of period or duty_cycle changes.
> > > 
> > > @Thierry, what is your position here? I tend to say a short stop is
> > > preferable.
> > 
> > It's not clear to me from the above description how exactly the device
> > behaves, but I suspect that it may latch the values in those registers
> > and only update the actual signal output once a period has finished. I
> > know of a couple of other devices that do that, so it wouldn't be
> > surprising.
> > 
> > Even if that was not the case, I think this is just the kind of thing
> > that we have to live with. Sometimes it just isn't possible to have all
> > supported devices adhere strictly to an API. So I think the best we can
> > do is have an API that loosely defines what's supposed to happen and
> > make a best effort to implement those semantics. If a device deviates
> > slightly from those expectations, we can always cross fingers and hope
> > that things still work. And it looks like they are.
> > 
> > So I think if Lokesh and Tony agree that this is the right thing to do
> > and have verified that things still work after this, that's about as
> > good as it's going to get.
> 
> I'd say this isn't for the platform people to decide. My position here
> is that the PWM drivers should behave as uniform as possible to minimize
> surprises for consumers. And so it's a "PWM decision" that is to be made
> here, not an "omap decision".

I think there's a fine line to be walked here. I agree that we should
aim to have as much consistency between drivers as possible. At the same
time I think we need to be pragmatic. As Lokesh said, the particular use
case here requires this type of on-the-fly adjustment of the PWM period
without stopping and restarting the PWM. It doesn't work otherwise. So
th alternative that you're proposing is to say that we don't support
that use-case, even though it works just fine given this particular
hardware. That's not really an option.

> > I know this is perhaps cheating a little, or turning a blind eye, but I
> > don't know what the alternative would be. Do we want to tell people that
> > a given PWM controller can't be used if it doesn't work according to our
> > expectations? That's hard to argue if that controller works just fine
> > for all known use-cases.
> 
> I'd like have some official policy here which of the alternatives is the
> preferred cheat.
> 
> The situation here is that period and duty_cycle cannot be updated
> atomically. So the two options are:
> 
>  - stop shortly
>  - update with hardware running and maybe emit a broken period

I think we can already support both of those with the existing API. If
a consumer wants to stop the PWM while reconfiguring, they should be
able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
equivalent) and for the second case they can just do pwm_config() (or
the atomic equivalent).

Some hardware may actually require the PWM to be disabled before
reconfiguring, so they won't be able to strictly adhere to the second
use-case.

But as discussed above, I don't want to strive for a lowest common
denominator that would preclude some more specific use-cases from
working if the hardware supports it.

So I think we should aim for drivers to implement the semantics as
closely as possible. If the hardware doesn't support some of these
requirements strictly while a particular use-case depends on that, then
that just means that the hardware isn't compatible with that use-case.
Chances are that the system just isn't going to be designed to support
that use-case in the first place if the hardware can't do it.

The sysfs interface is a bit of a special case here because it isn't
possible to know what use-cases people are going to come up with. It's
most likely that they'll try something and if it doesn't work they can
see if a driver patch can improve things. If not, perhaps the hardware
just isn't up to the task and that'll be the end of it.

I haven't yet come across a case where things actually fail because we
are too flexible in what the API permits, so I don't see a need to add
arbitrary restrictions.

> I tend to say "stop shortly" is the better alternative.

That's clearly subjective. In this particular case it's certainly not
the case. If the API had that assumption baked in there'd be no way to
support this use-case, even though hardware evidently supports it.

So I certainly think that there are areas where we need to find common
ground for abstraction, but I think being overly restrictive can make an
API completely useless.

One possible extension that I can imagine would be to introduce some
sort of capability structure that drivers can fill in to describe the
behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
could describe that they are able to change the period and/or duty cycle
while the PWM is on. There could be another capability bit that says
that the current period will finish before new settings are applied. Yet
another capability could describe that duty-cycle and period can be
applied atomically. Consumers could then check those capabilities to see
if they match their requirements.

But then again, I think that would just make things overly complicated.
None of the existing consumers need that, so it doesn't seem like there
is much demand for that feature. In practice I suspect most consumers
work fine despite potentially small deviations in how the PWM behaves.

Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello Thierry,

On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> > On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > > > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > > > 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.
> > > > 
> > > > I'm not sure what is sensible here. Stopping the PWM for a short period
> > > > is bad, but maybe emitting a wrong period isn't better. You can however
> > > > optimise it if only one of period or duty_cycle changes.
> > > > 
> > > > @Thierry, what is your position here? I tend to say a short stop is
> > > > preferable.
> > > 
> > > It's not clear to me from the above description how exactly the device
> > > behaves, but I suspect that it may latch the values in those registers
> > > and only update the actual signal output once a period has finished. I
> > > know of a couple of other devices that do that, so it wouldn't be
> > > surprising.
> > > 
> > > Even if that was not the case, I think this is just the kind of thing
> > > that we have to live with. Sometimes it just isn't possible to have all
> > > supported devices adhere strictly to an API. So I think the best we can
> > > do is have an API that loosely defines what's supposed to happen and
> > > make a best effort to implement those semantics. If a device deviates
> > > slightly from those expectations, we can always cross fingers and hope
> > > that things still work. And it looks like they are.
> > > 
> > > So I think if Lokesh and Tony agree that this is the right thing to do
> > > and have verified that things still work after this, that's about as
> > > good as it's going to get.
> > 
> > I'd say this isn't for the platform people to decide. My position here
> > is that the PWM drivers should behave as uniform as possible to minimize
> > surprises for consumers. And so it's a "PWM decision" that is to be made
> > here, not an "omap decision".
> 
> I think there's a fine line to be walked here. I agree that we should
> aim to have as much consistency between drivers as possible. At the same
> time I think we need to be pragmatic. As Lokesh said, the particular use
> case here requires this type of on-the-fly adjustment of the PWM period
> without stopping and restarting the PWM. It doesn't work otherwise. So
> th alternative that you're proposing is to say that we don't support
> that use-case, even though it works just fine given this particular
> hardware. That's not really an option.

I understand your opinion here. The situation now is that in current
mainline the driver stops the hardware for reconfiguration and it
doesn't fit Lokesh's use case so he changed to on-the-fly update
(accepting that maybe a wrong period is emitted). What if someone relies
on the old behaviour? What if in a year someone comes and claims the
wrong period is bad for their usecase and changes back to
stop-to-update?

When I write a consumer driver, do I have a chance to know how the PWM,
that I happen to use, behaves? To be able to get my consumer driver
reliable I might need to know that however.

> > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > don't know what the alternative would be. Do we want to tell people that
> > > a given PWM controller can't be used if it doesn't work according to our
> > > expectations? That's hard to argue if that controller works just fine
> > > for all known use-cases.
> > 
> > I'd like have some official policy here which of the alternatives is the
> > preferred cheat.
> > 
> > The situation here is that period and duty_cycle cannot be updated
> > atomically. So the two options are:
> > 
> >  - stop shortly
> >  - update with hardware running and maybe emit a broken period
> 
> I think we can already support both of those with the existing API. If
> a consumer wants to stop the PWM while reconfiguring, they should be
> able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> equivalent) and for the second case they can just do pwm_config() (or
> the atomic equivalent).

Yes, the consumer can force the stop and update. But assume I'm "only" a
consumer driver author and I want: atomic update and if this is not
possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
So I cannot benefit from a good driver/hardware that can do atomic
updates? Or I have to patch each driver that I actually use to use
stop-to-update?

> Some hardware may actually require the PWM to be disabled before
> reconfiguring, so they won't be able to strictly adhere to the second
> use-case.
> 
> But as discussed above, I don't want to strive for a lowest common
> denominator that would preclude some more specific use-cases from
> working if the hardware supports it.
> 
> So I think we should aim for drivers to implement the semantics as
> closely as possible. If the hardware doesn't support some of these
> requirements strictly while a particular use-case depends on that, then
> that just means that the hardware isn't compatible with that use-case.
> Chances are that the system just isn't going to be designed to support
> that use-case in the first place if the hardware can't do it.
> 
> The sysfs interface is a bit of a special case here because it isn't
> possible to know what use-cases people are going to come up with.

In my eyes the sysfs interface isn't special here. You also don't know
what the OMAP PWM hardware is used for.

> It's most likely that they'll try something and if it doesn't work
> they can see if a driver patch can improve things.

So either the group who prefers "stop-to-update" or the group who
prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
driver patch?

> One possible extension that I can imagine would be to introduce some
> sort of capability structure that drivers can fill in to describe the
> behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> could describe that they are able to change the period and/or duty cycle
> while the PWM is on. There could be another capability bit that says
> that the current period will finish before new settings are applied. Yet
> another capability could describe that duty-cycle and period can be
> applied atomically. Consumers could then check those capabilities to see
> if they match their requirements.
> 
> But then again, I think that would just make things overly complicated.
> None of the existing consumers need that, so it doesn't seem like there
> is much demand for that feature. In practice I suspect most consumers
> work fine despite potentially small deviations in how the PWM behaves.

I think the status quo is what I asked about above: People use sysfs and
if the PWM behaves different than needed, the driver is patched and most
of the time not mainlined. If your focus is to support a certain
industrial system with a defined use case, this is fine. If however you
target for an universal framework that works for any combination of
consumer + lowlevel driver without patching (that at least is able to
diagnose: This PWM cannot provide what my consumer needs), this is bad.
Also this means that whenever a system designer changes something on
their machine (kernel update, different hardware, an new usecase for a
PWM) they might have to reverify if the given PWM driver behaves as
needed.

My suggestion for now is to start documenting how the drivers behave
expanding how limitations are documented in some drivers. So maybe
change from "Limitations" to "Implementation and Hardware Details"?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Thierry,

On 01/04/20 1:40 AM, Thierry Reding wrote:
> On Tue, Mar 31, 2020 at 08:59:47PM +0530, Lokesh Vutla wrote:
>> Hi Thierry,
>>
>> On 30/03/20 7:44 PM, Thierry Reding wrote:
>>> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
>>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
>>>>> 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.
>>>>
>>>> I'm not sure what is sensible here. Stopping the PWM for a short period
>>>> is bad, but maybe emitting a wrong period isn't better. You can however
>>>> optimise it if only one of period or duty_cycle changes.
>>>>
>>>> @Thierry, what is your position here? I tend to say a short stop is
>>>> preferable.
>>>
>>> It's not clear to me from the above description how exactly the device
>>> behaves, but I suspect that it may latch the values in those registers
>>> and only update the actual signal output once a period has finished. I
>>> know of a couple of other devices that do that, so it wouldn't be
>>> surprising.
>>>
>>> Even if that was not the case, I think this is just the kind of thing
>>> that we have to live with. Sometimes it just isn't possible to have all
>>> supported devices adhere strictly to an API. So I think the best we can
>>> do is have an API that loosely defines what's supposed to happen and
>>> make a best effort to implement those semantics. If a device deviates
>>> slightly from those expectations, we can always cross fingers and hope
>>> that things still work. And it looks like they are.
>>>
>>> So I think if Lokesh and Tony agree that this is the right thing to do
>>> and have verified that things still work after this, that's about as
>>> good as it's going to get.
>>
>> Yes this is needed especially in the use-case[0] that I am trying to enable
>> using PWM. In this case PWM cannot be stopped in between and needs to be updated
>> dynamically. Also hardware doesn't provide any restrictions on updating the
>> period. So IMHO, this might be the right thing to do.
>>
>> Tony did provide tested-by and I measured PWM signals on scope with these
>> changes. Let me know if any thing else is required?
>>
>> [0] https://sourceforge.net/p/linuxptp/mailman/message/36943248/
> 
> From you measurements, can you tell whether or not the signal actually
> gets updated in the middle of a period, or does it only get updated at
> the end of a full period?

There is a corner case where period/duty-cycle gets updated in the middle of the
cycle. So let me give more details on how it works:

OMAP dual-mode timers with a upward  timer counter, can be configured in PWM
mode. When the timer counter overflows it gets reloaded with the load value(Load
register) and the pwm output goes up. When counter matches with match register,
the output goes down. So the load register is used to calculate the period and
similarly match register for duty cycle.

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 the PWM signal is high the following cases can happen:
1) When signal is high and new match value is > current timer counter. Then the
duty cycle gets reflected in the current cycle.(Duty_cycle for current period=
new match value -  previous load  value).
2) When signal is high and new match value is < current timer counter. Then the
period and duty cycle for the current cycle gets effected as well. Because the
signal is pulled down only when counter matches with match register, and this
happens only in the next cycle(after timer counter overflows). Then:
	- new Period for current cycle = (current period + new period)
	- new duty cycle for current cycle =  (current period + new duty_cycle).

Rest all the cases the values gets reflected in the next cycle.

As advised by Uwe, I have updated all these details in driver.

Thanks and regards,
Lokesh

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 01/04/20 1:52 PM, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
>> On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
>>> On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
>>>> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
>>>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
>>>>>> 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.
>>>>>
>>>>> I'm not sure what is sensible here. Stopping the PWM for a short period
>>>>> is bad, but maybe emitting a wrong period isn't better. You can however
>>>>> optimise it if only one of period or duty_cycle changes.
>>>>>
>>>>> @Thierry, what is your position here? I tend to say a short stop is
>>>>> preferable.
>>>>
>>>> It's not clear to me from the above description how exactly the device
>>>> behaves, but I suspect that it may latch the values in those registers
>>>> and only update the actual signal output once a period has finished. I
>>>> know of a couple of other devices that do that, so it wouldn't be
>>>> surprising.
>>>>
>>>> Even if that was not the case, I think this is just the kind of thing
>>>> that we have to live with. Sometimes it just isn't possible to have all
>>>> supported devices adhere strictly to an API. So I think the best we can
>>>> do is have an API that loosely defines what's supposed to happen and
>>>> make a best effort to implement those semantics. If a device deviates
>>>> slightly from those expectations, we can always cross fingers and hope
>>>> that things still work. And it looks like they are.
>>>>
>>>> So I think if Lokesh and Tony agree that this is the right thing to do
>>>> and have verified that things still work after this, that's about as
>>>> good as it's going to get.
>>>
>>> I'd say this isn't for the platform people to decide. My position here
>>> is that the PWM drivers should behave as uniform as possible to minimize
>>> surprises for consumers. And so it's a "PWM decision" that is to be made
>>> here, not an "omap decision".
>>
>> I think there's a fine line to be walked here. I agree that we should
>> aim to have as much consistency between drivers as possible. At the same
>> time I think we need to be pragmatic. As Lokesh said, the particular use
>> case here requires this type of on-the-fly adjustment of the PWM period
>> without stopping and restarting the PWM. It doesn't work otherwise. So
>> th alternative that you're proposing is to say that we don't support
>> that use-case, even though it works just fine given this particular
>> hardware. That's not really an option.
> 
> I understand your opinion here. The situation now is that in current
> mainline the driver stops the hardware for reconfiguration and it
> doesn't fit Lokesh's use case so he changed to on-the-fly update
> (accepting that maybe a wrong period is emitted). What if someone relies
> on the old behaviour? What if in a year someone comes and claims the
> wrong period is bad for their usecase and changes back to
> stop-to-update?
> 
> When I write a consumer driver, do I have a chance to know how the PWM,
> that I happen to use, behaves? To be able to get my consumer driver
> reliable I might need to know that however.
> 
>>>> I know this is perhaps cheating a little, or turning a blind eye, but I
>>>> don't know what the alternative would be. Do we want to tell people that
>>>> a given PWM controller can't be used if it doesn't work according to our
>>>> expectations? That's hard to argue if that controller works just fine
>>>> for all known use-cases.
>>>
>>> I'd like have some official policy here which of the alternatives is the
>>> preferred cheat.
>>>
>>> The situation here is that period and duty_cycle cannot be updated
>>> atomically. So the two options are:
>>>
>>>  - stop shortly
>>>  - update with hardware running and maybe emit a broken period
>>
>> I think we can already support both of those with the existing API. If
>> a consumer wants to stop the PWM while reconfiguring, they should be
>> able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
>> equivalent) and for the second case they can just do pwm_config() (or
>> the atomic equivalent).
> 
> Yes, the consumer can force the stop and update. But assume I'm "only" a
> consumer driver author and I want: atomic update and if this is not
> possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> So I cannot benefit from a good driver/hardware that can do atomic
> updates? Or I have to patch each driver that I actually use to use
> stop-to-update?
> 
>> Some hardware may actually require the PWM to be disabled before
>> reconfiguring, so they won't be able to strictly adhere to the second
>> use-case.
>>
>> But as discussed above, I don't want to strive for a lowest common
>> denominator that would preclude some more specific use-cases from
>> working if the hardware supports it.
>>
>> So I think we should aim for drivers to implement the semantics as
>> closely as possible. If the hardware doesn't support some of these
>> requirements strictly while a particular use-case depends on that, then
>> that just means that the hardware isn't compatible with that use-case.
>> Chances are that the system just isn't going to be designed to support
>> that use-case in the first place if the hardware can't do it.
>>
>> The sysfs interface is a bit of a special case here because it isn't
>> possible to know what use-cases people are going to come up with.
> 
> In my eyes the sysfs interface isn't special here. You also don't know
> what the OMAP PWM hardware is used for.
> 
>> It's most likely that they'll try something and if it doesn't work
>> they can see if a driver patch can improve things.
> 
> So either the group who prefers "stop-to-update" or the group who
> prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> driver patch?
> 
>> One possible extension that I can imagine would be to introduce some
>> sort of capability structure that drivers can fill in to describe the
>> behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
>> could describe that they are able to change the period and/or duty cycle
>> while the PWM is on. There could be another capability bit that says
>> that the current period will finish before new settings are applied. Yet
>> another capability could describe that duty-cycle and period can be
>> applied atomically. Consumers could then check those capabilities to see
>> if they match their requirements.
>>
>> But then again, I think that would just make things overly complicated.
>> None of the existing consumers need that, so it doesn't seem like there
>> is much demand for that feature. In practice I suspect most consumers
>> work fine despite potentially small deviations in how the PWM behaves.
> 
> I think the status quo is what I asked about above: People use sysfs and
> if the PWM behaves different than needed, the driver is patched and most
> of the time not mainlined. If your focus is to support a certain
> industrial system with a defined use case, this is fine. If however you
> target for an universal framework that works for any combination of
> consumer + lowlevel driver without patching (that at least is able to
> diagnose: This PWM cannot provide what my consumer needs), this is bad.
> Also this means that whenever a system designer changes something on
> their machine (kernel update, different hardware, an new usecase for a
> PWM) they might have to reverify if the given PWM driver behaves as
> needed.
> 
> My suggestion for now is to start documenting how the drivers behave
> expanding how limitations are documented in some drivers. So maybe
> change from "Limitations" to "Implementation and Hardware Details"?

Does it help if a new DT property is introduced across PWM subsystem,
representing dynamic period/duty-cycle updates. Based on this property driver
can handle the updates. If the property is not present existing behaviour can be
restored. This way based on the use-case things can be changed and need not
patch the driver :). Does this sound good or you have other thoughts?

Thanks and regards,
Lokesh

> 
> Best regards
> Uwe
> 

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

On Wed, Apr 01, 2020 at 03:52:21PM +0530, Lokesh Vutla wrote:
> Hi Uwe,
> 
> On 01/04/20 1:52 PM, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> >> On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> >>> On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> >>>> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> >>>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> >>>>>> 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.
> >>>>>
> >>>>> I'm not sure what is sensible here. Stopping the PWM for a short period
> >>>>> is bad, but maybe emitting a wrong period isn't better. You can however
> >>>>> optimise it if only one of period or duty_cycle changes.
> >>>>>
> >>>>> @Thierry, what is your position here? I tend to say a short stop is
> >>>>> preferable.
> >>>>
> >>>> It's not clear to me from the above description how exactly the device
> >>>> behaves, but I suspect that it may latch the values in those registers
> >>>> and only update the actual signal output once a period has finished. I
> >>>> know of a couple of other devices that do that, so it wouldn't be
> >>>> surprising.
> >>>>
> >>>> Even if that was not the case, I think this is just the kind of thing
> >>>> that we have to live with. Sometimes it just isn't possible to have all
> >>>> supported devices adhere strictly to an API. So I think the best we can
> >>>> do is have an API that loosely defines what's supposed to happen and
> >>>> make a best effort to implement those semantics. If a device deviates
> >>>> slightly from those expectations, we can always cross fingers and hope
> >>>> that things still work. And it looks like they are.
> >>>>
> >>>> So I think if Lokesh and Tony agree that this is the right thing to do
> >>>> and have verified that things still work after this, that's about as
> >>>> good as it's going to get.
> >>>
> >>> I'd say this isn't for the platform people to decide. My position here
> >>> is that the PWM drivers should behave as uniform as possible to minimize
> >>> surprises for consumers. And so it's a "PWM decision" that is to be made
> >>> here, not an "omap decision".
> >>
> >> I think there's a fine line to be walked here. I agree that we should
> >> aim to have as much consistency between drivers as possible. At the same
> >> time I think we need to be pragmatic. As Lokesh said, the particular use
> >> case here requires this type of on-the-fly adjustment of the PWM period
> >> without stopping and restarting the PWM. It doesn't work otherwise. So
> >> th alternative that you're proposing is to say that we don't support
> >> that use-case, even though it works just fine given this particular
> >> hardware. That's not really an option.
> > 
> > I understand your opinion here. The situation now is that in current
> > mainline the driver stops the hardware for reconfiguration and it
> > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > (accepting that maybe a wrong period is emitted). What if someone relies
> > on the old behaviour? What if in a year someone comes and claims the
> > wrong period is bad for their usecase and changes back to
> > stop-to-update?
> > 
> > When I write a consumer driver, do I have a chance to know how the PWM,
> > that I happen to use, behaves? To be able to get my consumer driver
> > reliable I might need to know that however.
> > 
> >>>> I know this is perhaps cheating a little, or turning a blind eye, but I
> >>>> don't know what the alternative would be. Do we want to tell people that
> >>>> a given PWM controller can't be used if it doesn't work according to our
> >>>> expectations? That's hard to argue if that controller works just fine
> >>>> for all known use-cases.
> >>>
> >>> I'd like have some official policy here which of the alternatives is the
> >>> preferred cheat.
> >>>
> >>> The situation here is that period and duty_cycle cannot be updated
> >>> atomically. So the two options are:
> >>>
> >>>  - stop shortly
> >>>  - update with hardware running and maybe emit a broken period
> >>
> >> I think we can already support both of those with the existing API. If
> >> a consumer wants to stop the PWM while reconfiguring, they should be
> >> able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> >> equivalent) and for the second case they can just do pwm_config() (or
> >> the atomic equivalent).
> > 
> > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > consumer driver author and I want: atomic update and if this is not
> > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > So I cannot benefit from a good driver/hardware that can do atomic
> > updates? Or I have to patch each driver that I actually use to use
> > stop-to-update?
> > 
> >> Some hardware may actually require the PWM to be disabled before
> >> reconfiguring, so they won't be able to strictly adhere to the second
> >> use-case.
> >>
> >> But as discussed above, I don't want to strive for a lowest common
> >> denominator that would preclude some more specific use-cases from
> >> working if the hardware supports it.
> >>
> >> So I think we should aim for drivers to implement the semantics as
> >> closely as possible. If the hardware doesn't support some of these
> >> requirements strictly while a particular use-case depends on that, then
> >> that just means that the hardware isn't compatible with that use-case.
> >> Chances are that the system just isn't going to be designed to support
> >> that use-case in the first place if the hardware can't do it.
> >>
> >> The sysfs interface is a bit of a special case here because it isn't
> >> possible to know what use-cases people are going to come up with.
> > 
> > In my eyes the sysfs interface isn't special here. You also don't know
> > what the OMAP PWM hardware is used for.
> > 
> >> It's most likely that they'll try something and if it doesn't work
> >> they can see if a driver patch can improve things.
> > 
> > So either the group who prefers "stop-to-update" or the group who
> > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > driver patch?
> > 
> >> One possible extension that I can imagine would be to introduce some
> >> sort of capability structure that drivers can fill in to describe the
> >> behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> >> could describe that they are able to change the period and/or duty cycle
> >> while the PWM is on. There could be another capability bit that says
> >> that the current period will finish before new settings are applied. Yet
> >> another capability could describe that duty-cycle and period can be
> >> applied atomically. Consumers could then check those capabilities to see
> >> if they match their requirements.
> >>
> >> But then again, I think that would just make things overly complicated.
> >> None of the existing consumers need that, so it doesn't seem like there
> >> is much demand for that feature. In practice I suspect most consumers
> >> work fine despite potentially small deviations in how the PWM behaves.
> > 
> > I think the status quo is what I asked about above: People use sysfs and
> > if the PWM behaves different than needed, the driver is patched and most
> > of the time not mainlined. If your focus is to support a certain
> > industrial system with a defined use case, this is fine. If however you
> > target for an universal framework that works for any combination of
> > consumer + lowlevel driver without patching (that at least is able to
> > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> > Also this means that whenever a system designer changes something on
> > their machine (kernel update, different hardware, an new usecase for a
> > PWM) they might have to reverify if the given PWM driver behaves as
> > needed.
> > 
> > My suggestion for now is to start documenting how the drivers behave
> > expanding how limitations are documented in some drivers. So maybe
> > change from "Limitations" to "Implementation and Hardware Details"?
> 
> Does it help if a new DT property is introduced across PWM subsystem,
> representing dynamic period/duty-cycle updates. Based on this property driver
> can handle the updates. If the property is not present existing behaviour can be
> restored. This way based on the use-case things can be changed and need not
> patch the driver :). Does this sound good or you have other thoughts?

That's something that I'd rather see in the pwm API. (Either by a rule
that drivers should prefer one or the other, or by making it
configurable.) IMHO this property doesn't belong into the hardware
description as it is a software property.

That's not constructive though as I don't have an idea how to map this
into the API.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

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

On Wed, Apr 01, 2020 at 10:22:27AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > > > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > > > > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > > > > 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.
> > > > > 
> > > > > I'm not sure what is sensible here. Stopping the PWM for a short period
> > > > > is bad, but maybe emitting a wrong period isn't better. You can however
> > > > > optimise it if only one of period or duty_cycle changes.
> > > > > 
> > > > > @Thierry, what is your position here? I tend to say a short stop is
> > > > > preferable.
> > > > 
> > > > It's not clear to me from the above description how exactly the device
> > > > behaves, but I suspect that it may latch the values in those registers
> > > > and only update the actual signal output once a period has finished. I
> > > > know of a couple of other devices that do that, so it wouldn't be
> > > > surprising.
> > > > 
> > > > Even if that was not the case, I think this is just the kind of thing
> > > > that we have to live with. Sometimes it just isn't possible to have all
> > > > supported devices adhere strictly to an API. So I think the best we can
> > > > do is have an API that loosely defines what's supposed to happen and
> > > > make a best effort to implement those semantics. If a device deviates
> > > > slightly from those expectations, we can always cross fingers and hope
> > > > that things still work. And it looks like they are.
> > > > 
> > > > So I think if Lokesh and Tony agree that this is the right thing to do
> > > > and have verified that things still work after this, that's about as
> > > > good as it's going to get.
> > > 
> > > I'd say this isn't for the platform people to decide. My position here
> > > is that the PWM drivers should behave as uniform as possible to minimize
> > > surprises for consumers. And so it's a "PWM decision" that is to be made
> > > here, not an "omap decision".
> > 
> > I think there's a fine line to be walked here. I agree that we should
> > aim to have as much consistency between drivers as possible. At the same
> > time I think we need to be pragmatic. As Lokesh said, the particular use
> > case here requires this type of on-the-fly adjustment of the PWM period
> > without stopping and restarting the PWM. It doesn't work otherwise. So
> > th alternative that you're proposing is to say that we don't support
> > that use-case, even though it works just fine given this particular
> > hardware. That's not really an option.
> 
> I understand your opinion here. The situation now is that in current
> mainline the driver stops the hardware for reconfiguration and it
> doesn't fit Lokesh's use case so he changed to on-the-fly update
> (accepting that maybe a wrong period is emitted). What if someone relies
> on the old behaviour? What if in a year someone comes and claims the
> wrong period is bad for their usecase and changes back to
> stop-to-update?

Relying on that old behaviour is wrong. If you really need to rely on
the PWM being stopped before reconfiguration, then you should explicitly
request that by first disabling the PWM, then reconfiguring it and then
reenabling it.

> When I write a consumer driver, do I have a chance to know how the PWM,
> that I happen to use, behaves? To be able to get my consumer driver
> reliable I might need to know that however.

No, there's currently no way of knowing. As such, I think the most
sensible thing to do at this point is to work with the API in order to
get the behaviour that you want. If you need to be able to reconfigure
the PWM without stopping it, then just call pwm_config(), or preferably
pwm_apply_state() with only the duty-cycle and/or period (or perhaps
even polarity) changed but keeping the PWM enabled.

Now if you're unlucky the driver won't support that and you'll notice
eventually. But there's also nothing you can do about that if the
hardware doesn't work that way. Even if the PWM framework had a way of
querying these types of peculiarities it wouldn't really proved much of
an advantage since the driver would just refuse to probe rather than
attempting to do this and maybe succeeding because you got lucky.

I suppose one variant that we could implement is to have drivers return
an error if they are being asked to reconfigure while the PWM remains
on. But then again, it's hard to know whether the driver writer really
depends on the PWM remaining on the whole time and seamlessly
transitioning to the new period/duty-cycle, or whether they just do it
out of convenience.

Take for example a backlight driver. Ideally you'd be able to seamlessly
change the brightness by just modifying the duty-cycle while the PWM is
enabled. If you have to turn the PWM off, reconfigure it and then turn
it back on, it could always happen that the backlight may flicker
because it actually goes completely dark for a little bit. In practice I
would expect the delay to be negligible because this is ultimately
converted to an analog power and hence the small dip to 0 isn't going to
matter very much. So if we change the API to refuse to reconfigure at
runtime for cases where the PWM has to be disabled and enabled during
reconfiguration, that may all of a sudden cause failures where none were
observed before.

> > > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > > don't know what the alternative would be. Do we want to tell people that
> > > > a given PWM controller can't be used if it doesn't work according to our
> > > > expectations? That's hard to argue if that controller works just fine
> > > > for all known use-cases.
> > > 
> > > I'd like have some official policy here which of the alternatives is the
> > > preferred cheat.
> > > 
> > > The situation here is that period and duty_cycle cannot be updated
> > > atomically. So the two options are:
> > > 
> > >  - stop shortly
> > >  - update with hardware running and maybe emit a broken period
> > 
> > I think we can already support both of those with the existing API. If
> > a consumer wants to stop the PWM while reconfiguring, they should be
> > able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > equivalent) and for the second case they can just do pwm_config() (or
> > the atomic equivalent).
> 
> Yes, the consumer can force the stop and update. But assume I'm "only" a
> consumer driver author and I want: atomic update and if this is not
> possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> So I cannot benefit from a good driver/hardware that can do atomic
> updates? Or I have to patch each driver that I actually use to use
> stop-to-update?

You could do what everybody does and just assume that atomic update
works. If it works with the particular PWM device/driver that you have
that may be all you care about. Obviously that may not be true for a
different chip.

On the other hand, if you absolutely must ensure that there must never
be any glitches whatsoever but don't care whether the PWM goes through a
disable/enable sequence, then doing so explicitly is going to be your
best bet. From a consumer driver point of view it isn't going to matter
because even if we had a way of distinguishing between these
capabilities you'd still have to have code to deal with both.

So the only relevant use-case here would be if you had a requirement to
perform on-the-fly-and-strictly-correct updates. The API doesn't give
those kinds of guarantees. So we would need an extension that consumers
can query to determine whether what they want to do will work.

But like I said above, the chances that you will run into this are slim
because use-cases are typically known ahead of time and devices are
designed to be able to support them. So if you build a device that needs
to support these strict requirements, then you need to make sure the
hardware supports it, too. And when the hardware supports it, then the
driver should implement ->config() or ->apply() in a way that allows
this.

> > Some hardware may actually require the PWM to be disabled before
> > reconfiguring, so they won't be able to strictly adhere to the second
> > use-case.
> > 
> > But as discussed above, I don't want to strive for a lowest common
> > denominator that would preclude some more specific use-cases from
> > working if the hardware supports it.
> > 
> > So I think we should aim for drivers to implement the semantics as
> > closely as possible. If the hardware doesn't support some of these
> > requirements strictly while a particular use-case depends on that, then
> > that just means that the hardware isn't compatible with that use-case.
> > Chances are that the system just isn't going to be designed to support
> > that use-case in the first place if the hardware can't do it.
> > 
> > The sysfs interface is a bit of a special case here because it isn't
> > possible to know what use-cases people are going to come up with.
> 
> In my eyes the sysfs interface isn't special here. You also don't know
> what the OMAP PWM hardware is used for.

But platform designers will know what their device will be used for. If
they know that it will be used for a case that the OMAP PWM doesn't
support, then they had better choose a different chip. Or add an extra
IC that provides a PWM that can do what they need.

> > It's most likely that they'll try something and if it doesn't work
> > they can see if a driver patch can improve things.
> 
> So either the group who prefers "stop-to-update" or the group who
> prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> driver patch?

No, the group that prefers "stop-to-update" should make that explicit
and write a consumer driver that first disables, then reconfigured and
then reenables the PWM.

If they don't *need* to update the PWM on the fly, then performing two
additional steps that would be happening anyway won't matter, right?

> > One possible extension that I can imagine would be to introduce some
> > sort of capability structure that drivers can fill in to describe the
> > behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > could describe that they are able to change the period and/or duty cycle
> > while the PWM is on. There could be another capability bit that says
> > that the current period will finish before new settings are applied. Yet
> > another capability could describe that duty-cycle and period can be
> > applied atomically. Consumers could then check those capabilities to see
> > if they match their requirements.
> > 
> > But then again, I think that would just make things overly complicated.
> > None of the existing consumers need that, so it doesn't seem like there
> > is much demand for that feature. In practice I suspect most consumers
> > work fine despite potentially small deviations in how the PWM behaves.
> 
> I think the status quo is what I asked about above: People use sysfs and
> if the PWM behaves different than needed, the driver is patched and most
> of the time not mainlined. If your focus is to support a certain
> industrial system with a defined use case, this is fine. If however you
> target for an universal framework that works for any combination of
> consumer + lowlevel driver without patching (that at least is able to
> diagnose: This PWM cannot provide what my consumer needs), this is bad.

Again, my response to this is: how is this going to be beneficial? In
practice the way that this would work is that the consumer driver would
fail if presented with a PWM that doesn't meet the strict requirements.
Now if the requirements really are that strict, then that sounds like a
good idea.

But one issue I foresee with this is that we'll end up giving consumers
too much of a toolkit to restrict things. What if the consumer driver
author assumes wrongly that a given set of requirements exists? What if
for some combination of hardware that doesn't strictly conform to those
requirements it might still work? Sometimes you may not notice the
difference, at other times there may be some impact like a visual glitch
or so, but that may be something that users are willing to accept rather
than not have support at all.

> Also this means that whenever a system designer changes something on
> their machine (kernel update, different hardware, an new usecase for a
> PWM) they might have to reverify if the given PWM driver behaves as
> needed.

I don't expect this type of change to happen very often. There's always
going to be some type of fine-tuning before a driver's behaviour is
completely stabilized. And then there could still always be other
factors impacting behaviour that aren't even related to the PWM
framework.

I suspect that most people will have an array of tests to validate that
everything still works after a kernel update. Obviously we don't want a
new kernel to behave completely differently, but we're not talking about
that here. This is merely dropping a needless disable/enable from a
configuration. If somebody was relying on this happening they were wrong
to rely on it to begin with because the API does not guarantee it.

> My suggestion for now is to start documenting how the drivers behave
> expanding how limitations are documented in some drivers. So maybe
> change from "Limitations" to "Implementation and Hardware Details"?

Yes, collecting such information is always a good idea.

Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

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

On Wed, Apr 01, 2020 at 01:47:32PM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 01, 2020 at 03:52:21PM +0530, Lokesh Vutla wrote:
> > Hi Uwe,
> > 
> > On 01/04/20 1:52 PM, Uwe Kleine-König wrote:
> > > Hello Thierry,
> > > 
> > > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > >> On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> > >>> On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > >>>> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > >>>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > >>>>>> 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.
> > >>>>>
> > >>>>> I'm not sure what is sensible here. Stopping the PWM for a short period
> > >>>>> is bad, but maybe emitting a wrong period isn't better. You can however
> > >>>>> optimise it if only one of period or duty_cycle changes.
> > >>>>>
> > >>>>> @Thierry, what is your position here? I tend to say a short stop is
> > >>>>> preferable.
> > >>>>
> > >>>> It's not clear to me from the above description how exactly the device
> > >>>> behaves, but I suspect that it may latch the values in those registers
> > >>>> and only update the actual signal output once a period has finished. I
> > >>>> know of a couple of other devices that do that, so it wouldn't be
> > >>>> surprising.
> > >>>>
> > >>>> Even if that was not the case, I think this is just the kind of thing
> > >>>> that we have to live with. Sometimes it just isn't possible to have all
> > >>>> supported devices adhere strictly to an API. So I think the best we can
> > >>>> do is have an API that loosely defines what's supposed to happen and
> > >>>> make a best effort to implement those semantics. If a device deviates
> > >>>> slightly from those expectations, we can always cross fingers and hope
> > >>>> that things still work. And it looks like they are.
> > >>>>
> > >>>> So I think if Lokesh and Tony agree that this is the right thing to do
> > >>>> and have verified that things still work after this, that's about as
> > >>>> good as it's going to get.
> > >>>
> > >>> I'd say this isn't for the platform people to decide. My position here
> > >>> is that the PWM drivers should behave as uniform as possible to minimize
> > >>> surprises for consumers. And so it's a "PWM decision" that is to be made
> > >>> here, not an "omap decision".
> > >>
> > >> I think there's a fine line to be walked here. I agree that we should
> > >> aim to have as much consistency between drivers as possible. At the same
> > >> time I think we need to be pragmatic. As Lokesh said, the particular use
> > >> case here requires this type of on-the-fly adjustment of the PWM period
> > >> without stopping and restarting the PWM. It doesn't work otherwise. So
> > >> th alternative that you're proposing is to say that we don't support
> > >> that use-case, even though it works just fine given this particular
> > >> hardware. That's not really an option.
> > > 
> > > I understand your opinion here. The situation now is that in current
> > > mainline the driver stops the hardware for reconfiguration and it
> > > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > > (accepting that maybe a wrong period is emitted). What if someone relies
> > > on the old behaviour? What if in a year someone comes and claims the
> > > wrong period is bad for their usecase and changes back to
> > > stop-to-update?
> > > 
> > > When I write a consumer driver, do I have a chance to know how the PWM,
> > > that I happen to use, behaves? To be able to get my consumer driver
> > > reliable I might need to know that however.
> > > 
> > >>>> I know this is perhaps cheating a little, or turning a blind eye, but I
> > >>>> don't know what the alternative would be. Do we want to tell people that
> > >>>> a given PWM controller can't be used if it doesn't work according to our
> > >>>> expectations? That's hard to argue if that controller works just fine
> > >>>> for all known use-cases.
> > >>>
> > >>> I'd like have some official policy here which of the alternatives is the
> > >>> preferred cheat.
> > >>>
> > >>> The situation here is that period and duty_cycle cannot be updated
> > >>> atomically. So the two options are:
> > >>>
> > >>>  - stop shortly
> > >>>  - update with hardware running and maybe emit a broken period
> > >>
> > >> I think we can already support both of those with the existing API. If
> > >> a consumer wants to stop the PWM while reconfiguring, they should be
> > >> able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > >> equivalent) and for the second case they can just do pwm_config() (or
> > >> the atomic equivalent).
> > > 
> > > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > > consumer driver author and I want: atomic update and if this is not
> > > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > > So I cannot benefit from a good driver/hardware that can do atomic
> > > updates? Or I have to patch each driver that I actually use to use
> > > stop-to-update?
> > > 
> > >> Some hardware may actually require the PWM to be disabled before
> > >> reconfiguring, so they won't be able to strictly adhere to the second
> > >> use-case.
> > >>
> > >> But as discussed above, I don't want to strive for a lowest common
> > >> denominator that would preclude some more specific use-cases from
> > >> working if the hardware supports it.
> > >>
> > >> So I think we should aim for drivers to implement the semantics as
> > >> closely as possible. If the hardware doesn't support some of these
> > >> requirements strictly while a particular use-case depends on that, then
> > >> that just means that the hardware isn't compatible with that use-case.
> > >> Chances are that the system just isn't going to be designed to support
> > >> that use-case in the first place if the hardware can't do it.
> > >>
> > >> The sysfs interface is a bit of a special case here because it isn't
> > >> possible to know what use-cases people are going to come up with.
> > > 
> > > In my eyes the sysfs interface isn't special here. You also don't know
> > > what the OMAP PWM hardware is used for.
> > > 
> > >> It's most likely that they'll try something and if it doesn't work
> > >> they can see if a driver patch can improve things.
> > > 
> > > So either the group who prefers "stop-to-update" or the group who
> > > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > > driver patch?
> > > 
> > >> One possible extension that I can imagine would be to introduce some
> > >> sort of capability structure that drivers can fill in to describe the
> > >> behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > >> could describe that they are able to change the period and/or duty cycle
> > >> while the PWM is on. There could be another capability bit that says
> > >> that the current period will finish before new settings are applied. Yet
> > >> another capability could describe that duty-cycle and period can be
> > >> applied atomically. Consumers could then check those capabilities to see
> > >> if they match their requirements.
> > >>
> > >> But then again, I think that would just make things overly complicated.
> > >> None of the existing consumers need that, so it doesn't seem like there
> > >> is much demand for that feature. In practice I suspect most consumers
> > >> work fine despite potentially small deviations in how the PWM behaves.
> > > 
> > > I think the status quo is what I asked about above: People use sysfs and
> > > if the PWM behaves different than needed, the driver is patched and most
> > > of the time not mainlined. If your focus is to support a certain
> > > industrial system with a defined use case, this is fine. If however you
> > > target for an universal framework that works for any combination of
> > > consumer + lowlevel driver without patching (that at least is able to
> > > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> > > Also this means that whenever a system designer changes something on
> > > their machine (kernel update, different hardware, an new usecase for a
> > > PWM) they might have to reverify if the given PWM driver behaves as
> > > needed.
> > > 
> > > My suggestion for now is to start documenting how the drivers behave
> > > expanding how limitations are documented in some drivers. So maybe
> > > change from "Limitations" to "Implementation and Hardware Details"?
> > 
> > Does it help if a new DT property is introduced across PWM subsystem,
> > representing dynamic period/duty-cycle updates. Based on this property driver
> > can handle the updates. If the property is not present existing behaviour can be
> > restored. This way based on the use-case things can be changed and need not
> > patch the driver :). Does this sound good or you have other thoughts?
> 
> That's something that I'd rather see in the pwm API. (Either by a rule
> that drivers should prefer one or the other, or by making it
> configurable.) IMHO this property doesn't belong into the hardware
> description as it is a software property.
> 
> That's not constructive though as I don't have an idea how to map this
> into the API.

We can already enforce disable/config/enable with the existing API. The
only think that we can't enforce is that a configuration will always be
applied atomically or without disabling and reenabling the PWM.

One possible solution would be to extend struct pwm_state with a set of
flags that can be set. For that PTP kind of applications, consumers
could set some pwm_state.strict (or whatever) flag and then a driver
could fail ->apply() if it doesn't support changing the period/duty-
cycle atomically and without disabling the PWM first. Or it could be
more fine-grained, like:

	state.on_the_fly = true;
	state.consistent = true;

To specify that the PWM needs to be changed on the fly (i.e. without
disabling and reenabling) and duty-cycle and period must be consistent
(i.e. be applied to the signal at the same time).

Some driver may be able to only respect state.on_the_fly == true but not
state.consistent == true.

But then again, I don't think we'll see those cases in practice, since
no hardware designer is going to make a board for a PTP use-case with a
PWM that doesn't support it.

That said, if somebody sees value in that and can come up with a good
series of patches and concrete use-cases to show how this would be
useful, I'd be willing to take those patches.

Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello Thierry,

On Wed, Apr 01, 2020 at 08:28:33PM +0200, Thierry Reding wrote:
> On Wed, Apr 01, 2020 at 10:22:27AM +0200, Uwe Kleine-König wrote:
> > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > > I think there's a fine line to be walked here. I agree that we should
> > > aim to have as much consistency between drivers as possible. At the same
> > > time I think we need to be pragmatic. As Lokesh said, the particular use
> > > case here requires this type of on-the-fly adjustment of the PWM period
> > > without stopping and restarting the PWM. It doesn't work otherwise. So
> > > th alternative that you're proposing is to say that we don't support
> > > that use-case, even though it works just fine given this particular
> > > hardware. That's not really an option.
> > 
> > I understand your opinion here. The situation now is that in current
> > mainline the driver stops the hardware for reconfiguration and it
> > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > (accepting that maybe a wrong period is emitted). What if someone relies
> > on the old behaviour? What if in a year someone comes and claims the
> > wrong period is bad for their usecase and changes back to
> > stop-to-update?
> 
> Relying on that old behaviour is wrong.  If you really need to rely on
> the PWM being stopped before reconfiguration, then you should
> explicitly request that by first disabling the PWM, then reconfiguring
> it and then reenabling it.

You got me wrong here. See below.

> > When I write a consumer driver, do I have a chance to know how the PWM,
> > that I happen to use, behaves? To be able to get my consumer driver
> > reliable I might need to know that however.
> 
> No, there's currently no way of knowing. As such, I think the most
> sensible thing to do at this point is to work with the API in order to
> get the behaviour that you want.

I want to switch from (duty_cycle, period) = (d1, p1) to
(duty_cycle, period) = (d2, p2) without seeing (d1, p2) or (d2, p1) on
the wire. How do I do this with Lokesh's patch applied?

In my eyes this is something sensible to rely on. I agree that relying
on a stop implemented by the driver is broken, but not wanting to seeing
a (d1, p2) is sane.

> Now if you're unlucky the driver won't support that and you'll notice
> eventually. But there's also nothing you can do about that if the
> hardware doesn't work that way. Even if the PWM framework had a way of
> querying these types of peculiarities it wouldn't really proved much of
> an advantage since the driver would just refuse to probe rather than
> attempting to do this and maybe succeeding because you got lucky.

It depends on the use case. If it's about a display that then has no
backlight the damage is limited. If however I drive a step motor that
rotates a laser some unexpected behaviour that only happens once in
100000 cases and so isn't catched during development is quite bad.

> > > > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > > > don't know what the alternative would be. Do we want to tell people that
> > > > > a given PWM controller can't be used if it doesn't work according to our
> > > > > expectations? That's hard to argue if that controller works just fine
> > > > > for all known use-cases.
> > > > 
> > > > I'd like have some official policy here which of the alternatives is the
> > > > preferred cheat.
> > > > 
> > > > The situation here is that period and duty_cycle cannot be updated
> > > > atomically. So the two options are:
> > > > 
> > > >  - stop shortly
> > > >  - update with hardware running and maybe emit a broken period
> > > 
> > > I think we can already support both of those with the existing API. If
> > > a consumer wants to stop the PWM while reconfiguring, they should be
> > > able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > > equivalent) and for the second case they can just do pwm_config() (or
> > > the atomic equivalent).
> > 
> > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > consumer driver author and I want: atomic update and if this is not
> > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > So I cannot benefit from a good driver/hardware that can do atomic
> > updates? Or I have to patch each driver that I actually use to use
> > stop-to-update?
> 
> You could do what everybody does and just assume that atomic update
> works.

There are people out there that are more demanding. If you have 1000000
machines in the field and only then find out that they all fail to
operate correctly with a certain small but positive probability and you
have to send someone to each machine to fix that, that's bad.

> If it works with the particular PWM device/driver that you
> have that may be all you care about. Obviously that may not be true
> for a different chip.
> 
> On the other hand, if you absolutely must ensure that there must never
> be any glitches whatsoever but don't care whether the PWM goes through a
> disable/enable sequence, then doing so explicitly is going to be your
> best bet. From a consumer driver point of view it isn't going to matter
> because even if we had a way of distinguishing between these
> capabilities you'd still have to have code to deal with both.

There is a nice upside however: You have a good chance to notice this
problem before the 1000000 devices are shipped to production and then
escalate the problem and let the responsible people make an informed
choice.

> So the only relevant use-case here would be if you had a requirement to
> perform on-the-fly-and-strictly-correct updates. The API doesn't give
> those kinds of guarantees. So we would need an extension that consumers
> can query to determine whether what they want to do will work.
> 
> But like I said above, the chances that you will run into this are slim
> because use-cases are typically known ahead of time and devices are
> designed to be able to support them. So if you build a device that needs
> to support these strict requirements, then you need to make sure the
> hardware supports it, too. And when the hardware supports it, then the
> driver should implement ->config() or ->apply() in a way that allows
> this.

In my bubble that sometimes fails. The hardware is done and only then
someone notices that a certain case wasn't considered.

> > > Some hardware may actually require the PWM to be disabled before
> > > reconfiguring, so they won't be able to strictly adhere to the second
> > > use-case.
> > > 
> > > But as discussed above, I don't want to strive for a lowest common
> > > denominator that would preclude some more specific use-cases from
> > > working if the hardware supports it.
> > > 
> > > So I think we should aim for drivers to implement the semantics as
> > > closely as possible. If the hardware doesn't support some of these
> > > requirements strictly while a particular use-case depends on that, then
> > > that just means that the hardware isn't compatible with that use-case.
> > > Chances are that the system just isn't going to be designed to support
> > > that use-case in the first place if the hardware can't do it.
> > > 
> > > The sysfs interface is a bit of a special case here because it isn't
> > > possible to know what use-cases people are going to come up with.
> > 
> > In my eyes the sysfs interface isn't special here. You also don't know
> > what the OMAP PWM hardware is used for.
> 
> But platform designers will know what their device will be used for.

Right. But if you rely on the people chosing the components for the
platform to notice that the SoC's PWM has a glitch that is bad for the
use case I think that's naive.

> > > It's most likely that they'll try something and if it doesn't work
> > > they can see if a driver patch can improve things.
> > 
> > So either the group who prefers "stop-to-update" or the group who
> > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > driver patch?
> 
> No, the group that prefers "stop-to-update" should make that explicit
> and write a consumer driver that first disables, then reconfigured and
> then reenables the PWM.

Both groups prefer atomic updates, and just prefer different cluches if
atomic isn't possible. So doing an explicit stop is bad. Also I'd expect
the timing of a driver doing the stop is better than when the consumer
explicitly stops.

> If they don't *need* to update the PWM on the fly, then performing two
> additional steps that would be happening anyway won't matter, right?

I don't agree here. Moreover the consumer might be preempted between
stop and reconfiguration. (And you don't want to disable preemption over
a pwm-API calls as they might take quite some time assuming they block
until periods are completed.)

> > > One possible extension that I can imagine would be to introduce some
> > > sort of capability structure that drivers can fill in to describe the
> > > behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > > could describe that they are able to change the period and/or duty cycle
> > > while the PWM is on. There could be another capability bit that says
> > > that the current period will finish before new settings are applied. Yet
> > > another capability could describe that duty-cycle and period can be
> > > applied atomically. Consumers could then check those capabilities to see
> > > if they match their requirements.
> > > 
> > > But then again, I think that would just make things overly complicated.
> > > None of the existing consumers need that, so it doesn't seem like there
> > > is much demand for that feature. In practice I suspect most consumers
> > > work fine despite potentially small deviations in how the PWM behaves.
> > 
> > I think the status quo is what I asked about above: People use sysfs and
> > if the PWM behaves different than needed, the driver is patched and most
> > of the time not mainlined. If your focus is to support a certain
> > industrial system with a defined use case, this is fine. If however you
> > target for an universal framework that works for any combination of
> > consumer + lowlevel driver without patching (that at least is able to
> > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> 
> Again, my response to this is: how is this going to be beneficial? In
> practice the way that this would work is that the consumer driver would
> fail if presented with a PWM that doesn't meet the strict requirements.
> Now if the requirements really are that strict, then that sounds like a
> good idea.
> 
> But one issue I foresee with this is that we'll end up giving consumers
> too much of a toolkit to restrict things. What if the consumer driver
> author assumes wrongly that a given set of requirements exists?

If you want to catch usage errors you're doomed anyhow.

> What if for some combination of hardware that doesn't strictly conform
> to those requirements it might still work?

Then this is a bug that can be fixed. And it's a bug that is not in the
PWM framework's area, so we even don't really have to care.

> Sometimes you may not notice the difference, at other times there may
> be some impact like a visual glitch or so, but that may be something
> that users are willing to accept rather than not have support at all.

If laxer requirements are ok, they shouldn't request strict ones.

> > Also this means that whenever a system designer changes something on
> > their machine (kernel update, different hardware, an new usecase for a
> > PWM) they might have to reverify if the given PWM driver behaves as
> > needed.
> 
> I don't expect this type of change to happen very often.

Yeah, I also expect that most things don't break on such a change. But
Lokesh's patch is an example that justifies to be vigilant because the
implemented change in behaviour is a good one for Lokesh but might break
things for someone else. And I don't expect that the common test
coverage to be that good to catch the glitch during test.

> There's always going to be some type of fine-tuning before a driver's
> behaviour is completely stabilized. And then there could still always
> be other factors impacting behaviour that aren't even related to the
> PWM framework.

I think I don't get what you intend to say here. I understand "As
updating the kernel might make a PLL unstable, changing behaviour for
the PWM is fine."

> I suspect that most people will have an array of tests to validate that
> everything still works after a kernel update. Obviously we don't want a
> new kernel to behave completely differently, but we're not talking about
> that here. This is merely dropping a needless disable/enable from a
> configuration. If somebody was relying on this happening they were wrong
> to rely on it to begin with because the API does not guarantee it.
> 
> > My suggestion for now is to start documenting how the drivers behave
> > expanding how limitations are documented in some drivers. So maybe
> > change from "Limitations" to "Implementation and Hardware Details"?
> 
> Yes, collecting such information is always a good idea.

So Lokesh's patch should at least be amended to add something like

 * Limitations:
 * - When changing both duty cycle and period, we cannot prevent in
 *   software that the output might produce a period with mixed
 *   settings (new period length and old duty cycle) without stopping
 *   the hardware.

at the top of the driver.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

On Wed, Apr 01, 2020 at 08:39:19PM +0200, Thierry Reding wrote:
> On Wed, Apr 01, 2020 at 01:47:32PM +0200, Uwe Kleine-König wrote:
> > On Wed, Apr 01, 2020 at 03:52:21PM +0530, Lokesh Vutla wrote:
> > > Hi Uwe,
> > > 
> > > On 01/04/20 1:52 PM, Uwe Kleine-König wrote:
> > > > Hello Thierry,
> > > > 
> > > > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > > >> On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-König wrote:
> > > >>> On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > > >>>> On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-König wrote:
> > > >>>>> On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > >>>>>> 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.
> > > >>>>>
> > > >>>>> I'm not sure what is sensible here. Stopping the PWM for a short period
> > > >>>>> is bad, but maybe emitting a wrong period isn't better. You can however
> > > >>>>> optimise it if only one of period or duty_cycle changes.
> > > >>>>>
> > > >>>>> @Thierry, what is your position here? I tend to say a short stop is
> > > >>>>> preferable.
> > > >>>>
> > > >>>> It's not clear to me from the above description how exactly the device
> > > >>>> behaves, but I suspect that it may latch the values in those registers
> > > >>>> and only update the actual signal output once a period has finished. I
> > > >>>> know of a couple of other devices that do that, so it wouldn't be
> > > >>>> surprising.
> > > >>>>
> > > >>>> Even if that was not the case, I think this is just the kind of thing
> > > >>>> that we have to live with. Sometimes it just isn't possible to have all
> > > >>>> supported devices adhere strictly to an API. So I think the best we can
> > > >>>> do is have an API that loosely defines what's supposed to happen and
> > > >>>> make a best effort to implement those semantics. If a device deviates
> > > >>>> slightly from those expectations, we can always cross fingers and hope
> > > >>>> that things still work. And it looks like they are.
> > > >>>>
> > > >>>> So I think if Lokesh and Tony agree that this is the right thing to do
> > > >>>> and have verified that things still work after this, that's about as
> > > >>>> good as it's going to get.
> > > >>>
> > > >>> I'd say this isn't for the platform people to decide. My position here
> > > >>> is that the PWM drivers should behave as uniform as possible to minimize
> > > >>> surprises for consumers. And so it's a "PWM decision" that is to be made
> > > >>> here, not an "omap decision".
> > > >>
> > > >> I think there's a fine line to be walked here. I agree that we should
> > > >> aim to have as much consistency between drivers as possible. At the same
> > > >> time I think we need to be pragmatic. As Lokesh said, the particular use
> > > >> case here requires this type of on-the-fly adjustment of the PWM period
> > > >> without stopping and restarting the PWM. It doesn't work otherwise. So
> > > >> th alternative that you're proposing is to say that we don't support
> > > >> that use-case, even though it works just fine given this particular
> > > >> hardware. That's not really an option.
> > > > 
> > > > I understand your opinion here. The situation now is that in current
> > > > mainline the driver stops the hardware for reconfiguration and it
> > > > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > > > (accepting that maybe a wrong period is emitted). What if someone relies
> > > > on the old behaviour? What if in a year someone comes and claims the
> > > > wrong period is bad for their usecase and changes back to
> > > > stop-to-update?
> > > > 
> > > > When I write a consumer driver, do I have a chance to know how the PWM,
> > > > that I happen to use, behaves? To be able to get my consumer driver
> > > > reliable I might need to know that however.
> > > > 
> > > >>>> I know this is perhaps cheating a little, or turning a blind eye, but I
> > > >>>> don't know what the alternative would be. Do we want to tell people that
> > > >>>> a given PWM controller can't be used if it doesn't work according to our
> > > >>>> expectations? That's hard to argue if that controller works just fine
> > > >>>> for all known use-cases.
> > > >>>
> > > >>> I'd like have some official policy here which of the alternatives is the
> > > >>> preferred cheat.
> > > >>>
> > > >>> The situation here is that period and duty_cycle cannot be updated
> > > >>> atomically. So the two options are:
> > > >>>
> > > >>>  - stop shortly
> > > >>>  - update with hardware running and maybe emit a broken period
> > > >>
> > > >> I think we can already support both of those with the existing API. If
> > > >> a consumer wants to stop the PWM while reconfiguring, they should be
> > > >> able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > > >> equivalent) and for the second case they can just do pwm_config() (or
> > > >> the atomic equivalent).
> > > > 
> > > > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > > > consumer driver author and I want: atomic update and if this is not
> > > > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > > > So I cannot benefit from a good driver/hardware that can do atomic
> > > > updates? Or I have to patch each driver that I actually use to use
> > > > stop-to-update?
> > > > 
> > > >> Some hardware may actually require the PWM to be disabled before
> > > >> reconfiguring, so they won't be able to strictly adhere to the second
> > > >> use-case.
> > > >>
> > > >> But as discussed above, I don't want to strive for a lowest common
> > > >> denominator that would preclude some more specific use-cases from
> > > >> working if the hardware supports it.
> > > >>
> > > >> So I think we should aim for drivers to implement the semantics as
> > > >> closely as possible. If the hardware doesn't support some of these
> > > >> requirements strictly while a particular use-case depends on that, then
> > > >> that just means that the hardware isn't compatible with that use-case.
> > > >> Chances are that the system just isn't going to be designed to support
> > > >> that use-case in the first place if the hardware can't do it.
> > > >>
> > > >> The sysfs interface is a bit of a special case here because it isn't
> > > >> possible to know what use-cases people are going to come up with.
> > > > 
> > > > In my eyes the sysfs interface isn't special here. You also don't know
> > > > what the OMAP PWM hardware is used for.
> > > > 
> > > >> It's most likely that they'll try something and if it doesn't work
> > > >> they can see if a driver patch can improve things.
> > > > 
> > > > So either the group who prefers "stop-to-update" or the group who
> > > > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > > > driver patch?
> > > > 
> > > >> One possible extension that I can imagine would be to introduce some
> > > >> sort of capability structure that drivers can fill in to describe the
> > > >> behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > > >> could describe that they are able to change the period and/or duty cycle
> > > >> while the PWM is on. There could be another capability bit that says
> > > >> that the current period will finish before new settings are applied. Yet
> > > >> another capability could describe that duty-cycle and period can be
> > > >> applied atomically. Consumers could then check those capabilities to see
> > > >> if they match their requirements.
> > > >>
> > > >> But then again, I think that would just make things overly complicated.
> > > >> None of the existing consumers need that, so it doesn't seem like there
> > > >> is much demand for that feature. In practice I suspect most consumers
> > > >> work fine despite potentially small deviations in how the PWM behaves.
> > > > 
> > > > I think the status quo is what I asked about above: People use sysfs and
> > > > if the PWM behaves different than needed, the driver is patched and most
> > > > of the time not mainlined. If your focus is to support a certain
> > > > industrial system with a defined use case, this is fine. If however you
> > > > target for an universal framework that works for any combination of
> > > > consumer + lowlevel driver without patching (that at least is able to
> > > > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> > > > Also this means that whenever a system designer changes something on
> > > > their machine (kernel update, different hardware, an new usecase for a
> > > > PWM) they might have to reverify if the given PWM driver behaves as
> > > > needed.
> > > > 
> > > > My suggestion for now is to start documenting how the drivers behave
> > > > expanding how limitations are documented in some drivers. So maybe
> > > > change from "Limitations" to "Implementation and Hardware Details"?
> > > 
> > > Does it help if a new DT property is introduced across PWM subsystem,
> > > representing dynamic period/duty-cycle updates. Based on this property driver
> > > can handle the updates. If the property is not present existing behaviour can be
> > > restored. This way based on the use-case things can be changed and need not
> > > patch the driver :). Does this sound good or you have other thoughts?
> > 
> > That's something that I'd rather see in the pwm API. (Either by a rule
> > that drivers should prefer one or the other, or by making it
> > configurable.) IMHO this property doesn't belong into the hardware
> > description as it is a software property.
> > 
> > That's not constructive though as I don't have an idea how to map this
> > into the API.
> 
> We can already enforce disable/config/enable with the existing API. The
> only think that we can't enforce is that a configuration will always be
> applied atomically or without disabling and reenabling the PWM.
> 
> One possible solution would be to extend struct pwm_state with a set of
> flags that can be set. For that PTP kind of applications, consumers
> could set some pwm_state.strict (or whatever) flag and then a driver
> could fail ->apply() if it doesn't support changing the period/duty-
> cycle atomically and without disabling the PWM first. Or it could be
> more fine-grained, like:
> 
> 	state.on_the_fly = true;
> 	state.consistent = true;
> 
> To specify that the PWM needs to be changed on the fly (i.e. without
> disabling and reenabling) and duty-cycle and period must be consistent
> (i.e. be applied to the signal at the same time).

I'm happy for now with explicitly documenting the quirks as we become
aware of them. This is already much better than the status quo and we're
not complicating stuff in a way that might be far from reality.

I still think stating a preference for one or the other might be
beneficial, but as we don't seem to agree on that, let's go with
documentation for now. Then maybe later with a better overview about the
capabilities of the available drivers we can make a good choice.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

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

On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Apr 01, 2020 at 08:28:33PM +0200, Thierry Reding wrote:
> > On Wed, Apr 01, 2020 at 10:22:27AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Mar 31, 2020 at 10:45:59PM +0200, Thierry Reding wrote:
> > > > I think there's a fine line to be walked here. I agree that we should
> > > > aim to have as much consistency between drivers as possible. At the same
> > > > time I think we need to be pragmatic. As Lokesh said, the particular use
> > > > case here requires this type of on-the-fly adjustment of the PWM period
> > > > without stopping and restarting the PWM. It doesn't work otherwise. So
> > > > th alternative that you're proposing is to say that we don't support
> > > > that use-case, even though it works just fine given this particular
> > > > hardware. That's not really an option.
> > > 
> > > I understand your opinion here. The situation now is that in current
> > > mainline the driver stops the hardware for reconfiguration and it
> > > doesn't fit Lokesh's use case so he changed to on-the-fly update
> > > (accepting that maybe a wrong period is emitted). What if someone relies
> > > on the old behaviour? What if in a year someone comes and claims the
> > > wrong period is bad for their usecase and changes back to
> > > stop-to-update?
> > 
> > Relying on that old behaviour is wrong.  If you really need to rely on
> > the PWM being stopped before reconfiguration, then you should
> > explicitly request that by first disabling the PWM, then reconfiguring
> > it and then reenabling it.
> 
> You got me wrong here. See below.
> 
> > > When I write a consumer driver, do I have a chance to know how the PWM,
> > > that I happen to use, behaves? To be able to get my consumer driver
> > > reliable I might need to know that however.
> > 
> > No, there's currently no way of knowing. As such, I think the most
> > sensible thing to do at this point is to work with the API in order to
> > get the behaviour that you want.
> 
> I want to switch from (duty_cycle, period) = (d1, p1) to
> (duty_cycle, period) = (d2, p2) without seeing (d1, p2) or (d2, p1) on
> the wire. How do I do this with Lokesh's patch applied?
> 
> In my eyes this is something sensible to rely on. I agree that relying
> on a stop implemented by the driver is broken, but not wanting to seeing
> a (d1, p2) is sane.

I suppose it could be a sane requirement under some circumstances. In
other circumstances it may not matter. Making it a hard requirement
unconditionally would preclude the driver's usage in cases where it
doesn't matter.

Now, for many cases that als can never happen because the period is
always fixed. For example, the PWM consumer bindings specify that a
consumer should provide a fixed period in device tree. So once the
PWM has been configured that will never change, so you can only ever
be in a situation where you want to go from (d1, p) to (d2, p), so
there won't actually be a glitch.

That's certainly not true for all cases, and specifically with sysfs
you can always change the period as well as the duty-cycle.

> > Now if you're unlucky the driver won't support that and you'll notice
> > eventually. But there's also nothing you can do about that if the
> > hardware doesn't work that way. Even if the PWM framework had a way of
> > querying these types of peculiarities it wouldn't really proved much of
> > an advantage since the driver would just refuse to probe rather than
> > attempting to do this and maybe succeeding because you got lucky.
> 
> It depends on the use case. If it's about a display that then has no
> backlight the damage is limited. If however I drive a step motor that
> rotates a laser some unexpected behaviour that only happens once in
> 100000 cases and so isn't catched during development is quite bad.

You probably wouldn't be using sysfs to control the PWM in such a case.
My understanding is that stepper motors also typically use either a
fixed duty-cycle or a fixed period. I would also think that it wouldn't
matter if you stopped a PWM, reconfigured it and then started it again
for that particular application.

So I understand that there can be situations where this kind of glitch
isn't optimal, but I think we also need to be pragmatic and see what the
real use-cases are. I doubt that anyone trying to control a potentially
lethal laser would be using some standard PWM IP and run Linux to
control it. There are just too many things that could go wrong
(scheduling interfering, kernel crashing, ...) for that to be a good
idea.

> > > > > > I know this is perhaps cheating a little, or turning a blind eye, but I
> > > > > > don't know what the alternative would be. Do we want to tell people that
> > > > > > a given PWM controller can't be used if it doesn't work according to our
> > > > > > expectations? That's hard to argue if that controller works just fine
> > > > > > for all known use-cases.
> > > > > 
> > > > > I'd like have some official policy here which of the alternatives is the
> > > > > preferred cheat.
> > > > > 
> > > > > The situation here is that period and duty_cycle cannot be updated
> > > > > atomically. So the two options are:
> > > > > 
> > > > >  - stop shortly
> > > > >  - update with hardware running and maybe emit a broken period
> > > > 
> > > > I think we can already support both of those with the existing API. If
> > > > a consumer wants to stop the PWM while reconfiguring, they should be
> > > > able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
> > > > equivalent) and for the second case they can just do pwm_config() (or
> > > > the atomic equivalent).
> > > 
> > > Yes, the consumer can force the stop and update. But assume I'm "only" a
> > > consumer driver author and I want: atomic update and if this is not
> > > possible I prefer "stop-to-update" over "on-the-fly-and-maybe-faulty".
> > > So I cannot benefit from a good driver/hardware that can do atomic
> > > updates? Or I have to patch each driver that I actually use to use
> > > stop-to-update?
> > 
> > You could do what everybody does and just assume that atomic update
> > works.
> 
> There are people out there that are more demanding. If you have 1000000
> machines in the field and only then find out that they all fail to
> operate correctly with a certain small but positive probability and you
> have to send someone to each machine to fix that, that's bad.

Agreed. But that's not really what we're talking about here, right? This
isn't some undefined behaviour that would spontaneously trigger. We're
still talking about deterministic behaviour that's going to be the same
whether you do it once in a test lab or a million times in the field. A
PWM-controlled backlight that you adjust the brightness of in the lab is
going to work the same way as in the field. Any potential glitch would
be happening in the lab as well. It would in fact, constantly happen.

> > If it works with the particular PWM device/driver that you
> > have that may be all you care about. Obviously that may not be true
> > for a different chip.
> > 
> > On the other hand, if you absolutely must ensure that there must never
> > be any glitches whatsoever but don't care whether the PWM goes through a
> > disable/enable sequence, then doing so explicitly is going to be your
> > best bet. From a consumer driver point of view it isn't going to matter
> > because even if we had a way of distinguishing between these
> > capabilities you'd still have to have code to deal with both.
> 
> There is a nice upside however: You have a good chance to notice this
> problem before the 1000000 devices are shipped to production and then
> escalate the problem and let the responsible people make an informed
> choice.

I suspect that what will really happen is that somebody will just hack
the driver and remove the restriction, check that it works and ship it.

> > So the only relevant use-case here would be if you had a requirement to
> > perform on-the-fly-and-strictly-correct updates. The API doesn't give
> > those kinds of guarantees. So we would need an extension that consumers
> > can query to determine whether what they want to do will work.
> > 
> > But like I said above, the chances that you will run into this are slim
> > because use-cases are typically known ahead of time and devices are
> > designed to be able to support them. So if you build a device that needs
> > to support these strict requirements, then you need to make sure the
> > hardware supports it, too. And when the hardware supports it, then the
> > driver should implement ->config() or ->apply() in a way that allows
> > this.
> 
> In my bubble that sometimes fails. The hardware is done and only then
> someone notices that a certain case wasn't considered.

Of course this sometimes happens. And then it's actually great if the
API isn't all too pedantic and still allows you to adjust that backlight
brightness rather than flatout refusing to do anything, leaving you with
a bunch of devices that you have to scrap and then do a redesign.

So ultimately you end up "faking it", but if it works, it works.

For the rare cases where you can't fake it, it doesn't really matter
whether the framework refuses to do something because it knows that the
hardware isn't capable of doing it, or whether the driver will simply
try its best to do what it was asked to do and fails to deliver. You'll
notice one way or another and then you need to go back to the drawing
board and redo the design.

> > > > Some hardware may actually require the PWM to be disabled before
> > > > reconfiguring, so they won't be able to strictly adhere to the second
> > > > use-case.
> > > > 
> > > > But as discussed above, I don't want to strive for a lowest common
> > > > denominator that would preclude some more specific use-cases from
> > > > working if the hardware supports it.
> > > > 
> > > > So I think we should aim for drivers to implement the semantics as
> > > > closely as possible. If the hardware doesn't support some of these
> > > > requirements strictly while a particular use-case depends on that, then
> > > > that just means that the hardware isn't compatible with that use-case.
> > > > Chances are that the system just isn't going to be designed to support
> > > > that use-case in the first place if the hardware can't do it.
> > > > 
> > > > The sysfs interface is a bit of a special case here because it isn't
> > > > possible to know what use-cases people are going to come up with.
> > > 
> > > In my eyes the sysfs interface isn't special here. You also don't know
> > > what the OMAP PWM hardware is used for.
> > 
> > But platform designers will know what their device will be used for.
> 
> Right. But if you rely on the people chosing the components for the
> platform to notice that the SoC's PWM has a glitch that is bad for the
> use case I think that's naive.

See above. If the glitch is bad enough to make it unusable you're going
to notice and you have to redesign, whether the PWM framework rejects a
configuration or not.

My point is that if the glitch is minor enough not to be noticed, the
PWM framework not getting in the way may make you (and whoever's got
their money in this) very happy.

> > > > It's most likely that they'll try something and if it doesn't work
> > > > they can see if a driver patch can improve things.
> > > 
> > > So either the group who prefers "stop-to-update" or the group who
> > > prefers "on-the-fly-and-maybe-faulty" has to carry a system specific
> > > driver patch?
> > 
> > No, the group that prefers "stop-to-update" should make that explicit
> > and write a consumer driver that first disables, then reconfigured and
> > then reenables the PWM.
> 
> Both groups prefer atomic updates, and just prefer different cluches if
> atomic isn't possible. So doing an explicit stop is bad. Also I'd expect
> the timing of a driver doing the stop is better than when the consumer
> explicitly stops.

Yes, perhaps. It obviously depends a lot on what exactly the time is
that we're talking about here. If we're talking about a single pulse
every second, whether the driver stops and restarts or whether the
consumer does is probably negligible.

But again, I think if we really run into cases where the consumer knows
exactly which behaviour it wants, then I think we need to extend the API
to enable it to transport such notions.

We currently can't, and assumptions baked into the API are bound to be
the wrong ones every now and then.

> > If they don't *need* to update the PWM on the fly, then performing two
> > additional steps that would be happening anyway won't matter, right?
> 
> I don't agree here. Moreover the consumer might be preempted between
> stop and reconfiguration. (And you don't want to disable preemption over
> a pwm-API calls as they might take quite some time assuming they block
> until periods are completed.)

I hate to break it to you, but if timing is really that critical, you're
probably better off not using Linux.

> > > > One possible extension that I can imagine would be to introduce some
> > > > sort of capability structure that drivers can fill in to describe the
> > > > behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
> > > > could describe that they are able to change the period and/or duty cycle
> > > > while the PWM is on. There could be another capability bit that says
> > > > that the current period will finish before new settings are applied. Yet
> > > > another capability could describe that duty-cycle and period can be
> > > > applied atomically. Consumers could then check those capabilities to see
> > > > if they match their requirements.
> > > > 
> > > > But then again, I think that would just make things overly complicated.
> > > > None of the existing consumers need that, so it doesn't seem like there
> > > > is much demand for that feature. In practice I suspect most consumers
> > > > work fine despite potentially small deviations in how the PWM behaves.
> > > 
> > > I think the status quo is what I asked about above: People use sysfs and
> > > if the PWM behaves different than needed, the driver is patched and most
> > > of the time not mainlined. If your focus is to support a certain
> > > industrial system with a defined use case, this is fine. If however you
> > > target for an universal framework that works for any combination of
> > > consumer + lowlevel driver without patching (that at least is able to
> > > diagnose: This PWM cannot provide what my consumer needs), this is bad.
> > 
> > Again, my response to this is: how is this going to be beneficial? In
> > practice the way that this would work is that the consumer driver would
> > fail if presented with a PWM that doesn't meet the strict requirements.
> > Now if the requirements really are that strict, then that sounds like a
> > good idea.
> > 
> > But one issue I foresee with this is that we'll end up giving consumers
> > too much of a toolkit to restrict things. What if the consumer driver
> > author assumes wrongly that a given set of requirements exists?
> 
> If you want to catch usage errors you're doomed anyhow.
> 
> > What if for some combination of hardware that doesn't strictly conform
> > to those requirements it might still work?
> 
> Then this is a bug that can be fixed. And it's a bug that is not in the
> PWM framework's area, so we even don't really have to care.

It's still the responsibility of the PWM framework to provide the knobs
that make sense in terms of what the hardware capabilities are.

> > Sometimes you may not notice the difference, at other times there may
> > be some impact like a visual glitch or so, but that may be something
> > that users are willing to accept rather than not have support at all.
> 
> If laxer requirements are ok, they shouldn't request strict ones.

Well yeah. That's exactly why I'm pushing back on this. So far I haven't
yet seen a case where there actually were any such strict requirements.

> > > Also this means that whenever a system designer changes something on
> > > their machine (kernel update, different hardware, an new usecase for a
> > > PWM) they might have to reverify if the given PWM driver behaves as
> > > needed.
> > 
> > I don't expect this type of change to happen very often.
> 
> Yeah, I also expect that most things don't break on such a change. But
> Lokesh's patch is an example that justifies to be vigilant because the
> implemented change in behaviour is a good one for Lokesh but might break
> things for someone else. And I don't expect that the common test
> coverage to be that good to catch the glitch during test.

That's certainly true, but so far nobody's complained about this patch
breaking their use-case, so I'm going to assume for now that it's
harmless. If that changes at some point we need to revisit this of
course.

However, it's no use sitting on patches waiting for somebody to report a
regression, otherwise we could never make changes to a driver. Does the
absence of a regression report mean that there is no regression, or that
the regression just hasn't been noticed yet?

> > There's always going to be some type of fine-tuning before a driver's
> > behaviour is completely stabilized. And then there could still always
> > be other factors impacting behaviour that aren't even related to the
> > PWM framework.
> 
> I think I don't get what you intend to say here. I understand "As
> updating the kernel might make a PLL unstable, changing behaviour for
> the PWM is fine."

What I'm trying to say is that it's normal for a driver to go through
these kinds of changes. It used to work for some use-cases, now it's
being extended to work for one more. The behaviour is now slightly
different, but hopefully it won't impact existing use-cases.

That's in contrast to saying that we can never change existing drivers
because it may cause existing use-cases to fail.

Furthermore there are so many moving parts in Linux that upgrading to a
new kernel always requires you to revalidate functionality.

> > I suspect that most people will have an array of tests to validate that
> > everything still works after a kernel update. Obviously we don't want a
> > new kernel to behave completely differently, but we're not talking about
> > that here. This is merely dropping a needless disable/enable from a
> > configuration. If somebody was relying on this happening they were wrong
> > to rely on it to begin with because the API does not guarantee it.
> > 
> > > My suggestion for now is to start documenting how the drivers behave
> > > expanding how limitations are documented in some drivers. So maybe
> > > change from "Limitations" to "Implementation and Hardware Details"?
> > 
> > Yes, collecting such information is always a good idea.
> 
> So Lokesh's patch should at least be amended to add something like
> 
>  * Limitations:
>  * - When changing both duty cycle and period, we cannot prevent in
>  *   software that the output might produce a period with mixed
>  *   settings (new period length and old duty cycle) without stopping
>  *   the hardware.
> 
> at the top of the driver.

Something to that effect already exists in the driver.

Thierry

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

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello Thierry,

On Wed, Apr 01, 2020 at 11:37:38PM +0200, Thierry Reding wrote:
> On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-König wrote:
> > There are people out there that are more demanding. If you have 1000000
> > machines in the field and only then find out that they all fail to
> > operate correctly with a certain small but positive probability and you
> > have to send someone to each machine to fix that, that's bad.
> 
> Agreed. But that's not really what we're talking about here, right? This
> isn't some undefined behaviour that would spontaneously trigger.
> We're still talking about deterministic behaviour that's going to be
> the same whether you do it once in a test lab or a million times in
> the field. A PWM-controlled backlight that you adjust the brightness
> of in the lab is going to work the same way as in the field. Any
> potential glitch would be happening in the lab as well. It would in
> fact, constantly happen.

The glitch we're talking about really doesn't happen that often that we
should rely on this problem popping up during testing. To change both
period and duty_cycle two register writes are necessary. The hickup only
happens if after the first register write a period ends before the
second register write hits the hardware. (For the omap driver it might
happen a bit more often, don't remember the details, but I think the
above is what could be reached theoretically.)

> For the rare cases where you can't fake it, it doesn't really matter
> whether the framework refuses to do something because it knows that the
> hardware isn't capable of doing it, or whether the driver will simply
> try its best to do what it was asked to do and fails to deliver. You'll
> notice one way or another and then you need to go back to the drawing
> board and redo the design.

I think you didn't understand up to now that the glitch depends on
timing between register access and the PWM period and so is really hard
to actually reproduce. In the hope you understood that now, I assume you
see that your argumentation is incomplete.

And I think to assume that somebody would complain about a race
condition in a patch that just hit next is quite optimistic.

Having said that I don't know how critical this really is. Given that
the PWM under discussion doesn't complete periods on stop, it probably
isn't.

I spend some time thinking about when the glitch actually happens.
Currently the load value is written first and then the match value.
If no period ends between the two writes there is only a problem when in
the currently running period the match event didn't happen yet. Then we
see a cycle with

   .period = oldperiod + newperiod
   .dutycycle = oldperiod + newdutycycle

(if the new match value isn't hit in the current cycle) or one with

   .period = oldperiod
   .duty_cycle = newdutycycle + (oldperiod - newperiod)

(if the new match value is hit in the current cycle). The probability
that one of the two happen is: olddutycycle / oldperiod which is quite
probable. (With olddutycycle = oldperiod there is no problem though.)

If after writing the new load value and before writing the new match
value a period ends it might happen that we see a cycle with

  .period = newperiod
  .dutycycle = olddutycycle + (newperiod - oldperiod)

(if the previous match value is used) or one with

  .period = 2 * newperiod
  .dutycycle = newperiod + newdutycycle

(if new match value is written too late for the first cycle with the new
period).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

On 02/04/20 7:32 PM, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Apr 01, 2020 at 11:37:38PM +0200, Thierry Reding wrote:
>> On Wed, Apr 01, 2020 at 10:31:56PM +0200, Uwe Kleine-König wrote:
>>> There are people out there that are more demanding. If you have 1000000
>>> machines in the field and only then find out that they all fail to
>>> operate correctly with a certain small but positive probability and you
>>> have to send someone to each machine to fix that, that's bad.
>>
>> Agreed. But that's not really what we're talking about here, right? This
>> isn't some undefined behaviour that would spontaneously trigger.
>> We're still talking about deterministic behaviour that's going to be
>> the same whether you do it once in a test lab or a million times in
>> the field. A PWM-controlled backlight that you adjust the brightness
>> of in the lab is going to work the same way as in the field. Any
>> potential glitch would be happening in the lab as well. It would in
>> fact, constantly happen.
> 
> The glitch we're talking about really doesn't happen that often that we
> should rely on this problem popping up during testing. To change both
> period and duty_cycle two register writes are necessary. The hickup only
> happens if after the first register write a period ends before the
> second register write hits the hardware. (For the omap driver it might
> happen a bit more often, don't remember the details, but I think the
> above is what could be reached theoretically.)
> 
>> For the rare cases where you can't fake it, it doesn't really matter
>> whether the framework refuses to do something because it knows that the
>> hardware isn't capable of doing it, or whether the driver will simply
>> try its best to do what it was asked to do and fails to deliver. You'll
>> notice one way or another and then you need to go back to the drawing
>> board and redo the design.
> 
> I think you didn't understand up to now that the glitch depends on
> timing between register access and the PWM period and so is really hard
> to actually reproduce. In the hope you understood that now, I assume you
> see that your argumentation is incomplete.
> 
> And I think to assume that somebody would complain about a race
> condition in a patch that just hit next is quite optimistic.
> 
> Having said that I don't know how critical this really is. Given that
> the PWM under discussion doesn't complete periods on stop, it probably
> isn't.

It is a limitation with the existing driver as well. Nothing is being changed
regarding stopping of PWM. The same is marked under the limitations in the driver.

> 
> I spend some time thinking about when the glitch actually happens.
> Currently the load value is written first and then the match value.
> If no period ends between the two writes there is only a problem when in
> the currently running period the match event didn't happen yet. Then we
> see a cycle with
> 
>    .period = oldperiod + newperiod
>    .dutycycle = oldperiod + newdutycycle
> 
> (if the new match value isn't hit in the current cycle) or one with
> 
>    .period = oldperiod
>    .duty_cycle = newdutycycle + (oldperiod - newperiod)
> 
> (if the new match value is hit in the current cycle). The probability
> that one of the two happen is: olddutycycle / oldperiod which is quite
> probable. (With olddutycycle = oldperiod there is no problem though.)
> 
> If after writing the new load value and before writing the new match
> value a period ends it might happen that we see a cycle with
> 
>   .period = newperiod
>   .dutycycle = olddutycycle + (newperiod - oldperiod)
> 
> (if the previous match value is used) or one with
> 
>   .period = 2 * newperiod
>   .dutycycle = newperiod + newdutycycle
> 
> (if new match value is written too late for the first cycle with the new
> period).


That's exactly why we have marked in the Limitations sections that the current
period might produce a cycle with mixed settings.  Frankly, I'm a bit torn here.
There are other PWMs inside Linux with  similar limitations and documented
similarly. If there is an overall objection for such hardware, the entire policy
should be changed or the framework should be updated to allow user to choose for
dynamic updates. IMHO, this series should not be blocked for this decision.
Please consider it for the coming merge window.

Thanks and regards,
Lokesh

Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello,

On Fri, Apr 03, 2020 at 02:21:38PM +0530, Lokesh Vutla wrote:
> On 02/04/20 7:32 PM, Uwe Kleine-König wrote:
> > Having said that I don't know how critical this really is. Given that
> > the PWM under discussion doesn't complete periods on stop, it probably
> > isn't.
> 
> It is a limitation with the existing driver as well. Nothing is being changed
> regarding stopping of PWM. The same is marked under the limitations in the driver.

What I wrote was ambiguous and I think you understood the meaning I
didn't intend. What I wanted to say is: Given that the PWM stops
abruptly there is only little (if any at all) advantage of
"stop-to-update" over "racy-atomic-update" as we see broken cycles no
matter which alternative we pick.

> > I spend some time thinking about when the glitch actually happens.
> > Currently the load value is written first and then the match value.
> > If no period ends between the two writes there is only a problem when in
> > the currently running period the match event didn't happen yet. Then we
> > see a cycle with
> > 
> >    .period = oldperiod + newperiod
> >    .dutycycle = oldperiod + newdutycycle
> > 
> > (if the new match value isn't hit in the current cycle) or one with
> > 
> >    .period = oldperiod
> >    .duty_cycle = newdutycycle + (oldperiod - newperiod)
> > 
> > (if the new match value is hit in the current cycle). The probability
> > that one of the two happen is: olddutycycle / oldperiod which is quite
> > probable. (With olddutycycle = oldperiod there is no problem though.)
> > 
> > If after writing the new load value and before writing the new match
> > value a period ends it might happen that we see a cycle with
> > 
> >   .period = newperiod
> >   .dutycycle = olddutycycle + (newperiod - oldperiod)
> > 
> > (if the previous match value is used) or one with
> > 
> >   .period = 2 * newperiod
> >   .dutycycle = newperiod + newdutycycle
> > 
> > (if new match value is written too late for the first cycle with the new
> > period).
> 
> That's exactly why we have marked in the Limitations sections that the current
> period might produce a cycle with mixed settings.  Frankly, I'm a bit torn here.
> There are other PWMs inside Linux with  similar limitations and documented
> similarly. If there is an overall objection for such hardware, the entire policy
> should be changed or the framework should be updated to allow user to choose for
> dynamic updates. IMHO, this series should not be blocked for this decision.

Yes, there are other drivers that have similar "problems" and the status
quo is that depending on the driver author one or the other workaround
is chosen. I think the PWM framework would benefit if there were a
common guideline which path to choose in such a situation.

> Please consider it for the coming merge window.

It's already in next, so I assume it will be merged.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |