linux-kernel - Re: [PATCH v2 3/3] cpufreq: qcom-nvmem: Add MSM8909

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 19 Oct 2023 12:50:03 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Stephan Gerhold <stephan.gerhold@...nkonzept.com>
Cc:     Viresh Kumar <viresh.kumar@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Ilia Lin <ilia.lin@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-pm@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, Stephan Gerhold <stephan@...hold.net>
Subject: Re: [PATCH v2 3/3] cpufreq: qcom-nvmem: Add MSM8909

On Wed, 18 Oct 2023 at 10:06, Stephan Gerhold
<stephan.gerhold@...nkonzept.com> wrote:
>
> When the MSM8909 SoC is used together with the PM8909 PMIC the primary
> power supply for the CPU (VDD_APC) is shared with other components to
> the SoC, namely the VDD_CX power domain typically supplied by the PM8909
> S1 regulator. This means that all votes for necessary performance states
> go via the RPM firmware which collects the requirements from all the
> processors in the SoC. The RPM firmware then chooses the actual voltage
> based on the performance states ("corners"), depending on calibration
> values in the NVMEM and other factors.
>
> The MSM8909 SoC is also sometimes used with the PM8916 or PM660 PMIC.
> In that case there is a dedicated regulator connected to VDD_APC and
> Linux is responsible to do adaptive voltage scaling using CPR (similar
> to the existing code for QCS404).
>
> This difference can be described in the device tree, by either assigning
> the CPU a power domain from RPMPD or from the CPR driver.
>
> Describe this using "perf" as generic power domain name, which is also
> used already for SCMI based platforms.
>
> Also add a simple function that reads the speedbin from a NVMEM cell
> and sets it as-is for opp-supported-hw. The actual bit position can be
> described in the device tree without additional driver changes.
>
> Signed-off-by: Stephan Gerhold <stephan.gerhold@...nkonzept.com>
> ---
>  drivers/cpufreq/qcom-cpufreq-nvmem.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index 3794390089b0..e52031863350 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -59,6 +59,24 @@ struct qcom_cpufreq_drv {
>
>  static struct platform_device *cpufreq_dt_pdev, *cpufreq_pdev;
>
> +static int qcom_cpufreq_simple_get_version(struct device *cpu_dev,
> +                                          struct nvmem_cell *speedbin_nvmem,
> +                                          char **pvs_name,
> +                                          struct qcom_cpufreq_drv *drv)
> +{
> +       u8 *speedbin;
> +
> +       *pvs_name = NULL;
> +       speedbin = nvmem_cell_read(speedbin_nvmem, NULL);
> +       if (IS_ERR(speedbin))
> +               return PTR_ERR(speedbin);
> +
> +       dev_dbg(cpu_dev, "speedbin: %d\n", *speedbin);
> +       drv->versions = 1 << *speedbin;
> +       kfree(speedbin);
> +       return 0;
> +}
> +
>  static void get_krait_bin_format_a(struct device *cpu_dev,
>                                           int *speed, int *pvs, int *pvs_ver,
>                                           u8 *buf)
> @@ -252,6 +270,8 @@ static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
>         return 0;
>  }
>
> +static const char *generic_genpd_names[] = { "perf", NULL };
> +

As discussed, using "perf" as a generic name for a performance domain
for CPUs makes perfect sense to me. However, we need to update the DT
doc bindings for this too. At least we should update
Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml as a
part of $subject series.

At a later step, we should have a look at updating the description for
the power-domain-names in the common
Documentation/devicetree/bindings/arm/cpus.yaml, I think.

>  static const struct qcom_cpufreq_match_data match_data_kryo = {
>         .get_version = qcom_cpufreq_kryo_name_version,
>  };
> @@ -260,6 +280,11 @@ static const struct qcom_cpufreq_match_data match_data_krait = {
>         .get_version = qcom_cpufreq_krait_name_version,
>  };
>
> +static const struct qcom_cpufreq_match_data match_data_msm8909 = {
> +       .get_version = qcom_cpufreq_simple_get_version,
> +       .genpd_names = generic_genpd_names,
> +};
> +
>  static const char *qcs404_genpd_names[] = { "cpr", NULL };
>
>  static const struct qcom_cpufreq_match_data match_data_qcs404 = {
> @@ -434,6 +459,7 @@ static struct platform_driver qcom_cpufreq_driver = {
>
>  static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
>         { .compatible = "qcom,apq8096", .data = &match_data_kryo },
> +       { .compatible = "qcom,msm8909", .data = &match_data_msm8909 },
>         { .compatible = "qcom,msm8996", .data = &match_data_kryo },
>         { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
>         { .compatible = "qcom,ipq8064", .data = &match_data_krait },
>
> --
> 2.39.2
>

Other than the above, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Kind regards
Uffe