diff mbox series

[v2,6/6] remoterproc: qcom: refactor to leverage exported minidump symbol

Message ID 1679491817-2498-7-git-send-email-quic_mojha@quicinc.com
State New
Headers
Series [v2,1/6] remoteproc: qcom: Expand MD_* as MINIDUMP_* |

Commit Message

Mukesh Ojha March 22, 2023, 1:30 p.m. UTC 
  qcom_minidump driver provides qcom_minidump_subsystem_desc()
exported API which other driver can use it query subsystem
descriptor. Refactor qcom_minidump() to use this symbol.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/remoteproc/qcom_common.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

Comments

Srinivas Kandagatla April 14, 2023, 10:44 a.m. UTC | #1 
On 22/03/2023 13:30, Mukesh Ojha wrote:
> qcom_minidump driver provides qcom_minidump_subsystem_desc()
> exported API which other driver can use it query subsystem
> descriptor. Refactor qcom_minidump() to use this symbol.
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 88fc984..240e9f7 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
>   {
>   	int ret;
>   	struct minidump_subsystem *subsystem;
> -	struct minidump_global_toc *toc;
>   
> -	/* Get Global minidump ToC*/
> -	toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
> -
> -	/* check if global table pointer exists and init is set */
> -	if (IS_ERR(toc) || !toc->status) {
> -		dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
> +	subsystem = qcom_minidump_subsystem_desc(minidump_id);
> +	if (IS_ERR(subsystem))
>   		return;

Sorry If I am missing something but I got lost looking at the below code 
snippet in drivers/remoteproc/qcom_common.c


-------------------->cut<-----------------------------
	subsystem = qcom_minidump_subsystem_desc(minidump_id);
	if (IS_ERR(subsystem))
		return;

	/**
	 * Collect minidump if SS ToC is valid and segment table
	 * is initialized in memory and encryption status is set.
	 */
	if (subsystem->regions_baseptr == 0 ||
	    le32_to_cpu(subsystem->status) != 1 ||
	    le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
	    le32_to_cpu(subsystem->encryption_status) != MINIDUMP_SS_ENCR_DONE) {
		dev_err(&rproc->dev, "Minidump not ready, skipping\n");
		return;
	}
-------------------->cut<-----------------------------

where does "subsystem->regions_baseptr" for this ADSP minidump 
descriptor get set?


--srini

> -	}
> -
> -	/* Get subsystem table of contents using the minidump id */
> -	subsystem = &toc->subsystems[minidump_id];
>   
>   	/**
>   	 * Collect minidump if SS ToC is valid and segment table
Mukesh Ojha April 14, 2023, 11:14 a.m. UTC | #2 
On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
> 
> 
> On 22/03/2023 13:30, Mukesh Ojha wrote:
>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>> exported API which other driver can use it query subsystem
>> descriptor. Refactor qcom_minidump() to use this symbol.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_common.c 
>> b/drivers/remoteproc/qcom_common.c
>> index 88fc984..240e9f7 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned 
>> int minidump_id,
>>   {
>>       int ret;
>>       struct minidump_subsystem *subsystem;
>> -    struct minidump_global_toc *toc;
>> -    /* Get Global minidump ToC*/
>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
>> -
>> -    /* check if global table pointer exists and init is set */
>> -    if (IS_ERR(toc) || !toc->status) {
>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>> +    if (IS_ERR(subsystem))
>>           return;
> 
> Sorry If I am missing something but I got lost looking at the below code 
> snippet in drivers/remoteproc/qcom_common.c
> 
> 
> -------------------->cut<-----------------------------
>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>      if (IS_ERR(subsystem))
>          return;
> 
>      /**
>       * Collect minidump if SS ToC is valid and segment table
>       * is initialized in memory and encryption status is set.
>       */
>      if (subsystem->regions_baseptr == 0 ||
>          le32_to_cpu(subsystem->status) != 1 ||
>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>          le32_to_cpu(subsystem->encryption_status) != 
> MINIDUMP_SS_ENCR_DONE) {
>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>          return;
>      }
> -------------------->cut<-----------------------------
> 
> where does "subsystem->regions_baseptr" for this ADSP minidump 
> descriptor get set?

Other co-processor such as adsp/cdsp/Mpss has their own way of 
registering their region/segment (mostly they are static known
regions) with minidump global infra and which could be happening
from firmware side .


-Mukesh

> 
> 
> --srini
> 
>> -    }
>> -
>> -    /* Get subsystem table of contents using the minidump id */
>> -    subsystem = &toc->subsystems[minidump_id];
>>       /**
>>        * Collect minidump if SS ToC is valid and segment table
Srinivas Kandagatla April 14, 2023, 11:40 a.m. UTC | #3 
On 14/04/2023 12:14, Mukesh Ojha wrote:
> 
> 
> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>> exported API which other driver can use it query subsystem
>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>
>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>> ---
>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>> b/drivers/remoteproc/qcom_common.c
>>> index 88fc984..240e9f7 100644
>>> --- a/drivers/remoteproc/qcom_common.c
>>> +++ b/drivers/remoteproc/qcom_common.c
>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned 
>>> int minidump_id,
>>>   {
>>>       int ret;
>>>       struct minidump_subsystem *subsystem;
>>> -    struct minidump_global_toc *toc;
>>> -    /* Get Global minidump ToC*/
>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>> NULL);
>>> -
>>> -    /* check if global table pointer exists and init is set */
>>> -    if (IS_ERR(toc) || !toc->status) {
>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>> +    if (IS_ERR(subsystem))
>>>           return;
>>
>> Sorry If I am missing something but I got lost looking at the below 
>> code snippet in drivers/remoteproc/qcom_common.c
>>
>>
>> -------------------->cut<-----------------------------
>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>      if (IS_ERR(subsystem))
>>          return;
>>
>>      /**
>>       * Collect minidump if SS ToC is valid and segment table
>>       * is initialized in memory and encryption status is set.
>>       */
>>      if (subsystem->regions_baseptr == 0 ||
>>          le32_to_cpu(subsystem->status) != 1 ||
>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>          le32_to_cpu(subsystem->encryption_status) != 
>> MINIDUMP_SS_ENCR_DONE) {
>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>          return;
>>      }
>> -------------------->cut<-----------------------------
>>
>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>> descriptor get set?
> 
> Other co-processor such as adsp/cdsp/Mpss has their own way of 
> registering their region/segment (mostly they are static known
> regions) with minidump global infra and which could be happening
> from firmware side .
If its happening from firmware side, then that ram phys address range 
should be reserved from kernel usage I guess.

Do you have more details on where exactly is this reserved from within 
linux kernel?


--srini

> 
> 
> -Mukesh
> 
>>
>>
>> --srini
>>
>>> -    }
>>> -
>>> -    /* Get subsystem table of contents using the minidump id */
>>> -    subsystem = &toc->subsystems[minidump_id];
>>>       /**
>>>        * Collect minidump if SS ToC is valid and segment table
Mukesh Ojha April 14, 2023, 11:49 a.m. UTC | #4 
On 4/14/2023 5:10 PM, Srinivas Kandagatla wrote:
> 
> 
> On 14/04/2023 12:14, Mukesh Ojha wrote:
>>
>>
>> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>>> exported API which other driver can use it query subsystem
>>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>>
>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>> ---
>>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>> b/drivers/remoteproc/qcom_common.c
>>>> index 88fc984..240e9f7 100644
>>>> --- a/drivers/remoteproc/qcom_common.c
>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, unsigned 
>>>> int minidump_id,
>>>>   {
>>>>       int ret;
>>>>       struct minidump_subsystem *subsystem;
>>>> -    struct minidump_global_toc *toc;
>>>> -    /* Get Global minidump ToC*/
>>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>>> NULL);
>>>> -
>>>> -    /* check if global table pointer exists and init is set */
>>>> -    if (IS_ERR(toc) || !toc->status) {
>>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>> +    if (IS_ERR(subsystem))
>>>>           return;
>>>
>>> Sorry If I am missing something but I got lost looking at the below 
>>> code snippet in drivers/remoteproc/qcom_common.c
>>>
>>>
>>> -------------------->cut<-----------------------------
>>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>      if (IS_ERR(subsystem))
>>>          return;
>>>
>>>      /**
>>>       * Collect minidump if SS ToC is valid and segment table
>>>       * is initialized in memory and encryption status is set.
>>>       */
>>>      if (subsystem->regions_baseptr == 0 ||
>>>          le32_to_cpu(subsystem->status) != 1 ||
>>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>>          le32_to_cpu(subsystem->encryption_status) != 
>>> MINIDUMP_SS_ENCR_DONE) {
>>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>>          return;
>>>      }
>>> -------------------->cut<-----------------------------
>>>
>>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>>> descriptor get set?
>>
>> Other co-processor such as adsp/cdsp/Mpss has their own way of 
>> registering their region/segment (mostly they are static known
>> regions) with minidump global infra and which could be happening
>> from firmware side .
> If its happening from firmware side, then that ram phys address range 
> should be reserved from kernel usage I guess.
> 
> Do you have more details on where exactly is this reserved from within 
> linux kernel?

These regions are inside remoteproc memory carve-out.
like.

adsp_mem: memory@85e00000 {
	reg = <0x0 0x85e00000 0x0 0x2100000>;
	no-map;
};



remoteproc_adsp: remoteproc@30000000 {
	compatible = "qcom,sm8450-adsp-pas";
	reg = <0 0x30000000 0 0x100>;
            ...
            ...
	memory-region = <&adsp_mem>; <==

-Mukesh
	
> 
> 
> --srini
> 
>>
>>
>> -Mukesh
>>
>>>
>>>
>>> --srini
>>>
>>>> -    }
>>>> -
>>>> -    /* Get subsystem table of contents using the minidump id */
>>>> -    subsystem = &toc->subsystems[minidump_id];
>>>>       /**
>>>>        * Collect minidump if SS ToC is valid and segment table
Srinivas Kandagatla April 14, 2023, 12:22 p.m. UTC | #5 
On 14/04/2023 12:49, Mukesh Ojha wrote:
> 
> 
> On 4/14/2023 5:10 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 14/04/2023 12:14, Mukesh Ojha wrote:
>>>
>>>
>>> On 4/14/2023 4:14 PM, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 22/03/2023 13:30, Mukesh Ojha wrote:
>>>>> qcom_minidump driver provides qcom_minidump_subsystem_desc()
>>>>> exported API which other driver can use it query subsystem
>>>>> descriptor. Refactor qcom_minidump() to use this symbol.
>>>>>
>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>>>>> ---
>>>>>   drivers/remoteproc/qcom_common.c | 13 ++-----------
>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/qcom_common.c 
>>>>> b/drivers/remoteproc/qcom_common.c
>>>>> index 88fc984..240e9f7 100644
>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>> @@ -94,19 +94,10 @@ void qcom_minidump(struct rproc *rproc, 
>>>>> unsigned int minidump_id,
>>>>>   {
>>>>>       int ret;
>>>>>       struct minidump_subsystem *subsystem;
>>>>> -    struct minidump_global_toc *toc;
>>>>> -    /* Get Global minidump ToC*/
>>>>> -    toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, 
>>>>> NULL);
>>>>> -
>>>>> -    /* check if global table pointer exists and init is set */
>>>>> -    if (IS_ERR(toc) || !toc->status) {
>>>>> -        dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
>>>>> +    subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>>> +    if (IS_ERR(subsystem))
>>>>>           return;
>>>>
>>>> Sorry If I am missing something but I got lost looking at the below 
>>>> code snippet in drivers/remoteproc/qcom_common.c
>>>>
>>>>
>>>> -------------------->cut<-----------------------------
>>>>      subsystem = qcom_minidump_subsystem_desc(minidump_id);
>>>>      if (IS_ERR(subsystem))
>>>>          return;
>>>>
>>>>      /**
>>>>       * Collect minidump if SS ToC is valid and segment table
>>>>       * is initialized in memory and encryption status is set.
>>>>       */
>>>>      if (subsystem->regions_baseptr == 0 ||
>>>>          le32_to_cpu(subsystem->status) != 1 ||
>>>>          le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED ||
>>>>          le32_to_cpu(subsystem->encryption_status) != 
>>>> MINIDUMP_SS_ENCR_DONE) {
>>>>          dev_err(&rproc->dev, "Minidump not ready, skipping\n");
>>>>          return;
>>>>      }
>>>> -------------------->cut<-----------------------------
>>>>
>>>> where does "subsystem->regions_baseptr" for this ADSP minidump 
>>>> descriptor get set?
>>>
>>> Other co-processor such as adsp/cdsp/Mpss has their own way of 
>>> registering their region/segment (mostly they are static known
>>> regions) with minidump global infra and which could be happening
>>> from firmware side .
>> If its happening from firmware side, then that ram phys address range 
>> should be reserved from kernel usage I guess.
>>
>> Do you have more details on where exactly is this reserved from within 
>> linux kernel?
> 
> These regions are inside remoteproc memory carve-out.
> like.
> 
> adsp_mem: memory@85e00000 {
>      reg = <0x0 0x85e00000 0x0 0x2100000>;
>      no-map;
> };

thanks for explaining.

--srini
> 
> 
> 
> remoteproc_adsp: remoteproc@30000000 {
>      compatible = "qcom,sm8450-adsp-pas";
>      reg = <0 0x30000000 0 0x100>;
>             ...
>             ...
>      memory-region = <&adsp_mem>; <==
> 
> -Mukesh
> 
>>
>>
>> --srini
>>
>>>
>>>
>>> -Mukesh
>>>
>>>>
>>>>
>>>> --srini
>>>>
>>>>> -    }
>>>>> -
>>>>> -    /* Get subsystem table of contents using the minidump id */
>>>>> -    subsystem = &toc->subsystems[minidump_id];
>>>>>       /**
>>>>>        * Collect minidump if SS ToC is valid and segment table
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 88fc984..240e9f7 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -94,19 +94,10 @@  void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
 {
 	int ret;
 	struct minidump_subsystem *subsystem;
-	struct minidump_global_toc *toc;
 
-	/* Get Global minidump ToC*/
-	toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
-
-	/* check if global table pointer exists and init is set */
-	if (IS_ERR(toc) || !toc->status) {
-		dev_err(&rproc->dev, "Minidump TOC not found in SMEM\n");
+	subsystem = qcom_minidump_subsystem_desc(minidump_id);
+	if (IS_ERR(subsystem))
 		return;
-	}
-
-	/* Get subsystem table of contents using the minidump id */
-	subsystem = &toc->subsystems[minidump_id];
 
 	/**
 	 * Collect minidump if SS ToC is valid and segment table