diff mbox series

watchdog: sbsa: Support architecture version 1

Message ID 1620618117-20135-1-git-send-email-zhangshaokun@hisilicon.com
State New
Headers show
Series watchdog: sbsa: Support architecture version 1 | expand

Commit Message

Shaokun Zhang May 10, 2021, 3:41 a.m. UTC
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(-)

Comments

Guenter Roeck May 10, 2021, 4:25 a.m. UTC | #1
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);
>
Shaokun Zhang May 10, 2021, 8:44 a.m. UTC | #2
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);
>>>
>>
>> .
Guenter Roeck May 10, 2021, 1:16 p.m. UTC | #3
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
Shaokun Zhang May 11, 2021, 2:49 a.m. UTC | #4
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

> .
Guenter Roeck May 11, 2021, 3:52 a.m. UTC | #5
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
Shaokun Zhang May 11, 2021, 6:03 a.m. UTC | #6
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

> 

> .
diff mbox series

Patch

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