Re: [RESEND PATCH v1 05/11] dt-bindings: perf: hisi: Add Devicetree bindings for Hisilicon SoC PMU

From: Anurup M <anurupvasu@gmail.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-doc@vger.kernel.org, will.deacon@arm.com, corbet@lwn.net,
	catalin.marinas@arm.com, robh+dt@kernel.org, arnd@arndb.de,
	f.fainelli@gmail.com, rmk+kernel@arm.linux.org.uk,
	krzk@kernel.org, anurup.m@huawei.com, zhangshaokun@hisilicon.com,
	tanxiaojun@huawei.com, xuwei5@hisilicon.com,
	sanil.kumar@hisilicon.com, john.garry@huawei.com,
	gabriele.paoloni@huawei.com, shiju.jose@huawei.com,
	wangkefeng.wang@huawei.com, guohanjun@huawei.com,
	shyju.pv@huawei.com, linuxarm@huawei.com
Subject: Re: [RESEND PATCH v1 05/11] dt-bindings: perf: hisi: Add Devicetree bindings for Hisilicon SoC PMU
Date: Wed, 16 Nov 2016 11:24:17 +0530	[thread overview]
Message-ID: <582BF489.6040506@gmail.com> (raw)
In-Reply-To: <20161115095148.GA29104@leverpostej>


On Tuesday 15 November 2016 03:21 PM, Mark Rutland wrote:
> On Mon, Nov 14, 2016 at 05:36:44AM +0530, Anurup M wrote:
>> On Friday 11 November 2016 12:00 AM, Mark Rutland wrote:
>>> On Thu, Nov 03, 2016 at 01:42:01AM -0400, Anurup M wrote:
>>>> +	- scl-id : The Super Cluster ID. This can be the ID of the CPU die
>>>> +		   or IO die in the chip.
>>> What's this needed for?
>> This is used as suffix to the PMU name. hisi_l3c<scl-id>. (hisi_l3c2
>> - for scl-id = 2).
>> This is to identify the pmu correspond to which CPU die in the socket.
>>>> +	- num-events : No of events supported by this PMU device.
>>>> +
>>>> +	- num-counters : No of hardware counters available for counting.
>>> This isn't probeable or well-known?
>> My idea is to have the common properties of SoC PMU added here.
>> The num-events, num-counters etc. So that handling can be made
>> common in the driver.
>> Is it not recommended? Please share your comments.
> This feels like something that should be well-known for the programming
> model of the device. If the number of events and/or counters shange, I'd
> expect other things to also change such that the device is no longer
> compatible with previous versions.
>
> [...]
>
Agreed, it is possible that the versions can be still incompatible. 
Shall move it to
driver in v2.

>> The below two properties (module-id, cfgen-map) differs between
>> chips hip05/06 and hip07.
> The module-id property sounds like a HW description, but it's not
> entirely clear to me what cfgen-map is; more comments on that below.
>
>> Please suggest.
>>>> +	- module-id : Module ID to input for djtag. This property is an array of
>>>> +		      module_id for each L3 cache banks.
>>>> +
>>>> +	- num-banks : Number of banks or instances of the device.
>>> What's a bank? Surely they have separate instances of the PMU?
>> Yes each bank is a separate instance of PMU.
>> If it is recommended to have each L3 cache bank registered as
>> separate PMU with perf, then this property will be removed.
> Generally, I think that separate instances are preferable.
>
>>> What order are these in?
>> The bank number will start from "1" till "4" for L3 cache as there
>> are four banks in hip05/06/07 chips.
>>>> +	- cfgen-map : Config enable array to select the bank.
>>> Huh?
> As above, it's not clear to me what this property represents. Could you
> please clarify?
This property is used to select the bank. The naming lead to confusion.
I shall change it to bank_select.
If it is recommended to register each L3 cache bank as separate PMU then
this property will be moved to driver.

Thanks
Anurup
>
> Thanks,
> Mark.