Re: [PATCH 2/6] dt-bindings: remoteproc: qcom,q6v5: Move MSM8916 to schema

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/07/2022 14:44, Stephan Gerhold wrote:
> qcom,q6v5.txt covers multiple SoCs with quite different binding
> requirements. Converting this into one DT schema would require
> several if statements, making the DT schema overall harder to
> read and understand.
> 
> To avoid this, follow the example of SC7180/SC7280 and split
> "qcom,msm8916-mss-pil" (and the equivalent deprecated "qcom,q6v5-pil"
> compatible) into a separate DT schema. The schema is somewhat based
> on the one for SC7180/SC7280 but adjusted for the old platforms.
> 
> Compared to the old plain text bindings, add missing documentation for
> the "bam-dmux" subnode and recommend one particular approach to specify
> the MBA/MPSS "memory-region" (the other one is marked as deprecated).
> 
> Cc: Sireesh Kodali <sireeshkodali1@gmail.com>
> Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com>
> ---
> Like Sibi's patch series for SC7180/SC7820 [1] this is somewhat related
> to Sireesh's series that converts all of qcom,q6v5.txt [2] (with a lot
> of if statements). However, this series focuses on MSM8916/MSM8974 (or
> actually MSM8909) only.
> 

Thank you for your patch. There is something to discuss/improve.

> [1]: https://lore.kernel.org/linux-arm-msm/1657020721-24939-1-git-send-email-quic_sibis@quicinc.com/
> [2]: https://lore.kernel.org/linux-arm-msm/20220511161602.117772-7-sireeshkodali1@gmail.com/
> ---
>  .../remoteproc/qcom,msm8916-mss-pil.yaml      | 246 ++++++++++++++++++
>  .../bindings/remoteproc/qcom,q6v5.txt         |  19 --
>  2 files changed, 246 insertions(+), 19 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml
> new file mode 100644
> index 000000000000..3968348dc982
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml
> @@ -0,0 +1,246 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,msm8916-mss-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm MSM8916 MSS Peripheral Image Loader (and similar)
> +
> +maintainers:
> +  - Stephan Gerhold <stephan@gerhold.net>
> +
> +description:
> +  This document describes the hardware for a component that loads and boots
> +  firmware on the Qualcomm MSM8916 Modem Hexagon Core (and similar).
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - qcom,msm8916-mss-pil
> +
> +      - const: qcom,q6v5-pil
> +        description: Deprecated, prefer using qcom,msm8916-mss-pil
> +        deprecated: true

The last compatible does not seem applicable here. Aren't you moving
only MSM8916 to new schema?

> +
> +  reg:
> +    items:
> +      - description: MSS QDSP6 registers
> +      - description: RMB registers
> +
> +  reg-names:
> +    items:
> +      - const: qdsp6
> +      - const: rmb
> +
> +  interrupts:
> +    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
> +
> +  clocks:
> +    items:
> +      - description: Configuration interface (AXI) clock
> +      - description: Configuration bus (AHB) clock
> +      - description: Boot ROM (AHB) clock
> +      - description: XO proxy clock (control handed over after startup)
> +
> +  clock-names:
> +    items:
> +      - const: iface
> +      - const: bus
> +      - const: mem
> +      - const: xo
> +
> +  power-domains:
> +    items:
> +      - description: CX proxy power domain (control handed over after startup)
> +      - description: MX proxy power domain (control handed over after startup)
> +
> +  power-domain-names:
> +    items:
> +      - const: cx
> +      - const: mx
> +
> +  pll-supply:
> +    description: PLL proxy supply (control handed over after startup)
> +
> +  resets:
> +    items:
> +      - description: MSS restart control
> +
> +  reset-names:
> +    items:
> +      - const: mss_restart
> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the Hexagon core
> +    items:
> +      - description: Stop modem
> +
> +  qcom,smem-state-names:
> +    description: Names of the states used by the AP to signal the Hexagon core
> +    items:
> +      - const: stop
> +
> +  qcom,halt-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Halt registers are used to halt transactions of various sub-components
> +      within MSS.
> +    items:
> +      - items:
> +          - description: phandle to TCSR syscon region
> +          - description: offset to the Q6 halt register
> +          - description: offset to the modem halt register
> +          - description: offset to the nc halt register
> +
> +  memory-region:
> +    items:
> +      - description: MBA reserved region
> +      - description: MPSS reserved region
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    items:
> +      - description: Name of MBA firmware
> +      - description: Name of modem firmware
> +
> +  bam-dmux:
> +    $ref: /schemas/net/qcom,bam-dmux.yaml#
> +    description:
> +      Qualcomm BAM Data Multiplexer (provides network interface to the modem)
> +
> +  smd-edge:
> +    $ref: qcom,smd-edge.yaml#
> +    description:
> +      Qualcomm SMD subnode which represents communication edge, channels
> +      and devices related to the DSP.
> +
> +    properties:
> +      label:
> +        enum:
> +          - modem
> +          - hexagon
> +
> +  # Deprecated properties
> +  cx-supply:
> +    description: CX power domain regulator supply (prefer using power-domains)
> +    deprecated: true

Blank line, here and in other places between top-level properties.

> +  mx-supply:
> +    description: MX power domain regulator supply (prefer using power-domains)
> +    deprecated: true
> +  mba:
> +    type: object
> +    description:
> +      MBA reserved region (prefer using memory-region with two items)
> +    properties:
> +      memory-region: true
> +    required:
> +      - memory-region
> +    deprecated: true
> +  mpss:
> +    type: object
> +    description:
> +      MPSS reserved region (prefer using memory-region with two items)
> +    properties:
> +      memory-region: true
> +    required:
> +      - memory-region
> +    deprecated: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - pll-supply
> +  - resets
> +  - reset-names
> +  - qcom,halt-regs
> +  - qcom,smem-states
> +  - qcom,smem-state-names
> +  - smd-edge
> +
> +# Fallbacks for deprecated properties
> +allOf:
> +  - oneOf:
> +      - required:
> +          - memory-region
> +      - required:
> +          - mba
> +          - mpss
> +        deprecated: true

Not sure if this is correct syntax.

> +  - oneOf:
> +      - required:
> +          - power-domains
> +          - power-domain-names
> +      - required:
> +          - cx-supply
> +          - mx-supply
> +        deprecated: true
> +
> +additionalProperties: false



Best regards,
Krzysztof