Hello,
On Wed, Sep 08, 2021 at 05:59:01PM +0200, 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@xxxxxxxx>
> ---
>
> 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/ |
Attachment:
signature.asc
Description: PGP signature
