From: JeffyChen <jeffy.chen@rock-chips.com> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Tomasz Figa <tfiga@chromium.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Dmitry Torokhov <dmitry.torokhov@gmail.com>, Robin Murphy <robin.murphy@arm.com>, Heiko Stuebner <heiko@sntech.de>, Caesar Wang <wxt@rock-chips.com>, Elaine Zhang <zhangqing@rock-chips.com>, "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>, Geert Uytterhoeven <geert+renesas@glider.be>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Ulf Hansson <ulf.hansson@linaro.org> Subject: Re: [PATCH] soc: rockchip: power-domain: remove PM clocks Date: Thu, 01 Mar 2018 17:09:25 +0800 [thread overview] Message-ID: <5A97C345.3060205@rock-chips.com> (raw) In-Reply-To: <CAMuHMdU9PFAXwo+1Z7Baw1fanst9yFKZ3ohoSTiQiMKTaYMVVw@mail.gmail.com> 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 > > >
WARNING: multiple messages have this Message-ID (diff)
From: jeffy.chen@rock-chips.com (JeffyChen) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] soc: rockchip: power-domain: remove PM clocks Date: Thu, 01 Mar 2018 17:09:25 +0800 [thread overview] Message-ID: <5A97C345.3060205@rock-chips.com> (raw) In-Reply-To: <CAMuHMdU9PFAXwo+1Z7Baw1fanst9yFKZ3ohoSTiQiMKTaYMVVw@mail.gmail.com> 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 at 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 > > >
next prev parent reply other threads:[~2018-03-01 9:09 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-28 11:11 [PATCH] soc: rockchip: power-domain: remove PM clocks Jeffy Chen 2018-02-28 11:11 ` Jeffy Chen 2018-02-28 11:59 ` Heiko Stübner 2018-02-28 11:59 ` Heiko Stübner 2018-02-28 12:17 ` Geert Uytterhoeven 2018-02-28 12:17 ` Geert Uytterhoeven 2018-02-28 12:29 ` Tomasz Figa 2018-02-28 12:29 ` Tomasz Figa 2018-02-28 12:32 ` Geert Uytterhoeven 2018-02-28 12:32 ` Geert Uytterhoeven 2018-02-28 12:49 ` Tomasz Figa 2018-02-28 12:49 ` Tomasz Figa 2018-02-28 13:11 ` Geert Uytterhoeven 2018-02-28 13:11 ` Geert Uytterhoeven 2018-02-28 14:07 ` Tomasz Figa 2018-02-28 14:07 ` Tomasz Figa 2018-03-01 3:40 ` JeffyChen 2018-03-01 3:40 ` JeffyChen 2018-03-01 8:33 ` Geert Uytterhoeven 2018-03-01 8:33 ` Geert Uytterhoeven 2018-03-01 9:09 ` JeffyChen [this message] 2018-03-01 9:09 ` JeffyChen 2018-03-01 10:18 ` Ulf Hansson 2018-03-01 10:18 ` Ulf Hansson 2018-03-01 10:37 ` Geert Uytterhoeven 2018-03-01 10:37 ` Geert Uytterhoeven 2018-03-01 11:22 ` Ulf Hansson 2018-03-01 11:22 ` Ulf Hansson 2018-03-01 11:54 ` Geert Uytterhoeven 2018-03-01 11:54 ` Geert Uytterhoeven 2018-02-28 12:36 ` JeffyChen 2018-02-28 12:36 ` JeffyChen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5A97C345.3060205@rock-chips.com \ --to=jeffy.chen@rock-chips.com \ --cc=dmitry.torokhov@gmail.com \ --cc=geert+renesas@glider.be \ --cc=geert@linux-m68k.org \ --cc=heiko@sntech.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=robin.murphy@arm.com \ --cc=tfiga@chromium.org \ --cc=ulf.hansson@linaro.org \ --cc=wxt@rock-chips.com \ --cc=zhangqing@rock-chips.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.