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: [RFC 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
References: <20191204093255.11849-1-psampat@linux.ibm.com>
 <20191204093255.11849-2-psampat@linux.ibm.com>
 <20200103232457.GH5556@oc0525413822.ibm.com>
From:   Pratik Sampat <psampat@linux.ibm.com>
Date:   Mon, 6 Jan 2020 15:16:51 +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: <20200103232457.GH5556@oc0525413822.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Message-Id: <75611f4d-51f8-33ac-dcd5-0016bebee183@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Ram,

Thank you for your reviewing the patches.

>> +/* 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  8
>> +#define PREFERENCE_MASK   0xff
>> +
>> +#define UNSUPPORTED         0x0
>> +#define SELF_RESTORE_STRICT 0x01
>> +#define SELF_SAVE_STRICT    0x10
>> +
>> +/*
>> + * Bitmask defining the kind of preferences available.
>> + * Note : The higher to lower preference is from LSB to MSB, with a shift of
>> + * 8 bits.
> A minor comment.
>
> Is there a reason why shift is 8?  Shift of 4 must be sufficient,
> and a mask of '0xf' should do. And SELF_SAVE_STRICT can be 0x2.
>
>
Yes, you're right! We could do away with using fewer bits here.

>> +/* Caching the lpcr & ptcr support to use later */
>> +static bool is_lpcr_self_save;
>> +static bool is_ptcr_self_save;
> I understand why you need to track the status of PTCR register.
> But its not clear, why LPCR register's save status need to be tracked?
>
Normally it does not but LPCR was previously unsupported in self-restore
and the kernel saved and restored its value in context. Now that we have
support for saving LPCR automatically I believe we leverage it and
make sure the kernel does not do redundant work.

>> +
>> +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,
>> +	},
>> +	{
>> +		.spr = SPRN_HMEER,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = SPRN_HID0,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = P9_STOP_SPR_MSR,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = P9_STOP_SPR_PSSCR,
>> +		.preferred_mode = PREFER_SAVE_RESTORE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = SPRN_HID1,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = SPRN_HID4,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	},
>> +	{
>> +		.spr = SPRN_HID5,
>> +		.preferred_mode = PREFER_RESTORE_SAVE,
>> +		.supported_mode = SELF_RESTORE_STRICT,
>> +	}
>> +};
> What determines the list of registers tracked in this table?
>
>
> .snip..
>
This list is of the SPRs of all the registers that the kernel is interested in
at wakeup. It has been refactored out as a list from what the kernel used
previously in the kernel.