diff mbox series

[v1,1/5] dt-bindings: soc: qcom: Add memory_dump driver bindings

Message ID 1698052857-6918-2-git-send-email-quic_zhenhuah@quicinc.com (mailing list archive)
State Changes Requested
Headers show
Series soc/arm64: qcom: add initial version of memory dump | expand

Commit Message

Zhenhua Huang Oct. 23, 2023, 9:20 a.m. UTC
Add bindings for the QCOM Memory Dump driver providing debug
facilities. Firmware dumps system cache, internal memory,
peripheral registers to reserved DDR as per the table which
populated by the driver, after crash and warm reset.

Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
---
 .../bindings/soc/qcom/qcom,mem-dump.yaml           | 42 +++++++++++++++++++++
 .../devicetree/bindings/sram/qcom,imem.yaml        | 44 ++++++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml

Comments

Krzysztof Kozlowski Oct. 23, 2023, 9:27 a.m. UTC | #1
On 23/10/2023 11:20, Zhenhua Huang wrote:
> Add bindings for the QCOM Memory Dump driver providing debug

Bindings are for hardware, not driver. This suggests it is not suitable
for bindings at all.

> facilities. Firmware dumps system cache, internal memory,
> peripheral registers to reserved DDR as per the table which
> populated by the driver, after crash and warm reset.

Again driver :/

> 
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom,mem-dump.yaml           | 42 +++++++++++++++++++++
>  .../devicetree/bindings/sram/qcom,imem.yaml        | 44 ++++++++++++++++++++++
>  2 files changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
> new file mode 100644
> index 0000000..87f8f51
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/soc/qcom/qcom,mem-dump.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

> +
> +title: Qualcomm memory dump

Describe hardware, not driver.

> +
> +description: |
> +  Qualcomm memory dump driver dynamically reserves memory and provides hints(id and size)

Again, driver, so not suitable for DTS and bindings.

Best regards,
Krzysztof
Zhenhua Huang Oct. 23, 2023, 11:54 a.m. UTC | #2
On 2023/10/23 17:27, Krzysztof Kozlowski wrote:
> On 23/10/2023 11:20, Zhenhua Huang wrote:
>> Add bindings for the QCOM Memory Dump driver providing debug
> 
> Bindings are for hardware, not driver. This suggests it is not suitable
> for bindings at all.
> 
>> facilities. Firmware dumps system cache, internal memory,
>> peripheral registers to reserved DDR as per the table which
>> populated by the driver, after crash and warm reset.
> 
> Again driver :/

Thanks for pointing out. Qualcomm memory dump device is a reserved 
memory region which is used to communicate with firmware. I will update 
description in next version.

Thanks,
Zhenhua

> 
>>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>> ---
>>   .../bindings/soc/qcom/qcom,mem-dump.yaml           | 42 +++++++++++++++++++++
>>   .../devicetree/bindings/sram/qcom,imem.yaml        | 44 ++++++++++++++++++++++
>>   2 files changed, 86 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
>> new file mode 100644
>> index 0000000..87f8f51
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/soc/qcom/qcom,mem-dump.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> 
> Drop quotes.
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
> 
>> +
>> +title: Qualcomm memory dump
> 
> Describe hardware, not driver.
> 
>> +
>> +description: |
>> +  Qualcomm memory dump driver dynamically reserves memory and provides hints(id and size)
> 
> Again, driver, so not suitable for DTS and bindings.
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Oct. 23, 2023, 12:52 p.m. UTC | #3
On 23/10/2023 13:54, Zhenhua Huang wrote:
> 
> 
> On 2023/10/23 17:27, Krzysztof Kozlowski wrote:
>> On 23/10/2023 11:20, Zhenhua Huang wrote:
>>> Add bindings for the QCOM Memory Dump driver providing debug
>>
>> Bindings are for hardware, not driver. This suggests it is not suitable
>> for bindings at all.
>>
>>> facilities. Firmware dumps system cache, internal memory,
>>> peripheral registers to reserved DDR as per the table which
>>> populated by the driver, after crash and warm reset.
>>
>> Again driver :/
> 
> Thanks for pointing out. Qualcomm memory dump device is a reserved 
> memory region which is used to communicate with firmware. I will update 
> description in next version.

I have still doubts that it is suitable for DT. I usually expect  such
firmware feature-drivers to be instantiated by existing firmware
drivers. You do not need DT for this.

Best regards,
Krzysztof
Rob Herring (Arm) Oct. 23, 2023, 5:40 p.m. UTC | #4
On Mon, 23 Oct 2023 17:20:53 +0800, Zhenhua Huang wrote:
> Add bindings for the QCOM Memory Dump driver providing debug
> facilities. Firmware dumps system cache, internal memory,
> peripheral registers to reserved DDR as per the table which
> populated by the driver, after crash and warm reset.
> 
> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
> ---
>  .../bindings/soc/qcom/qcom,mem-dump.yaml           | 42 +++++++++++++++++++++
>  .../devicetree/bindings/sram/qcom,imem.yaml        | 44 ++++++++++++++++++++++
>  2 files changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/sram/qcom,imem.yaml:55:1: [error] duplication of key "patternProperties" in mapping (key-duplicates)
./Documentation/devicetree/bindings/sram/qcom,imem.yaml:72:1: [error] duplication of key "patternProperties" in mapping (key-duplicates)
./Documentation/devicetree/bindings/sram/qcom,imem.yaml:119:1: [error] syntax error: found character '\t' that cannot start any token (syntax)
./Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml:5:10: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/sram/qcom,imem.example.dts'
Documentation/devicetree/bindings/sram/qcom,imem.yaml:119:1: found a tab character where an indentation space is expected
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/sram/qcom,imem.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/sram/qcom,imem.yaml:119:1: found a tab character where an indentation space is expected
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sram/qcom,imem.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1427: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/1698052857-6918-2-git-send-email-quic_zhenhuah@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Zhenhua Huang Oct. 24, 2023, 10:57 a.m. UTC | #5
On 2023/10/23 20:52, Krzysztof Kozlowski wrote:
> On 23/10/2023 13:54, Zhenhua Huang wrote:
>>
>>
>> On 2023/10/23 17:27, Krzysztof Kozlowski wrote:
>>> On 23/10/2023 11:20, Zhenhua Huang wrote:
>>>> Add bindings for the QCOM Memory Dump driver providing debug
>>>
>>> Bindings are for hardware, not driver. This suggests it is not suitable
>>> for bindings at all.
>>>
>>>> facilities. Firmware dumps system cache, internal memory,
>>>> peripheral registers to reserved DDR as per the table which
>>>> populated by the driver, after crash and warm reset.
>>>
>>> Again driver :/
>>
>> Thanks for pointing out. Qualcomm memory dump device is a reserved
>> memory region which is used to communicate with firmware. I will update
>> description in next version.
> 
> I have still doubts that it is suitable for DT. I usually expect  such
> firmware feature-drivers to be instantiated by existing firmware
> drivers. You do not need DT for this.

Got it, as it interacts with firmware, you think it should be a firmware 
driver? But it seems there should not be existing suitable place to put 
it now(qcom_scm.c is for TZ). Shall we create one new file like 
*qcom_sdi.c* in drivers/firmware and put it there? Because SDI(system 
debug image, which is part of bootloader) is the firmware doing the things.

> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Oct. 24, 2023, 1:55 p.m. UTC | #6
On 24/10/2023 12:57, Zhenhua Huang wrote:
> 
> 
> On 2023/10/23 20:52, Krzysztof Kozlowski wrote:
>> On 23/10/2023 13:54, Zhenhua Huang wrote:
>>>
>>>
>>> On 2023/10/23 17:27, Krzysztof Kozlowski wrote:
>>>> On 23/10/2023 11:20, Zhenhua Huang wrote:
>>>>> Add bindings for the QCOM Memory Dump driver providing debug
>>>>
>>>> Bindings are for hardware, not driver. This suggests it is not suitable
>>>> for bindings at all.
>>>>
>>>>> facilities. Firmware dumps system cache, internal memory,
>>>>> peripheral registers to reserved DDR as per the table which
>>>>> populated by the driver, after crash and warm reset.
>>>>
>>>> Again driver :/
>>>
>>> Thanks for pointing out. Qualcomm memory dump device is a reserved
>>> memory region which is used to communicate with firmware. I will update
>>> description in next version.
>>
>> I have still doubts that it is suitable for DT. I usually expect  such
>> firmware feature-drivers to be instantiated by existing firmware
>> drivers. You do not need DT for this.
> 
> Got it, as it interacts with firmware, you think it should be a firmware 
> driver? But it seems there should not be existing suitable place to put 
> it now(qcom_scm.c is for TZ). Shall we create one new file like 
> *qcom_sdi.c* in drivers/firmware and put it there? Because SDI(system 
> debug image, which is part of bootloader) is the firmware doing the things.

Dunno, didn't think about this. I comment here only about bindings. This
does not look suitable for bindings. That's it.

Best regards,
Krzysztof
Elliot Berman Oct. 26, 2023, 9:01 p.m. UTC | #7
Hi Zhenhua,

On 10/23/2023 2:27 AM, Krzysztof Kozlowski wrote:
> On 23/10/2023 11:20, Zhenhua Huang wrote:
>> Add bindings for the QCOM Memory Dump driver providing debug
> 
> Bindings are for hardware, not driver. This suggests it is not suitable
> for bindings at all.
> 
>> facilities. Firmware dumps system cache, internal memory,
>> peripheral registers to reserved DDR as per the table which
>> populated by the driver, after crash and warm reset.
> 
> Again driver :/
> 
>>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com>
>> ---
>>  .../bindings/soc/qcom/qcom,mem-dump.yaml           | 42 +++++++++++++++++++++
>>  .../devicetree/bindings/sram/qcom,imem.yaml        | 44 ++++++++++++++++++++++
>>  2 files changed, 86 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
>> new file mode 100644
>> index 0000000..87f8f51
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: "http://devicetree.org/schemas/soc/qcom/qcom,mem-dump.yaml#"
>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> 
> Drop quotes.
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
> 
>> +
>> +title: Qualcomm memory dump
> 
> Describe hardware, not driver.
> 
>> +
>> +description: |
>> +  Qualcomm memory dump driver dynamically reserves memory and provides hints(id and size)
> 
> Again, driver, so not suitable for DTS and bindings.

Could you create platform driver which binds directly to the

compatible = "qcom,qcom-imem-mem-dump-table"

You can look up the size from the dump table driver or have 2 reg properties 
in the -table node itself (so no need for the table-size node either).
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
new file mode 100644
index 0000000..87f8f51
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,mem-dump.yaml
@@ -0,0 +1,42 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/soc/qcom/qcom,mem-dump.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm memory dump
+
+description: |
+  Qualcomm memory dump driver dynamically reserves memory and provides hints(id and size)
+  of debugging information based on specified protocols with firmware. Firmware then does
+  the real data capture. The debugging information includes cache contents, internal
+  memory, registers. After crash and warm reboot, firmware scans ids,
+  sizes and stores contents into reserved memory accordingly. Firmware then enters into full
+  dump mode which dumps whole DDR to PC through USB.
+
+maintainers:
+  - Zhenhua Huang <quic_zhenhuah@quicinc.com>
+
+properties:
+  compatible:
+    const: qcom,mem-dump
+
+  memory-region:
+    maxItems: 1
+    description: phandle to memory reservation for qcom,mem-dump region.
+
+required:
+  - compatible
+  - memory-region
+
+additionalProperties: false
+
+examples:
+  # minimum memory dump definition.
+  - |
+    mem-dump {
+        compatible = "qcom,mem-dump";
+        memory-region = <&dump_mem>;
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/sram/qcom,imem.yaml b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
index 8025a85..e9eaa7a 100644
--- a/Documentation/devicetree/bindings/sram/qcom,imem.yaml
+++ b/Documentation/devicetree/bindings/sram/qcom,imem.yaml
@@ -52,6 +52,40 @@  patternProperties:
     $ref: /schemas/remoteproc/qcom,pil-info.yaml#
     description: Peripheral image loader relocation region
 
+patternProperties:
+  "^mem-dump-table@[0-9a-f]+$":
+    type: object
+    description: Used to store dump table base addr
+    properties:
+      compatible:
+        const: "qcom,qcom-imem-mem-dump-table"
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
+    additionalProperties: false
+
+patternProperties:
+  "^mem-dump-table-size@[0-9a-f]+$":
+    type: object
+    description: Used to store dump table size
+    properties:
+      compatible:
+        const: "qcom,qcom-imem-mem-dump-table-size"
+
+      reg:
+        maxItems: 1
+
+    required:
+      - compatible
+      - reg
+
+    additionalProperties: false
+
 required:
   - compatible
   - reg
@@ -76,5 +110,15 @@  examples:
                 compatible = "qcom,pil-reloc-info";
                 reg = <0x94c 0xc8>;
             };
+
+            mem-dump-table@10 {
+                compatible = "qcom,qcom-imem-mem-dump-table";
+                reg = <0x10 0x8>;
+            };
+
+	    mem-dump-table-size@724 {
+		compatible = "qcom,qcom-imem-mem-dump-table-size";
+		reg = <0x724 0x8>;
+            };
         };
     };