Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
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-a...@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-omapm=139968467307977w=2 -- 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 04/15] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Jul 3, 2014 at 12:14 AM, Suman Anna s-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
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-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, Jul 3, 2014 at 8:35 PM, Suman Anna s-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
Hi Ohad, On Thu, May 1, 2014 at 3:34 AM, Suman Anna s-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 04/15] hwspinlock/core: add common OF helpers
Hi Suman, On Thu, May 1, 2014 at 3:34 AM, Suman Anna s-a...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv5 04/15] hwspinlock/core: add common OF helpers
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-a...@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(-)
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);
+/**
