| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <CJXOUUY3IV5I.33259TNMB50JG@skynet-linux>
Date: Thu, 12 May 2022 15:20:59 +0530
From: "Sireesh Kodali" <sireeshkodali1@...il.com>
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>,
<linux-remoteproc@...r.kernel.org>
Cc: <linux-arm-msm@...r.kernel.org>,
<~postmarketos/upstreaming@...ts.sr.ht>,
<bjorn.andersson@...aro.org>, <devicetree@...r.kernel.org>,
<phone-devel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
"Andy Gross" <agross@...nel.org>,
"Mathieu Poirier" <mathieu.poirier@...aro.org>,
"Rob Herring" <robh+dt@...nel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH 6/9] dt-bindings: remoteproc: qcom: mss: Convert
bindings to YAML
On Wed May 11, 2022 at 11:20 PM IST, Krzysztof Kozlowski wrote:
> On 11/05/2022 18:15, Sireesh Kodali wrote:
> > Convert the bindings to YAML from txt. The bindings follow a similar
> > schema to `qcom,adsp.yaml`.
> >
> > Signed-off-by: Sireesh Kodali <sireeshkodali1@...il.com>
> > ---
> > .../bindings/remoteproc/qcom,q6v5.txt | 302 --------
> > .../bindings/remoteproc/qcom,q6v5.yaml | 702 ++++++++++++++++++
> > 2 files changed, 702 insertions(+), 302 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> > create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,q6v5.yaml
>
> Same comments as for patch 4 apply here. Some of them seems fixed so it
> makes me wonder - why you wrote two bindings in entirely different way?
>
This was largely based on the existing qcom,adsp.yaml. That's probably
why there are less mistakes.
> > + This document defines the binding for a component that loads and boots firmware
> > + on the Qualcomm Hexagon core.
> > +
> > +properties:
> > + compatible:
> > + # Special case, because older platforms like MSM8916 use both compatibles together
> > + minItems: 1
> > + maxItems: 2
> > + oneOf:
> > + - items:
> > + - enum:
> > + - qcom,msm8916-mss-pil
> > + - enum:
> > + - qcom,q6v5-pil
>
> Everywhere - same comments as in patch 4.
>
Will be fixed in v2.
> > + - items:
> > + - enum:
> > + - qcom,q6v5-pil
> > + - qcom,ipq8074-wcss-pil
> > + - qcom,qcs404-wcss-pil
> > + - qcom,msm8916-mss-pil
> > + - qcom,msm8974-mss-pil
> > + - qcom,msm8996-mss-pil
> > + - qcom,msm8998-mss-pil
> > + - qcom,sc7180-mss-pil
> > + - qcom,sc7280-mss-pil
> > + - qcom,sdm845-mss-pil
> > +
> > + reg:
> > + description: must specify the base address and size of the qdsp6 and rmb
> > + register blocks
> > + maxItems: 2
> > +
> > + reg-names:
> > + items:
> > + - const: qdsp6
> > + - const: rmb
> > +
> > + interrupts-extended:
> > + minItems: 5
> > + maxItems: 6
> > +
> > + interrupt-names:
> > + minItems: 5
> > + maxItems: 6
> > +
> > + firmware-name:
> > + $ref: /schemas/types.yaml#/definitions/string-array
> > + description: List the relative firmware image paths for the mba and
> > + modem. They are used for booting and authenticating the Hexagon core.
> > + maxItems: 2
> > +
> > + clocks:
> > + minItems: 4
> > + maxItems: 10
> > +
> > + clock-names:
> > + minItems: 4
> > + maxItems: 10
> > +
> > + resets:
> > + description: Reference to the reset-controllwer for the modem subsystem
> > + minItems: 1
> > + maxItems: 3
> > +
> > + reset-names:
> > + minItems: 1
> > + maxItems: 3
> > +
> > + memory-region:
> > + maxItems: 1
> > + description: Reference to the reserved-memory for the mba region followed
> > + by the mpss region. Required if the mba and mpss sub-nodes are not
> > + specified.
> > +
> > + cx-supply:
> > + description: Phandle to the CX regulator
> > +
> > + mx-supply:
> > + description: Phandle to the MX regulator
> > +
> > + pll-supply:
> > + description: Phandle to the PLL regulator, to be held on behalf of the
> > + booting Hexagon core
> > +
> > + mss-supply:
> > + description: Phandle to the mss regulator, to be held on behalf of the
> > + booting Hexagon core
>
> s/Phandle to//, everywhere.
>
> > +
> > + power-domains:
> > + minItems: 1
> > + maxItems: 4
> > +
> > + power-domain-names:
> > + minItems: 1
> > + maxItems: 4
> > +
> > + qcom,qmp:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: Reference to the AOSS side-channel message RAM.
> > +
> > + qcom,smem-states:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description: States used by the AP to signal the Hexagon core
> > + items:
> > + - description: Stop the modem
> > +
> > + qcom,smem-state-names:
> > + $ref: /schemas/types.yaml#/definitions/string-array
> > + description: The names of the state bits used for SMP2P output
> > + items:
> > + - const: stop
> > +
> > + qcom,halt-regs:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description:
> > + Phandle reference to a syscon representing TCSR followed by the
> > + three offsets within syscon for q6, modem and nc halt registers.
> > +
> > + qcom,spare-regs:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description:
> > + A phandle reference to a syscon representing TCSR followed by the
> > + offset within syscon for conn_box_spare0 register used by the modem
> > + sub-system running on SC7180 SoC.
> > +
> > + qcom,ext-regs:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description:
> > + Two phandle references to syscons representing TCSR_REG and TCSR
> > + register space followed by the two offsets within the syscon to
> > + force_clk_en/rscc_disable and axim1_clk_off/crypto_clk_off registers
> > + respectively.
> > +
> > + qcom,qaccept-regs:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description:
> > + A phandle reference to a syscon representing TCSR followed by the three
> > + offsets within syscon for mdm, cx and axi qaccept registers used by the
> > + modem sub-system running on SC7280 SoC.
> > +
> > + iommus:
> > + description:
> > + Only required on platforms that do not have TrustZone.
> > +
> > + smd-edge:
> > + type: object
> > + description:
> > + Qualcomm Shared Memory subnode which represents communication edge,
> > + channels and devices related to the ADSP.
> > +
> > + glink-edge:
> > + type: object
> > + description:
> > + Qualcomm G-Link subnode which represents communication edge, channels
> > + and devices related to the ADSP.
> > +
> > + mba:
> > + type: object
>
> description needed.
>
> > + properties:
> > + memory-region:
> > + maxItems: 1
> > +
> > + required:
> > + - memory-region
> > +
> > + mpss:
>
> description needed.
>
> > + type: object
> > + properties:
> > + memory-region:
> > + maxItems: 1
> > +
> > + required:
> > + - memory-region
> > +
> > +required:
> > + - compatible
> > + - clocks
> > + - clock-names
> > + - interrupts-extended
> > + - interrupt-names
> > + - qcom,smem-states
> > + - qcom,smem-state-names
> > +
> > +additionalProperties: false
> > +
> > +allOf:
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,ipq8074-wcss-pil
> > + then:
> > + properties:
> > + clocks: false
> > + clock-names: false
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,qcs404-wcss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: GCC_AHBS_CBCR clock
> > + - description: GCC_AXIM_CBCR clock
> > + - description: LCC_AHBFABRIC_CBC clock
> > + - description: TCSR_LCC_CBC clock
> > + - description: LCC_AHBS_CBC clock
> > + - description: LCC_TCM_SLAVE_CBC clock
> > + - description: LCC_ABHM_CBC clock
> > + - description: LCC_AXIM_CBC clock
> > + - description: LCC_BCR_SLEEP clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: gcc_ahbs_cbcr
> > + - const: gcc_axim_cbcr
> > + - const: lcc_ahbfabric_cbc
> > + - const: tcsr_lcc_cbc
> > + - const: lcc_abhs_cbc
> > + - const: lcc_tcm_slave_cbc
> > + - const: lcc_abhm_cbc
> > + - const: lcc_axim_cbc
> > + - const: lcc_bcr_sleep
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,q6v5-pil
> > + - qcom,msm8916-mss-pil
> > + - qcom,msm8974-mss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Interface clock
> > + - description: Bus clock
> > + - description: Memory clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: iface
> > + - const: bus
> > + - const: mem
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,msm8996-mss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Interface clock
> > + - description: Bus clock
> > + - description: Memory clock
> > + - description: GPLL0_MSS clock
> > + - description: SNOC_AXI clock
> > + - description: MNOC_AXI clock
> > + - description: PNOC clock
> > + - description: QDSS clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: iface
> > + - const: bus
> > + - const: mem
> > + - const: gpll0_mss
> > + - const: snoc_axi
> > + - const: mnoc_axi
> > + - const: pnoc
> > + - const: qdss
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,msm8998-mss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Interface clock
> > + - description: Bus clock
> > + - description: Memory clock
> > + - description: GPLL0_MSS clock
> > + - description: SNOC_AXI clock
> > + - description: MNOC_AXI clock
> > + - description: QDSS clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: iface
> > + - const: bus
> > + - const: mem
> > + - const: gpll0_mss
> > + - const: snoc_axi
> > + - const: mnoc_axi
> > + - const: qdss
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sc7180-mss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Interface clock
> > + - description: Bus clock
> > + - description: NAV clock
> > + - description: SNOC_AXI clock
> > + - description: MNOC_AXI clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: iface
> > + - const: bus
> > + - const: nav
> > + - const: snoc_axi
> > + - const: mnoc_axi
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sc7280-mss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Interface clock
> > + - description: Offline clock
> > + - description: SNOC_AXI clock
> > + - description: PKA clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: iface
> > + - const: offline
> > + - const: snoc_axi
> > + - const: pka
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sdm845-mss-pil
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Interface clock
> > + - description: Bus clock
> > + - description: Memory clock
> > + - description: GPLL0_MSS clock
> > + - description: SNOC_AXI clock
> > + - description: MNOC_AXI clock
> > + - description: PRNG clock
> > + - description: XO clock
> > + clock-names:
> > + items:
> > + - const: iface
> > + - const: bus
> > + - const: mem
> > + - const: gpll0_mss
> > + - const: snoc_axi
> > + - const: mnoc_axi
> > + - const: prng
> > + - const: xo
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,q6v5-pil
> > + - qcom,ipq8074-wcss-pil
> > + - qcom,qcs404-wcss-pil
> > + - qcom,msm8916-mss-pil
> > + - qcom,msm8974-mss-pil
> > + then:
> > + properties:
> > + interrupts-extended:
>
> No extended.
>
> > + items:
> > + - description: Watchdog interrupt
> > + - description: Fatal interrupt
> > + - description: Ready interrupt
> > + - description: Handover interrupt
> > + - description: Stop acknowledge interrupt
> > + interrupt-names:
> > + items:
> > + - const: wdog
> > + - const: fatal
> > + - const: ready
> > + - const: handover
> > + - const: stop-ack
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,msm8996-mss-pil
> > + - qcom,msm8998-mss-pil
> > + - qcom,sc7180-mss-pil
> > + - qcom,sc7280-mss-pil
> > + - qcom,sdm845-mss-pil
> > + then:
> > + properties:
> > + interrupts-extended:
> > + items:
> > + - description: Watchdog interrupt
> > + - description: Fatal interrupt
> > + - description: Ready interrupt
> > + - description: Handover interrupt
> > + - description: Stop acknowledge interrupt
> > + - description: Shutdown acknowledge interrupt
> > + interrupt-names:
> > + items:
> > + - const: wdog
> > + - const: fatal
> > + - const: ready
> > + - const: handover
> > + - const: stop-ack
> > + - const: shutdown-ack
>
> Both ifs should be written differently. Move the bigger list to
> properties with minItems: 5. For qcom,q6v5-pil you set maxItems:5. For
> qcom,msm8996-mss-pil you set minItems:6.
>
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + enum:
> > + - const: qcom,q6v5-pil
> > + - const: qcom,msm8916-mss-pil
> > + - const: qcom,msm8974-mss-pil
> > + then:
> > + properties:
> > + cx-supply:
> > + deprecated: true
> > + mx-supply:
> > + deprecated: true
> > +
> > + required:
> > + - pll-supply
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + enum:
> > + - qcom,msm8916-mss-pil
> > + - qcom,msm8974-mss-pil
> > + - qcom,msm8996-mss-pil
> > + - qcom,msm8998-mss-pil
> > + then:
> > + properties:
> > + power-domains:
> > + items:
> > + - description: CX power domain
> > + - description: MX power domain
> > + power-domain-names:
> > + items:
> > + - const: cx
> > + - const: mx
> > +
> > + required:
> > + - power-domains
> > + - power-domain-names
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + enum:
> > + - const: qcom,msm8996-mss-pil
> > + then:
> > + properties:
> > + cx-supply: false
> > + mx-supply: false
> > +
> > + required:
> > + - pll-supply
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + enum:
> > + - const: qcom,qcs404-wcss-pil
> > + then:
> > + required:
> > + - cx-supply
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sdm845-mss-pil
> > + - qcom,sc7180-mss-pil
> > + then:
> > + properties:
> > + power-domains:
> > + items:
> > + - description: CX power domain
> > + - description: MX power domain
> > + - description: MSS power domain
> > + power-domain-names:
> > + items:
> > + - const: cx
> > + - const: mx
> > + - const: mss
> > +
> > + required:
> > + - power-domains
> > + - power-domain-names
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sc7280-mss-pil
> > + then:
> > + properties:
> > + power-domains:
> > + items:
> > + - description: CX power domain
> > + - description: MSS power domain
> > + power-domain-names:
> > + items:
> > + - const: cx
> > + - const: mss
> > +
> > + required:
> > + - power-domains
> > + - power-domain-names
> > + - qcom,ext-regs
> > + - qcom,qaccept-regs
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,q6v5-pil
> > + - qcom,msm8916-mss-pil
> > + - qcom,msm8974-mss-pil
> > + - qcom,msm8996-mss-pil
> > + - qcom,msm8998-mss-pil
> > + then:
> > + properties:
> > + resets:
> > + items:
> > + - description: MSS reset
> > + reset-names:
> > + items:
> > + - const: mss_restart
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,ipq8074-wcss-pil
> > + - qcom,qcs404-wcss-pil
> > + then:
> > + properties:
> > + resets:
> > + items:
> > + - description: WCSS Always On restart
> > + - description: WCSS reset
> > + - description: WCSS Q6 reset
> > + reset-names:
> > + items:
> > + - const: wcss_aon_restart
> > + - const: wcss_reset
> > + - const: wcss_q6_reset
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,sc7180-mss-pil
> > + - qcom,sc7280-mss-pil
> > + - qcom,sdm845-mss-pil
> > + then:
> > + properties:
> > + resets:
> > + items:
> > + - description: MSS restart
> > + - description: PDC reset
> > + reset-names:
> > + items:
> > + - const: mss_restart
> > + - const: pdc_reset
> > +
> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - qcom,q6v5-pil
> > + - qcom,ipq8074-wcss-pil
> > + - qcom,qcs404-wcss-pil
> > + - qcom,msm8916-mss-pil
> > + - qcom,msm8974-mss-pil
> > + - qcom,msm8996-mss-pil
> > + - qcom,msm8998-mss-pil
> > + then:
> > + properties:
> > + qcom,qmp: false
> > + iommus: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/qcom,rpmcc.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/clock/qcom,gcc-msm8974.h>
> > + #include <dt-bindings/reset/qcom,gcc-msm8974.h>
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + modem-rproc@...80000 {
>
> Generic node name, so "remoteproc"
Will fix in v2
>
> > + compatible = "qcom,q6v5-pil";
> > + reg = <0xfc880000 0x100>,
> > + <0xfc820000 0x020>;
> > + reg-names = "qdsp6", "rmb";
> > +
> > + interrupts-extended = <&intc GIC_SPI 24 IRQ_TYPE_EDGE_RISING>,
> > + <&modem_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
> > + <&modem_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
> > + <&modem_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
> > + <&modem_smp2p_in 3 IRQ_TYPE_EDGE_RISING>;
> > + interrupt-names = "wdog",
> > + "fatal",
> > + "ready",
> > + "handover",
> > + "stop-ack";
> > +
> > + clocks = <&gcc GCC_MSS_Q6_BIMC_AXI_CLK>,
> > + <&gcc GCC_MSS_CFG_AHB_CLK>,
> > + <&gcc GCC_BOOT_ROM_AHB_CLK>,
> > + <&xo_board>;
> > + clock-names = "iface", "bus", "mem", "xo";
> > +
> > + qcom,halt-regs = <&tcsr_mutex_block 0x1180 0x1200 0x1280>;
>
> Does this pass the tests? Did you test the bindings?
Yes, both dtbs_check and dt_binding_check
>
> > +
> > + resets = <&gcc GCC_MSS_RESTART>;
> > + reset-names = "mss_restart";
> > +
> > + cx-supply = <&pm8841_s2>;
> > + mss-supply = <&pm8841_s3>;
> > + mx-supply = <&pm8841_s1>;
> > + pll-supply = <&pm8941_l12>;
> > +
> > + qcom,smem-states = <&modem_smp2p_out 0>;
> > + qcom,smem-state-names = "stop";
> > +
> > + mba {
> > + memory-region = <&mba_region>;
>
> Wrong indentation.
>
Will fix in v2
> > + };
> > +
> > + mpss {
> > + memory-region = <&mpss_region>;
> > + };
> > +
> > + smd-edge {
> > + interrupts = <GIC_SPI 25 IRQ_TYPE_EDGE_RISING>;
> > +
> > + qcom,ipc = <&apcs 8 12>;
> > + qcom,smd-edge = <0>;
> > +
> > + label = "modem";
> > + };
> > + };
>
>
> Best regards,
> Krzysztof
Thanks,
Sireesh Kodali
Powered by blists - more mailing lists