| Message ID | 20180228111113.13639-1-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeffy, Am Mittwoch, 28. Februar 2018, 12:11:13 CET schrieb Jeffy Chen: > Currently we are adding all of the attached devices' clocks as pm clocks > and enable them when powering on the power domain. > > This seems unnecessary, because those clocks are already controlled in > the devices' drivers with better error handling. > > Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> Looks ok to me and does not seem to interfere with the synchronous reset that still stays intact. @Ceasar: any objections? Heiko
Hi Jeffy, On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: > Currently we are adding all of the attached devices' clocks as pm clocks > and enable them when powering on the power domain. > > This seems unnecessary, because those clocks are already controlled in > the devices' drivers with better error handling. > > Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> Thanks for your patch! Just wondering: so you prefer to handle the clocks explicitly in all drivers, instead of delegating this task to Runtime PM? 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
Hi Geert, On Wed, Feb 28, 2018 at 9:17 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Jeffy, > > On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: >> Currently we are adding all of the attached devices' clocks as pm clocks >> and enable them when powering on the power domain. >> >> This seems unnecessary, because those clocks are already controlled in >> the devices' drivers with better error handling. >> >> Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > Thanks for your patch! > > Just wondering: so you prefer to handle the clocks explicitly in all drivers, > instead of delegating this task to Runtime PM? Is it already possible to gate clocks immediately when the device idles, but defer turning the power domain off until time long enough to cover the power down and up latency elapses? Also, how about systems where runtime PM is disabled? I think that's one of the reasons we control the clocks explicitly in the drivers anyway. Best regards, Tomasz
Hi Tomasz, On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa <tfiga@chromium.org> wrote: > On Wed, Feb 28, 2018 at 9:17 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: >>> Currently we are adding all of the attached devices' clocks as pm clocks >>> and enable them when powering on the power domain. >>> >>> This seems unnecessary, because those clocks are already controlled in >>> the devices' drivers with better error handling. >>> >>> Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). >>> >>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> >> Thanks for your patch! >> >> Just wondering: so you prefer to handle the clocks explicitly in all drivers, >> instead of delegating this task to Runtime PM? > > Is it already possible to gate clocks immediately when the device > idles, but defer turning the power domain off until time long enough > to cover the power down and up latency elapses? I'm not 100% sure. Note that clocks are turned off when the device idles, while powering down the power domain requires all devices in the domain to be idle. > Also, how about systems where runtime PM is disabled? I think that's > one of the reasons we control the clocks explicitly in the drivers > anyway. On many platforms, Runtime PM is always enabled. 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
Hi Geert, Thanks for you reply. On 02/28/2018 08:17 PM, Geert Uytterhoeven wrote: > Hi Jeffy, > > On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: >> Currently we are adding all of the attached devices' clocks as pm clocks >> and enable them when powering on the power domain. >> >> This seems unnecessary, because those clocks are already controlled in >> the devices' drivers with better error handling. >> >> Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > > Thanks for your patch! > > Just wondering: so you prefer to handle the clocks explicitly in all drivers, > instead of delegating this task to Runtime PM? hmmm, i think we should control PM clks here, but not all clocks...at least some of the clocks are not required to be enabled while pd power on(waste power?). and seems the drivers might have better control for error handling(decide to ignore or fail to probe or else). also Runtime PM seems optional(could be disabled in config), and some devices(or even chips) don't have PM. > > 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 > > >
On Wed, Feb 28, 2018 at 9:32 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Tomasz, > > On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa <tfiga@chromium.org> wrote: >> On Wed, Feb 28, 2018 at 9:17 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Wed, Feb 28, 2018 at 12:11 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote: >>>> Currently we are adding all of the attached devices' clocks as pm clocks >>>> and enable them when powering on the power domain. >>>> >>>> This seems unnecessary, because those clocks are already controlled in >>>> the devices' drivers with better error handling. >>>> >>>> Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). >>>> >>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>> >>> Thanks for your patch! >>> >>> Just wondering: so you prefer to handle the clocks explicitly in all drivers, >>> instead of delegating this task to Runtime PM? >> >> Is it already possible to gate clocks immediately when the device >> idles, but defer turning the power domain off until time long enough >> to cover the power down and up latency elapses? > > I'm not 100% sure. > Note that clocks are turned off when the device idles, while powering down > the power domain requires all devices in the domain to be idle. I remember seeing some ongoing work for multiple power states of PM domains on LWN, which could possibly solve it. I guess it's not done yet. Also, for Rockchip SoC, the typical setup is one device per domain, +/- some auxiliary devices such as IOMMU, which would have the same runtime PM state as the master device. (It isn't implemented yet, but Jeffy posted patches for using device links some time ago.) > >> Also, how about systems where runtime PM is disabled? I think that's >> one of the reasons we control the clocks explicitly in the drivers >> anyway. > > On many platforms, Runtime PM is always enabled. Can we make such assumption? If so, could we just make an explicit "select PM_RUNTIME" in Kconfig of the SoC? Best regards, Tomasz
Hi Tomasz, On Wed, Feb 28, 2018 at 1:49 PM, Tomasz Figa <tfiga@chromium.org> wrote: > On Wed, Feb 28, 2018 at 9:32 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa <tfiga@chromium.org> wrote: >>> Also, how about systems where runtime PM is disabled? I think that's >>> one of the reasons we control the clocks explicitly in the drivers >>> anyway. >> >> On many platforms, Runtime PM is always enabled. > > Can we make such assumption? If so, could we just make an explicit > "select PM_RUNTIME" in Kconfig of the SoC? Note that the PM_RUNTIME symbol was removed in commit 464ed18ebdb6236f ("PM: Eliminate CONFIG_PM_RUNTIME"), in favor of plain PM. The following already select PM unconditionally: - ARCH_OMAP2PLUS_TYPICAL - ARCH_RENESAS (except EMEV2) - ARCH_TEGRA - ARCH_VEXPRESS 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
On Wed, Feb 28, 2018 at 10:11 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Tomasz, > > On Wed, Feb 28, 2018 at 1:49 PM, Tomasz Figa <tfiga@chromium.org> wrote: >> On Wed, Feb 28, 2018 at 9:32 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa <tfiga@chromium.org> wrote: >>>> Also, how about systems where runtime PM is disabled? I think that's >>>> one of the reasons we control the clocks explicitly in the drivers >>>> anyway. >>> >>> On many platforms, Runtime PM is always enabled. >> >> Can we make such assumption? If so, could we just make an explicit >> "select PM_RUNTIME" in Kconfig of the SoC? > > Note that the PM_RUNTIME symbol was removed in commit 464ed18ebdb6236f > ("PM: Eliminate CONFIG_PM_RUNTIME"), in favor of plain PM. > > The following already select PM unconditionally: > - ARCH_OMAP2PLUS_TYPICAL > - ARCH_RENESAS (except EMEV2) > - ARCH_TEGRA > - ARCH_VEXPRESS Thanks! Sounds like we might be able to simplify a lot of things with doing the same for Rockchip. Best regards, Tomasz
Hi guys,
if i'm reading the code right, the PM clk means:
1/ the clocks which would be enabled while power on
2/ these clocks are optional, it's ok if anything wrong with them
3/ controlled by pm_domain(or USE_PM_CLK_RUNTIME_OPS & pm_clk_add_notifier)
and currently we're adding all clocks of the attached device as PM clk
in rockchip PM domain driver, which seems wrong. because we might have
these kinds of clocks:
1/ critical, should block power on if anything wrong with it(failed to
get/ prepare/ enable)
2/ optional, could ignore it if anything wrong
3/ only required in some special cases, for example register r/w, and
doesn't need to stay enabled while power on
so maybe we can:
1/ let the device(dts) or driver decide which clock is PM clk, and add
it using *pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are
pm clk)
2/ add support for critical PM clk, which would return error to the
driver if anything wrong
3/ make sure PM clk always be controlled(otherwise it might be
unexpected disabled by other clocks under the same clk parent?):
a) make sure Runtime PM is always enabled. and as discussed, we can
select PM in ARCH_ROCKCHIP
b) make sure the device has a PM domain to control PM clk:
select ROCKCHIP_PM_DOMAINS for ARCH_ROCKCHIP(that would also select
PM_GENERIC_DOMAINS)
check dev->pm_domain before hand over PM clk, since we only care about
EPROBE_DEFER when attach PM domain:
ret = dev_pm_domain_attach(_dev, true);
if (ret != -EPROBE_DEFER) {
if (drv->probe) {
ret = drv->probe(dev);
or
c) make pm_clk_suspend/pm_clk_resume part of the generic PM Runtime
flow(even without a PM domain)
On 02/28/2018 10:07 PM, Tomasz Figa wrote:
> On Wed, Feb 28, 2018 at 10:11 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> Hi Tomasz,
>>
>> On Wed, Feb 28, 2018 at 1:49 PM, Tomasz Figa <tfiga@chromium.org> wrote:
>>> On Wed, Feb 28, 2018 at 9:32 PM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>> On Wed, Feb 28, 2018 at 1:29 PM, Tomasz Figa <tfiga@chromium.org> wrote:
>>>>> Also, how about systems where runtime PM is disabled? I think that's
>>>>> one of the reasons we control the clocks explicitly in the drivers
>>>>> anyway.
>>>>
>>>> On many platforms, Runtime PM is always enabled.
>>>
>>> Can we make such assumption? If so, could we just make an explicit
>>> "select PM_RUNTIME" in Kconfig of the SoC?
>>
>> Note that the PM_RUNTIME symbol was removed in commit 464ed18ebdb6236f
>> ("PM: Eliminate CONFIG_PM_RUNTIME"), in favor of plain PM.
>>
>> The following already select PM unconditionally:
>> - ARCH_OMAP2PLUS_TYPICAL
>> - ARCH_RENESAS (except EMEV2)
>> - ARCH_TEGRA
>> - ARCH_VEXPRESS
>
> Thanks! Sounds like we might be able to simplify a lot of things with
> doing the same for Rockchip.
>
> Best regards,
> Tomasz
>
>
>
Hi Jeffy, On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote: > if i'm reading the code right, the PM clk means: > 1/ the clocks which would be enabled while power on > 2/ these clocks are optional, it's ok if anything wrong with them > 3/ controlled by pm_domain(or USE_PM_CLK_RUNTIME_OPS & pm_clk_add_notifier) > > and currently we're adding all clocks of the attached device as PM clk in > rockchip PM domain driver, which seems wrong. because we might have these > kinds of clocks: > 1/ critical, should block power on if anything wrong with it(failed to get/ > prepare/ enable) > 2/ optional, could ignore it if anything wrong > 3/ only required in some special cases, for example register r/w, and > doesn't need to stay enabled while power on > > so maybe we can: > 1/ let the device(dts) or driver decide which clock is PM clk, and add it > using *pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are pm clk) > > 2/ add support for critical PM clk, which would return error to the driver > if anything wrong > > 3/ make sure PM clk always be controlled(otherwise it might be unexpected > disabled by other clocks under the same clk parent?): > a) make sure Runtime PM is always enabled. and as discussed, we can select > PM in ARCH_ROCKCHIP On Renesas SoCs, we only add the device's module clock with pm_clk_add(). Drivers that don't care about properties of the module clock just call pm_runtime_*(). That way the same driver works on different SoCs using the same device, with and without power and/or clock domains. Drivers that care about properties of the module clock (mainly frequency) can still use the clk_*() API for that. Other (optional) clocks must be handled by the device driver itself. 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
Hi Geert, Thanks for your reply. On 03/01/2018 04:33 PM, Geert Uytterhoeven wrote: >> >so maybe we can: >> >1/ let the device(dts) or driver decide which clock is PM clk, and add it >> >using*pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are pm clk) >> > >> >2/ add support for critical PM clk, which would return error to the driver >> >if anything wrong >> > >> >3/ make sure PM clk always be controlled(otherwise it might be unexpected >> >disabled by other clocks under the same clk parent?): >> > a) make sure Runtime PM is always enabled. and as discussed, we can select >> >PM in ARCH_ROCKCHIP > On Renesas SoCs, we only add the device's module clock with pm_clk_add(). > Drivers that don't care about properties of the module clock just call > pm_runtime_*(). That way the same driver works on different SoCs using > the same device, with and without power and/or clock domains. well, i think we may need to check: 1/ so the driver might not know is the clock really enabled(currently): static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) { if (!ce->clk) ce->clk = clk_get(dev, ce->con_id); if (IS_ERR(ce->clk)) { ce->status = PCE_STATUS_ERROR; static inline void __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce) { int ret; if (ce->status < PCE_STATUS_ERROR) { ret = clk_enable(ce->clk); if (!ret) ce->status = PCE_STATUS_ENABLED; it seems the status is private. 2/ the pm_clk_resume/suspend seems only be called in the domain's suspend/resume ops(or USE_PM_CLK_RUNTIME_OPS, or called directly in the drivers for example tegra-aconnect): # cgrep pm_clk_resume -w ./include/linux/pm_clock.h:50:extern int pm_clk_resume(struct device *dev); ./include/linux/pm_clock.h:83:#define pm_clk_resume NULL ./drivers/bus/tegra-aconnect.c:62: return pm_clk_resume(dev); ./drivers/dma/tegra210-adma.c:649: ret = pm_clk_resume(dev); ./drivers/base/power/clock_ops.c:421: * pm_clk_resume - Enable clocks in a device's PM clock list. ./drivers/base/power/clock_ops.c:424:int pm_clk_resume(struct device *dev) ./drivers/base/power/clock_ops.c:444:EXPORT_SYMBOL_GPL(pm_clk_resume); ./drivers/base/power/clock_ops.c:533: ret = pm_clk_resume(dev); ./drivers/base/power/domain.c:1685: genpd->dev_ops.start = pm_clk_resume; ./drivers/irqchip/irq-gic-pm.c:36: ret = pm_clk_resume(dev); so if the device doesn't have a power domain, the PM clks could be unmanaged right? by the way, the tegra drivers(tegra-aconnect/tegra210-adma) seems like good examples of using current PM clks... but anyway, force adding all clocks as PM clks in the rockchip power domain driver seems wrong... > > Drivers that care about properties of the module clock (mainly frequency) > can still use the clk_*() API for that. Other (optional) clocks must be > handled by the device driver itself. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 --geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > > >
+linux-pm Geert, Jeffry, Tomasz, Apologize for side-tracking the discussion. Just wanted to add a few comments, whatever it's worth to you. On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Jeffy, > > On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote: >> if i'm reading the code right, the PM clk means: >> 1/ the clocks which would be enabled while power on >> 2/ these clocks are optional, it's ok if anything wrong with them >> 3/ controlled by pm_domain(or USE_PM_CLK_RUNTIME_OPS & pm_clk_add_notifier) >> >> and currently we're adding all clocks of the attached device as PM clk in >> rockchip PM domain driver, which seems wrong. because we might have these >> kinds of clocks: >> 1/ critical, should block power on if anything wrong with it(failed to get/ >> prepare/ enable) >> 2/ optional, could ignore it if anything wrong >> 3/ only required in some special cases, for example register r/w, and >> doesn't need to stay enabled while power on >> >> so maybe we can: >> 1/ let the device(dts) or driver decide which clock is PM clk, and add it >> using *pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are pm clk) We have already tried adding DT binding for this. Those got correctly nacked, as this seems like a SW config and not HW config, at least in my opinion. >> >> 2/ add support for critical PM clk, which would return error to the driver >> if anything wrong >> >> 3/ make sure PM clk always be controlled(otherwise it might be unexpected >> disabled by other clocks under the same clk parent?): >> a) make sure Runtime PM is always enabled. and as discussed, we can select >> PM in ARCH_ROCKCHIP I am fine enabling PM for ARCH_ROCKCHIP, if needed. However, what I don't agree with in general, is to make a generic driver to rely on having CONFIG_PM to be set to be functional. That's to me, bad practice. I understand, this approach exists in drivers today. I assume it works, because those drivers are being used on SoCs which always has CONFIG_PM set. > > On Renesas SoCs, we only add the device's module clock with pm_clk_add(). > Drivers that don't care about properties of the module clock just call > pm_runtime_*(). That way the same driver works on different SoCs using > the same device, with and without power and/or clock domains. I understand your point and I accept your view. However, I think this is more a mindset of which way one want implement things. This has been discussed several times in the mailing list as well. Surely we can cope with SoC specific constraints in drivers as well, we already do that. In principle I think this boils done to comparing a centralized method, where the PM domain deals with clocks vs a decentralized method, where the driver deals with clocks. Both works, both have positive and negative consequences. In my experience for ARM SoCs, I have found that centralized method doesn't work well, when one need flexibility. For example, if there are strict constraints on the order of how to put device's PM resources (clocks, pinctrl, etc) in low power states. For example, to avoid clock glitches. Another problem with the PM clk is, more exactly with pm_clk_suspend|resume(), that those invokes only clk_enable|disable(). pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we don't know if we running in atomic context when those are executed. Potentially this means leaving the clocks ungated - all the time. I have though about how to fix the above, several times, but I always ends up with thinking that's it more easy, to let the driver deal with the clocks, as then the problem goes away. > > Drivers that care about properties of the module clock (mainly frequency) > can still use the clk_*() API for that. Other (optional) clocks must be > handled by the device driver itself. A comment on that; Before we the PM clk was introduced, we didn't have the clk_bulk_*() interface. To me, using clk_bulk_*() in drivers could help to simplify the code in regards to manage clocks (including SoC specific clocks) during runtime PM. Perhaps this could be an option to using PM clk, as it provides both flexibility and could manage SoC specific clocks. Kind regards Uffe
Hi Ulf, On Thu, Mar 1, 2018 at 11:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote: >>> 3/ make sure PM clk always be controlled(otherwise it might be unexpected >>> disabled by other clocks under the same clk parent?): >>> a) make sure Runtime PM is always enabled. and as discussed, we can select >>> PM in ARCH_ROCKCHIP > > I am fine enabling PM for ARCH_ROCKCHIP, if needed. > > However, what I don't agree with in general, is to make a generic > driver to rely on having CONFIG_PM to be set to be functional. That's > to me, bad practice. > > I understand, this approach exists in drivers today. I assume it > works, because those drivers are being used on SoCs which always has > CONFIG_PM set. I agree. This is mostly useful for drivers that are used on multiple SoCs, where the driver doesn't care about the clock (doesn't care about its properties), and where the clock is optional (i.e. either tied to an always-running bus clock, or to a controllable module clock, depending on SoC). The latter needs CONFIG_PM=y, but that's a platform property, controlled by selecting support for that specific SoC. >> On Renesas SoCs, we only add the device's module clock with pm_clk_add(). >> Drivers that don't care about properties of the module clock just call >> pm_runtime_*(). That way the same driver works on different SoCs using >> the same device, with and without power and/or clock domains. > > I understand your point and I accept your view. > > However, I think this is more a mindset of which way one want > implement things. This has been discussed several times in the mailing > list as well. > > Surely we can cope with SoC specific constraints in drivers as well, > we already do that. > > In principle I think this boils done to comparing a centralized > method, where the PM domain deals with clocks vs a decentralized > method, where the driver deals with clocks. Both works, both have > positive and negative consequences. Is the clock (are the clocks) used purely for power management of the device, or does it/do they serve another (independent) purpose? Note that unlike clocks, power areas cannot be controlled explicitly by the driver. That always has to be done through the Runtime PM API. > In my experience for ARM SoCs, I have found that centralized method > doesn't work well, when one need flexibility. For example, if there > are strict constraints on the order of how to put device's PM > resources (clocks, pinctrl, etc) in low power states. For example, to > avoid clock glitches. With multiple clocks used by a device, there's sometimes also a lack of understanding of the real clock hierarchy. If these multiple clocks need to be enabled/disabled in a specific order, perhaps they are not independent, and modelling the hierarchy correctly, and describing in DT the device as being connected to the leaf clock may solve the ordering issue. > Another problem with the PM clk is, more exactly with > pm_clk_suspend|resume(), that those invokes only clk_enable|disable(). > pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we > don't know if we running in atomic context when those are executed. > Potentially this means leaving the clocks ungated - all the time. > > I have though about how to fix the above, several times, but I always > ends up with thinking that's it more easy, to let the driver deal with > the clocks, as then the problem goes away. There's a similar issue with powering on/off power areas. How do device start/stop differ from power domain on/off? >> Drivers that care about properties of the module clock (mainly frequency) >> can still use the clk_*() API for that. Other (optional) clocks must be >> handled by the device driver itself. > > A comment on that; > > Before we the PM clk was introduced, we didn't have the clk_bulk_*() > interface. To me, using clk_bulk_*() in drivers could help to simplify > the code in regards to manage clocks (including SoC specific clocks) > during runtime PM. > > Perhaps this could be an option to using PM clk, as it provides both > flexibility and could manage SoC specific clocks. See my comment about multiple clocks above... 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
On 1 March 2018 at 11:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Ulf, > > On Thu, Mar 1, 2018 at 11:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote: >>>> 3/ make sure PM clk always be controlled(otherwise it might be unexpected >>>> disabled by other clocks under the same clk parent?): >>>> a) make sure Runtime PM is always enabled. and as discussed, we can select >>>> PM in ARCH_ROCKCHIP >> >> I am fine enabling PM for ARCH_ROCKCHIP, if needed. >> >> However, what I don't agree with in general, is to make a generic >> driver to rely on having CONFIG_PM to be set to be functional. That's >> to me, bad practice. >> >> I understand, this approach exists in drivers today. I assume it >> works, because those drivers are being used on SoCs which always has >> CONFIG_PM set. > > I agree. This is mostly useful for drivers that are used on multiple SoCs, > where the driver doesn't care about the clock (doesn't care about its > properties), and where the clock is optional (i.e. either tied to an > always-running bus clock, or to a controllable module clock, depending on SoC). > The latter needs CONFIG_PM=y, but that's a platform property, controlled > by selecting support for that specific SoC. > >>> On Renesas SoCs, we only add the device's module clock with pm_clk_add(). >>> Drivers that don't care about properties of the module clock just call >>> pm_runtime_*(). That way the same driver works on different SoCs using >>> the same device, with and without power and/or clock domains. >> >> I understand your point and I accept your view. >> >> However, I think this is more a mindset of which way one want >> implement things. This has been discussed several times in the mailing >> list as well. >> >> Surely we can cope with SoC specific constraints in drivers as well, >> we already do that. >> >> In principle I think this boils done to comparing a centralized >> method, where the PM domain deals with clocks vs a decentralized >> method, where the driver deals with clocks. Both works, both have >> positive and negative consequences. > > Is the clock (are the clocks) used purely for power management of the device, > or does it/do they serve another (independent) purpose? Well, I am not talking about one specific case. I guess what you are saying is that, devices may need different kind of clocks. Some can be power managed, some can't. That seems reasonable. In either way, the driver should be able to cope with both kinds of clocks. Right!? > > Note that unlike clocks, power areas cannot be controlled explicitly by the > driver. That always has to be done through the Runtime PM API. Yes, agree! At least until/if we get multiple power areas (PM domain) support for devices. Then this may change. However, that's a different story. :-) > >> In my experience for ARM SoCs, I have found that centralized method >> doesn't work well, when one need flexibility. For example, if there >> are strict constraints on the order of how to put device's PM >> resources (clocks, pinctrl, etc) in low power states. For example, to >> avoid clock glitches. > > With multiple clocks used by a device, there's sometimes also a lack of > understanding of the real clock hierarchy. If these multiple clocks need > to be enabled/disabled in a specific order, perhaps they are not > independent, and modelling the hierarchy correctly, and describing in DT > the device as being connected to the leaf clock may solve the ordering issue. Nope, this scenario I had in mind isn't about the clock hierarchy. Instead this is about other resources that the driver deals with during runtime PM. Pinctrl, regulators, device internals registers, etc. I have stumbled of cases, where the order dealing with these resources, simply need to be strict, thus it is better managed from the driver's runtime PM callbacks. What I am saying is that, if you have these kind of constraints, then having clocks being managed at the PM domain level via PM clk and other resources in the driver, simply isn't a good mix. > >> Another problem with the PM clk is, more exactly with >> pm_clk_suspend|resume(), that those invokes only clk_enable|disable(). >> pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we >> don't know if we running in atomic context when those are executed. >> Potentially this means leaving the clocks ungated - all the time. >> >> I have though about how to fix the above, several times, but I always >> ends up with thinking that's it more easy, to let the driver deal with >> the clocks, as then the problem goes away. > > There's a similar issue with powering on/off power areas. I don't follow. Can you elaborate? > How do device start/stop differ from power domain on/off? I guess what you refer to is that, genpd's ->start|stop() callbacks may be assigned to pm_clk_suspend|resume(), and those may be called from genps's runtime PM callbacks and genpd's system sleep callbacks. Yes, both cases suffers from the same problem as I describe above. Clocks may stay ungated - all the time. > >>> Drivers that care about properties of the module clock (mainly frequency) >>> can still use the clk_*() API for that. Other (optional) clocks must be >>> handled by the device driver itself. >> >> A comment on that; >> >> Before we the PM clk was introduced, we didn't have the clk_bulk_*() >> interface. To me, using clk_bulk_*() in drivers could help to simplify >> the code in regards to manage clocks (including SoC specific clocks) >> during runtime PM. >> >> Perhaps this could be an option to using PM clk, as it provides both >> flexibility and could manage SoC specific clocks. > > See my comment about multiple clocks above... > Kind regards Uffe
Hi Ulf, On Thu, Mar 1, 2018 at 12:22 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 1 March 2018 at 11:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Thu, Mar 1, 2018 at 11:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> Another problem with the PM clk is, more exactly with >>> pm_clk_suspend|resume(), that those invokes only clk_enable|disable(). >>> pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we >>> don't know if we running in atomic context when those are executed. >>> Potentially this means leaving the clocks ungated - all the time. >>> >>> I have though about how to fix the above, several times, but I always >>> ends up with thinking that's it more easy, to let the driver deal with >>> the clocks, as then the problem goes away. >> >> There's a similar issue with powering on/off power areas. > > I don't follow. Can you elaborate? I intended to comment on the atomic context (or not). But I think I was wrong, and PM domain drivers do busy loops instead of sleeps. 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
diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c index 5c342167b9db..39723ef6f7dc 100644 --- a/drivers/soc/rockchip/pm_domains.c +++ b/drivers/soc/rockchip/pm_domains.c @@ -11,7 +11,6 @@ #include <linux/io.h> #include <linux/iopoll.h> #include <linux/err.h> -#include <linux/pm_clock.h> #include <linux/pm_domain.h> #include <linux/of_address.h> #include <linux/of_platform.h> @@ -320,44 +319,6 @@ static int rockchip_pd_power_off(struct generic_pm_domain *domain) return rockchip_pd_power(pd, false); } -static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd, - struct device *dev) -{ - struct clk *clk; - int i; - int error; - - dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name); - - error = pm_clk_create(dev); - if (error) { - dev_err(dev, "pm_clk_create failed %d\n", error); - return error; - } - - i = 0; - while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) { - dev_dbg(dev, "adding clock '%pC' to list of PM clocks\n", clk); - error = pm_clk_add_clk(dev, clk); - if (error) { - dev_err(dev, "pm_clk_add_clk failed %d\n", error); - clk_put(clk); - pm_clk_destroy(dev); - return error; - } - } - - return 0; -} - -static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd, - struct device *dev) -{ - dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name); - - pm_clk_destroy(dev); -} - static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu, struct device_node *node) { @@ -476,9 +437,6 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu, pd->genpd.name = node->name; pd->genpd.power_off = rockchip_pd_power_off; pd->genpd.power_on = rockchip_pd_power_on; - pd->genpd.attach_dev = rockchip_pd_attach_dev; - pd->genpd.detach_dev = rockchip_pd_detach_dev; - pd->genpd.flags = GENPD_FLAG_PM_CLK; if (pd_info->active_wakeup) pd->genpd.flags |= GENPD_FLAG_ACTIVE_WAKEUP; pm_genpd_init(&pd->genpd, NULL, false);
Currently we are adding all of the attached devices' clocks as pm clocks and enable them when powering on the power domain. This seems unnecessary, because those clocks are already controlled in the devices' drivers with better error handling. Tested on my chromebook minnie(rk3288) and chromebook kevin(rk3399). Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- drivers/soc/rockchip/pm_domains.c | 42 --------------------------------------- 1 file changed, 42 deletions(-)