Re: [RFC PATCH 06/10] hwspinlock: OMAP4: Add spinlock support in DT

["Cousson, Benoit" <b-cousson@ti.com>]

Hi Ohad,

On 9/7/2011 9:58 PM, Ohad Ben-Cohen wrote:
> On Wed, Aug 24, 2011 at 4:09 PM, Benoit Cousson<b-cousson@ti.com>  wrote:
>> Add a device-tree node for the spinlock.
>> Remove the static device build code if CONFIG_OF
>> is set.
>> Update the hwspinlock driver to use the of_match method.
>> Add the information in Documentation/devicetree.
>>
>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Cc: Ohad Ben-Cohen<ohad@wizery.com>
>> ---
> ...
>> +               spinlock {
>> +                       compatible = "ti,omap-spinlock";
>> +                       hwmods = "spinlock";
>> +               };
>
> This seem to satisfy the current hwspinlock driver, but I'm wondering
> about an issue which was discussed awhile ago by Arnd and Mathieu:
>
> Hwspinlock devices provide system-wide hardware locks that are used by
> remote processors that have no other way to achieve synchronization.
>
> For that to work, each physical lock must have a system-wide unique id
> number that all processors are familiar with, otherwise they can't
> possibly assume they're using the same hardware lock.
>
> Usually SoC have a single hwspinlock device, which provides several
> hardware spinlocks, and in this case, the locks can be trivially
> numbered 0 to (num-of-locks - 1).
>
> In case boards have several hwspinlocks devices (each of which
> providing numerous hardware spinlocks) a different base id should be
> used for each hwspinlock device (they can't all use 0 as a starting
> id!).
>
> While this is certainly not common, it's just plain wrong for the
> hwspinlock driver to silently use 0 as a base id whenever it is probed
> with a device (and by that implicitly assume there will always be only
> one device).

Hehe, I'm not the one who wrote that driver :-)

This is not wrong for the current HW. The point is do we want to 
anticipate potential HW evolution that might never happen on that IP?

> So we need to couple an hwspinlock device with a base id (which is
> trivially zero when there's only a single hwspinlock device). This can
> be easily achieved today using platform data, which boards will use to
> set a different base id for each of the hwspinlock devices they have
> (i'll send a patch demonstrating this soon), but I'm wondering how to
> specify this hwspinlock-specific data with DT: is there an existing
> binding we can use for this ? or should we create something like a
> "baseid" one especially for the hwspinlock driver ?

This is no different than the multiple GPIO controllers we have today.
Since we cannot rely on the DT nodes order, I added an explicit "id" 
attribute to provide that information to the driver. And then the baseid 
is "id * #gpios".

>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id spinlock_match[] = {
>> +       {.compatible = "ti,omap-spinlock", },
>> +       {},
>> +}
>
> you're missing a semicolon there (yeah I actually tried to build this ;)

That was a test :-)
In fact it looks like this driver is not built with a default 
omap2plus_defconfig :-(
I'll fix that.

Thanks for the review,
Benoit