| Message ID | 1698052857-6918-2-git-send-email-quic_zhenhuah@quicinc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | soc/arm64: qcom: add initial version of memory dump | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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
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 >
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
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.
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 >
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
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 --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>; + }; }; };
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