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
signature.asc
Description: This is a digitally signed message part