linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] watchdog: sbsa: Support architecture version 1
@ 2021-05-10  3:41 Shaokun Zhang
  2021-05-10  4:25 ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Shaokun Zhang @ 2021-05-10  3:41 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Shaokun Zhang, Wim Van Sebroeck, Guenter Roeck, Fu Wei,
	Suravee Suthikulpanit, Al Stone, Timur Tabi, Jianchao Hu,
	Huiqiang Wang

Arm Base System Architecture 1.0[1] has introduced watchdog
revision 1 that increases the length the watchdog offset
regiter to 48 bit, while other operation of the watchdog remains
the same.
Let's support the feature infered it from the architecture version
of watchdog in W_IID register. If the version is 0x1, the watchdog
offset register will be 48 bit, otherwise it will be 32 bit.

[1] https://developer.arm.com/documentation/den0094/latest

Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Fu Wei <fu.wei@linaro.org>
Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: Al Stone <al.stone@linaro.org>
Cc: Timur Tabi <timur@codeaurora.org>
Cc: Jianchao Hu <hujianchao@hisilicon.com>
Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
---
 drivers/watchdog/sbsa_gwdt.c | 46 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index f0f1e3b2e463..ca4f7c416f1e 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c
@@ -73,16 +73,21 @@
 #define SBSA_GWDT_WCS_WS0	BIT(1)
 #define SBSA_GWDT_WCS_WS1	BIT(2)
 
+#define SBSA_GWDT_VERSION_MASK  0xF
+#define SBSA_GWDT_VERSION_SHIFT 16
+
 /**
  * struct sbsa_gwdt - Internal representation of the SBSA GWDT
  * @wdd:		kernel watchdog_device structure
  * @clk:		store the System Counter clock frequency, in Hz.
+ * @version:            store the architecture version
  * @refresh_base:	Virtual address of the watchdog refresh frame
  * @control_base:	Virtual address of the watchdog control frame
  */
 struct sbsa_gwdt {
 	struct watchdog_device	wdd;
 	u32			clk;
+	int			version;
 	void __iomem		*refresh_base;
 	void __iomem		*control_base;
 };
@@ -113,6 +118,27 @@ MODULE_PARM_DESC(nowayout,
 		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
 /*
+ * Read and write are 32 or 64 bits depending on watchdog architecture
+ * version: if version is equal 0, its 32-bits operation; otherwise 64-bits
+ * operation is chosen.
+ */
+static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
+{
+	if (gwdt->version == 0)
+		return (u64)readl(gwdt->control_base + SBSA_GWDT_WOR);
+	else
+		return readq(gwdt->control_base + SBSA_GWDT_WOR);
+}
+
+static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt)
+{
+	if (gwdt->version == 0)
+		writel((u32)val, gwdt->control_base + SBSA_GWDT_WOR);
+	else
+		writeq(val, gwdt->control_base + SBSA_GWDT_WOR);
+}
+
+/*
  * watchdog operation functions
  */
 static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
@@ -123,16 +149,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
 	wdd->timeout = timeout;
 
 	if (action)
-		writel(gwdt->clk * timeout,
-		       gwdt->control_base + SBSA_GWDT_WOR);
+		sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt);
 	else
 		/*
 		 * In the single stage mode, The first signal (WS0) is ignored,
 		 * the timeout is (WOR * 2), so the WOR should be configured
 		 * to half value of timeout.
 		 */
-		writel(gwdt->clk / 2 * timeout,
-		       gwdt->control_base + SBSA_GWDT_WOR);
+		sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt);
 
 	return 0;
 }
@@ -149,7 +173,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
 	 */
 	if (!action &&
 	    !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
-		timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
+		timeleft += sbsa_gwdt_reg_read(gwdt);
 
 	timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
 		    arch_timer_read_counter();
@@ -172,6 +196,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
 	return 0;
 }
 
+static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
+	int ver;
+
+	ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
+	ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
+
+	gwdt->version = ver;
+}
+
 static int sbsa_gwdt_start(struct watchdog_device *wdd)
 {
 	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
@@ -300,6 +335,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	 * it's also a ping, if watchdog is enabled.
 	 */
 	sbsa_gwdt_set_timeout(wdd, wdd->timeout);
+	sbsa_gwdt_get_version(wdd);
 
 	watchdog_stop_on_reboot(wdd);
 	ret = devm_watchdog_register_device(dev, wdd);
-- 
2.7.4


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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-10  3:41 [PATCH] watchdog: sbsa: Support architecture version 1 Shaokun Zhang
@ 2021-05-10  4:25 ` Guenter Roeck
  2021-05-10  8:25   ` Shaokun Zhang
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2021-05-10  4:25 UTC (permalink / raw)
  To: Shaokun Zhang, linux-watchdog
  Cc: Wim Van Sebroeck, Fu Wei, Suravee Suthikulpanit, Al Stone,
	Timur Tabi, Jianchao Hu, Huiqiang Wang

On 5/9/21 8:41 PM, Shaokun Zhang wrote:
> Arm Base System Architecture 1.0[1] has introduced watchdog
> revision 1 that increases the length the watchdog offset

Is that how they call the watchdog count register ?

Also, doesn't that mean that the maximum timeout supported
by the hardware is now larger ?

> regiter to 48 bit, while other operation of the watchdog remains

register

> the same.
> Let's support the feature infered it from the architecture version

I can't parse this sentence.

> of watchdog in W_IID register. If the version is 0x1, the watchdog

W_IIDR ?

> offset register will be 48 bit, otherwise it will be 32 bit.

48 or 64 ? The code says 64.

> 
> [1] https://developer.arm.com/documentation/den0094/latest
> 

There is no download link at that location. Someone with access
to the documentation will have to confirm this.

> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Fu Wei <fu.wei@linaro.org>
> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> Cc: Al Stone <al.stone@linaro.org>
> Cc: Timur Tabi <timur@codeaurora.org>
> Cc: Jianchao Hu <hujianchao@hisilicon.com>
> Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> ---
>   drivers/watchdog/sbsa_gwdt.c | 46 +++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
> index f0f1e3b2e463..ca4f7c416f1e 100644
> --- a/drivers/watchdog/sbsa_gwdt.c
> +++ b/drivers/watchdog/sbsa_gwdt.c
> @@ -73,16 +73,21 @@
>   #define SBSA_GWDT_WCS_WS0	BIT(1)
>   #define SBSA_GWDT_WCS_WS1	BIT(2)
>   
> +#define SBSA_GWDT_VERSION_MASK  0xF
> +#define SBSA_GWDT_VERSION_SHIFT 16
> +
>   /**
>    * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>    * @wdd:		kernel watchdog_device structure
>    * @clk:		store the System Counter clock frequency, in Hz.
> + * @version:            store the architecture version
>    * @refresh_base:	Virtual address of the watchdog refresh frame
>    * @control_base:	Virtual address of the watchdog control frame
>    */
>   struct sbsa_gwdt {
>   	struct watchdog_device	wdd;
>   	u32			clk;
> +	int			version;
>   	void __iomem		*refresh_base;
>   	void __iomem		*control_base;
>   };
> @@ -113,6 +118,27 @@ MODULE_PARM_DESC(nowayout,
>   		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>   
>   /*
> + * Read and write are 32 or 64 bits depending on watchdog architecture
> + * version: if version is equal 0, its 32-bits operation; otherwise 64-bits
> + * operation is chosen.
> + */
> +static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
> +{
> +	if (gwdt->version == 0)
> +		return (u64)readl(gwdt->control_base + SBSA_GWDT_WOR);

Unnecessary typecast.

> +	else
> +		return readq(gwdt->control_base + SBSA_GWDT_WOR);
> +}
> +
> +static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt)

What is the point of making val an u64 variable ? Without changing
the maximum timeout it will never be larger than 0xffffffff.

> +{
> +	if (gwdt->version == 0)
> +		writel((u32)val, gwdt->control_base + SBSA_GWDT_WOR);
> +	else
> +		writeq(val, gwdt->control_base + SBSA_GWDT_WOR);
> +}
> +
> +/*
>    * watchdog operation functions
>    */
>   static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
> @@ -123,16 +149,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>   	wdd->timeout = timeout;
>   
>   	if (action)
> -		writel(gwdt->clk * timeout,
> -		       gwdt->control_base + SBSA_GWDT_WOR);
> +		sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt);
>   	else
>   		/*
>   		 * In the single stage mode, The first signal (WS0) is ignored,
>   		 * the timeout is (WOR * 2), so the WOR should be configured
>   		 * to half value of timeout.
>   		 */
> -		writel(gwdt->clk / 2 * timeout,
> -		       gwdt->control_base + SBSA_GWDT_WOR);
> +		sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt);
>   
>   	return 0;
>   }
> @@ -149,7 +173,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
>   	 */
>   	if (!action &&
>   	    !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
> -		timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
> +		timeleft += sbsa_gwdt_reg_read(gwdt);
>   
>   	timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
>   		    arch_timer_read_counter();
> @@ -172,6 +196,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
>   	return 0;
>   }
>   
> +static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
> +{
> +	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> +	int ver;
> +
> +	ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
> +	ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
> +
> +	gwdt->version = ver;
> +}
> +
>   static int sbsa_gwdt_start(struct watchdog_device *wdd)
>   {
>   	struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
> @@ -300,6 +335,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>   	 * it's also a ping, if watchdog is enabled.
>   	 */
>   	sbsa_gwdt_set_timeout(wdd, wdd->timeout);
> +	sbsa_gwdt_get_version(wdd);
>   
>   	watchdog_stop_on_reboot(wdd);
>   	ret = devm_watchdog_register_device(dev, wdd);
> 


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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-10  4:25 ` Guenter Roeck
@ 2021-05-10  8:25   ` Shaokun Zhang
  2021-05-10  8:44     ` Shaokun Zhang
  2021-05-10 13:16     ` Guenter Roeck
  0 siblings, 2 replies; 9+ messages in thread
From: Shaokun Zhang @ 2021-05-10  8:25 UTC (permalink / raw)
  To: Guenter Roeck, linux-watchdog
  Cc: Wim Van Sebroeck, Suravee Suthikulpanit, Al Stone, Jianchao Hu,
	Huiqiang Wang

Hi Guenter,

On 2021/5/10 12:25, Guenter Roeck wrote:
> On 5/9/21 8:41 PM, Shaokun Zhang wrote:
>> Arm Base System Architecture 1.0[1] has introduced watchdog
>> revision 1 that increases the length the watchdog offset
> 
> Is that how they call the watchdog count register ?
> 

I think yes.

> Also, doesn't that mean that the maximum timeout supported
> by the hardware is now larger ?

No, maximum timeout is the same. But the clock can be higher than
before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to
a frequency of 1GHz which will set gwdt->clk. If the timeout is
greater than 4(second), the 32-bit counter(WOR) is not enough.

> 
>> regiter to 48 bit, while other operation of the watchdog remains
> 
> register

Ok, will fix it.

> 
>> the same.
>> Let's support the feature infered it from the architecture version
> 
> I can't parse this sentence.
> 

Apologies for sentence, I mean that we can read or write the WOR using
readl/writel or readq/writeq depending on the architecture version. If
architecture version is 0, readl/writel are used. Otherwise, we use
readq/writeq.

>> of watchdog in W_IID register. If the version is 0x1, the watchdog
> 
> W_IIDR ?
> 

Yes

>> offset register will be 48 bit, otherwise it will be 32 bit.
> 
> 48 or 64 ? The code says 64.
> 

The whole WOR is 64-bits: WOR_L and WOR_H. WOR_L[31:0] contains the
lower 32 bits;
WOR_H[63:32] comprises two parts, Bits[15:0] of WOR_H contains the
upper 16 bits; Bits[31:16] of WOR_H is reserved that Read all zero
and write has no effect. So the real use is 48-bit.

>>
>> [1] https://developer.arm.com/documentation/den0094/latest
>>
> 
> There is no download link at that location. Someone with access
> to the documentation will have to confirm this.

Can you access this link? If yes, there is a 'Download' label and
you can upload the document and check page 47 of 96.

> 
>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Fu Wei <fu.wei@linaro.org>
>> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> Cc: Al Stone <al.stone@linaro.org>
>> Cc: Timur Tabi <timur@codeaurora.org>
>> Cc: Jianchao Hu <hujianchao@hisilicon.com>
>> Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> ---
>>   drivers/watchdog/sbsa_gwdt.c | 46 +++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
>> index f0f1e3b2e463..ca4f7c416f1e 100644
>> --- a/drivers/watchdog/sbsa_gwdt.c
>> +++ b/drivers/watchdog/sbsa_gwdt.c
>> @@ -73,16 +73,21 @@
>>   #define SBSA_GWDT_WCS_WS0    BIT(1)
>>   #define SBSA_GWDT_WCS_WS1    BIT(2)
>>   +#define SBSA_GWDT_VERSION_MASK  0xF
>> +#define SBSA_GWDT_VERSION_SHIFT 16
>> +
>>   /**
>>    * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>>    * @wdd:        kernel watchdog_device structure
>>    * @clk:        store the System Counter clock frequency, in Hz.
>> + * @version:            store the architecture version
>>    * @refresh_base:    Virtual address of the watchdog refresh frame
>>    * @control_base:    Virtual address of the watchdog control frame
>>    */
>>   struct sbsa_gwdt {
>>       struct watchdog_device    wdd;
>>       u32            clk;
>> +    int            version;
>>       void __iomem        *refresh_base;
>>       void __iomem        *control_base;
>>   };
>> @@ -113,6 +118,27 @@ MODULE_PARM_DESC(nowayout,
>>            __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>     /*
>> + * Read and write are 32 or 64 bits depending on watchdog architecture
>> + * version: if version is equal 0, its 32-bits operation; otherwise 64-bits
>> + * operation is chosen.
>> + */
>> +static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
>> +{
>> +    if (gwdt->version == 0)
>> +        return (u64)readl(gwdt->control_base + SBSA_GWDT_WOR);
> 
> Unnecessary typecast.
> 

Ok.

>> +    else
>> +        return readq(gwdt->control_base + SBSA_GWDT_WOR);
>> +}
>> +
>> +static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt)
> 
> What is the point of making val an u64 variable ? Without changing

Oops, unsigned int is enough.

> the maximum timeout it will never be larger than 0xffffffff.
> 

No, the reason that I have explained that the clock can be 1GHz now.

Thanks,
Shaokun

>> +{
>> +    if (gwdt->version == 0)
>> +        writel((u32)val, gwdt->control_base + SBSA_GWDT_WOR);
>> +    else
>> +        writeq(val, gwdt->control_base + SBSA_GWDT_WOR);
>> +}
>> +
>> +/*
>>    * watchdog operation functions
>>    */
>>   static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>> @@ -123,16 +149,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>>       wdd->timeout = timeout;
>>         if (action)
>> -        writel(gwdt->clk * timeout,
>> -               gwdt->control_base + SBSA_GWDT_WOR);
>> +        sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt);
>>       else
>>           /*
>>            * In the single stage mode, The first signal (WS0) is ignored,
>>            * the timeout is (WOR * 2), so the WOR should be configured
>>            * to half value of timeout.
>>            */
>> -        writel(gwdt->clk / 2 * timeout,
>> -               gwdt->control_base + SBSA_GWDT_WOR);
>> +        sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt);
>>         return 0;
>>   }
>> @@ -149,7 +173,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
>>        */
>>       if (!action &&
>>           !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
>> -        timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
>> +        timeleft += sbsa_gwdt_reg_read(gwdt);
>>         timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
>>               arch_timer_read_counter();
>> @@ -172,6 +196,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
>>       return 0;
>>   }
>>   +static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
>> +{
>> +    struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
>> +    int ver;
>> +
>> +    ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
>> +    ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
>> +
>> +    gwdt->version = ver;
>> +}
>> +
>>   static int sbsa_gwdt_start(struct watchdog_device *wdd)
>>   {
>>       struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
>> @@ -300,6 +335,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>>        * it's also a ping, if watchdog is enabled.
>>        */
>>       sbsa_gwdt_set_timeout(wdd, wdd->timeout);
>> +    sbsa_gwdt_get_version(wdd);
>>         watchdog_stop_on_reboot(wdd);
>>       ret = devm_watchdog_register_device(dev, wdd);
>>
> 
> .

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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-10  8:25   ` Shaokun Zhang
@ 2021-05-10  8:44     ` Shaokun Zhang
  2021-05-10 15:23       ` Guenter Roeck
  2021-05-10 13:16     ` Guenter Roeck
  1 sibling, 1 reply; 9+ messages in thread
From: Shaokun Zhang @ 2021-05-10  8:44 UTC (permalink / raw)
  To: Guenter Roeck, linux-watchdog
  Cc: Wim Van Sebroeck, Suravee Suthikulpanit, Al Stone, Jianchao Hu,
	Huiqiang Wang

Hi Guenter,

On 2021/5/10 16:25, Shaokun Zhang wrote:
> Hi Guenter,
> 
> On 2021/5/10 12:25, Guenter Roeck wrote:
>> On 5/9/21 8:41 PM, Shaokun Zhang wrote:
>>> Arm Base System Architecture 1.0[1] has introduced watchdog
>>> revision 1 that increases the length the watchdog offset
>>
>> Is that how they call the watchdog count register ?
>>
> 
> I think yes.
> 
>> Also, doesn't that mean that the maximum timeout supported
>> by the hardware is now larger ?
> 
> No, maximum timeout is the same. But the clock can be higher than
> before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to
> a frequency of 1GHz which will set gwdt->clk. If the timeout is
> greater than 4(second), the 32-bit counter(WOR) is not enough.
> 
>>
>>> regiter to 48 bit, while other operation of the watchdog remains
>>
>> register
> 
> Ok, will fix it.
> 
>>
>>> the same.
>>> Let's support the feature infered it from the architecture version
>>
>> I can't parse this sentence.
>>
> 
> Apologies for sentence, I mean that we can read or write the WOR using
> readl/writel or readq/writeq depending on the architecture version. If
> architecture version is 0, readl/writel are used. Otherwise, we use
> readq/writeq.
> 
>>> of watchdog in W_IID register. If the version is 0x1, the watchdog
>>
>> W_IIDR ?
>>
> 
> Yes
> 
>>> offset register will be 48 bit, otherwise it will be 32 bit.
>>
>> 48 or 64 ? The code says 64.
>>
> 
> The whole WOR is 64-bits: WOR_L and WOR_H. WOR_L[31:0] contains the
> lower 32 bits;
> WOR_H[63:32] comprises two parts, Bits[15:0] of WOR_H contains the
> upper 16 bits; Bits[31:16] of WOR_H is reserved that Read all zero
> and write has no effect. So the real use is 48-bit.
> 
>>>
>>> [1] https://developer.arm.com/documentation/den0094/latest
>>>
>>
>> There is no download link at that location. Someone with access
>> to the documentation will have to confirm this.
> 
> Can you access this link? If yes, there is a 'Download' label and
> you can upload the document and check page 47 of 96.
> 
>>
>>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: Fu Wei <fu.wei@linaro.org>
>>> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>> Cc: Al Stone <al.stone@linaro.org>
>>> Cc: Timur Tabi <timur@codeaurora.org>
>>> Cc: Jianchao Hu <hujianchao@hisilicon.com>
>>> Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>> ---
>>>   drivers/watchdog/sbsa_gwdt.c | 46 +++++++++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 41 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
>>> index f0f1e3b2e463..ca4f7c416f1e 100644
>>> --- a/drivers/watchdog/sbsa_gwdt.c
>>> +++ b/drivers/watchdog/sbsa_gwdt.c
>>> @@ -73,16 +73,21 @@
>>>   #define SBSA_GWDT_WCS_WS0    BIT(1)
>>>   #define SBSA_GWDT_WCS_WS1    BIT(2)
>>>   +#define SBSA_GWDT_VERSION_MASK  0xF
>>> +#define SBSA_GWDT_VERSION_SHIFT 16
>>> +
>>>   /**
>>>    * struct sbsa_gwdt - Internal representation of the SBSA GWDT
>>>    * @wdd:        kernel watchdog_device structure
>>>    * @clk:        store the System Counter clock frequency, in Hz.
>>> + * @version:            store the architecture version
>>>    * @refresh_base:    Virtual address of the watchdog refresh frame
>>>    * @control_base:    Virtual address of the watchdog control frame
>>>    */
>>>   struct sbsa_gwdt {
>>>       struct watchdog_device    wdd;
>>>       u32            clk;
>>> +    int            version;
>>>       void __iomem        *refresh_base;
>>>       void __iomem        *control_base;
>>>   };
>>> @@ -113,6 +118,27 @@ MODULE_PARM_DESC(nowayout,
>>>            __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>>     /*
>>> + * Read and write are 32 or 64 bits depending on watchdog architecture
>>> + * version: if version is equal 0, its 32-bits operation; otherwise 64-bits
>>> + * operation is chosen.
>>> + */
>>> +static u64 sbsa_gwdt_reg_read(struct sbsa_gwdt *gwdt)
>>> +{
>>> +    if (gwdt->version == 0)
>>> +        return (u64)readl(gwdt->control_base + SBSA_GWDT_WOR);
>>
>> Unnecessary typecast.
>>
> 
> Ok.
> 
>>> +    else
>>> +        return readq(gwdt->control_base + SBSA_GWDT_WOR);
>>> +}
>>> +
>>> +static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt)
>>
>> What is the point of making val an u64 variable ? Without changing
> 
> Oops, unsigned int is enough.
> 

Sorry, it shall be 'u64', because it is the value that clock * timeout
and will be written to WOR register which is 64-bit register in
architecture version 1.

Thanks,
Shaokun

>> the maximum timeout it will never be larger than 0xffffffff.
>>
> 
> No, the reason that I have explained that the clock can be 1GHz now.
> 
> Thanks,
> Shaokun
> 
>>> +{
>>> +    if (gwdt->version == 0)
>>> +        writel((u32)val, gwdt->control_base + SBSA_GWDT_WOR);
>>> +    else
>>> +        writeq(val, gwdt->control_base + SBSA_GWDT_WOR);
>>> +}
>>> +
>>> +/*
>>>    * watchdog operation functions
>>>    */
>>>   static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>>> @@ -123,16 +149,14 @@ static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
>>>       wdd->timeout = timeout;
>>>         if (action)
>>> -        writel(gwdt->clk * timeout,
>>> -               gwdt->control_base + SBSA_GWDT_WOR);
>>> +        sbsa_gwdt_reg_write(gwdt->clk * timeout, gwdt);
>>>       else
>>>           /*
>>>            * In the single stage mode, The first signal (WS0) is ignored,
>>>            * the timeout is (WOR * 2), so the WOR should be configured
>>>            * to half value of timeout.
>>>            */
>>> -        writel(gwdt->clk / 2 * timeout,
>>> -               gwdt->control_base + SBSA_GWDT_WOR);
>>> +        sbsa_gwdt_reg_write(gwdt->clk / 2 * timeout, gwdt);
>>>         return 0;
>>>   }
>>> @@ -149,7 +173,7 @@ static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
>>>        */
>>>       if (!action &&
>>>           !(readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0))
>>> -        timeleft += readl(gwdt->control_base + SBSA_GWDT_WOR);
>>> +        timeleft += sbsa_gwdt_reg_read(gwdt);
>>>         timeleft += lo_hi_readq(gwdt->control_base + SBSA_GWDT_WCV) -
>>>               arch_timer_read_counter();
>>> @@ -172,6 +196,17 @@ static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
>>>       return 0;
>>>   }
>>>   +static void sbsa_gwdt_get_version(struct watchdog_device *wdd)
>>> +{
>>> +    struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
>>> +    int ver;
>>> +
>>> +    ver = readl(gwdt->control_base + SBSA_GWDT_W_IIDR);
>>> +    ver = (ver >> SBSA_GWDT_VERSION_SHIFT) & SBSA_GWDT_VERSION_MASK;
>>> +
>>> +    gwdt->version = ver;
>>> +}
>>> +
>>>   static int sbsa_gwdt_start(struct watchdog_device *wdd)
>>>   {
>>>       struct sbsa_gwdt *gwdt = watchdog_get_drvdata(wdd);
>>> @@ -300,6 +335,7 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
>>>        * it's also a ping, if watchdog is enabled.
>>>        */
>>>       sbsa_gwdt_set_timeout(wdd, wdd->timeout);
>>> +    sbsa_gwdt_get_version(wdd);
>>>         watchdog_stop_on_reboot(wdd);
>>>       ret = devm_watchdog_register_device(dev, wdd);
>>>
>>
>> .

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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-10  8:25   ` Shaokun Zhang
  2021-05-10  8:44     ` Shaokun Zhang
@ 2021-05-10 13:16     ` Guenter Roeck
  2021-05-11  2:49       ` Shaokun Zhang
  1 sibling, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2021-05-10 13:16 UTC (permalink / raw)
  To: Shaokun Zhang, linux-watchdog
  Cc: Wim Van Sebroeck, Suravee Suthikulpanit, Al Stone, Jianchao Hu,
	Huiqiang Wang

On 5/10/21 1:25 AM, Shaokun Zhang wrote:
> Hi Guenter,
> 
> On 2021/5/10 12:25, Guenter Roeck wrote:
>> On 5/9/21 8:41 PM, Shaokun Zhang wrote:
>>> Arm Base System Architecture 1.0[1] has introduced watchdog
>>> revision 1 that increases the length the watchdog offset
>>
>> Is that how they call the watchdog count register ?
>>
> 
> I think yes.
> 
>> Also, doesn't that mean that the maximum timeout supported
>> by the hardware is now larger ?
> 
> No, maximum timeout is the same. But the clock can be higher than
> before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to
> a frequency of 1GHz which will set gwdt->clk. If the timeout is
> greater than 4(second), the 32-bit counter(WOR) is not enough.
> 

The maximuma timeout is limited with

wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;

You did not update that calculation. That means that the maximuma
timeout is still U32_MAX / gwdt->clk * 1000, which still fits
into 32 bit.

Please correct me if I am missing something.

Guenter

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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-10  8:44     ` Shaokun Zhang
@ 2021-05-10 15:23       ` Guenter Roeck
  0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2021-05-10 15:23 UTC (permalink / raw)
  To: Shaokun Zhang
  Cc: linux-watchdog, Wim Van Sebroeck, Suravee Suthikulpanit,
	Al Stone, Jianchao Hu, Huiqiang Wang

On Mon, May 10, 2021 at 04:44:21PM +0800, Shaokun Zhang wrote:

> >>> +static void sbsa_gwdt_reg_write(u64 val, struct sbsa_gwdt *gwdt)
> >>
> >> What is the point of making val an u64 variable ? Without changing
> > 
> > Oops, unsigned int is enough.
> > 
> 
> Sorry, it shall be 'u64', because it is the value that clock * timeout
> and will be written to WOR register which is 64-bit register in
> architecture version 1.
> 

As I have been trying to point out, that won't really happen unless
max_hw_heartbeat_ms is adjusted. The register may be a 64 bit register,
and the variable may be a 64-bit variable, but that doesn't make the
value passed in that variable larger than 32 bit unless the code is
in place to actually do that.

Guenter

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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-10 13:16     ` Guenter Roeck
@ 2021-05-11  2:49       ` Shaokun Zhang
  2021-05-11  3:52         ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Shaokun Zhang @ 2021-05-11  2:49 UTC (permalink / raw)
  To: Guenter Roeck, linux-watchdog
  Cc: Wim Van Sebroeck, Suravee Suthikulpanit, Al Stone, Jianchao Hu,
	Huiqiang Wang

Hi Guenter,

On 2021/5/10 21:16, Guenter Roeck wrote:
> On 5/10/21 1:25 AM, Shaokun Zhang wrote:
>> Hi Guenter,
>>
>> On 2021/5/10 12:25, Guenter Roeck wrote:
>>> On 5/9/21 8:41 PM, Shaokun Zhang wrote:
>>>> Arm Base System Architecture 1.0[1] has introduced watchdog
>>>> revision 1 that increases the length the watchdog offset
>>>
>>> Is that how they call the watchdog count register ?
>>>
>>
>> I think yes.
>>
>>> Also, doesn't that mean that the maximum timeout supported
>>> by the hardware is now larger ?
>>
>> No, maximum timeout is the same. But the clock can be higher than
>> before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to
>> a frequency of 1GHz which will set gwdt->clk. If the timeout is
>> greater than 4(second), the 32-bit counter(WOR) is not enough.
>>
> 
> The maximuma timeout is limited with
> 
> wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
> > You did not update that calculation. That means that the maximuma
> timeout is still U32_MAX / gwdt->clk * 1000, which still fits
> into 32 bit.

Correct, I will fix this in next version.

> 
> Please correct me if I am missing something.
> 

My bad, you are right. The maximum timeout shall be
0xFFFF.FFFF.FFFF / 1000.000.000(1GHz) which is larger than before
0xFFFF.FFFF / 100.000.000(100MHz) by the hardware.

Can I do like this, after the version is got and check the version?
        sbsa_gwdt_set_timeout(wdd, wdd->timeout);
+       sbsa_gwdt_get_version(wdd);
+       if (wdd->version > 0)
+               wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;


Thanks,
Shaokun

> Guenter
> .

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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-11  2:49       ` Shaokun Zhang
@ 2021-05-11  3:52         ` Guenter Roeck
  2021-05-11  6:03           ` Shaokun Zhang
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2021-05-11  3:52 UTC (permalink / raw)
  To: Shaokun Zhang, linux-watchdog
  Cc: Wim Van Sebroeck, Suravee Suthikulpanit, Al Stone, Jianchao Hu,
	Huiqiang Wang

On 5/10/21 7:49 PM, Shaokun Zhang wrote:
> Hi Guenter,
> 
> On 2021/5/10 21:16, Guenter Roeck wrote:
>> On 5/10/21 1:25 AM, Shaokun Zhang wrote:
>>> Hi Guenter,
>>>
>>> On 2021/5/10 12:25, Guenter Roeck wrote:
>>>> On 5/9/21 8:41 PM, Shaokun Zhang wrote:
>>>>> Arm Base System Architecture 1.0[1] has introduced watchdog
>>>>> revision 1 that increases the length the watchdog offset
>>>>
>>>> Is that how they call the watchdog count register ?
>>>>
>>>
>>> I think yes.
>>>
>>>> Also, doesn't that mean that the maximum timeout supported
>>>> by the hardware is now larger ?
>>>
>>> No, maximum timeout is the same. But the clock can be higher than
>>> before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to
>>> a frequency of 1GHz which will set gwdt->clk. If the timeout is
>>> greater than 4(second), the 32-bit counter(WOR) is not enough.
>>>
>>
>> The maximuma timeout is limited with
>>
>> wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
>>> You did not update that calculation. That means that the maximuma
>> timeout is still U32_MAX / gwdt->clk * 1000, which still fits
>> into 32 bit.
> 
> Correct, I will fix this in next version.
> 
>>
>> Please correct me if I am missing something.
>>
> 
> My bad, you are right. The maximum timeout shall be
> 0xFFFF.FFFF.FFFF / 1000.000.000(1GHz) which is larger than before
> 0xFFFF.FFFF / 100.000.000(100MHz) by the hardware.
> 
> Can I do like this, after the version is got and check the version?
>          sbsa_gwdt_set_timeout(wdd, wdd->timeout);
> +       sbsa_gwdt_get_version(wdd);
> +       if (wdd->version > 0)
> +               wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
> 

I would suggest to set max_hw_heartbeat_ms in one place
to avoid confusion. Either check the version earlier,
or move setting both max_hw_heartbeat_ms values
after the call to sbsa_gwdt_get_version().

Thanks,
Guenter


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

* Re: [PATCH] watchdog: sbsa: Support architecture version 1
  2021-05-11  3:52         ` Guenter Roeck
@ 2021-05-11  6:03           ` Shaokun Zhang
  0 siblings, 0 replies; 9+ messages in thread
From: Shaokun Zhang @ 2021-05-11  6:03 UTC (permalink / raw)
  To: Guenter Roeck, linux-watchdog
  Cc: Wim Van Sebroeck, Suravee Suthikulpanit, Al Stone, Jianchao Hu,
	Huiqiang Wang

Hi Guenter,

On 2021/5/11 11:52, Guenter Roeck wrote:
> On 5/10/21 7:49 PM, Shaokun Zhang wrote:
>> Hi Guenter,
>>
>> On 2021/5/10 21:16, Guenter Roeck wrote:
>>> On 5/10/21 1:25 AM, Shaokun Zhang wrote:
>>>> Hi Guenter,
>>>>
>>>> On 2021/5/10 12:25, Guenter Roeck wrote:
>>>>> On 5/9/21 8:41 PM, Shaokun Zhang wrote:
>>>>>> Arm Base System Architecture 1.0[1] has introduced watchdog
>>>>>> revision 1 that increases the length the watchdog offset
>>>>>
>>>>> Is that how they call the watchdog count register ?
>>>>>
>>>>
>>>> I think yes.
>>>>
>>>>> Also, doesn't that mean that the maximum timeout supported
>>>>> by the hardware is now larger ?
>>>>
>>>> No, maximum timeout is the same. But the clock can be higher than
>>>> before. For Armv8.6, The frequency of CNTFRQ_EL0 is standardized to
>>>> a frequency of 1GHz which will set gwdt->clk. If the timeout is
>>>> greater than 4(second), the 32-bit counter(WOR) is not enough.
>>>>
>>>
>>> The maximuma timeout is limited with
>>>
>>> wdd->max_hw_heartbeat_ms = U32_MAX / gwdt->clk * 1000;
>>>> You did not update that calculation. That means that the maximuma
>>> timeout is still U32_MAX / gwdt->clk * 1000, which still fits
>>> into 32 bit.
>>
>> Correct, I will fix this in next version.
>>
>>>
>>> Please correct me if I am missing something.
>>>
>>
>> My bad, you are right. The maximum timeout shall be
>> 0xFFFF.FFFF.FFFF / 1000.000.000(1GHz) which is larger than before
>> 0xFFFF.FFFF / 100.000.000(100MHz) by the hardware.
>>
>> Can I do like this, after the version is got and check the version?
>>          sbsa_gwdt_set_timeout(wdd, wdd->timeout);
>> +       sbsa_gwdt_get_version(wdd);
>> +       if (wdd->version > 0)
>> +               wdd->max_hw_heartbeat_ms = GENMASK_ULL(47, 0) / gwdt->clk * 1000;
>>
> 
> I would suggest to set max_hw_heartbeat_ms in one place
> to avoid confusion. Either check the version earlier,
> or move setting both max_hw_heartbeat_ms values
> after the call to sbsa_gwdt_get_version().
> 

Got it, I will follow the former that many members in @wdd are
initialized closely.

Thanks again,
Shaokun

> Thanks,
> Guenter
> 
> .

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

end of thread, other threads:[~2021-05-11  6:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10  3:41 [PATCH] watchdog: sbsa: Support architecture version 1 Shaokun Zhang
2021-05-10  4:25 ` Guenter Roeck
2021-05-10  8:25   ` Shaokun Zhang
2021-05-10  8:44     ` Shaokun Zhang
2021-05-10 15:23       ` Guenter Roeck
2021-05-10 13:16     ` Guenter Roeck
2021-05-11  2:49       ` Shaokun Zhang
2021-05-11  3:52         ` Guenter Roeck
2021-05-11  6:03           ` Shaokun Zhang

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).