Re: [PATCH 05/14] dt-bindings: remoteproc: Add Qualcomm RPM processor/subsystem

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Stephan Gerhold <stephan@gerhold.net>,
	Bjorn Andersson <andersson@kernel.org>
Cc: Andy Gross <agross@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org
Subject: Re: [PATCH 05/14] dt-bindings: remoteproc: Add Qualcomm RPM processor/subsystem
Date: Tue, 6 Jun 2023 08:36:10 +0200	[thread overview]
Message-ID: <2a479fd0-1d3c-2c28-d2fd-86a8cf610cf9@linaro.org> (raw)
In-Reply-To: <20230531-rpm-rproc-v1-5-e0a3b6de1f14@gerhold.net>

On 05/06/2023 09:08, Stephan Gerhold wrote:
> On Qualcomm platforms, most subsystems (e.g. audio/modem DSP) are
> described as remote processors in the device tree, with a dedicated
> node where properties and services related to them can be described.
> 
> The Resource Power Manager (RPM) is also such a subsystem, with a
> remote processor that is running a special firmware. Unfortunately,
> the RPM never got a dedicated node representing it properly in the
> device tree. Most of the RPM services are described below a top-level
> /smd or /rpm-glink node.

Then what is rpm-requests? This is the true RPM. It looks like you now
duplicate half of it in a node above. Unless you want here to describe
ways to communicate with the RPM, not the RPM itself.

> However, SMD/GLINK is just one of the communication channels to the RPM
> firmware. For example, the MPM interrupt functionality provided by the
> RPM does not use SMD/GLINK but writes directly to a special memory
> region allocated by the RPM firmware in combination with a mailbox.
> Currently there is no good place in the device tree to describe this
> functionality. It doesn't belong below SMD/GLINK but it's not an
> independent top-level device either.
> 
> Introduce a new "qcom,rpm-proc" compatible that allows describing the
> RPM as a remote processor/subsystem like all others. The SMD/GLINK node
> is moved to a "smd-edge"/"glink-edge" subnode consistent with other
> existing bindings. Additional subnodes (e.g. interrupt-controller for
> MPM, rpm-master-stats) can be also added there.

If this was about to stay, you should also update the qcom,smd.yaml, so
there will be no duplication.

> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../bindings/remoteproc/qcom,rpm-proc.yaml         | 125 +++++++++++++++++++++
>  1 file changed, 125 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,rpm-proc.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,rpm-proc.yaml
> new file mode 100644
> index 000000000000..c06dd4f66503
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,rpm-proc.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,rpm-proc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Resource Power Manager (RPM) Processor/Subsystem
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> +
> +description:
> +  Resource Power Manager (RPM) subsystem found in various Qualcomm platforms.
> +  The RPM allows each component in the system to vote for state of the system
> +  resources, such as clocks, regulators and bus frequencies. rpm-proc is the
> +  top-level device tree node that groups all the RPM functionality together.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,apq8084-rpm-proc
> +          - qcom,ipq6018-rpm-proc
> +          - qcom,ipq9574-rpm-proc
> +          - qcom,mdm9607-rpm-proc
> +          - qcom,msm8226-rpm-proc
> +          - qcom,msm8610-rpm-proc
> +          - qcom,msm8909-rpm-proc
> +          - qcom,msm8916-rpm-proc
> +          - qcom,msm8917-rpm-proc
> +          - qcom,msm8936-rpm-proc
> +          - qcom,msm8937-rpm-proc
> +          - qcom,msm8952-rpm-proc
> +          - qcom,msm8953-rpm-proc
> +          - qcom,msm8974-rpm-proc
> +          - qcom,msm8976-rpm-proc
> +          - qcom,msm8994-rpm-proc
> +          - qcom,msm8996-rpm-proc
> +          - qcom,msm8998-rpm-proc
> +          - qcom,qcm2290-rpm-proc
> +          - qcom,qcs404-rpm-proc
> +          - qcom,sdm660-rpm-proc
> +          - qcom,sm6115-rpm-proc
> +          - qcom,sm6125-rpm-proc
> +          - qcom,sm6375-rpm-proc
> +      - const: qcom,rpm-proc
> +
> +  smd-edge:
> +    $ref: /schemas/remoteproc/qcom,smd-edge.yaml#
> +    description:
> +      Qualcomm Shared Memory subnode which represents communication edge,
> +      channels and devices related to the RPM subsystem.
> +
> +  glink-rpm:
> +    $ref: /schemas/remoteproc/qcom,glink-rpm-edge.yaml#
> +    description:
> +      Qualcomm G-Link subnode which represents communication edge,
> +      channels and devices related to the RPM subsystem.
> +
> +  interrupt-controller:
> +    type: object
> +    $ref: /schemas/interrupt-controller/qcom,mpm.yaml#
> +    description:
> +      MSM Power Manager (MPM) interrupt controller that monitors interrupts
> +      when the system is asleep.

Isn't this a service of RPM?

> +
> +  master-stats:
> +    $ref: /schemas/soc/qcom/qcom,rpm-master-stats.yaml#
> +    description:
> +      Subsystem-level low-power mode statistics provided by RPM.

The same question.

> +
> +required:
> +  - compatible
> +
> +oneOf:
> +  - required:
> +      - smd-edge
> +  - required:
> +      - glink-rpm
> +
> +additionalProperties: false
> +
> +examples:
> +  # SMD
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    remoteproc-rpm {

remoteproc

> +      compatible = "qcom,msm8916-rpm-proc", "qcom,rpm-proc";
> +
> +      smd-edge {
> +        interrupts = <GIC_SPI 168 IRQ_TYPE_EDGE_RISING>;
> +        qcom,ipc = <&apcs 8 0>;
> +        qcom,smd-edge = <15>;
> +
> +        rpm-requests {
> +          compatible = "qcom,rpm-msm8916";
> +          qcom,smd-channels = "rpm_requests";
> +          /* ... */
> +        };
> +      };
> +    };
> +  # GLINK
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    remoteproc-rpm {

remoteproc

> +      compatible = "qcom,qcm2290-rpm-proc", "qcom,rpm-proc";
> +
> +      glink-rpm {
> +        compatible = "qcom,glink-rpm";
> +        interrupts = <GIC_SPI 194 IRQ_TYPE_EDGE_RISING>;
> +        qcom,rpm-msg-ram = <&rpm_msg_ram>;
> +        mboxes = <&apcs_glb 0>;
> +
> +        rpm-requests {
> +          compatible = "qcom,rpm-qcm2290";
> +          qcom,glink-channels = "rpm_requests";
> +          /* ... */
> +        };
> +      };
> +    };
> 

Best regards,
Krzysztof