[PATCH] soc: rockchip: power-domain: remove PM clocks

[PATCH] soc: rockchip: power-domain: remove PM clocks

[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>
---

 drivers/soc/rockchip/pm_domains.c | 42 ---------------------------------------
 1 file changed, 42 deletions(-)

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);
-- 
2.11.0

[PATCH] soc: rockchip: power-domain: remove PM clocks

[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>
---

 drivers/soc/rockchip/pm_domains.c | 42 ---------------------------------------
 1 file changed, 42 deletions(-)

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);
-- 
2.11.0

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Heiko Stübner]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Heiko Stübner]

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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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 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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Tomasz Figa]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Tomasz Figa]

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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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 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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Tomasz Figa]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Tomasz Figa]

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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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 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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Tomasz Figa]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Tomasz Figa]

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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[JeffyChen]

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
>
>
>

[PATCH] soc: rockchip: power-domain: remove PM clocks

[JeffyChen]

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
>
>
>

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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 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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[JeffyChen]

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
>
>
>

[PATCH] soc: rockchip: power-domain: remove PM clocks

[JeffyChen]

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
>
>
>

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Ulf Hansson]

+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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Ulf Hansson]

+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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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 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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Ulf Hansson]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Ulf Hansson]

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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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

[PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

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 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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[JeffyChen]

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
>
>
>

[PATCH] soc: rockchip: power-domain: remove PM clocks

[JeffyChen]

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 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
>
>
>