[PATCH v2 0/2] Work around for Hisilicon CPPC cpufreq

[PATCH v2 0/2] Work around for Hisilicon CPPC cpufreq

[Xiongfeng Wang]

Hisilicon chips do not support delivered performance counter register
and reference performance counter register. But the platform can
calculate the real performance using its own method. This patch provide
a workaround for this problem, and other platforms can also use this
workaround framework. We reuse the desired performance register to
store the real performance calculated by the platform. After the
platform finished the frequency adjust, it gets the real performance and
writes it into desired performance register. OS can use it to calculate
the real frequency.

Xiongfeng Wang (2):
  ACPI / CPPC: Add a helper to get desired performance
  cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

 drivers/acpi/cppc_acpi.c       | 38 +++++++++++++++++++++++
 drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 include/acpi/cppc_acpi.h       |  1 +
 3 files changed, 109 insertions(+)

-- 
1.7.12.4

[PATCH v2 1/2] ACPI / CPPC: Add a helper to get desired performance

[Xiongfeng Wang]

This patch add a helper to get the value of desired performance
register.

Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/acpi/cppc_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/acpi/cppc_acpi.h |  1 +
 2 files changed, 39 insertions(+)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 217a782..93588c5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -1051,6 +1051,44 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 }
 
 /**
+ * cppc_get_desired_perf - Get the value of desired performance register.
+ * @cpunum: CPU from which to get desired performance.
+ * @desired_perf: address of a variable to store the returned desired performance
+ *
+ * Return: 0 for success, -EIO otherwise.
+ */
+int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
+	struct cpc_register_resource *desired_reg;
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int regs_in_pcc = 0;
+
+	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
+
+	if (CPC_IN_PCC(desired_reg)) {
+		if (pcc_ss_id < 0)
+			return -EIO;
+		pcc_ss_data = pcc_data[pcc_ss_id];
+		down_write(&pcc_ss_data->pcc_lock);
+		regs_in_pcc = 1;
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
+			up_write(&pcc_ss_data->pcc_lock);
+			return -EIO;
+		}
+	}
+
+	cpc_read(cpunum, desired_reg, desired_perf);
+
+	if (regs_in_pcc)
+		up_write(&pcc_ss_data->pcc_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
+
+/**
  * cppc_get_perf_caps - Get a CPUs performance capabilities.
  * @cpunum: CPU from which to get capabilities info.
  * @perf_caps: ptr to cppc_perf_caps. See cppc_acpi.h
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 4f34734..ba6fd72 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -137,6 +137,7 @@ struct cppc_cpudata {
 	cpumask_var_t shared_cpu_map;
 };
 
+extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
-- 
1.7.12.4

Re: [PATCH v2 1/2] ACPI / CPPC: Add a helper to get desired performance

[Rafael J. Wysocki]

On Thu, Feb 14, 2019 at 8:46 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> This patch add a helper to get the value of desired performance
> register.
>
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/acpi/cppc_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
>  include/acpi/cppc_acpi.h |  1 +
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 217a782..93588c5 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1051,6 +1051,44 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>  }
>
>  /**
> + * cppc_get_desired_perf - Get the value of desired performance register.
> + * @cpunum: CPU from which to get desired performance.
> + * @desired_perf: address of a variable to store the returned desired performance
> + *
> + * Return: 0 for success, -EIO otherwise.
> + */
> +int cppc_get_desired_perf(int cpunum, u64 *desired_perf)
> +{
> +       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> +       int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> +       struct cpc_register_resource *desired_reg;
> +       struct cppc_pcc_data *pcc_ss_data = NULL;
> +       int regs_in_pcc = 0;

It would be better to use bool here, but it would be even better to
not try to avoid calling cpc_read() in two branches.

> +
> +       desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
> +
> +       if (CPC_IN_PCC(desired_reg)) {

    int ret = 0;

> +               if (pcc_ss_id < 0)
> +                       return -EIO;
> +               pcc_ss_data = pcc_data[pcc_ss_id];
> +               down_write(&pcc_ss_data->pcc_lock);

    if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
            cpc_read(cpunum, desired_reg, desired_perf);
    else
            ret = -EIO;

    up_write(&pcc_ss_data->pcc_lock);
    return ret;

> +               regs_in_pcc = 1;
> +               if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
> +                       up_write(&pcc_ss_data->pcc_lock);
> +                       return -EIO;
> +               }
> +       }

cpc_read(cpunum, desired_reg, desired_perf);
return 0;

> +
> +       cpc_read(cpunum, desired_reg, desired_perf);
> +
> +       if (regs_in_pcc)
> +               up_write(&pcc_ss_data->pcc_lock);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(cppc_get_desired_perf);
> +
> +/**
>   * cppc_get_perf_caps - Get a CPUs performance capabilities.
>   * @cpunum: CPU from which to get capabilities info.
>   * @perf_caps: ptr to cppc_perf_caps. See cppc_acpi.h

[PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[Xiongfeng Wang]

Hisilicon chips do not support delivered performance counter register
and reference performance counter register. But the platform can
calculate the real performance using its own method. This patch provide
a workaround for this problem, and other platforms can also use this
workaround framework. We reuse the desired performance register to
store the real performance calculated by the platform. After the
platform finished the frequency adjust, it gets the real performance and
writes it into desired performance register. OS can use it to calculate
the real frequency.

Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index fd25c21c..da96fec 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -33,6 +33,16 @@
 /* Offest in the DMI processor structure for the max frequency */
 #define DMI_PROCESSOR_MAX_SPEED  0x14
 
+struct cppc_workaround_info {
+	char oem_id[ACPI_OEM_ID_SIZE +1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+	unsigned int (*get_rate)(unsigned int cpu);
+};
+
+/* CPPC workaround for get_rate callback */
+unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
+
 /*
  * These structs contain information parsed from per CPU
  * ACPI _CPC structures.
@@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
 	struct cppc_cpudata *cpu = all_cpu_data[cpunum];
 	int ret;
 
+	if (cppc_wa_get_rate)
+		return cppc_wa_get_rate(cpunum);
+
 	ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
 	if (ret)
 		return ret;
@@ -357,6 +370,61 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
 	.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 struct cppc_workaround_info wa_info[] = {
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP07   ",
+		.oem_revision	= 0,
+		.get_rate = hisi_cppc_cpufreq_get_rate,
+	}, {
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP08   ",
+		.oem_revision	= 0,
+		.get_rate = hisi_cppc_cpufreq_get_rate,
+	}
+};
+
+static int cppc_check_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 -EINVAL;
+
+	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) {
+			cppc_wa_get_rate = wa_info[i].get_rate;
+			return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
 static int __init cppc_cpufreq_init(void)
 {
 	int i, ret = 0;
@@ -386,6 +454,8 @@ static int __init cppc_cpufreq_init(void)
 		goto out;
 	}
 
+	cppc_check_workaround();
+
 	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
 	if (ret)
 		goto out;
-- 
1.7.12.4

Re: [PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[Rafael J. Wysocki]

On Thu, Feb 14, 2019 at 8:46 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
> Hisilicon chips do not support delivered performance counter register
> and reference performance counter register. But the platform can
> calculate the real performance using its own method. This patch provide
> a workaround for this problem, and other platforms can also use this
> workaround framework. We reuse the desired performance register to
> store the real performance calculated by the platform. After the
> platform finished the frequency adjust, it gets the real performance and
> writes it into desired performance register. OS can use it to calculate
> the real frequency.
>
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index fd25c21c..da96fec 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -33,6 +33,16 @@
>  /* Offest in the DMI processor structure for the max frequency */
>  #define DMI_PROCESSOR_MAX_SPEED  0x14
>
> +struct cppc_workaround_info {
> +       char oem_id[ACPI_OEM_ID_SIZE +1];
> +       char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> +       u32 oem_revision;
> +       unsigned int (*get_rate)(unsigned int cpu);
> +};
> +
> +/* CPPC workaround for get_rate callback */
> +unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
> +

First off, please don't split the workaround material into two parts.
IOW, the other new function added below can go here just fine IMO.

>  /*
>   * These structs contain information parsed from per CPU
>   * ACPI _CPC structures.
> @@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>         struct cppc_cpudata *cpu = all_cpu_data[cpunum];
>         int ret;
>
> +       if (cppc_wa_get_rate)
> +               return cppc_wa_get_rate(cpunum);

Second, what is the value of using the function pointer above?

All we need for now is a flag to indicate whether or not to call
hisi_cppc_cpufreq_get_rate() here and return its return value.

> +
>         ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>         if (ret)
>                 return ret;
> @@ -357,6 +370,61 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>         .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 struct cppc_workaround_info wa_info[] = {
> +       {
> +               .oem_id         = "HISI  ",
> +               .oem_table_id   = "HIP07   ",
> +               .oem_revision   = 0,
> +               .get_rate = hisi_cppc_cpufreq_get_rate,
> +       }, {
> +               .oem_id         = "HISI  ",
> +               .oem_table_id   = "HIP08   ",
> +               .oem_revision   = 0,
> +               .get_rate = hisi_cppc_cpufreq_get_rate,
> +       }
> +};
> +
> +static int cppc_check_workaround(void)

And this function can be a void one.


> +{
> +       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 -EINVAL;
> +
> +       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) {
> +                       cppc_wa_get_rate = wa_info[i].get_rate;
> +                       return 0;
> +               }
> +       }
> +
> +       return -ENODEV;
> +}
> +
>  static int __init cppc_cpufreq_init(void)
>  {
>         int i, ret = 0;
> @@ -386,6 +454,8 @@ static int __init cppc_cpufreq_init(void)
>                 goto out;
>         }
>
> +       cppc_check_workaround();
> +
>         ret = cpufreq_register_driver(&cppc_cpufreq_driver);
>         if (ret)
>                 goto out;
> --

Re: [PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[Xiongfeng Wang]

On 2019/2/14 18:58, Rafael J. Wysocki wrote:
> On Thu, Feb 14, 2019 at 8:46 AM Xiongfeng Wang
> <wangxiongfeng2@huawei.com> wrote:
>>
>> Hisilicon chips do not support delivered performance counter register
>> and reference performance counter register. But the platform can
>> calculate the real performance using its own method. This patch provide
>> a workaround for this problem, and other platforms can also use this
>> workaround framework. We reuse the desired performance register to
>> store the real performance calculated by the platform. After the
>> platform finished the frequency adjust, it gets the real performance and
>> writes it into desired performance register. OS can use it to calculate
>> the real frequency.
>>
>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>> ---
>>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index fd25c21c..da96fec 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -33,6 +33,16 @@
>>  /* Offest in the DMI processor structure for the max frequency */
>>  #define DMI_PROCESSOR_MAX_SPEED  0x14
>>
>> +struct cppc_workaround_info {
>> +       char oem_id[ACPI_OEM_ID_SIZE +1];
>> +       char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>> +       u32 oem_revision;
>> +       unsigned int (*get_rate)(unsigned int cpu);
>> +};
>> +
>> +/* CPPC workaround for get_rate callback */
>> +unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
>> +
> 
> First off, please don't split the workaround material into two parts.
> IOW, the other new function added below can go here just fine IMO.
> 
>>  /*
>>   * These structs contain information parsed from per CPU
>>   * ACPI _CPC structures.
>> @@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>>         struct cppc_cpudata *cpu = all_cpu_data[cpunum];
>>         int ret;
>>
>> +       if (cppc_wa_get_rate)
>> +               return cppc_wa_get_rate(cpunum);
> 
> Second, what is the value of using the function pointer above?
> 
> All we need for now is a flag to indicate whether or not to call
> hisi_cppc_cpufreq_get_rate() here and return its return value.

How about adding a pointer of 'struct cppc_workaround_info' to indicate whether we have
found a matches workaround ?
If I use a flag, I will need another variable to indicate which item of the workaround array 'wa_info'
to use.

Thanks,
Xiongfeng

> 
>> +
>>         ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>>         if (ret)
>>                 return ret;
>> @@ -357,6 +370,61 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>>         .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 struct cppc_workaround_info wa_info[] = {
>> +       {
>> +               .oem_id         = "HISI  ",
>> +               .oem_table_id   = "HIP07   ",
>> +               .oem_revision   = 0,
>> +               .get_rate = hisi_cppc_cpufreq_get_rate,
>> +       }, {
>> +               .oem_id         = "HISI  ",
>> +               .oem_table_id   = "HIP08   ",
>> +               .oem_revision   = 0,
>> +               .get_rate = hisi_cppc_cpufreq_get_rate,
>> +       }
>> +};
>> +
>> +static int cppc_check_workaround(void)
> 
> And this function can be a void one.
> 
> 
>> +{
>> +       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 -EINVAL;
>> +
>> +       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) {
>> +                       cppc_wa_get_rate = wa_info[i].get_rate;
>> +                       return 0;
>> +               }
>> +       }
>> +
>> +       return -ENODEV;
>> +}
>> +
>>  static int __init cppc_cpufreq_init(void)
>>  {
>>         int i, ret = 0;
>> @@ -386,6 +454,8 @@ static int __init cppc_cpufreq_init(void)
>>                 goto out;
>>         }
>>
>> +       cppc_check_workaround();
>> +
>>         ret = cpufreq_register_driver(&cppc_cpufreq_driver);
>>         if (ret)
>>                 goto out;
>> --
> 
> .
> 

Re: [PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[Rafael J. Wysocki]

On Thursday, February 14, 2019 2:58:21 PM CET Xiongfeng Wang wrote:
> 
> On 2019/2/14 18:58, Rafael J. Wysocki wrote:
> > On Thu, Feb 14, 2019 at 8:46 AM Xiongfeng Wang
> > <wangxiongfeng2@huawei.com> wrote:
> >>
> >> Hisilicon chips do not support delivered performance counter register
> >> and reference performance counter register. But the platform can
> >> calculate the real performance using its own method. This patch provide
> >> a workaround for this problem, and other platforms can also use this
> >> workaround framework. We reuse the desired performance register to
> >> store the real performance calculated by the platform. After the
> >> platform finished the frequency adjust, it gets the real performance and
> >> writes it into desired performance register. OS can use it to calculate
> >> the real frequency.
> >>
> >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> >> ---
> >>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 70 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> >> index fd25c21c..da96fec 100644
> >> --- a/drivers/cpufreq/cppc_cpufreq.c
> >> +++ b/drivers/cpufreq/cppc_cpufreq.c
> >> @@ -33,6 +33,16 @@
> >>  /* Offest in the DMI processor structure for the max frequency */
> >>  #define DMI_PROCESSOR_MAX_SPEED  0x14
> >>
> >> +struct cppc_workaround_info {
> >> +       char oem_id[ACPI_OEM_ID_SIZE +1];
> >> +       char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> >> +       u32 oem_revision;
> >> +       unsigned int (*get_rate)(unsigned int cpu);
> >> +};
> >> +
> >> +/* CPPC workaround for get_rate callback */
> >> +unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
> >> +
> > 
> > First off, please don't split the workaround material into two parts.
> > IOW, the other new function added below can go here just fine IMO.
> > 
> >>  /*
> >>   * These structs contain information parsed from per CPU
> >>   * ACPI _CPC structures.
> >> @@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
> >>         struct cppc_cpudata *cpu = all_cpu_data[cpunum];
> >>         int ret;
> >>
> >> +       if (cppc_wa_get_rate)
> >> +               return cppc_wa_get_rate(cpunum);
> > 
> > Second, what is the value of using the function pointer above?
> > 
> > All we need for now is a flag to indicate whether or not to call
> > hisi_cppc_cpufreq_get_rate() here and return its return value.
> 
> How about adding a pointer of 'struct cppc_workaround_info' to indicate whether we have
> found a matches workaround ?
> If I use a flag, I will need another variable to indicate which item of the workaround array 'wa_info'
> to use.

And why do you need to distinguish one of them from the other?

Re: [PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[Xiongfeng Wang]

On 2019/2/15 7:15, Rafael J. Wysocki wrote:
> On Thursday, February 14, 2019 2:58:21 PM CET Xiongfeng Wang wrote:
>>
>> On 2019/2/14 18:58, Rafael J. Wysocki wrote:
>>> On Thu, Feb 14, 2019 at 8:46 AM Xiongfeng Wang
>>> <wangxiongfeng2@huawei.com> wrote:
>>>>
>>>> Hisilicon chips do not support delivered performance counter register
>>>> and reference performance counter register. But the platform can
>>>> calculate the real performance using its own method. This patch provide
>>>> a workaround for this problem, and other platforms can also use this
>>>> workaround framework. We reuse the desired performance register to
>>>> store the real performance calculated by the platform. After the
>>>> platform finished the frequency adjust, it gets the real performance and
>>>> writes it into desired performance register. OS can use it to calculate
>>>> the real frequency.
>>>>
>>>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>>>> ---
>>>>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 70 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>>> index fd25c21c..da96fec 100644
>>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>>> @@ -33,6 +33,16 @@
>>>>  /* Offest in the DMI processor structure for the max frequency */
>>>>  #define DMI_PROCESSOR_MAX_SPEED  0x14
>>>>
>>>> +struct cppc_workaround_info {
>>>> +       char oem_id[ACPI_OEM_ID_SIZE +1];
>>>> +       char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>>>> +       u32 oem_revision;
>>>> +       unsigned int (*get_rate)(unsigned int cpu);
>>>> +};
>>>> +
>>>> +/* CPPC workaround for get_rate callback */
>>>> +unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
>>>> +
>>>
>>> First off, please don't split the workaround material into two parts.
>>> IOW, the other new function added below can go here just fine IMO.
>>>
>>>>  /*
>>>>   * These structs contain information parsed from per CPU
>>>>   * ACPI _CPC structures.
>>>> @@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>>>>         struct cppc_cpudata *cpu = all_cpu_data[cpunum];
>>>>         int ret;
>>>>
>>>> +       if (cppc_wa_get_rate)
>>>> +               return cppc_wa_get_rate(cpunum);
>>>
>>> Second, what is the value of using the function pointer above?
>>>
>>> All we need for now is a flag to indicate whether or not to call
>>> hisi_cppc_cpufreq_get_rate() here and return its return value.
>>
>> How about adding a pointer of 'struct cppc_workaround_info' to indicate whether we have
>> found a matches workaround ?
>> If I use a flag, I will need another variable to indicate which item of the workaround array 'wa_info'
>> to use.
> 
> And why do you need to distinguish one of them from the other?
>

I was thinking about future extension. Different platform may have different implementation
of 'get_rate' in the future.

> 
> .
> 

Re: [PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[Rafael J. Wysocki]

On Fri, Feb 15, 2019 at 2:25 AM Xiongfeng Wang
<wangxiongfeng2@huawei.com> wrote:
>
>
>
> On 2019/2/15 7:15, Rafael J. Wysocki wrote:
> > On Thursday, February 14, 2019 2:58:21 PM CET Xiongfeng Wang wrote:
> >>
> >> On 2019/2/14 18:58, Rafael J. Wysocki wrote:
> >>> On Thu, Feb 14, 2019 at 8:46 AM Xiongfeng Wang
> >>> <wangxiongfeng2@huawei.com> wrote:
> >>>>
> >>>> Hisilicon chips do not support delivered performance counter register
> >>>> and reference performance counter register. But the platform can
> >>>> calculate the real performance using its own method. This patch provide
> >>>> a workaround for this problem, and other platforms can also use this
> >>>> workaround framework. We reuse the desired performance register to
> >>>> store the real performance calculated by the platform. After the
> >>>> platform finished the frequency adjust, it gets the real performance and
> >>>> writes it into desired performance register. OS can use it to calculate
> >>>> the real frequency.
> >>>>
> >>>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> >>>> ---
> >>>>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 70 insertions(+)
> >>>>
> >>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> >>>> index fd25c21c..da96fec 100644
> >>>> --- a/drivers/cpufreq/cppc_cpufreq.c
> >>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
> >>>> @@ -33,6 +33,16 @@
> >>>>  /* Offest in the DMI processor structure for the max frequency */
> >>>>  #define DMI_PROCESSOR_MAX_SPEED  0x14
> >>>>
> >>>> +struct cppc_workaround_info {
> >>>> +       char oem_id[ACPI_OEM_ID_SIZE +1];
> >>>> +       char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> >>>> +       u32 oem_revision;
> >>>> +       unsigned int (*get_rate)(unsigned int cpu);
> >>>> +};
> >>>> +
> >>>> +/* CPPC workaround for get_rate callback */
> >>>> +unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
> >>>> +
> >>>
> >>> First off, please don't split the workaround material into two parts.
> >>> IOW, the other new function added below can go here just fine IMO.
> >>>
> >>>>  /*
> >>>>   * These structs contain information parsed from per CPU
> >>>>   * ACPI _CPC structures.
> >>>> @@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
> >>>>         struct cppc_cpudata *cpu = all_cpu_data[cpunum];
> >>>>         int ret;
> >>>>
> >>>> +       if (cppc_wa_get_rate)
> >>>> +               return cppc_wa_get_rate(cpunum);
> >>>
> >>> Second, what is the value of using the function pointer above?
> >>>
> >>> All we need for now is a flag to indicate whether or not to call
> >>> hisi_cppc_cpufreq_get_rate() here and return its return value.
> >>
> >> How about adding a pointer of 'struct cppc_workaround_info' to indicate whether we have
> >> found a matches workaround ?
> >> If I use a flag, I will need another variable to indicate which item of the workaround array 'wa_info'
> >> to use.
> >
> > And why do you need to distinguish one of them from the other?
> >
>
> I was thinking about future extension. Different platform may have different implementation
> of 'get_rate' in the future.

That's right, but whoever adds this in the future may change the
driver accordingly.  They will have to make changes to it anyway.

Re: [PATCH v2 2/2] cpufreq / cppc: Work around for Hisilicon CPPC cpufreq

[John Garry]

On 14/02/2019 07:46, Xiongfeng Wang wrote:
> Hisilicon chips do not support delivered performance counter register
> and reference performance counter register. But the platform can
> calculate the real performance using its own method. This patch provide
> a workaround for this problem, and other platforms can also use this
> workaround framework. We reuse the desired performance register to
> store the real performance calculated by the platform. After the
> platform finished the frequency adjust, it gets the real performance and
> writes it into desired performance register. OS can use it to calculate
> the real frequency.
>
> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index fd25c21c..da96fec 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -33,6 +33,16 @@
>  /* Offest in the DMI processor structure for the max frequency */
>  #define DMI_PROCESSOR_MAX_SPEED  0x14
>
> +struct cppc_workaround_info {
> +	char oem_id[ACPI_OEM_ID_SIZE +1];
> +	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> +	u32 oem_revision;
> +	unsigned int (*get_rate)(unsigned int cpu);
> +};
> +
> +/* CPPC workaround for get_rate callback */
> +unsigned int (*cppc_wa_get_rate)(unsigned int cpu);
> +
>  /*
>   * These structs contain information parsed from per CPU
>   * ACPI _CPC structures.
> @@ -334,6 +344,9 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>  	struct cppc_cpudata *cpu = all_cpu_data[cpunum];
>  	int ret;
>
> +	if (cppc_wa_get_rate)
> +		return cppc_wa_get_rate(cpunum);
> +
>  	ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>  	if (ret)
>  		return ret;
> @@ -357,6 +370,61 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>  	.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 struct cppc_workaround_info wa_info[] = {
> +	{
> +		.oem_id		= "HISI  ",
> +		.oem_table_id	= "HIP07   ",
> +		.oem_revision	= 0,
> +		.get_rate = hisi_cppc_cpufreq_get_rate,
> +	}, {
> +		.oem_id		= "HISI  ",
> +		.oem_table_id	= "HIP08   ",
> +		.oem_revision	= 0,
> +		.get_rate = hisi_cppc_cpufreq_get_rate,
> +	}
> +};
> +
> +static int cppc_check_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 -EINVAL;
> +
> +	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) {

It would be nice to factor this out into a common helper at some stage, 
as it is almost same as this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c?h=v5.0-rc6#n449

> +			cppc_wa_get_rate = wa_info[i].get_rate;
> +			return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  static int __init cppc_cpufreq_init(void)
>  {
>  	int i, ret = 0;
> @@ -386,6 +454,8 @@ static int __init cppc_cpufreq_init(void)
>  		goto out;
>  	}
>
> +	cppc_check_workaround();
> +
>  	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
>  	if (ret)
>  		goto out;
>

Re: [PATCH v2 0/2] Work around for Hisilicon CPPC cpufreq

[John Garry]

On 14/02/2019 07:46, Xiongfeng Wang wrote:
> Hisilicon chips do not support delivered performance counter register
> and reference performance counter register. But the platform can
> calculate the real performance using its own method. This patch provide
> a workaround for this problem, and other platforms can also use this
> workaround framework. We reuse the desired performance register to
> store the real performance calculated by the platform. After the
> platform finished the frequency adjust, it gets the real performance and
> writes it into desired performance register. OS can use it to calculate
> the real frequency.
>

It would be convenient for reviewers to detail what changed between 
versions in future.

John

> Xiongfeng Wang (2):
>   ACPI / CPPC: Add a helper to get desired performance
>   cpufreq / cppc: Work around for Hisilicon CPPC cpufreq
>
>  drivers/acpi/cppc_acpi.c       | 38 +++++++++++++++++++++++
>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/cppc_acpi.h       |  1 +
>  3 files changed, 109 insertions(+)
>

Re: [PATCH v2 0/2] Work around for Hisilicon CPPC cpufreq

[Xiongfeng Wang]

On 2019/2/14 19:39, John Garry wrote:
> On 14/02/2019 07:46, Xiongfeng Wang wrote:
>> Hisilicon chips do not support delivered performance counter register
>> and reference performance counter register. But the platform can
>> calculate the real performance using its own method. This patch provide
>> a workaround for this problem, and other platforms can also use this
>> workaround framework. We reuse the desired performance register to
>> store the real performance calculated by the platform. After the
>> platform finished the frequency adjust, it gets the real performance and
>> writes it into desired performance register. OS can use it to calculate
>> the real frequency.
>>
> 
> It would be convenient for reviewers to detail what changed between versions in future.

Thanks, I will add changelog next time.

Xiongfeng

> 
> John
> 
>> Xiongfeng Wang (2):
>>   ACPI / CPPC: Add a helper to get desired performance
>>   cpufreq / cppc: Work around for Hisilicon CPPC cpufreq
>>
>>  drivers/acpi/cppc_acpi.c       | 38 +++++++++++++++++++++++
>>  drivers/cpufreq/cppc_cpufreq.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>>  include/acpi/cppc_acpi.h       |  1 +
>>  3 files changed, 109 insertions(+)
>>
> 
> 
> 
> .
>