From mboxrd@z Thu Jan  1 00:00:00 1970
From: Suzuki.Poulose@arm.com (Suzuki K Poulose)
Date: Mon, 16 Oct 2017 17:47:16 +0100
Subject: [PATCH v3 16/28] arm64/sve: Probe SVE capabilities and usable
 vector lengths
In-Reply-To: <20171016164407.GS19485@e103592.cambridge.arm.com>
References: <1507660725-7986-1-git-send-email-Dave.Martin@arm.com>
 <1507660725-7986-17-git-send-email-Dave.Martin@arm.com>
 <a39354fb-9f21-47ff-2ad9-b5bcb04e9990@arm.com>
 <20171016154557.GR19485@e103592.cambridge.arm.com>
 <812b6d11-2458-6d5d-c490-3421f7d3afb3@arm.com>
 <20171016164407.GS19485@e103592.cambridge.arm.com>
Message-ID: <7854a2cc-d3b1-ac76-8d12-2aa5e1be7aca@arm.com>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

On 16/10/17 17:44, Dave Martin wrote:
> On Mon, Oct 16, 2017 at 05:27:59PM +0100, Suzuki K Poulose wrote:
>> On 16/10/17 16:46, Dave Martin wrote:
>>> On Thu, Oct 12, 2017 at 01:56:51PM +0100, Suzuki K Poulose wrote:
>>>> On 10/10/17 19:38, Dave Martin wrote:
> 
> [...]
> 
>>>>> @@ -670,6 +689,14 @@ void update_cpu_features(int cpu,
>>>>>   					info->reg_mvfr2, boot->reg_mvfr2);
>>>>>   	}
>>>>> +	if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) {
>>>>> +		taint |= check_update_ftr_reg(SYS_ZCR_EL1, cpu,
>>>>> +					info->reg_zcr, boot->reg_zcr);
>>>>> +
>>>>> +		if (!sys_caps_initialised)
>>>>> +			sve_update_vq_map();
>>>>> +	}
>>>>
>>>> nit: I am not sure if we should also check if the "current" sanitised value
>>>> of the id_aa64pfr0 also supports sve and skip the update if it isn't. The code
>>>> is as such fine without the check, its just that we can avoid computing the
>>>> map. It is in the CPU boot up path and hence is not performance critical.
>>>> So, either way we are fine.
>>>>
>>>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>
>>> I think I prefer to avoid adding extra code to optimise the "broken SoC
>>> design" case.
>>>
>>
>> Sure.
>>
>>> Maybe we could revisit this later if needed.
>>>
>>> Can you suggest some code?  Maybe the check is simpler than I think.
>>
>> Something like :
>>
>> if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_IDAA64PFR0)) &&
>>      id_aa64pfr0_sve(id_aa64pfr0)) {
>>      ...
>> }
>>
>> should be enough.
>>
>> Or even we could hack it to :
>>
>> if (id_aa64pfr0_sve(id_aa64pfr0 | read_sanitised_ftr_reg(SYS_IDAA64PFR0)))
>>
>> As I mentioned, the code as such is fine. Its just that we try to detect
>> if the SVE is already moot and skip the steps for this CPU.
> 
> How about the following, keeping the outer
> if(id_aa64pfr0_sve(int->reg_id_aa64pfr0)) from my current code:
> 
> -		if (!sys_caps_initialised)
> +		/* Probe vector lengths, unless we already gave up on SVE */
> +		if (id_aa64pfr0_sve(read_sanitised_ftr_reg(ID_AA64PFR0_SVE)) &&
> +		    !sys_caps_initialised)
> 			sve_update_vq_map();

Yep, that looks neater.

Cheers
Suzuki