linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: add more managed APIs
@ 2017-01-28 18:40 Dmitry Torokhov
  2017-01-28 19:03 ` Russell King - ARM Linux
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-28 18:40 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Russell King, Viresh Kumar, Guenter Roeck, Andy Shevchenko,
	linux-clk, linux-kernel

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_enable()/devm_clk_disable();
- devm_clk_prepare_enable()/devm_clk_disable_unprepare().

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

This is a resend from about 5 years ago ;) At that time I also tried
uninlining clk_prepare_enable() and clk_disable_unprepare(), but got
lost in the maze of COMMON_CLK/HAVE_CLK/HAVE_CLK_PREPARE. This time
around I left these 2 fucntions alone (inline) and simply adding devm*
equivalents.

Thanks!

 drivers/clk/clk-devres.c |  99 +++++++++++++++++++++++++++++++---------------
 include/linux/clk.h      | 101 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 3a218c3a06ae..ef59eb4f3bc8 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,76 @@ 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_enable, clk_disable);
+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..03c21c5fee4c 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
  *
@@ -279,6 +302,19 @@ struct clk *devm_get_clk_from_child(struct device *dev,
 int clk_enable(struct clk *clk);
 
 /**
+ * devm_clk_enable - enable clock source as a managed resource
+ * @dev: device owning the resource
+ * @clk: clock source
+ *
+ * If the clock can not be enabled/disabled, this should return success.
+ *
+ * May be not called from atomic contexts.
+ *
+ * Returns success (0) or negative errno.
+ */
+int devm_clk_enable(struct device *dev, struct clk *clk);
+
+/**
  * clk_disable - inform the system when the clock source is no longer required.
  * @clk: clock source
  *
@@ -295,6 +331,40 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * devm_clk_disable - disable managed clock source resource
+ * @dev: device owning the clock source
+ * @clk: clock source
+ *
+ * Inform the system that a clock source is no longer required by
+ * a driver and may be shut down.
+ *
+ * Must not be called from atomic contexts.
+ */
+void devm_clk_disable(struct device *dev, 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 source
+ * @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 +530,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 +548,26 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline int devm_clk_enable(struct device *dev, struct clk *clk)
+{
+	might_sleep();
+	return 0;
+}
+
+static inline void devm_clk_disable(struct device *dev, 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;
-- 
2.11.0.483.g087da7b7c-goog


-- 
Dmitry

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH] clk: add more managed APIs
  2017-01-28 18:40 [PATCH] clk: add more managed APIs Dmitry Torokhov
@ 2017-01-28 19:03 ` Russell King - ARM Linux
  2017-01-28 19:22   ` Dmitry Torokhov
  0 siblings, 1 reply; 25+ messages in thread
From: Russell King - ARM Linux @ 2017-01-28 19:03 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Michael Turquette, Stephen Boyd, Viresh Kumar, Guenter Roeck,
	Andy Shevchenko, linux-clk, linux-kernel

On Sat, Jan 28, 2017 at 10:40:47AM -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
> clocks in the same way we manage many other resources.
> 
> This adds the following managed APIs:
> 
> - devm_clk_prepare()/devm_clk_unprepare();
> - devm_clk_enable()/devm_clk_disable();
> - devm_clk_prepare_enable()/devm_clk_disable_unprepare().

Does it make any sense what so ever to have devm_clk_enable() and
devm_clk_disable()?

Take a moment to think about where you use all of these.  The devm_*
functions are there to be used in probe functions so that cleanup
paths can be streamlined and less erroneous.  They aren't for general
use throughout the driver.

Given that, there are two operations that you may wish to do in the
probe path:

1. prepare a clock (avoiding the enable because you want to perform
   the enable elsewhere in the driver.)
2. prepare and enable a clock

So, does having devm_clk_enable() really make sense?  I don't think
it does, and I suspect they'll get very little if any use.  So, I
think best not add them until someone comes up with a good and
wide-spread use case.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] clk: add more managed APIs
  2017-01-28 19:03 ` Russell King - ARM Linux
@ 2017-01-28 19:22   ` Dmitry Torokhov
  2017-01-28 21:44     ` Guenter Roeck
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-28 19:22 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Michael Turquette, Stephen Boyd, Viresh Kumar, Guenter Roeck,
	Andy Shevchenko, linux-clk, linux-kernel

On Sat, Jan 28, 2017 at 07:03:10PM +0000, Russell King - ARM Linux wrote:
> On Sat, Jan 28, 2017 at 10:40:47AM -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
> > clocks in the same way we manage many other resources.
> > 
> > This adds the following managed APIs:
> > 
> > - devm_clk_prepare()/devm_clk_unprepare();
> > - devm_clk_enable()/devm_clk_disable();
> > - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
> 
> Does it make any sense what so ever to have devm_clk_enable() and
> devm_clk_disable()?
> 
> Take a moment to think about where you use all of these.  The devm_*
> functions are there to be used in probe functions so that cleanup
> paths can be streamlined and less erroneous.  They aren't for general
> use throughout the driver.
> 
> Given that, there are two operations that you may wish to do in the
> probe path:
> 
> 1. prepare a clock (avoiding the enable because you want to perform
>    the enable elsewhere in the driver.)
> 2. prepare and enable a clock
> 
> So, does having devm_clk_enable() really make sense?  I don't think
> it does, and I suspect they'll get very little if any use.  So, I
> think best not add them until someone comes up with a good and
> wide-spread use case.

That makes sense.

Guenter, I know you are a coccinelle wizard, can you cook a script that
can find current users of clk_enable() in probe paths? Then we can make
informed decision on devm_clk_enable.

Thanks!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] clk: add more managed APIs
  2017-01-28 19:22   ` Dmitry Torokhov
@ 2017-01-28 21:44     ` Guenter Roeck
  2017-01-28 23:39       ` Russell King - ARM Linux
  0 siblings, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2017-01-28 21:44 UTC (permalink / raw)
  To: Dmitry Torokhov, Russell King - ARM Linux
  Cc: Michael Turquette, Stephen Boyd, Viresh Kumar, Andy Shevchenko,
	linux-clk, linux-kernel

On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
> On Sat, Jan 28, 2017 at 07:03:10PM +0000, Russell King - ARM Linux wrote:
>> On Sat, Jan 28, 2017 at 10:40:47AM -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
>>> clocks in the same way we manage many other resources.
>>>
>>> This adds the following managed APIs:
>>>
>>> - devm_clk_prepare()/devm_clk_unprepare();
>>> - devm_clk_enable()/devm_clk_disable();
>>> - devm_clk_prepare_enable()/devm_clk_disable_unprepare().
>>
>> Does it make any sense what so ever to have devm_clk_enable() and
>> devm_clk_disable()?
>>
>> Take a moment to think about where you use all of these.  The devm_*
>> functions are there to be used in probe functions so that cleanup
>> paths can be streamlined and less erroneous.  They aren't for general
>> use throughout the driver.
>>
>> Given that, there are two operations that you may wish to do in the
>> probe path:
>>
>> 1. prepare a clock (avoiding the enable because you want to perform
>>    the enable elsewhere in the driver.)
>> 2. prepare and enable a clock
>>
>> So, does having devm_clk_enable() really make sense?  I don't think
>> it does, and I suspect they'll get very little if any use.  So, I
>> think best not add them until someone comes up with a good and
>> wide-spread use case.
>
> That makes sense.
>
> Guenter, I know you are a coccinelle wizard, can you cook a script that
> can find current users of clk_enable() in probe paths? Then we can make
> informed decision on devm_clk_enable.
>

Questionable use:
	drivers/input/keyboard/st-keyscan.c

clk_enable() in probe, clk_disable() in remove:
	drivers/i2c/busses/i2c-tegra.c
	drivers/media/platform/exynos4-is/fimc-core.c
	drivers/media/platform/exynos4-is/mipi-csis.c
	drivers/thermal/spear_thermal.c
	drivers/usb/musb/am35x.c
	drivers/usb/musb/davinci.c

This does not count drivers which call clk_enable() in probe, but disable
the clock at some point, and only re-enable it when needed.
	
Not that many. A quick browse suggests that clk_enable()/clk_disable()
is more commonly used to temporarily enable the clock while needed.

For clk_prepare(), I get 33 hits in drivers/.

patching file drivers/usb/gadget/udc/at91_udc.c
patching file drivers/i2c/busses/i2c-tegra.c
patching file drivers/pwm/pwm-spear.c
patching file drivers/pinctrl/spear/pinctrl-plgpio.c
patching file drivers/input/keyboard/samsung-keypad.c
patching file drivers/crypto/atmel-aes.c
patching file drivers/usb/gadget/udc/pxa27x_udc.c
patching file drivers/iommu/msm_iommu.c
patching file drivers/i2c/busses/i2c-rk3x.c
patching file drivers/tty/serial/pxa.c
patching file drivers/remoteproc/st_remoteproc.c
patching file drivers/pwm/pwm-tiehrpwm.c
patching file drivers/media/platform/mtk-vpu/mtk_vpu.c
patching file drivers/i2c/busses/i2c-meson.c
patching file drivers/crypto/ux500/hash/hash_core.c
patching file drivers/pwm/pwm-vt8500.c
patching file drivers/pwm/pwm-sti.c
patching file drivers/tty/serial/xilinx_uartps.c
patching file drivers/pwm/pwm-atmel.c
patching file drivers/phy/phy-dm816x-usb.c
patching file drivers/input/keyboard/spear-keyboard.c
patching file drivers/gpu/host1x/mipi.c
patching file drivers/crypto/ux500/cryp/cryp_core.c
patching file drivers/spi/spi-armada-3700.c
patching file drivers/pwm/pwm-mtk-disp.c
patching file drivers/nvmem/mxs-ocotp.c
patching file drivers/media/platform/s5p-g2d/g2d.c
patching file drivers/i2c/busses/i2c-sirf.c
patching file drivers/crypto/atmel-sha.c
patching file drivers/tty/serial/8250/8250_pxa.c
patching file drivers/thermal/samsung/exynos_tmu.c
patching file drivers/media/platform/sti/bdisp/bdisp-v4l2.c
patching file drivers/dma/s3c24xx-dma.c

Those drivers call clk_prepare() in the probe function. The list
may not be complete; my script currently only checks for clk_prepare()
in the probe function of platform, i2c, and spi drivers.

I quick glance through the generated diffs suggests that most
if not all of those call clk_prepare() in probe and clk_unprepare()
in remove.

For clk_prepare_enable() in probe functions, I get 288 hits in drivers/.
I didn't check those for validity - there are just too many. I did check
watchdog and input earlier, though. For those, almost all would be
candidates for devm_clk_prepare_enable().

Overall, I think we should have devm_clk_prepare() and most definitely
devm_clk_prepare_enable(). I am not that sure about clk_enable().

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] clk: add more managed APIs
  2017-01-28 21:44     ` Guenter Roeck
@ 2017-01-28 23:39       ` Russell King - ARM Linux
  2017-01-29 16:00         ` Guenter Roeck
  2017-01-29 18:07         ` [PATCH v2] " Dmitry Torokhov
  0 siblings, 2 replies; 25+ messages in thread
From: Russell King - ARM Linux @ 2017-01-28 23:39 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Dmitry Torokhov, Michael Turquette, Stephen Boyd, Viresh Kumar,
	Andy Shevchenko, linux-clk, linux-kernel

On Sat, Jan 28, 2017 at 01:44:35PM -0800, Guenter Roeck wrote:
> On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
> >Guenter, I know you are a coccinelle wizard, can you cook a script that
> >can find current users of clk_enable() in probe paths? Then we can make
> >informed decision on devm_clk_enable.
> >
> 
> Questionable use:
> 	drivers/input/keyboard/st-keyscan.c

clk_enable() without preceding clk_prepare() - buggy.

> 
> clk_enable() in probe, clk_disable() in remove:
> 	drivers/i2c/busses/i2c-tegra.c

Could be converted to use clk_prepare_enable() or clk_prepare()
depending on i2c_dev->is_multimaster_mode in the initialisation
path (and their devm_* equivalents.)

> 	drivers/media/platform/exynos4-is/fimc-core.c
> 	drivers/media/platform/exynos4-is/mipi-csis.c

Looks like it does a sequence of:

	devm_clk_get()
	clk_prepare()
	clk_set_rate()
	clk_enable()

which seems a little wrong - clk_set_rate() should be before
clk_prepare() if you care about not having the clock output.  Remember
that clk_prepare() _may_ result in the clock output being enabled.  So
these two look buggy, and when fixed could use devm_clk_prepare_enable().

> 	drivers/thermal/spear_thermal.c

clk_enable() without a preceding clk_prepare().  Buggy driver.

> 	drivers/usb/musb/am35x.c

Ditto.

> 	drivers/usb/musb/davinci.c

Ditto.

> Not that many. A quick browse suggests that clk_enable()/clk_disable()
> is more commonly used to temporarily enable the clock while needed.

So out of all those, there's probably only _one_ which is legit - the
rest are all technically buggy.

Given that, I'd say there's real reason _not_ to provide devm_clk_enable()
to persuade people to use the correct interfaces in the probe path.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] clk: add more managed APIs
  2017-01-28 23:39       ` Russell King - ARM Linux
@ 2017-01-29 16:00         ` Guenter Roeck
  2017-01-29 18:07         ` [PATCH v2] " Dmitry Torokhov
  1 sibling, 0 replies; 25+ messages in thread
From: Guenter Roeck @ 2017-01-29 16:00 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Dmitry Torokhov, Michael Turquette, Stephen Boyd, Viresh Kumar,
	Andy Shevchenko, linux-clk, linux-kernel

On 01/28/2017 03:39 PM, Russell King - ARM Linux wrote:
> On Sat, Jan 28, 2017 at 01:44:35PM -0800, Guenter Roeck wrote:
>> On 01/28/2017 11:22 AM, Dmitry Torokhov wrote:
>>> Guenter, I know you are a coccinelle wizard, can you cook a script that
>>> can find current users of clk_enable() in probe paths? Then we can make
>>> informed decision on devm_clk_enable.
>>>
>>
>> Questionable use:
>> 	drivers/input/keyboard/st-keyscan.c
>
> clk_enable() without preceding clk_prepare() - buggy.
>
>>
>> clk_enable() in probe, clk_disable() in remove:
>> 	drivers/i2c/busses/i2c-tegra.c
>
> Could be converted to use clk_prepare_enable() or clk_prepare()
> depending on i2c_dev->is_multimaster_mode in the initialisation
> path (and their devm_* equivalents.)
>
>> 	drivers/media/platform/exynos4-is/fimc-core.c
>> 	drivers/media/platform/exynos4-is/mipi-csis.c
>
> Looks like it does a sequence of:
>
> 	devm_clk_get()
> 	clk_prepare()
> 	clk_set_rate()
> 	clk_enable()
>
> which seems a little wrong - clk_set_rate() should be before
> clk_prepare() if you care about not having the clock output.  Remember
> that clk_prepare() _may_ result in the clock output being enabled.  So
> these two look buggy, and when fixed could use devm_clk_prepare_enable().
>
>> 	drivers/thermal/spear_thermal.c
>
> clk_enable() without a preceding clk_prepare().  Buggy driver.
>
>> 	drivers/usb/musb/am35x.c
>
> Ditto.
>
>> 	drivers/usb/musb/davinci.c
>
> Ditto.
>
>> Not that many. A quick browse suggests that clk_enable()/clk_disable()
>> is more commonly used to temporarily enable the clock while needed.
>
> So out of all those, there's probably only _one_ which is legit - the
> rest are all technically buggy.
>
> Given that, I'd say there's real reason _not_ to provide devm_clk_enable()
> to persuade people to use the correct interfaces in the probe path.
>

I tend to agree, after looking into its existing use.

Do we have agreement on the other two ? I would like to see those in the
next release so we can start using them.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v2] clk: add more managed APIs
  2017-01-28 23:39       ` Russell King - ARM Linux
  2017-01-29 16:00         ` Guenter Roeck
@ 2017-01-29 18:07         ` Dmitry Torokhov
  2017-01-29 18:31           ` Guenter Roeck
  2017-01-30 18:55           ` Stephen Boyd
  1 sibling, 2 replies; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-29 18:07 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Guenter Roeck, Michael Turquette, Stephen Boyd, Viresh Kumar,
	Andy Shevchenko, linux-clk, linux-kernel

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

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;

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-29 18:07         ` [PATCH v2] " Dmitry Torokhov
@ 2017-01-29 18:31           ` Guenter Roeck
  2017-01-30 18:55           ` Stephen Boyd
  1 sibling, 0 replies; 25+ messages in thread
From: Guenter Roeck @ 2017-01-29 18:31 UTC (permalink / raw)
  To: Dmitry Torokhov, Russell King - ARM Linux
  Cc: Michael Turquette, Stephen Boyd, Viresh Kumar, Andy Shevchenko,
	linux-clk, linux-kernel

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;
>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-29 18:07         ` [PATCH v2] " Dmitry Torokhov
  2017-01-29 18:31           ` Guenter Roeck
@ 2017-01-30 18:55           ` Stephen Boyd
  2017-01-30 19:22             ` Guenter Roeck
  2017-01-31  0:57             ` [PATCH v3] " Dmitry Torokhov
  1 sibling, 2 replies; 25+ messages in thread
From: Stephen Boyd @ 2017-01-30 18:55 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Russell King - ARM Linux, Guenter Roeck, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 18:55           ` Stephen Boyd
@ 2017-01-30 19:22             ` Guenter Roeck
  2017-01-30 21:42               ` Russell King - ARM Linux
  2017-01-31  0:59               ` Dmitry Torokhov
  2017-01-31  0:57             ` [PATCH v3] " Dmitry Torokhov
  1 sibling, 2 replies; 25+ messages in thread
From: Guenter Roeck @ 2017-01-30 19:22 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dmitry Torokhov, Russell King - ARM Linux, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 19:22             ` Guenter Roeck
@ 2017-01-30 21:42               ` Russell King - ARM Linux
  2017-01-30 21:58                 ` Dmitry Torokhov
  2017-01-30 22:51                 ` Guenter Roeck
  2017-01-31  0:59               ` Dmitry Torokhov
  1 sibling, 2 replies; 25+ messages in thread
From: Russell King - ARM Linux @ 2017-01-30 21:42 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Stephen Boyd, Dmitry Torokhov, Michael Turquette, Viresh Kumar,
	Andy Shevchenko, linux-clk, linux-kernel

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 21:42               ` Russell King - ARM Linux
@ 2017-01-30 21:58                 ` Dmitry Torokhov
  2017-01-30 22:25                   ` Russell King - ARM Linux
  2017-01-30 22:51                 ` Guenter Roeck
  1 sibling, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-30 21:58 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Guenter Roeck, Stephen Boyd, Michael Turquette, Viresh Kumar,
	Andy Shevchenko, linux-clk, lkml

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 21:58                 ` Dmitry Torokhov
@ 2017-01-30 22:25                   ` Russell King - ARM Linux
  0 siblings, 0 replies; 25+ messages in thread
From: Russell King - ARM Linux @ 2017-01-30 22:25 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Guenter Roeck, Stephen Boyd, Michael Turquette, Viresh Kumar,
	Andy Shevchenko, linux-clk, lkml

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 21:42               ` Russell King - ARM Linux
  2017-01-30 21:58                 ` Dmitry Torokhov
@ 2017-01-30 22:51                 ` Guenter Roeck
  2017-01-31  8:43                   ` Geert Uytterhoeven
  1 sibling, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2017-01-30 22:51 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Stephen Boyd, Dmitry Torokhov, Michael Turquette, Viresh Kumar,
	Andy Shevchenko, linux-clk, linux-kernel

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v3] clk: add more managed APIs
  2017-01-30 18:55           ` Stephen Boyd
  2017-01-30 19:22             ` Guenter Roeck
@ 2017-01-31  0:57             ` Dmitry Torokhov
  2017-02-07  3:51               ` Dmitry Torokhov
  1 sibling, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-31  0:57 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Russell King - ARM Linux, Guenter Roeck, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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

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;

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 19:22             ` Guenter Roeck
  2017-01-30 21:42               ` Russell King - ARM Linux
@ 2017-01-31  0:59               ` Dmitry Torokhov
  2017-01-31 17:20                 ` Guenter Roeck
  1 sibling, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-31  0:59 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Stephen Boyd, Russell King - ARM Linux, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-30 22:51                 ` Guenter Roeck
@ 2017-01-31  8:43                   ` Geert Uytterhoeven
  0 siblings, 0 replies; 25+ messages in thread
From: Geert Uytterhoeven @ 2017-01-31  8:43 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Russell King - ARM Linux, Stephen Boyd, Dmitry Torokhov,
	Michael Turquette, Viresh Kumar, Andy Shevchenko, linux-clk,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-31  0:59               ` Dmitry Torokhov
@ 2017-01-31 17:20                 ` Guenter Roeck
  2017-01-31 18:26                   ` Dmitry Torokhov
  0 siblings, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2017-01-31 17:20 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Stephen Boyd, Russell King - ARM Linux, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-31 17:20                 ` Guenter Roeck
@ 2017-01-31 18:26                   ` Dmitry Torokhov
  2017-01-31 19:34                     ` Guenter Roeck
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-01-31 18:26 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Stephen Boyd, Russell King - ARM Linux, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, lkml

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2] clk: add more managed APIs
  2017-01-31 18:26                   ` Dmitry Torokhov
@ 2017-01-31 19:34                     ` Guenter Roeck
  0 siblings, 0 replies; 25+ messages in thread
From: Guenter Roeck @ 2017-01-31 19:34 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Stephen Boyd, Russell King - ARM Linux, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, lkml

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] clk: add more managed APIs
  2017-01-31  0:57             ` [PATCH v3] " Dmitry Torokhov
@ 2017-02-07  3:51               ` Dmitry Torokhov
  2017-02-14 19:44                 ` Stephen Boyd
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-02-07  3:51 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Russell King - ARM Linux, Guenter Roeck, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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;

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] clk: add more managed APIs
  2017-02-07  3:51               ` Dmitry Torokhov
@ 2017-02-14 19:44                 ` Stephen Boyd
  2017-02-14 19:55                   ` Dmitry Torokhov
  2017-02-14 20:01                   ` Russell King - ARM Linux
  0 siblings, 2 replies; 25+ messages in thread
From: Stephen Boyd @ 2017-02-14 19:44 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Russell King - ARM Linux, Guenter Roeck, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, linux-kernel

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] clk: add more managed APIs
  2017-02-14 19:44                 ` Stephen Boyd
@ 2017-02-14 19:55                   ` Dmitry Torokhov
  2017-02-14 20:31                     ` Guenter Roeck
  2017-02-14 20:01                   ` Russell King - ARM Linux
  1 sibling, 1 reply; 25+ messages in thread
From: Dmitry Torokhov @ 2017-02-14 19:55 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Russell King - ARM Linux, Guenter Roeck, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, lkml

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] clk: add more managed APIs
  2017-02-14 19:44                 ` Stephen Boyd
  2017-02-14 19:55                   ` Dmitry Torokhov
@ 2017-02-14 20:01                   ` Russell King - ARM Linux
  1 sibling, 0 replies; 25+ messages in thread
From: Russell King - ARM Linux @ 2017-02-14 20:01 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dmitry Torokhov, Guenter Roeck, Michael Turquette, Viresh Kumar,
	Andy Shevchenko, linux-clk, linux-kernel

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

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v3] clk: add more managed APIs
  2017-02-14 19:55                   ` Dmitry Torokhov
@ 2017-02-14 20:31                     ` Guenter Roeck
  0 siblings, 0 replies; 25+ messages in thread
From: Guenter Roeck @ 2017-02-14 20:31 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Stephen Boyd, Russell King - ARM Linux, Michael Turquette,
	Viresh Kumar, Andy Shevchenko, linux-clk, lkml

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

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2017-02-14 20:31 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-28 18:40 [PATCH] clk: add more managed APIs Dmitry Torokhov
2017-01-28 19:03 ` Russell King - ARM Linux
2017-01-28 19:22   ` Dmitry Torokhov
2017-01-28 21:44     ` Guenter Roeck
2017-01-28 23:39       ` Russell King - ARM Linux
2017-01-29 16:00         ` Guenter Roeck
2017-01-29 18:07         ` [PATCH v2] " Dmitry Torokhov
2017-01-29 18:31           ` Guenter Roeck
2017-01-30 18:55           ` Stephen Boyd
2017-01-30 19:22             ` Guenter Roeck
2017-01-30 21:42               ` Russell King - ARM Linux
2017-01-30 21:58                 ` Dmitry Torokhov
2017-01-30 22:25                   ` Russell King - ARM Linux
2017-01-30 22:51                 ` Guenter Roeck
2017-01-31  8:43                   ` Geert Uytterhoeven
2017-01-31  0:59               ` Dmitry Torokhov
2017-01-31 17:20                 ` Guenter Roeck
2017-01-31 18:26                   ` Dmitry Torokhov
2017-01-31 19:34                     ` Guenter Roeck
2017-01-31  0:57             ` [PATCH v3] " Dmitry Torokhov
2017-02-07  3:51               ` Dmitry Torokhov
2017-02-14 19:44                 ` Stephen Boyd
2017-02-14 19:55                   ` Dmitry Torokhov
2017-02-14 20:31                     ` Guenter Roeck
2017-02-14 20:01                   ` Russell King - ARM Linux

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).