From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Marco Felsch <m.felsch@pengutronix.de> Cc: thierry.reding@gmail.com, lee.jones@linaro.org, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com, Anson.Huang@nxp.com, michal.vokac@ysoft.com, l.majewski@majess.pl, linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de Subject: Re: [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied Date: Mon, 28 Sep 2020 10:04:25 +0200 [thread overview] Message-ID: <20200928080425.ugyrgznw6o3kwdz5@pengutronix.de> (raw) In-Reply-To: <20200925155330.32301-6-m.felsch@pengutronix.de> [-- Attachment #1: Type: text/plain, Size: 2595 bytes --] On Fri, Sep 25, 2020 at 05:53:30PM +0200, Marco Felsch wrote: > Currently the driver don't check if the new state was applied or not. s/don't/doesn't/ > This can cause glitches on the output pin if the new state disables the > PWM. In this case the PWM clocks are disabled before the new duty cycle > value gets applied. Hmm, the problem that is addressed here is that .apply() might turn off the clock input for the counter before the inactive value is on the pin, right? So an alternative fix would be to not disable the clock, wouldn't it? > The fix is to wait till the desired duty cycle was applied. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > v2: > - new patch > > drivers/pwm/pwm-imx27.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 07c6a263a39c..ffa00bcd81da 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -222,6 +222,26 @@ static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip, > return fifoav; > } > > +static int pwm_imx27_wait_till_applied(struct pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + unsigned int attempts = 4; > + unsigned int period_ms; > + int busy_slots; > + > + do { > + busy_slots = pwm_imx27_get_fifo_slot(chip, pwm); > + if (busy_slots == 0) > + return 0; > + > + period_ms = DIV_ROUND_UP(pwm_get_period(pwm), I was glad you removed the call to pwm_get_state() from .apply(), now it is back in disguised form here :-\ Also the value shouldn't change over the iteration of this loop, so determining it once should be enough. > + NSEC_PER_MSEC); > + msleep(period_ms); > + } while (attempts--); > + > + return -ETIMEDOUT; > +} > + > static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > @@ -277,6 +297,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > else > writel(0, imx->mmio_base + MX3_PWMSAR); > + > + ret = pwm_imx27_wait_till_applied(chip, pwm); > + if (ret) > + goto out; > + The framework doesn't define (and this is a problem there) if .apply is supposed to sleep. OTOH at least sun4i has a similar behaviour. Thierry, what is your thought on this? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Marco Felsch <m.felsch@pengutronix.de> Cc: linux-pwm@vger.kernel.org, michal.vokac@ysoft.com, kernel@pengutronix.de, Anson.Huang@nxp.com, lee.jones@linaro.org, s.hauer@pengutronix.de, thierry.reding@gmail.com, linux-imx@nxp.com, festevam@gmail.com, shawnguo@kernel.org, l.majewski@majess.pl, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied Date: Mon, 28 Sep 2020 10:04:25 +0200 [thread overview] Message-ID: <20200928080425.ugyrgznw6o3kwdz5@pengutronix.de> (raw) In-Reply-To: <20200925155330.32301-6-m.felsch@pengutronix.de> [-- Attachment #1.1: Type: text/plain, Size: 2595 bytes --] On Fri, Sep 25, 2020 at 05:53:30PM +0200, Marco Felsch wrote: > Currently the driver don't check if the new state was applied or not. s/don't/doesn't/ > This can cause glitches on the output pin if the new state disables the > PWM. In this case the PWM clocks are disabled before the new duty cycle > value gets applied. Hmm, the problem that is addressed here is that .apply() might turn off the clock input for the counter before the inactive value is on the pin, right? So an alternative fix would be to not disable the clock, wouldn't it? > The fix is to wait till the desired duty cycle was applied. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > v2: > - new patch > > drivers/pwm/pwm-imx27.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c > index 07c6a263a39c..ffa00bcd81da 100644 > --- a/drivers/pwm/pwm-imx27.c > +++ b/drivers/pwm/pwm-imx27.c > @@ -222,6 +222,26 @@ static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip, > return fifoav; > } > > +static int pwm_imx27_wait_till_applied(struct pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + unsigned int attempts = 4; > + unsigned int period_ms; > + int busy_slots; > + > + do { > + busy_slots = pwm_imx27_get_fifo_slot(chip, pwm); > + if (busy_slots == 0) > + return 0; > + > + period_ms = DIV_ROUND_UP(pwm_get_period(pwm), I was glad you removed the call to pwm_get_state() from .apply(), now it is back in disguised form here :-\ Also the value shouldn't change over the iteration of this loop, so determining it once should be enough. > + NSEC_PER_MSEC); > + msleep(period_ms); > + } while (attempts--); > + > + return -ETIMEDOUT; > +} > + > static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > @@ -277,6 +297,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, > writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); > else > writel(0, imx->mmio_base + MX3_PWMSAR); > + > + ret = pwm_imx27_wait_till_applied(chip, pwm); > + if (ret) > + goto out; > + The framework doesn't define (and this is a problem there) if .apply is supposed to sleep. OTOH at least sun4i has a similar behaviour. Thierry, what is your thought on this? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-28 8:04 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-25 15:53 [PATCH v2 0/5] PWM i.MX27 fix disabled state for inverted signals Marco Felsch 2020-09-25 15:53 ` Marco Felsch 2020-09-25 15:53 ` [PATCH v2 1/5] pwm: imx27: enable clock unconditional for register access Marco Felsch 2020-09-25 15:53 ` Marco Felsch 2020-09-26 13:28 ` Uwe Kleine-König 2020-09-26 13:28 ` Uwe Kleine-König 2020-09-26 13:48 ` Uwe Kleine-König 2020-09-26 13:48 ` Uwe Kleine-König 2020-09-28 5:52 ` Marco Felsch 2020-09-28 5:52 ` Marco Felsch 2020-09-25 15:53 ` [PATCH v2 2/5] pwm: imx27: move constant PWMCR register values into probe Marco Felsch 2020-09-25 15:53 ` Marco Felsch 2020-09-26 13:46 ` Uwe Kleine-König 2020-09-26 13:46 ` Uwe Kleine-König 2020-09-28 5:50 ` Marco Felsch 2020-09-28 5:50 ` Marco Felsch 2020-09-25 15:53 ` [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running Marco Felsch 2020-09-25 15:53 ` Marco Felsch 2020-09-28 7:30 ` Uwe Kleine-König 2020-09-28 7:30 ` Uwe Kleine-König 2020-09-28 9:29 ` Marco Felsch 2020-09-28 9:29 ` Marco Felsch 2020-09-25 15:53 ` [PATCH v2 4/5] pwm: imx27: fix disable state for inverted PWMs Marco Felsch 2020-09-25 15:53 ` Marco Felsch 2020-09-28 7:47 ` Uwe Kleine-König 2020-09-28 7:47 ` Uwe Kleine-König 2020-09-28 9:52 ` Marco Felsch 2020-09-28 9:52 ` Marco Felsch 2020-09-28 19:06 ` Uwe Kleine-König 2020-09-28 19:06 ` Uwe Kleine-König 2020-09-29 5:23 ` Marco Felsch 2020-09-29 5:23 ` Marco Felsch 2020-09-25 15:53 ` [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied Marco Felsch 2020-09-25 15:53 ` Marco Felsch 2020-09-28 8:04 ` Uwe Kleine-König [this message] 2020-09-28 8:04 ` Uwe Kleine-König 2020-09-28 9:59 ` Marco Felsch 2020-09-28 9:59 ` Marco Felsch
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=20200928080425.ugyrgznw6o3kwdz5@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=Anson.Huang@nxp.com \ --cc=festevam@gmail.com \ --cc=kernel@pengutronix.de \ --cc=l.majewski@majess.pl \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-pwm@vger.kernel.org \ --cc=m.felsch@pengutronix.de \ --cc=michal.vokac@ysoft.com \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@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: 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.