Re: [PATCH v2] soc: qcom: ice: Add HWKM v1 support for wrapped keys

[Neeraj Soni <neeraj.soni@oss.qualcomm.com>]

Hi Konrad,

On 10/30/2025 3:51 PM, Konrad Dybcio wrote:
> On 10/28/25 6:56 PM, Neeraj Soni wrote:
>> HWKM v1 and v2 differ slightly in wrapped key size and the bit fields for
>> certain status registers and operating mode (legacy or standard).
>>
>> Add support to select HWKM version based on the major and minor revisions.
>> Use this HWKM version to select wrapped key size and to configure the bit
>> fields in registers for operating modes and hardware status.
>>
>> Support for SCM calls for wrapped keys is being added in the TrustZone for
>> few SoCs with HWKM v1. Existing check of qcom_scm_has_wrapped_key_support()
>> API ensures that HWKM is used only if these SCM calls are supported in
>> TrustZone for that SoC.
>>
>> Signed-off-by: Neeraj Soni <neeraj.soni@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +	/* HWKM version v2 is present from ICE 3.2.1 onwards while version v1
>> +	 * is present only in ICE 3.2.0. Earlier ICE version don't have HWKM.
>> +	 */
>> +	if (major > 3 ||
>> +	   (major == 3 && (minor >= 3 || (minor == 2 && step >= 1))))
>> +		ice->hwkm_version = QCOM_ICE_HWKM_V2;
>> +	else if ((major == 3) && (minor == 2))
>> +		ice->hwkm_version = QCOM_ICE_HWKM_V1;
>> +	else
>> +		ice->hwkm_version = 0;
>> +
>>  	dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
>>  		 major, minor, step);
>>  
>> +	if (!ice->hwkm_version)
>> +		dev_info(dev, "QC Hardware Key Manager (HWKM) not supported\n");
> 
> This isn't an error condition (ICE vers 3.0.0..<3.2.0 simply don't have it),
> so I think it's fair not to print this possibly somewhat concerning message
Okay i will remove it in next revision.
> 
> [...]
> 
>>  static unsigned int translate_hwkm_slot(struct qcom_ice *ice, unsigned int slot)
>>  {
>> -	return slot * 2;
>> +	/* The slot offset depends upon HWKM version */
> 
> This comment doesn't provide any additional context (e.g. what is the
> underlying reason for V2 requiring a wider stride) - please provide
> some or remove it, as otherwise it reiterates the single LoC below
Okay i think it is better to remove it. I will do so in next revision.
> 
> Looks good otherwise
Thanks.
> 
> Konrad
> 
Regards
Neeraj