Re: [PATCH v3 1/3] dt-bindings: remoteproc: qcom: adsp: Convert binding to YAML

[Rob Herring <robh@kernel.org>]

On Thu, Sep 16, 2021 at 11:24:10PM -0700, Stephen Boyd wrote:
> Quoting Rakesh Pillai (2021-09-16 09:55:51)
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml
> > new file mode 100644
> > index 0000000..051da43
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,hexagon-v56.yaml
> > @@ -0,0 +1,267 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/remoteproc/qcom,hexagon-v56.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Hexagon v56 Peripheral Image Loader
> > +
> > +maintainers:
> > +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> > +
> > +description:
> > +  This document defines the binding for a component that loads and boots firmware
> > +  on the Qualcomm Technology Inc. Hexagon v56 core.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,qcs404-cdsp-pil
> > +      - qcom,sdm845-adsp-pil
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      The base address and size of the qdsp6ss register
> > +
> > +  interrupts-extended:
> > +    minItems: 5
> > +    items:
> > +      - description: Watchdog interrupt
> > +      - description: Fatal interrupt
> > +      - description: Ready interrupt
> > +      - description: Handover interrupt
> > +      - description: Stop acknowledge interrupt
> > +
> > +  interrupt-names:
> > +    minItems: 5
> > +    items:
> > +      - const: wdog
> > +      - const: fatal
> > +      - const: ready
> > +      - const: handover
> > +      - const: stop-ack
> > +
> > +  clocks:
> > +    minItems: 7
> > +    maxItems: 8
> > +    description:
> > +      List of phandles and clock specifier pairs for the Hexagon,
> > +      per clock-names below.
> > +
> > +  clock-names:
> > +    minItems: 7
> > +    maxItems: 8
> > +
> > +  power-domains:
> > +    minItems: 1
> > +    items:
> > +      - description: CX power domain
> > +
> > +  resets:
> > +    minItems: 1
> > +    maxItems: 2
> > +    description:
> > +      reference to the list of resets for the Hexagon.
> > +
> > +  reset-names:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +    description: Reference to the reserved-memory for the Hexagon core
> > +
> > +  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,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
> > +
> > +  glink-edge:
> > +    type: object
> > +    description:
> > +      Qualcomm G-Link subnode which represents communication edge, channels
> > +      and devices related to the ADSP.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts-extended
> > +  - interrupt-names
> > +  - clocks
> > +  - clock-names
> > +  - power-domains
> > +  - qcom,halt-regs
> > +  - memory-region
> > +  - qcom,smem-states
> > +  - qcom,smem-state-names
> 
> Is there some way to make sure that 'resets' and 'reset-names' is
> present when the compatible that defines them is used and not required
> otherwise?

Yes, plenty of examples of that.

> 
> > +
> > +additionalProperties: false
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sdm845-adsp-pil
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: XO clock
> > +            - description: SWAY clock
> > +            - description: LPASS AHBS AON clock
> > +            - description: LPASS AHBM AON clock
> > +            - description: QDSP6SS XO clock
> > +            - description: QDSP6SS SLEEP clock
> > +            - description: QDSP6SS CORE clock
> > +        clock-names:
> > +          items:
> > +            - const: xo
> > +            - const: sway_cbcr
> > +            - const: lpass_ahbs_aon_cbcr
> > +            - const: lpass_ahbm_aon_cbcr
> > +            - const: qdsp6ss_xo
> > +            - const: qdsp6ss_sleep
> > +            - const: qdsp6ss_core
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,qcs404-cdsp-pil
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: XO clock
> > +            - description: SWAY clock
> > +            - description: TBU clock
> > +            - description: BIMC clock
> > +            - description: AHB AON clock
> > +            - description: Q6SS SLAVE clock
> > +            - description: Q6SS MASTER clock
> > +            - description: Q6 AXIM clock
> > +        clock-names:
> > +          items:
> > +            - const: xo
> > +            - const: sway
> > +            - const: tbu
> > +            - const: bimc
> > +            - const: ahb_aon
> > +            - const: q6ss_slave
> > +            - const: q6ss_master
> > +            - const: q6_axim
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sc7280-wpss-pil
> 
> This should be documented above in the compatible list?
> 
> > +    then:
> > +      properties:
> > +        clocks:
> > +          items:
> > +            - description: GCC WPSS AHB BDG Master clock
> > +            - description: GCC WPSS AHB clock
> > +            - description: GCC WPSS RSCP clock
> > +        clock-names:
> > +          items:
> > +            - const: gcc_wpss_ahb_bdg_mst_clk
> > +            - const: gcc_wpss_ahb_clk
> > +            - const: gcc_wpss_rscp_clk
> 
> Is the 'gcc_wpss' prefix important? It would be shorter if it wasn't
> there.

Yes, and adding this new platform should be a separate patch.

Rob