| Message ID | 1398904476-26200-5-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: > 2. The of_hwspin_lock_simple_xlate() is a simple default > translator function for hwspinlock provider implementations > that use a single cell number for requesting a specific lock > (relatively indexed) within a hwlock bank. Do we have a use case today that require the xlate() method? If not, let's remove it as we could always add it back if some new hardware shows up that needs it. As long as the dt data is unchanged, we should generally only add kernel code that we really need. > 3. The of_hwspin_lock_request_specific() API can be used by > hwspinlock clients to request a specific lock using the > phandle + args specifier. This function relies on the > implementation providing back a relative hwlock id within > the bank from the args specifier. It seems to me we can just introduce a of_hwspin_lock_get_id() method which will provide the global lock id to dt users, with which they could just invoke the existing hwspin_lock_request_specific(). This way we will have more common code between dt users and users who get the lock id from a remote processor. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ohad, > > On Thu, May 1, 2014 at 3:34 AM, Suman Anna <s-anna@ti.com> wrote: >> 2. The of_hwspin_lock_simple_xlate() is a simple default >> translator function for hwspinlock provider implementations >> that use a single cell number for requesting a specific lock >> (relatively indexed) within a hwlock bank. > > Do we have a use case today that require the xlate() method? > > If not, let's remove it as we could always add it back if some new > hardware shows up that needs it. The xlate() method is to support the phandle + args specifier way of requesting hwlocks, platform implementations are free to implement their own xlate functions, but the above supports the simplest case of controller + relative lock index within controller. > > As long as the dt data is unchanged, we should generally only add > kernel code that we really need. > >> 3. The of_hwspin_lock_request_specific() API can be used by >> hwspinlock clients to request a specific lock using the >> phandle + args specifier. This function relies on the >> implementation providing back a relative hwlock id within >> the bank from the args specifier. > > It seems to me we can just introduce a of_hwspin_lock_get_id() method > which will provide the global lock id to dt users, with which they > could just invoke the existing hwspin_lock_request_specific(). This > way we will have more common code between dt users and users who get > the lock id from a remote processor. This function again is to support the phandle + args specifier way of requesting hwlocks, the hwspin_lock_request_specific() is invoked internally within this function, so we are still reusing the actual request code other than handling the DT parsing portion. If we go back to using global locks in client hwlocks property, we don't need a of_hwspin_lock_get_id(), the same can be achieved using the existing DT function, of_property_read_u32_index(). regards Suman -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna <s-anna@ti.com> wrote: >> Do we have a use case today that require the xlate() method? >> >> If not, let's remove it as we could always add it back if some new >> hardware shows up that needs it. > > The xlate() method is to support the phandle + args specifier way of > requesting hwlocks, platform implementations are free to implement their > own xlate functions, but the above supports the simplest case of > controller + relative lock index within controller. Do we have a use case for a different implementation other than the simplest case? If not, it seems to me this will just become redundant boilerplate code (every platform will use the simple xlate method). > This function again is to support the phandle + args specifier way of > requesting hwlocks, the hwspin_lock_request_specific() is invoked > internally within this function, so we are still reusing the actual > request code other than handling the DT parsing portion. If we go back > to using global locks in client hwlocks property, we don't need a > of_hwspin_lock_get_id(), the same can be achieved using the existing DT > function, of_property_read_u32_index(). I think you may have misunderstood me, sorry. I'm ok with the phandle + args specifier. I just think we can use it, together with the base_id property, to infer the global lock id from the DT data. This is not only a must to support heterogenous multi-processing systems, it will also substantially simplify the code. Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ohad, On 07/03/2014 02:15 AM, Ohad Ben-Cohen wrote: > Hi Suman, > > On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna <s-anna@ti.com> wrote: >>> Do we have a use case today that require the xlate() method? >>> >>> If not, let's remove it as we could always add it back if some new >>> hardware shows up that needs it. >> >> The xlate() method is to support the phandle + args specifier way of >> requesting hwlocks, platform implementations are free to implement their >> own xlate functions, but the above supports the simplest case of >> controller + relative lock index within controller. > > Do we have a use case for a different implementation other than the > simplest case? If not, it seems to me this will just become redundant > boilerplate code (every platform will use the simple xlate method). Not at the moment, with the existing platform implementations. So, if I understand you correctly, you are asking to leave out the xlate ops and make the of_hwspin_lock_simple_xlate() internal until a need for an xlate method arises. This also means, we only support a value of 1 for #hwlock-cells. > >> This function again is to support the phandle + args specifier way of >> requesting hwlocks, the hwspin_lock_request_specific() is invoked >> internally within this function, so we are still reusing the actual >> request code other than handling the DT parsing portion. If we go back >> to using global locks in client hwlocks property, we don't need a >> of_hwspin_lock_get_id(), the same can be achieved using the existing DT >> function, of_property_read_u32_index(). > > I think you may have misunderstood me, sorry. I'm ok with the phandle > + args specifier. I just think we can use it, together with the > base_id property, to infer the global lock id from the DT data. Aah ok, its minor code rearrangement for what you are asking - I just need to leave out invoking the request_specific invocation in the OF request specific existing function, just return the global id and let the client users call the normal request_specific API themselves. But, that would mean DT-based client users would have to invoke two function calls to request a hwspinlock. We already have an API to get the lock id given a hwspinlock - hwspin_lock_get_id(), so I added the OF API for requesting a lock directly rather than giving an OF API for getting the lock id. This is in keeping in convention with what other drivers do normally - a regular get and an OF get. I can modify it if you still prefer the OF API for getting a global lock id, but I feel its a burden for client users. Also, do you prefer an open property-name in client users (like I am doing at the moment) or imposing a standard property name "hwlocks"? regards Suman > This is not only a must to support heterogenous multi-processing systems, > it will also substantially simplify the code. > > Thanks, > Ohad. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suman, On Thu, Jul 3, 2014 at 8:35 PM, Suman Anna <s-anna@ti.com> wrote: > Not at the moment, with the existing platform implementations. So, if I > understand you correctly, you are asking to leave out the xlate ops and > make the of_hwspin_lock_simple_xlate() internal until a need for an > xlate method arises. Yes, please. It seems to me this way we're reducing complexity without compromising anything. > This also means, we only support a value of 1 for #hwlock-cells. When a different #hwlock-cells value shows up, someone would have to write a new xlate implementation anyway. At the same time, the xlate ops could be brought back quite easily. Since we're not imposing anything on the DT data, this doesn't affect our ability to support these scenarios in the future, but unless I'm missing a current use case, these scenarios right now seems somewhat fictional. > But, that would mean DT-based client users would have to invoke two > function calls to request a hwspinlock. We already have an API to get > the lock id given a hwspinlock - hwspin_lock_get_id(), so I added the OF > API for requesting a lock directly rather than giving an OF API for > getting the lock id. This is in keeping in convention with what other > drivers do normally - a regular get and an OF get. I can modify it if > you still prefer the OF API for getting a global lock id, but I feel its > a burden for client users. Let's discuss this. I was actually thinking of the more general use case of an heterogenous system: - driver gets the global lock id from a remote processor - driver then requests the specific lock Looking at these cases, the OF scenario only differs in the small part of fetching the lock id, and if we keep the regular request_specific API we'll have more common code between drivers. What do you think? > Also, do you prefer an open property-name in client users (like I am > doing at the moment) or imposing a standard property name "hwlocks"? Good point - I think we can start with an imposed "hwlocks" name, and evolve in the future as needed (again, only because we're not really imposing anything on the DT data - this is just kernel code that we could always extend as needed). Thanks, Ohad. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ohad, On 07/03/2014 11:58 PM, Ohad Ben-Cohen wrote: > Hi Suman, > > On Thu, Jul 3, 2014 at 8:35 PM, Suman Anna <s-anna@ti.com> wrote: >> Not at the moment, with the existing platform implementations. So, if I >> understand you correctly, you are asking to leave out the xlate ops and >> make the of_hwspin_lock_simple_xlate() internal until a need for an >> xlate method arises. > > Yes, please. It seems to me this way we're reducing complexity without > compromising anything. OK, will make this change and add a comment in the code in the next version. > >> This also means, we only support a value of 1 for #hwlock-cells. > > When a different #hwlock-cells value shows up, someone would have to > write a new xlate implementation anyway. At the same time, the xlate > ops could be brought back quite easily. > > Since we're not imposing anything on the DT data, this doesn't affect > our ability to support these scenarios in the future, but unless I'm > missing a current use case, these scenarios right now seems somewhat > fictional. OK, fair enough. > >> But, that would mean DT-based client users would have to invoke two >> function calls to request a hwspinlock. We already have an API to get >> the lock id given a hwspinlock - hwspin_lock_get_id(), so I added the OF >> API for requesting a lock directly rather than giving an OF API for >> getting the lock id. This is in keeping in convention with what other >> drivers do normally - a regular get and an OF get. I can modify it if >> you still prefer the OF API for getting a global lock id, but I feel its >> a burden for client users. > > Let's discuss this. > > I was actually thinking of the more general use case of an heterogenous system: > > - driver gets the global lock id from a remote processor > - driver then requests the specific lock > > Looking at these cases, the OF scenario only differs in the small part > of fetching the lock id, and if we keep the regular request_specific > API we'll have more common code between drivers. > > What do you think? We should also be thinking about the how to add the support for the reserved locks. Please take a look at the added RFC patches 9 through 13, specifically the reworked Patch 12 [1]. I moved away from adding a reserved property to the controller node, as it means updating both controller and client nodes. Depending on where we enforce the check (in the OF API or in the common request_specific, the behavior would change. > >> Also, do you prefer an open property-name in client users (like I am >> doing at the moment) or imposing a standard property name "hwlocks"? > > Good point - I think we can start with an imposed "hwlocks" name, and > evolve in the future as needed (again, only because we're not really > imposing anything on the DT data - this is just kernel code that we > could always extend as needed). > OK. Actually, I didn't realize that I had already made this change as part of the reserved locks RFC patch. regards Suman [1] http://marc.info/?l=linux-omap&m=139968467307977&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt index 640ae47..903d477 100644 --- a/Documentation/hwspinlock.txt +++ b/Documentation/hwspinlock.txt @@ -48,6 +48,15 @@ independent, drivers. ids for predefined purposes. Should be called from a process context (might sleep). + struct hwspinlock *of_hwspin_lock_request_specific( + struct device_node *np, const char *propname, int index); + - assign a specific hwspinlock id and return its address, or NULL + if that hwspinlock is already in use. This function is the OF + equivalent of hwspin_lock_request_specific() function, and provides + a means for users of the hwspinlock module to request a specific + hwspinlock using the phandle of the hwspinlock device. + Should be called from a process context (might sleep). + int hwspin_lock_free(struct hwspinlock *hwlock); - free a previously-assigned hwspinlock; returns 0 on success, or an appropriate error code on failure (e.g. -EINVAL if the hwspinlock @@ -243,6 +252,23 @@ int hwspinlock_example2(void) Returns the address of hwspinlock on success, or NULL on error (e.g. if the hwspinlock is still in use). + int of_hwspin_lock_simple_xlate(struct hwspinlock_device *bank, + const struct of_phandle_args *hwlock_spec); + - is a simple default OF translate helper function that can be plugged in + as the underlying vendor-specific implementation's of_xlate ops function. + This can be used by implementations that use a single integer argument in + the DT node argument specifier, that indicates the hwlock index number. + Returns a hwlock index within a bank, or appropriate error code on + failure. + + int of_hwspin_lock_get_num_locks(struct device_node *dn); + - is a common OF helper function that can be used by some underlying + vendor-specific implementations. This can be used by implementations + that require and define the number of locks supported within a hwspinlock + bank as a device tree node property. This function should be called by + needed implementations before registering a hwspinlock device with the + core. + 5. Important structs struct hwspinlock_device is a device which usually contains a bank @@ -288,12 +314,14 @@ initialized by the hwspinlock core itself. 6. Implementation callbacks -There are three possible callbacks defined in 'struct hwspinlock_ops': +There are four possible callbacks defined in 'struct hwspinlock_ops': struct hwspinlock_ops { int (*trylock)(struct hwspinlock *lock); void (*unlock)(struct hwspinlock *lock); void (*relax)(struct hwspinlock *lock); + int (*of_xlate)(struct hwspinlock_device *bank, + const struct of_phandle_args *hwlock_spec); }; The first two callbacks are mandatory: @@ -307,3 +335,7 @@ may _not_ sleep. The ->relax() callback is optional. It is called by hwspinlock core while spinning on a lock, and can be used by the underlying implementation to force a delay between two successive invocations of ->trylock(). It may _not_ sleep. + +The ->of_xlate() callback is mandatory to support requesting hwlocks through +device-tree nodes. It is called by hwspinlock core to retrieve the relative +lock index within a bank from the underlying implementation. diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c index 48f7866..3966c0c 100644 --- a/drivers/hwspinlock/hwspinlock_core.c +++ b/drivers/hwspinlock/hwspinlock_core.c @@ -27,6 +27,7 @@ #include <linux/hwspinlock.h> #include <linux/pm_runtime.h> #include <linux/mutex.h> +#include <linux/of.h> #include "hwspinlock_internal.h" @@ -262,6 +263,33 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags) } EXPORT_SYMBOL_GPL(__hwspin_unlock); +/** + * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks + * @dn: device node pointer + * + * This is an OF helper function that can be called by the underlying + * platform-specific implementations, to retrieve the number of locks + * present within a hwspinlock device instance. The hwlock-num-locks + * DT property may be optional for some platforms, while mandatory for + * some others, so this function is typically called only by needed + * platform-specific implementations. + * + * Returns a positive number of locks on success, -ENODEV on generic + * failure or an appropriate error code as returned by the OF layer + */ +int of_hwspin_lock_get_num_locks(struct device_node *dn) +{ + unsigned int val; + int ret = -ENODEV; + + ret = of_property_read_u32(dn, "hwlock-num-locks", &val); + if (!ret) + ret = val ? val : -ENODEV; + + return ret; +} +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks); + static int hwspin_lock_register_single(struct hwspinlock *hwlock, int id) { struct hwspinlock *tmp; @@ -312,6 +340,31 @@ out: return hwlock; } +/** + * of_hwspin_lock_simple_xlate - translate hwlock_spec to return a lock id + * @bank: the hwspinlock device bank + * @hwlock_spec: hwlock specifier as found in the device tree + * + * This is a simple translation function, suitable for hwspinlock platform + * drivers that only has a lock specifier length of 1. + * + * Returns a negative value on error, and a relative index of the lock within + * a specified bank on success. + */ +int of_hwspin_lock_simple_xlate(struct hwspinlock_device *bank, + const struct of_phandle_args *hwlock_spec) +{ + /* sanity check (these shouldn't happen) */ + if (WARN_ON(!bank->dev->of_node)) + return -EINVAL; + + if (WARN_ON(hwlock_spec->args_count != 1)) + return -EINVAL; + + return hwlock_spec->args[0]; +} +EXPORT_SYMBOL_GPL(of_hwspin_lock_simple_xlate); + /* * Add a new hwspinlock device to the global list, keeping the list of * devices sorted by base order. @@ -368,7 +421,7 @@ int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, int ret = 0, i; if (!bank || !ops || !dev || !num_locks || !ops->trylock || - !ops->unlock) { + !ops->unlock || (dev->of_node && !ops->of_xlate)) { pr_err("invalid parameters\n"); return -EINVAL; } @@ -592,6 +645,51 @@ out: EXPORT_SYMBOL_GPL(hwspin_lock_request_specific); /** + * of_hwspin_lock_request_specific() - request a OF phandle-based specific lock + * @np: device node from which to request the specific hwlock + * @propname: property name containing hwlock specifier(s) + * @index: index of the hwlock + * + * This function is the OF equivalent of hwspin_lock_request_specific(). This + * function provides a means for users of the hwspinlock module to request a + * specific hwspinlock using the phandle of the hwspinlock device. The requested + * lock number is indexed relative to the hwspinlock device, unlike the + * hwspin_lock_request_specific() which is an absolute lock number. + * + * Returns the address of the assigned hwspinlock, or NULL on error + */ +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np, + const char *propname, + int index) +{ + struct hwspinlock_device *bank; + struct of_phandle_args args; + int id; + int ret; + + ret = of_parse_phandle_with_args(np, propname, "#hwlock-cells", index, + &args); + if (ret) + return NULL; + + mutex_lock(&hwspinlock_tree_lock); + list_for_each_entry(bank, &hwspinlock_devices, list) + if (bank->dev->of_node == args.np) + break; + mutex_unlock(&hwspinlock_tree_lock); + if (&bank->list == &hwspinlock_devices) + return NULL; + + id = bank->ops->of_xlate(bank, &args); + if (id < 0 || id >= bank->num_locks) + return NULL; + + id += bank->base_id; + return hwspin_lock_request_specific(id); +} +EXPORT_SYMBOL_GPL(of_hwspin_lock_request_specific); + +/** * hwspin_lock_free() - free a specific hwspinlock * @hwlock: the specific hwspinlock to free * diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h index aff560c..5e42613 100644 --- a/drivers/hwspinlock/hwspinlock_internal.h +++ b/drivers/hwspinlock/hwspinlock_internal.h @@ -32,11 +32,15 @@ struct hwspinlock_device; * @relax: optional, platform-specific relax handler, called by hwspinlock * core while spinning on a lock, between two successive * invocations of @trylock. may _not_ sleep. + * @of_xlate: platform-specific hwlock specifier translate function, to + * return the relative index of the lock within a bank */ struct hwspinlock_ops { int (*trylock)(struct hwspinlock *lock); void (*unlock)(struct hwspinlock *lock); void (*relax)(struct hwspinlock *lock); + int (*of_xlate)(struct hwspinlock_device *bank, + const struct of_phandle_args *hwlock_spec); }; /** diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h index 3343298..068e628 100644 --- a/include/linux/hwspinlock.h +++ b/include/linux/hwspinlock.h @@ -26,6 +26,8 @@ #define HWLOCK_IRQ 0x02 /* Disable interrupts, don't save state */ struct device; +struct device_node; +struct of_phandle_args; struct hwspinlock; struct hwspinlock_device; struct hwspinlock_ops; @@ -60,11 +62,17 @@ struct hwspinlock_pdata { #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE) +int of_hwspin_lock_simple_xlate(struct hwspinlock_device *bank, + const struct of_phandle_args *hwlock_spec); +int of_hwspin_lock_get_num_locks(struct device_node *dn); int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, const struct hwspinlock_ops *ops, int base_id, int num_locks); int hwspin_lock_unregister(struct hwspinlock_device *bank); struct hwspinlock *hwspin_lock_request(void); struct hwspinlock *hwspin_lock_request_specific(unsigned int id); +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np, + const char *propname, + int index); int hwspin_lock_free(struct hwspinlock *hwlock); int hwspin_lock_get_id(struct hwspinlock *hwlock); int __hwspin_lock_timeout(struct hwspinlock *, unsigned int, int, @@ -80,9 +88,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. @@ -97,6 +105,14 @@ static inline struct hwspinlock *hwspin_lock_request_specific(unsigned int id) return ERR_PTR(-ENODEV); } +static inline +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np, + const char *propname, + int index) +{ + return ERR_PTR(-ENODEV); +} + static inline int hwspin_lock_free(struct hwspinlock *hwlock) { return 0;
This patch adds three new OF helper functions to use/request locks from a hwspinlock device instantiated through a device-tree blob. 1. The of_hwspin_lock_get_num_locks() is a common helper function to read the common 'hwlock-num-locks' property. 2. The of_hwspin_lock_simple_xlate() is a simple default translator function for hwspinlock provider implementations that use a single cell number for requesting a specific lock (relatively indexed) within a hwlock bank. 3. The of_hwspin_lock_request_specific() API can be used by hwspinlock clients to request a specific lock using the phandle + args specifier. This function relies on the implementation providing back a relative hwlock id within the bank from the args specifier. Signed-off-by: Suman Anna <s-anna@ti.com> --- Documentation/hwspinlock.txt | 34 ++++++++++- drivers/hwspinlock/hwspinlock_core.c | 100 ++++++++++++++++++++++++++++++- drivers/hwspinlock/hwspinlock_internal.h | 4 ++ include/linux/hwspinlock.h | 22 ++++++- 4 files changed, 155 insertions(+), 5 deletions(-)