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

[<pillair@codeaurora.org>]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, September 22, 2021 4:59 AM
> To: Stephen Boyd <swboyd@chromium.org>
> Cc: Rakesh Pillai <pillair@codeaurora.org>; agross@kernel.org;
> bjorn.andersson@linaro.org; mathieu.poirier@linaro.org; ohad@wizery.com;
> p.zabel@pengutronix.de; linux-arm-msm@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> sibis@codeaurora.org; mpubbise@codeaurora.org; kuabhs@chromium.org;
> Rakesh Pillai <pillair@qti.qualcomm.com>
> Subject: Re: [PATCH v3 1/3] dt-bindings: remoteproc: qcom: adsp: Convert
> binding to YAML
> 
> 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

Hi Rob,

I have posted v4 for this patch series, where the dt-bindings for wpss has
been moved to a separate file.
Can you please review v4 ?

Thanks,
Rakesh Pillai