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, [email protected] wrote:
> > On Fri, Jul 23, 2021 at 11:26:58AM +0300, Andy Shevchenko wrote:
> >> On Thursday, July 22, 2021, Wolfram Sang <[email protected]> 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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) ?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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 <[email protected]> (maintainer:CLK API)
[email protected] (open list:CLK API)
[email protected] (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).
On Sat, Jul 31, 2021 at 10:41 AM Stephen Boyd <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Sat, 31 Jul 2021 14:00:04 +0200
Uwe Kleine-K?nig <[email protected]> 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
>
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Aug 2, 2021 at 7:38 PM Russell King (Oracle)
<[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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)
> <[email protected]> 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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.
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.
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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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? :)
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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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.