Re: [PATCHv2 2/9] hwspinlock/omap: add support for dt nodes

[Suman Anna <s-anna@ti.com>]

Hi Mark,

>>
>>> On Fri, Sep 27, 2013 at 05:06:38PM +0100, Kumar Gala wrote:
>>>>
>>>> On Sep 17, 2013, at 2:30 PM, Suman Anna wrote:
>>>>
>>>>> HwSpinlock IP is present only on OMAP4 and other newer SoCs,
>>>>> which are all device-tree boot only. This patch adds the
>>>>> base support for parsing the DT nodes, and removes the code
>>>>> dealing with the traditional platform device instantiation.
>>>>>
>>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>>> ---
>>>>> .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
>>>>> arch/arm/mach-omap2/Makefile                       |  3 --
>>>>> arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
>>>>> drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
>>>>> 4 files changed, 50 insertions(+), 67 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
>>>>> delete mode 100644 arch/arm/mach-omap2/hwspinlock.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
>>>>> new file mode 100644
>>>>> index 0000000..235b7c5
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
>>>>> @@ -0,0 +1,31 @@
>>>>> +OMAP4+ HwSpinlock Driver
>>>>> +========================
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible:		Currently supports only "ti,omap4-hwspinlock" for
>>>>> +				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs
>>>
>>> "Currently supports" is not something I expect to see in a binding
>>> document. That sounds like a description of the driver rather than the
>>> binding.
>>>
>>> How similar are these hardware modules? What are the differences?
>>
>> The IP is almost the same, they all have the same revision id. The
>> number of locks (each represented by a register) though vary from one
>> SoC to another (OMAP4, OMAP5, DRA7 have same number of locks, and
>> AM33xx/AM43xx have a different number). The number of locks is directly
>> read by the driver from a module register. There is no separate .data
>> associated with the of_device_id table, so I used a single compatible
>> property for all the SoCs.
> 
> Ok. Probeability is good, it keeps these simpler :)
> 
> I think This can be reworded to say "should contain" rather than "currently
> supports only":
> 
> - compatible:		Should contain "ti,omap4-hwspinlock" for
> 			OMAP44xx, OMAP54xx, AM33xx, AM43xx, or DRA7xx SoCs
> 
> That way the binding allows for a future backwards-compatible variant, and
> doesn't mention the current level of support in Linux.

Yes, that is the change I have made in my current working set as well.

> 
>>
>>>
>>>>> +- reg:			Contains the hwspinlock register address range (base
>>>>> +			address and length)
>>>
>>> Is there only one register bank for the hwlock module?
>>
>> The lock registers start at a certain offset (0x800) within the module
>> register space, and the offsets for various registers are identical
>> between all SoCs.
> 
> What are the other registers within the module? Are they shared with other
> devices, or are they simply unused by the hwspinlock driver?

No, they are not shared with other devices. These are like revision
register, and a SYSCONFIG register which is used by the OMAP hwmod
layer. This register definition is in line with other modules on OMAP.

> 
>>
>>>
>>>>> +- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
>>>>> +
>>>>> +Common hwlock properties:
>>>>> +The following describes the usage of the common hwlock properties (defined in
>>>>> +Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
>>>>> +
>>>>> +- hwlock-base-id:	There are currently no OMAP SoCs with multiple
>>>>> +			hwspinlock devices. The OMAP driver uses a default
>>>>> +			base id value of 0 for the locks present within the
>>>>> +			single hwspinlock device on all SoCs.
>>>
>>>
>>> Driver details should not leak into bindngs...
>>
>> OK, will remove the info on driver details.
>>
>>>
>>> As mentioned in the other patch, I don't think this is the way to handle
>>> this. I think we need a phandle + args representation.
>>
>> This is an optional parameter for now and I was going to revise the
>> description based on comments from Kumar Gala on this thread, but I will
>> wait and adjust this based on the outcome on the first patch.
> 
> Ok.

I have removed this property altogether in my current working set. Will
post the v3 of the series soon.

regards
Suman