[v5,3/7] powerpc/powernv: Nest PMU detection and device tree parser

Create a file "nest-pmu.c" to contain nest pmu related functions. Code
to detect nest pmu support and parser to collect per-chip reserved memory
region information from device tree (DT).

Detection mechanism is to look for specific property "ibm,ima-chip" in DT.
For Nest pmu, device tree will have two set of information.
1) Per-chip reserved memory region for nest pmu counter collection area.
2) Supported Nest PMUs and events

Device tree layout for the Nest PMU as follows.

  /                     -- DT root folder
  |
  -nest-ima             -- Nest PMU folder
   |

   -ima-chip@<chip-id>  -- Per-chip folder for reserved region information
    |
    -ibm,chip-id        -- Chip id
    -ibm,ima-chip
    -reg                -- HOMER PORE Nest Counter collection Address (RA)
    -size               -- size to map in kernel space

   -Alink_BW            -- Nest PMU folder
    |
    -Alink0             -- Nest PMU Alink Event file
    -scale.Alink0.scale -- Event scale file
    -unit.Alink0.unit   -- Event unit file
    -device_type        -- "nest-ima-unit" marker
  ....

Subsequent patch will parse the next part of the DT to find various
Nest PMUs and their events.

Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Anton Blanchard <anton@samba.org>
Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Cc: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Cc: Stephane Eranian <eranian@google.com>
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
 arch/powerpc/perf/Makefile   |  2 +-
 arch/powerpc/perf/nest-pmu.c | 85 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/perf/nest-pmu.c

Hi,

> +static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS];
> +
> +static int nest_ima_dt_parser(void)
> +{
> +	const __be32 *gcid;
> +	const __be64 *chip_ima_reg;
> +	const __be64 *chip_ima_size;
> +	struct device_node *dev;
> +	struct perchip_nest_info *p8ni;
> +	int idx;
> +
> +	/*
> +	 * "nest-ima" folder contains two things,
> +	 * a) per-chip reserved memory region for Nest PMU Counter data
> +	 * b) Support Nest PMU units and their event files
> +	 */
> +	for_each_node_with_property(dev, "ibm,ima-chip") {
> +		gcid = of_get_property(dev, "ibm,chip-id", NULL);
> +		chip_ima_reg = of_get_property(dev, "reg", NULL);
> +		chip_ima_size = of_get_property(dev, "size", NULL);
> +
> +		if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) {
> +			pr_err("Nest_PMU: device %s missing property\n",
> +							dev->full_name);
> +			return -ENODEV;
> +		}
> +
> +		/* chip id to save reserve memory region */
> +		idx = (uint32_t)be32_to_cpup(gcid);
So be32_to_cpup returns a __u32. You're casting to a uint32_t and then
assigning to an int.
 - Do you need the intermediate cast?
 - Should idx be an unsigned type?
> +
> +		/*
> +		 * Using a local variable to make it compact and
> +		 * easier to read
> +		 */
We probably don't need this comment. But a better variable name would be
helpful! 
> +		p8ni = &p8_nest_perchip_info[idx];
> +		p8ni->pbase = be64_to_cpup(chip_ima_reg);
> +		p8ni->size = be64_to_cpup(chip_ima_size);
> +		p8ni->vbase = (uint64_t) phys_to_virt(p8ni->pbase);
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init nest_pmu_init(void)
> +{
> +	int ret = -ENODEV;
> +
> +	/*
> +	 * Lets do this only if we are hypervisor
> +	 */
> +	if (!cur_cpu_spec->oprofile_cpu_type ||
> +	    !(strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") == 0) ||
> +	    !cpu_has_feature(CPU_FTR_HVMODE))
> +		return ret;
> +
I'm still really uncomfortable with doing this via oprofile_cpu_type.
If the kernel is compiled without oprofile support, will that get
populated?

I'm also curious about why we need checking for power8 at all. We
already know we're not going to run on hardware without a nest PMU
because of the device tree check below. 

What happens if there's a future generation of chip that supports nest
PMUs?

If it's really important to check versions in this function, maybe you
could put a version property in the ibm,ima-chip node?
> +	/*
> +	 * Nest PMU information is grouped under "nest-ima" node
> +	 * of the top-level device-tree directory. Detect Nest PMU
> +	 * by the "ibm,ima-chip" property.
> +	 */
> +	if (!of_find_node_with_property(NULL, "ibm,ima-chip"))
> +		return ret;
> +
> +	/*
> +	 * Parse device-tree for Nest PMU information
> +	 */
> +	ret = nest_ima_dt_parser();
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +device_initcall(nest_pmu_init);

On Wednesday 22 July 2015 09:19 AM, Daniel Axtens wrote:
> Hi,
>
>> +static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS];
>> +
>> +static int nest_ima_dt_parser(void)
>> +{
>> +	const __be32 *gcid;
>> +	const __be64 *chip_ima_reg;
>> +	const __be64 *chip_ima_size;
>> +	struct device_node *dev;
>> +	struct perchip_nest_info *p8ni;
>> +	int idx;
>> +
>> +	/*
>> +	 * "nest-ima" folder contains two things,
>> +	 * a) per-chip reserved memory region for Nest PMU Counter data
>> +	 * b) Support Nest PMU units and their event files
>> +	 */
>> +	for_each_node_with_property(dev, "ibm,ima-chip") {
>> +		gcid = of_get_property(dev, "ibm,chip-id", NULL);
>> +		chip_ima_reg = of_get_property(dev, "reg", NULL);
>> +		chip_ima_size = of_get_property(dev, "size", NULL);
>> +
>> +		if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) {
>> +			pr_err("Nest_PMU: device %s missing property\n",
>> +							dev->full_name);
>> +			return -ENODEV;
>> +		}
>> +
>> +		/* chip id to save reserve memory region */
>> +		idx = (uint32_t)be32_to_cpup(gcid);
> So be32_to_cpup returns a __u32. You're casting to a uint32_t and then
> assigning to an int.
>  - Do you need the intermediate cast?
>  - Should idx be an unsigned type?

my bad, sorry abt type case of uint to int.
And your are right, idx can be __u32 (__u32 and uint32_t are same i
guess). 

>> +
>> +		/*
>> +		 * Using a local variable to make it compact and
>> +		 * easier to read
>> +		 */
> We probably don't need this comment. But a better variable name would be
> helpful! 

I dont want a long name since i end up with 80 char limit warning.
but let me see.

>> +		p8ni = &p8_nest_perchip_info[idx];
>> +		p8ni->pbase = be64_to_cpup(chip_ima_reg);
>> +		p8ni->size = be64_to_cpup(chip_ima_size);
>> +		p8ni->vbase = (uint64_t) phys_to_virt(p8ni->pbase);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init nest_pmu_init(void)
>> +{
>> +	int ret = -ENODEV;
>> +
>> +	/*
>> +	 * Lets do this only if we are hypervisor
>> +	 */
>> +	if (!cur_cpu_spec->oprofile_cpu_type ||
>> +	    !(strcmp(cur_cpu_spec->oprofile_cpu_type, "ppc64/power8") == 0) ||
>> +	    !cpu_has_feature(CPU_FTR_HVMODE))
>> +		return ret;
>> +
> I'm still really uncomfortable with doing this via oprofile_cpu_type.
> If the kernel is compiled without oprofile support, will that get
> populated?

I checked the per thread pmu register code and it all does the name.
But that should not stop nest pmu to enable. So probability,
I can carry only the HV mode check and drop the oprofile check.

>
> I'm also curious about why we need checking for power8 at all. We
> already know we're not going to run on hardware without a nest PMU
> because of the device tree check below. 
> What happens if there's a future generation of chip that supports nest
> PMUs?
>
> If it's really important to check versions in this function, maybe you
> could put a version property in the ibm,ima-chip node?

True. I should not checkout power8 now, since we enable based on device tree
entries for Nest pmu.

>> +	/*
>> +	 * Nest PMU information is grouped under "nest-ima" node
>> +	 * of the top-level device-tree directory. Detect Nest PMU
>> +	 * by the "ibm,ima-chip" property.
>> +	 */
>> +	if (!of_find_node_with_property(NULL, "ibm,ima-chip"))
>> +		return ret;
>> +
>> +	/*
>> +	 * Parse device-tree for Nest PMU information
>> +	 */
>> +	ret = nest_ima_dt_parser();
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +device_initcall(nest_pmu_init);

On Thu, 2015-07-23 at 11:24 +0530, Madhavan Srinivasan wrote:
> 
> On Wednesday 22 July 2015 09:19 AM, Daniel Axtens wrote:
> > Hi,
> >
> >> +static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS];
> >> +
> >> +static int nest_ima_dt_parser(void)
> >> +{
> >> +	const __be32 *gcid;
> >> +	const __be64 *chip_ima_reg;
> >> +	const __be64 *chip_ima_size;
> >> +	struct device_node *dev;
> >> +	struct perchip_nest_info *p8ni;
> >> +	int idx;
> >> +
> >> +	/*
> >> +	 * "nest-ima" folder contains two things,
> >> +	 * a) per-chip reserved memory region for Nest PMU Counter data
> >> +	 * b) Support Nest PMU units and their event files
> >> +	 */
> >> +	for_each_node_with_property(dev, "ibm,ima-chip") {
> >> +		gcid = of_get_property(dev, "ibm,chip-id", NULL);
> >> +		chip_ima_reg = of_get_property(dev, "reg", NULL);
> >> +		chip_ima_size = of_get_property(dev, "size", NULL);
> >> +
> >> +		if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) {
> >> +			pr_err("Nest_PMU: device %s missing property\n",
> >> +							dev->full_name);
> >> +			return -ENODEV;
> >> +		}
> >> +
> >> +		/* chip id to save reserve memory region */
> >> +		idx = (uint32_t)be32_to_cpup(gcid);
> > So be32_to_cpup returns a __u32. You're casting to a uint32_t and then
> > assigning to an int.
> >  - Do you need the intermediate cast?
> >  - Should idx be an unsigned type?
> 
> my bad, sorry abt type case of uint to int.
> And your are right, idx can be __u32 (__u32 and uint32_t are same i
> guess). 

It should be u32. Don't use the uintx_t types in kernel code unless there's
some good reason for it.

The __u32 etc. types are for things that are exposed to userspace, which this
is not, so u32 is correct.

Having said that, this code should be using of_property_read_u32() etc.

And having said that, this is all based on a device tree binding that hasn't
been reviewed yet on the OPAL side, so it's subject to change too.

cheers

On Thursday 23 July 2015 02:46 PM, Michael Ellerman wrote:
> On Thu, 2015-07-23 at 11:24 +0530, Madhavan Srinivasan wrote:
>> On Wednesday 22 July 2015 09:19 AM, Daniel Axtens wrote:
>>> Hi,
>>>
>>>> +static struct perchip_nest_info p8_nest_perchip_info[P8_NEST_MAX_CHIPS];
>>>> +
>>>> +static int nest_ima_dt_parser(void)
>>>> +{
>>>> +	const __be32 *gcid;
>>>> +	const __be64 *chip_ima_reg;
>>>> +	const __be64 *chip_ima_size;
>>>> +	struct device_node *dev;
>>>> +	struct perchip_nest_info *p8ni;
>>>> +	int idx;
>>>> +
>>>> +	/*
>>>> +	 * "nest-ima" folder contains two things,
>>>> +	 * a) per-chip reserved memory region for Nest PMU Counter data
>>>> +	 * b) Support Nest PMU units and their event files
>>>> +	 */
>>>> +	for_each_node_with_property(dev, "ibm,ima-chip") {
>>>> +		gcid = of_get_property(dev, "ibm,chip-id", NULL);
>>>> +		chip_ima_reg = of_get_property(dev, "reg", NULL);
>>>> +		chip_ima_size = of_get_property(dev, "size", NULL);
>>>> +
>>>> +		if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) {
>>>> +			pr_err("Nest_PMU: device %s missing property\n",
>>>> +							dev->full_name);
>>>> +			return -ENODEV;
>>>> +		}
>>>> +
>>>> +		/* chip id to save reserve memory region */
>>>> +		idx = (uint32_t)be32_to_cpup(gcid);
>>> So be32_to_cpup returns a __u32. You're casting to a uint32_t and then
>>> assigning to an int.
>>>  - Do you need the intermediate cast?
>>>  - Should idx be an unsigned type?
>> my bad, sorry abt type case of uint to int.
>> And your are right, idx can be __u32 (__u32 and uint32_t are same i
>> guess). 
> It should be u32. Don't use the uintx_t types in kernel code unless there's
> some good reason for it.
>
> The __u32 etc. types are for things that are exposed to userspace, which this
> is not, so u32 is correct.
>
> Having said that, this code should be using of_property_read_u32() etc.

Ok will change it to use of_property_read_u32.

> And having said that, this is all based on a device tree binding that hasn't
> been reviewed yet on the OPAL side, so it's subject to change too.

Have posted new version in the skiboot mailinglist based on the reviews.
hoping to get it reviewed soon.

Maddy
> cheers
>
>