[Skiboot] [PATCH v8 0/7] OPAL support for Nest instrumentation

[Michael Neuling]

On Thu, 2016-03-17 at 17:04 +0530, Madhavan Srinivasan wrote:
> 
> On Wednesday 16 March 2016 11:06 AM, Michael Neuling wrote:
> > Maddy,
> > 
> > >                 chip at 7fdb20000 {
> > >                         ibm,chip-id = <0x0>;
> > >                         ranges = <0x0 0x7 0xfdb20000 0x30000>;
> > Should this be a reg property?
> > 
> > 
> > >                         #address-cells = <0x1>;
> > >                         phandle = <0x100001e1>;
> > >                         #size-cells = <0x1>;
> > >                         linux,phandle = <0x100001e1>;
> > > 
> > >                         mcs {
> > >                                 compatible = "ibm,nest-counters
> > > -chip";
> > >                                 ranges;
> > >                                 #address-cells = <0x1>;
> > >                                 phandle = <0x100001e2>;
> > >                                 #size-cells = <0x1>;
> > >                                 linux,phandle = <0x100001e2>;
> > > 
> > >                                 write_mc at 38 {
> > >                                         id = <0x0>;
> > >                                         reg = <0x38 0x8>;
> > >                                         unit = "MiB";
> > >                                         scale = "1.2207e-4";
> > As I mentioned elsewhere, I'm not keen on these bindings.
> 
> Mikey,
> 
> Considering the MCS unit, the raw counter value (say X)
> may not be of much use. That said, mcs counter give
> us no. of 128byte blocks of reads or writes. Now, user level
> tool must know how to convert the raw value into some
> metric like GiB or MiB. Hence, have added the "scale"
> and "unit" binding for the events. With this, kernel
> pmu driver creates scale and unit files in the sysfs.
> 
> x86 also does something similiar, meaning consider the commit
> 
>  433678bdc6ed3 perf/rapl: Fix sysfs_show() initialization for RAPL PMU
> 
> Where they have hardcoded "scaling" and "unit" factor to
> get a metric out of the counter data from their
> Uncore RAPL PMU unit.

That's based on x86 BIOS right?  I'm not sure that's an 
interface I want to actively replicate.

> Hardcoding these values in the kernel PMU driver may work,
> but, then having the unit and scale in device tree will
> allow the kernel drive to be independent of the microcode
> and platform firmware versions. Kernel PMU driver will be
> clean and extensible for new nest units in P8 and
> P9 generations.

I'm not saying hardcode them in the kernel.  I'm saying just 
put the scale value in the device tree, rather than what you 
have now.

Mikey

> > 1.2204e-4 seems to just be 1/8192.  MiB is just a number also.  I think
> > you are combining these to give a final scale number which in this case
> > (I think is just 128).
> > 
> >   1.2204e-4 * 2**20 = 127.96821504
> > 
> > I'd prefer you just put that in the device tree.
> 
> Now, for the scaling factor value, if we have "128"
> then the unit has to be in "Bytes". Since, these
> memory bandwidth counters are per chip level,
> preferred representing them in "MiB" instead of "Bytes".
> 
> In that case scaling factor value comes out as
> 
>  (128/(1024x1024))  = "1.2207e-4"
> 
> Regardring the "reg" property question,
> 
> IIUC, these devices counters are memory mapped on the
> CPU domain, having these as "ranges" is right and
> individual event offsets are represented as "regs".
> 
> Maddy
> 
> > Mikey
>