Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-22 Thread Tony Lindgren
* Ohad Ben-Cohen o...@wizery.com [101018 00:41]: From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially introduced in OMAP4, provides hardware assistance for synchronization between the multiple processors in the device

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-21 Thread Arnd Bergmann
On Thursday 21 October 2010, Ohad Ben-Cohen wrote: This sounds like adding a set of API that resembles spin_{unlock,lock}_irq. My gut feeling here is that while this may be useful and simple at certain places, it is somewhat error prone; a driver which would erroneously use this at the wrong

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-21 Thread Ohad Ben-Cohen
On Thu, Oct 21, 2010 at 11:04 AM, Arnd Bergmann a...@arndb.de wrote: On Thursday 21 October 2010, Ohad Ben-Cohen wrote: This sounds like adding a set of API that resembles spin_{unlock,lock}_irq. My gut feeling here is that while this may be useful and simple at certain places, it is somewhat

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-21 Thread Arnd Bergmann
On Thursday 21 October 2010, Ohad Ben-Cohen wrote: The change is also pretty trivial: * move the internal locking implementation to raw_ methods * the raw_ methods would save the current interrupt state only if given a placeholder * wrap those raw_ methods with the desired API (but only

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-20 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 7:16 PM, Kevin Hilman khil...@deeprootsystems.com wrote: Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially introduced in OMAP4, provides hardware assistance

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-20 Thread Kevin Hilman
Ohad Ben-Cohen o...@wizery.com writes: On Tue, Oct 19, 2010 at 7:16 PM, Kevin Hilman khil...@deeprootsystems.com wrote: Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-20 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 11:08 PM, Arnd Bergmann a...@arndb.de wrote: Right. There are two more things to consider though: If you know that interrupts are disabled, you may still want to avoid having to save the interrupt flag to the stack, to save some cycles saving and restoring it. I don't

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Greg KH
On Mon, Oct 18, 2010 at 09:44:33AM +0200, Ohad Ben-Cohen wrote: +#else /* !CONFIG_OMAP_HWSPINLOCK */ + +static inline struct omap_hwspinlock *omap_hwspinlock_request(void) +{ + return ERR_PTR(-ENOSYS); +} One note, do you really want to fail if this option isn't built into the kernel,

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Kevin Hilman
Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially introduced in OMAP4, provides hardware assistance for synchronization between the multiple processors in the device (Cortex-A9,

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Grant Likely
On Mon, Oct 18, 2010 at 1:44 AM, Ohad Ben-Cohen o...@wizery.com wrote: From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially introduced in OMAP4, provides hardware assistance for synchronization between the multiple

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Kevin Hilman
Ohad Ben-Cohen o...@wizery.com writes: From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially introduced in OMAP4, provides hardware assistance for synchronization between the multiple processors in the device (Cortex-A9,

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Arnd Bergmann
On Monday 18 October 2010 09:44:33 Ohad Ben-Cohen wrote: + int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags); ... + The flags parameter is a pointer to where the interrupts state of the + caller will be saved at. This seems to encourage sloppy coding: The

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 5:46 PM, Greg KH g...@kroah.com wrote: On Mon, Oct 18, 2010 at 09:44:33AM +0200, Ohad Ben-Cohen wrote: +#else /* !CONFIG_OMAP_HWSPINLOCK */ + +static inline struct omap_hwspinlock *omap_hwspinlock_request(void) +{ +     return ERR_PTR(-ENOSYS); +} One note, do you

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 6:58 PM, Kevin Hilman khil...@deeprootsystems.com wrote: +      * This spin_trylock_irqsave serves two purposes: + +      * 1. Disable local interrupts and preemption, in order to +      *    minimize the period of time in which the hwspinlock +      *    is taken (so

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 7:01 PM, Grant Likely grant.lik...@secretlab.ca wrote: +  struct omap_hwspinlock *omap_hwspinlock_request(void); ... ERR_PTR() is only appropriate when the caller actually cares about the failure code and has different behaviour depending on the result. Agree; the

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 7:21 PM, Arnd Bergmann a...@arndb.de wrote: On Monday 18 October 2010 09:44:33 Ohad Ben-Cohen wrote: +  int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags); ... +     The flags parameter is a pointer to where the interrupts state of the +    

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Arnd Bergmann
On Tuesday 19 October 2010 22:43:34 Ohad Ben-Cohen wrote: Disabling irqs might be a concern as a source of RT latency. It might be better to make the caller responsible for managing local spin locks and irq disable/enable. This a coming from an hardware requirement, rather than a choice

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Arnd Bergmann
On Tuesday 19 October 2010 22:51:22 Ohad Ben-Cohen wrote: , which is generally discouraged in all places where you know if you need to disable interrupts or not. IMHO the default should be a version that only allows locks that don't get taken at IRQ time and consequently don't require

Re: [PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-19 Thread Ohad Ben-Cohen
On Tue, Oct 19, 2010 at 10:58 PM, Arnd Bergmann a...@arndb.de wrote: If a hwspinlock is taken over a long period of time, its other user (with which we try to achieve synchronization) might be polling the OMAP interconnect for too long (trying to take the hwspinlock) and thus preventing it to

[PATCH 1/3] drivers: misc: add omap_hwspinlock driver

2010-10-18 Thread Ohad Ben-Cohen
From: Simon Que s...@ti.com Add driver for OMAP's Hardware Spinlock module. The OMAP Hardware Spinlock module, initially introduced in OMAP4, provides hardware assistance for synchronization between the multiple processors in the device (Cortex-A9, Cortex-M3 and C64x+ DSP). Signed-off-by: Simon