Re: [PATCH v3 03/18] docs: qcom: Add qualcomm minidump guide

[Mukesh Ojha <quic_mojha@quicinc.com>]

On 5/8/2023 3:31 PM, Bagas Sanjaya wrote:
> On Wed, May 03, 2023 at 10:32:17PM +0530, Mukesh Ojha wrote:
>>   Documentation/admin-guide/qcom_minidump.rst | 246 ++++++++++++++++++++++++++++
>>   1 file changed, 246 insertions(+)
> 
> You forget to add toctree entry:
> 
> ---- >8 ----
> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
> index 43ea35613dfcd4..251d070486c2ab 100644
> --- a/Documentation/admin-guide/index.rst
> +++ b/Documentation/admin-guide/index.rst
> @@ -120,6 +120,7 @@ configure specific aspects of kernel behavior to your liking.
>      perf-security
>      pm/index
>      pnp
> +   qcom_minidump
>      rapidio
>      ras
>      rtc
> 
>> +Writing to sysfs node can also be used to set the mode to minidump.
>> +
>> +::
>> +	echo "mini" > /sys/module/qcom_scm/parameter/download_mode
> 
> The snippet above isn't rendered to code block, so I have to fix it up:
> 
> ---- >8 ----
> diff --git a/Documentation/admin-guide/qcom_minidump.rst b/Documentation/admin-guide/qcom_minidump.rst
> index 062c797e90d9cf..408fe1beed1b78 100644
> --- a/Documentation/admin-guide/qcom_minidump.rst
> +++ b/Documentation/admin-guide/qcom_minidump.rst
> @@ -208,9 +208,8 @@ Similarly, "full" is passed to set the download mode to full dump
>   where entire ddr dump will be collected while setting it "full,mini"
>   will collect minidump along with fulldump.
>   
> -Writing to sysfs node can also be used to set the mode to minidump.
> +Writing to sysfs node can also be used to set the mode to minidump::
>   
> -::
>   	echo "mini" > /sys/module/qcom_scm/parameter/download_mode
>   
>   Once the download mode is set, any kind of crash will make the device collect
> 
>> +By default, dumps are downloaded via USB to the attached x86_64 machine
>> +running PCAT (Qualcomm tool) software. Upon download, we will see
>> +a set of binary blobs starts with name md_* in PCAT configured directory
>> +in x86_64 machine, so for above example from the client it will be
>> +md_REGION_A.BIN. This binary blob depends on region content to determine
>> +whether it needs external parser support to get the content of the region,
>> +so for simple plain ASCII text we don't need any parsing and the content
>> +can be seen just opening the binary file.
>> +
>> <snipped> ...
>> +One need to read the entire rawdump partition and pull out content to
>> +save it onto the attached x86_64 machine over USB. Later, this rawdump
>> +can be pass it to another tool dexter.exe(Qualcomm tool) which converts
>> +this into the similar binary blobs which we have got it when download type
>> +was set to USB i.e a set of registered region as blobs and their name
>> +starts with md_*.
>> +
>> +Replacing the dexter.exe with some open source tool can be added as future
>> +scope of this document.
> 
> There is inconsistency on inlining code keywords, so I have to inline
> the rest:
> 
> ---- >8 ----
> diff --git a/Documentation/admin-guide/qcom_minidump.rst b/Documentation/admin-guide/qcom_minidump.rst
> index 408fe1beed1b78..117e61ef9fde67 100644
> --- a/Documentation/admin-guide/qcom_minidump.rst
> +++ b/Documentation/admin-guide/qcom_minidump.rst
> @@ -118,7 +118,7 @@ Client driver can use ``qcom_apss_minidump_region_register`` API's to
>   register and ``qcom_apss_minidump_region_unregister`` to unregister
>   their region from minidump driver.
>   
> -Client need to fill their region by filling qcom_apss_minidump_region
> +Client need to fill their region by filling ``qcom_apss_minidump_region``
>   structure object which consist of the region name, region's
>   virtual and physical address and its size.
>   
> @@ -166,7 +166,7 @@ Test
>   
>   Existing Qualcomm devices already supports entire ddr dump (also called
>   full dump) by writing appropriate value to Qualcomm's top control and
> -status register(tcsr) in driver/firmware/qcom_scm.c .
> +status register(tcsr) in ``driver/firmware/qcom_scm.c``.
>   
>   SCM device Tree bindings required to support download mode
>   For example (sm8450) ::
> @@ -202,10 +202,10 @@ For example (sm8450) ::
>   
>   	};
>   
> -User of minidump can pass qcom_scm.download_mode="mini" to kernel
> +User of minidump can pass ``qcom_scm.download_mode="mini"`` to kernel
>   commandline to set the current download mode to minidump.
> -Similarly, "full" is passed to set the download mode to full dump
> -where entire ddr dump will be collected while setting it "full,mini"
> +Similarly, ``"full"`` is passed to set the download mode to full dump
> +where entire ddr dump will be collected while setting it ``"full,mini"``
>   will collect minidump along with fulldump.
>   
>   Writing to sysfs node can also be used to set the mode to minidump::
> @@ -223,9 +223,9 @@ stored to an attached storage device.
>   
>   By default, dumps are downloaded via USB to the attached x86_64 machine
>   running PCAT (Qualcomm tool) software. Upon download, we will see
> -a set of binary blobs starts with name md_* in PCAT configured directory
> +a set of binary blobs starts with name ``md_*`` in PCAT configured directory
>   in x86_64 machine, so for above example from the client it will be
> -md_REGION_A.BIN. This binary blob depends on region content to determine
> +``md_REGION_A.BIN``. This binary blob depends on region content to determine
>   whether it needs external parser support to get the content of the region,
>   so for simple plain ASCII text we don't need any parsing and the content
>   can be seen just opening the binary file.
> @@ -236,10 +236,10 @@ partition on the target device itself.
>   
>   One need to read the entire rawdump partition and pull out content to
>   save it onto the attached x86_64 machine over USB. Later, this rawdump
> -can be pass it to another tool dexter.exe(Qualcomm tool) which converts
> +can be pass it to another tool (``dexter.exe`` [Qualcomm tool]) which converts
>   this into the similar binary blobs which we have got it when download type
>   was set to USB i.e a set of registered region as blobs and their name
> -starts with md_*.
> +starts with ``md_*``.
>   
> -Replacing the dexter.exe with some open source tool can be added as future
> +Replacing the ``dexter.exe`` with some open source tool can be added as future
>   scope of this document.

Thanks for the review and the shared diffs, have applied the changes for 
the next version.

-- Mukesh

> 
> Thanks.
>