| Message ID | 20230202140005.1.I4b30aaa027c73372ec4068cc0f0dc665af8b938d@changeid (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cpufreq: qcom-hw: Fix cpufreq_driver->get() for non-LMH systems | expand |
On 2.02.2023 23:00, Douglas Anderson wrote: > On a sc7180-based Chromebook, when I go to > /sys/devices/system/cpu/cpu0/cpufreq I can see: > > cpuinfo_cur_freq:2995200 > cpuinfo_max_freq:1804800 > scaling_available_frequencies:300000 576000 ... 1708800 1804800 > scaling_cur_freq:1804800 > scaling_max_freq:1804800 > > As you can see the `cpuinfo_cur_freq` is bogus. It turns out that this > bogus info started showing up as of commit 205f5e984d30 ("cpufreq: > qcom-hw: Fix the frequency returned by cpufreq_driver->get()"). That > commit seems to assume that everyone is on the LMH bandwagon, but > sc7180 isn't. > > Let's go back to the old code in the case where LMH isn't used. > > Fixes: 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Actually I hit the exact same issue when working on CPRh-aware cpufreq with manual OSM programming.. LMh gets enabled by the firmware on most recent platforms, but it's not the case for some old-timers. I figured that adding a bool broken_lmh_freq in driver data would be a good middleground between reverting that patch and ignoring the issue, because it *does* matter what this function reports on LMh- enabled platforms (yes, the subsystems are bluepilled between each other and OSM/EPSS does not know the *real* throttled frequency), but obviously we don't want to report 2.99Ghz otherwise.. I think 7280 had an issue where a SoC-specific compatible was not introduced when the DT part was first merged, same goes for 6115. 6115 does have firmware-enabled LMh, not sure about 7280. In case you wanted to go that route, I think it would be suitable to add a blacklist of retroactively-broken platforms (match-by-machine- compatible; don't scream at me bindings folks, I guess that's the least messy solution) in addition to either matching the SoC-specific compatible to epss_broken_lmh_driver_data. Or we can forget about old DTs and just bind qcom,sc7180-cpufreq-hw (and 7280, maybe? please check.) to this new driver data without checking the machine compatible. Konrad > > drivers/cpufreq/qcom-cpufreq-hw.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 9505a812d6a1..957cf6bb8c05 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -143,40 +143,42 @@ static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) > return lval * xo_rate; > } > > -/* Get the current frequency of the CPU (after throttling) */ > -static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) > +/* Get the frequency requested by the cpufreq core for the CPU */ > +static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) > { > struct qcom_cpufreq_data *data; > + const struct qcom_cpufreq_soc_data *soc_data; > struct cpufreq_policy *policy; > + unsigned int index; > > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) > return 0; > > data = policy->driver_data; > + soc_data = qcom_cpufreq.soc_data; > > - return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; > + index = readl_relaxed(data->base + soc_data->reg_perf_state); > + index = min(index, LUT_MAX_ENTRIES - 1); > + > + return policy->freq_table[index].frequency; > } > > -/* Get the frequency requested by the cpufreq core for the CPU */ > -static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) > +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) > { > struct qcom_cpufreq_data *data; > - const struct qcom_cpufreq_soc_data *soc_data; > struct cpufreq_policy *policy; > - unsigned int index; > > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) > return 0; > > data = policy->driver_data; > - soc_data = qcom_cpufreq.soc_data; > > - index = readl_relaxed(data->base + soc_data->reg_perf_state); > - index = min(index, LUT_MAX_ENTRIES - 1); > + if (data->throttle_irq >= 0) > + return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; > > - return policy->freq_table[index].frequency; > + return qcom_cpufreq_get_freq(cpu); > } > > static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
On 2.02.2023 23:35, Konrad Dybcio wrote: > > > On 2.02.2023 23:00, Douglas Anderson wrote: >> On a sc7180-based Chromebook, when I go to >> /sys/devices/system/cpu/cpu0/cpufreq I can see: >> >> cpuinfo_cur_freq:2995200 >> cpuinfo_max_freq:1804800 >> scaling_available_frequencies:300000 576000 ... 1708800 1804800 >> scaling_cur_freq:1804800 >> scaling_max_freq:1804800 >> >> As you can see the `cpuinfo_cur_freq` is bogus. It turns out that this >> bogus info started showing up as of commit 205f5e984d30 ("cpufreq: >> qcom-hw: Fix the frequency returned by cpufreq_driver->get()"). That >> commit seems to assume that everyone is on the LMH bandwagon, but >> sc7180 isn't. >> >> Let's go back to the old code in the case where LMH isn't used. >> >> Fixes: 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- > Actually I hit the exact same issue when working on CPRh-aware > cpufreq with manual OSM programming.. LMh gets enabled by the firmware > on most recent platforms, but it's not the case for some old-timers. > Ignore this email, I can't read. Konrad > I figured that adding a bool broken_lmh_freq in driver data would be > a good middleground between reverting that patch and ignoring the > issue, because it *does* matter what this function reports on LMh- > enabled platforms (yes, the subsystems are bluepilled between each > other and OSM/EPSS does not know the *real* throttled frequency), > but obviously we don't want to report 2.99Ghz otherwise.. > > I think 7280 had an issue where a SoC-specific compatible was not > introduced when the DT part was first merged, same goes for 6115. > 6115 does have firmware-enabled LMh, not sure about 7280. In case > you wanted to go that route, I think it would be suitable to add > a blacklist of retroactively-broken platforms (match-by-machine- > compatible; don't scream at me bindings folks, I guess that's the > least messy solution) in addition to either matching the SoC-specific > compatible to epss_broken_lmh_driver_data. > > Or we can forget about old DTs and just bind qcom,sc7180-cpufreq-hw > (and 7280, maybe? please check.) to this new driver data without > checking the machine compatible. > > > > Konrad >> >> drivers/cpufreq/qcom-cpufreq-hw.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c >> index 9505a812d6a1..957cf6bb8c05 100644 >> --- a/drivers/cpufreq/qcom-cpufreq-hw.c >> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c >> @@ -143,40 +143,42 @@ static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) >> return lval * xo_rate; >> } >> >> -/* Get the current frequency of the CPU (after throttling) */ >> -static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) >> +/* Get the frequency requested by the cpufreq core for the CPU */ >> +static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) >> { >> struct qcom_cpufreq_data *data; >> + const struct qcom_cpufreq_soc_data *soc_data; >> struct cpufreq_policy *policy; >> + unsigned int index; >> >> policy = cpufreq_cpu_get_raw(cpu); >> if (!policy) >> return 0; >> >> data = policy->driver_data; >> + soc_data = qcom_cpufreq.soc_data; >> >> - return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; >> + index = readl_relaxed(data->base + soc_data->reg_perf_state); >> + index = min(index, LUT_MAX_ENTRIES - 1); >> + >> + return policy->freq_table[index].frequency; >> } >> >> -/* Get the frequency requested by the cpufreq core for the CPU */ >> -static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) >> +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) >> { >> struct qcom_cpufreq_data *data; >> - const struct qcom_cpufreq_soc_data *soc_data; >> struct cpufreq_policy *policy; >> - unsigned int index; >> >> policy = cpufreq_cpu_get_raw(cpu); >> if (!policy) >> return 0; >> >> data = policy->driver_data; >> - soc_data = qcom_cpufreq.soc_data; >> >> - index = readl_relaxed(data->base + soc_data->reg_perf_state); >> - index = min(index, LUT_MAX_ENTRIES - 1); >> + if (data->throttle_irq >= 0) >> + return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; >> >> - return policy->freq_table[index].frequency; >> + return qcom_cpufreq_get_freq(cpu); >> } >> >> static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
On 2.02.2023 23:00, Douglas Anderson wrote: > On a sc7180-based Chromebook, when I go to > /sys/devices/system/cpu/cpu0/cpufreq I can see: > > cpuinfo_cur_freq:2995200 > cpuinfo_max_freq:1804800 > scaling_available_frequencies:300000 576000 ... 1708800 1804800 > scaling_cur_freq:1804800 > scaling_max_freq:1804800 > > As you can see the `cpuinfo_cur_freq` is bogus. It turns out that this > bogus info started showing up as of commit 205f5e984d30 ("cpufreq: > qcom-hw: Fix the frequency returned by cpufreq_driver->get()"). That > commit seems to assume that everyone is on the LMH bandwagon, but > sc7180 isn't. > > Let's go back to the old code in the case where LMH isn't used. > > Fixes: 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- I read it again, this time properly. Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad > > drivers/cpufreq/qcom-cpufreq-hw.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 9505a812d6a1..957cf6bb8c05 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -143,40 +143,42 @@ static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) > return lval * xo_rate; > } > > -/* Get the current frequency of the CPU (after throttling) */ > -static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) > +/* Get the frequency requested by the cpufreq core for the CPU */ > +static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) > { > struct qcom_cpufreq_data *data; > + const struct qcom_cpufreq_soc_data *soc_data; > struct cpufreq_policy *policy; > + unsigned int index; > > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) > return 0; > > data = policy->driver_data; > + soc_data = qcom_cpufreq.soc_data; > > - return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; > + index = readl_relaxed(data->base + soc_data->reg_perf_state); > + index = min(index, LUT_MAX_ENTRIES - 1); > + > + return policy->freq_table[index].frequency; > } > > -/* Get the frequency requested by the cpufreq core for the CPU */ > -static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) > +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) > { > struct qcom_cpufreq_data *data; > - const struct qcom_cpufreq_soc_data *soc_data; > struct cpufreq_policy *policy; > - unsigned int index; > > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) > return 0; > > data = policy->driver_data; > - soc_data = qcom_cpufreq.soc_data; > > - index = readl_relaxed(data->base + soc_data->reg_perf_state); > - index = min(index, LUT_MAX_ENTRIES - 1); > + if (data->throttle_irq >= 0) > + return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; > > - return policy->freq_table[index].frequency; > + return qcom_cpufreq_get_freq(cpu); > } > > static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
On Thu, Feb 02, 2023 at 02:00:23PM -0800, Douglas Anderson wrote: > On a sc7180-based Chromebook, when I go to > /sys/devices/system/cpu/cpu0/cpufreq I can see: > > cpuinfo_cur_freq:2995200 > cpuinfo_max_freq:1804800 > scaling_available_frequencies:300000 576000 ... 1708800 1804800 > scaling_cur_freq:1804800 > scaling_max_freq:1804800 > > As you can see the `cpuinfo_cur_freq` is bogus. It turns out that this > bogus info started showing up as of commit 205f5e984d30 ("cpufreq: > qcom-hw: Fix the frequency returned by cpufreq_driver->get()"). That > commit seems to assume that everyone is on the LMH bandwagon, but > sc7180 isn't. > Ah, missed that part. > Let's go back to the old code in the case where LMH isn't used. > Thanks for fixing! > Fixes: 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> Thanks, Mani > --- > > drivers/cpufreq/qcom-cpufreq-hw.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > index 9505a812d6a1..957cf6bb8c05 100644 > --- a/drivers/cpufreq/qcom-cpufreq-hw.c > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c > @@ -143,40 +143,42 @@ static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) > return lval * xo_rate; > } > > -/* Get the current frequency of the CPU (after throttling) */ > -static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) > +/* Get the frequency requested by the cpufreq core for the CPU */ > +static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) > { > struct qcom_cpufreq_data *data; > + const struct qcom_cpufreq_soc_data *soc_data; > struct cpufreq_policy *policy; > + unsigned int index; > > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) > return 0; > > data = policy->driver_data; > + soc_data = qcom_cpufreq.soc_data; > > - return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; > + index = readl_relaxed(data->base + soc_data->reg_perf_state); > + index = min(index, LUT_MAX_ENTRIES - 1); > + > + return policy->freq_table[index].frequency; > } > > -/* Get the frequency requested by the cpufreq core for the CPU */ > -static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) > +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) > { > struct qcom_cpufreq_data *data; > - const struct qcom_cpufreq_soc_data *soc_data; > struct cpufreq_policy *policy; > - unsigned int index; > > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) > return 0; > > data = policy->driver_data; > - soc_data = qcom_cpufreq.soc_data; > > - index = readl_relaxed(data->base + soc_data->reg_perf_state); > - index = min(index, LUT_MAX_ENTRIES - 1); > + if (data->throttle_irq >= 0) > + return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; > > - return policy->freq_table[index].frequency; > + return qcom_cpufreq_get_freq(cpu); > } > > static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy, > -- > 2.39.1.519.gcb327c4b5f-goog >
On 02-02-23, 14:00, Douglas Anderson wrote: > On a sc7180-based Chromebook, when I go to > /sys/devices/system/cpu/cpu0/cpufreq I can see: > > cpuinfo_cur_freq:2995200 > cpuinfo_max_freq:1804800 > scaling_available_frequencies:300000 576000 ... 1708800 1804800 > scaling_cur_freq:1804800 > scaling_max_freq:1804800 > > As you can see the `cpuinfo_cur_freq` is bogus. It turns out that this > bogus info started showing up as of commit 205f5e984d30 ("cpufreq: > qcom-hw: Fix the frequency returned by cpufreq_driver->get()"). That > commit seems to assume that everyone is on the LMH bandwagon, but > sc7180 isn't. > > Let's go back to the old code in the case where LMH isn't used. > > Fixes: 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") This is incorrect. Fixes: c72cf0cb1d77 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") Applied. Thanks.
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index 9505a812d6a1..957cf6bb8c05 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -143,40 +143,42 @@ static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data) return lval * xo_rate; } -/* Get the current frequency of the CPU (after throttling) */ -static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) +/* Get the frequency requested by the cpufreq core for the CPU */ +static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) { struct qcom_cpufreq_data *data; + const struct qcom_cpufreq_soc_data *soc_data; struct cpufreq_policy *policy; + unsigned int index; policy = cpufreq_cpu_get_raw(cpu); if (!policy) return 0; data = policy->driver_data; + soc_data = qcom_cpufreq.soc_data; - return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; + index = readl_relaxed(data->base + soc_data->reg_perf_state); + index = min(index, LUT_MAX_ENTRIES - 1); + + return policy->freq_table[index].frequency; } -/* Get the frequency requested by the cpufreq core for the CPU */ -static unsigned int qcom_cpufreq_get_freq(unsigned int cpu) +static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) { struct qcom_cpufreq_data *data; - const struct qcom_cpufreq_soc_data *soc_data; struct cpufreq_policy *policy; - unsigned int index; policy = cpufreq_cpu_get_raw(cpu); if (!policy) return 0; data = policy->driver_data; - soc_data = qcom_cpufreq.soc_data; - index = readl_relaxed(data->base + soc_data->reg_perf_state); - index = min(index, LUT_MAX_ENTRIES - 1); + if (data->throttle_irq >= 0) + return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ; - return policy->freq_table[index].frequency; + return qcom_cpufreq_get_freq(cpu); } static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
On a sc7180-based Chromebook, when I go to /sys/devices/system/cpu/cpu0/cpufreq I can see: cpuinfo_cur_freq:2995200 cpuinfo_max_freq:1804800 scaling_available_frequencies:300000 576000 ... 1708800 1804800 scaling_cur_freq:1804800 scaling_max_freq:1804800 As you can see the `cpuinfo_cur_freq` is bogus. It turns out that this bogus info started showing up as of commit 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()"). That commit seems to assume that everyone is on the LMH bandwagon, but sc7180 isn't. Let's go back to the old code in the case where LMH isn't used. Fixes: 205f5e984d30 ("cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/cpufreq/qcom-cpufreq-hw.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)