lkml.org 
[lkml]   [2015]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 3/7] powerpc/powernv: Nest PMU detection and device tree parser
    From
    Date
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2015-07-22 06:01    [W:4.192 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site