[PATCH v2] pwm: pwm-samsung: Trigger manual update when disabling PWM

Subsystems: arm/samsung s3c, s5p and exynos arm architectures, pwm subsystem, the rest

STALE1859d

4 messages, 3 authors, 2021-09-09 · open the first message on its own page

[PATCH v2] pwm: pwm-samsung: Trigger manual update when disabling PWM

From: Mårten Lindahl <hidden>
Date: 2021-09-08 16:00:52 

When duty-cycle is at full level (100%), the TCNTn and TCMPn registers
needs to be flushed in order to disable the signal. The PWM manual does
not say anything about this, but states that only clearing the TCON
auto-reload bit should be needed, and this seems to be true when the PWM
duty-cycle is not at full level. This can be observed on an Axis
ARTPEC-8, by running:

  echo <period> > pwm/period
  echo <period> > pwm/duty_cycle
  echo 1 > pwm/enable
  echo 0 > pwm/enable

Since the TCNTn and TCMPn registers are activated when enabling the PWM
(setting TCON auto-reload bit), and are not touched when disabling the
PWM, the double buffered auto-reload function seems to be still active.
Lowering duty-cycle, and restoring it again in between the enabling and
disabling, makes the disable work since it triggers a reload of the
TCNTn and TCMPn registers.

Fix this by securing a reload of the TCNTn and TCMPn registers when
disabling the PWM and having a full duty-cycle.

Signed-off-by: Mårten Lindahl <redacted>
---

v2:
 - Move fix above setting of disabled_mask

 drivers/pwm/pwm-samsung.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index f6c528f02d43..53edc0da3ff8 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -105,6 +105,9 @@ struct samsung_pwm_chip {
 static DEFINE_SPINLOCK(samsung_pwm_lock);
 #endif
 
+static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm);
+
 static inline
 struct samsung_pwm_chip *to_samsung_pwm_chip(struct pwm_chip *chip)
 {
@@ -276,19 +279,23 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	tcon &= ~TCON_AUTORELOAD(tcon_chan);
 	writel(tcon, our_chip->base + REG_TCON);
 
+	/*
+	 * In case the PWM is at 100% duty cycle, force a manual
+	 * update to prevent the signal from staying high.
+	 */
+	if (readl(our_chip->base + REG_TCMPB(pwm->hwpwm)) == (u32)-1U)
+		__pwm_samsung_manual_update(our_chip, pwm);
+
 	our_chip->disabled_mask |= BIT(pwm->hwpwm);
 
 	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
 }
 
-static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
 				      struct pwm_device *pwm)
 {
 	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
 	u32 tcon;
-	unsigned long flags;
-
-	spin_lock_irqsave(&samsung_pwm_lock, flags);
 
 	tcon = readl(chip->base + REG_TCON);
 	tcon |= TCON_MANUALUPDATE(tcon_chan);
@@ -296,6 +303,16 @@ static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
 
 	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
 	writel(tcon, chip->base + REG_TCON);
+}
+
+static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	__pwm_samsung_manual_update(chip, pwm);
 
 	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
 }
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pwm: pwm-samsung: Trigger manual update when disabling PWM

From: Uwe Kleine-König <hidden>
Date: 2021-09-09 08:07:47 

Hello,

On Wed, Sep 08, 2021 at 05:59:01PM +0200, Mårten Lindahl wrote:
quoted hunk
When duty-cycle is at full level (100%), the TCNTn and TCMPn registers
needs to be flushed in order to disable the signal. The PWM manual does
not say anything about this, but states that only clearing the TCON
auto-reload bit should be needed, and this seems to be true when the PWM
duty-cycle is not at full level. This can be observed on an Axis
ARTPEC-8, by running:

  echo <period> > pwm/period
  echo <period> > pwm/duty_cycle
  echo 1 > pwm/enable
  echo 0 > pwm/enable

Since the TCNTn and TCMPn registers are activated when enabling the PWM
(setting TCON auto-reload bit), and are not touched when disabling the
PWM, the double buffered auto-reload function seems to be still active.
Lowering duty-cycle, and restoring it again in between the enabling and
disabling, makes the disable work since it triggers a reload of the
TCNTn and TCMPn registers.

Fix this by securing a reload of the TCNTn and TCMPn registers when
disabling the PWM and having a full duty-cycle.

Signed-off-by: Mårten Lindahl <redacted>
---

v2:
 - Move fix above setting of disabled_mask

 drivers/pwm/pwm-samsung.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index f6c528f02d43..53edc0da3ff8 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -105,6 +105,9 @@ struct samsung_pwm_chip {
 static DEFINE_SPINLOCK(samsung_pwm_lock);
 #endif
 
+static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm);
+
If you move the definition of __pwm_samsung_manual_update before
pwm_samsung_disable() you can drop this declaration:

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index d904a2480849..b405dd434ad6 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -105,9 +105,6 @@ struct samsung_pwm_chip {
 static DEFINE_SPINLOCK(samsung_pwm_lock);
 #endif
 
-static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
-				      struct pwm_device *pwm);
-
 static inline
 struct samsung_pwm_chip *to_samsung_pwm_chip(struct pwm_chip *chip)
 {
@@ -120,6 +117,32 @@ static inline unsigned int to_tcon_channel(unsigned int channel)
 	return (channel == 0) ? 0 : (channel + 1);
 }
 
+static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm)
+{
+	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
+	u32 tcon;
+
+	tcon = readl(chip->base + REG_TCON);
+	tcon |= TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, chip->base + REG_TCON);
+
+	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, chip->base + REG_TCON);
+}
+
+static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	__pwm_samsung_manual_update(chip, pwm);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
 static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
 				    unsigned int channel, u8 divisor)
 {
@@ -291,32 +314,6 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
 }
 
-static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
-				      struct pwm_device *pwm)
-{
-	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
-	u32 tcon;
-
-	tcon = readl(chip->base + REG_TCON);
-	tcon |= TCON_MANUALUPDATE(tcon_chan);
-	writel(tcon, chip->base + REG_TCON);
-
-	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
-	writel(tcon, chip->base + REG_TCON);
-}
-
-static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
-				      struct pwm_device *pwm)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&samsung_pwm_lock, flags);
-
-	__pwm_samsung_manual_update(chip, pwm);
-
-	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
-}
-
 static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 				int duty_ns, int period_ns, bool force_period)
 {

)

Maybe split the patch to have it nice and reviewable?

Orthogonal to your patch: I wonder what &samsung_pwm_lock actually
protects and why it disables irqs. In general the pwm functions might
sleep, at least some implementations do.

Best regards
Uwe

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

Re: [PATCH v2] pwm: pwm-samsung: Trigger manual update when disabling PWM

From: Marten Lindahl <hidden>
Date: 2021-09-09 09:26:05 

On Thu, Sep 09, 2021 at 10:05:17AM +0200, Uwe Kleine-König wrote:

Hi Uwe!
Thanks for your feedback!

Hello,

On Wed, Sep 08, 2021 at 05:59:01PM +0200, Mårten Lindahl wrote:
quoted
When duty-cycle is at full level (100%), the TCNTn and TCMPn registers
needs to be flushed in order to disable the signal. The PWM manual does
not say anything about this, but states that only clearing the TCON
auto-reload bit should be needed, and this seems to be true when the PWM
duty-cycle is not at full level. This can be observed on an Axis
ARTPEC-8, by running:

  echo <period> > pwm/period
  echo <period> > pwm/duty_cycle
  echo 1 > pwm/enable
  echo 0 > pwm/enable

Since the TCNTn and TCMPn registers are activated when enabling the PWM
(setting TCON auto-reload bit), and are not touched when disabling the
PWM, the double buffered auto-reload function seems to be still active.
Lowering duty-cycle, and restoring it again in between the enabling and
disabling, makes the disable work since it triggers a reload of the
TCNTn and TCMPn registers.

Fix this by securing a reload of the TCNTn and TCMPn registers when
disabling the PWM and having a full duty-cycle.

Signed-off-by: Mårten Lindahl <redacted>
---

v2:
 - Move fix above setting of disabled_mask

 drivers/pwm/pwm-samsung.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index f6c528f02d43..53edc0da3ff8 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -105,6 +105,9 @@ struct samsung_pwm_chip {
 static DEFINE_SPINLOCK(samsung_pwm_lock);
 #endif
 
+static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm);
+
If you move the definition of __pwm_samsung_manual_update before
pwm_samsung_disable() you can drop this declaration:
Yes, I will do that. Thanks.
quoted hunk
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index d904a2480849..b405dd434ad6 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -105,9 +105,6 @@ struct samsung_pwm_chip {
 static DEFINE_SPINLOCK(samsung_pwm_lock);
 #endif
 
-static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
-				      struct pwm_device *pwm);
-
 static inline
 struct samsung_pwm_chip *to_samsung_pwm_chip(struct pwm_chip *chip)
 {
@@ -120,6 +117,32 @@ static inline unsigned int to_tcon_channel(unsigned int channel)
 	return (channel == 0) ? 0 : (channel + 1);
 }
 
+static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm)
+{
+	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
+	u32 tcon;
+
+	tcon = readl(chip->base + REG_TCON);
+	tcon |= TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, chip->base + REG_TCON);
+
+	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, chip->base + REG_TCON);
+}
+
+static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	__pwm_samsung_manual_update(chip, pwm);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
 static void pwm_samsung_set_divisor(struct samsung_pwm_chip *pwm,
 				    unsigned int channel, u8 divisor)
 {
@@ -291,32 +314,6 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
 }
 
-static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
-				      struct pwm_device *pwm)
-{
-	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
-	u32 tcon;
-
-	tcon = readl(chip->base + REG_TCON);
-	tcon |= TCON_MANUALUPDATE(tcon_chan);
-	writel(tcon, chip->base + REG_TCON);
-
-	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
-	writel(tcon, chip->base + REG_TCON);
-}
-
-static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
-				      struct pwm_device *pwm)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&samsung_pwm_lock, flags);
-
-	__pwm_samsung_manual_update(chip, pwm);
-
-	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
-}
-
 static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 				int duty_ns, int period_ns, bool force_period)
 {

)

Maybe split the patch to have it nice and reviewable?
If I only move up the definition of __pwm_samsung_manual_update, and
leave pwm_samsung_manual_update at its place, the patch becomes quite
straightforward and overviewable. Or do you prefer to group the definitions
of those two functions together?

Kind regards
Mårten

Orthogonal to your patch: I wonder what &samsung_pwm_lock actually
protects and why it disables irqs. In general the pwm functions might
sleep, at least some implementations do.

Best regards
Uwe

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] pwm: pwm-samsung: Trigger manual update when disabling PWM

From: Uwe Kleine-König <hidden>
Date: 2021-09-09 09:33:37 

Hello Marten,

On Thu, Sep 09, 2021 at 11:24:10AM +0200, Marten Lindahl wrote:
If I only move up the definition of __pwm_samsung_manual_update, and
leave pwm_samsung_manual_update at its place, the patch becomes quite
straightforward and overviewable. Or do you prefer to group the definitions
of those two functions together?
No strong preference. I found it natural to keep
__pwm_samsung_manual_update and pwm_samsung_manual_update together, but
I wouldn't object your suggestion either.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help