All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Anderson <dianders@chromium.org>
To: Heiko Stuebner <heiko@sntech.de>
Cc: linux-rockchip@lists.infradead.org, zhangqing@rock-chips.com,
	wxt@rock-chips.com, zhengxing@rock-chips.com,
	dbasehore@chromium.org, Douglas Anderson <dianders@chromium.org>,
	shawn.lin@rock-chips.com, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: [PATCH] soc: rockchip: power-domain: Don't (incorrectly) set rk3399 up/down counts
Date: Thu, 18 Aug 2016 11:56:01 -0700	[thread overview]
Message-ID: <1471546561-4459-1-git-send-email-dianders@chromium.org> (raw)

On rk3288 it was important that powerdown and powerup counts for the
CPU/GPU in the kernel because:
* The power on default was crazy long.
* We couldn't rely on the firmware to set this up because really this
  wasn't the firmware's job--the kernel was the only one that really
  cared about bringing up / down CPUs and the GPU and doing suspend /
  resume (which involves bringing up / down CPUs).

On newer ARM systems (like rk3399) ARM Trusted Firmware is in charge of
bringing up and down the CPUs and it really should be in charge of
setting all these counts right.  After all ATF is in charge of suspend /
resume and CPU up / down.  Let's get out of the way and let ATF do its
job.

A few other motivations for doing this:
* Depending on another configuration (PMU_24M_EN_CFG) these counts can
  be either in 24M or 32k cycles.  Thus, though ATF isn't really so
  involved in bringing up the GPU, ATF should probably manage the counts
  for everything so it can also manage the 24M / 32k choice.
* It turns out that (right now) 24M mode is broken on rk3399 and not
  being used.  That means that the count the kernel was programming
  in (24) was not 1 us (which it seems was intended) but was actually
  .75 ms
* On rk3399 there are actually 2 separate registers for setting CPU
  up/down time plus 1 register for GPU up/down time.  The curent kernel
  code actually was putting the register for the "little" cores in the
  "CPU" slot and the register for the "big" cores in the "GPU" slot.  It
  was never initting the GPU counts.

Note: this change assumes that ATF will actually set these values at
boot, as I'm proposing in <http://crosreview.com/372381>.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/soc/rockchip/pm_domains.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 44842a205e4b..91c425d3e66c 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -583,10 +583,12 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	 * Configure power up and down transition delays for CORE
 	 * and GPU domains.
 	 */
-	rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
-				  pmu_info->core_power_transition_time);
-	rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
-				  pmu_info->gpu_power_transition_time);
+	if (pmu_info->core_power_transition_time)
+		rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
+					pmu_info->core_power_transition_time);
+	if (pmu_info->gpu_pwrcnt_offset)
+		rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
+					pmu_info->gpu_power_transition_time);
 
 	error = -ENODEV;
 
@@ -708,11 +710,7 @@ static const struct rockchip_pmu_info rk3399_pmu = {
 	.idle_offset = 0x64,
 	.ack_offset = 0x68,
 
-	.core_pwrcnt_offset = 0x9c,
-	.gpu_pwrcnt_offset = 0xa4,
-
-	.core_power_transition_time = 24,
-	.gpu_power_transition_time = 24,
+	/* ARM Trusted Firmware manages power transition times */
 
 	.num_domains = ARRAY_SIZE(rk3399_pm_domains),
 	.domain_info = rk3399_pm_domains,
-- 
2.8.0.rc3.226.g39d4020

WARNING: multiple messages have this Message-ID (diff)
From: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
To: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Cc: zhangqing-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	zhengxing-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org,
	Douglas Anderson
	<dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org
Subject: [PATCH] soc: rockchip: power-domain: Don't (incorrectly) set rk3399 up/down counts
Date: Thu, 18 Aug 2016 11:56:01 -0700	[thread overview]
Message-ID: <1471546561-4459-1-git-send-email-dianders@chromium.org> (raw)

On rk3288 it was important that powerdown and powerup counts for the
CPU/GPU in the kernel because:
* The power on default was crazy long.
* We couldn't rely on the firmware to set this up because really this
  wasn't the firmware's job--the kernel was the only one that really
  cared about bringing up / down CPUs and the GPU and doing suspend /
  resume (which involves bringing up / down CPUs).

On newer ARM systems (like rk3399) ARM Trusted Firmware is in charge of
bringing up and down the CPUs and it really should be in charge of
setting all these counts right.  After all ATF is in charge of suspend /
resume and CPU up / down.  Let's get out of the way and let ATF do its
job.

A few other motivations for doing this:
* Depending on another configuration (PMU_24M_EN_CFG) these counts can
  be either in 24M or 32k cycles.  Thus, though ATF isn't really so
  involved in bringing up the GPU, ATF should probably manage the counts
  for everything so it can also manage the 24M / 32k choice.
* It turns out that (right now) 24M mode is broken on rk3399 and not
  being used.  That means that the count the kernel was programming
  in (24) was not 1 us (which it seems was intended) but was actually
  .75 ms
* On rk3399 there are actually 2 separate registers for setting CPU
  up/down time plus 1 register for GPU up/down time.  The curent kernel
  code actually was putting the register for the "little" cores in the
  "CPU" slot and the register for the "big" cores in the "GPU" slot.  It
  was never initting the GPU counts.

Note: this change assumes that ATF will actually set these values at
boot, as I'm proposing in <http://crosreview.com/372381>.

Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/soc/rockchip/pm_domains.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 44842a205e4b..91c425d3e66c 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -583,10 +583,12 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	 * Configure power up and down transition delays for CORE
 	 * and GPU domains.
 	 */
-	rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
-				  pmu_info->core_power_transition_time);
-	rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
-				  pmu_info->gpu_power_transition_time);
+	if (pmu_info->core_power_transition_time)
+		rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
+					pmu_info->core_power_transition_time);
+	if (pmu_info->gpu_pwrcnt_offset)
+		rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
+					pmu_info->gpu_power_transition_time);
 
 	error = -ENODEV;
 
@@ -708,11 +710,7 @@ static const struct rockchip_pmu_info rk3399_pmu = {
 	.idle_offset = 0x64,
 	.ack_offset = 0x68,
 
-	.core_pwrcnt_offset = 0x9c,
-	.gpu_pwrcnt_offset = 0xa4,
-
-	.core_power_transition_time = 24,
-	.gpu_power_transition_time = 24,
+	/* ARM Trusted Firmware manages power transition times */
 
 	.num_domains = ARRAY_SIZE(rk3399_pm_domains),
 	.domain_info = rk3399_pm_domains,
-- 
2.8.0.rc3.226.g39d4020

WARNING: multiple messages have this Message-ID (diff)
From: dianders@chromium.org (Douglas Anderson)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] soc: rockchip: power-domain: Don't (incorrectly) set rk3399 up/down counts
Date: Thu, 18 Aug 2016 11:56:01 -0700	[thread overview]
Message-ID: <1471546561-4459-1-git-send-email-dianders@chromium.org> (raw)

On rk3288 it was important that powerdown and powerup counts for the
CPU/GPU in the kernel because:
* The power on default was crazy long.
* We couldn't rely on the firmware to set this up because really this
  wasn't the firmware's job--the kernel was the only one that really
  cared about bringing up / down CPUs and the GPU and doing suspend /
  resume (which involves bringing up / down CPUs).

On newer ARM systems (like rk3399) ARM Trusted Firmware is in charge of
bringing up and down the CPUs and it really should be in charge of
setting all these counts right.  After all ATF is in charge of suspend /
resume and CPU up / down.  Let's get out of the way and let ATF do its
job.

A few other motivations for doing this:
* Depending on another configuration (PMU_24M_EN_CFG) these counts can
  be either in 24M or 32k cycles.  Thus, though ATF isn't really so
  involved in bringing up the GPU, ATF should probably manage the counts
  for everything so it can also manage the 24M / 32k choice.
* It turns out that (right now) 24M mode is broken on rk3399 and not
  being used.  That means that the count the kernel was programming
  in (24) was not 1 us (which it seems was intended) but was actually
  .75 ms
* On rk3399 there are actually 2 separate registers for setting CPU
  up/down time plus 1 register for GPU up/down time.  The curent kernel
  code actually was putting the register for the "little" cores in the
  "CPU" slot and the register for the "big" cores in the "GPU" slot.  It
  was never initting the GPU counts.

Note: this change assumes that ATF will actually set these values at
boot, as I'm proposing in <http://crosreview.com/372381>.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/soc/rockchip/pm_domains.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 44842a205e4b..91c425d3e66c 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -583,10 +583,12 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	 * Configure power up and down transition delays for CORE
 	 * and GPU domains.
 	 */
-	rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
-				  pmu_info->core_power_transition_time);
-	rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
-				  pmu_info->gpu_power_transition_time);
+	if (pmu_info->core_power_transition_time)
+		rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
+					pmu_info->core_power_transition_time);
+	if (pmu_info->gpu_pwrcnt_offset)
+		rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
+					pmu_info->gpu_power_transition_time);
 
 	error = -ENODEV;
 
@@ -708,11 +710,7 @@ static const struct rockchip_pmu_info rk3399_pmu = {
 	.idle_offset = 0x64,
 	.ack_offset = 0x68,
 
-	.core_pwrcnt_offset = 0x9c,
-	.gpu_pwrcnt_offset = 0xa4,
-
-	.core_power_transition_time = 24,
-	.gpu_power_transition_time = 24,
+	/* ARM Trusted Firmware manages power transition times */
 
 	.num_domains = ARRAY_SIZE(rk3399_pm_domains),
 	.domain_info = rk3399_pm_domains,
-- 
2.8.0.rc3.226.g39d4020

             reply	other threads:[~2016-08-19  1:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-18 18:56 Douglas Anderson [this message]
2016-08-18 18:56 ` [PATCH] soc: rockchip: power-domain: Don't (incorrectly) set rk3399 up/down counts Douglas Anderson
2016-08-18 18:56 ` Douglas Anderson
2016-08-18 22:03 ` Heiko Stuebner
2016-08-18 22:03   ` Heiko Stuebner
2016-08-18 22:03   ` Heiko Stuebner
2016-08-18 22:08   ` Doug Anderson
2016-08-18 22:08     ` Doug Anderson
2016-08-18 22:08     ` Doug Anderson
2016-08-18 22:20     ` Heiko Stuebner
2016-08-18 22:20       ` Heiko Stuebner
2016-08-18 22:20       ` Heiko Stuebner
2016-09-26 20:18 ` Heiko Stuebner
2016-09-26 20:18   ` Heiko Stuebner

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=1471546561-4459-1-git-send-email-dianders@chromium.org \
    --to=dianders@chromium.org \
    --cc=dbasehore@chromium.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=shawn.lin@rock-chips.com \
    --cc=wxt@rock-chips.com \
    --cc=zhangqing@rock-chips.com \
    --cc=zhengxing@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: link
Be 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.