| Message ID | 1471546561-4459-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Doug, Am Donnerstag, 18. August 2016, 11:56:01 CEST schrieb Douglas Anderson: > On rk3288 it was important that powerdown and powerup counts for the > CPU/GPU in the kernel because: somehow this sentence seems to miss some verb or so :-) > * 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>. I'd hope to see a link to an ATF github pull request here :-) But I guess that simply needs some more discussion on your side. > Signed-off-by: Douglas Anderson <dianders@chromium.org> change itself looks good to me. So I guess we'll just need to wait for the counterpart to land in the ATF or do you know if the poweron-defaults are somewhat sane? Heiko
Hi, On Thu, Aug 18, 2016 at 3:03 PM, Heiko Stuebner <heiko@sntech.de> wrote: > Hi Doug, > > Am Donnerstag, 18. August 2016, 11:56:01 CEST schrieb Douglas Anderson: >> On rk3288 it was important that powerdown and powerup counts for the >> CPU/GPU in the kernel because: > > somehow this sentence seems to miss some verb or so :-) Sigh. I guess I can't type. On rk3288 it was important that powerdown and powerup counts for the CPU/GPU be set 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>. > > I'd hope to see a link to an ATF github pull request here :-) > But I guess that simply needs some more discussion on your side. Caesar is going to get confirmation that the patch is OK then I think he'll work on the ATF pull request. Once done we can update the link here? >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > change itself looks good to me. > > So I guess we'll just need to wait for the counterpart to land in the ATF or > do you know if the poweron-defaults are somewhat sane? Power on defaults are crappy (750 ms to turn on/off a CPU), so non-ideal. Probably best to wait for ATF to land. -Doug
Am Donnerstag, 18. August 2016, 15:08:12 CEST schrieb Doug Anderson: > Hi, > > On Thu, Aug 18, 2016 at 3:03 PM, Heiko Stuebner <heiko@sntech.de> wrote: > > Hi Doug, > > > > Am Donnerstag, 18. August 2016, 11:56:01 CEST schrieb Douglas Anderson: > >> On rk3288 it was important that powerdown and powerup counts for the > > > >> CPU/GPU in the kernel because: > > somehow this sentence seems to miss some verb or so :-) > > Sigh. I guess I can't type. > > On rk3288 it was important that powerdown and powerup counts for the > > CPU/GPU be set 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>. > > > > I'd hope to see a link to an ATF github pull request here :-) > > But I guess that simply needs some more discussion on your side. > > Caesar is going to get confirmation that the patch is OK then I think > he'll work on the ATF pull request. Once done we can update the link > here? yep and I can then also update your sentence above :-) > >> Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > change itself looks good to me. > > > > So I guess we'll just need to wait for the counterpart to land in the ATF > > or do you know if the poweron-defaults are somewhat sane? > > Power on defaults are crappy (750 ms to turn on/off a CPU), so > non-ideal. Probably best to wait for ATF to land. ok, so we'll wait. As I might miss it when the other side gets merged into the ATF, can you ping here once that is done please? Heiko
Am Donnerstag, 18. August 2016, 11:56:01 CEST schrieb Douglas Anderson: > 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> applied to my drivers branch for 4.10 Thanks Heiko
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,
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(-)