Message ID | 20170131005713.GA35974@dtor-ws (mailing list archive) |
---|---|
State | Not Applicable, archived |
Delegated to: | Stephen Boyd |
Headers | show |
On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote: > When converting a driver to managed resources it is desirable to be able to > manage all resources in the same fashion. This change allows managing clock > prepared and enabled state in the same way we manage many other resources. > > This adds the following managed APIs: > > - devm_clk_prepare()/devm_clk_unprepare(); > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> It would be awesome if we could get it into 4.11... > --- > > v3: adjusted commit message, added Guenter's reviewed-by > v2: dropped devm_clk_enable() and devm_clk_disable() > > drivers/clk/clk-devres.c | 98 +++++++++++++++++++++++++++++++--------------- > include/linux/clk.h | 68 ++++++++++++++++++++++++++++++++ > 2 files changed, 134 insertions(+), 32 deletions(-) > > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c > index 3a218c3a06ae..2ff94ffe11d3 100644 > --- a/drivers/clk/clk-devres.c > +++ b/drivers/clk/clk-devres.c > @@ -9,30 +9,20 @@ > #include <linux/export.h> > #include <linux/gfp.h> > > -static void devm_clk_release(struct device *dev, void *res) > +static int devm_clk_create_devres(struct device *dev, struct clk *clk, > + void (*release)(struct device *, void *)) > { > - clk_put(*(struct clk **)res); > -} > + struct clk **ptr; > > -struct clk *devm_clk_get(struct device *dev, const char *id) > -{ > - struct clk **ptr, *clk; > - > - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); > + ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL); > if (!ptr) > - return ERR_PTR(-ENOMEM); > + return -ENOMEM; > > - clk = clk_get(dev, id); > - if (!IS_ERR(clk)) { > - *ptr = clk; > - devres_add(dev, ptr); > - } else { > - devres_free(ptr); > - } > + *ptr = clk; > + devres_add(dev, ptr); > > - return clk; > + return 0; > } > -EXPORT_SYMBOL(devm_clk_get); > > static int devm_clk_match(struct device *dev, void *res, void *data) > { > @@ -44,31 +34,75 @@ static int devm_clk_match(struct device *dev, void *res, void *data) > return *c == data; > } > > -void devm_clk_put(struct device *dev, struct clk *clk) > +#define DEFINE_DEVM_CLK_DESTROY_OP(destroy_op) \ > +static void devm_##destroy_op##_release(struct device *dev, void *res) \ > +{ \ > + destroy_op(*(struct clk **)res); \ > +} \ > + \ > +void devm_##destroy_op(struct device *dev, struct clk *clk) \ > +{ \ > + WARN_ON(devres_release(dev, devm_##destroy_op##_release, \ > + devm_clk_match, clk)); \ > +} \ > +EXPORT_SYMBOL(devm_##destroy_op) > + > +#define DEFINE_DEVM_CLK_OP(create_op, destroy_op) \ > +DEFINE_DEVM_CLK_DESTROY_OP(destroy_op); \ > +int devm_##create_op(struct device *dev, struct clk *clk) \ > +{ \ > + int error; \ > + \ > + error = create_op(clk); \ > + if (error) \ > + return error; \ > + \ > + error = devm_clk_create_devres(dev, clk, \ > + devm_##destroy_op##_release); \ > + if (error) { \ > + destroy_op(clk); \ > + return error; \ > + } \ > + \ > + return 0; \ > +} \ > +EXPORT_SYMBOL(devm_##create_op) > + > +DEFINE_DEVM_CLK_DESTROY_OP(clk_put); > +DEFINE_DEVM_CLK_OP(clk_prepare, clk_unprepare); > +DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare); > + > +struct clk *devm_clk_get(struct device *dev, const char *id) > { > - int ret; > + struct clk *clk; > + int error; > > - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk); > + clk = clk_get(dev, id); > + if (!IS_ERR(clk)) { > + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); > + if (error) { > + clk_put(clk); > + return ERR_PTR(error); > + } > + } > > - WARN_ON(ret); > + return clk; > } > -EXPORT_SYMBOL(devm_clk_put); > +EXPORT_SYMBOL(devm_clk_get); > > struct clk *devm_get_clk_from_child(struct device *dev, > struct device_node *np, const char *con_id) > { > - struct clk **ptr, *clk; > - > - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); > - if (!ptr) > - return ERR_PTR(-ENOMEM); > + struct clk *clk; > + int error; > > clk = of_clk_get_by_name(np, con_id); > if (!IS_ERR(clk)) { > - *ptr = clk; > - devres_add(dev, ptr); > - } else { > - devres_free(ptr); > + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); > + if (error) { > + clk_put(clk); > + return ERR_PTR(error); > + } > } > > return clk; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index e9d36b3e49de..413dc8f636bd 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -267,6 +267,29 @@ struct clk *devm_get_clk_from_child(struct device *dev, > struct device_node *np, const char *con_id); > > /** > + * devm_clk_prepare - prepare clock source as a managed resource > + * @dev: device owning the resource > + * @clk: clock source > + * > + * This prepares the clock source for use. > + * > + * Must not be called from within atomic context. > + */ > +int devm_clk_prepare(struct device *dev, struct clk *clk); > + > +/** > + * devm_clk_unprepare - undo preparation of a managed clock source > + * @dev: device used to prepare the clock > + * @clk: clock source > + * > + * This undoes preparation of a clock, previously prepared with a call > + * to devm_clk_pepare(). > + * > + * Must not be called from within atomic context. > + */ > +void devm_clk_unprepare(struct device *dev, struct clk *clk); > + > +/** > * clk_enable - inform the system when the clock source should be running. > * @clk: clock source > * > @@ -295,6 +318,28 @@ int clk_enable(struct clk *clk); > void clk_disable(struct clk *clk); > > /** > + * devm_clk_prepare_enable - prepare and enable a managed clock source > + * @dev: device owning the clock source > + * @clk: clock source > + * > + * This prepares the clock source for use and enables it. > + * > + * Must not be called from within atomic context. > + */ > +int devm_clk_prepare_enable(struct device *dev, struct clk *clk); > + > +/** > + * devm_clk_disable_unprepare - disable and undo preparation of a managed clock > + * @dev: device used to prepare and enable the clock > + * @clk: clock source > + * > + * This disables and undoes a previously prepared clock. > + * > + * Must not be called from within atomic context. > + */ > +void devm_clk_disable_unprepare(struct device *dev, struct clk *clk); > + > +/** > * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. > * This is only valid once the clock source has been enabled. > * @clk: clock source > @@ -460,6 +505,17 @@ static inline void clk_put(struct clk *clk) {} > > static inline void devm_clk_put(struct device *dev, struct clk *clk) {} > > +static inline int devm_clk_prepare(struct device *dev, struct clk *clk) > +{ > + might_sleep(); > + return 0; > +} > + > +static inline void devm_clk_unprepare(struct device *dev, struct clk *clk) > +{ > + might_sleep(); > +} > + > static inline int clk_enable(struct clk *clk) > { > return 0; > @@ -467,6 +523,18 @@ static inline int clk_enable(struct clk *clk) > > static inline void clk_disable(struct clk *clk) {} > > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > +{ > + might_sleep(); > + return 0; > +} > + > +static inline void devm_clk_disable_unprepare(struct device *dev, > + struct clk *clk) > +{ > + might_sleep(); > +} > + > static inline unsigned long clk_get_rate(struct clk *clk) > { > return 0;
On 02/06, Dmitry Torokhov wrote: > On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote: > > When converting a driver to managed resources it is desirable to be able to > > manage all resources in the same fashion. This change allows managing clock > > prepared and enabled state in the same way we manage many other resources. > > > > This adds the following managed APIs: > > > > - devm_clk_prepare()/devm_clk_unprepare(); > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > It would be awesome if we could get it into 4.11... I'd prefer we didn't do this. Instead, make clk_put() drop any prepare or enables that were done via that clk pointer. Mike started to do this before[1], but we have some code that assumes it can do: clk = clk_get(...) clk_prepare_enable(clk) clk_put(clk) and have the clk stay on. Those would need to be changed. We would also need Russell's approval to update the clk_put() documentation to describe this change in behavior. [1] http://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette@baylibre.com
On Tue, Feb 14, 2017 at 11:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 02/06, Dmitry Torokhov wrote: >> On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote: >> > When converting a driver to managed resources it is desirable to be able to >> > manage all resources in the same fashion. This change allows managing clock >> > prepared and enabled state in the same way we manage many other resources. >> > >> > This adds the following managed APIs: >> > >> > - devm_clk_prepare()/devm_clk_unprepare(); >> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). >> > >> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> >> It would be awesome if we could get it into 4.11... > > I'd prefer we didn't do this. Instead, make clk_put() drop any > prepare or enables that were done via that clk pointer. Mike > started to do this before[1], but we have some code that assumes > it can do: > > clk = clk_get(...) > clk_prepare_enable(clk) > clk_put(clk) > > and have the clk stay on. Those would need to be changed. That means we'd need to audit entire code base ;( > > We would also need Russell's approval to update the clk_put() > documentation to describe this change in behavior. > > [1] http://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette@baylibre.com > Note that devm* APIs do not preclude from changing clk_put() behavior down the road and it is extremely easy to go and s/devm_clk_prepare_enable/clk_prepare_enable/ once cleanup is automatic. Having devm now will help make driver code better (because right now we either need to add wrappers so devm_add_action_or_reset() can be used, or continue mixing devm* and goto cleanups, which are often wrong). Thanks.
On Tue, Feb 14, 2017 at 11:44:08AM -0800, Stephen Boyd wrote: > I'd prefer we didn't do this. Instead, make clk_put() drop any > prepare or enables that were done via that clk pointer. Mike > started to do this before[1], but we have some code that assumes > it can do: > > clk = clk_get(...) > clk_prepare_enable(clk) > clk_put(clk) > > and have the clk stay on. Those would need to be changed. Yes, I've seen from time to time code that plays this game over the years. However, from my simple grepping, it seems that we only have a small number of cases: arch/mips/alchemy/devboards/db1200.c: clk_prepare_enable(c); arch/mips/alchemy/devboards/db1200.c- clk_put(c); arch/mips/alchemy/devboards/db1300.c: clk_prepare_enable(c); arch/mips/alchemy/devboards/db1300.c- clk_put(c); arch/mips/alchemy/devboards/db1550.c: clk_prepare_enable(c); arch/mips/alchemy/devboards/db1550.c- clk_put(c); drivers/base/power/clock_ops.c: clk_prepare_enable(clk); drivers/base/power/clock_ops.c- clk_put(clk); drivers/mtd/nand/orion_nand.c: clk_prepare_enable(clk); drivers/mtd/nand/orion_nand.c- clk_put(clk); I've always hated that - and it goes against the API: * Note: drivers must ensure that all clk_enable calls made on this * clock source are balanced by clk_disable calls prior to calling * this function. (That comment should have been updated when clk_prepare() & clk_unprepare() was added to include balancing those.) So really, all the cases above are buggy. However, the statement in the API doesn't give permission for what you're suggesting! The suggestion requires that we also cast in stone that every "struct clk" which is handed out from clk_get() becomes unique _everywhere_, because that's the only way to really track the prepare/enable state on a per- clk_get() basis, so we can properly clean up at clk_put(). However, I think we still have some non-CCF clock API implementations around, which hand out shared "struct clk"s. Changing this requirement will impact those, since they would need to be updated before the change could be made. So, although it would be a nice change to make (which would rule out the abuse) I don't think it's one that could be practically made at this stage without (a) fixing all instances like those quoted above and (b) converting all non-CCF implementations to either CCF or making them hand out unique struct clk's. (I do have patches which converts sa11x0 to CCF... which would be one less non-CCF implementation.)
On Tue, Feb 14, 2017 at 11:55:20AM -0800, Dmitry Torokhov wrote: > On Tue, Feb 14, 2017 at 11:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 02/06, Dmitry Torokhov wrote: > >> On Mon, Jan 30, 2017 at 04:57:13PM -0800, Dmitry Torokhov wrote: > >> > When converting a driver to managed resources it is desirable to be able to > >> > manage all resources in the same fashion. This change allows managing clock > >> > prepared and enabled state in the same way we manage many other resources. > >> > > >> > This adds the following managed APIs: > >> > > >> > - devm_clk_prepare()/devm_clk_unprepare(); > >> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare(). > >> > > >> > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > >> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> > >> It would be awesome if we could get it into 4.11... > > > > I'd prefer we didn't do this. Instead, make clk_put() drop any > > prepare or enables that were done via that clk pointer. Mike > > started to do this before[1], but we have some code that assumes > > it can do: > > > > clk = clk_get(...) > > clk_prepare_enable(clk) > > clk_put(clk) > > > > and have the clk stay on. Those would need to be changed. > > That means we'd need to audit entire code base ;( > > > > > We would also need Russell's approval to update the clk_put() > > documentation to describe this change in behavior. > > > > [1] http://lkml.kernel.org/r/1438974570-20812-1-git-send-email-mturquette@baylibre.com > > > > Note that devm* APIs do not preclude from changing clk_put() behavior > down the road and it is extremely easy to go and > s/devm_clk_prepare_enable/clk_prepare_enable/ once cleanup is > automatic. > > Having devm now will help make driver code better (because right now > we either need to add wrappers so devm_add_action_or_reset() can be > used, or continue mixing devm* and goto cleanups, which are often > wrong). > Absolutely agree. The combination of clk_prepare_enable() in probe and clk_disable_unprepare() in remove is used all over the place. Sure, we _can_ use devm_add_action_or_reset() for all those cases, and I do have a coccinelle script doing just that. While I'll have to accept going forward with that approach if needed, I have to admit that I completely fail to miss the point why that would be a good idea. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-clk" 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/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index 3a218c3a06ae..2ff94ffe11d3 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -9,30 +9,20 @@ #include <linux/export.h> #include <linux/gfp.h> -static void devm_clk_release(struct device *dev, void *res) +static int devm_clk_create_devres(struct device *dev, struct clk *clk, + void (*release)(struct device *, void *)) { - clk_put(*(struct clk **)res); -} + struct clk **ptr; -struct clk *devm_clk_get(struct device *dev, const char *id) -{ - struct clk **ptr, *clk; - - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); + ptr = devres_alloc(release, sizeof(*ptr), GFP_KERNEL); if (!ptr) - return ERR_PTR(-ENOMEM); + return -ENOMEM; - clk = clk_get(dev, id); - if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); - } + *ptr = clk; + devres_add(dev, ptr); - return clk; + return 0; } -EXPORT_SYMBOL(devm_clk_get); static int devm_clk_match(struct device *dev, void *res, void *data) { @@ -44,31 +34,75 @@ static int devm_clk_match(struct device *dev, void *res, void *data) return *c == data; } -void devm_clk_put(struct device *dev, struct clk *clk) +#define DEFINE_DEVM_CLK_DESTROY_OP(destroy_op) \ +static void devm_##destroy_op##_release(struct device *dev, void *res) \ +{ \ + destroy_op(*(struct clk **)res); \ +} \ + \ +void devm_##destroy_op(struct device *dev, struct clk *clk) \ +{ \ + WARN_ON(devres_release(dev, devm_##destroy_op##_release, \ + devm_clk_match, clk)); \ +} \ +EXPORT_SYMBOL(devm_##destroy_op) + +#define DEFINE_DEVM_CLK_OP(create_op, destroy_op) \ +DEFINE_DEVM_CLK_DESTROY_OP(destroy_op); \ +int devm_##create_op(struct device *dev, struct clk *clk) \ +{ \ + int error; \ + \ + error = create_op(clk); \ + if (error) \ + return error; \ + \ + error = devm_clk_create_devres(dev, clk, \ + devm_##destroy_op##_release); \ + if (error) { \ + destroy_op(clk); \ + return error; \ + } \ + \ + return 0; \ +} \ +EXPORT_SYMBOL(devm_##create_op) + +DEFINE_DEVM_CLK_DESTROY_OP(clk_put); +DEFINE_DEVM_CLK_OP(clk_prepare, clk_unprepare); +DEFINE_DEVM_CLK_OP(clk_prepare_enable, clk_disable_unprepare); + +struct clk *devm_clk_get(struct device *dev, const char *id) { - int ret; + struct clk *clk; + int error; - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk); + clk = clk_get(dev, id); + if (!IS_ERR(clk)) { + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); + if (error) { + clk_put(clk); + return ERR_PTR(error); + } + } - WARN_ON(ret); + return clk; } -EXPORT_SYMBOL(devm_clk_put); +EXPORT_SYMBOL(devm_clk_get); struct clk *devm_get_clk_from_child(struct device *dev, struct device_node *np, const char *con_id) { - struct clk **ptr, *clk; - - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); - if (!ptr) - return ERR_PTR(-ENOMEM); + struct clk *clk; + int error; clk = of_clk_get_by_name(np, con_id); if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); + error = devm_clk_create_devres(dev, clk, devm_clk_put_release); + if (error) { + clk_put(clk); + return ERR_PTR(error); + } } return clk; diff --git a/include/linux/clk.h b/include/linux/clk.h index e9d36b3e49de..413dc8f636bd 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -267,6 +267,29 @@ struct clk *devm_get_clk_from_child(struct device *dev, struct device_node *np, const char *con_id); /** + * devm_clk_prepare - prepare clock source as a managed resource + * @dev: device owning the resource + * @clk: clock source + * + * This prepares the clock source for use. + * + * Must not be called from within atomic context. + */ +int devm_clk_prepare(struct device *dev, struct clk *clk); + +/** + * devm_clk_unprepare - undo preparation of a managed clock source + * @dev: device used to prepare the clock + * @clk: clock source + * + * This undoes preparation of a clock, previously prepared with a call + * to devm_clk_pepare(). + * + * Must not be called from within atomic context. + */ +void devm_clk_unprepare(struct device *dev, struct clk *clk); + +/** * clk_enable - inform the system when the clock source should be running. * @clk: clock source * @@ -295,6 +318,28 @@ int clk_enable(struct clk *clk); void clk_disable(struct clk *clk); /** + * devm_clk_prepare_enable - prepare and enable a managed clock source + * @dev: device owning the clock source + * @clk: clock source + * + * This prepares the clock source for use and enables it. + * + * Must not be called from within atomic context. + */ +int devm_clk_prepare_enable(struct device *dev, struct clk *clk); + +/** + * devm_clk_disable_unprepare - disable and undo preparation of a managed clock + * @dev: device used to prepare and enable the clock + * @clk: clock source + * + * This disables and undoes a previously prepared clock. + * + * Must not be called from within atomic context. + */ +void devm_clk_disable_unprepare(struct device *dev, struct clk *clk); + +/** * clk_get_rate - obtain the current clock rate (in Hz) for a clock source. * This is only valid once the clock source has been enabled. * @clk: clock source @@ -460,6 +505,17 @@ static inline void clk_put(struct clk *clk) {} static inline void devm_clk_put(struct device *dev, struct clk *clk) {} +static inline int devm_clk_prepare(struct device *dev, struct clk *clk) +{ + might_sleep(); + return 0; +} + +static inline void devm_clk_unprepare(struct device *dev, struct clk *clk) +{ + might_sleep(); +} + static inline int clk_enable(struct clk *clk) { return 0; @@ -467,6 +523,18 @@ static inline int clk_enable(struct clk *clk) static inline void clk_disable(struct clk *clk) {} +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) +{ + might_sleep(); + return 0; +} + +static inline void devm_clk_disable_unprepare(struct device *dev, + struct clk *clk) +{ + might_sleep(); +} + static inline unsigned long clk_get_rate(struct clk *clk) { return 0;