Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 10:45:09 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Beata Michalska <beata.michalska@arm.com>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
        mark.rutland@arm.com, will@kernel.org, rafael@kernel.org,
        sudeep.holla@arm.com, ionela.voinescu@arm.com, sumitg@nvidia.com,
        yang@os.amperecomputing.com, Len Brown <len.brown@intel.com>
Subject: Re: [PATCH] arm64: Provide an AMU-based version of
 arch_freq_get_on_cpu
Message-ID: <20230608051509.h4a6gn572mjgdusv@vireshk-i7>
References: <20230606155754.245998-1-beata.michalska@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230606155754.245998-1-beata.michalska@arm.com>
Precedence: bulk

+Len

On 06-06-23, 16:57, Beata Michalska wrote:
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> +unsigned int arch_freq_get_on_cpu(int cpu)
> +{
> +	unsigned int freq;
> +	u64 scale;
> +
> +	if (!cpumask_test_cpu(cpu, amu_fie_cpus))
> +		return 0;
> +
> +	if (!housekeeping_cpu(cpu, HK_TYPE_TICK)) {

I am not sure what we are doing in the `if` block here, at least a comment would
be useful.

> +		struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +		int ref_cpu = nr_cpu_ids;
> +
> +		if (cpumask_intersects(housekeeping_cpumask(HK_TYPE_TICK),
> +				       policy->cpus))
> +			ref_cpu = cpumask_nth_and(cpu, policy->cpus,
> +						  housekeeping_cpumask(HK_TYPE_TICK));
> +		cpufreq_cpu_put(policy);
> +		if (ref_cpu >= nr_cpu_ids)
> +			return 0;
> +		cpu = ref_cpu;
> +	}

A blank line here please.

> +	/*
> +	 * Reversed computation to the one used to determine
> +	 * the arch_freq_scale value
> +	 * (see amu_scale_freq_tick for details)
> +	 */
> +	scale = per_cpu(arch_freq_scale, cpu);
> +	scale *= cpufreq_get_hw_max_freq(cpu);
> +	freq = scale >> SCHED_CAPACITY_SHIFT;
> +
> +	return freq;
> +}
> +
>  #ifdef CONFIG_ACPI_CPPC_LIB
>  #include <acpi/cppc_acpi.h>
>  
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6b52ebe5a890..9f2cf45bf190 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -710,7 +710,8 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
>  	ssize_t ret;
>  	unsigned int freq;
>  
> -	freq = arch_freq_get_on_cpu(policy->cpu);
> +	freq = !cpufreq_driver->get ? arch_freq_get_on_cpu(policy->cpu)
> +				    : 0;

You may have changed the logic for X86 parts as well here. For a x86 platform
with setpolicy() and get() callbacks, we will not call arch_freq_get_on_cpu()
anymore ?

>  	if (freq)
>  		ret = sprintf(buf, "%u\n", freq);
>  	else if (cpufreq_driver->setpolicy && cpufreq_driver->get)
> @@ -747,7 +748,11 @@ store_one(scaling_max_freq, max);
>  static ssize_t show_cpuinfo_cur_freq(struct cpufreq_policy *policy,
>  					char *buf)
>  {
> -	unsigned int cur_freq = __cpufreq_get(policy);
> +	unsigned int cur_freq;
> +
> +	cur_freq = arch_freq_get_on_cpu(policy->cpu);
> +	if (!cur_freq)
> +		cur_freq = __cpufreq_get(policy);

For this and the above change, I am not sure what is the right thing to do.

From Len's commit [1]:

    Here we provide an x86 routine to make this calculation
    on supported hardware, and use it in preference to any
    driver driver-specific cpufreq_driver.get() routine.

I am not sure why Len updated `show_scaling_cur_freq()` and not
`show_cpuinfo_cur_freq()` ? Maybe we should update both these routines ?

Also, I don't think this is something that should have different logic for ARM
and X86, we should be consistent here as a cpufreq decision. Since both these
routines are reached via a read operation to a sysfs file, we shouldn't be
concerned about performance too.

What about doing this for both the routines, for all platforms now:

	cur_freq = arch_freq_get_on_cpu(policy->cpu);
	if (!cur_freq)
                ... get freq via policy->get() or policy->cur;

-- 
viresh

[1] commit f8475cef9008 ("x86: use common aperfmperf_khz_on_cpu() to calculate KHz using APERF/MPERF")