On Thu, Nov 03, 2016 at 01:42:03AM -0400, Anurup M wrote: > + do { > + /* Get count from individual L3C banks and sum them up */ > + for (i = 0; i < num_banks; i++) { > + total_raw_count += hisi_read_l3c_counter(l3c_hwmod_data, > + idx, i); > + } > + prev_raw_count = local64_read(&hwc->prev_count); > + > + /* > + * As prev_raw_count is updated with average value of > + * L3 cache banks, we multiply it by no of banks and > + * compute the delta > + */ > + delta = (total_raw_count - (prev_raw_count * num_banks)) & > + HISI_MAX_PERIOD; > + > + local64_add(delta, &event->count); > + > + /* > + * Divide by num of banks to get average count and > + * update prev_count with this value > + */ > + avg_raw_count = total_raw_count / num_banks; > + } while (local64_cmpxchg( > + &hwc->prev_count, prev_raw_count, avg_raw_count) != > + prev_raw_count); Please don't aggregate like this; expose separate PMUs instead. This is racy, and by averaging and multiplying we're making up and/or throwing away data. [...] > + event_value = (val - > + HISI_HWEVENT_L3C_READ_ALLOCATE); > + > + /* Select the appropriate Event select register */ > + if (idx > 3) > + reg_offset += 4; > + > + /* Value to write to event type register */ > + val = event_value << (8 * idx); > + Please add helpers for these, and explain *why* the transformations are necessary. > + /* Find the djtag Identifier of the Unit */ > + client = l3c_hwmod_data->client; > + > + /* > + * Set the event in L3C_EVENT_TYPEx Register > + * for all L3C banks > + */ As above, it seems like you should expose a separate PMU per bank instead. That applies for all the other instances where you iterate over banks. [...] > + for (i = 0; i < l3c_hwmod_data->l3c_hwcfg.num_banks; i++) { > + module_id = l3c_hwmod_data->l3c_hwcfg.module_id[i]; > + cfg_en = l3c_hwmod_data->l3c_hwcfg.bank_cfgen[i]; > + ret = hisi_djtag_writereg(module_id, > + cfg_en, > + reg_offset, > + value, > + client); > + if (!ret) > + ret = value; > + } This is impossible to read. Please factor this into helpers such that you don't need this amount of indentation. Please do similarly elsewhere when you see this indentation pattern. [...] > +static int hisi_l3c_get_event_idx(struct hisi_pmu *pl3c_pmu) > +{ > + struct hisi_l3c_data *l3c_hwmod_data = pl3c_pmu->hwmod_data; > + int event_idx; > + > + event_idx = > + find_first_zero_bit( > + l3c_hwmod_data->hisi_l3c_event_used_mask, > + pl3c_pmu->num_counters); > + > + if (event_idx == HISI_MAX_CFG_L3C_CNTR) > + return -EAGAIN; > + > + __set_bit(event_idx, > + l3c_hwmod_data->hisi_l3c_event_used_mask); > + > + return event_idx; > +} Please get rid of the weird hungarian notation (i.e. don't use 'p' as a prefix for pointers), and use temporary variables consistently, e.g. static int hisi_l3c_get_event_idx(struct hisi_pmu *l3c_pmu) { struct hisi_l3c_data *l3c_hwmod_data = l3c_pmu->hwmod_data; unsigned long *used_mask = l3c_hwmod_data->hisi_l3c_event_used_mask; int num_counters = pl3c_pmu->num_counters int idx; idx = find_first_zero_bit(used_mask, num_counters); if (idx == num_counters) return -EAGAIN; set_bit(idx, used_mask); return idx; } [...] > + if (of_property_read_u32(node, "counter-reg", > + &pl3c_hwcfg->counter_reg0_off)) { > + dev_err(dev, "DT:Couldnot read counter-reg!\n"); > + return -EINVAL; > + } Please use spaces in these messages. Otherwise, my comments on the binding apply here. [...] > +static int init_hisi_l3c_data(struct device *dev, > + struct hisi_pmu *pl3c_pmu, > + struct hisi_djtag_client *client) > +{ > + struct hisi_l3c_data *l3c_hwmod_data = NULL; > + int ret; > + > + l3c_hwmod_data = kzalloc(sizeof(struct hisi_l3c_data), > + GFP_KERNEL); Use: l3c_hwmod_data = kzalloc(sizeof(*l3c_hwmod_data, GFP_KERNEL): [...] > +static int hisi_pmu_l3c_dev_probe(struct hisi_djtag_client *client) > +{ > + struct hisi_pmu *pl3c_pmu = NULL; > + struct device *dev = &client->dev; > + int ret; > + > + pl3c_pmu = hisi_pmu_alloc(dev); > + if (IS_ERR(pl3c_pmu)) > + return PTR_ERR(pl3c_pmu); Why use error pointers for this? hisi_pmu_alloc() only ever returns ERR_PTR(-ENOMEM) if it failed to allocate. It's far simpler to have it pass on NULL there, and here do: pl3c_pmu = hisi_pmu_alloc(dev); if (!pl3c_pmu) return -ENOMEM; Please also s/pl3c_pmu/l3c_pmu/ here, and elsewhere throughout the driver. The 'p' only serves to make this harder to read. [...] > + /* Register with perf PMU */ > + pl3c_pmu->pmu = (struct pmu) { > + .name = pl3c_pmu->name, > + .task_ctx_nr = perf_invalid_context, > + .event_init = hisi_uncore_pmu_event_init, > + .add = hisi_uncore_pmu_add, > + .del = hisi_uncore_pmu_del, > + .start = hisi_uncore_pmu_start, > + .stop = hisi_uncore_pmu_stop, > + .read = hisi_uncore_pmu_read, > + }; Please remove the comment above this. [...] > +int hisi_uncore_pmu_event_init(struct perf_event *event) > +{ > + int err; > + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu); This is undefined behaviour. This must be done *after* we check the event->pmu->type. > + > + if (event->attr.type != event->pmu->type) > + return -ENOENT; > + > + /* we do not support sampling as the counters are all > + * shared by all CPU cores in a CPU die(SCCL). Also we > + * donot support attach to a task(per-process mode) > + */ > + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK) > + return -EOPNOTSUPP; > + > + /* counters do not have these bits */ > + if (event->attr.exclude_user || > + event->attr.exclude_kernel || > + event->attr.exclude_host || > + event->attr.exclude_guest || > + event->attr.exclude_hv || > + event->attr.exclude_idle) > + return -EINVAL; > + > + if (event->cpu < 0) > + return -EINVAL; > + > + event->cpu = cpumask_first(&phisi_pmu->cpu); You should also check the event grouping. Take a look at what we do in arch/arm/mm/cache-l2x0-pmu.c. [...] > +/* > + * Enable counter and set the counter to count > + * the event that we're interested in. > + */ > +void hisi_uncore_pmu_enable_event(struct perf_event *event) > +{ > + struct hw_perf_event *hwc = &event->hw; > + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu); > + > + /* Disable the hardware event counting */ > + if (phisi_pmu->ops->disable_counter) > + phisi_pmu->ops->disable_counter(phisi_pmu, GET_CNTR_IDX(hwc)); Why isn't the counter already disabled? > + /* > + * Set event (if destined for Hisilicon SoC counters). > + */ > + if (phisi_pmu->ops->set_evtype) > + phisi_pmu->ops->set_evtype(phisi_pmu, GET_CNTR_IDX(hwc), > + hwc->config_base); Why isn't this done in the pmu::event_add callback? > + > + /* Enable the hardware event counting */ > + if (phisi_pmu->ops->enable_counter) > + phisi_pmu->ops->enable_counter(phisi_pmu, GET_CNTR_IDX(hwc)); This should be the only necessary part of this function. > +} > + > +void hisi_pmu_set_event_period(struct perf_event *event) > +{ > + struct hw_perf_event *hwc = &event->hw; > + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu); > + > + /* > + * The Hisilicon PMU counters have a period of 2^32. To account for the > + * possiblity of extreme interrupt latency we program for a period of > + * half that. Hopefully we can handle the interrupt before another 2^31 > + * events occur and the counter overtakes its previous value. > + */ > + u64 val = 1ULL << 31; > + > + local64_set(&hwc->prev_count, val); > + > + /* Write to the hardware event counter */ > + phisi_pmu->ops->write_counter(phisi_pmu, hwc, val); > +} > + > +void hisi_uncore_pmu_start(struct perf_event *event, int flags) > +{ > + struct hw_perf_event *hwc = &event->hw; > + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu); > + struct hisi_pmu_hw_events *hw_events; > + > + hw_events = &phisi_pmu->hw_events; > + > + if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED))) > + return; > + > + WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE)); > + hwc->state = 0; > + > + if (phisi_pmu->ops->set_event_period) > + phisi_pmu->ops->set_event_period(event); When will this differ from hisi_pmu_set_event_period() above? > + if (flags & PERF_EF_RELOAD) { > + u64 prev_raw_count = local64_read(&hwc->prev_count); > + > + phisi_pmu->ops->write_counter(phisi_pmu, hwc, > + (u32)prev_raw_count); > + } If we always go through hisi_pmu_set_event_period(), this looks redundant. > + > + hisi_uncore_pmu_enable_event(event); There's no matching disable_event() call in this function, so this looks suspicious. > + perf_event_update_userpage(event); > +} > + > +void hisi_uncore_pmu_stop(struct perf_event *event, int flags) > +{ > + struct hw_perf_event *hwc = &event->hw; > + struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu); > + > + if (hwc->state & PERF_HES_UPTODATE) > + return; Why? [...] > +int hisi_uncore_common_fwprop_read(struct device *dev, > + struct hisi_pmu *phisi_pmu) > +{ > + if (device_property_read_u32(dev, "num-events", > + &phisi_pmu->num_events)) { > + dev_err(dev, "Cant read num-events from DT!\n"); > + return -EINVAL; > + } For consistency with the rest of the driver, and given there is no ACPI support, please use the of_property_* API here. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html