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?
> >
> > 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.
> > +
> > + 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?
> > + 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.
> > + 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.
> > +{
> > + 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
