| Message ID | 1389658764-39199-5-git-send-email-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote: > This patch adds three new OF helper functions to use/request > locks from a hwspinlock device instantiated through a > device-tree blob. Nice, I ran in to the problem of needing a probe deferral on a hwspinlock earlier this week so I implemented this yesterday...then I got a pointer to your series. [snip] > /** > + * 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) { > + pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n", > + __func__, np->full_name, index, ret); > + return NULL; > + } of_parse_phandle_with_args() already does pr_err if it can't find the phandle and on some of the issues related to arguments. So please remove this pr_warn(). It seems to be standard practice to pass the error value back to the consumer, so you should return ERR_PTR(ret); here instead of the 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) { > + pr_warn("%s: requested hwspinlock device %s is not registered\n", > + __func__, args.np->full_name); > + return NULL; ...especially since you want the consumer to have the ability to identify this error. Here you should return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this lock is not _yet_ registered, but will be in the future. You should remove this pr_warn as well. The standard use of this function would be in a probe() and just returning this error value from that probe will give you a line in the log indicating that this was in fact the issue. > + } > + > + id = bank->ops->of_xlate(bank, &args); > + if (id < 0 || id >= bank->num_locks) { > + pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n", > + __func__, id, bank->num_locks - 1); > + return NULL; Please return ERR_PTR(-EINVAL); here. Looking forward to your next spin, as I will actually use this interface :) Regards, Bjorn -- 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
Bjorn, On 02/07/2014 04:49 PM, Bjorn Andersson wrote: > On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote: >> This patch adds three new OF helper functions to use/request >> locks from a hwspinlock device instantiated through a >> device-tree blob. > > Nice, I ran in to the problem of needing a probe deferral on a > hwspinlock earlier this week so I implemented this yesterday...then I > got a pointer to your series. > > [snip] >> /** >> + * 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) { >> + pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n", >> + __func__, np->full_name, index, ret); >> + return NULL; >> + } > > of_parse_phandle_with_args() already does pr_err if it can't find the > phandle and on some of the issues related to arguments. So please > remove this pr_warn(). Yes, I will clean this up. > > It seems to be standard practice to pass the error value back to the > consumer, so you should > return ERR_PTR(ret); here instead of the NULL... I have modelled the return values in this function based on the return values in the existing hwspin_lock_request interfaces. I would need to change those functions as well. Ohad, Do you have any objections to the return code convention change? I agree with Bjorn on the changes. If you are ok, then I will add a separate patch for the existing functions and revise this patch as well. > >> + >> + 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) { >> + pr_warn("%s: requested hwspinlock device %s is not registered\n", >> + __func__, args.np->full_name); >> + return NULL; > > ...especially since you want the consumer to have the ability to > identify this error. Here you should > return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this > lock is not _yet_ registered, but will be in the future. > > You should remove this pr_warn as well. The standard use of this > function would be in a probe() and just returning this error value > from that probe will give you a line in the log indicating that this > was in fact the issue. OK. > >> + } >> + >> + id = bank->ops->of_xlate(bank, &args); >> + if (id < 0 || id >= bank->num_locks) { >> + pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n", >> + __func__, id, bank->num_locks - 1); >> + return NULL; > > Please return ERR_PTR(-EINVAL); here. OK, will change this based on Ohad's ack/nack. > > Looking forward to your next spin, as I will actually use this interface :) Thanks for your comments. I will wait to see if there are any additional comments before I refresh the series later this week. 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
On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote: > On 02/07/2014 04:49 PM, Bjorn Andersson wrote: >> It seems to be standard practice to pass the error value back to the >> consumer, so you should >> return ERR_PTR(ret); here instead of the NULL... > > > I have modelled the return values in this function based on the return > values in the existing hwspin_lock_request interfaces. I would need to > change those functions as well. > > Ohad, > Do you have any objections to the return code convention change? Unless strictly needed, I prefer we don't switch to the ERR_PTR code convention, as it reduces code readability and increases chances of user bugs. In our case, switching to ERR_PTR and friends seems only to optimize a few error paths, and I'm not sure it's a big win over simplicity. 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
On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote: > On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote: >> On 02/07/2014 04:49 PM, Bjorn Andersson wrote: >>> It seems to be standard practice to pass the error value back to the >>> consumer, so you should >>> return ERR_PTR(ret); here instead of the NULL... >> >> >> I have modelled the return values in this function based on the return >> values in the existing hwspin_lock_request interfaces. I would need to >> change those functions as well. >> >> Ohad, >> Do you have any objections to the return code convention change? > > Unless strictly needed, I prefer we don't switch to the ERR_PTR code > convention, as it reduces code readability and increases chances of > user bugs. > > In our case, switching to ERR_PTR and friends seems only to optimize a > few error paths, and I'm not sure it's a big win over simplicity. When introducing the ability to reference a hwspin lock via a phandle in device tree it makes a big difference to be able to differ between the case of "initialization failed" or "device not yet probed"; so that the client knows if it should fail or retry later. Regards, Bjorn -- 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
Ohad, On 03/02/2014 02:19 PM, Bjorn Andersson wrote: > On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote: >> On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote: >>> On 02/07/2014 04:49 PM, Bjorn Andersson wrote: >>>> It seems to be standard practice to pass the error value back to the >>>> consumer, so you should >>>> return ERR_PTR(ret); here instead of the NULL... >>> >>> >>> I have modelled the return values in this function based on the return >>> values in the existing hwspin_lock_request interfaces. I would need to >>> change those functions as well. >>> >>> Ohad, >>> Do you have any objections to the return code convention change? >> >> Unless strictly needed, I prefer we don't switch to the ERR_PTR code >> convention, as it reduces code readability and increases chances of >> user bugs. >> From a current user/client perspectives, I didn't find any clients of hwspinlock within the kernel. So, this is probably the right time to change the return code convention. >> In our case, switching to ERR_PTR and friends seems only to optimize a >> few error paths, and I'm not sure it's a big win over simplicity. The usage on the clients will also not become too complicated. The only change on the clients is mostly the base error check change from if (!hwlock) to if (IS_ERR(hwlock)). regards Suman > When introducing the ability to reference a hwspin lock via a phandle > in device tree it makes a big difference to be able to differ between > the case of "initialization failed" or "device not yet probed"; so > that the client knows if it should fail or retry later. > > Regards, > Bjorn > -- 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 03/02/2014 02:19 PM, Bjorn Andersson wrote: > On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote: >> On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote: >>> On 02/07/2014 04:49 PM, Bjorn Andersson wrote: >>>> It seems to be standard practice to pass the error value back to the >>>> consumer, so you should >>>> return ERR_PTR(ret); here instead of the NULL... >>> >>> >>> I have modelled the return values in this function based on the return >>> values in the existing hwspin_lock_request interfaces. I would need to >>> change those functions as well. >>> >>> Ohad, >>> Do you have any objections to the return code convention change? >> >> Unless strictly needed, I prefer we don't switch to the ERR_PTR code >> convention, as it reduces code readability and increases chances of >> user bugs. >> >> In our case, switching to ERR_PTR and friends seems only to optimize a >> few error paths, and I'm not sure it's a big win over simplicity. > > When introducing the ability to reference a hwspin lock via a phandle > in device tree it makes a big difference to be able to differ between > the case of "initialization failed" or "device not yet probed"; so > that the client knows if it should fail or retry later. > Can you confirm the changes you want me to make, so that I can refresh and post a v5 for 3.15? 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
On Tue, Mar 04, 2014 at 11:38:23AM -0600, Suman Anna wrote: > Hi Ohad, > > On 03/02/2014 02:19 PM, Bjorn Andersson wrote: > >On Sat, Mar 1, 2014 at 9:14 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote: > >>On Mon, Feb 10, 2014 at 9:14 PM, Suman Anna <s-anna@ti.com> wrote: > >>>On 02/07/2014 04:49 PM, Bjorn Andersson wrote: > >>>Ohad, > >>>Do you have any objections to the return code convention change? > >> > >>Unless strictly needed, I prefer we don't switch to the ERR_PTR code > >>convention, as it reduces code readability and increases chances of > >>user bugs. > >> > >>In our case, switching to ERR_PTR and friends seems only to optimize a > >>few error paths, and I'm not sure it's a big win over simplicity. > > > >When introducing the ability to reference a hwspin lock via a phandle > >in device tree it makes a big difference to be able to differ between > >the case of "initialization failed" or "device not yet probed"; so > >that the client knows if it should fail or retry later. > > > > Can you confirm the changes you want me to make, so that I can refresh and > post a v5 for 3.15? What's the status on this? I'm assuming this is going to miss 3.15? Having DT support in the core will be useful to move the Qualcomm hwspinlock driver forward as well.
Hi Suman, On Tue, Mar 4, 2014 at 7:38 PM, Suman Anna <s-anna@ti.com> wrote: >>>> Do you have any objections to the return code convention change? >>> >>> Unless strictly needed, I prefer we don't switch to the ERR_PTR code >>> convention, as it reduces code readability and increases chances of >>> user bugs. >>> >>> In our case, switching to ERR_PTR and friends seems only to optimize a >>> few error paths, and I'm not sure it's a big win over simplicity. >> >> >> When introducing the ability to reference a hwspin lock via a phandle >> in device tree it makes a big difference to be able to differ between >> the case of "initialization failed" or "device not yet probed"; so >> that the client knows if it should fail or retry later. >> > > Can you confirm the changes you want me to make, so that I can refresh and > post a v5 for 3.15? Sorry, I missed your replies for some reason. I prefer we stick with the current error handling code because I find the alternative inferior (as long as it's not strictly 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
On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson <bjorn@kryo.se> wrote: > When introducing the ability to reference a hwspin lock via a phandle > in device tree it makes a big difference to be able to differ between > the case of "initialization failed" or "device not yet probed"; so > that the client knows if it should fail or retry later. I'm not convinced. The only advantage this brings is to avoid retrying in case a fatal error has occurred. Such fatal errors are extremely rare, and when they show up - extremely painful, and I suspect that optimizing them wouldn't be a big win. OTOH, keeping the code easier to read and less error prone is a big win. I prefer we keep it simple for now. 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
On Fri, Mar 14, 2014 at 03:12:26PM +0200, Ohad Ben-Cohen wrote: > On Sun, Mar 2, 2014 at 10:19 PM, Bjorn Andersson <bjorn@kryo.se> wrote: > > When introducing the ability to reference a hwspin lock via a phandle > > in device tree it makes a big difference to be able to differ between > > the case of "initialization failed" or "device not yet probed"; so > > that the client knows if it should fail or retry later. > > I'm not convinced. > > The only advantage this brings is to avoid retrying in case a fatal > error has occurred. Such fatal errors are extremely rare, and when > they show up - extremely painful, and I suspect that optimizing them > wouldn't be a big win. So, are you suggesting that because fatal errors should be "extremely rare", a consuming driver should just assume that if NULL is returned from a hwspin_lock_request*() function that it was the "device not yet probed" case that was hit? Note that having the consumer/hwspinlock device relationship modeled in devicetree introduces more potential failure cases...
On Fri, Mar 14, 2014 at 5:23 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > So, are you suggesting that because fatal errors should be "extremely > rare", a consuming driver should just assume that if NULL is returned > from a hwspin_lock_request*() function that it was the "device not yet > probed" case that was hit? No - it's not the scarcity, it's the severity. The error path that will be optimized here is an invalid id. If this happens, the consumer will crash and burn, and I'm not sure that slightly optimizing his death is very interesting? BTW the hwspinlock core once did use ERR_PTR and friends, and it was changed due to convincing arguments against that methodology on this mailing list. We can change it back but we need a strong(er) case. > Note that having the consumer/hwspinlock device relationship modeled in > devicetree introduces more potential failure cases... Yeah. Even the error above, presumed to be EPROBE_DEFER, may be a symptom of some other fatal error that occurred, and we can't be sure that a future request will surely be satisfied. So before we bloat our code, I suggest that we wait for consumers to show up and see if there's real benefit. 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
On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote: > This patch adds three new OF helper functions to use/request > locks from a hwspinlock device instantiated through a > device-tree blob. > Hi Ohad, Suman I'm about to send out some patches that depends on this functionality, how do we move forward? I still think it's wrong to not return -EPROBE_DEFER, but I much rather have the code returning NULL than not having it in the tree (we can always argue about it later...). @Suman, do you remember if there was any other comments on the patch? @Ohad, do you object merging Suman's patch in it's current form? I think it should still apply cleanly. Regards, Bjorn -- 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 Bjorn, On 09/26/2014 09:40 AM, Bjorn Andersson wrote: > On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote: >> This patch adds three new OF helper functions to use/request >> locks from a hwspinlock device instantiated through a >> device-tree blob. >> > > Hi Ohad, Suman > > I'm about to send out some patches that depends on this functionality, > how do we move forward? > > I still think it's wrong to not return -EPROBE_DEFER, but I much > rather have the code returning NULL than not having it in the tree (we > can always argue about it later...). > > @Suman, do you remember if there was any other comments on the patch? I have posted two further revisions of this series, the latest is v6 [1]. I added additional patches in v5 that added the concept of reserved locks, and I have posted them as a separate RFC [2] for v6 so as to not block the core DT support. In anycase, the latest v6 version does not define the of_hwspin_lock_request_specific() function anymore, and it is replaced with of_hwspin_lock_get_id() function, based on Ohad's review comments on v5, and I did add the support for -EPROBE_DEFER in this API, without changing any of the existing return code conventions. I am yet to receive any comments on v6, but that series should address both your need for a probe deferral and Ohad's request to not change any return types. Please give it a try and let me know if you have any comments. regards Suman [1] http://marc.info/?l=linux-arm-kernel&m=141055365513902&w=2 [2] http://marc.info/?l=linux-arm-kernel&m=141055554214657&w=2 > > @Ohad, do you object merging Suman's patch in it's current form? I > think it should still apply cleanly. > > Regards, > Bjorn > -- 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
On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna <s-anna@ti.com> wrote: > I am yet to receive any comments on v6, but that series should address > both your need for a probe deferral and Ohad's request to not change any > return types. Please give it a try and let me know if you have any comments. Guys, Just to let you know we have several holidays around here these days, and I intend to review all pending patches immediately soon after. 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 10/06/2014 04:44 AM, Ohad Ben-Cohen wrote: > On Fri, Sep 26, 2014 at 7:25 PM, Suman Anna <s-anna@ti.com> wrote: >> I am yet to receive any comments on v6, but that series should address >> both your need for a probe deferral and Ohad's request to not change any >> return types. Please give it a try and let me know if you have any comments. > > Guys, > > Just to let you know we have several holidays around here these days, > and I intend to review all pending patches immediately soon after. > Ping on this. Can you review the latest series v6 [1] and pick it up for 3.19? The MSM spinlock driver is also blocked/dependent on that series. regards Suman [1] http://marc.info/?l=linux-arm-kernel&m=141055365513902&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
Hi Suman, On Thu, Nov 6, 2014 at 8:24 PM, Suman Anna <s-anna@ti.com> wrote: > Ping on this. Can you review the latest series v6 [1] and pick it up for > 3.19? The MSM spinlock driver is also blocked/dependent on that series. Sure, it's on my mind, I hope I'll get to it next week. 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
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..1e299f7 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,59 @@ 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) { + pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n", + __func__, np->full_name, index, 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) { + pr_warn("%s: requested hwspinlock device %s is not registered\n", + __func__, args.np->full_name); + return NULL; + } + + id = bank->ops->of_xlate(bank, &args); + if (id < 0 || id >= bank->num_locks) { + pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n", + __func__, id, bank->num_locks - 1); + 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..2453057 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,16 @@ 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 +87,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 +104,13 @@ 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 | 108 ++++++++++++++++++++++++++++++- drivers/hwspinlock/hwspinlock_internal.h | 4 ++ include/linux/hwspinlock.h | 20 +++++- 4 files changed, 161 insertions(+), 5 deletions(-)