[PATCHv6 0/5] hwspinlock core/omap dt support

[PATCHv6 0/5] hwspinlock core/omap dt support

[Suman Anna]

Hi Ohad,

This is an update to the hwspinlock dt support series. The series
is rebased onto v3.17-rc3, and addresses the review comments on the
previous v5 series. I have also split and left out the RFC patches
about the support for reserved locks (will post these as a separate
series) and return code convention changes in the hwspinlock core
(will not be needed anymore). The support for deferred probing of
clients is supported in the new of_hwspin_lock_get_id() function
itself.

Following are the main changes in v6 w.r.t v5:
- [Patch 1] Updated generic hwspinlock bindings after folding back
  the hwlock-base-id property from v5 Patch 8 [1]. [Patch 1]
- [Patch 4] Updated the common OF helpers patch based on review
  comments on v5. of_hwspin_lock_request_specific() is replaced
  with of_hwspin_lock_get_id(). of_hwspin_lock_simple_xlate() is
  made internal, and of_hwspin_lock_get_base_id() is added.
- [Patch 5] Updated the OMAP hwspinlock DT support patch to address
  review comments about hard-coded base-id, it is parsed from DT
  if present.
- Patches 2 and 3 are unchanged from previous version.
- Support patches for AM335x and AM437x SoCs (v5 patches 6 and 7)
  have been dropped as they are merged in separately for 3.17
- RFC patches (v5 patches 9 through 15) adding the concept of
  reserved locks and return code change convention dropped.

I had looked into dropping the patch to maintain the list of registered
spinlocks, but had to retain the patch as it was needed to perform the
validity of the args-specifier value in of_hwspin_lock_get_id().

The validation logs on all the applicable OMAP SoCs are at:
  OMAP4  - http://pastebin.ubuntu.com/8329228
  OMAP5  - http://pastebin.ubuntu.com/8329301
  DRA74x - http://pastebin.ubuntu.com/8329261
  AM33xx - http://pastebin.ubuntu.com/8329199
  AM43xx - http://pastebin.ubuntu.com/8329273

The above logs are generated with some additional test patches staged
here for reference:
https://github.com/sumananna/omap-kernel/commits/hwspinlock/test/3.17-rc3-dt-v6
https://github.com/sumananna/omap-kernel/commits/hwspinlock/submit/3.17-rc3-dt-v6

regards
Suman

[1] https://patchwork.kernel.org/patch/4096741/

---
v5:
http://marc.info/?l=linux-omap&m=139890478402807&w=2
- Rebased v4 patches plus additional RFC patches.
- Added back the patch to support DT-based hwlock-base-id property,
  for registration purposes.
- RFC patches introducing the concept of reserved locks.
- Staged patches for converting return convention to better support
  deferred probing of client drivers.

v4:
- The DT bindings are split into separate patches, and updated to
  add comments about #hwlock-cells
- Fixed a registration issue with repeated module installation and
  removal.
- Added a new OF helper to support #hwlock-cells in addition to the
  previous OF functions. The OMAP adaptation patch is updated to use
  the default translate function
- Updated hwspinlock documentation to adjust for the structure
  changes and the new api additions.
- Added build support for AM335x, AM43xx and DRA7xx
http://marc.info/?l=linux-omap&m=138965904015225&w=2

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 (5):
  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

 .../devicetree/bindings/hwlock/hwlock.txt          |  55 +++++++
 .../devicetree/bindings/hwlock/omap-hwspinlock.txt |  24 +++
 Documentation/hwspinlock.txt                       |  28 ++++
 MAINTAINERS                                        |   1 -
 arch/arm/mach-omap2/Makefile                       |   3 -
 arch/arm/mach-omap2/hwspinlock.c                   |  60 -------
 drivers/hwspinlock/hwspinlock_core.c               | 173 +++++++++++++++++++++
 drivers/hwspinlock/hwspinlock_internal.h           |   2 +
 drivers/hwspinlock/omap_hwspinlock.c               |  23 ++-
 include/linux/hwspinlock.h                         |  15 +-
 10 files changed, 312 insertions(+), 72 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

-- 
2.0.4

[PATCHv6 1/5] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

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. This base id
needs to be unique across multiple IP instances of a hwspinlock
device, so that each hwlock can be represented uniquely in a
system.

The number of locks is represented by 'hwlock-num-locks' property,
and the base id is represented by the 'hwlock-base-id' property.
The args specifier length is dependent on each vendor-specific
implementation and is represented through the '#hwlock-cells'
property. Client users need to use the property 'hwlocks' for
requesting specific lock(s).

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          | 55 ++++++++++++++++++++++
 1 file changed, 55 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..24993dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/hwlock.txt
@@ -0,0 +1,55 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations.
+
+The validity and need of these common properties may vary from one platform
+implementation to another. The platform specific bindings should explicitly
+state if an optional property is used. Please also 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. This property is mandatory for all
+			platform implementations.
+- 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-base-id:	An unique base Id for the locks for a particular hwlock
+			device. This property is mandatory if a SoC has several
+			hwlock devices.
+
+Hwlock Users:
+=============
+
+Nodes that require specific hwlock(s) should specify them using the property
+"hwlocks", 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>;
+		...
+	};
-- 
2.0.4

[PATCHv6 2/5] Documentation: dt: add the omap hwspinlock bindings document

[Suman Anna]

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>;
+};
-- 
2.0.4

[PATCHv6 3/5] hwspinlock/core: maintain a list of registered hwspinlock banks

[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;
-- 
2.0.4

[PATCHv6 4/5] hwspinlock/core: add common OF helpers

[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 OF helper
   function to read the 'hwlock-num-locks' property.
2. The of_hwspin_lock_get_base_id() is a common OF helper
   function to read the 'hwlock-base-id' property.
3. The of_hwspin_lock_get_id() API can be used by hwspinlock
   clients to get the id for a specific lock using the phandle
   + args specifier, so that it can be requested using the
   available hwspin_lock_request_specific() API.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 Documentation/hwspinlock.txt         |  26 ++++++++
 drivers/hwspinlock/hwspinlock_core.c | 122 +++++++++++++++++++++++++++++++++++
 include/linux/hwspinlock.h           |  15 ++++-
 3 files changed, 160 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
index 640ae47..c15dc9f 100644
--- a/Documentation/hwspinlock.txt
+++ b/Documentation/hwspinlock.txt
@@ -48,6 +48,16 @@ independent, drivers.
      ids for predefined purposes.
      Should be called from a process context (might sleep).
 
+  int of_hwspin_lock_get_id(struct device_node *np, int index);
+   - retrieve the global lock id for an OF phandle-based specific lock.
+     This function provides a means for DT users of a hwspinlock module
+     to get the global lock id of a specific hwspinlock, so that it can
+     be requested using the normal hwspin_lock_request_specific() API.
+     The function returns a valid lock id number on success, -EPROBE_DEFER
+     if the hwspinlock device is not yet registered with the core, or other
+     error values.
+     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 +253,22 @@ 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_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.
+
+  int of_hwspin_lock_get_base_id(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 base index for a block of locks present
+     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
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 48f7866..7d9f749 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,127 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 }
 EXPORT_SYMBOL_GPL(__hwspin_unlock);
 
+/**
+ * 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.
+ */
+static 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];
+}
+
+/**
+ * of_hwspin_lock_get_id() - get lock id for an OF phandle-based specific lock
+ * @np: device node from which to request the specific hwlock
+ * @index: index of the hwlock in the list of values
+ *
+ * This function provides a means for DT users of the hwspinlock module to
+ * get the global lock id of a specific hwspinlock using the phandle of the
+ * hwspinlock device, so that it can be requested using the normal
+ * hwspin_lock_request_specific() API.
+ *
+ * Returns the global lock id number on success, -EPROBE_DEFER if the hwspinlock
+ * device is not yet registered, -EINVAL on invalid args specifier value or an
+ * appropriate error as returned from the OF parsing of the DT user node.
+ */
+int of_hwspin_lock_get_id(struct device_node *np, int index)
+{
+	struct hwspinlock_device *bank;
+	struct of_phandle_args args;
+	int id;
+	int ret;
+
+	ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells", index,
+					 &args);
+	if (ret)
+		return ret;
+
+	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) {
+		ret = -EPROBE_DEFER;
+		goto out;
+	}
+
+	id = of_hwspin_lock_simple_xlate(bank, &args);
+	if (id < 0 || id >= bank->num_locks) {
+		ret = -EINVAL;
+		goto out;
+	}
+	id += bank->base_id;
+
+out:
+	of_node_put(args.np);
+	return ret ? ret : id;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_id);
+
+/**
+ * of_hwspin_lock_get_base_id() - OF helper to retrieve base id
+ * @dn: device node pointer
+ *
+ * This is an OF helper function that can be called by the underlying
+ * platform-specific implementations, to retrieve the base id for the
+ * set of locks present within a hwspinlock device instance.
+ *
+ * Returns the base id value on success, or an appropriate error code
+ * as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{
+	unsigned int val;
+	int ret;
+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	return ret ? ret : val;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
+
+/**
+ * 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;
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 3343298..a9eeb4f 100644
--- a/include/linux/hwspinlock.h
+++ b/include/linux/hwspinlock.h
@@ -26,6 +26,7 @@
 #define HWLOCK_IRQ	0x02	/* Disable interrupts, don't save state */
 
 struct device;
+struct device_node;
 struct hwspinlock;
 struct hwspinlock_device;
 struct hwspinlock_ops;
@@ -60,12 +61,15 @@ struct hwspinlock_pdata {
 
 #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
 
+int of_hwspin_lock_get_base_id(struct device_node *dn);
+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);
 int hwspin_lock_free(struct hwspinlock *hwlock);
+int of_hwspin_lock_get_id(struct device_node *np, int index);
 int hwspin_lock_get_id(struct hwspinlock *hwlock);
 int __hwspin_lock_timeout(struct hwspinlock *, unsigned int, int,
 							unsigned long *);
@@ -80,9 +84,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.
@@ -120,6 +124,11 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 {
 }
 
+static inline int of_hwspin_lock_get_id(struct device_node *np, int index)
+{
+	return 0;
+}
+
 static inline int hwspin_lock_get_id(struct hwspinlock *hwlock)
 {
 	return 0;
-- 
2.0.4

[PATCHv6 5/5] hwspinlock/omap: add support for dt nodes

[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@ti.com>
[tony@atomide.com: ack for legacy file removal]
Acked-by: Tony Lindgren <tony@atomide.com>
---
 MAINTAINERS                          |  1 -
 arch/arm/mach-omap2/Makefile         |  3 --
 arch/arm/mach-omap2/hwspinlock.c     | 60 ------------------------------------
 drivers/hwspinlock/omap_hwspinlock.c | 23 +++++++++++---
 4 files changed, 18 insertions(+), 69 deletions(-)
 delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cf24bb5..351885e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6531,7 +6531,6 @@ M:	Ohad Ben-Cohen <ohad@wizery.com>
 L:	linux-omap@vger.kernel.org
 S:	Maintained
 F:	drivers/hwspinlock/omap_hwspinlock.c
-F:	arch/arm/mach-omap2/hwspinlock.c
 
 OMAP MMC SUPPORT
 M:	Jarkko Lavinen <jarkko.lavinen@nokia.com>
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 69bbcba..088b6b0 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -286,9 +286,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@ti.com>
- *          Hari Kanigeri <h-kanigeri2@ti.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- */
-
-#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 c1e2cd4..2e8c7c3 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@ti.com>
  *          Hari Kanigeri <h-kanigeri2@ti.com>
@@ -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"
@@ -80,16 +81,21 @@ static const struct hwspinlock_ops omap_hwspinlock_ops = {
 
 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 num_locks, i, ret, base_id;
 
-	if (!pdata)
+	if (!node)
 		return -ENODEV;
 
+	ret = of_hwspin_lock_get_base_id(node);
+	if (ret < 0 && ret != -EINVAL)
+		return -ENODEV;
+	base_id = (ret > 0 ? ret : 0);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
@@ -141,7 +147,7 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
 		hwlock->priv = io_base + LOCK_BASE_OFFSET + sizeof(u32) * i;
 
 	ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops,
-						pdata->base_id, num_locks);
+						base_id, num_locks);
 	if (ret)
 		goto reg_fail;
 
@@ -174,12 +180,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),
 	},
 };
 
-- 
2.0.4

Re: [PATCHv6 0/5] hwspinlock core/omap dt support

[Suman Anna]

Hi Ohad,

>
> This is an update to the hwspinlock dt support series. The series
> is rebased onto v3.17-rc3, and addresses the review comments on the
> previous v5 series. I have also split and left out the RFC patches
> about the support for reserved locks (will post these as a separate
> series) and return code convention changes in the hwspinlock core
> (will not be needed anymore). The support for deferred probing of
> clients is supported in the new of_hwspin_lock_get_id() function
> itself.
> 
> Following are the main changes in v6 w.r.t v5:
> - [Patch 1] Updated generic hwspinlock bindings after folding back
>   the hwlock-base-id property from v5 Patch 8 [1]. [Patch 1]
> - [Patch 4] Updated the common OF helpers patch based on review
>   comments on v5. of_hwspin_lock_request_specific() is replaced
>   with of_hwspin_lock_get_id(). of_hwspin_lock_simple_xlate() is
>   made internal, and of_hwspin_lock_get_base_id() is added.
> - [Patch 5] Updated the OMAP hwspinlock DT support patch to address
>   review comments about hard-coded base-id, it is parsed from DT
>   if present.
> - Patches 2 and 3 are unchanged from previous version.
> - Support patches for AM335x and AM437x SoCs (v5 patches 6 and 7)
>   have been dropped as they are merged in separately for 3.17
> - RFC patches (v5 patches 9 through 15) adding the concept of
>   reserved locks and return code change convention dropped.
> 
> I had looked into dropping the patch to maintain the list of registered
> spinlocks, but had to retain the patch as it was needed to perform the
> validity of the args-specifier value in of_hwspin_lock_get_id().

Ping, do you have any comments on this revised series? If not, can we
queue this for 3.18?

regards
Suman

> 
> The validation logs on all the applicable OMAP SoCs are at:
>   OMAP4  - http://pastebin.ubuntu.com/8329228
>   OMAP5  - http://pastebin.ubuntu.com/8329301
>   DRA74x - http://pastebin.ubuntu.com/8329261
>   AM33xx - http://pastebin.ubuntu.com/8329199
>   AM43xx - http://pastebin.ubuntu.com/8329273
> 
> The above logs are generated with some additional test patches staged
> here for reference:
> https://github.com/sumananna/omap-kernel/commits/hwspinlock/test/3.17-rc3-dt-v6
> https://github.com/sumananna/omap-kernel/commits/hwspinlock/submit/3.17-rc3-dt-v6
> 
> regards
> Suman
> 
> [1] https://patchwork.kernel.org/patch/4096741/
> 
> ---
> v5:
> http://marc.info/?l=linux-omap&m=139890478402807&w=2
> - Rebased v4 patches plus additional RFC patches.
> - Added back the patch to support DT-based hwlock-base-id property,
>   for registration purposes.
> - RFC patches introducing the concept of reserved locks.
> - Staged patches for converting return convention to better support
>   deferred probing of client drivers.
> 
> v4:
> - The DT bindings are split into separate patches, and updated to
>   add comments about #hwlock-cells
> - Fixed a registration issue with repeated module installation and
>   removal.
> - Added a new OF helper to support #hwlock-cells in addition to the
>   previous OF functions. The OMAP adaptation patch is updated to use
>   the default translate function
> - Updated hwspinlock documentation to adjust for the structure
>   changes and the new api additions.
> - Added build support for AM335x, AM43xx and DRA7xx
> http://marc.info/?l=linux-omap&m=138965904015225&w=2
> 
> 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 (5):
>   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
> 
>  .../devicetree/bindings/hwlock/hwlock.txt          |  55 +++++++
>  .../devicetree/bindings/hwlock/omap-hwspinlock.txt |  24 +++
>  Documentation/hwspinlock.txt                       |  28 ++++
>  MAINTAINERS                                        |   1 -
>  arch/arm/mach-omap2/Makefile                       |   3 -
>  arch/arm/mach-omap2/hwspinlock.c                   |  60 -------
>  drivers/hwspinlock/hwspinlock_core.c               | 173 +++++++++++++++++++++
>  drivers/hwspinlock/hwspinlock_internal.h           |   2 +
>  drivers/hwspinlock/omap_hwspinlock.c               |  23 ++-
>  include/linux/hwspinlock.h                         |  15 +-
>  10 files changed, 312 insertions(+), 72 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
> 

Re: [PATCHv6 0/5] hwspinlock core/omap dt support

[Bjorn Andersson]

On Fri, Sep 12, 2014 at 1:24 PM, Suman Anna <s-anna@ti.com> wrote:
> Hi Ohad,
>
> This is an update to the hwspinlock dt support series. The series
> is rebased onto v3.17-rc3, and addresses the review comments on the
> previous v5 series. I have also split and left out the RFC patches
> about the support for reserved locks (will post these as a separate
> series) and return code convention changes in the hwspinlock core
> (will not be needed anymore). The support for deferred probing of
> clients is supported in the new of_hwspin_lock_get_id() function
> itself.
>

Thanks for your reply to me, I had missed that you continued this work.


I find it somewhat awkward to have to call both of_hwspin_lock_get_id() and
then hwspin_lock_request_specific(), but I found the request from Ohad, so
let's stick with it.


Am I right that hwlock-num-locks and hwlock-base-id are optional from the
frameworks perspective and only there to aid the hwspin drivers? If so it is
strange to have in the common binding and have the helper functions in the core
for simply reading hwspin device specific properties.

Otherwise I think it looks sane, although I haven't spend that much time
reviewing it.

I did throw it into my tree and gave it a testrun with the Qualcomm code I've
been working on. So for the non-omap parts:

Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

Regards,
Bjorn

Re: [PATCHv6 0/5] hwspinlock core/omap dt support

[Suman Anna]

Hi Bjorn,

On 09/30/2014 03:54 PM, Bjorn Andersson wrote:
> On Fri, Sep 12, 2014 at 1:24 PM, Suman Anna <s-anna@ti.com> wrote:
>> Hi Ohad,
>>
>> This is an update to the hwspinlock dt support series. The series
>> is rebased onto v3.17-rc3, and addresses the review comments on the
>> previous v5 series. I have also split and left out the RFC patches
>> about the support for reserved locks (will post these as a separate
>> series) and return code convention changes in the hwspinlock core
>> (will not be needed anymore). The support for deferred probing of
>> clients is supported in the new of_hwspin_lock_get_id() function
>> itself.
>>
> 
> Thanks for your reply to me, I had missed that you continued this work.
> 
> 
> I find it somewhat awkward to have to call both of_hwspin_lock_get_id() and
> then hwspin_lock_request_specific(), but I found the request from Ohad, so
> let's stick with it.
> 
> Am I right that hwlock-num-locks and hwlock-base-id are optional from the
> frameworks perspective and only there to aid the hwspin drivers? If so it is
> strange to have in the common binding and have the helper functions in the core
> for simply reading hwspin device specific properties.

The hwlock-num-locks and hwlock-base-id would be common features to all
the hwspinlock drivers, so they are added as common bindings which the
individual implementations should use instead of defining their own
properties. These are added based on discussion way back on v1. You
ought to replace the "qcom,num-locks" with the first one.

I will respond to your v4 with a few comments so that we don't loose the
context in that thread.

> 
> Otherwise I think it looks sane, although I haven't spend that much time
> reviewing it.
> 
> I did throw it into my tree and gave it a testrun with the Qualcomm code I've
> been working on. So for the non-omap parts:
> 
> Tested-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> 

Thanks for testing this with the new Qualcomm driver.

regards
Suman

Re: [PATCHv6 1/5] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

Hi Suman,

On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
> This patch adds the generic common bindings used to represent
> a hwlock device and use/request locks in a device-tree build.
>
...
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  .../devicetree/bindings/hwlock/hwlock.txt          | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Please ask a DT maintainer to ACK this - otherwise I can't send this to Linus.

Thanks,
Ohad.

Re: [PATCHv6 2/5] Documentation: dt: add the omap hwspinlock bindings document

[Ohad Ben-Cohen]

Hi Suman,

On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
> 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 ++++++++++++++++++++++

I need an ACK from a DT maintainer here as well, please try to get it.

Thanks,
Ohad.

Re: [PATCHv6 1/5] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

Kumar, Mark, Rob,

On 11/12/2014 09:14 AM, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
>> This patch adds the generic common bindings used to represent
>> a hwlock device and use/request locks in a device-tree build.
>>
> ...
>>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  .../devicetree/bindings/hwlock/hwlock.txt          | 55 ++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
> 
> Please ask a DT maintainer to ACK this - otherwise I can't send this to Linus.
> 

Can one of you ack this patch and the next patch [1] so that Ohad can
pick up the series for 3.19? This series has been in works for quite
some time now and was reviewed by each of you at some point of time
(thanks!!), the cover letter [2] has the history of the changes.

regards
Suman

[1] https://patchwork.kernel.org/patch/4898001/
[2] http://marc.info/?l=linux-omap&m=141055365213895&w=2

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Ohad Ben-Cohen]

Hi Suman,

On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
> +int of_hwspin_lock_get_id(struct device_node *np, int index)
> +{
> +       struct hwspinlock_device *bank;
> +       struct of_phandle_args args;
> +       int id;
> +       int ret;
> +
> +       ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells", index,
> +                                        &args);
> +       if (ret)
> +               return ret;
> +
> +       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) {
> +               ret = -EPROBE_DEFER;
> +               goto out;
> +       }

Is this the validation you mentioned which requires the existence of
"hwspinlock/core: maintain a list of registered hwspinlock banks" ?

I'm not convinced this is needed for several reasons:
- the user isn't using the lock yet at this point, and may only need
the id in order to communicate it to a remote processor
- if the user will try to use the lock prematurely,
hwspin_lock_request_specific should stop her
- moreover, hwspin_lock_request_specific must be the one who validates
the id, since in heterogeneous systems the user may get the id from a
remote processor and not via of_hwspin_lock_get_id

 "hwspinlock/core: maintain a list of registered hwspinlock banks"
adds complexity which must be very strongly justified.

If we're not sure there is a strong justification for it, we better
not merge it.

> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
...
> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);

Do we really must expose these two helpers globally?

Can we instead make these "static inline" methods and embed them in
hwspinlock_internal.h ?

Thanks,
Ohad.

Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes

[Ohad Ben-Cohen]

Hi Suman,

On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
>  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 num_locks, i, ret, base_id;
>
> -       if (!pdata)
> +       if (!node)
>                 return -ENODEV;
>
> +       ret = of_hwspin_lock_get_base_id(node);
> +       if (ret < 0 && ret != -EINVAL)
> +               return -ENODEV;
> +       base_id = (ret > 0 ? ret : 0);

Does this mean you allow nodes not to have the base_id property? How
do we protect against multiple nodes not having a base_id property
then?

Implicitly assuming a base_id value (zero in this case) may not be always safe.

Thanks,
Ohad.

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Suman Anna]

Hi Ohad,

Thanks for the review.

On 11/12/2014 01:08 PM, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
>> +int of_hwspin_lock_get_id(struct device_node *np, int index)
>> +{
>> +       struct hwspinlock_device *bank;
>> +       struct of_phandle_args args;
>> +       int id;
>> +       int ret;
>> +
>> +       ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells", index,
>> +                                        &args);
>> +       if (ret)
>> +               return ret;
>> +
>> +       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) {
>> +               ret = -EPROBE_DEFER;
>> +               goto out;
>> +       }
> 
> Is this the validation you mentioned which requires the existence of
> "hwspinlock/core: maintain a list of registered hwspinlock banks" ?

Well, not exactly. The validation is on the following segment,

+	id = of_hwspin_lock_simple_xlate(bank, &args);
+	if (id < 0 || id >= bank->num_locks) {
+		ret = -EINVAL;
+		goto out;
+	}

That said, it is also needed to provide the support for deferred probing
without changing the return code conventions on the existing API.

> 
> I'm not convinced this is needed for several reasons:
> - the user isn't using the lock yet at this point, and may only need
> the id in order to communicate it to a remote processor

Yes, and wouldn't that require that the id is validated? It just cannot
return any return value, and expect it will be verified somewhere else
or in a following API call. Not doing the validation unnecessarily
complicates the system usage of a lock as you are sharing an invalid
lock to a remote processor and then you have two validation failure
paths on different processors.

> - if the user will try to use the lock prematurely,
> hwspin_lock_request_specific should stop her
> - moreover, hwspin_lock_request_specific must be the one who validates
> the id, since in heterogeneous systems the user may get the id from a
> remote processor and not via of_hwspin_lock_get_id
> 
>  "hwspinlock/core: maintain a list of registered hwspinlock banks"
> adds complexity which must be very strongly justified.
> 
> If we're not sure there is a strong justification for it, we better
> not merge it.
> 
>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
> ...
>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
> 
> Do we really must expose these two helpers globally?
> 
> Can we instead make these "static inline" methods and embed them in
> hwspinlock_internal.h ?

Actually, not a bad idea, I will move it, thanks. All the client drivers
would need it, and they already have to include the internal header.

regards
Suman

Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes

[Suman Anna]

Hi Ohad,

On 11/12/2014 01:14 PM, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
>>  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 num_locks, i, ret, base_id;
>>
>> -       if (!pdata)
>> +       if (!node)
>>                 return -ENODEV;
>>
>> +       ret = of_hwspin_lock_get_base_id(node);
>> +       if (ret < 0 && ret != -EINVAL)
>> +               return -ENODEV;
>> +       base_id = (ret > 0 ? ret : 0);
> 
> Does this mean you allow nodes not to have the base_id property? How
> do we protect against multiple nodes not having a base_id property
> then?
> 
> Implicitly assuming a base_id value (zero in this case) may not be always safe.

None of the OMAPs have multiple IP instances, and as such the base-id is
an optional property. I have made this change to make sure we atleast
attempt to use the value if mentioned in DT and not hard-coding the
value to begin with (going by the optional property semantics). If and
when multiple instances get added and a secondary node doesn't add the
property, the node will not be registered with the core due to an
overlap failure. Here is the previous version [1] for reference.

regards
Suman

[1] https://patchwork.kernel.org/patch/4096881/

Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes

[Ohad Ben-Cohen]

Hi Suman,

On Wed, Nov 12, 2014 at 9:50 PM, Suman Anna <s-anna@ti.com> wrote:
> None of the OMAPs have multiple IP instances, and as such the base-id is
> an optional property. I have made this change to make sure we atleast
> attempt to use the value if mentioned in DT and not hard-coding the
> value to begin with (going by the optional property semantics). If and
> when multiple instances get added and a secondary node doesn't add the
> property, the node will not be registered with the core due to an
> overlap failure.

Ok, that sounds good.

Thanks,
Ohad.

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Ohad Ben-Cohen]

Hi Suman,

On Wed, Nov 12, 2014 at 9:32 PM, Suman Anna <s-anna@ti.com> wrote:
>> Is this the validation you mentioned which requires the existence of
>> "hwspinlock/core: maintain a list of registered hwspinlock banks" ?
>
> Well, not exactly. The validation is on the following segment,
>
> +       id = of_hwspin_lock_simple_xlate(bank, &args);
> +       if (id < 0 || id >= bank->num_locks) {
> +               ret = -EINVAL;
> +               goto out;
> +       }

I'm not entirely convinced that this justifies adding the proposed
linked list. Can't we can get the base_id and number of locks by
traversing the DT?

> That said, it is also needed to provide the support for deferred probing
> without changing the return code conventions on the existing API.

Here too, adding a data structure maintaining memory objects and
taking care of it life cycle seems a bit overkill for anything to do
with return values.

> Yes, and wouldn't that require that the id is validated? It just cannot
> return any return value, and expect it will be verified somewhere else
> or in a following API call. Not doing the validation unnecessarily
> complicates the system usage of a lock as you are sharing an invalid
> lock to a remote processor and then you have two validation failure
> paths on different processors.

Validation is great but I'm still not convinced it can't be done
without maintaining another data structure.

Please show me specific technical issues that can't be resolved
without adding the proposed linked list.

Thanks,
Ohad.

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Suman Anna]

Hi Ohad,

On 11/13/2014 04:03 AM, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Wed, Nov 12, 2014 at 9:32 PM, Suman Anna <s-anna@ti.com> wrote:
>>> Is this the validation you mentioned which requires the existence of
>>> "hwspinlock/core: maintain a list of registered hwspinlock banks" ?
>>
>> Well, not exactly. The validation is on the following segment,
>>
>> +       id = of_hwspin_lock_simple_xlate(bank, &args);
>> +       if (id < 0 || id >= bank->num_locks) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
> 
> I'm not entirely convinced that this justifies adding the proposed
> linked list. Can't we can get the base_id and number of locks by
> traversing the DT?

No, not always, because, either of them can be optional between
different platform implementations. For example, on OMAP, the number of
locks is read from an IP register, and not coded in DT. Similarly,
base_id can be optional on SoCs that don't have multiple IP instances.
The only place the hwspinlock core knows both of them for sure is at the
device registration time, but the core only stores the locks and not the
devices at the moment. Any operation on the device is not possible
without knowing the exact global lock we are dealing with, and this API
is about returning that exact global lock id.

> 
>> That said, it is also needed to provide the support for deferred probing
>> without changing the return code conventions on the existing API.
> 
> Here too, adding a data structure maintaining memory objects and
> taking care of it life cycle seems a bit overkill for anything to do
> with return values.

IMHO, this life cycle management is not that complicated, it is managed
alongside the addition/removal of the locks during the device
registration/unregistration time.

> 
>> Yes, and wouldn't that require that the id is validated? It just cannot
>> return any return value, and expect it will be verified somewhere else
>> or in a following API call. Not doing the validation unnecessarily
>> complicates the system usage of a lock as you are sharing an invalid
>> lock to a remote processor and then you have two validation failure
>> paths on different processors.
> 
> Validation is great but I'm still not convinced it can't be done
> without maintaining another data structure.
> 
> Please show me specific technical issues that can't be resolved
> without adding the proposed linked list.

Same as above.

regards
Suman

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Ohad Ben-Cohen]

Hi Suman,

On Thu, Nov 13, 2014 at 7:38 PM, Suman Anna <s-anna@ti.com> wrote:
> No, not always, because, either of them can be optional between
> different platform implementations. For example, on OMAP, the number of
> locks is read from an IP register, and not coded in DT. Similarly,
> base_id can be optional on SoCs that don't have multiple IP instances.

It can, but base_id isn't optional today --- it must be provided via
platform_data --- and I'm not sure we should implicitly change this
semantics while moving to DT. We never guessed the base_id before,
even if there was only a single IP instance, and I'm not convinced we
should start doing it now.

About the number of locks - why do we even need it at this point? the
global lock id should just be base_id + the lock index.

> IMHO, this life cycle management is not that complicated

It's always very easy to add code, really. It is never complicated.
But then gradually the code becomes harder to maintain, debug, and
change.

On the contrary, achieving the same functionality with less code is
always harder. But when we get there, the results are pretty
satisfying.

In this case I still feel the extra linked list is redundant.

Please try it: ditch the "hwspinlock/core: maintain a list of
registered hwspinlock banks" patch, and replace of_hwspin_lock_get_id
with a slim method that just returns the base_id + the lock index. Let
me know if it works for you.

Thanks,
Ohad.

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Suman Anna]

Hi Ohad,

On 11/13/2014 01:45 PM, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Thu, Nov 13, 2014 at 7:38 PM, Suman Anna <s-anna@ti.com> wrote:
>> No, not always, because, either of them can be optional between
>> different platform implementations. For example, on OMAP, the number of
>> locks is read from an IP register, and not coded in DT. Similarly,
>> base_id can be optional on SoCs that don't have multiple IP instances.
> 
> It can, but base_id isn't optional today --- it must be provided via
> platform_data --- 

That was the case before DT, right. As it is, the hwspinlock core has no
knowledge of platform_data, it was used by the individual implementation
drivers to supply the base_id in the registration API.
The platform_data will/should become obsolete with DT devices.

and I'm not sure we should implicitly change this
> semantics while moving to DT. We never guessed the base_id before,
> even if there was only a single IP instance, and I'm not convinced we
> should start doing it now.

OK, if we make hwlock-base-id mandatory in DT, we will get past 1
problem (that of looking up base-id for a specific device).

> 
> About the number of locks - why do we even need it at this point? the
> global lock id should just be base_id + the lock index.

The number of locks would still be needed for validation purposes.

OK, lets take an example. I have say 2 device instances, say
	hwlock1: hwlock@0 {
		hwlock-num-locks = <32>
		hwlock-base-id = <0>;
		#hwlock-cells = <1>;
	};
	hwlock2: hwlock@0 {
		hwlock-num-locks = <32>
		hwlock-base-id = <32>;
		#hwlock-cells = <1>;
	};

	some-client {
		hwlocks = <&hwlock1 32>, <&hwlock2 0>;
	};

The first args value is incorrect, and I expect the API to return an
error. I don't think the API can make assumptions that all passed in
values will be correct. What do you suggest that the API do here?
The above locks are equivalent if we forgo checking.

> 
>> IMHO, this life cycle management is not that complicated
> 
> It's always very easy to add code, really. It is never complicated.
> But then gradually the code becomes harder to maintain, debug, and
> change.
> 
> On the contrary, achieving the same functionality with less code is
> always harder. But when we get there, the results are pretty
> satisfying.
> 
> In this case I still feel the extra linked list is redundant.
> 
> Please try it: ditch the "hwspinlock/core: maintain a list of
> registered hwspinlock banks" patch, and replace of_hwspin_lock_get_id
> with a slim method that just returns the base_id + the lock index. Let
> me know if it works for you.

Yeah, it will work (provided base-id is mandatory) and we drop the
support for
1. phandle args-specifier validation
2. Deferred probing

One solution to handle #1 is to also make the hwlock-num-locks property
also mandatory (even though it could have been read from a register and
I wonder what implementations should do if there is a mismatch between
DT provided value and the value read from IP register). I am not sure
what the DT maintainers' stance is on this, as we are forcing that to
solve an implementation detail in the hwspinlock core.

And, how do you propose we solve the problem of deferred probing? This
was the solution that is resolving both the problems without changing
return codes on existing API (that was my first attempt, but you
preferred not to change API return convention).

Kumar, Mark,
Any comments here?

regards
Suman

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Ohad Ben-Cohen]

Hi Suman,

On Thu, Nov 13, 2014 at 11:02 PM, Suman Anna <s-anna@ti.com> wrote:
> OK, lets take an example. I have say 2 device instances, say
>         hwlock1: hwlock@0 {
>                 hwlock-num-locks = <32>
>                 hwlock-base-id = <0>;
>                 #hwlock-cells = <1>;
>         };
>         hwlock2: hwlock@0 {
>                 hwlock-num-locks = <32>
>                 hwlock-base-id = <32>;
>                 #hwlock-cells = <1>;
>         };
>
>         some-client {
>                 hwlocks = <&hwlock1 32>, <&hwlock2 0>;
>         };
>
> The first args value is incorrect, and I expect the API to return an
> error. I don't think the API can make assumptions that all passed in
> values will be correct. What do you suggest that the API do here?

I think this is just one example of many potential mistakes the DT
developer can have, and that there's nothing really special about it.

What if the '5' digit below is a typo, and the actual hardware
assignment was supposed to be '4' ?

         some-client {
                 hwlocks = <&hwlock1 5>, <&hwlock2 5>;
         };

I don't see how much different this mistake is from the one you
mentioned above. There's a limit to how much we can really catch DT
mistakes in the code, just like we couldn't really catch past mistakes
in the platform data structures.

If we can easily add some validations then great, but if we have to go
to great lengths just to gain very limited validations, then I'd vote
against doing so.

> One solution to handle #1 is to also make the hwlock-num-locks property
> also mandatory (even though it could have been read from a register

Yeah, this is awkward. We shouldn't impose this on implementations
like OMAP which don't need it.

Again I think for the very limited validation this buys us - it's not worth it.

> And, how do you propose we solve the problem of deferred probing?

It seems to me that hwspin_lock_request_specific failures should be
used by clients to defer their probing. Why wouldn't such a simple
solution work?

Thanks,
Ohad.

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Suman Anna]

Hi Ohad,

On 11/14/2014 01:11 AM, Ohad Ben-Cohen wrote:
> Hi Suman,
> 
> On Thu, Nov 13, 2014 at 11:02 PM, Suman Anna <s-anna@ti.com> wrote:
>> OK, lets take an example. I have say 2 device instances, say
>>         hwlock1: hwlock@0 {
>>                 hwlock-num-locks = <32>
>>                 hwlock-base-id = <0>;
>>                 #hwlock-cells = <1>;
>>         };
>>         hwlock2: hwlock@0 {
>>                 hwlock-num-locks = <32>
>>                 hwlock-base-id = <32>;
>>                 #hwlock-cells = <1>;
>>         };
>>
>>         some-client {
>>                 hwlocks = <&hwlock1 32>, <&hwlock2 0>;
>>         };
>>
>> The first args value is incorrect, and I expect the API to return an
>> error. I don't think the API can make assumptions that all passed in
>> values will be correct. What do you suggest that the API do here?
> 
> I think this is just one example of many potential mistakes the DT
> developer can have, and that there's nothing really special about it.
> 
> What if the '5' digit below is a typo, and the actual hardware
> assignment was supposed to be '4' ?
> 
>          some-client {
>                  hwlocks = <&hwlock1 5>, <&hwlock2 5>;
>          };
> 
> I don't see how much different this mistake is from the one you
> mentioned above. There's a limit to how much we can really catch DT
> mistakes in the code, just like we couldn't really catch past mistakes
> in the platform data structures.
> 
> If we can easily add some validations then great, but if we have to go
> to great lengths just to gain very limited validations, then I'd vote
> against doing so.

OK, personally, I am not too comfortable to not perform any validation
on an API.

> 
>> One solution to handle #1 is to also make the hwlock-num-locks property
>> also mandatory (even though it could have been read from a register
> 
> Yeah, this is awkward. We shouldn't impose this on implementations
> like OMAP which don't need it.
> 
> Again I think for the very limited validation this buys us - it's not worth it.
> 
>> And, how do you propose we solve the problem of deferred probing?
> 
> It seems to me that hwspin_lock_request_specific failures should be
> used by clients to defer their probing. Why wouldn't such a simple
> solution work?

Because the API always returns NULL on failures and there is no way for
the clients to figure out if the lock id passed is invalid or the bank
containing the lock is not registered. This was an old discussion on v4
[1], and you insisted that we don't change the return code convention on
the API. I had couple of patches on v5 that dealt with this, but even
that requires the list of registered devices.

regards
Suman

[1] https://patchwork.kernel.org/patch/3481331/

Re: [PATCHv6 4/5] hwspinlock/core: add common OF helpers

[Ohad Ben-Cohen]

On Fri, Nov 14, 2014 at 7:09 PM, Suman Anna <s-anna@ti.com> wrote:
>> It seems to me that hwspin_lock_request_specific failures should be
>> used by clients to defer their probing. Why wouldn't such a simple
>> solution work?

> Because the API always returns NULL on failures and there is no way for
> the clients to figure out if the lock id passed is invalid or the bank
> containing the lock is not registered.

It seems to me this may always be the case - lock ids may be wrong and
there's no way to fully validate them.

Let's start with the simpler approach where
hwspin_lock_request_specific failures are used by clients to defer
their probing.

If a real use case will require changes we can always do that later,
though it seems to me the only gain by changing this API is to catch a
small subset of fatal DT mistakes which will anyway be caught very
early in the development.

Thanks,
Ohad.

Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes

[Bjorn Andersson]

On Wed, Nov 12, 2014 at 11:14 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Suman,
[..]
>
> Does this mean you allow nodes not to have the base_id property? How
> do we protect against multiple nodes not having a base_id property
> then?
>
> Implicitly assuming a base_id value (zero in this case) may not be always safe.
>

Hi Ohad,

I still have a huge problem understanding the awesomeness with the
"base_id". If you have a SoC with 2 hwlock blocks; say 8+8 locks, used
for interaction with e.g. a modem and a video core respectively.
Why would you in either remote system offset the locks with 8?
Wouldn't e.g the modem use locks hwlock0:0-7 and video core use locks
hwlock1:0-7?

What systems use more than one hwlock block and do you know of any
reasons why these hwlocks are globally numbered?

Regards,
Bjorn

Re: [PATCHv6 5/5] hwspinlock/omap: add support for dt nodes

[Ohad Ben-Cohen]

Hi Bjorn,

On Thu, Nov 20, 2014 at 2:43 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> I still have a huge problem understanding the awesomeness with the
> "base_id". If you have a SoC with 2 hwlock blocks; say 8+8 locks, used
> for interaction with e.g. a modem and a video core respectively.
> Why would you in either remote system offset the locks with 8?
> Wouldn't e.g the modem use locks hwlock0:0-7 and video core use locks
> hwlock1:0-7?

Please see below

> What systems use more than one hwlock block

None that we know of. This concern was raised some time ago by Arnd
and since it was easy to deal with we added the 'base_id' notion.

> and do you know of any reasons why these hwlocks are globally numbered?

Because on an heterogeneous asymmetric multiprocessing systems you
sometimes have use cases where hwlocks are dynamically allocated and
their id numbers need to be synchronized between user space
applications, the Linux kernel, and entities running on remote
processors (which are likely not running Linux).

For this to work we have to have some system-wide global hwlocks
naming that is predefined and known to all processors. A mere number
seems like the simplest naming method. A dynamic hwlock request API
could return "hwlock1:0" but a mere 8 seems easier to deal with.

Thanks,
Ohad.

Re: [PATCHv6 1/5] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Wed, Nov 12, 2014 at 9:08 AM, Suman Anna <s-anna@ti.com> wrote:
> Kumar, Mark, Rob,
>
> On 11/12/2014 09:14 AM, Ohad Ben-Cohen wrote:
>> Hi Suman,
>>
>> On Fri, Sep 12, 2014 at 11:24 PM, Suman Anna <s-anna@ti.com> wrote:
>>> This patch adds the generic common bindings used to represent
>>> a hwlock device and use/request locks in a device-tree build.
>>>
>> ...
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>

Reviewed-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

>>> ---
>>>  .../devicetree/bindings/hwlock/hwlock.txt          | 55 ++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
>>
>> Please ask a DT maintainer to ACK this - otherwise I can't send this to Linus.
>>
>
> Can one of you ack this patch and the next patch [1] so that Ohad can
> pick up the series for 3.19? This series has been in works for quite
> some time now and was reviewed by each of you at some point of time
> (thanks!!), the cover letter [2] has the history of the changes.
>

Could we please have a statement from a DT maintainer so we could move
forward with this?

Regards,
Bjorn