[PATCH v7 0/4] hwspinlock core & omap dt support

[PATCH v7 0/4] hwspinlock core & omap dt support

[Suman Anna]

Hi Ohad,

This is an updated version of the hwspinlock dt support series,
rebased onto v3.19-rc3 and mainly addresses the continued discussion
on the need to maintain a list of registered spinlock banks [1].
I have removed this patch as per your wish, and as a result the
burden of the spinlock validation and deferred probing is pushed
onto the hwspinlock clients. Sorry for the delay in refreshing this
series, hopefully this can be the last revision.

Following are the main changes in v7 w.r.t v6:
- Dropped the patch "hwspinlock/core: maintain a list of registered
  hwspinlock banks"
- Updated generic hwspinlock bindings to make hwlock-base-id property
  mandatory.
- Updated the OMAP hwspinlock binding and DT support patches to correct
  for the mandatory hwlock-base-id property.
- Updated the common OF helpers patch to move the of_hwspin_lock_get_base_id()
  and of_hwspin_lock_get_num_locks() functions into the internal header,
  these are no longer exported, but available for platform implementations.
  of_hwspin_lock_get_id() is also simplified now.

The validation logs on all the applicable OMAP SoCs are at:
  OMAP4  - http://slexy.org/view/s21Mh1SqP8
  OMAP5  - http://slexy.org/view/s21TYWxoKu
  DRA74x - http://slexy.org/view/s2dQdghLPr
  DRA72x - http://slexy.org/view/s2QajhhWYu
  AM33xx - http://slexy.org/view/s21DvKQOpa
  AM43xx - http://slexy.org/view/s21L3YB95Q

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

The test branch also includes a required patch that adds the hwlock-base-id
to all the OMAP hwspinlock DT nodes (will submit that patch separately for
Tony to pick it up).

Bjorn,
I didn't add your Tested-by: or Reviewed-by as I have modified the
series a bit. Care to give those once again, thanks.

regards
Suman

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

---
v6:
http://marc.info/?l=linux-omap&m=141055365213895&w=2
- 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.
- Updated the OMAP hwspinlock DT support patch to assign base-id
  from DT if present
- RFC patches adding the concept of reserved locks and return code
  change convention dropped.

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 (4):
  Documentation: dt: add common bindings for hwspinlock
  Documentation: dt: add the omap hwspinlock bindings document
  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 | 28 ++++++++++
 Documentation/hwspinlock.txt                       | 25 +++++++++
 MAINTAINERS                                        |  1 -
 arch/arm/mach-omap2/Makefile                       |  3 -
 arch/arm/mach-omap2/hwspinlock.c                   | 60 --------------------
 drivers/hwspinlock/hwspinlock_core.c               | 65 ++++++++++++++++++++++
 drivers/hwspinlock/hwspinlock_internal.h           | 47 ++++++++++++++++
 drivers/hwspinlock/omap_hwspinlock.c               | 22 ++++++--
 include/linux/hwspinlock.h                         |  7 +++
 10 files changed, 244 insertions(+), 69 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.2.1

[PATCH v7 1/4] 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>
---
v7: Revised binding info for hwlock-base-id, it is mandatory now

 .../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 000000000000..8de7aaf878f9
--- /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 for all platform
+			implementations.
+
+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.2.1

[PATCH v7 2/4] 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>
---
v7: Added information about hwlock-base-id and updated example to use it

 .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 28 ++++++++++++++++++++++
 1 file changed, 28 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 000000000000..935173ec6b58
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
@@ -0,0 +1,28 @@
+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-base-id:	Should be 0 for the first IP block instance, or a proper
+			positive value for any subsequent instance (if present)
+			of the IP block.
+- #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-base-id = <0>;
+	#hwlock-cells = <1>;
+};
-- 
2.2.1

[PATCH v7 3/4] hwspinlock/core: add common OF helpers

[Suman Anna]

This patch adds two new OF helper functions for platform
implementations and one new API 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>
---
v7:
- Moved of_hwspin_lock_get_base_id() and of_hwspin_lock_get_num_locks
  into hwspinlock_internal.h
- Simplified of_hwspin_lock_get_id(), removed deferred probing and
  args specifier validation
- updated comments and documentation

 Documentation/hwspinlock.txt             | 25 ++++++++++++
 drivers/hwspinlock/hwspinlock_core.c     | 65 ++++++++++++++++++++++++++++++++
 drivers/hwspinlock/hwspinlock_internal.h | 47 +++++++++++++++++++++++
 include/linux/hwspinlock.h               |  7 ++++
 4 files changed, 144 insertions(+)

diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
index 62f7d4ea6e26..a29bb47e4637 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 lock id number on success, or other error
+     values. The function does not perform any validation of the args
+     specifier lock values, this burden is placed on the user.
+     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,21 @@ 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
+     hwspinlock core.
+
+  int of_hwspin_lock_get_base_id(struct device_node *dn);
+   - is a common OF helper function that can be used by the underlying
+     vendor-specific implementations. This function should be called by
+     implementations to retrieve the base index for a block of locks present
+     within a hwspinlock device for registering that device with the
+     hwspinlock 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 461a0d739d75..8f107bc34281 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"
 
@@ -257,6 +258,70 @@ 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 relative index of the lock within a specified bank on success,
+ * or -EINVAL on invalid specifier cell count.
+ */
+static inline int
+of_hwspin_lock_simple_xlate(const struct of_phandle_args *hwlock_spec)
+{
+	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, -EINVAL on invalid args
+ * specifier count or an appropriate error as returned from the OF parsing
+ * logic.
+ */
+int of_hwspin_lock_get_id(struct device_node *np, int index)
+{
+	struct of_phandle_args args;
+	int id, base_id;
+	int ret;
+
+	ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells", index,
+					 &args);
+	if (ret)
+		return ret;
+
+	id = of_hwspin_lock_simple_xlate(&args);
+	if (id < 0) {
+		ret = id;
+		goto out;
+	}
+
+	base_id = of_hwspin_lock_get_base_id(args.np);
+	if (base_id < 0) {
+		ret = base_id;
+		goto out;
+	}
+	id += base_id;
+
+out:
+	of_node_put(args.np);
+	return ret ? ret : id;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_id);
+
 static int hwspin_lock_register_single(struct hwspinlock *hwlock, int id)
 {
 	struct hwspinlock *tmp;
diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index d26f78b8f214..7c8b148761f0 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -20,6 +20,7 @@
 
 #include <linux/spinlock.h>
 #include <linux/device.h>
+#include <linux/of.h>
 
 struct hwspinlock_device;
 
@@ -74,4 +75,50 @@ static inline int hwlock_to_id(struct hwspinlock *hwlock)
 	return hwlock->bank->base_id + local_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
+ */
+static inline 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;
+}
+
+/**
+ * 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 a value
+ * of zero locks or an appropriate error code as returned by the OF layer
+ */
+static inline 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;
+}
+
 #endif /* __HWSPINLOCK_HWSPINLOCK_H */
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 3343298e40e8..859d673d98c8 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;
@@ -66,6 +67,7 @@ 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 *);
@@ -120,6 +122,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.2.1

[PATCH v7 4/4] 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>
---
v7: hwlock-base-id is no longer optional, probe will fail now
    if property is absent.

 MAINTAINERS                          |  1 -
 arch/arm/mach-omap2/Makefile         |  3 --
 arch/arm/mach-omap2/hwspinlock.c     | 60 ------------------------------------
 drivers/hwspinlock/omap_hwspinlock.c | 22 ++++++++++---
 4 files changed, 17 insertions(+), 69 deletions(-)
 delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ddb9ac8d32b3..3de03037e90a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6861,7 +6861,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 5d27dfdef66b..6fa36846d5b4 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -282,9 +282,6 @@ obj-y					+= $(nand-m) $(nand-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 ef175acaeaa2..000000000000
--- 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 47a275c6ece1..18edb3b4ab32 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-2015 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.h>
 #include <linux/platform_device.h>
 
 #include "hwspinlock_internal.h"
@@ -80,16 +81,20 @@ 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;
 
+	base_id = of_hwspin_lock_get_base_id(node);
+	if (base_id < 0)
+		return base_id;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
@@ -141,7 +146,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,11 +179,18 @@ 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",
+		.of_match_table = of_match_ptr(omap_hwspinlock_of_match),
 	},
 };
 
-- 
2.2.1

Re: [PATCH v7 0/4] hwspinlock core & omap dt support

[Ohad Ben-Cohen]

Hi Suman,

On Wed, Jan 14, 2015 at 10:58 PM, Suman Anna <s-anna@ti.com> wrote:
> This is an updated version of the hwspinlock dt support series,
> rebased onto v3.19-rc3 and mainly addresses the continued discussion
> on the need to maintain a list of registered spinlock banks [1].
> I have removed this patch as per your wish, and as a result the
> burden of the spinlock validation and deferred probing is pushed
> onto the hwspinlock clients. Sorry for the delay in refreshing this
> series, hopefully this can be the last revision.
>
> Following are the main changes in v7 w.r.t v6:
> - Dropped the patch "hwspinlock/core: maintain a list of registered
>   hwspinlock banks"
> - Updated generic hwspinlock bindings to make hwlock-base-id property
>   mandatory.
> - Updated the OMAP hwspinlock binding and DT support patches to correct
>   for the mandatory hwlock-base-id property.
> - Updated the common OF helpers patch to move the of_hwspin_lock_get_base_id()
>   and of_hwspin_lock_get_num_locks() functions into the internal header,
>   these are no longer exported, but available for platform implementations.
>   of_hwspin_lock_get_id() is also simplified now.

This looks good, thanks.

Please try to get an ACK from a DT maintainer on the first two dt
patches, and then I can take the patches to Linus.

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
> 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>
> ---
> v7: Revised binding info for hwlock-base-id, it is mandatory now
> 
>  .../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 000000000000..8de7aaf878f9
> --- /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 for all platform
> +			implementations.

This property makes no sense. The ID encoded in the hwlock cells is
relative to the instance (identified by phandle), not global. So the DT
has no global ID space.

Why do you think you need this?

The definition here isn't sufficient, and the example is incomplete
(lacking provider nodes and hence this property), which is unhelpful.

Thanks,
Mark.

> +
> +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.2.1
> 
> 

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
> > 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>
> > ---
> > v7: Revised binding info for hwlock-base-id, it is mandatory now
> > 
> >  .../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 000000000000..8de7aaf878f9
> > --- /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 for all platform
> > +			implementations.
> 
> This property makes no sense. The ID encoded in the hwlock cells is
> relative to the instance (identified by phandle), not global. So the DT
> has no global ID space.
> 
> Why do you think you need this?

Having looked at the way this proeprty is used, NAK.

If you need to carve up a Linux-internal ID space, do that dynamically.
There is no need for this property.

Mark.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Rob Herring]

On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
>> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
>> > 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>
>> > ---
>> > v7: Revised binding info for hwlock-base-id, it is mandatory now
>> >
>> >  .../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 000000000000..8de7aaf878f9
>> > --- /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 for all platform
>> > +                   implementations.
>>
>> This property makes no sense. The ID encoded in the hwlock cells is
>> relative to the instance (identified by phandle), not global. So the DT
>> has no global ID space.
>>
>> Why do you think you need this?
>
> Having looked at the way this proeprty is used, NAK.
>
> If you need to carve up a Linux-internal ID space, do that dynamically.
> There is no need for this property.

Better yet, don't create a Linux ID space for this. Everywhere we have
one, we want to get rid of it.

Rob

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

On 01/15/2015 08:42 AM, Rob Herring wrote:
> On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
>>> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
>>>> 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>
>>>> ---
>>>> v7: Revised binding info for hwlock-base-id, it is mandatory now
>>>>
>>>>  .../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 000000000000..8de7aaf878f9
>>>> --- /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 for all platform
>>>> +                   implementations.
>>>
>>> This property makes no sense. The ID encoded in the hwlock cells is
>>> relative to the instance (identified by phandle), not global. So the DT
>>> has no global ID space.
>>>
>>> Why do you think you need this?
>>
>> Having looked at the way this proeprty is used, NAK.
>>
>> If you need to carve up a Linux-internal ID space, do that dynamically.
>> There is no need for this property.
> 
> Better yet, don't create a Linux ID space for this. Everywhere we have
> one, we want to get rid of it.

Hi Mark, Rob,

Thank you for your comments. The id space is not limited to Linux, as
this is indeed a global id space for all the processors in an SoC, as
that is the primary usage of the individual locks in these IP blocks. It
needs to be static across the SoC irrespective of whether a node was
enabled or not. Now, it is debatable whether we do this in DT or do this
in each individual implementation. This is something that the hwspinlock
core cannot do, and probably would have to be done as static
driver match data.

I have gone from having this as an optional property, to not having it,
and now back to having it as mandatory - because of the different
perspectives from bindings vs driver subsystem maintainers. I brought
this back mainly based on the reasonings in [1]

Ohad,
Do you have any comments or suggestions here? It looks like I have no
choice but to bring back "hwspinlock/core: maintain a list of registered
hwspinlock banks", if we were to have the bindings without a
hwlock-base-id property.

regards
Suman

[1] http://marc.info/?l=linux-omap&m=139510004009415&w=2

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

On Thu, Jan 15, 2015 at 4:42 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
>>> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
>>> > +- hwlock-base-id:  An unique base Id for the locks for a particular hwlock
>>> > +                   device. This property is mandatory for all platform
>>> > +                   implementations.
>>>
>>> This property makes no sense. The ID encoded in the hwlock cells is
>>> relative to the instance (identified by phandle), not global. So the DT
>>> has no global ID space.
>>>
>>> Why do you think you need this?
>>
>> Having looked at the way this proeprty is used, NAK.
>>
>> If you need to carve up a Linux-internal ID space, do that dynamically.
>> There is no need for this property.
>
> Better yet, don't create a Linux ID space for this. Everywhere we have
> one, we want to get rid of it.

Rob, Mark,

The hwlock is a basic hardware primitive that allow synchronization
between different processors in the system, which may be running Linux
as well as other operating systems, and may have no other means of
communication.

The hwlock id numbers are predefined, global and static across the
entire system: Linux may boot well after other operating systems are
already running and using these hwlocks to communicate, and therefore,
in order to use these hardware devices, it must not enumerate them
differently than the rest of the system.

Given that these id numbers are global, system-wide, static and
predefined, where Linux may just be one user of them, please
reconsider the approach as implemented by Suman, or suggest an
alternative one you may prefer.

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

On Fri, Jan 16, 2015 at 06:09:00AM +0000, Ohad Ben-Cohen wrote:
> On Thu, Jan 15, 2015 at 4:42 PM, Rob Herring <robherring2@gmail.com> wrote:
> > On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
> >>> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
> >>> > +- hwlock-base-id:  An unique base Id for the locks for a particular hwlock
> >>> > +                   device. This property is mandatory for all platform
> >>> > +                   implementations.
> >>>
> >>> This property makes no sense. The ID encoded in the hwlock cells is
> >>> relative to the instance (identified by phandle), not global. So the DT
> >>> has no global ID space.
> >>>
> >>> Why do you think you need this?
> >>
> >> Having looked at the way this proeprty is used, NAK.
> >>
> >> If you need to carve up a Linux-internal ID space, do that dynamically.
> >> There is no need for this property.
> >
> > Better yet, don't create a Linux ID space for this. Everywhere we have
> > one, we want to get rid of it.
> 
> Rob, Mark,
> 
> The hwlock is a basic hardware primitive that allow synchronization
> between different processors in the system, which may be running Linux
> as well as other operating systems, and may have no other means of
> communication.
> 
> The hwlock id numbers are predefined, global and static across the
> entire system: Linux may boot well after other operating systems are
> already running and using these hwlocks to communicate, and therefore,
> in order to use these hardware devices, it must not enumerate them
> differently than the rest of the system.

That's not true.

In order to communicate it must agree with the other users as to the
meaning of each instance, and the protocol for use. That doesn't
necessarily mean that Linux needs to know the numerical ID from a
datasheet, and regardless that ID is separate from the logical ID Linux
uses internally.

> Given that these id numbers are global, system-wide, static and
> predefined, where Linux may just be one user of them, please
> reconsider the approach as implemented by Suman, or suggest an
> alternative one you may prefer.

To communicate with the other processors, Linux will need to understand
the protocol. So there will need to be nodes in the DT describing the
protocol. Those nodes can refer to the relevant locks using phandle +
args (with a hwlock-names list if indexing is not appropriate). The
entire point of the hwlock-cells is to allow individual locks to be
referred to in this way.

Thanks,
Mark.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

On Thu, Jan 15, 2015 at 02:42:23PM +0000, Rob Herring wrote:
> On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
> >> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
> >> > 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>
> >> > ---
> >> > v7: Revised binding info for hwlock-base-id, it is mandatory now
> >> >
> >> >  .../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 000000000000..8de7aaf878f9
> >> > --- /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 for all platform
> >> > +                   implementations.
> >>
> >> This property makes no sense. The ID encoded in the hwlock cells is
> >> relative to the instance (identified by phandle), not global. So the DT
> >> has no global ID space.
> >>
> >> Why do you think you need this?
> >
> > Having looked at the way this proeprty is used, NAK.
> >
> > If you need to carve up a Linux-internal ID space, do that dynamically.
> > There is no need for this property.
> 
> Better yet, don't create a Linux ID space for this. Everywhere we have
> one, we want to get rid of it.

Agreed. A completely opaque token / desc structure would prevent a lot
of potential abuse and save us from painful breakage.

Mark

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

On 01/16/2015 04:19 AM, Mark Rutland wrote:
> On Thu, Jan 15, 2015 at 02:42:23PM +0000, Rob Herring wrote:
>> On Thu, Jan 15, 2015 at 7:55 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> On Thu, Jan 15, 2015 at 01:52:01PM +0000, Mark Rutland wrote:
>>>> On Wed, Jan 14, 2015 at 08:58:18PM +0000, Suman Anna wrote:
>>>>> 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>
>>>>> ---
>>>>> v7: Revised binding info for hwlock-base-id, it is mandatory now
>>>>>
>>>>>  .../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 000000000000..8de7aaf878f9
>>>>> --- /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 for all platform
>>>>> +                   implementations.
>>>>
>>>> This property makes no sense. The ID encoded in the hwlock cells is
>>>> relative to the instance (identified by phandle), not global. So the DT
>>>> has no global ID space.
>>>>
>>>> Why do you think you need this?
>>>
>>> Having looked at the way this proeprty is used, NAK.
>>>
>>> If you need to carve up a Linux-internal ID space, do that dynamically.
>>> There is no need for this property.
>>
>> Better yet, don't create a Linux ID space for this. Everywhere we have
>> one, we want to get rid of it.
> 
> Agreed. A completely opaque token / desc structure would prevent a lot
> of potential abuse and save us from painful breakage.

The regular API to acquire or release a lock on Linux does indeed use
opaque handles, but the id space is needed for communication with other
processors as the handles have no meaning on non-Linux processors. The
id space is the simplest to exchange which lock to use with other
processors compared to the device instance plus the lock number within
that device.

regards
Suman

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

Mark,

On Fri, Jan 16, 2015 at 12:17 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> The hwlock is a basic hardware primitive that allow synchronization
>> between different processors in the system, which may be running Linux
>> as well as other operating systems, and may have no other means of
>> communication.
>>
>> The hwlock id numbers are predefined, global and static across the
>> entire system: Linux may boot well after other operating systems are
>> already running and using these hwlocks to communicate, and therefore,
>> in order to use these hardware devices, it must not enumerate them
>> differently than the rest of the system.
>
> That's not true.
>
> In order to communicate it must agree with the other users as to the
> meaning of each instance, and the protocol for use. That doesn't
> necessarily mean that Linux needs to know the numerical ID from a
> datasheet, and regardless that ID is separate from the logical ID Linux
> uses internally.

Let me describe hwspinlocks a bit more so we all get to know it better
and can then agree on a proper solution.

- What makes handling of hwspinlock ID numbers convenient is the fact
that it's not based on random datasheet numbers. In fact, hwspinlocks
is just special memory: usually datasheets just define the base
address and the size of the hwspinlock area. So any numerical ID we
use to call the locks themselves are already logical and sane, similar
to the way we handle memory (i.e. if we have 32 locks we'll always use
0..31). So hwlocks ids are very much like memory addressing, and not
irq numbers.

- Sometimes Linux will have to dynamically allocate a hwlock, and send
the ID of the allocated lock to a remote processor (which may not be
running Linux).
- Sometimes a remote processor, which may not be running Linux, will
have to dynamically allocate a hwlock, and send the ID of the
allocated lock to us (another processor running Linux)

We cannot tell in advance what kind of IPC is going to be used for
sending and receiving this hwlock ID. Some are handled by Linux
(kernel) and some by the user space. So we must be able to expose an
ID the system will understand as well as receive one.

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Tony Lindgren]

* Ohad Ben-Cohen <ohad@wizery.com> [150116 16:50]:
> Mark,
> 
> On Fri, Jan 16, 2015 at 12:17 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> The hwlock is a basic hardware primitive that allow synchronization
> >> between different processors in the system, which may be running Linux
> >> as well as other operating systems, and may have no other means of
> >> communication.
> >>
> >> The hwlock id numbers are predefined, global and static across the
> >> entire system: Linux may boot well after other operating systems are
> >> already running and using these hwlocks to communicate, and therefore,
> >> in order to use these hardware devices, it must not enumerate them
> >> differently than the rest of the system.
> >
> > That's not true.
> >
> > In order to communicate it must agree with the other users as to the
> > meaning of each instance, and the protocol for use. That doesn't
> > necessarily mean that Linux needs to know the numerical ID from a
> > datasheet, and regardless that ID is separate from the logical ID Linux
> > uses internally.
> 
> Let me describe hwspinlocks a bit more so we all get to know it better
> and can then agree on a proper solution.
> 
> - What makes handling of hwspinlock ID numbers convenient is the fact
> that it's not based on random datasheet numbers. In fact, hwspinlocks
> is just special memory: usually datasheets just define the base
> address and the size of the hwspinlock area. So any numerical ID we
> use to call the locks themselves are already logical and sane, similar
> to the way we handle memory (i.e. if we have 32 locks we'll always use
> 0..31). So hwlocks ids are very much like memory addressing, and not
> irq numbers.
> 
> - Sometimes Linux will have to dynamically allocate a hwlock, and send
> the ID of the allocated lock to a remote processor (which may not be
> running Linux).
> - Sometimes a remote processor, which may not be running Linux, will
> have to dynamically allocate a hwlock, and send the ID of the
> allocated lock to us (another processor running Linux)
> 
> We cannot tell in advance what kind of IPC is going to be used for
> sending and receiving this hwlock ID. Some are handled by Linux
> (kernel) and some by the user space. So we must be able to expose an
> ID the system will understand as well as receive one.

How about default to Linux id space and allow overriding that with
a module param option if needed?

Regards,

Tony

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

On Tue, Jan 20, 2015 at 8:05 PM, Tony Lindgren <tony@atomide.com> wrote:
> How about default to Linux id space and allow overriding that with
> a module param option if needed?

I'm not sure I'm following.

If the main point of contention is the base_id field, I'm also fine
with removing it entirely, as I'm not aware of any actual user for it
(Suman please confirm?).

Mark? Rob? Will you accept Suman's patches if the base_id field is removed?

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

On 01/21/2015 06:41 AM, Ohad Ben-Cohen wrote:
> On Tue, Jan 20, 2015 at 8:05 PM, Tony Lindgren <tony@atomide.com> wrote:
>> How about default to Linux id space and allow overriding that with
>> a module param option if needed?
> 
> I'm not sure I'm following.
> 
> If the main point of contention is the base_id field, I'm also fine
> with removing it entirely, as I'm not aware of any actual user for it
> (Suman please confirm?).

Yeah, well the current implementations that I am aware of only have a
single bank, so all of them would be using a value of 0. I am yet to see
a platform with multiple instances where the property really makes a
difference. v7 has the property mandatory, so all the implementations
would need to define this value even if it is 0.

regards
Suman

> 
> Mark? Rob? Will you accept Suman's patches if the base_id field is removed?
> 
> Thanks,
> Ohad.
> 

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

On Wed, Jan 21, 2015 at 05:56:37PM +0000, Suman Anna wrote:
> On 01/21/2015 06:41 AM, Ohad Ben-Cohen wrote:
> > On Tue, Jan 20, 2015 at 8:05 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> How about default to Linux id space and allow overriding that with
> >> a module param option if needed?
> > 
> > I'm not sure I'm following.
> > 
> > If the main point of contention is the base_id field, I'm also fine
> > with removing it entirely, as I'm not aware of any actual user for it
> > (Suman please confirm?).
> 
> Yeah, well the current implementations that I am aware of only have a
> single bank, so all of them would be using a value of 0. I am yet to see
> a platform with multiple instances where the property really makes a
> difference. v7 has the property mandatory, so all the implementations
> would need to define this value even if it is 0.
> 
> regards
> Suman
> 
> > 
> > Mark? Rob? Will you accept Suman's patches if the base_id field is removed?

My concern is that the mapping of hwspinlock IDs doesn't seem to be
explicit in the DT on a per-context basis, which is what I'd expect.

e.g.

lck: hwspinlock-device@f00 {
	...
	#hwlock-cells = <1>;
};

some-other-os-interface {
	...
	hwlocks = <&lck 0>, <&lck 1>, <&lck 2>, <&lck 3>;
	hwlock-names = "glbl", "pool0", "pool1", "pool2";
};

a-different-os-interface {
	...
	hwlocks = <&lck 18>, <&lck 21>, <&lck 4>, <&lck 5>;
	hwlock-names = "init", "teardown", "pool0", "pool1";
};

That's the only way I would expect this to possibly remain a stable
over time, and it's the entire reason for #hwlock-cells, no?

How do you expect the other components sharing the hwspinlocks to be
described?

Thanks,
Mark.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

On 01/22/2015 12:56 PM, Mark Rutland wrote:
> On Wed, Jan 21, 2015 at 05:56:37PM +0000, Suman Anna wrote:
>> On 01/21/2015 06:41 AM, Ohad Ben-Cohen wrote:
>>> On Tue, Jan 20, 2015 at 8:05 PM, Tony Lindgren <tony@atomide.com> wrote:
>>>> How about default to Linux id space and allow overriding that with
>>>> a module param option if needed?
>>>
>>> I'm not sure I'm following.
>>>
>>> If the main point of contention is the base_id field, I'm also fine
>>> with removing it entirely, as I'm not aware of any actual user for it
>>> (Suman please confirm?).
>>
>> Yeah, well the current implementations that I am aware of only have a
>> single bank, so all of them would be using a value of 0. I am yet to see
>> a platform with multiple instances where the property really makes a
>> difference. v7 has the property mandatory, so all the implementations
>> would need to define this value even if it is 0.
>>
>> regards
>> Suman
>>
>>>
>>> Mark? Rob? Will you accept Suman's patches if the base_id field is removed?
> 
> My concern is that the mapping of hwspinlock IDs doesn't seem to be
> explicit in the DT on a per-context basis, which is what I'd expect.
> 
> e.g.
> 
> lck: hwspinlock-device@f00 {
> 	...
> 	#hwlock-cells = <1>;
> };
> 
> some-other-os-interface {
> 	...
> 	hwlocks = <&lck 0>, <&lck 1>, <&lck 2>, <&lck 3>;
> 	hwlock-names = "glbl", "pool0", "pool1", "pool2";
> };
> 
> a-different-os-interface {
> 	...
> 	hwlocks = <&lck 18>, <&lck 21>, <&lck 4>, <&lck 5>;
> 	hwlock-names = "init", "teardown", "pool0", "pool1";
> };
> 
> That's the only way I would expect this to possibly remain a stable
> over time, and it's the entire reason for #hwlock-cells, no?
> 
> How do you expect the other components sharing the hwspinlocks to be
> described?

Yes indeed, this is what any of the clients will use on Linux. But this is not necessarily the semantics for exchanging hwlocks with the other processor(s) which is where the global id space comes into picture.

regards
Suman

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Fri, Jan 16, 2015 at 4:46 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Mark,
>
> On Fri, Jan 16, 2015 at 12:17 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>> The hwlock is a basic hardware primitive that allow synchronization
>>> between different processors in the system, which may be running Linux
>>> as well as other operating systems, and may have no other means of
>>> communication.
>>>
>>> The hwlock id numbers are predefined, global and static across the
>>> entire system: Linux may boot well after other operating systems are
>>> already running and using these hwlocks to communicate, and therefore,
>>> in order to use these hardware devices, it must not enumerate them
>>> differently than the rest of the system.
>>
>> That's not true.
>>
>> In order to communicate it must agree with the other users as to the
>> meaning of each instance, and the protocol for use. That doesn't
>> necessarily mean that Linux needs to know the numerical ID from a
>> datasheet, and regardless that ID is separate from the logical ID Linux
>> uses internally.
>
> Let me describe hwspinlocks a bit more so we all get to know it better
> and can then agree on a proper solution.
>
> - What makes handling of hwspinlock ID numbers convenient is the fact
> that it's not based on random datasheet numbers. In fact, hwspinlocks
> is just special memory: usually datasheets just define the base
> address and the size of the hwspinlock area. So any numerical ID we
> use to call the locks themselves are already logical and sane, similar
> to the way we handle memory (i.e. if we have 32 locks we'll always use
> 0..31). So hwlocks ids are very much like memory addressing, and not
> irq numbers.
>

But that's exactly how irqs or gpios work as well. If you have 32
gpios in a system they used to be numbered 0-31 and people would
reference them directly by that number. Every one of the systems that
was designed in this way is moving away from it.

> - Sometimes Linux will have to dynamically allocate a hwlock, and send
> the ID of the allocated lock to a remote processor (which may not be
> running Linux).

In a system where you have two hwlock blocks lckA and lckB, each
consisting of 8 locks and you have dspB that can only access lckB;
will you tell the firmware engineers to always subtract 8 from the
numbers you pass them?

Wouldn't it make much more sense to have local indexes here and pass
them e.g lckB:2?

> - Sometimes a remote processor, which may not be running Linux, will
> have to dynamically allocate a hwlock, and send the ID of the
> allocated lock to us (another processor running Linux)
>

I'm sorry but you cannot have a system on both sides that is allowed
to do dynamic allocation from a limited set of resources.

Further more this dynamic allocation leads to interesting race
conditions as what happens if you dynamically allocate a hwlock that
is statically allocated by another part of the system?
The only solution I can think of is to have a static allocation of ids
that the dynamic allocator might use, and then we're just carrying
extra code when the system is already statically configured...

> We cannot tell in advance what kind of IPC is going to be used for
> sending and receiving this hwlock ID. Some are handled by Linux
> (kernel) and some by the user space. So we must be able to expose an
> ID the system will understand as well as receive one.
>

Designing this interface to take into consideration that someone might
send us something completely crazy isn't productive.


The only reason for having num-locks and base-id in device tree is
because of the current Linux implementation. base-id is not a property
of the hardware and num-locks is not needed for anything but book
keeping of base-id's in the hwlock framework.

This is why I preferred Sumans earlier suggestion of having the
binding consist of #hwlock-cells = <X> and the necessary accessor
functions for resolving a hwlock based on a dt reference.

Regards,
Bjorn

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> In a system where you have two hwlock blocks lckA and lckB, each
> consisting of 8 locks and you have dspB that can only access lckB

This is a good example - thanks. To be able to cope with such cases we
will have to pass a hwlock block reference and its relative lock id.

The DT binding should definitely be prepared for such cases (just kill
the base-id field?), but let's see what it means about the Linux
implementation.

Since the existence of several hwblocks is still fictional (Bjorn,
please confirm too?), we may prefer to introduce changes to support it
only when it shows up; it all depends on the amount of changes needed.
Suman, care to take a look please?

>> - Sometimes a remote processor, which may not be running Linux, will
>> have to dynamically allocate a hwlock, and send the ID of the
>> allocated lock to us (another processor running Linux)
>>
> I'm sorry but you cannot have a system on both sides that is allowed
> to do dynamic allocation from a limited set of resources.

Of course not. On such systems, Linux is not the one responsible for
allocating the hwlocks, at least not during part of the time or from
part of the hwlocks. There were a few different use cases, with
different semantics, that required communicating to Linux an hwlock
id, but since none of them have reached mainline, we should only
remember they may show up one day, but not put too much effort to
support them right now.

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

On Sat, Jan 31, 2015 at 7:41 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> > In a system where you have two hwlock blocks lckA and lckB, each
> > consisting of 8 locks and you have dspB that can only access lckB
>
> This is a good example - thanks. To be able to cope with such cases we
> will have to pass a hwlock block reference and its relative lock id.

Additionally, to support such a scenario, we can no longer retain the
simple dynamic allocation API we have today, because it might end up
allocating dspB an hwlock from IckA.

We will have to make sure hwlocks are allocated only from pools
visible to the user, something that will change not only the
hwspinlock API but also the way it maintains the hwlocks.

I suspect we want to wait for such hardware to show up first, and only
then add framework support for it. Regardless, we obviously do want to
make sure our DT binding is prepared for the worse, so we'll drop the
"base-id" field.

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>> In a system where you have two hwlock blocks lckA and lckB, each
>> consisting of 8 locks and you have dspB that can only access lckB
>
> This is a good example - thanks. To be able to cope with such cases we
> will have to pass a hwlock block reference and its relative lock id.
>

Correct, so the #hwlock-cells and hwlock part from the proposal are
the important one. Having an optional hwlock-names will make things
easier to read as well, but is not necessary.

> The DT binding should definitely be prepared for such cases (just kill
> the base-id field?), but let's see what it means about the Linux
> implementation.
>

>From the dt binding PoV, we should be able to skip num-locks as well.
It seems most hwlock blocks have a fixed amount of locks provided and
the drivers are reporting this to the core when registering.

So I think we can reduce the binding to:

Providers:
#hwlock-cells

Consumers:
hwlocks
hwlock-names

For the hardware where number of locks is actually variable (e.g.
different variants of same block) there can be driver specific entries
for this.

> Since the existence of several hwblocks is still fictional (Bjorn,
> please confirm too?), we may prefer to introduce changes to support it
> only when it shows up; it all depends on the amount of changes needed.
> Suman, care to take a look please?
>

I haven't seen any such systems yet.

We could introduce the logic found in other subsystems of allowing -1
as base_id and having the core find a available range. This will work
for all cases where the global ids doesn't have to be static; either
due to the fact that we use block:local-id or that the indices are
hard coded. (Referencing locks via dt is equivalent of the
block:local-id case)

>>> - Sometimes a remote processor, which may not be running Linux, will
>>> have to dynamically allocate a hwlock, and send the ID of the
>>> allocated lock to us (another processor running Linux)
>>>
>> I'm sorry but you cannot have a system on both sides that is allowed
>> to do dynamic allocation from a limited set of resources.
>
> Of course not. On such systems, Linux is not the one responsible for
> allocating the hwlocks, at least not during part of the time or from
> part of the hwlocks. There were a few different use cases, with
> different semantics, that required communicating to Linux an hwlock
> id, but since none of them have reached mainline, we should only
> remember they may show up one day, but not put too much effort to
> support them right now.
>

That makes more sense, although I still believe that you end up with a
system wide setup where locks are statically allocated in some
document beforehand. Either that or you will have to feed that other
system with the list of constraints.

Non the less, that's "unrelated".

If we get an incoming message saying lckX:Y (or the global lckZ), we
probably wouldn't define that in DT. We would need a way to resolve
the hwlock-block "lckX" so we can request lock Y from that block. We
would basically build the DT reference on the fly.

I think this is a future problem to be solved and more importantly I
don't think it's limited to hwlocks. If a system architect designs a
system where a remote entity will do allocation of resources for us,
they will most likely do so not only for hwlocks but also for gpios,
irqs and other resources that we today reference with arguments in DT.
Hopefully that will not happen anytime soon, so let's just ignore that
problem for now and go for a simple binding that will cover todays use
cases (with some thought into multi-block support).

Regards,
Bjorn

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

On 02/01/2015 11:55 AM, Bjorn Andersson wrote:
> On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>>> In a system where you have two hwlock blocks lckA and lckB, each
>>> consisting of 8 locks and you have dspB that can only access lckB
>>
>> This is a good example - thanks. To be able to cope with such cases we
>> will have to pass a hwlock block reference and its relative lock id.
>>
> 
> Correct, so the #hwlock-cells and hwlock part from the proposal are
> the important one. Having an optional hwlock-names will make things
> easier to read as well, but is not necessary.

Right, if anything, it would be useful only for the clients, but the
hwspinlock core itself would not need it. So, I would forgo adding the
hwlock-names for now.

> 
>> The DT binding should definitely be prepared for such cases (just kill
>> the base-id field?), but let's see what it means about the Linux
>> implementation.
>>
> 
> From the dt binding PoV, we should be able to skip num-locks as well.
> It seems most hwlock blocks have a fixed amount of locks provided and
> the drivers are reporting this to the core when registering.

I added this originally based on the initial MSM HW Mutex block bindings.

> 
> So I think we can reduce the binding to:
> 
> Providers:
> #hwlock-cells
> 
> Consumers:
> hwlocks
> hwlock-names
> 
> For the hardware where number of locks is actually variable (e.g.
> different variants of same block) there can be driver specific entries
> for this.

Right, we should be able to drop this and use the driver match data. As
it is, the field is used during registration of the block with the
hwspinlock core.

regards
Suman

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

On 02/01/2015 05:00 AM, Ohad Ben-Cohen wrote:
> On Sat, Jan 31, 2015 at 7:41 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>>> In a system where you have two hwlock blocks lckA and lckB, each
>>> consisting of 8 locks and you have dspB that can only access lckB
>>
>> This is a good example - thanks. To be able to cope with such cases we
>> will have to pass a hwlock block reference and its relative lock id.

So, you mean lckB is only between the host and dspB. Obviously, if it
were shared between dspA and dspB only, then the allocation and
management would be completely outside the host Linux driver's scope.

> 
> Additionally, to support such a scenario, we can no longer retain the
> simple dynamic allocation API we have today, because it might end up
> allocating dspB an hwlock from IckA.
> 
> We will have to make sure hwlocks are allocated only from pools
> visible to the user, something that will change not only the
> hwspinlock API but also the way it maintains the hwlocks.

Right, the current API definitely will not scale for that. It was
designed around the concept that it's easier to exchange a single global
id, rather than a lcbB:id or some other similar semantics that needs to
be interpreted.

> 
> I suspect we want to wait for such hardware to show up first, and only
> then add framework support for it. 

Agreed.

regards
Suman

> Regardless, we obviously do want to
> make sure our DT binding is prepared for the worse, so we'll drop the
> "base-id" field.
> 
> Thanks,
> Ohad.
> 

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Mon, Feb 2, 2015 at 1:07 PM, Suman Anna <s-anna@ti.com> wrote:
> On 02/01/2015 11:55 AM, Bjorn Andersson wrote:
>> On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>> On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>>>> In a system where you have two hwlock blocks lckA and lckB, each
>>>> consisting of 8 locks and you have dspB that can only access lckB
>>>
>>> This is a good example - thanks. To be able to cope with such cases we
>>> will have to pass a hwlock block reference and its relative lock id.
>>>
>>
>> Correct, so the #hwlock-cells and hwlock part from the proposal are
>> the important one. Having an optional hwlock-names will make things
>> easier to read as well, but is not necessary.
>
> Right, if anything, it would be useful only for the clients, but the
> hwspinlock core itself would not need it. So, I would forgo adding the
> hwlock-names for now.
>
>>
>>> The DT binding should definitely be prepared for such cases (just kill
>>> the base-id field?), but let's see what it means about the Linux
>>> implementation.
>>>
>>
>> From the dt binding PoV, we should be able to skip num-locks as well.
>> It seems most hwlock blocks have a fixed amount of locks provided and
>> the drivers are reporting this to the core when registering.
>
> I added this originally based on the initial MSM HW Mutex block bindings.
>

It's not entirely correct to have this in DT for the MSM HW, as the
hardware has a fixed number of mutexes. As soon as we have the binding
sorted out I will follow up with a new revision of the tcsr/sfpb-mutex
driver.

>>
>> So I think we can reduce the binding to:
>>
>> Providers:
>> #hwlock-cells
>>
>> Consumers:
>> hwlocks
>> hwlock-names
>>
>> For the hardware where number of locks is actually variable (e.g.
>> different variants of same block) there can be driver specific entries
>> for this.
>
> Right, we should be able to drop this and use the driver match data. As
> it is, the field is used during registration of the block with the
> hwspinlock core.
>

If we have certain systems where it actually is a property to be
configured then they can have individual properties, extending the
standard set. Either way, it's not a dynamic property shared by all
hwlock drivers, so it should not be in the common binding.

Will you send out a new revision of the binding? I would love to get
this integrated so I can move on with the dependents.

Regards,
Bjorn

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Suman Anna]

Hi Bjorn,

On 02/05/2015 05:01 PM, Bjorn Andersson wrote:
> On Mon, Feb 2, 2015 at 1:07 PM, Suman Anna <s-anna@ti.com> wrote:
>> On 02/01/2015 11:55 AM, Bjorn Andersson wrote:
>>> On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>>>> On Sat, Jan 31, 2015 at 1:29 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>>>>> In a system where you have two hwlock blocks lckA and lckB, each
>>>>> consisting of 8 locks and you have dspB that can only access lckB
>>>>
>>>> This is a good example - thanks. To be able to cope with such cases we
>>>> will have to pass a hwlock block reference and its relative lock id.
>>>>
>>>
>>> Correct, so the #hwlock-cells and hwlock part from the proposal are
>>> the important one. Having an optional hwlock-names will make things
>>> easier to read as well, but is not necessary.
>>
>> Right, if anything, it would be useful only for the clients, but the
>> hwspinlock core itself would not need it. So, I would forgo adding the
>> hwlock-names for now.
>>
>>>
>>>> The DT binding should definitely be prepared for such cases (just kill
>>>> the base-id field?), but let's see what it means about the Linux
>>>> implementation.
>>>>
>>>
>>> From the dt binding PoV, we should be able to skip num-locks as well.
>>> It seems most hwlock blocks have a fixed amount of locks provided and
>>> the drivers are reporting this to the core when registering.
>>
>> I added this originally based on the initial MSM HW Mutex block bindings.
>>
> 
> It's not entirely correct to have this in DT for the MSM HW, as the
> hardware has a fixed number of mutexes. As soon as we have the binding
> sorted out I will follow up with a new revision of the tcsr/sfpb-mutex
> driver.
> 
>>>
>>> So I think we can reduce the binding to:
>>>
>>> Providers:
>>> #hwlock-cells
>>>
>>> Consumers:
>>> hwlocks
>>> hwlock-names
>>>
>>> For the hardware where number of locks is actually variable (e.g.
>>> different variants of same block) there can be driver specific entries
>>> for this.
>>
>> Right, we should be able to drop this and use the driver match data. As
>> it is, the field is used during registration of the block with the
>> hwspinlock core.
>>
> 
> If we have certain systems where it actually is a property to be
> configured then they can have individual properties, extending the
> standard set. Either way, it's not a dynamic property shared by all
> hwlock drivers, so it should not be in the common binding.
> 
> Will you send out a new revision of the binding? I would love to get
> this integrated so I can move on with the dependents.

Yep, will do as soon as I hear from Ohad on what to do with the patch
"hwspinlock/core: maintain a list of registered hwspinlock banks" that I
dropped from v7. Without that and dropping hwlock-base-id, I can't get
the translations done.

regards
Suman

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Thu, Feb 5, 2015 at 4:11 PM, Suman Anna <s-anna@ti.com> wrote:
> Hi Bjorn,
>
> On 02/05/2015 05:01 PM, Bjorn Andersson wrote:
[..]
>> Will you send out a new revision of the binding? I would love to get
>> this integrated so I can move on with the dependents.
>
> Yep, will do as soon as I hear from Ohad on what to do with the patch
> "hwspinlock/core: maintain a list of registered hwspinlock banks" that I
> dropped from v7. Without that and dropping hwlock-base-id, I can't get
> the translations done.
>

My suggestion is to replace the global id-tree with a list of hwlocks
and iterate over these if we ever get more than one driver registered.
As long as #hwlock-drivers < log(#total-hwlocks) this should be
faster.

I would however argue that clients that would notice any kind of
difference are using the API incorrectly.


Unfortunately this is a somewhat larger change than just slapping DT
support on the framework :/

Regards,
Bjorn

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Ohad Ben-Cohen]

On Fri, Feb 6, 2015 at 2:34 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
>> Yep, will do as soon as I hear from Ohad on what to do with the patch
>> "hwspinlock/core: maintain a list of registered hwspinlock banks" that I
>> dropped from v7. Without that and dropping hwlock-base-id, I can't get
>> the translations done.
>>
>
> My suggestion is to replace the global id-tree with a list of hwlocks
> and iterate over these if we ever get more than one driver registered.
> As long as #hwlock-drivers < log(#total-hwlocks) this should be
> faster.
>
> I would however argue that clients that would notice any kind of
> difference are using the API incorrectly.
>
> Unfortunately this is a somewhat larger change than just slapping DT
> support on the framework :/

I suspect we want to wait with framework changes until there's a real
hardware use case justifying them.

With regard to DT, however, we obviously do want to be prepared for
multiple hwlock blocks even if they do not exist today.

So how about adopting an approach where:
- DT fully supports multi hwlock blocks (i.e. no global id field)
- Framework left mostly unchanged at the moment. This means issuing an
explicit error in case a secondary hwlock block shows up, and then
hwlock id could trivially be the lock index.

If multi hwlock hardware use case, or some new hwlock id translation
requirements, do show up in the future, it's always easy to add
framework support for it. At that point we'll know better what the
requirements are, and framework changes would be justifiable.

Thanks,
Ohad.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

On Thu, Jan 29, 2015 at 03:58:42AM +0000, Suman Anna wrote:
> On 01/22/2015 12:56 PM, Mark Rutland wrote:
> > On Wed, Jan 21, 2015 at 05:56:37PM +0000, Suman Anna wrote:
> >> On 01/21/2015 06:41 AM, Ohad Ben-Cohen wrote:
> >>> On Tue, Jan 20, 2015 at 8:05 PM, Tony Lindgren <tony@atomide.com> wrote:
> >>>> How about default to Linux id space and allow overriding that with
> >>>> a module param option if needed?
> >>>
> >>> I'm not sure I'm following.
> >>>
> >>> If the main point of contention is the base_id field, I'm also fine
> >>> with removing it entirely, as I'm not aware of any actual user for it
> >>> (Suman please confirm?).
> >>
> >> Yeah, well the current implementations that I am aware of only have a
> >> single bank, so all of them would be using a value of 0. I am yet to see
> >> a platform with multiple instances where the property really makes a
> >> difference. v7 has the property mandatory, so all the implementations
> >> would need to define this value even if it is 0.
> >>
> >> regards
> >> Suman
> >>
> >>>
> >>> Mark? Rob? Will you accept Suman's patches if the base_id field is removed?
> > 
> > My concern is that the mapping of hwspinlock IDs doesn't seem to be
> > explicit in the DT on a per-context basis, which is what I'd expect.
> > 
> > e.g.
> > 
> > lck: hwspinlock-device@f00 {
> > 	...
> > 	#hwlock-cells = <1>;
> > };
> > 
> > some-other-os-interface {
> > 	...
> > 	hwlocks = <&lck 0>, <&lck 1>, <&lck 2>, <&lck 3>;
> > 	hwlock-names = "glbl", "pool0", "pool1", "pool2";
> > };
> > 
> > a-different-os-interface {
> > 	...
> > 	hwlocks = <&lck 18>, <&lck 21>, <&lck 4>, <&lck 5>;
> > 	hwlock-names = "init", "teardown", "pool0", "pool1";
> > };
> > 
> > That's the only way I would expect this to possibly remain a stable
> > over time, and it's the entire reason for #hwlock-cells, no?
> > 
> > How do you expect the other components sharing the hwspinlocks to be
> > described?
> 
> Yes indeed, this is what any of the clients will use on Linux. But
> this is not necessarily the semantics for exchanging hwlocks with the
> other processor(s) which is where the global id space comes into
> picture.

I did try to consider that above. Rather than thinking about the
numbering as "global", think of it as unique within the a given pool
shared between processors. That's what the "poolN" names are about
above.

That way you can dynamically allocate within the pool and know that
Linux and the SW on the other processors will use the same ID. You can
have pools that span multiple hwlock hardware blocks, and you can have
multiple separate pools in operation at once.

Surely that covers the cases you care about?

If using names is clunky, we could instead have a pool-hwlocks property
for that purpose.

Thanks,
Mark.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Mark Rutland]

Hi,

Sorry I've been away from this thread for a while.

On Wed, Feb 11, 2015 at 10:29:37AM +0000, Ohad Ben-Cohen wrote:
> On Fri, Feb 6, 2015 at 2:34 AM, Bjorn Andersson <bjorn@kryo.se> wrote:
> >> Yep, will do as soon as I hear from Ohad on what to do with the patch
> >> "hwspinlock/core: maintain a list of registered hwspinlock banks" that I
> >> dropped from v7. Without that and dropping hwlock-base-id, I can't get
> >> the translations done.
> >>
> >
> > My suggestion is to replace the global id-tree with a list of hwlocks
> > and iterate over these if we ever get more than one driver registered.
> > As long as #hwlock-drivers < log(#total-hwlocks) this should be
> > faster.
> >
> > I would however argue that clients that would notice any kind of
> > difference are using the API incorrectly.
> >
> > Unfortunately this is a somewhat larger change than just slapping DT
> > support on the framework :/
> 
> I suspect we want to wait with framework changes until there's a real
> hardware use case justifying them.
> 
> With regard to DT, however, we obviously do want to be prepared for
> multiple hwlock blocks even if they do not exist today.
> 
> So how about adopting an approach where:
> - DT fully supports multi hwlock blocks (i.e. no global id field)
> - Framework left mostly unchanged at the moment. This means issuing an
> explicit error in case a secondary hwlock block shows up, and then
> hwlock id could trivially be the lock index.
> 
> If multi hwlock hardware use case, or some new hwlock id translation
> requirements, do show up in the future, it's always easy to add
> framework support for it. At that point we'll know better what the
> requirements are, and framework changes would be justifiable.

As mentioned in my other reply I think we need to be explicit now when
defining the set of hwlocks (and their namming/numbering) shared between
a given set of processors, as we do with other resources
(GPIOs/regulators/whatever).

We also need to be explicit in describing the set of actors which use
those locks.

I think my previous proposal covered both of those.

Thanks,
Mark.

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Wed, Feb 11, 2015 at 3:29 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 29, 2015 at 03:58:42AM +0000, Suman Anna wrote:
>> On 01/22/2015 12:56 PM, Mark Rutland wrote:
[..]
>> > That's the only way I would expect this to possibly remain a stable
>> > over time, and it's the entire reason for #hwlock-cells, no?
>> >
>> > How do you expect the other components sharing the hwspinlocks to be
>> > described?
>>
>> Yes indeed, this is what any of the clients will use on Linux. But
>> this is not necessarily the semantics for exchanging hwlocks with the
>> other processor(s) which is where the global id space comes into
>> picture.
>
> I did try to consider that above. Rather than thinking about the
> numbering as "global", think of it as unique within the a given pool
> shared between processors. That's what the "poolN" names are about
> above.
>
> That way you can dynamically allocate within the pool and know that
> Linux and the SW on the other processors will use the same ID. You can
> have pools that span multiple hwlock hardware blocks, and you can have
> multiple separate pools in operation at once.
>
> Surely that covers the cases you care about?
>
> If using names is clunky, we could instead have a pool-hwlocks property
> for that purpose.
>

Just to make I understand your suggestion.

We would have the communication entity list all the potential hwlocks
(and gpios etc) that it can share and the key to be communicated would
then basically be the index in that list?

Like:
awesome-hub {
    pool-hwlocks = <&a 1>, <&a 3>, <&b 5>;
};

And a communicated "lock 2" would mean lock 3 from block a?


This would make it possible to describe what locks are available in
this "allocation pool" and would keep such allocation logic out from
the hwlock core - as the awesome-hub driver could simply trial and
error (with some logic) through the list.

Is this understanding correct?

Regards,
Bjorn

Re: [PATCH v7 1/4] Documentation: dt: add common bindings for hwspinlock

[Bjorn Andersson]

On Fri, Jan 30, 2015 at 9:41 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
[..]
> Since the existence of several hwblocks is still fictional (Bjorn,
> please confirm too?), we may prefer to introduce changes to support it
> only when it shows up; it all depends on the amount of changes needed.
> Suman, care to take a look please?
>

It turns out that the Qualcomm platforms have two additional "remote
spinlock" mechanisms - both using shared memory.

On most platforms only 1 (out of these 3) is actually consumed, but
e.g. the older msm7x30 uses 2 of them (SMEM and DAL). Due to its age I
don't think it's on anyones todo list to actually support this
platform as of today; but it's out there.


None of these hwlocks are allocated dynamically, so if needed a
dynamic base_id (-1 like other frameworks) would be a acceptable
solution - and can easily be implemented when we need it.

Regards,
Bjorn