From: Maxime Ripard <maxime.ripard@free-electrons.com> To: Olliver Schinagl <oliver@schinagl.nl> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>, Thierry Reding <thierry.reding@gmail.com>, Chen-Yu Tsai <wens@csie.org>, linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable Date: Sat, 24 Sep 2016 22:25:02 +0200 [thread overview] Message-ID: <20160924202502.GF16901@lukather> (raw) In-Reply-To: <1473411668.731.75.camel@schinagl.nl> [-- Attachment #1: Type: text/plain, Size: 1966 bytes --] Hi Oliver, Sorry for the slow answer. On Fri, Sep 09, 2016 at 11:01:08AM +0200, Olliver Schinagl wrote: > > > > > *chip, struct pwm_device *pwm) > > > > > spin_lock(&sun4i_pwm->ctrl_lock); > > > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > > val &= ~BIT_CH(PWM_EN, pwm->hwpwm); > > > > > + sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG); > > > > > + spin_unlock(&sun4i_pwm->ctrl_lock); > > > > > + > > > > > + /* Allow for the PWM hardware to finish its last > > > > > toggle. > > > > > The pulse > > > > > + * may have just started and thus we should wait a > > > > > full > > > > > period. > > > > > + */ > > > > > + ndelay(pwm_get_period(pwm)); > > > > > > > > Can't that use the ready bit as well? > > > It depends whatever is cheaper. If we disable the pwm, we have to > > > commit that request to hardware first. Then we have to read back > > > the > > > has ready and in the strange situation it is not, wait for it to > > > become > > > ready? > > > > If it works like you were suggesting, yes. > > > > > > > > Also, that would mean we would loop in a spin lock, or keep > > > setting/clearing an additional spinlock to read the ready bit. > > > > You're using a spin_lock, so it's not that bad, but I was just > > suggesting replacing the ndelay. > > If you say the spin_lock + wait for the ready is just as expensive as > the ndelay, or the ndelay is less preferred, then I gladly make the > change; For the spin_lock part, I was just comparing it to a spin_lock_irqsave, which is pretty expensive since it masks all the interrupts in the system, introducing latencies. > but I think we need the ndelay for the else where we do not > have the ready flag (A10 or A13 iirc?) Hmmmm, good point. But that would also apply to your second patch then, wouldn't it? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: maxime.ripard@free-electrons.com (Maxime Ripard) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable Date: Sat, 24 Sep 2016 22:25:02 +0200 [thread overview] Message-ID: <20160924202502.GF16901@lukather> (raw) In-Reply-To: <1473411668.731.75.camel@schinagl.nl> Hi Oliver, Sorry for the slow answer. On Fri, Sep 09, 2016 at 11:01:08AM +0200, Olliver Schinagl wrote: > > > > > *chip, struct pwm_device *pwm) > > > > > ? spin_lock(&sun4i_pwm->ctrl_lock); > > > > > ? val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG); > > > > > ? val &= ~BIT_CH(PWM_EN, pwm->hwpwm); > > > > > + sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG); > > > > > + spin_unlock(&sun4i_pwm->ctrl_lock); > > > > > + > > > > > + /* Allow for the PWM hardware to finish its last > > > > > toggle. > > > > > The pulse > > > > > + ?* may have just started and thus we should wait a > > > > > full > > > > > period. > > > > > + ?*/ > > > > > + ndelay(pwm_get_period(pwm)); > > > > > > > > Can't that use the ready bit as well? > > > It depends whatever is cheaper. If we disable the pwm, we have to > > > commit that request to hardware first. Then we have to read back > > > the > > > has ready and in the strange situation it is not, wait for it to > > > become > > > ready? > > > > If it works like you were suggesting, yes. > > > > > > > > Also, that would mean we would loop in a spin lock, or keep > > > setting/clearing an additional spinlock to read the ready bit. > > > > You're using a spin_lock, so it's not that bad, but I was just > > suggesting replacing the ndelay. > > If you say the spin_lock + wait for the ready is just as expensive as > the ndelay, or the ndelay is less preferred, then I gladly make the > change; For the spin_lock part, I was just comparing it to a spin_lock_irqsave, which is pretty expensive since it masks all the interrupts in the system, introducing latencies. > but I think we need the ndelay for the else where we do not > have the ready flag (A10 or A13 iirc?) Hmmmm, good point. But that would also apply to your second patch then, wouldn't it? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160924/cb63cb8f/attachment.sig>
next prev parent reply other threads:[~2016-09-26 6:31 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-25 17:50 [PATCHv2 0/2] pwm: sunxi: give the pwm IP block more time Olliver Schinagl 2016-08-25 17:50 ` Olliver Schinagl 2016-08-25 17:50 ` [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable Olliver Schinagl 2016-08-25 17:50 ` Olliver Schinagl 2016-08-26 22:19 ` Maxime Ripard 2016-08-26 22:19 ` Maxime Ripard 2016-09-06 7:12 ` Olliver Schinagl 2016-09-06 7:12 ` Olliver Schinagl 2016-09-06 19:51 ` Maxime Ripard 2016-09-06 19:51 ` Maxime Ripard 2016-09-09 9:01 ` Olliver Schinagl 2016-09-09 9:01 ` Olliver Schinagl 2016-09-24 20:25 ` Maxime Ripard [this message] 2016-09-24 20:25 ` Maxime Ripard 2016-09-26 8:46 ` Olliver Schinagl 2016-09-26 8:46 ` Olliver Schinagl 2016-09-27 20:16 ` Maxime Ripard 2016-09-27 20:16 ` Maxime Ripard 2016-12-08 13:23 ` Olliver Schinagl 2016-12-12 12:24 ` Maxime Ripard 2016-12-12 12:24 ` Maxime Ripard 2017-01-03 15:59 ` Olliver Schinagl 2017-01-03 15:59 ` Olliver Schinagl 2017-01-03 16:55 ` Alexandre Belloni 2017-01-03 16:55 ` Alexandre Belloni 2017-01-03 16:55 ` Alexandre Belloni 2017-01-04 6:36 ` Thierry Reding 2017-01-04 6:36 ` Thierry Reding 2017-01-04 6:36 ` Thierry Reding 2016-09-23 14:02 ` [1/2] " Jonathan Liu 2016-09-23 14:02 ` Jonathan Liu 2016-09-23 14:03 ` Olliver Schinagl 2016-09-23 14:03 ` Olliver Schinagl 2017-05-05 1:54 ` Jonathan Liu 2017-05-05 1:54 ` Jonathan Liu 2016-08-25 17:50 ` [PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to become ready Olliver Schinagl 2016-08-25 17:50 ` Olliver Schinagl 2016-08-26 22:25 ` Maxime Ripard 2016-08-26 22:25 ` Maxime Ripard
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=20160924202502.GF16901@lukather \ --to=maxime.ripard@free-electrons.com \ --cc=alexandre.belloni@free-electrons.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=oliver@schinagl.nl \ --cc=thierry.reding@gmail.com \ --cc=wens@csie.org \ /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: linkBe 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.