* Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-19 16:50 ` [PATCH 3/5] omap:hwspinlock-added hwspinlock driver Hari Kanigeri
@ 2010-07-24 16:43 ` Cousson, Benoit
2010-07-26 20:39 ` Kanigeri, Hari
2010-07-28 16:58 ` Premi, Sanjeev
2010-07-29 14:05 ` Nishanth Menon
2 siblings, 1 reply; 33+ messages in thread
From: Cousson, Benoit @ 2010-07-24 16:43 UTC (permalink / raw)
To: Kanigeri, Hari; +Cc: Linux Omap, Tony Lindgren, Shilimkar, Santosh, Que, Simon
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-24 16:43 ` Cousson, Benoit
@ 2010-07-26 20:39 ` Kanigeri, Hari
0 siblings, 0 replies; 33+ messages in thread
From: Kanigeri, Hari @ 2010-07-26 20:39 UTC (permalink / raw)
To: Cousson, Benoit; +Cc: Linux Omap, Tony Lindgren, Shilimkar, Santosh, Que, Simon
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
> >
^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-19 16:50 ` [PATCH 3/5] omap:hwspinlock-added hwspinlock driver Hari Kanigeri
2010-07-24 16:43 ` Cousson, Benoit
@ 2010-07-28 16:58 ` Premi, Sanjeev
2010-07-29 4:56 ` Marathe, Yogesh
2010-07-29 13:29 ` Kanigeri, Hari
2010-07-29 14:05 ` Nishanth Menon
2 siblings, 2 replies; 33+ messages in thread
From: Premi, Sanjeev @ 2010-07-28 16:58 UTC (permalink / raw)
To: Kanigeri, Hari, Linux Omap, Tony Lindgren
Cc: Shilimkar, Santosh, Cousson, Benoit, Que, Simon
> -----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
>
^ permalink raw reply [flat|nested] 33+ messages in thread* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-28 16:58 ` Premi, Sanjeev
@ 2010-07-29 4:56 ` Marathe, Yogesh
2010-07-29 13:14 ` Kanigeri, Hari
2010-07-29 13:29 ` Kanigeri, Hari
1 sibling, 1 reply; 33+ messages in thread
From: Marathe, Yogesh @ 2010-07-29 4:56 UTC (permalink / raw)
To: Premi, Sanjeev, Kanigeri, Hari, Linux Omap, Tony Lindgren
Cc: Shilimkar, Santosh, Cousson, Benoit, Que, Simon
> -----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
^ permalink raw reply [flat|nested] 33+ messages in thread* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-29 4:56 ` Marathe, Yogesh
@ 2010-07-29 13:14 ` Kanigeri, Hari
2010-07-29 13:23 ` Cousson, Benoit
2010-07-30 6:53 ` Marathe, Yogesh
0 siblings, 2 replies; 33+ messages in thread
From: Kanigeri, Hari @ 2010-07-29 13:14 UTC (permalink / raw)
To: Marathe, Yogesh, Premi, Sanjeev, Linux Omap, Tony Lindgren
Cc: Shilimkar, Santosh, Cousson, Benoit, Que, Simon
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-29 13:14 ` Kanigeri, Hari
@ 2010-07-29 13:23 ` Cousson, Benoit
2010-07-30 6:53 ` Marathe, Yogesh
1 sibling, 0 replies; 33+ messages in thread
From: Cousson, Benoit @ 2010-07-29 13:23 UTC (permalink / raw)
To: Kanigeri, Hari
Cc: Marathe, Yogesh, Premi, Sanjeev, Linux Omap, Tony Lindgren,
Shilimkar, Santosh, Que, Simon
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
^ permalink raw reply [flat|nested] 33+ messages in thread* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-29 13:14 ` Kanigeri, Hari
2010-07-29 13:23 ` Cousson, Benoit
@ 2010-07-30 6:53 ` Marathe, Yogesh
2010-08-02 1:55 ` Kanigeri, Hari
1 sibling, 1 reply; 33+ messages in thread
From: Marathe, Yogesh @ 2010-07-30 6:53 UTC (permalink / raw)
To: Kanigeri, Hari, Premi, Sanjeev, Linux Omap, Tony Lindgren
Cc: Shilimkar, Santosh, Cousson, Benoit, Que, Simon
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
^ permalink raw reply [flat|nested] 33+ messages in thread* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-30 6:53 ` Marathe, Yogesh
@ 2010-08-02 1:55 ` Kanigeri, Hari
0 siblings, 0 replies; 33+ messages in thread
From: Kanigeri, Hari @ 2010-08-02 1:55 UTC (permalink / raw)
To: Marathe, Yogesh, Premi, Sanjeev, Linux Omap, Tony Lindgren
Cc: Shilimkar, Santosh, Cousson, Benoit, Que, Simon
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-28 16:58 ` Premi, Sanjeev
2010-07-29 4:56 ` Marathe, Yogesh
@ 2010-07-29 13:29 ` Kanigeri, Hari
1 sibling, 0 replies; 33+ messages in thread
From: Kanigeri, Hari @ 2010-07-29 13:29 UTC (permalink / raw)
To: Premi, Sanjeev, Linux Omap, Tony Lindgren
Cc: Shilimkar, Santosh, Cousson, Benoit, Que, Simon
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
> >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-19 16:50 ` [PATCH 3/5] omap:hwspinlock-added hwspinlock driver Hari Kanigeri
2010-07-24 16:43 ` Cousson, Benoit
2010-07-28 16:58 ` Premi, Sanjeev
@ 2010-07-29 14:05 ` Nishanth Menon
2010-08-11 22:03 ` Que, Simon
2 siblings, 1 reply; 33+ messages in thread
From: Nishanth Menon @ 2010-07-29 14:05 UTC (permalink / raw)
To: Kanigeri, Hari
Cc: Linux Omap, Tony Lindgren, Shilimkar, Santosh, Cousson, Benoit,
Que, Simon
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* RE: [PATCH 3/5] omap:hwspinlock-added hwspinlock driver
2010-07-29 14:05 ` Nishanth Menon
@ 2010-08-11 22:03 ` Que, Simon
2010-08-12 2:02 ` linux hardware spinlock framework for heterogeneous processor SOCs? Nishanth Menon
0 siblings, 1 reply; 33+ messages in thread
From: Que, Simon @ 2010-08-11 22:03 UTC (permalink / raw)
To: Menon, Nishanth, Kanigeri, Hari
Cc: Linux Omap, Tony Lindgren, Shilimkar, Santosh, Cousson, Benoit
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* linux hardware spinlock framework for heterogeneous processor SOCs?
2010-08-11 22:03 ` Que, Simon
@ 2010-08-12 2:02 ` Nishanth Menon
0 siblings, 0 replies; 33+ messages in thread
From: Nishanth Menon @ 2010-08-12 2:02 UTC (permalink / raw)
To: Que, Simon, LKML
Cc: Kanigeri, Hari, Linux Omap, Tony Lindgren, Shilimkar, Santosh,
Cousson, Benoit
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
^ permalink raw reply [flat|nested] 33+ messages in thread