From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751858AbdLYCWQ (ORCPT ); Sun, 24 Dec 2017 21:22:16 -0500 Received: from mail-pg0-f65.google.com ([74.125.83.65]:37435 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751178AbdLYCWN (ORCPT ); Sun, 24 Dec 2017 21:22:13 -0500 X-Google-Smtp-Source: ACJfBouj8sffWrf56x/hR3O9OiVHTgRoWvaegmvhLmj3pnJobAnX7WJwXG658lD2/gYR+01onKVu1g== Date: Mon, 25 Dec 2017 10:22:02 +0800 From: Leo Yan To: Vincent Guittot Cc: Wei Xu , Rob Herring , Mark Rutland , Catalin Marinas , Will Deacon , LAK , devicetree@vger.kernel.org, linux-kernel , Daniel Lezcano , Sudeep Holla , Soby Mathew , John Stultz , kevin.wangtao@hisilicon.com Subject: Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id Message-ID: <20171225022202.GB600@leoy-linaro> References: <1513069954-22827-1-git-send-email-leo.yan@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Vincent, [ + John, Kevin Wang ] On Fri, Dec 22, 2017 at 03:22:51PM +0100, Vincent Guittot wrote: > Hi Leo, > > Sorry for jumping late in the discussion but should we also remove > the NAP state from the property cpu-idle-states of the CPUs because > this state not supported by the platform at least for now and may be > not in a near future ? Thanks for bringing up this. I don't want to hide anything for patch discussion :) this patch is to resolve the PSCI parameter mismatching issue between kernel and ARM-TF and it's not used to resolve the bug for CPU_NAP, so I didn't mention the CPU_NAP malfunction issue to avoid complex discussion context. I want to keep CPU_NAP state and track bug for CPU_NAP fixing; if we remove this state, I suspect we might have no chance to enable it anymore. Finally this is up to Hisilicon colleague decision and if they have time to fix this. I will offline to check with Daniel and Kevin for this; and if we finally decide to remove it we can commit extra patch for this later, how about you think? > Then, I have another question regarding the update of the > psci-suspend-parameter. These changes implies an update of the psci > firmawre which means that we will now have 2 different firmware > version compatible with 2 different dt. > > Is there any way to check that the ATF on the board is the one that > compatible with the parameter with something like a version ? I > currently use the previous firmware which works fine with current > kernel and dt binding once the NAP state is removed from the table. > When moving on recent kernel, I will have to take care of updating the > firmware and if i need to go back on a previous kernel, i will have to > make sure that i have the right ATF version. This make a lot of chance > of having the wrong configuration AFAIK, we cannot distinguish the PSCI parameter by PSCI version or ARM-TF version number; alternatively one simple way for checking ARM-TF is we can get commit ID (e.g. 83df7ce) from the ARM-TF log; so any ARM-TF commit ID is newer than the patch fdae60b6ba27: "Hikey960: Change to use recommended power state id format" should apply this kernel patch. NOTICE: BL1: Booting BL31 NOTICE: BL31: v1.4(debug):v1.4-441-g83df7ce-dirty NOTICE: BL31: Built : 17:31:35, Dec 22 2017 BTW, I hope we can upgrade Linux kernel and ARM-TF to latest code base to avoid compatible issue; for Android offical releasing it uses the old PSCI parameters with Hisilicon legacy booting images, so they can work well, but if someone uses ARM-TF mainline code + Android kernel 4.4/4.9, there must have compatible issue. I am monitoring the integration ARM-TF/UEFI into Android on Hikey960, we need backport this patch onto Android kernel 4.4/4.9 ASAP after integration ARM-TF/UEFI. Thanks, Leo Yan > Regards, > Vincent > > On 12 December 2017 at 10:12, Leo Yan wrote: > > Thanks a lot for Vincent Guittot careful work to find bug for 'CPU_NAP' > > idle state. From ftrace log we can observe CA73 CPUs can be easily > > waken up from 'CPU_NAP' state but the 'waken up' CPUs doesn't handle > > anything and sleep again; so there have tons of trace events for CA73 > > CPUs entering and exiting idle state. > > > > On Hi3660 CA73 has retention state 'CPU_NAP' for CPU idle, this state we > > set its psci parameter as '0x0000001' and from this parameter it can > > calculate state id is 1. Unfortunately ARM trusted firmware (ARM-TF) > > takes 1 as a invalid value for state id, so the CPU cannot enter idle > > state and directly bail out to kernel. > > > > We want to create good practice for psci parameters platform definition, > > so review the psci specification. The spec "ARM Power State Coordination > > Interface - Platform Design Document (ARM DEN 0022D)" recommends state > > ID in chapter "6.5 Recommended StateID Encoding". The recommended power > > state IDs can be presented by below listed values; and it divides into > > three fields, every field can use 4 bits to present power states > > corresponding to core level, cluster level and system level: > > 0: Run > > 1: Standby > > 2: Retention > > 3: Powerdown > > > > This commit changes psci parameter to compliance with the suggested > > state ID in the doc. Except we change 'CPU_NAP' state psci parameter > > to '0x0000002', this commit also changes 'CPU_SLEEP' and 'CLUSTER_SLEEP' > > state parameters to '0x0010003' and '0x1010033' respectively. > > > > Credits to Daniel, Sudeep and Soby for suggestion and consolidation. > > > > Cc: Vincent Guittot > > Cc: Daniel Lezcano > > Cc: Sudeep Holla > > Cc: Soby Mathew > > Signed-off-by: Leo Yan > > --- > > arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > index ab0b95b..99d5a46 100644 > > --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi > > @@ -147,7 +147,7 @@ > > > > CPU_NAP: cpu-nap { > > compatible = "arm,idle-state"; > > - arm,psci-suspend-param = <0x0000001>; > > + arm,psci-suspend-param = <0x0000002>; > > entry-latency-us = <7>; > > exit-latency-us = <2>; > > min-residency-us = <15>; > > @@ -156,7 +156,7 @@ > > CPU_SLEEP: cpu-sleep { > > compatible = "arm,idle-state"; > > local-timer-stop; > > - arm,psci-suspend-param = <0x0010000>; > > + arm,psci-suspend-param = <0x0010003>; > > entry-latency-us = <40>; > > exit-latency-us = <70>; > > min-residency-us = <3000>; > > @@ -165,7 +165,7 @@ > > CLUSTER_SLEEP_0: cluster-sleep-0 { > > compatible = "arm,idle-state"; > > local-timer-stop; > > - arm,psci-suspend-param = <0x1010000>; > > + arm,psci-suspend-param = <0x1010033>; > > entry-latency-us = <500>; > > exit-latency-us = <5000>; > > min-residency-us = <20000>; > > @@ -174,7 +174,7 @@ > > CLUSTER_SLEEP_1: cluster-sleep-1 { > > compatible = "arm,idle-state"; > > local-timer-stop; > > - arm,psci-suspend-param = <0x1010000>; > > + arm,psci-suspend-param = <0x1010033>; > > entry-latency-us = <1000>; > > exit-latency-us = <5000>; > > min-residency-us = <20000>; > > -- > > 2.7.4 > >