omap hwspinlock dt support

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +26 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:
+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.
+
+			See documentation on struct hwspinlock_pdata in
+			linux/hwspinlock.h for more details.
+
+- 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.

drivers/hwspinlock/hwspinlock_core.c +10 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or

drivers/hwspinlock/hwspinlock_core.c +19 -0

+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	if (!ret)
+		ret = val;
+#endif
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
+
+/**
+ * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
+ * @dn: device node pointer
+ *
+ * This is an OF helper function that can be called by the underlying
+ * platform-specific implementations, to retrieve the number of locks
+ * present within a hwspinlock device instance.
+ *
+ * Returns a positive number of locks on success, -ENODEV on generic

drivers/hwspinlock/hwspinlock_core.c +11 -0

+
+	ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
+	if (!ret)
+		ret = val ? val : -ENODEV;
+#endif
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
+
+/**
 * hwspin_lock_register() - register a new hw spinlock device
 * @bank: the hwspinlock device, which usually provides numerous hw locks
 * @dev: the backing device

include/linux/hwspinlock.h +7 -4

index 3343298..1f6a5b8 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock public header
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <[email protected]>
 *

@@ -26,6 +26,7 @@
 #define HWLOCK_IRQ	0x02	/* Disable interrupts, don't save state */
 
 struct device;
+struct device_node;
 struct hwspinlock;
 struct hwspinlock_device;
 struct hwspinlock_ops;

@@ -60,6 +61,8 @@ struct hwspinlock_pdata {
 
 #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
 
+int of_hwspin_lock_get_base_id(struct device_node *dn);
+int of_hwspin_lock_get_num_locks(struct device_node *dn);
 int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 		const struct hwspinlock_ops *ops, int base_id, int num_locks);
 int hwspin_lock_unregister(struct hwspinlock_device *bank);

@@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
 * code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
 * required on a given setup, users will still work.
 *
- * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
- * we _do_ want users to fail (no point in registering hwspinlock instances if
- * the framework is not available).
+ * The only exception is hwspin_lock_register/hwspin_lock_unregister and
+ * associated OF helpers, with which we _do_ want users to fail (no point
+ * in registering hwspinlock instances if the framework is not available).
 *
 * Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
 * users. Others, which care, can still check this with IS_ERR.

Signature · 6 lines

1.8.3.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 84 lines · Suman Anna

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +26 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:
+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.
+
+			See documentation on struct hwspinlock_pdata in
+			linux/hwspinlock.h for more details.
+
+- 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.

drivers/hwspinlock/hwspinlock_core.c +14 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or
+ * an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 6 lines

drivers/hwspinlock/hwspinlock_core.c +6 -0

+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

Quoted 24 lines

drivers/hwspinlock/hwspinlock_core.c +23 -0

+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	if (!ret)
+		ret = val;
+#endif
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
+
+/**
+ * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
+ * @dn: device node pointer
+ *
+ * This is an OF helper function that can be called by the underlying
+ * platform-specific implementations, to retrieve the number of locks
+ * present within a hwspinlock device instance.
+ *
+ * Returns a positive number of locks on success, -ENODEV on generic
+ * failure or an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_num_locks(struct device_node *dn)
+{

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 6 lines

drivers/hwspinlock/hwspinlock_core.c +6 -0

+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

Quoted 61 lines

drivers/hwspinlock/hwspinlock_core.c +11 -0

+
+	ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
+	if (!ret)
+		ret = val ? val : -ENODEV;
+#endif
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
+
+/**
 * hwspin_lock_register() - register a new hw spinlock device
 * @bank: the hwspinlock device, which usually provides numerous hw locks
 * @dev: the backing device

include/linux/hwspinlock.h +7 -4

index 3343298..1f6a5b8 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock public header
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -26,6 +26,7 @@
 #define HWLOCK_IRQ	0x02	/* Disable interrupts, don't save state */
 
 struct device;
+struct device_node;
 struct hwspinlock;
 struct hwspinlock_device;
 struct hwspinlock_ops;

@@ -60,6 +61,8 @@ struct hwspinlock_pdata {
 
 #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
 
+int of_hwspin_lock_get_base_id(struct device_node *dn);
+int of_hwspin_lock_get_num_locks(struct device_node *dn);
 int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 		const struct hwspinlock_ops *ops, int base_id, int num_locks);
 int hwspin_lock_unregister(struct hwspinlock_device *bank);

@@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
 * code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
 * required on a given setup, users will still work.
 *
- * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
- * we _do_ want users to fail (no point in registering hwspinlock instances if
- * the framework is not available).
+ * The only exception is hwspin_lock_register/hwspin_lock_unregister and
+ * associated OF helpers, with which we _do_ want users to fail (no point
+ * in registering hwspinlock instances if the framework is not available).
 *
 * Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
 * users. Others, which care, can still check this with IS_ERR.

Signature · 1 line

1.8.3.3

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Kumar,

Quoted 42 lines · Kumar Gala

Quoted 39 lines · Suman Anna

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +18 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:
+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt line 26

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +8 -0

+
+			See documentation on struct hwspinlock_pdata in
+			linux/hwspinlock.h for more details.
+
+- 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.

Quoted 37 lines

Quoted 37 lines

drivers/hwspinlock/hwspinlock_core.c +14 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or
+ * an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{

Quoted 8 lines

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 6 lines

drivers/hwspinlock/hwspinlock_core.c +6 -0

+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

I have added this specifically since my intention is to differentiate the validity of the node vs the presence of the property within a node. This property may be optional for some platforms and a must for some others, and differentiating would allow the individual driver implementations to make the distinction.

Quoted 25 lines

Quoted 25 lines

drivers/hwspinlock/hwspinlock_core.c +23 -0

+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	if (!ret)
+		ret = val;
+#endif
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
+
+/**
+ * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
+ * @dn: device node pointer
+ *
+ * This is an OF helper function that can be called by the underlying
+ * platform-specific implementations, to retrieve the number of locks
+ * present within a hwspinlock device instance.
+ *
+ * Returns a positive number of locks on success, -ENODEV on generic
+ * failure or an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_num_locks(struct device_node *dn)
+{

Quoted 8 lines

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 6 lines

drivers/hwspinlock/hwspinlock_core.c +6 -0

+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

Same comment as above.

regards
Suman

Quoted 61 lines

Quoted 61 lines

drivers/hwspinlock/hwspinlock_core.c +11 -0

+
+	ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
+	if (!ret)
+		ret = val ? val : -ENODEV;
+#endif
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
+
+/**
 * hwspin_lock_register() - register a new hw spinlock device
 * @bank: the hwspinlock device, which usually provides numerous hw locks
 * @dev: the backing device

include/linux/hwspinlock.h +7 -4

index 3343298..1f6a5b8 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock public header
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -26,6 +26,7 @@
 #define HWLOCK_IRQ	0x02	/* Disable interrupts, don't save state */
 
 struct device;
+struct device_node;
 struct hwspinlock;
 struct hwspinlock_device;
 struct hwspinlock_ops;

@@ -60,6 +61,8 @@ struct hwspinlock_pdata {
 
 #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
 
+int of_hwspin_lock_get_base_id(struct device_node *dn);
+int of_hwspin_lock_get_num_locks(struct device_node *dn);
 int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
 		const struct hwspinlock_ops *ops, int base_id, int num_locks);
 int hwspin_lock_unregister(struct hwspinlock_device *bank);

@@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
 * code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
 * required on a given setup, users will still work.
 *
- * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
- * we _do_ want users to fail (no point in registering hwspinlock instances if
- * the framework is not available).
+ * The only exception is hwspin_lock_register/hwspin_lock_unregister and
+ * associated OF helpers, with which we _do_ want users to fail (no point
+ * in registering hwspinlock instances if the framework is not available).
 *
 * Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
 * users. Others, which care, can still check this with IS_ERR.

Signature · 1 line

1.8.3.3

Quoted 5 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 46 lines · Kumar Gala

Quoted 44 lines · Suman Anna

Quoted 42 lines

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +20 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:
+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.
+
+			See documentation on struct hwspinlock_pdata in

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt line 26

Quoted 6 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +6 -0

+			linux/hwspinlock.h for more details.
+
+- 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.

Was meaning to say this before, another reason for hwlock-num-locks is to limit the # of locks available to the processors running linux vs what other processors in the SoC might be using.

Quoted 48 lines

Quoted 47 lines

Quoted 35 lines

drivers/hwspinlock/hwspinlock_core.c +10 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 10 lines

drivers/hwspinlock/hwspinlock_core.c +10 -0

+ * an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{
+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

Quoted 5 lines

I have added this specifically since my intention is to differentiate the validity of the node vs the presence of the property within a node. This property may be optional for some platforms and a must for some others, and differentiating would allow the individual driver implementations to make the distinction.

Maybe add a comment for both cases.

Quoted 34 lines · Suman Anna

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +13 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:

Documentation/devicetree/bindings/hwlock/hwlock.txt line 21

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +8 -0

+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.
+
+			See documentation on struct hwspinlock_pdata in
+			linux/hwspinlock.h for more details.

Documentation/devicetree/bindings/hwlock/hwlock.txt line 26

Quoted 5 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +5 -0

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

Quoted 33 lines

drivers/hwspinlock/hwspinlock_core.c +10 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 10 lines

drivers/hwspinlock/hwspinlock_core.c +10 -0

+ * an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{
+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

drivers/hwspinlock/hwspinlock_core.c out: · line 334

Quoted 5 lines

drivers/hwspinlock/hwspinlock_core.c +5 -0

+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	if (!ret)
+		ret = val;
+#endif

Hi Suman,

Apologies for replying to a subthread, due to an earlier mistake on my part I don't have the original to hand.

Quoted 37 lines · Suman Anna

Quoted 35 lines

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +13 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt line 21

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +8 -0

+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.
+
+			See documentation on struct hwspinlock_pdata in
+			linux/hwspinlock.h for more details.

I don't like this, as it seems to be encoding a Linux implementation detail (the ID of the lock, which means that we have to have a numeric representation of each hwlock) in the device tree.

I don't see why the ID within Linux should be a concern of the device tree binding. I think that whatever internal identifier we have in Linux (be it an integer or struct) should be allocated by Linux. If a driver needs to request specific hwlocks, then we should have a phandle + args representation for referring to a specific hwlock that bidnings can use, and some code for parsing that and returning a Linux-internal identifier/struct as we do with interrupts and clocks.

Quoted 6 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt line 26

Quoted 6 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +5 -0

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

Hmm... this seems generic, but it doesn't allow for sparse ID spaces on the hwlock module. It should probably be common for the moment, but if we encounter a hwlock module with a sparse ID space, we'll need to come up with a way of handling sparse IDs (that might be device-specific).

Quoted 47 lines

Quoted 34 lines

drivers/hwspinlock/hwspinlock_core.c +10 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 10 lines

drivers/hwspinlock/hwspinlock_core.c +10 -0

+ * an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{
+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

Quoted 5 lines

drivers/hwspinlock/hwspinlock_core.c out: · line 334

Quoted 5 lines

drivers/hwspinlock/hwspinlock_core.c +5 -0

+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	if (!ret)
+		ret = val;
+#endif

Do we need the CONFIG_OF check? of_property_read_u32 is defined to return -ENOSYS if CONFIG_OF isn't defined. Or would that confuse a higher layer?

Hi Mark,

Quoted 4 lines · Mark Rutland

Hi Suman,

Apologies for replying to a subthread, due to an earlier mistake on my part I don't have the original to hand.

No issues, thanks for your review.

Quoted 52 lines · Kumar Gala

Quoted 37 lines · Suman Anna

Quoted 35 lines

All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
include/linux/hwspinlock.h                         | 11 ++--
3 files changed, 93 insertions(+), 5 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +13 -0

new file mode 100644
index 0000000..789930e
@@ -0,0 +1,26 @@
+Generic hwlock bindings
+=======================
+
+Generic bindings that are common to all the hwlock platform specific driver
+implementations, the retrieved values are used for registering the device
+specific parameters with the hwspinlock core.
+
+The validity and need of these common properties may vary from one driver
+implementation to another. Look through the individual hwlock driver
+binding documentations for identifying which are mandatory and which are
+optional for that specific driver.
+
+Common properties:

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt line 21

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +8 -0

+- hwlock-base-id:	Base Id for the locks for a particular hwlock device.
+			This property is used for representing a set of locks
+			present in a hwlock device with a unique base id in
+			the driver core. This property is mandatory ONLY if a
+			SoC has several hwlock devices.
+
+			See documentation on struct hwspinlock_pdata in
+			linux/hwspinlock.h for more details.

I don't like this, as it seems to be encoding a Linux implementation detail (the ID of the lock, which means that we have to have a numeric representation of each hwlock) in the device tree.

Quoted 7 lines

I don't see why the ID within Linux should be a concern of the device tree binding. I think that whatever internal identifier we have in Linux (be it an integer or struct) should be allocated by Linux. If a driver needs to request specific hwlocks, then we should have a phandle + args representation for referring to a specific hwlock that bidnings can use, and some code for parsing that and returning a Linux-internal identifier/struct as we do with interrupts and clocks.

This is based on gathering the information required by the platform implementation drivers for registering with the driver core. The driver core currently maintains all the locks from different instances as a radix tree, as it is simpler to manage when granting locks to users. The current version is based on allowing the platform implementation drivers to retrieve the required data for registering with the hwspinlock driver core.

The users would either get a lock dynamically by requesting any free one (and extract the id thereafter to share with others), or a specific one which is currently by id. I agree that the phandle + args approach is best suited for requesting a specific one through DT, but I would think that the args would still have to be a relative lock number from 0 w.r.t a phandle. I will look into the feasibility of such an approach retaining the radix tree implementation, as this requires new OF friendly registration and request functions.

Quoted 12 lines

Quoted 7 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt line 26

Quoted 7 lines

Documentation/devicetree/bindings/hwlock/hwlock.txt new file +5 -0

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

Hmm... this seems generic, but it doesn't allow for sparse ID spaces on the hwlock module. It should probably be common for the moment, but if we encounter a hwlock module with a sparse ID space, we'll need to come up with a way of handling sparse IDs (that might be device-specific).

Agreed.

Quoted 47 lines

Quoted 47 lines

Quoted 34 lines

drivers/hwspinlock/hwspinlock_core.c +10 -1

index 461a0d7..aec32e7 100644
@@ -1,7 +1,7 @@
 /*
 * Hardware spinlock framework
 *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
 *
 * Contact: Ohad Ben-Cohen <ohad-Ix1uc/[email protected]>
 *

@@ -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"
 

@@ -308,6 +309,64 @@ out:
 }
 
 /**
+ * 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, -ENODEV on generic failure or

drivers/hwspinlock/hwspinlock_core.c out: · line 329

Quoted 10 lines

drivers/hwspinlock/hwspinlock_core.c +10 -0

+ * an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_base_id(struct device_node *dn)
+{
+	unsigned int val;
+	int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+	if (!dn)
+		return -ENODEV;

Checking !dn is done in of_property_read_u32, so you don't need to duplicate

Quoted 9 lines

Quoted 5 lines

drivers/hwspinlock/hwspinlock_core.c out: · line 334

Quoted 5 lines

drivers/hwspinlock/hwspinlock_core.c +5 -0

+
+	ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+	if (!ret)
+		ret = val;
+#endif

Do we need the CONFIG_OF check? of_property_read_u32 is defined to return -ENOSYS if CONFIG_OF isn't defined. Or would that confuse a higher layer?

These are primarily OF helpers and provided for the SoC implementation drivers, and I have used the CONFIG_OF check within the function to streamline the function prototypes and behavior in the common hwspinlock.h header file between combinations of CONFIG_HWSPINLOCK and CONFIG_OF.

The check for !dn is done deliberately to help the implementation drivers.

HwSpinlock IP is present only on OMAP4 and other newer SoCs, which are all device-tree boot only. This patch adds the base support for parsing the DT nodes, and removes the code dealing with the traditional platform device instantiation.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +11 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs
+- reg:			Contains the hwspinlock register address range (base
+			address and length)
+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:

Quoted 36 lines · 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 <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +15 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs
+- reg:			Contains the hwspinlock register address range (base
+			address and length)
+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:
+The following describes the usage of the common hwlock properties (defined in
+Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
+
+- hwlock-base-id:	There are currently no OMAP SoCs with multiple

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 21

Quoted 6 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +6 -0

+			hwspinlock devices. The OMAP driver uses a default
+			base id value of 0 for the locks present within the
+			single hwspinlock device on all SoCs.
+- hwlock-num-locks:	This property is not required on OMAP SoCs, since the
+			number of locks present within a device can be deduced
+			from the SPINLOCK_SYSSTATUS device register.

If you are going to be explicit about this properties, you should probably be a bit more clear about them NOT being in the OMAP dts because of the reasons you specify.

Quoted 39 lines · Kumar Gala

Quoted 36 lines · 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 <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +15 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs
+- reg:			Contains the hwspinlock register address range (base
+			address and length)
+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:
+The following describes the usage of the common hwlock properties (defined in
+Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
+
+- hwlock-base-id:	There are currently no OMAP SoCs with multiple

Quoted 8 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 21

Quoted 6 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +6 -0

+			hwspinlock devices. The OMAP driver uses a default
+			base id value of 0 for the locks present within the
+			single hwspinlock device on all SoCs.
+- hwlock-num-locks:	This property is not required on OMAP SoCs, since the
+			number of locks present within a device can be deduced
+			from the SPINLOCK_SYSSTATUS device register.

If you are going to be explicit about this properties, you should probably be a bit more clear about them NOT being in the OMAP dts because of the reasons you specify.

You mean on the hwlock-base-id? I wanted to document how these properties (do not) apply for OMAP, I will revise the text to be a bit more explicit. Thanks for the review.

Quoted 15 lines · 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 <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 6

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +6 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 8

Quoted 2 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +2 -0

+- reg:			Contains the hwspinlock register address range (base
+			address and length)

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 18

Quoted 10 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +10 -0

+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:
+The following describes the usage of the common hwlock properties (defined in
+Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
+
+- hwlock-base-id:	There are currently no OMAP SoCs with multiple
+			hwspinlock devices. The OMAP driver uses a default
+			base id value of 0 for the locks present within the
+			single hwspinlock device on all SoCs.

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 21

Quoted 3 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +3 -0

+- hwlock-num-locks:	This property is not required on OMAP SoCs, since the
+			number of locks present within a device can be deduced
+			from the SPINLOCK_SYSSTATUS device register.

Quoted 18 lines · Suman Anna

Quoted 16 lines

HwSpinlock IP is present only on OMAP4 and other newer SoCs, which are all device-tree boot only. This patch adds the base support for parsing the DT nodes, and removes the code dealing with the traditional platform device instantiation.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 6

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +6 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs

"Currently supports" is not something I expect to see in a binding document. That sounds like a description of the driver rather than the binding.

How similar are these hardware modules? What are the differences?

Quoted 3 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 8

Quoted 3 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +2 -0

+- reg:			Contains the hwspinlock register address range (base
+			address and length)

Is there only one register bank for the hwlock module?

Quoted 11 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 18

Quoted 11 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +10 -0

+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:
+The following describes the usage of the common hwlock properties (defined in
+Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
+
+- hwlock-base-id:	There are currently no OMAP SoCs with multiple
+			hwspinlock devices. The OMAP driver uses a default
+			base id value of 0 for the locks present within the
+			single hwspinlock device on all SoCs.

Driver details should not leak into bindngs...

As mentioned in the other patch, I don't think this is the way to handle this. I think we need a phandle + args representation.

Quoted 4 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 21

Quoted 4 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +3 -0

+- hwlock-num-locks:	This property is not required on OMAP SoCs, since the
+			number of locks present within a device can be deduced
+			from the SPINLOCK_SYSSTATUS device register.

Huh? Why define this property at all here if we don't need it and don't use it?

The common document should state that specific bindings may use it and should explicitly state if they do, rather than stating they don't...

Hi Mark,

Quoted 36 lines · Kumar Gala

Quoted 18 lines · Suman Anna

Quoted 16 lines

HwSpinlock IP is present only on OMAP4 and other newer SoCs, which are all device-tree boot only. This patch adds the base support for parsing the DT nodes, and removes the code dealing with the traditional platform device instantiation.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 6

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +6 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs

"Currently supports" is not something I expect to see in a binding document. That sounds like a description of the driver rather than the binding.

Quoted 1 line

How similar are these hardware modules? What are the differences?

The IP is almost the same, they all have the same revision id. The number of locks (each represented by a register) though vary from one SoC to another (OMAP4, OMAP5, DRA7 have same number of locks, and AM33xx/AM43xx have a different number). The number of locks is directly read by the driver from a module register. There is no separate .data associated with the of_device_id table, so I used a single compatible property for all the SoCs.

Quoted 6 lines

Quoted 4 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 8

Quoted 4 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +2 -0

+- reg:			Contains the hwspinlock register address range (base
+			address and length)

Is there only one register bank for the hwlock module?

The lock registers start at a certain offset (0x800) within the module register space, and the offsets for various registers are identical between all SoCs.

Quoted 15 lines

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 18

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +10 -0

+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:
+The following describes the usage of the common hwlock properties (defined in
+Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
+
+- hwlock-base-id:	There are currently no OMAP SoCs with multiple
+			hwspinlock devices. The OMAP driver uses a default
+			base id value of 0 for the locks present within the
+			single hwspinlock device on all SoCs.

Driver details should not leak into bindngs...

OK, will remove the info on driver details.

Quoted 1 line

Quoted 2 lines

As mentioned in the other patch, I don't think this is the way to handle this. I think we need a phandle + args representation.

This is an optional parameter for now and I was going to revise the description based on comments from Kumar Gala on this thread, but I will wait and adjust this based on the outcome on the first patch.

Quoted 9 lines

Quoted 5 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 21

Quoted 5 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +3 -0

+- hwlock-num-locks:	This property is not required on OMAP SoCs, since the
+			number of locks present within a device can be deduced
+			from the SPINLOCK_SYSSTATUS device register.

Huh? Why define this property at all here if we don't need it and don't use it?

Quoted 2 lines

The common document should state that specific bindings may use it and should explicitly state if they do, rather than stating they don't...

Yeah, I wasn't sure how to go about the split between the common file and the platform-specific bindings. I will clean this up and revise the common bindings.

Quoted 39 lines

Quoted 37 lines · Kumar Gala

Quoted 18 lines · Suman Anna

Quoted 16 lines

HwSpinlock IP is present only on OMAP4 and other newer SoCs, which are all device-tree boot only. This patch adds the base support for parsing the DT nodes, and removes the code dealing with the traditional platform device instantiation.

Signed-off-by Suman Anna <[email protected]>

---

.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/mach-omap2/Makefile                       |  3 --
arch/arm/mach-omap2/hwspinlock.c                   | 60 ----------------------
drivers/hwspinlock/omap_hwspinlock.c               | 23 +++++++--
4 files changed, 50 insertions(+), 67 deletions(-)

create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 6

Quoted 12 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +6 -0

new file mode 100644
index 0000000..235b7c5
@@ -0,0 +1,31 @@
+OMAP4+ HwSpinlock Driver
+========================
+
+Required properties:
+- compatible:		Currently supports only "ti,omap4-hwspinlock" for
+				OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs

"Currently supports" is not something I expect to see in a binding document. That sounds like a description of the driver rather than the binding.

Quoted 1 line

How similar are these hardware modules? What are the differences?

Quoted 7 lines

The IP is almost the same, they all have the same revision id. The number of locks (each represented by a register) though vary from one SoC to another (OMAP4, OMAP5, DRA7 have same number of locks, and AM33xx/AM43xx have a different number). The number of locks is directly read by the driver from a module register. There is no separate .data associated with the of_device_id table, so I used a single compatible property for all the SoCs.

Ok. Probeability is good, it keeps these simpler :)

I think This can be reworded to say "should contain" rather than "currently supports only":

- compatible: Should contain "ti,omap4-hwspinlock" for

			OMAP44xx, OMAP54xx, AM33xx, AM43xx, or DRA7xx SoCs

That way the binding allows for a future backwards-compatible variant, and doesn't mention the current level of support in Linux.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 8

Quoted 5 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +2 -0

+- reg:			Contains the hwspinlock register address range (base
+			address and length)

Is there only one register bank for the hwlock module?

Quoted 3 lines

The lock registers start at a certain offset (0x800) within the module register space, and the offsets for various registers are identical between all SoCs.

What are the other registers within the module? Are they shared with other devices, or are they simply unused by the hwspinlock driver?

Quoted 20 lines

Quoted 16 lines

Quoted 13 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt line 18

Quoted 13 lines

Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file +10 -0

+- ti,hwmods:		Name of the hwmod associated with the hwspinlock device
+
+Common hwlock properties:
+The following describes the usage of the common hwlock properties (defined in
+Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP.
+
+- hwlock-base-id:	There are currently no OMAP SoCs with multiple
+			hwspinlock devices. The OMAP driver uses a default
+			base id value of 0 for the locks present within the
+			single hwspinlock device on all SoCs.

Driver details should not leak into bindngs...

OK, will remove the info on driver details.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

As mentioned in the other patch, I don't think this is the way to handle this. I think we need a phandle + args representation.

This is an optional parameter for now and I was going to revise the description based on comments from Kumar Gala on this thread, but I will wait and adjust this based on the outcome on the first patch.

Ok.

Add the hwmod data for the spinlock IP in OMAP5 SoC. This is needed to be able to enable the OMAP spinlock support for OMAP5.

Signed-off-by Suman Anna <[email protected]>

Quoted 5 lines · Suman Anna

Add the hwmod data for the spinlock IP in OMAP5 SoC. This is needed to be able to enable the OMAP spinlock support for OMAP5.

Signed-off-by Suman Anna <[email protected]>

Thanks, queued. You can omit this one from future reposts of this series.

Add the missing sysc configuration to the AM335 spinlock hwmod data. This ensures that smart-idle is enabled whenever the module is enabled by the driver.

Signed-off-by Suman Anna <[email protected]>

Hi,

This is an updated series for adding the device tree support to the OMAP hwspinlock driver. The series is based on 3.12-rc1, and includes patches on hwspinlock driver, OMAP hwmod data files and OMAP DTS files. The updated series adds new patches to enable the hwspinlock driver on OMAP5 and AM335x SoCs as well.

Tested this on Panda4, BeagleBone Black and OMAP5 uEVM (with out-of-tree clock patches from Tero).

Changes new in v2:
- Added a new common DT binding documentation and OF helpers (Patch1), addressing the review comments from v1 [2]. The MSM support [1] needs to be reworked on top of this common patch.
- Revised OMAP DT parse support to use the new OF helper (Patch2)
- OMAP5 hwspinlock support including the hwmod entry and DT node (Patches 4, 5)
- Add AM335x support to OMAP hwspinlock driver, including a fix needed in driver given that AM335 spinlock module requires s/w wakeup (Patches 6, 7)
- AM335 DT node for spinlock, and a hwmod change to enable smart-idle for AM335 (Patches 8, 9). The sysc patch is not essential for AM335 spinlock functionality, but is needed for smart-idling the IP when the module is enabled.
- OMAP4 DT node patch is unchanged (Patch 3)

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

[1] https://lkml.org/lkml/2013/8/14/528
[2] http://marc.info/?t=137823090300005&r=1&w=2

Suman Anna (9):
hwspinlock/core: add common dt bindings and OF helpers hwspinlock/omap: add support for dt nodes
ARM: dts: OMAP4: Add hwspinlock node
ARM: OMAP5: hwmod data: Add spinlock data
ARM: dts: OMAP5: Add hwspinlock node
hwspinlock/omap: support AM33xx
hwspinlock/omap: enable module before reading SYSSTATUS register
ARM: dts: AM33XX: Add hwspinlock node
ARM: AM33xx: hwmod_data: add the sysc configuration for spinlock

 .../devicetree/bindings/hwlock/hwlock.txt          | 26 +++++++++
 .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
 arch/arm/boot/dts/am33xx.dtsi                      |  6 +++
 arch/arm/boot/dts/omap4.dtsi                       |  6 +++
 arch/arm/boot/dts/omap5.dtsi                       |  6 +++
 arch/arm/mach-omap2/Makefile                       |  3 --
 arch/arm/mach-omap2/hwspinlock.c                   | 60 ---------------------
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c         | 13 +++++
 arch/arm/mach-omap2/omap_hwmod_54xx_data.c         | 44 ++++++++++++++++

 drivers/hwspinlock/Kconfig                         |  2 +-
 drivers/hwspinlock/hwspinlock_core.c               | 61 +++++++++++++++++++++-
 drivers/hwspinlock/omap_hwspinlock.c               | 44 ++++++++++++----
 include/linux/hwspinlock.h                         | 11 ++--
 13 files changed, 233 insertions(+), 80 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

Paul,
The hwmod data patches needs to be merged only after the respective DT node patches are merged, without which the hwmod entry will not have a base address while enabling and idling (using sysc) the hwmod during hwmod initialization.

Hi

Quoted 5 lines · Suman Anna

Paul,
The hwmod data patches needs to be merged only after the respective DT node patches are merged, without which the hwmod entry will not have a base address while enabling and idling (using sysc) the hwmod during hwmod initialization.

Hmm, ideally there shouldn't be a merge dependency here. Could you please send a patch to the hwmod core that logs a warning and skips the operation in these circumstances? Or maybe just skips the registration of the device?