On Thu, May 17, 2018 at 4:42 PM, John Garry <john.garry@xxxxxxxxxx> wrote: > On 16/05/2018 05:55, Ganapatrao Kulkarni wrote: >> >> This patch adds a perf driver for the PMU UNCORE devices DDR4 Memory >> Controller(DMC) and Level 3 Cache(L3C). >> > > Hi, > > Just some coding comments below: > >> ThunderX2 has 8 independent DMC PMUs to capture performance events >> corresponding to 8 channels of DDR4 Memory Controller and 16 independent >> L3C PMUs to capture events corresponding to 16 tiles of L3 cache. >> Each PMU supports up to 4 counters. All counters lack overflow interrupt >> and are sampled periodically. >> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@xxxxxxxxxx> >> --- >> drivers/perf/Kconfig | 8 + >> drivers/perf/Makefile | 1 + >> drivers/perf/thunderx2_pmu.c | 965 >> +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/cpuhotplug.h | 1 + >> 4 files changed, 975 insertions(+) >> create mode 100644 drivers/perf/thunderx2_pmu.c >> >> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig >> index 28bb5a0..eafd0fc 100644 >> --- a/drivers/perf/Kconfig >> +++ b/drivers/perf/Kconfig >> @@ -85,6 +85,14 @@ config QCOM_L3_PMU >> Adds the L3 cache PMU into the perf events subsystem for >> monitoring L3 cache events. >> >> +config THUNDERX2_PMU >> + bool "Cavium ThunderX2 SoC PMU UNCORE" >> + depends on ARCH_THUNDER2 && PERF_EVENTS && ACPI > > > Is the explicit dependency for PERF_EVENTS required, since we're under the > PERF_EVENTS menu? not really, i can drop this. > > And IIRC for other perf drivers we required a dependency on ARM64 - is that > required here also? I see arm_smccc_smc() calls in the code... ok. > > >> + help >> + Provides support for ThunderX2 UNCORE events. >> + The SoC has PMU support in its L3 cache controller (L3C) and >> + in the DDR4 Memory Controller (DMC). >> + >> config XGENE_PMU >> depends on ARCH_XGENE >> bool "APM X-Gene SoC PMU" >> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile >> index b3902bd..909f27f 100644 >> --- a/drivers/perf/Makefile >> +++ b/drivers/perf/Makefile >> @@ -7,5 +7,6 @@ obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o >> obj-$(CONFIG_HISI_PMU) += hisilicon/ >> obj-$(CONFIG_QCOM_L2_PMU) += qcom_l2_pmu.o >> obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o >> +obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o >> obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o >> obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o >> diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c >> new file mode 100644 >> index 0000000..0401443 >> --- /dev/null >> +++ b/drivers/perf/thunderx2_pmu.c >> @@ -0,0 +1,965 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * CAVIUM THUNDERX2 SoC PMU UNCORE >> + * >> + * Copyright (C) 2018 Cavium Inc. >> + * Author: Ganapatrao Kulkarni <gkulkarni@xxxxxxxxxx> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >> + */ > > > Isn't this the same as the SPDX? ok, i will remove it. > >> + >> +#include <linux/acpi.h> >> +#include <linux/arm-smccc.h> >> +#include <linux/cpuhotplug.h> >> +#include <linux/perf_event.h> >> +#include <linux/platform_device.h> >> + >> +/* L3c and DMC has 16 and 8 channels per socket respectively. > > > L3C, right? ok > >> + * Each Channel supports UNCORE PMU device and consists of >> + * 4 independent programmable counters. Counters are 32 bit >> + * and does not support overflow interrupt, they needs to be > > > /s/needs/need/, /s/does/do/ ok > >> + * sampled before overflow(i.e, at every 2 seconds). > > > how can you ensure that this value is low enough? > > "I saw this comment in previous patch: >> Given that all channels compete for access to the muxed register >> interface, I suspect we need to try more often than once every 2 >> seconds... > > 2 seconds seems to be sufficient. So far testing looks good." > > Can you provide any more analytical reasoning than this? > >> + */ >> + >> +#define UNCORE_MAX_COUNTERS 4 >> +#define UNCORE_L3_MAX_TILES 16 >> +#define UNCORE_DMC_MAX_CHANNELS 8 >> + >> +#define UNCORE_HRTIMER_INTERVAL (2 * NSEC_PER_SEC) >> +#define GET_EVENTID(ev) ((ev->hw.config) & 0x1ff) >> +#define GET_COUNTERID(ev) ((ev->hw.idx) & 0xf) >> +#define GET_CHANNELID(pmu_uncore) (pmu_uncore->channel) >> +#define DMC_EVENT_CFG(idx, val) ((val) << (((idx) * 8) + >> 1)) >> + >> +#define DMC_COUNTER_CTL 0x234 >> +#define DMC_COUNTER_DATA 0x240 >> +#define L3C_COUNTER_CTL 0xA8 >> +#define L3C_COUNTER_DATA 0xAC > > > I feel it's generally better to keep register offsets in numeric order (if > indeed, that is what they are) ok. > > >> + >> +#define THUNDERX2_SMC_CALL_ID 0xC200FF00 >> +#define THUNDERX2_SMC_SET_CHANNEL 0xB010 >> + >> +enum thunderx2_uncore_l3_events { >> + L3_EVENT_NONE, >> + L3_EVENT_NBU_CANCEL, >> + L3_EVENT_DIB_RETRY, >> + L3_EVENT_DOB_RETRY, >> + L3_EVENT_DIB_CREDIT_RETRY, >> + L3_EVENT_DOB_CREDIT_RETRY, >> + L3_EVENT_FORCE_RETRY, >> + L3_EVENT_IDX_CONFLICT_RETRY, >> + L3_EVENT_EVICT_CONFLICT_RETRY, >> + L3_EVENT_BANK_CONFLICT_RETRY, >> + L3_EVENT_FILL_ENTRY_RETRY, >> + L3_EVENT_EVICT_NOT_READY_RETRY, >> + L3_EVENT_L3_RETRY, >> + L3_EVENT_READ_REQ, >> + L3_EVENT_WRITE_BACK_REQ, >> + L3_EVENT_INVALIDATE_NWRITE_REQ, >> + L3_EVENT_INV_REQ, >> + L3_EVENT_SELF_REQ, >> + L3_EVENT_REQ, >> + L3_EVENT_EVICT_REQ, >> + L3_EVENT_INVALIDATE_NWRITE_HIT, >> + L3_EVENT_INVALIDATE_HIT, >> + L3_EVENT_SELF_HIT, >> + L3_EVENT_READ_HIT, >> + L3_EVENT_MAX, > > > ',' required? not really; however, having this will help in moving around without worring about adding it. > >> +}; >> + >> +enum thunderx2_uncore_dmc_events { >> + DMC_EVENT_NONE, >> + DMC_EVENT_COUNT_CYCLES, >> + DMC_EVENT_RES2, >> + DMC_EVENT_RES3, >> + DMC_EVENT_RES4, >> + DMC_EVENT_RES5, >> + DMC_EVENT_RES6, >> + DMC_EVENT_RES7, >> + DMC_EVENT_RES8, >> + DMC_EVENT_READ_64B_TXNS, >> + DMC_EVENT_READ_BELOW_64B_TXNS, >> + DMC_EVENT_WRITE_TXNS, >> + DMC_EVENT_TXN_CYCLES, >> + DMC_EVENT_DATA_TRANSFERS, >> + DMC_EVENT_CANCELLED_READ_TXNS, >> + DMC_EVENT_CONSUMED_READ_TXNS, >> + DMC_EVENT_MAX, > > > ditto > >> +}; >> + >> +enum thunderx2_uncore_type { >> + PMU_TYPE_L3C, >> + PMU_TYPE_DMC, >> + PMU_TYPE_INVALID, >> +}; >> + >> +/* >> + * pmu on each socket has 2 uncore devices(dmc and l3), >> + * each uncore device has up to 16 channels, each channel can sample >> + * events independently with counters up to 4. >> + * >> + * struct thunderx2_pmu_uncore_channel created per channel. >> + * struct thunderx2_pmu_uncore_dev per uncore device. > > > this comment is a bit obvious ok. > > >> + */ >> +struct thunderx2_pmu_uncore_channel { >> + struct pmu pmu; >> + struct hlist_node node; >> + struct thunderx2_pmu_uncore_dev *uncore_dev; >> + int channel; >> + int cpu; >> + DECLARE_BITMAP(active_counters, UNCORE_MAX_COUNTERS); >> + struct perf_event *events[UNCORE_MAX_COUNTERS]; >> + struct hrtimer hrtimer; >> + /* to sync counter alloc/release */ >> + raw_spinlock_t lock; >> +}; >> + >> +struct thunderx2_pmu_uncore_dev { >> + char *name; >> + struct device *dev; >> + enum thunderx2_uncore_type type; >> + void __iomem *base; >> + int node; >> + u32 max_counters; >> + u32 max_channels; >> + u32 max_events; >> + u64 hrtimer_interval; >> + /* this lock synchronizes across channels */ >> + raw_spinlock_t lock; >> + const struct attribute_group **attr_groups; >> + void (*init_cntr_base)(struct perf_event *event, >> + struct thunderx2_pmu_uncore_dev *uncore_dev); >> + void (*select_channel)(struct perf_event *event); >> + void (*stop_event)(struct perf_event *event); >> + void (*start_event)(struct perf_event *event, int flags); >> +}; >> + >> +static inline struct thunderx2_pmu_uncore_channel * > > > is inline keyword required or even used generally? Since static, can't the > compiler figure this out? > >> +pmu_to_thunderx2_pmu_uncore(struct pmu *pmu) >> +{ >> + return container_of(pmu, struct thunderx2_pmu_uncore_channel, >> pmu); >> +} >> + >> +/* >> + * sysfs format attributes >> + */ > > > can't this comment fit on a single line? > > >> +static ssize_t thunderx2_pmu_format_show(struct device *dev, >> + struct device_attribute *attr, char >> *buf) >> +{ >> + struct dev_ext_attribute *eattr; >> + >> + eattr = container_of(attr, struct dev_ext_attribute, attr); >> + return sprintf(buf, "%s\n", (char *) eattr->var); >> +} >> + >> +#define FORMAT_ATTR(_name, _config) \ >> + (&((struct dev_ext_attribute[]) { \ >> + { \ >> + .attr = __ATTR(_name, 0444, thunderx2_pmu_format_show, NULL), \ >> + .var = (void *) _config, \ >> + } \ >> + })[0].attr.attr) >> + >> +static struct attribute *l3c_pmu_format_attrs[] = { >> + FORMAT_ATTR(event, "config:0-4"), >> + NULL, >> +}; >> + >> +static struct attribute *dmc_pmu_format_attrs[] = { >> + FORMAT_ATTR(event, "config:0-4"), >> + NULL, >> +}; >> + >> +static const struct attribute_group l3c_pmu_format_attr_group = { >> + .name = "format", >> + .attrs = l3c_pmu_format_attrs, >> +}; >> + >> +static const struct attribute_group dmc_pmu_format_attr_group = { >> + .name = "format", >> + .attrs = dmc_pmu_format_attrs, >> +}; >> + > > > [ ... ] > > >> + * Per PMU device attribute groups >> + */ >> +static const struct attribute_group *l3c_pmu_attr_groups[] = { >> + &l3c_pmu_format_attr_group, >> + &pmu_cpumask_attr_group, >> + &l3c_pmu_events_attr_group, >> + NULL >> +}; >> + >> +static const struct attribute_group *dmc_pmu_attr_groups[] = { >> + &dmc_pmu_format_attr_group, >> + &pmu_cpumask_attr_group, >> + &dmc_pmu_events_attr_group, >> + NULL >> +}; >> + >> +static inline u32 reg_readl(unsigned long addr) >> +{ >> + return readl((void __iomem *)addr); >> +} >> + >> +static inline void reg_writel(u32 val, unsigned long addr) >> +{ >> + writel(val, (void __iomem *)addr); >> +} >> + >> +static int alloc_counter(struct thunderx2_pmu_uncore_channel *pmu_uncore) >> +{ >> + int counter; >> + >> + raw_spin_lock(&pmu_uncore->lock); >> + counter = find_first_zero_bit(pmu_uncore->active_counters, >> + pmu_uncore->uncore_dev->max_counters); >> + if (counter == pmu_uncore->uncore_dev->max_counters) { >> + raw_spin_unlock(&pmu_uncore->lock); >> + return -ENOSPC; >> + } >> + set_bit(counter, pmu_uncore->active_counters); >> + raw_spin_unlock(&pmu_uncore->lock); >> + return counter; >> +} >> + >> +static void free_counter( >> + struct thunderx2_pmu_uncore_channel *pmu_uncore, int >> counter) > > > strange formatting it is to fit in 80 char line. > >> +{ >> + raw_spin_lock(&pmu_uncore->lock); >> + clear_bit(counter, pmu_uncore->active_counters); >> + raw_spin_unlock(&pmu_uncore->lock); >> +} >> + > > > [ ... ] > > >> +static void uncore_stop_event_l3c(struct perf_event *event) >> +{ >> + reg_writel(0, event->hw.config_base); >> +} >> + >> +static void uncore_start_event_dmc(struct perf_event *event, int flags) >> +{ >> + u32 val; >> + struct hw_perf_event *hwc = &event->hw; >> + int idx = GET_COUNTERID(event); >> + int event_type = GET_EVENTID(event); >> + >> + /* enable and start counters. >> + * 8 bits for each counter, bits[05:01] of a counter to set event >> type. >> + */ >> + val = reg_readl(hwc->config_base); >> + val &= ~DMC_EVENT_CFG(idx, 0x1f); >> + val |= DMC_EVENT_CFG(idx, event_type); >> + reg_writel(val, hwc->config_base); >> + local64_set(&hwc->prev_count, 0); >> + reg_writel(0, hwc->event_base); >> +} >> + >> +static void uncore_stop_event_dmc(struct perf_event *event) >> +{ >> + u32 val; >> + struct hw_perf_event *hwc = &event->hw; >> + int idx = GET_COUNTERID(event); >> + >> + /* clear event type(bits[05:01]) to stop counter */ >> + val = reg_readl(hwc->config_base); >> + val &= ~DMC_EVENT_CFG(idx, 0x1f); >> + reg_writel(val, hwc->config_base); >> +} >> + >> +static void init_cntr_base_l3c(struct perf_event *event, >> + struct thunderx2_pmu_uncore_dev *uncore_dev) >> +{ >> + struct hw_perf_event *hwc = &event->hw; >> + >> + /* counter ctrl/data reg offset at 8 */ >> + hwc->config_base = (unsigned long)uncore_dev->base >> + + L3C_COUNTER_CTL + (8 * GET_COUNTERID(event)); >> + hwc->event_base = (unsigned long)uncore_dev->base >> + + L3C_COUNTER_DATA + (8 * GET_COUNTERID(event)); > > > Is there a better way to hold this, since we're casting back to a void * > when writing to the register? > dont think so. > >> +} >> + >> +static void init_cntr_base_dmc(struct perf_event *event, >> + struct thunderx2_pmu_uncore_dev *uncore_dev) >> +{ >> + struct hw_perf_event *hwc = &event->hw; >> + >> + hwc->config_base = (unsigned long)uncore_dev->base >> + + DMC_COUNTER_CTL; >> + /* counter data reg offset at 0xc */ >> + hwc->event_base = (unsigned long)uncore_dev->base >> + + DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event)); >> +} >> + >> +static void thunderx2_uncore_update(struct perf_event *event) >> +{ >> + s64 prev, new = 0; >> + u64 delta; >> + struct hw_perf_event *hwc = &event->hw; >> + struct thunderx2_pmu_uncore_channel *pmu_uncore; >> + enum thunderx2_uncore_type type; >> + >> + pmu_uncore = pmu_to_thunderx2_pmu_uncore(event->pmu); >> + type = pmu_uncore->uncore_dev->type; >> + >> + pmu_uncore->uncore_dev->select_channel(event); >> + >> + new = reg_readl(hwc->event_base); >> + prev = local64_xchg(&hwc->prev_count, new); >> + >> + /* handles rollover of 32 bit counter */ >> + delta = (u32)(((1UL << 32) - prev) + new); >> + local64_add(delta, &event->count); >> +} >> + >> +enum thunderx2_uncore_type get_uncore_device_type(struct acpi_device >> *adev) >> +{ >> + int i = 0; >> + struct acpi_uncore_device { >> + __u8 id[ACPI_ID_LEN]; >> + enum thunderx2_uncore_type type; >> + } devices[] = { >> + {"CAV901D", PMU_TYPE_L3C}, >> + {"CAV901F", PMU_TYPE_DMC}, >> + {"", PMU_TYPE_INVALID}, > > > > for sentinels, ',' should not be required ok, i will remove. > >> + }; >> + >> + while (devices[i].type != PMU_TYPE_INVALID) { >> + if (!strcmp(acpi_device_hid(adev), devices[i].id)) >> + return devices[i].type; > > > Can't you use acpi_match_device()? alredy i have acpi device id, just i have to compare to find type. acpi_match_device is unnecessary which goes in loop over all device ids/names. > >> + i++; >> + } >> + return PMU_TYPE_INVALID; >> +} >> + >> +/* >> + * We must NOT create groups containing events from multiple hardware >> PMUs, >> + * although mixing different software and hardware PMUs is allowed. >> + */ >> +static bool thunderx2_uncore_validate_event_group(struct perf_event >> *event) > > > [ ... ] > > >> + >> +static int thunderx2_pmu_uncore_add(struct thunderx2_pmu_uncore_dev >> *uncore_dev, >> + int channel) >> +{ >> + struct thunderx2_pmu_uncore_channel *pmu_uncore; >> + int ret, cpu; >> + >> + pmu_uncore = devm_kzalloc(uncore_dev->dev, sizeof(*pmu_uncore), >> + GFP_KERNEL); >> + if (!pmu_uncore) >> + return -ENOMEM; >> + >> + cpu = cpumask_any_and(cpumask_of_node(uncore_dev->node), >> + cpu_online_mask); >> + if (cpu >= nr_cpu_ids) >> + return -EINVAL; >> + >> + pmu_uncore->cpu = cpu; >> + pmu_uncore->channel = channel; >> + pmu_uncore->uncore_dev = uncore_dev; >> + >> + hrtimer_init(&pmu_uncore->hrtimer, CLOCK_MONOTONIC, >> HRTIMER_MODE_REL); >> + pmu_uncore->hrtimer.function = thunderx2_uncore_hrtimer_callback; >> + >> + ret = thunderx2_pmu_uncore_register(pmu_uncore); >> + if (ret) { >> + dev_err(uncore_dev->dev, "%s PMU: Failed to init >> driver\n", >> + uncore_dev->name); >> + return -ENODEV; >> + } >> + >> + /* register hotplug callback for the pmu */ >> + ret = cpuhp_state_add_instance( >> + CPUHP_AP_PERF_ARM_THUNDERX2_UNCORE_ONLINE, >> + &pmu_uncore->node); >> + if (ret) { >> + dev_err(uncore_dev->dev, "Error %d registering hotplug", >> ret); >> + return ret; >> + } >> + >> + dev_dbg(uncore_dev->dev, "%s PMU UNCORE registered\n", >> + pmu_uncore->pmu.name); > > > strange alignment, and many more times in the code same as said above. > > >> + return ret; >> +} >> + >> +static struct thunderx2_pmu_uncore_dev *init_pmu_uncore_dev( >> + struct device *dev, acpi_handle handle, >> + struct acpi_device *adev, u32 type) >> +{ >> + struct thunderx2_pmu_uncore_dev *uncore_dev; >> + void __iomem *base; >> + struct resource res; >> + struct resource_entry *rentry; >> + struct list_head list; >> + int ret; >> + >> + INIT_LIST_HEAD(&list); >> + ret = acpi_dev_get_resources(adev, &list, NULL, NULL); >> + if (ret <= 0) { >> + dev_err(dev, "failed to parse _CRS method, error %d\n", >> ret); >> + return NULL; >> + } >> + >> + list_for_each_entry(rentry, &list, node) { >> + if (resource_type(rentry->res) == IORESOURCE_MEM) { >> + res = *rentry->res; >> + break; >> + } >> + } > > > what I am missing that you can not use > platform_get_resource(,IORESOURCE_MEM,)? i have uncore device per socket defined in acpi table and inturn each device has 2 sub devices(l3c and dmc) > > > And I also wonder if you need all the device-related arguments for the code > >> + >> + if (!rentry->res) >> + return NULL; >> + >> + acpi_dev_free_resource_list(&list); >> + base = devm_ioremap_resource(dev, &res); >> + if (IS_ERR(base)) { >> + dev_err(dev, "PMU type %d: Fail to map resource\n", type); >> + return NULL; >> + } >> + >> + uncore_dev = devm_kzalloc(dev, sizeof(*uncore_dev), GFP_KERNEL); >> + if (!uncore_dev) >> + return NULL; >> + >> + uncore_dev->dev = dev; >> + uncore_dev->type = type; >> + uncore_dev->base = base; >> + uncore_dev->node = dev_to_node(dev); >> + >> + raw_spin_lock_init(&uncore_dev->lock); >> + >> + switch (uncore_dev->type) { > > > if we can re-arrange, isn't it better to do the steps which can fail before > the steps which can't? > >> + case PMU_TYPE_L3C: >> + uncore_dev->max_counters = UNCORE_MAX_COUNTERS; >> + uncore_dev->max_channels = UNCORE_L3_MAX_TILES; >> + uncore_dev->max_events = L3_EVENT_MAX; >> + uncore_dev->hrtimer_interval = UNCORE_HRTIMER_INTERVAL; >> + uncore_dev->attr_groups = l3c_pmu_attr_groups; >> + uncore_dev->name = devm_kasprintf(dev, GFP_KERNEL, >> + "uncore_l3c_%d", uncore_dev->node); >> + uncore_dev->init_cntr_base = init_cntr_base_l3c; >> + uncore_dev->start_event = uncore_start_event_l3c; >> + uncore_dev->stop_event = uncore_stop_event_l3c; >> + uncore_dev->select_channel = uncore_select_channel; > > > it's possible to bring the common code outside the swicth statement, but > probably not worth it > >> + break; >> + case PMU_TYPE_DMC: >> + uncore_dev->max_counters = UNCORE_MAX_COUNTERS; >> + uncore_dev->max_channels = UNCORE_DMC_MAX_CHANNELS; >> + uncore_dev->max_events = DMC_EVENT_MAX; >> + uncore_dev->hrtimer_interval = UNCORE_HRTIMER_INTERVAL; >> + uncore_dev->attr_groups = dmc_pmu_attr_groups; >> + uncore_dev->name = devm_kasprintf(dev, GFP_KERNEL, >> + "uncore_dmc_%d", uncore_dev->node); >> + uncore_dev->init_cntr_base = init_cntr_base_dmc; >> + uncore_dev->start_event = uncore_start_event_dmc; >> + uncore_dev->stop_event = uncore_stop_event_dmc; >> + uncore_dev->select_channel = uncore_select_channel; >> + break; >> + case PMU_TYPE_INVALID: >> + devm_kfree(dev, uncore_dev); > > > do you really need this? yes, in case corrupted table. > >> + uncore_dev = NULL; >> + break; > > > return NULL > > And don't we require a default statement? > >> + } >> + >> + return uncore_dev; >> +} >> + >> +static acpi_status thunderx2_pmu_uncore_dev_add(acpi_handle handle, u32 >> level, >> + void *data, void **return_value) >> +{ >> + struct thunderx2_pmu_uncore_dev *uncore_dev; >> + struct acpi_device *adev; >> + enum thunderx2_uncore_type type; >> + int channel; >> + >> + if (acpi_bus_get_device(handle, &adev)) >> + return AE_OK; >> + if (acpi_bus_get_status(adev) || !adev->status.present) >> + return AE_OK; >> + >> + type = get_uncore_device_type(adev); >> + if (type == PMU_TYPE_INVALID) >> + return AE_OK; >> + >> + uncore_dev = init_pmu_uncore_dev((struct device *)data, handle, > > > no need to cast void * > > >> + adev, type); >> + >> + if (!uncore_dev) >> + return AE_ERROR; >> + >> + for (channel = 0; channel < uncore_dev->max_channels; channel++) { >> + if (thunderx2_pmu_uncore_add(uncore_dev, channel)) { >> + /* Can't add the PMU device, abort */ >> + return AE_ERROR; >> + } >> + } >> + return AE_OK; >> +} >> + >> +static int thunderx2_uncore_pmu_offline_cpu(unsigned int cpu, >> + struct hlist_node *node) >> +{ >> + int new_cpu; >> + struct thunderx2_pmu_uncore_channel *pmu_uncore; >> + >> + pmu_uncore = hlist_entry_safe(node, >> + struct thunderx2_pmu_uncore_channel, node); >> + if (cpu != pmu_uncore->cpu) >> + return 0; >> + >> + new_cpu = cpumask_any_and( >> + cpumask_of_node(pmu_uncore->uncore_dev->node), >> + cpu_online_mask); >> + if (new_cpu >= nr_cpu_ids) >> + return 0; >> + >> + pmu_uncore->cpu = new_cpu; >> + perf_pmu_migrate_context(&pmu_uncore->pmu, cpu, new_cpu); >> + return 0; >> +} >> + >> +static const struct acpi_device_id thunderx2_uncore_acpi_match[] = { >> + {"CAV901C", 0}, >> + {}, > > > no ',' required ok > >> +}; >> +MODULE_DEVICE_TABLE(acpi, thunderx2_uncore_acpi_match); >> + >> +static int thunderx2_uncore_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + acpi_handle handle; >> + acpi_status status; >> + >> + set_dev_node(dev, acpi_get_node(ACPI_HANDLE(dev))); > > > Is this already done when the platform device is created in ACPI > enumeration? I assume the child devices have enumerated at this point. no > > >> + >> + /* Make sure firmware supports DMC/L3C set channel smc call */ >> + if (test_uncore_select_channel_early(dev)) >> + return -ENODEV; >> + >> + if (!has_acpi_companion(dev)) >> + return -ENODEV; >> + >> + handle = ACPI_HANDLE(dev); >> + if (!handle) >> + return -EINVAL; >> + >> + /* Walk through the tree for all PMU UNCORE devices */ >> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, >> + thunderx2_pmu_uncore_dev_add, >> + NULL, dev, NULL); >> + if (ACPI_FAILURE(status)) { >> + dev_err(dev, "failed to probe PMU devices\n"); >> + return_ACPI_STATUS(status); >> + } >> + >> + dev_info(dev, "node%d: pmu uncore registered\n", >> dev_to_node(dev)); >> + return 0; >> +} >> + >> +static struct platform_driver thunderx2_uncore_driver = { >> + .probe = thunderx2_uncore_probe, > > > why no remove? this is built in driver. > > >> + .driver = { >> + .name = "thunderx2-uncore-pmu", >> + .acpi_match_table = ACPI_PTR(thunderx2_uncore_acpi_match), >> + }, >> +}; >> + >> +static int __init register_thunderx2_uncore_driver(void) >> +{ >> + int ret; >> + >> + ret = >> cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_THUNDERX2_UNCORE_ONLINE, >> + "perf/tx2/uncore:online", >> + NULL, >> + thunderx2_uncore_pmu_offline_cpu); >> + if (ret) >> + return ret; >> + >> + return platform_driver_register(&thunderx2_uncore_driver); >> + >> +} >> +device_initcall(register_thunderx2_uncore_driver); >> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h >> index 8796ba3..eb0c896 100644 >> --- a/include/linux/cpuhotplug.h >> +++ b/include/linux/cpuhotplug.h >> @@ -161,6 +161,7 @@ enum cpuhp_state { >> CPUHP_AP_PERF_ARM_L2X0_ONLINE, >> CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE, >> CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE, >> + CPUHP_AP_PERF_ARM_THUNDERX2_UNCORE_ONLINE, >> CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE, >> CPUHP_AP_PERF_POWERPC_CORE_IMC_ONLINE, >> CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE, >> > > thanks Ganapat -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
