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

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

[Mårten Lindahl]

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 <marten.lindahl@axis.com>
---
 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..7d90652402a0 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)
 {
@@ -278,17 +281,21 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	our_chip->disabled_mask |= BIT(pwm->hwpwm);
 
+	/*
+	 * 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);
+
 	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

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

[Mårten Lindahl]

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 <marten.lindahl@axis.com>
---
 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..7d90652402a0 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)
 {
@@ -278,17 +281,21 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	our_chip->disabled_mask |= BIT(pwm->hwpwm);
 
+	/*
+	 * 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);
+
 	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] pwm: pwm-samsung: Trigger manual update when disabling PWM

[Krzysztof Kozlowski]

On 08/09/2021 16:27, Mårten Lindahl wrote:
> 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 <marten.lindahl@axis.com>
> ---
>  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..7d90652402a0 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)
>  {
> @@ -278,17 +281,21 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	our_chip->disabled_mask |= BIT(pwm->hwpwm);
>  
> +	/*
> +	 * 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);

I did not test it but looks reasonable. Just please move it above
setting "our_chip->disabled_mask", so the code will be close to
pwm_samsung_enable.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 08/09/2021 16:27, Mårten Lindahl wrote:
> 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 <marten.lindahl@axis.com>
> ---
>  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..7d90652402a0 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)
>  {
> @@ -278,17 +281,21 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	our_chip->disabled_mask |= BIT(pwm->hwpwm);
>  
> +	/*
> +	 * 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);

I did not test it but looks reasonable. Just please move it above
setting "our_chip->disabled_mask", so the code will be close to
pwm_samsung_enable.


Best regards,
Krzysztof

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

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

[Marten Lindahl]

On Wed, Sep 08, 2021 at 04:54:04PM +0200, Krzysztof Kozlowski wrote:
> On 08/09/2021 16:27, Mårten Lindahl wrote:
> > 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 <marten.lindahl@axis.com>
> > ---
> >  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..7d90652402a0 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)
> >  {
> > @@ -278,17 +281,21 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  
> >  	our_chip->disabled_mask |= BIT(pwm->hwpwm);
> >  
> > +	/*
> > +	 * 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);
> 
> I did not test it but looks reasonable. Just please move it above
> setting "our_chip->disabled_mask", so the code will be close to
> pwm_samsung_enable.
> 

Hi Krzysztof!

Thank you for the quick reply! I will do so.

Kind regards
Mårten

> 
> Best regards,
> Krzysztof

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

[Marten Lindahl]

On Wed, Sep 08, 2021 at 04:54:04PM +0200, Krzysztof Kozlowski wrote:
> On 08/09/2021 16:27, Mårten Lindahl wrote:
> > 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 <marten.lindahl@axis.com>
> > ---
> >  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..7d90652402a0 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)
> >  {
> > @@ -278,17 +281,21 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  
> >  	our_chip->disabled_mask |= BIT(pwm->hwpwm);
> >  
> > +	/*
> > +	 * 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);
> 
> I did not test it but looks reasonable. Just please move it above
> setting "our_chip->disabled_mask", so the code will be close to
> pwm_samsung_enable.
> 

Hi Krzysztof!

Thank you for the quick reply! I will do so.

Kind regards
Mårten

> 
> Best regards,
> Krzysztof

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