Re: [PATCH v2 7/9] watchdog: orion: Update device-tree binding documentation

Re: [PATCH v2 7/9] watchdog: orion: Update device-tree binding documentation

[Jason Cooper]

On Fri, Aug 23, 2013 at 07:12:20PM -0300, Ezequiel Garcia wrote:
> Change the 'reg' property meaning, by defining two required cells.
> It's important to note this commit breaks DT-compatibility for this
> device.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> index 5dc8d30..a74c9c8 100644
> --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> @@ -3,7 +3,9 @@
>  Required Properties:
>  
>  - Compatibility : "marvell,orion-wdt"
> -- reg		: Address of the timer registers
> +- reg		: Two cells are required:
> +		  First  cell contains the global timer control register.
> +		  Second cell contains the watchdog counter register.
>  
>  Optional properties:
>  
> @@ -13,7 +15,8 @@ Example:
>  
>  	wdt@20300 {
>  		compatible = "marvell,orion-wdt";
> -		reg = <0x20300 0x28>;
> +		reg = <0x20300 0x4


> +		       0x20324 0x4>;

Is there a reason we're going this route over adding a new compatible
string? iow, why can't we keep this knowledge in the driver and have a
"marvell,armada-wdt" or similar compat string that the orion-wdt driver
also serves?

thx,

Jason.

Re: [PATCH v2 7/9] watchdog: orion: Update device-tree binding documentation

[Ezequiel Garcia]

On Mon, Aug 26, 2013 at 10:08:43AM -0400, Jason Cooper wrote:
> On Fri, Aug 23, 2013 at 07:12:20PM -0300, Ezequiel Garcia wrote:
> > Change the 'reg' property meaning, by defining two required cells.
> > It's important to note this commit breaks DT-compatibility for this
> > device.
> > 
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > index 5dc8d30..a74c9c8 100644
> > --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
> > @@ -3,7 +3,9 @@
> >  Required Properties:
> >  
> >  - Compatibility : "marvell,orion-wdt"
> > -- reg		: Address of the timer registers
> > +- reg		: Two cells are required:
> > +		  First  cell contains the global timer control register.
> > +		  Second cell contains the watchdog counter register.
> >  
> >  Optional properties:
> >  
> > @@ -13,7 +15,8 @@ Example:
> >  
> >  	wdt@20300 {
> >  		compatible = "marvell,orion-wdt";
> > -		reg = <0x20300 0x28>;
> > +		reg = <0x20300 0x4
> 
> 
> > +		       0x20324 0x4>;
> 
> Is there a reason we're going this route over adding a new compatible
> string?

Well, it seemed to me that this register splitting was more device-treeish:
it prevents you from fixing your driver, adding a new compatible-string,
and rebuilding a kernel each time a new SoC appears with a different offset
between registers.

Instead, and trying to follow the DT-preachers, we would just change the
"reg" values and -bang!- have forward-compatibility :-)

That said...

> iow, why can't we keep this knowledge in the driver and have a
> "marvell,armada-wdt" or similar compat string that the orion-wdt driver
> also serves?
> 

... we still need new compatible strings for armada-xp-wdt and
armada-370-wdt, for they have differences between each other
and with the orion-wdt:

* The clock input is obtained in a different way in each case

* The watchdog enable bit inside the timer control register
  is at a different location.

So, thinking about this again, perhaps we should simply let alone the
"reg" property and add the watchdog counter offset as yet another field
in the compatible-data?

What do you think?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/9] watchdog: orion: Update device-tree binding documentation

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Tue, 27 Aug 2013 05:36:04 -0300, Ezequiel Garcia wrote:

> > Is there a reason we're going this route over adding a new compatible
> > string?
> 
> Well, it seemed to me that this register splitting was more device-treeish:
> it prevents you from fixing your driver, adding a new compatible-string,
> and rebuilding a kernel each time a new SoC appears with a different offset
> between registers.

I don't think encoding register offsets in the DT is a good idea. The
compatible string is here to identify different revisions/versions of
the same hardware block, and the driver should abstract out the
details. Otherwise, you'll start encoding bit numbers, mask values, and
other crazy things in the Device Tree.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2 7/9] watchdog: orion: Update device-tree binding documentation

[Jason Cooper]

On Tue, Aug 27, 2013 at 10:39:07AM +0200, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
> 
> On Tue, 27 Aug 2013 05:36:04 -0300, Ezequiel Garcia wrote:
> 
> > > Is there a reason we're going this route over adding a new compatible
> > > string?
> > 
> > Well, it seemed to me that this register splitting was more device-treeish:
> > it prevents you from fixing your driver, adding a new compatible-string,
> > and rebuilding a kernel each time a new SoC appears with a different offset
> > between registers.
> 
> I don't think encoding register offsets in the DT is a good idea. The
> compatible string is here to identify different revisions/versions of
> the same hardware block, and the driver should abstract out the
> details. Otherwise, you'll start encoding bit numbers, mask values, and
> other crazy things in the Device Tree.

Agreed.  DT is for describing how the hardware is wired together on a
specific board.  The compatible strings should tell you which variant of
an IP block you have.  The information you're trying to encode doesn't
belong in the DT, it should be in the driver.

As an added bonus, by doing so, you avoid breaking compatibility. ;-)

thx,

Jason.

Re: [PATCH v2 7/9] watchdog: orion: Update device-tree binding documentation

[Stephen Warren]

On 08/27/2013 02:36 AM, Ezequiel Garcia wrote:
> On Mon, Aug 26, 2013 at 10:08:43AM -0400, Jason Cooper wrote:
>> On Fri, Aug 23, 2013 at 07:12:20PM -0300, Ezequiel Garcia wrote:
>>> Change the 'reg' property meaning, by defining two required cells.
>>> It's important to note this commit breaks DT-compatibility for this
>>> device.
>>>
>>> Cc: devicetree@vger.kernel.org
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>>> ---
>>>  Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
>>> index 5dc8d30..a74c9c8 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/orion-wdt.txt
>>> @@ -3,7 +3,9 @@
>>>  Required Properties:
>>>  
>>>  - Compatibility : "marvell,orion-wdt"
>>> -- reg		: Address of the timer registers
>>> +- reg		: Two cells are required:
>>> +		  First  cell contains the global timer control register.
>>> +		  Second cell contains the watchdog counter register.

BTW, you don't have 2 cells here, but 4 cells (assuming typical
#address-cells and #size-cells values). Instead, you have 2 register
specifiers.