| Message ID | 1d7a1b3b-e9bf-1d80-609d-a9c0c932b15a@free.fr (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1] clk: Add devm_clk_{prepare,enable,prepare_enable} | expand |
On Mon 15 Jul 08:34 PDT 2019, Marc Gonzalez wrote: [..] > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index c0990703ce54..5e85548357c0 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_prepare); > > +static void unprepare(void *clk) This deserves a less generic name. > +{ > + clk_unprepare(clk); > +} > + > +int devm_clk_prepare(struct device *dev, struct clk *clk) > +{ > + int rc = clk_prepare(clk); > + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); > +} > +EXPORT_SYMBOL_GPL(devm_clk_prepare); > + > static void clk_core_disable(struct clk_core *core) > { > lockdep_assert_held(&enable_lock); > @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_enable); > > +static void disable(void *clk) > +{ > + clk_disable(clk); > +} > + > +int devm_clk_enable(struct device *dev, struct clk *clk) clk_enable() is used in code that can't sleep, in what scenario do you envision it being useful to enable a clock from such region until devres cleans up the associated device? > +{ > + int rc = clk_enable(clk); > + return rc ? : devm_add_action_or_reset(dev, disable, clk); devm_add_action_or_reset() allocates the devres object with GFP_KERNEL, so this won't work. > +} > +EXPORT_SYMBOL_GPL(devm_clk_enable); > + > static int clk_core_prepare_enable(struct clk_core *core) > { > int ret; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 3c096c7a51dc..d09b5207e3f1 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} > > #endif > > +int devm_clk_prepare(struct device *dev, struct clk *clk); > +int devm_clk_enable(struct device *dev, struct clk *clk); > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) devm_clk_prepare_enable() sounds very useful, devm_clk_prepare() might be useful, so keep those and drop devm_clk_enable(). Regards, Bjorn > +{ > + int rc = devm_clk_prepare(dev, clk); > + return rc ? : devm_clk_enable(dev, clk); > +} > + > /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ > static inline int clk_prepare_enable(struct clk *clk) > { > -- > 2.17.1
On 7/15/19 8:34 AM, Marc Gonzalez wrote: > Provide devm variants for automatic resource release on device removal. > probe() error-handling is simpler, and remove is no longer required. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> Again ? https://lore.kernel.org/patchwork/patch/755667/ This must be at least the third time this is tried. I don't think anything changed since the previous submissions. I long since gave up and use devm_add_action_or_reset() in affected drivers instead. Guenter > --- > Documentation/driver-model/devres.rst | 3 +++ > drivers/clk/clk.c | 24 ++++++++++++++++++++++++ > include/linux/clk.h | 8 ++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst > index 1b6ced8e4294..9357260576ef 100644 > --- a/Documentation/driver-model/devres.rst > +++ b/Documentation/driver-model/devres.rst > @@ -253,6 +253,9 @@ CLOCK > devm_clk_hw_register() > devm_of_clk_add_hw_provider() > devm_clk_hw_register_clkdev() > + devm_clk_prepare() > + devm_clk_enable() > + devm_clk_prepare_enable() > > DMA > dmaenginem_async_device_register() > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index c0990703ce54..5e85548357c0 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_prepare); > > +static void unprepare(void *clk) > +{ > + clk_unprepare(clk); > +} > + > +int devm_clk_prepare(struct device *dev, struct clk *clk) > +{ > + int rc = clk_prepare(clk); > + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); > +} > +EXPORT_SYMBOL_GPL(devm_clk_prepare); > + > static void clk_core_disable(struct clk_core *core) > { > lockdep_assert_held(&enable_lock); > @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_enable); > > +static void disable(void *clk) > +{ > + clk_disable(clk); > +} > + > +int devm_clk_enable(struct device *dev, struct clk *clk) > +{ > + int rc = clk_enable(clk); > + return rc ? : devm_add_action_or_reset(dev, disable, clk); > +} > +EXPORT_SYMBOL_GPL(devm_clk_enable); > + > static int clk_core_prepare_enable(struct clk_core *core) > { > int ret; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 3c096c7a51dc..d09b5207e3f1 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} > > #endif > > +int devm_clk_prepare(struct device *dev, struct clk *clk); > +int devm_clk_enable(struct device *dev, struct clk *clk); > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > +{ > + int rc = devm_clk_prepare(dev, clk); > + return rc ? : devm_clk_enable(dev, clk); > +} > + > /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ > static inline int clk_prepare_enable(struct clk *clk) > { >
On 16/07/2019 02:25, Guenter Roeck wrote: > On 7/15/19 8:34 AM, Marc Gonzalez wrote: > >> Provide devm variants for automatic resource release on device removal. >> probe() error-handling is simpler, and remove is no longer required. >> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > > Again ? > > https://lore.kernel.org/patchwork/patch/755667/ IMHO, my proposal is very simple and (somewhat) easier to review. I'm sure it will enthrall Stephen/Mike, thus they'll merge it in a heart-beat ^_^ > This must be at least the third time this is tried. I don't think anything changed > since the previous submissions. I long since gave up and use devm_add_action_or_reset() > in affected drivers instead. "Tonight's the night we're gonna make it happen Tonight we'll put all other things aside" ^_^ It's silly to have driver authors worry about probe() error-handling in 2020. (What? It's not 2020 yet?!) If I could get two or three reviews, it would help to show support for this essential, yet still missing, API. Regards.
On 15/07/2019 17:34, Marc Gonzalez wrote: > Provide devm variants for automatic resource release on device removal. > probe() error-handling is simpler, and remove is no longer required. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > Documentation/driver-model/devres.rst | 3 +++ > drivers/clk/clk.c | 24 ++++++++++++++++++++++++ > include/linux/clk.h | 8 ++++++++ > 3 files changed, 35 insertions(+) Stephen, Mike, Thoughts? Comments? Regards.
On 15/07/2019 17:34, Marc Gonzalez wrote: > Provide devm variants for automatic resource release on device removal. > probe() error-handling is simpler, and remove is no longer required. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > Documentation/driver-model/devres.rst | 3 +++ > drivers/clk/clk.c | 24 ++++++++++++++++++++++++ > include/linux/clk.h | 8 ++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst > index 1b6ced8e4294..9357260576ef 100644 > --- a/Documentation/driver-model/devres.rst > +++ b/Documentation/driver-model/devres.rst > @@ -253,6 +253,9 @@ CLOCK > devm_clk_hw_register() > devm_of_clk_add_hw_provider() > devm_clk_hw_register_clkdev() > + devm_clk_prepare() > + devm_clk_enable() > + devm_clk_prepare_enable() > > DMA > dmaenginem_async_device_register() > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index c0990703ce54..5e85548357c0 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_prepare); > > +static void unprepare(void *clk) > +{ > + clk_unprepare(clk); > +} > + > +int devm_clk_prepare(struct device *dev, struct clk *clk) > +{ > + int rc = clk_prepare(clk); > + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); > +} > +EXPORT_SYMBOL_GPL(devm_clk_prepare); > + > static void clk_core_disable(struct clk_core *core) > { > lockdep_assert_held(&enable_lock); > @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_enable); > > +static void disable(void *clk) > +{ > + clk_disable(clk); > +} > + > +int devm_clk_enable(struct device *dev, struct clk *clk) > +{ > + int rc = clk_enable(clk); > + return rc ? : devm_add_action_or_reset(dev, disable, clk); > +} > +EXPORT_SYMBOL_GPL(devm_clk_enable); > + > static int clk_core_prepare_enable(struct clk_core *core) > { > int ret; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 3c096c7a51dc..d09b5207e3f1 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} > > #endif > > +int devm_clk_prepare(struct device *dev, struct clk *clk); > +int devm_clk_enable(struct device *dev, struct clk *clk); > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > +{ > + int rc = devm_clk_prepare(dev, clk); > + return rc ? : devm_clk_enable(dev, clk); > +} > + > /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ > static inline int clk_prepare_enable(struct clk *clk) > { Thoughts? Comments? Regards.
On 25/11/2019 13:46, Marc Gonzalez wrote:
> Thoughts? Comments?
Bjorn's comment never made it to my Inbox.
https://lore.kernel.org/patchwork/patch/1100771/
On Mon, Nov 25, 2019 at 01:46:51PM +0100, Marc Gonzalez wrote: > On 15/07/2019 17:34, Marc Gonzalez wrote: > > > Provide devm variants for automatic resource release on device removal. > > probe() error-handling is simpler, and remove is no longer required. > > > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > > --- > > Documentation/driver-model/devres.rst | 3 +++ > > drivers/clk/clk.c | 24 ++++++++++++++++++++++++ > > include/linux/clk.h | 8 ++++++++ > > 3 files changed, 35 insertions(+) > > > > diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst > > index 1b6ced8e4294..9357260576ef 100644 > > --- a/Documentation/driver-model/devres.rst > > +++ b/Documentation/driver-model/devres.rst > > @@ -253,6 +253,9 @@ CLOCK > > devm_clk_hw_register() > > devm_of_clk_add_hw_provider() > > devm_clk_hw_register_clkdev() > > + devm_clk_prepare() > > + devm_clk_enable() > > + devm_clk_prepare_enable() > > > > DMA > > dmaenginem_async_device_register() > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index c0990703ce54..5e85548357c0 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) > > } > > EXPORT_SYMBOL_GPL(clk_prepare); > > > > +static void unprepare(void *clk) > > +{ > > + clk_unprepare(clk); > > +} > > + > > +int devm_clk_prepare(struct device *dev, struct clk *clk) > > +{ > > + int rc = clk_prepare(clk); > > + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); > > +} > > +EXPORT_SYMBOL_GPL(devm_clk_prepare); > > + > > static void clk_core_disable(struct clk_core *core) > > { > > lockdep_assert_held(&enable_lock); > > @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) > > } > > EXPORT_SYMBOL_GPL(clk_enable); > > > > +static void disable(void *clk) > > +{ > > + clk_disable(clk); > > +} > > + > > +int devm_clk_enable(struct device *dev, struct clk *clk) > > +{ > > + int rc = clk_enable(clk); > > + return rc ? : devm_add_action_or_reset(dev, disable, clk); > > +} > > +EXPORT_SYMBOL_GPL(devm_clk_enable); > > + > > static int clk_core_prepare_enable(struct clk_core *core) > > { > > int ret; > > diff --git a/include/linux/clk.h b/include/linux/clk.h > > index 3c096c7a51dc..d09b5207e3f1 100644 > > --- a/include/linux/clk.h > > +++ b/include/linux/clk.h > > @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} > > > > #endif > > > > +int devm_clk_prepare(struct device *dev, struct clk *clk); > > +int devm_clk_enable(struct device *dev, struct clk *clk); > > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > > +{ > > + int rc = devm_clk_prepare(dev, clk); > > + return rc ? : devm_clk_enable(dev, clk); > > +} > > + > > /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ > > static inline int clk_prepare_enable(struct clk *clk) > > { > > Thoughts? Comments? These are part of the clk API rather than the CCF API, and belong in drivers/clk/clk-devres.c.
On Mon, Nov 25, 2019 at 01:46:51PM +0100, Marc Gonzalez wrote: > On 15/07/2019 17:34, Marc Gonzalez wrote: > > > Provide devm variants for automatic resource release on device removal. > > probe() error-handling is simpler, and remove is no longer required. > > > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > > --- > > Documentation/driver-model/devres.rst | 3 +++ > > drivers/clk/clk.c | 24 ++++++++++++++++++++++++ > > include/linux/clk.h | 8 ++++++++ > > 3 files changed, 35 insertions(+) > > > > diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst > > index 1b6ced8e4294..9357260576ef 100644 > > --- a/Documentation/driver-model/devres.rst > > +++ b/Documentation/driver-model/devres.rst > > @@ -253,6 +253,9 @@ CLOCK > > devm_clk_hw_register() > > devm_of_clk_add_hw_provider() > > devm_clk_hw_register_clkdev() > > + devm_clk_prepare() > > + devm_clk_enable() > > + devm_clk_prepare_enable() > > > > DMA > > dmaenginem_async_device_register() > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index c0990703ce54..5e85548357c0 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) > > } > > EXPORT_SYMBOL_GPL(clk_prepare); > > > > +static void unprepare(void *clk) > > +{ > > + clk_unprepare(clk); > > +} > > + > > +int devm_clk_prepare(struct device *dev, struct clk *clk) > > +{ > > + int rc = clk_prepare(clk); > > + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); > > +} > > +EXPORT_SYMBOL_GPL(devm_clk_prepare); > > + > > static void clk_core_disable(struct clk_core *core) > > { > > lockdep_assert_held(&enable_lock); > > @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) > > } > > EXPORT_SYMBOL_GPL(clk_enable); > > > > +static void disable(void *clk) > > +{ > > + clk_disable(clk); > > +} > > + > > +int devm_clk_enable(struct device *dev, struct clk *clk) > > +{ > > + int rc = clk_enable(clk); > > + return rc ? : devm_add_action_or_reset(dev, disable, clk); > > +} > > +EXPORT_SYMBOL_GPL(devm_clk_enable); > > + > > static int clk_core_prepare_enable(struct clk_core *core) > > { > > int ret; > > diff --git a/include/linux/clk.h b/include/linux/clk.h > > index 3c096c7a51dc..d09b5207e3f1 100644 > > --- a/include/linux/clk.h > > +++ b/include/linux/clk.h > > @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} > > > > #endif > > > > +int devm_clk_prepare(struct device *dev, struct clk *clk); > > +int devm_clk_enable(struct device *dev, struct clk *clk); > > +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > > +{ > > + int rc = devm_clk_prepare(dev, clk); > > + return rc ? : devm_clk_enable(dev, clk); > > +} > > + > > /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ > > static inline int clk_prepare_enable(struct clk *clk) > > { > > Thoughts? Comments? It's also worth reading https://lore.kernel.org/patchwork/patch/755667/ and considering whether you really are using the clk_prepare() and clk_enable() APIs correctly. Wanting these devm functions suggests you aren't...
On 25/11/2019 13:55, Russell King - ARM Linux admin wrote: > It's also worth reading https://lore.kernel.org/patchwork/patch/755667/ > and considering whether you really are using the clk_prepare() and > clk_enable() APIs correctly. Wanting these devm functions suggests > you aren't... In that older thread, you wrote: > 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 run-time 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 nuisance. In the few drivers I've written, I call clk_prepare_enable() at probe. And since clk_prepare_enable() is the only non-devm function in probe, I need either a remove function, or an explicit registration step. You seem to be saying I'm using the clk API in the wrong way? Regards.
On 25/11/2019 13:52, Russell King - ARM Linux admin wrote: > On Mon, Nov 25, 2019 at 01:46:51PM +0100, Marc Gonzalez wrote: > >> On 15/07/2019 17:34, Marc Gonzalez wrote: >> >>> Provide devm variants for automatic resource release on device removal. >>> probe() error-handling is simpler, and remove is no longer required. >>> >>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> >>> --- >>> Documentation/driver-model/devres.rst | 3 +++ >>> drivers/clk/clk.c | 24 ++++++++++++++++++++++++ >>> include/linux/clk.h | 8 ++++++++ >>> 3 files changed, 35 insertions(+) >>> >>> diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst >>> index 1b6ced8e4294..9357260576ef 100644 >>> --- a/Documentation/driver-model/devres.rst >>> +++ b/Documentation/driver-model/devres.rst >>> @@ -253,6 +253,9 @@ CLOCK >>> devm_clk_hw_register() >>> devm_of_clk_add_hw_provider() >>> devm_clk_hw_register_clkdev() >>> + devm_clk_prepare() >>> + devm_clk_enable() >>> + devm_clk_prepare_enable() >>> >>> DMA >>> dmaenginem_async_device_register() >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>> index c0990703ce54..5e85548357c0 100644 >>> --- a/drivers/clk/clk.c >>> +++ b/drivers/clk/clk.c >>> @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) >>> } >>> EXPORT_SYMBOL_GPL(clk_prepare); >>> >>> +static void unprepare(void *clk) >>> +{ >>> + clk_unprepare(clk); >>> +} >>> + >>> +int devm_clk_prepare(struct device *dev, struct clk *clk) >>> +{ >>> + int rc = clk_prepare(clk); >>> + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); >>> +} >>> +EXPORT_SYMBOL_GPL(devm_clk_prepare); >>> + >>> static void clk_core_disable(struct clk_core *core) >>> { >>> lockdep_assert_held(&enable_lock); >>> @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) >>> } >>> EXPORT_SYMBOL_GPL(clk_enable); >>> >>> +static void disable(void *clk) >>> +{ >>> + clk_disable(clk); >>> +} >>> + >>> +int devm_clk_enable(struct device *dev, struct clk *clk) >>> +{ >>> + int rc = clk_enable(clk); >>> + return rc ? : devm_add_action_or_reset(dev, disable, clk); >>> +} >>> +EXPORT_SYMBOL_GPL(devm_clk_enable); >>> + >>> static int clk_core_prepare_enable(struct clk_core *core) >>> { >>> int ret; >>> diff --git a/include/linux/clk.h b/include/linux/clk.h >>> index 3c096c7a51dc..d09b5207e3f1 100644 >>> --- a/include/linux/clk.h >>> +++ b/include/linux/clk.h >>> @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} >>> >>> #endif >>> >>> +int devm_clk_prepare(struct device *dev, struct clk *clk); >>> +int devm_clk_enable(struct device *dev, struct clk *clk); >>> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) >>> +{ >>> + int rc = devm_clk_prepare(dev, clk); >>> + return rc ? : devm_clk_enable(dev, clk); >>> +} >>> + >>> /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ >>> static inline int clk_prepare_enable(struct clk *clk) >>> { >> >> Thoughts? Comments? > > These are part of the clk API rather than the CCF API, and belong in > drivers/clk/clk-devres.c. I'm totally confused. Are you saying that a hypothetical devm_clk_prepare() function should not be implemented in the same file as the "raw" clk_prepare() ? Regards.
On Mon, Nov 25, 2019 at 02:16:17PM +0100, Marc Gonzalez wrote: > On 25/11/2019 13:52, Russell King - ARM Linux admin wrote: > > > On Mon, Nov 25, 2019 at 01:46:51PM +0100, Marc Gonzalez wrote: > > > >> On 15/07/2019 17:34, Marc Gonzalez wrote: > >> > >>> Provide devm variants for automatic resource release on device removal. > >>> probe() error-handling is simpler, and remove is no longer required. > >>> > >>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > >>> --- > >>> Documentation/driver-model/devres.rst | 3 +++ > >>> drivers/clk/clk.c | 24 ++++++++++++++++++++++++ > >>> include/linux/clk.h | 8 ++++++++ > >>> 3 files changed, 35 insertions(+) > >>> > >>> diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst > >>> index 1b6ced8e4294..9357260576ef 100644 > >>> --- a/Documentation/driver-model/devres.rst > >>> +++ b/Documentation/driver-model/devres.rst > >>> @@ -253,6 +253,9 @@ CLOCK > >>> devm_clk_hw_register() > >>> devm_of_clk_add_hw_provider() > >>> devm_clk_hw_register_clkdev() > >>> + devm_clk_prepare() > >>> + devm_clk_enable() > >>> + devm_clk_prepare_enable() > >>> > >>> DMA > >>> dmaenginem_async_device_register() > >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > >>> index c0990703ce54..5e85548357c0 100644 > >>> --- a/drivers/clk/clk.c > >>> +++ b/drivers/clk/clk.c > >>> @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) > >>> } > >>> EXPORT_SYMBOL_GPL(clk_prepare); > >>> > >>> +static void unprepare(void *clk) > >>> +{ > >>> + clk_unprepare(clk); > >>> +} > >>> + > >>> +int devm_clk_prepare(struct device *dev, struct clk *clk) > >>> +{ > >>> + int rc = clk_prepare(clk); > >>> + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); > >>> +} > >>> +EXPORT_SYMBOL_GPL(devm_clk_prepare); > >>> + > >>> static void clk_core_disable(struct clk_core *core) > >>> { > >>> lockdep_assert_held(&enable_lock); > >>> @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) > >>> } > >>> EXPORT_SYMBOL_GPL(clk_enable); > >>> > >>> +static void disable(void *clk) > >>> +{ > >>> + clk_disable(clk); > >>> +} > >>> + > >>> +int devm_clk_enable(struct device *dev, struct clk *clk) > >>> +{ > >>> + int rc = clk_enable(clk); > >>> + return rc ? : devm_add_action_or_reset(dev, disable, clk); > >>> +} > >>> +EXPORT_SYMBOL_GPL(devm_clk_enable); > >>> + > >>> static int clk_core_prepare_enable(struct clk_core *core) > >>> { > >>> int ret; > >>> diff --git a/include/linux/clk.h b/include/linux/clk.h > >>> index 3c096c7a51dc..d09b5207e3f1 100644 > >>> --- a/include/linux/clk.h > >>> +++ b/include/linux/clk.h > >>> @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} > >>> > >>> #endif > >>> > >>> +int devm_clk_prepare(struct device *dev, struct clk *clk); > >>> +int devm_clk_enable(struct device *dev, struct clk *clk); > >>> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > >>> +{ > >>> + int rc = devm_clk_prepare(dev, clk); > >>> + return rc ? : devm_clk_enable(dev, clk); > >>> +} > >>> + > >>> /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ > >>> static inline int clk_prepare_enable(struct clk *clk) > >>> { > >> > >> Thoughts? Comments? > > > > These are part of the clk API rather than the CCF API, and belong in > > drivers/clk/clk-devres.c. > > I'm totally confused. > > Are you saying that a hypothetical devm_clk_prepare() function should not be > implemented in the same file as the "raw" clk_prepare() ? The clk API and CCF are two different things. I look after the clk API. The CCF is an implementation of the clk API. Do not introduce clk API code in files that are CCF specific.
On 25/11/2019 14:31, Russell King - ARM Linux admin wrote: > The clk API and CCF are two different things. I look after the clk API. > The CCF is an implementation of the clk API. Do not introduce clk API > code in files that are CCF specific. CCF is the acronym for Common Clock Framework?
On Mon, Nov 25, 2019 at 02:10:21PM +0100, Marc Gonzalez wrote: > On 25/11/2019 13:55, Russell King - ARM Linux admin wrote: > > > It's also worth reading https://lore.kernel.org/patchwork/patch/755667/ > > and considering whether you really are using the clk_prepare() and > > clk_enable() APIs correctly. Wanting these devm functions suggests > > you aren't... > > In that older thread, you wrote: > > > 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 run-time 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 nuisance. > > In the few drivers I've written, I call clk_prepare_enable() at probe. Right, so the clocks are enabled as soon as the device is probed, in other words at boot time. It remains enabled for as long as the device is bound to its driver, whether or not the device is actually being used. Every switching edge causes heat to be generated. Every switching edge causes energy to be wasted. That's fine if you have an infinite energy supply. That hasn't been discovered yet. Given the prevalence of technology, don't you think we should be doing as much as we possibly can to reduce the energy consumption of the devices we use? It may be peanuts per device, but at scale it all adds up.
On Mon, Nov 25, 2019 at 02:34:35PM +0100, Marc Gonzalez wrote: > On 25/11/2019 14:31, Russell King - ARM Linux admin wrote: > > > The clk API and CCF are two different things. I look after the clk API. > > The CCF is an implementation of the clk API. Do not introduce clk API > > code in files that are CCF specific. > > CCF is the acronym for Common Clock Framework? Yes.
Doh! Your reply never made it to my inbox, and I never thought to check the mailing list... On 15/07/2019 23:46, Bjorn Andersson wrote: > On Mon 15 Jul 08:34 PDT 2019, Marc Gonzalez wrote: > > [..] >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index c0990703ce54..5e85548357c0 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) >> } >> EXPORT_SYMBOL_GPL(clk_prepare); >> >> +static void unprepare(void *clk) > > This deserves a less generic name. Fair enough. Though it's only because of C's function pointer idiosyncrasies that a function wrapper is even needed. > clk_enable() is used in code that can't sleep, in what scenario do you > envision it being useful to enable a clock from such region until devres > cleans up the associated device? The use-case I had in mind was "Device drivers that call 1) clk_prepare_enable from probe() 2) clk_disable_unprepare() in remove()" (Russell King has pointed out the short-comings of such an approach in a different sub-thread.) >> +int devm_clk_prepare(struct device *dev, struct clk *clk); >> +int devm_clk_enable(struct device *dev, struct clk *clk); >> +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) > > devm_clk_prepare_enable() sounds very useful, devm_clk_prepare() might > be useful, so keep those and drop devm_clk_enable(). Oooh, I think I understand what you mean... I saw clk_prepare_enable() defined as clk_prepare() + clk_enable(), and figured I'd define devm_clk_prepare_enable() as devm_clk_prepare() + devm_clk_enable() without realizing that devm_clk_enable() made no sense. Solution: drop devm_clk_enable() from include/linux/clk.h Consequence devm_clk_prepare_enable() cannot be static inline, but that may not be a big deal... Regards.
On 25/11/2019 14:37, Russell King - ARM Linux admin wrote: > On Mon, Nov 25, 2019 at 02:10:21PM +0100, Marc Gonzalez wrote: > >> On 25/11/2019 13:55, Russell King - ARM Linux admin wrote: >> >>> It's also worth reading https://lore.kernel.org/patchwork/patch/755667/ >>> and considering whether you really are using the clk_prepare() and >>> clk_enable() APIs correctly. Wanting these devm functions suggests >>> you aren't... >> >> In that older thread, you wrote: >> >>> 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 run-time 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 nuisance. >> >> In the few drivers I've written, I call clk_prepare_enable() at probe. > > Right, so the clocks are enabled as soon as the device is probed, > in other words at boot time. It remains enabled for as long as the > device is bound to its driver, whether or not the device is actually > being used. Every switching edge causes heat to be generated. Every > switching edge causes energy to be wasted. > > That's fine if you have an infinite energy supply. That hasn't been > discovered yet. > > Given the prevalence of technology, don't you think we should be > doing as much as we possibly can to reduce the energy consumption > of the devices we use? It may be peanuts per device, but at scale > it all adds up. OK, I'm starting to see the bigger picture. (To provide some rationale for the patch, I think devm is a huge improvement for probe error-handling, and I did not understand why every driver must do manual error-handling when dealing with clocks in probe.) I did envision kernel modules being loaded on an as-needed basis, somewhat side-stepping the energy-waste issue you point out. But I realize that such a use-case may be uncommon. (Especially due to module auto-loading.) A few months ago, I was discussing a similar issue with GKH: Consider a device with a "START" register. Basically, if we write 0, the device turns itself off; if we write 1, it runs as configured. I was trying to start the device only when at least one user had it "open". So I used reference counting, and started the device on 0->1 open transitions, and stopped the device on 1->0 close transitions. GKH told me that was the wrong way to do it, and IIRC suggested to start the device in probe. I probably misunderstood Greg's suggestion. Where is the right place to start/stop a device (or gate its clocks)? Regards.
Hi Marc, On Mon, Nov 25, 2019 at 3:11 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 25/11/2019 14:37, Russell King - ARM Linux admin wrote: > > On Mon, Nov 25, 2019 at 02:10:21PM +0100, Marc Gonzalez wrote: > >> On 25/11/2019 13:55, Russell King - ARM Linux admin wrote: > >>> It's also worth reading https://lore.kernel.org/patchwork/patch/755667/ > >>> and considering whether you really are using the clk_prepare() and > >>> clk_enable() APIs correctly. Wanting these devm functions suggests > >>> you aren't... > >> > >> In that older thread, you wrote: > >> > >>> 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 run-time 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 nuisance. > >> > >> In the few drivers I've written, I call clk_prepare_enable() at probe. > > > > Right, so the clocks are enabled as soon as the device is probed, > > in other words at boot time. It remains enabled for as long as the > > device is bound to its driver, whether or not the device is actually > > being used. Every switching edge causes heat to be generated. Every > > switching edge causes energy to be wasted. > > > > That's fine if you have an infinite energy supply. That hasn't been > > discovered yet. > > > > Given the prevalence of technology, don't you think we should be > > doing as much as we possibly can to reduce the energy consumption > > of the devices we use? It may be peanuts per device, but at scale > > it all adds up. > > OK, I'm starting to see the bigger picture. > > (To provide some rationale for the patch, I think devm is a huge > improvement for probe error-handling, and I did not understand > why every driver must do manual error-handling when dealing with > clocks in probe.) > > I did envision kernel modules being loaded on an as-needed basis, > somewhat side-stepping the energy-waste issue you point out. > But I realize that such a use-case may be uncommon. (Especially > due to module auto-loading.) > > A few months ago, I was discussing a similar issue with GKH: > Consider a device with a "START" register. Basically, if we write 0, > the device turns itself off; if we write 1, it runs as configured. > > I was trying to start the device only when at least one user had > it "open". So I used reference counting, and started the device > on 0->1 open transitions, and stopped the device on 1->0 close > transitions. GKH told me that was the wrong way to do it, and IIRC > suggested to start the device in probe. > > I probably misunderstood Greg's suggestion. Where is the right place > to start/stop a device (or gate its clocks)? In the device driver's Runtime PM callbacks? In the Power/Clock Domain Controller driver? See drivers/base/power/domain.c:genpd_{start,stop}_dev(), and how/when it's called. Embedded device driver writers typically care. Server device driver writes typically don't. Gr{oetje,eeting}s, Geert
diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst index 1b6ced8e4294..9357260576ef 100644 --- a/Documentation/driver-model/devres.rst +++ b/Documentation/driver-model/devres.rst @@ -253,6 +253,9 @@ CLOCK devm_clk_hw_register() devm_of_clk_add_hw_provider() devm_clk_hw_register_clkdev() + devm_clk_prepare() + devm_clk_enable() + devm_clk_prepare_enable() DMA dmaenginem_async_device_register() diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index c0990703ce54..5e85548357c0 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -914,6 +914,18 @@ int clk_prepare(struct clk *clk) } EXPORT_SYMBOL_GPL(clk_prepare); +static void unprepare(void *clk) +{ + clk_unprepare(clk); +} + +int devm_clk_prepare(struct device *dev, struct clk *clk) +{ + int rc = clk_prepare(clk); + return rc ? : devm_add_action_or_reset(dev, unprepare, clk); +} +EXPORT_SYMBOL_GPL(devm_clk_prepare); + static void clk_core_disable(struct clk_core *core) { lockdep_assert_held(&enable_lock); @@ -1136,6 +1148,18 @@ int clk_enable(struct clk *clk) } EXPORT_SYMBOL_GPL(clk_enable); +static void disable(void *clk) +{ + clk_disable(clk); +} + +int devm_clk_enable(struct device *dev, struct clk *clk) +{ + int rc = clk_enable(clk); + return rc ? : devm_add_action_or_reset(dev, disable, clk); +} +EXPORT_SYMBOL_GPL(devm_clk_enable); + static int clk_core_prepare_enable(struct clk_core *core) { int ret; diff --git a/include/linux/clk.h b/include/linux/clk.h index 3c096c7a51dc..d09b5207e3f1 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -895,6 +895,14 @@ static inline void clk_restore_context(void) {} #endif +int devm_clk_prepare(struct device *dev, struct clk *clk); +int devm_clk_enable(struct device *dev, struct clk *clk); +static inline int devm_clk_prepare_enable(struct device *dev, struct clk *clk) +{ + int rc = devm_clk_prepare(dev, clk); + return rc ? : devm_clk_enable(dev, clk); +} + /* clk_prepare_enable helps cases using clk_enable in non-atomic context. */ static inline int clk_prepare_enable(struct clk *clk) {
Provide devm variants for automatic resource release on device removal. probe() error-handling is simpler, and remove is no longer required. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- Documentation/driver-model/devres.rst | 3 +++ drivers/clk/clk.c | 24 ++++++++++++++++++++++++ include/linux/clk.h | 8 ++++++++ 3 files changed, 35 insertions(+)