[PATCH v3] clocksource: sh_cmt: fixup for 64-bit machines
When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
/proc/timer_list. It turned out that when calculating it, the driver did
1 << 32 (causing what I think was undefined behavior) resulting in a zero
delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
to be that the driver abused *unsigned long* for the CMT register values
(which are 16/32-bit), so that the calculation of 'ch->max_match_value'
in sh_cmt_setup_channel() used the wrong branch. Using more proper 'u32'
instead fixed 'max_delta_ns' and even fixed the switching an active
clocksource to CMT (which caused the system to turn non-interactive
before).
Signed-off-by: Sergei Shtylyov
---
This patch is against the 'tip.git' repo's 'timers/core' branch.
The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
enabling building of this driver now, so not sure how urgent is this...
Changes in version 3:
- made the 'overflow_bit' and 'clear_bits' members of 'struct sh_cmt_info'
'u32';
- made the 2nd parameter of sh_cmt_write_cmstr() 'u32';
- made the result, the 2nd parameter, and 'o{1|2}' local variables of
sh_cmt_get_counter() 'u32';
- made the 'has_wrapped' local variables 'u32' in sh_cmt_clocksource_read()
and sh_cmt_clock_event_program_verify();
- fixed a typo in the patch description.
Changes in version 2:
- completely redid the patch, fixing abuse of *unsigned long* for the CMT
register values.
drivers/clocksource/sh_cmt.c | 72 +++
1 file changed, 33 insertions(+), 39 deletions(-)
Index: tip/drivers/clocksource/sh_cmt.c
===
--- tip.orig/drivers/clocksource/sh_cmt.c
+++ tip/drivers/clocksource/sh_cmt.c
@@ -78,18 +78,17 @@ struct sh_cmt_info {
unsigned int channels_mask;
unsigned long width; /* 16 or 32 bit version of hardware block */
- unsigned long overflow_bit;
- unsigned long clear_bits;
+ u32 overflow_bit;
+ u32 clear_bits;
/* callbacks for CMSTR and CMCSR access */
- unsigned long (*read_control)(void __iomem *base, unsigned long offs);
+ u32 (*read_control)(void __iomem *base, unsigned long offs);
void (*write_control)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 value);
/* callbacks for CMCNT and CMCOR access */
- unsigned long (*read_count)(void __iomem *base, unsigned long offs);
- void (*write_count)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 (*read_count)(void __iomem *base, unsigned long offs);
+ void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
};
struct sh_cmt_channel {
@@ -103,9 +102,9 @@ struct sh_cmt_channel {
unsigned int timer_bit;
unsigned long flags;
- unsigned long match_value;
- unsigned long next_match_value;
- unsigned long max_match_value;
+ u32 match_value;
+ u32 next_match_value;
+ u32 max_match_value;
raw_spinlock_t lock;
struct clock_event_device ced;
struct clocksource cs;
@@ -160,24 +159,22 @@ struct sh_cmt_device {
#define SH_CMT32_CMCSR_CKS_RCLK1 (7 << 0)
#define SH_CMT32_CMCSR_CKS_MASK(7 << 0)
-static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read16(void __iomem *base, unsigned long offs)
{
return ioread16(base + (offs << 1));
}
-static unsigned long sh_cmt_read32(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read32(void __iomem *base, unsigned long offs)
{
return ioread32(base + (offs << 2));
}
-static void sh_cmt_write16(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write16(void __iomem *base, unsigned long offs, u32 value)
{
iowrite16(value, base + (offs << 1));
}
-static void sh_cmt_write32(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write32(void __iomem *base, unsigned long offs, u32 value)
{
iowrite32(value, base + (offs << 2));
}
@@ -242,7 +239,7 @@ static const struct sh_cmt_info sh_cmt_i
#define CMCNT 1 /* channel register */
#define CMCOR 2 /* channel register */
-static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
{
if (ch->iostart)
return ch->cmt->info->read_control(ch->iostart, 0);
@@ -250,8 +247,7 @@ static inline unsigned long sh_cmt_read_
return ch->cmt->info->read_control(ch->cmt->mapbase, 0);
}
-static inline void sh_cmt_write_cmstr(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void
Re: [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
Hello!
On 09/06/2018 03:18 PM, Geert Uytterhoeven wrote:
>> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
>> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
>> /proc/timer_list. It turned out that when calculating it, the driver did
>> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
>> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
>> to be that the driver abused *unsigned long* for the CMT register values
>> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
>> in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
>> instead fixed 'max_delta_ns' and even fixed the switching an active
>> clocksource to CMT (which caused the system to turn non-interactive
>> before).
>>
>> Signed-off-by: Sergei Shtylyov
>>
>> ---
>> This patch is against the 'tip.git' repo's 'timers/core' branch.
>> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
>> enabling building of this driver now, so not sure how urgent is this...
>>
>> Changes in version 2:
>> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
>> register values.
>
> Thanks for the update!
>
>> --- tip.orig/drivers/clocksource/sh_cmt.c
>> +++ tip/drivers/clocksource/sh_cmt.c
>> @@ -82,14 +82,13 @@ struct sh_cmt_info {
>> unsigned long clear_bits;
>
> "clear_bits" should be u32, as it corresponds to a register value.
OK.
>> @@ -103,9 +102,9 @@ struct sh_cmt_channel {
>>
>> unsigned int timer_bit;
>> unsigned long flags;
>
> While no register value, "flags" can be unsigned int, too.
OK to leave that for another patch?
>> - unsigned long match_value;
>> - unsigned long next_match_value;
>> - unsigned long max_match_value;
>> + u32 match_value;
>> + u32 next_match_value;
>> + u32 max_match_value;
>> raw_spinlock_t lock;
>> struct clock_event_device ced;
>> struct clocksource cs;
>
>> @@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
>> static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
>
> Return type should be "u32".
Thanks, missed it...
>> int *has_wrapped)
>
> While not "unsigned long", "has_wrapped" is used to store a register value,
> so I think it should be changed to "u32 *".
Hm, indeed...
>> {
>> - unsigned long v1, v2, v3;
>> + u32 v1, v2, v3;
>> int o1, o2;
>
> While not "unsigned long", "o1" and "o2" contain register values, so I think
> they should be "u32".
OK, lemme try that...
>> o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
>
> "overflow_bit" should become "u32", too.
I've figured. :-)
>
>> @@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
>> static u64 sh_cmt_clocksource_read(struct clocksource *cs)
>> {
>> struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
>> - unsigned long flags, raw;
>> + unsigned long flags;
>> unsigned long value;
>> int has_wrapped;
>> + u32 raw;
>>
>> raw_spin_lock_irqsave(&ch->lock, flags);
>> value = ch->total_cycles;
>
> "value" and "total_cycles" should be "u64". But that's a separate bug fix.
And for 32-bit CPUs, I guess? Will look into it...
> Not in the context, so I cannot comment to it, but "sh_cmt_info.width" should
> be "unsigned int", too.
Separate patch?
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
Re: watchdog: renesas_wdt: convert to SPDX identifiers
On Fri, Sep 07, 2018 at 02:10:58AM +, Kuninori Morimoto wrote: > From: Kuninori Morimoto > > This patch updates license to use SPDX-License-Identifier > instead of verbose license text. > > Signed-off-by: Kuninori Morimoto > Reviewed-by: Guenter Roeck Hmmm .. any chance for you folks to synchronize with each other ? I already have "watchdog: use SPDX identifier for Renesas drivers", submitted by Wolfram Sang a couple of weeks ago. Guenter > --- > drivers/watchdog/renesas_wdt.c | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c > index 88d81fe..84bb9d3 100644 > --- a/drivers/watchdog/renesas_wdt.c > +++ b/drivers/watchdog/renesas_wdt.c > @@ -1,12 +1,9 @@ > +// SPDX-License-Identifier: GPL-2.0 > /* > * Watchdog driver for Renesas WDT watchdog > * > * Copyright (C) 2015-17 Wolfram Sang, Sang Engineering > > * Copyright (C) 2015-17 Renesas Electronics Corporation > - * > - * This program is free software; you can redistribute it and/or modify it > - * under the terms of the GNU General Public License version 2 as published > by > - * the Free Software Foundation. > */ > #include > #include
Re: [PATCH v3 1/2] watchdog: rza_wdt: Support longer timeouts
On 09/06/2018 06:22 PM, Chris Brandt wrote:
The RZ/A2 watchdog timer extends the clock source options in order to
allow for longer timeouts.
Signed-off-by: Chris Brandt
---
v3:
* Removed + 1 from DIV_ROUND_UP line
* resetting to 0 if time to big did not make as much sense are resetting
to 256
v2:
* use DIV_ROUND_UP
* use %u for pr_debug
* use of_match data to determine the size of CKS register
---
drivers/watchdog/rza_wdt.c | 81 +++---
1 file changed, 63 insertions(+), 18 deletions(-)
diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c
index e618218d2374..64a733492b96 100644
--- a/drivers/watchdog/rza_wdt.c
+++ b/drivers/watchdog/rza_wdt.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
#include
@@ -34,12 +35,40 @@
#define WRCSR_RSTEBIT(6)
#define WRCSR_CLEAR_WOVF 0xA500 /* special value */
+#define CKS_3BIT 0x7
+#define CKS_4BIT 0xF
+
Any special reason for the value of those defines ? They are just used as flags,
or am I missing something ? Why not just use 0 / 1 or an enum ?
struct rza_wdt {
struct watchdog_device wdev;
void __iomem *base;
struct clk *clk;
+ u8 count;
+ u8 cks;
+ u8 timeout;
Hmm ... this limits the effective timeout to 255 seconds. That seems odd.
Maybe it is true in practice, if the clock is always guaranteed to be
above 4194304 Hz, but it is an odd assumption that isn't really reflected
in the code.
};
+static void rza_wdt_calc_timeout(struct rza_wdt *priv, int timeout)
+{
+ unsigned long rate = clk_get_rate(priv->clk);
+ unsigned int ticks;
+
+ if (priv->cks == CKS_4BIT) {
+ ticks = DIV_ROUND_UP((timeout * rate), 4194304);
The ( ) around timeout * rate is unnecessary. Also, it would be nice
to have a define and explanation for 4194304 (and 0x40 would probably
be a better value to use).
+ if (ticks > 256)
+ ticks = 256;
If you keep this, you should as well recalculate timeout since it won't
match the expected value.
if (ticks > 256) {
ticks = 256;
timeout = ticks * 4194304 / rate;
}
Not that it can ever happen, since max_timeout limits the value.
Personally I would rather see this dropped with a comment stating that
ticks <= 256 is guaranteed by max_timeout; I am not a friend of dead code
in the kernel.
+
+ priv->count = 256 - ticks;
+ } else {
+ /* Start timer with longest timeout */
+ priv->count = 0;
+ }
+
+ priv->timeout = timeout;
+
+ pr_debug("%s: timeout set to %u (WTCNT=%d)\n", __func__,
+timeout, priv->count);
+}
+
static int rza_wdt_start(struct watchdog_device *wdev)
{
struct rza_wdt *priv = watchdog_get_drvdata(wdev);
@@ -51,13 +80,12 @@ static int rza_wdt_start(struct watchdog_device *wdev)
readb(priv->base + WRCSR);
writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR);
- /*
-* Start timer with slowest clock source and reset option enabled.
-*/
+ rza_wdt_calc_timeout(priv, wdev->timeout);
+
writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR);
- writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
- writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7),
- priv->base + WTCSR);
+ writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
+ writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME |
+ WTSCR_CKS(priv->cks), priv->base + WTCSR);
return 0;
}
@@ -75,7 +103,12 @@ static int rza_wdt_ping(struct watchdog_device *wdev)
{
struct rza_wdt *priv = watchdog_get_drvdata(wdev);
- writew(WTCNT_MAGIC | 0, priv->base + WTCNT);
+ if (priv->timeout != wdev->timeout)
+ rza_wdt_calc_timeout(priv, wdev->timeout);
+
FWIW, odd way of updating the timeout. Why not do it in the set_timeout()
function where it belongs. Which makes me wonder why priv->timeout is needed
in the first place (and why it is u8 - as mentioned above).
+ writew(WTCNT_MAGIC | priv->count, priv->base + WTCNT);
+
+ pr_debug("%s: timeout = %u\n", __func__, wdev->timeout);
Do you really want this displayed with each ping, even as debug message ?
Just wondering.
return 0;
}
@@ -150,20 +183,31 @@ static int rza_wdt_probe(struct platform_device *pdev)
return -ENOENT;
}
- /* Assume slowest clock rate possible (CKS=7) */
- rate /= 16384;
-
priv->wdev.info = &rza_wdt_ident,
priv->wdev.ops = &rza_wdt_ops,
priv->wdev.parent = &pdev->dev;
- /*
-* Since the max possible timeout of our 8-bit count register is less
-* than a second, we must use max_hw_heartbeat_ms.
-*/
- priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX) / rate;
- dev_dbg
