| Message ID | 1398904476-26200-4-git-send-email-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna <s-anna@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-anna@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.
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-anna@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-anna@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
Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna <s-anna@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.
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-anna@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. >
Hi Suman, On Thu, Jul 3, 2014 at 8:28 PM, Suman Anna <s-anna@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.
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-anna@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
diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt index 62f7d4e..640ae47 100644 --- a/Documentation/hwspinlock.txt +++ b/Documentation/hwspinlock.txt @@ -251,6 +251,7 @@ implementation using the hwspin_lock_register() API. /** * struct hwspinlock_device - a device which usually spans numerous hwspinlocks + * @list: list element to link hwspinlock devices together * @dev: underlying device, will be used to invoke runtime PM api * @ops: platform-specific hwspinlock handlers * @base_id: id index of the first lock in this device @@ -258,6 +259,7 @@ implementation using the hwspin_lock_register() API. * @lock: dynamically allocated array of 'struct hwspinlock' */ struct hwspinlock_device { + struct list_head list; struct device *dev; const struct hwspinlock_ops *ops; int base_id; diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c index 461a0d7..48f7866 100644 --- a/drivers/hwspinlock/hwspinlock_core.c +++ b/drivers/hwspinlock/hwspinlock_core.c @@ -59,6 +59,11 @@ static RADIX_TREE(hwspinlock_tree, GFP_KERNEL); */ static DEFINE_MUTEX(hwspinlock_tree_lock); +/* + * A linked list for maintaining all the registered hwspinlock devices. + * The list is maintained in an ordered-list of the supported locks group. + */ +static LIST_HEAD(hwspinlock_devices); /** * __hwspin_trylock() - attempt to lock a specific hwspinlock @@ -307,6 +312,40 @@ out: return hwlock; } +/* + * Add a new hwspinlock device to the global list, keeping the list of + * devices sorted by base order. + * + * Returns 0 on success, or -EBUSY if the new device overlaps with some + * other device's lock space. + */ +static int hwspinlock_device_add(struct hwspinlock_device *bank) +{ + struct list_head *entry = &hwspinlock_devices; + struct hwspinlock_device *_bank; + int ret = 0; + + list_for_each(entry, &hwspinlock_devices) { + _bank = list_entry(entry, struct hwspinlock_device, list); + if (_bank->base_id >= bank->base_id + bank->num_locks) + break; + } + + if (entry != &hwspinlock_devices && + entry->prev != &hwspinlock_devices) { + _bank = list_entry(entry->prev, struct hwspinlock_device, list); + if (_bank->base_id + _bank->num_locks > bank->base_id) { + dev_err(bank->dev, "hwlock space overlap, cannot add device\n"); + ret = -EBUSY; + } + } + + if (!ret) + list_add_tail(&bank->list, entry); + + return ret; +} + /** * hwspin_lock_register() - register a new hw spinlock device * @bank: the hwspinlock device, which usually provides numerous hw locks @@ -339,6 +378,12 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, bank->base_id = base_id; bank->num_locks = num_locks; + mutex_lock(&hwspinlock_tree_lock); + ret = hwspinlock_device_add(bank); + mutex_unlock(&hwspinlock_tree_lock); + if (ret) + return ret; + for (i = 0; i < num_locks; i++) { hwlock = &bank->lock[i]; @@ -355,6 +400,9 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, reg_failed: while (--i >= 0) hwspin_lock_unregister_single(base_id + i); + mutex_lock(&hwspinlock_tree_lock); + list_del(&bank->list); + mutex_unlock(&hwspinlock_tree_lock); return ret; } EXPORT_SYMBOL_GPL(hwspin_lock_register); @@ -386,6 +434,9 @@ int hwspin_lock_unregister(struct hwspinlock_device *bank) WARN_ON(tmp != hwlock); } + mutex_lock(&hwspinlock_tree_lock); + list_del(&bank->list); + mutex_unlock(&hwspinlock_tree_lock); return 0; } EXPORT_SYMBOL_GPL(hwspin_lock_unregister); diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h index d26f78b..aff560c 100644 --- a/drivers/hwspinlock/hwspinlock_internal.h +++ b/drivers/hwspinlock/hwspinlock_internal.h @@ -53,6 +53,7 @@ struct hwspinlock { /** * struct hwspinlock_device - a device which usually spans numerous hwspinlocks + * @list: list element to link hwspinlock devices together * @dev: underlying device, will be used to invoke runtime PM api * @ops: platform-specific hwspinlock handlers * @base_id: id index of the first lock in this device @@ -60,6 +61,7 @@ struct hwspinlock { * @lock: dynamically allocated array of 'struct hwspinlock' */ struct hwspinlock_device { + struct list_head list; struct device *dev; const struct hwspinlock_ops *ops; int base_id;
The hwspinlock_device structure is used for registering a bank of locks with the driver core. The structure already contains the necessary members to identify the bank of locks. The core does not maintain the hwspinlock_devices itself, but maintains only a radix tree for all the registered locks. A specific lock can be requested by users using a global lock id, and any device-specific fields can be retrieved through a reference to the hwspinlock_device in each lock. The global lock id, however, is not friendly to be requested for users using the device-tree model. The device-tree representation will typically have each of the hwspinlock devices represented as a DT node, and a specific lock can be requested using the device's phandle and a lock specifier. Add support to the core therefore to maintain all the registered hwspinlock_devices, so that a device can be looked up and a specific lock belonging to the device requested through a phandle + args approach. Signed-off-by: Suman Anna <s-anna@ti.com> --- Documentation/hwspinlock.txt | 2 ++ drivers/hwspinlock/hwspinlock_core.c | 51 ++++++++++++++++++++++++++++++++ drivers/hwspinlock/hwspinlock_internal.h | 2 ++ 3 files changed, 55 insertions(+)