Messages in this thread | | | Subject | Re: [PATCH v1 5/9]powerpc/powernv: nest pmu feature detection support | From | Daniel Axtens <> | Date | Wed, 03 Jun 2015 10:21:16 +1000 |
| |
On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote: > Patch adds a device tree function to detect the nest pmu > support. Function will look for specific dt property "ibm,ima-chip" > as a detection mechanism for the nest pmu. > > For Nest pmu, device tree will have two set of information. > 1) Per-chip Homer address region for nest pmu counter collection area. > 2) Supported Nest PMUs and events What's HOMER?
> > +static int nest_ima_detect_parse(void) > +{ > + const __be32 *gcid; > + const __be64 *chip_ima_reg; > + const __be64 *chip_ima_size; > + struct device_node *dev; > + int rc = -EINVAL, idx; > + > + 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("%s: device %s missing property \n", > + __func__, dev->full_name); This is not a particularly informative error message. It'd be good if it mentioned that it was for PMU. > + return rc; > + } > + > + idx = (uint32_t)be32_to_cpup(gcid); > + p8_perchip_nest_info[idx].pbase = be64_to_cpup(chip_ima_reg); > + p8_perchip_nest_info[idx].size = be64_to_cpup(chip_ima_size); > + p8_perchip_nest_info[idx].vbase = (uint64_t) > + phys_to_virt(p8_perchip_nest_info[idx].pbase); > + > + rc = 0; > + } > + > + return rc;
I'm not sure your rc handling is correct. As I understand it: - Start with rc = -EINVAL. - If your first node is missing a property, return -EINVAL. - Once your first node succeeds, set rc = 0 - If any subsequent node is missing a property, return 0. - Return 0 if any node is successfully processed, otherwise return -EINVAL.
If that's what you intended (especially with regards to returning 0 when a subsequent node is missing a property), a comment explaining it would be great.
Also, why bail out if a property is missing on any node? Why not try all of them and see if any succeed?
> +} > + > static int __init nest_pmu_init(void) > { > int ret = 0; > @@ -256,6 +287,12 @@ static int __init nest_pmu_init(void) > > cpumask_chip(); > > + /* > + * Detect the Nest PMU feature > + */ > + if (nest_ima_detect_parse()) > + return 0; > + > return 0; > } Zero is returned regardless of the output of nest_ima_detect_parse. Is that intentional? If so, do you need the 'if'?
> device_initcall(nest_pmu_init);
Regards, Daniel Axtens
[unhandled content-type:application/pgp-signature] | |