Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[ivan.khoronzhuk]

The Keystone arch is using clocks in DT and source clock for watchdog
has to be specified, so add this to binding.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../devicetree/bindings/watchdog/davinci-wdt.txt   |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
index fddced9..4db4d0e 100644
--- a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
@@ -7,6 +7,10 @@ Required properties:
 
 - reg : Should contain WDT registers location and length
 
+- clocks: phandle reference to the controller clock.
+	  Required only for Keystone arch.
+	  See clock-bindings.txt
+
 Optional properties:
 
 - timeout-sec:		Contains the watchdog timeout in seconds
@@ -21,4 +25,5 @@ wdt: wdt@2320000 {
 	compatible = "ti,davinci-wdt";
 	reg = <0x02320000 0x80>;
 	timeout-sec = <30>;
+	clocks = <&clkwdtimer0>;
 };
-- 
1.7.9.5

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[Santosh Shilimkar]

On Wednesday 06 November 2013 06:32 AM, ivan.khoronzhuk wrote:
> The Keystone arch is using clocks in DT and source clock for watchdog
> has to be specified, so add this to binding.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[Guenter Roeck]

On 11/06/2013 03:32 AM, ivan.khoronzhuk wrote:
> The Keystone arch is using clocks in DT and source clock for watchdog
> has to be specified, so add this to binding.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>   .../devicetree/bindings/watchdog/davinci-wdt.txt   |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
> index fddced9..4db4d0e 100644
> --- a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
> @@ -7,6 +7,10 @@ Required properties:
>
>   - reg : Should contain WDT registers location and length
>
> +- clocks: phandle reference to the controller clock.
> +	  Required only for Keystone arch.
> +	  See clock-bindings.txt
> +

Yet another form of formatting. Also, wonder if it makes sense to merge this with the patch adding keystone support.

>   Optional properties:
>
>   - timeout-sec:		Contains the watchdog timeout in seconds
> @@ -21,4 +25,5 @@ wdt: wdt@2320000 {
>   	compatible = "ti,davinci-wdt";
>   	reg = <0x02320000 0x80>;
>   	timeout-sec = <30>;
> +	clocks = <&clkwdtimer0>;
>   };
>

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[ivan.khoronzhuk]

On 11/17/2013 04:28 AM, Guenter Roeck wrote:
> On 11/06/2013 03:32 AM, ivan.khoronzhuk wrote:
>> The Keystone arch is using clocks in DT and source clock for watchdog
>> has to be specified, so add this to binding.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   .../devicetree/bindings/watchdog/davinci-wdt.txt   |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
>> b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
>> index fddced9..4db4d0e 100644
>> --- a/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
>> +++ b/Documentation/devicetree/bindings/watchdog/davinci-wdt.txt
>> @@ -7,6 +7,10 @@ Required properties:
>>
>>   - reg : Should contain WDT registers location and length
>>
>> +- clocks: phandle reference to the controller clock.
>> +      Required only for Keystone arch.
>> +      See clock-bindings.txt
>> +
>
> Yet another form of formatting. Also, wonder if it makes sense to merge
> this with the patch adding keystone support.
>

Ok, I'll squash them.

>>   Optional properties:
>>
>>   - timeout-sec:        Contains the watchdog timeout in seconds
>> @@ -21,4 +25,5 @@ wdt: wdt@2320000 {
>>       compatible = "ti,davinci-wdt";
>>       reg = <0x02320000 0x80>;
>>       timeout-sec = <30>;
>> +    clocks = <&clkwdtimer0>;
>>   };
>>
>

-- 
Regards,
Ivan Khoronzhuk

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[Arnd Bergmann]

On Wednesday 06 November 2013, ivan.khoronzhuk wrote:
> @@ -7,6 +7,10 @@ Required properties:
>  
>  - reg : Should contain WDT registers location and length
>  
> +- clocks: phandle reference to the controller clock.
> +         Required only for Keystone arch.
> +         See clock-bindings.txt
> +
>  Optional properties:
>  
>  - timeout-sec:         Contains the watchdog timeout in seconds

I think it should really be listed under "Optional properties" and the
reference to Keystone removed. Note how the binding would need
to change otherwise if another platform started to use the clock, which
is a little silly.

	Arnd

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[ivan.khoronzhuk]

On 11/23/2013 07:57 PM, Arnd Bergmann wrote:
> On Wednesday 06 November 2013, ivan.khoronzhuk wrote:
>> @@ -7,6 +7,10 @@ Required properties:
>>   
>>   - reg : Should contain WDT registers location and length
>>   
>> +- clocks: phandle reference to the controller clock.
>> +         Required only for Keystone arch.
>> +         See clock-bindings.txt
>> +
>>   Optional properties:
>>   
>>   - timeout-sec:         Contains the watchdog timeout in seconds
> 
> I think it should really be listed under "Optional properties" and the
> reference to Keystone removed. Note how the binding would need
> to change otherwise if another platform started to use the clock, which
> is a little silly.
> 
> 	Arnd
> 

Ok, I'll move clocks property under "Optional properties" and describe it
as following:

Optional properties:
- timeout-sec : Contains the watchdog timeout in seconds
- clocks: phandle reference to the controller clock.
	  Needed if platform uses clocks.
	  See clock-bindings.txt

FYI:
The new patch series had been already presented, where the patches
"watchdog: davinci: add "clocks" property" and
"watchdog: davinci: reuse driver for keystone arch" were combined.
http://www.spinics.net/lists/devicetree/msg12542.html

-- 
Regards,
Ivan Khoronzhuk

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[Mark Rutland]

On Mon, Nov 25, 2013 at 10:59:45AM +0000, ivan.khoronzhuk wrote:
> On 11/23/2013 07:57 PM, Arnd Bergmann wrote:
> > On Wednesday 06 November 2013, ivan.khoronzhuk wrote:
> >> @@ -7,6 +7,10 @@ Required properties:
> >>   
> >>   - reg : Should contain WDT registers location and length
> >>   
> >> +- clocks: phandle reference to the controller clock.
> >> +         Required only for Keystone arch.
> >> +         See clock-bindings.txt
> >> +
> >>   Optional properties:
> >>   
> >>   - timeout-sec:         Contains the watchdog timeout in seconds
> > 
> > I think it should really be listed under "Optional properties" and the
> > reference to Keystone removed. Note how the binding would need
> > to change otherwise if another platform started to use the clock, which
> > is a little silly.
> > 
> > 	Arnd
> > 
> 
> Ok, I'll move clocks property under "Optional properties" and describe it
> as following:
> 
> Optional properties:
> - timeout-sec : Contains the watchdog timeout in seconds
> - clocks: phandle reference to the controller clock.
> 	  Needed if platform uses clocks.
> 	  See clock-bindings.txt

Nit: clocks aren't just phandles, they have a clock-specifier too (which
might be 0 cells).

Otherwise this looks fine to me.

Mark.

Re: Fwd: [PATCH 7/8] watchdog: davinci: add "clocks" property

[ivan.khoronzhuk]

On 11/25/2013 02:15 PM, Mark Rutland wrote:
> On Mon, Nov 25, 2013 at 10:59:45AM +0000, ivan.khoronzhuk wrote:
>> On 11/23/2013 07:57 PM, Arnd Bergmann wrote:
>>> On Wednesday 06 November 2013, ivan.khoronzhuk wrote:
>>>> @@ -7,6 +7,10 @@ Required properties:
>>>>
>>>>    - reg : Should contain WDT registers location and length
>>>>
>>>> +- clocks: phandle reference to the controller clock.
>>>> +         Required only for Keystone arch.
>>>> +         See clock-bindings.txt
>>>> +
>>>>    Optional properties:
>>>>
>>>>    - timeout-sec:         Contains the watchdog timeout in seconds
>>>
>>> I think it should really be listed under "Optional properties" and the
>>> reference to Keystone removed. Note how the binding would need
>>> to change otherwise if another platform started to use the clock, which
>>> is a little silly.
>>>
>>> 	Arnd
>>>
>>
>> Ok, I'll move clocks property under "Optional properties" and describe it
>> as following:
>>
>> Optional properties:
>> - timeout-sec : Contains the watchdog timeout in seconds
>> - clocks: phandle reference to the controller clock.
>> 	  Needed if platform uses clocks.
>> 	  See clock-bindings.txt
>
> Nit: clocks aren't just phandles, they have a clock-specifier too (which
> might be 0 cells).
>
> Otherwise this looks fine to me.
>
> Mark.
>

... I will replace it to:
Optional properties:
- timeout-sec : Contains the watchdog timeout in seconds
- clocks: the clock feeding the watchdog timer.
	  Needed if platform uses clocks.
	  See clock-bindings.txt

Is it OK?

-- 
Regards,
Ivan Khoronzhuk