* [PATCH] cpufreq: cppc: Reorder code and remove apply_hisi_workaround variable
@ 2020-06-23 5:15 Viresh Kumar
2020-06-23 7:03 ` Xiongfeng Wang
2020-06-30 18:37 ` Rafael J. Wysocki
0 siblings, 2 replies; 3+ messages in thread
From: Viresh Kumar @ 2020-06-23 5:15 UTC (permalink / raw)
To: Rafael Wysocki, Xiongfeng Wang, Viresh Kumar
Cc: linux-pm, Vincent Guittot, linux-kernel
With the current approach we have an extra check in the
cppc_cpufreq_get_rate() callback, which checks if hisilicon's get rate
implementation should be used instead. While it works fine, the approach
isn't very straight forward, over that we have an extra check in the
routine.
Rearrange code and update the cpufreq driver's get() callback pointer
directly for the hisilicon case. This gets the extra variable is removed
and the extra check isn't required anymore as well.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Xiongfeng Wang, will it be possible for you to give this a try as I
can't really test it locally.
drivers/cpufreq/cppc_cpufreq.c | 91 ++++++++++++++++------------------
1 file changed, 42 insertions(+), 49 deletions(-)
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 257d726a4456..03a21daddbec 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -45,8 +45,6 @@ struct cppc_workaround_oem_info {
u32 oem_revision;
};
-static bool apply_hisi_workaround;
-
static struct cppc_workaround_oem_info wa_info[] = {
{
.oem_id = "HISI ",
@@ -59,50 +57,6 @@ static struct cppc_workaround_oem_info wa_info[] = {
}
};
-static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
- unsigned int perf);
-
-/*
- * HISI platform does not support delivered performance counter and
- * reference performance counter. It can calculate the performance using the
- * platform specific mechanism. We reuse the desired performance register to
- * store the real performance calculated by the platform.
- */
-static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
-{
- struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
- u64 desired_perf;
- int ret;
-
- ret = cppc_get_desired_perf(cpunum, &desired_perf);
- if (ret < 0)
- return -EIO;
-
- return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
-}
-
-static void cppc_check_hisi_workaround(void)
-{
- struct acpi_table_header *tbl;
- acpi_status status = AE_OK;
- int i;
-
- status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
- if (ACPI_FAILURE(status) || !tbl)
- return;
-
- for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
- if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
- !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
- wa_info[i].oem_revision == tbl->oem_revision) {
- apply_hisi_workaround = true;
- break;
- }
- }
-
- acpi_put_table(tbl);
-}
-
/* Callback function used to retrieve the max frequency from DMI */
static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
{
@@ -402,9 +356,6 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
struct cppc_cpudata *cpu = all_cpu_data[cpunum];
int ret;
- if (apply_hisi_workaround)
- return hisi_cppc_cpufreq_get_rate(cpunum);
-
ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
if (ret)
return ret;
@@ -455,6 +406,48 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
.name = "cppc_cpufreq",
};
+/*
+ * HISI platform does not support delivered performance counter and
+ * reference performance counter. It can calculate the performance using the
+ * platform specific mechanism. We reuse the desired performance register to
+ * store the real performance calculated by the platform.
+ */
+static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
+{
+ struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
+ u64 desired_perf;
+ int ret;
+
+ ret = cppc_get_desired_perf(cpunum, &desired_perf);
+ if (ret < 0)
+ return -EIO;
+
+ return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
+}
+
+static void cppc_check_hisi_workaround(void)
+{
+ struct acpi_table_header *tbl;
+ acpi_status status = AE_OK;
+ int i;
+
+ status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
+ if (ACPI_FAILURE(status) || !tbl)
+ return;
+
+ for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
+ if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
+ !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+ wa_info[i].oem_revision == tbl->oem_revision) {
+ /* Overwrite the get() callback */
+ cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
+ break;
+ }
+ }
+
+ acpi_put_table(tbl);
+}
+
static int __init cppc_cpufreq_init(void)
{
int i, ret = 0;
--
2.25.0.rc1.19.g042ed3e048af
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] cpufreq: cppc: Reorder code and remove apply_hisi_workaround variable
2020-06-23 5:15 [PATCH] cpufreq: cppc: Reorder code and remove apply_hisi_workaround variable Viresh Kumar
@ 2020-06-23 7:03 ` Xiongfeng Wang
2020-06-30 18:37 ` Rafael J. Wysocki
1 sibling, 0 replies; 3+ messages in thread
From: Xiongfeng Wang @ 2020-06-23 7:03 UTC (permalink / raw)
To: Viresh Kumar, Rafael Wysocki; +Cc: linux-pm, Vincent Guittot, linux-kernel
Hi Viresh,
On 2020/6/23 13:15, Viresh Kumar wrote:
> With the current approach we have an extra check in the
> cppc_cpufreq_get_rate() callback, which checks if hisilicon's get rate
> implementation should be used instead. While it works fine, the approach
> isn't very straight forward, over that we have an extra check in the
> routine.
>
> Rearrange code and update the cpufreq driver's get() callback pointer
> directly for the hisilicon case. This gets the extra variable is removed
> and the extra check isn't required anymore as well.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Xiongfeng Wang, will it be possible for you to give this a try as I
> can't really test it locally.
I have tested it on D05. It works well.
Tested-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
Thanks,
Xiongfeng
>
> drivers/cpufreq/cppc_cpufreq.c | 91 ++++++++++++++++------------------
> 1 file changed, 42 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 257d726a4456..03a21daddbec 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -45,8 +45,6 @@ struct cppc_workaround_oem_info {
> u32 oem_revision;
> };
>
> -static bool apply_hisi_workaround;
> -
> static struct cppc_workaround_oem_info wa_info[] = {
> {
> .oem_id = "HISI ",
> @@ -59,50 +57,6 @@ static struct cppc_workaround_oem_info wa_info[] = {
> }
> };
>
> -static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
> - unsigned int perf);
> -
> -/*
> - * HISI platform does not support delivered performance counter and
> - * reference performance counter. It can calculate the performance using the
> - * platform specific mechanism. We reuse the desired performance register to
> - * store the real performance calculated by the platform.
> - */
> -static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
> -{
> - struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
> - u64 desired_perf;
> - int ret;
> -
> - ret = cppc_get_desired_perf(cpunum, &desired_perf);
> - if (ret < 0)
> - return -EIO;
> -
> - return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
> -}
> -
> -static void cppc_check_hisi_workaround(void)
> -{
> - struct acpi_table_header *tbl;
> - acpi_status status = AE_OK;
> - int i;
> -
> - status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
> - if (ACPI_FAILURE(status) || !tbl)
> - return;
> -
> - for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> - if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> - !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> - wa_info[i].oem_revision == tbl->oem_revision) {
> - apply_hisi_workaround = true;
> - break;
> - }
> - }
> -
> - acpi_put_table(tbl);
> -}
> -
> /* Callback function used to retrieve the max frequency from DMI */
> static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
> {
> @@ -402,9 +356,6 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
> struct cppc_cpudata *cpu = all_cpu_data[cpunum];
> int ret;
>
> - if (apply_hisi_workaround)
> - return hisi_cppc_cpufreq_get_rate(cpunum);
> -
> ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
> if (ret)
> return ret;
> @@ -455,6 +406,48 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
> .name = "cppc_cpufreq",
> };
>
> +/*
> + * HISI platform does not support delivered performance counter and
> + * reference performance counter. It can calculate the performance using the
> + * platform specific mechanism. We reuse the desired performance register to
> + * store the real performance calculated by the platform.
> + */
> +static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
> +{
> + struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
> + u64 desired_perf;
> + int ret;
> +
> + ret = cppc_get_desired_perf(cpunum, &desired_perf);
> + if (ret < 0)
> + return -EIO;
> +
> + return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
> +}
> +
> +static void cppc_check_hisi_workaround(void)
> +{
> + struct acpi_table_header *tbl;
> + acpi_status status = AE_OK;
> + int i;
> +
> + status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
> + if (ACPI_FAILURE(status) || !tbl)
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> + wa_info[i].oem_revision == tbl->oem_revision) {
> + /* Overwrite the get() callback */
> + cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
> + break;
> + }
> + }
> +
> + acpi_put_table(tbl);
> +}
> +
> static int __init cppc_cpufreq_init(void)
> {
> int i, ret = 0;
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] cpufreq: cppc: Reorder code and remove apply_hisi_workaround variable
2020-06-23 5:15 [PATCH] cpufreq: cppc: Reorder code and remove apply_hisi_workaround variable Viresh Kumar
2020-06-23 7:03 ` Xiongfeng Wang
@ 2020-06-30 18:37 ` Rafael J. Wysocki
1 sibling, 0 replies; 3+ messages in thread
From: Rafael J. Wysocki @ 2020-06-30 18:37 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Xiongfeng Wang, Linux PM, Vincent Guittot,
Linux Kernel Mailing List
On Tue, Jun 23, 2020 at 7:15 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> With the current approach we have an extra check in the
> cppc_cpufreq_get_rate() callback, which checks if hisilicon's get rate
> implementation should be used instead. While it works fine, the approach
> isn't very straight forward, over that we have an extra check in the
> routine.
>
> Rearrange code and update the cpufreq driver's get() callback pointer
> directly for the hisilicon case. This gets the extra variable is removed
> and the extra check isn't required anymore as well.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Xiongfeng Wang, will it be possible for you to give this a try as I
> can't really test it locally.
>
> drivers/cpufreq/cppc_cpufreq.c | 91 ++++++++++++++++------------------
> 1 file changed, 42 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 257d726a4456..03a21daddbec 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -45,8 +45,6 @@ struct cppc_workaround_oem_info {
> u32 oem_revision;
> };
>
> -static bool apply_hisi_workaround;
> -
> static struct cppc_workaround_oem_info wa_info[] = {
> {
> .oem_id = "HISI ",
> @@ -59,50 +57,6 @@ static struct cppc_workaround_oem_info wa_info[] = {
> }
> };
>
> -static unsigned int cppc_cpufreq_perf_to_khz(struct cppc_cpudata *cpu,
> - unsigned int perf);
> -
> -/*
> - * HISI platform does not support delivered performance counter and
> - * reference performance counter. It can calculate the performance using the
> - * platform specific mechanism. We reuse the desired performance register to
> - * store the real performance calculated by the platform.
> - */
> -static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
> -{
> - struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
> - u64 desired_perf;
> - int ret;
> -
> - ret = cppc_get_desired_perf(cpunum, &desired_perf);
> - if (ret < 0)
> - return -EIO;
> -
> - return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
> -}
> -
> -static void cppc_check_hisi_workaround(void)
> -{
> - struct acpi_table_header *tbl;
> - acpi_status status = AE_OK;
> - int i;
> -
> - status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
> - if (ACPI_FAILURE(status) || !tbl)
> - return;
> -
> - for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> - if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> - !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> - wa_info[i].oem_revision == tbl->oem_revision) {
> - apply_hisi_workaround = true;
> - break;
> - }
> - }
> -
> - acpi_put_table(tbl);
> -}
> -
> /* Callback function used to retrieve the max frequency from DMI */
> static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
> {
> @@ -402,9 +356,6 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
> struct cppc_cpudata *cpu = all_cpu_data[cpunum];
> int ret;
>
> - if (apply_hisi_workaround)
> - return hisi_cppc_cpufreq_get_rate(cpunum);
> -
> ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
> if (ret)
> return ret;
> @@ -455,6 +406,48 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
> .name = "cppc_cpufreq",
> };
>
> +/*
> + * HISI platform does not support delivered performance counter and
> + * reference performance counter. It can calculate the performance using the
> + * platform specific mechanism. We reuse the desired performance register to
> + * store the real performance calculated by the platform.
> + */
> +static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpunum)
> +{
> + struct cppc_cpudata *cpudata = all_cpu_data[cpunum];
> + u64 desired_perf;
> + int ret;
> +
> + ret = cppc_get_desired_perf(cpunum, &desired_perf);
> + if (ret < 0)
> + return -EIO;
> +
> + return cppc_cpufreq_perf_to_khz(cpudata, desired_perf);
> +}
> +
> +static void cppc_check_hisi_workaround(void)
> +{
> + struct acpi_table_header *tbl;
> + acpi_status status = AE_OK;
> + int i;
> +
> + status = acpi_get_table(ACPI_SIG_PCCT, 0, &tbl);
> + if (ACPI_FAILURE(status) || !tbl)
> + return;
> +
> + for (i = 0; i < ARRAY_SIZE(wa_info); i++) {
> + if (!memcmp(wa_info[i].oem_id, tbl->oem_id, ACPI_OEM_ID_SIZE) &&
> + !memcmp(wa_info[i].oem_table_id, tbl->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
> + wa_info[i].oem_revision == tbl->oem_revision) {
> + /* Overwrite the get() callback */
> + cppc_cpufreq_driver.get = hisi_cppc_cpufreq_get_rate;
> + break;
> + }
> + }
> +
> + acpi_put_table(tbl);
> +}
> +
> static int __init cppc_cpufreq_init(void)
> {
> int i, ret = 0;
> --
Applied as 5.9 material, thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-30 18:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-23 5:15 [PATCH] cpufreq: cppc: Reorder code and remove apply_hisi_workaround variable Viresh Kumar
2020-06-23 7:03 ` Xiongfeng Wang
2020-06-30 18:37 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).