diff mbox series

[V7,5/5] firmware: qcom_scm: ipq5332: add support to pass metadata size

Message ID 20240820055618.267554-6-quic_gokulsri@quicinc.com (mailing list archive)
State New
Headers show
Series remove unnecessary q6 clocks | expand

Commit Message

Gokul Sriram P Aug. 20, 2024, 5:56 a.m. UTC 
From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>

IPQ5332 security software running under trustzone
requires metadata size. With V2 cmd, pass metadata
size as well.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
Changes in v7:
	- No changes.
	- Rebased on top of linux-next.

Changes in v6:
	- Rebased on linux-next

Changes in v5:
	- Rebased on linux-next

Changes in v4:
	- Rebased on linux-next

 drivers/firmware/qcom/qcom_scm.c | 8 ++++++++
 drivers/firmware/qcom/qcom_scm.h | 1 +
 2 files changed, 9 insertions(+)

Comments

Bjorn Andersson Oct. 23, 2024, 3:52 p.m. UTC | #1 
On Tue, Aug 20, 2024 at 11:26:18AM GMT, Gokul Sriram Palanisamy wrote:
> From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> 
> IPQ5332 security software running under trustzone
> requires metadata size. With V2 cmd, pass metadata
> size as well.

Documentation says commit messages should be wrapped at 75 characters,
not 50...

Please improve the second sentence here, "v2 cmd" is coming out of
nowhere. Say that there is a new command with a size parameter added.

Is this operation available on all targets, or is it IPQ-specific?


I don't see the relationship between this patch and the cover letter
subject "remove unnecessary q6 clocks". Should this have been send on
its own?

> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> ---
> Changes in v7:
> 	- No changes.
> 	- Rebased on top of linux-next.
> 
> Changes in v6:
> 	- Rebased on linux-next
> 
> Changes in v5:
> 	- Rebased on linux-next
> 
> Changes in v4:
> 	- Rebased on linux-next
> 
>  drivers/firmware/qcom/qcom_scm.c | 8 ++++++++
>  drivers/firmware/qcom/qcom_scm.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index e60bef68401c..aa559fd01932 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -607,6 +607,14 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>  
>  	desc.args[1] = mdata_phys;
>  
> +	if (__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
> +					 QCOM_SCM_PAS_INIT_IMAGE_V2)) {
> +		desc.cmd = QCOM_SCM_PAS_INIT_IMAGE_V2;
> +		desc.arginfo =
> +			QCOM_SCM_ARGS(3, QCOM_SCM_VAL, QCOM_SCM_RW, QCOM_SCM_VAL);
> +		desc.args[2] = size;
> +	}
> +

Please avoid default initialization and then conditionally overwrite
parts of the values. Make a clear:

if (v2 availble) {
	prepare v2 request;
} else {
	prepare v1 request;
}

Regards,
Bjorn

>  	ret = qcom_scm_call(__scm->dev, &desc, &res);
>  	qcom_scm_bw_disable();
>  
> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
> index 685b8f59e7a6..008b59cbad36 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -96,6 +96,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
>  
>  #define QCOM_SCM_SVC_PIL		0x02
>  #define QCOM_SCM_PIL_PAS_INIT_IMAGE	0x01
> +#define QCOM_SCM_PAS_INIT_IMAGE_V2	0x1a
>  #define QCOM_SCM_PIL_PAS_MEM_SETUP	0x02
>  #define QCOM_SCM_PIL_PAS_AUTH_AND_RESET	0x05
>  #define QCOM_SCM_PIL_PAS_SHUTDOWN	0x06
> -- 
> 2.34.1
>
Gokul Sriram P Nov. 28, 2024, 4:45 a.m. UTC | #2 
On 10/23/2024 9:22 PM, Bjorn Andersson wrote:
> On Tue, Aug 20, 2024 at 11:26:18AM GMT, Gokul Sriram Palanisamy wrote:
>> From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>
>> IPQ5332 security software running under trustzone
>> requires metadata size. With V2 cmd, pass metadata
>> size as well.
> Documentation says commit messages should be wrapped at 75 characters,
> not 50...
>
> Please improve the second sentence here, "v2 cmd" is coming out of
> nowhere. Say that there is a new command with a size parameter added.
>
> Is this operation available on all targets, or is it IPQ-specific?
>
>
> I don't see the relationship between this patch and the cover letter
> subject "remove unnecessary q6 clocks". Should this have been send on
> its own?
>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
>> ---
>> Changes in v7:
>> 	- No changes.
>> 	- Rebased on top of linux-next.
>>
>> Changes in v6:
>> 	- Rebased on linux-next
>>
>> Changes in v5:
>> 	- Rebased on linux-next
>>
>> Changes in v4:
>> 	- Rebased on linux-next
>>
>>   drivers/firmware/qcom/qcom_scm.c | 8 ++++++++
>>   drivers/firmware/qcom/qcom_scm.h | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index e60bef68401c..aa559fd01932 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -607,6 +607,14 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>>   
>>   	desc.args[1] = mdata_phys;
>>   
>> +	if (__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
>> +					 QCOM_SCM_PAS_INIT_IMAGE_V2)) {
>> +		desc.cmd = QCOM_SCM_PAS_INIT_IMAGE_V2;
>> +		desc.arginfo =
>> +			QCOM_SCM_ARGS(3, QCOM_SCM_VAL, QCOM_SCM_RW, QCOM_SCM_VAL);
>> +		desc.args[2] = size;
>> +	}
>> +
> Please avoid default initialization and then conditionally overwrite
> parts of the values. Make a clear:
>
> if (v2 availble) {
> 	prepare v2 request;
> } else {
> 	prepare v1 request;
> }
>
> Regards,
> Bjorn

  sure, will address. Thank you.

Regards,

Gokul

>>   	ret = qcom_scm_call(__scm->dev, &desc, &res);
>>   	qcom_scm_bw_disable();
>>   
>> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
>> index 685b8f59e7a6..008b59cbad36 100644
>> --- a/drivers/firmware/qcom/qcom_scm.h
>> +++ b/drivers/firmware/qcom/qcom_scm.h
>> @@ -96,6 +96,7 @@ struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
>>   
>>   #define QCOM_SCM_SVC_PIL		0x02
>>   #define QCOM_SCM_PIL_PAS_INIT_IMAGE	0x01
>> +#define QCOM_SCM_PAS_INIT_IMAGE_V2	0x1a
>>   #define QCOM_SCM_PIL_PAS_MEM_SETUP	0x02
>>   #define QCOM_SCM_PIL_PAS_AUTH_AND_RESET	0x05
>>   #define QCOM_SCM_PIL_PAS_SHUTDOWN	0x06
>> -- 
>> 2.34.1
>>
diff mbox series

Patch

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index e60bef68401c..aa559fd01932 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -607,6 +607,14 @@  int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 
 	desc.args[1] = mdata_phys;
 
+	if (__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
+					 QCOM_SCM_PAS_INIT_IMAGE_V2)) {
+		desc.cmd = QCOM_SCM_PAS_INIT_IMAGE_V2;
+		desc.arginfo =
+			QCOM_SCM_ARGS(3, QCOM_SCM_VAL, QCOM_SCM_RW, QCOM_SCM_VAL);
+		desc.args[2] = size;
+	}
+
 	ret = qcom_scm_call(__scm->dev, &desc, &res);
 	qcom_scm_bw_disable();
 
diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
index 685b8f59e7a6..008b59cbad36 100644
--- a/drivers/firmware/qcom/qcom_scm.h
+++ b/drivers/firmware/qcom/qcom_scm.h
@@ -96,6 +96,7 @@  struct qcom_tzmem_pool *qcom_scm_get_tzmem_pool(void);
 
 #define QCOM_SCM_SVC_PIL		0x02
 #define QCOM_SCM_PIL_PAS_INIT_IMAGE	0x01
+#define QCOM_SCM_PAS_INIT_IMAGE_V2	0x1a
 #define QCOM_SCM_PIL_PAS_MEM_SETUP	0x02
 #define QCOM_SCM_PIL_PAS_AUTH_AND_RESET	0x05
 #define QCOM_SCM_PIL_PAS_SHUTDOWN	0x06