diff mbox

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

Message ID 1314191356-10963-7-git-send-email-b-cousson@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Benoit Cousson Aug. 24, 2011, 1:09 p.m. UTC
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>
---
 arch/arm/boot/dts/omap4.dtsi         |    5 +++++
 arch/arm/mach-omap2/Makefile         |    6 +++++-
 drivers/hwspinlock/omap_hwspinlock.c |   11 +++++++++++
 3 files changed, 21 insertions(+), 1 deletions(-)

Comments

Ohad Ben-Cohen Sept. 7, 2011, 7:58 p.m. UTC | #1
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).

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 ?

> +#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 ;)
Benoit Cousson Sept. 8, 2011, 7:14 a.m. UTC | #2
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
Ohad Ben-Cohen Sept. 8, 2011, 7:56 a.m. UTC | #3
Hi Benoit,

On Thu, Sep 8, 2011 at 10:14 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
> 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?

I originally really thought we can ignore those cases (hence the 0
base id ;), and personally I still think the scenario is a bit
fictional, and wouldn't even mind just having omap_hwspinlock_probe()
return an error if it is unexpectedly probed with a second device.

But if fixing this entirely only means doing a small change, then it's
surely nicer.

> 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".

That would work if #hwspinlock is a fixed number, but a "baseid"
attribute would allow supporting devices with different #hwspinlocks
per device. Since I am not aware of any real hardware that does this
kind of blasphemy, I can't say if the latter is really necessary :) If
you prefer the former, I'm entirely fine with it.

Thanks,
Ohad.
Benoit Cousson Sept. 8, 2011, 8:07 a.m. UTC | #4
On 9/8/2011 9:56 AM, Ohad Ben-Cohen wrote:
> Hi Benoit,
>
> On Thu, Sep 8, 2011 at 10:14 AM, Cousson, Benoit<b-cousson@ti.com>  wrote:
>> 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?
>
> I originally really thought we can ignore those cases (hence the 0
> base id ;), and personally I still think the scenario is a bit
> fictional, and wouldn't even mind just having omap_hwspinlock_probe()
> return an error if it is unexpectedly probed with a second device.
>
> But if fixing this entirely only means doing a small change, then it's
> surely nicer.

That should not be a big deal to add that kind of support.

>> 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".
>
> That would work if #hwspinlock is a fixed number, but a "baseid"
> attribute would allow supporting devices with different #hwspinlocks
> per device. Since I am not aware of any real hardware that does this
> kind of blasphemy, I can't say if the latter is really necessary :) If
> you prefer the former, I'm entirely fine with it.

The (small) issue for my point of view is that the #hwspinlock is 
already encoded in the IP itself. So adding a baseid directly in DT will 
look like duplicating indirectly something that is already there in the HW.
That being said, since we cannot rely on the order, we will not be able 
to get the proper baseid until the driver probe every hwspinlock devices 
:-(
So baseid might be a easier choice.

Regards,
Benoit
Ohad Ben-Cohen Sept. 8, 2011, 8:11 a.m. UTC | #5
On Thu, Sep 8, 2011 at 11:07 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
> The (small) issue for my point of view is that the #hwspinlock is already
> encoded in the IP itself. So adding a baseid directly in DT will look like
> duplicating indirectly something that is already there in the HW.
> That being said, since we cannot rely on the order, we will not be able to
> get the proper baseid until the driver probe every hwspinlock devices :-(
> So baseid might be a easier choice.

Sounds good. Thanks a lot !
Arnd Bergmann Sept. 8, 2011, 2:47 p.m. UTC | #6
On Thursday 08 September 2011, Ohad Ben-Cohen wrote:
> On Thu, Sep 8, 2011 at 11:07 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
> > The (small) issue for my point of view is that the #hwspinlock is already
> > encoded in the IP itself. So adding a baseid directly in DT will look like
> > duplicating indirectly something that is already there in the HW.
> > That being said, since we cannot rely on the order, we will not be able to
> > get the proper baseid until the driver probe every hwspinlock devices :-(
> > So baseid might be a easier choice.
> 
> Sounds good. Thanks a lot !

I think a number would work here but is not optimal for the device tree
representation. I think a better binding would be to encode it like
interrupt numbers, where every device that uses a hwspinlock will describe
that as a combination of phandle to the hwspinlock controller and
identifier to be used by that controller, e.g.

	spinlock1 {
		compatible = "ti,omap-spinlock";
		regs = ...
		interrupts = <42>;
		interrupt-parent = &irq-controller;
	};

	dsp {
		compatible = ...
		regs = ...
		spinlocks = <23>; // local number withing &spinlock1;
		spinlock-controller = &spinlock1;
	};

or possibly shorter

		spinlocks = <&spinlock1 23>;

	Arnd
diff mbox

Patch

diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index a3efe76..231d7b4 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -105,5 +105,10 @@ 
 			reg = <0x48241000 0x1000>,
 			      <0x48240100 0x0200>;
 		};
+
+		spinlock {
+			compatible = "ti,omap-spinlock";
+			hwmods = "spinlock";
+		};
 	};
 };
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 79e42a1..6ab9116 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -263,9 +263,13 @@  obj-y					+= $(smc91x-m) $(smc91x-y)
 
 smsc911x-$(CONFIG_SMSC911X)		:= gpmc-smsc911x.o
 obj-y					+= $(smsc911x-m) $(smsc911x-y)
-obj-$(CONFIG_ARCH_OMAP4)		+= hwspinlock.o
 
 disp-$(CONFIG_OMAP2_DSS)		:= display.o
 obj-y					+= $(disp-m) $(disp-y)
 
 obj-y					+= common-board-devices.o twl-common.o
+
+# Do not build static device init code in case of DT build
+ifneq ($(CONFIG_OF),y)
+obj-$(CONFIG_ARCH_OMAP4)		+= hwspinlock.o
+endif
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index a8f0273..09dd132 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -203,11 +203,22 @@  static int omap_hwspinlock_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id spinlock_match[] = {
+	{.compatible = "ti,omap-spinlock", },
+	{},
+}
+MODULE_DEVICE_TABLE(of, spinlock_match);
+#else
+#define spinlock_match NULL
+#endif
+
 static struct platform_driver omap_hwspinlock_driver = {
 	.probe		= omap_hwspinlock_probe,
 	.remove		= omap_hwspinlock_remove,
 	.driver		= {
 		.name	= "omap_hwspinlock",
+		.of_match_table = spinlock_match,
 	},
 };