| Message ID | 20220629130303.3288306-5-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | qcom-cpufreq-nvmem: Add msm8939 with some fixups | expand |
On Wed, Jun 29, 2022 at 02:03:02PM +0100, Bryan O'Donoghue wrote: > At the moment the CPR genpd based code is named after the qcs404 however > msm8936, msm8939 and other antecedent processors of the qcs404 can also > make use of this data. > > Rename it to reflect a more generic use. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> There is another power domain that needs to be scaled together with the CPU frequency on MSM8916 and MSM8939: (VDD)MX. How do you handle that? In downstream this is part of the CPR driver and specified as follows: qcom,vdd-mx-corner-map = <4 5 7>; qcom,vdd-mx-vmin-method = <4>; vdd-mx-supply = <&pm8916_l3_corner_ao>; qcom,vdd-mx-vmax = <7>; qcom,cpr-corner-map = <1 1 2 2 3 3 3 3 3>; /* MSM8916 */ qcom,cpr-corner-map = <1 1 1 2 2 2 2 2 2 2 2 2 2 2 3 3 3 3 3 3 3 3 3 3 3 3 3 3>; /* MSM8939 */ On MSM8916 this means to vote for MX corner 4 (&rpmpd_opp_svs_soc) for the first two frequencies, then corner 5 (&rpmpd_opp_nominal) and corner 7 (&rpmpd_opp_super_turbo) for the remaining CPU frequencies. It's similar for MSM8939, you just have more OPPs there. There was a semi-related discussion about this in [1] (a bit mixed with potential ways how to do CPU frequency scaling without CPR). It's been a while but I think the conclusion back then was that it's easiest to attach both "mx" and "cpr" in this driver and then scale it as part of the OPP table. [1]: https://lore.kernel.org/linux-arm-msm/20200403013119.GB20625@builder.lan/ I'll attach an excerpt of the changes that I used on MSM8916 at the end of the mail. Note that it does not actually work as-is: IIRC at the moment there is still nothing that actually enables the power domains listed for CPUs. The CPR driver does not check that, but rpmpd does and just ignores the votes. I submitted a possible patch for this but it just got stuck at some point because of all the complexity involved: https://lore.kernel.org/linux-pm/20200826093328.88268-1-stephan@gerhold.net/ Shortly said: I think you do not just want "cpr" here but also "mx". Thanks, Stephan static const char *msm8916_genpd_names[] = { "mx", "cpr", NULL }; cpu@0 { power-domains = <&CPU_PD0>, <&rpmpd MSM8916_VDDMX_AO>, <&cpr>; power-domain-names = "psci", "mx", "cpr"; }; cpu_opp_table: cpu-opp-table { compatible = "operating-points-v2-kryo-cpu"; opp-shared; opp-200000000 { opp-hz = /bits/ 64 <200000000>; required-opps = <&rpmpd_opp_svs_soc>, <&cpr_opp1>; }; opp-400000000 { opp-hz = /bits/ 64 <400000000>; required-opps = <&rpmpd_opp_svs_soc>, <&cpr_opp2>; }; opp-533333000 { opp-hz = /bits/ 64 <533333000>; required-opps = <&rpmpd_opp_nominal>, <&cpr_opp3>; }; opp-800000000 { opp-hz = /bits/ 64 <800000000>; required-opps = <&rpmpd_opp_nominal>, <&cpr_opp4>; }; opp-998400000 { opp-hz = /bits/ 64 <998400000>; required-opps = <&rpmpd_opp_super_turbo>, <&cpr_opp5>; }; opp-1094400000 { opp-hz = /bits/ 64 <1094400000>; required-opps = <&rpmpd_opp_super_turbo>, <&cpr_opp6>; }; opp-1152000000 { opp-hz = /bits/ 64 <1152000000>; required-opps = <&rpmpd_opp_super_turbo>, <&cpr_opp7>; }; opp-1209600000 { opp-hz = /bits/ 64 <1209600000>; required-opps = <&rpmpd_opp_super_turbo>, <&cpr_opp8>; }; opp-1363200000 { opp-hz = /bits/ 64 <1363200000>; required-opps = <&rpmpd_opp_super_turbo>, <&cpr_opp9>; }; };
On 29/06/2022 20:11, Stephan Gerhold wrote: > On Wed, Jun 29, 2022 at 02:03:02PM +0100, Bryan O'Donoghue wrote: >> At the moment the CPR genpd based code is named after the qcs404 however >> msm8936, msm8939 and other antecedent processors of the qcs404 can also >> make use of this data. >> >> Rename it to reflect a more generic use. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > There is another power domain that needs to be scaled together with the > CPU frequency on MSM8916 and MSM8939: (VDD)MX. How do you handle that? > Short answer, in another series to enable CPR on 5.x We have code for CPR in a 4.19 tree that works but, it needs more work to be upstream-fit on 5.x. CPR is deliberately omitted here to be submitted later. In this series I'm just switching away from the default cpufreq-dt-platdev which breaks booting to qcom-cpufreq-nvmem. Fair enough ? --- bod
On Thu, Jun 30, 2022 at 05:05:59AM +0100, Bryan O'Donoghue wrote: > On 29/06/2022 20:11, Stephan Gerhold wrote: > > On Wed, Jun 29, 2022 at 02:03:02PM +0100, Bryan O'Donoghue wrote: > > > At the moment the CPR genpd based code is named after the qcs404 however > > > msm8936, msm8939 and other antecedent processors of the qcs404 can also > > > make use of this data. > > > > > > Rename it to reflect a more generic use. > > > > > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > > > There is another power domain that needs to be scaled together with the > > CPU frequency on MSM8916 and MSM8939: (VDD)MX. How do you handle that? > > > > Short answer, in another series to enable CPR on 5.x > > We have code for CPR in a 4.19 tree that works but, it needs more work to be > upstream-fit on 5.x. > > CPR is deliberately omitted here to be submitted later. > I agree with this decision (doing CPR properly is way too complicated to block the entire msm8939.dtsi with this). > In this series I'm just switching away from the default cpufreq-dt-platdev > which breaks booting to qcom-cpufreq-nvmem. > > Fair enough ? ... but then I don't understand: Why do you need this patch set? You only need to attach the "cpr" power domain in qcom-cpufreq-nvmem if you're actually using CPR. I would recommend adding MSM8939 in a similar way to MSM8916, so without using qcom-cpufreq-nvmem for now. Then we can add CPR on both platforms in a similar way later. Thanks, Stephan
On 29/06/2022 16:03, Bryan O'Donoghue wrote: > At the moment the CPR genpd based code is named after the qcs404 however > msm8936, msm8939 and other antecedent processors of the qcs404 can also > make use of this data. > > Rename it to reflect a more generic use. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-nvmem.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c > index 6dfa86971a757..355c8b99e974a 100644 > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c > @@ -252,10 +252,10 @@ static const struct qcom_cpufreq_match_data match_data_krait = { > .get_version = qcom_cpufreq_krait_name_version, > }; > > -static const char *qcs404_genpd_names[] = { "cpr", NULL }; > +static const char *cpr_genpd_names[] = { "cpr", NULL }; As a generic comment, as you are touching this piece of code, code you please move cpr_genpd_names above match_data_kryo? So that all match_data instances can use it. > > -static const struct qcom_cpufreq_match_data match_data_qcs404 = { > - .genpd_names = qcs404_genpd_names, > +static const struct qcom_cpufreq_match_data match_data_cpr_genpd = { > + .genpd_names = cpr_genpd_names, > }; > > static int qcom_cpufreq_probe(struct platform_device *pdev) > @@ -454,7 +454,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,msm8996", .data = &match_data_kryo }, > - { .compatible = "qcom,qcs404", .data = &match_data_qcs404 }, > + { .compatible = "qcom,qcs404", .data = &match_data_cpr_genpd }, > { .compatible = "qcom,ipq8064", .data = &match_data_krait }, > { .compatible = "qcom,apq8064", .data = &match_data_krait }, > { .compatible = "qcom,msm8974", .data = &match_data_krait },
On 13/07/2022 14:50, Dmitry Baryshkov wrote: > On 29/06/2022 16:03, Bryan O'Donoghue wrote: >> At the moment the CPR genpd based code is named after the qcs404 however >> msm8936, msm8939 and other antecedent processors of the qcs404 can also >> make use of this data. >> >> Rename it to reflect a more generic use. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> drivers/cpufreq/qcom-cpufreq-nvmem.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c >> b/drivers/cpufreq/qcom-cpufreq-nvmem.c >> index 6dfa86971a757..355c8b99e974a 100644 >> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c >> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c >> @@ -252,10 +252,10 @@ static const struct qcom_cpufreq_match_data >> match_data_krait = { >> .get_version = qcom_cpufreq_krait_name_version, >> }; >> -static const char *qcs404_genpd_names[] = { "cpr", NULL }; >> +static const char *cpr_genpd_names[] = { "cpr", NULL }; > > As a generic comment, as you are touching this piece of code, code you > please move cpr_genpd_names above match_data_kryo? So that all > match_data instances can use it. NP. This has been dropped in V3 per Stephan's preference to not touch anything CPR related until doing the whole thing for 8939. --- bod
diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c index 6dfa86971a757..355c8b99e974a 100644 --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c @@ -252,10 +252,10 @@ static const struct qcom_cpufreq_match_data match_data_krait = { .get_version = qcom_cpufreq_krait_name_version, }; -static const char *qcs404_genpd_names[] = { "cpr", NULL }; +static const char *cpr_genpd_names[] = { "cpr", NULL }; -static const struct qcom_cpufreq_match_data match_data_qcs404 = { - .genpd_names = qcs404_genpd_names, +static const struct qcom_cpufreq_match_data match_data_cpr_genpd = { + .genpd_names = cpr_genpd_names, }; static int qcom_cpufreq_probe(struct platform_device *pdev) @@ -454,7 +454,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,msm8996", .data = &match_data_kryo }, - { .compatible = "qcom,qcs404", .data = &match_data_qcs404 }, + { .compatible = "qcom,qcs404", .data = &match_data_cpr_genpd }, { .compatible = "qcom,ipq8064", .data = &match_data_krait }, { .compatible = "qcom,apq8064", .data = &match_data_krait }, { .compatible = "qcom,msm8974", .data = &match_data_krait },
At the moment the CPR genpd based code is named after the qcs404 however msm8936, msm8939 and other antecedent processors of the qcs404 can also make use of this data. Rename it to reflect a more generic use. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/cpufreq/qcom-cpufreq-nvmem.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)