Hi,
This is an updated series mainly addressing Mark Rutland's comments
about hwlock specifier being always one-cell. The series adds the
support for #hwlock-cells property and adds a simple default OF
translate function.
The DTS patches from previous series have already been merged, and
needs this property to be added. This is handled in a separate series
that only deals with OMAP hwspinlock DTS patches.
The series, along with the DTS patches, is tested on top of v3.13-rc8
plus Tero's v13 clock DT series and Tony's 3.14 staged branches. The
validation on OMAP5, DRA7, AM437 requires Tero's series with couple of
additional base patches for AM43xx. AM43xx functionality needs a hwmod
fix [1] for creating the associated omap_device as well.
The validation logs on all the applicable OMAP SoCs are at:
OMAP4 - http://paste2.org/YJ7ZwG80
OMAP5 - http://paste2.org/c6vO96b9
DRA7 - http://paste2.org/tHvxN439
AM33x - http://paste2.org/AjCv0U4t
AM43x - http://paste2.org/2AKIPa55
The kernel with the test patches plus the various pulled in branches
is here for reference (not for merging)
https://github.com/sumananna/omap-kernel/commits/hwspinlock/3.13-rc8-v4-test
[1] http://marc.info/?l=linux-omap&m=138939747524820&w=2
Changes new in v4:
- The DT bindings are split into separate patches, and updated to
add comments about #hwlock-cells (Patches 1 & 2)
- Fixed a registration issue with repeated module installation and
removal. (Patch 3)
- Added a new OF helper to support #hwlock-cells in addition to the
previous OF functions (Patch 4). The OMAP adaptation patch is
updated to use the default translate function (Patch 5)
- Updated hwspinlock documentation to adjust for the structure
changes and the new api additions. (Patches 3, 4)
- Added build support for AM335x, AM43xx and DRA7xx (Patch 7)
- The AM335/AM43x fix patch is unchanged (Patch 6)
v3:
- Removed the DT property hwlock-base-id and associated OF helper
- Added changes in core to support requesting a specific hwlock using
phandle + args approach
- Revised both the common and OMAP DT bindings document
http://marc.info/?l=linux-omap&m=138143992932197&w=2
v2:
- Added a new common DT binding documentation and OF helpers.
- Revised OMAP DT parse support to use the new OF helper (Patch2)
- OMAP5 hwspinlock support including the hwmod entry and DT node
- Add AM335x support to OMAP hwspinlock driver, including a fix
needed in driver given that AM335 spinlock module requires s/w wakeup
- AM335 DT node for spinlock, and a hwmod change to enable smart-idle
for AM335.
- OMAP4 DT node patch is unchanged
http://marc.info/?l=linux-omap&m=137944644112727&w=2
v1:
- Add DT parse support to OMAP hwspinlock driver
- Add OMAP4 DT node and bindings information
http://marc.info/?l=linux-omap&m=137823082308009&w=2
Suman Anna (7):
Documentation: dt: add common bindings for hwspinlock
Documentation: dt: add the omap hwspinlock bindings document
hwspinlock/core: maintain a list of registered hwspinlock banks
hwspinlock/core: add common OF helpers
hwspinlock/omap: add support for dt nodes
hwspinlock/omap: enable module before reading SYSSTATUS register
hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
.../devicetree/bindings/hwlock/hwlock.txt | 52 +++++++
.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 24 ++++
Documentation/hwspinlock.txt | 36 ++++-
MAINTAINERS | 1 -
arch/arm/mach-omap2/Makefile | 3 -
arch/arm/mach-omap2/hwspinlock.c | 60 --------
drivers/hwspinlock/Kconfig | 2 +-
drivers/hwspinlock/hwspinlock_core.c | 159 ++++++++++++++++++++-
drivers/hwspinlock/hwspinlock_internal.h | 6 +
drivers/hwspinlock/omap_hwspinlock.c | 39 +++--
include/linux/hwspinlock.h | 20 ++-
11 files changed, 321 insertions(+), 81 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
delete mode 100644 arch/arm/mach-omap2/hwspinlock.c
--
1.8.4.3
This patch adds the generic common bindings used to represent
a hwlock device and use/request locks in a device-tree build.
All the platform-specific hwlock driver implementations need the
number of locks and associated base id for registering the locks
present within the device with the driver core. The number of locks
is represented by 'hwlock-num-locks' property in DT bindings. A
property for base id is not needed in DT binding, as it can be
satisfied using a phandle + args specifier. The args specifier
length is dependent on each vendor-specific implementation and
is represented through the '#hwlock-cells' property.
Note that the document is named hwlock.txt deliberately to keep it
a bit more generic.
Cc: Rob Herring <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
---
.../devicetree/bindings/hwlock/hwlock.txt | 52 ++++++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
diff --git a/Documentation/devicetree/bindings/hwlock/hwlock.txt b/Documentation/devicetree/bindings/hwlock/hwlock.txt
new file mode 100644
index 0000000..32381cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/hwlock.txt
@@ -0,0 +1,52 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one platform
+implementation to another. The platform specific bindings should explicitly
+state if a property is mandatory or optional. Please look through the
+individual platform specific hwlock binding documentations for identifying
+the applicable properties.
+
+Common properties:
+- #hwlock-cells: Specifies the number of cells needed to represent a
+ specific lock.
+- hwlock-num-locks: Number of locks present in a hwlock device. This
+ property is needed on hwlock devices, where the number
+ of supported locks within a hwlock device cannot be
+ read from a register.
+
+Hwlock Users:
+=============
+
+Nodes that require specific hwlock(s) should specify them using one or more
+properties, each containing a phandle to the hwlock node and an args specifier
+value as indicated by #hwlock-cells. Multiple hwlocks can be requested using
+an array of the phandle and hwlock number specifier tuple.
+
+1. Example of a node using a single specific hwlock:
+
+The following example has a node requesting a hwlock in the bank defined by
+the node hwlock1. hwlock1 is a hwlock provider with an argument specifier
+of length 1.
+
+ node {
+ ...
+ hwlocks = <&hwlock1 2>;
+ ...
+ };
+
+2. Example of a node using multiple specific hwlocks:
+
+The following example has a node requesting two hwlocks, a hwlock within
+the hwlock device node 'hwlock1' with #hwlock-cells value of 1, and another
+hwlock within the hwlock device node 'hwlock2' with #hwlock-cells value of 2.
+
+ node {
+ ...
+ hwlocks = <&hwlock1 2>, <&hwlock2 0 3>;
+ ...
+ };
--
1.8.4.3
The hwspinlock_device structure is used for registering a bank of
locks with the driver core. The structure already contains the
necessary members to identify the bank of locks. The core does not
maintain the hwspinlock_devices itself, but maintains only a radix
tree for all the registered locks. A specific lock can be requested
by users using a global lock id, and any device-specific fields
can be retrieved through a reference to the hwspinlock_device in
each lock.
The global lock id, however, is not friendly to be requested for
users using the device-tree model. The device-tree representation
will typically have each of the hwspinlock devices represented as
a DT node, and a specific lock can be requested using the device's
phandle and a lock specifier. Add support to the core therefore to
maintain all the registered hwspinlock_devices, so that a device
can be looked up and a specific lock belonging to the device
requested through a phandle + args approach.
Signed-off-by: Suman Anna <[email protected]>
---
Documentation/hwspinlock.txt | 2 ++
drivers/hwspinlock/hwspinlock_core.c | 51 ++++++++++++++++++++++++++++++++
drivers/hwspinlock/hwspinlock_internal.h | 2 ++
3 files changed, 55 insertions(+)
diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
index 62f7d4e..640ae47 100644
--- a/Documentation/hwspinlock.txt
+++ b/Documentation/hwspinlock.txt
@@ -251,6 +251,7 @@ implementation using the hwspin_lock_register() API.
/**
* struct hwspinlock_device - a device which usually spans numerous hwspinlocks
+ * @list: list element to link hwspinlock devices together
* @dev: underlying device, will be used to invoke runtime PM api
* @ops: platform-specific hwspinlock handlers
* @base_id: id index of the first lock in this device
@@ -258,6 +259,7 @@ implementation using the hwspin_lock_register() API.
* @lock: dynamically allocated array of 'struct hwspinlock'
*/
struct hwspinlock_device {
+ struct list_head list;
struct device *dev;
const struct hwspinlock_ops *ops;
int base_id;
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 461a0d7..48f7866 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -59,6 +59,11 @@ static RADIX_TREE(hwspinlock_tree, GFP_KERNEL);
*/
static DEFINE_MUTEX(hwspinlock_tree_lock);
+/*
+ * A linked list for maintaining all the registered hwspinlock devices.
+ * The list is maintained in an ordered-list of the supported locks group.
+ */
+static LIST_HEAD(hwspinlock_devices);
/**
* __hwspin_trylock() - attempt to lock a specific hwspinlock
@@ -307,6 +312,40 @@ out:
return hwlock;
}
+/*
+ * Add a new hwspinlock device to the global list, keeping the list of
+ * devices sorted by base order.
+ *
+ * Returns 0 on success, or -EBUSY if the new device overlaps with some
+ * other device's lock space.
+ */
+static int hwspinlock_device_add(struct hwspinlock_device *bank)
+{
+ struct list_head *entry = &hwspinlock_devices;
+ struct hwspinlock_device *_bank;
+ int ret = 0;
+
+ list_for_each(entry, &hwspinlock_devices) {
+ _bank = list_entry(entry, struct hwspinlock_device, list);
+ if (_bank->base_id >= bank->base_id + bank->num_locks)
+ break;
+ }
+
+ if (entry != &hwspinlock_devices &&
+ entry->prev != &hwspinlock_devices) {
+ _bank = list_entry(entry->prev, struct hwspinlock_device, list);
+ if (_bank->base_id + _bank->num_locks > bank->base_id) {
+ dev_err(bank->dev, "hwlock space overlap, cannot add device\n");
+ ret = -EBUSY;
+ }
+ }
+
+ if (!ret)
+ list_add_tail(&bank->list, entry);
+
+ return ret;
+}
+
/**
* hwspin_lock_register() - register a new hw spinlock device
* @bank: the hwspinlock device, which usually provides numerous hw locks
@@ -339,6 +378,12 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
bank->base_id = base_id;
bank->num_locks = num_locks;
+ mutex_lock(&hwspinlock_tree_lock);
+ ret = hwspinlock_device_add(bank);
+ mutex_unlock(&hwspinlock_tree_lock);
+ if (ret)
+ return ret;
+
for (i = 0; i < num_locks; i++) {
hwlock = &bank->lock[i];
@@ -355,6 +400,9 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
reg_failed:
while (--i >= 0)
hwspin_lock_unregister_single(base_id + i);
+ mutex_lock(&hwspinlock_tree_lock);
+ list_del(&bank->list);
+ mutex_unlock(&hwspinlock_tree_lock);
return ret;
}
EXPORT_SYMBOL_GPL(hwspin_lock_register);
@@ -386,6 +434,9 @@ int hwspin_lock_unregister(struct hwspinlock_device *bank)
WARN_ON(tmp != hwlock);
}
+ mutex_lock(&hwspinlock_tree_lock);
+ list_del(&bank->list);
+ mutex_unlock(&hwspinlock_tree_lock);
return 0;
}
EXPORT_SYMBOL_GPL(hwspin_lock_unregister);
diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index d26f78b..aff560c 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -53,6 +53,7 @@ struct hwspinlock {
/**
* struct hwspinlock_device - a device which usually spans numerous hwspinlocks
+ * @list: list element to link hwspinlock devices together
* @dev: underlying device, will be used to invoke runtime PM api
* @ops: platform-specific hwspinlock handlers
* @base_id: id index of the first lock in this device
@@ -60,6 +61,7 @@ struct hwspinlock {
* @lock: dynamically allocated array of 'struct hwspinlock'
*/
struct hwspinlock_device {
+ struct list_head list;
struct device *dev;
const struct hwspinlock_ops *ops;
int base_id;
--
1.8.4.3
This patch adds three new OF helper functions to use/request
locks from a hwspinlock device instantiated through a
device-tree blob.
1. The of_hwspin_lock_get_num_locks() is a common helper
function to read the common 'hwlock-num-locks' property.
2. The of_hwspin_lock_simple_xlate() is a simple default
translator function for hwspinlock provider implementations
that use a single cell number for requesting a specific lock
(relatively indexed) within a hwlock bank.
3. The of_hwspin_lock_request_specific() API can be used by
hwspinlock clients to request a specific lock using the
phandle + args specifier. This function relies on the
implementation providing back a relative hwlock id within
the bank from the args specifier.
Signed-off-by: Suman Anna <[email protected]>
---
Documentation/hwspinlock.txt | 34 +++++++++-
drivers/hwspinlock/hwspinlock_core.c | 108 ++++++++++++++++++++++++++++++-
drivers/hwspinlock/hwspinlock_internal.h | 4 ++
include/linux/hwspinlock.h | 20 +++++-
4 files changed, 161 insertions(+), 5 deletions(-)
diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
index 640ae47..903d477 100644
--- a/Documentation/hwspinlock.txt
+++ b/Documentation/hwspinlock.txt
@@ -48,6 +48,15 @@ independent, drivers.
ids for predefined purposes.
Should be called from a process context (might sleep).
+ struct hwspinlock *of_hwspin_lock_request_specific(
+ struct device_node *np, const char *propname, int index);
+ - assign a specific hwspinlock id and return its address, or NULL
+ if that hwspinlock is already in use. This function is the OF
+ equivalent of hwspin_lock_request_specific() function, and provides
+ a means for users of the hwspinlock module to request a specific
+ hwspinlock using the phandle of the hwspinlock device.
+ Should be called from a process context (might sleep).
+
int hwspin_lock_free(struct hwspinlock *hwlock);
- free a previously-assigned hwspinlock; returns 0 on success, or an
appropriate error code on failure (e.g. -EINVAL if the hwspinlock
@@ -243,6 +252,23 @@ int hwspinlock_example2(void)
Returns the address of hwspinlock on success, or NULL on error (e.g.
if the hwspinlock is still in use).
+ int of_hwspin_lock_simple_xlate(struct hwspinlock_device *bank,
+ const struct of_phandle_args *hwlock_spec);
+ - is a simple default OF translate helper function that can be plugged in
+ as the underlying vendor-specific implementation's of_xlate ops function.
+ This can be used by implementations that use a single integer argument in
+ the DT node argument specifier, that indicates the hwlock index number.
+ Returns a hwlock index within a bank, or appropriate error code on
+ failure.
+
+ int of_hwspin_lock_get_num_locks(struct device_node *dn);
+ - is a common OF helper function that can be used by some underlying
+ vendor-specific implementations. This can be used by implementations
+ that require and define the number of locks supported within a hwspinlock
+ bank as a device tree node property. This function should be called by
+ needed implementations before registering a hwspinlock device with the
+ core.
+
5. Important structs
struct hwspinlock_device is a device which usually contains a bank
@@ -288,12 +314,14 @@ initialized by the hwspinlock core itself.
6. Implementation callbacks
-There are three possible callbacks defined in 'struct hwspinlock_ops':
+There are four possible callbacks defined in 'struct hwspinlock_ops':
struct hwspinlock_ops {
int (*trylock)(struct hwspinlock *lock);
void (*unlock)(struct hwspinlock *lock);
void (*relax)(struct hwspinlock *lock);
+ int (*of_xlate)(struct hwspinlock_device *bank,
+ const struct of_phandle_args *hwlock_spec);
};
The first two callbacks are mandatory:
@@ -307,3 +335,7 @@ may _not_ sleep.
The ->relax() callback is optional. It is called by hwspinlock core while
spinning on a lock, and can be used by the underlying implementation to force
a delay between two successive invocations of ->trylock(). It may _not_ sleep.
+
+The ->of_xlate() callback is mandatory to support requesting hwlocks through
+device-tree nodes. It is called by hwspinlock core to retrieve the relative
+lock index within a bank from the underlying implementation.
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 48f7866..1e299f7 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -27,6 +27,7 @@
#include <linux/hwspinlock.h>
#include <linux/pm_runtime.h>
#include <linux/mutex.h>
+#include <linux/of.h>
#include "hwspinlock_internal.h"
@@ -262,6 +263,33 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
}
EXPORT_SYMBOL_GPL(__hwspin_unlock);
+/**
+ * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
+ * @dn: device node pointer
+ *
+ * This is an OF helper function that can be called by the underlying
+ * platform-specific implementations, to retrieve the number of locks
+ * present within a hwspinlock device instance. The hwlock-num-locks
+ * DT property may be optional for some platforms, while mandatory for
+ * some others, so this function is typically called only by needed
+ * platform-specific implementations.
+ *
+ * Returns a positive number of locks on success, -ENODEV on generic
+ * failure or an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_num_locks(struct device_node *dn)
+{
+ unsigned int val;
+ int ret = -ENODEV;
+
+ ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
+ if (!ret)
+ ret = val ? val : -ENODEV;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
+
static int hwspin_lock_register_single(struct hwspinlock *hwlock, int id)
{
struct hwspinlock *tmp;
@@ -312,6 +340,31 @@ out:
return hwlock;
}
+/**
+ * of_hwspin_lock_simple_xlate - translate hwlock_spec to return a lock id
+ * @bank: the hwspinlock device bank
+ * @hwlock_spec: hwlock specifier as found in the device tree
+ *
+ * This is a simple translation function, suitable for hwspinlock platform
+ * drivers that only has a lock specifier length of 1.
+ *
+ * Returns a negative value on error, and a relative index of the lock within
+ * a specified bank on success.
+ */
+int of_hwspin_lock_simple_xlate(struct hwspinlock_device *bank,
+ const struct of_phandle_args *hwlock_spec)
+{
+ /* sanity check (these shouldn't happen) */
+ if (WARN_ON(!bank->dev->of_node))
+ return -EINVAL;
+
+ if (WARN_ON(hwlock_spec->args_count != 1))
+ return -EINVAL;
+
+ return hwlock_spec->args[0];
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_simple_xlate);
+
/*
* Add a new hwspinlock device to the global list, keeping the list of
* devices sorted by base order.
@@ -368,7 +421,7 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
int ret = 0, i;
if (!bank || !ops || !dev || !num_locks || !ops->trylock ||
- !ops->unlock) {
+ !ops->unlock || (dev->of_node && !ops->of_xlate)) {
pr_err("invalid parameters\n");
return -EINVAL;
}
@@ -592,6 +645,59 @@ out:
EXPORT_SYMBOL_GPL(hwspin_lock_request_specific);
/**
+ * of_hwspin_lock_request_specific() - request a OF phandle-based specific lock
+ * @np: device node from which to request the specific hwlock
+ * @propname: property name containing hwlock specifier(s)
+ * @index: index of the hwlock
+ *
+ * This function is the OF equivalent of hwspin_lock_request_specific(). This
+ * function provides a means for users of the hwspinlock module to request a
+ * specific hwspinlock using the phandle of the hwspinlock device. The requested
+ * lock number is indexed relative to the hwspinlock device, unlike the
+ * hwspin_lock_request_specific() which is an absolute lock number.
+ *
+ * Returns the address of the assigned hwspinlock, or NULL on error
+ */
+struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
+ const char *propname, int index)
+{
+ struct hwspinlock_device *bank;
+ struct of_phandle_args args;
+ int id;
+ int ret;
+
+ ret = of_parse_phandle_with_args(np, propname, "#hwlock-cells", index,
+ &args);
+ if (ret) {
+ pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n",
+ __func__, np->full_name, index, ret);
+ return NULL;
+ }
+
+ mutex_lock(&hwspinlock_tree_lock);
+ list_for_each_entry(bank, &hwspinlock_devices, list)
+ if (bank->dev->of_node == args.np)
+ break;
+ mutex_unlock(&hwspinlock_tree_lock);
+ if (&bank->list == &hwspinlock_devices) {
+ pr_warn("%s: requested hwspinlock device %s is not registered\n",
+ __func__, args.np->full_name);
+ return NULL;
+ }
+
+ id = bank->ops->of_xlate(bank, &args);
+ if (id < 0 || id >= bank->num_locks) {
+ pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n",
+ __func__, id, bank->num_locks - 1);
+ return NULL;
+ }
+
+ id += bank->base_id;
+ return hwspin_lock_request_specific(id);
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_request_specific);
+
+/**
* hwspin_lock_free() - free a specific hwspinlock
* @hwlock: the specific hwspinlock to free
*
diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index aff560c..5e42613 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -32,11 +32,15 @@ struct hwspinlock_device;
* @relax: optional, platform-specific relax handler, called by hwspinlock
* core while spinning on a lock, between two successive
* invocations of @trylock. may _not_ sleep.
+ * @of_xlate: platform-specific hwlock specifier translate function, to
+ * return the relative index of the lock within a bank
*/
struct hwspinlock_ops {
int (*trylock)(struct hwspinlock *lock);
void (*unlock)(struct hwspinlock *lock);
void (*relax)(struct hwspinlock *lock);
+ int (*of_xlate)(struct hwspinlock_device *bank,
+ const struct of_phandle_args *hwlock_spec);
};
/**
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 3343298..2453057 100644
--- a/include/linux/hwspinlock.h
+++ b/include/linux/hwspinlock.h
@@ -26,6 +26,8 @@
#define HWLOCK_IRQ 0x02 /* Disable interrupts, don't save state */
struct device;
+struct device_node;
+struct of_phandle_args;
struct hwspinlock;
struct hwspinlock_device;
struct hwspinlock_ops;
@@ -60,11 +62,16 @@ struct hwspinlock_pdata {
#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
+int of_hwspin_lock_simple_xlate(struct hwspinlock_device *bank,
+ const struct of_phandle_args *hwlock_spec);
+int of_hwspin_lock_get_num_locks(struct device_node *dn);
int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
const struct hwspinlock_ops *ops, int base_id, int num_locks);
int hwspin_lock_unregister(struct hwspinlock_device *bank);
struct hwspinlock *hwspin_lock_request(void);
struct hwspinlock *hwspin_lock_request_specific(unsigned int id);
+struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
+ const char *propname, int index);
int hwspin_lock_free(struct hwspinlock *hwlock);
int hwspin_lock_get_id(struct hwspinlock *hwlock);
int __hwspin_lock_timeout(struct hwspinlock *, unsigned int, int,
@@ -80,9 +87,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
* code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
* required on a given setup, users will still work.
*
- * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
- * we _do_ want users to fail (no point in registering hwspinlock instances if
- * the framework is not available).
+ * The only exception is hwspin_lock_register/hwspin_lock_unregister and
+ * associated OF helpers, with which we _do_ want users to fail (no point
+ * in registering hwspinlock instances if the framework is not available).
*
* Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
* users. Others, which care, can still check this with IS_ERR.
@@ -97,6 +104,13 @@ static inline struct hwspinlock *hwspin_lock_request_specific(unsigned int id)
return ERR_PTR(-ENODEV);
}
+static inline
+struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
+ const char *propname, int index)
+{
+ return ERR_PTR(-ENODEV);
+}
+
static inline int hwspin_lock_free(struct hwspinlock *hwlock)
{
return 0;
--
1.8.4.3
HwSpinlocks are supported on AM33xx, AM43xx and DRA7xx SoC
device families as well. The IPs are identical to that of
OMAP4/OMAP5, except for the number of locks.
Add a depends on to the above family of SoCs to enable the
build support for OMAP hwspinlock driver for any of the above
SoC configs.
Signed-off-by: Suman Anna <[email protected]>
---
drivers/hwspinlock/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 70637d2..3612cb5 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -10,7 +10,7 @@ menu "Hardware Spinlock drivers"
config HWSPINLOCK_OMAP
tristate "OMAP Hardware Spinlock device"
- depends on ARCH_OMAP4 || SOC_OMAP5
+ depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
select HWSPINLOCK
help
Say y here to support the OMAP Hardware Spinlock device (firstly
--
1.8.4.3
The number of hwspinlocks are determined based on the value read
from the IP block's SYSSTATUS register. However, the module may
not be enabled and clocked, and the read may result in a bus error.
This particular issue is seen rather easily on AM33XX, since the
module wakeup is software controlled, and it is disabled out of
reset. Make sure the module is enabled and clocked before reading
the SYSSTATUS register.
Signed-off-by: Suman Anna <[email protected]>
---
drivers/hwspinlock/omap_hwspinlock.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 9f56fb2..194886e 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
if (!io_base)
return -ENOMEM;
+ /*
+ * make sure the module is enabled and clocked before reading
+ * the module SYSSTATUS register
+ */
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_sync(&pdev->dev);
+
/* Determine number of locks */
i = readl(io_base + SYSSTATUS_OFFSET);
i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
+ /*
+ * runtime PM will make sure the clock of this module is
+ * enabled again iff at least one lock is requested
+ */
+ pm_runtime_put(&pdev->dev);
+
/* one of the four lsb's must be set, and nothing else */
if (hweight_long(i & 0xf) != 1 || i > 8) {
ret = -EINVAL;
@@ -124,12 +137,6 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
hwlock->priv = io_base + LOCK_BASE_OFFSET + sizeof(u32) * i;
- /*
- * runtime PM will make sure the clock of this module is
- * enabled iff at least one lock is requested
- */
- pm_runtime_enable(&pdev->dev);
-
ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops,
base_id, num_locks);
if (ret)
@@ -138,9 +145,9 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
return 0;
reg_fail:
- pm_runtime_disable(&pdev->dev);
kfree(bank);
iounmap_base:
+ pm_runtime_disable(&pdev->dev);
iounmap(io_base);
return ret;
}
--
1.8.4.3
HwSpinlock IP is present only on OMAP4 and other newer SoCs,
which are all device-tree boot only. This patch adds the
base support for parsing the DT nodes, and removes the code
dealing with the traditional platform device instantiation.
Signed-off-by: Suman Anna <[email protected]>
[[email protected]: ack for legacy file removal]
Acked-by: Tony Lindgren <[email protected]>
---
MAINTAINERS | 1 -
arch/arm/mach-omap2/Makefile | 3 --
arch/arm/mach-omap2/hwspinlock.c | 60 ------------------------------------
drivers/hwspinlock/omap_hwspinlock.c | 18 ++++++++---
4 files changed, 14 insertions(+), 68 deletions(-)
delete mode 100644 arch/arm/mach-omap2/hwspinlock.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 31a0462..cc93496 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6148,7 +6148,6 @@ M: Ohad Ben-Cohen <[email protected]>
L: [email protected]
S: Maintained
F: drivers/hwspinlock/omap_hwspinlock.c
-F: arch/arm/mach-omap2/hwspinlock.c
OMAP MMC SUPPORT
M: Jarkko Lavinen <[email protected]>
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index adcef40..015d931 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -289,9 +289,6 @@ obj-y += $(smc91x-m) $(smc91x-y)
smsc911x-$(CONFIG_SMSC911X) := gpmc-smsc911x.o
obj-y += $(smsc911x-m) $(smsc911x-y)
-ifneq ($(CONFIG_HWSPINLOCK_OMAP),)
-obj-y += hwspinlock.o
-endif
emac-$(CONFIG_TI_DAVINCI_EMAC) := am35xx-emac.o
obj-y += $(emac-m) $(emac-y)
diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c
deleted file mode 100644
index ef175ac..0000000
--- a/arch/arm/mach-omap2/hwspinlock.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * OMAP hardware spinlock device initialization
- *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
- *
- * Contact: Simon Que <[email protected]>
- * Hari Kanigeri <[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/kernel.h>
-#include <linux/init.h>
-#include <linux/err.h>
-#include <linux/hwspinlock.h>
-
-#include "soc.h"
-#include "omap_hwmod.h"
-#include "omap_device.h"
-
-static struct hwspinlock_pdata omap_hwspinlock_pdata __initdata = {
- .base_id = 0,
-};
-
-static int __init hwspinlocks_init(void)
-{
- int retval = 0;
- struct omap_hwmod *oh;
- struct platform_device *pdev;
- const char *oh_name = "spinlock";
- const char *dev_name = "omap_hwspinlock";
-
- /*
- * Hwmod lookup will fail in case our platform doesn't support the
- * hardware spinlock module, so it is safe to run this initcall
- * on all omaps
- */
- oh = omap_hwmod_lookup(oh_name);
- if (oh == NULL)
- return -EINVAL;
-
- pdev = omap_device_build(dev_name, 0, oh, &omap_hwspinlock_pdata,
- sizeof(struct hwspinlock_pdata));
- if (IS_ERR(pdev)) {
- pr_err("Can't build omap_device for %s:%s\n", dev_name,
- oh_name);
- retval = PTR_ERR(pdev);
- }
-
- return retval;
-}
-/* early board code might need to reserve specific hwspinlock instances */
-omap_postcore_initcall(hwspinlocks_init);
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 292869c..9f56fb2 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -1,7 +1,7 @@
/*
* OMAP hardware spinlock driver
*
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2014 Texas Instruments Incorporated - http://www.ti.com
*
* Contact: Simon Que <[email protected]>
* Hari Kanigeri <[email protected]>
@@ -27,6 +27,7 @@
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/hwspinlock.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include "hwspinlock_internal.h"
@@ -76,18 +77,20 @@ static const struct hwspinlock_ops omap_hwspinlock_ops = {
.trylock = omap_hwspinlock_trylock,
.unlock = omap_hwspinlock_unlock,
.relax = omap_hwspinlock_relax,
+ .of_xlate = of_hwspin_lock_simple_xlate,
};
static int omap_hwspinlock_probe(struct platform_device *pdev)
{
- struct hwspinlock_pdata *pdata = pdev->dev.platform_data;
+ struct device_node *node = pdev->dev.of_node;
struct hwspinlock_device *bank;
struct hwspinlock *hwlock;
struct resource *res;
void __iomem *io_base;
int num_locks, i, ret;
+ int base_id = 0;
- if (!pdata)
+ if (!node)
return -ENODEV;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -128,7 +131,7 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops,
- pdata->base_id, num_locks);
+ base_id, num_locks);
if (ret)
goto reg_fail;
@@ -161,12 +164,19 @@ static int omap_hwspinlock_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id omap_hwspinlock_of_match[] = {
+ { .compatible = "ti,omap4-hwspinlock", },
+ { /* end */ },
+};
+MODULE_DEVICE_TABLE(of, omap_hwspinlock_of_match);
+
static struct platform_driver omap_hwspinlock_driver = {
.probe = omap_hwspinlock_probe,
.remove = omap_hwspinlock_remove,
.driver = {
.name = "omap_hwspinlock",
.owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(omap_hwspinlock_of_match),
},
};
--
1.8.4.3
HwSpinlock IP is present only on OMAP4 and other newer SoCs,
which are all device-tree boot only. This patch adds the
DT bindings information for OMAP hwspinlock module.
Cc: Rob Herring <[email protected]>
Signed-off-by: Suman Anna <[email protected]>
---
.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
diff --git a/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
new file mode 100644
index 0000000..568eae2
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
@@ -0,0 +1,24 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible: Should be "ti,omap4-hwspinlock" for
+ OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs
+- reg: Contains the hwspinlock module register address space
+ (base address and length)
+- ti,hwmods: Name of the hwmod associated with the hwspinlock device
+- #hwlock-cells: Should be 1. The OMAP hwspinlock users will use a
+ 0-indexed relative hwlock number as the argument
+ specifier value for requesting a specific hwspinlock
+ within a hwspinlock bank.
+
+
+Example:
+
+/* OMAP4 */
+hwspinlock: spinlock@4a0f6000 {
+ compatible = "ti,omap4-hwspinlock";
+ reg = <0x4a0f6000 0x1000>;
+ ti,hwmods = "spinlock";
+ #hwlock-cells = <1>;
+};
--
1.8.4.3
On Mon, Jan 13, 2014 at 06:19:23PM -0600, Suman Anna wrote:
> The number of hwspinlocks are determined based on the value read
> from the IP block's SYSSTATUS register. However, the module may
> not be enabled and clocked, and the read may result in a bus error.
>
> This particular issue is seen rather easily on AM33XX, since the
> module wakeup is software controlled, and it is disabled out of
> reset. Make sure the module is enabled and clocked before reading
> the SYSSTATUS register.
>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/hwspinlock/omap_hwspinlock.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
> index 9f56fb2..194886e 100644
> --- a/drivers/hwspinlock/omap_hwspinlock.c
> +++ b/drivers/hwspinlock/omap_hwspinlock.c
> @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
> if (!io_base)
> return -ENOMEM;
>
> + /*
> + * make sure the module is enabled and clocked before reading
> + * the module SYSSTATUS register
> + */
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_get_sync(&pdev->dev);
> +
> /* Determine number of locks */
> i = readl(io_base + SYSSTATUS_OFFSET);
> i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
>
> + /*
> + * runtime PM will make sure the clock of this module is
> + * enabled again iff at least one lock is requested
> + */
> + pm_runtime_put(&pdev->dev);
there is a small race here (which was already present previously) where
you could return from probe() in a failure case before your PM runtime
put request then you would disable pm_runtime while the device was still
alive.
--
balbi
On Mon, Jan 13, 2014 at 06:19:24PM -0600, Suman Anna wrote:
> HwSpinlocks are supported on AM33xx, AM43xx and DRA7xx SoC
> device families as well. The IPs are identical to that of
> OMAP4/OMAP5, except for the number of locks.
>
> Add a depends on to the above family of SoCs to enable the
> build support for OMAP hwspinlock driver for any of the above
> SoC configs.
>
> Signed-off-by: Suman Anna <[email protected]>
> ---
> drivers/hwspinlock/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> index 70637d2..3612cb5 100644
> --- a/drivers/hwspinlock/Kconfig
> +++ b/drivers/hwspinlock/Kconfig
> @@ -10,7 +10,7 @@ menu "Hardware Spinlock drivers"
>
> config HWSPINLOCK_OMAP
> tristate "OMAP Hardware Spinlock device"
> - depends on ARCH_OMAP4 || SOC_OMAP5
> + depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
how about just using ARCH_OMAP2PLUS ?
--
balbi
On Mon, Jan 13, 2014 at 06:19:17PM -0600, Suman Anna wrote:
> This is an updated series mainly addressing Mark Rutland's comments
> about hwlock specifier being always one-cell. The series adds the
> support for #hwlock-cells property and adds a simple default OF
> translate function.
>
> The DTS patches from previous series have already been merged, and
> needs this property to be added. This is handled in a separate series
> that only deals with OMAP hwspinlock DTS patches.
>
> The series, along with the DTS patches, is tested on top of v3.13-rc8
> plus Tero's v13 clock DT series and Tony's 3.14 staged branches. The
> validation on OMAP5, DRA7, AM437 requires Tero's series with couple of
> additional base patches for AM43xx. AM43xx functionality needs a hwmod
> fix [1] for creating the associated omap_device as well.
>
> The validation logs on all the applicable OMAP SoCs are at:
> OMAP4 - http://paste2.org/YJ7ZwG80
> OMAP5 - http://paste2.org/c6vO96b9
> DRA7 - http://paste2.org/tHvxN439
> AM33x - http://paste2.org/AjCv0U4t
> AM43x - http://paste2.org/2AKIPa55
I build tested your 3.13-rc8-v4 branch with the same script I used to
build CCF series and hspinlock built without any problems ;-)
Acked-by: Felipe Balbi <[email protected]>
--
balbi
Hi again,
On Tue, Jan 14, 2014 at 07:10:52AM -0600, Felipe Balbi wrote:
> > diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
> > index 9f56fb2..194886e 100644
> > --- a/drivers/hwspinlock/omap_hwspinlock.c
> > +++ b/drivers/hwspinlock/omap_hwspinlock.c
> > @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
> > if (!io_base)
> > return -ENOMEM;
> >
> > + /*
> > + * make sure the module is enabled and clocked before reading
> > + * the module SYSSTATUS register
> > + */
> > + pm_runtime_enable(&pdev->dev);
> > + pm_runtime_get_sync(&pdev->dev);
another thing, you need to check return of pm_runtime_get_sync()
--
balbi
Felipe,
On 01/14/2014 07:12 AM, Felipe Balbi wrote:
> On Mon, Jan 13, 2014 at 06:19:24PM -0600, Suman Anna wrote:
>> HwSpinlocks are supported on AM33xx, AM43xx and DRA7xx SoC
>> device families as well. The IPs are identical to that of
>> OMAP4/OMAP5, except for the number of locks.
>>
>> Add a depends on to the above family of SoCs to enable the
>> build support for OMAP hwspinlock driver for any of the above
>> SoC configs.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>> ---
>> drivers/hwspinlock/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
>> index 70637d2..3612cb5 100644
>> --- a/drivers/hwspinlock/Kconfig
>> +++ b/drivers/hwspinlock/Kconfig
>> @@ -10,7 +10,7 @@ menu "Hardware Spinlock drivers"
>>
>> config HWSPINLOCK_OMAP
>> tristate "OMAP Hardware Spinlock device"
>> - depends on ARCH_OMAP4 || SOC_OMAP5
>> + depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
>
> how about just using ARCH_OMAP2PLUS ?
We do not want the driver to build in OMAP2-only and/or OMAP3-only
configurations, on which the hwspinlock IP is not even present.
regards
Suman
Felipe,
On 01/14/2014 08:04 AM, Felipe Balbi wrote:
> Hi again,
>
> On Tue, Jan 14, 2014 at 07:10:52AM -0600, Felipe Balbi wrote:
>>> diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
>>> index 9f56fb2..194886e 100644
>>> --- a/drivers/hwspinlock/omap_hwspinlock.c
>>> +++ b/drivers/hwspinlock/omap_hwspinlock.c
>>> @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
>>> if (!io_base)
>>> return -ENOMEM;
>>>
>>> + /*
>>> + * make sure the module is enabled and clocked before reading
>>> + * the module SYSSTATUS register
>>> + */
>>> + pm_runtime_enable(&pdev->dev);
>>> + pm_runtime_get_sync(&pdev->dev);
>
> another thing, you need to check return of pm_runtime_get_sync()
OK, let me check this and your other comment, and the fix is probably a
separate patch.
regards
Suman
On Tue, Jan 14, 2014 at 10:51:31AM -0600, Anna, Suman wrote:
> Felipe,
>
> On 01/14/2014 07:12 AM, Felipe Balbi wrote:
> >On Mon, Jan 13, 2014 at 06:19:24PM -0600, Suman Anna wrote:
> >>HwSpinlocks are supported on AM33xx, AM43xx and DRA7xx SoC
> >>device families as well. The IPs are identical to that of
> >>OMAP4/OMAP5, except for the number of locks.
> >>
> >>Add a depends on to the above family of SoCs to enable the
> >>build support for OMAP hwspinlock driver for any of the above
> >>SoC configs.
> >>
> >>Signed-off-by: Suman Anna <[email protected]>
> >>---
> >> drivers/hwspinlock/Kconfig | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
> >>index 70637d2..3612cb5 100644
> >>--- a/drivers/hwspinlock/Kconfig
> >>+++ b/drivers/hwspinlock/Kconfig
> >>@@ -10,7 +10,7 @@ menu "Hardware Spinlock drivers"
> >>
> >> config HWSPINLOCK_OMAP
> >> tristate "OMAP Hardware Spinlock device"
> >>- depends on ARCH_OMAP4 || SOC_OMAP5
> >>+ depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
> >
> >how about just using ARCH_OMAP2PLUS ?
>
> We do not want the driver to build in OMAP2-only and/or OMAP3-only
> configurations, on which the hwspinlock IP is not even present.
It won't be enabled by default, will it ? You're just saying that it
_can_ be enabled. In fact, I would go one step further and use:
depends on ARCH_OMAP2PLUS || COMPILE_TEST
your choice though ;-)
--
balbi
On 01/14/2014 11:29 AM, Felipe Balbi wrote:
> On Tue, Jan 14, 2014 at 10:51:31AM -0600, Anna, Suman wrote:
>> Felipe,
>>
>> On 01/14/2014 07:12 AM, Felipe Balbi wrote:
>>> On Mon, Jan 13, 2014 at 06:19:24PM -0600, Suman Anna wrote:
>>>> HwSpinlocks are supported on AM33xx, AM43xx and DRA7xx SoC
>>>> device families as well. The IPs are identical to that of
>>>> OMAP4/OMAP5, except for the number of locks.
>>>>
>>>> Add a depends on to the above family of SoCs to enable the
>>>> build support for OMAP hwspinlock driver for any of the above
>>>> SoC configs.
>>>>
>>>> Signed-off-by: Suman Anna <[email protected]>
>>>> ---
>>>> drivers/hwspinlock/Kconfig | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
>>>> index 70637d2..3612cb5 100644
>>>> --- a/drivers/hwspinlock/Kconfig
>>>> +++ b/drivers/hwspinlock/Kconfig
>>>> @@ -10,7 +10,7 @@ menu "Hardware Spinlock drivers"
>>>>
>>>> config HWSPINLOCK_OMAP
>>>> tristate "OMAP Hardware Spinlock device"
>>>> - depends on ARCH_OMAP4 || SOC_OMAP5
>>>> + depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX || SOC_AM33XX || SOC_AM43XX
>>>
>>> how about just using ARCH_OMAP2PLUS ?
>>
>> We do not want the driver to build in OMAP2-only and/or OMAP3-only
>> configurations, on which the hwspinlock IP is not even present.
>
> It won't be enabled by default, will it ? You're just saying that it
> _can_ be enabled.
>
Yes, that's correct. The menuconfig will not even show this driver at
present on OMAP2-only and/or OMAP3-only configs. I would prefer to keep
it that way.
regards
Suman
The number of hwspinlocks are determined based on the value read
from the IP block's SYSSTATUS register. However, the module may
not be enabled and clocked, and the read may result in a bus error.
This particular issue is seen rather easily on AM33XX, since the
module wakeup is software controlled, and it is disabled out of
reset. Make sure the module is enabled and clocked before reading
the SYSSTATUS register.
Signed-off-by: Suman Anna <[email protected]>
---
drivers/hwspinlock/omap_hwspinlock.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 9f56fb2..7764291 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -101,10 +101,29 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
if (!io_base)
return -ENOMEM;
+ /*
+ * make sure the module is enabled and clocked before reading
+ * the module SYSSTATUS register
+ */
+ pm_runtime_enable(&pdev->dev);
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ goto iounmap_base;
+ }
+
/* Determine number of locks */
i = readl(io_base + SYSSTATUS_OFFSET);
i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
+ /*
+ * runtime PM will make sure the clock of this module is
+ * enabled again iff at least one lock is requested
+ */
+ ret = pm_runtime_put_sync(&pdev->dev);
+ if (ret < 0)
+ goto iounmap_base;
+
/* one of the four lsb's must be set, and nothing else */
if (hweight_long(i & 0xf) != 1 || i > 8) {
ret = -EINVAL;
@@ -124,12 +143,6 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
hwlock->priv = io_base + LOCK_BASE_OFFSET + sizeof(u32) * i;
- /*
- * runtime PM will make sure the clock of this module is
- * enabled iff at least one lock is requested
- */
- pm_runtime_enable(&pdev->dev);
-
ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops,
base_id, num_locks);
if (ret)
@@ -138,9 +151,9 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
return 0;
reg_fail:
- pm_runtime_disable(&pdev->dev);
kfree(bank);
iounmap_base:
+ pm_runtime_disable(&pdev->dev);
iounmap(io_base);
return ret;
}
--
1.8.4.3
Felipe,
>
> On 01/14/2014 08:04 AM, Felipe Balbi wrote:
>> Hi again,
>>
>> On Tue, Jan 14, 2014 at 07:10:52AM -0600, Felipe Balbi wrote:
>>>> diff --git a/drivers/hwspinlock/omap_hwspinlock.c
>>>> b/drivers/hwspinlock/omap_hwspinlock.c
>>>> index 9f56fb2..194886e 100644
>>>> --- a/drivers/hwspinlock/omap_hwspinlock.c
>>>> +++ b/drivers/hwspinlock/omap_hwspinlock.c
>>>> @@ -101,10 +101,23 @@ static int omap_hwspinlock_probe(struct
>>>> platform_device *pdev)
>>>> if (!io_base)
>>>> return -ENOMEM;
>>>>
>>>> + /*
>>>> + * make sure the module is enabled and clocked before reading
>>>> + * the module SYSSTATUS register
>>>> + */
>>>> + pm_runtime_enable(&pdev->dev);
>>>> + pm_runtime_get_sync(&pdev->dev);
>>
>> another thing, you need to check return of pm_runtime_get_sync()
>
> OK, let me check this and your other comment, and the fix is probably a
> separate patch.
>
I realized the changes relevant to your comments were introduced in this
patch, so just refreshed the patch with fixes instead of doing a
separate patch. I didn't do a v5 just for these change, and will do so
if there are more comments on the DT adaptation.
regards
Suman
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <[email protected]> wrote:
> This patch adds three new OF helper functions to use/request
> locks from a hwspinlock device instantiated through a
> device-tree blob.
Nice, I ran in to the problem of needing a probe deferral on a
hwspinlock earlier this week so I implemented this yesterday...then I
got a pointer to your series.
[snip]
> /**
> + * of_hwspin_lock_request_specific() - request a OF phandle-based specific lock
> + * @np: device node from which to request the specific hwlock
> + * @propname: property name containing hwlock specifier(s)
> + * @index: index of the hwlock
> + *
> + * This function is the OF equivalent of hwspin_lock_request_specific(). This
> + * function provides a means for users of the hwspinlock module to request a
> + * specific hwspinlock using the phandle of the hwspinlock device. The requested
> + * lock number is indexed relative to the hwspinlock device, unlike the
> + * hwspin_lock_request_specific() which is an absolute lock number.
> + *
> + * Returns the address of the assigned hwspinlock, or NULL on error
> + */
> +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
> + const char *propname, int index)
> +{
> + struct hwspinlock_device *bank;
> + struct of_phandle_args args;
> + int id;
> + int ret;
> +
> + ret = of_parse_phandle_with_args(np, propname, "#hwlock-cells", index,
> + &args);
> + if (ret) {
> + pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n",
> + __func__, np->full_name, index, ret);
> + return NULL;
> + }
of_parse_phandle_with_args() already does pr_err if it can't find the
phandle and on some of the issues related to arguments. So please
remove this pr_warn().
It seems to be standard practice to pass the error value back to the
consumer, so you should
return ERR_PTR(ret); here instead of the NULL...
> +
> + mutex_lock(&hwspinlock_tree_lock);
> + list_for_each_entry(bank, &hwspinlock_devices, list)
> + if (bank->dev->of_node == args.np)
> + break;
> + mutex_unlock(&hwspinlock_tree_lock);
> + if (&bank->list == &hwspinlock_devices) {
> + pr_warn("%s: requested hwspinlock device %s is not registered\n",
> + __func__, args.np->full_name);
> + return NULL;
...especially since you want the consumer to have the ability to
identify this error. Here you should
return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this
lock is not _yet_ registered, but will be in the future.
You should remove this pr_warn as well. The standard use of this
function would be in a probe() and just returning this error value
from that probe will give you a line in the log indicating that this
was in fact the issue.
> + }
> +
> + id = bank->ops->of_xlate(bank, &args);
> + if (id < 0 || id >= bank->num_locks) {
> + pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n",
> + __func__, id, bank->num_locks - 1);
> + return NULL;
Please return ERR_PTR(-EINVAL); here.
Looking forward to your next spin, as I will actually use this interface :)
Regards,
Bjorn
Bjorn,
On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
> On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <[email protected]> wrote:
>> This patch adds three new OF helper functions to use/request
>> locks from a hwspinlock device instantiated through a
>> device-tree blob.
>
> Nice, I ran in to the problem of needing a probe deferral on a
> hwspinlock earlier this week so I implemented this yesterday...then I
> got a pointer to your series.
>
> [snip]
>> /**
>> + * of_hwspin_lock_request_specific() - request a OF phandle-based specific lock
>> + * @np: device node from which to request the specific hwlock
>> + * @propname: property name containing hwlock specifier(s)
>> + * @index: index of the hwlock
>> + *
>> + * This function is the OF equivalent of hwspin_lock_request_specific(). This
>> + * function provides a means for users of the hwspinlock module to request a
>> + * specific hwspinlock using the phandle of the hwspinlock device. The requested
>> + * lock number is indexed relative to the hwspinlock device, unlike the
>> + * hwspin_lock_request_specific() which is an absolute lock number.
>> + *
>> + * Returns the address of the assigned hwspinlock, or NULL on error
>> + */
>> +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
>> + const char *propname, int index)
>> +{
>> + struct hwspinlock_device *bank;
>> + struct of_phandle_args args;
>> + int id;
>> + int ret;
>> +
>> + ret = of_parse_phandle_with_args(np, propname, "#hwlock-cells", index,
>> + &args);
>> + if (ret) {
>> + pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n",
>> + __func__, np->full_name, index, ret);
>> + return NULL;
>> + }
>
> of_parse_phandle_with_args() already does pr_err if it can't find the
> phandle and on some of the issues related to arguments. So please
> remove this pr_warn().
Yes, I will clean this up.
>
> It seems to be standard practice to pass the error value back to the
> consumer, so you should
> return ERR_PTR(ret); here instead of the NULL...
I have modelled the return values in this function based on the return
values in the existing hwspin_lock_request interfaces. I would need to
change those functions as well.
Ohad,
Do you have any objections to the return code convention change? I agree
with Bjorn on the changes. If you are ok, then I will add a separate
patch for the existing functions and revise this patch as well.
>
>> +
>> + mutex_lock(&hwspinlock_tree_lock);
>> + list_for_each_entry(bank, &hwspinlock_devices, list)
>> + if (bank->dev->of_node == args.np)
>> + break;
>> + mutex_unlock(&hwspinlock_tree_lock);
>> + if (&bank->list == &hwspinlock_devices) {
>> + pr_warn("%s: requested hwspinlock device %s is not registered\n",
>> + __func__, args.np->full_name);
>> + return NULL;
>
> ...especially since you want the consumer to have the ability to
> identify this error. Here you should
> return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this
> lock is not _yet_ registered, but will be in the future.
>
> You should remove this pr_warn as well. The standard use of this
> function would be in a probe() and just returning this error value
> from that probe will give you a line in the log indicating that this
> was in fact the issue.
OK.
>
>> + }
>> +
>> + id = bank->ops->of_xlate(bank, &args);
>> + if (id < 0 || id >= bank->num_locks) {
>> + pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n",
>> + __func__, id, bank->num_locks - 1);
>> + return NULL;
>
> Please return ERR_PTR(-EINVAL); here.
OK, will change this based on Ohad's ack/nack.
>
> Looking forward to your next spin, as I will actually use this interface :)
Thanks for your comments. I will wait to see if there are any additional
comments before I refresh the series later this week.
regards
Suman
Mark,
On 01/13/2014 06:19 PM, Suman Anna wrote:
> Hi,
>
> This is an updated series mainly addressing Mark Rutland's comments
> about hwlock specifier being always one-cell. The series adds the
> support for #hwlock-cells property and adds a simple default OF
> translate function.
>
> The DTS patches from previous series have already been merged, and
> needs this property to be added. This is handled in a separate series
> that only deals with OMAP hwspinlock DTS patches.
>
> The series, along with the DTS patches, is tested on top of v3.13-rc8
> plus Tero's v13 clock DT series and Tony's 3.14 staged branches. The
> validation on OMAP5, DRA7, AM437 requires Tero's series with couple of
> additional base patches for AM43xx. AM43xx functionality needs a hwmod
> fix [1] for creating the associated omap_device as well.
>
Can you please take a look at this series and give your ack on the
bindings if you do not have any further comments? The only comments so
far are from Bjorn on the OF helpers.
regards
Suman
Mark, Ohad,
On 02/10/2014 01:27 PM, Suman Anna wrote:
> Mark,
>
> On 01/13/2014 06:19 PM, Suman Anna wrote:
>> Hi,
>>
>> This is an updated series mainly addressing Mark Rutland's comments
>> about hwlock specifier being always one-cell. The series adds the
>> support for #hwlock-cells property and adds a simple default OF
>> translate function.
>>
>> The DTS patches from previous series have already been merged, and
>> needs this property to be added. This is handled in a separate series
>> that only deals with OMAP hwspinlock DTS patches.
>>
>> The series, along with the DTS patches, is tested on top of v3.13-rc8
>> plus Tero's v13 clock DT series and Tony's 3.14 staged branches. The
>> validation on OMAP5, DRA7, AM437 requires Tero's series with couple of
>> additional base patches for AM43xx. AM43xx functionality needs a hwmod
>> fix [1] for creating the associated omap_device as well.
>>
>
> Can you please take a look at this series and give your ack on the
> bindings if you do not have any further comments? The only comments so
> far are from Bjorn on the OF helpers.
>
Gentle reminder, can you provide your acks/comments?
regards
Suman
On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna <[email protected]> wrote:
> I am yet to receive any comments on v6, but that series should address
> both your need for a probe deferral and Ohad's request to not change any
> return types. Please give it a try and let me know if you have any comments.
Guys,
Just to let you know we have several holidays around here these days,
and I intend to review all pending patches immediately soon after.
Thanks,
Ohad.
Hi Ohad,
On 10/06/2014 04:44 AM, Ohad Ben-Cohen wrote:
> On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna <[email protected]> wrote:
>> I am yet to receive any comments on v6, but that series should address
>> both your need for a probe deferral and Ohad's request to not change any
>> return types. Please give it a try and let me know if you have any comments.
>
> Guys,
>
> Just to let you know we have several holidays around here these days,
> and I intend to review all pending patches immediately soon after.
>
Ping on this. Can you review the latest series v6 [1] and pick it up for
3.19? The MSM spinlock driver is also blocked/dependent on that series.
regards
Suman
[1] http://marc.info/?l=linux-arm-kernel&m=141055365513902&w=2
Hi Suman,
On Thu, Nov 6, 2014 at 8:24 PM, Suman Anna <[email protected]> wrote:
> Ping on this. Can you review the latest series v6 [1] and pick it up for
> 3.19? The MSM spinlock driver is also blocked/dependent on that series.
Sure, it's on my mind, I hope I'll get to it next week.
Thanks,
Ohad.