[RFC PATCH] clocksource/drivers/sh_cmt: access registers according to spec

* [RFC PATCH] clocksource/drivers/sh_cmt: access registers according to spec
@ 2022-11-16 20:21 Wolfram Sang
  2022-11-16 21:10 ` Wolfram Sang
  2022-11-17  8:45 ` Geert Uytterhoeven
  0 siblings, 2 replies; 7+ messages in thread
From: Wolfram Sang @ 2022-11-16 20:21 UTC (permalink / raw)
  To: linux-renesas-soc
  Cc: Geert Uytterhoeven, Laurent Pinchart, Yoshihiro Shimoda, Wolfram Sang

Documentation for most CMTs say that we need to wait two input clocks
before changes propagate to the timer. This is especially relevant when
we stop the timer to change further settings. Implement the delays
according to the spec. To avoid unnecessary delays in atomic mode, we
also check if the to-be-written value actually differs. CMCNT is a bit
special because testing showed that we need to wait 3 cycles instead.
AFAIU, this is also true for all CMTs. Also, the WRFLAG needs to be
checked before writing. This fixes "cannot clear CMCNT" messages which
occur often on R-Car Gen4 SoCs, but only very rarely on older SoCs for
some reason.

Fixes: 81b3b2711072 ("clocksource: sh_cmt: Add support for multiple channels per device")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

With this patch, I can run the 'clocksource-switch' test (from the Linux
selftests) without any warnings printed on the Spider S4 and the Ebisu
E3 board. Both printed the warnings before, the Spider immediately, the
Ebisu rarely but still. The price for this correctness is that the tests
run much longer due to the udelays in atomic mode. However, I consider
the massive switching a corner case. Usually, one switches rarely so the
extra delay is worth the correctness IMHO.

The only question left for me: Do the old 16 and 32 bit versions of CMT
really *not* need the delay? They are not explicitly mentioned in the
docs but maybe it doesn't hurt?

For the record: it took a while to find out what exactly was the problem
for the error message. And then, it was also two different delays
needed. This is why I chose to implement all the delays mentioned in the
specs and not only a working subset. The latter sounded too fragile in
case register accesses would change in the future.

So much for now, looking forward to comments!

   Wolfram

 drivers/clocksource/sh_cmt.c | 77 ++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 30 deletions(-)

diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
index 64dcb082d4cf..474552be26e7 100644
--- a/drivers/clocksource/sh_cmt.c
+++ b/drivers/clocksource/sh_cmt.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/ioport.h>
 #include <linux/irq.h>
 #include <linux/module.h>
@@ -116,6 +117,7 @@ struct sh_cmt_device {
 	void __iomem *mapbase;
 	struct clk *clk;
 	unsigned long rate;
+	unsigned long reg_delay;
 
 	raw_spinlock_t lock; /* Protect the shared start/stop register */
 
@@ -247,10 +249,17 @@ static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
 
 static inline void sh_cmt_write_cmstr(struct sh_cmt_channel *ch, u32 value)
 {
-	if (ch->iostart)
-		ch->cmt->info->write_control(ch->iostart, 0, value);
-	else
-		ch->cmt->info->write_control(ch->cmt->mapbase, 0, value);
+	u32 old_value = sh_cmt_read_cmstr(ch);
+
+	if (value != old_value) {
+		if (ch->iostart) {
+			ch->cmt->info->write_control(ch->iostart, 0, value);
+			udelay(ch->cmt->reg_delay);
+		} else {
+			ch->cmt->info->write_control(ch->cmt->mapbase, 0, value);
+			udelay(ch->cmt->reg_delay);
+		}
+	}
 }
 
 static inline u32 sh_cmt_read_cmcsr(struct sh_cmt_channel *ch)
@@ -260,7 +269,12 @@ static inline u32 sh_cmt_read_cmcsr(struct sh_cmt_channel *ch)
 
 static inline void sh_cmt_write_cmcsr(struct sh_cmt_channel *ch, u32 value)
 {
-	ch->cmt->info->write_control(ch->ioctrl, CMCSR, value);
+	u32 old_value = sh_cmt_read_cmcsr(ch);
+
+	if (value != old_value) {
+		ch->cmt->info->write_control(ch->ioctrl, CMCSR, value);
+		udelay(ch->cmt->reg_delay);
+	}
 }
 
 static inline u32 sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
@@ -270,12 +284,26 @@ static inline u32 sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
 
 static inline void sh_cmt_write_cmcnt(struct sh_cmt_channel *ch, u32 value)
 {
+	/* Tests showed that we need to wait 3 clocks here */
+	unsigned long cmcnt_delay = ch->cmt->reg_delay * 3 / 2;
+	u32 reg;
+
+	if (ch->cmt->info->model > SH_CMT_16BIT)
+		read_poll_timeout_atomic(sh_cmt_read_cmcsr, reg, !(reg & SH_CMT32_CMCSR_WRFLG),
+					 1, cmcnt_delay, false, ch);
+
 	ch->cmt->info->write_count(ch->ioctrl, CMCNT, value);
+	udelay(cmcnt_delay);
 }
 
 static inline void sh_cmt_write_cmcor(struct sh_cmt_channel *ch, u32 value)
 {
-	ch->cmt->info->write_count(ch->ioctrl, CMCOR, value);
+	u32 old_value = ch->cmt->info->read_count(ch->ioctrl, CMCOR);
+
+	if (value != old_value) {
+		ch->cmt->info->write_count(ch->ioctrl, CMCOR, value);
+		udelay(ch->cmt->reg_delay);
+	}
 }
 
 static u32 sh_cmt_get_counter(struct sh_cmt_channel *ch, u32 *has_wrapped)
@@ -319,7 +347,7 @@ static void sh_cmt_start_stop_ch(struct sh_cmt_channel *ch, int start)
 
 static int sh_cmt_enable(struct sh_cmt_channel *ch)
 {
-	int k, ret;
+	int ret;
 
 	dev_pm_syscore_device(&ch->cmt->pdev->dev, true);
 
@@ -349,23 +377,6 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
 	sh_cmt_write_cmcor(ch, 0xffffffff);
 	sh_cmt_write_cmcnt(ch, 0);
 
-	/*
-	 * According to the sh73a0 user's manual, as CMCNT can be operated
-	 * only by the RCLK (Pseudo 32 kHz), there's one restriction on
-	 * modifying CMCNT register; two RCLK cycles are necessary before
-	 * this register is either read or any modification of the value
-	 * it holds is reflected in the LSI's actual operation.
-	 *
-	 * While at it, we're supposed to clear out the CMCNT as of this
-	 * moment, so make sure it's processed properly here.  This will
-	 * take RCLKx2 at maximum.
-	 */
-	for (k = 0; k < 100; k++) {
-		if (!sh_cmt_read_cmcnt(ch))
-			break;
-		udelay(1);
-	}
-
 	if (sh_cmt_read_cmcnt(ch)) {
 		dev_err(&ch->cmt->pdev->dev, "ch%u: cannot clear CMCNT\n",
 			ch->index);
@@ -995,8 +1006,8 @@ MODULE_DEVICE_TABLE(of, sh_cmt_of_table);
 
 static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev)
 {
-	unsigned int mask;
-	unsigned int i;
+	unsigned int mask, i;
+	unsigned long rate;
 	int ret;
 
 	cmt->pdev = pdev;
@@ -1032,10 +1043,16 @@ static int sh_cmt_setup(struct sh_cmt_device *cmt, struct platform_device *pdev)
 	if (ret < 0)
 		goto err_clk_unprepare;
 
-	if (cmt->info->width == 16)
-		cmt->rate = clk_get_rate(cmt->clk) / 512;
-	else
-		cmt->rate = clk_get_rate(cmt->clk) / 8;
+	rate = clk_get_rate(cmt->clk);
+	if (!rate) {
+		ret = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	/* We shall wait 2 input clks after register writes */
+	if (cmt->info->model >= SH_CMT_48BIT) // FIXME: Really not needed for older ones?
+		cmt->reg_delay = 2000000UL / rate + 1;
+	cmt->rate = cmt->info->width == 16 ? rate / 512 : rate / 8;
 
 	/* Map the memory resource(s). */
 	ret = sh_cmt_map_memory(cmt);
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread