Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Russell King - ARM Linux admin]

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Russell King - ARM Linux admin]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Geert Uytterhoeven]

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable}

[Marc Gonzalez]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel