[PATCH] watchdog: designware: make reset really optional

[PATCH] watchdog: designware: make reset really optional

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

Checking for DM_RESET is not enough since not all watchdog
implementations use a reset lane. Such is the case for Rockchip
implementation for example. Since reset_assert_bulk will only succeed if
the resets property exists in the watchdog DT node, it needs to be
called only if a reset property is present.

This adds a condition on the resets property presence in the watchdog DT
node before assuming a reset lane needs to be fetched with
reset_assert_bulk, by calling ofnode_read_prop.

Cc: Quentin Schulz <foss+uboot@0leil.net>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
To: Stefan Roese <sr@denx.de>
Cc: u-boot@lists.denx.de
---
 drivers/watchdog/designware_wdt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/designware_wdt.c b/drivers/watchdog/designware_wdt.c
index cad756aeaf..6155939f49 100644
--- a/drivers/watchdog/designware_wdt.c
+++ b/drivers/watchdog/designware_wdt.c
@@ -72,13 +72,13 @@ static int designware_wdt_reset(struct udevice *dev)
 static int designware_wdt_stop(struct udevice *dev)
 {
 	struct designware_wdt_priv *priv = dev_get_priv(dev);
+	__maybe_unused int ret;
 
 	designware_wdt_reset(dev);
 	writel(0, priv->base + DW_WDT_CR);
 
-        if (CONFIG_IS_ENABLED(DM_RESET)) {
-		int ret;
-
+        if (CONFIG_IS_ENABLED(DM_RESET) &&
+	    ofnode_read_prop(dev_ofnode(dev), "resets", &ret)) {
 		ret = reset_assert_bulk(&priv->resets);
 		if (ret)
 			return ret;
@@ -135,7 +135,8 @@ static int designware_wdt_probe(struct udevice *dev)
 	priv->clk_khz = CONFIG_DW_WDT_CLOCK_KHZ;
 #endif
 
-	if (CONFIG_IS_ENABLED(DM_RESET)) {
+	if (CONFIG_IS_ENABLED(DM_RESET) &&
+	    ofnode_read_prop(dev_ofnode(dev), "resets", &ret)) {
 		ret = reset_get_bulk(dev, &priv->resets);
 		if (ret)
 			goto err;

---
base-commit: 0cbeed4f6648e0e4966475e3544280a69ecb59d3
change-id: 20221114-dw-wdt-no-reset-2296a2bf37b3

Best regards,
-- 
Quentin Schulz <quentin.schulz@theobroma-systems.com>

Re: [PATCH] watchdog: designware: make reset really optional

[Stefan Roese]

On 14.11.22 13:52, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> Checking for DM_RESET is not enough since not all watchdog
> implementations use a reset lane. Such is the case for Rockchip
> implementation for example. Since reset_assert_bulk will only succeed if
> the resets property exists in the watchdog DT node, it needs to be
> called only if a reset property is present.
> 
> This adds a condition on the resets property presence in the watchdog DT
> node before assuming a reset lane needs to be fetched with
> reset_assert_bulk, by calling ofnode_read_prop.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
> To: Stefan Roese <sr@denx.de>
> Cc: u-boot@lists.denx.de

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/watchdog/designware_wdt.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/designware_wdt.c b/drivers/watchdog/designware_wdt.c
> index cad756aeaf..6155939f49 100644
> --- a/drivers/watchdog/designware_wdt.c
> +++ b/drivers/watchdog/designware_wdt.c
> @@ -72,13 +72,13 @@ static int designware_wdt_reset(struct udevice *dev)
>   static int designware_wdt_stop(struct udevice *dev)
>   {
>   	struct designware_wdt_priv *priv = dev_get_priv(dev);
> +	__maybe_unused int ret;
>   
>   	designware_wdt_reset(dev);
>   	writel(0, priv->base + DW_WDT_CR);
>   
> -        if (CONFIG_IS_ENABLED(DM_RESET)) {
> -		int ret;
> -
> +        if (CONFIG_IS_ENABLED(DM_RESET) &&

You seem to be adding spaces instead of a tab here.

> +	    ofnode_read_prop(dev_ofnode(dev), "resets", &ret)) {
>   		ret = reset_assert_bulk(&priv->resets);
>   		if (ret)
>   			return ret;
> @@ -135,7 +135,8 @@ static int designware_wdt_probe(struct udevice *dev)
>   	priv->clk_khz = CONFIG_DW_WDT_CLOCK_KHZ;
>   #endif
>   
> -	if (CONFIG_IS_ENABLED(DM_RESET)) {
> +	if (CONFIG_IS_ENABLED(DM_RESET) &&
> +	    ofnode_read_prop(dev_ofnode(dev), "resets", &ret)) {
>   		ret = reset_get_bulk(dev, &priv->resets);
>   		if (ret)
>   			goto err;
> 
> ---
> base-commit: 0cbeed4f6648e0e4966475e3544280a69ecb59d3
> change-id: 20221114-dw-wdt-no-reset-2296a2bf37b3
> 
> Best regards,

Other than this:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

Re: [PATCH] watchdog: designware: make reset really optional

[Quentin Schulz]

Hi Stefan,

On 11/15/22 08:22, Stefan Roese wrote:
> On 14.11.22 13:52, Quentin Schulz wrote:
>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>
>> Checking for DM_RESET is not enough since not all watchdog
>> implementations use a reset lane. Such is the case for Rockchip
>> implementation for example. Since reset_assert_bulk will only succeed if
>> the resets property exists in the watchdog DT node, it needs to be
>> called only if a reset property is present.
>>
>> This adds a condition on the resets property presence in the watchdog DT
>> node before assuming a reset lane needs to be fetched with
>> reset_assert_bulk, by calling ofnode_read_prop.
>>
>> Cc: Quentin Schulz <foss+uboot@0leil.net>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>> To: Stefan Roese <sr@denx.de>
>> Cc: u-boot@lists.denx.de
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> Thanks,
> Stefan
> 
>> ---
>>   drivers/watchdog/designware_wdt.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/watchdog/designware_wdt.c 
>> b/drivers/watchdog/designware_wdt.c
>> index cad756aeaf..6155939f49 100644
>> --- a/drivers/watchdog/designware_wdt.c
>> +++ b/drivers/watchdog/designware_wdt.c
>> @@ -72,13 +72,13 @@ static int designware_wdt_reset(struct udevice *dev)
>>   static int designware_wdt_stop(struct udevice *dev)
>>   {
>>       struct designware_wdt_priv *priv = dev_get_priv(dev);
>> +    __maybe_unused int ret;
>>       designware_wdt_reset(dev);
>>       writel(0, priv->base + DW_WDT_CR);
>> -        if (CONFIG_IS_ENABLED(DM_RESET)) {
>> -        int ret;
>> -
>> +        if (CONFIG_IS_ENABLED(DM_RESET) &&
> 
> You seem to be adding spaces instead of a tab here.
> 

It is currently indented with spaces, I just added my condition there :)

But will spin a v2 with this suggested change, I'm all for consistency :)

Cheers,
Quentin