From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1B0D9C433EF for ; Thu, 9 Sep 2021 09:26:05 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CF07661131 for ; Thu, 9 Sep 2021 09:26:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org CF07661131 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=axis.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:From:In-Reply-To:MIME-Version: References:Message-ID:Subject:CC:To:Date:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=wgl7w0pyE0jkYnfjYaFTlrhT7VcT82rBX3w72+0NkTo=; b=0T11+8ViU2wDThOn3qtL34d625 LAFWTJ0ZAZVLSNXp+r+cjuj2HJrtH3cq0MOL+1/Wlamd4AxfVTttBft2hby0Vc/PTf3mLzvVf0HGI L1nvS9SnyBCJ3kupb4At6CqsPMQHSgXawhpNfpipkNNQdXFJIRi4rPdSitx6qVL2L30Ju9PnDpksz aP5EBDf7oqyIcI6ctuAZDJfW77lfJcETezl6llzZWmFbcBEOyazjEGjcw81eC5f22yQfY8D0fE26G LWKsYt1vSJ77uG1pQFFxZoY7D3hGwbnBiJ0aCw5qQW+/3pEoOEpqqjwll1azAcH2XaWsQG5dM0lka 2dZRDugw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mOGI6-008lxr-38; Thu, 09 Sep 2021 09:24:18 +0000 Received: from smtp1.axis.com ([195.60.68.17]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mOGI1-008lwV-5G for linux-arm-kernel@lists.infradead.org; Thu, 09 Sep 2021 09:24:15 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axis.com; q=dns/txt; s=axis-central1; t=1631179453; x=1662715453; h=date:to:cc:subject:message-id:references:mime-version: content-transfer-encoding:in-reply-to:from; bh=8yKQxOzbJX3oMJUiB0rw/AtRDsAlIREmsFIhtiMgZ7w=; b=BFadNIZ/IwM49J1cGmx99vVJ8vM4CGv18RDLyrhpkj35hKHwh/z8VrF6 cyQDKW6M0cRbOgZvM6m58DXGOerE7F1YvV0KW+viZ53SYEmtDdjEvz2Xo JIbC8N4iyxh7wqg+q7YH24jEF1fvMtM9KBV/Ne1i8ZCwEkkeEFQ9J+bHd IDZndn70V4Ld2dSoTTkpfG3gv5WExH60FKu4UzbvSdPSt9eRGgwsw7iZr fPlPqkB6b/ZkZg6AlojimyBqenx+zMWfWR5JEJY8xDaR7KxYAUNRctNpc vxsRadU36NctV2Ali1ZESr07JW6mpEV564xW7nFhDUmE2RSTPyAM77c6u A==; Date: Thu, 9 Sep 2021 11:24:10 +0200 To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= CC: =?iso-8859-1?Q?M=E5rten?= Lindahl , Krzysztof Kozlowski , Thierry Reding , Lee Jones , , , , Subject: Re: [PATCH v2] pwm: pwm-samsung: Trigger manual update when disabling PWM Message-ID: <20210909092409.GA11367@axis.com> References: <20210908155901.18944-1-marten.lindahl@axis.com> <20210909080517.rsrohvdqqcnnjv2x@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210909080517.rsrohvdqqcnnjv2x@pengutronix.de> User-Agent: Mutt/1.10.1 (2018-07-13) From: Marten Lindahl X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210909_022413_821071_9E02C479 X-CRM114-Status: GOOD ( 33.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 09, 2021 at 10:05:17AM +0200, Uwe Kleine-K=F6nig wrote: Hi Uwe! Thanks for your feedback! > Hello, > = > On Wed, Sep 08, 2021 at 05:59:01PM +0200, M=E5rten 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 > pwm/period > > echo > 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=E5rten Lindahl > > --- > > = > > 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. > 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 =3D=3D 0) ? 0 : (channel + 1); > } > = > +static void __pwm_samsung_manual_update(struct samsung_pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + unsigned int tcon_chan =3D to_tcon_channel(pwm->hwpwm); > + u32 tcon; > + > + tcon =3D readl(chip->base + REG_TCON); > + tcon |=3D TCON_MANUALUPDATE(tcon_chan); > + writel(tcon, chip->base + REG_TCON); > + > + tcon &=3D ~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 *chi= p, 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 =3D to_tcon_channel(pwm->hwpwm); > - u32 tcon; > - > - tcon =3D readl(chip->base + REG_TCON); > - tcon |=3D TCON_MANUALUPDATE(tcon_chan); > - writel(tcon, chip->base + REG_TCON); > - > - tcon &=3D ~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=E5rten > = > 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=F6nig = | > 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