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

[Mark Rutland <mark.rutland@arm.com>]

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.

[...]

> 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?

Thanks,
Mark.