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

[Xiongfeng Wang]

Hi George,

Thanks for your reply !

On 2019/1/25 15:07, George Cherian wrote:
> Hi Wang,
> 
> On Thu, Jan 24, 2019 at 12:27 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>>
>> +George/Prashanth.
>>
>> Guys please see if you have any objections to this patch. I am not
>> very familiar with this stuff and it would be good to get some
>> feedback from you guys.
>>
>> @Rafael: Do you have any comments on this ?
>>
>> On 17-01-19, 19:00, 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.
> Does your platform support Autonomous Selection mode?
> This register is not valid when autonomous mode is enabled. In such case
> how are you calculating the frequency?

Our platform does not support Autonomous Selection mode.
When it's enabled, this method won't work. But I think the current doesn't support
Autonomous Selection mode. I only found a defition 'AUTO_SEL_ENABLE' in enum cppc_regs
and there is no one using it.

>>>
>>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@xxxxxxxxxx>
>>> ---
>>>  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;
> If there is a pending command in the channel then returning zero
> will give bogus frequency value. You may return the previous written value.

How about returning a error code here ?
In current kernel, when 'cppc_cpufreq_get_rate' returns -EFAULT, 'cpuinfo_cur_freq'
shows a very big value. I was thinking about writing a patch to fix it.

>>> +
>>> +     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
>>> +
>>>  /**
>>>   * 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"
>>> +     default y
>>> +     depends on ACPI_CPPC_CPUFREQ && ARM64
> Do you really want this to be applied to all ARM64? or just only
> for affected HISI platforms?

I was thinking about applying the framwork to all ARM64.
CONF_HISILICON_CPPC_CPUFREQ_WORKAROUND adds the OEM ID for HISI.

>>> +     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 {
> If your intention is to make a generic framework for future extensions
> then you might need to name it differently. Something like
> struct cppc_workaround_info, so that you can extend the same for any
> future workarounds.

Thanks for your advice. I will change it in the next version.

>>> +     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);
> This can be  unsigned int (*get_rate)(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,
>>> +     },
>>> +     {
>>
>> This should be:
>>
>>         }, {
>>
>>> +             .oem_id         = "HISI  ",
>>> +             .oem_table_id   = "HIP08   ",
>>> +             .oem_revision   = 0,
>>> +             .get = hisi_cppc_cpufreq_get_rate,
>>> +     },
>>> +#endif
>>> +     {},
>>> +};
>>> +
>>> +static void cppc_check_get_rate_workaround(struct cpufreq_driver *cppc_cpufreq_driver)
> This function can be renamed to cppc_check_workaround to make it
> generic also can have a
> proper return value so that the caller can validate.

Thanks, I will change it and all above.
If I want to apply it to ARM64, I think I will need a CONFIG_ARM64 here.

>>> +{
>>> +     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*/
>>> --
>>> 1.7.12.4
>>
>> --
>> viresh
> -George
> 
> .
> 
Thanks,
Xiongfeng