[v2] clk: add more managed APIs
diff mbox series

Message ID 20170129180743.GA10917@dtor-ws
State New, archived
Headers show
Series
  • [v2] clk: add more managed APIs
Related show

Commit Message

Dmitry Torokhov Jan. 29, 2017, 6:07 p.m. UTC
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
clocks 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().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

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(-)

Comments

Guenter Roeck Jan. 29, 2017, 6:31 p.m. UTC | #1
On 01/29/2017 10:07 AM, 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
> clocks 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().
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Not really my area of expertise, but LGTM.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>
> 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;
>
Stephen Boyd Jan. 30, 2017, 6:55 p.m. UTC | #2
On 01/29, 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
> clocks in the same way we manage many other resources.

Can you please add 'managing clock prepared and enabled state in
the same way'?

The current wording makes it sound like we don't have
devm_clk_get() when we do.

> 
> This adds the following managed APIs:
> 
> - devm_clk_prepare()/devm_clk_unprepare();
> - devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
be even shorter to have the APIs

  devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
  devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()

instead?

Is there any other subsystem that has similar functionality?
Regulators? GPIOs? Resets? I'm just curious if those subsystems
also need similar changes.
Guenter Roeck Jan. 30, 2017, 7:22 p.m. UTC | #3
On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> On 01/29, 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
> > clocks in the same way we manage many other resources.
> 
> Can you please add 'managing clock prepared and enabled state in
> the same way'?
> 
> The current wording makes it sound like we don't have
> devm_clk_get() when we do.
> 
> > 
> > This adds the following managed APIs:
> > 
> > - devm_clk_prepare()/devm_clk_unprepare();
> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> 
> Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> be even shorter to have the APIs
> 
>   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
>   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> 
> instead?
> 
In many cases I see

	devm_clk_get(clk1);
	devm_clk_get(clk2);
	clk_prepare_enable(clk1);
	clk_prepare_enable(clk2);

Sometimes the calls are intertwined with setting the clock rates.

	devm_clk_get(clk);
	clk_set_rate(clk, rate);
	clk_prepare_enable(clk);

Maybe the additional calls make sense; I can imagine they would.
However, I personally would be a bit wary of changing the initialization
order of multi-clock initializations, and I am not sure how a single call
could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
seems like a bit too much).

[ On a side note, why is there no clk_get_prepare_enable() and
  clk_get_prepare() ? Maybe it would be better to introduce those
  together with the matching devm_ functions in a separate patch
  if they are useful. ]

> Is there any other subsystem that has similar functionality?
> Regulators? GPIOs? Resets? I'm just curious if those subsystems
> also need similar changes.
> 
Ultimately yes, and most already do. If I recall correctly, I tried to
introduce devm_ functions for regulators some time ago, but that was
rejected with the comment that it would invite misuse.  At the time
I accepted that; today my reaction would be to counter that pretty much
everything can be misused, and that the potential for misuse should not
penaltize all the valid use cases.

Thanks,
Guenter
Russell King - ARM Linux admin Jan. 30, 2017, 9:42 p.m. UTC | #4
On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> Maybe the additional calls make sense; I can imagine they would.
> However, I personally would be a bit wary of changing the initialization
> order of multi-clock initializations, and I am not sure how a single call
> could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> seems like a bit too much).
> 
> [ On a side note, why is there no clk_get_prepare_enable() and
>   clk_get_prepare() ? Maybe it would be better to introduce those
>   together with the matching devm_ functions in a separate patch
>   if they are useful. ]

If you take the view that trying to keep clocks disabled is a good way
to save power, then you'd have the clk_prepare() or maybe
clk_prepare_enable() in your runtime PM resume handler, or maybe even
deeper in the driver... the original design goal of the clk API was to
allow power saving and clock control.

With that in mind, getting and enabling the clock together in the
probe function didn't make sense.

I feel that aspect has been somewhat lost, and people now regard much
of the clk API as a bit of a probe-time nusience.
Dmitry Torokhov Jan. 30, 2017, 9:58 p.m. UTC | #5
On Mon, Jan 30, 2017 at 1:42 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
>> Maybe the additional calls make sense; I can imagine they would.
>> However, I personally would be a bit wary of changing the initialization
>> order of multi-clock initializations, and I am not sure how a single call
>> could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
>> seems like a bit too much).
>>
>> [ On a side note, why is there no clk_get_prepare_enable() and
>>   clk_get_prepare() ? Maybe it would be better to introduce those
>>   together with the matching devm_ functions in a separate patch
>>   if they are useful. ]
>
> If you take the view that trying to keep clocks disabled is a good way
> to save power, then you'd have the clk_prepare() or maybe
> clk_prepare_enable() in your runtime PM resume handler, or maybe even
> deeper in the driver... the original design goal of the clk API was to
> allow power saving and clock control.
>
> With that in mind, getting and enabling the clock together in the
> probe function didn't make sense.
>
> I feel that aspect has been somewhat lost, and people now regard much
> of the clk API as a bit of a probe-time nusience.

It really depends on the driver. Devices that are expected to be used
"all the time", like keyboards, that get "opened" very early in the
boot process, maybe even by the kernel itself, usually enable hardware
in probe as doing it later just makes code more complex. Devices that
are "less used" and could provide more power savings (like GPU) may
have more elaborate power management with clocks being turned on and
off as needed. I would not paint clk API as "probe-time nuisance", but
for some class of devices devm CLK API is really helpful.

FWIW I do not think that kitchen sink calls, like
"devm_clk_get_prepare_set_rate_enable" are helpful. Resource
acquisition and use of said resource are logically separate.

Thanks.
Russell King - ARM Linux admin Jan. 30, 2017, 10:25 p.m. UTC | #6
On Mon, Jan 30, 2017 at 01:58:05PM -0800, Dmitry Torokhov wrote:
> FWIW I do not think that kitchen sink calls, like
> "devm_clk_get_prepare_set_rate_enable" are helpful. Resource
> acquisition and use of said resource are logically separate.

That alone is an argument against devm_clk_get_prepare_enable() in
so far that drivers should really get all their resources before
they start to enable anything.
Guenter Roeck Jan. 30, 2017, 10:51 p.m. UTC | #7
On Mon, Jan 30, 2017 at 09:42:28PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> > Maybe the additional calls make sense; I can imagine they would.
> > However, I personally would be a bit wary of changing the initialization
> > order of multi-clock initializations, and I am not sure how a single call
> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> > seems like a bit too much).
> > 
> > [ On a side note, why is there no clk_get_prepare_enable() and
> >   clk_get_prepare() ? Maybe it would be better to introduce those
> >   together with the matching devm_ functions in a separate patch
> >   if they are useful. ]
> 
> If you take the view that trying to keep clocks disabled is a good way
> to save power, then you'd have the clk_prepare() or maybe
> clk_prepare_enable() in your runtime PM resume handler, or maybe even
> deeper in the driver... the original design goal of the clk API was to
> allow power saving and clock control.
> 
> With that in mind, getting and enabling the clock together in the
> probe function didn't make sense.
> 
> I feel that aspect has been somewhat lost, and people now regard much
> of the clk API as a bit of a probe-time nusience.
> 

While I understand what you are saying, I think just focusing on power
savings paints a bit of a simplistic view of the clock API and its use.
Power saving is not its only use case. In a system where power saving isn't
the highest priority (say, in a high end switch), it is still extremely
valuable, providing a unified API to the clocks in the system (and there
are lots of clocks in a high end switch). Having seen what happens if there
is _no_ unified API (ie a complete mess of per-clock-driver calls all over
the place), I consider this as at least as or even more important than its
power savings potential. In this use case, one would normally both get and
enable the clock (or, much more likely, clocks) in the probe function.
One would also need remove functions, since interfaces in a high end switch
are typically OIR capable.

For my part, I stumbled over the lack of devm functions for clock APIs recently
when trying to convert watchdog drivers to use devm functions where possible.
Many watchdog drivers do use the clock API to only enable the clock when the
watchdog is actually running. However, there are several which prepare and
enable the clock at probe time, and disable/unprepare on remove. Would it be
possible to convert those to only prepare/enable the clocks if the watchdog
is actually enabled ? Possibly, but I am sure that there are cases where that
is not possible, or feasible. Either way, watchdog drivers are usually only
loaded when actually used, so trying to optimize clock usage would often be
more pain than it is worth.

When I did that conversion, I started out using devm_add_action_or_reset().
While that does work, it was pointed out that using devm functions for the
clock APIs would be a much better solution. As it turns out, devm_add_action()
and devm_add_action_or_reset() is already being used by various drivers to
work around the lack of devm functions for the clock API. With that in mind,
have a choice to make - we can continue forcing people to do that, or we can
introduce devm functions. My vote is for the latter.

Thanks,
Guenter
Dmitry Torokhov Jan. 31, 2017, 12:59 a.m. UTC | #8
On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> > On 01/29, 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
> > > clocks in the same way we manage many other resources.
> > 
> > Can you please add 'managing clock prepared and enabled state in
> > the same way'?
> > 
> > The current wording makes it sound like we don't have
> > devm_clk_get() when we do.
> > 
> > > 
> > > This adds the following managed APIs:
> > > 
> > > - devm_clk_prepare()/devm_clk_unprepare();
> > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> > 
> > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> > be even shorter to have the APIs
> > 
> >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
> >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> > 
> > instead?
> > 
> In many cases I see
> 
> 	devm_clk_get(clk1);
> 	devm_clk_get(clk2);
> 	clk_prepare_enable(clk1);
> 	clk_prepare_enable(clk2);
> 
> Sometimes the calls are intertwined with setting the clock rates.
> 
> 	devm_clk_get(clk);
> 	clk_set_rate(clk, rate);
> 	clk_prepare_enable(clk);
> 
> Maybe the additional calls make sense; I can imagine they would.
> However, I personally would be a bit wary of changing the initialization
> order of multi-clock initializations, and I am not sure how a single call
> could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> seems like a bit too much).
> 
> [ On a side note, why is there no clk_get_prepare_enable() and
>   clk_get_prepare() ? Maybe it would be better to introduce those
>   together with the matching devm_ functions in a separate patch
>   if they are useful. ]
> 
> > Is there any other subsystem that has similar functionality?
> > Regulators? GPIOs? Resets? I'm just curious if those subsystems
> > also need similar changes.
> > 
> Ultimately yes, and most already do. If I recall correctly, I tried to
> introduce devm_ functions for regulators some time ago, but that was
> rejected with the comment that it would invite misuse.  At the time
> I accepted that; today my reaction would be to counter that pretty much
> everything can be misused, and that the potential for misuse should not
> penaltize all the valid use cases.

I think we should ping Mark again. The only thing we are achieving is
that everyone is using devm_add_action_or_reset() with wrappers around
regulator_put().

As I said elsewhere, there are "always used" devices where it isn't
worth it to postpone enabling regulators.

Thanks.
Geert Uytterhoeven Jan. 31, 2017, 8:43 a.m. UTC | #9
Hi G√ľnter,

On Mon, Jan 30, 2017 at 11:51 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Jan 30, 2017 at 09:42:28PM +0000, Russell King - ARM Linux wrote:
>> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
>> > Maybe the additional calls make sense; I can imagine they would.
>> > However, I personally would be a bit wary of changing the initialization
>> > order of multi-clock initializations, and I am not sure how a single call
>> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
>> > seems like a bit too much).
>> >
>> > [ On a side note, why is there no clk_get_prepare_enable() and
>> >   clk_get_prepare() ? Maybe it would be better to introduce those
>> >   together with the matching devm_ functions in a separate patch
>> >   if they are useful. ]
>>
>> If you take the view that trying to keep clocks disabled is a good way
>> to save power, then you'd have the clk_prepare() or maybe
>> clk_prepare_enable() in your runtime PM resume handler, or maybe even
>> deeper in the driver... the original design goal of the clk API was to
>> allow power saving and clock control.
>>
>> With that in mind, getting and enabling the clock together in the
>> probe function didn't make sense.
>>
>> I feel that aspect has been somewhat lost, and people now regard much
>> of the clk API as a bit of a probe-time nusience.
>
> While I understand what you are saying, I think just focusing on power
> savings paints a bit of a simplistic view of the clock API and its use.
> Power saving is not its only use case. In a system where power saving isn't
> the highest priority (say, in a high end switch), it is still extremely
> valuable, providing a unified API to the clocks in the system (and there
> are lots of clocks in a high end switch). Having seen what happens if there
> is _no_ unified API (ie a complete mess of per-clock-driver calls all over
> the place), I consider this as at least as or even more important than its
> power savings potential. In this use case, one would normally both get and
> enable the clock (or, much more likely, clocks) in the probe function.
> One would also need remove functions, since interfaces in a high end switch
> are typically OIR capable.
>
> For my part, I stumbled over the lack of devm functions for clock APIs recently
> when trying to convert watchdog drivers to use devm functions where possible.
> Many watchdog drivers do use the clock API to only enable the clock when the
> watchdog is actually running. However, there are several which prepare and
> enable the clock at probe time, and disable/unprepare on remove. Would it be
> possible to convert those to only prepare/enable the clocks if the watchdog
> is actually enabled ? Possibly, but I am sure that there are cases where that
> is not possible, or feasible. Either way, watchdog drivers are usually only
> loaded when actually used, so trying to optimize clock usage would often be
> more pain than it is worth.
>
> When I did that conversion, I started out using devm_add_action_or_reset().
> While that does work, it was pointed out that using devm functions for the
> clock APIs would be a much better solution. As it turns out, devm_add_action()
> and devm_add_action_or_reset() is already being used by various drivers to
> work around the lack of devm functions for the clock API. With that in mind,
> have a choice to make - we can continue forcing people to do that, or we can
> introduce devm functions. My vote is for the latter.

W.r.t. clocks, there may be multiple reasons why you care about them:
  1. You need to control a clock output to a slave device (e.g. SPI, i2c,
     audio, ...).
     Here your driver cares about both enabling/disabling the clock, and
     programming its clock rate.
  2. You need to control a clock because synchronous hardware needs a running
     clock to operate.
     Here you only care about enabling/disabling the clock, and this falls
     under the "power saving" umbrella.
     These days it's typically handled by a clock domain driver implementing
     a PM Domain using the genpd framework.  Hence your driver doesn't need
     to manage the clock explicitly (no clk_get()/clk_prepare_enable() etc.),
     but just calls the pm_runtime_*() functions.
     Using pm_runtime_*() is always a good idea, even if currently there is
     no clock to control, as your device may end up in a future SoC that does
     have a clock (or power domains).
  3. Combinations of the above.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Guenter Roeck Jan. 31, 2017, 5:20 p.m. UTC | #10
On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> > > On 01/29, 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
> > > > clocks in the same way we manage many other resources.
> > > 
> > > Can you please add 'managing clock prepared and enabled state in
> > > the same way'?
> > > 
> > > The current wording makes it sound like we don't have
> > > devm_clk_get() when we do.
> > > 
> > > > 
> > > > This adds the following managed APIs:
> > > > 
> > > > - devm_clk_prepare()/devm_clk_unprepare();
> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> > > 
> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> > > be even shorter to have the APIs
> > > 
> > >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
> > >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> > > 
> > > instead?
> > > 
> > In many cases I see
> > 
> > 	devm_clk_get(clk1);
> > 	devm_clk_get(clk2);
> > 	clk_prepare_enable(clk1);
> > 	clk_prepare_enable(clk2);
> > 
> > Sometimes the calls are intertwined with setting the clock rates.
> > 
> > 	devm_clk_get(clk);
> > 	clk_set_rate(clk, rate);
> > 	clk_prepare_enable(clk);
> > 
> > Maybe the additional calls make sense; I can imagine they would.
> > However, I personally would be a bit wary of changing the initialization
> > order of multi-clock initializations, and I am not sure how a single call
> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> > seems like a bit too much).
> > 
> > [ On a side note, why is there no clk_get_prepare_enable() and
> >   clk_get_prepare() ? Maybe it would be better to introduce those
> >   together with the matching devm_ functions in a separate patch
> >   if they are useful. ]
> > 
> > > Is there any other subsystem that has similar functionality?
> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems
> > > also need similar changes.
> > > 
> > Ultimately yes, and most already do. If I recall correctly, I tried to
> > introduce devm_ functions for regulators some time ago, but that was
> > rejected with the comment that it would invite misuse.  At the time
> > I accepted that; today my reaction would be to counter that pretty much
> > everything can be misused, and that the potential for misuse should not
> > penaltize all the valid use cases.
> 
> I think we should ping Mark again. The only thing we are achieving is
> that everyone is using devm_add_action_or_reset() with wrappers around
> regulator_put().
> 
regulator_get() has an equivalent devm_regulator_get(). Maybe it was since
added, or I was thinking about a different function.

Guenter
Dmitry Torokhov Jan. 31, 2017, 6:26 p.m. UTC | #11
On Tue, Jan 31, 2017 at 9:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote:
>> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
>> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
>> > > On 01/29, 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
>> > > > clocks in the same way we manage many other resources.
>> > >
>> > > Can you please add 'managing clock prepared and enabled state in
>> > > the same way'?
>> > >
>> > > The current wording makes it sound like we don't have
>> > > devm_clk_get() when we do.
>> > >
>> > > >
>> > > > This adds the following managed APIs:
>> > > >
>> > > > - devm_clk_prepare()/devm_clk_unprepare();
>> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
>> > >
>> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
>> > > be even shorter to have the APIs
>> > >
>> > >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
>> > >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
>> > >
>> > > instead?
>> > >
>> > In many cases I see
>> >
>> >     devm_clk_get(clk1);
>> >     devm_clk_get(clk2);
>> >     clk_prepare_enable(clk1);
>> >     clk_prepare_enable(clk2);
>> >
>> > Sometimes the calls are intertwined with setting the clock rates.
>> >
>> >     devm_clk_get(clk);
>> >     clk_set_rate(clk, rate);
>> >     clk_prepare_enable(clk);
>> >
>> > Maybe the additional calls make sense; I can imagine they would.
>> > However, I personally would be a bit wary of changing the initialization
>> > order of multi-clock initializations, and I am not sure how a single call
>> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
>> > seems like a bit too much).
>> >
>> > [ On a side note, why is there no clk_get_prepare_enable() and
>> >   clk_get_prepare() ? Maybe it would be better to introduce those
>> >   together with the matching devm_ functions in a separate patch
>> >   if they are useful. ]
>> >
>> > > Is there any other subsystem that has similar functionality?
>> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems
>> > > also need similar changes.
>> > >
>> > Ultimately yes, and most already do. If I recall correctly, I tried to
>> > introduce devm_ functions for regulators some time ago, but that was
>> > rejected with the comment that it would invite misuse.  At the time
>> > I accepted that; today my reaction would be to counter that pretty much
>> > everything can be misused, and that the potential for misuse should not
>> > penaltize all the valid use cases.
>>
>> I think we should ping Mark again. The only thing we are achieving is
>> that everyone is using devm_add_action_or_reset() with wrappers around
>> regulator_put().
>>
> regulator_get() has an equivalent devm_regulator_get(). Maybe it was since
> added, or I was thinking about a different function.

I think we also need devm_regulator_enable().

Thanks.
Guenter Roeck Jan. 31, 2017, 7:34 p.m. UTC | #12
On Tue, Jan 31, 2017 at 10:26:29AM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 31, 2017 at 9:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Mon, Jan 30, 2017 at 04:59:57PM -0800, Dmitry Torokhov wrote:
> >> On Mon, Jan 30, 2017 at 11:22:14AM -0800, Guenter Roeck wrote:
> >> > On Mon, Jan 30, 2017 at 10:55:51AM -0800, Stephen Boyd wrote:
> >> > > On 01/29, 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
> >> > > > clocks in the same way we manage many other resources.
> >> > >
> >> > > Can you please add 'managing clock prepared and enabled state in
> >> > > the same way'?
> >> > >
> >> > > The current wording makes it sound like we don't have
> >> > > devm_clk_get() when we do.
> >> > >
> >> > > >
> >> > > > This adds the following managed APIs:
> >> > > >
> >> > > > - devm_clk_prepare()/devm_clk_unprepare();
> >> > > > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> >> > >
> >> > > Wouldn't this be preceded by a devm_clk_get() call? Wouldn't it
> >> > > be even shorter to have the APIs
> >> > >
> >> > >   devm_clk_get_and_prepare()/devm_clk_unprepare_and_put()
> >> > >   devm_clk_get_and_prepare_enable()/devm_clk_disable_unprepare_and_put()
> >> > >
> >> > > instead?
> >> > >
> >> > In many cases I see
> >> >
> >> >     devm_clk_get(clk1);
> >> >     devm_clk_get(clk2);
> >> >     clk_prepare_enable(clk1);
> >> >     clk_prepare_enable(clk2);
> >> >
> >> > Sometimes the calls are intertwined with setting the clock rates.
> >> >
> >> >     devm_clk_get(clk);
> >> >     clk_set_rate(clk, rate);
> >> >     clk_prepare_enable(clk);
> >> >
> >> > Maybe the additional calls make sense; I can imagine they would.
> >> > However, I personally would be a bit wary of changing the initialization
> >> > order of multi-clock initializations, and I am not sure how a single call
> >> > could address setting the rate ([devm_]clk_get_setrate_prepare_enable()
> >> > seems like a bit too much).
> >> >
> >> > [ On a side note, why is there no clk_get_prepare_enable() and
> >> >   clk_get_prepare() ? Maybe it would be better to introduce those
> >> >   together with the matching devm_ functions in a separate patch
> >> >   if they are useful. ]
> >> >
> >> > > Is there any other subsystem that has similar functionality?
> >> > > Regulators? GPIOs? Resets? I'm just curious if those subsystems
> >> > > also need similar changes.
> >> > >
> >> > Ultimately yes, and most already do. If I recall correctly, I tried to
> >> > introduce devm_ functions for regulators some time ago, but that was
> >> > rejected with the comment that it would invite misuse.  At the time
> >> > I accepted that; today my reaction would be to counter that pretty much
> >> > everything can be misused, and that the potential for misuse should not
> >> > penaltize all the valid use cases.
> >>
> >> I think we should ping Mark again. The only thing we are achieving is
> >> that everyone is using devm_add_action_or_reset() with wrappers around
> >> regulator_put().
> >>
> > regulator_get() has an equivalent devm_regulator_get(). Maybe it was since
> > added, or I was thinking about a different function.
> 
> I think we also need devm_regulator_enable().
> 
Ah, yes, that was it [1]. And, yes, I do see some devm_add_action() calls
around regulator_enable().

Guenter

---
[1] https://lkml.org/lkml/2014/2/1/131

Patch
diff mbox series

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;