| Message ID | 1433260778-26497-6-git-send-email-maddy@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
On Wednesday 03 June 2015 05:51 AM, Daniel Axtens wrote: > 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? Nest PMUs are configured via PORE engine interface and PORE Engine collections the Nest counter value and updates in the main memory which is reserved for this use. >> >> +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. Sure will changes. >> + 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. Main loop is only for nodes with property "ibm,ima-chip". Not all the nodes will have this property. > 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. Yes. I will add comment explaining it. But i did add this in the commit message. > Also, why bail out if a property is missing on any node? Why not try all > of them and see if any succeed? Only the Nest Unit nodes in the device tree will have this property. Commit has the device tree hierarchy for the Nest instrumentation. So if we dont find this property then Nest instrumentation is not supported, hence bail out. > >> +} >> + >> 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'? No it should return "ret" which should be initialized to error value. WIll fix it >> device_initcall(nest_pmu_init); > Regards, > Daniel Axtens > Thanks for the review MAddy
diff --git a/arch/powerpc/perf/nest-pmu.c b/arch/powerpc/perf/nest-pmu.c index 345707c..c979e57 100644 --- a/arch/powerpc/perf/nest-pmu.c +++ b/arch/powerpc/perf/nest-pmu.c @@ -11,6 +11,7 @@ #include "nest-pmu.h" +static struct perchip_nest_info p8_perchip_nest_info[P8_MAX_CHIP]; static cpumask_t cpu_mask_nest_pmu; static ssize_t cpumask_nest_pmu_get_attr(struct device *dev, @@ -242,6 +243,36 @@ static int update_pmu_ops(struct nest_pmu *pmu) return 0; } +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); + 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; +} + 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; } device_initcall(nest_pmu_init);
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 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 HOMER 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 .... Patch save per-chip HOMER offset and maps the same to kernel structure. 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: 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/nest-pmu.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)