Linux kernel development

Ohad Ben-Cohen | 18 Oct 2010 09:44

[PATCH 0/3] Add OMAP hardware spinlock misc driver

OMAP4 introduces a Spinlock hardware module, which provides hardware
assistance for synchronization and mutual exclusion between heterogeneous
processors and those not operating under a single, shared operating system
(e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).

The intention of this hardware module is to allow remote processors,
that have no alternative mechanism to accomplish synchronization and mutual
exclusion operations, to share resources (such as memory and/or any other
hardware resource).

This patchset adds a new misc driver for this OMAP hwspinlock module.

Currently there are two users for this driver:

1. Inter-Processor Communications: on OMAP4, cpu-intensive multimedia
tasks are offloaded by the host to the remote M3 and/or C64x+ slave
processors.

To achieve fast message-based communications, a minimal kernel support
is needed to deliver messages arriving from a remote processor to the
appropriate user process.

This communication is based on simple data structures that are shared between
the remote processors, and access to it is synchronized using the hwspinlock
module (to allow the remote processor to directly place new messages in this
shared data structure).

This OMAP IPC system, called Syslink, is being actively developed by TI and
will be gradually submitted upstream.

(Continue reading)

headers

Ohad Ben-Cohen | 18 Oct 2010 09:44

[PATCH 2/3] OMAP4: hwmod data: Add hwspinlock

From: Benoit Cousson <b-cousson <at> ti.com>

Add hwspinlock hwmod data for OMAP4 chip

Signed-off-by: Cousson, Benoit <b-cousson <at> ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Paul Walmsley <paul <at> pwsan.com>
---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   63 ++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 0d5c6eb..07c3654 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
 <at>  <at>  -1043,6 +1043,66  <at>  <at>  static struct omap_hwmod omap44xx_uart4_hwmod = {
 	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
 };

+/*
+ * 'spinlock' class
+ * spinlock provides hardware assistance for synchronizing the processes
+ * running on multiple processors
+ */
+
+static struct omap_hwmod_class_sysconfig omap44xx_spinlock_sysc = {
+	.rev_offs	= 0x0000,
+	.sysc_offs	= 0x0010,
+	.syss_offs	= 0x0014,

(Continue reading)

headers

Ohad Ben-Cohen | 18 Oct 2010 09:44

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

From: Simon Que <sque <at> 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 Que <sque <at> ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Signed-off-by: Krishnamoorthy, Balaji T <balajitk <at> ti.com>
[ohad <at> wizery.com: disable interrupts/preemption to prevent hw abuse]
[ohad <at> wizery.com: add memory barriers to prevent memory reordering issues]
[ohad <at> wizery.com: relax omap interconnect between subsequent lock attempts]
[ohad <at> wizery.com: timeout param to use jiffies instead of number of attempts]
[ohad <at> wizery.com: remove code duplication in lock, trylock, lock_timeout]
[ohad <at> wizery.com: runtime pm usage count to reflect num of requested locks]
[ohad <at> wizery.com: move to drivers/misc, general cleanups, document]
Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Benoit Cousson <b-cousson <at> ti.com>
Cc: Tony Lindgren <tony <at> atomide.com>
---
 Documentation/misc-devices/omap_hwspinlock.txt |  199 +++++++++
 drivers/misc/Kconfig                           |   10 +
 drivers/misc/Makefile                          |    1 +
 drivers/misc/omap_hwspinlock.c                 |  555 ++++++++++++++++++++++++
 include/linux/omap_hwspinlock.h                |  108 +++++
 5 files changed, 873 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/misc-devices/omap_hwspinlock.txt

(Continue reading)

headers

Greg KH | 19 Oct 2010 17:46

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

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, yet you have a driver that is asking for it?  Shouldn't you
instead just silently succeed, and let the code path get compiled away?

We did that for debugfs, after learning the pain that procfs had with
its api for "is not built".  Doing it the way you are requires the user
to always test for -ENOSYS, when in reality, if that is returned,
there's nothing the driver can do about it, so it should just not worry
about it.

Just something to think about.

thanks,

greg k-h

headers

Ohad Ben-Cohen | 19 Oct 2010 22:18

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

On Tue, Oct 19, 2010 at 5:46 PM, Greg KH <greg <at> 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 really want to fail if this option isn't built into the
> kernel, yet you have a driver that is asking for it?  Shouldn't you
> instead just silently succeed, and let the code path get compiled away?
>
> We did that for debugfs, after learning the pain that procfs had with
> its api for "is not built".  Doing it the way you are requires the user
> to always test for -ENOSYS, when in reality, if that is returned,
> there's nothing the driver can do about it, so it should just not worry
> about it.
>
> Just something to think about.

Completely agree; if hwspinlock support is not needed, we better let
its users run uninterruptedly. I'll change it.

Thanks,
Ohad.

>
> thanks,
>

(Continue reading)

headers

Kevin Hilman | 19 Oct 2010 18:58

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

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> From: Simon Que <sque <at> 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).

[...]

> +/**
> + * omap_hwspin_trylock() - attempt to lock a specific hwspinlock
> + *  <at> hwlock: a hwspinlock which we want to trylock
> + *  <at> flags: a pointer to where the caller's interrupt state will be saved at
> + *
> + * This function attempt to lock the underlying hwspinlock. Unlike
> + * hwspinlock_lock, this function will immediately fail if the hwspinlock
> + * is already taken.
> + *
> + * Upon a successful return from this function, preemption and interrupts
> + * are disabled, so the caller must not sleep, and is advised to release
> + * the hwspinlock as soon as possible. This is required in order to minimize
> + * remote cores polling on the hardware interconnect.
> + *
> + * This function can be called from any context.
> + *
> + * Returns 0 if we successfully locked the hwspinlock, -EBUSY if

(Continue reading)

headers

Ohad Ben-Cohen | 19 Oct 2010 22:21

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

On Tue, Oct 19, 2010 at 6:58 PM, Kevin Hilman
<khilman <at> 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 caller will not preempted). This is
>> +      *    important in order to minimize the possible polling on
>> +      *    the hardware interconnect by a remote user of this lock.
>> +      *
>> +      * 2. Make this hwspinlock primitive SMP-safe (so we can try to
>> +      *    take it from additional contexts on the local cpu)
>> +      */
>
> 3. Ensures that in_atomic/might_sleep checks catch potential problems
>   with hwspinlock usage (e.g. scheduler checks like 'scheduling while
>   atomic' etc.)

Nice one. Added.

>> +/**
>> + * omap_hwspinlock_unlock() - unlock a specific hwspinlock
>
> minor nit: s/lock_unlock/_unlock/  to match name below

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(Continue reading)

headers

Grant Likely | 19 Oct 2010 19:01

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

On Mon, Oct 18, 2010 at 1:44 AM, Ohad Ben-Cohen <ohad <at> wizery.com> wrote:
> From: Simon Que <sque <at> 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).

Hi Ohad, A couple of comments below.

>
> Signed-off-by: Simon Que <sque <at> ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
> Signed-off-by: Krishnamoorthy, Balaji T <balajitk <at> ti.com>
> [ohad <at> wizery.com: disable interrupts/preemption to prevent hw abuse]
> [ohad <at> wizery.com: add memory barriers to prevent memory reordering issues]
> [ohad <at> wizery.com: relax omap interconnect between subsequent lock attempts]
> [ohad <at> wizery.com: timeout param to use jiffies instead of number of attempts]
> [ohad <at> wizery.com: remove code duplication in lock, trylock, lock_timeout]
> [ohad <at> wizery.com: runtime pm usage count to reflect num of requested locks]
> [ohad <at> wizery.com: move to drivers/misc, general cleanups, document]
> Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
> Cc: Benoit Cousson <b-cousson <at> ti.com>
> Cc: Tony Lindgren <tony <at> atomide.com>
> ---
>  Documentation/misc-devices/omap_hwspinlock.txt |  199 +++++++++
>  drivers/misc/Kconfig                           |   10 +
>  drivers/misc/Makefile                          |    1 +

(Continue reading)

headers

Ohad Ben-Cohen | 19 Oct 2010 22:43

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

On Tue, Oct 19, 2010 at 7:01 PM, Grant Likely <grant.likely <at> 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 hwspinlock users can surely live without an explicit error
code from the _request APIs, and some extra robustness can only do
good.

I'll remove it.

> 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 the
user should take.

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 be used for other transactions.

To prevent such lengthy polling on the interconnect, the hwspinlock
should only be used for very short period of times, with preemption
and interrupts disabled.

That's why we don't give users the choice whether to disable
interrupts or not - it's simply not a decision they should take.

(Continue reading)

headers

Arnd Bergmann | 19 Oct 2010 22:58

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

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 the
> user should take.
> 
> 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 be used for other transactions.

This sounds exactly like any other spinlock.

> To prevent such lengthy polling on the interconnect, the hwspinlock
> should only be used for very short period of times, with preemption
> and interrupts disabled.

Interrupts disabled in general might go a bit too far -- they are also
short and infrequent events unless you have seriously broken drivers.

When running with CONFIG_PREEMPT, I would guess you actually want to
turn the omap_hwspinlock into a sleeping operation, though that would
require much extra work to implement. Disabling preemption while the
hwspinlock is held is of course a correct implementation technically,
but it might not be what someone enabling CONFIG_PREEMPT expects.

> That's why we don't give users the choice whether to disable
> interrupts or not - it's simply not a decision they should take.

(Continue reading)

headers

Ohad Ben-Cohen | 19 Oct 2010 23:57

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

On Tue, Oct 19, 2010 at 10:58 PM, Arnd Bergmann <arnd <at> 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 be used for other transactions.
>
> This sounds exactly like any other spinlock.

The difference is hardware-specific: the hwspinlock device is located
on the OMAP's L4 interconnect where access is slow, wasteful of power,
and spinning on it may prevent other peripherals from interconnecting.

headers

Kevin Hilman | 19 Oct 2010 19:16

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

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> From: Simon Que <sque <at> 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).

[...]

> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index b743312..ce4b7a6 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
>  <at>  <at>  -390,6 +390,16  <at>  <at>  config BMP085
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bmp085.
>  
> +config OMAP_HWSPINLOCK
> +	bool "OMAP Hardware Spinlock module"

Should be tristate so it can also be built as a module by default.

e.g., when building multi-OMAP kernels, no reason or this to be
built-in.  It can then be loaded as a module on OMAP4 only.

Kevin

(Continue reading)

headers

Ohad Ben-Cohen | 20 Oct 2010 15:00

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

On Tue, Oct 19, 2010 at 7:16 PM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
> Ohad Ben-Cohen <ohad <at> wizery.com> writes:
>
>> From: Simon Que <sque <at> 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).
>
...

>> +config OMAP_HWSPINLOCK
>> +     bool "OMAP Hardware Spinlock module"
>
> Should be tristate

Agree,

> so it can also be built as a module by default.
>
> e.g., when building multi-OMAP kernels, no reason or this to be
> built-in.  It can then be loaded as a module on OMAP4 only.

But considering the current built-in use cases (i2c) we would then
need to have the relevant MACH_OMAP4_* select OMAP_HWSPINLOCK (only on
the OMAP4 machines on which the I2C bus is shared).

(Continue reading)

headers

Kevin Hilman | 20 Oct 2010 20:18

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

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> On Tue, Oct 19, 2010 at 7:16 PM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
>> Ohad Ben-Cohen <ohad <at> wizery.com> writes:
>>
>>> From: Simon Que <sque <at> 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).
>>
> ...
>
>>> +config OMAP_HWSPINLOCK
>>> +     bool "OMAP Hardware Spinlock module"
>>
>> Should be tristate
>
> Agree,
>
>> so it can also be built as a module by default.
>>
>> e.g., when building multi-OMAP kernels, no reason or this to be
>> built-in.  It can then be loaded as a module on OMAP4 only.
>
> But considering the current built-in use cases (i2c) we would then

(Continue reading)

headers

Arnd Bergmann | 19 Oct 2010 19:21

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

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 only variant you allow is the one
that corresponds to Linux's spin_lock_irqsave(), 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 saving the interrupt flags.

	Arnd

headers

Ohad Ben-Cohen | 19 Oct 2010 22:51

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

On Tue, Oct 19, 2010 at 7:21 PM, Arnd Bergmann <arnd <at> 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
>> +     caller will be saved at.
>
> This seems to encourage sloppy coding: The only variant you allow is the one
> that corresponds to Linux's spin_lock_irqsave()

Yes, this is a hardware requirement to minimize the period of time in
which the hwspinlock is taken, in order to prevent lengthy polling on
the interconnect (preventing it from serving other transactions).

>, 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 saving the interrupt flags.

Please note that the hwspinlocks should never be used to achieve
synchronization with Linux contexts running on the same cpu - it's
always about achieving mutual exclusion with a remote processor.

So whether the lock is taken at IRQ time or not does not affect the
requirement to disable interrupts while it is taken (very differently
from local spin_lock{_irqsave} synchronizations).

Thanks,
Ohad.

(Continue reading)

headers

Arnd Bergmann | 19 Oct 2010 23:08

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

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 saving the interrupt flags.
> 
> Please note that the hwspinlocks should never be used to achieve
> synchronization with Linux contexts running on the same cpu - it's
> always about achieving mutual exclusion with a remote processor.

Ok, I see.

> So whether the lock is taken at IRQ time or not does not affect the
> requirement to disable interrupts while it is taken (very differently
> from local spin_lock{_irqsave} synchronizations).

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 know how expensive that is on ARM,
some other architectures take an microseconds to restore the interrupt
enabled flag from a register.

Even in the case where you know that interrupts are enabled, you may
want to avoid saving the interrupt flag to the stack, the simpler
API (one argument instead of two) gives less room for screwing up.

	Arnd

(Continue reading)

headers

Ohad Ben-Cohen | 21 Oct 2010 00:43

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

On Tue, Oct 19, 2010 at 11:08 PM, Arnd Bergmann <arnd <at> 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 know how expensive that is on ARM,
> some other architectures take an microseconds to restore the interrupt
> enabled flag from a register.

To do this we need to introduce a new set of API which will not
disable interrupts, and which should only be used when the caller
knows that interrupts are already disabled.

This will save some cycles, but my concern is that this API will be
abused by drivers, and will eventually be used to take the hwspinlocks
for lengthy period of times (which might even involve sleeping).

Given that the access to the L4 interconnect is anyway slow I also
feel that the gain is negligible.

Nevertheless, it's a viable way to squeeze some cycles.

We don't have a use case in which we know the interrupts are already
disabled, but when we will, we will consider adding such API.

> Even in the case where you know that interrupts are enabled, you may
> want to avoid saving the interrupt flag to the stack, the simpler
> API (one argument instead of two) gives less room for screwing up.

This sounds like adding a set of API that resembles spin_{unlock,lock}_irq.

(Continue reading)

headers

Arnd Bergmann | 21 Oct 2010 11:04

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

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 place will end up enabling
> interrupts where it really shouldn't.
> 
> Don't you feel that proving a simple spin_lock_irqsave-like API is
> actually safer and less error prone ?
> 
> I guess that is one of the reasons why spin_lock_irqsave is much more
> popular than spin_lock_irq - people just know it will never screw up.

People can screw that up in different ways, e.g.

	spin_lock_irqsave(&lock_a, flags);
	spin_lock_irqsave(&lock_b, flags); /* overwrites flags */

	spin_lock_irqrestore(&lock_b, flags);
	spin_lock_irqrestore(&lock_a, flags); /* still disabled! */

I use the presence of spin_lock_irqsave in driver code as an indication
of whether the author had any clue about locking. Most experienced
coders use the right version intuitively, while beginners often just
use _irqsave because they didn't understand the API and they were told
that using this is safe.

	Arnd

(Continue reading)

headers

Ohad Ben-Cohen | 21 Oct 2010 12:13

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

On Thu, Oct 21, 2010 at 11:04 AM, Arnd Bergmann <arnd <at> 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 error prone; a driver which would
>> erroneously use this at the wrong place will end up enabling
>> interrupts where it really shouldn't.
>>
>> Don't you feel that proving a simple spin_lock_irqsave-like API is
>> actually safer and less error prone ?
>>
>> I guess that is one of the reasons why spin_lock_irqsave is much more
>> popular than spin_lock_irq - people just know it will never screw up.
>
> People can screw that up in different ways, e.g.

Sure...

> I use the presence of spin_lock_irqsave in driver code as an indication
> of whether the author had any clue about locking. Most experienced
> coders use the right version intuitively, while beginners often just
> use _irqsave because they didn't understand the API and they were told
> that using this is safe.

Agree.

I'll add the _irq pair of APIs, this will make the users' code
cleaner. This is valuable especially since all of our current users
have their interrupts enabled anyway.

(Continue reading)

headers

Arnd Bergmann | 21 Oct 2010 14:02

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

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 support the
> _irq and _irqsave flavors)
> 

Sounds good!

	Arnd

headers

Tony Lindgren | 22 Oct 2010 19:00

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

* Ohad Ben-Cohen <ohad <at> wizery.com> [101018 00:41]:
> From: Simon Que <sque <at> 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).

...

> +EXPORT_SYMBOL_GPL(omap_hwspin_trylock);
> +EXPORT_SYMBOL_GPL(omap_hwspin_lock_timeout);
> +EXPORT_SYMBOL_GPL(omap_hwspin_unlock);
> +EXPORT_SYMBOL_GPL(omap_hwspinlock_request);
> +EXPORT_SYMBOL_GPL(omap_hwspinlock_request_specific);
> +EXPORT_SYMBOL_GPL(omap_hwspinlock_free);
> +EXPORT_SYMBOL_GPL(omap_hwspinlock_get_id);

Please let's not add yet another omap specific layer that will
make it incrementally harder to have generic drivers.

Instead, we can do the omap specific locking in the
platform code and then the drivers can use the functions
passed in the platform_data if they're implemented.

Regards,

Tony

(Continue reading)

headers

Ohad Ben-Cohen | 18 Oct 2010 09:44

[PATCH 3/3] omap: add hwspinlock device

From: Simon Que <sque <at> ti.com>

Build and register an hwspinlock platform device.

Although only OMAP4 supports the hardware spinlock module (for now),
it is still safe to run this initcall on all omaps, because hwmod lookup
will simply fail on hwspinlock-less platforms.

Signed-off-by: Simon Que <sque <at> ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
Cc: Benoit Cousson <b-cousson <at> ti.com>
---
 arch/arm/mach-omap2/Makefile     |    1 +
 arch/arm/mach-omap2/hwspinlock.c |   67 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/hwspinlock.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 7352412..e55d1c5 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
 <at>  <at>  -190,3 +190,4  <at>  <at>  obj-y					+= $(smc91x-m) $(smc91x-y)

 smsc911x-$(CONFIG_SMSC911X)		:= gpmc-smsc911x.o
 obj-y					+= $(smsc911x-m) $(smsc911x-y)
+obj-$(CONFIG_ARCH_OMAP4)		+= hwspinlock.o
diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c
new file mode 100644
index 0000000..641a6d4

(Continue reading)

headers

Kevin Hilman | 19 Oct 2010 19:03

Re: [PATCH 3/3] omap: add hwspinlock device

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> From: Simon Que <sque <at> ti.com>
>
> Build and register an hwspinlock platform device.
>
> Although only OMAP4 supports the hardware spinlock module (for now),
> it is still safe to run this initcall on all omaps, because hwmod lookup
> will simply fail on hwspinlock-less platforms.
>
> Signed-off-by: Simon Que <sque <at> ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
> Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
> Cc: Benoit Cousson <b-cousson <at> ti.com>
> ---
>  arch/arm/mach-omap2/Makefile     |    1 +
>  arch/arm/mach-omap2/hwspinlock.c |   67 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/hwspinlock.c
>
> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
> index 7352412..e55d1c5 100644
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
>  <at>  <at>  -190,3 +190,4  <at>  <at>  obj-y					+= $(smc91x-m) $(smc91x-y)
>  
>  smsc911x-$(CONFIG_SMSC911X)		:= gpmc-smsc911x.o
>  obj-y					+= $(smsc911x-m) $(smsc911x-y)
> +obj-$(CONFIG_ARCH_OMAP4)		+= hwspinlock.o
> diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c

(Continue reading)

headers

Grant Likely | 19 Oct 2010 19:05

Re: [PATCH 3/3] omap: add hwspinlock device

On Tue, Oct 19, 2010 at 11:03 AM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
> Ohad Ben-Cohen <ohad <at> wizery.com> writes:
>
>> From: Simon Que <sque <at> ti.com>
>>
>> Build and register an hwspinlock platform device.
>>
>> Although only OMAP4 supports the hardware spinlock module (for now),
>> it is still safe to run this initcall on all omaps, because hwmod lookup
>> will simply fail on hwspinlock-less platforms.
>>
>> Signed-off-by: Simon Que <sque <at> ti.com>
>> Signed-off-by: Hari Kanigeri <h-kanigeri2 <at> ti.com>
>> Signed-off-by: Ohad Ben-Cohen <ohad <at> wizery.com>
>> Cc: Benoit Cousson <b-cousson <at> ti.com>
>> ---
>>  arch/arm/mach-omap2/Makefile     |    1 +
>>  arch/arm/mach-omap2/hwspinlock.c |   67 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 68 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-omap2/hwspinlock.c
>>
>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>> index 7352412..e55d1c5 100644
>> --- a/arch/arm/mach-omap2/Makefile
>> +++ b/arch/arm/mach-omap2/Makefile
>>  <at>  <at>  -190,3 +190,4  <at>  <at>  obj-y                                     += $(smc91x-m) $(smc91x-y)
>>
>>  smsc911x-$(CONFIG_SMSC911X)          := gpmc-smsc911x.o
>>  obj-y                                        += $(smsc911x-m) $(smsc911x-y)

(Continue reading)

headers

Ohad Ben-Cohen | 19 Oct 2010 23:02

Re: [PATCH 3/3] omap: add hwspinlock device

On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
>> +postcore_initcall(hwspinlocks_init);
>
> Any reason this needs to be a postcore_initcall?  Are there users of
> hwspinlocks this early in boot?

i2c-omap, which is subsys_initcall (the I2C bus is shared between the
A9 and the M3 on some OMAP4 boards).

And to allow early board code to reserve specific hwspinlock numbers
for predefined use-cases, we probably want to be before arch_initcall.

> The I2C   Probaly subsys or even device_initcall
> is more appropriate here.
>
> I would've suspected that any users of hwspinlocks will be dependent on
> drivers for the other cores (e.g. syslink) which would likely be
> initialized much later.
>
> Kevin
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Grant Likely | 20 Oct 2010 01:12

Re: [PATCH 3/3] omap: add hwspinlock device

On Tue, Oct 19, 2010 at 3:02 PM, Ohad Ben-Cohen <ohad <at> wizery.com> wrote:
> On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
>>> +postcore_initcall(hwspinlocks_init);
>>
>> Any reason this needs to be a postcore_initcall?  Are there users of
>> hwspinlocks this early in boot?
>
> i2c-omap, which is subsys_initcall (the I2C bus is shared between the
> A9 and the M3 on some OMAP4 boards).
>
> And to allow early board code to reserve specific hwspinlock numbers
> for predefined use-cases, we probably want to be before arch_initcall.

Man. this is getting ugly.  I think we need to discuss how to solve
this at the Plumbers micro-conference. It kind of fits in with the
whole embedded (ab)use of the device model topic anyway. Actually,
this particular case isn't bad, but the moving of i2c and spi busses
to an earlier initcall is just band-aiding the real problem of driver
probe order dependencies.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Ohad Ben-Cohen | 20 Oct 2010 16:09

Re: [PATCH 3/3] omap: add hwspinlock device

On Wed, Oct 20, 2010 at 1:12 AM, Grant Likely <grant.likely <at> secretlab.ca> wrote:
> On Tue, Oct 19, 2010 at 3:02 PM, Ohad Ben-Cohen <ohad <at> wizery.com> wrote:
...
>> i2c-omap, which is subsys_initcall (the I2C bus is shared between the
>> A9 and the M3 on some OMAP4 boards).
...
> Man. this is getting ugly.  I think we need to discuss how to solve
> this at the Plumbers micro-conference. It kind of fits in with the
> whole embedded (ab)use of the device model topic anyway. Actually,
> this particular case isn't bad, but the moving of i2c and spi busses
> to an earlier initcall is just band-aiding the real problem of driver
> probe order dependencies.

+1

On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
> Rather than moving towards having more drivers have to be built in (and
> depend on their probe order) we need to be moving towards building all
> these drivers as modules, including omap-i2c.

+1

This whole thing is a mess, and today it's being solved in the wrong,
non-scalable and error-prone way.

The question is whether we want to gate hwspinlock until this issue is solved ?

On Wed, Oct 20, 2010 at 3:20 AM, Ryan Mallon <ryan <at> bluewatersys.com> wrote:
> The issue of probe order still needs to be resolved for those of us who

(Continue reading)

headers

Grant Likely | 20 Oct 2010 17:51

Re: [PATCH 3/3] omap: add hwspinlock device

On Wed, Oct 20, 2010 at 04:09:22PM +0200, Ohad Ben-Cohen wrote:
> On Wed, Oct 20, 2010 at 1:12 AM, Grant Likely <grant.likely <at> secretlab.ca> wrote:
> > On Tue, Oct 19, 2010 at 3:02 PM, Ohad Ben-Cohen <ohad <at> wizery.com> wrote:
> ...
> >> i2c-omap, which is subsys_initcall (the I2C bus is shared between the
> >> A9 and the M3 on some OMAP4 boards).
> ...
> > Man. this is getting ugly.  I think we need to discuss how to solve
> > this at the Plumbers micro-conference. It kind of fits in with the
> > whole embedded (ab)use of the device model topic anyway. Actually,
> > this particular case isn't bad, but the moving of i2c and spi busses
> > to an earlier initcall is just band-aiding the real problem of driver
> > probe order dependencies.
> 
> +1



> 
> On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
> > Rather than moving towards having more drivers have to be built in (and
> > depend on their probe order) we need to be moving towards building all
> > these drivers as modules, including omap-i2c.
> 
> +1

-1. Like Ryan points out below, the problem isn't modules vs.
built-in, it is drivers depending on implicit init order which sort of
works, but is fragile and will break in subtle ways. It needs to work

(Continue reading)

headers

Kevin Hilman | 20 Oct 2010 01:53

Re: [PATCH 3/3] omap: add hwspinlock device

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
>>> +postcore_initcall(hwspinlocks_init);
>>
>> Any reason this needs to be a postcore_initcall?  Are there users of
>> hwspinlocks this early in boot?
>
> i2c-omap, which is subsys_initcall (the I2C bus is shared between the
> A9 and the M3 on some OMAP4 boards).

Rather than moving towards having more drivers have to be built in (and
depend on their probe order) we need to be moving towards building all
these drivers as modules, including omap-i2c.

> And to allow early board code to reserve specific hwspinlock numbers
> for predefined use-cases, we probably want to be before arch_initcall.

There's no reason for board code to have to do this at initcall time.

This kind of thing needs to be done by platform_data function pointers,
as is done for every other driver that needs platform-specific driver
customization.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(Continue reading)

headers

Ryan Mallon | 20 Oct 2010 03:20

Re: [PATCH 3/3] omap: add hwspinlock device

On 10/20/2010 12:53 PM, Kevin Hilman wrote:
> Ohad Ben-Cohen <ohad <at> wizery.com> writes:
> 
>> On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
>> <khilman <at> deeprootsystems.com> wrote:
>>>> +postcore_initcall(hwspinlocks_init);
>>>
>>> Any reason this needs to be a postcore_initcall?  Are there users of
>>> hwspinlocks this early in boot?
>>
>> i2c-omap, which is subsys_initcall (the I2C bus is shared between the
>> A9 and the M3 on some OMAP4 boards).
> 
> Rather than moving towards having more drivers have to be built in (and
> depend on their probe order) we need to be moving towards building all
> these drivers as modules, including omap-i2c.

The issue of probe order still needs to be resolved for those of us who
do want all the drivers built into the kernel. Everything comes out in
the wash already if everything is built as modules by installing the
modules in the correct order right?

~Ryan

--

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan <at> bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand

(Continue reading)

headers

Ohad Ben-Cohen | 20 Oct 2010 16:38

Re: [PATCH 3/3] omap: add hwspinlock device

On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
>> And to allow early board code to reserve specific hwspinlock numbers
>> for predefined use-cases, we probably want to be before arch_initcall.
>
> There's no reason for board code to have to do this at initcall time.

If we want to have allow both allocations of predefined hwspinlocks
with omap_hwspinlock_request_specific(int), and dynamic allocations
(where we don't care about the specific instance of the hwspinlock we
will get) with omap_hwspinlock_request(), we must ensure that the
former _specific() API will never be called after the latter.

If we will allow drivers to call omap_hwspinlock_request() before all
callers of omap_hwspinlock_request_specific() completed, then things
will break (because drivers might start getting hwspinlocks that are
predefined for dedicated use cases on the system).

So if we want the _specific API to work, we can only allow early board
code to use it in order to reserve those predefined hwspinlocks before
drivers get the chance to call omap_hwspinlock_request().

The tempting alternative is not to provide the
omap_hwspinlock_request_specific() API at all (which is something we
discussed internally).

Let's take the i2c-omap for example.

It sounds like it must have a predefined hwspinlock, but what if:

(Continue reading)

headers

Grant Likely | 20 Oct 2010 17:55

Re: [PATCH 3/3] omap: add hwspinlock device

On Wed, Oct 20, 2010 at 04:38:32PM +0200, Ohad Ben-Cohen wrote:
> On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
> >> And to allow early board code to reserve specific hwspinlock numbers
> >> for predefined use-cases, we probably want to be before arch_initcall.
> >
> > There's no reason for board code to have to do this at initcall time.
> 
> If we want to have allow both allocations of predefined hwspinlocks
> with omap_hwspinlock_request_specific(int), and dynamic allocations
> (where we don't care about the specific instance of the hwspinlock we
> will get) with omap_hwspinlock_request(), we must ensure that the
> former _specific() API will never be called after the latter.
> 
> If we will allow drivers to call omap_hwspinlock_request() before all
> callers of omap_hwspinlock_request_specific() completed, then things
> will break (because drivers might start getting hwspinlocks that are
> predefined for dedicated use cases on the system).
> 
> So if we want the _specific API to work, we can only allow early board
> code to use it in order to reserve those predefined hwspinlocks before
> drivers get the chance to call omap_hwspinlock_request().
> 
> The tempting alternative is not to provide the
> omap_hwspinlock_request_specific() API at all (which is something we
> discussed internally).
> 
> Let's take the i2c-omap for example.
> 
> It sounds like it must have a predefined hwspinlock, but what if:

(Continue reading)

headers

Kevin Hilman | 20 Oct 2010 20:37

Re: [PATCH 3/3] omap: add hwspinlock device

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
>>> And to allow early board code to reserve specific hwspinlock numbers
>>> for predefined use-cases, we probably want to be before arch_initcall.
>>
>> There's no reason for board code to have to do this at initcall time.
>
> If we want to have allow both allocations of predefined hwspinlocks
> with omap_hwspinlock_request_specific(int), and dynamic allocations
> (where we don't care about the specific instance of the hwspinlock we
> will get) with omap_hwspinlock_request(), we must ensure that the
> former _specific() API will never be called after the latter.
>
> If we will allow drivers to call omap_hwspinlock_request() before all
> callers of omap_hwspinlock_request_specific() completed, then things
> will break (because drivers might start getting hwspinlocks that are
> predefined for dedicated use cases on the system).
>
> So if we want the _specific API to work, we can only allow early board
> code to use it in order to reserve those predefined hwspinlocks before
> drivers get the chance to call omap_hwspinlock_request().
>
> The tempting alternative is not to provide the
> omap_hwspinlock_request_specific() API at all (which is something we
> discussed internally).
>
> Let's take the i2c-omap for example.
>

(Continue reading)

headers

Ohad Ben-Cohen | 20 Oct 2010 21:21

Re: [PATCH 3/3] omap: add hwspinlock device

On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
>> Let's take the i2c-omap for example.
>>
>> It sounds like it must have a predefined hwspinlock, but what if:
>>
>> 1. It will use omap_hwspinlock_request() to dynamically allocate a hwspinlock
>> 2. Obviously, the hwspinlock id number must be communicated to the M3
>> BIOS, so the i2c-omap will publish that id using a small shared memory
>> entry that will be allocated for this sole purpose
>> 3. we will make sure that 1+2 completes before the remote processor is
>> taken out of reset
>>
>> This does not require any smart IPC and it will allow us to get rid of
>> the omap_hwspinlock_request_specific() API and its early-callers
>> requirement.
>
> Yes, that would indeed simplify things.

Balaji, Nishant, are you OK with this ?

It means that the I2C driver will dynamically get a hwspinlock and
then it would need to announce the id of that hwspinlock before the M3
is taken out of reset.

It will help us get rid of static allocations that will never scale
nicely and static vs. dynamic allocation races.

>
>> All we will be left to take care of is the order of the ->probe()

(Continue reading)

headers

Kevin Hilman | 21 Oct 2010 01:58

Re: [PATCH 3/3] omap: add hwspinlock device

Ohad Ben-Cohen <ohad <at> wizery.com> writes:

> On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
>>> Let's take the i2c-omap for example.
>>>
>>> It sounds like it must have a predefined hwspinlock, but what if:
>>>
>>> 1. It will use omap_hwspinlock_request() to dynamically allocate a hwspinlock
>>> 2. Obviously, the hwspinlock id number must be communicated to the M3
>>> BIOS, so the i2c-omap will publish that id using a small shared memory
>>> entry that will be allocated for this sole purpose
>>> 3. we will make sure that 1+2 completes before the remote processor is
>>> taken out of reset
>>>
>>> This does not require any smart IPC and it will allow us to get rid of
>>> the omap_hwspinlock_request_specific() API and its early-callers
>>> requirement.
>>
>> Yes, that would indeed simplify things.
>
> Balaji, Nishant, are you OK with this ?
>
> It means that the I2C driver will dynamically get a hwspinlock and
> then it would need to announce the id of that hwspinlock before the M3
> is taken out of reset.
>
> It will help us get rid of static allocations that will never scale
> nicely and static vs. dynamic allocation races.
>

(Continue reading)

headers

Ohad Ben-Cohen | 21 Oct 2010 08:11

Re: [PATCH 3/3] omap: add hwspinlock device

[removed that bouncing email address we had all along :]

On Thu, Oct 21, 2010 at 1:58 AM, Kevin Hilman
<khilman <at> deeprootsystems.com> wrote:
>>>> Why would we need platform-specific function pointers here ? I'm not
>>>> sure I'm following this one.
>>>
>>> So that board code (built-in) does not call the hwspinlock driver
>>> (potentially a module.)
>>>
>>> The way to solve this is to have platform_data with function pointers,
>>> so that when the driver's ->probe() is done, it can call cany custom
>>> hooks registered by the board code.
>>
>> Sorry, still not following.
>>
>> Do you refer to the i2c driver calling the hwspinlcok_request function
>> at probe time via platform_data function pointers ?
>
> No, earlier in this discussion, in response to my question about users
> of this API early in boot, you said:
>
>> And to allow early board code to reserve specific hwspinlock numbers
>> for predefined use-cases, we probably want to be before arch_initcall.

yes.

> My suggestion to use platform_data + function pointers was to address
> the requesting of hwspinlocks in board/platform-specific code.
>

(Continue reading)

headers

Kamoolkar, Mugdha | 21 Oct 2010 10:36

RE: [PATCH 3/3] omap: add hwspinlock device

> <khilman <at> deeprootsystems.com> wrote:
> >> Let's take the i2c-omap for example.
> >>
> >> It sounds like it must have a predefined hwspinlock, but what if:
> >>
> >> 1. It will use omap_hwspinlock_request() to dynamically allocate
> a hwspinlock
> >> 2. Obviously, the hwspinlock id number must be communicated to
> the M3
> >> BIOS, so the i2c-omap will publish that id using a small shared
> memory
> >> entry that will be allocated for this sole purpose
> >> 3. we will make sure that 1+2 completes before the remote
> processor is
> >> taken out of reset
> >>
> >> This does not require any smart IPC and it will allow us to get
> rid of
> >> the omap_hwspinlock_request_specific() API and its early-callers
> >> requirement.
> >
> > Yes, that would indeed simplify things.
> 
> Balaji, Nishant, are you OK with this ?
> 
The problem with this approach is that the i2c driver would have to 
sync up on the shared memory location that it uses to share the 
information of the spinlock ID. What location would this be? How would 
the i2c driver on the m3 know about this location? Does this mean 
hard-coding of the shared memory address? If yes, then there would be

(Continue reading)

headers

Ohad Ben-Cohen | 21 Oct 2010 11:06

Re: [PATCH 3/3] omap: add hwspinlock device

On Thu, Oct 21, 2010 at 10:36 AM, Kamoolkar, Mugdha <mugdha <at> ti.com> wrote:
>> <khilman <at> deeprootsystems.com> wrote:
>> > Yes, that would indeed simplify things.
>>
>> Balaji, Nishant, are you OK with this ?
>>
> The problem with this approach is that the i2c driver would have to
> sync up on the shared memory location that it uses to share the
> information of the spinlock ID.

I agree.

But we seem to have this sort of problem anyway:

Since we are forbidden to take a hwspinlock over a lengthy period of
time, i2c-omap can't really use it directly to achieve mutual
exclusion over the entire i2c transfer (which is long and involved
sleeping).

It was suggested that i2c-omap would only use the hwspinlock to
protect a shared memory entry which would indicate the owner of the
i2c bus. Either that, or use something like Peterson's mutual
exclusion algorithm which is entirely implemented in software.

Both of the latter approaches involves sync'ing up on a shared memory
location..  so it seems like i2c-omap anyway has to deal with this
kind of pain, and having a predefined hwspinlock id number doesn't
relieve it.

What do you think ?

(Continue reading)

headers

Kamoolkar, Mugdha | 22 Oct 2010 11:59

RE: [PATCH 3/3] omap: add hwspinlock device

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad <at> wizery.com]
> Sent: Thursday, October 21, 2010 2:37 PM
> To: Kamoolkar, Mugdha
> Cc: Kevin Hilman; Krishnamoorthy, Balaji T; Kamat, Nishant; linux-
> omap <at> vger.kernel.org; linux-kernel <at> vger.kernel.org; linux-arm-
> kernel <at> lists.infradead.org; akpm <at> linux-foundation.org; Greg KH; Tony
> Lindgren; Cousson, Benoit; Grant Likely; Kanigeri, Hari; Anna, Suman
> Subject: Re: [PATCH 3/3] omap: add hwspinlock device
> 
> On Thu, Oct 21, 2010 at 10:36 AM, Kamoolkar, Mugdha <mugdha <at> ti.com>
> wrote:
> >> <khilman <at> deeprootsystems.com> wrote:
> >> > Yes, that would indeed simplify things.
> >>
> >> Balaji, Nishant, are you OK with this ?
> >>
> > The problem with this approach is that the i2c driver would have to
> > sync up on the shared memory location that it uses to share the
> > information of the spinlock ID.
> 
> I agree.
> 
> But we seem to have this sort of problem anyway:
> 
> Since we are forbidden to take a hwspinlock over a lengthy period of
> time, i2c-omap can't really use it directly to achieve mutual
> exclusion over the entire i2c transfer (which is long and involved
> sleeping).
>

(Continue reading)

headers

Ohad Ben-Cohen | 22 Oct 2010 13:16

Re: [PATCH 3/3] omap: add hwspinlock device

On Fri, Oct 22, 2010 at 11:59 AM, Kamoolkar, Mugdha <mugdha <at> ti.com> wrote:
>> From: Ohad Ben-Cohen [mailto:ohad <at> wizery.com]
>> I agree.
>>
>> But we seem to have this sort of problem anyway:
>>
...
> That is true. Perhaps we should consider adding a software
> implementation over HW spinlocks.

Yes, this is probably inevitable.

It would also be useful for the user space implementation of TI's
RingIO and FrameQueue protocols coming in Syslink 3.0, which will also
not be able to directly use the hwspinlock because of the same
reasons.

This layer would maintain the owner of each (virtual) lock in a
non-cacheable shared memory entry, together with the id of the
hwspinlock that would protect that 'owner' entry.

This way we no longer need to use predefined hwspinlocks (the id of
the hwspinlock is announced in the shared memory entry). So we can
ditch the _request_specific() API, and we can move to arch_initcall
(just to beat the current race with i2c-omap).

> We were anyway considering doing this,
> because the number of hw spinlocks available for our usage are not
> sufficient when we look at multi-channel use-cases.

(Continue reading)

headers

Kanigeri, Hari | 21 Oct 2010 14:26

RE: [PATCH 3/3] omap: add hwspinlock device

Mugdha,

> > >>
> > >> This does not require any smart IPC and it will allow us to get
> > rid of
> > >> the omap_hwspinlock_request_specific() API and its early-callers
> > >> requirement.
> > >
> > > Yes, that would indeed simplify things.
> >
> > Balaji, Nishant, are you OK with this ?
> >
> The problem with this approach is that the i2c driver would have to
> sync up on the shared memory location that it uses to share the
> information of the spinlock ID. What location would this be? How would
> the i2c driver on the m3 know about this location? Does this mean
> hard-coding of the shared memory address? If yes, then there would be
> an impact to users if they wanted to change their memory map. Any kind
> of hard-coding of this sort can be painful in such cases, especially
> if it happens in multiple drivers. On the other hand, hard-coding
> (reserving) spinlock IDs is a much safer option and does not impact
> anything else.
> 

We can avoid the hard-coding of shared memory location if I2C Driver maps using iommu some dynamic memory
for shared memory location to M3 virtual address and shares this information to I2c driver counter-part
on M3 using the IPC call. But this might not be trivial and this would be against what Ohad mentioned about
not requiring any smart IPC :).
I prefer hard-coding of spinlock id to keep things simple.

(Continue reading)

headers

Kamoolkar, Mugdha | 22 Oct 2010 12:14

RE: [PATCH 3/3] omap: add hwspinlock device

Hari,

> -----Original Message-----
> From: Kanigeri, Hari
> Sent: Thursday, October 21, 2010 5:56 PM
> To: Kamoolkar, Mugdha; 'Ohad Ben-Cohen'; Kevin Hilman; Krishnamoorthy,
> Balaji T; Kamat, Nishant
> Cc: linux-omap <at> vger.kernel.org; linux-kernel <at> vger.kernel.org; linux-
> arm-kernel <at> lists.infradead.org; akpm <at> linux-foundation.org; Greg KH;
> Tony Lindgren; Cousson, Benoit; Grant Likely; Anna, Suman; Simon Que
> Subject: RE: [PATCH 3/3] omap: add hwspinlock device
> 
> Mugdha,
> 
> > > >>
> > > >> This does not require any smart IPC and it will allow us to get
> > > rid of
> > > >> the omap_hwspinlock_request_specific() API and its early-
> callers
> > > >> requirement.
> > > >
> > > > Yes, that would indeed simplify things.
> > >
> > > Balaji, Nishant, are you OK with this ?
> > >
> > The problem with this approach is that the i2c driver would have to
> > sync up on the shared memory location that it uses to share the
> > information of the spinlock ID. What location would this be? How
> would
> > the i2c driver on the m3 know about this location? Does this mean

(Continue reading)

headers

Tony Lindgren | 22 Oct 2010 18:56

Re: [PATCH 3/3] omap: add hwspinlock device

* Ohad Ben-Cohen <ohad <at> wizery.com> [101020 12:12]:
> On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> <khilman <at> deeprootsystems.com> wrote:
> >> Let's take the i2c-omap for example.
> >>
> >> It sounds like it must have a predefined hwspinlock, but what if:
> >>
> >> 1. It will use omap_hwspinlock_request() to dynamically allocate a hwspinlock
> >> 2. Obviously, the hwspinlock id number must be communicated to the M3
> >> BIOS, so the i2c-omap will publish that id using a small shared memory
> >> entry that will be allocated for this sole purpose
> >> 3. we will make sure that 1+2 completes before the remote processor is
> >> taken out of reset

Guys, let's try to come up with a generic interface for this instead of
polluting the device drivers with more omap specific interfaces.

We really want to keep the drivers generic and platform independent.

Sure we still have some omap specific stuff in the drivers, like
cpu_is_omapxxxx, and omap specific dma calls, but those will be going
away.

Unless somebody has better ideas, I suggest we pass a lock function
in the platform_data to the drivers that need it, and do the omap
specific nasty stuff in the platform code.

Regards,

Tony

(Continue reading)

headers

Grant Likely | 22 Oct 2010 19:03

Re: [PATCH 3/3] omap: add hwspinlock device

On Fri, Oct 22, 2010 at 09:56:13AM -0700, Tony Lindgren wrote:
> * Ohad Ben-Cohen <ohad <at> wizery.com> [101020 12:12]:
> > On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> > <khilman <at> deeprootsystems.com> wrote:
> > >> Let's take the i2c-omap for example.
> > >>
> > >> It sounds like it must have a predefined hwspinlock, but what if:
> > >>
> > >> 1. It will use omap_hwspinlock_request() to dynamically allocate a hwspinlock
> > >> 2. Obviously, the hwspinlock id number must be communicated to the M3
> > >> BIOS, so the i2c-omap will publish that id using a small shared memory
> > >> entry that will be allocated for this sole purpose
> > >> 3. we will make sure that 1+2 completes before the remote processor is
> > >> taken out of reset
> 
> Guys, let's try to come up with a generic interface for this instead of
> polluting the device drivers with more omap specific interfaces.
> 
> We really want to keep the drivers generic and platform independent.
> 
> Sure we still have some omap specific stuff in the drivers, like
> cpu_is_omapxxxx, and omap specific dma calls, but those will be going
> away.
> 
> Unless somebody has better ideas, I suggest we pass a lock function
> in the platform_data to the drivers that need it, and do the omap
> specific nasty stuff in the platform code.

For those of you going to plumbers, I'll put this on the embedded
microconference agenda when we're talking about common infrastructure.

(Continue reading)

headers

Tony Lindgren | 22 Oct 2010 19:28

Re: [PATCH 3/3] omap: add hwspinlock device

* Grant Likely <grant.likely <at> secretlab.ca> [101022 09:54]:
> On Fri, Oct 22, 2010 at 09:56:13AM -0700, Tony Lindgren wrote:
> > * Ohad Ben-Cohen <ohad <at> wizery.com> [101020 12:12]:
> > > On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> > > <khilman <at> deeprootsystems.com> wrote:
> > > >> Let's take the i2c-omap for example.
> > > >>
> > > >> It sounds like it must have a predefined hwspinlock, but what if:
> > > >>
> > > >> 1. It will use omap_hwspinlock_request() to dynamically allocate a hwspinlock
> > > >> 2. Obviously, the hwspinlock id number must be communicated to the M3
> > > >> BIOS, so the i2c-omap will publish that id using a small shared memory
> > > >> entry that will be allocated for this sole purpose
> > > >> 3. we will make sure that 1+2 completes before the remote processor is
> > > >> taken out of reset
> > 
> > Guys, let's try to come up with a generic interface for this instead of
> > polluting the device drivers with more omap specific interfaces.
> > 
> > We really want to keep the drivers generic and platform independent.
> > 
> > Sure we still have some omap specific stuff in the drivers, like
> > cpu_is_omapxxxx, and omap specific dma calls, but those will be going
> > away.
> > 
> > Unless somebody has better ideas, I suggest we pass a lock function
> > in the platform_data to the drivers that need it, and do the omap
> > specific nasty stuff in the platform code.
> 
> For those of you going to plumbers, I'll put this on the embedded

(Continue reading)

headers

Ohad Ben-Cohen | 24 Oct 2010 19:54

Re: [PATCH 3/3] omap: add hwspinlock device

Hi Tony,

On Fri, Oct 22, 2010 at 6:56 PM, Tony Lindgren <tony <at> atomide.com> wrote:
> Guys, let's try to come up with a generic interface for this instead of
> polluting the device drivers with more omap specific interfaces.
>
> We really want to keep the drivers generic and platform independent.

I share this concern as well.

We basically have three options here:

1. Have the hwspinlock driver inside the omap folders and use pdata func ptrs
2. Have a generic hwspinlock framework, with a small omap-specific
implementation
3. Have a misc driver which is currently omap specific, but can very
easily be converted to a common framework

I don't like (1) so much; it's a driver that has very little that is
hardware specific (mainly just the two lines that actually access the
hardware - the lock and the unlock), and it's growing. E.g., we will
need to add it a user space interface (to allow userland IPC
implementations), we will need to add it a "virtual" locks layer that
would provide more locks than the hardware currently has, etc..

In addition, there seem to be a general discontent about drivers
piling up in the ARM folders, instead of having them visible in
drivers/ so they can become useful for other platforms as well.

Here's something Linus wrote about this awhile back:

(Continue reading)

headers

Tony Lindgren | 25 Oct 2010 21:02

Re: [PATCH 3/3] omap: add hwspinlock device

* Ohad Ben-Cohen <ohad <at> wizery.com> [101024 10:45]:
> Hi Tony,
> 
> On Fri, Oct 22, 2010 at 6:56 PM, Tony Lindgren <tony <at> atomide.com> wrote:
> > Guys, let's try to come up with a generic interface for this instead of
> > polluting the device drivers with more omap specific interfaces.
> >
> > We really want to keep the drivers generic and platform independent.
> 
> I share this concern as well.
> 
> We basically have three options here:
> 
> 1. Have the hwspinlock driver inside the omap folders and use pdata func ptrs
> 2. Have a generic hwspinlock framework, with a small omap-specific
> implementation
> 3. Have a misc driver which is currently omap specific, but can very
> easily be converted to a common framework
> 
> I don't like (1) so much; it's a driver that has very little that is
> hardware specific (mainly just the two lines that actually access the
> hardware - the lock and the unlock), and it's growing. E.g., we will
> need to add it a user space interface (to allow userland IPC
> implementations), we will need to add it a "virtual" locks layer that
> would provide more locks than the hardware currently has, etc..
> 
> In addition, there seem to be a general discontent about drivers
> piling up in the ARM folders, instead of having them visible in
> drivers/ so they can become useful for other platforms as well.
>

(Continue reading)

headers

Ohad Ben-Cohen | 26 Oct 2010 13:54

Re: [PATCH 3/3] omap: add hwspinlock device

On Mon, Oct 25, 2010 at 9:02 PM, Tony Lindgren <tony <at> atomide.com> wrote:
>> if you feel that (2) is justifiable/desirable, I would be more
>> than happy to submit that version.
>
> Yes (2) please. I would assume there will be more use of this. And then
> we (or probably me again!) don't have to deal with cleaning up the drivers
> again in the future.

Sounds good.

>> Or do you mean a variation of (2) with only the specific locking bits
>> coming from pdata func pointers ? I guess that in this case we just
>> might as well go with the full (2).
>
> Yes variation of (2) where you only pass the locking function via
> platform data would be best.

It feels a bit funky to me because we would still have code that is
omap-specific inside the "common" probe()/remove() calls.

I suggest to move everything that is omap-specific to a small omap
module that, once probed, would register itself with the common
hwspinlock framework (after initializing its hardware).

That small platfom-specific module probably doesn't have to sit in the
arch/ folder; we can follow established conventions like
mmc/i2c/gpio/spi/etc..

With that in hand, the hwspinlock would really be hardware-agnostic,
and then applying s/omap_hwspin/hwspin/ would be justified.

(Continue reading)

headers

Tony Lindgren | 26 Oct 2010 21:06

Re: [PATCH 3/3] omap: add hwspinlock device

* Ohad Ben-Cohen <ohad <at> wizery.com> [101026 04:45]:
> On Mon, Oct 25, 2010 at 9:02 PM, Tony Lindgren <tony <at> atomide.com> wrote:
> >> if you feel that (2) is justifiable/desirable, I would be more
> >> than happy to submit that version.
> >
> > Yes (2) please. I would assume there will be more use of this. And then
> > we (or probably me again!) don't have to deal with cleaning up the drivers
> > again in the future.
> 
> Sounds good.
> 
> >> Or do you mean a variation of (2) with only the specific locking bits
> >> coming from pdata func pointers ? I guess that in this case we just
> >> might as well go with the full (2).
> >
> > Yes variation of (2) where you only pass the locking function via
> > platform data would be best.
> 
> It feels a bit funky to me because we would still have code that is
> omap-specific inside the "common" probe()/remove() calls.
> 
> I suggest to move everything that is omap-specific to a small omap
> module that, once probed, would register itself with the common
> hwspinlock framework (after initializing its hardware).
> 
> That small platfom-specific module probably doesn't have to sit in the
> arch/ folder; we can follow established conventions like
> mmc/i2c/gpio/spi/etc..
> 
> With that in hand, the hwspinlock would really be hardware-agnostic,

(Continue reading)

headers

Peter Zijlstra | 18 Oct 2010 14:46

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
> OMAP4 introduces a Spinlock hardware module, which provides hardware
> assistance for synchronization and mutual exclusion between heterogeneous
> processors and those not operating under a single, shared operating system
> (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP). 

I just have to ask... was it really easier to build silicon than to
agree on a spinlock ABI?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Russell King - ARM Linux | 18 Oct 2010 15:35

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, Oct 18, 2010 at 02:46:55PM +0200, Peter Zijlstra wrote:
> On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
> > OMAP4 introduces a Spinlock hardware module, which provides hardware
> > assistance for synchronization and mutual exclusion between heterogeneous
> > processors and those not operating under a single, shared operating system
> > (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP). 
> 
> I just have to ask... was it really easier to build silicon than to
> agree on a spinlock ABI?

I'm not really sure what point you're trying to make, but if you're
suggesting that Linux's spinlock should be exposed to these other
processors, you're completely off your rocker.

Doing so would set the kernels spinlock API in stone, which is really
something you don't want to do.  Not only that, but it would mean that
software written for the M3 and DSP would have to know about the GPL'd
spinlock layout, and I suspect that would cause major licencing headaches.

In any case, Linux's spinlock API (or more accurately, the ARM exclusive
access instructions) relies upon hardware coherency support (a piece of
hardware called an exclusive monitor) which isn't present on the M3 nor
DSP processors.  So there's no way to ensure that updates from the M3
and DSP are atomic wrt the A9 updates.

headers

Peter Zijlstra | 18 Oct 2010 15:43

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 14:35 +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 18, 2010 at 02:46:55PM +0200, Peter Zijlstra wrote:
> > On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
> > > OMAP4 introduces a Spinlock hardware module, which provides hardware
> > > assistance for synchronization and mutual exclusion between heterogeneous
> > > processors and those not operating under a single, shared operating system
> > > (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP). 
> > 
> > I just have to ask... was it really easier to build silicon than to
> > agree on a spinlock ABI?
> 
> I'm not really sure what point you're trying to make, but if you're
> suggesting that Linux's spinlock should be exposed to these other
> processors, you're completely off your rocker.

Of course not, that would indeed be utterly silly, nor would it serve
any purpose, the Linux kernel spinlocks are internal spinlocks and need
not interact with anything out side of it.

But for the purpose of communicating with a heterogeneous CPU/DSP it
would make perfect sense to specify a spinlock ABI. Creating specific
hardware just to serialise between these components seems like overkill.

> In any case, Linux's spinlock API (or more accurately, the ARM exclusive
> access instructions) relies upon hardware coherency support (a piece of
> hardware called an exclusive monitor) which isn't present on the M3 nor
> DSP processors.  So there's no way to ensure that updates from the M3
> and DSP are atomic wrt the A9 updates.

Right, so the problem is that there simply is no way to do atomic memory

(Continue reading)

headers

Ohad Ben-Cohen | 18 Oct 2010 16:28

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, Oct 18, 2010 at 3:43 PM, Peter Zijlstra <peterz <at> infradead.org> wrote:
> Right, so the problem is that there simply is no way to do atomic memory
> access from these auxiliary processing units wrt the main CPU?

Yes. There are a few relevant system-wide limitations, one of them is
that simply the system interconnect does not support those fancy
atomic operations.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Peter Zijlstra | 18 Oct 2010 16:33

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 16:28 +0200, Ohad Ben-Cohen wrote:
> On Mon, Oct 18, 2010 at 3:43 PM, Peter Zijlstra <peterz <at> infradead.org> wrote:
> > Right, so the problem is that there simply is no way to do atomic memory
> > access from these auxiliary processing units wrt the main CPU?
> 
> Yes. There are a few relevant system-wide limitations, one of them is
> that simply the system interconnect does not support those fancy
> atomic operations.

Does it support full memory coherency though? Otherwise I can see memory
based message passing becoming rather interesting.

Without coherency everybody needs to be damn sure to flush the relevant
bits before unlocking and such.

headers

Ohad Ben-Cohen | 18 Oct 2010 16:39

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, Oct 18, 2010 at 4:33 PM, Peter Zijlstra <peterz <at> infradead.org> wrote:
> Without coherency everybody needs to be damn sure to flush the relevant
> bits before unlocking and such.

Yes, either that, or use non-cacheable memory.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Catalin Marinas | 18 Oct 2010 17:27

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

Peter Zijlstra <peterz <at> infradead.org> wrote:
> On Mon, 2010-10-18 at 14:35 +0100, Russell King - ARM Linux wrote:
>> In any case, Linux's spinlock API (or more accurately, the ARM exclusive
>> access instructions) relies upon hardware coherency support (a piece of
>> hardware called an exclusive monitor) which isn't present on the M3 nor
>> DSP processors.  So there's no way to ensure that updates from the M3
>> and DSP are atomic wrt the A9 updates.
>
> Right, so the problem is that there simply is no way to do atomic memory
> access from these auxiliary processing units wrt the main CPU? Seeing as
> they operate on the same memory space, wouldn't it make sense to have
> them cache-coherent and thus provide atomicy guarantees through that?

With cache coherency you may get atomicity of writes or reads but
usually not atomic modifications.

--

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Peter Zijlstra | 18 Oct 2010 17:32

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 16:27 +0100, Catalin Marinas wrote:
> Peter Zijlstra <peterz <at> infradead.org> wrote:
> > On Mon, 2010-10-18 at 14:35 +0100, Russell King - ARM Linux wrote:
> >> In any case, Linux's spinlock API (or more accurately, the ARM exclusive
> >> access instructions) relies upon hardware coherency support (a piece of
> >> hardware called an exclusive monitor) which isn't present on the M3 nor
> >> DSP processors.  So there's no way to ensure that updates from the M3
> >> and DSP are atomic wrt the A9 updates.
> >
> > Right, so the problem is that there simply is no way to do atomic memory
> > access from these auxiliary processing units wrt the main CPU? Seeing as
> > they operate on the same memory space, wouldn't it make sense to have
> > them cache-coherent and thus provide atomicy guarantees through that?
> 
> With cache coherency you may get atomicity of writes or reads but
> usually not atomic modifications.

Sure, but you can 'easily' extend your coherency protocols with support
for things like ll/sc (or larger transactions).

Have ll bring the cacheline into exclusive state and tag it, then
anything that demotes the cacheline will clear the tag and make sc fail.

headers

Ohad Ben-Cohen | 18 Oct 2010 17:35

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, Oct 18, 2010 at 5:32 PM, Peter Zijlstra <peterz <at> infradead.org> wrote:
> Sure, but you can 'easily' extend your coherency protocols with support

In our case, there are no coherency protocols supported between the
A9, M3 and the DSP.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Peter Zijlstra | 18 Oct 2010 17:48

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 17:35 +0200, Ohad Ben-Cohen wrote:
> On Mon, Oct 18, 2010 at 5:32 PM, Peter Zijlstra <peterz <at> infradead.org> wrote:
> > Sure, but you can 'easily' extend your coherency protocols with support
> 
> In our case, there are no coherency protocols supported between the
> A9, M3 and the DSP.

Sure, I got that, I was simply commenting on Catalin's statement that
cache coherency doesn't (need to) bring atomic modifications.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Catalin Marinas | 18 Oct 2010 17:51

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

Peter Zijlstra <peterz <at> infradead.org> wrote:
> On Mon, 2010-10-18 at 16:27 +0100, Catalin Marinas wrote:
>> Peter Zijlstra <peterz <at> infradead.org> wrote:
>> > On Mon, 2010-10-18 at 14:35 +0100, Russell King - ARM Linux wrote:
>> >> In any case, Linux's spinlock API (or more accurately, the ARM exclusive
>> >> access instructions) relies upon hardware coherency support (a piece of
>> >> hardware called an exclusive monitor) which isn't present on the M3 nor
>> >> DSP processors.  So there's no way to ensure that updates from the M3
>> >> and DSP are atomic wrt the A9 updates.
>> >
>> > Right, so the problem is that there simply is no way to do atomic memory
>> > access from these auxiliary processing units wrt the main CPU? Seeing as
>> > they operate on the same memory space, wouldn't it make sense to have
>> > them cache-coherent and thus provide atomicy guarantees through that?
>> 
>> With cache coherency you may get atomicity of writes or reads but
>> usually not atomic modifications.
>
> Sure, but you can 'easily' extend your coherency protocols with support
> for things like ll/sc (or larger transactions).
>
> Have ll bring the cacheline into exclusive state and tag it, then
> anything that demotes the cacheline will clear the tag and make sc fail.

For the ll/sc operations on ARM (exclusive load/store) there is a
per-CPU local exclusive monitor and a (virtual) global one. The global
one may either be a separate piece of hardware or emulated via cache
lines as you said. But if you need synchronisation with a CPU (or DSP)
like Cortex-M3 which doesn't have any built-in caches, you can only get
atomic operations on the main processor (A9) but not on the M3 (as you

(Continue reading)

headers

Peter Zijlstra | 18 Oct 2010 17:58

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 16:51 +0100, Catalin Marinas wrote:
> Peter Zijlstra <peterz <at> infradead.org> wrote:
> > On Mon, 2010-10-18 at 16:27 +0100, Catalin Marinas wrote:
> >> Peter Zijlstra <peterz <at> infradead.org> wrote:
> >> > On Mon, 2010-10-18 at 14:35 +0100, Russell King - ARM Linux wrote:
> >> >> In any case, Linux's spinlock API (or more accurately, the ARM exclusive
> >> >> access instructions) relies upon hardware coherency support (a piece of
> >> >> hardware called an exclusive monitor) which isn't present on the M3 nor
> >> >> DSP processors.  So there's no way to ensure that updates from the M3
> >> >> and DSP are atomic wrt the A9 updates.
> >> >
> >> > Right, so the problem is that there simply is no way to do atomic memory
> >> > access from these auxiliary processing units wrt the main CPU? Seeing as
> >> > they operate on the same memory space, wouldn't it make sense to have
> >> > them cache-coherent and thus provide atomicy guarantees through that?
> >> 
> >> With cache coherency you may get atomicity of writes or reads but
> >> usually not atomic modifications.

Right, so you forgot the qualifying part of your stmt: on ARM.

> > Sure, but you can 'easily' extend your coherency protocols with support
> > for things like ll/sc (or larger transactions).
> >
> > Have ll bring the cacheline into exclusive state and tag it, then
> > anything that demotes the cacheline will clear the tag and make sc fail.
> 
> For the ll/sc operations on ARM (exclusive load/store) there is a
> per-CPU local exclusive monitor and a (virtual) global one. The global
> one may either be a separate piece of hardware or emulated via cache

(Continue reading)

headers

Daniel Walker | 20 Oct 2010 01:31

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
> OMAP4 introduces a Spinlock hardware module, which provides hardware
> assistance for synchronization and mutual exclusion between heterogeneous
> processors and those not operating under a single, shared operating system
> (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).
> 
> The intention of this hardware module is to allow remote processors,
> that have no alternative mechanism to accomplish synchronization and mutual
> exclusion operations, to share resources (such as memory and/or any other
> hardware resource).
> 
> This patchset adds a new misc driver for this OMAP hwspinlock module.

Does this code interface with some hardware unit (other than the other
processors) to accomplish this locking ?

The reason I ask is because MSM has similar code, and from what I can
tell the MSM version has some structures in memory but that's all. It
just operates on the structures in memory.

It might be worth looking over the two implementation so we aren't both
remaking the wheel.

Daniel

--

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

(Continue reading)

headers

Ohad Ben-Cohen | 20 Oct 2010 08:13

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Wed, Oct 20, 2010 at 1:31 AM, Daniel Walker <dwalker <at> codeaurora.org> wrote:
> On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
>> OMAP4 introduces a Spinlock hardware module, which provides hardware
>> assistance for synchronization and mutual exclusion between heterogeneous
>> processors and those not operating under a single, shared operating system
>> (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).
>>
>> The intention of this hardware module is to allow remote processors,
>> that have no alternative mechanism to accomplish synchronization and mutual
>> exclusion operations, to share resources (such as memory and/or any other
>> hardware resource).
>>
>> This patchset adds a new misc driver for this OMAP hwspinlock module.
>
> Does this code interface with some hardware unit (other than the other
> processors) to accomplish this locking ?

Yes, it's a special-purpose hardware peripheral.

> The reason I ask is because MSM has similar code, and from what I can
> tell the MSM version has some structures in memory but that's all. It
> just operates on the structures in memory.

That's interesting.

We did have thoughts of making this a generic framework, in the hope
that it would be useful for other vendors too, but we didn't find
additional users.

> It might be worth looking over the two implementation so we aren't both

(Continue reading)

headers

Ohad Ben-Cohen | 20 Oct 2010 12:00

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

[resubmitting due to l-o being dropped from this discussion fork.
Thanks Russell for catching this]

 On Wed, Oct 20, 2010 at 1:31 AM, Daniel Walker <dwalker <at> codeaurora.org> wrote:
> On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
>> OMAP4 introduces a Spinlock hardware module, which provides hardware
>> assistance for synchronization and mutual exclusion between heterogeneous
>> processors and those not operating under a single, shared operating system
>> (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).
>>
>> The intention of this hardware module is to allow remote processors,
>> that have no alternative mechanism to accomplish synchronization and mutual
>> exclusion operations, to share resources (such as memory and/or any other
>> hardware resource).
>>
>> This patchset adds a new misc driver for this OMAP hwspinlock module.
>
> Does this code interface with some hardware unit (other than the other
> processors) to accomplish this locking ?

 Yes, it's a special-purpose hardware peripheral.

> The reason I ask is because MSM has similar code, and from what I can
> tell the MSM version has some structures in memory but that's all. It
> just operates on the structures in memory.

 That's interesting.

 We did have thoughts of making this a generic framework, in the hope
 that it would be useful for other vendors too, but we didn't find

(Continue reading)

headers

Bryan Huntsman | 21 Oct 2010 00:29

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver


>  Indeed. Where is that MSM code ?

We don't have an equivalent feature on MSM, nor any plans for one.  Daniel was thinking of an internal test
feature that had been deprecated a while ago.

- Bryan

--

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

headers

Russell King - ARM Linux | 20 Oct 2010 11:53

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Tue, Oct 19, 2010 at 04:31:30PM -0700, Daniel Walker wrote:
> On Mon, 2010-10-18 at 09:44 +0200, Ohad Ben-Cohen wrote:
> > OMAP4 introduces a Spinlock hardware module, which provides hardware
> > assistance for synchronization and mutual exclusion between heterogeneous
> > processors and those not operating under a single, shared operating system
> > (e.g. OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP).
> > 
> > The intention of this hardware module is to allow remote processors,
> > that have no alternative mechanism to accomplish synchronization and mutual
> > exclusion operations, to share resources (such as memory and/or any other
> > hardware resource).
> > 
> > This patchset adds a new misc driver for this OMAP hwspinlock module.
> 
> Does this code interface with some hardware unit (other than the other
> processors) to accomplish this locking ?
> 
> The reason I ask is because MSM has similar code, and from what I can
> tell the MSM version has some structures in memory but that's all. It
> just operates on the structures in memory.
> 
> It might be worth looking over the two implementation so we aren't both
> remaking the wheel.

Ohad's message to which you replied had:

To: linux-omap <at> vger.kernel.org, linux-kernel <at> vger.kernel.org,
        linux-arm-kernel <at> lists.infradead.org
Cc: Ohad Ben-Cohen <ohad <at> wizery.com>, Hari Kanigeri <h-kanigeri2 <at> ti.com>,
        Benoit Cousson <b-cousson <at> ti.com>, Tony Lindgren <tony <at> atomide.com>,

(Continue reading)

headers

Daniel Walker | 21 Oct 2010 00:15

Re: [PATCH 0/3] Add OMAP hardware spinlock misc driver

On Wed, 2010-10-20 at 10:53 +0100, Russell King - ARM Linux wrote:
> 
> To: Ohad Ben-Cohen <ohad <at> wizery.com>
> Cc: Hari Kanigeri <h-kanigeri2 <at> ti.com>, Benoit Cousson <b-cousson <at> ti.com>,
>         Tony Lindgren <tony <at> atomide.com>, Greg KH <greg <at> kroah.com>,
>         linux-kernel <at> vger.kernel.org,
>         Grant Likely <grant.likely <at> secretlab.ca>, mattw <at> codeaurora.org,
>         akpm <at> linux-foundation.org, Suman Anna <s-anna <at> ti.com>,
>         mattw <at> codeaurora.orgmattw, linux-arm-kernel <at> lists.infradead.org
> 
> which includes an invalid address "mattw <at> codeaurora.orgmattw".  Is there
> a reason why you're excluding the linux-omap list from your message and
> subsequent discussion?

I was trying to add Matt to the CC, but I guess I accidentally deleted
some CC's .. So it was purely an accident.

Daniel

--

-- 

Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(Continue reading)