RE: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
Ohad,
Sorry for the late response, was away from linux-omap mailing list last few
days.
Please see my response.
It would probably make more sense to call pm_runtime_get_sync during
hwspinlock_request{_specific}, and then call pm_runtime_put during
hwspinlock_free.
I agree, this looks like a good approach.
Thank you,
Best regards,
Hari
--
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
Re: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
On 9/20/2010 7:30 PM, Kevin Hilman wrote:
Ohad Ben-Coheno...@wizery.com writes:
Hi Hari,
On Thu, Aug 12, 2010 at 12:44 AM, Kanigeri, Harih-kanige...@ti.com wrote:
+/* 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);
Any reason for using pm_runtime_put instead of pm_runtime_put_sync?
Using pm_runtime_gett_sync pm_runtime_put_sync have been recommended by
Kevin Hilman.
Actually is there a need to call pm_runtime_put_sync for hwspinlock ? Spinlocks
are used by the co-processors and we have to ensure that the device doesn't
enter some low power mode without the knowledge of Co-processor. I don't think
run time PM is needed for hwspinlock.
Just doing pm_runtime_get_sync at probe time for all the spinlock instances
should be good.
It would probably make more sense to call pm_runtime_get_sync during
hwspinlock_request{_specific}, and then call pm_runtime_put during
hwspinlock_free.
This way the runtime PM's usage_count will reflect the number of locks
that are actually used, and if that number drops to (or never go
beyond) zero, it is desirable to have the hwspinlock's clock disabled.
This is also safe since no other core will use the hwspinlock if it
wasn't requested by the MPU beforehand (and if it does, we better know
about it and fix it).
FWIW, I agree with Ohad.
An additional benefit of using runtime PM is that the runtime PM core is
growing some useful debug and statistics features so that userspace
tools (including newer versions of powertop) can present useful stats
about which devices are active and how often etc.
And I agree with both of you.
Just to explain the context to Hari. IP like hwspinlock or mailbox does
not require functional clock to work. Because of that you can use it
without explicit clock enable thanks to the PRCM automatic modes.
So in theory you do not have to enable / idle hwmod for each spinlock
request or even during the probe, and this is what the original patches
from Simon were doing. I asked Simon to add explicit pm_runtime call,
even if useless, because of the reason mentioned by Kevin, but also
because the driver should not assume any automatic mode in the HW. That
IP can be used in other SoC that will not have PRCM at all.
In our case these calls will be mostly NOP, but that's still needed.
Regards,
Benoit
Kevin
--
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
Re: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
Ohad Ben-Cohen o...@wizery.com writes:
Hi Hari,
On Thu, Aug 12, 2010 at 12:44 AM, Kanigeri, Hari h-kanige...@ti.com wrote:
+/* 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);
Any reason for using pm_runtime_put instead of pm_runtime_put_sync?
Using pm_runtime_gett_sync pm_runtime_put_sync have been recommended by
Kevin Hilman.
Actually is there a need to call pm_runtime_put_sync for hwspinlock ?
Spinlocks are used by the co-processors and we have to ensure that the
device doesn't enter some low power mode without the knowledge of
Co-processor. I don't think run time PM is needed for hwspinlock.
Just doing pm_runtime_get_sync at probe time for all the spinlock instances
should be good.
It would probably make more sense to call pm_runtime_get_sync during
hwspinlock_request{_specific}, and then call pm_runtime_put during
hwspinlock_free.
This way the runtime PM's usage_count will reflect the number of locks
that are actually used, and if that number drops to (or never go
beyond) zero, it is desirable to have the hwspinlock's clock disabled.
This is also safe since no other core will use the hwspinlock if it
wasn't requested by the MPU beforehand (and if it does, we better know
about it and fix it).
FWIW, I agree with Ohad.
An additional benefit of using runtime PM is that the runtime PM core is
growing some useful debug and statistics features so that userspace
tools (including newer versions of powertop) can present useful stats
about which devices are active and how often etc.
Kevin
--
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
Re: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
Hi Hari,
On Thu, Aug 12, 2010 at 12:44 AM, Kanigeri, Hari h-kanige...@ti.com wrote:
+/* 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);
Any reason for using pm_runtime_put instead of pm_runtime_put_sync?
Using pm_runtime_gett_sync pm_runtime_put_sync have been recommended by
Kevin Hilman.
Actually is there a need to call pm_runtime_put_sync for hwspinlock ?
Spinlocks are used by the co-processors and we have to ensure that the device
doesn't enter some low power mode without the knowledge of Co-processor. I
don't think run time PM is needed for hwspinlock.
Just doing pm_runtime_get_sync at probe time for all the spinlock instances
should be good.
It would probably make more sense to call pm_runtime_get_sync during
hwspinlock_request{_specific}, and then call pm_runtime_put during
hwspinlock_free.
This way the runtime PM's usage_count will reflect the number of locks
that are actually used, and if that number drops to (or never go
beyond) zero, it is desirable to have the hwspinlock's clock disabled.
This is also safe since no other core will use the hwspinlock if it
wasn't requested by the MPU beforehand (and if it does, we better know
about it and fix it).
Thanks,
Ohad.
--
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
Re: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
Basak, Partha p-bas...@ti.com writes:
+/* 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);
Any reason for using pm_runtime_put instead of pm_runtime_put_sync?
Using pm_runtime_gett_sync pm_runtime_put_sync have been recommended
by Kevin Hilman.
err, not exacly.
I recommend use of the runtime PM API. Driver writers can decide
whether they want to use the _sync() version of the API (which has
immediate side effect) or the normal version which may have delayed side
effect.
Kevin
--
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
RE: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
Partha and Benoit,
+/* 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);
Any reason for using pm_runtime_put instead of pm_runtime_put_sync?
Using pm_runtime_gett_sync pm_runtime_put_sync have been recommended by
Kevin Hilman.
Actually is there a need to call pm_runtime_put_sync for hwspinlock ? Spinlocks
are used by the co-processors and we have to ensure that the device doesn't
enter some low power mode without the knowledge of Co-processor. I don't think
run time PM is needed for hwspinlock.
Just doing pm_runtime_get_sync at probe time for all the spinlock instances
should be good.
Thank you,
Best regards,
Hari
--
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
RE: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
-Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Que, Simon Sent: Wednesday, July 07, 2010 1:55 AM To: linux-omap@vger.kernel.org Cc: Kanigeri, Hari; Ohad Ben-Cohen; Shilimkar, Santosh Subject: [RFC v.4] omap: hwspinlock: Added hwspinlock driver Hello, This is the fourth and final RFC for the hardware spinlock driver. I have incorporated some changes and fixes based on Santosh's comments. Please give your comments. Thanks, Simon = From d4794eff60e5e509581fedaf2660b0d2d92463ab Mon Sep 17 00:00:00 2001 From: Simon Que s...@ti.com Date: Wed, 23 Jun 2010 18:40:30 -0500 Subject: [PATCH] omap: hwspinlock: Added hwspinlock driver 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 hardware spinlock. It will pass spinlock register addresses to the driver. The device initialization file is: arch/arm/mach-omap2/hwspinlocks.c The driver takes in data passed in device initialization. The function hwspinlock_probe() initializes the array of spinlock structures, each containing a spinlock register address provided by the device initialization. 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 --- arch/arm/mach-omap2/Makefile |2 + arch/arm/mach-omap2/hwspinlocks.c| 71 ++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c |2 +- arch/arm/plat-omap/Makefile |3 +- arch/arm/plat-omap/hwspinlock.c | 295 ++ arch/arm/plat-omap/include/plat/hwspinlock.h | 29 +++ arch/arm/plat-omap/include/plat/omap44xx.h |2 + 7 files changed, 402 insertions(+), 2 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/Makefile b/arch/arm/mach-omap2/Makefile index 6725b3a..5f5c87b 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -170,3 +170,5 @@ obj-y += $(nand- m) $(nand-y) smc91x-$(CONFIG_SMC91X):= gpmc-smc91x.o obj-y += $(smc91x-m) $(smc91x-y) + +obj-$(CONFIG_ARCH_OMAP4) += hwspinlocks.o \ No newline at end of file diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach- omap2/hwspinlocks.c new file mode 100644 index 000..58a6483 --- /dev/null +++ b/arch/arm/mach-omap2/hwspinlocks.c @@ -0,0 +1,71 @@ +/* + * 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
[RFC v.4] omap: hwspinlock: Added hwspinlock driver
Hello, This is the fourth and final RFC for the hardware spinlock driver. I have incorporated some changes and fixes based on Santosh's comments. Please give your comments. Thanks, Simon = From d4794eff60e5e509581fedaf2660b0d2d92463ab Mon Sep 17 00:00:00 2001 From: Simon Que s...@ti.com Date: Wed, 23 Jun 2010 18:40:30 -0500 Subject: [PATCH] omap: hwspinlock: Added hwspinlock driver 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 hardware spinlock. It will pass spinlock register addresses to the driver. The device initialization file is: arch/arm/mach-omap2/hwspinlocks.c The driver takes in data passed in device initialization. The function hwspinlock_probe() initializes the array of spinlock structures, each containing a spinlock register address provided by the device initialization. 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 --- arch/arm/mach-omap2/Makefile |2 + arch/arm/mach-omap2/hwspinlocks.c| 71 ++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c |2 +- arch/arm/plat-omap/Makefile |3 +- arch/arm/plat-omap/hwspinlock.c | 295 ++ arch/arm/plat-omap/include/plat/hwspinlock.h | 29 +++ arch/arm/plat-omap/include/plat/omap44xx.h |2 + 7 files changed, 402 insertions(+), 2 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/Makefile b/arch/arm/mach-omap2/Makefile index 6725b3a..5f5c87b 100644 --- a/arch/arm/mach-omap2/Makefile +++ b/arch/arm/mach-omap2/Makefile @@ -170,3 +170,5 @@ obj-y += $(nand-m) $(nand-y) smc91x-$(CONFIG_SMC91X):= gpmc-smc91x.o obj-y += $(smc91x-m) $(smc91x-y) + +obj-$(CONFIG_ARCH_OMAP4) += hwspinlocks.o \ No newline at end of file diff --git a/arch/arm/mach-omap2/hwspinlocks.c b/arch/arm/mach-omap2/hwspinlocks.c new file mode 100644 index 000..58a6483 --- /dev/null +++ b/arch/arm/mach-omap2/hwspinlocks.c @@ -0,0 +1,71 @@ +/* + * 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
RE: [RFC v.4] omap: hwspinlock: Added hwspinlock driver
Simon, -Original Message- From: Que, Simon Sent: Wednesday, July 07, 2010 1:55 AM To: linux-omap@vger.kernel.org Cc: Kanigeri, Hari; Ohad Ben-Cohen; Shilimkar, Santosh Subject: [RFC v.4] omap: hwspinlock: Added hwspinlock driver snip 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 hardware spinlock. It will pass spinlock register addresses to the driver. The device initialization file is: arch/arm/mach-omap2/hwspinlocks.c The driver takes in data passed in device initialization. The function hwspinlock_probe() initializes the array of spinlock structures, each containing a spinlock register address provided by the device initialization. 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). The above API description also should be present in the source file. Add It on top of respective API. Signed-off-by: Simon Que s...@ti.com --- arch/arm/mach-omap2/Makefile |2 + arch/arm/mach-omap2/hwspinlocks.c| 71 ++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c |2 +- arch/arm/plat-omap/Makefile |3 +- arch/arm/plat-omap/hwspinlock.c | 295 ++ arch/arm/plat-omap/include/plat/hwspinlock.h | 29 +++ arch/arm/plat-omap/include/plat/omap44xx.h |2 + 7 files changed, 402 insertions(+), 2 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 Apart from the documentation comments, patch looks good to me. Regards, Santosh -- 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
