* [PATCHv4 1/7] Documentation: dt: add common bindings for hwspinlock
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
@ 2014-01-14 0:19 ` Suman Anna
2014-01-14 0:19 ` [PATCHv4 2/7] Documentation: dt: add the omap hwspinlock bindings document Suman Anna
` (6 subsequent siblings)
7 siblings, 0 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel, Suman Anna, Rob Herring
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 <robh+dt@kernel.org>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
.../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
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCHv4 2/7] Documentation: dt: add the omap hwspinlock bindings document
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
2014-01-14 0:19 ` [PATCHv4 1/7] Documentation: dt: add common bindings for hwspinlock Suman Anna
@ 2014-01-14 0:19 ` Suman Anna
2014-01-14 0:19 ` [PATCHv4 3/7] hwspinlock/core: maintain a list of registered hwspinlock banks Suman Anna
` (5 subsequent siblings)
7 siblings, 0 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel, Suman Anna, Rob Herring
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 <robh+dt@kernel.org>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
.../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
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCHv4 3/7] hwspinlock/core: maintain a list of registered hwspinlock banks
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
2014-01-14 0:19 ` [PATCHv4 1/7] Documentation: dt: add common bindings for hwspinlock Suman Anna
2014-01-14 0:19 ` [PATCHv4 2/7] Documentation: dt: add the omap hwspinlock bindings document Suman Anna
@ 2014-01-14 0:19 ` Suman Anna
2014-01-14 0:19 ` [PATCHv4 4/7] hwspinlock/core: add common OF helpers Suman Anna
` (4 subsequent siblings)
7 siblings, 0 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel, Suman Anna
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 <s-anna@ti.com>
---
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
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
` (2 preceding siblings ...)
2014-01-14 0:19 ` [PATCHv4 3/7] hwspinlock/core: maintain a list of registered hwspinlock banks Suman Anna
@ 2014-01-14 0:19 ` Suman Anna
2014-02-07 22:49 ` Bjorn Andersson
2014-09-26 14:40 ` Bjorn Andersson
[not found] ` <1389658764-39199-1-git-send-email-s-anna-l0cyMroinI0@public.gmane.org>
` (3 subsequent siblings)
7 siblings, 2 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel, Suman Anna
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 <s-anna@ti.com>
---
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
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-01-14 0:19 ` [PATCHv4 4/7] hwspinlock/core: add common OF helpers Suman Anna
@ 2014-02-07 22:49 ` Bjorn Andersson
2014-02-10 19:14 ` Suman Anna
2014-09-26 14:40 ` Bjorn Andersson
1 sibling, 1 reply; 44+ messages in thread
From: Bjorn Andersson @ 2014-02-07 22:49 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> 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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-02-07 22:49 ` Bjorn Andersson
@ 2014-02-10 19:14 ` Suman Anna
2014-03-02 5:14 ` Ohad Ben-Cohen
0 siblings, 1 reply; 44+ messages in thread
From: Suman Anna @ 2014-02-10 19:14 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen
Cc: Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Bjorn,
On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
> On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> 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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-02-10 19:14 ` Suman Anna
@ 2014-03-02 5:14 ` Ohad Ben-Cohen
2014-03-02 20:19 ` Bjorn Andersson
0 siblings, 1 reply; 44+ messages in thread
From: Ohad Ben-Cohen @ 2014-03-02 5:14 UTC (permalink / raw)
To: Suman Anna
Cc: Bjorn Andersson, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Grant Likely
On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote:
> On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
>> 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?
Unless strictly needed, I prefer we don't switch to the ERR_PTR code
convention, as it reduces code readability and increases chances of
user bugs.
In our case, switching to ERR_PTR and friends seems only to optimize a
few error paths, and I'm not sure it's a big win over simplicity.
Thanks,
Ohad.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-03-02 5:14 ` Ohad Ben-Cohen
@ 2014-03-02 20:19 ` Bjorn Andersson
2014-03-03 18:46 ` Suman Anna
` (2 more replies)
0 siblings, 3 replies; 44+ messages in thread
From: Bjorn Andersson @ 2014-03-02 20:19 UTC (permalink / raw)
To: Ohad Ben-Cohen
Cc: Suman Anna, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Grant Likely
On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote:
>> On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
>>> 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?
>
> Unless strictly needed, I prefer we don't switch to the ERR_PTR code
> convention, as it reduces code readability and increases chances of
> user bugs.
>
> In our case, switching to ERR_PTR and friends seems only to optimize a
> few error paths, and I'm not sure it's a big win over simplicity.
When introducing the ability to reference a hwspin lock via a phandle
in device tree it makes a big difference to be able to differ between
the case of "initialization failed" or "device not yet probed"; so
that the client knows if it should fail or retry later.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-03-02 20:19 ` Bjorn Andersson
@ 2014-03-03 18:46 ` Suman Anna
[not found] ` <CAJAp7Ohf43hbKatCwS5Y1+OfEkJYWOkuhZhW-E_=t_9mfM+UaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-03-14 13:12 ` Ohad Ben-Cohen
2 siblings, 0 replies; 44+ messages in thread
From: Suman Anna @ 2014-03-03 18:46 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen
Cc: Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Grant Likely
Ohad,
On 03/02/2014 02:19 PM, Bjorn Andersson wrote:
> On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote:
>>> On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
>>>> 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?
>>
>> Unless strictly needed, I prefer we don't switch to the ERR_PTR code
>> convention, as it reduces code readability and increases chances of
>> user bugs.
>>
From a current user/client perspectives, I didn't find any clients of
hwspinlock within the kernel. So, this is probably the right time to
change the return code convention.
>> In our case, switching to ERR_PTR and friends seems only to optimize a
>> few error paths, and I'm not sure it's a big win over simplicity.
The usage on the clients will also not become too complicated. The only
change on the clients is mostly the base error check change from if
(!hwlock) to if (IS_ERR(hwlock)).
regards
Suman
> When introducing the ability to reference a hwspin lock via a phandle
> in device tree it makes a big difference to be able to differ between
> the case of "initialization failed" or "device not yet probed"; so
> that the client knows if it should fail or retry later.
>
> Regards,
> Bjorn
>
^ permalink raw reply [flat|nested] 44+ messages in thread[parent not found: <CAJAp7Ohf43hbKatCwS5Y1+OfEkJYWOkuhZhW-E_=t_9mfM+UaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
[not found] ` <CAJAp7Ohf43hbKatCwS5Y1+OfEkJYWOkuhZhW-E_=t_9mfM+UaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2014-03-04 17:38 ` Suman Anna
[not found] ` <53160F8F.9060405-l0cyMroinI0@public.gmane.org>
0 siblings, 1 reply; 44+ messages in thread
From: Suman Anna @ 2014-03-04 17:38 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen
Cc: Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Grant Likely
Hi Ohad,
On 03/02/2014 02:19 PM, Bjorn Andersson wrote:
> On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad-Ix1uc/W3ht7QT0dZR+AlfA@public.gmane.org> wrote:
>> On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna-l0cyMroinI0@public.gmane.org> wrote:
>>> On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
>>>> 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?
>>
>> Unless strictly needed, I prefer we don't switch to the ERR_PTR code
>> convention, as it reduces code readability and increases chances of
>> user bugs.
>>
>> In our case, switching to ERR_PTR and friends seems only to optimize a
>> few error paths, and I'm not sure it's a big win over simplicity.
>
> When introducing the ability to reference a hwspin lock via a phandle
> in device tree it makes a big difference to be able to differ between
> the case of "initialization failed" or "device not yet probed"; so
> that the client knows if it should fail or retry later.
>
Can you confirm the changes you want me to make, so that I can refresh
and post a v5 for 3.15?
regards
Suman
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-03-02 20:19 ` Bjorn Andersson
2014-03-03 18:46 ` Suman Anna
[not found] ` <CAJAp7Ohf43hbKatCwS5Y1+OfEkJYWOkuhZhW-E_=t_9mfM+UaA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2014-03-14 13:12 ` Ohad Ben-Cohen
2014-03-14 15:23 ` Josh Cartwright
2 siblings, 1 reply; 44+ messages in thread
From: Ohad Ben-Cohen @ 2014-03-14 13:12 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Suman Anna, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Grant Likely
On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> When introducing the ability to reference a hwspin lock via a phandle
> in device tree it makes a big difference to be able to differ between
> the case of "initialization failed" or "device not yet probed"; so
> that the client knows if it should fail or retry later.
I'm not convinced.
The only advantage this brings is to avoid retrying in case a fatal
error has occurred. Such fatal errors are extremely rare, and when
they show up - extremely painful, and I suspect that optimizing them
wouldn't be a big win.
OTOH, keeping the code easier to read and less error prone is a big
win. I prefer we keep it simple for now.
Thanks,
Ohad.
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-03-14 13:12 ` Ohad Ben-Cohen
@ 2014-03-14 15:23 ` Josh Cartwright
2014-03-15 17:32 ` Ohad Ben-Cohen
0 siblings, 1 reply; 44+ messages in thread
From: Josh Cartwright @ 2014-03-14 15:23 UTC (permalink / raw)
To: Ohad Ben-Cohen
Cc: Bjorn Andersson, Mark Rutland, devicetree@vger.kernel.org,
Suman Anna, Tony Lindgren, linux-kernel@vger.kernel.org,
Grant Likely, Kumar Gala, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
On Fri, Mar 14, 2014 at 03:12:26PM +0200, Ohad Ben-Cohen wrote:
> On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson <bjorn@kryo.se> wrote:
> > When introducing the ability to reference a hwspin lock via a phandle
> > in device tree it makes a big difference to be able to differ between
> > the case of "initialization failed" or "device not yet probed"; so
> > that the client knows if it should fail or retry later.
>
> I'm not convinced.
>
> The only advantage this brings is to avoid retrying in case a fatal
> error has occurred. Such fatal errors are extremely rare, and when
> they show up - extremely painful, and I suspect that optimizing them
> wouldn't be a big win.
So, are you suggesting that because fatal errors should be "extremely
rare", a consuming driver should just assume that if NULL is returned
from a hwspin_lock_request*() function that it was the "device not yet
probed" case that was hit?
Note that having the consumer/hwspinlock device relationship modeled in
devicetree introduces more potential failure cases...
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-03-14 15:23 ` Josh Cartwright
@ 2014-03-15 17:32 ` Ohad Ben-Cohen
0 siblings, 0 replies; 44+ messages in thread
From: Ohad Ben-Cohen @ 2014-03-15 17:32 UTC (permalink / raw)
To: Josh Cartwright
Cc: Bjorn Andersson, Mark Rutland, devicetree@vger.kernel.org,
Suman Anna, Tony Lindgren, linux-kernel@vger.kernel.org,
Grant Likely, Kumar Gala, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
On Fri, Mar 14, 2014 at 5:23 PM, Josh Cartwright <joshc@codeaurora.org> wrote:
> So, are you suggesting that because fatal errors should be "extremely
> rare", a consuming driver should just assume that if NULL is returned
> from a hwspin_lock_request*() function that it was the "device not yet
> probed" case that was hit?
No - it's not the scarcity, it's the severity.
The error path that will be optimized here is an invalid id. If this
happens, the consumer will crash and burn, and I'm not sure that
slightly optimizing his death is very interesting?
BTW the hwspinlock core once did use ERR_PTR and friends, and it was
changed due to convincing arguments against that methodology on this
mailing list. We can change it back but we need a strong(er) case.
> Note that having the consumer/hwspinlock device relationship modeled in
> devicetree introduces more potential failure cases...
Yeah. Even the error above, presumed to be EPROBE_DEFER, may be a
symptom of some other fatal error that occurred, and we can't be sure
that a future request will surely be satisfied. So before we bloat our
code, I suggest that we wait for consumers to show up and see if
there's real benefit.
Thanks,
Ohad.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-01-14 0:19 ` [PATCHv4 4/7] hwspinlock/core: add common OF helpers Suman Anna
2014-02-07 22:49 ` Bjorn Andersson
@ 2014-09-26 14:40 ` Bjorn Andersson
2014-09-26 16:25 ` Suman Anna
1 sibling, 1 reply; 44+ messages in thread
From: Bjorn Andersson @ 2014-09-26 14:40 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote:
> This patch adds three new OF helper functions to use/request
> locks from a hwspinlock device instantiated through a
> device-tree blob.
>
Hi Ohad, Suman
I'm about to send out some patches that depends on this functionality,
how do we move forward?
I still think it's wrong to not return -EPROBE_DEFER, but I much
rather have the code returning NULL than not having it in the tree (we
can always argue about it later...).
@Suman, do you remember if there was any other comments on the patch?
@Ohad, do you object merging Suman's patch in it's current form? I
think it should still apply cleanly.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
2014-09-26 14:40 ` Bjorn Andersson
@ 2014-09-26 16:25 ` Suman Anna
[not found] ` <5425938C.6070007-l0cyMroinI0@public.gmane.org>
0 siblings, 1 reply; 44+ messages in thread
From: Suman Anna @ 2014-09-26 16:25 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Hi Bjorn,
On 09/26/2014 09:40 AM, Bjorn Andersson wrote:
> On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote:
>> This patch adds three new OF helper functions to use/request
>> locks from a hwspinlock device instantiated through a
>> device-tree blob.
>>
>
> Hi Ohad, Suman
>
> I'm about to send out some patches that depends on this functionality,
> how do we move forward?
>
> I still think it's wrong to not return -EPROBE_DEFER, but I much
> rather have the code returning NULL than not having it in the tree (we
> can always argue about it later...).
>
> @Suman, do you remember if there was any other comments on the patch?
I have posted two further revisions of this series, the latest is v6
[1]. I added additional patches in v5 that added the concept of reserved
locks, and I have posted them as a separate RFC [2] for v6 so as to not
block the core DT support.
In anycase, the latest v6 version does not define the
of_hwspin_lock_request_specific() function anymore, and it is replaced
with of_hwspin_lock_get_id() function, based on Ohad's review comments
on v5, and I did add the support for -EPROBE_DEFER in this API, without
changing any of the existing return code conventions.
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.
regards
Suman
[1] http://marc.info/?l=linux-arm-kernel&m=141055365513902&w=2
[2] http://marc.info/?l=linux-arm-kernel&m=141055554214657&w=2
>
> @Ohad, do you object merging Suman's patch in it's current form? I
> think it should still apply cleanly.
>
> Regards,
> Bjorn
>
^ permalink raw reply [flat|nested] 44+ messages in thread
[parent not found: <1389658764-39199-1-git-send-email-s-anna-l0cyMroinI0@public.gmane.org>]
* [PATCHv4 5/7] hwspinlock/omap: add support for dt nodes
[not found] ` <1389658764-39199-1-git-send-email-s-anna-l0cyMroinI0@public.gmane.org>
@ 2014-01-14 0:19 ` Suman Anna
2014-02-10 19:27 ` [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
1 sibling, 0 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Suman Anna
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 <s-anna-l0cyMroinI0@public.gmane.org>
[tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org: ack for legacy file removal]
Acked-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
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 <ohad-Ix1uc/W3ht7QT0dZR+AlfA@public.gmane.org>
L: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
S: Maintained
F: drivers/hwspinlock/omap_hwspinlock.c
-F: arch/arm/mach-omap2/hwspinlock.c
OMAP MMC SUPPORT
M: Jarkko Lavinen <jarkko.lavinen-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
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 <sque-l0cyMroinI0@public.gmane.org>
- * Hari Kanigeri <h-kanigeri2-l0cyMroinI0@public.gmane.org>
- *
- * 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 <sque-l0cyMroinI0@public.gmane.org>
* Hari Kanigeri <h-kanigeri2-l0cyMroinI0@public.gmane.org>
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCHv4 0/7] omap hwspinlock dt support
[not found] ` <1389658764-39199-1-git-send-email-s-anna-l0cyMroinI0@public.gmane.org>
2014-01-14 0:19 ` [PATCHv4 5/7] hwspinlock/omap: add support for dt nodes Suman Anna
@ 2014-02-10 19:27 ` Suman Anna
2014-02-24 18:14 ` Suman Anna
1 sibling, 1 reply; 44+ messages in thread
From: Suman Anna @ 2014-02-10 19:27 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Suman Anna
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCHv4 0/7] omap hwspinlock dt support
2014-02-10 19:27 ` [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
@ 2014-02-24 18:14 ` Suman Anna
[not found] ` <530B8C00.8020001-l0cyMroinI0@public.gmane.org>
0 siblings, 1 reply; 44+ messages in thread
From: Suman Anna @ 2014-02-24 18:14 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: devicetree, Tony Lindgren, linux-kernel, Kumar Gala, linux-omap,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
` (4 preceding siblings ...)
[not found] ` <1389658764-39199-1-git-send-email-s-anna-l0cyMroinI0@public.gmane.org>
@ 2014-01-14 0:19 ` Suman Anna
2014-01-14 13:10 ` Felipe Balbi
2014-01-15 23:36 ` [UPDATED PATCHv4 " Suman Anna
2014-01-14 0:19 ` [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx Suman Anna
2014-01-14 13:12 ` [PATCHv4 0/7] omap hwspinlock dt support Felipe Balbi
7 siblings, 2 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel, Suman Anna
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 <s-anna@ti.com>
---
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
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register
2014-01-14 0:19 ` [PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register Suman Anna
@ 2014-01-14 13:10 ` Felipe Balbi
2014-01-14 14:04 ` Felipe Balbi
2014-01-15 23:36 ` [UPDATED PATCHv4 " Suman Anna
1 sibling, 1 reply; 44+ messages in thread
From: Felipe Balbi @ 2014-01-14 13:10 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Mark Rutland, devicetree, Tony Lindgren,
linux-kernel, Kumar Gala, linux-omap, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1774 bytes --]
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 <s-anna@ti.com>
> ---
> 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
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register
2014-01-14 13:10 ` Felipe Balbi
@ 2014-01-14 14:04 ` Felipe Balbi
[not found] ` <20140114140440.GA15785-HgARHv6XitL9zxVx7UNMDg@public.gmane.org>
0 siblings, 1 reply; 44+ messages in thread
From: Felipe Balbi @ 2014-01-14 14:04 UTC (permalink / raw)
To: Felipe Balbi
Cc: Suman Anna, Ohad Ben-Cohen, Mark Rutland, Tony Lindgren,
Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 733 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [UPDATED PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register
2014-01-14 0:19 ` [PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register Suman Anna
2014-01-14 13:10 ` Felipe Balbi
@ 2014-01-15 23:36 ` Suman Anna
1 sibling, 0 replies; 44+ messages in thread
From: Suman Anna @ 2014-01-15 23:36 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: devicetree, Suman Anna, Tony Lindgren, linux-kernel, Felipe Balbi,
Kumar Gala, linux-omap, linux-arm-kernel
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 <s-anna@ti.com>
---
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
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
` (5 preceding siblings ...)
2014-01-14 0:19 ` [PATCHv4 6/7] hwspinlock/omap: enable module before reading SYSSTATUS register Suman Anna
@ 2014-01-14 0:19 ` Suman Anna
2014-01-14 13:12 ` Felipe Balbi
2014-01-14 13:12 ` [PATCHv4 0/7] omap hwspinlock dt support Felipe Balbi
7 siblings, 1 reply; 44+ messages in thread
From: Suman Anna @ 2014-01-14 0:19 UTC (permalink / raw)
To: Ohad Ben-Cohen, Mark Rutland
Cc: Tony Lindgren, Kumar Gala, linux-kernel, linux-omap, devicetree,
linux-arm-kernel, Suman Anna
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 <s-anna@ti.com>
---
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
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
2014-01-14 0:19 ` [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx Suman Anna
@ 2014-01-14 13:12 ` Felipe Balbi
2014-01-14 16:51 ` Anna, Suman
0 siblings, 1 reply; 44+ messages in thread
From: Felipe Balbi @ 2014-01-14 13:12 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel, linux-omap, devicetree, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]
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 <s-anna@ti.com>
> ---
> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
2014-01-14 13:12 ` Felipe Balbi
@ 2014-01-14 16:51 ` Anna, Suman
2014-01-14 17:29 ` Felipe Balbi
0 siblings, 1 reply; 44+ messages in thread
From: Anna, Suman @ 2014-01-14 16:51 UTC (permalink / raw)
To: balbi
Cc: Ohad Ben-Cohen, Mark Rutland, devicetree, Tony Lindgren,
linux-kernel, Kumar Gala, linux-omap, linux-arm-kernel
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 <s-anna@ti.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
2014-01-14 16:51 ` Anna, Suman
@ 2014-01-14 17:29 ` Felipe Balbi
2014-01-14 18:36 ` Anna, Suman
0 siblings, 1 reply; 44+ messages in thread
From: Felipe Balbi @ 2014-01-14 17:29 UTC (permalink / raw)
To: Anna, Suman
Cc: balbi, Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel, linux-omap, devicetree, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]
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 <s-anna@ti.com>
> >>---
> >> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx
2014-01-14 17:29 ` Felipe Balbi
@ 2014-01-14 18:36 ` Anna, Suman
0 siblings, 0 replies; 44+ messages in thread
From: Anna, Suman @ 2014-01-14 18:36 UTC (permalink / raw)
To: balbi
Cc: Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel, linux-omap, devicetree, linux-arm-kernel
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 <s-anna@ti.com>
>>>> ---
>>>> 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCHv4 0/7] omap hwspinlock dt support
2014-01-14 0:19 [PATCHv4 0/7] omap hwspinlock dt support Suman Anna
` (6 preceding siblings ...)
2014-01-14 0:19 ` [PATCHv4 7/7] hwspinlock/omap: enable build for AM33xx, AM43xx & DRA7xx Suman Anna
@ 2014-01-14 13:12 ` Felipe Balbi
7 siblings, 0 replies; 44+ messages in thread
From: Felipe Balbi @ 2014-01-14 13:12 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Mark Rutland, Tony Lindgren, Kumar Gala,
linux-kernel, linux-omap, devicetree, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]
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 <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread