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

[Daniel Axtens <dja@axtens.net>]

[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]

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);

-- 
Regards,
Daniel

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]