Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster

From: Sudeep Holla <sudeep.holla@arm.com>
To: Quentin Perret <quentin.perret@arm.com>
Cc: Chandra Sekhar Lingutla <clingutla@codeaurora.org>,
	catalin.marinas@arm.com, will.deacon@arm.com,
	Jeremy Linton <jeremy.linton@arm.com>,
	Morten Rasmussen <morten.rasmussen@arm.com>,
	dietmar.eggemann@arm.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] arch_topology: Update user supplied capacity to possible cpus in cluster
Date: Tue, 5 Mar 2019 11:36:33 +0000	[thread overview]
Message-ID: <20190305113633.GA2066@e107155-lin> (raw)
In-Reply-To: <20190305112952.zux4bafbnhgnyvlh@queper01-lin>

On Tue, Mar 05, 2019 at 11:29:55AM +0000, Quentin Perret wrote:
> On Tuesday 05 Mar 2019 at 11:13:21 (+0000), Sudeep Holla wrote:
> [...]
> > While I like the idea, I am afraid that linking this to cpufreq policy
> > may not be good. How will we deal with it on systems without CPUfreq ?
>
> Maybe something like this ?
>
> 	policy = cpufreq_cpu_get(cpu);
> 	if (policy) {
> 		for_each_cpu(i, policy->related_cpus) {
> 			/* Update capacity for @i*/
> 		}
> 		cpufreq_cpu_put(policy);
> 	} else {
> 		/* Update capacity for @cpu*/
> 	}
>
> I think it's OK to assume per-cpu capacities w/o CPUFreq. The only
> case where it makes sense to 'bind' the capacity of several CPUs
> together is when they're part of the same perf domain, I think. If you
> don't know what the perf domains are, then there's nothing sensible you
> can do.
>

Makes sense.

> And for the dependency, a large part of the arch_topology driver is
> already dependent on CPUFreq -- it registers a CPUFreq notifier on boot
> to re-scale the CPU capacities depending on the max freq of the various
> policies and so on. So the dependency is already there somehow.
>

Sorry when I mentioned dependency, I meant absence of it needs to be
dealt with. Your suggestion looks good.

> [...]
>
> > I think if there are no valid users of this, we *must* remove it. As I
> > have pointed out in past, giving user such access will need platform
> > knowledge. Though it's debatable topic, firmware providing this
> > information is the only correct solution IMO.
>
> Yeah, if nobody is using it then maybe we can just remove it. Or at
> least we can give it a go and if somebody complains then we can 'fix' it
> with something like my snippet above :-)
>

Happy to Ack code removing it ;). The argument that it can't be provided
by firmware is no longer valid. We already have some dependency on DVFS
data from the firmware for this to be functional correctly.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel