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

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

[Ohad Ben-Cohen]

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.

2. OMAP I2C driver: On some OMAP4 boards, the I2C bus is shared between
the A9 and the M3, and the hwspinlock is used to synchronize access to it.


Patches are against linux-omap-2.6 master, which is 2.6.36-rc8 plus
2.6.37 omap material (needed for the omap specific patches in this set).

Tested on OMAP4 blaze.

Contributions
=============

Previous versions of this driver circulated in linux-omap several times,
and received substantial attention and contribution from many developers
(see [1][2][3][4][5][6]):

Simon Que did the initial implementation and pushed several iterations
Benoit Cousson provided extensive review, help, improvements and hwmod support
Hari Kanigeri helped out when Simon was away
Sanjeev Premi, Santosh Shilimkar and Nishanth Menon did lots of review

I'd like to thank Benoit Cousson, Steve Krueger, Hari Kanigeri,
Nourredine Hamoudi and Richard Woodruff for useful discussions about
the OMAP Spinlock requirements and use-cases.

Relevant linux-omap threads:

[1] http://thread.gmane.org/gmane.linux.ports.arm.omap/38755
[2] http://thread.gmane.org/gmane.linux.ports.arm.omap/38917
[3] http://thread.gmane.org/gmane.linux.ports.arm.omap/39187
[4] http://thread.gmane.org/gmane.linux.ports.arm.omap/39365
[5] http://thread.gmane.org/gmane.linux.ports.arm.omap/39815
[6] http://thread.gmane.org/gmane.linux.ports.arm.omap/40901

Changes since the last linux-omap iteration:

1. Disable interrupts and preemption when the lock is taken.
   This is required in order to minimize the period of time in which
   the other core might be spinning on this lock, trying to take it.
   Otherwise the other core will be polling the omap interconnect for
   too long, which would prevent other transactions from being executed.
   spin_trylock_irqsave is used to achieve this, with a dedicated spinlock
   (per hwspinlock). This also makes the hwspinlock primitive SMP-safe (but
   you shouldn't use this primitive to achieve synchronizations between
   different contexts on the same local processor, of course).

2. Add memory barriers
   The hwspinlock is used to achieve mutual exclusion when accessing some
   shared memory data. To guarantee that the memory operations, performed
   inside the critical section, are not reordered by the processor outside
   that critical section, we must use explicit memory barriers.

3. Provide three different locking API: trylock, lock_timeout, and lock,
   and implement them on top of each other (in that order). For the timeout
   API, use a jiffies-based parameter, instead of number of attempts.

4. Relax the OMAP interconnect between two subsequent lock() attempts,
   as recommended by the hw spec.

5. Move driver to drivers/misc, do some general cleanups and documentation

6. Use runtime PM get/put API to couple the number of locks that have being
   requested with the runtime PM's usage_count of the underlying device

Benoit Cousson (1):
  OMAP4: hwmod data: Add hwspinlock

Simon Que (2):
  drivers: misc: add omap_hwspinlock driver
  omap: add hwspinlock device

 Documentation/misc-devices/omap_hwspinlock.txt |  199 +++++++++
 arch/arm/mach-omap2/Makefile                   |    1 +
 arch/arm/mach-omap2/hwspinlock.c               |   67 +++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c     |   63 +++
 drivers/misc/Kconfig                           |   10 +
 drivers/misc/Makefile                          |    1 +
 drivers/misc/omap_hwspinlock.c                 |  555 ++++++++++++++++++++++++
 include/linux/omap_hwspinlock.h                |  108 +++++
 8 files changed, 1004 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/misc-devices/omap_hwspinlock.txt
 create mode 100644 arch/arm/mach-omap2/hwspinlock.c
 create mode 100644 drivers/misc/omap_hwspinlock.c
 create mode 100644 include/linux/omap_hwspinlock.h

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

[Ohad Ben-Cohen]

From: Simon Que <sque@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@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Krishnamoorthy, Balaji T <balajitk@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@wizery.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Tony Lindgren <tony@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
 create mode 100644 drivers/misc/omap_hwspinlock.c
 create mode 100644 include/linux/omap_hwspinlock.h

diff --git a/Documentation/misc-devices/omap_hwspinlock.txt b/Documentation/misc-devices/omap_hwspinlock.txt
new file mode 100644
index 0000000..b093347
--- /dev/null
+++ b/Documentation/misc-devices/omap_hwspinlock.txt
@@ -0,0 +1,199 @@
+OMAP Hardware Spinlocks
+
+1. Introduction
+
+Hardware spinlock modules provide hardware assistance for synchronization
+and mutual exclusion between heterogeneous processors and those not operating
+under a single, shared operating system.
+
+For example, OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP,
+each of which is running a different Operating System (the master, A9,
+is usually running Linux and the slave processors, the M3 and the DSP,
+are running some flavor of RTOS).
+
+A hwspinlock driver allows kernel code to access data structures (or hardware
+resources) that are shared with any of the existing remote processors, with
+which there is no alternative mechanism to accomplish synchronization and
+mutual exclusion operations.
+
+This is necessary, for example, for Inter-processor communications:
+on OMAP4, cpu-intensive multimedia tasks are offloaded by the host to the
+remote M3 and/or C64x+ slave processors (by an IPC subsystem called Syslink).
+
+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 them is synchronized using the hwspinlock
+module (remote processor directly places new messages in this shared data
+structure).
+
+2. User API
+
+  struct omap_hwspinlock *omap_hwspinlock_request(void);
+   - dynamically assign an hwspinlock and return its address, or
+     ERR_PTR(-EBUSY) if an unused hwspinlock isn't available. Users of this
+     API will usually want to communicate the lock's id to the remote core
+     before it can be used to achieve synchronization (to get the id of the
+     lock, use omap_hwspinlock_get_id()).
+     Can be called from an atomic context (this function will not sleep) but
+     not from within interrupt context.
+
+  struct omap_hwspinlock *omap_hwspinlock_request_specific(unsigned int id);
+   - assign a specific hwspinlock id and return its address, or
+     ERR_PTR(-EBUSY) if that hwspinlock is already in use. Usually board code
+     will be calling this function in order to reserve specific hwspinlock
+     ids for predefined purposes.
+     Can be called from an atomic context (this function will not sleep) but
+     not from within interrupt context.
+
+  int omap_hwspinlock_free(struct omap_hwspinlock *hwlock);
+   - free a previously-assigned hwspinlock; returns 0 on success, or an
+     appropriate error code on failure (e.g. -EINVAL if the hwspinlock
+     was not assigned).
+     Can be called from an atomic context (this function will not sleep) but
+     not from within interrupt context.
+
+  int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags);
+   - lock a previously assigned hwspinlock. If the hwspinlock is already
+     taken, the function will busy loop waiting for it to be released.
+     Note: if a faulty remote core never releases this lock, this function
+     will deadlock.
+     This function will fail if hwlock is invalid, but otherwise it will
+     always succeed (or deadlock; see above) and will never sleep. It is safe
+     to call it from any context.
+     Upon a successful return from this function, interrupts and preemption
+     are disabled so the caller must not sleep, and is advised to release the
+     hwspinlock as soon as possible, in order to minimize remote cores polling
+     on the hardware interconnect.
+     The flags parameter is a pointer to where the interrupts state of the
+     caller will be saved at.
+
+  int omap_hwspin_lock_timeout(struct omap_hwspinlock *hwlock,
+				unsigned long timeout, unsigned long *flags);
+   - lock a previously-assigned hwspinlock with a timeout limit (specified in
+     jiffies). If the hwspinlock is already taken, the function will busy loop
+     waiting for it to be released, but give up when the timeout meets jiffies.
+     If timeout is 0, the function will never give up (therefore if a faulty
+     remote core never releases the hwspinlock, it will deadlock).
+     Upon a successful return from this function, interrupts and preemption
+     are disabled so the caller must not sleep, and is advised to release the
+     hwspinlock as soon as possible, in order to minimize remote cores polling
+     on the hardware interconnect.
+     This function can be called from any context (it will never sleep). It
+     returns 0 when successful and an appropriate error code otherwise (most
+     notably -ETIMEDOUT if the hwspinlock is still busy after timeout meets
+     jiffies).
+     The flags parameter is a pointer to where the interrupts state of the
+     caller will be saved at.
+
+  int
+  omap_hwspin_trylock(struct omap_hwspinlock *hwlock, unsigned long *flags);
+   - attempt to lock a previously-assigned hwspinlock, but immediately fail if
+     it is already taken.
+     Upon a successful return from this function, interrupts and preemption
+     are disabled so the caller must not sleep, and is advised to release the
+     hwspinlock as soon as possible, in order to minimize remote cores polling
+     on the hardware interconnect.
+     This function can be called from any context (it will never sleep). It
+     returns 0 when successful and an appropriate error code otherwise (most
+     notably -EBUSY if the hwspinlock was already taken when it was called).
+     The flags parameter is a pointer to where the interrupts state of the
+     caller will be saved at.
+
+  int
+  omap_hwspin_unlock(struct omap_hwspinlock *hwlock, unsigned long *flags);
+   - unlock a previously-locked hwspinlock. Always succeed, and can be called
+     from any context (the function never sleeps). Note: code should _never_
+     unlock an hwspinlock which is already unlocked (there is no protection
+     against this).
+     Upon a successful return from this function, the interrupts state of the
+     caller is restored and preemption is reenabled.
+     This function can be called from any context (it will never sleep). It
+     returns 0 when successful and -EINVAL if hwlock is invalid.
+     The flags parameter points to the caller saved interrupts state.
+
+  int omap_hwspinlock_get_id(struct omap_hwspinlock *hwlock);
+   - returns the id of hwlock, or -EINVAL if hwlock is invalid.
+
+3. Typical usage
+
+#include <linux/omap_hwspinlock.h>
+#include <linux/err.h>
+
+int hwspinlock_example1(void)
+{
+	struct omap_hwspinlock *lock;
+	unsigned long flags;
+	int ret;
+
+	/* dynamically assign a hwspinlock */
+	lock = omap_hwspinlock_request();
+	if (IS_ERR(lock))
+		... handle error ...
+
+	/*
+	 * probably need to communicate the id of the hwspinlock to the
+	 * remote processor which we intend to achieve synchronization
+	 * with. The id of the lock can be obtained by calling
+	 * omap_hwspinlock_get_id(lock)
+	 */
+
+	/* take the lock, spin if it's already taken */
+	ret = omap_hwspin_lock(hwlock, &flags);
+	if (ret)
+		... handle error ...
+
+	/*
+	 * we took the lock, do our thing asap, and do NOT sleep
+	 */
+
+	/* release the lock */
+	ret = omap_hwspin_unlock(hwlock, &flags);
+	if (ret)
+		... handle error ...
+
+	/* free the lock */
+	ret = omap_hwspinlock_free(hwlock);
+	if (ret)
+		... handle error ...
+
+	return ret;
+}
+
+int hwspinlock_example2(void)
+{
+	struct omap_hwspinlock *lock;
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * request a specific hwspinlock id - this should be called by early
+	 * board init code.
+	 */
+	lock = omap_hwspinlock_request_specific(PREDEFINED_LOCK_ID);
+	if (IS_ERR(lock))
+		... handle error ...
+
+	/* try to take it, but don't spin on it */
+	ret = omap_hwspin_trylock(hwlock);
+	if (!ret)
+		... handle error ... /* lock is busy */
+
+	/*
+	 * we took the lock, do our thing asap, but do NOT sleep
+	 */
+
+	/* release the lock */
+	ret = omap_hwspin_unlock(hwlock, &flags);
+	if (ret)
+		... handle error ...
+
+	/* free the lock */
+	ret = omap_hwspinlock_free(hwlock);
+	if (ret)
+		... handle error ...
+
+	return ret;
+}
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b743312..ce4b7a6 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -390,6 +390,16 @@ 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"
+	help
+	  Say Y here to enable OMAP's hardware spinlock module (first
+	  introduced in OMAP4). This module is needed to achieve
+	  synchronization and mutual exclusion between the several
+	  remote processors on the system.
+
+	  If unsure, say N.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 42eab95..a4427ad 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -35,3 +35,4 @@ obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmw_balloon.o
 obj-$(CONFIG_ARM_CHARLCD)	+= arm-charlcd.o
+obj-$(CONFIG_OMAP_HWSPINLOCK)	+= omap_hwspinlock.o
diff --git a/drivers/misc/omap_hwspinlock.c b/drivers/misc/omap_hwspinlock.c
new file mode 100644
index 0000000..63dc5a2
--- /dev/null
+++ b/drivers/misc/omap_hwspinlock.c
@@ -0,0 +1,555 @@
+/*
+ * hardware spinlock driver for OMAP
+ *
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ *
+ * Authors: Simon Que <sque@ti.com>
+ *	    Hari Kanigeri <h-kanigeri2@ti.com>
+ *	    Ohad Ben-Cohen <ohad@wizery.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt)	"%s: " fmt, __func__
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/err.h>
+#include <linux/jiffies.h>
+#include <linux/omap_hwspinlock.h>
+#include <linux/pm_runtime.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+
+/* Spinlock register offsets */
+#define SPINLOCK_SYSSTATUS_OFFSET	0x0014
+#define SPINLOCK_BASE_OFFSET		0x0800
+
+#define SPINLOCK_NUMLOCKS_BIT_OFFSET	(24)
+
+/* Possible values of SPINLOCK_LOCK_REG */
+#define SPINLOCK_NOTTAKEN		(0)	/* free */
+#define SPINLOCK_TAKEN			(1)	/* locked */
+
+/**
+ * struct omap_hwspinlock - represents a single hardware spinlock
+ *
+ * @node: linked list member
+ * @id: unique and system-wide index number of this lock
+ * @lock: regular spinlock; we have one for every hwspinlock instance
+ * @addr: mapped address of the register which contains this lock's hw state
+ * @dev: underlying device, will be used to invoke runtime PM api
+ */
+struct omap_hwspinlock {
+	struct list_head node;
+	int id;
+	spinlock_t lock;
+	void __iomem *addr;
+	struct device *dev;
+};
+
+/**
+ * struct omap_hwspinlock_state - represents state of the underlying device
+ *
+ * @io_base: mapped base address of the hwspinlock device
+ * @hwlocks: array of omap_hwspinlocks that belong to this device
+ * @num_locks: number of hwspinlocks provided by this device
+ */
+struct omap_hwspinlock_state {
+	void __iomem *io_base;
+	struct omap_hwspinlock *hwlocks;
+	int num_locks;
+};
+
+/* List for keeping track of free locks */
+static LIST_HEAD(free_hwlocks);
+
+/* Access to the list of free locks is protected by this spinlock */
+static DEFINE_SPINLOCK(free_hwlocks_lock);
+
+/*
+ * "Multiple hwspinlock devices" is still an imaginary scenario,
+ * so maintaining a global state of our device is just fine for now.
+ */
+static struct omap_hwspinlock_state hwspinlock_state;
+
+/**
+ * omap_hwspin_trylock() - attempt to lock a specific hwspinlock
+ * @hwlock: a hwspinlock which we want to trylock
+ * @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
+ * the hwspinlock was already taken, and -EINVAL if @hwlock is invalid.
+ */
+int omap_hwspin_trylock(struct omap_hwspinlock *hwlock, unsigned long *flags)
+{
+	u32 ret;
+
+	if (IS_ERR_OR_NULL(hwlock)) {
+		pr_err("invalid hwlock\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * 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)
+	 */
+	if (!spin_trylock_irqsave(&hwlock->lock, *flags))
+		return -EBUSY;
+
+	/* attempt to acquire the lock by reading its value */
+	ret = readl(hwlock->addr);
+
+	/* lock is already taken */
+	if (ret == SPINLOCK_TAKEN) {
+		spin_unlock_irqrestore(&hwlock->lock, *flags);
+		return -EBUSY;
+	}
+
+	/*
+	 * We can be sure the other core's memory operations
+	 * are observable to us only _after_ we successfully take
+	 * the hwspinlock, so we must make sure that subsequent memory
+	 * operations will not be reordered before we actually took the
+	 * hwspinlock.
+	 * Note: the implicit memory barrier of the spinlock above is too
+	 * early, so we need this additional explicit memory barrier.
+	 */
+	mb();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(omap_hwspin_trylock);
+
+/**
+ * omap_hwspin_lock_timeout() - lock a specific hwspinlock with timeout limit
+ * @hwlock: the hwspinlock to be locked
+ * @to: timeout, in jiffies
+ * @flags: a pointer to where the caller's interrupts state will be saved at
+ *
+ * This function locks the underlying @hwlock. If the @hwlock
+ * is already taken, the function will busy loop waiting for it to
+ * be released, but give up when @timeout meets jiffies. If @timeout
+ * is 0, the function will never give up (therefore if a
+ * faulty remote core never releases the @hwlock, it will deadlock).
+ *
+ * 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 when the @hwlock was successfully taken, and an appropriate
+ * error code otherwise (most notably an -ETIMEDOUT if the @hwlock is still
+ * busy after @timeout meets jiffies). The function will never sleep.
+ */
+int omap_hwspin_lock_timeout(struct omap_hwspinlock *hwlock, unsigned long to,
+							unsigned long *flags)
+{
+	int ret;
+
+	for (;;) {
+		/* Try to take the hwspinlock */
+		ret = omap_hwspin_trylock(hwlock, flags);
+		if (ret != -EBUSY)
+			break;
+
+		/*
+		 * The lock is already taken, let's check if the user wants
+		 * us to try again
+		 */
+		if (to && time_is_before_eq_jiffies(to))
+			return -ETIMEDOUT;
+
+		/*
+		 * Do not hog the omap interconnect.
+		 *
+		 * It is recommended that the retry delay time will be
+		 * just over half of the time that a requester would be
+		 * expected to hold the lock.
+		 *
+		 * The number below is taken from an hardware specs example,
+		 * obviously it is somewhat arbitrary.
+		 */
+		ndelay(50);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(omap_hwspin_lock_timeout);
+
+/**
+ * omap_hwspinlock_unlock() - unlock a specific hwspinlock
+ * @hwlock: a previously-acquired hwspinlock which we want to unlock
+ * @flags: a pointer to the caller's saved interrupts state
+ *
+ * This function will unlock a specific hwspinlock, enable preemption and
+ * restore the interrupts state. @hwlock must be taken (by us!) before
+ * calling this function: it is a bug to call unlock on a @hwlock that was
+ * not taken by us, i.e. using one of omap_hwspin_{lock trylock, lock_timeout}.
+ *
+ * This function can be called from any context.
+ *
+ * Returns 0 when the @hwlock on success, or -EINVAL if @hwlock is invalid.
+ */
+int omap_hwspin_unlock(struct omap_hwspinlock *hwlock, unsigned long *flags)
+{
+	if (IS_ERR_OR_NULL(hwlock)) {
+		pr_err("invalid hwlock\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * We must make sure that memory operations, done before unlocking
+	 * the hwspinlock, will not be reordered after the lock is released.
+	 * The memory barrier induced by the spin_unlock below is too late:
+	 * the other core is going to access memory soon after it will take
+	 * the hwspinlock, and by then we want to be sure our memory operations
+	 * were already observable.
+	 */
+	mb();
+
+	/* release the lock by writing 0 to it (NOTTAKEN) */
+	writel(SPINLOCK_NOTTAKEN, hwlock->addr);
+
+	/* undo the spin_trylock_irqsave called in the locking function */
+	spin_unlock_irqrestore(&hwlock->lock, *flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(omap_hwspin_unlock);
+
+/**
+ * __omap_hwspinlock_assign() - prepare hwspinlock for assignment
+ * @hwlock: a hwspinlock which we want to assign to the user
+ *
+ * This is an internal function that prepares a specific hwspinlock
+ * for assignment.
+ *
+ * Returns 0 or 1 for success, or an appropriate error code on a failue
+ */
+static int __omap_hwspinlock_assign(struct omap_hwspinlock *hwlock)
+{
+	int ret;
+
+	/* notify the underlying device that power is now needed */
+	ret = pm_runtime_get_sync(hwlock->dev);
+	if (ret < 0) {
+		dev_err(hwlock->dev, "%s: can't power on device\n", __func__);
+		goto out;
+	}
+
+	/* remove the lock from the list of free locks */
+	list_del(&hwlock->node);
+
+	/*
+	 * mark this hwlock as used; needed in case someone will try to request
+	 * this hwlock specifically (using omap_hwspinlock_request_specific)
+	 * while it is already being used
+	 */
+	INIT_LIST_HEAD(&hwlock->node);
+
+out:
+	return ret;
+}
+
+/**
+ * omap_hwspinlock_request() - request a hw spinlock
+ *
+ * This function should be called by users of the hwspinlock module,
+ * in order to dynamically assign them an unused hwspinlock.
+ * Usually the user of this lock will then have to communicate the lock's id
+ * to the remote core before it can be used to synchronize (to get the id
+ * of a given hwlock, use omap_hwspinlock_get_id()).
+ *
+ * Can be called from an atomic context (will not sleep) but not from
+ * within interrupt context (simply because there is no use case for
+ * that yet).
+ *
+ * Returns the address of the assigned hwspinlock, or an appropriate error
+ * code on a failue (such as ERR_PTR(-EBUSY) if an unused hwspinlock wasn't
+ * found)
+ */
+struct omap_hwspinlock *omap_hwspinlock_request(void)
+{
+	struct omap_hwspinlock *hwlock;
+	int ret;
+
+	spin_lock(&free_hwlocks_lock);
+
+	if (list_empty(&free_hwlocks)) {
+		pr_warn("a free hwspinlock is not available\n");
+		hwlock = ERR_PTR(-EBUSY);
+		goto out;
+	}
+
+	hwlock = list_first_entry(&free_hwlocks, struct omap_hwspinlock, node);
+
+	ret = __omap_hwspinlock_assign(hwlock);
+	if (ret < 0)
+		hwlock = ERR_PTR(ret);
+
+out:
+	spin_unlock(&free_hwlocks_lock);
+	return hwlock;
+}
+EXPORT_SYMBOL_GPL(omap_hwspinlock_request);
+
+/**
+ * omap_hwspinlock_request_specific() - request for a specific hwspinlock
+ * @id: index of the specific hwspinlock that is requested
+ *
+ * This function should be called by users of the hwspinlock module,
+ * in order to assign them a specific hwspinlock.
+ * Usually board code will be calling this function in order to
+ * reserve specific hwspinlock ids for predefined purposes.
+ *
+ * Can be called from an atomic context (will not sleep) but not from
+ * within interrupt context (simply because there is no use case for
+ * that yet).
+ *
+ * Returns the address of the assigned lock on success, or appropriate
+ * error codes on failures (should be tested with IS_ERR)
+ */
+struct omap_hwspinlock *omap_hwspinlock_request_specific(unsigned int id)
+{
+	struct omap_hwspinlock *hwlock;
+	int ret;
+
+	if (id >= hwspinlock_state.num_locks) {
+		pr_warn("invalid id requested\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	spin_lock(&free_hwlocks_lock);
+
+	hwlock = &hwspinlock_state.hwlocks[id];
+	if (list_empty(&hwlock->node)) {
+		pr_warn("hwspinlock %d was already assigned\n", id);
+		hwlock = ERR_PTR(-EBUSY);
+		goto out;
+	}
+
+	ret = __omap_hwspinlock_assign(hwlock);
+	if (ret < 0)
+		hwlock = ERR_PTR(ret);
+
+out:
+	spin_unlock(&free_hwlocks_lock);
+	return hwlock;
+}
+EXPORT_SYMBOL_GPL(omap_hwspinlock_request_specific);
+
+/**
+ * omap_hwspinlock_free() - free a specific hwspinlock
+ * @hwlock: the specific hwspinlock to free
+ *
+ * This function returns @hwlock to the list of free hwspinlocks.
+ * Should only be called with an @hwlock that was retrieved from
+ * an earlier call to omap_hwspinlock_request{_specific}.
+ *
+ * Can be called from an atomic context (will not sleep) but not from
+ * within interrupt context (simply because there is no use case for
+ * that yet).
+ *
+ * Returns 0 on success, or an appropriate error code on failure
+ */
+int omap_hwspinlock_free(struct omap_hwspinlock *hwlock)
+{
+	int ret;
+
+	if (IS_ERR_OR_NULL(hwlock)) {
+		pr_err("invalid hwlock\n");
+		return -EINVAL;
+	}
+
+	spin_lock(&free_hwlocks_lock);
+
+	/* make sure hwlock is marked as used */
+	if (!list_empty(&hwlock->node)) {
+		pr_err("hwlock doesn't seem to be used\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	list_add(&hwlock->node, &free_hwlocks);
+
+	/* notify the underlying device that power is not needed */
+	ret = pm_runtime_put(hwlock->dev);
+	if (ret < 0)
+		dev_err(hwlock->dev, "%s: pm_runtime_put failed\n", __func__);
+
+out:
+	spin_unlock(&free_hwlocks_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(omap_hwspinlock_free);
+
+/**
+ * omap_hwspinlock_get_id() - retrieves id number of a given hwspinlock
+ * @hwlock: a valid hwspinlock instance
+ *
+ * Returns id number of a given @hwlock, or -EINVAL if @hwlock is invalid.
+ */
+int omap_hwspinlock_get_id(struct omap_hwspinlock *hwlock)
+{
+	if (IS_ERR_OR_NULL(hwlock)) {
+		pr_err("invalid hwlock\n");
+		return -EINVAL;
+	}
+
+	return hwlock->id;
+}
+EXPORT_SYMBOL_GPL(omap_hwspinlock_get_id);
+
+static int __devinit omap_hwspinlock_probe(struct platform_device *pdev)
+{
+	struct omap_hwspinlock *hwlock, *hwlocks;
+	struct resource *res;
+	void __iomem *io_base;
+	int i, ret, num_locks;
+
+	/* Multiple hwspinlock devices have no meaning, yet */
+	if (hwspinlock_state.hwlocks) {
+		dev_err(&pdev->dev, "unexpected spinlock device\n");
+		return -EBUSY;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	io_base = ioremap(res->start, resource_size(res));
+	if (!io_base)
+		return -ENOMEM;
+
+	/* Determine number of locks */
+	i = readl(io_base + SPINLOCK_SYSSTATUS_OFFSET);
+	i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
+
+	/* exactly one of the four least significant bits must be 1 */
+	if (!i || !is_power_of_2(i) || i > 8) {
+		ret = -EINVAL;
+		goto iounmap_base;
+	}
+
+	num_locks = i * 32;
+
+	hwlocks = kzalloc(sizeof(*hwlock) * num_locks, GFP_KERNEL);
+	if (!hwlocks) {
+		ret = -ENOMEM;
+		goto iounmap_base;
+	}
+
+	for (i = 0; i < num_locks; i++) {
+		hwlock = &hwlocks[i];
+
+		spin_lock_init(&hwlock->lock);
+
+		hwlock->id	= i;
+		hwlock->dev	= &pdev->dev;
+		hwlock->addr	= io_base + SPINLOCK_BASE_OFFSET +
+							sizeof(u32) * i;
+
+		/* Add hwlock to the list of free locks */
+		list_add(&hwlock->node, &free_hwlocks);
+	}
+
+	hwspinlock_state.io_base = io_base;
+	hwspinlock_state.hwlocks = hwlocks;
+	hwspinlock_state.num_locks = num_locks;
+
+	/*
+	 * runtime PM will make sure the clock of this module is
+	 * enabled iff at least one lock is requested
+	 */
+	pm_runtime_enable(&pdev->dev);
+
+	pr_info("registered %d hwspinlocks\n", num_locks);
+
+	return 0;
+
+iounmap_base:
+	iounmap(hwspinlock_state.io_base);
+	return ret;
+}
+
+static int omap_hwspinlock_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+
+	hwspinlock_state.num_locks = 0;
+	hwspinlock_state.hwlocks = NULL;
+
+	/* clear list of free locks */
+	INIT_LIST_HEAD(&free_hwlocks);
+
+	kfree(hwspinlock_state.hwlocks);
+
+	iounmap(hwspinlock_state.io_base);
+
+	return 0;
+}
+
+static struct platform_driver omap_hwspinlock_driver = {
+	.probe		= omap_hwspinlock_probe,
+	.remove		= omap_hwspinlock_remove,
+	.driver		= {
+		.name	= "omap_hwspinlock",
+	},
+};
+
+static int __init omap_hwspinlock_init(void)
+{
+	return platform_driver_register(&omap_hwspinlock_driver);
+}
+/* early board code might need to reserve specific hwspinlocks */
+postcore_initcall(omap_hwspinlock_init);
+
+static void __exit omap_hwspinlock_exit(void)
+{
+	platform_driver_unregister(&omap_hwspinlock_driver);
+}
+module_exit(omap_hwspinlock_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware spinlock driver for OMAP");
+MODULE_AUTHOR("Simon Que <sque@ti.com>");
+MODULE_AUTHOR("Hari Kanigeri <h-kanigeri2@ti.com>");
+MODULE_AUTHOR("Ohad Ben-Cohen <ohad@wizery.com>");
diff --git a/include/linux/omap_hwspinlock.h b/include/linux/omap_hwspinlock.h
new file mode 100644
index 0000000..8faabb8
--- /dev/null
+++ b/include/linux/omap_hwspinlock.h
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2010 Texas Instruments, Inc.
+ *
+ * Author: Ohad Ben-Cohen <ohad@wizery.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ */
+
+#ifndef __LINUX_OMAP_HWSPINLOCK_H
+#define __LINUX_OMAP_HWSPINLOCK_H
+
+#include <linux/err.h>
+
+struct omap_hwspinlock;
+
+#ifdef CONFIG_OMAP_HWSPINLOCK
+
+struct omap_hwspinlock *omap_hwspinlock_request(void);
+struct omap_hwspinlock *omap_hwspinlock_request_specific(unsigned int id);
+int omap_hwspinlock_free(struct omap_hwspinlock *hwlock);
+int omap_hwspinlock_get_id(struct omap_hwspinlock *hwlock);
+int omap_hwspin_lock_timeout(struct omap_hwspinlock *hwlock, unsigned long to,
+							unsigned long *flags);
+int omap_hwspin_trylock(struct omap_hwspinlock *hwlock, unsigned long *flags);
+int omap_hwspin_unlock(struct omap_hwspinlock *hwlock, unsigned long *flags);
+
+#else /* !CONFIG_OMAP_HWSPINLOCK */
+
+static inline struct omap_hwspinlock *omap_hwspinlock_request(void)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline
+struct omap_hwspinlock *omap_hwspinlock_request_specific(unsigned int id)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline int omap_hwspinlock_free(struct omap_hwspinlock *hwlock)
+{
+	return -ENOSYS;
+}
+
+static inline int omap_hwspin_lock_timeout(struct omap_hwspinlock *hwlock,
+							unsigned long timeout)
+{
+	return -ENOSYS;
+}
+
+static inline int omap_hwspin_trylock(struct omap_hwspinlock *hwlock)
+{
+	return -ENOSYS;
+}
+
+static inline int omap_hwspin_unlock(struct omap_hwspinlock *hwlock)
+{
+	return -ENOSYS;
+}
+
+static inline int omap_hwspinlock_get_id(struct omap_hwspinlock *hwlock)
+{
+	return -ENOSYS;
+}
+
+#endif /* !CONFIG_OMAP_HWSPINLOCK */
+
+/**
+ * omap_hwspin_lock - take a specific hwspinlock, with no time limits
+ * @hwlock: the hwspinlock to be locked
+ * @flags: a pointer to where the caller's interrups state will be saved at
+ *
+ * This function locks the underlying @hwlock. If the @hwlock
+ * is already taken, the function will busy loop waiting for it to
+ * be released. Note: if a faulty remote core never releases the
+ * @hwlock, this function will deadlock.
+ *
+ * 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.
+ *
+ * Calling this function with a @hwlock which is already taken will have the
+ * same effects as calling spin_lock_irqsave on a taken spinlock.
+ *
+ * This function can be called from any context.
+ *
+ * Returns 0 when the @hwlock was successfully taken, and an appropriate
+ * error code otherwise (can only fail if @hwlock is invalid).
+ */
+static inline
+int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags)
+{
+	return omap_hwspin_lock_timeout(hwlock, 0, flags);
+}
+
+#endif /* __LINUX_OMAP_HWSPINLOCK_H */
-- 
1.7.0.4

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

[Ohad Ben-Cohen]

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

Add hwspinlock hwmod data for OMAP4 chip

Signed-off-by: Cousson, Benoit <b-cousson@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Paul Walmsley <paul@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
@@ -1043,6 +1043,66 @@ 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,
+	.sysc_flags	= (SYSC_HAS_AUTOIDLE | SYSC_HAS_CLOCKACTIVITY |
+			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SIDLEMODE |
+			   SYSC_HAS_SOFTRESET | SYSS_HAS_RESET_STATUS),
+	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.sysc_fields	= &omap_hwmod_sysc_type1,
+};
+
+static struct omap_hwmod_class omap44xx_spinlock_hwmod_class = {
+	.name = "spinlock",
+	.sysc = &omap44xx_spinlock_sysc,
+};
+
+/* spinlock */
+static struct omap_hwmod omap44xx_spinlock_hwmod;
+static struct omap_hwmod_addr_space omap44xx_spinlock_addrs[] = {
+	{
+		.pa_start	= 0x4a0f6000,
+		.pa_end		= 0x4a0f6fff,
+		.flags		= ADDR_TYPE_RT
+	},
+};
+
+/* l4_cfg -> spinlock */
+static struct omap_hwmod_ocp_if omap44xx_l4_cfg__spinlock = {
+	.master		= &omap44xx_l4_cfg_hwmod,
+	.slave		= &omap44xx_spinlock_hwmod,
+	.clk		= "l4_div_ck",
+	.addr		= omap44xx_spinlock_addrs,
+	.addr_cnt	= ARRAY_SIZE(omap44xx_spinlock_addrs),
+	.user		= OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+/* spinlock slave ports */
+static struct omap_hwmod_ocp_if *omap44xx_spinlock_slaves[] = {
+	&omap44xx_l4_cfg__spinlock,
+};
+
+static struct omap_hwmod omap44xx_spinlock_hwmod = {
+	.name		= "spinlock",
+	.class		= &omap44xx_spinlock_hwmod_class,
+	.prcm = {
+		.omap4 = {
+			.clkctrl_reg = OMAP4430_CM_L4CFG_HW_SEM_CLKCTRL,
+		},
+	},
+	.slaves		= omap44xx_spinlock_slaves,
+	.slaves_cnt	= ARRAY_SIZE(omap44xx_spinlock_slaves),
+	.omap_chip	= OMAP_CHIP_INIT(CHIP_IS_OMAP4430),
+};
+
 static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 	/* dmm class */
 	&omap44xx_dmm_hwmod,
@@ -1077,6 +1137,9 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 	&omap44xx_uart2_hwmod,
 	&omap44xx_uart3_hwmod,
 	&omap44xx_uart4_hwmod,
+
+	/* spinlock class */
+	&omap44xx_spinlock_hwmod,
 	NULL,
 };
 
-- 
1.7.0.4

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

From: Simon Que <sque@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@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Benoit Cousson <b-cousson@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
@@ -190,3 +190,4 @@ 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
--- /dev/null
+++ b/arch/arm/mach-omap2/hwspinlock.c
@@ -0,0 +1,67 @@
+/*
+ * OMAP hardware spinlock device initialization
+ *
+ * Copyright (C) 2010 Texas Instruments. All rights reserved.
+ *
+ * Contact: Simon Que <sque@ti.com>
+ *          Hari Kanigeri <h-kanigeri2@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/err.h>
+
+#include <plat/omap_hwmod.h>
+#include <plat/omap_device.h>
+
+struct omap_device_pm_latency omap_spinlock_latency[] = {
+	{
+		.deactivate_func = omap_device_idle_hwmods,
+		.activate_func   = omap_device_enable_hwmods,
+		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	}
+};
+
+int __init hwspinlocks_init(void)
+{
+	int retval = 0;
+	struct omap_hwmod *oh;
+	struct omap_device *od;
+	const char *oh_name = "spinlock";
+	const char *dev_name = "omap_hwspinlock";
+
+	/*
+	 * Hwmod lookup will fail in case our platform doesn't support the
+	 * hardware spinlock module, so it is safe to run this initcall
+	 * on all omaps
+	 */
+	oh = omap_hwmod_lookup(oh_name);
+	if (oh == NULL)
+		return -EINVAL;
+
+	od = omap_device_build(dev_name, 0, oh, NULL, 0,
+				omap_spinlock_latency,
+				ARRAY_SIZE(omap_spinlock_latency), false);
+	if (IS_ERR(od)) {
+		pr_err("Can't build omap_device for %s:%s\n", dev_name,
+								oh_name);
+		retval = PTR_ERR(od);
+	}
+
+	return retval;
+}
+postcore_initcall(hwspinlocks_init);
-- 
1.7.0.4

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

[Peter Zijlstra]

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?

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

[Russell King - ARM Linux]

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.

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

[Peter Zijlstra]

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
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?

But that's water under the bridge, and your last paragraph does indeed
answer my question.

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

[Ohad Ben-Cohen]

On Mon, Oct 18, 2010 at 3:43 PM, Peter Zijlstra <peterz@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.

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

[Peter Zijlstra]

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

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

[Ohad Ben-Cohen]

On Mon, Oct 18, 2010 at 4:33 PM, Peter Zijlstra <peterz@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.

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

[Catalin Marinas]

Peter Zijlstra <peterz@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

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

[Peter Zijlstra]

On Mon, 2010-10-18 at 16:27 +0100, Catalin Marinas wrote:
> Peter Zijlstra <peterz@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.

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

[Ohad Ben-Cohen]

On Mon, Oct 18, 2010 at 5:32 PM, Peter Zijlstra <peterz@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.

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

[Peter Zijlstra]

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

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

[Catalin Marinas]

Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, 2010-10-18 at 16:27 +0100, Catalin Marinas wrote:
>> Peter Zijlstra <peterz@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
can't have a cache line in exclusive state on the M3).

The M3 may have a local exclusive monitor (like the main CPU) but it
isn't cleared by memory accesses from the main CPU.

-- 
Catalin

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

[Peter Zijlstra]

On Mon, 2010-10-18 at 16:51 +0100, Catalin Marinas wrote:
> Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, 2010-10-18 at 16:27 +0100, Catalin Marinas wrote:
> >> Peter Zijlstra <peterz@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
> 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
> can't have a cache line in exclusive state on the M3).

Right, and I take it that modifying the M3 to participate in the full
coherency/exclusive monitor thing would have been more work.

> The M3 may have a local exclusive monitor (like the main CPU) but it
> isn't cleared by memory accesses from the main CPU.

Sounds like asking for trouble if you ask me ;-)

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

[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, 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

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

[Kevin Hilman]

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

> From: Simon Que <sque@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
> + * @hwlock: a hwspinlock which we want to trylock
> + * @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
> + * the hwspinlock was already taken, and -EINVAL if @hwlock is invalid.
> + */
> +int omap_hwspin_trylock(struct omap_hwspinlock *hwlock, unsigned long *flags)
> +{
> +	u32 ret;
> +
> +	if (IS_ERR_OR_NULL(hwlock)) {
> +		pr_err("invalid hwlock\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * 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.)

> +	if (!spin_trylock_irqsave(&hwlock->lock, *flags))
> +		return -EBUSY;
> +
> +	/* attempt to acquire the lock by reading its value */
> +	ret = readl(hwlock->addr);
> +
> +	/* lock is already taken */
> +	if (ret == SPINLOCK_TAKEN) {
> +		spin_unlock_irqrestore(&hwlock->lock, *flags);
> +		return -EBUSY;
> +	}
> +
> +	/*
> +	 * We can be sure the other core's memory operations
> +	 * are observable to us only _after_ we successfully take
> +	 * the hwspinlock, so we must make sure that subsequent memory
> +	 * operations will not be reordered before we actually took the
> +	 * hwspinlock.
> +	 * Note: the implicit memory barrier of the spinlock above is too
> +	 * early, so we need this additional explicit memory barrier.
> +	 */
> +	mb();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(omap_hwspin_trylock);

[...]

> +/**
> + * omap_hwspinlock_unlock() - unlock a specific hwspinlock

minor nit: s/lock_unlock/_unlock/  to match name below

> + * @hwlock: a previously-acquired hwspinlock which we want to unlock
> + * @flags: a pointer to the caller's saved interrupts state
> + *
> + * This function will unlock a specific hwspinlock, enable preemption and
> + * restore the interrupts state. @hwlock must be taken (by us!) before
> + * calling this function: it is a bug to call unlock on a @hwlock that was
> + * not taken by us, i.e. using one of omap_hwspin_{lock trylock, lock_timeout}.
> + *
> + * This function can be called from any context.
> + *
> + * Returns 0 when the @hwlock on success, or -EINVAL if @hwlock is invalid.
> + */
> +int omap_hwspin_unlock(struct omap_hwspinlock *hwlock, unsigned long *flags)
> +{
> +	if (IS_ERR_OR_NULL(hwlock)) {
> +		pr_err("invalid hwlock\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * We must make sure that memory operations, done before unlocking
> +	 * the hwspinlock, will not be reordered after the lock is released.
> +	 * The memory barrier induced by the spin_unlock below is too late:
> +	 * the other core is going to access memory soon after it will take
> +	 * the hwspinlock, and by then we want to be sure our memory operations
> +	 * were already observable.
> +	 */
> +	mb();
> +
> +	/* release the lock by writing 0 to it (NOTTAKEN) */
> +	writel(SPINLOCK_NOTTAKEN, hwlock->addr);
> +
> +	/* undo the spin_trylock_irqsave called in the locking function */
> +	spin_unlock_irqrestore(&hwlock->lock, *flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(omap_hwspin_unlock);

[...]

Kevin

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

[Grant Likely]

On Mon, Oct 18, 2010 at 1:44 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> From: Simon Que <sque@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@ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> Signed-off-by: Krishnamoorthy, Balaji T <balajitk@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@wizery.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Tony Lindgren <tony@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
> ?create mode 100644 drivers/misc/omap_hwspinlock.c
> ?create mode 100644 include/linux/omap_hwspinlock.h
>
> diff --git a/Documentation/misc-devices/omap_hwspinlock.txt b/Documentation/misc-devices/omap_hwspinlock.txt
> new file mode 100644
> index 0000000..b093347
> --- /dev/null
> +++ b/Documentation/misc-devices/omap_hwspinlock.txt
> @@ -0,0 +1,199 @@
> +OMAP Hardware Spinlocks
> +
> +1. Introduction
> +
> +Hardware spinlock modules provide hardware assistance for synchronization
> +and mutual exclusion between heterogeneous processors and those not operating
> +under a single, shared operating system.
> +
> +For example, OMAP4 has dual Cortex-A9, dual Cortex-M3 and a C64x+ DSP,
> +each of which is running a different Operating System (the master, A9,
> +is usually running Linux and the slave processors, the M3 and the DSP,
> +are running some flavor of RTOS).
> +
> +A hwspinlock driver allows kernel code to access data structures (or hardware
> +resources) that are shared with any of the existing remote processors, with
> +which there is no alternative mechanism to accomplish synchronization and
> +mutual exclusion operations.
> +
> +This is necessary, for example, for Inter-processor communications:
> +on OMAP4, cpu-intensive multimedia tasks are offloaded by the host to the
> +remote M3 and/or C64x+ slave processors (by an IPC subsystem called Syslink).
> +
> +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 them is synchronized using the hwspinlock
> +module (remote processor directly places new messages in this shared data
> +structure).
> +
> +2. User API
> +
> + ?struct omap_hwspinlock *omap_hwspinlock_request(void);
> + ? - dynamically assign an hwspinlock and return its address, or
> + ? ? ERR_PTR(-EBUSY) if an unused hwspinlock isn't available. Users of this
> + ? ? API will usually want to communicate the lock's id to the remote core
> + ? ? before it can be used to achieve synchronization (to get the id of the
> + ? ? lock, use omap_hwspinlock_get_id()).
> + ? ? Can be called from an atomic context (this function will not sleep) but
> + ? ? not from within interrupt context.

I strongly recommend reconsidering the ERR_PTR() pattern in new driver
code.  It is impossible to tell from looking at the prototype of a
function if it returns an ERR_PTR() value, or a NULL on failure.  I
pretty much guarantee that new users of this code will miss the
subtlety and introduce new bugs by assuming that the return value can
be tested with "if (!hwlock)".

ERR_PTR() is only appropriate when the caller actually cares about the
failure code and has different behaviour depending on the result.  For
example, filesystem code that needs to return to userspace a specific
error code.  Very seldom does driver code like users of this API
actually care.  Using it is just asking for bugs with no benefit.

> + ?struct omap_hwspinlock *omap_hwspinlock_request_specific(unsigned int id);
> + ? - assign a specific hwspinlock id and return its address, or
> + ? ? ERR_PTR(-EBUSY) if that hwspinlock is already in use. Usually board code
> + ? ? will be calling this function in order to reserve specific hwspinlock
> + ? ? ids for predefined purposes.
> + ? ? Can be called from an atomic context (this function will not sleep) but
> + ? ? not from within interrupt context.
> +
> + ?int omap_hwspinlock_free(struct omap_hwspinlock *hwlock);
> + ? - free a previously-assigned hwspinlock; returns 0 on success, or an
> + ? ? appropriate error code on failure (e.g. -EINVAL if the hwspinlock
> + ? ? was not assigned).
> + ? ? Can be called from an atomic context (this function will not sleep) but
> + ? ? not from within interrupt context.
> +
> + ?int omap_hwspin_lock(struct omap_hwspinlock *hwlock, unsigned long *flags);
> + ? - lock a previously assigned hwspinlock. If the hwspinlock is already
> + ? ? taken, the function will busy loop waiting for it to be released.
> + ? ? Note: if a faulty remote core never releases this lock, this function
> + ? ? will deadlock.
> + ? ? This function will fail if hwlock is invalid, but otherwise it will
> + ? ? always succeed (or deadlock; see above) and will never sleep. It is safe
> + ? ? to call it from any context.
> + ? ? Upon a successful return from this function, interrupts and preemption
> + ? ? are disabled so the caller must not sleep, and is advised to release the
> + ? ? hwspinlock as soon as possible, in order to minimize remote cores polling
> + ? ? on the hardware interconnect.
> + ? ? The flags parameter is a pointer to where the interrupts state of the
> + ? ? caller will be saved at.
> +
> + ?int omap_hwspin_lock_timeout(struct omap_hwspinlock *hwlock,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned long timeout, unsigned long *flags);
> + ? - lock a previously-assigned hwspinlock with a timeout limit (specified in
> + ? ? jiffies). If the hwspinlock is already taken, the function will busy loop
> + ? ? waiting for it to be released, but give up when the timeout meets jiffies.
> + ? ? If timeout is 0, the function will never give up (therefore if a faulty
> + ? ? remote core never releases the hwspinlock, it will deadlock).
> + ? ? Upon a successful return from this function, interrupts and preemption
> + ? ? are disabled so the caller must not sleep, and is advised to release the
> + ? ? hwspinlock as soon as possible, in order to minimize remote cores polling

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.

OTOH, I also see that a spin lock is still needed internally to
protect the hwspinlock data structure.

g.

[PATCH 3/3] omap: add hwspinlock device

[Kevin Hilman]

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

> From: Simon Que <sque@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@ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Benoit Cousson <b-cousson@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
> @@ -190,3 +190,4 @@ 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
> --- /dev/null
> +++ b/arch/arm/mach-omap2/hwspinlock.c
> @@ -0,0 +1,67 @@
> +/*
> + * OMAP hardware spinlock device initialization
> + *
> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> + *
> + * Contact: Simon Que <sque@ti.com>
> + *          Hari Kanigeri <h-kanigeri2@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +
> +#include <plat/omap_hwmod.h>
> +#include <plat/omap_device.h>
> +
> +struct omap_device_pm_latency omap_spinlock_latency[] = {
> +	{
> +		.deactivate_func = omap_device_idle_hwmods,
> +		.activate_func   = omap_device_enable_hwmods,
> +		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> +	}
> +};
> +
> +int __init hwspinlocks_init(void)
> +{
> +	int retval = 0;
> +	struct omap_hwmod *oh;
> +	struct omap_device *od;
> +	const char *oh_name = "spinlock";
> +	const char *dev_name = "omap_hwspinlock";
> +
> +	/*
> +	 * Hwmod lookup will fail in case our platform doesn't support the
> +	 * hardware spinlock module, so it is safe to run this initcall
> +	 * on all omaps
> +	 */
> +	oh = omap_hwmod_lookup(oh_name);
> +	if (oh == NULL)
> +		return -EINVAL;
> +
> +	od = omap_device_build(dev_name, 0, oh, NULL, 0,
> +				omap_spinlock_latency,
> +				ARRAY_SIZE(omap_spinlock_latency), false);
> +	if (IS_ERR(od)) {
> +		pr_err("Can't build omap_device for %s:%s\n", dev_name,
> +								oh_name);
> +		retval = PTR_ERR(od);
> +	}
> +
> +	return retval;
> +}
> +postcore_initcall(hwspinlocks_init);

Any reason this needs to be a postcore_initcall?  Are there users of
hwspinlocks this early in boot?  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

[PATCH 3/3] omap: add hwspinlock device

[Grant Likely]

On Tue, Oct 19, 2010 at 11:03 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Ohad Ben-Cohen <ohad@wizery.com> writes:
>
>> From: Simon Que <sque@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@ti.com>
>> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
>> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
>> Cc: Benoit Cousson <b-cousson@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
>> @@ -190,3 +190,4 @@ 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
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/hwspinlock.c
>> @@ -0,0 +1,67 @@
>> +/*
>> + * OMAP hardware spinlock device initialization
>> + *
>> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
>> + *
>> + * Contact: Simon Que <sque@ti.com>
>> + * ? ? ? ? ?Hari Kanigeri <h-kanigeri2@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>> + * 02110-1301 USA
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +
>> +#include <plat/omap_hwmod.h>
>> +#include <plat/omap_device.h>
>> +
>> +struct omap_device_pm_latency omap_spinlock_latency[] = {
>> + ? ? {
>> + ? ? ? ? ? ? .deactivate_func = omap_device_idle_hwmods,
>> + ? ? ? ? ? ? .activate_func ? = omap_device_enable_hwmods,
>> + ? ? ? ? ? ? .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> + ? ? }
>> +};
>> +
>> +int __init hwspinlocks_init(void)
>> +{
>> + ? ? int retval = 0;
>> + ? ? struct omap_hwmod *oh;
>> + ? ? struct omap_device *od;
>> + ? ? const char *oh_name = "spinlock";
>> + ? ? const char *dev_name = "omap_hwspinlock";
>> +
>> + ? ? /*
>> + ? ? ?* Hwmod lookup will fail in case our platform doesn't support the
>> + ? ? ?* hardware spinlock module, so it is safe to run this initcall
>> + ? ? ?* on all omaps
>> + ? ? ?*/
>> + ? ? oh = omap_hwmod_lookup(oh_name);
>> + ? ? if (oh == NULL)
>> + ? ? ? ? ? ? return -EINVAL;
>> +
>> + ? ? od = omap_device_build(dev_name, 0, oh, NULL, 0,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap_spinlock_latency,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ARRAY_SIZE(omap_spinlock_latency), false);
>> + ? ? if (IS_ERR(od)) {
>> + ? ? ? ? ? ? pr_err("Can't build omap_device for %s:%s\n", dev_name,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? oh_name);
>> + ? ? ? ? ? ? retval = PTR_ERR(od);
>> + ? ? }
>> +
>> + ? ? return retval;
>> +}
>> +postcore_initcall(hwspinlocks_init);
>
> Any reason this needs to be a postcore_initcall? ?Are there users of
> hwspinlocks this early in boot? ?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.

On that note, is there any reason why this file cannot be selected as a module?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

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

[Kevin Hilman]

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

> From: Simon Que <sque@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
> @@ -390,6 +390,16 @@ 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

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

[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 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

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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 5:46 PM, Greg KH <greg@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,
>
> greg k-h
>

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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 6:58 PM, Kevin Hilman
<khilman@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.

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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 7:01 PM, Grant Likely <grant.likely@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.

Thanks,
Ohad.

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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 7:21 PM, Arnd Bergmann <arnd@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.

>
> ? ? ? ?Arnd
>

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

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

What about those cases where you already know that interrupts are
disabled, e.g. while holding a regular spin_lock_irq or inside
of an interrupt handler?

	Arnd

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
<khilman@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
>

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

[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 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

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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 10:58 PM, Arnd Bergmann <arnd@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.

[PATCH 3/3] omap: add hwspinlock device

[Grant Likely]

On Tue, Oct 19, 2010 at 3:02 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
> <khilman@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.

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

[Daniel Walker]

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.

[PATCH 3/3] omap: add hwspinlock device

[Kevin Hilman]

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

> On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
> <khilman@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

[PATCH 3/3] omap: add hwspinlock device

[Ryan Mallon]

On 10/20/2010 12:53 PM, Kevin Hilman wrote:
> Ohad Ben-Cohen <ohad@wizery.com> writes:
> 
>> On Tue, Oct 19, 2010 at 7:03 PM, Kevin Hilman
>> <khilman@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
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

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

[Ohad Ben-Cohen]

On Wed, Oct 20, 2010 at 1:31 AM, Daniel Walker <dwalker@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
> remaking the wheel.

Indeed. Where is that MSM code ?

Thanks,
Ohad.

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

[Russell King - ARM Linux]

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@wizery.com>, Hari Kanigeri <h-kanigeri2@ti.com>,
        Benoit Cousson <b-cousson@ti.com>, Tony Lindgren <tony@atomide.com>,
        Greg KH <greg@kroah.com>, Grant Likely <grant.likely@secretlab.ca>,
        akpm at linux-foundation.org, Suman Anna <s-anna@ti.com>

Yours has:

To: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Hari Kanigeri <h-kanigeri2@ti.com>, Benoit Cousson <b-cousson@ti.com>,
        Tony Lindgren <tony@atomide.com>, Greg KH <greg@kroah.com>,
        linux-kernel at vger.kernel.org,
        Grant Likely <grant.likely@secretlab.ca>, mattw at codeaurora.org,
        akpm at linux-foundation.org, Suman Anna <s-anna@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?

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

[Ohad Ben-Cohen]

[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@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
> remaking the wheel.

 Indeed. Where is that MSM code ?

 Thanks,
 Ohad.

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

[Ohad Ben-Cohen]

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


>
> Kevin
>

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Wed, Oct 20, 2010 at 1:12 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Oct 19, 2010 at 3:02 PM, Ohad Ben-Cohen <ohad@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@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@bluewatersys.com> wrote:
> The issue of probe order still needs to be resolved for those of us who
> do want all the drivers built into the kernel.

What about doing something similar to the way suspend/resume and the
device hierarchy interact ?

device_resume waits for its parent to be resumed before waking up the
device - this sounds similar to what ->probe() should do: wait for its
device dependency to probe first (so in this case, i2c-omap should
wait for hwspinlock).

Conversely, device_suspend waits for all its children to be suspended
before continuing, which sounds just like what ->remove() should do
(so again, in this case, the hwspinlock device should wait for all its
users to be removed before bailing).

This is just a quick thought, I haven't even began to think of all the
use cases and requirements.

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
<khilman@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:

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.

All we will be left to take care of is the order of the ->probe()
execution (assuming we want both the i2c and the hwspinlock drivers to
be device_initcall)

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

Why would we need platform-specific function pointers here ? I'm not
sure I'm following this one.

Thanks,
Ohad.


>
> Kevin
>

[PATCH 3/3] omap: add hwspinlock device

[Grant Likely]

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@secretlab.ca> wrote:
> > On Tue, Oct 19, 2010 at 3:02 PM, Ohad Ben-Cohen <ohad@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@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
in both cases, and the only solution to fix the dependency issue.

I completely agree that more drivers need to support modules, but
that is an orthogonal issue.

I suspect that in most cases the driver model already provides the
needed functionality via the parent->child relationships, and a lot of
the problems can be removed by getting the driver of the parent device
to register the children.

It gets more complex (like in this case) when a single device needs
multiple devices to be initialized before it is probed. I can think of
a couple of ways to solve this. One option is for such drivers to
explicitly look for its dependant devices and defer .probe() work
until they appear. I hacked up some code for this a while back, but I
never pursued it through to completion[1].

Another option is to defer even registering the dependent devices
until the prerequisites are all probed.  This could potentially be
done in board support code by using a bus notifier to look for the
critical device.

[1]http://forum.soft32.com/linux/RFC-drivers-base-Add-bus_register_notifier_alldev-variant-ftopict478522.html

> 
> 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 ?

No, don't gate hwspinlock.  I don't see any reason to defer it for
this issue.

> On Wed, Oct 20, 2010 at 3:20 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
> > The issue of probe order still needs to be resolved for those of us who
> > do want all the drivers built into the kernel.

+1

> 
> What about doing something similar to the way suspend/resume and the
> device hierarchy interact ?
> 
> device_resume waits for its parent to be resumed before waking up the
> device - this sounds similar to what ->probe() should do: wait for its
> device dependency to probe first (so in this case, i2c-omap should
> wait for hwspinlock).
> 
> Conversely, device_suspend waits for all its children to be suspended
> before continuing, which sounds just like what ->remove() should do
> (so again, in this case, the hwspinlock device should wait for all its
> users to be removed before bailing).

This works for the simple hierarchy case, but it doesn't help the
multi-dependency case.

g.

[PATCH 3/3] omap: add hwspinlock device

[Grant Likely]

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@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:
> 
> 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.
> 
> All we will be left to take care of is the order of the ->probe()
> execution (assuming we want both the i2c and the hwspinlock drivers to
> be device_initcall)

Having fought with this kind of thing before, I would strongly
recommend making the interface either all-dynamic, or all-predefined,
but not a mixture of the two.

> 
> >
> > 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.
> 
> Why would we need platform-specific function pointers here ? I'm not
> sure I'm following this one.
> 
> Thanks,
> Ohad.
> 
> 
> >
> > Kevin
> >

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

[Kevin Hilman]

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

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

Yes, that is ok.  At least then it can be tested by default as a module.

Kevin

[PATCH 3/3] omap: add hwspinlock device

[Kevin Hilman]

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

> On Wed, Oct 20, 2010 at 1:53 AM, Kevin Hilman
> <khilman@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:
>
> 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. 

> All we will be left to take care of is the order of the ->probe()
> execution (assuming we want both the i2c and the hwspinlock drivers to
> be device_initcall)

I understand the dependency between i2c and hwspinlock for some
platforms with a shared i2c bus.  Besides that being a broken hardware
design, I can see the need for the i2c driver to take a hwspinlock for
i2c xfers, but why does the i2c driver need to take the hwspinlock at
probe time?  Presumably, this is before the remote cores are executing
code.

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

Kevin

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
<khilman@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()
>> execution (assuming we want both the i2c and the hwspinlock drivers to
>> be device_initcall)
>
> I understand the dependency between i2c and hwspinlock for some
> platforms with a shared i2c bus. ?Besides that being a broken hardware
> design, I can see the need for the i2c driver to take a hwspinlock for
> i2c xfers, but why does the i2c driver need to take the hwspinlock at
> probe time?

It doesn't; it just needs to reserve a hwspinlock, and for that, we
need the hwspinlock driver in place.

> ?Presumably, this is before the remote cores are executing
> code.
>
>>>
>>> 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.
>>
>> 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 ?

With the previous _request_specific() allocation API, the important
issue to follow was the timing:  it had to be called before any driver
gets the chance to call the dynamic _request() API.
That's why we had to have early board code calling it. Obviously the
hwspinlock instance would then had to be delivered to the driver via
platform data.

But now, if we get rid of that static allocation method entirely,
i2c_omap can just call hwspinlock_request() on probe(), but again, no
pdata function pointers are involved because this API is
EXPORT_SYMBOL'ed.

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

[Daniel Walker]

On Wed, 2010-10-20 at 10:53 +0100, Russell King - ARM Linux wrote:
> 
> To: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Hari Kanigeri <h-kanigeri2@ti.com>, Benoit Cousson <b-cousson@ti.com>,
>         Tony Lindgren <tony@atomide.com>, Greg KH <greg@kroah.com>,
>         linux-kernel at vger.kernel.org,
>         Grant Likely <grant.likely@secretlab.ca>, mattw at codeaurora.org,
>         akpm at linux-foundation.org, Suman Anna <s-anna@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.

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

[Bryan Huntsman]

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

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

[Ohad Ben-Cohen]

On Tue, Oct 19, 2010 at 11:08 PM, Arnd Bergmann <arnd@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.

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.

Thanks,
Ohad.

[PATCH 3/3] omap: add hwspinlock device

[Kevin Hilman]

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

> On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> <khilman@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()
>>> execution (assuming we want both the i2c and the hwspinlock drivers to
>>> be device_initcall)
>>
>> I understand the dependency between i2c and hwspinlock for some
>> platforms with a shared i2c bus. ?Besides that being a broken hardware
>> design, I can see the need for the i2c driver to take a hwspinlock for
>> i2c xfers, but why does the i2c driver need to take the hwspinlock at
>> probe time?
>
> It doesn't; it just needs to reserve a hwspinlock, and for that, we
> need the hwspinlock driver in place.

OK

>> ?Presumably, this is before the remote cores are executing
>> code.
>>
>>>>
>>>> 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.
>>>
>>> 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.

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

If the _request_specific() API is removed, and board code no longer
needs to call hwspinlock API, then this issue is moot.  However, if
board code ever needs to call the hwspinlock API, then pdata func
pointers are needed to handle both the case of late driver probe
or driver built as a module.

> With the previous _request_specific() allocation API, the important
> issue to follow was the timing:  it had to be called before any driver
> gets the chance to call the dynamic _request() API.
> That's why we had to have early board code calling it. Obviously the
> hwspinlock instance would then had to be delivered to the driver via
> platform data.
>
> But now, if we get rid of that static allocation method entirely,
> i2c_omap can just call hwspinlock_request() on probe(), but again, no
> pdata function pointers are involved because this API is
> EXPORT_SYMBOL'ed.

Agreed.  If you get rid of the _request_specific() API, then this is not
needed in the board code and pdata function pointers should not be
needed.

So then, we're back to how to ensure probe order of hwspinlock vs
i2c. :/

I agree with others that this is a much broader problem, and should not
hold up the hwspinlock driver, so for now, making hwspinlock have an
initcall before subsys_initcall is OK with me.  Probably arch_initcall()
is fine here.

I suggest you add a comment in the code at the initcall point as to why
it is using arch_initcall(), namely it has to load before i2c because...

Kevin

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

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

On Thu, Oct 21, 2010 at 1:58 AM, Kevin Hilman
<khilman@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.
>
...
> ... if
> board code ever needs to call the hwspinlock API, then pdata func
> pointers are needed to handle both the case of late driver probe
> or driver built as a module.

How would pdata func pointers help here ?

As you say, pdata func pointers are used in scenarios where
platform-specific code needs to be executed from a driver.

But this is not the case here; here we have driver code ( the
_request_specific() API) that must be called very early in the boot,
before any other driver get the chance to call the dynamic _request()
API.

And to be able to call _request_specific() API that early in the boot,
we must have the hwspinlock driver in place (loaded and already
probe()'ed). So we had to give both the hwspinlock device creation
code and its driver a very early initcall state.

Of course, as we all agree, if we get rid of the _request_specific()
API, hwspinlock will only race with its users (namely i2c_omap), and
in this case arch_initcall() is enough.

> I agree with others that this is a much broader problem, and should not
> hold up the hwspinlock driver, so for now, making hwspinlock have an
> initcall before subsys_initcall is OK with me. ?Probably arch_initcall()
> is fine here.
>
> I suggest you add a comment in the code at the initcall point as to why
> it is using arch_initcall(), namely it has to load before i2c because...

Sure.

Thanks,
Ohad.

[PATCH 3/3] omap: add hwspinlock device

[Kamoolkar, Mugdha]

> <khilman@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 
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.

> 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()
> >> execution (assuming we want both the i2c and the hwspinlock
> drivers to
> >> be device_initcall)
> >
> > I understand the dependency between i2c and hwspinlock for some
> > platforms with a shared i2c bus. ?Besides that being a broken
> hardware
> > design, I can see the need for the i2c driver to take a hwspinlock
> for
> > i2c xfers, but why does the i2c driver need to take the hwspinlock
> at
> > probe time?
> 
> It doesn't; it just needs to reserve a hwspinlock, and for that, we
> need the hwspinlock driver in place.
> 
> > ?Presumably, this is before the remote cores are executing
> > code.
> >
> >>>
> >>> 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.
> >>
> >> 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 ?
> 
> With the previous _request_specific() allocation API, the important
> issue to follow was the timing:  it had to be called before any
> driver
> gets the chance to call the dynamic _request() API.
> That's why we had to have early board code calling it. Obviously the
> hwspinlock instance would then had to be delivered to the driver via
> platform data.
> 
> But now, if we get rid of that static allocation method entirely,
> i2c_omap can just call hwspinlock_request() on probe(), but again,
> no
> pdata function pointers are involved because this API is
> EXPORT_SYMBOL'ed.
> --
> 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

Regards,
Mugdha

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

[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 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

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Thu, Oct 21, 2010 at 10:36 AM, Kamoolkar, Mugdha <mugdha@ti.com> wrote:
>> <khilman@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 ?

Thanks,
Ohad.

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

[Ohad Ben-Cohen]

On Thu, Oct 21, 2010 at 11:04 AM, Arnd Bergmann <arnd@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.

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)

Thanks,
Ohad.

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

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

Sounds good!

	Arnd

[PATCH 3/3] omap: add hwspinlock device

[Kanigeri, Hari]

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.

Thank you,
Best regards,
Hari

[PATCH 3/3] omap: add hwspinlock device

[Kamoolkar, Mugdha]

> -----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@ti.com>
> wrote:
> >> <khilman@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 ?
That is true. Perhaps we should consider adding a software 
implementation over HW spinlocks. 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. So adding a software 
module that uses a hardware spinlock for protection of its shared memory 
could be written, and then i2c could use that software module. In that 
case, if this is the only reason why i2c driver needs shared memory, 
that need would go away.

> 
> Thanks,
> Ohad.

[PATCH 3/3] omap: add hwspinlock device

[Kamoolkar, Mugdha]

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
> > 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.
> 
I also considered this, and had the same issue with it as what you 
mentioned above, that the virtual address for the i2c driver on m3 needs 
to be known to the Linux driver. This could potentially be taken in 
through kconfig, because there's no harm in fixing virtual maps for the 
slaves. The problem is in fixing physical memory regions. Note that this 
assumes presence of a slave MMU. In case slave MMU is not present, the 
physical address needs to be taken in through kconfig, which makes 
rearranging the memory map a virtual nightmare if multiple drivers start 
doing such things.

We do need to consider the possibility that in future there might be 
even more multi-core drivers which not only need hw spinlocks, but also 
need notifications and some shared memory to do their work effectively. As shared peripherals increase, this is a very likely possibility.
So I do believe that any effort we put on fixing this the right way 
needs to take into account all three (hw spinlocks, ipc interrupts and 
shared memory). That's of course, out of the scope of this hw spinlock 
patch, but more in the scope of syslink discussions at LPC.

> Thank you,
> Best regards,
> Hari

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Fri, Oct 22, 2010 at 11:59 AM, Kamoolkar, Mugdha <mugdha@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.

Sure, this layer can provide any number of locks over a single
hwspinlock (with the obvious price of hwspinlock contention).

[PATCH 3/3] omap: add hwspinlock device

[Tony Lindgren]

* Ohad Ben-Cohen <ohad@wizery.com> [101020 12:12]:
> On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> <khilman@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

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

[Tony Lindgren]

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

[PATCH 3/3] omap: add hwspinlock device

[Grant Likely]

On Fri, Oct 22, 2010 at 09:56:13AM -0700, Tony Lindgren wrote:
> * Ohad Ben-Cohen <ohad@wizery.com> [101020 12:12]:
> > On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> > <khilman@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.

g.

[PATCH 3/3] omap: add hwspinlock device

[Tony Lindgren]

* Grant Likely <grant.likely@secretlab.ca> [101022 09:54]:
> On Fri, Oct 22, 2010 at 09:56:13AM -0700, Tony Lindgren wrote:
> > * Ohad Ben-Cohen <ohad@wizery.com> [101020 12:12]:
> > > On Wed, Oct 20, 2010 at 8:37 PM, Kevin Hilman
> > > <khilman@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.

Great, thanks.

Tony

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

Hi Tony,

On Fri, Oct 22, 2010 at 6:56 PM, Tony Lindgren <tony@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:

http://www.spinics.net/lists/linux-arm-msm/msg00324.html

So initially I opted for (2). I have an implementation ready - it's a
common drivers/hwspinlock/core.c framework with a small omap-specific
part at drivers/hwspinlock/omap_hwspinlock.c. The core has all the
logic while the latter, omap-specific implementation, is really small
- it is basically just registering lock instances, that have a
trylock/unlock/relax ops struct, with the common framework.

But lack of additional users (besides the OMAP hardware), and lack of
generic drivers that would use it (we have syslink, which currently
tend to only need this from user space, i2c-omap and omap's upcoming
resource manager), made it look like an overkill for now.

So simplicity won, and (3) was eventually submitted. It exposes
exactly the same interface like (2) would (s/omap_hwspin/hwspin/), and
it's relatively easy to turn it back into a common framework in case
additional users show up.

But if you feel that (2) is justifiable/desirable, I would be more
than happy to submit that version.

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

Do you mean (1) and then pass the whole bunch of APIs
(request/free/lock variants/unlock/get_id) via pdata ?

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

Thanks,
Ohad.

[PATCH 3/3] omap: add hwspinlock device

[Tony Lindgren]

* Ohad Ben-Cohen <ohad@wizery.com> [101024 10:45]:
> Hi Tony,
> 
> On Fri, Oct 22, 2010 at 6:56 PM, Tony Lindgren <tony@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:
> 
> http://www.spinics.net/lists/linux-arm-msm/msg00324.html
> 
> So initially I opted for (2). I have an implementation ready - it's a
> common drivers/hwspinlock/core.c framework with a small omap-specific
> part at drivers/hwspinlock/omap_hwspinlock.c. The core has all the
> logic while the latter, omap-specific implementation, is really small
> - it is basically just registering lock instances, that have a
> trylock/unlock/relax ops struct, with the common framework.
> 
> But lack of additional users (besides the OMAP hardware), and lack of
> generic drivers that would use it (we have syslink, which currently
> tend to only need this from user space, i2c-omap and omap's upcoming
> resource manager), made it look like an overkill for now.
> 
> So simplicity won, and (3) was eventually submitted. It exposes
> exactly the same interface like (2) would (s/omap_hwspin/hwspin/), and
> it's relatively easy to turn it back into a common framework in case
> additional users show up.
> 
> But 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.
 
> > 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.
> 
> Do you mean (1) and then pass the whole bunch of APIs
> (request/free/lock variants/unlock/get_id) via pdata ?
> 
> 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.

Regards,

Tony

[PATCH 3/3] omap: add hwspinlock device

[Ohad Ben-Cohen]

On Mon, Oct 25, 2010 at 9:02 PM, Tony Lindgren <tony@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.

Does this sound reasonable to you ?

Thanks,
Ohad.

[PATCH 3/3] omap: add hwspinlock device

[Tony Lindgren]

* Ohad Ben-Cohen <ohad@wizery.com> [101026 04:45]:
> On Mon, Oct 25, 2010 at 9:02 PM, Tony Lindgren <tony@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.
> 
> Does this sound reasonable to you ?

Sounds good to me.

Tony