[PATCH v2 1/4] watchdog: always populate watchdog priority from device tree if possible

[PATCH v2 1/4] watchdog: always populate watchdog priority from device tree if possible

[Ahmad Fatoum]

So far, only the da9063 and da9053 have made use of the optional barebox
watchdog-priority binding. Move it into the core, so other device
drivers automatically have their watchdog-priority property parsed as
well. This patch doesn't introduce any functional changes for upstream
boards.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  Unchanged.
---
 drivers/mfd/da9053.c       |  1 -
 drivers/mfd/da9063.c       |  1 -
 drivers/watchdog/wd_core.c | 34 ++++++++++++++++++----------------
 include/watchdog.h         |  7 -------
 4 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/mfd/da9053.c b/drivers/mfd/da9053.c
index 1faba813bee8..f4bfb68d0301 100644
--- a/drivers/mfd/da9053.c
+++ b/drivers/mfd/da9053.c
@@ -271,7 +271,6 @@ static int da9053_probe(struct device_d *dev)
 	da9053->dev = dev;
 	da9053->client = to_i2c_client(dev);
 	da9053->wd.set_timeout = da9053_set_timeout;
-	da9053->wd.priority = of_get_watchdog_priority(dev->device_node);
 	da9053->wd.hwdev = dev;
 
 	ret = da9053_enable_multiwrite(da9053);
diff --git a/drivers/mfd/da9063.c b/drivers/mfd/da9063.c
index b61e7648768e..342424a8b377 100644
--- a/drivers/mfd/da9063.c
+++ b/drivers/mfd/da9063.c
@@ -365,7 +365,6 @@ static int da9063_probe(struct device_d *dev)
 	dev_data = ret < 0 ? NULL : dev_data_tmp;
 
 	priv = xzalloc(sizeof(struct da9063));
-	priv->wd.priority = of_get_watchdog_priority(dev->device_node);
 	priv->wd.set_timeout = da9063_watchdog_set_timeout;
 	priv->wd.hwdev = dev;
 	priv->timeout = DA9063_INITIAL_TIMEOUT;
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 8b13950238c9..39cac6f6c494 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -127,6 +127,23 @@ static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
 	return register_device(&wd->dev);
 }
 
+/**
+ * dev_get_watchdog_priority() - get a device's desired watchdog priority
+ * @dev:	The device, which device_node to read the property from
+ *
+ * return: The priority
+ */
+static unsigned int dev_get_watchdog_priority(struct device_d *dev)
+{
+	unsigned int priority = WATCHDOG_DEFAULT_PRIORITY;
+
+	if (dev)
+		of_property_read_u32(dev->device_node, "watchdog-priority",
+				     &priority);
+
+	return priority;
+}
+
 int watchdog_register(struct watchdog *wd)
 {
 	struct param_d *p;
@@ -146,7 +163,7 @@ int watchdog_register(struct watchdog *wd)
 		return ret;
 
 	if (!wd->priority)
-		wd->priority = WATCHDOG_DEFAULT_PRIORITY;
+		wd->priority = dev_get_watchdog_priority(wd->hwdev);
 
 	p = dev_add_param_uint32(&wd->dev, "priority",
 				 watchdog_set_priority, NULL,
@@ -232,18 +249,3 @@ struct watchdog *watchdog_get_by_name(const char *name)
 	return NULL;
 }
 EXPORT_SYMBOL(watchdog_get_by_name);
-
-/**
- * of_get_watchdog_priority() - get the desired watchdog priority from device tree
- * @node:	The device_node to read the property from
- *
- * return: The priority
- */
-unsigned int of_get_watchdog_priority(struct device_node *node)
-{
-	unsigned int priority = WATCHDOG_DEFAULT_PRIORITY;
-
-	of_property_read_u32(node, "watchdog-priority", &priority);
-
-	return priority;
-}
diff --git a/include/watchdog.h b/include/watchdog.h
index 184a218916f4..105b7ca81093 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -35,7 +35,6 @@ int watchdog_deregister(struct watchdog *);
 struct watchdog *watchdog_get_default(void);
 struct watchdog *watchdog_get_by_name(const char *name);
 int watchdog_set_timeout(struct watchdog*, unsigned);
-unsigned int of_get_watchdog_priority(struct device_node *node);
 #else
 static inline int watchdog_register(struct watchdog *w)
 {
@@ -61,12 +60,6 @@ static inline int watchdog_set_timeout(struct watchdog*w, unsigned t)
 {
 	return 0;
 }
-
-
-static inline unsigned int of_get_watchdog_priority(struct device_node *node)
-{
-	return 0;
-}
 #endif
 
 #define WATCHDOG_DEFAULT_PRIORITY 100
-- 
2.24.0.rc1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Ahmad Fatoum]

Linux watchdog have an optional WDOG_HW_RUNNING bit that is used in
conjunction with CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED to automatically
ping running watchdogs until userspace takes over.

So far, when we ported Linux drivers, we dropped this detection, but it
would be useful to have this information in barebox as well:

The American Megatrends BIOS I am using allows configuring the hardware
watchdog from the BIOS. barebox enables the WDT as well, so in normal
operation we would never notice if after a BIOS update, the watchdog is
no longer enabled. If we maintain a running parameter on watchdog
devices, board code can be written to check whether the watchdog device
is indeed running.

To achieve this, add the necessary bits to the watchdog API. How we go
about it differs from Linux a little:

- We add a WDOG_HW_RUNNING_SUPPORTED bit to differentiate between
  watchdogs that are not running and watchdogs whose running status is
  unknown.
- Because we can check whether watchdog_hw_running is supported, it now
  can fail and return a negative value in that case
- We do the maintenance of the running parameter after barebox
  feeds/disables the watchdog in the core, so it doesn't need to
  be replicated across drivers. Drivers will only need to initialize the
  bitmask once at probe time.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  - WDOG_HW_RUNNING: add comment describing use
  - struct watchdog: drop status_supported member in favor of having a
    single WDOG_HW_RUNNING_SUPPORTED bit
  - watchdog_set_timeout: return 0 instead of ret, when we now that ret
    == 0
---
 drivers/watchdog/wd_core.c | 33 ++++++++++++++++++++++++++++++++-
 include/watchdog.h         | 19 +++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index 39cac6f6c494..034508d4ad73 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -37,6 +37,8 @@ static const char *watchdog_name(struct watchdog *wd)
  */
 int watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 {
+	int ret;
+
 	if (!wd)
 		return -ENODEV;
 
@@ -45,7 +47,18 @@ int watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
 
 	pr_debug("setting timeout on %s to %ds\n", watchdog_name(wd), timeout);
 
-	return wd->set_timeout(wd, timeout);
+	ret = wd->set_timeout(wd, timeout);
+	if (ret)
+		return ret;
+
+	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
+		if (timeout)
+			set_bit(WDOG_HW_RUNNING, &wd->status);
+		else
+			clear_bit(WDOG_HW_RUNNING, &wd->status);
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(watchdog_set_timeout);
 
@@ -118,6 +131,15 @@ static int watchdog_register_poller(struct watchdog *wd)
 	return PTR_ERR_OR_ZERO(p);
 }
 
+static const char *watchdog_get_running(struct device_d *dev, struct param_d *p)
+{
+	/*
+	 * This won't ever fail, because the parameter is only registed when
+	 * test_bit(WDOG_HW_RUNNING_SUPPORTED, &w->status) is true
+	 */
+	return watchdog_hw_running((struct watchdog *)p->value) ? "1" : "0";
+}
+
 static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
 {
 	wd->dev.parent = wd->hwdev;
@@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
 	if (ret)
 		return ret;
 
+	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
+		p = dev_add_param(&wd->dev, "running", NULL,
+				  watchdog_get_running, PARAM_FLAG_RO);
+		if (IS_ERR(p))
+			return PTR_ERR(p);
+
+		p->value = (char *)wd;
+	}
+
 	if (!wd->priority)
 		wd->priority = dev_get_watchdog_priority(wd->hwdev);
 
diff --git a/include/watchdog.h b/include/watchdog.h
index 105b7ca81093..25b9d6840922 100644
--- a/include/watchdog.h
+++ b/include/watchdog.h
@@ -15,6 +15,7 @@
 
 #include <poller.h>
 #include <driver.h>
+#include <linux/bitops.h>
 
 struct watchdog {
 	int (*set_timeout)(struct watchdog *, unsigned);
@@ -27,8 +28,26 @@ struct watchdog {
 	unsigned int poller_enable;
 	struct poller_async poller;
 	struct list_head list;
+/* Bit numbers for status flags */
+#define	WDOG_HW_RUNNING			3	/* True if HW watchdog running */
+#define	WDOG_HW_RUNNING_SUPPORTED	31	/* True if querying HW for
+						 * running status is supported
+						 */
+	unsigned long status;
 };
 
+/*
+ * Use the following function to check whether or not the hardware watchdog
+ * is running
+ */
+static inline int watchdog_hw_running(struct watchdog *w)
+{
+	if (!test_bit(WDOG_HW_RUNNING_SUPPORTED, &w->status))
+		return -ENOSYS;
+
+	return !!test_bit(WDOG_HW_RUNNING, &w->status);
+}
+
 #ifdef CONFIG_WATCHDOG
 int watchdog_register(struct watchdog *);
 int watchdog_deregister(struct watchdog *);
-- 
2.24.0.rc1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Sascha Hauer]

Hi Ahmad,

On Mon, Nov 04, 2019 at 11:14:05PM +0100, Ahmad Fatoum wrote:
> Linux watchdog have an optional WDOG_HW_RUNNING bit that is used in
> conjunction with CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED to automatically
> ping running watchdogs until userspace takes over.
> 
> So far, when we ported Linux drivers, we dropped this detection, but it
> would be useful to have this information in barebox as well:
> 
> The American Megatrends BIOS I am using allows configuring the hardware
> watchdog from the BIOS. barebox enables the WDT as well, so in normal
> operation we would never notice if after a BIOS update, the watchdog is
> no longer enabled. If we maintain a running parameter on watchdog
> devices, board code can be written to check whether the watchdog device
> is indeed running.

To write such code I would prefer to have a function which returns the
running status rather than playing with getenv(). Additionally there is
a function missing in the watchdog code which returns a watchdog by its
name. This can be a future excercise, no need to do it in this patch.

> @@ -45,7 +47,18 @@ int watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
>  
>  	pr_debug("setting timeout on %s to %ds\n", watchdog_name(wd), timeout);
>  
> -	return wd->set_timeout(wd, timeout);
> +	ret = wd->set_timeout(wd, timeout);
> +	if (ret)
> +		return ret;
> +
> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
> +		if (timeout)
> +			set_bit(WDOG_HW_RUNNING, &wd->status);
> +		else
> +			clear_bit(WDOG_HW_RUNNING, &wd->status);
> +	}

When we just started the watchdog we actually know that it is running,
so we could support the parameter for all watchdogs once it's started.

> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(watchdog_set_timeout);
>  
> @@ -118,6 +131,15 @@ static int watchdog_register_poller(struct watchdog *wd)
>  	return PTR_ERR_OR_ZERO(p);
>  }
>  
> +static const char *watchdog_get_running(struct device_d *dev, struct param_d *p)
> +{
> +	/*
> +	 * This won't ever fail, because the parameter is only registed when
> +	 * test_bit(WDOG_HW_RUNNING_SUPPORTED, &w->status) is true
> +	 */
> +	return watchdog_hw_running((struct watchdog *)p->value) ? "1" : "0";
> +}

Casting p->value to (struct watchdog *) seems wrong. However, this
function shouldn't be needed, see below.

> +
>  static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
>  {
>  	wd->dev.parent = wd->hwdev;
> @@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
>  	if (ret)
>  		return ret;
>  
> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
> +		p = dev_add_param(&wd->dev, "running", NULL,
> +				  watchdog_get_running, PARAM_FLAG_RO);
> +		if (IS_ERR(p))
> +			return PTR_ERR(p);
> +
> +		p->value = (char *)wd;
> +	}

How about adding this parameter unconditionally, with "unknown" as value
when WDOG_HW_RUNNING_SUPPORTED is not set. You can use
dev_add_param_enum() here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Ahmad Fatoum]

Hi,

On 11/5/19 11:40 AM, Sascha Hauer wrote:
> Hi Ahmad,
> 
> On Mon, Nov 04, 2019 at 11:14:05PM +0100, Ahmad Fatoum wrote:
>> Linux watchdog have an optional WDOG_HW_RUNNING bit that is used in
>> conjunction with CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED to automatically
>> ping running watchdogs until userspace takes over.
>>
>> So far, when we ported Linux drivers, we dropped this detection, but it
>> would be useful to have this information in barebox as well:
>>
>> The American Megatrends BIOS I am using allows configuring the hardware
>> watchdog from the BIOS. barebox enables the WDT as well, so in normal
>> operation we would never notice if after a BIOS update, the watchdog is
>> no longer enabled. If we maintain a running parameter on watchdog
>> devices, board code can be written to check whether the watchdog device
>> is indeed running.
> 
> To write such code I would prefer to have a function which returns the
> running status rather than playing with getenv().

Ye, board code can use watchdog_hw_running added in this patch.
Hush scripts can use the device parameter. I'll amend the commit message.

> Additionally there is
> a function missing in the watchdog code which returns a watchdog by its
> name. This can be a future excercise, no need to do it in this patch.

Ok.

> 
>> @@ -45,7 +47,18 @@ int watchdog_set_timeout(struct watchdog *wd, unsigned timeout)
>>  
>>  	pr_debug("setting timeout on %s to %ds\n", watchdog_name(wd), timeout);
>>  
>> -	return wd->set_timeout(wd, timeout);
>> +	ret = wd->set_timeout(wd, timeout);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
>> +		if (timeout)
>> +			set_bit(WDOG_HW_RUNNING, &wd->status);
>> +		else
>> +			clear_bit(WDOG_HW_RUNNING, &wd->status);
>> +	}
> 
> When we just started the watchdog we actually know that it is running,
> so we could support the parameter for all watchdogs once it's started.
> 
>> +
>> +	return 0;
>>  }
>>  EXPORT_SYMBOL(watchdog_set_timeout);
>>  
>> @@ -118,6 +131,15 @@ static int watchdog_register_poller(struct watchdog *wd)
>>  	return PTR_ERR_OR_ZERO(p);
>>  }
>>  
>> +static const char *watchdog_get_running(struct device_d *dev, struct param_d *p)
>> +{
>> +	/*
>> +	 * This won't ever fail, because the parameter is only registed when
>> +	 * test_bit(WDOG_HW_RUNNING_SUPPORTED, &w->status) is true
>> +	 */
>> +	return watchdog_hw_running((struct watchdog *)p->value) ? "1" : "0";
>> +}
> 
> Casting p->value to (struct watchdog *) seems wrong. However, this
> function shouldn't be needed, see below.
> 
>> +
>>  static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
>>  {
>>  	wd->dev.parent = wd->hwdev;
>> @@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
>> +		p = dev_add_param(&wd->dev, "running", NULL,
>> +				  watchdog_get_running, PARAM_FLAG_RO);
>> +		if (IS_ERR(p))
>> +			return PTR_ERR(p);
>> +
>> +		p->value = (char *)wd;
>> +	}
> 
> How about adding this parameter unconditionally, with "unknown" as value
> when WDOG_HW_RUNNING_SUPPORTED is not set. You can use
> dev_add_param_enum() here.

Sounds good.

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Sascha Hauer]

On Tue, Nov 05, 2019 at 11:46:18AM +0100, Ahmad Fatoum wrote:
> Hi,
> 
> On 11/5/19 11:40 AM, Sascha Hauer wrote:
> > Hi Ahmad,
> > 
> > On Mon, Nov 04, 2019 at 11:14:05PM +0100, Ahmad Fatoum wrote:
> >> Linux watchdog have an optional WDOG_HW_RUNNING bit that is used in
> >> conjunction with CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED to automatically
> >> ping running watchdogs until userspace takes over.
> >>
> >> So far, when we ported Linux drivers, we dropped this detection, but it
> >> would be useful to have this information in barebox as well:
> >>
> >> The American Megatrends BIOS I am using allows configuring the hardware
> >> watchdog from the BIOS. barebox enables the WDT as well, so in normal
> >> operation we would never notice if after a BIOS update, the watchdog is
> >> no longer enabled. If we maintain a running parameter on watchdog
> >> devices, board code can be written to check whether the watchdog device
> >> is indeed running.
> > 
> > To write such code I would prefer to have a function which returns the
> > running status rather than playing with getenv().
> 
> Ye, board code can use watchdog_hw_running added in this patch.
> Hush scripts can use the device parameter. I'll amend the commit message.

You are right, I missed watchdog_hw_running() in your patch.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Ahmad Fatoum]

On 11/5/19 11:46 AM, Ahmad Fatoum wrote:
> Hi,
> 
> On 11/5/19 11:40 AM, Sascha Hauer wrote:
>> When we just started the watchdog we actually know that it is running,
>> so we could support the parameter for all watchdogs once it's started.

I'll do this.

>> Casting p->value to (struct watchdog *) seems wrong. However, this
>> function shouldn't be needed, see below.

It's well-defined, but ye, it feels a bit wrong.

>>> +
>>>  static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
>>>  {
>>>  	wd->dev.parent = wd->hwdev;
>>> @@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
>>> +		p = dev_add_param(&wd->dev, "running", NULL,
>>> +				  watchdog_get_running, PARAM_FLAG_RO);
>>> +		if (IS_ERR(p))
>>> +			return PTR_ERR(p);
>>> +
>>> +		p->value = (char *)wd;
>>> +	}
>>
>> How about adding this parameter unconditionally, with "unknown" as value
>> when WDOG_HW_RUNNING_SUPPORTED is not set. You can use
>> dev_add_param_enum() here.

Thinking about it, I'd rather not use dev_add_param_enum here. I would like
to leave the driver-side API not too different from Linux, i.e. status bits
that can be set. If I use dev_add_param_enum, I'll have to add a new struct
member for every new status bit. I can respin the patch with an unconditional
dev_add_param though.

> 
> Sounds good.
> 
>>
>> Sascha
>>
> 


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Sascha Hauer]

On Tue, Nov 05, 2019 at 12:10:51PM +0100, Ahmad Fatoum wrote:
> On 11/5/19 11:46 AM, Ahmad Fatoum wrote:
> > Hi,
> > 
> > On 11/5/19 11:40 AM, Sascha Hauer wrote:
> >> When we just started the watchdog we actually know that it is running,
> >> so we could support the parameter for all watchdogs once it's started.
> 
> I'll do this.
> 
> >> Casting p->value to (struct watchdog *) seems wrong. However, this
> >> function shouldn't be needed, see below.
> 
> It's well-defined, but ye, it feels a bit wrong.
> 
> >>> +
> >>>  static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
> >>>  {
> >>>  	wd->dev.parent = wd->hwdev;
> >>> @@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
> >>>  	if (ret)
> >>>  		return ret;
> >>>  
> >>> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
> >>> +		p = dev_add_param(&wd->dev, "running", NULL,
> >>> +				  watchdog_get_running, PARAM_FLAG_RO);
> >>> +		if (IS_ERR(p))
> >>> +			return PTR_ERR(p);
> >>> +
> >>> +		p->value = (char *)wd;
> >>> +	}
> >>
> >> How about adding this parameter unconditionally, with "unknown" as value
> >> when WDOG_HW_RUNNING_SUPPORTED is not set. You can use
> >> dev_add_param_enum() here.
> 
> Thinking about it, I'd rather not use dev_add_param_enum here. I would like
> to leave the driver-side API not too different from Linux, i.e. status bits
> that can be set. If I use dev_add_param_enum, I'll have to add a new struct
> member for every new status bit.

Not for every bit, but for every flag you want to export as a parameter.
But where's the problem with that?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/4] watchdog: implement generic support for .running device parameter

[Ahmad Fatoum]

On 11/5/19 12:18 PM, Sascha Hauer wrote:
> On Tue, Nov 05, 2019 at 12:10:51PM +0100, Ahmad Fatoum wrote:
>> On 11/5/19 11:46 AM, Ahmad Fatoum wrote:
>>> Hi,
>>>
>>> On 11/5/19 11:40 AM, Sascha Hauer wrote:
>>>> When we just started the watchdog we actually know that it is running,
>>>> so we could support the parameter for all watchdogs once it's started.
>>
>> I'll do this.
>>
>>>> Casting p->value to (struct watchdog *) seems wrong. However, this
>>>> function shouldn't be needed, see below.
>>
>> It's well-defined, but ye, it feels a bit wrong.
>>
>>>>> +
>>>>>  static int watchdog_register_dev(struct watchdog *wd, const char *name, int id)
>>>>>  {
>>>>>  	wd->dev.parent = wd->hwdev;
>>>>> @@ -162,6 +184,15 @@ int watchdog_register(struct watchdog *wd)
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>  
>>>>> +	if (test_bit(WDOG_HW_RUNNING_SUPPORTED, &wd->status)) {
>>>>> +		p = dev_add_param(&wd->dev, "running", NULL,
>>>>> +				  watchdog_get_running, PARAM_FLAG_RO);
>>>>> +		if (IS_ERR(p))
>>>>> +			return PTR_ERR(p);
>>>>> +
>>>>> +		p->value = (char *)wd;
>>>>> +	}
>>>>
>>>> How about adding this parameter unconditionally, with "unknown" as value
>>>> when WDOG_HW_RUNNING_SUPPORTED is not set. You can use
>>>> dev_add_param_enum() here.
>>
>> Thinking about it, I'd rather not use dev_add_param_enum here. I would like
>> to leave the driver-side API not too different from Linux, i.e. status bits
>> that can be set. If I use dev_add_param_enum, I'll have to add a new struct
>> member for every new status bit.
> 
> Not for every bit, but for every flag you want to export as a parameter.
> But where's the problem with that?

Ok, sent v3 with your suggestions incorporated.

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/4] watchdog: imxwd: support .running device parameter on i.MX2+

[Ahmad Fatoum]

The i.MX can be fused to start the watchdog on power-on reset.
To give users an easy way to determine whether the watchdog is running,
implement support for WDOG_HW_RUNNING.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  Use new WDOG_HW_RUNNING_SUPPORTED
---
 drivers/watchdog/imxwd.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/watchdog/imxwd.c b/drivers/watchdog/imxwd.c
index 77a3bd76cefa..e4a4b5ee71f5 100644
--- a/drivers/watchdog/imxwd.c
+++ b/drivers/watchdog/imxwd.c
@@ -28,6 +28,7 @@ struct imx_wd_ops {
 	int (*set_timeout)(struct imx_wd *, unsigned);
 	void (*soc_reset)(struct imx_wd *);
 	int (*init)(struct imx_wd *);
+	bool (*is_running)(struct imx_wd *);
 	unsigned int timeout_max;
 };
 
@@ -111,6 +112,11 @@ static void imx1_soc_reset(struct imx_wd *priv)
 	writew(IMX1_WDOG_WCR_WDE, priv->base + IMX1_WDOG_WCR);
 }
 
+static inline bool imx21_watchdog_is_running(struct imx_wd *priv)
+{
+	return imxwd_read(priv, IMX21_WDOG_WCR) & IMX21_WDOG_WCR_WDE;
+}
+
 static int imx21_watchdog_set_timeout(struct imx_wd *priv, unsigned timeout)
 {
 	u16 val;
@@ -243,6 +249,12 @@ static int imx_wd_probe(struct device_d *dev)
 						"fsl,ext-reset-output");
 
 	if (IS_ENABLED(CONFIG_WATCHDOG_IMX)) {
+		if (priv->ops->is_running) {
+			set_bit(WDOG_HW_RUNNING_SUPPORTED, &priv->wd.status);
+			if (priv->ops->is_running(priv))
+				set_bit(WDOG_HW_RUNNING, &priv->wd.status);
+		}
+
 		ret = watchdog_register(&priv->wd);
 		if (ret)
 			goto on_error;
@@ -277,6 +289,7 @@ static const struct imx_wd_ops imx21_wd_ops = {
 	.set_timeout = imx21_watchdog_set_timeout,
 	.soc_reset = imx21_soc_reset,
 	.init = imx21_wd_init,
+	.is_running = imx21_watchdog_is_running,
 	.timeout_max = 128,
 };
 
-- 
2.24.0.rc1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/4] watchdog: f71808e: support .running device parameter

[Ahmad Fatoum]

The American Megatrends BIOS I am using can be configured to start the
Fintek watchdog prior to the UEFI payloads. To avoid BIOS updates that reset
this functionality going unnoticed, implement support for WDOG_HW_RUNNING.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
v1 -> v2:
  Use new WDOG_HW_RUNNING_SUPPORTED
---
 drivers/watchdog/f71808e_wdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
index 4f881a1d02bc..bc17264619d2 100644
--- a/drivers/watchdog/f71808e_wdt.c
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -222,7 +222,7 @@ static int f71808e_wdt_init(struct f71808e_wdt *wd, struct device_d *dev)
 {
 	struct watchdog *wdd = &wd->wdd;
 	const char * const *names = pulse_width_names;
-	int wdt_conf;
+	unsigned long wdt_conf;
 	int ret;
 
 	superio_enter(wd->sioaddr);
@@ -262,6 +262,10 @@ static int f71808e_wdt_init(struct f71808e_wdt *wd, struct device_d *dev)
 
 	dev_info(dev, "reset reason: %s\n", reset_source_name());
 
+	set_bit(WDOG_HW_RUNNING_SUPPORTED, &wdd->status);
+	if (test_bit(F71808FG_FLAG_WD_EN, &wdt_conf))
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+
 	ret = watchdog_register(wdd);
 	if (ret)
 		return ret;
-- 
2.24.0.rc1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox