Re: [PATCH v2] soc: qcom: rpmpd: Add MSM8939 power-domains

[Stephan Gerhold <stephan@gerhold.net>]

Hi,

Thanks for sending a v2!

On Wed, Sep 16, 2020 at 03:49:24PM +0800, Jun Nie wrote:
> Add the shared modemcx/cx/mx power-domains found on MSM8939.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../devicetree/bindings/power/qcom,rpmpd.yaml |  1 +
>  drivers/soc/qcom/rpmpd.c                      | 27 +++++++++++++++++++
>  include/dt-bindings/power/qcom-rpmpd.h        | 10 +++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml b/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
> index 8058955fb3b9..ce9b556ac155 100644
> --- a/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
> +++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.yaml
> @@ -16,6 +16,7 @@ description:
>  properties:
>    compatible:
>      enum:
> +      - qcom,msm8939-rpmpd
>        - qcom,msm8976-rpmpd
>        - qcom,msm8996-rpmpd
>        - qcom,msm8998-rpmpd
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> index f2168e4259b2..829f51881e41 100644
> --- a/drivers/soc/qcom/rpmpd.c
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -220,7 +220,34 @@ static const struct rpmpd_desc qcs404_desc = {
>  	.max_state = RPM_SMD_LEVEL_BINNING,
>  };
>  
> +/* msm8939 RPM Power Domains */
> +DEFINE_RPMPD_PAIR(msm8939, vddmd, vddmd_ao, SMPA, CORNER, 1);
> +DEFINE_RPMPD_VFC(msm8939, vddmd_vfc, SMPA, 1);
> +
> +DEFINE_RPMPD_PAIR(msm8939, vddcx, vddcx_ao, SMPA, CORNER, 2);
> +DEFINE_RPMPD_VFC(msm8939, vddcx_vfc, SMPA, 2);
> +
> +DEFINE_RPMPD_PAIR(msm8939, vddmx, vddmx_ao, LDOA, CORNER, 3);
> +
> +static struct rpmpd *msm8939_rpmpds[] = {
> +	[MSM8939_VDDMDCX] =	&msm8939_vddmd,
> +	[MSM8939_VDDMDCX_AO] =	&msm8939_vddmd_ao,
> +	[MSM8939_VDDMDCX_VFC] =	&msm8939_vddmd_vfc,
> +	[MSM8939_VDDCX] =	&msm8939_vddcx,
> +	[MSM8939_VDDCX_AO] =	&msm8939_vddcx_ao,
> +	[MSM8939_VDDCX_VFC] =	&msm8939_vddcx_vfc,
> +	[MSM8939_VDDMX] =	&msm8939_vddmx,
> +	[MSM8939_VDDMX_AO] =	&msm8939_vddmx_ao,
> +};
> +
> +static const struct rpmpd_desc msm8939_desc = {
> +	.rpmpds = msm8939_rpmpds,
> +	.num_pds = ARRAY_SIZE(msm8939_rpmpds),
> +	.max_state = MSM8939_VDDMX_AO,
> +};

For consistent ordering this block should be also above
/* msm8976 RPM Power Domains */

Also: .max_state still looks wrong to me. It has nothing to do with the
number of power domains (MSM8939_VDDMX_AO). Instead, it is supposed to
be the maximum possible peformance state (in your case: voltage corner).

As far as I know, all SoCs using voltage corners use the same set of
corners, a value from 0-6 for the following corners [1]:

    0. None
    1. Retention
    2. SVS Krait
    3. SVS SoC
    4. Normal/Nominal
    5. Turbo
    6. Super Turbo

(The corners are shifted by one in the msm-3.10 driver...)

Super Turbo (= 6) is the .max_state you want to set here.
That's why I suggested setting .max_state to MAX_8996_RPMPD_STATE (= 6).

In my patch series for MSM8916, I renamed MAX_8996_RPMPD_STATE to
MAX_CORNER_RPMPD_STATE [2]. Maybe that makes it a bit more clear.

Thanks!
Stephan

[1]: https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/include/linux/regulator/rpm-smd-regulator.h?h=LA.BR.1.2.9.1-02310-8x16.0#n31
[2]: https://lore.kernel.org/linux-arm-msm/20200916104135.25085-2-stephan@gerhold.net/