> -----Original Message-----
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@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-ow...@vger.kernel.org
> > [mailto:linux-omap-ow...@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 <s...@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 <s...@ti.com>
> > Signed-off-by: Hari Kanigeri <h-kanige...@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 <s...@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 <s...@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 majord...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
