Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Mon, 25 Nov 2019 21:43:33 +0100 | Subject | Re: [PATCH v1] clk: Add devm_clk_{prepare,enable,prepare_enable} |
| |
Hi Marc,
On Mon, Nov 25, 2019 at 3:11 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 25/11/2019 14:37, Russell King - ARM Linux admin wrote: > > On Mon, Nov 25, 2019 at 02:10:21PM +0100, Marc Gonzalez wrote: > >> On 25/11/2019 13:55, Russell King - ARM Linux admin wrote: > >>> It's also worth reading https://lore.kernel.org/patchwork/patch/755667/ > >>> and considering whether you really are using the clk_prepare() and > >>> clk_enable() APIs correctly. Wanting these devm functions suggests > >>> you aren't... > >> > >> In that older thread, you wrote: > >> > >>> If you take the view that trying to keep clocks disabled is a good way > >>> to save power, then you'd have the clk_prepare() or maybe > >>> clk_prepare_enable() in your run-time PM resume handler, or maybe even > >>> deeper in the driver... the original design goal of the clk API was to > >>> allow power saving and clock control. > >>> > >>> With that in mind, getting and enabling the clock together in the > >>> probe function didn't make sense. > >>> > >>> I feel that aspect has been somewhat lost, and people now regard much > >>> of the clk API as a bit of a probe-time nuisance. > >> > >> In the few drivers I've written, I call clk_prepare_enable() at probe. > > > > Right, so the clocks are enabled as soon as the device is probed, > > in other words at boot time. It remains enabled for as long as the > > device is bound to its driver, whether or not the device is actually > > being used. Every switching edge causes heat to be generated. Every > > switching edge causes energy to be wasted. > > > > That's fine if you have an infinite energy supply. That hasn't been > > discovered yet. > > > > Given the prevalence of technology, don't you think we should be > > doing as much as we possibly can to reduce the energy consumption > > of the devices we use? It may be peanuts per device, but at scale > > it all adds up. > > OK, I'm starting to see the bigger picture. > > (To provide some rationale for the patch, I think devm is a huge > improvement for probe error-handling, and I did not understand > why every driver must do manual error-handling when dealing with > clocks in probe.) > > I did envision kernel modules being loaded on an as-needed basis, > somewhat side-stepping the energy-waste issue you point out. > But I realize that such a use-case may be uncommon. (Especially > due to module auto-loading.) > > A few months ago, I was discussing a similar issue with GKH: > Consider a device with a "START" register. Basically, if we write 0, > the device turns itself off; if we write 1, it runs as configured. > > I was trying to start the device only when at least one user had > it "open". So I used reference counting, and started the device > on 0->1 open transitions, and stopped the device on 1->0 close > transitions. GKH told me that was the wrong way to do it, and IIRC > suggested to start the device in probe. > > I probably misunderstood Greg's suggestion. Where is the right place > to start/stop a device (or gate its clocks)?
In the device driver's Runtime PM callbacks? In the Power/Clock Domain Controller driver?
See drivers/base/power/domain.c:genpd_{start,stop}_dev(), and how/when it's called.
Embedded device driver writers typically care. Server device driver writes typically don't.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
| |