[PATCH 07/10] watchdog: xilinx: Fix OF binding

[PATCH 07/10] watchdog: xilinx: Fix OF binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Use of_property_read_u32 functions to clean OF probing.

Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---

 drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
index c229cc4..475440a6 100644
--- a/drivers/watchdog/of_xilinx_wdt.c
+++ b/drivers/watchdog/of_xilinx_wdt.c
@@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
 static int xwdt_probe(struct platform_device *pdev)
 {
 	int rc;
-	u32 *tmptr;
-	u32 *pfreq;
+	u32 pfreq, enable_once;
 	struct resource *res;
 	struct xwdt_device *xdev;
 	bool no_timeout = false;
@@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(xdev->base))
 		return PTR_ERR(xdev->base);

-	pfreq = (u32 *)of_get_property(pdev->dev.of_node,
-					"clock-frequency", NULL);
-
-	if (pfreq == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
+	if (rc) {
 		dev_warn(&pdev->dev,
 			 "The watchdog clock frequency cannot be obtained\n");
 		no_timeout = true;
 	}

-	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
-					"xlnx,wdt-interval", NULL);
-	if (tmptr == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
+				  &xdev->wdt_interval);
+	if (rc) {
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-interval\" not found\n");
 		no_timeout = true;
-	} else {
-		xdev->wdt_interval = *tmptr;
 	}

-	tmptr = (u32 *)of_get_property(pdev->dev.of_node,
-					"xlnx,wdt-enable-once", NULL);
-	if (tmptr == NULL) {
+	rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
+				  &enable_once);
+	if (!rc && enable_once) {
 		dev_warn(&pdev->dev,
 			 "Parameter \"xlnx,wdt-enable-once\" not found\n");
 		watchdog_set_nowayout(xilinx_wdt_wdd, true);
@@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev)
  */
 	if (!no_timeout)
 		xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
-					  *pfreq);
+					  pfreq);

 	spin_lock_init(&xdev->spinlock);
 	watchdog_set_drvdata(xilinx_wdt_wdd, xdev);
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 09/10] watchdog: xilinx: Add missing binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

Document current driver binding.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 .../devicetree/bindings/watchdog/of-xilinx-wdt.txt | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
new file mode 100644
index 0000000..6d63782
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
@@ -0,0 +1,23 @@
+Xilinx AXI/PLB soft-core watchdog Device Tree Bindings
+---------------------------------------------------------
+
+Required properties:
+- compatible		: Should be "xlnx,xps-timebase-wdt-1.00.a" or
+			  "xlnx,xps-timebase-wdt-1.01.a".
+- reg			: Physical base address and size
+
+Optional properties:
+- clock-frequency	: Frequency of clock in Hz
+- xlnx,wdt-enable-once	: 0 - Watchdog can be restarted
+			  1 - Watchdog can be enabled just once
+- xlnx,wdt-interval	: Watchdog timeout interval in 2^<val> clock cycles,
+			  <val> is integer from 8 to 31.
+
+Example:
+axi-timebase-wdt@40100000 {
+	clock-frequency = <50000000>;
+	compatible = "xlnx,xps-timebase-wdt-1.00.a";
+	reg = <0x40100000 0x10000>;
+	xlnx,wdt-enable-once = <0x0>;
+	xlnx,wdt-interval = <0x1b>;
+} ;
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 07/10] watchdog: xilinx: Fix OF binding

[Rob Herring]

On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> Use of_property_read_u32 functions to clean OF probing.

The subject is a bit misleading as this doesn't really fix anything.

>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
> index c229cc4..475440a6 100644
> --- a/drivers/watchdog/of_xilinx_wdt.c
> +++ b/drivers/watchdog/of_xilinx_wdt.c
> @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
>  static int xwdt_probe(struct platform_device *pdev)
>  {
>         int rc;
> -       u32 *tmptr;
> -       u32 *pfreq;
> +       u32 pfreq, enable_once;
>         struct resource *res;
>         struct xwdt_device *xdev;
>         bool no_timeout = false;
> @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev)
>         if (IS_ERR(xdev->base))
>                 return PTR_ERR(xdev->base);
>
> -       pfreq = (u32 *)of_get_property(pdev->dev.of_node,
> -                                       "clock-frequency", NULL);
> -
> -       if (pfreq == NULL) {
> +       rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
> +       if (rc) {
>                 dev_warn(&pdev->dev,
>                          "The watchdog clock frequency cannot be obtained\n");
>                 no_timeout = true;

You can kill this...

>         }
>
> -       tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> -                                       "xlnx,wdt-interval", NULL);
> -       if (tmptr == NULL) {
> +       rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
> +                                 &xdev->wdt_interval);
> +       if (rc) {
>                 dev_warn(&pdev->dev,
>                          "Parameter \"xlnx,wdt-interval\" not found\n");
>                 no_timeout = true;

and this...

> -       } else {
> -               xdev->wdt_interval = *tmptr;
>         }
>
> -       tmptr = (u32 *)of_get_property(pdev->dev.of_node,
> -                                       "xlnx,wdt-enable-once", NULL);
> -       if (tmptr == NULL) {
> +       rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
> +                                 &enable_once);
> +       if (!rc && enable_once) {
>                 dev_warn(&pdev->dev,
>                          "Parameter \"xlnx,wdt-enable-once\" not found\n");
>                 watchdog_set_nowayout(xilinx_wdt_wdd, true);
> @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev)
>   */
>         if (!no_timeout)

and use "if (pfreq && xdev->wdt_interval)" if you initialize pfreq to 0.

>                 xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
> -                                         *pfreq);
> +                                         pfreq);

Is the wdog really usable if the timeout properties are missing? Seems
like you should fail to probe rather than warn.

Rob

Re: [PATCH 07/10] watchdog: xilinx: Fix OF binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 4214 bytes --]

On 01/31/2014 06:33 PM, Rob Herring wrote:
> On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>> Use of_property_read_u32 functions to clean OF probing.
> 
> The subject is a bit misleading as this doesn't really fix anything.

fair enough. Will change it.

> 
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>  drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++---------------
>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
>> index c229cc4..475440a6 100644
>> --- a/drivers/watchdog/of_xilinx_wdt.c
>> +++ b/drivers/watchdog/of_xilinx_wdt.c
>> @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
>>  static int xwdt_probe(struct platform_device *pdev)
>>  {
>>         int rc;
>> -       u32 *tmptr;
>> -       u32 *pfreq;
>> +       u32 pfreq, enable_once;
>>         struct resource *res;
>>         struct xwdt_device *xdev;
>>         bool no_timeout = false;
>> @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev)
>>         if (IS_ERR(xdev->base))
>>                 return PTR_ERR(xdev->base);
>>
>> -       pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>> -                                       "clock-frequency", NULL);
>> -
>> -       if (pfreq == NULL) {
>> +       rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
>> +       if (rc) {
>>                 dev_warn(&pdev->dev,
>>                          "The watchdog clock frequency cannot be obtained\n");
>>                 no_timeout = true;
> 
> You can kill this...
> 
>>         }
>>
>> -       tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>> -                                       "xlnx,wdt-interval", NULL);
>> -       if (tmptr == NULL) {
>> +       rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>> +                                 &xdev->wdt_interval);
>> +       if (rc) {
>>                 dev_warn(&pdev->dev,
>>                          "Parameter \"xlnx,wdt-interval\" not found\n");
>>                 no_timeout = true;
> 
> and this...
> 
>> -       } else {
>> -               xdev->wdt_interval = *tmptr;
>>         }
>>
>> -       tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>> -                                       "xlnx,wdt-enable-once", NULL);
>> -       if (tmptr == NULL) {
>> +       rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>> +                                 &enable_once);
>> +       if (!rc && enable_once) {
>>                 dev_warn(&pdev->dev,
>>                          "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>                 watchdog_set_nowayout(xilinx_wdt_wdd, true);
>> @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev)
>>   */
>>         if (!no_timeout)
> 
> and use "if (pfreq && xdev->wdt_interval)" if you initialize pfreq to 0.


I just wanted to to change functions not logic in the driver.
But it can be changed too.

>>                 xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
>> -                                         *pfreq);
>> +                                         pfreq);
> 
> Is the wdog really usable if the timeout properties are missing? Seems
> like you should fail to probe rather than warn.

There are 2 things.
1. timeout - you don't need pfreq and wdt_interval to use this driver
but for that there should be module parameter timeout there.
It should be added.

2. about warn - based on 1 I don't think driver should failed
but I am looking at logic above which I have added there but should be different.

u32 enable_once = 0;
if (!rc)
	dev_warn

if (enable_once)
	watchdog_set_nowayout(xilinx_wdt_wdd, true);

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 07/10] watchdog: xilinx: Fix OF binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 4391 bytes --]

On 02/03/2014 08:59 AM, Michal Simek wrote:
> On 01/31/2014 06:33 PM, Rob Herring wrote:
>> On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
>>> Use of_property_read_u32 functions to clean OF probing.
>>
>> The subject is a bit misleading as this doesn't really fix anything.
> 
> fair enough. Will change it.
> 
>>
>>>
>>> Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>
>>>  drivers/watchdog/of_xilinx_wdt.c | 25 ++++++++++---------------
>>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/of_xilinx_wdt.c b/drivers/watchdog/of_xilinx_wdt.c
>>> index c229cc4..475440a6 100644
>>> --- a/drivers/watchdog/of_xilinx_wdt.c
>>> +++ b/drivers/watchdog/of_xilinx_wdt.c
>>> @@ -147,8 +147,7 @@ static u32 xwdt_selftest(struct xwdt_device *xdev)
>>>  static int xwdt_probe(struct platform_device *pdev)
>>>  {
>>>         int rc;
>>> -       u32 *tmptr;
>>> -       u32 *pfreq;
>>> +       u32 pfreq, enable_once;
>>>         struct resource *res;
>>>         struct xwdt_device *xdev;
>>>         bool no_timeout = false;
>>> @@ -168,28 +167,24 @@ static int xwdt_probe(struct platform_device *pdev)
>>>         if (IS_ERR(xdev->base))
>>>                 return PTR_ERR(xdev->base);
>>>
>>> -       pfreq = (u32 *)of_get_property(pdev->dev.of_node,
>>> -                                       "clock-frequency", NULL);
>>> -
>>> -       if (pfreq == NULL) {
>>> +       rc = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &pfreq);
>>> +       if (rc) {
>>>                 dev_warn(&pdev->dev,
>>>                          "The watchdog clock frequency cannot be obtained\n");
>>>                 no_timeout = true;
>>
>> You can kill this...
>>
>>>         }
>>>
>>> -       tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>> -                                       "xlnx,wdt-interval", NULL);
>>> -       if (tmptr == NULL) {
>>> +       rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-interval",
>>> +                                 &xdev->wdt_interval);
>>> +       if (rc) {
>>>                 dev_warn(&pdev->dev,
>>>                          "Parameter \"xlnx,wdt-interval\" not found\n");
>>>                 no_timeout = true;
>>
>> and this...
>>
>>> -       } else {
>>> -               xdev->wdt_interval = *tmptr;
>>>         }
>>>
>>> -       tmptr = (u32 *)of_get_property(pdev->dev.of_node,
>>> -                                       "xlnx,wdt-enable-once", NULL);
>>> -       if (tmptr == NULL) {
>>> +       rc = of_property_read_u32(pdev->dev.of_node, "xlnx,wdt-enable-once",
>>> +                                 &enable_once);
>>> +       if (!rc && enable_once) {
>>>                 dev_warn(&pdev->dev,
>>>                          "Parameter \"xlnx,wdt-enable-once\" not found\n");
>>>                 watchdog_set_nowayout(xilinx_wdt_wdd, true);
>>> @@ -201,7 +196,7 @@ static int xwdt_probe(struct platform_device *pdev)
>>>   */
>>>         if (!no_timeout)
>>
>> and use "if (pfreq && xdev->wdt_interval)" if you initialize pfreq to 0.
> 
> 
> I just wanted to to change functions not logic in the driver.
> But it can be changed too.
> 
>>>                 xilinx_wdt_wdd->timeout = 2 * ((1 << xdev->wdt_interval) /
>>> -                                         *pfreq);
>>> +                                         pfreq);
>>
>> Is the wdog really usable if the timeout properties are missing? Seems
>> like you should fail to probe rather than warn.
> 
> There are 2 things.
> 1. timeout - you don't need pfreq and wdt_interval to use this driver
> but for that there should be module parameter timeout there.
> It should be added.
> 
> 2. about warn - based on 1 I don't think driver should failed
> but I am looking at logic above which I have added there but should be different.
> 
> u32 enable_once = 0;
> if (!rc)
> 	dev_warn
> 

if (rc) here sorry.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Arnd Bergmann]

On Friday 31 January 2014, Michal Simek wrote:
> +Optional properties:
> +- clock-frequency      : Frequency of clock in Hz
> +- xlnx,wdt-enable-once : 0 - Watchdog can be restarted
> +                         1 - Watchdog can be enabled just once
> +- xlnx,wdt-interval    : Watchdog timeout interval in 2^<val> clock cycles,
> +                         <val> is integer from 8 to 31.
> +

The latter two don't really seem to be xilinx specific, it would be
reasonable to have a standard watchdog binding that mandates a common
format for them.

I'm not sure about the enable-once flag, which seems to just map to the
"nowayout" watchdog option that is not a hardware feature at all
and should probably be kept as a software setting only, rather than
settable through DT. If it is kept, it should have a standard name and
get turned into a boolean (present/absent) property rather than a
0/1 integer property.

The interval should really be specified in terms of seconds or miliseconds,
not in clock cycles.

	Arnd

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Michal Simek]

On 02/03/2014 04:06 PM, Arnd Bergmann wrote:
> On Friday 31 January 2014, Michal Simek wrote:
>> +Optional properties:
>> +- clock-frequency      : Frequency of clock in Hz
>> +- xlnx,wdt-enable-once : 0 - Watchdog can be restarted
>> +                         1 - Watchdog can be enabled just once
>> +- xlnx,wdt-interval    : Watchdog timeout interval in 2^<val> clock cycles,
>> +                         <val> is integer from 8 to 31.
>> +
> 
> The latter two don't really seem to be xilinx specific, it would be
> reasonable to have a standard watchdog binding that mandates a common
> format for them.
> 
> I'm not sure about the enable-once flag, which seems to just map to the
> "nowayout" watchdog option that is not a hardware feature at all
> and should probably be kept as a software setting only, rather than
> settable through DT. If it is kept, it should have a standard name and
> get turned into a boolean (present/absent) property rather than a
> 0/1 integer property.
> 
> The interval should really be specified in terms of seconds or miliseconds,
> not in clock cycles.

Intention wasn't to fix binding but document current one
which is in mainline for a long time.

Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout
is seconds, and clock-frequency should go out and use CCF for getting clock.

Thanks,
Michal

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Arnd Bergmann]

On Monday 03 February 2014 16:13:47 Michal Simek wrote:
> Intention wasn't to fix binding but document current one
> which is in mainline for a long time.

Ok, I see.

> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout
> is seconds, and clock-frequency should go out and use CCF for getting clock.

Could we make a common binding then, and document that the xilinx
watchdog can optionally provide either one?

	Arnd

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Michal Simek]

On 02/03/2014 04:32 PM, Arnd Bergmann wrote:
> On Monday 03 February 2014 16:13:47 Michal Simek wrote:
>> Intention wasn't to fix binding but document current one
>> which is in mainline for a long time.
> 
> Ok, I see.
> 
>> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout
>> is seconds, and clock-frequency should go out and use CCF for getting clock.
> 
> Could we make a common binding then, and document that the xilinx
> watchdog can optionally provide either one?

Do you mean to have 2 DT bindings?

This binding is used from 2011-07.
It means it was generated for all hw designs at least from this time.
I would say from DT usage on Microblaze because it is not special case
in our dt generator.

xlnx,XXX are XXX parameters which you have to setup in tools
and get synthesized. This is valid for all xilinx IPs. We have full
IP description by generating xlnx,XXX parameters directly from tools
because we know all variants which can happen.

Just back to your previous post:
"I'm not sure about the enable-once flag, which seems to just map to the
"nowayout" watchdog option that is not a hardware feature at all"
this is hw feature which you can select in tools because this is fpga. :-)

Thanks,
Michal

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Arnd Bergmann]

On Monday 03 February 2014, Michal Simek wrote:
> On 02/03/2014 04:32 PM, Arnd Bergmann wrote:
> > On Monday 03 February 2014 16:13:47 Michal Simek wrote:
> >> Intention wasn't to fix binding but document current one
> >> which is in mainline for a long time.
> > 
> > Ok, I see.
> > 
> >> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout
> >> is seconds, and clock-frequency should go out and use CCF for getting clock.
> > 
> > Could we make a common binding then, and document that the xilinx
> > watchdog can optionally provide either one?
> 
> Do you mean to have 2 DT bindings?
> 
> This binding is used from 2011-07.
> It means it was generated for all hw designs at least from this time.
> I would say from DT usage on Microblaze because it is not special case
> in our dt generator.

I certainly wasn't suggesting to break the binding, quite the contrary.
What I tried to say is that the properties look like they should be
useful for different kinds of watchdogs, not just xilinx, so it would
be good to have a common definition using generic strings.

The xilinx driver would definitely have to keep supporting the traditional
property names, but it could also support the generic names in the
future.

> xlnx,XXX are XXX parameters which you have to setup in tools
> and get synthesized. This is valid for all xilinx IPs. We have full
> IP description by generating xlnx,XXX parameters directly from tools
> because we know all variants which can happen.
>
> Just back to your previous post:
> "I'm not sure about the enable-once flag, which seems to just map to the
> "nowayout" watchdog option that is not a hardware feature at all"
> this is hw feature which you can select in tools because this is fpga. :-)

Ah, so you mean the properties are not settings that the driver
programs into the hardware, but they are hardware properties that the
driver reports to user space?

	Arnd

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]

On 02/04/2014 08:27 PM, Arnd Bergmann wrote:
> On Monday 03 February 2014, Michal Simek wrote:
>> On 02/03/2014 04:32 PM, Arnd Bergmann wrote:
>>> On Monday 03 February 2014 16:13:47 Michal Simek wrote:
>>>> Intention wasn't to fix binding but document current one
>>>> which is in mainline for a long time.
>>>
>>> Ok, I see.
>>>
>>>> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout
>>>> is seconds, and clock-frequency should go out and use CCF for getting clock.
>>>
>>> Could we make a common binding then, and document that the xilinx
>>> watchdog can optionally provide either one?
>>
>> Do you mean to have 2 DT bindings?
>>
>> This binding is used from 2011-07.
>> It means it was generated for all hw designs at least from this time.
>> I would say from DT usage on Microblaze because it is not special case
>> in our dt generator.
> 
> I certainly wasn't suggesting to break the binding, quite the contrary.
> What I tried to say is that the properties look like they should be
> useful for different kinds of watchdogs, not just xilinx, so it would
> be good to have a common definition using generic strings.
> 
> The xilinx driver would definitely have to keep supporting the traditional
> property names, but it could also support the generic names in the
> future.

No problem with to do in future.

>> xlnx,XXX are XXX parameters which you have to setup in tools
>> and get synthesized. This is valid for all xilinx IPs. We have full
>> IP description by generating xlnx,XXX parameters directly from tools
>> because we know all variants which can happen.
>>
>> Just back to your previous post:
>> "I'm not sure about the enable-once flag, which seems to just map to the
>> "nowayout" watchdog option that is not a hardware feature at all"
>> this is hw feature which you can select in tools because this is fpga. :-)
> 
> Ah, so you mean the properties are not settings that the driver
> programs into the hardware, but they are hardware properties that the
> driver reports to user space?

yes, they are hardware properties which you can choose based on your
configuration. Every user just decides if this watchdog can be started just
once and how long it takes in synthesis time. There is no option to program
this by software.

I am not quite sure what you mean by reports to user space.
If you mean to get timeout through ioctl for example - then yes it is working
through standard watchdog ioctl interface and timeout is calculated
from hardware setup.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Arnd Bergmann]

On Wednesday 05 February 2014, Michal Simek wrote:
> I am not quite sure what you mean by reports to user space.
> If you mean to get timeout through ioctl for example - then yes it is working
> through standard watchdog ioctl interface and timeout is calculated
> from hardware setup.

Yes, that is what I meant. I believe most other watchdogs let
you program the timeout, but I don't see anything wrong with
having that fixed in the FPGA in your case.

Still, the choice of putting the timeout into DT in terms of
cycles rather than miliseconds wasn't ideal from an interface
perspective and we should change that if/when we do a generic
binding. I can definitely see where it's coming from for your
case, as the cycle count totally makes sense from an FPGA
tool perspective...

	Arnd

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

On 02/05/2014 10:36 AM, Arnd Bergmann wrote:
> On Wednesday 05 February 2014, Michal Simek wrote:
>> I am not quite sure what you mean by reports to user space.
>> If you mean to get timeout through ioctl for example - then yes it is working
>> through standard watchdog ioctl interface and timeout is calculated
>> from hardware setup.
> 
> Yes, that is what I meant. I believe most other watchdogs let
> you program the timeout, but I don't see anything wrong with
> having that fixed in the FPGA in your case.
> 
> Still, the choice of putting the timeout into DT in terms of
> cycles rather than miliseconds wasn't ideal from an interface
> perspective and we should change that if/when we do a generic
> binding. I can definitely see where it's coming from for your
> case, as the cycle count totally makes sense from an FPGA
> tool perspective...

Thanks. I take this like ACK for this current binding description.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 09/10] watchdog: xilinx: Add missing binding

[Arnd Bergmann]

On Wednesday 05 February 2014, Michal Simek wrote:
> On 02/05/2014 10:36 AM, Arnd Bergmann wrote:
> > On Wednesday 05 February 2014, Michal Simek wrote:
> > Still, the choice of putting the timeout into DT in terms of
> > cycles rather than miliseconds wasn't ideal from an interface
> > perspective and we should change that if/when we do a generic
> > binding. I can definitely see where it's coming from for your
> > case, as the cycle count totally makes sense from an FPGA
> > tool perspective...
> 
> Thanks. I take this like ACK for this current binding description.
> 

Yes, please add my 'Acked-by'.

	Arnd

Re: [PATCH 07/10] watchdog: xilinx: Fix OF binding

[Guenter Roeck]

On 01/31/2014 06:18 AM, Michal Simek wrote:
> Use of_property_read_u32 functions to clean OF probing.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>