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@xxxxxx>
> >
> > 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@xxxxxx>
> > Signed-off-by: Hari Kanigeri<h-kanigeri2@xxxxxx>
> > ---
> > 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@xxxxxx>
> > + *
> > + * 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@xxxxxx>
> > + *
> > + * 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
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
