2018-02-28 10:54:58

by Jayachandran C

[permalink] [raw]
Subject: [PATCH] watchdog: sbsa: use 32-bit read for WCV

According to SBSA spec v3.1 section 5.3:
All registers are 32 bits in size and should be accessed using
32-bit reads and writes. If an access size other than 32 bits
is used then the results are IMPLEMENTATION DEFINED.
[...]
The Generic Watchdog is little-endian

The current code uses readq to read the watchdog compare register
which does a 64-bit access. This fails on ThunderX2 which does not
implement 64-bit access to this register.

Fix this by using lo_hi_readq() that does two 32-bit reads.

Signed-off-by: Jayachandran C <[email protected]>
---
drivers/watchdog/sbsa_gwdt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index 316c2eb..e8bd988 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -50,6 +50,7 @@
*/

#include <linux/io.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
@@ -159,7 +160,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
!(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);

- timeleft += readq(gwdt->control_base + SBSA_GWDT_WCV) -
+ timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
arch_counter_get_cntvct();

do_div(timeleft, gwdt->clk);
--
2.7.4



2018-02-28 14:36:40

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH] watchdog: sbsa: use 32-bit read for WCV

On 02/28/2018 02:52 AM, Jayachandran C wrote:
> According to SBSA spec v3.1 section 5.3:
> All registers are 32 bits in size and should be accessed using
> 32-bit reads and writes. If an access size other than 32 bits
> is used then the results are IMPLEMENTATION DEFINED.
> [...]
> The Generic Watchdog is little-endian
>
> The current code uses readq to read the watchdog compare register
> which does a 64-bit access. This fails on ThunderX2 which does not
> implement 64-bit access to this register.
>
> Fix this by using lo_hi_readq() that does two 32-bit reads.
>
> Signed-off-by: Jayachandran C <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/watchdog/sbsa_gwdt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index 316c2eb..e8bd988 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -50,6 +50,7 @@
> */
>
> #include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> #include <linux/interrupt.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> @@ -159,7 +160,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
> !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
> timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
>
> - timeleft += readq(gwdt->control_base + SBSA_GWDT_WCV) -
> + timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
> arch_counter_get_cntvct();
>
> do_div(timeleft, gwdt->clk);
>


2018-03-07 10:06:32

by Richter, Robert

[permalink] [raw]
Subject: Re: [PATCH] watchdog: sbsa: use 32-bit read for WCV

On 28.02.18 02:52:20, Jayachandran C wrote:
> According to SBSA spec v3.1 section 5.3:
> All registers are 32 bits in size and should be accessed using
> 32-bit reads and writes. If an access size other than 32 bits
> is used then the results are IMPLEMENTATION DEFINED.
> [...]
> The Generic Watchdog is little-endian
>
> The current code uses readq to read the watchdog compare register
> which does a 64-bit access. This fails on ThunderX2 which does not
> implement 64-bit access to this register.
>
> Fix this by using lo_hi_readq() that does two 32-bit reads.
>
> Signed-off-by: Jayachandran C <[email protected]>

Reviewed-by: Robert Richter <[email protected]>

I have looked into the non-atomic use of the register access and it
looks sane as the WCV register is only incremented by hardware. There
is no concurrent (write) access from the kernel.

-Robert