From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753421AbbDHI2i (ORCPT ); Wed, 8 Apr 2015 04:28:38 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:33362 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751403AbbDHI22 (ORCPT ); Wed, 8 Apr 2015 04:28:28 -0400 X-AuditID: cbfee61b-f79536d000000f1f-1a-5524e6a9be38 Date: Wed, 08 Apr 2015 10:28:11 +0200 From: Lukasz Majewski To: Anand Moon Cc: Eduardo Valentin , Sjoerd Simons , Russell King , Kukjin Kim , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org Subject: Re: [PATCH 5/6] pwm: samsung: Fix output race on disabling Message-id: <20150408102811.219b964f@amdc2363> In-reply-to: <1427387955-5129-6-git-send-email-linux.amoon@gmail.com> References: <1427387955-5129-1-git-send-email-linux.amoon@gmail.com> <1427387955-5129-6-git-send-email-linux.amoon@gmail.com> Organization: SPRC Poland X-Mailer: Claws Mail 3.8.1 (GTK+ 2.24.10; x86_64-pc-linux-gnu) MIME-version: 1.0 Content-type: text/plain; charset=US-ASCII Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrJLMWRmVeSWpSXmKPExsVy+t9jQd2Vz1RCDbbPlLSYf+Qcq8X8K9dY Lfofv2a22PQYyLq8aw6bxd27qxgtZpzfx2SxbuMtdovbl3ktph9/y+rA5dHS3MPm8ff5dRaP nbPusntsWtXJ5rF5Sb3H501yAWxRXDYpqTmZZalF+nYJXBntV+exFzQrVlzcdoWxgfGgVBcj J4eEgInEkbuvWSBsMYkL99azdTFycQgJTGeUeDjjDytIQkjgDaPE848qXYwcHCwCqhIv3lSA hNkE9CQ+333KBGKLCKhJXHm6ghWkl1ngOJPEqZ/7mEESwgLOEl/3nmAEsXmBGu5P3gAW5xRw lbh6u50NYn69xK6eO2A1/AKSEu3/fjBDHGQnce7TBnaIXkGJH5PvgR3KLKAlsXlbEyuELS+x ec1b5gmMgrOQlM1CUjYLSdkCRuZVjKKpBckFxUnpuUZ6xYm5xaV56XrJ+bmbGMER8kx6B+Oq BotDjAIcjEo8vAKLVUKFWBPLiitzDzFKcDArifCK3gAK8aYkVlalFuXHF5XmpBYfYpTmYFES 552jKxcqJJCeWJKanZpakFoEk2Xi4JRqYIy8xhtxszzMWWBxOkO+94J7j+9O4r9jx3gisWqx rM3c05qFUR+ezHXQ3hRTI3reeHJ3YNKVHveV1lbV3qHW2hNXqv/qefj83c9+XX2f5SWRfafE Dt87znKmdhLblim9P4VZ2kwalPYd1+mzZqld6fNxXdCdnYtSo37z8awM4vkkdm6LSfBXZSWW 4oxEQy3mouJEAIRAptOMAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Anand, > From: Sjoerd Simons > > 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 > Signed-off-by: Anand Moon > --- > 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 Cosmetic comment: Wasn't checkpatch complaining about this comment style? /* ..... * ..... instead of: /* * ..... * ..... > + * 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; Despite the above, Acked-by: Lukasz Majewski -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group From mboxrd@z Thu Jan 1 00:00:00 1970 From: l.majewski@samsung.com (Lukasz Majewski) Date: Wed, 08 Apr 2015 10:28:11 +0200 Subject: [PATCH 5/6] pwm: samsung: Fix output race on disabling In-Reply-To: <1427387955-5129-6-git-send-email-linux.amoon@gmail.com> References: <1427387955-5129-1-git-send-email-linux.amoon@gmail.com> <1427387955-5129-6-git-send-email-linux.amoon@gmail.com> Message-ID: <20150408102811.219b964f@amdc2363> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Anand, > From: Sjoerd Simons > > 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 > Signed-off-by: Anand Moon > --- > 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 Cosmetic comment: Wasn't checkpatch complaining about this comment style? /* ..... * ..... instead of: /* * ..... * ..... > + * 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; Despite the above, Acked-by: Lukasz Majewski -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group