From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hanjun Guo Subject: Re: [PATCH v7 1/8] arm64: kernel: refactor the CPU suspend API for retention states Date: Mon, 18 Aug 2014 15:47:51 +0800 Message-ID: <53F1AFA7.5040003@linaro.org> References: <1407945127-27554-1-git-send-email-lorenzo.pieralisi@arm.com> <1407945127-27554-2-git-send-email-lorenzo.pieralisi@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1407945127-27554-2-git-send-email-lorenzo.pieralisi@arm.com> Sender: linux-pm-owner@vger.kernel.org To: Lorenzo Pieralisi , linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org Cc: devicetree@vger.kernel.org, Mark Rutland , Sudeep Holla , Catalin Marinas , Charles Garcia Tobin , Nicolas Pitre , Rob Herring , Grant Likely , Peter De Schrijver , Santosh Shilimkar , Daniel Lezcano , Amit Kucheria , Vincent Guittot , Antti Miettinen , Stephen Boyd , Kevin Hilman , Sebastian Capella , Tomasz Figa , Mark Brown , Paul Walmsley , Chander Kashyap , Geoff Levand , Bartlomiej List-Id: devicetree@vger.kernel.org On 2014-8-13 23:52, Lorenzo Pieralisi wrote: > CPU suspend is the standard kernel interface to be used to enter > low-power states on ARM64 systems. Current cpu_suspend implementation > by default assumes that all low power states are losing the CPU context, > so the CPU registers must be saved and cleaned to DRAM upon state > entry. Furthermore, the current cpu_suspend() implementation assumes > that if the CPU suspend back-end method returns when called, this has > to be considered an error regardless of the return code (which can be > successful) since the CPU was not expected to return from a code path that > is different from cpu_resume code path - eg returning from the reset vector. > > All in all this means that the current API does not cope well with low-power > states that preserve the CPU context when entered (ie retention states), > since first of all the context is saved for nothing on state entry for > those states and a successful state entry can return as a normal function > return, which is considered an error by the current CPU suspend > implementation. > > This patch refactors the cpu_suspend() API so that it can be split in > two separate functionalities. The arm64 cpu_suspend API just provides > a wrapper around CPU suspend operation hook. A new function is > introduced (for architecture code use only) for states that require > context saving upon entry: > > __cpu_suspend(unsigned long arg, int (*fn)(unsigned long)) > > __cpu_suspend() saves the context on function entry and calls the > so called suspend finisher (ie fn) to complete the suspend operation. > The finisher is not expected to return, unless it fails in which case > the error is propagated back to the __cpu_suspend caller. > > The API refactoring results in the following pseudo code call sequence for a > suspending CPU, when triggered from a kernel subsystem: > > /* > * int cpu_suspend(unsigned long idx) > * @idx: idle state index > */ > { > -> cpu_suspend(idx) > |---> CPU operations suspend hook called, if present > |--> if (retention_state) > |--> direct suspend back-end call (eg PSCI suspend) > else > |--> __cpu_suspend(idx, &back_end_finisher); > } > > By refactoring the cpu_suspend API this way, the CPU operations back-end > has a chance to detect whether idle states require state saving or not > and can call the required suspend operations accordingly either through > simple function call or indirectly through __cpu_suspend() which carries out > state saving and suspend finisher dispatching to complete idle state entry. > > Signed-off-by: Lorenzo Pieralisi This patch is pretty fine to me, Reviewed-by: Hanjun Guo Thanks Hanjun From mboxrd@z Thu Jan 1 00:00:00 1970 From: hanjun.guo@linaro.org (Hanjun Guo) Date: Mon, 18 Aug 2014 15:47:51 +0800 Subject: [PATCH v7 1/8] arm64: kernel: refactor the CPU suspend API for retention states In-Reply-To: <1407945127-27554-2-git-send-email-lorenzo.pieralisi@arm.com> References: <1407945127-27554-1-git-send-email-lorenzo.pieralisi@arm.com> <1407945127-27554-2-git-send-email-lorenzo.pieralisi@arm.com> Message-ID: <53F1AFA7.5040003@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2014-8-13 23:52, Lorenzo Pieralisi wrote: > CPU suspend is the standard kernel interface to be used to enter > low-power states on ARM64 systems. Current cpu_suspend implementation > by default assumes that all low power states are losing the CPU context, > so the CPU registers must be saved and cleaned to DRAM upon state > entry. Furthermore, the current cpu_suspend() implementation assumes > that if the CPU suspend back-end method returns when called, this has > to be considered an error regardless of the return code (which can be > successful) since the CPU was not expected to return from a code path that > is different from cpu_resume code path - eg returning from the reset vector. > > All in all this means that the current API does not cope well with low-power > states that preserve the CPU context when entered (ie retention states), > since first of all the context is saved for nothing on state entry for > those states and a successful state entry can return as a normal function > return, which is considered an error by the current CPU suspend > implementation. > > This patch refactors the cpu_suspend() API so that it can be split in > two separate functionalities. The arm64 cpu_suspend API just provides > a wrapper around CPU suspend operation hook. A new function is > introduced (for architecture code use only) for states that require > context saving upon entry: > > __cpu_suspend(unsigned long arg, int (*fn)(unsigned long)) > > __cpu_suspend() saves the context on function entry and calls the > so called suspend finisher (ie fn) to complete the suspend operation. > The finisher is not expected to return, unless it fails in which case > the error is propagated back to the __cpu_suspend caller. > > The API refactoring results in the following pseudo code call sequence for a > suspending CPU, when triggered from a kernel subsystem: > > /* > * int cpu_suspend(unsigned long idx) > * @idx: idle state index > */ > { > -> cpu_suspend(idx) > |---> CPU operations suspend hook called, if present > |--> if (retention_state) > |--> direct suspend back-end call (eg PSCI suspend) > else > |--> __cpu_suspend(idx, &back_end_finisher); > } > > By refactoring the cpu_suspend API this way, the CPU operations back-end > has a chance to detect whether idle states require state saving or not > and can call the required suspend operations accordingly either through > simple function call or indirectly through __cpu_suspend() which carries out > state saving and suspend finisher dispatching to complete idle state entry. > > Signed-off-by: Lorenzo Pieralisi This patch is pretty fine to me, Reviewed-by: Hanjun Guo Thanks Hanjun