LinuxLists.cc - [PATCH 01/10] watchdog: xilinx: Convert driver to the watchdog framework

[permalink] [raw]

Subject: Re: [PATCH 10/10] watchdog: xilinx: Enable this driver for Zynq

On 01/31/2014 03:52 PM, Guenter Roeck wrote:
> On 01/31/2014 06:18 AM, Michal Simek wrote:
>> Enable this driver for Zynq.
>> Move it to architecture independent Kconfig part.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>> ---
>>
>> Build tested by zero day testing system.
>> ---
>> drivers/watchdog/Kconfig | 22 +++++++++-------------
>> 1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 9db5d3c..6120403 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -111,6 +111,15 @@ config WM8350_WATCHDOG
>> Support for the watchdog in the WM8350 AudioPlus PMIC. When
>> the watchdog triggers the system will be reset.
>>
>> +config XILINX_WATCHDOG
>> + tristate "Xilinx Watchdog timer"
>> + select WATCHDOG_CORE
>
> This needs to depend on HAS_IOMEM.

Are you sure?
I have no problem to do this change.
Zero day testing system doesn't report any problem with it.

I have checked dependencies and only score, tile and um has NO_IOMEM option
enables. And below in log is tile with allyesconfig that's why I believe
this driver has been also tested without any issue.

Thanks,
Michal

git://git.monstr.eu/linux-2.6-microblaze xnext/watchdog
f7bdfada576e93eaab8f6dc2ecd881da8f43911c watchdog: xilinx: Enable this driver for Zynq

elapsed time: 81m

configs tested: 122

alpha defconfig
parisc allnoconfig
parisc b180_defconfig
parisc c3000_defconfig
parisc defconfig
arm allnoconfig
arm almodconfig
arm at91_dt_defconfig
arm imx_v6_v7_defconfig
arm marzen_defconfig
arm omap2plus_defconfig
arm prima2_defconfig
arm s3c2410_defconfig
arm spear13xx_defconfig
arm tegra_defconfig
m32r m32104ut_defconfig
m32r mappi3.smp_defconfig
m32r opsput_defconfig
m32r usrv_defconfig
xtensa common_defconfig
xtensa iss_defconfig
x86_64 allnoconfig
sh allnoconfig
sh rsk7269_defconfig
sh sh7785lcr_32bit_defconfig
sh titan_defconfig
x86_64 randconfig-c0-0131
x86_64 randconfig-c1-0131
x86_64 randconfig-c2-0131
x86_64 randconfig-c3-0131
x86_64 randconfig-c4-0131
x86_64 randconfig-c5-0131
x86_64 randconfig-c6-0131
x86_64 randconfig-c7-0131
x86_64 randconfig-c8-0131
x86_64 randconfig-c9-0131
x86_64 allyesconfig
alpha allyesconfig
avr32 allyesconfig
blackfin allyesconfig
cris allyesconfig
ia64 allyesconfig
m68k allyesconfig
mips allyesconfig
parisc allyesconfig
powerpc allyesconfig
s390 allyesconfig
sh allyesconfig
sparc allyesconfig
sparc64 allyesconfig
tile allyesconfig
xtensa allyesconfig
ia64 alldefconfig
ia64 allmodconfig
ia64 allnoconfig
ia64 defconfig
x86_64 lkp
powerpc chroma_defconfig
powerpc corenet64_smp_defconfig
powerpc gamecube_defconfig
powerpc linkstation_defconfig
powerpc wii_defconfig
x86_64 randconfig-j0-0131
x86_64 randconfig-j1-0131
x86_64 randconfig-j2-0131
x86_64 randconfig-j3-0131
x86_64 randconfig-j4-0131
x86_64 randconfig-j5-0131
m68k allmodconfig
m68k amiga_defconfig
m68k m5475evb_defconfig
m68k multi_defconfig
blackfin BF526-EZBRD_defconfig
blackfin BF533-EZKIT_defconfig
blackfin BF561-EZKIT-SMP_defconfig
blackfin TCM-BF537_defconfig
cris etrax-100lx_v2_defconfig
i386 randconfig-r0-0131
i386 randconfig-r1-0131
i386 randconfig-r2-0131
i386 randconfig-r3-0131
i386 randconfig-r4-0131
i386 randconfig-r5-0131
s390 allmodconfig
s390 allnoconfig
s390 defconfig
i386 allyesconfig
x86_64 allmodconfig
i386 alldefconfig
i386 allmodconfig
i386 allnoconfig
i386 defconfig
x86_64 randconfig-x000
x86_64 randconfig-x001
x86_64 randconfig-x002
x86_64 randconfig-x003
x86_64 randconfig-x004
x86_64 randconfig-x005
x86_64 randconfig-x006
x86_64 randconfig-x007
x86_64 randconfig-x008
x86_64 randconfig-x009
i386 randconfig-x000
i386 randconfig-x001
i386 randconfig-x002
i386 randconfig-x003
i386 randconfig-x004
i386 randconfig-x005
i386 randconfig-x006
i386 randconfig-x007
i386 randconfig-x008
i386 randconfig-x009
powerpc allmodconfig
powerpc allnoconfig
powerpc defconfig
powerpc ppc64_defconfig
x86_64 acpi-redef
x86_64 allyesdebian
x86_64 nfsroot
microblaze allyesconfig
microblaze mmu_defconfig
microblaze nommu_defconfig

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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

Attachments:

2014-02-03 07:59:50

[permalink] [raw]

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

On 01/31/2014 06:33 PM, Rob Herring wrote:
> On Fri, Jan 31, 2014 at 8:18 AM, Michal Simek <[email protected]> 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 <[email protected]>
>> ---
>>
>> 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: http://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

Attachments:

2014-02-03 08:01:22

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]>
>>> ---
>>>
>>> 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: http://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

Attachments:

2014-02-03 08:26:21

[permalink] [raw]

Subject: Re: [PATCH 10/10] watchdog: xilinx: Enable this driver for Zynq

On 02/02/2014 11:01 PM, Michal Simek wrote:
> On 01/31/2014 03:52 PM, Guenter Roeck wrote:
>> On 01/31/2014 06:18 AM, Michal Simek wrote:
>>> Enable this driver for Zynq.
>>> Move it to architecture independent Kconfig part.
>>>
>>> Signed-off-by: Michal Simek <[email protected]>
>>> ---
>>>
>>> Build tested by zero day testing system.
>>> ---
>>> drivers/watchdog/Kconfig | 22 +++++++++-------------
>>> 1 file changed, 9 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 9db5d3c..6120403 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -111,6 +111,15 @@ config WM8350_WATCHDOG
>>> Support for the watchdog in the WM8350 AudioPlus PMIC. When
>>> the watchdog triggers the system will be reset.
>>>
>>> +config XILINX_WATCHDOG
>>> + tristate "Xilinx Watchdog timer"
>>> + select WATCHDOG_CORE
>>
>> This needs to depend on HAS_IOMEM.
>
> Are you sure?
> I have no problem to do this change.
> Zero day testing system doesn't report any problem with it.
>
> I have checked dependencies and only score, tile and um has NO_IOMEM option
> enables. And below in log is tile with allyesconfig that's why I believe
> this driver has been also tested without any issue.
>

https://lkml.org/lkml/2014/1/31/123

I would have assumed the same applies here, but if you are sure that it doesn't I am fine.

Thanks,
Guenter

2014-02-03 08:45:57

[permalink] [raw]

Subject: Re: [PATCH 10/10] watchdog: xilinx: Enable this driver for Zynq

On 02/03/2014 09:26 AM, Guenter Roeck wrote:
> On 02/02/2014 11:01 PM, Michal Simek wrote:
>> On 01/31/2014 03:52 PM, Guenter Roeck wrote:
>>> On 01/31/2014 06:18 AM, Michal Simek wrote:
>>>> Enable this driver for Zynq.
>>>> Move it to architecture independent Kconfig part.
>>>>
>>>> Signed-off-by: Michal Simek <[email protected]>
>>>> ---
>>>>
>>>> Build tested by zero day testing system.
>>>> ---
>>>> drivers/watchdog/Kconfig | 22 +++++++++-------------
>>>> 1 file changed, 9 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index 9db5d3c..6120403 100644
>>>> --- a/drivers/watchdog/Kconfig
>>>> +++ b/drivers/watchdog/Kconfig
>>>> @@ -111,6 +111,15 @@ config WM8350_WATCHDOG
>>>> Support for the watchdog in the WM8350 AudioPlus PMIC. When
>>>> the watchdog triggers the system will be reset.
>>>>
>>>> +config XILINX_WATCHDOG
>>>> + tristate "Xilinx Watchdog timer"
>>>> + select WATCHDOG_CORE
>>>
>>> This needs to depend on HAS_IOMEM.
>>
>> Are you sure?
>> I have no problem to do this change.
>> Zero day testing system doesn't report any problem with it.
>>
>> I have checked dependencies and only score, tile and um has NO_IOMEM option
>> enables. And below in log is tile with allyesconfig that's why I believe
>> this driver has been also tested without any issue.
>>
>
> https://lkml.org/lkml/2014/1/31/123
>
> I would have assumed the same applies here, but if you are sure that it doesn't I am fine.

I am not 100% sure because I didn't compile it myself for that 3 archs
but one was just fine.

Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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

Attachments:

2014-02-03 15:06:24

[permalink] [raw]

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

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

2014-02-03 15:14:06

[permalink] [raw]

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

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

2014-02-03 15:32:44

[permalink] [raw]

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

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

2014-02-03 15:48:17

[permalink] [raw]

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

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

2014-02-04 19:27:33

[permalink] [raw]

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

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

2014-02-05 09:25:45

[permalink] [raw]

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

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: http://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

Attachments:

2014-02-05 09:37:19

[permalink] [raw]

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

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

2014-02-05 09:41:27

[permalink] [raw]

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

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: http://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

Attachments:

2014-02-05 14:00:50

[permalink] [raw]

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

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

2014-02-09 20:04:01

[permalink] [raw]

Subject: Re: [PATCH 01/10] watchdog: xilinx: Convert driver to the watchdog framework

On 01/31/2014 06:18 AM, Michal Simek wrote:
> - Remove uneeded headers, fops functions
> - Use xilinx_wdt prefix in start/stop/keepalive functions
> and in new structures
>
> Signed-off-by: Michal Simek <[email protected]>

Hi Michal,

> static int xwdt_probe(struct platform_device *pdev)
> {
> int rc;
> @@ -314,7 +184,7 @@ static int xwdt_probe(struct platform_device *pdev)
> "xlnx,wdt-enable-once", NULL);
> if (tmptr == NULL) {
> pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
> - xdev.nowayout = WATCHDOG_NOWAYOUT;
> + watchdog_set_nowayout(&xilinx_wdt_wdd, true);

Sure you want to set this to always true instead of using WATCHDOG_NOWAYOUT ?

Assuming this is what you want:

Reviewed-by: Guenter Roeck <[email protected]>

Thanks,
Guenter

2014-02-09 20:05:28

[permalink] [raw]

Subject: Re: [PATCH 02/10] watchdog: xilinx: Move control_status_reg to functions

On 01/31/2014 06:18 AM, Michal Simek wrote:
> control_status_reg is temp variables and should be
> used locally by specific function.
>
> Signed-off-by: Michal Simek <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

2014-02-09 20:08:27

[permalink] [raw]

Subject: Re: [03/10] watchdog: xilinx: Simplify probe and remove functions

On Fri, Jan 31, 2014 at 10:18:12PM -0000, Michal Simek wrote:
> Use devm_ helper function to simplify probe and error path.
> Move ioremap to the beginning of probe function.
>
> Signed-off-by: Michal Simek <[email protected]>
>
Reviewed-by: Guenter Roeck <[email protected]>

2014-02-09 20:09:08

[permalink] [raw]

Subject: Re: [PATCH 04/10] watchdog: xilinx: Move no_timeout to probe function

On 01/31/2014 06:18 AM, Michal Simek wrote:
> no_timeout should be local variable because it is used
> only in probe function.
>
> Signed-off-by: Michal Simek <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

2014-02-09 20:13:04

[permalink] [raw]

Subject: Re: [PATCH 05/10] watchdog: xilinx: Allocate private structure per device

On 01/31/2014 06:18 AM, Michal Simek wrote:
> Only one watchdog could be used by this driver.
> Create driver private data structure and move there
> all variables for one instance.
>
> Signed-off-by: Michal Simek <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

2014-02-09 20:14:41

[permalink] [raw]

Subject: Re: [PATCH 06/10] watchdog: xilinx: Fix all printk messages

On 01/31/2014 06:18 AM, Michal Simek wrote:
> Use dev_ functions for printk messages.
>
> Signed-off-by: Michal Simek <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

2014-02-09 20:18:14

[permalink] [raw]

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

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 <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

2014-02-09 20:19:35

[permalink] [raw]

Subject: Re: [PATCH 08/10] watchdog: xilinx: Use correct comment indentation

On 01/31/2014 06:18 AM, Michal Simek wrote:
> No functional changes.
>
> Signed-off-by: Michal Simek <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

2014-02-10 00:51:37

[permalink] [raw]

Subject: Re: [PATCH 10/10] watchdog: xilinx: Enable this driver for Zynq

On 01/31/2014 06:18 AM, Michal Simek wrote:
> Enable this driver for Zynq.
> Move it to architecture independent Kconfig part.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
>
> Build tested by zero day testing system.
> ---

Hi Michal,

I tried myself, and I don't see any build failures on any platform.
So let's assume this works as-is.

Reviewed-by: Guenter Roeck <[email protected]>

Guenter

2014-02-10 07:03:57

[permalink] [raw]

Subject: Re: [PATCH 01/10] watchdog: xilinx: Convert driver to the watchdog framework

On 02/09/2014 09:03 PM, Guenter Roeck wrote:
> On 01/31/2014 06:18 AM, Michal Simek wrote:
>> - Remove uneeded headers, fops functions
>> - Use xilinx_wdt prefix in start/stop/keepalive functions
>> and in new structures
>>
>> Signed-off-by: Michal Simek <[email protected]>
>
> Hi Michal,
>
>> static int xwdt_probe(struct platform_device *pdev)
>> {
>> int rc;
>> @@ -314,7 +184,7 @@ static int xwdt_probe(struct platform_device *pdev)
>> "xlnx,wdt-enable-once", NULL);
>> if (tmptr == NULL) {
>> pr_warn("Parameter \"xlnx,wdt-enable-once\" not found in device tree!\n");
>> - xdev.nowayout = WATCHDOG_NOWAYOUT;
>> + watchdog_set_nowayout(&xilinx_wdt_wdd, true);
>
> Sure you want to set this to always true instead of using WATCHDOG_NOWAYOUT ?

I have checked it and
option CONFIG_WATCHDOG_NOWAYOUT - Disable watchdog shutdown on close

with this part in the header

100 #ifdef CONFIG_WATCHDOG_NOWAYOUT
101 #define WATCHDOG_NOWAYOUT 1
102 #define WATCHDOG_NOWAYOUT_INIT_STATUS (1 << WDOG_NO_WAY_OUT)
103 #else
104 #define WATCHDOG_NOWAYOUT 0
105 #define WATCHDOG_NOWAYOUT_INIT_STATUS 0
106 #endif

enable once is hardware option and it means when this option is setup in
hw you can't stop watchdog. That's why I think that setting up true
instead of WATCHDOG_NOWAYOUT is correct.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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

Attachments:

2014-02-10 07:06:08

[permalink] [raw]

Subject: Re: [PATCH 10/10] watchdog: xilinx: Enable this driver for Zynq

On 02/10/2014 01:51 AM, Guenter Roeck wrote:
> On 01/31/2014 06:18 AM, Michal Simek wrote:
>> Enable this driver for Zynq.
>> Move it to architecture independent Kconfig part.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>> ---
>>
>> Build tested by zero day testing system.
>> ---
>
> Hi Michal,
>
> I tried myself, and I don't see any build failures on any platform.
> So let's assume this works as-is.
>
> Reviewed-by: Guenter Roeck <[email protected]>
>

Thanks for your review. I have fixed comments from Rob and
also add one more patch which was suggested by Rob.
I will wait for your reactions for 1/10 and then will send
v2.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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

Attachments:

2014-02-10 17:18:22