| Message ID | 1314191356-10963-7-git-send-email-b-cousson@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ;)
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
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.
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
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 !
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 --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, }, };
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(-)