[PATCH 0/5] omap:hwspinlock support-omap4

[PATCH 0/5] omap:hwspinlock support-omap4

[Hari Kanigeri]

Resending the hwspinlock patch Simon Que sent few days ago by splitting
the patch into logical pieces. The first 4 patches are based on Simon's
patch.
https://patchwork.kernel.org/patch/110672/

The 5th patch is a new patch to address race condition issue with
I2C driver usage.


Created driver for OMAP hardware spinlock.  This driver supports:
- Reserved spinlocks for internal use
- Dynamic allocation of unreserved locks
- Lock, unlock, and trylock functions, with or without disabling irqs/preempt
- Registered as a platform device driver

The device initialization uses hwmod to configure the devices.
One device will be created for each IP.
It will pass spinlock register offset info to the driver.
The device initialization file is:
                arch/arm/mach-omap2/hwspinlocks.c

The driver takes in register offset info passed in device initialization.
It uses hwmod to obtain the base address of the hardware spinlock module.
Then it reads info from the registers.
The function hwspinlock_probe() initializes the array of spinlock structures,
each containing a spinlock register address calculated from the base address
and lock offsets.  The device driver file is:
                arch/arm/plat-omap/hwspinlock.c

Here's an API summary:
int hwspinlock_lock(struct hwspinlock *);
        Attempt to lock a hardware spinlock.  If it is busy, the function will
        keep trying until it succeeds.  This is a blocking function.
int hwspinlock_trylock(struct hwspinlock *);
        Attempt to lock a hardware spinlock.  If it is busy, the function will
        return BUSY.  If it succeeds in locking, the function will return
        ACQUIRED.  This is a non-blocking function.
int hwspinlock_unlock(struct hwspinlock *);
        Unlock a hardware spinlock.

struct hwspinlock *hwspinlock_request(void);
        Provides for "dynamic allocation" of a hardware spinlock.  It returns
        the handle to the next available (unallocated) spinlock.  If no more
        locks are available, it returns NULL.
struct hwspinlock *hwspinlock_request_specific(unsigned int);
        Provides for "static allocation" of a specific hardware spinlock. This
        allows the system to use a specific spinlock, identified by an ID. If
        the ID is invalid or if the desired lock is already allocated, this
        will return NULL.  Otherwise it returns a spinlock handle.
int hwspinlock_free(struct hwspinlock *);
        Frees an allocated hardware spinlock (either reserved or unreserved).

Hari Kanigeri (1):
  omap:hwspinlocks-ensure the order of registration

Simon Que (4):
  omap:hwmod-hwspinlock-enable
  omap:hwspinlock-define HWSPINLOCK base address
  omap:hwspinlock-added hwspinlock driver
  omap:hwspinlock-add build support

 arch/arm/mach-omap2/Makefile                 |    2 +
 arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    2 +-
 arch/arm/plat-omap/Makefile                  |    2 +
 arch/arm/plat-omap/hwspinlock.c              |  334 ++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
 arch/arm/plat-omap/include/plat/omap44xx.h   |    5 +
 7 files changed, 443 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
 create mode 100644 arch/arm/plat-omap/hwspinlock.c
 create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h

[PATCH 1/5] omap:hwmod-hwspinlock-enable

[Hari Kanigeri]

From: Simon Que <sque@ti.com>

uncomment the hwmod part for hwspinlock

Signed-off-by: Simon Que <sque@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 03bb3db..41dc77d 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -4966,7 +4966,7 @@ static __initdata struct omap_hwmod *omap44xx_hwmods[] = {
 /*	&omap44xx_smartreflex_iva_hwmod, */
 /*	&omap44xx_smartreflex_mpu_hwmod, */
 	/* spinlock class */
-/*	&omap44xx_spinlock_hwmod, */
+	&omap44xx_spinlock_hwmod,
 	/* timer class */
 	&omap44xx_timer1_hwmod,
 	&omap44xx_timer2_hwmod,
-- 
1.7.0

[PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Hari Kanigeri]

From: Simon Que <sque@ti.com>

Add HWSPINLCOK base address information in omap44xx.h

Signed-off-by: Simon Que <sque@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/plat-omap/include/plat/omap44xx.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap44xx.h b/arch/arm/plat-omap/include/plat/omap44xx.h
index 76832d3..8016508 100644
--- a/arch/arm/plat-omap/include/plat/omap44xx.h
+++ b/arch/arm/plat-omap/include/plat/omap44xx.h
@@ -49,5 +49,10 @@
 #define OMAP44XX_MAILBOX_BASE		(L4_44XX_BASE + 0xF4000)
 #define OMAP44XX_HSUSB_OTG_BASE		(L4_44XX_BASE + 0xAB000)
 
+#define OMAP4_MMU1_BASE			0x55082000
+#define OMAP4_MMU2_BASE			0x4A066000
+
+#define OMAP44XX_SPINLOCK_BASE		(L4_44XX_BASE + 0xF6000)
+
 #endif /* __ASM_ARCH_OMAP44XX_H */
 
-- 
1.7.0

[PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Hari Kanigeri]

From: Simon Que <sque@ti.com>

Created driver for OMAP hardware spinlock.  This driver supports:
- Reserved spinlocks for internal use
- Dynamic allocation of unreserved locks
- Lock, unlock, and trylock functions, with or without disabling irqs/preempt
- Registered as a platform device driver

The device initialization uses hwmod to configure the devices.
One device will be created for each IP.  It will pass spinlock register offset
info to the driver.  The device initialization file is:
		arch/arm/mach-omap2/hwspinlocks.c

The driver takes in register offset info passed in device initialization.
It uses hwmod to obtain the base address of the hardware spinlock module.
Then it reads info from the registers.  The function hwspinlock_probe()
initializes the array of spinlock structures, each containing a spinlock
register address calculated from the base address and lock offsets.
The device driver file is:
		arch/arm/plat-omap/hwspinlock.c

Here's an API summary:
int hwspinlock_lock(struct hwspinlock *);
	Attempt to lock a hardware spinlock.  If it is busy, the function will
	keep trying until it succeeds.  This is a blocking function.
int hwspinlock_trylock(struct hwspinlock *);
	Attempt to lock a hardware spinlock.  If it is busy, the function will
	return BUSY.  If it succeeds in locking, the function will return
	ACQUIRED.  This is a non-blocking function.
int hwspinlock_unlock(struct hwspinlock *);
	Unlock a hardware spinlock.

struct hwspinlock *hwspinlock_request(void);
	Provides for "dynamic allocation" of a hardware spinlock.  It returns
	the handle to the next available (unallocated) spinlock.  If no more
	locks are available, it returns NULL.
struct hwspinlock *hwspinlock_request_specific(unsigned int);
	Provides for "static allocation" of a specific hardware spinlock. This
	allows the system to use a specific spinlock, identified by an ID. If
	the ID is invalid or if the desired lock is already allocated, this
	will return NULL.  Otherwise it returns a spinlock handle.
int hwspinlock_free(struct hwspinlock *);
	Frees an allocated hardware spinlock (either reserved or unreserved).

Signed-off-by: Simon Que <sque@ti.com>
Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
 arch/arm/plat-omap/hwspinlock.c              |  335 ++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
 3 files changed, 434 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
 create mode 100644 arch/arm/plat-omap/hwspinlock.c
 create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h

diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach-omap2/hwspinlocks.c
new file mode 100644
index 0000000..f0f05e1
--- /dev/null
+++ b/arch/arm/mach-omap2/hwspinlocks.c
@@ -0,0 +1,70 @@
+/*
+ * OMAP hardware spinlock device initialization
+ *
+ * Copyright (C) 2010 Texas Instruments. All rights reserved.
+ *
+ * Contact: Simon Que <sque@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/module.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+
+#include <plat/hwspinlock.h>
+#include <plat/omap_device.h>
+#include <plat/omap_hwmod.h>
+
+/* Spinlock register offsets */
+#define REVISION_OFFSET			0x0000
+#define SYSCONFIG_OFFSET		0x0010
+#define SYSSTATUS_OFFSET		0x0014
+#define LOCK_BASE_OFFSET		0x0800
+#define LOCK_OFFSET(i)			(LOCK_BASE_OFFSET + 0x4 * (i))
+
+/* Initialization function */
+int __init hwspinlocks_init(void)
+{
+	int retval = 0;
+
+	struct hwspinlock_plat_info *pdata;
+	struct omap_hwmod *oh;
+	char *oh_name, *pdev_name;
+
+	oh_name = "spinlock";
+	oh = omap_hwmod_lookup(oh_name);
+	if (WARN_ON(oh == NULL))
+		return -EINVAL;
+
+	pdev_name = "hwspinlock";
+
+	/* Pass data to device initialization */
+	pdata = kzalloc(sizeof(struct hwspinlock_plat_info), GFP_KERNEL);
+	if (WARN_ON(pdata == NULL))
+		return -ENOMEM;
+	pdata->sysstatus_offset = SYSSTATUS_OFFSET;
+	pdata->lock_base_offset = LOCK_BASE_OFFSET;
+
+	omap_device_build(pdev_name, 0, oh, pdata,
+			sizeof(struct hwspinlock_plat_info), NULL, 0, false);
+
+	return retval;
+}
+module_init(hwspinlocks_init);
diff --git a/arch/arm/plat-omap/hwspinlock.c b/arch/arm/plat-omap/hwspinlock.c
new file mode 100644
index 0000000..1883add
--- /dev/null
+++ b/arch/arm/plat-omap/hwspinlock.c
@@ -0,0 +1,335 @@
+/*
+ * OMAP hardware spinlock driver
+ *
+ * Copyright (C) 2010 Texas Instruments. All rights reserved.
+ *
+ * Contact: Simon Que <sque@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
+ *
+ * This driver supports:
+ * - Reserved spinlocks for internal use
+ * - Dynamic allocation of unreserved locks
+ * - Lock, unlock, and trylock functions, with or without disabling irqs/preempt
+ * - Registered as a platform device driver
+ *
+ * The device initialization uses hwmod to configure the devices.  One device
+ * will be created for each IP.  It will pass spinlock register offset info to
+ * the driver.  The device initialization file is:
+ *          arch/arm/mach-omap2/hwspinlocks.c
+ *
+ * The driver takes in register offset info passed in device initialization.
+ * It uses hwmod to obtain the base address of the hardware spinlock module.
+ * Then it reads info from the registers.  The function hwspinlock_probe()
+ * initializes the array of spinlock structures, each containing a spinlock
+ * register address calculated from the base address and lock offsets.
+ *
+ * Here's an API summary:
+ *
+ * int hwspinlock_lock(struct hwspinlock *);
+ *      Attempt to lock a hardware spinlock.  If it is busy, the function will
+ *      keep trying until it succeeds.  This is a blocking function.
+ * int hwspinlock_trylock(struct hwspinlock *);
+ *      Attempt to lock a hardware spinlock.  If it is busy, the function will
+ *      return BUSY.  If it succeeds in locking, the function will return
+ *      ACQUIRED.  This is a non-blocking function
+ * int hwspinlock_unlock(struct hwspinlock *);
+ *      Unlock a hardware spinlock.
+ *
+ * struct hwspinlock *hwspinlock_request(void);
+ *      Provides for "dynamic allocation" of a hardware spinlock.  It returns
+ *      the handle to the next available (unallocated) spinlock.  If no more
+ *      locks are available, it returns NULL.
+ * struct hwspinlock *hwspinlock_request_specific(unsigned int);
+ *      Provides for "static allocation" of a specific hardware spinlock. This
+ *      allows the system to use a specific spinlock, identified by an ID. If
+ *      the ID is invalid or if the desired lock is already allocated, this
+ *      will return NULL.  Otherwise it returns a spinlock handle.
+ * int hwspinlock_free(struct hwspinlock *);
+ *      Frees an allocated hardware spinlock (either reserved or unreserved).
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include <plat/hwspinlock.h>
+
+/* Spinlock count code */
+#define SPINLOCK_32_REGS		1
+#define SPINLOCK_64_REGS		2
+#define SPINLOCK_128_REGS		4
+#define SPINLOCK_256_REGS		8
+#define SPINLOCK_NUMLOCKS_OFFSET	24
+
+/* for managing a hardware spinlock module */
+struct hwspinlock_state {
+	bool is_init;			/* For first-time initialization */
+	int num_locks;			/* Total number of locks in system */
+	spinlock_t local_lock;		/* Local protection */
+	void __iomem *io_base;		/* Mapped base address */
+};
+
+/* Points to the hardware spinlock module */
+static struct hwspinlock_state hwspinlock_state;
+static struct hwspinlock_state *hwspinlock_module = &hwspinlock_state;
+
+/* Spinlock object */
+struct hwspinlock {
+	bool is_init;
+	int id;
+	void __iomem *lock_reg;
+	bool is_allocated;
+	struct platform_device *pdev;
+};
+
+/* Array of spinlocks */
+static struct hwspinlock *hwspinlocks;
+
+/* API functions */
+
+/* Busy loop to acquire a spinlock */
+int hwspinlock_lock(struct hwspinlock *handle)
+{
+	int retval;
+
+	if (WARN_ON(handle == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(in_irq()))
+		return -EPERM;
+
+	if (pm_runtime_get(&handle->pdev->dev) < 0)
+		return -ENODEV;
+
+	/* Attempt to acquire the lock by reading from it */
+	do {
+		retval = readl(handle->lock_reg);
+	} while (retval == HWSPINLOCK_BUSY);
+
+	return 0;
+}
+EXPORT_SYMBOL(hwspinlock_lock);
+
+/* Attempt to acquire a spinlock once */
+int hwspinlock_trylock(struct hwspinlock *handle)
+{
+	int retval = 0;
+
+	if (WARN_ON(handle == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(in_irq()))
+		return -EPERM;
+
+	if (pm_runtime_get(&handle->pdev->dev) < 0)
+		return -ENODEV;
+
+	/* Attempt to acquire the lock by reading from it */
+	retval = readl(handle->lock_reg);
+
+	if (retval == HWSPINLOCK_BUSY)
+		pm_runtime_put(&handle->pdev->dev);
+
+	return retval;
+}
+EXPORT_SYMBOL(hwspinlock_trylock);
+
+/* Release a spinlock */
+int hwspinlock_unlock(struct hwspinlock *handle)
+{
+	if (WARN_ON(handle == NULL))
+		return -EINVAL;
+
+	/* Release it by writing 0 to it */
+	writel(0, handle->lock_reg);
+
+	pm_runtime_put(&handle->pdev->dev);
+
+	return 0;
+}
+EXPORT_SYMBOL(hwspinlock_unlock);
+
+/* Request an unclaimed spinlock */
+struct hwspinlock *hwspinlock_request(void)
+{
+	int i;
+	bool found = false;
+	struct hwspinlock *handle = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
+	/* Search for an unclaimed, unreserved lock */
+	for (i = 0; i < hwspinlock_module->num_locks && !found; i++) {
+		if (!hwspinlocks[i].is_allocated) {
+			found = true;
+			handle = &hwspinlocks[i];
+		}
+	}
+	spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
+
+	/* Return error if no more locks available */
+	if (!found)
+		return NULL;
+
+	handle->is_allocated = true;
+
+	return handle;
+}
+EXPORT_SYMBOL(hwspinlock_request);
+
+/* Request an unclaimed spinlock by ID */
+struct hwspinlock *hwspinlock_request_specific(unsigned int id)
+{
+	struct hwspinlock *handle = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
+
+	if (WARN_ON(hwspinlocks[id].is_allocated))
+		goto exit;
+
+	handle = &hwspinlocks[id];
+	handle->is_allocated = true;
+
+exit:
+	spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
+	return handle;
+}
+EXPORT_SYMBOL(hwspinlock_request_specific);
+
+/* Release a claimed spinlock */
+int hwspinlock_free(struct hwspinlock *handle)
+{
+	if (WARN_ON(handle == NULL))
+		return -EINVAL;
+
+	if (WARN_ON(!handle->is_allocated))
+		return -ENOMEM;
+
+	handle->is_allocated = false;
+
+	return 0;
+}
+EXPORT_SYMBOL(hwspinlock_free);
+
+/* Probe function */
+static int __devinit hwspinlock_probe(struct platform_device *pdev)
+{
+	struct hwspinlock_plat_info *pdata = pdev->dev.platform_data;
+	struct resource *res;
+	void __iomem *io_base;
+	int id;
+
+	void __iomem *sysstatus_reg;
+
+	/* Determine number of locks */
+	sysstatus_reg = ioremap(OMAP44XX_SPINLOCK_BASE +
+					pdata->sysstatus_offset, sizeof(u32));
+	switch (readl(sysstatus_reg) >> SPINLOCK_NUMLOCKS_OFFSET) {
+	case SPINLOCK_32_REGS:
+		hwspinlock_module->num_locks = 32;
+		break;
+	case SPINLOCK_64_REGS:
+		hwspinlock_module->num_locks = 64;
+		break;
+	case SPINLOCK_128_REGS:
+		hwspinlock_module->num_locks = 128;
+		break;
+	case SPINLOCK_256_REGS:
+		hwspinlock_module->num_locks = 256;
+		break;
+	default:
+		return -EINVAL;	/* Invalid spinlock count code */
+	}
+	iounmap(sysstatus_reg);
+
+	/* Allocate spinlock device objects */
+	hwspinlocks = kmalloc(sizeof(struct hwspinlock) *
+			hwspinlock_module->num_locks, GFP_KERNEL);
+	if (WARN_ON(hwspinlocks == NULL))
+		return -ENOMEM;
+
+	/* Initialize local lock */
+	spin_lock_init(&hwspinlock_module->local_lock);
+
+	/* Get address info */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	/* Map spinlock module address space */
+	io_base = ioremap(res->start, resource_size(res));
+	hwspinlock_module->io_base = io_base;
+
+	/* Set up each individual lock handle */
+	for (id = 0; id < hwspinlock_module->num_locks; id++) {
+		hwspinlocks[id].id		= id;
+		hwspinlocks[id].pdev		= pdev;
+
+		hwspinlocks[id].is_init		= true;
+		hwspinlocks[id].is_allocated	= false;
+
+		hwspinlocks[id].lock_reg	= io_base + pdata->
+					lock_base_offset + sizeof(u32) * id;
+	}
+
+	return 0;
+}
+
+static struct platform_driver hwspinlock_driver = {
+	.probe		= hwspinlock_probe,
+	.driver		= {
+		.name	= "hwspinlock",
+	},
+};
+
+/* Initialization function */
+static int __init hwspinlock_init(void)
+{
+	int retval = 0;
+
+	/* Register spinlock driver */
+	retval = platform_driver_register(&hwspinlock_driver);
+
+	return retval;
+}
+
+/* Cleanup function */
+static void __exit hwspinlock_exit(void)
+{
+	int id;
+
+	platform_driver_unregister(&hwspinlock_driver);
+
+	for (id = 0; id < hwspinlock_module->num_locks; id++)
+		hwspinlocks[id].is_init = false;
+	iounmap(hwspinlock_module->io_base);
+
+	/* Free spinlock device objects */
+	if (hwspinlock_module->is_init)
+		kfree(hwspinlocks);
+}
+
+module_init(hwspinlock_init);
+module_exit(hwspinlock_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware spinlock driver");
+MODULE_AUTHOR("Simon Que");
+MODULE_AUTHOR("Hari Kanigeri");
diff --git a/arch/arm/plat-omap/include/plat/hwspinlock.h b/arch/arm/plat-omap/include/plat/hwspinlock.h
new file mode 100644
index 0000000..8c69ca5
--- /dev/null
+++ b/arch/arm/plat-omap/include/plat/hwspinlock.h
@@ -0,0 +1,29 @@
+/* hwspinlock.h */
+
+#ifndef HWSPINLOCK_H
+#define HWSPINLOCK_H
+
+#include <linux/platform_device.h>
+#include <plat/omap44xx.h>
+
+/* Read values from the spinlock register */
+#define HWSPINLOCK_ACQUIRED 0
+#define HWSPINLOCK_BUSY 1
+
+/* Device data */
+struct hwspinlock_plat_info {
+	u32 sysstatus_offset;		/* System status register offset */
+	u32 lock_base_offset;		/* Offset of spinlock registers */
+};
+
+struct hwspinlock;
+
+int hwspinlock_lock(struct hwspinlock *handle);
+int hwspinlock_trylock(struct hwspinlock *handle);
+int hwspinlock_unlock(struct hwspinlock *handle);
+
+struct hwspinlock *hwspinlock_request(void);
+struct hwspinlock *hwspinlock_request_specific(unsigned int id);
+int hwspinlock_free(struct hwspinlock *hwspinlock_ptr);
+
+#endif /* HWSPINLOCK_H */
-- 
1.7.0

[PATCH 4/5] omap:hwspinlock-add build support

[Hari Kanigeri]

From: Simon Que <sque@ti.com>

Patch to add suport to build hwspinlock modules

Signed-off-by: Simon Que <sque@ti.com>
---
 arch/arm/mach-omap2/Makefile |    2 ++
 arch/arm/plat-omap/Makefile  |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 13825a2..d29ea57 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -177,3 +177,5 @@ smc91x-$(CONFIG_SMC91X)			:= gpmc-smc91x.o
 obj-y					+= $(smc91x-m) $(smc91x-y)
 
 obj-$(CONFIG_LEDS_PWM)			+= twl6030_pwm.o
+
+obj-$(CONFIG_ARCH_OMAP4)		+= hwspinlocks.o
diff --git a/arch/arm/plat-omap/Makefile b/arch/arm/plat-omap/Makefile
index 87e5124..5d08f1c 100644
--- a/arch/arm/plat-omap/Makefile
+++ b/arch/arm/plat-omap/Makefile
@@ -38,3 +38,5 @@ obj-y += $(i2c-omap-m) $(i2c-omap-y)
 obj-$(CONFIG_OMAP_MBOX_FWK) += mailbox.o
 
 obj-$(CONFIG_OMAP_PM_NOOP) += omap-pm-noop.o
+
+obj-$(CONFIG_ARCH_OMAP4) += hwspinlock.o
-- 
1.7.0

[PATCH 5/5] omap:hwspinlocks-ensure the order of registration

[Hari Kanigeri]

Ensure that the hwspinlock driver is registered prior to
I2C driver registration since I2C is dependent on hwspinlock.

Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
---
 arch/arm/mach-omap2/hwspinlocks.c |    2 +-
 arch/arm/plat-omap/hwspinlock.c   |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach-omap2/hwspinlocks.c
index f0f05e1..a9f85b9 100644
--- a/arch/arm/mach-omap2/hwspinlocks.c
+++ b/arch/arm/mach-omap2/hwspinlocks.c
@@ -67,4 +67,4 @@ int __init hwspinlocks_init(void)
 
 	return retval;
 }
-module_init(hwspinlocks_init);
+postcore_initcall(hwspinlocks_init);
diff --git a/arch/arm/plat-omap/hwspinlock.c b/arch/arm/plat-omap/hwspinlock.c
index 1883add..3742b04 100644
--- a/arch/arm/plat-omap/hwspinlock.c
+++ b/arch/arm/plat-omap/hwspinlock.c
@@ -309,6 +309,7 @@ static int __init hwspinlock_init(void)
 
 	return retval;
 }
+postcore_initcall(hwspinlock_init);
 
 /* Cleanup function */
 static void __exit hwspinlock_exit(void)
@@ -325,8 +326,6 @@ static void __exit hwspinlock_exit(void)
 	if (hwspinlock_module->is_init)
 		kfree(hwspinlocks);
 }
-
-module_init(hwspinlock_init);
 module_exit(hwspinlock_exit);
 
 MODULE_LICENSE("GPL v2");
-- 
1.7.0

RE: [PATCH 0/5] omap:hwspinlock support-omap4

[Shilimkar, Santosh]

Hari,
> -----Original Message-----
> From: Kanigeri, Hari
> Sent: Monday, July 19, 2010 10:20 PM
> To: Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> Subject: [PATCH 0/5] omap:hwspinlock support-omap4
> 
> Resending the hwspinlock patch Simon Que sent few days ago by splitting
> the patch into logical pieces. The first 4 patches are based on Simon's
> patch.
> https://patchwork.kernel.org/patch/110672/
> 
> The 5th patch is a new patch to address race condition issue with
> I2C driver usage.
> 
> 
> Created driver for OMAP hardware spinlock.  This driver supports:
> - Reserved spinlocks for internal use
> - Dynamic allocation of unreserved locks
> - Lock, unlock, and trylock functions, with or without disabling
> irqs/preempt
> - Registered as a platform device driver
> 
> The device initialization uses hwmod to configure the devices.
> One device will be created for each IP.
> It will pass spinlock register offset info to the driver.
> The device initialization file is:
>                 arch/arm/mach-omap2/hwspinlocks.c
> 
> The driver takes in register offset info passed in device initialization.
> It uses hwmod to obtain the base address of the hardware spinlock module.
> Then it reads info from the registers.
> The function hwspinlock_probe() initializes the array of spinlock
> structures,
> each containing a spinlock register address calculated from the base
> address
> and lock offsets.  The device driver file is:
>                 arch/arm/plat-omap/hwspinlock.c
> 
> Here's an API summary:
> int hwspinlock_lock(struct hwspinlock *);
>         Attempt to lock a hardware spinlock.  If it is busy, the function
> will
>         keep trying until it succeeds.  This is a blocking function.
> int hwspinlock_trylock(struct hwspinlock *);
>         Attempt to lock a hardware spinlock.  If it is busy, the function
> will
>         return BUSY.  If it succeeds in locking, the function will return
>         ACQUIRED.  This is a non-blocking function.
> int hwspinlock_unlock(struct hwspinlock *);
>         Unlock a hardware spinlock.
> 
> struct hwspinlock *hwspinlock_request(void);
>         Provides for "dynamic allocation" of a hardware spinlock.  It
> returns
>         the handle to the next available (unallocated) spinlock.  If no
> more
>         locks are available, it returns NULL.
> struct hwspinlock *hwspinlock_request_specific(unsigned int);
>         Provides for "static allocation" of a specific hardware spinlock.
> This
>         allows the system to use a specific spinlock, identified by an ID.
> If
>         the ID is invalid or if the desired lock is already allocated,
> this
>         will return NULL.  Otherwise it returns a spinlock handle.
> int hwspinlock_free(struct hwspinlock *);
>         Frees an allocated hardware spinlock (either reserved or
> unreserved).
> 
> Hari Kanigeri (1):
>   omap:hwspinlocks-ensure the order of registration
> 
> Simon Que (4):
>   omap:hwmod-hwspinlock-enable
>   omap:hwspinlock-define HWSPINLOCK base address
I think you should fold patch 1/5 , 2/5 into patch 3/5. 
At least patch 2/5 o.w git-bisect will break 

>   omap:hwspinlock-added hwspinlock driver
>   omap:hwspinlock-add build support
> 
>  arch/arm/mach-omap2/Makefile                 |    2 +
>  arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
>  arch/arm/mach-omap2/omap_hwmod_44xx_data.c   |    2 +-
>  arch/arm/plat-omap/Makefile                  |    2 +
>  arch/arm/plat-omap/hwspinlock.c              |  334
> ++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
>  arch/arm/plat-omap/include/plat/omap44xx.h   |    5 +
>  7 files changed, 443 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
>  create mode 100644 arch/arm/plat-omap/hwspinlock.c
>  create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h

RE: [PATCH 0/5] omap:hwspinlock support-omap4

[Kanigeri, Hari]

Santosh,


> >
> > Hari Kanigeri (1):
> >   omap:hwspinlocks-ensure the order of registration
> >
> > Simon Que (4):
> >   omap:hwmod-hwspinlock-enable
> >   omap:hwspinlock-define HWSPINLOCK base address
> I think you should fold patch 1/5 , 2/5 into patch 3/5.
> At least patch 2/5 o.w git-bisect will break

Can you please explain why this would break ? 
> 

Thank you,
Best regards,
Hari

Re: [PATCH 0/5] omap:hwspinlock support-omap4

[Cousson, Benoit]

Hi Santosh,

On 7/20/2010 4:12 PM, Kanigeri, Hari wrote:
> Santosh,
>
>
>>>
>>> Hari Kanigeri (1):
>>>    omap:hwspinlocks-ensure the order of registration
>>>
>>> Simon Que (4):
>>>    omap:hwmod-hwspinlock-enable
>>>    omap:hwspinlock-define HWSPINLOCK base address
>> I think you should fold patch 1/5 , 2/5 into patch 3/5.
>> At least patch 2/5 o.w git-bisect will break
>
> Can you please explain why this would break ?

Yep, I'm confused as well...

In fact 3 & 4 should be merged. In patch 3 we introduce 2 news files 
that will not be compiled until the next patch. We'd better modify the 
makefile in the same patch. For the build point of view, the patch  3 
will be a noop.

Regards,
Benoit

RE: [PATCH 0/5] omap:hwspinlock support-omap4

[Shilimkar, Santosh]

> -----Original Message-----
> From: Cousson, Benoit
> Sent: Saturday, July 24, 2010 8:35 PM
> To: Kanigeri, Hari
> Cc: Shilimkar, Santosh; Linux Omap; Tony Lindgren; Que, Simon
> Subject: Re: [PATCH 0/5] omap:hwspinlock support-omap4
> 
> Hi Santosh,
> 
> On 7/20/2010 4:12 PM, Kanigeri, Hari wrote:
> > Santosh,
> >
> >
> >>>
> >>> Hari Kanigeri (1):
> >>>    omap:hwspinlocks-ensure the order of registration
> >>>
> >>> Simon Que (4):
> >>>    omap:hwmod-hwspinlock-enable
> >>>    omap:hwspinlock-define HWSPINLOCK base address
> >> I think you should fold patch 1/5 , 2/5 into patch 3/5.
> >> At least patch 2/5 o.w git-bisect will break
> >
> > Can you please explain why this would break ?
> 
> Yep, I'm confused as well...
> 
> In fact 3 & 4 should be merged. In patch 3 we introduce 2 news files
> that will not be compiled until the next patch. We'd better modify the
> makefile in the same patch. For the build point of view, the patch  3
> will be a noop.
> 
I see your point now. So bisect should be ok but I agree with Benoit suggestion.

Additionally, the base address patch can be avoided because
this information can be retrieved from platform data.
	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	"res->start" is base address
Regards,
Santosh

Re: [PATCH 0/5] omap:hwspinlock support-omap4

[Cousson, Benoit]

On 7/24/2010 5:35 PM, Shilimkar, Santosh wrote:
>> From: Cousson, Benoit
>> Sent: Saturday, July 24, 2010 8:35 PM
>>
>> Hi Santosh,
>>
>> On 7/20/2010 4:12 PM, Kanigeri, Hari wrote:
>>> Santosh,
>>>
>>>
>>>>>
>>>>> Hari Kanigeri (1):
>>>>>     omap:hwspinlocks-ensure the order of registration
>>>>>
>>>>> Simon Que (4):
>>>>>     omap:hwmod-hwspinlock-enable
>>>>>     omap:hwspinlock-define HWSPINLOCK base address
>>>> I think you should fold patch 1/5 , 2/5 into patch 3/5.
>>>> At least patch 2/5 o.w git-bisect will break
>>>
>>> Can you please explain why this would break ?
>>
>> Yep, I'm confused as well...
>>
>> In fact 3&  4 should be merged. In patch 3 we introduce 2 news files
>> that will not be compiled until the next patch. We'd better modify the
>> makefile in the same patch. For the build point of view, the patch  3
>> will be a noop.
>>
> I see your point now. So bisect should be ok but I agree with Benoit suggestion.
>
> Additionally, the base address patch can be avoided because
> this information can be retrieved from platform data.
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	"res->start" is base address

I've just checked, and this now done like that in this latest patch series.
The base address should just be used for the hwmod definition. For the 
moment I'm still using the hex value to populate the hwmod struct, but 
in the near future I will use the defines defined in omap44xx.h.

Regards,
Benoit

Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Cousson, Benoit]

Hi Simon and Hari,

On 7/19/2010 6:50 PM, Kanigeri, Hari wrote:
> From: Simon Que<sque@ti.com>
>
> Created driver for OMAP hardware spinlock.  This driver supports:
> - Reserved spinlocks for internal use
> - Dynamic allocation of unreserved locks
> - Lock, unlock, and trylock functions, with or without disabling irqs/preempt
> - Registered as a platform device driver
>
> The device initialization uses hwmod to configure the devices.
> One device will be created for each IP.  It will pass spinlock register offset
> info to the driver.  The device initialization file is:
>                  arch/arm/mach-omap2/hwspinlocks.c
>
> The driver takes in register offset info passed in device initialization.
> It uses hwmod to obtain the base address of the hardware spinlock module.
> Then it reads info from the registers.  The function hwspinlock_probe()
> initializes the array of spinlock structures, each containing a spinlock
> register address calculated from the base address and lock offsets.
> The device driver file is:
>                  arch/arm/plat-omap/hwspinlock.c
>
> Here's an API summary:
> int hwspinlock_lock(struct hwspinlock *);
>          Attempt to lock a hardware spinlock.  If it is busy, the function will
>          keep trying until it succeeds.  This is a blocking function.
> int hwspinlock_trylock(struct hwspinlock *);
>          Attempt to lock a hardware spinlock.  If it is busy, the function will
>          return BUSY.  If it succeeds in locking, the function will return
>          ACQUIRED.  This is a non-blocking function.
> int hwspinlock_unlock(struct hwspinlock *);
>          Unlock a hardware spinlock.
>
> struct hwspinlock *hwspinlock_request(void);
>          Provides for "dynamic allocation" of a hardware spinlock.  It returns
>          the handle to the next available (unallocated) spinlock.  If no more
>          locks are available, it returns NULL.
> struct hwspinlock *hwspinlock_request_specific(unsigned int);
>          Provides for "static allocation" of a specific hardware spinlock. This
>          allows the system to use a specific spinlock, identified by an ID. If
>          the ID is invalid or if the desired lock is already allocated, this
>          will return NULL.  Otherwise it returns a spinlock handle.
> int hwspinlock_free(struct hwspinlock *);
>          Frees an allocated hardware spinlock (either reserved or unreserved).
>
> Signed-off-by: Simon Que<sque@ti.com>
> Signed-off-by: Hari Kanigeri<h-kanigeri2@ti.com>
> ---
>   arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
>   arch/arm/plat-omap/hwspinlock.c              |  335 ++++++++++++++++++++++++++
>   arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
>   3 files changed, 434 insertions(+), 0 deletions(-)
>   create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
>   create mode 100644 arch/arm/plat-omap/hwspinlock.c
>   create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h
>
> diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach-omap2/hwspinlocks.c
> new file mode 100644
> index 0000000..f0f05e1
> --- /dev/null
> +++ b/arch/arm/mach-omap2/hwspinlocks.c
> @@ -0,0 +1,70 @@
> +/*
> + * OMAP hardware spinlock device initialization
> + *
> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> + *
> + * Contact: Simon Que<sque@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/module.h>
> +#include<linux/interrupt.h>
> +#include<linux/device.h>
> +#include<linux/delay.h>
> +#include<linux/io.h>
> +#include<linux/slab.h>
> +
> +#include<plat/hwspinlock.h>
> +#include<plat/omap_device.h>
> +#include<plat/omap_hwmod.h>
> +
> +/* Spinlock register offsets */
> +#define REVISION_OFFSET                        0x0000
> +#define SYSCONFIG_OFFSET               0x0010
> +#define SYSSTATUS_OFFSET               0x0014
> +#define LOCK_BASE_OFFSET               0x0800
> +#define LOCK_OFFSET(i)                 (LOCK_BASE_OFFSET + 0x4 * (i))

Since these defines are not dependant of the device instance, we should
move them in the driver itself.

> +
> +/* Initialization function */
> +int __init hwspinlocks_init(void)
> +{
> +       int retval = 0;
> +
> +       struct hwspinlock_plat_info *pdata;
> +       struct omap_hwmod *oh;
> +       char *oh_name, *pdev_name;

In this case, both should be const.

> +
> +       oh_name = "spinlock";
> +       oh = omap_hwmod_lookup(oh_name);
> +       if (WARN_ON(oh == NULL))
> +               return -EINVAL;
> +
> +       pdev_name = "hwspinlock";
> +
> +       /* Pass data to device initialization */
> +       pdata = kzalloc(sizeof(struct hwspinlock_plat_info), GFP_KERNEL);
> +       if (WARN_ON(pdata == NULL))
> +               return -ENOMEM;
> +       pdata->sysstatus_offset = SYSSTATUS_OFFSET;
> +       pdata->lock_base_offset = LOCK_BASE_OFFSET;

For the moment sysstatus and lock_base will not change per device, so
there is no need to add them in a platform_data structure. You can just
consider them as constants in the driver code.

> +
> +       omap_device_build(pdev_name, 0, oh, pdata,
> +                       sizeof(struct hwspinlock_plat_info), NULL, 0, false);
> +
> +       return retval;
> +}
> +module_init(hwspinlocks_init);
> diff --git a/arch/arm/plat-omap/hwspinlock.c b/arch/arm/plat-omap/hwspinlock.c
> new file mode 100644
> index 0000000..1883add
> --- /dev/null
> +++ b/arch/arm/plat-omap/hwspinlock.c
> @@ -0,0 +1,335 @@
> +/*
> + * OMAP hardware spinlock driver
> + *
> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> + *
> + * Contact: Simon Que<sque@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
> + *
> + * This driver supports:
> + * - Reserved spinlocks for internal use
> + * - Dynamic allocation of unreserved locks
> + * - Lock, unlock, and trylock functions, with or without disabling irqs/preempt
> + * - Registered as a platform device driver
> + *
> + * The device initialization uses hwmod to configure the devices.  One device
> + * will be created for each IP.  It will pass spinlock register offset info to
> + * the driver.  The device initialization file is:
> + *          arch/arm/mach-omap2/hwspinlocks.c
> + *
> + * The driver takes in register offset info passed in device initialization.
> + * It uses hwmod to obtain the base address of the hardware spinlock module.
> + * Then it reads info from the registers.  The function hwspinlock_probe()
> + * initializes the array of spinlock structures, each containing a spinlock
> + * register address calculated from the base address and lock offsets.
> + *
> + * Here's an API summary:
> + *
> + * int hwspinlock_lock(struct hwspinlock *);
> + *      Attempt to lock a hardware spinlock.  If it is busy, the function will
> + *      keep trying until it succeeds.  This is a blocking function.
> + * int hwspinlock_trylock(struct hwspinlock *);
> + *      Attempt to lock a hardware spinlock.  If it is busy, the function will
> + *      return BUSY.  If it succeeds in locking, the function will return
> + *      ACQUIRED.  This is a non-blocking function
> + * int hwspinlock_unlock(struct hwspinlock *);
> + *      Unlock a hardware spinlock.
> + *
> + * struct hwspinlock *hwspinlock_request(void);
> + *      Provides for "dynamic allocation" of a hardware spinlock.  It returns
> + *      the handle to the next available (unallocated) spinlock.  If no more
> + *      locks are available, it returns NULL.
> + * struct hwspinlock *hwspinlock_request_specific(unsigned int);
> + *      Provides for "static allocation" of a specific hardware spinlock. This
> + *      allows the system to use a specific spinlock, identified by an ID. If
> + *      the ID is invalid or if the desired lock is already allocated, this
> + *      will return NULL.  Otherwise it returns a spinlock handle.
> + * int hwspinlock_free(struct hwspinlock *);
> + *      Frees an allocated hardware spinlock (either reserved or unreserved).
> + */
> +
> +#include<linux/module.h>
> +#include<linux/interrupt.h>
> +#include<linux/device.h>
> +#include<linux/delay.h>
> +#include<linux/io.h>
> +#include<linux/pm_runtime.h>
> +#include<linux/slab.h>
> +#include<linux/spinlock.h>
> +
> +#include<plat/hwspinlock.h>
> +
> +/* Spinlock count code */
> +#define SPINLOCK_32_REGS               1
> +#define SPINLOCK_64_REGS               2
> +#define SPINLOCK_128_REGS              4
> +#define SPINLOCK_256_REGS              8
> +#define SPINLOCK_NUMLOCKS_OFFSET       24
> +
> +/* for managing a hardware spinlock module */
> +struct hwspinlock_state {
> +       bool is_init;                   /* For first-time initialization */
> +       int num_locks;                  /* Total number of locks in system */

That one can be global based on the definition, but in fact it will be 
populated using a per device information, so you can move it to pdata or 
add a disclaimer saying that only one device will be in the system.

> +       spinlock_t local_lock;          /* Local protection */
> +       void __iomem *io_base;          /* Mapped base address */

io_base is not a global information, it is a per device data, so you 
should move it to the pdata struct.

It is true that for the moment only one device will be in the system, so 
in order to simplify the design we might assume that these data are 
"global", but in that case you should add a test and a huge warning in 
the probe function to ensure that only one instance will be ever be 
initialized.

> +};
> +
> +/* Points to the hardware spinlock module */
> +static struct hwspinlock_state hwspinlock_state;
> +static struct hwspinlock_state *hwspinlock_module =&hwspinlock_state;

Just a minor details, but do you really need that extra global variable? 
hwspinlock_state is never use anywhere else, and then you need to 
de-reference a pointer (hwspinlock_module->num_locks) instead of 
accessing directly the struct attribute.

In fact, I even think that you'd better create 4 globals variables, it 
will simplify the code a little bit and improve the readability.

> +
> +/* Spinlock object */
> +struct hwspinlock {
> +       bool is_init;
> +       int id;
> +       void __iomem *lock_reg;
> +       bool is_allocated;
> +       struct platform_device *pdev;
> +};
> +
> +/* Array of spinlocks */
> +static struct hwspinlock *hwspinlocks;
> +
> +/* API functions */
> +
> +/* Busy loop to acquire a spinlock */
> +int hwspinlock_lock(struct hwspinlock *handle)
> +{
> +       int retval;
> +
> +       if (WARN_ON(handle == NULL))
> +               return -EINVAL;
> +
> +       if (WARN_ON(in_irq()))
> +               return -EPERM;
> +
> +       if (pm_runtime_get(&handle->pdev->dev)<  0)
> +               return -ENODEV;
> +
> +       /* Attempt to acquire the lock by reading from it */
> +       do {
> +               retval = readl(handle->lock_reg);
> +       } while (retval == HWSPINLOCK_BUSY);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(hwspinlock_lock);
> +
> +/* Attempt to acquire a spinlock once */
> +int hwspinlock_trylock(struct hwspinlock *handle)
> +{
> +       int retval = 0;
> +
> +       if (WARN_ON(handle == NULL))
> +               return -EINVAL;
> +
> +       if (WARN_ON(in_irq()))
> +               return -EPERM;
> +
> +       if (pm_runtime_get(&handle->pdev->dev)<  0)
> +               return -ENODEV;
> +
> +       /* Attempt to acquire the lock by reading from it */
> +       retval = readl(handle->lock_reg);
> +
> +       if (retval == HWSPINLOCK_BUSY)
> +               pm_runtime_put(&handle->pdev->dev);
> +
> +       return retval;
> +}
> +EXPORT_SYMBOL(hwspinlock_trylock);
> +
> +/* Release a spinlock */
> +int hwspinlock_unlock(struct hwspinlock *handle)
> +{
> +       if (WARN_ON(handle == NULL))
> +               return -EINVAL;
> +
> +       /* Release it by writing 0 to it */
> +       writel(0, handle->lock_reg);
> +
> +       pm_runtime_put(&handle->pdev->dev);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(hwspinlock_unlock);
> +
> +/* Request an unclaimed spinlock */
> +struct hwspinlock *hwspinlock_request(void)
> +{
> +       int i;
> +       bool found = false;
> +       struct hwspinlock *handle = NULL;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> +       /* Search for an unclaimed, unreserved lock */
> +       for (i = 0; i<  hwspinlock_module->num_locks&&  !found; i++) {
> +               if (!hwspinlocks[i].is_allocated) {
> +                       found = true;
> +                       handle =&hwspinlocks[i];
> +               }
> +       }
> +       spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> +
> +       /* Return error if no more locks available */
> +       if (!found)
> +               return NULL;
> +
> +       handle->is_allocated = true;
> +
> +       return handle;
> +}
> +EXPORT_SYMBOL(hwspinlock_request);
> +
> +/* Request an unclaimed spinlock by ID */
> +struct hwspinlock *hwspinlock_request_specific(unsigned int id)
> +{
> +       struct hwspinlock *handle = NULL;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> +
> +       if (WARN_ON(hwspinlocks[id].is_allocated))
> +               goto exit;
> +
> +       handle =&hwspinlocks[id];
> +       handle->is_allocated = true;
> +
> +exit:
> +       spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> +       return handle;
> +}
> +EXPORT_SYMBOL(hwspinlock_request_specific);
> +
> +/* Release a claimed spinlock */
> +int hwspinlock_free(struct hwspinlock *handle)
> +{
> +       if (WARN_ON(handle == NULL))
> +               return -EINVAL;
> +
> +       if (WARN_ON(!handle->is_allocated))
> +               return -ENOMEM;
> +
> +       handle->is_allocated = false;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(hwspinlock_free);
> +
> +/* Probe function */
> +static int __devinit hwspinlock_probe(struct platform_device *pdev)
> +{
> +       struct hwspinlock_plat_info *pdata = pdev->dev.platform_data;
> +       struct resource *res;
> +       void __iomem *io_base;
> +       int id;
> +
> +       void __iomem *sysstatus_reg;
> +
> +       /* Determine number of locks */
> +       sysstatus_reg = ioremap(OMAP44XX_SPINLOCK_BASE +
> +                                       pdata->sysstatus_offset, sizeof(u32));

You should not do that anymore. You will have the base address in a 
couple of lines using:
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       /* Map spinlock module address space */
+       io_base = ioremap(res->start, resource_size(res));
+       hwspinlock_module->io_base = io_base;

You just have to move this code after the platform_get_resource / ioremap.

> +       switch (readl(sysstatus_reg)>>  SPINLOCK_NUMLOCKS_OFFSET) {
> +       case SPINLOCK_32_REGS:
> +               hwspinlock_module->num_locks = 32;
> +               break;
> +       case SPINLOCK_64_REGS:
> +               hwspinlock_module->num_locks = 64;
> +               break;
> +       case SPINLOCK_128_REGS:
> +               hwspinlock_module->num_locks = 128;
> +               break;
> +       case SPINLOCK_256_REGS:
> +               hwspinlock_module->num_locks = 256;
> +               break;
> +       default:
> +               return -EINVAL; /* Invalid spinlock count code */
> +       }
> +       iounmap(sysstatus_reg);
> +
> +       /* Allocate spinlock device objects */
> +       hwspinlocks = kmalloc(sizeof(struct hwspinlock) *
> +                       hwspinlock_module->num_locks, GFP_KERNEL);
> +       if (WARN_ON(hwspinlocks == NULL))
> +               return -ENOMEM;
> +
> +       /* Initialize local lock */
> +       spin_lock_init(&hwspinlock_module->local_lock);
> +
> +       /* Get address info */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       /* Map spinlock module address space */
> +       io_base = ioremap(res->start, resource_size(res));
> +       hwspinlock_module->io_base = io_base;
> +
> +       /* Set up each individual lock handle */
> +       for (id = 0; id<  hwspinlock_module->num_locks; id++) {
> +               hwspinlocks[id].id              = id;
> +               hwspinlocks[id].pdev            = pdev;
> +
> +               hwspinlocks[id].is_init         = true;
> +               hwspinlocks[id].is_allocated    = false;
> +
> +               hwspinlocks[id].lock_reg        = io_base + pdata->
> +                                       lock_base_offset + sizeof(u32) * id;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct platform_driver hwspinlock_driver = {
> +       .probe          = hwspinlock_probe,
> +       .driver         = {
> +               .name   = "hwspinlock",
> +       },
> +};
> +
> +/* Initialization function */
> +static int __init hwspinlock_init(void)
> +{
> +       int retval = 0;
> +
> +       /* Register spinlock driver */
> +       retval = platform_driver_register(&hwspinlock_driver);
> +
> +       return retval;
> +}
> +
> +/* Cleanup function */
> +static void __exit hwspinlock_exit(void)
> +{
> +       int id;
> +
> +       platform_driver_unregister(&hwspinlock_driver);
> +
> +       for (id = 0; id<  hwspinlock_module->num_locks; id++)
> +               hwspinlocks[id].is_init = false;
> +       iounmap(hwspinlock_module->io_base);
> +
> +       /* Free spinlock device objects */
> +       if (hwspinlock_module->is_init)
> +               kfree(hwspinlocks);
> +}
> +
> +module_init(hwspinlock_init);
> +module_exit(hwspinlock_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Hardware spinlock driver");
> +MODULE_AUTHOR("Simon Que");
> +MODULE_AUTHOR("Hari Kanigeri");
> diff --git a/arch/arm/plat-omap/include/plat/hwspinlock.h b/arch/arm/plat-omap/include/plat/hwspinlock.h
> new file mode 100644
> index 0000000..8c69ca5
> --- /dev/null
> +++ b/arch/arm/plat-omap/include/plat/hwspinlock.h
> @@ -0,0 +1,29 @@
> +/* hwspinlock.h */
> +
> +#ifndef HWSPINLOCK_H
> +#define HWSPINLOCK_H
> +
> +#include<linux/platform_device.h>
> +#include<plat/omap44xx.h>

As pointed by Santosh, this include is useless, since the base address 
will be extracted from hwmod.

> +
> +/* Read values from the spinlock register */
> +#define HWSPINLOCK_ACQUIRED 0
> +#define HWSPINLOCK_BUSY 1
> +
> +/* Device data */
> +struct hwspinlock_plat_info {
> +       u32 sysstatus_offset;           /* System status register offset */
> +       u32 lock_base_offset;           /* Offset of spinlock registers */
> +};

You can now get rid of this platform_data struct since both attributes 
are constant.

Regards,
Benoit

> +
> +struct hwspinlock;
> +
> +int hwspinlock_lock(struct hwspinlock *handle);
> +int hwspinlock_trylock(struct hwspinlock *handle);
> +int hwspinlock_unlock(struct hwspinlock *handle);
> +
> +struct hwspinlock *hwspinlock_request(void);
> +struct hwspinlock *hwspinlock_request_specific(unsigned int id);
> +int hwspinlock_free(struct hwspinlock *hwspinlock_ptr);
> +
> +#endif /* HWSPINLOCK_H */
> --
> 1.7.0
>

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Kanigeri, Hari]

Benoit and Santosh,

Thanks for your comments. I will get back after reviewing your comments.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Cousson, Benoit
> Sent: Saturday, July 24, 2010 11:44 AM
> To: Kanigeri, Hari
> Cc: Linux Omap; Tony Lindgren; Shilimkar, Santosh; Que, Simon
> Subject: Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
>
> Hi Simon and Hari,
>
> On 7/19/2010 6:50 PM, Kanigeri, Hari wrote:
> > From: Simon Que<sque@ti.com>
> >
> > Created driver for OMAP hardware spinlock.  This driver supports:
> > - Reserved spinlocks for internal use
> > - Dynamic allocation of unreserved locks
> > - Lock, unlock, and trylock functions, with or without disabling
> irqs/preempt
> > - Registered as a platform device driver
> >
> > The device initialization uses hwmod to configure the devices.
> > One device will be created for each IP.  It will pass spinlock register
> offset
> > info to the driver.  The device initialization file is:
> >                  arch/arm/mach-omap2/hwspinlocks.c
> >
> > The driver takes in register offset info passed in device
> initialization.
> > It uses hwmod to obtain the base address of the hardware spinlock
> module.
> > Then it reads info from the registers.  The function hwspinlock_probe()
> > initializes the array of spinlock structures, each containing a spinlock
> > register address calculated from the base address and lock offsets.
> > The device driver file is:
> >                  arch/arm/plat-omap/hwspinlock.c
> >
> > Here's an API summary:
> > int hwspinlock_lock(struct hwspinlock *);
> >          Attempt to lock a hardware spinlock.  If it is busy, the
> function will
> >          keep trying until it succeeds.  This is a blocking function.
> > int hwspinlock_trylock(struct hwspinlock *);
> >          Attempt to lock a hardware spinlock.  If it is busy, the
> function will
> >          return BUSY.  If it succeeds in locking, the function will
> return
> >          ACQUIRED.  This is a non-blocking function.
> > int hwspinlock_unlock(struct hwspinlock *);
> >          Unlock a hardware spinlock.
> >
> > struct hwspinlock *hwspinlock_request(void);
> >          Provides for "dynamic allocation" of a hardware spinlock.  It
> returns
> >          the handle to the next available (unallocated) spinlock.  If no
> more
> >          locks are available, it returns NULL.
> > struct hwspinlock *hwspinlock_request_specific(unsigned int);
> >          Provides for "static allocation" of a specific hardware
> spinlock. This
> >          allows the system to use a specific spinlock, identified by an
> ID. If
> >          the ID is invalid or if the desired lock is already allocated,
> this
> >          will return NULL.  Otherwise it returns a spinlock handle.
> > int hwspinlock_free(struct hwspinlock *);
> >          Frees an allocated hardware spinlock (either reserved or
> unreserved).
> >
> > Signed-off-by: Simon Que<sque@ti.com>
> > Signed-off-by: Hari Kanigeri<h-kanigeri2@ti.com>
> > ---
> >   arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
> >   arch/arm/plat-omap/hwspinlock.c              |  335
> ++++++++++++++++++++++++++
> >   arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
> >   3 files changed, 434 insertions(+), 0 deletions(-)
> >   create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
> >   create mode 100644 arch/arm/plat-omap/hwspinlock.c
> >   create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h
> >
> > diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach-
> omap2/hwspinlocks.c
> > new file mode 100644
> > index 0000000..f0f05e1
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/hwspinlocks.c
> > @@ -0,0 +1,70 @@
> > +/*
> > + * OMAP hardware spinlock device initialization
> > + *
> > + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> > + *
> > + * Contact: Simon Que<sque@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/module.h>
> > +#include<linux/interrupt.h>
> > +#include<linux/device.h>
> > +#include<linux/delay.h>
> > +#include<linux/io.h>
> > +#include<linux/slab.h>
> > +
> > +#include<plat/hwspinlock.h>
> > +#include<plat/omap_device.h>
> > +#include<plat/omap_hwmod.h>
> > +
> > +/* Spinlock register offsets */
> > +#define REVISION_OFFSET                        0x0000
> > +#define SYSCONFIG_OFFSET               0x0010
> > +#define SYSSTATUS_OFFSET               0x0014
> > +#define LOCK_BASE_OFFSET               0x0800
> > +#define LOCK_OFFSET(i)                 (LOCK_BASE_OFFSET + 0x4 * (i))
>
> Since these defines are not dependant of the device instance, we should
> move them in the driver itself.
>
> > +
> > +/* Initialization function */
> > +int __init hwspinlocks_init(void)
> > +{
> > +       int retval = 0;
> > +
> > +       struct hwspinlock_plat_info *pdata;
> > +       struct omap_hwmod *oh;
> > +       char *oh_name, *pdev_name;
>
> In this case, both should be const.
>
> > +
> > +       oh_name = "spinlock";
> > +       oh = omap_hwmod_lookup(oh_name);
> > +       if (WARN_ON(oh == NULL))
> > +               return -EINVAL;
> > +
> > +       pdev_name = "hwspinlock";
> > +
> > +       /* Pass data to device initialization */
> > +       pdata = kzalloc(sizeof(struct hwspinlock_plat_info),
> GFP_KERNEL);
> > +       if (WARN_ON(pdata == NULL))
> > +               return -ENOMEM;
> > +       pdata->sysstatus_offset = SYSSTATUS_OFFSET;
> > +       pdata->lock_base_offset = LOCK_BASE_OFFSET;
>
> For the moment sysstatus and lock_base will not change per device, so
> there is no need to add them in a platform_data structure. You can just
> consider them as constants in the driver code.
>
> > +
> > +       omap_device_build(pdev_name, 0, oh, pdata,
> > +                       sizeof(struct hwspinlock_plat_info), NULL, 0,
> false);
> > +
> > +       return retval;
> > +}
> > +module_init(hwspinlocks_init);
> > diff --git a/arch/arm/plat-omap/hwspinlock.c b/arch/arm/plat-
> omap/hwspinlock.c
> > new file mode 100644
> > index 0000000..1883add
> > --- /dev/null
> > +++ b/arch/arm/plat-omap/hwspinlock.c
> > @@ -0,0 +1,335 @@
> > +/*
> > + * OMAP hardware spinlock driver
> > + *
> > + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> > + *
> > + * Contact: Simon Que<sque@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
> > + *
> > + * This driver supports:
> > + * - Reserved spinlocks for internal use
> > + * - Dynamic allocation of unreserved locks
> > + * - Lock, unlock, and trylock functions, with or without disabling
> irqs/preempt
> > + * - Registered as a platform device driver
> > + *
> > + * The device initialization uses hwmod to configure the devices.  One
> device
> > + * will be created for each IP.  It will pass spinlock register offset
> info to
> > + * the driver.  The device initialization file is:
> > + *          arch/arm/mach-omap2/hwspinlocks.c
> > + *
> > + * The driver takes in register offset info passed in device
> initialization.
> > + * It uses hwmod to obtain the base address of the hardware spinlock
> module.
> > + * Then it reads info from the registers.  The function
> hwspinlock_probe()
> > + * initializes the array of spinlock structures, each containing a
> spinlock
> > + * register address calculated from the base address and lock offsets.
> > + *
> > + * Here's an API summary:
> > + *
> > + * int hwspinlock_lock(struct hwspinlock *);
> > + *      Attempt to lock a hardware spinlock.  If it is busy, the
> function will
> > + *      keep trying until it succeeds.  This is a blocking function.
> > + * int hwspinlock_trylock(struct hwspinlock *);
> > + *      Attempt to lock a hardware spinlock.  If it is busy, the
> function will
> > + *      return BUSY.  If it succeeds in locking, the function will
> return
> > + *      ACQUIRED.  This is a non-blocking function
> > + * int hwspinlock_unlock(struct hwspinlock *);
> > + *      Unlock a hardware spinlock.
> > + *
> > + * struct hwspinlock *hwspinlock_request(void);
> > + *      Provides for "dynamic allocation" of a hardware spinlock.  It
> returns
> > + *      the handle to the next available (unallocated) spinlock.  If no
> more
> > + *      locks are available, it returns NULL.
> > + * struct hwspinlock *hwspinlock_request_specific(unsigned int);
> > + *      Provides for "static allocation" of a specific hardware
> spinlock. This
> > + *      allows the system to use a specific spinlock, identified by an
> ID. If
> > + *      the ID is invalid or if the desired lock is already allocated,
> this
> > + *      will return NULL.  Otherwise it returns a spinlock handle.
> > + * int hwspinlock_free(struct hwspinlock *);
> > + *      Frees an allocated hardware spinlock (either reserved or
> unreserved).
> > + */
> > +
> > +#include<linux/module.h>
> > +#include<linux/interrupt.h>
> > +#include<linux/device.h>
> > +#include<linux/delay.h>
> > +#include<linux/io.h>
> > +#include<linux/pm_runtime.h>
> > +#include<linux/slab.h>
> > +#include<linux/spinlock.h>
> > +
> > +#include<plat/hwspinlock.h>
> > +
> > +/* Spinlock count code */
> > +#define SPINLOCK_32_REGS               1
> > +#define SPINLOCK_64_REGS               2
> > +#define SPINLOCK_128_REGS              4
> > +#define SPINLOCK_256_REGS              8
> > +#define SPINLOCK_NUMLOCKS_OFFSET       24
> > +
> > +/* for managing a hardware spinlock module */
> > +struct hwspinlock_state {
> > +       bool is_init;                   /* For first-time initialization
> */
> > +       int num_locks;                  /* Total number of locks in
> system */
>
> That one can be global based on the definition, but in fact it will be
> populated using a per device information, so you can move it to pdata or
> add a disclaimer saying that only one device will be in the system.
>
> > +       spinlock_t local_lock;          /* Local protection */
> > +       void __iomem *io_base;          /* Mapped base address */
>
> io_base is not a global information, it is a per device data, so you
> should move it to the pdata struct.
>
> It is true that for the moment only one device will be in the system, so
> in order to simplify the design we might assume that these data are
> "global", but in that case you should add a test and a huge warning in
> the probe function to ensure that only one instance will be ever be
> initialized.
>
> > +};
> > +
> > +/* Points to the hardware spinlock module */
> > +static struct hwspinlock_state hwspinlock_state;
> > +static struct hwspinlock_state *hwspinlock_module =&hwspinlock_state;
>
> Just a minor details, but do you really need that extra global variable?
> hwspinlock_state is never use anywhere else, and then you need to
> de-reference a pointer (hwspinlock_module->num_locks) instead of
> accessing directly the struct attribute.
>
> In fact, I even think that you'd better create 4 globals variables, it
> will simplify the code a little bit and improve the readability.
>
> > +
> > +/* Spinlock object */
> > +struct hwspinlock {
> > +       bool is_init;
> > +       int id;
> > +       void __iomem *lock_reg;
> > +       bool is_allocated;
> > +       struct platform_device *pdev;
> > +};
> > +
> > +/* Array of spinlocks */
> > +static struct hwspinlock *hwspinlocks;
> > +
> > +/* API functions */
> > +
> > +/* Busy loop to acquire a spinlock */
> > +int hwspinlock_lock(struct hwspinlock *handle)
> > +{
> > +       int retval;
> > +
> > +       if (WARN_ON(handle == NULL))
> > +               return -EINVAL;
> > +
> > +       if (WARN_ON(in_irq()))
> > +               return -EPERM;
> > +
> > +       if (pm_runtime_get(&handle->pdev->dev)<  0)
> > +               return -ENODEV;
> > +
> > +       /* Attempt to acquire the lock by reading from it */
> > +       do {
> > +               retval = readl(handle->lock_reg);
> > +       } while (retval == HWSPINLOCK_BUSY);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_lock);
> > +
> > +/* Attempt to acquire a spinlock once */
> > +int hwspinlock_trylock(struct hwspinlock *handle)
> > +{
> > +       int retval = 0;
> > +
> > +       if (WARN_ON(handle == NULL))
> > +               return -EINVAL;
> > +
> > +       if (WARN_ON(in_irq()))
> > +               return -EPERM;
> > +
> > +       if (pm_runtime_get(&handle->pdev->dev)<  0)
> > +               return -ENODEV;
> > +
> > +       /* Attempt to acquire the lock by reading from it */
> > +       retval = readl(handle->lock_reg);
> > +
> > +       if (retval == HWSPINLOCK_BUSY)
> > +               pm_runtime_put(&handle->pdev->dev);
> > +
> > +       return retval;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_trylock);
> > +
> > +/* Release a spinlock */
> > +int hwspinlock_unlock(struct hwspinlock *handle)
> > +{
> > +       if (WARN_ON(handle == NULL))
> > +               return -EINVAL;
> > +
> > +       /* Release it by writing 0 to it */
> > +       writel(0, handle->lock_reg);
> > +
> > +       pm_runtime_put(&handle->pdev->dev);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_unlock);
> > +
> > +/* Request an unclaimed spinlock */
> > +struct hwspinlock *hwspinlock_request(void)
> > +{
> > +       int i;
> > +       bool found = false;
> > +       struct hwspinlock *handle = NULL;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> > +       /* Search for an unclaimed, unreserved lock */
> > +       for (i = 0; i<  hwspinlock_module->num_locks&&  !found; i++) {
> > +               if (!hwspinlocks[i].is_allocated) {
> > +                       found = true;
> > +                       handle =&hwspinlocks[i];
> > +               }
> > +       }
> > +       spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> > +
> > +       /* Return error if no more locks available */
> > +       if (!found)
> > +               return NULL;
> > +
> > +       handle->is_allocated = true;
> > +
> > +       return handle;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_request);
> > +
> > +/* Request an unclaimed spinlock by ID */
> > +struct hwspinlock *hwspinlock_request_specific(unsigned int id)
> > +{
> > +       struct hwspinlock *handle = NULL;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> > +
> > +       if (WARN_ON(hwspinlocks[id].is_allocated))
> > +               goto exit;
> > +
> > +       handle =&hwspinlocks[id];
> > +       handle->is_allocated = true;
> > +
> > +exit:
> > +       spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> > +       return handle;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_request_specific);
> > +
> > +/* Release a claimed spinlock */
> > +int hwspinlock_free(struct hwspinlock *handle)
> > +{
> > +       if (WARN_ON(handle == NULL))
> > +               return -EINVAL;
> > +
> > +       if (WARN_ON(!handle->is_allocated))
> > +               return -ENOMEM;
> > +
> > +       handle->is_allocated = false;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_free);
> > +
> > +/* Probe function */
> > +static int __devinit hwspinlock_probe(struct platform_device *pdev)
> > +{
> > +       struct hwspinlock_plat_info *pdata = pdev->dev.platform_data;
> > +       struct resource *res;
> > +       void __iomem *io_base;
> > +       int id;
> > +
> > +       void __iomem *sysstatus_reg;
> > +
> > +       /* Determine number of locks */
> > +       sysstatus_reg = ioremap(OMAP44XX_SPINLOCK_BASE +
> > +                                       pdata->sysstatus_offset,
> sizeof(u32));
>
> You should not do that anymore. You will have the base address in a
> couple of lines using:
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       /* Map spinlock module address space */
> +       io_base = ioremap(res->start, resource_size(res));
> +       hwspinlock_module->io_base = io_base;
>
> You just have to move this code after the platform_get_resource / ioremap.
>
> > +       switch (readl(sysstatus_reg)>>  SPINLOCK_NUMLOCKS_OFFSET) {
> > +       case SPINLOCK_32_REGS:
> > +               hwspinlock_module->num_locks = 32;
> > +               break;
> > +       case SPINLOCK_64_REGS:
> > +               hwspinlock_module->num_locks = 64;
> > +               break;
> > +       case SPINLOCK_128_REGS:
> > +               hwspinlock_module->num_locks = 128;
> > +               break;
> > +       case SPINLOCK_256_REGS:
> > +               hwspinlock_module->num_locks = 256;
> > +               break;
> > +       default:
> > +               return -EINVAL; /* Invalid spinlock count code */
> > +       }
> > +       iounmap(sysstatus_reg);
> > +
> > +       /* Allocate spinlock device objects */
> > +       hwspinlocks = kmalloc(sizeof(struct hwspinlock) *
> > +                       hwspinlock_module->num_locks, GFP_KERNEL);
> > +       if (WARN_ON(hwspinlocks == NULL))
> > +               return -ENOMEM;
> > +
> > +       /* Initialize local lock */
> > +       spin_lock_init(&hwspinlock_module->local_lock);
> > +
> > +       /* Get address info */
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +       /* Map spinlock module address space */
> > +       io_base = ioremap(res->start, resource_size(res));
> > +       hwspinlock_module->io_base = io_base;
> > +
> > +       /* Set up each individual lock handle */
> > +       for (id = 0; id<  hwspinlock_module->num_locks; id++) {
> > +               hwspinlocks[id].id              = id;
> > +               hwspinlocks[id].pdev            = pdev;
> > +
> > +               hwspinlocks[id].is_init         = true;
> > +               hwspinlocks[id].is_allocated    = false;
> > +
> > +               hwspinlocks[id].lock_reg        = io_base + pdata->
> > +                                       lock_base_offset + sizeof(u32) *
> id;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver hwspinlock_driver = {
> > +       .probe          = hwspinlock_probe,
> > +       .driver         = {
> > +               .name   = "hwspinlock",
> > +       },
> > +};
> > +
> > +/* Initialization function */
> > +static int __init hwspinlock_init(void)
> > +{
> > +       int retval = 0;
> > +
> > +       /* Register spinlock driver */
> > +       retval = platform_driver_register(&hwspinlock_driver);
> > +
> > +       return retval;
> > +}
> > +
> > +/* Cleanup function */
> > +static void __exit hwspinlock_exit(void)
> > +{
> > +       int id;
> > +
> > +       platform_driver_unregister(&hwspinlock_driver);
> > +
> > +       for (id = 0; id<  hwspinlock_module->num_locks; id++)
> > +               hwspinlocks[id].is_init = false;
> > +       iounmap(hwspinlock_module->io_base);
> > +
> > +       /* Free spinlock device objects */
> > +       if (hwspinlock_module->is_init)
> > +               kfree(hwspinlocks);
> > +}
> > +
> > +module_init(hwspinlock_init);
> > +module_exit(hwspinlock_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Hardware spinlock driver");
> > +MODULE_AUTHOR("Simon Que");
> > +MODULE_AUTHOR("Hari Kanigeri");
> > diff --git a/arch/arm/plat-omap/include/plat/hwspinlock.h
> b/arch/arm/plat-omap/include/plat/hwspinlock.h
> > new file mode 100644
> > index 0000000..8c69ca5
> > --- /dev/null
> > +++ b/arch/arm/plat-omap/include/plat/hwspinlock.h
> > @@ -0,0 +1,29 @@
> > +/* hwspinlock.h */
> > +
> > +#ifndef HWSPINLOCK_H
> > +#define HWSPINLOCK_H
> > +
> > +#include<linux/platform_device.h>
> > +#include<plat/omap44xx.h>
>
> As pointed by Santosh, this include is useless, since the base address
> will be extracted from hwmod.
>
> > +
> > +/* Read values from the spinlock register */
> > +#define HWSPINLOCK_ACQUIRED 0
> > +#define HWSPINLOCK_BUSY 1
> > +
> > +/* Device data */
> > +struct hwspinlock_plat_info {
> > +       u32 sysstatus_offset;           /* System status register offset
> */
> > +       u32 lock_base_offset;           /* Offset of spinlock registers
> */
> > +};
>
> You can now get rid of this platform_data struct since both attributes
> are constant.
>
> Regards,
> Benoit
>
> > +
> > +struct hwspinlock;
> > +
> > +int hwspinlock_lock(struct hwspinlock *handle);
> > +int hwspinlock_trylock(struct hwspinlock *handle);
> > +int hwspinlock_unlock(struct hwspinlock *handle);
> > +
> > +struct hwspinlock *hwspinlock_request(void);
> > +struct hwspinlock *hwspinlock_request_specific(unsigned int id);
> > +int hwspinlock_free(struct hwspinlock *hwspinlock_ptr);
> > +
> > +#endif /* HWSPINLOCK_H */
> > --
> > 1.7.0
> >

RE: [PATCH 1/5] omap:hwmod-hwspinlock-enable

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> Sent: Monday, July 19, 2010 10:20 PM
> To: Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> Subject: [PATCH 1/5] omap:hwmod-hwspinlock-enable
> 
> From: Simon Que <sque@ti.com>
> 
> uncomment the hwmod part for hwspinlock
> 
> Signed-off-by: Simon Que <sque@ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

[sp] Does this patch depend upon any other patch waiting in the queue?
     I don't see omap_hwmod_44xx_data.c on the master branch.

~sanjeev

> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c 
> b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index 03bb3db..41dc77d 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -4966,7 +4966,7 @@ static __initdata struct omap_hwmod 
> *omap44xx_hwmods[] = {
>  /*	&omap44xx_smartreflex_iva_hwmod, */
>  /*	&omap44xx_smartreflex_mpu_hwmod, */
>  	/* spinlock class */
> -/*	&omap44xx_spinlock_hwmod, */
> +	&omap44xx_spinlock_hwmod,
>  	/* timer class */
>  	&omap44xx_timer1_hwmod,
>  	&omap44xx_timer2_hwmod,
> -- 
> 1.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> Sent: Monday, July 19, 2010 10:20 PM
> To: Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> Subject: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address
> 
> From: Simon Que <sque@ti.com>
> 
> Add HWSPINLCOK base address information in omap44xx.h
> 
> Signed-off-by: Simon Que <sque@ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
>  arch/arm/plat-omap/include/plat/omap44xx.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/plat/omap44xx.h 
> b/arch/arm/plat-omap/include/plat/omap44xx.h
> index 76832d3..8016508 100644
> --- a/arch/arm/plat-omap/include/plat/omap44xx.h
> +++ b/arch/arm/plat-omap/include/plat/omap44xx.h
> @@ -49,5 +49,10 @@
>  #define OMAP44XX_MAILBOX_BASE		(L4_44XX_BASE + 0xF4000)
>  #define OMAP44XX_HSUSB_OTG_BASE		(L4_44XX_BASE + 0xAB000)
>  
> +#define OMAP4_MMU1_BASE			0x55082000
> +#define OMAP4_MMU2_BASE			0x4A066000

[sp] Are these 2 base addresses related to this patch?

~sanjeev

> +
> +#define OMAP44XX_SPINLOCK_BASE		(L4_44XX_BASE + 0xF6000)
> +
>  #endif /* __ASM_ARCH_OMAP44XX_H */
>  
> -- 
> 1.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/5] omap:hwmod-hwspinlock-enable

[Cousson, Benoit]

Hi Sanjeev,

On 7/27/2010 5:57 PM, Premi, Sanjeev wrote:
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org
>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
>> Sent: Monday, July 19, 2010 10:20 PM
>> To: Linux Omap; Tony Lindgren
>> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
>> Subject: [PATCH 1/5] omap:hwmod-hwspinlock-enable
>>
>> From: Simon Que<sque@ti.com>
>>
>> uncomment the hwmod part for hwspinlock
>>
>> Signed-off-by: Simon Que<sque@ti.com>
>> Signed-off-by: Hari Kanigeri<h-kanigeri2@ti.com>
>> ---
>>   arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> [sp] Does this patch depend upon any other patch waiting in the queue?
>       I don't see omap_hwmod_44xx_data.c on the master branch.

Yes, you need to use Kevin's pm-wip/hwmods-omap4 branch in order to get 
the OMAP4 data.

Regards,
Benoit

>
> ~sanjeev
>
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>> b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>> index 03bb3db..41dc77d 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
>> @@ -4966,7 +4966,7 @@ static __initdata struct omap_hwmod
>> *omap44xx_hwmods[] = {
>>   /*	&omap44xx_smartreflex_iva_hwmod, */
>>   /*	&omap44xx_smartreflex_mpu_hwmod, */
>>   	/* spinlock class */
>> -/*	&omap44xx_spinlock_hwmod, */
>> +	&omap44xx_spinlock_hwmod,
>>   	/* timer class */
>>   	&omap44xx_timer1_hwmod,
>>   	&omap44xx_timer2_hwmod,
>> --
>> 1.7.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Kanigeri, Hari]

Sanjeev,

> > @@ -49,5 +49,10 @@
> >  #define OMAP44XX_MAILBOX_BASE		(L4_44XX_BASE + 0xF4000)
> >  #define OMAP44XX_HSUSB_OTG_BASE		(L4_44XX_BASE + 0xAB000)
> >
> > +#define OMAP4_MMU1_BASE			0x55082000
> > +#define OMAP4_MMU2_BASE			0x4A066000
> 
> [sp] Are these 2 base addresses related to this patch?

Nope. Thanks for finding this. I will update the patch.

> 
> ~sanjeev
> 
> > +
> > +#define OMAP44XX_SPINLOCK_BASE		(L4_44XX_BASE + 0xF6000)
> > +
> >  #endif /* __ASM_ARCH_OMAP44XX_H */
> >
> > --
> > 1.7.0
> >
> >

Best regards,
Hari

RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Kanigeri, Hari 
> Sent: Tuesday, July 27, 2010 10:20 PM
> To: Premi, Sanjeev; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK 
> base address
> 
> Sanjeev,
> 
> > > @@ -49,5 +49,10 @@
> > >  #define OMAP44XX_MAILBOX_BASE		(L4_44XX_BASE + 0xF4000)
> > >  #define OMAP44XX_HSUSB_OTG_BASE		(L4_44XX_BASE + 0xAB000)
> > >
> > > +#define OMAP4_MMU1_BASE			0x55082000
> > > +#define OMAP4_MMU2_BASE			0x4A066000
> > 
> > [sp] Are these 2 base addresses related to this patch?
> 
> Nope. Thanks for finding this. I will update the patch.

[sp] Then additional patch only to add the base address won't make much sense.
     You may want to combine it with appropriately with another one in this
     series.

> 
> > 
> > ~sanjeev
> > 
> > > +
> > > +#define OMAP44XX_SPINLOCK_BASE		(L4_44XX_BASE + 0xF6000)
> > > +
> > >  #endif /* __ASM_ARCH_OMAP44XX_H */
> > >
> > > --
> > > 1.7.0
> > >
> > >
> 
> Best regards,
> Hari
> 

RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Kanigeri, Hari]

Sanjeev,


> -----Original Message-----
> From: Premi, Sanjeev
> Sent: Wednesday, July 28, 2010 9:12 AM
> To: Kanigeri, Hari; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address
> 
> 
> 
> > -----Original Message-----
> > From: Kanigeri, Hari
> > Sent: Tuesday, July 27, 2010 10:20 PM
> > To: Premi, Sanjeev; Linux Omap; Tony Lindgren
> > Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> > Subject: RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK
> > base address
> >
> > Sanjeev,
> >
> > > > @@ -49,5 +49,10 @@
> > > >  #define OMAP44XX_MAILBOX_BASE		(L4_44XX_BASE + 0xF4000)
> > > >  #define OMAP44XX_HSUSB_OTG_BASE		(L4_44XX_BASE + 0xAB000)
> > > >
> > > > +#define OMAP4_MMU1_BASE			0x55082000
> > > > +#define OMAP4_MMU2_BASE			0x4A066000
> > >
> > > [sp] Are these 2 base addresses related to this patch?
> >
> > Nope. Thanks for finding this. I will update the patch.
> 
> [sp] Then additional patch only to add the base address won't make much
> sense.
>      You may want to combine it with appropriately with another one in
> this
>      series.

I think the define to add SPINLOCK base address is independent of adding spinlock driver functionality, and I don't see a reason why it shouldn't be a separate patch.
Example: The driver patches might take time to get upstreamed, but that shouldn't stop this patch that adds the missing Base address.

> 
> >
> > >
> > > ~sanjeev
> > >
> > > > +
> > > > +#define OMAP44XX_SPINLOCK_BASE		(L4_44XX_BASE + 0xF6000)
> > > > +
> > > >  #endif /* __ASM_ARCH_OMAP44XX_H */
> > > >
> > > > --
> > > > 1.7.0
> > > >
> > > >
> >
> > Best regards,
> > Hari
> >

RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Premi, Sanjeev]

 

> -----Original Message-----
> From: Kanigeri, Hari 
> Sent: Wednesday, July 28, 2010 7:53 PM
> To: Premi, Sanjeev; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK 
> base address
> 
[snip]

> > > Sanjeev,
> > >
> > > > > @@ -49,5 +49,10 @@
> > > > >  #define OMAP44XX_MAILBOX_BASE		
> (L4_44XX_BASE + 0xF4000)
> > > > >  #define OMAP44XX_HSUSB_OTG_BASE		
> (L4_44XX_BASE + 0xAB000)
> > > > >
> > > > > +#define OMAP4_MMU1_BASE			0x55082000
> > > > > +#define OMAP4_MMU2_BASE			0x4A066000
> > > >
> > > > [sp] Are these 2 base addresses related to this patch?
> > >
> > > Nope. Thanks for finding this. I will update the patch.
> > 
> > [sp] Then additional patch only to add the base address 
> won't make much
> > sense.
> >      You may want to combine it with appropriately with 
> another one in
> > this
> >      series.
> 
> I think the define to add SPINLOCK base address is 
> independent of adding spinlock driver functionality, and I 
> don't see a reason why it shouldn't be a separate patch.
> Example: The driver patches might take time to get 
> upstreamed, but that shouldn't stop this patch that adds the 
> missing Base address.

[sp] Without driver would this base address be of any use?

> 
> > 
> > >
> > > >
> > > > ~sanjeev
> > > >
> > > > > +
> > > > > +#define OMAP44XX_SPINLOCK_BASE		
> (L4_44XX_BASE + 0xF6000)
> > > > > +
> > > > >  #endif /* __ASM_ARCH_OMAP44XX_H */
> > > > >
> > > > > --
> > > > > 1.7.0
> > > > >
> > > > >
> > >
> > > Best regards,
> > > Hari
> > >
> 

RE: [PATCH 2/5] omap:hwspinlock-define HWSPINLOCK base address

[Kanigeri, Hari]

> 
> > > > Sanjeev,
> > > >
> > > > > > @@ -49,5 +49,10 @@
> > > > > >  #define OMAP44XX_MAILBOX_BASE
> > (L4_44XX_BASE + 0xF4000)
> > > > > >  #define OMAP44XX_HSUSB_OTG_BASE
> > (L4_44XX_BASE + 0xAB000)
> > > > > >
> > > > > > +#define OMAP4_MMU1_BASE			0x55082000
> > > > > > +#define OMAP4_MMU2_BASE			0x4A066000
> > > > >
> > > > > [sp] Are these 2 base addresses related to this patch?
> > > >
> > > > Nope. Thanks for finding this. I will update the patch.
> > >
> > > [sp] Then additional patch only to add the base address
> > won't make much
> > > sense.
> > >      You may want to combine it with appropriately with
> > another one in
> > > this
> > >      series.
> >
> > I think the define to add SPINLOCK base address is
> > independent of adding spinlock driver functionality, and I
> > don't see a reason why it shouldn't be a separate patch.
> > Example: The driver patches might take time to get
> > upstreamed, but that shouldn't stop this patch that adds the
> > missing Base address.
> 
> [sp] Without driver would this base address be of any use?
I could find you examples where there are definitions without being used.
But any case, it is a small change I will follow your suggestion.

> 
> >
> > >
> > > >
> > > > >
> > > > > ~sanjeev
> > > > >
> > > > > > +
> > > > > > +#define OMAP44XX_SPINLOCK_BASE
> > (L4_44XX_BASE + 0xF6000)
> > > > > > +
> > > > > >  #endif /* __ASM_ARCH_OMAP44XX_H */
> > > > > >
> > > > > > --
> > > > > > 1.7.0
> > > > > >
> > > > > >
> > > >
> > > > Best regards,
> > > > Hari
> > > >
> >

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> Sent: Monday, July 19, 2010 10:20 PM
> To: Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> Subject: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
>
> From: Simon Que <sque@ti.com>
>
> Created driver for OMAP hardware spinlock.  This driver supports:
> - Reserved spinlocks for internal use
[sp] How do we reserver the spinlocks? I didn't see any API or parameter
     used to reserve them.
     If this refers to hwspinlock_request_specific(), then it isn't
     really reservation. Reservation is usually is blocks and I would
     expect range input.

     How is this reservation made known to other processors who would
     be attempting to use these spinlocks - in a multi processor scenario
     e.g. OMAP4. Both processors need to be aware of this "reservation".

> - Dynamic allocation of unreserved locks
> - Lock, unlock, and trylock functions, with or without
> disabling irqs/preempt
> - Registered as a platform device driver
>
> The device initialization uses hwmod to configure the devices.
> One device will be created for each IP.  It will pass
> spinlock register offset
> info to the driver.  The device initialization file is:
>               arch/arm/mach-omap2/hwspinlocks.c
>
> The driver takes in register offset info passed in device
> initialization.
> It uses hwmod to obtain the base address of the hardware
> spinlock module.
> Then it reads info from the registers.  The function
> hwspinlock_probe()
> initializes the array of spinlock structures, each containing
> a spinlock
> register address calculated from the base address and lock offsets.
> The device driver file is:
>               arch/arm/plat-omap/hwspinlock.c
>
> Here's an API summary:
> int hwspinlock_lock(struct hwspinlock *);
>       Attempt to lock a hardware spinlock.  If it is busy,
> the function will
>       keep trying until it succeeds.  This is a blocking function.
> int hwspinlock_trylock(struct hwspinlock *);
>       Attempt to lock a hardware spinlock.  If it is busy,
> the function will
>       return BUSY.  If it succeeds in locking, the function
> will return
>       ACQUIRED.  This is a non-blocking function.
> int hwspinlock_unlock(struct hwspinlock *);
>       Unlock a hardware spinlock.
>
> struct hwspinlock *hwspinlock_request(void);
>       Provides for "dynamic allocation" of a hardware
> spinlock.  It returns
>       the handle to the next available (unallocated)
> spinlock.  If no more
>       locks are available, it returns NULL.
> struct hwspinlock *hwspinlock_request_specific(unsigned int);
>       Provides for "static allocation" of a specific hardware
> spinlock. This
>       allows the system to use a specific spinlock,
> identified by an ID. If
>       the ID is invalid or if the desired lock is already
> allocated, this
>       will return NULL.  Otherwise it returns a spinlock handle.
> int hwspinlock_free(struct hwspinlock *);
>       Frees an allocated hardware spinlock (either reserved
> or unreserved).
>
> Signed-off-by: Simon Que <sque@ti.com>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
>  arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
>  arch/arm/plat-omap/hwspinlock.c              |  335
> ++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
>  3 files changed, 434 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
>  create mode 100644 arch/arm/plat-omap/hwspinlock.c
>  create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h
>
> diff --git a/arch/arm/mach-omap2/hwspinlocks.c
> b/arch/arm/mach-omap2/hwspinlocks.c
> new file mode 100644
> index 0000000..f0f05e1
> --- /dev/null
> +++ b/arch/arm/mach-omap2/hwspinlocks.c
> @@ -0,0 +1,70 @@
> +/*
> + * OMAP hardware spinlock device initialization
> + *
> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> + *
> + * Contact: Simon Que <sque@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/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +
> +#include <plat/hwspinlock.h>
> +#include <plat/omap_device.h>
> +#include <plat/omap_hwmod.h>
> +
> +/* Spinlock register offsets */
> +#define REVISION_OFFSET                      0x0000

[sp] Don't see this being used in the patch.

> +#define SYSCONFIG_OFFSET             0x0010

[sp] Ditto.

> +#define SYSSTATUS_OFFSET             0x0014
> +#define LOCK_BASE_OFFSET             0x0800
> +#define LOCK_OFFSET(i)
> (LOCK_BASE_OFFSET + 0x4 * (i))
> +
> +/* Initialization function */
> +int __init hwspinlocks_init(void)
> +{
> +     int retval = 0;
> +
> +     struct hwspinlock_plat_info *pdata;
> +     struct omap_hwmod *oh;
> +     char *oh_name, *pdev_name;
> +
> +     oh_name = "spinlock";
> +     oh = omap_hwmod_lookup(oh_name);
> +     if (WARN_ON(oh == NULL))
> +             return -EINVAL;
> +
> +     pdev_name = "hwspinlock";
> +
> +     /* Pass data to device initialization */
> +     pdata = kzalloc(sizeof(struct hwspinlock_plat_info),
> GFP_KERNEL);
> +     if (WARN_ON(pdata == NULL))
> +             return -ENOMEM;
> +     pdata->sysstatus_offset = SYSSTATUS_OFFSET;
> +     pdata->lock_base_offset = LOCK_BASE_OFFSET;
> +
> +     omap_device_build(pdev_name, 0, oh, pdata,
> +                     sizeof(struct hwspinlock_plat_info),
> NULL, 0, false);
> +
> +     return retval;
> +}
> +module_init(hwspinlocks_init);

[sp] Why do we need a separate file for one function?

> diff --git a/arch/arm/plat-omap/hwspinlock.c
> b/arch/arm/plat-omap/hwspinlock.c
> new file mode 100644
> index 0000000..1883add
> --- /dev/null
> +++ b/arch/arm/plat-omap/hwspinlock.c
> @@ -0,0 +1,335 @@
> +/*
> + * OMAP hardware spinlock driver
> + *
> + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> + *
> + * Contact: Simon Que <sque@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
> + *
> + * This driver supports:
> + * - Reserved spinlocks for internal use
> + * - Dynamic allocation of unreserved locks
> + * - Lock, unlock, and trylock functions, with or without
> disabling irqs/preempt
> + * - Registered as a platform device driver
> + *
> + * The device initialization uses hwmod to configure the
> devices.  One device
> + * will be created for each IP.  It will pass spinlock
> register offset info to
> + * the driver.  The device initialization file is:
> + *          arch/arm/mach-omap2/hwspinlocks.c
> + *
> + * The driver takes in register offset info passed in device
> initialization.
> + * It uses hwmod to obtain the base address of the hardware
> spinlock module.
> + * Then it reads info from the registers.  The function
> hwspinlock_probe()
> + * initializes the array of spinlock structures, each
> containing a spinlock
> + * register address calculated from the base address and
> lock offsets.
> + *
> + * Here's an API summary:
> + *
> + * int hwspinlock_lock(struct hwspinlock *);
> + *      Attempt to lock a hardware spinlock.  If it is busy,
> the function will
> + *      keep trying until it succeeds.  This is a blocking function.
> + * int hwspinlock_trylock(struct hwspinlock *);
> + *      Attempt to lock a hardware spinlock.  If it is busy,
> the function will
> + *      return BUSY.  If it succeeds in locking, the
> function will return
> + *      ACQUIRED.  This is a non-blocking function
> + * int hwspinlock_unlock(struct hwspinlock *);
> + *      Unlock a hardware spinlock.
> + *
> + * struct hwspinlock *hwspinlock_request(void);
> + *      Provides for "dynamic allocation" of a hardware
> spinlock.  It returns
> + *      the handle to the next available (unallocated)
> spinlock.  If no more
> + *      locks are available, it returns NULL.
> + * struct hwspinlock *hwspinlock_request_specific(unsigned int);
> + *      Provides for "static allocation" of a specific
> hardware spinlock. This
> + *      allows the system to use a specific spinlock,
> identified by an ID. If
> + *      the ID is invalid or if the desired lock is already
> allocated, this
> + *      will return NULL.  Otherwise it returns a spinlock handle.
> + * int hwspinlock_free(struct hwspinlock *);
> + *      Frees an allocated hardware spinlock (either
> reserved or unreserved).
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include <plat/hwspinlock.h>
> +
> +/* Spinlock count code */
> +#define SPINLOCK_32_REGS             1
> +#define SPINLOCK_64_REGS             2
> +#define SPINLOCK_128_REGS            4
> +#define SPINLOCK_256_REGS            8
> +#define SPINLOCK_NUMLOCKS_OFFSET     24

[sp] Shouldn't we combine this offset with other offset definitions?

> +
> +/* for managing a hardware spinlock module */
> +struct hwspinlock_state {
> +     bool is_init;                   /* For first-time
> initialization */
> +     int num_locks;                  /* Total number of
> locks in system */
> +     spinlock_t local_lock;          /* Local protection */
> +     void __iomem *io_base;          /* Mapped base address */
> +};

[sp] The name seems to be misleading. The usage suggest that it
     indicates the module state. We could use suffix _mod_state
     or similar.

> +
> +/* Points to the hardware spinlock module */
> +static struct hwspinlock_state hwspinlock_state;
> +static struct hwspinlock_state *hwspinlock_module =
> &hwspinlock_state;
> +
> +/* Spinlock object */
> +struct hwspinlock {
> +     bool is_init;
> +     int id;
> +     void __iomem *lock_reg;
> +     bool is_allocated;
> +     struct platform_device *pdev;

[sp] 2 different bools for "init" and "allocated" could be avoided
     by a "state" field with possible values as xx_RESET, xx_INIT,
     xx_ALLOC, ...

> +};
> +
> +/* Array of spinlocks */
> +static struct hwspinlock *hwspinlocks;

[sp] Do we really want to stress the compilers by making the
     name of structure and variable to be same? This situation
     is avoidable.

> +
> +/* API functions */
> +
> +/* Busy loop to acquire a spinlock */
> +int hwspinlock_lock(struct hwspinlock *handle)
> +{
> +     int retval;
> +
> +     if (WARN_ON(handle == NULL))
> +             return -EINVAL;
> +
> +     if (WARN_ON(in_irq()))
> +             return -EPERM;
> +
> +     if (pm_runtime_get(&handle->pdev->dev) < 0)
> +             return -ENODEV;

[sp] I may be missing discussions on this list; but
     can you confirm if pm_runtime_get()/put() are
     waiting to be merged on the master branch.

> +
> +     /* Attempt to acquire the lock by reading from it */
> +     do {
> +             retval = readl(handle->lock_reg);
> +     } while (retval == HWSPINLOCK_BUSY);

[sp] The description did say that call is blocking but this
     tight-loop could lead to deadlock situations.
     Do we intend it this way?

> +
> +     return 0;
> +}
> +EXPORT_SYMBOL(hwspinlock_lock);
> +
> +/* Attempt to acquire a spinlock once */
> +int hwspinlock_trylock(struct hwspinlock *handle)
> +{
> +     int retval = 0;
> +
> +     if (WARN_ON(handle == NULL))
> +             return -EINVAL;
> +
> +     if (WARN_ON(in_irq()))
> +             return -EPERM;

[sp] Would be good to add a comment - why it is not permitted?

> +
> +     if (pm_runtime_get(&handle->pdev->dev) < 0)
> +             return -ENODEV;
> +
> +     /* Attempt to acquire the lock by reading from it */
> +     retval = readl(handle->lock_reg);
> +
> +     if (retval == HWSPINLOCK_BUSY)
> +             pm_runtime_put(&handle->pdev->dev);
> +
> +     return retval;
> +}
> +EXPORT_SYMBOL(hwspinlock_trylock);
> +
> +/* Release a spinlock */
> +int hwspinlock_unlock(struct hwspinlock *handle)
> +{
> +     if (WARN_ON(handle == NULL))
> +             return -EINVAL;
> +
> +     /* Release it by writing 0 to it */
> +     writel(0, handle->lock_reg);
> +
> +     pm_runtime_put(&handle->pdev->dev);
> +
> +     return 0;
> +}
> +EXPORT_SYMBOL(hwspinlock_unlock);
> +
> +/* Request an unclaimed spinlock */
> +struct hwspinlock *hwspinlock_request(void)
> +{
> +     int i;
> +     bool found = false;
> +     struct hwspinlock *handle = NULL;
> +     unsigned long flags;
> +
> +     spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> +     /* Search for an unclaimed, unreserved lock */
> +     for (i = 0; i < hwspinlock_module->num_locks && !found; i++) {
> +             if (!hwspinlocks[i].is_allocated) {
> +                     found = true;
> +                     handle = &hwspinlocks[i];
> +             }
> +     }
> +     spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> +
> +     /* Return error if no more locks available */
> +     if (!found)
> +             return NULL;
> +
> +     handle->is_allocated = true;
> +
> +     return handle;
> +}
> +EXPORT_SYMBOL(hwspinlock_request);
> +
> +/* Request an unclaimed spinlock by ID */
> +struct hwspinlock *hwspinlock_request_specific(unsigned int id)
> +{
> +     struct hwspinlock *handle = NULL;
> +     unsigned long flags;
> +
> +     spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> +
> +     if (WARN_ON(hwspinlocks[id].is_allocated))
> +             goto exit;
> +
> +     handle = &hwspinlocks[id];
> +     handle->is_allocated = true;
> +
> +exit:
> +     spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> +     return handle;
> +}
> +EXPORT_SYMBOL(hwspinlock_request_specific);
> +
> +/* Release a claimed spinlock */
> +int hwspinlock_free(struct hwspinlock *handle)
> +{
> +     if (WARN_ON(handle == NULL))
> +             return -EINVAL;
> +
> +     if (WARN_ON(!handle->is_allocated))
> +             return -ENOMEM;
> +
> +     handle->is_allocated = false;
> +
> +     return 0;
> +}
> +EXPORT_SYMBOL(hwspinlock_free);
> +
> +/* Probe function */
> +static int __devinit hwspinlock_probe(struct platform_device *pdev)
> +{
> +     struct hwspinlock_plat_info *pdata = pdev->dev.platform_data;
> +     struct resource *res;
> +     void __iomem *io_base;
> +     int id;
> +
> +     void __iomem *sysstatus_reg;
> +
> +     /* Determine number of locks */
> +     sysstatus_reg = ioremap(OMAP44XX_SPINLOCK_BASE +
> +
> pdata->sysstatus_offset, sizeof(u32));

[sp] Is this the only place where sysstatus_reg is used? If so,
     do we really need to pass it via pdata?

> +     switch (readl(sysstatus_reg) >> SPINLOCK_NUMLOCKS_OFFSET) {

[sp] ioremap() is not guaranteed to succeed. Check if return value
     is valid before using it.

> +     case SPINLOCK_32_REGS:
> +             hwspinlock_module->num_locks = 32;
> +             break;
> +     case SPINLOCK_64_REGS:
> +             hwspinlock_module->num_locks = 64;
> +             break;
> +     case SPINLOCK_128_REGS:
> +             hwspinlock_module->num_locks = 128;
> +             break;
> +     case SPINLOCK_256_REGS:
> +             hwspinlock_module->num_locks = 256;
> +             break;
> +     default:
> +             return -EINVAL; /* Invalid spinlock count code */
> +     }
> +     iounmap(sysstatus_reg);
> +
> +     /* Allocate spinlock device objects */
> +     hwspinlocks = kmalloc(sizeof(struct hwspinlock) *
> +                     hwspinlock_module->num_locks, GFP_KERNEL);
> +     if (WARN_ON(hwspinlocks == NULL))
> +             return -ENOMEM;
> +
> +     /* Initialize local lock */
> +     spin_lock_init(&hwspinlock_module->local_lock);
> +
> +     /* Get address info */
> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +     /* Map spinlock module address space */
> +     io_base = ioremap(res->start, resource_size(res));
> +     hwspinlock_module->io_base = io_base;
> +
> +     /* Set up each individual lock handle */
> +     for (id = 0; id < hwspinlock_module->num_locks; id++) {
> +             hwspinlocks[id].id              = id;
> +             hwspinlocks[id].pdev            = pdev;
> +
> +             hwspinlocks[id].is_init         = true;
> +             hwspinlocks[id].is_allocated    = false;
> +
> +             hwspinlocks[id].lock_reg        = io_base + pdata->
> +                                     lock_base_offset +
> sizeof(u32) * id;
> +     }
> +
> +     return 0;
> +}
> +
> +static struct platform_driver hwspinlock_driver = {
> +     .probe          = hwspinlock_probe,
> +     .driver         = {
> +             .name   = "hwspinlock",
> +     },
> +};
> +
> +/* Initialization function */
> +static int __init hwspinlock_init(void)
> +{
> +     int retval = 0;
> +
> +     /* Register spinlock driver */
> +     retval = platform_driver_register(&hwspinlock_driver);
> +
> +     return retval;
> +}
> +
> +/* Cleanup function */
> +static void __exit hwspinlock_exit(void)
> +{
> +     int id;
> +
> +     platform_driver_unregister(&hwspinlock_driver);
> +
> +     for (id = 0; id < hwspinlock_module->num_locks; id++)
> +             hwspinlocks[id].is_init = false;
> +     iounmap(hwspinlock_module->io_base);
> +
> +     /* Free spinlock device objects */
> +     if (hwspinlock_module->is_init)
> +             kfree(hwspinlocks);
> +}
> +
> +module_init(hwspinlock_init);
> +module_exit(hwspinlock_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Hardware spinlock driver");
> +MODULE_AUTHOR("Simon Que");
> +MODULE_AUTHOR("Hari Kanigeri");
> diff --git a/arch/arm/plat-omap/include/plat/hwspinlock.h
> b/arch/arm/plat-omap/include/plat/hwspinlock.h
> new file mode 100644
> index 0000000..8c69ca5
> --- /dev/null
> +++ b/arch/arm/plat-omap/include/plat/hwspinlock.h
> @@ -0,0 +1,29 @@
> +/* hwspinlock.h */
> +
> +#ifndef HWSPINLOCK_H
> +#define HWSPINLOCK_H
> +

[sp] License is missing

> +#include <linux/platform_device.h>
> +#include <plat/omap44xx.h>
> +
> +/* Read values from the spinlock register */
> +#define HWSPINLOCK_ACQUIRED 0
> +#define HWSPINLOCK_BUSY 1

[sp] What is the value after initialization?
     Also, what is the difference between acquired and busy?
     If a spinlock is acquired, won't it be busy? OR vice-versa?

> +
> +/* Device data */
> +struct hwspinlock_plat_info {
> +     u32 sysstatus_offset;           /* System status
> register offset */
> +     u32 lock_base_offset;           /* Offset of spinlock
> registers */
> +};
> +
> +struct hwspinlock;

[sp] Any specific reason for forward declaration?

> +
> +int hwspinlock_lock(struct hwspinlock *handle);
> +int hwspinlock_trylock(struct hwspinlock *handle);
> +int hwspinlock_unlock(struct hwspinlock *handle);
> +
> +struct hwspinlock *hwspinlock_request(void);
> +struct hwspinlock *hwspinlock_request_specific(unsigned int id);
> +int hwspinlock_free(struct hwspinlock *hwspinlock_ptr);
> +
> +#endif /* HWSPINLOCK_H */
> --
> 1.7.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [PATCH 5/5] omap:hwspinlocks-ensure the order of registration

[Premi, Sanjeev]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org 
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> Sent: Monday, July 19, 2010 10:20 PM
> To: Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> Subject: [PATCH 5/5] omap:hwspinlocks-ensure the order of registration
> 
> Ensure that the hwspinlock driver is registered prior to
> I2C driver registration since I2C is dependent on hwspinlock.

[sp] Is there another patch where this dependency is introduced?

> 
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> ---
>  arch/arm/mach-omap2/hwspinlocks.c |    2 +-
>  arch/arm/plat-omap/hwspinlock.c   |    3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/hwspinlocks.c 
> b/arch/arm/mach-omap2/hwspinlocks.c
> index f0f05e1..a9f85b9 100644
> --- a/arch/arm/mach-omap2/hwspinlocks.c
> +++ b/arch/arm/mach-omap2/hwspinlocks.c
> @@ -67,4 +67,4 @@ int __init hwspinlocks_init(void)
>  
>  	return retval;
>  }
> -module_init(hwspinlocks_init);
> +postcore_initcall(hwspinlocks_init);
> diff --git a/arch/arm/plat-omap/hwspinlock.c 
> b/arch/arm/plat-omap/hwspinlock.c
> index 1883add..3742b04 100644
> --- a/arch/arm/plat-omap/hwspinlock.c
> +++ b/arch/arm/plat-omap/hwspinlock.c
> @@ -309,6 +309,7 @@ static int __init hwspinlock_init(void)
>  
>  	return retval;
>  }
> +postcore_initcall(hwspinlock_init);
>  
>  /* Cleanup function */
>  static void __exit hwspinlock_exit(void)
> @@ -325,8 +326,6 @@ static void __exit hwspinlock_exit(void)
>  	if (hwspinlock_module->is_init)
>  		kfree(hwspinlocks);
>  }
> -
> -module_init(hwspinlock_init);
>  module_exit(hwspinlock_exit);
>  
>  MODULE_LICENSE("GPL v2");
> -- 
> 1.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH 5/5] omap:hwspinlocks-ensure the order of registration

[Kanigeri, Hari]

Sanjeev,

Thanks for your comments.

> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> > Sent: Monday, July 19, 2010 10:20 PM
> > To: Linux Omap; Tony Lindgren
> > Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> > Subject: [PATCH 5/5] omap:hwspinlocks-ensure the order of registration
> >
> > Ensure that the hwspinlock driver is registered prior to
> > I2C driver registration since I2C is dependent on hwspinlock.
> 
> [sp] Is there another patch where this dependency is introduced?

Basically, hwspinlock driver should be registered prior to any other modules that uses it. I2C driver is currently using hwspinlock and we noticed an issue if hwspinlock wasn't registered early on.

I don't know if the I2C patch was communicated on mailing list yet. Balaji, can you please share your patch.

> 
> >

Best regards,
Hari

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Marathe, Yogesh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Premi, Sanjeev
> Sent: Wednesday, July 28, 2010 10:28 PM
> To: Kanigeri, Hari; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
>
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> > Sent: Monday, July 19, 2010 10:20 PM
> > To: Linux Omap; Tony Lindgren
> > Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> > Subject: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
> >
> > From: Simon Que <sque@ti.com>
> >
> > Created driver for OMAP hardware spinlock.  This driver supports:
> > - Reserved spinlocks for internal use
> [sp] How do we reserver the spinlocks? I didn't see any API or parameter
>      used to reserve them.
>      If this refers to hwspinlock_request_specific(), then it isn't
>      really reservation. Reservation is usually is blocks and I would
>      expect range input.
>
>      How is this reservation made known to other processors who would
>      be attempting to use these spinlocks - in a multi processor scenario
>      e.g. OMAP4. Both processors need to be aware of this "reservation".
>
> > - Dynamic allocation of unreserved locks
> > - Lock, unlock, and trylock functions, with or without
> > disabling irqs/preempt
> > - Registered as a platform device driver
> >
> > The device initialization uses hwmod to configure the devices.
> > One device will be created for each IP.  It will pass
> > spinlock register offset
> > info to the driver.  The device initialization file is:
> >               arch/arm/mach-omap2/hwspinlocks.c
> >
> > The driver takes in register offset info passed in device
> > initialization.
> > It uses hwmod to obtain the base address of the hardware
> > spinlock module.
> > Then it reads info from the registers.  The function
> > hwspinlock_probe()
> > initializes the array of spinlock structures, each containing
> > a spinlock
> > register address calculated from the base address and lock offsets.
> > The device driver file is:
> >               arch/arm/plat-omap/hwspinlock.c
> >
> > Here's an API summary:
> > int hwspinlock_lock(struct hwspinlock *);
> >       Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> >       keep trying until it succeeds.  This is a blocking function.
> > int hwspinlock_trylock(struct hwspinlock *);
> >       Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> >       return BUSY.  If it succeeds in locking, the function
> > will return
> >       ACQUIRED.  This is a non-blocking function.
> > int hwspinlock_unlock(struct hwspinlock *);
> >       Unlock a hardware spinlock.
> >
> > struct hwspinlock *hwspinlock_request(void);
> >       Provides for "dynamic allocation" of a hardware
> > spinlock.  It returns
> >       the handle to the next available (unallocated)
> > spinlock.  If no more
> >       locks are available, it returns NULL.
> > struct hwspinlock *hwspinlock_request_specific(unsigned int);
> >       Provides for "static allocation" of a specific hardware
> > spinlock. This
> >       allows the system to use a specific spinlock,
> > identified by an ID. If
> >       the ID is invalid or if the desired lock is already
> > allocated, this
> >       will return NULL.  Otherwise it returns a spinlock handle.
> > int hwspinlock_free(struct hwspinlock *);
> >       Frees an allocated hardware spinlock (either reserved
> > or unreserved).
> >
> > Signed-off-by: Simon Que <sque@ti.com>
> > Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> > ---
> >  arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
> >  arch/arm/plat-omap/hwspinlock.c              |  335
> > ++++++++++++++++++++++++++
> >  arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
> >  3 files changed, 434 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
> >  create mode 100644 arch/arm/plat-omap/hwspinlock.c
> >  create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h
> >
> > diff --git a/arch/arm/mach-omap2/hwspinlocks.c
> > b/arch/arm/mach-omap2/hwspinlocks.c
> > new file mode 100644
> > index 0000000..f0f05e1
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/hwspinlocks.c
> > @@ -0,0 +1,70 @@
> > +/*
> > + * OMAP hardware spinlock device initialization
> > + *
> > + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> > + *
> > + * Contact: Simon Que <sque@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/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +
> > +#include <plat/hwspinlock.h>
> > +#include <plat/omap_device.h>
> > +#include <plat/omap_hwmod.h>
> > +
> > +/* Spinlock register offsets */
> > +#define REVISION_OFFSET                      0x0000
>
> [sp] Don't see this being used in the patch.
>
> > +#define SYSCONFIG_OFFSET             0x0010
>
> [sp] Ditto.
>
> > +#define SYSSTATUS_OFFSET             0x0014
> > +#define LOCK_BASE_OFFSET             0x0800
> > +#define LOCK_OFFSET(i)
> > (LOCK_BASE_OFFSET + 0x4 * (i))
> > +
> > +/* Initialization function */
> > +int __init hwspinlocks_init(void)
> > +{
> > +     int retval = 0;
> > +
> > +     struct hwspinlock_plat_info *pdata;
> > +     struct omap_hwmod *oh;
> > +     char *oh_name, *pdev_name;
> > +
> > +     oh_name = "spinlock";
> > +     oh = omap_hwmod_lookup(oh_name);
> > +     if (WARN_ON(oh == NULL))
> > +             return -EINVAL;
> > +
> > +     pdev_name = "hwspinlock";
> > +
> > +     /* Pass data to device initialization */
> > +     pdata = kzalloc(sizeof(struct hwspinlock_plat_info),
> > GFP_KERNEL);
> > +     if (WARN_ON(pdata == NULL))
> > +             return -ENOMEM;
> > +     pdata->sysstatus_offset = SYSSTATUS_OFFSET;
> > +     pdata->lock_base_offset = LOCK_BASE_OFFSET;
> > +
> > +     omap_device_build(pdev_name, 0, oh, pdata,
> > +                     sizeof(struct hwspinlock_plat_info),
> > NULL, 0, false);
> > +
> > +     return retval;
> > +}
> > +module_init(hwspinlocks_init);
>
> [sp] Why do we need a separate file for one function?
>
> > diff --git a/arch/arm/plat-omap/hwspinlock.c
> > b/arch/arm/plat-omap/hwspinlock.c
> > new file mode 100644
> > index 0000000..1883add
> > --- /dev/null
> > +++ b/arch/arm/plat-omap/hwspinlock.c
> > @@ -0,0 +1,335 @@
> > +/*
> > + * OMAP hardware spinlock driver
> > + *
> > + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> > + *
> > + * Contact: Simon Que <sque@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
> > + *
> > + * This driver supports:
> > + * - Reserved spinlocks for internal use
> > + * - Dynamic allocation of unreserved locks
> > + * - Lock, unlock, and trylock functions, with or without
> > disabling irqs/preempt
> > + * - Registered as a platform device driver
> > + *
> > + * The device initialization uses hwmod to configure the
> > devices.  One device
> > + * will be created for each IP.  It will pass spinlock
> > register offset info to
> > + * the driver.  The device initialization file is:
> > + *          arch/arm/mach-omap2/hwspinlocks.c
> > + *
> > + * The driver takes in register offset info passed in device
> > initialization.
> > + * It uses hwmod to obtain the base address of the hardware
> > spinlock module.
> > + * Then it reads info from the registers.  The function
> > hwspinlock_probe()
> > + * initializes the array of spinlock structures, each
> > containing a spinlock
> > + * register address calculated from the base address and
> > lock offsets.
> > + *
> > + * Here's an API summary:
> > + *
> > + * int hwspinlock_lock(struct hwspinlock *);
> > + *      Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> > + *      keep trying until it succeeds.  This is a blocking function.
> > + * int hwspinlock_trylock(struct hwspinlock *);
> > + *      Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> > + *      return BUSY.  If it succeeds in locking, the
> > function will return
> > + *      ACQUIRED.  This is a non-blocking function
> > + * int hwspinlock_unlock(struct hwspinlock *);
> > + *      Unlock a hardware spinlock.
> > + *
> > + * struct hwspinlock *hwspinlock_request(void);
> > + *      Provides for "dynamic allocation" of a hardware
> > spinlock.  It returns
> > + *      the handle to the next available (unallocated)
> > spinlock.  If no more
> > + *      locks are available, it returns NULL.
> > + * struct hwspinlock *hwspinlock_request_specific(unsigned int);
> > + *      Provides for "static allocation" of a specific
> > hardware spinlock. This
> > + *      allows the system to use a specific spinlock,
> > identified by an ID. If
> > + *      the ID is invalid or if the desired lock is already
> > allocated, this
> > + *      will return NULL.  Otherwise it returns a spinlock handle.
> > + * int hwspinlock_free(struct hwspinlock *);
> > + *      Frees an allocated hardware spinlock (either
> > reserved or unreserved).
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include <plat/hwspinlock.h>
> > +
> > +/* Spinlock count code */
> > +#define SPINLOCK_32_REGS             1
> > +#define SPINLOCK_64_REGS             2
> > +#define SPINLOCK_128_REGS            4
> > +#define SPINLOCK_256_REGS            8
> > +#define SPINLOCK_NUMLOCKS_OFFSET     24
>
> [sp] Shouldn't we combine this offset with other offset definitions?
>
> > +
> > +/* for managing a hardware spinlock module */
> > +struct hwspinlock_state {
> > +     bool is_init;                   /* For first-time
> > initialization */
> > +     int num_locks;                  /* Total number of
> > locks in system */
> > +     spinlock_t local_lock;          /* Local protection */
> > +     void __iomem *io_base;          /* Mapped base address */
> > +};
>
> [sp] The name seems to be misleading. The usage suggest that it
>      indicates the module state. We could use suffix _mod_state
>      or similar.
>
> > +
> > +/* Points to the hardware spinlock module */
> > +static struct hwspinlock_state hwspinlock_state;
> > +static struct hwspinlock_state *hwspinlock_module =
> > &hwspinlock_state;
> > +
> > +/* Spinlock object */
> > +struct hwspinlock {
> > +     bool is_init;
> > +     int id;
> > +     void __iomem *lock_reg;
> > +     bool is_allocated;
> > +     struct platform_device *pdev;
>
> [sp] 2 different bools for "init" and "allocated" could be avoided
>      by a "state" field with possible values as xx_RESET, xx_INIT,
>      xx_ALLOC, ...
>
> > +};
> > +
> > +/* Array of spinlocks */
> > +static struct hwspinlock *hwspinlocks;
>
> [sp] Do we really want to stress the compilers by making the
>      name of structure and variable to be same? This situation
>      is avoidable.
>
> > +
> > +/* API functions */
> > +
> > +/* Busy loop to acquire a spinlock */
> > +int hwspinlock_lock(struct hwspinlock *handle)
> > +{
> > +     int retval;
> > +
> > +     if (WARN_ON(handle == NULL))
> > +             return -EINVAL;
> > +
> > +     if (WARN_ON(in_irq()))
> > +             return -EPERM;
> > +
> > +     if (pm_runtime_get(&handle->pdev->dev) < 0)
> > +             return -ENODEV;
>
> [sp] I may be missing discussions on this list; but
>      can you confirm if pm_runtime_get()/put() are
>      waiting to be merged on the master branch.
>
> > +
> > +     /* Attempt to acquire the lock by reading from it */
> > +     do {
> > +             retval = readl(handle->lock_reg);
> > +     } while (retval == HWSPINLOCK_BUSY);
>
> [sp] The description did say that call is blocking but this
>      tight-loop could lead to deadlock situations.
>      Do we intend it this way?
>
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_lock);
> > +
> > +/* Attempt to acquire a spinlock once */
> > +int hwspinlock_trylock(struct hwspinlock *handle)
> > +{
> > +     int retval = 0;
> > +
> > +     if (WARN_ON(handle == NULL))
> > +             return -EINVAL;
> > +
> > +     if (WARN_ON(in_irq()))
> > +             return -EPERM;
>
> [sp] Would be good to add a comment - why it is not permitted?
>
> > +
> > +     if (pm_runtime_get(&handle->pdev->dev) < 0)
> > +             return -ENODEV;
> > +
> > +     /* Attempt to acquire the lock by reading from it */
> > +     retval = readl(handle->lock_reg);
> > +
> > +     if (retval == HWSPINLOCK_BUSY)
> > +             pm_runtime_put(&handle->pdev->dev);
> > +
> > +     return retval;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_trylock);
> > +
> > +/* Release a spinlock */
> > +int hwspinlock_unlock(struct hwspinlock *handle)
> > +{
> > +     if (WARN_ON(handle == NULL))
> > +             return -EINVAL;
> > +
> > +     /* Release it by writing 0 to it */
> > +     writel(0, handle->lock_reg);

[[Yogesh Marathe]] Releasing the spinlock without knowing who owns it is risky. There should be a check for ownership and if authenticated user has called this api  only then it should be released otherwise permission denied error should be returned.

> > +
> > +     pm_runtime_put(&handle->pdev->dev);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_unlock);
> > +
> > +/* Request an unclaimed spinlock */
> > +struct hwspinlock *hwspinlock_request(void)
> > +{
> > +     int i;
> > +     bool found = false;
> > +     struct hwspinlock *handle = NULL;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> > +     /* Search for an unclaimed, unreserved lock */
> > +     for (i = 0; i < hwspinlock_module->num_locks && !found; i++) {
> > +             if (!hwspinlocks[i].is_allocated) {
> > +                     found = true;
> > +                     handle = &hwspinlocks[i];
> > +             }
> > +     }
> > +     spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> > +
> > +     /* Return error if no more locks available */
> > +     if (!found)
> > +             return NULL;
> > +
> > +     handle->is_allocated = true;
> > +
> > +     return handle;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_request);
> > +
> > +/* Request an unclaimed spinlock by ID */
> > +struct hwspinlock *hwspinlock_request_specific(unsigned int id)
> > +{
> > +     struct hwspinlock *handle = NULL;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> > +
> > +     if (WARN_ON(hwspinlocks[id].is_allocated))
> > +             goto exit;
> > +
> > +     handle = &hwspinlocks[id];
> > +     handle->is_allocated = true;
> > +
> > +exit:
> > +     spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> > +     return handle;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_request_specific);
> > +
> > +/* Release a claimed spinlock */
> > +int hwspinlock_free(struct hwspinlock *handle)
> > +{
> > +     if (WARN_ON(handle == NULL))
> > +             return -EINVAL;
> > +
> > +     if (WARN_ON(!handle->is_allocated))
> > +             return -ENOMEM;
> > +
> > +     handle->is_allocated = false;
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_free);
> > +
> > +/* Probe function */
> > +static int __devinit hwspinlock_probe(struct platform_device *pdev)
> > +{
> > +     struct hwspinlock_plat_info *pdata = pdev->dev.platform_data;
> > +     struct resource *res;
> > +     void __iomem *io_base;
> > +     int id;
> > +
> > +     void __iomem *sysstatus_reg;
> > +
> > +     /* Determine number of locks */
> > +     sysstatus_reg = ioremap(OMAP44XX_SPINLOCK_BASE +
> > +
> > pdata->sysstatus_offset, sizeof(u32));
>
> [sp] Is this the only place where sysstatus_reg is used? If so,
>      do we really need to pass it via pdata?
>
> > +     switch (readl(sysstatus_reg) >> SPINLOCK_NUMLOCKS_OFFSET) {
>
> [sp] ioremap() is not guaranteed to succeed. Check if return value
>      is valid before using it.
>
> > +     case SPINLOCK_32_REGS:
> > +             hwspinlock_module->num_locks = 32;
> > +             break;
> > +     case SPINLOCK_64_REGS:
> > +             hwspinlock_module->num_locks = 64;
> > +             break;
> > +     case SPINLOCK_128_REGS:
> > +             hwspinlock_module->num_locks = 128;
> > +             break;
> > +     case SPINLOCK_256_REGS:
> > +             hwspinlock_module->num_locks = 256;
> > +             break;
> > +     default:
> > +             return -EINVAL; /* Invalid spinlock count code */
> > +     }
> > +     iounmap(sysstatus_reg);
> > +
> > +     /* Allocate spinlock device objects */
> > +     hwspinlocks = kmalloc(sizeof(struct hwspinlock) *
> > +                     hwspinlock_module->num_locks, GFP_KERNEL);
> > +     if (WARN_ON(hwspinlocks == NULL))
> > +             return -ENOMEM;
> > +
> > +     /* Initialize local lock */
> > +     spin_lock_init(&hwspinlock_module->local_lock);
> > +
> > +     /* Get address info */
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +     /* Map spinlock module address space */
> > +     io_base = ioremap(res->start, resource_size(res));
> > +     hwspinlock_module->io_base = io_base;
> > +
> > +     /* Set up each individual lock handle */
> > +     for (id = 0; id < hwspinlock_module->num_locks; id++) {
> > +             hwspinlocks[id].id              = id;
> > +             hwspinlocks[id].pdev            = pdev;
> > +
> > +             hwspinlocks[id].is_init         = true;
> > +             hwspinlocks[id].is_allocated    = false;
> > +
> > +             hwspinlocks[id].lock_reg        = io_base + pdata->
> > +                                     lock_base_offset +
> > sizeof(u32) * id;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static struct platform_driver hwspinlock_driver = {
> > +     .probe          = hwspinlock_probe,
> > +     .driver         = {
> > +             .name   = "hwspinlock",
> > +     },
> > +};
> > +
> > +/* Initialization function */
> > +static int __init hwspinlock_init(void)
> > +{
> > +     int retval = 0;
> > +
> > +     /* Register spinlock driver */
> > +     retval = platform_driver_register(&hwspinlock_driver);
> > +
> > +     return retval;
> > +}
> > +
> > +/* Cleanup function */
> > +static void __exit hwspinlock_exit(void)
> > +{
> > +     int id;
> > +
> > +     platform_driver_unregister(&hwspinlock_driver);
> > +
> > +     for (id = 0; id < hwspinlock_module->num_locks; id++)
> > +             hwspinlocks[id].is_init = false;
> > +     iounmap(hwspinlock_module->io_base);
> > +
> > +     /* Free spinlock device objects */
> > +     if (hwspinlock_module->is_init)
> > +             kfree(hwspinlocks);
> > +}
> > +
> > +module_init(hwspinlock_init);
> > +module_exit(hwspinlock_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Hardware spinlock driver");
> > +MODULE_AUTHOR("Simon Que");
> > +MODULE_AUTHOR("Hari Kanigeri");
> > diff --git a/arch/arm/plat-omap/include/plat/hwspinlock.h
> > b/arch/arm/plat-omap/include/plat/hwspinlock.h
> > new file mode 100644
> > index 0000000..8c69ca5
> > --- /dev/null
> > +++ b/arch/arm/plat-omap/include/plat/hwspinlock.h
> > @@ -0,0 +1,29 @@
> > +/* hwspinlock.h */
> > +
> > +#ifndef HWSPINLOCK_H
> > +#define HWSPINLOCK_H
> > +
>
> [sp] License is missing
>
> > +#include <linux/platform_device.h>
> > +#include <plat/omap44xx.h>
> > +
> > +/* Read values from the spinlock register */
> > +#define HWSPINLOCK_ACQUIRED 0
> > +#define HWSPINLOCK_BUSY 1
>
> [sp] What is the value after initialization?
>      Also, what is the difference between acquired and busy?
>      If a spinlock is acquired, won't it be busy? OR vice-versa?
>
> > +
> > +/* Device data */
> > +struct hwspinlock_plat_info {
> > +     u32 sysstatus_offset;           /* System status
> > register offset */
> > +     u32 lock_base_offset;           /* Offset of spinlock
> > registers */
> > +};
> > +
> > +struct hwspinlock;
>
> [sp] Any specific reason for forward declaration?
>
> > +
> > +int hwspinlock_lock(struct hwspinlock *handle);
> > +int hwspinlock_trylock(struct hwspinlock *handle);
> > +int hwspinlock_unlock(struct hwspinlock *handle);
> > +
> > +struct hwspinlock *hwspinlock_request(void);
> > +struct hwspinlock *hwspinlock_request_specific(unsigned int id);
> > +int hwspinlock_free(struct hwspinlock *hwspinlock_ptr);
> > +
> > +#endif /* HWSPINLOCK_H */
> > --
> > 1.7.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Kanigeri, Hari]

Yogesh,

Nice to see your email.

> > > +/* Release a spinlock */
> > > +int hwspinlock_unlock(struct hwspinlock *handle)
> > > +{
> > > +     if (WARN_ON(handle == NULL))
> > > +             return -EINVAL;
> > > +
> > > +     /* Release it by writing 0 to it */
> > > +     writel(0, handle->lock_reg);
> 
> [[Yogesh Marathe]] Releasing the spinlock without knowing who owns it is
> risky. There should be a check for ownership and if authenticated user has
> called this api  only then it should be released otherwise permission
> denied error should be returned.

-- I think if there is another Kernel client that is trying to release that is not owned by it then that Kernel client itself is buggy and needs to be fixed. Please share your thoughts on how we can ensure that we can add some protection.


> 
> > > +
> > > +     pm_runtime_put(&handle->pdev->dev);
> > > +
> > > +     return 0;

Thank you,
Best regards,
Hari

Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Cousson, Benoit]

On 7/29/2010 3:14 PM, Kanigeri, Hari wrote:
> Yogesh,
>
> Nice to see your email.
>
>>>> +/* Release a spinlock */
>>>> +int hwspinlock_unlock(struct hwspinlock *handle)
>>>> +{
>>>> +     if (WARN_ON(handle == NULL))
>>>> +             return -EINVAL;
>>>> +
>>>> +     /* Release it by writing 0 to it */
>>>> +     writel(0, handle->lock_reg);
>>
>> [[Yogesh Marathe]] Releasing the spinlock without knowing who owns it is
>> risky. There should be a check for ownership and if authenticated user has
>> called this api  only then it should be released otherwise permission
>> denied error should be returned.
>
> -- I think if there is another Kernel client that is trying to release that is not owned by it then that Kernel client itself is buggy and needs to be fixed. Please share your thoughts on how we can ensure that we can add some protection.

Yes, I think so as well. The hwspinlock itself was done to be simple. 
The HW is not even checking that the processor that is taking the 
spinlock is the one that will release it.

This is maybe not the ideal and safe way to do that, but I'm not sure 
that we need to add some more complexity than needed for such a basic IP.

BTW, is a Linux kernel spinlock taking care of ownership?

Benoit

>
>>
>>>> +
>>>> +     pm_runtime_put(&handle->pdev->dev);
>>>> +
>>>> +     return 0;
>
> Thank you,
> Best regards,
> Hari

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Kanigeri, Hari]

Sanjeev,

Thanks for your comments.

> -----Original Message-----
> From: Premi, Sanjeev
> Sent: Wednesday, July 28, 2010 11:58 AM
> To: Kanigeri, Hari; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
>
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kanigeri, Hari
> > Sent: Monday, July 19, 2010 10:20 PM
> > To: Linux Omap; Tony Lindgren
> > Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon; Kanigeri, Hari
> > Subject: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
> >
> > From: Simon Que <sque@ti.com>
> >
> > Created driver for OMAP hardware spinlock.  This driver supports:
> > - Reserved spinlocks for internal use
> [sp] How do we reserver the spinlocks? I didn't see any API or parameter
>      used to reserve them.
>      If this refers to hwspinlock_request_specific(), then it isn't
>      really reservation. Reservation is usually is blocks and I would
>      expect range input.

hwspinlock_request_specific() is the one. If you want range you can either call hwspinlock_request_specific() in a loop or we can create a new function to handle it. It shouldn't be difficult to add a new function to reserve range of spinlocks but I would rather wait till we hit that use-case.

>
>      How is this reservation made known to other processors who would
>      be attempting to use these spinlocks - in a multi processor scenario
>      e.g. OMAP4. Both processors need to be aware of this "reservation".
>

This should be handled either:
        - Based on pre-agreement on what spinlock should be used by its counter-part on Co-Processor.
        - Communicate this information using IPC call.

> > - Dynamic allocation of unreserved locks
> > - Lock, unlock, and trylock functions, with or without
> > disabling irqs/preempt
> > - Registered as a platform device driver
> >
> > The device initialization uses hwmod to configure the devices.
> > One device will be created for each IP.  It will pass
> > spinlock register offset
> > info to the driver.  The device initialization file is:
> >             arch/arm/mach-omap2/hwspinlocks.c
> >
> > The driver takes in register offset info passed in device
> > initialization.
> > It uses hwmod to obtain the base address of the hardware
> > spinlock module.
> > Then it reads info from the registers.  The function
> > hwspinlock_probe()
> > initializes the array of spinlock structures, each containing
> > a spinlock
> > register address calculated from the base address and lock offsets.
> > The device driver file is:
> >             arch/arm/plat-omap/hwspinlock.c
> >
> > Here's an API summary:
> > int hwspinlock_lock(struct hwspinlock *);
> >     Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> >     keep trying until it succeeds.  This is a blocking function.
> > int hwspinlock_trylock(struct hwspinlock *);
> >     Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> >     return BUSY.  If it succeeds in locking, the function
> > will return
> >     ACQUIRED.  This is a non-blocking function.
> > int hwspinlock_unlock(struct hwspinlock *);
> >     Unlock a hardware spinlock.
> >
> > struct hwspinlock *hwspinlock_request(void);
> >     Provides for "dynamic allocation" of a hardware
> > spinlock.  It returns
> >     the handle to the next available (unallocated)
> > spinlock.  If no more
> >     locks are available, it returns NULL.
> > struct hwspinlock *hwspinlock_request_specific(unsigned int);
> >     Provides for "static allocation" of a specific hardware
> > spinlock. This
> >     allows the system to use a specific spinlock,
> > identified by an ID. If
> >     the ID is invalid or if the desired lock is already
> > allocated, this
> >     will return NULL.  Otherwise it returns a spinlock handle.
> > int hwspinlock_free(struct hwspinlock *);
> >     Frees an allocated hardware spinlock (either reserved
> > or unreserved).
> >
> > Signed-off-by: Simon Que <sque@ti.com>
> > Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> > ---
> >  arch/arm/mach-omap2/hwspinlocks.c            |   70 ++++++
> >  arch/arm/plat-omap/hwspinlock.c              |  335
> > ++++++++++++++++++++++++++
> >  arch/arm/plat-omap/include/plat/hwspinlock.h |   29 +++
> >  3 files changed, 434 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-omap2/hwspinlocks.c
> >  create mode 100644 arch/arm/plat-omap/hwspinlock.c
> >  create mode 100644 arch/arm/plat-omap/include/plat/hwspinlock.h
> >
> > diff --git a/arch/arm/mach-omap2/hwspinlocks.c
> > b/arch/arm/mach-omap2/hwspinlocks.c
> > new file mode 100644
> > index 0000000..f0f05e1
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/hwspinlocks.c
> > @@ -0,0 +1,70 @@
> > +/*
> > + * OMAP hardware spinlock device initialization
> > + *
> > + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> > + *
> > + * Contact: Simon Que <sque@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/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +
> > +#include <plat/hwspinlock.h>
> > +#include <plat/omap_device.h>
> > +#include <plat/omap_hwmod.h>
> > +
> > +/* Spinlock register offsets */
> > +#define REVISION_OFFSET                    0x0000
>
> [sp] Don't see this being used in the patch.

Thanks for pointing this.

>
> > +#define SYSCONFIG_OFFSET           0x0010
>
> [sp] Ditto.

Thanks again.

>
> > +#define SYSSTATUS_OFFSET           0x0014
> > +#define LOCK_BASE_OFFSET           0x0800
> > +#define LOCK_OFFSET(i)
> > (LOCK_BASE_OFFSET + 0x4 * (i))
> > +
> > +/* Initialization function */
> > +int __init hwspinlocks_init(void)
> > +{
> > +   int retval = 0;
> > +
> > +   struct hwspinlock_plat_info *pdata;
> > +   struct omap_hwmod *oh;
> > +   char *oh_name, *pdev_name;
> > +
> > +   oh_name = "spinlock";
> > +   oh = omap_hwmod_lookup(oh_name);
> > +   if (WARN_ON(oh == NULL))
> > +           return -EINVAL;
> > +
> > +   pdev_name = "hwspinlock";
> > +
> > +   /* Pass data to device initialization */
> > +   pdata = kzalloc(sizeof(struct hwspinlock_plat_info),
> > GFP_KERNEL);
> > +   if (WARN_ON(pdata == NULL))
> > +           return -ENOMEM;
> > +   pdata->sysstatus_offset = SYSSTATUS_OFFSET;
> > +   pdata->lock_base_offset = LOCK_BASE_OFFSET;
> > +
> > +   omap_device_build(pdev_name, 0, oh, pdata,
> > +                   sizeof(struct hwspinlock_plat_info),
> > NULL, 0, false);
> > +
> > +   return retval;
> > +}
> > +module_init(hwspinlocks_init);
>
> [sp] Why do we need a separate file for one function?

I think we need separate files to separate out hardware specific details. This is following the convention of what files need to be in plat-omap and mach-omap2.

>
> > diff --git a/arch/arm/plat-omap/hwspinlock.c
> > b/arch/arm/plat-omap/hwspinlock.c
> > new file mode 100644
> > index 0000000..1883add
> > --- /dev/null
> > +++ b/arch/arm/plat-omap/hwspinlock.c
> > @@ -0,0 +1,335 @@
> > +/*
> > + * OMAP hardware spinlock driver
> > + *
> > + * Copyright (C) 2010 Texas Instruments. All rights reserved.
> > + *
> > + * Contact: Simon Que <sque@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
> > + *
> > + * This driver supports:
> > + * - Reserved spinlocks for internal use
> > + * - Dynamic allocation of unreserved locks
> > + * - Lock, unlock, and trylock functions, with or without
> > disabling irqs/preempt
> > + * - Registered as a platform device driver
> > + *
> > + * The device initialization uses hwmod to configure the
> > devices.  One device
> > + * will be created for each IP.  It will pass spinlock
> > register offset info to
> > + * the driver.  The device initialization file is:
> > + *          arch/arm/mach-omap2/hwspinlocks.c
> > + *
> > + * The driver takes in register offset info passed in device
> > initialization.
> > + * It uses hwmod to obtain the base address of the hardware
> > spinlock module.
> > + * Then it reads info from the registers.  The function
> > hwspinlock_probe()
> > + * initializes the array of spinlock structures, each
> > containing a spinlock
> > + * register address calculated from the base address and
> > lock offsets.
> > + *
> > + * Here's an API summary:
> > + *
> > + * int hwspinlock_lock(struct hwspinlock *);
> > + *      Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> > + *      keep trying until it succeeds.  This is a blocking function.
> > + * int hwspinlock_trylock(struct hwspinlock *);
> > + *      Attempt to lock a hardware spinlock.  If it is busy,
> > the function will
> > + *      return BUSY.  If it succeeds in locking, the
> > function will return
> > + *      ACQUIRED.  This is a non-blocking function
> > + * int hwspinlock_unlock(struct hwspinlock *);
> > + *      Unlock a hardware spinlock.
> > + *
> > + * struct hwspinlock *hwspinlock_request(void);
> > + *      Provides for "dynamic allocation" of a hardware
> > spinlock.  It returns
> > + *      the handle to the next available (unallocated)
> > spinlock.  If no more
> > + *      locks are available, it returns NULL.
> > + * struct hwspinlock *hwspinlock_request_specific(unsigned int);
> > + *      Provides for "static allocation" of a specific
> > hardware spinlock. This
> > + *      allows the system to use a specific spinlock,
> > identified by an ID. If
> > + *      the ID is invalid or if the desired lock is already
> > allocated, this
> > + *      will return NULL.  Otherwise it returns a spinlock handle.
> > + * int hwspinlock_free(struct hwspinlock *);
> > + *      Frees an allocated hardware spinlock (either
> > reserved or unreserved).
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include <plat/hwspinlock.h>
> > +
> > +/* Spinlock count code */
> > +#define SPINLOCK_32_REGS           1
> > +#define SPINLOCK_64_REGS           2
> > +#define SPINLOCK_128_REGS          4
> > +#define SPINLOCK_256_REGS          8
> > +#define SPINLOCK_NUMLOCKS_OFFSET   24
>
> [sp] Shouldn't we combine this offset with other offset definitions?
Sure. Let me check.

>
> > +
> > +/* for managing a hardware spinlock module */
> > +struct hwspinlock_state {
> > +   bool is_init;                   /* For first-time
> > initialization */
> > +   int num_locks;                  /* Total number of
> > locks in system */
> > +   spinlock_t local_lock;          /* Local protection */
> > +   void __iomem *io_base;          /* Mapped base address */
> > +};
>
> [sp] The name seems to be misleading. The usage suggest that it
>      indicates the module state. We could use suffix _mod_state
>      or similar.

Sure.

>
> > +
> > +/* Points to the hardware spinlock module */
> > +static struct hwspinlock_state hwspinlock_state;
> > +static struct hwspinlock_state *hwspinlock_module =
> > &hwspinlock_state;
> > +
> > +/* Spinlock object */
> > +struct hwspinlock {
> > +   bool is_init;
> > +   int id;
> > +   void __iomem *lock_reg;
> > +   bool is_allocated;
> > +   struct platform_device *pdev;
>
> [sp] 2 different bools for "init" and "allocated" could be avoided
>      by a "state" field with possible values as xx_RESET, xx_INIT,
>      xx_ALLOC, ...

Let me check. But I don't think creating multiple definitions to meet all permutations is good to save 1 byte of memory.

>
> > +};
> > +
> > +/* Array of spinlocks */
> > +static struct hwspinlock *hwspinlocks;
>
> [sp] Do we really want to stress the compilers by making the
>      name of structure and variable to be same? This situation
>      is avoidable.
>
> > +
> > +/* API functions */
> > +
> > +/* Busy loop to acquire a spinlock */
> > +int hwspinlock_lock(struct hwspinlock *handle)
> > +{
> > +   int retval;
> > +
> > +   if (WARN_ON(handle == NULL))
> > +           return -EINVAL;
> > +
> > +   if (WARN_ON(in_irq()))
> > +           return -EPERM;
> > +
> > +   if (pm_runtime_get(&handle->pdev->dev) < 0)
> > +           return -ENODEV;
>
> [sp] I may be missing discussions on this list; but
>      can you confirm if pm_runtime_get()/put() are
>      waiting to be merged on the master branch.

I think this question is for Benoit or Kevin. But as far as I know all the Drivers are migrating to use PM run time APIs.


>
> > +
> > +   /* Attempt to acquire the lock by reading from it */
> > +   do {
> > +           retval = readl(handle->lock_reg);
> > +   } while (retval == HWSPINLOCK_BUSY);
>
> [sp] The description did say that call is blocking but this
>      tight-loop could lead to deadlock situations.
>      Do we intend it this way?

Probably we should add some time out then.

>
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_lock);
> > +
> > +/* Attempt to acquire a spinlock once */
> > +int hwspinlock_trylock(struct hwspinlock *handle)
> > +{
> > +   int retval = 0;
> > +
> > +   if (WARN_ON(handle == NULL))
> > +           return -EINVAL;
> > +
> > +   if (WARN_ON(in_irq()))
> > +           return -EPERM;
>
> [sp] Would be good to add a comment - why it is not permitted?

Sure.

>
> > +
> > +   if (pm_runtime_get(&handle->pdev->dev) < 0)
> > +           return -ENODEV;
> > +
> > +   /* Attempt to acquire the lock by reading from it */
> > +   retval = readl(handle->lock_reg);
> > +
> > +   if (retval == HWSPINLOCK_BUSY)
> > +           pm_runtime_put(&handle->pdev->dev);
> > +
> > +   return retval;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_trylock);
> > +
> > +/* Release a spinlock */
> > +int hwspinlock_unlock(struct hwspinlock *handle)
> > +{
> > +   if (WARN_ON(handle == NULL))
> > +           return -EINVAL;
> > +
> > +   /* Release it by writing 0 to it */
> > +   writel(0, handle->lock_reg);
> > +
> > +   pm_runtime_put(&handle->pdev->dev);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_unlock);
> > +
> > +/* Request an unclaimed spinlock */
> > +struct hwspinlock *hwspinlock_request(void)
> > +{
> > +   int i;
> > +   bool found = false;
> > +   struct hwspinlock *handle = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> > +   /* Search for an unclaimed, unreserved lock */
> > +   for (i = 0; i < hwspinlock_module->num_locks && !found; i++) {
> > +           if (!hwspinlocks[i].is_allocated) {
> > +                   found = true;
> > +                   handle = &hwspinlocks[i];
> > +           }
> > +   }
> > +   spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> > +
> > +   /* Return error if no more locks available */
> > +   if (!found)
> > +           return NULL;
> > +
> > +   handle->is_allocated = true;
> > +
> > +   return handle;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_request);
> > +
> > +/* Request an unclaimed spinlock by ID */
> > +struct hwspinlock *hwspinlock_request_specific(unsigned int id)
> > +{
> > +   struct hwspinlock *handle = NULL;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(&hwspinlock_module->local_lock, flags);
> > +
> > +   if (WARN_ON(hwspinlocks[id].is_allocated))
> > +           goto exit;
> > +
> > +   handle = &hwspinlocks[id];
> > +   handle->is_allocated = true;
> > +
> > +exit:
> > +   spin_unlock_irqrestore(&hwspinlock_module->local_lock, flags);
> > +   return handle;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_request_specific);
> > +
> > +/* Release a claimed spinlock */
> > +int hwspinlock_free(struct hwspinlock *handle)
> > +{
> > +   if (WARN_ON(handle == NULL))
> > +           return -EINVAL;
> > +
> > +   if (WARN_ON(!handle->is_allocated))
> > +           return -ENOMEM;
> > +
> > +   handle->is_allocated = false;
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(hwspinlock_free);
> > +
> > +/* Probe function */
> > +static int __devinit hwspinlock_probe(struct platform_device *pdev)
> > +{
> > +   struct hwspinlock_plat_info *pdata = pdev->dev.platform_data;
> > +   struct resource *res;
> > +   void __iomem *io_base;
> > +   int id;
> > +
> > +   void __iomem *sysstatus_reg;
> > +
> > +   /* Determine number of locks */
> > +   sysstatus_reg = ioremap(OMAP44XX_SPINLOCK_BASE +
> > +
> > pdata->sysstatus_offset, sizeof(u32));
>
> [sp] Is this the only place where sysstatus_reg is used? If so,
>      do we really need to pass it via pdata?
>
> > +   switch (readl(sysstatus_reg) >> SPINLOCK_NUMLOCKS_OFFSET) {
>
> [sp] ioremap() is not guaranteed to succeed. Check if return value
>      is valid before using it.

Sure.

>
> > +   case SPINLOCK_32_REGS:
> > +           hwspinlock_module->num_locks = 32;
> > +           break;
> > +   case SPINLOCK_64_REGS:
> > +           hwspinlock_module->num_locks = 64;
> > +           break;
> > +   case SPINLOCK_128_REGS:
> > +           hwspinlock_module->num_locks = 128;
> > +           break;
> > +   case SPINLOCK_256_REGS:
> > +           hwspinlock_module->num_locks = 256;
> > +           break;
> > +   default:
> > +           return -EINVAL; /* Invalid spinlock count code */
> > +   }
> > +   iounmap(sysstatus_reg);
> > +
> > +   /* Allocate spinlock device objects */
> > +   hwspinlocks = kmalloc(sizeof(struct hwspinlock) *
> > +                   hwspinlock_module->num_locks, GFP_KERNEL);
> > +   if (WARN_ON(hwspinlocks == NULL))
> > +           return -ENOMEM;
> > +
> > +   /* Initialize local lock */
> > +   spin_lock_init(&hwspinlock_module->local_lock);
> > +
> > +   /* Get address info */
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > +   /* Map spinlock module address space */
> > +   io_base = ioremap(res->start, resource_size(res));
> > +   hwspinlock_module->io_base = io_base;
> > +
> > +   /* Set up each individual lock handle */
> > +   for (id = 0; id < hwspinlock_module->num_locks; id++) {
> > +           hwspinlocks[id].id              = id;
> > +           hwspinlocks[id].pdev            = pdev;
> > +
> > +           hwspinlocks[id].is_init         = true;
> > +           hwspinlocks[id].is_allocated    = false;
> > +
> > +           hwspinlocks[id].lock_reg        = io_base + pdata->
> > +                                   lock_base_offset +
> > sizeof(u32) * id;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static struct platform_driver hwspinlock_driver = {
> > +   .probe          = hwspinlock_probe,
> > +   .driver         = {
> > +           .name   = "hwspinlock",
> > +   },
> > +};
> > +
> > +/* Initialization function */
> > +static int __init hwspinlock_init(void)
> > +{
> > +   int retval = 0;
> > +
> > +   /* Register spinlock driver */
> > +   retval = platform_driver_register(&hwspinlock_driver);
> > +
> > +   return retval;
> > +}
> > +
> > +/* Cleanup function */
> > +static void __exit hwspinlock_exit(void)
> > +{
> > +   int id;
> > +
> > +   platform_driver_unregister(&hwspinlock_driver);
> > +
> > +   for (id = 0; id < hwspinlock_module->num_locks; id++)
> > +           hwspinlocks[id].is_init = false;
> > +   iounmap(hwspinlock_module->io_base);
> > +
> > +   /* Free spinlock device objects */
> > +   if (hwspinlock_module->is_init)
> > +           kfree(hwspinlocks);
> > +}
> > +
> > +module_init(hwspinlock_init);
> > +module_exit(hwspinlock_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Hardware spinlock driver");
> > +MODULE_AUTHOR("Simon Que");
> > +MODULE_AUTHOR("Hari Kanigeri");
> > diff --git a/arch/arm/plat-omap/include/plat/hwspinlock.h
> > b/arch/arm/plat-omap/include/plat/hwspinlock.h
> > new file mode 100644
> > index 0000000..8c69ca5
> > --- /dev/null
> > +++ b/arch/arm/plat-omap/include/plat/hwspinlock.h
> > @@ -0,0 +1,29 @@
> > +/* hwspinlock.h */
> > +
> > +#ifndef HWSPINLOCK_H
> > +#define HWSPINLOCK_H
> > +
>
> [sp] License is missing

Thanks for pointing this out.

>
> > +#include <linux/platform_device.h>
> > +#include <plat/omap44xx.h>
> > +
> > +/* Read values from the spinlock register */
> > +#define HWSPINLOCK_ACQUIRED 0
> > +#define HWSPINLOCK_BUSY 1
>
> [sp] What is the value after initialization?
>      Also, what is the difference between acquired and busy?
>      If a spinlock is acquired, won't it be busy? OR vice-versa?

Acquired is it is reserved to be used by a Client. Busy is spinlock is taken by remote processor.

>
> > +
> > +/* Device data */
> > +struct hwspinlock_plat_info {
> > +   u32 sysstatus_offset;           /* System status
> > register offset */
> > +   u32 lock_base_offset;           /* Offset of spinlock
> > registers */
> > +};
> > +
> > +struct hwspinlock;
>
> [sp] Any specific reason for forward declaration?

Don't know. Need to check with Simon as why he added it.

>
> > +
> > +int hwspinlock_lock(struct hwspinlock *handle);
> > +int hwspinlock_trylock(struct hwspinlock *handle);
> > +int hwspinlock_unlock(struct hwspinlock *handle);
> > +
> > +struct hwspinlock *hwspinlock_request(void);
> > +struct hwspinlock *hwspinlock_request_specific(unsigned int id);
> > +int hwspinlock_free(struct hwspinlock *hwspinlock_ptr);
> > +
> > +#endif /* HWSPINLOCK_H */
> > --
> > 1.7.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >

Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Nishanth Menon]

Kanigeri, Hari had written, on 07/19/2010 11:50 AM, the following:
> From: Simon Que <sque@ti.com>
> 
> Created driver for OMAP hardware spinlock.  This driver supports:
> - Reserved spinlocks for internal use
> - Dynamic allocation of unreserved locks
> - Lock, unlock, and trylock functions, with or without disabling irqs/preempt
> - Registered as a platform device driver
> 
> The device initialization uses hwmod to configure the devices.
> One device will be created for each IP.  It will pass spinlock register offset
> info to the driver.  The device initialization file is:
>                 arch/arm/mach-omap2/hwspinlocks.c
> 
> The driver takes in register offset info passed in device initialization.
> It uses hwmod to obtain the base address of the hardware spinlock module.
> Then it reads info from the registers.  The function hwspinlock_probe()
> initializes the array of spinlock structures, each containing a spinlock
> register address calculated from the base address and lock offsets.
> The device driver file is:
>                 arch/arm/plat-omap/hwspinlock.c

just a curious question:

Is there no h/w spinlock implementation for other architectures in 
linux? I mean the concept does not seem unique for a heterogenous 
processor environments now a days.. if it does exist, maybe we have two 
options:
* extend standard spinlock architecture to handle h/w spinlocks as well
* establish a new framework for h/w spinlocks..

[...]

-- 
Regards,
Nishanth Menon

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Marathe, Yogesh]

Hari,

> -----Original Message-----
> From: Kanigeri, Hari
> Sent: Thursday, July 29, 2010 6:44 PM
> To: Marathe, Yogesh; Premi, Sanjeev; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
> 
> Yogesh,
> 
> Nice to see your email.
> 
> > > > +/* Release a spinlock */
> > > > +int hwspinlock_unlock(struct hwspinlock *handle)
> > > > +{
> > > > +     if (WARN_ON(handle == NULL))
> > > > +             return -EINVAL;
> > > > +
> > > > +     /* Release it by writing 0 to it */
> > > > +     writel(0, handle->lock_reg);
> >
> > [[Yogesh Marathe]] Releasing the spinlock without knowing who owns it is
> > risky. There should be a check for ownership and if authenticated user has
> > called this api  only then it should be released otherwise permission
> > denied error should be returned.
> 
> -- I think if there is another Kernel client that is trying to release that is not owned by
> it then that Kernel client itself is buggy and needs to be fixed. Please share your
> thoughts on how we can ensure that we can add some protection.

[Yogesh Marathe] I agree the client has to be fixed but at least the client should get some error message instead of crashing someone else. I think if hwspinlock_request() and hwspinlock_request_specific() can keep the owner info (say thread id) in some module state structure it is possible to verify against it before acquiring and releasing the lock. 
 
> 
> 
> >
> > > > +
> > > > +     pm_runtime_put(&handle->pdev->dev);
> > > > +
> > > > +     return 0;
> 
> Thank you,
> Best regards,
> Hari

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Kanigeri, Hari]

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Marathe, Yogesh
> Sent: Friday, July 30, 2010 1:53 AM
> To: Kanigeri, Hari; Premi, Sanjeev; Linux Omap; Tony Lindgren
> Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> Subject: RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
> 
> Hari,
> 
> > -----Original Message-----
> > From: Kanigeri, Hari
> > Sent: Thursday, July 29, 2010 6:44 PM
> > To: Marathe, Yogesh; Premi, Sanjeev; Linux Omap; Tony Lindgren
> > Cc: Shilimkar, Santosh; Cousson, Benoit; Que, Simon
> > Subject: RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
> >
> > Yogesh,
> >
> > Nice to see your email.
> >
> > > > > +/* Release a spinlock */
> > > > > +int hwspinlock_unlock(struct hwspinlock *handle)
> > > > > +{
> > > > > +     if (WARN_ON(handle == NULL))
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     /* Release it by writing 0 to it */
> > > > > +     writel(0, handle->lock_reg);
> > >
> > > [[Yogesh Marathe]] Releasing the spinlock without knowing who owns it
> is
> > > risky. There should be a check for ownership and if authenticated user
> has
> > > called this api  only then it should be released otherwise permission
> > > denied error should be returned.
> >
> > -- I think if there is another Kernel client that is trying to release
> that is not owned by
> > it then that Kernel client itself is buggy and needs to be fixed. Please
> share your
> > thoughts on how we can ensure that we can add some protection.
> 
> [Yogesh Marathe] I agree the client has to be fixed but at least the
> client should get some error message instead of crashing someone else. I
> think if hwspinlock_request() and hwspinlock_request_specific() can keep
> the owner info (say thread id) in some module state structure it is
> possible to verify against it before acquiring and releasing the lock

-- I agree with Benoit's point. We don't have to complicate this basic IP block by adding additional checks. 
I2C Kernel module is one of the client and I am not sure if you can rely on thread id for ownership. 

> 
> >
> >
> > >
> > > > > +
> > > > > +     pm_runtime_put(&handle->pdev->dev);
> > > > > +
> > > > > +     return 0;
> >
> > Thank you,
> > Best regards,
> > Hari

RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver

[Que, Simon]

Nishanth,

> just a curious question:
> 
> Is there no h/w spinlock implementation for other architectures in
> linux? I mean the concept does not seem unique for a heterogenous
> processor environments now a days.. if it does exist, maybe we have two
> options:
> * extend standard spinlock architecture to handle h/w spinlocks as well
> * establish a new framework for h/w spinlocks..
> 

Thank you for bringing that to my attention.  I did a quick search and haven't found any hwspinlock module elsewhere.  But should there be hwspinlocks over multiple architectures in the future, we can definitely consider something like the two solutions that you suggested, so that they can be used from platform-independent kernel code.  However, right now we can develop OMAP hwspinlock independently of that.

Thanks,
Simon

linux hardware spinlock framework for heterogeneous processor SOCs?

[Nishanth Menon]

Changing subject, the original thread is at [1], looping in lkml for a 
wider audience for advice.

Que, Simon had written, on 08/11/2010 05:03 PM, the following:
>> just a curious question:
>>
>> Is there no h/w spinlock implementation for other architectures in
>> linux? I mean the concept does not seem unique for a heterogenous
>> processor environments now a days.. if it does exist, maybe we have two
>> options:
>> * extend standard spinlock architecture to handle h/w spinlocks as well
>> * establish a new framework for h/w spinlocks..
>>
> 
> Thank you for bringing that to my attention.  I did a quick search and
 > haven't found any hwspinlock module elsewhere.  But should there be
 > hwspinlocks over multiple architectures in the future, we can definitely
 > consider something like the two solutions that you suggested, so that
 > they can be used from platform-independent kernel code.  However, right
 > now we can develop OMAP hwspinlock independently of that.

I did glance through the kernel commit log, and nothing remotely seems 
to match up at least in my opinion (closest seemed to be [2] 
drivers/gpu/drm/ - but it did not seem relevant here). If this concept 
is unique to OMAP family alone, then I guess it makes sense to introduce 
SOC specific API library for usage. hoping for some suggestions to 
enlighten us.

Ref:
[1] http://marc.info/?t=127955774200003&r=1&w=2
[2] http://dri.sourceforge.net/doc/hardware_locking_low_level.html

-- 
Regards,
Nishanth Menon