Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Ohad, On 07/04/2014 12:01 AM, Ohad Ben-Cohen wrote: Hi Suman, On Thu, Jul 3, 2014 at 8:28 PM, Suman Anna s-a...@ti.com wrote: OK, but we would still require this function to lookup the registered device from the controller-phandle to retrieve the base_id. Can we retrieve the base_id from the parent DT node itself? Yeah, it is possible using the of_hwspin_lock_get_base_id function added in Patch8, once we convert the phandle to a device node. The platform implementations would be using this function during the registration phase to register the base id, and we have to do the DT parse/lookup again in the core. regards Suman -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna s-a...@ti.com wrote: I'm not sure we need this patch. This patch is needed if we use the controller-phandle + args specifier for requesting hwlocks by a client, as we need to translate controller-phandle to the corresponding hwspinlock_device. Looks like we still don't have a closure on the semantics of how clients have to request a lock in DT. You are suggesting something like hwlocks = global_lock1 global_lock2 ...; whereas this patch is built to support based on comments from DT-maintainers, hwlocks = controller-phandle lock-specifier1, controller-phandle lock-specifier2...; I'm actually ok with this suggestion and haven't suggested otherwise. All I propose is that we add the base_id property to the controller node (as you have done in the subsequent patches), and then drivers will be able to infer the global lock id from the DT data by adding the controller's base_id to the lock specifier. Controllers with non standard lock indexing may use an xlate() method if needed but frankly this is fictional right now. We can start without this, and add it later when needed, as this doesn't affect the DT data. With the global lock id in hand, drivers could simply use the existing hwspin_lock_request_specific API to obtain a specific lock, and then we don't need this patch. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Ohad, On 07/03/2014 02:00 AM, Ohad Ben-Cohen wrote: Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna s-a...@ti.com wrote: I'm not sure we need this patch. This patch is needed if we use the controller-phandle + args specifier for requesting hwlocks by a client, as we need to translate controller-phandle to the corresponding hwspinlock_device. Looks like we still don't have a closure on the semantics of how clients have to request a lock in DT. You are suggesting something like hwlocks = global_lock1 global_lock2 ...; whereas this patch is built to support based on comments from DT-maintainers, hwlocks = controller-phandle lock-specifier1, controller-phandle lock-specifier2...; I'm actually ok with this suggestion and haven't suggested otherwise. OK, thanks for confirming and sorry for the misinterpretation. All I propose is that we add the base_id property to the controller node (as you have done in the subsequent patches), and then drivers will be able to infer the global lock id from the DT data by adding the controller's base_id to the lock specifier. OK, but we would still require this function to lookup the registered device from the controller-phandle to retrieve the base_id. Do note that the hwspinlock core currently only maintains the registered locks in an integrated radix tree, but not the registered hwspinlock banks themselves. regards Suman Controllers with non standard lock indexing may use an xlate() method if needed but frankly this is fictional right now. We can start without this, and add it later when needed, as this doesn't affect the DT data. With the global lock id in hand, drivers could simply use the existing hwspin_lock_request_specific API to obtain a specific lock, and then we don't need this patch. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Suman, On Thu, Jul 3, 2014 at 8:28 PM, Suman Anna s-a...@ti.com wrote: OK, but we would still require this function to lookup the registered device from the controller-phandle to retrieve the base_id. Can we retrieve the base_id from the parent DT node itself? Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Ohad, On 07/01/2014 07:26 AM, Ohad Ben-Cohen wrote: Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna s-a...@ti.com wrote: The hwspinlock_device structure is used for registering a bank of locks with the driver core. The structure already contains the necessary members to identify the bank of locks. The core does not maintain the hwspinlock_devices itself, but maintains only a radix tree for all the registered locks. A specific lock can be requested by users using a global lock id, and any device-specific fields can be retrieved through a reference to the hwspinlock_device in each lock. The global lock id, however, is not friendly to be requested for users using the device-tree model. The device-tree representation will typically have each of the hwspinlock devices represented as a DT node, and a specific lock can be requested using the device's phandle and a lock specifier. Add support to the core therefore to maintain all the registered hwspinlock_devices, so that a device can be looked up and a specific lock belonging to the device requested through a phandle + args approach. Signed-off-by: Suman Anna s-a...@ti.com I'm not sure we need this patch. This patch is needed if we use the controller-phandle + args specifier for requesting hwlocks by a client, as we need to translate controller-phandle to the corresponding hwspinlock_device. Looks like we still don't have a closure on the semantics of how clients have to request a lock in DT. You are suggesting something like hwlocks = global_lock1 global_lock2 ...; whereas this patch is built to support based on comments from DT-maintainers, hwlocks = controller-phandle lock-specifier1, controller-phandle lock-specifier2...; Mark, Kumar, We need your input here as DT maintainers. Some of the discussion is on the v4 cover-letter thread [1]. Kumar, Josh, How does this fit with the MSM spinlock driver? It seems to me that the global lock id can be the base_id + lock index, where the former should be a property of the parent dt node, and the latter can just be the phandle argument. Then, with the global lock id in hand, drivers could just use the existing hwspin_lock_request_specific API. If future hardware will bring a more complex scenario, we could then introduce the xlate proposal to resolve it. As long as we're not changing the dt data, and this is all handled by kernel code, Once we define dt data one way, how can we support a different mechanism later on? Are you implying that we support both controller-phandle + specifier and global-lock convention somehow through driver changes? then I'd prefer opting for less code now as long as it addresses the requirements. Please let me know if currently there is a use case that can't be addressed using this simpler model. This is just a question of DT semantics for the longer term, it can be done both ways. I have started out the series with exactly what you are suggesting here. regards Suman [1] https://lkml.org/lkml/2014/3/17/576 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna s-a...@ti.com wrote: The hwspinlock_device structure is used for registering a bank of locks with the driver core. The structure already contains the necessary members to identify the bank of locks. The core does not maintain the hwspinlock_devices itself, but maintains only a radix tree for all the registered locks. A specific lock can be requested by users using a global lock id, and any device-specific fields can be retrieved through a reference to the hwspinlock_device in each lock. The global lock id, however, is not friendly to be requested for users using the device-tree model. The device-tree representation will typically have each of the hwspinlock devices represented as a DT node, and a specific lock can be requested using the device's phandle and a lock specifier. Add support to the core therefore to maintain all the registered hwspinlock_devices, so that a device can be looked up and a specific lock belonging to the device requested through a phandle + args approach. Signed-off-by: Suman Anna s-a...@ti.com I'm not sure we need this patch. It seems to me that the global lock id can be the base_id + lock index, where the former should be a property of the parent dt node, and the latter can just be the phandle argument. Then, with the global lock id in hand, drivers could just use the existing hwspin_lock_request_specific API. If future hardware will bring a more complex scenario, we could then introduce the xlate proposal to resolve it. As long as we're not changing the dt data, and this is all handled by kernel code, then I'd prefer opting for less code now as long as it addresses the requirements. Please let me know if currently there is a use case that can't be addressed using this simpler model. Thanks, Ohad. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv5 03/15] hwspinlock/core: maintain a list of registered hwspinlock banks
The hwspinlock_device structure is used for registering a bank of
locks with the driver core. The structure already contains the
necessary members to identify the bank of locks. The core does not
maintain the hwspinlock_devices itself, but maintains only a radix
tree for all the registered locks. A specific lock can be requested
by users using a global lock id, and any device-specific fields
can be retrieved through a reference to the hwspinlock_device in
each lock.
The global lock id, however, is not friendly to be requested for
users using the device-tree model. The device-tree representation
will typically have each of the hwspinlock devices represented as
a DT node, and a specific lock can be requested using the device's
phandle and a lock specifier. Add support to the core therefore to
maintain all the registered hwspinlock_devices, so that a device
can be looked up and a specific lock belonging to the device
requested through a phandle + args approach.
Signed-off-by: Suman Anna s-a...@ti.com
---
Documentation/hwspinlock.txt | 2 ++
drivers/hwspinlock/hwspinlock_core.c | 51
drivers/hwspinlock/hwspinlock_internal.h | 2 ++
3 files changed, 55 insertions(+)
diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
index 62f7d4e..640ae47 100644
--- a/Documentation/hwspinlock.txt
+++ b/Documentation/hwspinlock.txt
@@ -251,6 +251,7 @@ implementation using the hwspin_lock_register() API.
/**
* struct hwspinlock_device - a device which usually spans numerous hwspinlocks
+ * @list: list element to link hwspinlock devices together
* @dev: underlying device, will be used to invoke runtime PM api
* @ops: platform-specific hwspinlock handlers
* @base_id: id index of the first lock in this device
@@ -258,6 +259,7 @@ implementation using the hwspin_lock_register() API.
* @lock: dynamically allocated array of 'struct hwspinlock'
*/
struct hwspinlock_device {
+ struct list_head list;
struct device *dev;
const struct hwspinlock_ops *ops;
int base_id;
diff --git a/drivers/hwspinlock/hwspinlock_core.c
b/drivers/hwspinlock/hwspinlock_core.c
index 461a0d7..48f7866 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -59,6 +59,11 @@ static RADIX_TREE(hwspinlock_tree, GFP_KERNEL);
*/
static DEFINE_MUTEX(hwspinlock_tree_lock);
+/*
+ * A linked list for maintaining all the registered hwspinlock devices.
+ * The list is maintained in an ordered-list of the supported locks group.
+ */
+static LIST_HEAD(hwspinlock_devices);
/**
* __hwspin_trylock() - attempt to lock a specific hwspinlock
@@ -307,6 +312,40 @@ out:
return hwlock;
}
+/*
+ * Add a new hwspinlock device to the global list, keeping the list of
+ * devices sorted by base order.
+ *
+ * Returns 0 on success, or -EBUSY if the new device overlaps with some
+ * other device's lock space.
+ */
+static int hwspinlock_device_add(struct hwspinlock_device *bank)
+{
+ struct list_head *entry = hwspinlock_devices;
+ struct hwspinlock_device *_bank;
+ int ret = 0;
+
+ list_for_each(entry, hwspinlock_devices) {
+ _bank = list_entry(entry, struct hwspinlock_device, list);
+ if (_bank-base_id = bank-base_id + bank-num_locks)
+ break;
+ }
+
+ if (entry != hwspinlock_devices
+ entry-prev != hwspinlock_devices) {
+ _bank = list_entry(entry-prev, struct hwspinlock_device, list);
+ if (_bank-base_id + _bank-num_locks bank-base_id) {
+ dev_err(bank-dev, hwlock space overlap, cannot add
device\n);
+ ret = -EBUSY;
+ }
+ }
+
+ if (!ret)
+ list_add_tail(bank-list, entry);
+
+ return ret;
+}
+
/**
* hwspin_lock_register() - register a new hw spinlock device
* @bank: the hwspinlock device, which usually provides numerous hw locks
@@ -339,6 +378,12 @@ int hwspin_lock_register(struct hwspinlock_device *bank,
struct device *dev,
bank-base_id = base_id;
bank-num_locks = num_locks;
+ mutex_lock(hwspinlock_tree_lock);
+ ret = hwspinlock_device_add(bank);
+ mutex_unlock(hwspinlock_tree_lock);
+ if (ret)
+ return ret;
+
for (i = 0; i num_locks; i++) {
hwlock = bank-lock[i];
@@ -355,6 +400,9 @@ int hwspin_lock_register(struct hwspinlock_device *bank,
struct device *dev,
reg_failed:
while (--i = 0)
hwspin_lock_unregister_single(base_id + i);
+ mutex_lock(hwspinlock_tree_lock);
+ list_del(bank-list);
+ mutex_unlock(hwspinlock_tree_lock);
return ret;
}
EXPORT_SYMBOL_GPL(hwspin_lock_register);
@@ -386,6 +434,9 @@ int hwspin_lock_unregister(struct hwspinlock_device *bank)
WARN_ON(tmp != hwlock);
}
+ mutex_lock(hwspinlock_tree_lock);
+
