Re: [PATCH 1/8] dt-bindings: remoteproc: qcom,msm8916-mss-pil: Add MSM8226

From: Stephan Gerhold <stephan@gerhold.net>
To: "Matti Lehtimäki" <matti.lehtimaki@gmail.com>
Cc: linux-arm-msm@vger.kernel.org,
	~postmarketos/upstreaming@lists.sr.ht,
	phone-devel@vger.kernel.org, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/8] dt-bindings: remoteproc: qcom,msm8916-mss-pil: Add MSM8226
Date: Thu, 12 Jan 2023 23:03:15 +0100	[thread overview]
Message-ID: <Y8CDo0jI/ygpnNtR@gerhold.net> (raw)
In-Reply-To: <20230112202612.791455-2-matti.lehtimaki@gmail.com>

Hi Matti,

On Thu, Jan 12, 2023 at 10:26:04PM +0200, Matti Lehtimäki wrote:
> Adds support for platforms with only single power domain.

This sentence is a bit misleading. MSM8226 also has both CX and MX power
domains. The difference is only the way they are exposed by the firmware
and the drivers in Linux.

The RPM firmware allows Linux to vote for either
 - Voltages (exposed as regulators in Linux), or
 - Performance states/"voltage corners" (exposed as power domains in Linux)

For the hardware there is no difference: When using power domains the
performance states are simply translated to corresponding voltages
within the RPM firmware.

All newer platforms have moved towards using power domains for CX and
MX, so for consistency it would be preferable to do the same for MSM8226
and MSM8974. Perhaps the RPM firmware even allows using them with
voltage corners? In that case you could just add PM8226 L3 to rpmpd and
use it as power domain like on other platforms.

For some reason I assumed this is the case for MSM8974 2.5 years ago.
I have to admit I no longer remember why, and verifying this reliably is
probably hard... :/

But the VDD_MX setup looks identical for MSM8974 and MSM8226 to me, so
please also apply the same changes for MSM8974. I would also appreciate
a small comment in the commit message that the MX voltage rail is still
represented as regulator on these platforms. Also, perhaps this should
even be a separate patch given that it kind of fixes what I added for
MSM8974 back then.

> Adds support for external power block headswitch (BHS) registers
> 
> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
> ---
>  .../remoteproc/qcom,msm8916-mss-pil.yaml      | 41 +++++++++++++++++--
>  1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml
> index 6e6e69ad9cd7..6a921f2711b2 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,msm8916-mss-pil.yaml
> [...]
> @@ -106,6 +108,15 @@ properties:
>      items:
>        - const: stop
>  
> +  qcom,ext-bhs-reg:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: External power block headswitch (BHS) register
> +                 (only valid for qcom,msm8226-mss-pil)
> +    items:
> +      - items:
> +          - description: phandle to external BHS syscon region
> +          - description: offset to the external BHS register
> +

Please disallow this (qcom,ext-bhs-reg: false) for everything except
qcom,msm8226-mss-pil.

>    qcom,halt-regs:
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      description:
> @@ -205,13 +216,35 @@ allOf:
>        required:
>          - power-domains
>          - power-domain-names
> -    else:
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,msm8909-mss-pil
> +              - qcom,msm8916-mss-pil
> +              - qcom,msm8974-mss-pil
> +    then:
>        properties:
>          power-domains:
>            maxItems: 2
>          power-domain-names:
>            maxItems: 2
>  

You also need to add minItems here now.

> +  - if:
> +      properties:
> +        compatible:
> +          const: qcom,msm8226-mss-pil
> +    then:
> +      properties:
> +        power-domains:
> +          maxItems: 1
> +        power-domain-names:
> +          maxItems: 1
> +      required:
> +        - qcom,ext-bhs-reg

And here you need to add the mx-supply as required, since you don't have
it as power domain.

Thanks,
Stephan