[PULL] Add variants of devm_clk_get for prepared and enabled clocks enabled clocks

Pull-request

https://git.pengutronix.de/git/ukl/linux tags/devm-clk-get-enabled

Message

Uwe Kleine-König June 9, 2021, 8:21 p.m. UTC

Hello Stephen,

given that I don't succeed in getting any feedback for my patch set, I'm
trying with a pull request today. It would be really great if this pull
request made it finally in for the next merge window.

The changes are not as bad or complex as the diffstat suggests. The
first patch contains all the complexity and only has
 1 file changed, 50 insertions(+), 17 deletions(-)
. The second patch makes use of this and just adds kernel-doc, four
functions that are one-line wrappers around the newly introduced
__devm_clk_get() function in the first patch and dummy implementations
for the !CONFIG_HAVE_CLK case.

The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:

  Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)

are available in the Git repository at:

  https://git.pengutronix.de/git/ukl/linux tags/devm-clk-get-enabled

for you to fetch changes up to fec74d434d6f6016b6b2d5ab13aa28a0c657f5fb:

  clk: Provide new devm_clk_helpers for prepared and enabled clocks (2021-05-11 14:20:13 +0200)

----------------------------------------------------------------
New variants of devm_clk_get() for prepared and enabled clocks

These two patches create a set of new devm helpers that return clocks
already prepared or prepared-and-enabled. The automatic cleanup cares
for unpreparing and disabling+unpreparing respectively.

This allows to simplify various drivers as was demonstrated with
additional patches sent with the various revisions of this patch set.
See
https://lore.kernel.org/r/[e-mail]
for the last submission round. This pull request doesn't contain these
patches though.

----------------------------------------------------------------
Uwe Kleine-König (2):
      clk: generalize devm_clk_get() a bit
      clk: Provide new devm_clk_helpers for prepared and enabled clocks

 drivers/clk/clk-devres.c | 96 ++++++++++++++++++++++++++++++++++++++++--------
 include/linux/clk.h      | 90 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 169 insertions(+), 17 deletions(-)

Comments

Uwe Kleine-König June 25, 2021, 5:14 p.m. UTC | #1

Hello Stephen,

On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote:
> given that I don't succeed in getting any feedback for my patch set, I'm
> trying with a pull request today. It would be really great if this pull
> request made it finally in for the next merge window.

It seems sending a pull request didn't help either :-\

I'm waiting since October for feedback, several people expressed to like
this series and I want to make use of it to simplify a few drivers. I'm
quite annoyed that your missing feedback blocks me from further
improving stuff.

> The changes are not as bad or complex as the diffstat suggests. The
> first patch contains all the complexity and only has
>  1 file changed, 50 insertions(+), 17 deletions(-)
> . The second patch makes use of this and just adds kernel-doc, four
> functions that are one-line wrappers around the newly introduced
> __devm_clk_get() function in the first patch and dummy implementations
> for the !CONFIG_HAVE_CLK case.
> 
> The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:
> 
>   Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)
> 
> are available in the Git repository at:
> 
>   https://git.pengutronix.de/git/ukl/linux tags/devm-clk-get-enabled
> 
> for you to fetch changes up to fec74d434d6f6016b6b2d5ab13aa28a0c657f5fb:
> 
>   clk: Provide new devm_clk_helpers for prepared and enabled clocks (2021-05-11 14:20:13 +0200)
> 
> ----------------------------------------------------------------
> New variants of devm_clk_get() for prepared and enabled clocks
> 
> These two patches create a set of new devm helpers that return clocks
> already prepared or prepared-and-enabled. The automatic cleanup cares
> for unpreparing and disabling+unpreparing respectively.
> 
> This allows to simplify various drivers as was demonstrated with
> additional patches sent with the various revisions of this patch set.
> See
> https://lore.kernel.org/r/[e-mail]
> for the last submission round. This pull request doesn't contain these
> patches though.
> 
> ----------------------------------------------------------------
> Uwe Kleine-König (2):
>       clk: generalize devm_clk_get() a bit
>       clk: Provide new devm_clk_helpers for prepared and enabled clocks
> 
>  drivers/clk/clk-devres.c | 96 ++++++++++++++++++++++++++++++++++++++++--------
>  include/linux/clk.h      | 90 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 169 insertions(+), 17 deletions(-)

Best regards
Uwe

Uwe Kleine-König July 5, 2021, 8:01 a.m. UTC | #2

Hello Stephen,

On Fri, Jun 25, 2021 at 07:14:34PM +0200, Uwe Kleine-König wrote:
> On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote:
> > given that I don't succeed in getting any feedback for my patch set, I'm
> > trying with a pull request today. It would be really great if this pull
> > request made it finally in for the next merge window.
> 
> It seems sending a pull request didn't help either :-\
> 
> I'm waiting since October for feedback, several people expressed to like
> this series and I want to make use of it to simplify a few drivers. I'm
> quite annoyed that your missing feedback blocks me from further
> improving stuff.

There is still no feedback, not even something like: "I saw your
nagging, sorry. I'm drown in other missions, please have some more
patience."

I assume it's not to much to expect at least such a reply after more
than 8 months?

Best regards
Uwe

Uwe Kleine-König July 22, 2021, 6:06 a.m. UTC | #3

Hello Stephen,

On Mon, Jul 05, 2021 at 10:01:44AM +0200, Uwe Kleine-König wrote:
> On Fri, Jun 25, 2021 at 07:14:34PM +0200, Uwe Kleine-König wrote:
> > On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote:
> > > given that I don't succeed in getting any feedback for my patch set, I'm
> > > trying with a pull request today. It would be really great if this pull
> > > request made it finally in for the next merge window.
> > 
> > It seems sending a pull request didn't help either :-\
> > 
> > I'm waiting since October for feedback, several people expressed to like
> > this series and I want to make use of it to simplify a few drivers. I'm
> > quite annoyed that your missing feedback blocks me from further
> > improving stuff.
> 
> There is still no feedback, not even something like: "I saw your
> nagging, sorry. I'm drown in other missions, please have some more
> patience."
> 
> I assume it's not to much to expect at least such a reply after more
> than 8 months?

The next merge window is over now. The pull request still merges fine
into v5.14-rc2. I'm still convinced it adds some benefit and I want to
use it to simplify a bunch of drivers. But I cannot without this being
merged.

Do I have to consider creating these functions in the pwm namespace to
continue here? This cannot be the right thing to do?!

Best regards
Uwe

Wolfram Sang July 22, 2021, 7:40 a.m. UTC | #4

> The next merge window is over now. The pull request still merges fine
> into v5.14-rc2. I'm still convinced it adds some benefit and I want to
> use it to simplify a bunch of drivers. But I cannot without this being
> merged.
> 
> Do I have to consider creating these functions in the pwm namespace to
> continue here? This cannot be the right thing to do?!

What about adding gkh to the list explaining the situation to him?

Uwe Kleine-König July 22, 2021, 8:18 a.m. UTC | #5

Hey Wolfram,

On Thu, Jul 22, 2021 at 09:40:03AM +0200, Wolfram Sang wrote:
> 
> > The next merge window is over now. The pull request still merges fine
> > into v5.14-rc2. I'm still convinced it adds some benefit and I want to
> > use it to simplify a bunch of drivers. But I cannot without this being
> > merged.
> > 
> > Do I have to consider creating these functions in the pwm namespace to
> > continue here? This cannot be the right thing to do?!
> 
> What about adding gkh to the list explaining the situation to him?

Greg doesn't like devm_ stuff.

I already asked Arnd who doesn't want to interfere and akpm who didn't
react either up to now.

Best regards
Uwe

Wolfram Sang July 22, 2021, 12:07 p.m. UTC | #6

> > What about adding gkh to the list explaining the situation to him?
> 
> Greg doesn't like devm_ stuff.
> 
> I already asked Arnd who doesn't want to interfere and akpm who didn't
> react either up to now.

Wow, okay, that is frustrating.

Uwe Kleine-König July 23, 2021, 9:13 a.m. UTC | #7

Hello,

[adding Linus and lkml to Cc: and adding some more context] 

On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote:
> given that I don't succeed in getting any feedback for my patch set, I'm
> trying with a pull request today.

This is for a series that is currently in v7 and didn't get any feedback
at all yet. The history is:

v1: 2020-10-13, https://lore.kernel.org/linux-clk/[e-mail]
    no feedback at all

v2: 2021-03-01, https://lore.kernel.org/linux-clk/[e-mail]
    kernel test robot identified some issues

v3: 2021-03-01, https://lore.kernel.org/linux-clk/[e-mail]
    I added a few driver patches to show the benefit. (However in a way
    that the autobuilders don't understand, so there were some false
    positive build failure reports.)

v4: 2021-03-30, https://lore.kernel.org/linux-clk/[e-mail]
    Got some feedback for the converted drivers by the respective
    maintainers. Some were indifferent, some found it good

v5: 2021-04-22, https://lore.kernel.org/linux-clk/[e-mail]
    Fixed a problem in one of the driver changes (i2c-imx), no feedback
    apart from pointing out a few typos, silence from the clk
    maintainers

v6: 2021-04-26, https://lore.kernel.org/linux-clk/[e-mail]
    Just the typos fixed, no feedback

v6 resend: 2021-05-10, https://lore.kernel.org/linux-clk/[e-mail]
    no changes in code. Got some feedback from Jonathan Cameron

v7: 2021-05-10, https://lore.kernel.org/linux-clk/[e-mail]
    Adress Jonathan's feedback, recieved some more acks from non-clk
    people

pull request: 2021-07-09, https://lore.kernel.org/linux-clk/[e-mail]

On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote:
> On Thursday, July 22, 2021, Wolfram Sang <[e-mail]> wrote:
> 
> >
> > > > What about adding gkh to the list explaining the situation to him?
> > >
> > > Greg doesn't like devm_ stuff.
> > >
> > > I already asked Arnd who doesn't want to interfere and akpm who didn't
> > > react either up to now.
> >
> > Wow, okay, that is frustrating.
> 
> The situation simply shows the process gap and One Maintainer nowadays is
> far from enough to satisfy demands.

Technically there are two maintainers for drivers/clk, Michael Turquette
and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the
capacity to address all requests.

> What I think about is that we need to escalate this to Linus and
> others and elaborate the mechanisms how to squeeze a new (additional)
> maintainer when the original one is not responsive. Let’s say some
> procedural steps. Otherwise we doomed because of human factor.

Assuming there was some process for this, is there someone who is
willing to take responsibility here?

Best regards
Uwe

Claudiu Beznea July 26, 2021, 9:18 a.m. UTC | #8

On 23.07.2021 12:13, Uwe Kleine-König wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 
> ForwardedMessage.eml
> 
> Subject:
> Re: [PULL] Add variants of devm_clk_get for prepared and enabled clocks
> enabled clocks
> From:
> Uwe Kleine-König <[e-mail]>
> Date:
> 23.07.2021, 12:13
> 
> To:
> Andy Shevchenko <[e-mail]>
> CC:
> Wolfram Sang <[e-mail]>, Stephen Boyd <[e-mail]>,
> "[e-mail]" <[e-mail]>,
> "[e-mail]" <[e-mail]>, Alexandre Belloni
> <[e-mail]>, Claudiu Beznea
> <[e-mail]>, Alessandro Zummo <[e-mail]>,
> Michael Turquette <[e-mail]>, Nicolas Ferre
> <[e-mail]>, "[e-mail]"
> <[e-mail]>, Oleksij Rempel <[e-mail]>,
> Ludovic Desroches <[e-mail]>, Mark Brown
> <[e-mail]>, Thierry Reding <[e-mail]>, Alexandru
> Ardelean <[e-mail]>, "[e-mail]"
> <[e-mail]>, Jonathan Cameron <[e-mail]>,
> Andrew Morton <[e-mail]>, Lee Jones
> <[e-mail]>, "[e-mail]"
> <[e-mail]>, "[e-mail]"
> <[e-mail]>, Linus Torvalds
> <[e-mail]>, <[e-mail]>
> 
> 
> Hello,
> 
> [adding Linus and lkml to Cc: and adding some more context] 
> 
> On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote:
>> given that I don't succeed in getting any feedback for my patch set, I'm
>> trying with a pull request today.
> This is for a series that is currently in v7 and didn't get any feedback
> at all yet. The history is:
> 
> v1: 2020-10-13, https://lore.kernel.org/linux-clk/[e-mail]
>     no feedback at all
> 
> v2: 2021-03-01, https://lore.kernel.org/linux-clk/[e-mail]
>     kernel test robot identified some issues
> 
> v3: 2021-03-01, https://lore.kernel.org/linux-clk/[e-mail]
>     I added a few driver patches to show the benefit. (However in a way
>     that the autobuilders don't understand, so there were some false
>     positive build failure reports.)
> 
> v4: 2021-03-30, https://lore.kernel.org/linux-clk/[e-mail]
>     Got some feedback for the converted drivers by the respective
>     maintainers. Some were indifferent, some found it good
> 
> v5: 2021-04-22, https://lore.kernel.org/linux-clk/[e-mail]
>     Fixed a problem in one of the driver changes (i2c-imx), no feedback
>     apart from pointing out a few typos, silence from the clk
>     maintainers
> 
> v6: 2021-04-26, https://lore.kernel.org/linux-clk/[e-mail]
>     Just the typos fixed, no feedback
> 
> v6 resend: 2021-05-10, https://lore.kernel.org/linux-clk/[e-mail]
>     no changes in code. Got some feedback from Jonathan Cameron
> 
> v7: 2021-05-10, https://lore.kernel.org/linux-clk/[e-mail]
>     Adress Jonathan's feedback, recieved some more acks from non-clk
>     people
> 
> pull request: 2021-07-09, https://lore.kernel.org/linux-clk/[e-mail]
> 
> On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote:
>> On Thursday, July 22, 2021, Wolfram Sang <[e-mail]> wrote:
>>
>>>>> What about adding gkh to the list explaining the situation to him?
>>>> Greg doesn't like devm_ stuff.
>>>>
>>>> I already asked Arnd who doesn't want to interfere and akpm who didn't
>>>> react either up to now.
>>> Wow, okay, that is frustrating.
>> The situation simply shows the process gap and One Maintainer nowadays is
>> far from enough to satisfy demands.
> Technically there are two maintainers for drivers/clk, Michael Turquette
> and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the
> capacity to address all requests.
> 
>> What I think about is that we need to escalate this to Linus and
>> others and elaborate the mechanisms how to squeeze a new (additional)
>> maintainer when the original one is not responsive. Let’s say some
>> procedural steps. Otherwise we doomed because of human factor.
> Assuming there was some process for this, is there someone who is
> willing to take responsibility here?

Hi,

In the last year I worked on AT91 clock drivers and I would be available
for taking responsibility beyond AT91 clocks (if everyone's OK with this),
in whatever form the current maintainers and people in the audience would
agree, if any (co-maintainer or other forms that could be useful). The idea
is to help things progress as I also have patches waiting for feedback on
clock mailing list for almost 6 months.

Let me know if I can be helpful.

Thank you,
Claudiu Beznea

> 
> Best regards
> Uwe
>  
> -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions |
> https://www.pengutronix.de/ |
>

Andy Shevchenko July 26, 2021, 9:52 a.m. UTC | #9

On Mon, Jul 26, 2021 at 12:18 PM <[e-mail]> wrote:
> On 23.07.2021 12:13, Uwe Kleine-König wrote:
> > From:
> > Uwe Kleine-König <[e-mail]>
> > Date:
> > 23.07.2021, 12:13
> > On Wed, Jun 09, 2021 at 10:21:23PM +0200, Uwe Kleine-König wrote:
> >> given that I don't succeed in getting any feedback for my patch set, I'm
> >> trying with a pull request today.
> > This is for a series that is currently in v7 and didn't get any feedback
> > at all yet. The history is:
> >
> > v1: 2020-10-13, https://lore.kernel.org/linux-clk/[e-mail]
> >     no feedback at all
> >
> > v2: 2021-03-01, https://lore.kernel.org/linux-clk/[e-mail]
> >     kernel test robot identified some issues
> >
> > v3: 2021-03-01, https://lore.kernel.org/linux-clk/[e-mail]
> >     I added a few driver patches to show the benefit. (However in a way
> >     that the autobuilders don't understand, so there were some false
> >     positive build failure reports.)
> >
> > v4: 2021-03-30, https://lore.kernel.org/linux-clk/[e-mail]
> >     Got some feedback for the converted drivers by the respective
> >     maintainers. Some were indifferent, some found it good
> >
> > v5: 2021-04-22, https://lore.kernel.org/linux-clk/[e-mail]
> >     Fixed a problem in one of the driver changes (i2c-imx), no feedback
> >     apart from pointing out a few typos, silence from the clk
> >     maintainers
> >
> > v6: 2021-04-26, https://lore.kernel.org/linux-clk/[e-mail]
> >     Just the typos fixed, no feedback
> >
> > v6 resend: 2021-05-10, https://lore.kernel.org/linux-clk/[e-mail]
> >     no changes in code. Got some feedback from Jonathan Cameron
> >
> > v7: 2021-05-10, https://lore.kernel.org/linux-clk/[e-mail]
> >     Adress Jonathan's feedback, recieved some more acks from non-clk
> >     people
> >
> > pull request: 2021-07-09, https://lore.kernel.org/linux-clk/[e-mail]
> >
> > On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote:
> >> On Thursday, July 22, 2021, Wolfram Sang <[e-mail]> wrote:
> >>
> >>>>> What about adding gkh to the list explaining the situation to him?
> >>>> Greg doesn't like devm_ stuff.
> >>>>
> >>>> I already asked Arnd who doesn't want to interfere and akpm who didn't
> >>>> react either up to now.
> >>> Wow, okay, that is frustrating.
> >> The situation simply shows the process gap and One Maintainer nowadays is
> >> far from enough to satisfy demands.
> > Technically there are two maintainers for drivers/clk, Michael Turquette
> > and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the
> > capacity to address all requests.
> >
> >> What I think about is that we need to escalate this to Linus and
> >> others and elaborate the mechanisms how to squeeze a new (additional)
> >> maintainer when the original one is not responsive. Let’s say some
> >> procedural steps. Otherwise we doomed because of human factor.
> > Assuming there was some process for this, is there someone who is
> > willing to take responsibility here?
>
> Hi,
>
> In the last year I worked on AT91 clock drivers and I would be available
> for taking responsibility beyond AT91 clocks (if everyone's OK with this),
> in whatever form the current maintainers and people in the audience would
> agree, if any (co-maintainer or other forms that could be useful). The idea
> is to help things progress as I also have patches waiting for feedback on
> clock mailing list for almost 6 months.
>
> Let me know if I can be helpful.

I think so. Many subsystems relatively recently (in the last couple of
years or so) enforced that new drivers have to have official
maintainers. Besides that it's warmly welcome to register existing
drivers in the MAINTAINERS database. I would tell you go ahead and
become a maintainer of AT91 clocks and it will definitely reduce the
burden on Stephan's shoulders.

The idea is that you will send a PR to CCF maintainers instead of
patches, although it's expected that patches appear in the mailing
list beforehand anyway.

Wolfram Sang July 26, 2021, 12:32 p.m. UTC | #10

> The idea is that you will send a PR to CCF maintainers instead of
> patches, although it's expected that patches appear in the mailing
> list beforehand anyway.

Depends a little. For me, a Rev-by from the driver maintainer is enough,
and I'll pick them. That lowers the burden on the drivers maintainer
side. May not work for high volumes of patches, but for I2C this works
enough.

Andy Shevchenko July 26, 2021, 1:28 p.m. UTC | #11

On Mon, Jul 26, 2021 at 3:33 PM Wolfram Sang <[e-mail]> wrote:
>
>
> > The idea is that you will send a PR to CCF maintainers instead of
> > patches, although it's expected that patches appear in the mailing
> > list beforehand anyway.
>
> Depends a little. For me, a Rev-by from the driver maintainer is enough,
> and I'll pick them. That lowers the burden on the drivers maintainer
> side. May not work for high volumes of patches, but for I2C this works
> enough.

AFAICT in practice it's a mandatory requirement in I²C subsys (in the
past you hadn't accepted a patch from us without a tag from the
maintainer) which makes it equal to sending PR by a maintainer. PR
makes less burden since subsys maintainer don't need to run many tools
or a tool many times to get the pile of patches.

Wolfram Sang July 26, 2021, 5:40 p.m. UTC | #12

> AFAICT in practice it's a mandatory requirement in I²C subsys (in the
> past you hadn't accepted a patch from us without a tag from the
> maintainer) which makes it equal to sending PR by a maintainer. PR

Right. I require a tag from the driver maintainer.

> makes less burden since subsys maintainer don't need to run many tools
> or a tool many times to get the pile of patches.

I had driver maintainers who found it difficult to have a public tree to
pull from or forgot how to send properly prepared pull requests. They
were happy to send Rev-by, though. And I am happy with that, too. At
least in I2C, picking up patches is small work compared to the actual
review.

Uwe Kleine-König July 28, 2021, 8:25 p.m. UTC | #13

Hello,

I adapted the Subject in the hope to catch Stephen's and Michael's
attention. My impression is that this thread isn't on their radar yet,
but the topic here seems important enough to get a matching Subject.

On Mon, Jul 26, 2021 at 09:18:16AM +0000, [e-mail] wrote:
> > On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote:
> >> On Thursday, July 22, 2021, Wolfram Sang <[e-mail]> wrote:
> >>>>>> [ some frustration for not getting feedback for clk patches ]
> >>>>> What about adding gkh to the list explaining the situation to him?
> >>>> Greg doesn't like devm_ stuff.
> >>>>
> >>>> I already asked Arnd who doesn't want to interfere and akpm who didn't
> >>>> react either up to now.
> >>> Wow, okay, that is frustrating.
> >> The situation simply shows the process gap and One Maintainer nowadays is
> >> far from enough to satisfy demands.
> > 
> > Technically there are two maintainers for drivers/clk, Michael Turquette
> > and Stephen Boyd. It seems Michael is MIA and Stephen doesn't have the
> > capacity to address all requests.
> > 
> >> What I think about is that we need to escalate this to Linus and
> >> others and elaborate the mechanisms how to squeeze a new (additional)
> >> maintainer when the original one is not responsive. Let’s say some
> >> procedural steps. Otherwise we doomed because of human factor.
> > 
> > Assuming there was some process for this, is there someone who is
> > willing to take responsibility here?
> 
> In the last year I worked on AT91 clock drivers and I would be available
> for taking responsibility beyond AT91 clocks (if everyone's OK with this),
> in whatever form the current maintainers and people in the audience would
> agree, if any (co-maintainer or other forms that could be useful). The idea
> is to help things progress as I also have patches waiting for feedback on
> clock mailing list for almost 6 months.
> 
> Let me know if I can be helpful.

Wondering about how we can progress here I think it's crucial that
Stephen and/or Michael share their thoughts about how they intend to
care for drivers/clk in the future.

Do you want to keep the maintainer post long-term? Or only for a
transitional period until someone else can take care? Is your
non-presence only temporal and is it foreseeable that you will increase
your efforts in the next weeks/months again? Do you welcome a
co-maintainer? What kind of involvement would you consider helpful?

Thanks to Claudiu for offering to support here, at least from my side
this is very appreciated.

Best regards
Uwe

Russell King (Oracle) July 28, 2021, 8:40 p.m. UTC | #14

On Wed, Jul 28, 2021 at 10:25:47PM +0200, Uwe Kleine-König wrote:
> I adapted the Subject in the hope to catch Stephen's and Michael's
> attention. My impression is that this thread isn't on their radar yet,
> but the topic here seems important enough to get a matching Subject.

Have you thought about sending your pull request to the clk API
maintainer (iow, me) ?

Stephen Boyd July 31, 2021, 7:41 a.m. UTC | #15

Quoting Russell King (Oracle) (2021-07-28 13:40:34)
> > I adapted the Subject in the hope to catch Stephen's and Michael's
> > attention. My impression is that this thread isn't on their radar yet,
> > but the topic here seems important enough to get a matching Subject.

The thread is on my radar but...

> 
> Have you thought about sending your pull request to the clk API
> maintainer (iow, me) ?
> 

+1 This patch doesn't fall under CCF maintainer.

 $ ./scripts/get_maintainer.pl -f include/linux/clk.h
 Russell King <[e-mail]> (maintainer:CLK API)
 [e-mail] (open list:CLK API)
 [e-mail] (open list)

Finally, this sort of patch has been discussed for years and I didn't
see any mention of those previous attempts in the patch series. Has
something changed since that time? I think we've got a bunch of hand
rolled devm things in the meantime but not much else. 

I still wonder if it would be better if we had some sort of DT binding
that said "turn this clk on when the driver probes this device and keep
it on until the driver is unbound". That would probably work well for
quite a few drivers that don't want to ever call clk_get() or
clk_prepare_enable() and could tie into the assigned-clock-rates
property in a way that let us keep the platform details out of the
drivers. We could also go one step further and make a clk pm domain when
this new property is present and then have the clk be enabled based on
runtime PM of the device (and if runtime PM is disabled then just enable
it at driver probe time).

Andy Shevchenko July 31, 2021, 8:07 a.m. UTC | #16

On Sat, Jul 31, 2021 at 10:41 AM Stephen Boyd <[e-mail]> wrote:
> Quoting Russell King (Oracle) (2021-07-28 13:40:34)
> > > I adapted the Subject in the hope to catch Stephen's and Michael's
> > > attention. My impression is that this thread isn't on their radar yet,
> > > but the topic here seems important enough to get a matching Subject.

> I still wonder if it would be better if we had some sort of DT binding
> that said "turn this clk on when the driver probes this device and keep
> it on until the driver is unbound".

DT is not the only way the clocks are established in the kernel.

> That would probably work well for
> quite a few drivers that don't want to ever call clk_get() or
> clk_prepare_enable() and could tie into the assigned-clock-rates
> property in a way that let us keep the platform details out of the
> drivers.

> We could also go one step further and make a clk pm domain when
> this new property is present and then have the clk be enabled based on
> runtime PM of the device (and if runtime PM is disabled then just enable
> it at driver probe time).

This sounds like a good idea from a usage perspective.

Uwe Kleine-König July 31, 2021, noon UTC | #17

Hi Russell, hi Stephen,

On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote:
> Quoting Russell King (Oracle) (2021-07-28 13:40:34)
> > > I adapted the Subject in the hope to catch Stephen's and Michael's
> > > attention. My impression is that this thread isn't on their radar yet,
> > > but the topic here seems important enough to get a matching Subject.
> 
> The thread is on my radar but...
> 
> > 
> > Have you thought about sending your pull request to the clk API
> > maintainer (iow, me) ?

I wasn't really aware that Russell has the clk API hat (or that this
separate hat actually exists and this isn't purely a CCF topic). I
assume I only did

	$ scripts/get_maintainer.pl -f drivers/clk/clk-devres.c

which is where the current and new code implementing devm_clk_get et al
lives.

@Russell: What is your position here, do you like the approach of
devm_clk_get_enabled? I can send a new pull request in your direction if
you like it and are willing to take it.

> +1 This patch doesn't fall under CCF maintainer.

Given that CCF is the only implementer of devm_clk_get at least an Ack
from your side would still be good I guess?

> Finally, this sort of patch has been discussed for years and I didn't
> see any mention of those previous attempts in the patch series. Has
> something changed since that time? I think we've got a bunch of hand
> rolled devm things in the meantime but not much else. 

I found a patch set adding devm variants of clk_enable (e.g.
https://lore.kernel.org/patchwork/patch/755667/) but this approach is
different as it also contains clk_get which IMHO makes more sense 
The discussion considered wrapping get+enable at one point, but I didn't
find a followup.

> I still wonder if it would be better if we had some sort of DT binding
> that said "turn this clk on when the driver probes this device and keep
> it on until the driver is unbound".

This doesn't sound like a hardware property and so I don't think this
belongs into DT and I would be surprised if the dt maintainers would be
willing to accept an idea with this semantic.

> That would probably work well for quite a few drivers that don't want
> to ever call clk_get() or clk_prepare_enable() and could tie into the
> assigned-clock-rates property in a way that let us keep the platform
> details out of the drivers. We could also go one step further and make
> a clk pm domain when this new property is present and then have the
> clk be enabled based on runtime PM of the device (and if runtime PM is
> disabled then just enable it at driver probe time).

clk pm domain sounds good, but introducing devm_clk_get_enabled() is
much easier and converting to it can be done without dt changes and more
or less mechanically. So I consider the cost-usage-value of
devm_clk_get_enabled() much better.

Best regards
Uwe

Jonathan Cameron Aug. 2, 2021, 9:36 a.m. UTC | #18

On Sat, 31 Jul 2021 14:00:04 +0200
Uwe Kleine-König <[e-mail]> wrote:

> Hi Russell, hi Stephen,
> 
> On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote:
> > Quoting Russell King (Oracle) (2021-07-28 13:40:34)  
> > > > I adapted the Subject in the hope to catch Stephen's and Michael's
> > > > attention. My impression is that this thread isn't on their radar yet,
> > > > but the topic here seems important enough to get a matching Subject.  
> > 
> > The thread is on my radar but...
> >   
> > > 
> > > Have you thought about sending your pull request to the clk API
> > > maintainer (iow, me) ?  
> 
> I wasn't really aware that Russell has the clk API hat (or that this
> separate hat actually exists and this isn't purely a CCF topic). I
> assume I only did
> 
> 	$ scripts/get_maintainer.pl -f drivers/clk/clk-devres.c
> 
> which is where the current and new code implementing devm_clk_get et al
> lives.
> 
> @Russell: What is your position here, do you like the approach of
> devm_clk_get_enabled? I can send a new pull request in your direction if
> you like it and are willing to take it.
> 
> > +1 This patch doesn't fall under CCF maintainer.  
> 
> Given that CCF is the only implementer of devm_clk_get at least an Ack
> from your side would still be good I guess?
> 
> > Finally, this sort of patch has been discussed for years and I didn't
> > see any mention of those previous attempts in the patch series. Has
> > something changed since that time? I think we've got a bunch of hand
> > rolled devm things in the meantime but not much else.   
> 
> I found a patch set adding devm variants of clk_enable (e.g.
> https://lore.kernel.org/patchwork/patch/755667/) but this approach is
> different as it also contains clk_get which IMHO makes more sense 
> The discussion considered wrapping get+enable at one point, but I didn't
> find a followup.
> 
> > I still wonder if it would be better if we had some sort of DT binding
> > that said "turn this clk on when the driver probes this device and keep
> > it on until the driver is unbound".  
> 
> This doesn't sound like a hardware property and so I don't think this
> belongs into DT and I would be surprised if the dt maintainers would be
> willing to accept an idea with this semantic.

Agreed. It's not unheard of to have a driver start out just enabing
clock at probe and dropping it at remove. When the driver gets more
sophisticated it will then manage the clock more frequently.
Whilst that's often tied to runtime_pm I'm not sure it always is.

Given the mess that would be involved in having a property that we
need to later ignore for particular drivers, I'd keep this management
explicit in the driver. This series makes that trivial to do for these
easy cases.

Jonathan

> 
> > That would probably work well for quite a few drivers that don't want
> > to ever call clk_get() or clk_prepare_enable() and could tie into the
> > assigned-clock-rates property in a way that let us keep the platform
> > details out of the drivers. We could also go one step further and make
> > a clk pm domain when this new property is present and then have the
> > clk be enabled based on runtime PM of the device (and if runtime PM is
> > disabled then just enable it at driver probe time).  
> 
> clk pm domain sounds good, but introducing devm_clk_get_enabled() is
> much easier and converting to it can be done without dt changes and more
> or less mechanically. So I consider the cost-usage-value of
> devm_clk_get_enabled() much better.
> 
> Best regards
> Uwe
>

Russell King (Oracle) Aug. 2, 2021, 9:48 a.m. UTC | #19

On Sat, Jul 31, 2021 at 02:00:04PM +0200, Uwe Kleine-König wrote:
> Hi Russell, hi Stephen,
> 
> On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote:
> > +1 This patch doesn't fall under CCF maintainer.
> 
> Given that CCF is the only implementer of devm_clk_get at least an Ack
> from your side would still be good I guess?

I think devm_clk_get() should not be part of CCF but should be
part of the interface level - it's silly to have devm_clk_get()
being CCF but not clk_get(). The same should go for the other
devm wrappers around the plain clk_* interfaces.

> I found a patch set adding devm variants of clk_enable (e.g.
> https://lore.kernel.org/patchwork/patch/755667/) but this approach is
> different as it also contains clk_get which IMHO makes more sense 
> The discussion considered wrapping get+enable at one point, but I didn't
> find a followup.

There have been several different approaches to wrapping things up,
but here's a question: should we make it easier to do the lazy thing
(get+enable) or should we make it easier to be power efficient?
Shouldn't we be encouraging people to write power efficient drivers?

> > I still wonder if it would be better if we had some sort of DT binding
> > that said "turn this clk on when the driver probes this device and keep
> > it on until the driver is unbound".
> 
> This doesn't sound like a hardware property and so I don't think this
> belongs into DT and I would be surprised if the dt maintainers would be
> willing to accept an idea with this semantic.

I really don't like that idea - enabling or disabling a clock is
an implementation decision, one which can change over time. Even
if a clock is required to be on for e.g. accessing device registers,
we may decide that, if we want maximum power savings, to disable
that clock when the device is not being used, but an initial driver
implementation may not do that. If we encourage people to throw a
property in DT, then the driver's runtime behaviour becomes a
combination of the DT being used and the driver implementation.
Let's keep that to a minimum.

Uwe Kleine-König Aug. 2, 2021, 3:27 p.m. UTC | #20

Hello Russell,

On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote:
> On Sat, Jul 31, 2021 at 02:00:04PM +0200, Uwe Kleine-König wrote:
> > Hi Russell, hi Stephen,
> > 
> > On Sat, Jul 31, 2021 at 12:41:19AM -0700, Stephen Boyd wrote:
> > > +1 This patch doesn't fall under CCF maintainer.
> > 
> > Given that CCF is the only implementer of devm_clk_get at least an Ack
> > from your side would still be good I guess?
> 
> I think devm_clk_get() should not be part of CCF but should be
> part of the interface level - it's silly to have devm_clk_get()
> being CCF but not clk_get(). The same should go for the other
> devm wrappers around the plain clk_* interfaces.

What is the practical difference between "Function X is part of CCF" and
"Function X is part of the clk interface and there is only CCF who
implements it"?

> > I found a patch set adding devm variants of clk_enable (e.g.
> > https://lore.kernel.org/patchwork/patch/755667/) but this approach is
> > different as it also contains clk_get which IMHO makes more sense 
> > The discussion considered wrapping get+enable at one point, but I didn't
> > find a followup.
> 
> There have been several different approaches to wrapping things up,
> but here's a question: should we make it easier to do the lazy thing
> (get+enable) or should we make it easier to be power efficient?
> Shouldn't we be encouraging people to write power efficient drivers?

Yeah, sounds compelling, but I wonder if that's of practical importance.
How many driver authors do you expect to lure into making a better
driver just because devm_clk_get_prepared() doesn't exist? In contrast:
How many drivers become simpler with devm_clk_get_prepared() and so
it becomes easier to maintain them and easier to spot bugs?
In the absence of devm_clk_get_prepared(), is it better that several
frameworks (or drivers) open code it?

Best regards
Uwe

Russell King (Oracle) Aug. 2, 2021, 4:38 p.m. UTC | #21

On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-König wrote:
> Hello Russell,
> 
> On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote:
> > I think devm_clk_get() should not be part of CCF but should be
> > part of the interface level - it's silly to have devm_clk_get()
> > being CCF but not clk_get(). The same should go for the other
> > devm wrappers around the plain clk_* interfaces.
> 
> What is the practical difference between "Function X is part of CCF" and
> "Function X is part of the clk interface and there is only CCF who
> implements it"?

clkdev is maintained by me as part of the API, and provides clk_get()
functionality for all clk implementations. To then have devm_clk_get(),
which merely provides a wrapper around clk_get() adding the devm
semantics being part of CCF is not sane - devm_clk_get() isn't
specific to CCF or in fact any clk API implementation.

> > There have been several different approaches to wrapping things up,
> > but here's a question: should we make it easier to do the lazy thing
> > (get+enable) or should we make it easier to be power efficient?
> > Shouldn't we be encouraging people to write power efficient drivers?
> 
> Yeah, sounds compelling, but I wonder if that's of practical importance.
> How many driver authors do you expect to lure into making a better
> driver just because devm_clk_get_prepared() doesn't exist? In contrast:
> How many drivers become simpler with devm_clk_get_prepared() and so
> it becomes easier to maintain them and easier to spot bugs?
> In the absence of devm_clk_get_prepared(), is it better that several
> frameworks (or drivers) open code it?

It probably depends on where you stand on power management and power
efficiency issues. Personally, I would like to see more effort put
into drivers to make them more power efficient, and I believe in the
coming years, power efficiency is going to become a big issue.

Andy Shevchenko Aug. 2, 2021, 5:13 p.m. UTC | #22

On Mon, Aug 2, 2021 at 7:38 PM Russell King (Oracle)
<[e-mail]> wrote:
> On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-König wrote:
> > Hello Russell,
> > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote:

...

> > > There have been several different approaches to wrapping things up,
> > > but here's a question: should we make it easier to do the lazy thing
> > > (get+enable) or should we make it easier to be power efficient?
> > > Shouldn't we be encouraging people to write power efficient drivers?
> >
> > Yeah, sounds compelling, but I wonder if that's of practical importance.
> > How many driver authors do you expect to lure into making a better
> > driver just because devm_clk_get_prepared() doesn't exist? In contrast:
> > How many drivers become simpler with devm_clk_get_prepared() and so
> > it becomes easier to maintain them and easier to spot bugs?
> > In the absence of devm_clk_get_prepared(), is it better that several
> > frameworks (or drivers) open code it?
>
> It probably depends on where you stand on power management and power
> efficiency issues. Personally, I would like to see more effort put
> into drivers to make them more power efficient, and I believe in the
> coming years, power efficiency is going to become a big issue.

While in the ideal world I 100% agree with the approach, IRL we have
to deal with constantly degrading quality of the code and instead of
thinking about power management and efficiency the absence of APIs
such as discussed provokes not only creating the power management
inefficient code, but also memory leaks here and there.

Russell King (Oracle) Aug. 2, 2021, 8:28 p.m. UTC | #23

On Mon, Aug 02, 2021 at 08:13:05PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 2, 2021 at 7:38 PM Russell King (Oracle)
> <[e-mail]> wrote:
> > It probably depends on where you stand on power management and power
> > efficiency issues. Personally, I would like to see more effort put
> > into drivers to make them more power efficient, and I believe in the
> > coming years, power efficiency is going to become a big issue.
> 
> While in the ideal world I 100% agree with the approach, IRL we have
> to deal with constantly degrading quality of the code and instead of
> thinking about power management and efficiency the absence of APIs
> such as discussed provokes not only creating the power management
> inefficient code, but also memory leaks here and there.

The point of my previous reply that you quoted above was to make a
prediction, it wasn't a rejection of the approach.

Stephen Boyd Aug. 3, 2021, 7:44 a.m. UTC | #24

Quoting Russell King (Oracle) (2021-08-02 02:48:10)
> 
> > > I still wonder if it would be better if we had some sort of DT binding
> > > that said "turn this clk on when the driver probes this device and keep
> > > it on until the driver is unbound".
> > 
> > This doesn't sound like a hardware property and so I don't think this
> > belongs into DT and I would be surprised if the dt maintainers would be
> > willing to accept an idea with this semantic.
> 
> I really don't like that idea - enabling or disabling a clock is
> an implementation decision, one which can change over time. Even
> if a clock is required to be on for e.g. accessing device registers,
> we may decide that, if we want maximum power savings, to disable
> that clock when the device is not being used, but an initial driver
> implementation may not do that. If we encourage people to throw a
> property in DT, then the driver's runtime behaviour becomes a
> combination of the DT being used and the driver implementation.
> Let's keep that to a minimum.
> 

I suspect that sometimes we want to express that some clk is on all the
time in DT because there isn't any sort of consumer driver for it. We
have fixed rate clks that sort of do that, but I'm thinking about
something like a GPIO clk gate that is downstream of some clk source,
and this gpio gate is enabled once at boot and then forgotten about. I
suppose in this case we could have a property in the clk gpio binding
that expresses this property of the hardware so it's best to not make
something more generic that could be abused.

Stephen Boyd Aug. 3, 2021, 8:11 a.m. UTC | #25

Quoting Russell King (Oracle) (2021-08-02 09:38:24)
> On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-Konig wrote:
> > Hello Russell,
> > 
> > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote:
> 
> > > There have been several different approaches to wrapping things up,
> > > but here's a question: should we make it easier to do the lazy thing
> > > (get+enable) or should we make it easier to be power efficient?
> > > Shouldn't we be encouraging people to write power efficient drivers?
> > 
> > Yeah, sounds compelling, but I wonder if that's of practical importance.
> > How many driver authors do you expect to lure into making a better
> > driver just because devm_clk_get_prepared() doesn't exist? In contrast:
> > How many drivers become simpler with devm_clk_get_prepared() and so
> > it becomes easier to maintain them and easier to spot bugs?
> > In the absence of devm_clk_get_prepared(), is it better that several
> > frameworks (or drivers) open code it?
> 
> It probably depends on where you stand on power management and power
> efficiency issues. Personally, I would like to see more effort put
> into drivers to make them more power efficient, and I believe in the
> coming years, power efficiency is going to become a big issue.
> 

I agree we should put more effort into power efficiency in the kernel.
I've occasionally heard from driver writers that they never will turn
the clk off even in low power modes though. They feel like it's a
nuisance to have to do anything with the clk framework in their driver.
When I say "why not use runtime PM?" I get told that they're not turning
the clk off because it needs to be on all the time, so using runtime PM
makes the driver more complicated, not less, and adds no value. I think
some touchscreens are this way, and watchdogs too. Looking at the
drivers being converted in this series I suspect RTC is one of those
sorts of devices as well. But SPI and I2C most likely could benefit from
using runtime PM and so those ones don't feel appropriate to convert.

Maybe this series would be more compelling if those various drivers that
are hand rolling the devm action were converted to the consolidated
official devm function. The truth is it's already happening in various
subsystems so consolidating that logic into one place would be a win
code size wise and very hard to ignore.

Doing

 $ git grep devm_add_action | grep clk

seems to catch quite a few of them.

Uwe Kleine-König Aug. 3, 2021, 10:40 a.m. UTC | #26

On Tue, Aug 03, 2021 at 01:11:54AM -0700, Stephen Boyd wrote:
> Quoting Russell King (Oracle) (2021-08-02 09:38:24)
> > On Mon, Aug 02, 2021 at 05:27:55PM +0200, Uwe Kleine-Konig wrote:
> > > Hello Russell,
> > > 
> > > On Mon, Aug 02, 2021 at 10:48:10AM +0100, Russell King (Oracle) wrote:
> > 
> > > > There have been several different approaches to wrapping things up,
> > > > but here's a question: should we make it easier to do the lazy thing
> > > > (get+enable) or should we make it easier to be power efficient?
> > > > Shouldn't we be encouraging people to write power efficient drivers?
> > > 
> > > Yeah, sounds compelling, but I wonder if that's of practical importance.
> > > How many driver authors do you expect to lure into making a better
> > > driver just because devm_clk_get_prepared() doesn't exist? In contrast:
> > > How many drivers become simpler with devm_clk_get_prepared() and so
> > > it becomes easier to maintain them and easier to spot bugs?
> > > In the absence of devm_clk_get_prepared(), is it better that several
> > > frameworks (or drivers) open code it?
> > 
> > It probably depends on where you stand on power management and power
> > efficiency issues. Personally, I would like to see more effort put
> > into drivers to make them more power efficient, and I believe in the
> > coming years, power efficiency is going to become a big issue.
> > 
> 
> I agree we should put more effort into power efficiency in the kernel.
> I've occasionally heard from driver writers that they never will turn
> the clk off even in low power modes though. They feel like it's a
> nuisance to have to do anything with the clk framework in their driver.
> When I say "why not use runtime PM?" I get told that they're not turning
> the clk off because it needs to be on all the time, so using runtime PM
> makes the driver more complicated, not less, and adds no value. I think
> some touchscreens are this way, and watchdogs too. Looking at the
> drivers being converted in this series I suspect RTC is one of those
> sorts of devices as well. But SPI and I2C most likely could benefit from
> using runtime PM and so those ones don't feel appropriate to convert.
> 
> Maybe this series would be more compelling if those various drivers that
> are hand rolling the devm action were converted to the consolidated
> official devm function. The truth is it's already happening in various
> subsystems so consolidating that logic into one place would be a win
> code size wise and very hard to ignore.
> 
> Doing
> 
>  $ git grep devm_add_action | grep clk
> 
> seems to catch quite a few of them.

Another upside is that grepping for these drivers with a potential for
further improvement become easier to grep for as
devm_clk_get_{prepared,enabled} is a much better hint :-)

The changes to these drivers probably won't go through a clk tree, so
adding these patches before adding devm_clk_get_enabled() would only
help for the warm and cozy feeling that it is right to do so, correct?

As my focus is limited to (mostly) drivers/pwm and I already have quite
some other patch quests on my list:

So can I lure you in merging the new functions and I will create a
kernel janitor task to convert more existing drivers?

Best regards
Uwe

Stephen Boyd Aug. 6, 2021, 12:26 a.m. UTC | #27

Quoting Uwe Kleine-König (2021-08-03 03:40:12)
> On Tue, Aug 03, 2021 at 01:11:54AM -0700, Stephen Boyd wrote:
> > 
> > Maybe this series would be more compelling if those various drivers that
> > are hand rolling the devm action were converted to the consolidated
> > official devm function. The truth is it's already happening in various
> > subsystems so consolidating that logic into one place would be a win
> > code size wise and very hard to ignore.
> > 
> > Doing
> > 
> >  $ git grep devm_add_action | grep clk
> > 
> > seems to catch quite a few of them.
> 
> Another upside is that grepping for these drivers with a potential for
> further improvement become easier to grep for as
> devm_clk_get_{prepared,enabled} is a much better hint :-)

Sorry, but that's a pretty weak argument. I'd think grepping for the
absence of pm_ops in drivers would be the same hint.

> 
> The changes to these drivers probably won't go through a clk tree, so
> adding these patches before adding devm_clk_get_enabled() would only
> help for the warm and cozy feeling that it is right to do so, correct?

It isn't to feel warm and cozy. It's to demonstrate the need for
consolidating code. Converting the i2c and spi drivers to use this is
actively damaging the cause though. Those driver frameworks are more
likely to encourage proper power management around bus transfers, so
converting them to use the devm API moves them away from power
management, not closer to it.

This proves why this topic is always contentious. It's too easy to
blindly convert drivers to get the clk and leave it enabled forever and
then they never use power management. The janitors win and nobody else.

Is there some way to avoid that trap? Maybe through some combination of
a device PM function that indicates the driver has no runtime PM
callbacks (pm_runtime_no_callbacks() perhaps?) and
devm_clk_get_enabled() checking for that and returning the clk only when
that call has been made (i.e. pm_runtime_has_no_callbacks())? This
approach would fail to catch the case where system wide suspend/resume
could turn the clk off but the driver doesn't do it. I'm not sure how
much we care about that case though.

> 
> As my focus is limited to (mostly) drivers/pwm and I already have quite
> some other patch quests on my list:

Don't we all? :)

Uwe Kleine-König Sept. 14, 2021, 1:22 p.m. UTC | #28

On Thu, Aug 05, 2021 at 05:26:16PM -0700, Stephen Boyd wrote:
> Quoting Uwe Kleine-König (2021-08-03 03:40:12)
> > On Tue, Aug 03, 2021 at 01:11:54AM -0700, Stephen Boyd wrote:
> > > 
> > > Maybe this series would be more compelling if those various drivers that
> > > are hand rolling the devm action were converted to the consolidated
> > > official devm function. The truth is it's already happening in various
> > > subsystems so consolidating that logic into one place would be a win
> > > code size wise and very hard to ignore.
> > > 
> > > Doing
> > > 
> > >  $ git grep devm_add_action | grep clk
> > > 
> > > seems to catch quite a few of them.

Will do.
 
> > Another upside is that grepping for these drivers with a potential for
> > further improvement become easier to grep for as
> > devm_clk_get_{prepared,enabled} is a much better hint :-)
> 
> Sorry, but that's a pretty weak argument. I'd think grepping for the
> absence of pm_ops in drivers would be the same hint.

To be honest: Yes, it's a weak argument, but grepping for drivers
without pm_ops is a tad more complicated and yields a different set of
drivers. For example take the i2c-imx driver
(drivers/i2c/busses/i2c-imx.c) which has a pm_ops but still can make use
of devm_clk_get_enabled.

> > The changes to these drivers probably won't go through a clk tree, so
> > adding these patches before adding devm_clk_get_enabled() would only
> > help for the warm and cozy feeling that it is right to do so, correct?
> 
> It isn't to feel warm and cozy. It's to demonstrate the need for
> consolidating code. Converting the i2c and spi drivers to use this is
> actively damaging the cause though. Those driver frameworks are more
> likely to encourage proper power management around bus transfers, so
> converting them to use the devm API moves them away from power
> management, not closer to it.

Well I think one could disagree here. Today these drivers are not power
efficient as they just enable the clock in their probe routine and keep
it on even though it might not be needed.

My patch still is beneficial as it simplifies the drivers without making
them worse. Agreed, this isn't the best optimisation to the drivers
(assuming it is possible to disable the clocks while the device isn't in
use).

> This proves why this topic is always contentious. It's too easy to
> blindly convert drivers to get the clk and leave it enabled forever and
> then they never use power management. The janitors win and nobody else.

If the janitors win and nobody else looses anything, this is fine for
me. And if in the future someone turns up who cares enough to improve
the converted drivers to a more efficient clock usage, they will
probably not stop their efforts just because then the driver uses
devm_clk_get_enabled.

> Is there some way to avoid that trap? Maybe through some combination of
> a device PM function that indicates the driver has no runtime PM
> callbacks (pm_runtime_no_callbacks() perhaps?) and
> devm_clk_get_enabled() checking for that and returning the clk only when
> that call has been made (i.e. pm_runtime_has_no_callbacks())? This
> approach would fail to catch the case where system wide suspend/resume
> could turn the clk off but the driver doesn't do it. I'm not sure how
> much we care about that case though.
> 
> > As my focus is limited to (mostly) drivers/pwm and I already have quite
> > some other patch quests on my list:
> 
> Don't we all? :)

Might be. The patches in your queue are however not a reason to drop my
efforts to make my queue shorter :-P

Best regards
Uwe

Mark Brown Sept. 14, 2021, 1:52 p.m. UTC | #29

On Tue, Sep 14, 2021 at 03:22:56PM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 05, 2021 at 05:26:16PM -0700, Stephen Boyd wrote:

> > This proves why this topic is always contentious. It's too easy to
> > blindly convert drivers to get the clk and leave it enabled forever and
> > then they never use power management. The janitors win and nobody else.

> If the janitors win and nobody else looses anything, this is fine for
> me. And if in the future someone turns up who cares enough to improve
> the converted drivers to a more efficient clock usage, they will
> probably not stop their efforts just because then the driver uses
> devm_clk_get_enabled.

The patterns that concern me are people either blindly converting to
devm without checking for other usage and causing problems as a result
(some of the janitorial stuff is done very mechanically) or thinking
it's important to keep devm_ used (or not thinking through the
interaction) and trying to do that while doing something more active.