From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Claudiu Beznea <claudiu.beznea@microchip.com>,
Thierry Reding <thierry.reding@gmail.com>,
Lee Jones <lee.jones@linaro.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Ludovic Desroches <ludovic.desroches@microchip.com>,
linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org,
kernel@pengutronix.de
Subject: [PATCH] pwm: atmel: rework tracking updates pending in hardware
Date: Wed, 21 Apr 2021 11:26:08 +0200 [thread overview]
Message-ID: <20210421092606.1634092-1-u.kleine-koenig@pengutronix.de> (raw)
In-Reply-To: <20210420095118.1571344-1-u.kleine-koenig@pengutronix.de>
This improves the driver's behavior in several ways:
- The lock is held for shorter periods and so a channel that is currently
waited for doesn't block disabling another channel.
- It's easier to understand because the procedure is split into more
semantic units and documentation is improved
- A channel is only set to pending when such an event is actually
scheduled in hardware (by writing the CUPD register).
- Also wait in .get_state() to report the last configured state instead
of (maybe) the previous one. This fixes the read back duty cycle and so
prevents a warning being emitted when PWM_DEBUG is on.
Tested on an AriettaG25.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,
I tested this patch on top of the series I sent for this driver
yesterday. (I set In-Reply-To accordingly.)
With these three patches PWM_DEBUG is now happy. (At least I couldn't
trigger a warning any more. I think there are still a few problems with
integer overflows.)
Best regards
Uwe
drivers/pwm/pwm-atmel.c | 101 +++++++++++++++++++++++++++++++---------
1 file changed, 78 insertions(+), 23 deletions(-)
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 29b5ad03f715..38d86340201c 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -84,9 +84,19 @@ struct atmel_pwm_chip {
void __iomem *base;
const struct atmel_pwm_data *data;
- unsigned int updated_pwms;
- /* ISR is cleared when read, ensure only one thread does that */
- struct mutex isr_lock;
+ /*
+ * The hardware supports a mechanism to update a channel's duty cycle at
+ * the end of the currently running period. When such an update is
+ * pending we delay disabling the PWM until the new configuration is
+ * active because otherwise pmw_config(duty_cycle=0); pwm_disable();
+ * might not result in an inactive output.
+ * This bitmask tracks for which channels an update is pending in
+ * hardware.
+ */
+ u32 update_pending;
+
+ /* Protects .update_pending */
+ spinlock_t lock;
};
static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
@@ -123,6 +133,63 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip,
atmel_pwm_writel(chip, base + offset, val);
}
+static void atmel_pwm_update_pending(struct atmel_pwm_chip *chip)
+{
+ /*
+ * Each channel that has its bit in ISR set started a new period since
+ * ISR was cleared and so there is no more update pending. Note that
+ * reading ISR clears it, so this needs to handle all channels to not
+ * loose information.
+ */
+ u32 isr = atmel_pwm_readl(chip, PWM_ISR);
+ chip->update_pending &= ~isr;
+}
+
+static void atmel_pwm_set_pending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+ spin_lock(&chip->lock);
+
+ /*
+ * Clear pending flags in hardware because otherwise there might still
+ * be a stale flag in ISR.
+ */
+ atmel_pwm_update_pending(chip);
+
+ chip->update_pending |= (1 << ch);
+
+ spin_unlock(&chip->lock);
+}
+
+static int atmel_pwm_test_pending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+ int ret = 0;
+
+ spin_lock(&chip->lock);
+
+ if (chip->update_pending & (1 << ch)) {
+ atmel_pwm_update_pending(chip);
+
+ if (chip->update_pending & (1 << ch))
+ ret = 1;
+ }
+
+ spin_unlock(&chip->lock);
+
+ return ret;
+}
+
+static int atmel_pwm_wait_nonpending(struct atmel_pwm_chip *chip, unsigned int ch)
+{
+ unsigned long timeout = jiffies + 2 * HZ;
+ int ret;
+
+ while ((ret = atmel_pwm_test_pending(chip, ch)) &&
+ time_before(jiffies, timeout))
+ usleep_range(10, 100);
+
+ return ret ? -ETIMEDOUT : 0;
+}
+
static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
unsigned long clkrate,
const struct pwm_state *state,
@@ -185,6 +252,7 @@ static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm,
atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm,
atmel_pwm->data->regs.duty_upd, cdty);
+ atmel_pwm_set_pending(atmel_pwm, pwm->hwpwm);
}
static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip,
@@ -205,20 +273,8 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm,
struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
unsigned long timeout = jiffies + 2 * HZ;
- /*
- * Wait for at least a complete period to have passed before disabling a
- * channel to be sure that CDTY has been updated
- */
- mutex_lock(&atmel_pwm->isr_lock);
- atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
-
- while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
- time_before(jiffies, timeout)) {
- usleep_range(10, 100);
- atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
- }
+ atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
- mutex_unlock(&atmel_pwm->isr_lock);
atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
/*
@@ -292,10 +348,6 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
val |= PWM_CMR_CPOL;
atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty);
- mutex_lock(&atmel_pwm->isr_lock);
- atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
- atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
- mutex_unlock(&atmel_pwm->isr_lock);
atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
} else if (cstate.enabled) {
atmel_pwm_disable(chip, pwm, true);
@@ -326,6 +378,9 @@ static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
tmp <<= pres;
state->period = DIV64_U64_ROUND_UP(tmp, rate);
+ /* Wait for an updated duty_cycle queued in hardware */
+ atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
+
cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
atmel_pwm->data->regs.duty);
tmp = (u64)(cprd - cdty) * NSEC_PER_SEC;
@@ -416,9 +471,10 @@ static int atmel_pwm_probe(struct platform_device *pdev)
if (!atmel_pwm)
return -ENOMEM;
- mutex_init(&atmel_pwm->isr_lock);
atmel_pwm->data = of_device_get_match_data(&pdev->dev);
- atmel_pwm->updated_pwms = 0;
+
+ atmel_pwm->update_pending = 0;
+ spin_lock_init(&atmel_pwm->lock);
atmel_pwm->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(atmel_pwm->base))
@@ -462,7 +518,6 @@ static int atmel_pwm_remove(struct platform_device *pdev)
pwmchip_remove(&atmel_pwm->chip);
clk_unprepare(atmel_pwm->clk);
- mutex_destroy(&atmel_pwm->isr_lock);
return 0;
}
--
2.30.2
next prev parent reply other threads:[~2021-04-21 9:27 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-20 9:51 [PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state() Uwe Kleine-König
2021-04-20 9:51 ` [PATCH 2/2] pwm: atmel: Improve duty cycle calculation in .apply() Uwe Kleine-König
2021-04-23 17:07 ` Thierry Reding
2021-04-21 9:26 ` Uwe Kleine-König [this message]
2021-04-21 11:03 ` overflow and wrong timeout errors in pwm-atmel Uwe Kleine-König
2021-04-21 13:48 ` Uwe Kleine-König
2021-04-21 14:18 ` Alexandre Belloni
2021-04-21 15:26 ` Uwe Kleine-König
2021-07-05 7:55 ` [PATCH] pwm: atmel: rework tracking updates pending in hardware Uwe Kleine-König
2021-04-23 17:07 ` [PATCH 1/2] pwm: atmel: Fix duty cycle calculation in .get_state() Thierry Reding
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=20210421092606.1634092-1-u.kleine-koenig@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=alexandre.belloni@bootlin.com \
--cc=claudiu.beznea@microchip.com \
--cc=kernel@pengutronix.de \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-pwm@vger.kernel.org \
--cc=ludovic.desroches@microchip.com \
--cc=nicolas.ferre@microchip.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).