Re: [PATCH v2 2/2] dt-bindings: remoteproc: Add documentation for SPSS remoteproc

[Bjorn Andersson]

On Fri 20 Mar 18:32 PDT 2020, Rishabh Bhatnagar wrote:

> Add devicetree binding for Secure Subsystem remote processor
> support in remoteproc framework. This describes all the resources
> needed by SPSS to boot and handle crash and shutdown scenarios.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
> ---
>  .../devicetree/bindings/remoteproc/qcom,spss.yaml  | 125 +++++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,spss.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,spss.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,spss.yaml
> new file mode 100644
> index 0000000..9ca7947a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,spss.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/qcom,spss.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SPSS Peripheral Image Loader
> +
> +maintainers:
> +  - Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
> +description: |
> +  This document defines the binding for a component that loads and boots firmware
> +  on the Qualcomm Secure Peripheral Processor. This processor is booted in the
> +  bootloader stage and it attaches itself to linux later on in the boot process.
> +
> +properties:
> +  compatible:
> +    enum:
> +      "qcom,sm8250-spss-pas"
> +
> +  reg:
> +    items:
> +      - description: IRQ status register
> +      - description: IRQ clear register
> +      - description: IRQ mask register
> +      - description: Error register
> +      - description: Error spare register
> +
> +  reg-names:
> +    items:
> +      - const: sp2soc_irq_status
> +      - const: sp2soc_irq_clr
> +      - const: sp2soc_irq_mask
> +      - const: rmb_err
> +      - const: rmb_err_spare2
> +
> +  interrupts:
> +    maxitems: 1
> +    items:
> +      - description: rx interrupt
> +
> +  clocks:
> +    items:
> +      - description:
> +                    reference to the xo clock to be held on behalf
> +                    of the booting Hexagon core
> +
> +  clock-names:
> +    items:
> +      - const: xo
> +
> +  cx-supply: true
> +
> +  px-supply: true
> +
> +  memory-region: true
> +    items:
> +      - description: reference to the reserved-memory for the SPSS
> +
> +  qcom,spss-scsr-bits:
> +    - description: Bits that are set by remote processor in the irq status
> +                   register region to represent different states during
> +                   boot process
> +
> +  child-node:
> +    description: Subnode named either "smd-edge" or "glink-edge" that
> +                 describes the communication edge, channels and devices
> +                 related to the SPSS.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - cx-supply
> +  - px-supply
> +  - memory-region
> +  - qcom,spss-scsr-bits
> +
> +
> +examples:
> +  - |
> +    spss {

remoteproc@188101c but probably rather remoteproc@1880000



> +        compatible = "qcom,sm8250-spss-pil";

s/pil/pas/

> +        reg = <0x188101c 0x4>,
> +                <0x1881024 0x4>,
> +                <0x1881028 0x4>,
> +                <0x188103c 0x4>,
> +                <0x1882014 0x4>;

As noted in the driver review, these are all from the same block, map it
once.

> +        reg-names = "sp2soc_irq_status", "sp2soc_irq_clr",
> +                    "sp2soc_irq_mask", "rmb_err", "rmb_err_spare2";
> +        interrupts = <0 352 1>;

interrupts = <GIC_SPI 352 IRQ_TYPE_EDGE_RISING>;

> +
> +        cx-supply = <&VDD_CX_LEVEL>;

These are power domains.

> +        cx-uV-uA = <RPMH_REGULATOR_LEVEL_TURBO 100000>;

And we'll just vote for max.

> +        px-supply = <&VDD_MX_LEVEL>;
> +        px-uV = <RPMH_REGULATOR_LEVEL_TURBO 100000>;
> +
> +        clocks = <&clock_rpmh RPMH_CXO_CLK>;
> +        clock-names = "xo";
> +        qcom,proxy-clock-names = "xo";

We don't specify this in DT.

> +        status = "ok";

This can be omitted from the example.

> +
> +        memory-region = <&pil_spss_mem>;
> +        qcom,spss-scsr-bits = <24 25>;

As requested, just hard code these in the driver instead.

> +
> +        glink-edge {

This depends on a separate binding, which we haven't yet discussed.
Perhaps worth omitting it for now?

> +                qcom,remote-pid = <8>;
> +                transport = "spss";

The registered subdev should always be of "spss" type, shouldn't be a
need to describe that here.

> +                mboxes = <&sp_scsr 0>;
> +                mbox-names = "spss_spss";
> +                interrupt-parent = <&intsp>;
> +                interrupts = <0 0 IRQ_TYPE_LEVEL_HIGH>;

As you map the entire scsr region in the remoteproc driver, this should
reference the spss rproc node above.

> +
> +                reg = <0x1885008 0x8>,
> +                      <0x1885010 0x4>;
> +                reg-names = "qcom,spss-addr",
> +                            "qcom,spss-size";

And it seems reasonable that we pass this information from the rproc
when we create the subdev, rather than having the glink implementation
dig it out from the scsr.

Regards,
Bjorn

> +
> +                label = "spss";
> +                qcom,glink-label = "spss";
> +        };
> +    };
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project