All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
To: Thierry Reding <thierry.reding@gmail.com>,
	Jingoo Han <jg1.han@samsung.com>,
	Kukjin Kim <kgene.kim@samsung.com>
Cc: linux-pwm@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
	Javier Martinez Canillas <javier.martinez@collabora.co.uk>,
	Lukasz Majewski <l.majewski@samsung.com>
Subject: [PATCH v2 Resend] pwm: samsung: Fix output race on disabling
Date: Thu,  5 Mar 2015 09:14:03 +0100	[thread overview]
Message-ID: <1425543243-6587-1-git-send-email-sjoerd.simons@collabora.co.uk> (raw)

When disabling the samsung PWM the output state remains at the level it
was in the end of a pwm cycle. In other words, calling pwm_disable when
at 100% duty will keep the output active, while at all other setting the
output will go/stay inactive. On top of that the samsung PWM settings are
double-buffered, which means the new settings only get applied at the
start of a new PWM cycle.

This results in a race if the PWM is at 100% duty and a driver calls:
  pwm_config (pwm, 0, period);
  pwm_disable (pwm);

In this case the PWMs output will unexpectedly stay active, unless a new
PWM cycle happened to start between the register writes in _config and
_disable. As far as i can tell this is a regression introduced by 3bdf878,
before that a call to pwm_config would call pwm_samsung_enable which,
while heavy-handed, made sure the expected settings were live.

To resolve this, while not re-introducing the issues 3bdf878 (flickering
as the PWM got reset while in a PWM cycle). Only force an update of the
settings when at 100% duty, which shouldn't have a noticeable effect on
the output but is enough to ensure the behaviour is as expected on
disable.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
Changes since v1:
  Fix small issues pointed out by Tomasz Figa
  - Correct various coding style issues
  - Read the current value of the tcmp register for comparison rather then
    using a non-trivial comparison to decide whether the current state was
    100% duty
  - Move the code to force manual update out into its own function
  - Clarify the comment indicating why a manual update is sometimes required

 drivers/pwm/pwm-samsung.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index 3e9b583..649f6c4 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -269,12 +269,31 @@ 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;
+	unsigned long flags;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	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);
+
+	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)
 {
 	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
 	struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm);
-	u32 tin_ns = chan->tin_ns, tcnt, tcmp;
+	u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp;
 
 	/*
 	 * We currently avoid using 64bit arithmetic by using the
@@ -288,6 +307,7 @@ static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 
 	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
+	oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
 
 	/* We need tick count for calculation, not last tick. */
 	++tcnt;
@@ -335,6 +355,15 @@ static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(tcnt, our_chip->base + REG_TCNTB(pwm->hwpwm));
 	writel(tcmp, our_chip->base + REG_TCMPB(pwm->hwpwm));
 
+	/* In case the PWM is currently at 100% duty, force a manual update
+	 * to prevent the signal staying high in the pwm is disabled shortly
+	 * afer this update (before it autoreloaded the new values) .
+	 */
+	if (oldtcmp == (u32) -1) {
+		dev_dbg(our_chip->chip.dev, "Forcing manual update");
+		pwm_samsung_manual_update(our_chip, pwm);
+	}
+
 	chan->period_ns = period_ns;
 	chan->tin_ns = tin_ns;
 	chan->duty_ns = duty_ns;
-- 
2.1.4


             reply	other threads:[~2015-03-05  8:14 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-05  8:14 Sjoerd Simons [this message]
2015-03-18  7:50 ` [PATCH v2 Resend] pwm: samsung: Fix output race on disabling Sjoerd Simons
2015-03-18  8:08   ` Lukasz Majewski
2015-03-18  8:27     ` Sjoerd Simons
2015-03-19  9:19 ` Javier Martinez Canillas
2015-03-27  7:41 ` Lukasz Majewski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1425543243-6587-1-git-send-email-sjoerd.simons@collabora.co.uk \
    --to=sjoerd.simons@collabora.co.uk \
    --cc=javier.martinez@collabora.co.uk \
    --cc=jg1.han@samsung.com \
    --cc=kgene.kim@samsung.com \
    --cc=l.majewski@samsung.com \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.