Re: [PATCH v2 0/3] Arm CMN-600 PMU driver

From: Robin Murphy <robin.murphy@arm.com>
To: Will Deacon <will@kernel.org>, "Zidenberg, Tsahi" <tsahee@amazon.com>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Saidi, Ali" <alisaidi@amazon.com>,
	"harb@amperecomputing.com" <harb@amperecomputing.com>,
	"tuanphan@os.amperecomputing.com"
	<tuanphan@os.amperecomputing.com>,
	"james.yang@arm.com" <james.yang@arm.com>,
	"patrik.berglund@arm.com" <patrik.berglund@arm.com>
Subject: Re: [PATCH v2 0/3] Arm CMN-600 PMU driver
Date: Wed, 14 Oct 2020 17:31:17 +0100	[thread overview]
Message-ID: <b86a8e7e-4def-3cd0-d2c1-b9560304cf15@arm.com> (raw)
In-Reply-To: <20201005102115.GA23702@willie-the-truck>

On 2020-10-05 11:21, Will Deacon wrote:
> On Mon, Oct 05, 2020 at 09:16:13AM +0000, Zidenberg, Tsahi wrote:
>>
>>
>> On 18/09/2020, 16:29, "Robin Murphy" <robin.murphy@arm.com> wrote:
>>      > Here's an update with some very minor cleanup tweaks, plus a proposal
>>      > for some more in-depth debug information. Given that nobody seems to
>>      > have any significant complaints about the interface, I assume we're all
>>      > happy to merge the basic driver as-is and fix anything later if needed.
>>      >
>>      > Robin.
>>
>> Agree with merging the driver as-is.
>> Tested both the updated driver and the debugfs on Graviton2 (ACPI).
>> Both map and initial numbers seem right.
>>
>> Tested-by: Tsahi Zidenberg <tsahee@amazon.com>
>>
>> May I offer a small addition to the documentation?
>> Something along these lines:
>>
>> --- a/Documentation/admin-guide/perf/arm-cmn.rst
>> +++ b/Documentation/admin-guide/perf/arm-cmn.rst
>> @@ -41,8 +41,14 @@ specified by "occupid".
>>   
>>   By default each event provides an aggregate count over all nodes of the
>>   given type. To target a specific node, "bynodeid" must be set to 1 and
>> -"nodeid" to the appropriate value derived from the CMN configuration
>> -(as defined in the "Node ID Mapping" section of the TRM).
>> +"nodeid" to the appropriate value derived from the CMN configuration.
>> +
>> +The CMN map is available in /sys/kernel/debug/arm-cmn/map.
>> +A nodeid could be calculated with this formulae:
>> +  node-id = d | (p << 2) | (y << 3) | (x << (3 + xybits))
>> +where:
>> +  x,y,p,d are node location as can be seen in the map
>> +  xybits is 2 for meshes <= 2*2, and 3 otherwise.

Note that I disagree strongly with removing the reference to the 
documentation that canonically defines the node ID format, especially 
when the proposal is to replace it with a harder-to-read definition that 
is also wrong ;)

If there's actually interest in using the coordinate format rather than 
just whole ID values, then we should generate format attributes that 
overlay the appropriate sections of the nodeid field. I haven't tried to 
implement that so far since it's not entirely trivial and I wasn't sure 
if anyone would care.

> I already queued the driver, but if you send this as a stand-alone patch (on
> top of the arm64 for-next/perf branch) then I'm happy to take it.

Ha, so much for taking my holiday after -rc6 and expecting to come back 
to chasing this up at -rc1... cheers Will, that was a very nice surprise 
to return to!

Robin.