Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RESEND PATCH v2 1/3] powerpc/powernv: Interface to define
 support and preference for a SPR
To:     Ram Pai <linuxram@us.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org,
        mpe@ellerman.id.au, svaidy@linux.ibm.com, ego@linux.vnet.ibm.com,
        pratik.sampat@in.ibm.com, pratik.r.sampat@gmail.com
References: <cover.1578886602.git.psampat@linux.ibm.com>
 <926baad3fd0bf0b01b0adf83c71f2f4f6e9cf1e7.1578886602.git.psampat@linux.ibm.com>
 <20200113074440.GC5419@oc0525413822.ibm.com>
From:   Pratik Sampat <psampat@linux.ibm.com>
Date:   Mon, 13 Jan 2020 15:19:02 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.3.1
MIME-Version: 1.0
In-Reply-To: <20200113074440.GC5419@oc0525413822.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Message-Id: <f3452bd6-4555-0fe1-f3d0-da3f7397012b@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for the review.
The support just signifies what is default. Self restore is known to be
supported by legacy systems.

I'll mention a comment saying that this can change when the system is
initialized.


On 13/01/20 1:14 pm, Ram Pai wrote:
> On Mon, Jan 13, 2020 at 09:15:07AM +0530, Pratik Rajesh Sampat wrote:
>> Define a bitmask interface to determine support for the Self Restore,
>> Self Save or both.
>>
>> Also define an interface to determine the preference of that SPR to
>> be strictly saved or restored or encapsulated with an order of preference.
>>
>> The preference bitmask is shown as below:
>> ----------------------------
>> |... | 2nd pref | 1st pref |
>> ----------------------------
>> MSB			  LSB
>>
>> The preference from higher to lower is from LSB to MSB with a shift of 8
>> bits.
>> Example:
>> Prefer self save first, if not available then prefer self
>> restore
>> The preference mask for this scenario will be seen as below.
>> ((SELF_RESTORE_STRICT << PREFERENCE_SHIFT) | SELF_SAVE_STRICT)
>> ---------------------------------
>> |... | Self restore | Self save |
>> ---------------------------------
>> MSB			        LSB
>>
>> Finally, declare a list of preferred SPRs which encapsulate the bitmaks
>> for preferred and supported with defaults of both being set to support
>> legacy firmware.
>>
>> This commit also implements using the above interface and retains the
>> legacy functionality of self restore.
>>
>> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/powernv/idle.c | 327 +++++++++++++++++++++-----
>>   1 file changed, 271 insertions(+), 56 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
>> index 78599bca66c2..2f328403b0dc 100644
>> --- a/arch/powerpc/platforms/powernv/idle.c
>> +++ b/arch/powerpc/platforms/powernv/idle.c
>> @@ -32,9 +32,106 @@
>>   #define P9_STOP_SPR_MSR 2000
>>   #define P9_STOP_SPR_PSSCR      855
>>
>> +/* Interface for the stop state supported and preference */
>> +#define SELF_RESTORE_TYPE    0
>> +#define SELF_SAVE_TYPE       1
>> +
>> +#define NR_PREFERENCES    2
>> +#define PREFERENCE_SHIFT  4
>> +#define PREFERENCE_MASK   0xf
>> +
>> +#define UNSUPPORTED         0x0
>> +#define SELF_RESTORE_STRICT 0x1
>> +#define SELF_SAVE_STRICT    0x2
>> +
>> +/*
>> + * Bitmask defining the kind of preferences available.
>> + * Note : The higher to lower preference is from LSB to MSB, with a shift of
>> + * 4 bits.
>> + * ----------------------------
>> + * |    | 2nd pref | 1st pref |
>> + * ----------------------------
>> + * MSB			      LSB
>> + */
>> +/* Prefer Restore if available, otherwise unsupported */
>> +#define PREFER_SELF_RESTORE_ONLY	SELF_RESTORE_STRICT
>> +/* Prefer Save if available, otherwise unsupported */
>> +#define PREFER_SELF_SAVE_ONLY		SELF_SAVE_STRICT
>> +/* Prefer Restore when available, otherwise prefer Save */
>> +#define PREFER_RESTORE_SAVE		((SELF_SAVE_STRICT << \
>> +					  PREFERENCE_SHIFT)\
>> +					  | SELF_RESTORE_STRICT)
>> +/* Prefer Save when available, otherwise prefer Restore*/
>> +#define PREFER_SAVE_RESTORE		((SELF_RESTORE_STRICT <<\
>> +					  PREFERENCE_SHIFT)\
>> +					  | SELF_SAVE_STRICT)
>>   static u32 supported_cpuidle_states;
>>   struct pnv_idle_states_t *pnv_idle_states;
>>   int nr_pnv_idle_states;
>> +/* Caching the lpcr & ptcr support to use later */
>> +static bool is_lpcr_self_save;
>> +static bool is_ptcr_self_save;
>> +
>> +struct preferred_sprs {
>> +	u64 spr;
>> +	u32 preferred_mode;
>> +	u32 supported_mode;
>> +};
>> +
>> +struct preferred_sprs preferred_sprs[] = {
>> +	{
>> +		.spr = SPRN_HSPRG0,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = SPRN_LPCR,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = SPRN_PTCR,
>> +		.preferred_mode = PREFER_SAVE_RESTORE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
> This confuses me.  It says SAVE takes precedence over RESTORE.
> and than it says it is strictly 'RESTORE' only.
>
> Maybe you should not initialize the 'supported_mode' ?
> or put a comment somewhere here, saying this value will be overwritten
> during system initialization?
>
>
> Otherwise the code looks correct.
>
> Reviewed-by: Ram Pai <linuxram@us.ibm.com>
> RP