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

[Xiongfeng Wang]

Hi, Rafael

Thanks for your reply and advice below.
Sorry, I'am out of the office because of the Chinese spring festival.
I will send another version when I come back. It's 14th February.

Thanks,
Xiongfeng

On 2019/1/31 7:42, Rafael J. Wysocki wrote:
> On Thursday, January 17, 2019 12:00:11 PM CET 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@xxxxxxxxxx>
> 
> Overall, I would do it differently.
> 
>> ---
>>  drivers/acpi/cppc_acpi.c       | 29 ++++++++++++++++++++
>>  drivers/cpufreq/Kconfig.arm    |  7 +++++
>>  drivers/cpufreq/cppc_cpufreq.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>>  include/acpi/cppc_acpi.h       |  4 +++
>>  4 files changed, 102 insertions(+)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 217a782..0cdaf7e 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1050,6 +1050,35 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>>  	return ret_val;
>>  }
>>  
>> +#ifdef CONFIG_HISILICON_CPPC_CPUFREQ_WORKAROUND
>> +/*
>> + * We reuse the desired performance register to store the real performance
>> + * calculated by the platform.
>> + */
>> +u64 hisi_cppc_get_real_perf(unsigned int cpunum)
>> +{
>> +	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
>> +	struct cpc_register_resource *desired_reg;
>> +	u64 desired_perf;
>> +	int ret;
>> +
>> +	/*
>> +	 * Make sure the platform has finished the frequency adjust
>> +	 * and wrote the real performance in desired performance register
>> +	 */
>> +	ret = check_pcc_chan(pcc_ss_id, false);
>> +	if (ret)
>> +		return 0;
>> +
>> +	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
>> +	cpc_read(cpunum, desired_reg, &desired_perf);
>> +
>> +	return desired_perf;
>> +}
>> +EXPORT_SYMBOL_GPL(hisi_cppc_get_real_perf);
>> +#endif
> 
> I would provide library routine to read desired_perf and it doesn't
> need to have anything to do with HiSi at this level.  Also, it doesn't
> need to be compiled conditionally.  Just call it cppc_get_desired_perf()
> or similar.
> 
> Then, you can use it to implement the quirk in the cpufreq driver.
> 
>> +
>>  /**
>>   * cppc_get_perf_caps - Get a CPUs performance capabilities.
>>   * @cpunum: CPU from which to get capabilities info.
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 688f102..236bd07 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -18,6 +18,13 @@ config ACPI_CPPC_CPUFREQ
>>  
>>  	  If in doubt, say N.
>>  
>> +config HISILICON_CPPC_CPUFREQ_WORKAROUND
>> +	bool "Workaround for Hisilicon CPPC Cpufreq"
> 
> What is the reason for this to be a user-selectable option?
> 
>> +	default y
>> +	depends on ACPI_CPPC_CPUFREQ && ARM64
>> +	help
>> +	  This option enables a workaround for Hisilicon CPPC Cpufreq.
>> +
>>  config ARM_ARMADA_37XX_CPUFREQ
>>  	tristate "Armada 37xx CPUFreq support"
>>  	depends on ARCH_MVEBU && CPUFREQ_DT
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index fd25c21c..b910e84 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -33,6 +33,13 @@
>>  /* Offest in the DMI processor structure for the max frequency */
>>  #define DMI_PROCESSOR_MAX_SPEED  0x14
>>  
>> +struct cppc_get_rate_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)(unsigned int cpu);
>> +};
>> +
>>  /*
>>   * These structs contain information parsed from per CPU
>>   * ACPI _CPC structures.
>> @@ -357,6 +364,59 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>>  	.name = "cppc_cpufreq",
>>  };
>>  
>> +#ifdef CONFIG_HISILICON_CPPC_CPUFREQ_WORKAROUND
>> +/*
>> + * When the platform does not support delivered performance counter or
>> + * 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 *cpu = all_cpu_data[cpunum];
>> +	u64 desired_perf = hisi_cppc_get_real_perf(cpunum);
>> +
>> +	return cppc_cpufreq_perf_to_khz(cpu, desired_perf);
>> +}
>> +#endif
>> +
>> +static struct cppc_get_rate_workaround_info wa_info[] = {
>> +#ifdef CONFIG_HISILICON_CPPC_CPUFREQ_WORKAROUND
>> +	{
>> +		.oem_id		= "HISI  ",
>> +		.oem_table_id	= "HIP07   ",
>> +		.oem_revision	= 0,
>> +		.get = hisi_cppc_cpufreq_get_rate,
>> +	},
>> +	{
>> +		.oem_id		= "HISI  ",
>> +		.oem_table_id	= "HIP08   ",
>> +		.oem_revision	= 0,
>> +		.get = hisi_cppc_cpufreq_get_rate,
>> +	},
>> +#endif
> 
> Is this #ifdef really necessary?
> 
>> +	{},
>> +};
>> +
>> +static void cppc_check_get_rate_workaround(struct cpufreq_driver *cppc_cpufreq_driver)
>> +{
>> +	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) - 1; 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_cpufreq_driver->get = wa_info[i].get;
>> +			return;
>> +		}
>> +	}
>> +}
>>  static int __init cppc_cpufreq_init(void)
>>  {
>>  	int i, ret = 0;
>> @@ -386,6 +446,8 @@ static int __init cppc_cpufreq_init(void)
>>  		goto out;
>>  	}
>>  
>> +	cppc_check_get_rate_workaround(&cppc_cpufreq_driver);
>> +
>>  	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
>>  	if (ret)
>>  		goto out;
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 4f34734..be7400b 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -146,4 +146,8 @@ struct cppc_cpudata {
>>  extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
>>  extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
>>  
>> +#ifdef CONFIG_HISILICON_CPPC_CPUFREQ_WORKAROUND
>> +u64 hisi_cppc_get_real_perf(unsigned int cpunum);
>> +#endif
>> +
>>  #endif /* _CPPC_ACPI_H*/
>>
> 
> 
> 
> .
>