From: Mathieu J. Poirier <[email protected]>
This is the second spin on STE's Hsem driver that is implemented
through the hwspinlock scheme. More specifically:
More comments have been added in the code.
Cleanup of included header files.
One of the original contributor's name corrected.
Calls to 'pm_runtime_disable'restored.
Signed-off-by: Mathieu Poirier <[email protected]>
---
drivers/hwspinlock/Kconfig | 23 +++-
drivers/hwspinlock/Makefile | 1 +
drivers/hwspinlock/u8500_hsem.c | 246 +++++++++++++++++++++++++++++++++++++++
3 files changed, 266 insertions(+), 4 deletions(-)
create mode 100644 drivers/hwspinlock/u8500_hsem.c
diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 1f29bab..9b4f135 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -2,10 +2,11 @@
# Generic HWSPINLOCK framework
#
-config HWSPINLOCK
- tristate "Generic Hardware Spinlock framework"
- depends on ARCH_OMAP4
- help
+menuconfig HWSPINLOCK
+ bool "Generic Hardware Spinlock framework"
+ depends on ARCH_OMAP4 || ARCH_U8500
+ default n
+ ---help---
Say y here to support the generic hardware spinlock framework.
You only need to enable this if you have hardware spinlock module
on your system (usually only relevant if your system has remote slave
@@ -13,6 +14,8 @@ config HWSPINLOCK
If unsure, say N.
+if HWSPINLOCK
+
config HWSPINLOCK_OMAP
tristate "OMAP Hardware Spinlock device"
depends on HWSPINLOCK && ARCH_OMAP4
@@ -21,3 +24,15 @@ config HWSPINLOCK_OMAP
introduced in OMAP4).
If unsure, say N.
+
+config HSEM_U8500
+ tristate "STE Hardware Semaphore functionality"
+ depends on HWSPINLOCK && ARCH_U8500
+ help
+ Say y here to support the STE Hardware Semaphore functionality. They
+ provide a synchronisation mechanism for the various processor on the
+ SoC.
+
+ If unsure, say N.
+
+endif # HWSPINLOCK
diff --git a/drivers/hwspinlock/Makefile b/drivers/hwspinlock/Makefile
index 5729a3f..93eb64b 100644
--- a/drivers/hwspinlock/Makefile
+++ b/drivers/hwspinlock/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_HWSPINLOCK) += hwspinlock_core.o
obj-$(CONFIG_HWSPINLOCK_OMAP) += omap_hwspinlock.o
+obj-$(CONFIG_HSEM_U8500) += u8500_hsem.o
diff --git a/drivers/hwspinlock/u8500_hsem.c b/drivers/hwspinlock/u8500_hsem.c
new file mode 100644
index 0000000..d5bedd3
--- /dev/null
+++ b/drivers/hwspinlock/u8500_hsem.c
@@ -0,0 +1,246 @@
+/*
+ * u8500 HWSEM driver
+ *
+ * Copyright (C) 2010-2011 ST-Ericsson
+ *
+ * Implements u8500 semaphore handling for protocol 1, no interrupts.
+ *
+ * Author: Mathieu Poirier <[email protected]>
+ * Heavily borrowed from the work of :
+ * Simon Que <[email protected]>
+ * Hari Kanigeri <[email protected]>
+ * Ohad Ben-Cohen <[email protected]>
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/hwspinlock.h>
+#include <linux/platform_device.h>
+
+#include "hwspinlock_internal.h"
+
+/*
+ * Implementation of STE's HSem protocol 1 without interrutps.
+ * The only masterID we allow is '0x01' to force people to use
+ * HSems for synchronisation between processors rather than processes
+ * on the ARM core.
+ */
+
+#define U8500_MAX_SEMAPHORE 32 /* a total of 32 semaphore */
+#define RESET_SEMAPHORE (0) /* free */
+
+/* CPU ID for master running u8500 kernel.
+ * Hswpinlocks should only be used to synchonise operations
+ * between the Cortex A9 core and the other CPUs. Hence
+ * forcing the masterID to a preset value.
+ */
+#define HSEM_MASTER_ID 0x01
+
+#define HSEM_REGISTER_OFFSET 0x08
+
+#define HSEM_CTRL_REG 0x00
+#define HSEM_ICRALL 0x90
+#define HSEM_PROTOCOL_1 0x01
+
+#define to_u8500_hsem(lock) \
+ container_of(lock, struct u8500_hsem, lock)
+
+struct u8500_hsem {
+ struct hwspinlock lock;
+ void __iomem *addr;
+};
+
+struct u8500_hsem_state {
+ void __iomem *io_base; /* Mapped base address */
+};
+
+static int u8500_hsem_trylock(struct hwspinlock *lock)
+{
+ struct u8500_hsem *u8500_lock = to_u8500_hsem(lock);
+
+ writel(HSEM_MASTER_ID, u8500_lock->addr);
+
+ /* get only first 4 bit and compare to masterID.
+ * if equal, we have the semaphore, otherwise
+ * someone else has it.
+ */
+ return (HSEM_MASTER_ID == (0x0F & readl(u8500_lock->addr)));
+}
+
+static void u8500_hsem_unlock(struct hwspinlock *lock)
+{
+ struct u8500_hsem *u8500_lock = to_u8500_hsem(lock);
+
+ /* release the lock by writing 0 to it */
+ writel(RESET_SEMAPHORE, u8500_lock->addr);
+}
+
+/*
+ * u8500: what value is recommended here ?
+ */
+static void u8500_hsem_relax(struct hwspinlock *lock)
+{
+ ndelay(50);
+}
+
+static const struct hwspinlock_ops u8500_hwspinlock_ops = {
+ .trylock = u8500_hsem_trylock,
+ .unlock = u8500_hsem_unlock,
+ .relax = u8500_hsem_relax,
+};
+
+static int __devinit u8500_hsem_probe(struct platform_device *pdev)
+{
+
+ struct u8500_hsem *u8500_lock;
+ struct u8500_hsem_state *state;
+ struct hwspinlock *lock;
+ struct resource *res;
+ void __iomem *io_base;
+ int i, ret;
+ ulong offset, val;
+
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ io_base = ioremap(res->start, resource_size(res));
+ if (!io_base) {
+ ret = -ENOMEM;
+ goto free_state;
+ }
+
+ /* make sure protocol 1 is selected */
+ val = readl(io_base + HSEM_CTRL_REG);
+ writel((val & ~HSEM_PROTOCOL_1), io_base + HSEM_CTRL_REG);
+
+ /* clear all interrupts */
+ writel(0xFFFF, io_base + HSEM_ICRALL);
+
+ state->io_base = io_base;
+
+ platform_set_drvdata(pdev, state);
+
+ /*
+ * no pm needed for HSem but required to comply
+ * with hwspilock core.
+ */
+ pm_runtime_enable(&pdev->dev);
+
+ offset = HSEM_REGISTER_OFFSET;
+
+ for (i = 0; i < U8500_MAX_SEMAPHORE; i++) {
+ u8500_lock = kzalloc(sizeof(*u8500_lock), GFP_KERNEL);
+ if (!u8500_lock) {
+ ret = -ENOMEM;
+ goto free_locks;
+ }
+
+ u8500_lock->lock.dev = &pdev->dev;
+ u8500_lock->lock.owner = THIS_MODULE;
+ u8500_lock->lock.id = i;
+ u8500_lock->lock.ops = &u8500_hwspinlock_ops;
+ u8500_lock->addr = io_base + offset + sizeof(u32) * i;
+
+ ret = hwspin_lock_register(&u8500_lock->lock);
+ if (ret) {
+ kfree(u8500_lock);
+ goto free_locks;
+ }
+ }
+
+ return 0;
+
+free_locks:
+ while (--i >= 0) {
+ lock = hwspin_lock_unregister(i);
+ /* this should't happen, but let's give our best effort */
+ if (!lock) {
+ dev_err(&pdev->dev, "%s: cleanups failed\n", __func__);
+ continue;
+ }
+ u8500_lock = to_u8500_hsem(lock);
+ kfree(u8500_lock);
+ }
+
+ pm_runtime_disable(&pdev->dev);
+ iounmap(io_base);
+free_state:
+ kfree(state);
+ return ret;
+}
+
+static int u8500_hsem_remove(struct platform_device *pdev)
+{
+ struct u8500_hsem_state *state = platform_get_drvdata(pdev);
+ struct hwspinlock *lock;
+ struct u8500_hsem *u8500_lock;
+ void __iomem *io_base;
+ int i;
+
+ io_base = state->io_base;
+
+ /* clear all interrupts */
+ writel(0xFFFF, io_base + HSEM_ICRALL);
+
+ for (i = 0; i < U8500_MAX_SEMAPHORE; i++) {
+ lock = hwspin_lock_unregister(i);
+ /* this shouldn't happen at this point. if it does, at least
+ * don't continue with the remove */
+ if (!lock) {
+ dev_err(&pdev->dev, "%s: failed on %d\n", __func__, i);
+ return -EBUSY;
+ }
+
+ u8500_lock = to_u8500_hsem(lock);
+ kfree(u8500_lock);
+ }
+
+ pm_runtime_disable(&pdev->dev);
+ iounmap(io_base);
+ kfree(state);
+
+ return 0;
+}
+
+static struct platform_driver u8500_hsem_driver = {
+ .probe = u8500_hsem_probe,
+ .remove = u8500_hsem_remove,
+ .driver = {
+ .name = "u8500_hsem",
+ },
+};
+
+static int __init u8500_hsem_init(void)
+{
+ return platform_driver_register(&u8500_hsem_driver);
+}
+/* board init code might need to reserve hwspinlocks for predefined purposes */
+postcore_initcall(u8500_hsem_init);
+
+static void __exit u8500_hsem_exit(void)
+{
+ platform_driver_unregister(&u8500_hsem_driver);
+}
+module_exit(u8500_hsem_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Hardware Semaphore driver for u8500");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]");
--
1.7.1
On Mon, Apr 11, 2011 at 6:24 PM, <[email protected]> wrote:
>
> From: Mathieu J. Poirier <[email protected]>
>
> This is the second spin on STE's Hsem driver that is implemented
> through the hwspinlock scheme. ?More specifically:
>
> ?More comments have been added in the code.
> ?Cleanup of included header files.
> ?One of the original contributor's name corrected.
> ?Calls to 'pm_runtime_disable'restored.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
> ---
Looks good, but please revise the commit log (think of a random reader
who tries to understand what does this patch do, and is less
interested to hear about its revisions' history).
Otherwise,
Acked-by: Ohad Ben-Cohen <[email protected]>
(btw I assume the "PATCH 1/2" in the subject is just a typo, and
there's no missing "PATCH 2/2")
On 11-04-12 07:44 AM, Ohad Ben-Cohen wrote:
> On Mon, Apr 11, 2011 at 6:24 PM,<[email protected]> wrote:
>> From: Mathieu J. Poirier<[email protected]>
>>
>> This is the second spin on STE's Hsem driver that is implemented
>> through the hwspinlock scheme. More specifically:
>>
>> More comments have been added in the code.
>> Cleanup of included header files.
>> One of the original contributor's name corrected.
>> Calls to 'pm_runtime_disable'restored.
>>
>> Signed-off-by: Mathieu Poirier<[email protected]>
>> ---
> Looks good, but please revise the commit log (think of a random reader
> who tries to understand what does this patch do, and is less
> interested to hear about its revisions' history).
>
> Otherwise,
>
> Acked-by: Ohad Ben-Cohen<[email protected]>
>
> (btw I assume the "PATCH 1/2" in the subject is just a typo, and
> there's no missing "PATCH 2/2")
>
Ohad,
I will revise the commit log and send a V3. The PATCH 1/2 was generated
by send-email even if there was only one patch file... But you are
correct, there was only just one patch.
Mathieu.
On Monday 11 April 2011, [email protected] wrote:
> From: Mathieu J. Poirier <[email protected]>
>
> This is the second spin on STE's Hsem driver that is implemented
> through the hwspinlock scheme. More specifically:
>
> More comments have been added in the code.
> Cleanup of included header files.
> One of the original contributor's name corrected.
> Calls to 'pm_runtime_disable'restored.
>
> Signed-off-by: Mathieu Poirier <[email protected]>
Looks very nice overall, just a few small details I noticed:
> +
> +#define HSEM_REGISTER_OFFSET 0x08
> +
> +#define HSEM_CTRL_REG 0x00
> +#define HSEM_ICRALL 0x90
> +#define HSEM_PROTOCOL_1 0x01
> +
> +#define to_u8500_hsem(lock) \
> + container_of(lock, struct u8500_hsem, lock)
> +
> +struct u8500_hsem {
> + struct hwspinlock lock;
> + void __iomem *addr;
> +};
It seems inconsistent to name it sem instead of spinlock.
> +struct u8500_hsem_state {
> + void __iomem *io_base; /* Mapped base address */
> +};
If you make that one data structure, you only need a single allocation:
struct u8500_hsem_state {
void __iomem *io_base;
struct u8500_hsem hsem[U8500_MAX_SEMAPHORE];
}
> +
> + for (i = 0; i < U8500_MAX_SEMAPHORE; i++) {
> + u8500_lock = kzalloc(sizeof(*u8500_lock), GFP_KERNEL);
> + if (!u8500_lock) {
> + ret = -ENOMEM;
> + goto free_locks;
> + }
> +
> + u8500_lock->lock.dev = &pdev->dev;
> + u8500_lock->lock.owner = THIS_MODULE;
> + u8500_lock->lock.id = i;
> + u8500_lock->lock.ops = &u8500_hwspinlock_ops;
> + u8500_lock->addr = io_base + offset + sizeof(u32) * i;
> +
> + ret = hwspin_lock_register(&u8500_lock->lock);
> + if (ret) {
> + kfree(u8500_lock);
> + goto free_locks;
> + }
> + }
When you do that, this can be a single allocation.
Thinking about it some more, it may actually be worthwhile to still improve
the API here: I think the owner field should be part of the operations structure,
because it is constant. It would also be nice to have a "private" pointer
in struct hwspinlock, so you don't need to wrap it if you don't want to.
Finally, the hwspin_lock_register could take the specific values as arguments
instead of requiring you to fill it out first.
Arnd
On 11-04-12 11:46 AM, Arnd Bergmann wrote:
> On Monday 11 April 2011, [email protected] wrote:
>> From: Mathieu J. Poirier<[email protected]>
>>
>> This is the second spin on STE's Hsem driver that is implemented
>> through the hwspinlock scheme. More specifically:
>>
>> More comments have been added in the code.
>> Cleanup of included header files.
>> One of the original contributor's name corrected.
>> Calls to 'pm_runtime_disable'restored.
>>
>> Signed-off-by: Mathieu Poirier<[email protected]>
> Looks very nice overall, just a few small details I noticed:
>
>> +
>> +#define HSEM_REGISTER_OFFSET 0x08
>> +
>> +#define HSEM_CTRL_REG 0x00
>> +#define HSEM_ICRALL 0x90
>> +#define HSEM_PROTOCOL_1 0x01
>> +
>> +#define to_u8500_hsem(lock) \
>> + container_of(lock, struct u8500_hsem, lock)
>> +
>> +struct u8500_hsem {
>> + struct hwspinlock lock;
>> + void __iomem *addr;
>> +};
> It seems inconsistent to name it sem instead of spinlock.
>
This is a good point and I've been going back and forth on that one.
TI's implementation is based on 'spinlock' but in this case there is not
a single mention of a 'spinlock' in the CPU's reference manual, leaving
potential users to wonder if spinlock == hsem. I think using 'hsem'
makes more sense here.
>> +struct u8500_hsem_state {
>> + void __iomem *io_base; /* Mapped base address */
>> +};
> If you make that one data structure, you only need a single allocation:
>
> struct u8500_hsem_state {
> void __iomem *io_base;
> struct u8500_hsem hsem[U8500_MAX_SEMAPHORE];
> }
I don't see the real advantage in doing a single allocation - the
dynamic allocation method is also used in 'omap_hwspinlock.c'. Is
modification mandatory to get the driver accepted ?
>> +
>> + for (i = 0; i< U8500_MAX_SEMAPHORE; i++) {
>> + u8500_lock = kzalloc(sizeof(*u8500_lock), GFP_KERNEL);
>> + if (!u8500_lock) {
>> + ret = -ENOMEM;
>> + goto free_locks;
>> + }
>> +
>> + u8500_lock->lock.dev =&pdev->dev;
>> + u8500_lock->lock.owner = THIS_MODULE;
>> + u8500_lock->lock.id = i;
>> + u8500_lock->lock.ops =&u8500_hwspinlock_ops;
>> + u8500_lock->addr = io_base + offset + sizeof(u32) * i;
>> +
>> + ret = hwspin_lock_register(&u8500_lock->lock);
>> + if (ret) {
>> + kfree(u8500_lock);
>> + goto free_locks;
>> + }
>> + }
> When you do that, this can be a single allocation.
If you don't mind, I will let Ohad and friends deal with the API
improvement.
> Thinking about it some more, it may actually be worthwhile to still improve
> the API here: I think the owner field should be part of the operations structure,
> because it is constant. It would also be nice to have a "private" pointer
> in struct hwspinlock, so you don't need to wrap it if you don't want to.
>
> Finally, the hwspin_lock_register could take the specific values as arguments
> instead of requiring you to fill it out first.
>
> Arnd
>
Thanks for the review,
Mathieu.
On Tue, Apr 12, 2011 at 8:46 PM, Arnd Bergmann <[email protected]> wrote:
> When you do that, this can be a single allocation.
>
> Thinking about it some more, it may actually be worthwhile to still improve
> the API here: I think the owner field should be part of the operations structure,
> because it is constant. It would also be nice to have a "private" pointer
> in struct hwspinlock, so you don't need to wrap it if you don't want to.
These are good improvements, part of which I planned to do so too
(especially to take out the dev, ops and owner fields into a dedicated
structure, and have all the hwspinlock instances, which belong to that
block, point at it).
By introducing a "private" pointer, and removing the platform-specific
wrapping around it, we can also get rid of the 'id' field, and induce
it with a simple pointers arithmetic (namely, id = pointer of instance
- array base address).
On Tue, Apr 12, 2011 at 10:13 PM, Mathieu Poirier
<[email protected]> wrote:
>>> +struct u8500_hsem {
>>> + ? ? ? struct hwspinlock lock;
>>> + ? ? ? void __iomem *addr;
>>> +};
>>
>> It seems inconsistent to name it sem instead of spinlock.
>>
> This is a good point and I've been going back and forth on that one. ?TI's
> implementation is based on 'spinlock' but in this case there is not a single
> mention of a 'spinlock' in the CPU's reference manual, leaving potential
> users to wonder if spinlock == hsem.
I figured out this was your line of thought.
Looking forward, though, we have some design decisions to do which are
related to this spinlock vs. semaphore issue - it's much more than
just names.
While the OMAP hardware currently has only these hwspinlocks modules,
which require the host to spin until they become free, we now have two
other systems (the u8500 and TI's C6474) which also support additional
semaphore-like functionality of queuing, owner semantics and interrupt
notification (so the host wouldn't have to spin, and the lock can be
used for a wider range of non-atomic use cases).
Sadly, the specs of the u8500 are closed, which makes it somewhat hard
to start any reasonable design discussions.
Btw, last I heard, mainline c6x support is coming. but it's not there yet.
On Tuesday 12 April 2011, Mathieu Poirier wrote:
> > struct u8500_hsem_state {
> > void __iomem *io_base;
> > struct u8500_hsem hsem[U8500_MAX_SEMAPHORE];
> > }
> I don't see the real advantage in doing a single allocation - the
> dynamic allocation method is also used in 'omap_hwspinlock.c'. Is
> modification mandatory to get the driver accepted ?
Not strictly required, but somewhat cleaner IMHO. If you have a good
reason for splitting the allocations, just document that clearly.
One more thing I just noticed: the hwspinlock_internal.h file defines
the hwspinlock->id field as "a global, unique, system-wide, index of
the lock", but the u8500 hsem just sets it to an integer starting
at zero. If there are multiple devices providing hwspinlocks in the
same system, that cannot work.
Arnd
On Tuesday 19 April 2011, Mathieu Poirier wrote:
> > One more thing I just noticed: the hwspinlock_internal.h file defines
> > the hwspinlock->id field as "a global, unique, system-wide, index of
> > the lock", but the u8500 hsem just sets it to an integer starting
> > at zero. If there are multiple devices providing hwspinlocks in the
> > same system, that cannot work.
> >
> I have to admit I'm not sure of what your asking here. Hwspinlocks
> should be administered by only one entity and this is what this driver
> is doing.
>
> Please get back to me with a clarification.
You could have a system with multiple instances of the hardware block
that holds your spinlock. I've seen SMP systems that basically take
multiple SOCs and put them into a common address space.
More importantly, there could be a device on an external bus that also
provides a hwspinlock. The API can deal with this, but your driver will
conflict with any other driver trying to do the same.
Arnd
On 11-04-17 02:39 PM, Arnd Bergmann wrote:
> On Tuesday 12 April 2011, Mathieu Poirier wrote:
>>> struct u8500_hsem_state {
>>> void __iomem *io_base;
>>> struct u8500_hsem hsem[U8500_MAX_SEMAPHORE];
>>> }
>> I don't see the real advantage in doing a single allocation - the
>> dynamic allocation method is also used in 'omap_hwspinlock.c'. Is
>> modification mandatory to get the driver accepted ?
> Not strictly required, but somewhat cleaner IMHO. If you have a good
> reason for splitting the allocations, just document that clearly.
I don't have a reason other than I thought what was found in
omap_hwspinlock.c looked perfectly fine to me and there was no reason to
proceed otherwise in 'u8500_hsem.c'.
> One more thing I just noticed: the hwspinlock_internal.h file defines
> the hwspinlock->id field as "a global, unique, system-wide, index of
> the lock", but the u8500 hsem just sets it to an integer starting
> at zero. If there are multiple devices providing hwspinlocks in the
> same system, that cannot work.
>
> Arnd
I have to admit I'm not sure of what your asking here. Hwspinlocks
should be administered by only one entity and this is what this driver
is doing.
Please get back to me with a clarification.