Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676184 > unrolled thread

[PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config Christopher Bostic <cbostic@linux.vnet.ibm.com> - 2017-06-27 23:20 +0200
    Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Guenter Roeck <linux@roeck-us.net> - 2017-06-28 13:40 +0200
      Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Christopher Bostic <cbostic@linux.vnet.ibm.com> - 2017-06-28 16:40 +0200
        Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Guenter Roeck <linux@roeck-us.net> - 2017-06-28 17:00 +0200
          Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Christopher Bostic <cbostic@linux.vnet.ibm.com> - 2017-06-28 17:10 +0200
            Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Guenter Roeck <linux@roeck-us.net> - 2017-06-28 17:10 +0200
              Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Christopher Bostic <cbostic@linux.vnet.ibm.com> - 2017-06-28 18:00 +0200
                Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Guenter Roeck <linux@roeck-us.net> - 2017-06-28 18:10 +0200
                  Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Christopher Bostic <cbostic@linux.vnet.ibm.com> - 2017-06-28 18:10 +0200
                    Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree  properties for config Guenter Roeck <linux@roeck-us.net> - 2017-06-28 20:00 +0200

#1676184 — [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

Reference the system device tree when configuring the watchdog
engines.  Set external signal mode on timeout if specified.
Set system reset on timeout if specified.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
v2 - Change of_get_property() to of_property_read_bool()
   - Remove redundant check for NULL struct device_node pointer
   - Optional property names now start with prefix 'aspeed,'
---
 drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 1c65258..71ce5f5 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 {
 	struct aspeed_wdt *wdt;
 	struct resource *res;
+	struct device_node *np;
 	int ret;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 	 * the SOC and not the full chip
 	 */
 	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
-		WDT_CTRL_1MHZ_CLK |
-		WDT_CTRL_RESET_SYSTEM;
+		WDT_CTRL_1MHZ_CLK;
+
+	np = pdev->dev.of_node;
+	if (of_property_read_bool(np, "aspeed,sys-reset"))
+		wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
+
+	if (of_property_read_bool(np, "aspeed,external-signal"))
+		wdt->ctrl |= WDT_CTRL_WDT_EXT;
+
+	writel(wdt->ctrl, wdt->base + WDT_CTRL);
 
 	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
 		aspeed_wdt_start(&wdt->wdd);
-- 
1.8.2.2

[toc] | [next] | [standalone]

#1676569 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On 06/27/2017 02:17 PM, Christopher Bostic wrote:
> Reference the system device tree when configuring the watchdog
> engines.  Set external signal mode on timeout if specified.
> Set system reset on timeout if specified.
> 
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
> v2 - Change of_get_property() to of_property_read_bool()
>     - Remove redundant check for NULL struct device_node pointer
>     - Optional property names now start with prefix 'aspeed,'
> ---
>   drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 1c65258..71ce5f5 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>   {
>   	struct aspeed_wdt *wdt;
>   	struct resource *res;
> +	struct device_node *np;
>   	int ret;
>   
>   	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>   	 * the SOC and not the full chip
>   	 */
>   	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> -		WDT_CTRL_1MHZ_CLK |
> -		WDT_CTRL_RESET_SYSTEM;
> +		WDT_CTRL_1MHZ_CLK;
> +
> +	np = pdev->dev.of_node;
> +	if (of_property_read_bool(np, "aspeed,sys-reset"))
> +		wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> +

For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
if no optional property is provided.

> +	if (of_property_read_bool(np, "aspeed,external-signal"))
> +		wdt->ctrl |= WDT_CTRL_WDT_EXT;
> +
> +	writel(wdt->ctrl, wdt->base + WDT_CTRL);
>   
>   	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>   		aspeed_wdt_start(&wdt->wdd);
> 

[toc] | [prev] | [next] | [standalone]

#1676729 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On 6/28/17 6:31 AM, Guenter Roeck wrote:
> On 06/27/2017 02:17 PM, Christopher Bostic wrote:
>> Reference the system device tree when configuring the watchdog
>> engines.  Set external signal mode on timeout if specified.
>> Set system reset on timeout if specified.
>>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>> v2 - Change of_get_property() to of_property_read_bool()
>>     - Remove redundant check for NULL struct device_node pointer
>>     - Optional property names now start with prefix 'aspeed,'
>> ---
>>   drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/watchdog/aspeed_wdt.c 
>> b/drivers/watchdog/aspeed_wdt.c
>> index 1c65258..71ce5f5 100644
>> --- a/drivers/watchdog/aspeed_wdt.c
>> +++ b/drivers/watchdog/aspeed_wdt.c
>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct 
>> platform_device *pdev)
>>   {
>>       struct aspeed_wdt *wdt;
>>       struct resource *res;
>> +    struct device_node *np;
>>       int ret;
>>         wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct 
>> platform_device *pdev)
>>        * the SOC and not the full chip
>>        */
>>       wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
>> -        WDT_CTRL_1MHZ_CLK |
>> -        WDT_CTRL_RESET_SYSTEM;
>> +        WDT_CTRL_1MHZ_CLK;
>> +
>> +    np = pdev->dev.of_node;
>> +    if (of_property_read_bool(np, "aspeed,sys-reset"))
>> +        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
>> +
>
> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
> if no optional property is provided.
>
I had the logic inverted for this property in a previous patch.  The 
property was 'no-system-reset' so that when not present the default was 
to set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to 
indicate that no system reset is to be done is to not specify the 
property.  No system reset is desired under circumstances when another 
wdt engine is to be responsible for this.   Given the issue with 
backward compatibility that's not a solution.  Given this, would 
creating a property 'no-system-reset' be acceptable?

Thanks,
Chris

>> +    if (of_property_read_bool(np, "aspeed,external-signal"))
>> +        wdt->ctrl |= WDT_CTRL_WDT_EXT;
>> +
>> +    writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>         if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>>           aspeed_wdt_start(&wdt->wdd);
>>
>

[toc] | [prev] | [next] | [standalone]

#1676751 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote:
> 
> 
> On 6/28/17 6:31 AM, Guenter Roeck wrote:
> >On 06/27/2017 02:17 PM, Christopher Bostic wrote:
> >>Reference the system device tree when configuring the watchdog
> >>engines.  Set external signal mode on timeout if specified.
> >>Set system reset on timeout if specified.
> >>
> >>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >>---
> >>v2 - Change of_get_property() to of_property_read_bool()
> >>    - Remove redundant check for NULL struct device_node pointer
> >>    - Optional property names now start with prefix 'aspeed,'
> >>---
> >>  drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
> >>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/watchdog/aspeed_wdt.c
> >>b/drivers/watchdog/aspeed_wdt.c
> >>index 1c65258..71ce5f5 100644
> >>--- a/drivers/watchdog/aspeed_wdt.c
> >>+++ b/drivers/watchdog/aspeed_wdt.c
> >>@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device
> >>*pdev)
> >>  {
> >>      struct aspeed_wdt *wdt;
> >>      struct resource *res;
> >>+    struct device_node *np;
> >>      int ret;
> >>        wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> >>@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device
> >>*pdev)
> >>       * the SOC and not the full chip
> >>       */
> >>      wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> >>-        WDT_CTRL_1MHZ_CLK |
> >>-        WDT_CTRL_RESET_SYSTEM;
> >>+        WDT_CTRL_1MHZ_CLK;
> >>+
> >>+    np = pdev->dev.of_node;
> >>+    if (of_property_read_bool(np, "aspeed,sys-reset"))
> >>+        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> >>+
> >
> >For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
> >if no optional property is provided.
> >
> I had the logic inverted for this property in a previous patch.  The
> property was 'no-system-reset' so that when not present the default was to
> set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to
> indicate that no system reset is to be done is to not specify the property.
> No system reset is desired under circumstances when another wdt engine is to
> be responsible for this.   Given the issue with backward compatibility
> that's not a solution.  Given this, would creating a property
> 'no-system-reset' be acceptable?
> 
Sorry, I fail to see the problem. There are half a dozen properties. What is the
problem with having a default if no property is specified ? Your default is "do
nothing", which does not really make any sense to me. If the user wants the
watchdog to do nothing, the simple means to accomplish that would be to not
instantiate it.

Sure, that means specifying "system-reset" is redundant, but I don't see a
problem with that either. But I do see a problem with loading a watchdog driver
that doesn't do anything.

If there is really some use case where it makes sense to load a watchdog driver
and have it do nothing, please explain and provide a respective devicetree
property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but
at least makes it obvious that the driver isn't doing anything besides
creating a false sense of "the system is watchdog protected".

Thanks,
Guenter


> Thanks,
> Chris
> 
> >>+    if (of_property_read_bool(np, "aspeed,external-signal"))
> >>+        wdt->ctrl |= WDT_CTRL_WDT_EXT;
> >>+
> >>+    writel(wdt->ctrl, wdt->base + WDT_CTRL);
> >>        if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> >>          aspeed_wdt_start(&wdt->wdd);
> >>
> >
> 

[toc] | [prev] | [next] | [standalone]

#1676760 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On 6/28/17 9:54 AM, Guenter Roeck wrote:
> On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote:
>>
>> On 6/28/17 6:31 AM, Guenter Roeck wrote:
>>> On 06/27/2017 02:17 PM, Christopher Bostic wrote:
>>>> Reference the system device tree when configuring the watchdog
>>>> engines.  Set external signal mode on timeout if specified.
>>>> Set system reset on timeout if specified.
>>>>
>>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>>>> ---
>>>> v2 - Change of_get_property() to of_property_read_bool()
>>>>     - Remove redundant check for NULL struct device_node pointer
>>>>     - Optional property names now start with prefix 'aspeed,'
>>>> ---
>>>>   drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/aspeed_wdt.c
>>>> b/drivers/watchdog/aspeed_wdt.c
>>>> index 1c65258..71ce5f5 100644
>>>> --- a/drivers/watchdog/aspeed_wdt.c
>>>> +++ b/drivers/watchdog/aspeed_wdt.c
>>>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device
>>>> *pdev)
>>>>   {
>>>>       struct aspeed_wdt *wdt;
>>>>       struct resource *res;
>>>> +    struct device_node *np;
>>>>       int ret;
>>>>         wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device
>>>> *pdev)
>>>>        * the SOC and not the full chip
>>>>        */
>>>>       wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
>>>> -        WDT_CTRL_1MHZ_CLK |
>>>> -        WDT_CTRL_RESET_SYSTEM;
>>>> +        WDT_CTRL_1MHZ_CLK;
>>>> +
>>>> +    np = pdev->dev.of_node;
>>>> +    if (of_property_read_bool(np, "aspeed,sys-reset"))
>>>> +        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
>>>> +
>>> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
>>> if no optional property is provided.
>>>
>> I had the logic inverted for this property in a previous patch.  The
>> property was 'no-system-reset' so that when not present the default was to
>> set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to
>> indicate that no system reset is to be done is to not specify the property.
>> No system reset is desired under circumstances when another wdt engine is to
>> be responsible for this.   Given the issue with backward compatibility
>> that's not a solution.  Given this, would creating a property
>> 'no-system-reset' be acceptable?
>>
> Sorry, I fail to see the problem. There are half a dozen properties. What is the
> problem with having a default if no property is specified ? Your default is "do
> nothing", which does not really make any sense to me. If the user wants the
> watchdog to do nothing, the simple means to accomplish that would be to not
> instantiate it.
>
> Sure, that means specifying "system-reset" is redundant, but I don't see a
> problem with that either. But I do see a problem with loading a watchdog driver
> that doesn't do anything.
>
> If there is really some use case where it makes sense to load a watchdog driver
> and have it do nothing, please explain and provide a respective devicetree
> property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but
> at least makes it obvious that the driver isn't doing anything besides
> creating a false sense of "the system is watchdog protected".
If system-reset is not wanted there are other properties that might 
still be needed, for example
ARM reset only.   We'd still want to instantiate it.

Thanks,
Chris
>
> Thanks,
> Guenter
>
>
>> Thanks,
>> Chris
>>
>>>> +    if (of_property_read_bool(np, "aspeed,external-signal"))
>>>> +        wdt->ctrl |= WDT_CTRL_WDT_EXT;
>>>> +
>>>> +    writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>>>         if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>>>>           aspeed_wdt_start(&wdt->wdd);
>>>>

[toc] | [prev] | [next] | [standalone]

#1676761 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote:
> 
> 
> On 6/28/17 9:54 AM, Guenter Roeck wrote:
> >On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote:
> >>
> >>On 6/28/17 6:31 AM, Guenter Roeck wrote:
> >>>On 06/27/2017 02:17 PM, Christopher Bostic wrote:
> >>>>Reference the system device tree when configuring the watchdog
> >>>>engines.  Set external signal mode on timeout if specified.
> >>>>Set system reset on timeout if specified.
> >>>>
> >>>>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >>>>---
> >>>>v2 - Change of_get_property() to of_property_read_bool()
> >>>>    - Remove redundant check for NULL struct device_node pointer
> >>>>    - Optional property names now start with prefix 'aspeed,'
> >>>>---
> >>>>  drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
> >>>>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/drivers/watchdog/aspeed_wdt.c
> >>>>b/drivers/watchdog/aspeed_wdt.c
> >>>>index 1c65258..71ce5f5 100644
> >>>>--- a/drivers/watchdog/aspeed_wdt.c
> >>>>+++ b/drivers/watchdog/aspeed_wdt.c
> >>>>@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device
> >>>>*pdev)
> >>>>  {
> >>>>      struct aspeed_wdt *wdt;
> >>>>      struct resource *res;
> >>>>+    struct device_node *np;
> >>>>      int ret;
> >>>>        wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> >>>>@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device
> >>>>*pdev)
> >>>>       * the SOC and not the full chip
> >>>>       */
> >>>>      wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> >>>>-        WDT_CTRL_1MHZ_CLK |
> >>>>-        WDT_CTRL_RESET_SYSTEM;
> >>>>+        WDT_CTRL_1MHZ_CLK;
> >>>>+
> >>>>+    np = pdev->dev.of_node;
> >>>>+    if (of_property_read_bool(np, "aspeed,sys-reset"))
> >>>>+        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> >>>>+
> >>>For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
> >>>if no optional property is provided.
> >>>
> >>I had the logic inverted for this property in a previous patch.  The
> >>property was 'no-system-reset' so that when not present the default was to
> >>set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to
> >>indicate that no system reset is to be done is to not specify the property.
> >>No system reset is desired under circumstances when another wdt engine is to
> >>be responsible for this.   Given the issue with backward compatibility
> >>that's not a solution.  Given this, would creating a property
> >>'no-system-reset' be acceptable?
> >>
> >Sorry, I fail to see the problem. There are half a dozen properties. What is the
> >problem with having a default if no property is specified ? Your default is "do
> >nothing", which does not really make any sense to me. If the user wants the
> >watchdog to do nothing, the simple means to accomplish that would be to not
> >instantiate it.
> >
> >Sure, that means specifying "system-reset" is redundant, but I don't see a
> >problem with that either. But I do see a problem with loading a watchdog driver
> >that doesn't do anything.
> >
> >If there is really some use case where it makes sense to load a watchdog driver
> >and have it do nothing, please explain and provide a respective devicetree
> >property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but
> >at least makes it obvious that the driver isn't doing anything besides
> >creating a false sense of "the system is watchdog protected".
> If system-reset is not wanted there are other properties that might still be
> needed, for example
> ARM reset only.   We'd still want to instantiate it.
> 
But then you would presumably specify that property, so there would be one.
The question here is what to do if _no_ property is provided.

Sorry, I don't get your point.

Guenter

> Thanks,
> Chris
> >
> >Thanks,
> >Guenter
> >
> >
> >>Thanks,
> >>Chris
> >>
> >>>>+    if (of_property_read_bool(np, "aspeed,external-signal"))
> >>>>+        wdt->ctrl |= WDT_CTRL_WDT_EXT;
> >>>>+
> >>>>+    writel(wdt->ctrl, wdt->base + WDT_CTRL);
> >>>>        if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> >>>>          aspeed_wdt_start(&wdt->wdd);
> >>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1676874 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On 6/28/17 10:08 AM, Guenter Roeck wrote:
> On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote:
>>
>> On 6/28/17 9:54 AM, Guenter Roeck wrote:
>>> On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote:
>>>> On 6/28/17 6:31 AM, Guenter Roeck wrote:
>>>>> On 06/27/2017 02:17 PM, Christopher Bostic wrote:
>>>>>> Reference the system device tree when configuring the watchdog
>>>>>> engines.  Set external signal mode on timeout if specified.
>>>>>> Set system reset on timeout if specified.
>>>>>>
>>>>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>>>>>> ---
>>>>>> v2 - Change of_get_property() to of_property_read_bool()
>>>>>>     - Remove redundant check for NULL struct device_node pointer
>>>>>>     - Optional property names now start with prefix 'aspeed,'
>>>>>> ---
>>>>>>   drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>>>>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/watchdog/aspeed_wdt.c
>>>>>> b/drivers/watchdog/aspeed_wdt.c
>>>>>> index 1c65258..71ce5f5 100644
>>>>>> --- a/drivers/watchdog/aspeed_wdt.c
>>>>>> +++ b/drivers/watchdog/aspeed_wdt.c
>>>>>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device
>>>>>> *pdev)
>>>>>>   {
>>>>>>       struct aspeed_wdt *wdt;
>>>>>>       struct resource *res;
>>>>>> +    struct device_node *np;
>>>>>>       int ret;
>>>>>>         wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>>>>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device
>>>>>> *pdev)
>>>>>>        * the SOC and not the full chip
>>>>>>        */
>>>>>>       wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
>>>>>> -        WDT_CTRL_1MHZ_CLK |
>>>>>> -        WDT_CTRL_RESET_SYSTEM;
>>>>>> +        WDT_CTRL_1MHZ_CLK;
>>>>>> +
>>>>>> +    np = pdev->dev.of_node;
>>>>>> +    if (of_property_read_bool(np, "aspeed,sys-reset"))
>>>>>> +        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
>>>>>> +
>>>>> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
>>>>> if no optional property is provided.
>>>>>
>>>> I had the logic inverted for this property in a previous patch.  The
>>>> property was 'no-system-reset' so that when not present the default was to
>>>> set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to
>>>> indicate that no system reset is to be done is to not specify the property.
>>>> No system reset is desired under circumstances when another wdt engine is to
>>>> be responsible for this.   Given the issue with backward compatibility
>>>> that's not a solution.  Given this, would creating a property
>>>> 'no-system-reset' be acceptable?
>>>>
>>> Sorry, I fail to see the problem. There are half a dozen properties. What is the
>>> problem with having a default if no property is specified ? Your default is "do
>>> nothing", which does not really make any sense to me. If the user wants the
>>> watchdog to do nothing, the simple means to accomplish that would be to not
>>> instantiate it.
>>>
>>> Sure, that means specifying "system-reset" is redundant, but I don't see a
>>> problem with that either. But I do see a problem with loading a watchdog driver
>>> that doesn't do anything.
>>>
>>> If there is really some use case where it makes sense to load a watchdog driver
>>> and have it do nothing, please explain and provide a respective devicetree
>>> property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but
>>> at least makes it obvious that the driver isn't doing anything besides
>>> creating a false sense of "the system is watchdog protected".
>> If system-reset is not wanted there are other properties that might still be
>> needed, for example
>> ARM reset only.   We'd still want to instantiate it.
>>
> But then you would presumably specify that property, so there would be one.
> The question here is what to do if _no_ property is provided.
I agree that the default should be to perform a system reset  when no 
property is provided.  I will make that change.   Additionally for 
backwards compatibility the SOC reset should be enabled by default when 
no property is provided.   The other parameters are not configured in 
the watchdog control register if no property is provided.

There is still a need to to configure for no system reset.  For example, 
optional parameter 'aspeed,no-sys-reset' that can be specified if system 
reset is to be disabled.  In addition the default 'SOC reset enabled' 
behavior could be disabled with an optional parameter 'aspeed,no-soc-reset'.

Why would there even be a need configure for no system reset?  The 
external mode 'wdt ext: External signal enable after timeout' can be 
sent off chip even if system reset is disabled.  The watchdog is doing 
something in this case and would require wdt instantiation.

Hope this is more clear, sorry for the confusion.

Thanks,
-Chris AST2500 Software Programming Guide
> Sorry, I don't get your point.
>
> Guenter
>
>> Thanks,
>> Chris
>>> Thanks,
>>> Guenter
>>>
>>>
>>>> Thanks,
>>>> Chris
>>>>
>>>>>> +    if (of_property_read_bool(np, "aspeed,external-signal"))
>>>>>> +        wdt->ctrl |= WDT_CTRL_WDT_EXT;
>>>>>> +
>>>>>> +    writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>>>>>         if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>>>>>>           aspeed_wdt_start(&wdt->wdd);
>>>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1676886 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On Wed, Jun 28, 2017 at 10:55:08AM -0500, Christopher Bostic wrote:
> 
> 
> On 6/28/17 10:08 AM, Guenter Roeck wrote:
> >On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote:
> >>
> >>On 6/28/17 9:54 AM, Guenter Roeck wrote:
> >>>On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote:
> >>>>On 6/28/17 6:31 AM, Guenter Roeck wrote:
> >>>>>On 06/27/2017 02:17 PM, Christopher Bostic wrote:
> >>>>>>Reference the system device tree when configuring the watchdog
> >>>>>>engines.  Set external signal mode on timeout if specified.
> >>>>>>Set system reset on timeout if specified.
> >>>>>>
> >>>>>>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >>>>>>---
> >>>>>>v2 - Change of_get_property() to of_property_read_bool()
> >>>>>>    - Remove redundant check for NULL struct device_node pointer
> >>>>>>    - Optional property names now start with prefix 'aspeed,'
> >>>>>>---
> >>>>>>  drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
> >>>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>>diff --git a/drivers/watchdog/aspeed_wdt.c
> >>>>>>b/drivers/watchdog/aspeed_wdt.c
> >>>>>>index 1c65258..71ce5f5 100644
> >>>>>>--- a/drivers/watchdog/aspeed_wdt.c
> >>>>>>+++ b/drivers/watchdog/aspeed_wdt.c
> >>>>>>@@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device
> >>>>>>*pdev)
> >>>>>>  {
> >>>>>>      struct aspeed_wdt *wdt;
> >>>>>>      struct resource *res;
> >>>>>>+    struct device_node *np;
> >>>>>>      int ret;
> >>>>>>        wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> >>>>>>@@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device
> >>>>>>*pdev)
> >>>>>>       * the SOC and not the full chip
> >>>>>>       */
> >>>>>>      wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> >>>>>>-        WDT_CTRL_1MHZ_CLK |
> >>>>>>-        WDT_CTRL_RESET_SYSTEM;
> >>>>>>+        WDT_CTRL_1MHZ_CLK;
> >>>>>>+
> >>>>>>+    np = pdev->dev.of_node;
> >>>>>>+    if (of_property_read_bool(np, "aspeed,sys-reset"))
> >>>>>>+        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> >>>>>>+
> >>>>>For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
> >>>>>if no optional property is provided.
> >>>>>
> >>>>I had the logic inverted for this property in a previous patch.  The
> >>>>property was 'no-system-reset' so that when not present the default was to
> >>>>set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to
> >>>>indicate that no system reset is to be done is to not specify the property.
> >>>>No system reset is desired under circumstances when another wdt engine is to
> >>>>be responsible for this.   Given the issue with backward compatibility
> >>>>that's not a solution.  Given this, would creating a property
> >>>>'no-system-reset' be acceptable?
> >>>>
> >>>Sorry, I fail to see the problem. There are half a dozen properties. What is the
> >>>problem with having a default if no property is specified ? Your default is "do
> >>>nothing", which does not really make any sense to me. If the user wants the
> >>>watchdog to do nothing, the simple means to accomplish that would be to not
> >>>instantiate it.
> >>>
> >>>Sure, that means specifying "system-reset" is redundant, but I don't see a
> >>>problem with that either. But I do see a problem with loading a watchdog driver
> >>>that doesn't do anything.
> >>>
> >>>If there is really some use case where it makes sense to load a watchdog driver
> >>>and have it do nothing, please explain and provide a respective devicetree
> >>>property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but
> >>>at least makes it obvious that the driver isn't doing anything besides
> >>>creating a false sense of "the system is watchdog protected".
> >>If system-reset is not wanted there are other properties that might still be
> >>needed, for example
> >>ARM reset only.   We'd still want to instantiate it.
> >>
> >But then you would presumably specify that property, so there would be one.
> >The question here is what to do if _no_ property is provided.
> I agree that the default should be to perform a system reset  when no
> property is provided.  I will make that change.   Additionally for backwards
> compatibility the SOC reset should be enabled by default when no property is
> provided.   The other parameters are not configured in the watchdog control
> register if no property is provided.
> 
> There is still a need to to configure for no system reset.  For example,
> optional parameter 'aspeed,no-sys-reset' that can be specified if system
> reset is to be disabled.  In addition the default 'SOC reset enabled'
> behavior could be disabled with an optional parameter 'aspeed,no-soc-reset'.
> 
> Why would there even be a need configure for no system reset?  The external
> mode 'wdt ext: External signal enable after timeout' can be sent off chip
> even if system reset is disabled.  The watchdog is doing something in this
> case and would require wdt instantiation.
> 

Wouldn't that be "aspeed,external-signal" ?

Guenter

> Hope this is more clear, sorry for the confusion.
> 
> Thanks,
> -Chris AST2500 Software Programming Guide
> >Sorry, I don't get your point.
> >
> >Guenter
> >
> >>Thanks,
> >>Chris
> >>>Thanks,
> >>>Guenter
> >>>
> >>>
> >>>>Thanks,
> >>>>Chris
> >>>>
> >>>>>>+    if (of_property_read_bool(np, "aspeed,external-signal"))
> >>>>>>+        wdt->ctrl |= WDT_CTRL_WDT_EXT;
> >>>>>>+
> >>>>>>+    writel(wdt->ctrl, wdt->base + WDT_CTRL);
> >>>>>>        if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> >>>>>>          aspeed_wdt_start(&wdt->wdd);
> >>>>>>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1676891 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On 6/28/17 11:06 AM, Guenter Roeck wrote:
> On Wed, Jun 28, 2017 at 10:55:08AM -0500, Christopher Bostic wrote:
>>
>> On 6/28/17 10:08 AM, Guenter Roeck wrote:
>>> On Wed, Jun 28, 2017 at 09:59:22AM -0500, Christopher Bostic wrote:
>>>> On 6/28/17 9:54 AM, Guenter Roeck wrote:
>>>>> On Wed, Jun 28, 2017 at 09:29:50AM -0500, Christopher Bostic wrote:
>>>>>> On 6/28/17 6:31 AM, Guenter Roeck wrote:
>>>>>>> On 06/27/2017 02:17 PM, Christopher Bostic wrote:
>>>>>>>> Reference the system device tree when configuring the watchdog
>>>>>>>> engines.  Set external signal mode on timeout if specified.
>>>>>>>> Set system reset on timeout if specified.
>>>>>>>>
>>>>>>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>>>>>>>> ---
>>>>>>>> v2 - Change of_get_property() to of_property_read_bool()
>>>>>>>>     - Remove redundant check for NULL struct device_node pointer
>>>>>>>>     - Optional property names now start with prefix 'aspeed,'
>>>>>>>> ---
>>>>>>>>   drivers/watchdog/aspeed_wdt.c | 13 +++++++++++--
>>>>>>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/watchdog/aspeed_wdt.c
>>>>>>>> b/drivers/watchdog/aspeed_wdt.c
>>>>>>>> index 1c65258..71ce5f5 100644
>>>>>>>> --- a/drivers/watchdog/aspeed_wdt.c
>>>>>>>> +++ b/drivers/watchdog/aspeed_wdt.c
>>>>>>>> @@ -140,6 +140,7 @@ static int aspeed_wdt_probe(struct platform_device
>>>>>>>> *pdev)
>>>>>>>>   {
>>>>>>>>       struct aspeed_wdt *wdt;
>>>>>>>>       struct resource *res;
>>>>>>>> +    struct device_node *np;
>>>>>>>>       int ret;
>>>>>>>>         wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>>>>>>> @@ -170,8 +171,16 @@ static int aspeed_wdt_probe(struct platform_device
>>>>>>>> *pdev)
>>>>>>>>        * the SOC and not the full chip
>>>>>>>>        */
>>>>>>>>       wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
>>>>>>>> -        WDT_CTRL_1MHZ_CLK |
>>>>>>>> -        WDT_CTRL_RESET_SYSTEM;
>>>>>>>> +        WDT_CTRL_1MHZ_CLK;
>>>>>>>> +
>>>>>>>> +    np = pdev->dev.of_node;
>>>>>>>> +    if (of_property_read_bool(np, "aspeed,sys-reset"))
>>>>>>>> +        wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
>>>>>>>> +
>>>>>>> For backward compatibility, this should default to WDT_CTRL_RESET_SYSTEM
>>>>>>> if no optional property is provided.
>>>>>>>
>>>>>> I had the logic inverted for this property in a previous patch.  The
>>>>>> property was 'no-system-reset' so that when not present the default was to
>>>>>> set WDT_CTRL_RESET_SYSTEM.   As it is in this patch, the only way to
>>>>>> indicate that no system reset is to be done is to not specify the property.
>>>>>> No system reset is desired under circumstances when another wdt engine is to
>>>>>> be responsible for this.   Given the issue with backward compatibility
>>>>>> that's not a solution.  Given this, would creating a property
>>>>>> 'no-system-reset' be acceptable?
>>>>>>
>>>>> Sorry, I fail to see the problem. There are half a dozen properties. What is the
>>>>> problem with having a default if no property is specified ? Your default is "do
>>>>> nothing", which does not really make any sense to me. If the user wants the
>>>>> watchdog to do nothing, the simple means to accomplish that would be to not
>>>>> instantiate it.
>>>>>
>>>>> Sure, that means specifying "system-reset" is redundant, but I don't see a
>>>>> problem with that either. But I do see a problem with loading a watchdog driver
>>>>> that doesn't do anything.
>>>>>
>>>>> If there is really some use case where it makes sense to load a watchdog driver
>>>>> and have it do nothing, please explain and provide a respective devicetree
>>>>> property. "aspeed,do-nothing" (or whatever similar) doesn't sound good, but
>>>>> at least makes it obvious that the driver isn't doing anything besides
>>>>> creating a false sense of "the system is watchdog protected".
>>>> If system-reset is not wanted there are other properties that might still be
>>>> needed, for example
>>>> ARM reset only.   We'd still want to instantiate it.
>>>>
>>> But then you would presumably specify that property, so there would be one.
>>> The question here is what to do if _no_ property is provided.
>> I agree that the default should be to perform a system reset  when no
>> property is provided.  I will make that change.   Additionally for backwards
>> compatibility the SOC reset should be enabled by default when no property is
>> provided.   The other parameters are not configured in the watchdog control
>> register if no property is provided.
>>
>> There is still a need to to configure for no system reset.  For example,
>> optional parameter 'aspeed,no-sys-reset' that can be specified if system
>> reset is to be disabled.  In addition the default 'SOC reset enabled'
>> behavior could be disabled with an optional parameter 'aspeed,no-soc-reset'.
>>
>> Why would there even be a need configure for no system reset?  The external
>> mode 'wdt ext: External signal enable after timeout' can be sent off chip
>> even if system reset is disabled.  The watchdog is doing something in this
>> case and would require wdt instantiation.
>>
> Wouldn't that be "aspeed,external-signal" ?
Yes that optional parameter would need to be specified in that 
situation, as well as 'aspeed,no-system-reset'.

Chris
>
> Guenter
>
>> Hope this is more clear, sorry for the confusion.
>>
>> Thanks,
>> -Chris AST2500 Software Programming Guide
>>> Sorry, I don't get your point.
>>>
>>> Guenter
>>>
>>>> Thanks,
>>>> Chris
>>>>> Thanks,
>>>>> Guenter
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Chris
>>>>>>
>>>>>>>> +    if (of_property_read_bool(np, "aspeed,external-signal"))
>>>>>>>> +        wdt->ctrl |= WDT_CTRL_WDT_EXT;
>>>>>>>> +
>>>>>>>> +    writel(wdt->ctrl, wdt->base + WDT_CTRL);
>>>>>>>>         if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
>>>>>>>>           aspeed_wdt_start(&wdt->wdd);
>>>>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1676930 — Re: [PATCH v2 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

On Wed, Jun 28, 2017 at 11:09:15AM -0500, Christopher Bostic wrote:
> 
> >>
> >>Why would there even be a need configure for no system reset?  The external
> >>mode 'wdt ext: External signal enable after timeout' can be sent off chip
> >>even if system reset is disabled.  The watchdog is doing something in this
> >>case and would require wdt instantiation.
> >>
> >Wouldn't that be "aspeed,external-signal" ?
> Yes that optional parameter would need to be specified in that situation, as
> well as 'aspeed,no-system-reset'.
> 

Ok, makes sense (after looking into the datasheet). The missing detail
is that there is a separate configuration bit for "Reset system after
timeout", which is set in addition to reset_{sys,soc,arm}.

Thanks,
Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web