Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 27 Apr 2020 19:18:40 +0530
From:   Gautham R Shenoy <ego@linux.vnet.ibm.com>
To:     Abhishek Goel <huntbag@linux.vnet.ibm.com>
Cc:     linuxppc-dev@ozlabs.org, linux-kernel@vger.kernel.org,
        npiggin@gmail.com, ego@linux.vnet.ibm.com, svaidy@linux.ibm.com,
        mpe@ellerman.id.au, oohall@gmail.com, mikey@neuling.org,
        psampat@linux.ibm.com
Subject: Re: [RFC 3/3] powernv/cpuidle : Introduce capability for
 firmware-enabled-stop
Message-ID: <20200427134840.GE29708@in.ibm.com>
Reply-To: ego@linux.vnet.ibm.com
References: <20200427021027.114582-1-huntbag@linux.vnet.ibm.com>
 <20200427021027.114582-3-huntbag@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200427021027.114582-3-huntbag@linux.vnet.ibm.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Apr 26, 2020 at 09:10:27PM -0500, Abhishek Goel wrote:
> This patch introduces the capability for firmware to handle the stop
> states instead. A bit is set based on the discovery of the feature
> and correspondingly also the responsibility to handle the stop states.
> 
> If Kernel does not know about stop version, it can fallback to opal for
> idle stop support if firmware-stop-supported property is present.
> 
> Earlier this patch was posted as part of this series :
> https://lkml.org/lkml/2020/3/4/589
> 
> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
> Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
> ---
> 
>  v1->v2: This patch is newly added in this series.
> 
>  arch/powerpc/include/asm/processor.h  |  1 +
>  arch/powerpc/kernel/dt_cpu_ftrs.c     |  8 ++++++++
>  arch/powerpc/platforms/powernv/idle.c | 27 ++++++++++++++++-----------
>  3 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 66fa20476d0e..d5c6532b11ef 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -430,6 +430,7 @@ extern unsigned long cpuidle_disable;
>  enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
> 
>  #define STOP_ENABLE		0x00000001
> +#define FIRMWARE_STOP_ENABLE	0x00000010
> 
>  #define STOP_VERSION_P9       0x1
> 
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index db1a525e090d..ff4a87b79702 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -298,6 +298,13 @@ static int __init feat_enable_idle_stop(struct dt_cpu_feature *f)
>  	return 1;
>  }
> 
> +static int __init feat_enable_firmware_stop(struct dt_cpu_feature *f)
> +{
> +	stop_dep.cpuidle_prop |= FIRMWARE_STOP_ENABLE;
> +
> +	return 1;
> +}
> +
>  static int __init feat_enable_mmu_hash(struct dt_cpu_feature *f)
>  {
>  	u64 lpcr;
> @@ -592,6 +599,7 @@ static struct dt_cpu_feature_match __initdata
>  	{"idle-nap", feat_enable_idle_nap, 0},
>  	{"alignment-interrupt-dsisr", feat_enable_align_dsisr, 0},
>  	{"idle-stop", feat_enable_idle_stop, 0},
> +	{"firmware-stop-supported", feat_enable_firmware_stop, 0},
>  	{"machine-check-power8", feat_enable_mce_power8, 0},
>  	{"performance-monitor-power8", feat_enable_pmu_power8, 0},
>  	{"data-stream-control-register", feat_enable_dscr, CPU_FTR_DSCR},
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 538f0842ac3f..0de5de81902e 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -633,16 +633,6 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
>  	unsigned long mmcr0 = 0;
>  	struct p9_sprs sprs = {}; /* avoid false used-uninitialised */
>  	bool sprs_saved = false;
> -	int rc = 0;
> -
> -	/*
> -	 * Kernel takes decision whether to make OPAL call or not. This logic
> -	 * will be combined with the logic for BE opal to take decision.
> -	 */
> -	if (firmware_stop_supported) {
> -		rc = opal_cpu_idle(cpu_to_be64(__pa(&srr1)), (uint64_t) psscr);
> -		goto out;
> -	}
> 
>  	if (!(psscr & (PSSCR_EC|PSSCR_ESL))) {
>  		/* EC=ESL=0 case */
> @@ -835,6 +825,19 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
>  	return srr1;
>  }
> 
> +static unsigned long power9_firmware_idle_stop(unsigned long psscr, bool mmu_on)
> +{
> +	unsigned long srr1;
> +	int rc;
> +
> +	rc = opal_cpu_idle(cpu_to_be64(__pa(&srr1)), (uint64_t) psscr);
> +
> +	if (mmu_on)
> +		mtmsr(MSR_KERNEL);
> +	return srr1;
> +
> +}
> +
>  #ifdef CONFIG_HOTPLUG_CPU
>  static unsigned long power9_offline_stop(unsigned long psscr)
>  {
> @@ -1394,9 +1397,11 @@ static int __init pnv_init_idle_states(void)
>  	    !(stop_dep.cpuidle_prop & STOP_ENABLE))
>  		goto out;
> 
> -	/* Check for supported version in kernel */
> +	/* Check for supported version in kernel or fallback to kernel*/
>  	if (stop_dep.stop_version & STOP_VERSION_P9) {
>  		stop_dep.idle_stop = power9_idle_stop;
> +	} else if (stop_dep.cpuidle_prop & FIRMWARE_STOP_ENABLE) {
> +		stop_dep.idle_stop = power9_firmware_idle_stop;

Ok, so in this patch you first check if the "idle-stop" feature is
available. Only otherwise you fallback to the OPAL based cpuidle
driver.

This looks ok to me.


>  	} else {
>  		stop_dep.idle_stop = NULL;
>  		goto out;
> -- 
> 2.17.1
> 

--
Thanks and Regards
gautham.