Re: [PATCH v2] perf/hx_arm_ni: Support uncore ARM NI-700 PMU
From: Krzysztof Kozlowski
Date: Wed Jan 31 2024 - 02:59:25 EST
On 31/01/2024 08:08, JiaLong.Yang wrote:
> This code is based on uncore PMUs arm_smmuv3_pmu and arm-cmn.
> One ni-700 can have many clock domains. Each of them has only one PMU.
> Here one PMU corresponds to one 'struct ni_pmu' instance.
> PMU name will be ni_pmu_N_M, which N means different NI-700s and M means
> different PMU in one NI-700. If only one NI-700 found in NI-700, name will
> be ni_pmu_N.
> Node interface event name will be xxni_N_eventname, such as
> asni_0_rdreq_any. There are many kinds of type of nodes in one clock
> domain. Also means that there are many kinds of that in one PMU. So we
> distinguish them by xxni string. Besides, maybe there are many nodes
> have same type. So we have number N in event name.
> By ni_pmu_0_0/asni_0_rdreq_any/, we can pinpoint accurate bus traffic.
> Example1: perf stat -a -e ni_pmu_0_0/asni_0_rdreq_any/,ni_pmu_0_0/cycles/
> EXample2: perf stat -a -e ni_pmu_0_0/asni,id=0,event=0x0/
>
> Signed-off-by: JiaLong.Yang <jialong.yang@xxxxxxxxxxxx>
> ---
> v1 --> v2:
> 1. Submit MAINTANER Documentation/ files seperately.
SEPARATE PATCHES, not patchsets. You have now checkpatch warnings
because of this...
> 2. Delete some useless info printing.
> 3. Change print from pr_xxx to dev_xxx.
> 4. Fix more than 75 length log info.
> 5. Fix dts attribute pccs-id.
> 6. Fix generic name according to DT specification.
> 7. Some indentation.
> 8. Del of_match_ptr macro.
>
> drivers/perf/Kconfig | 11 +
> drivers/perf/Makefile | 1 +
> drivers/perf/hx_arm_ni.c | 1284 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1296 insertions(+)
> create mode 100644 drivers/perf/hx_arm_ni.c
>
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index ec6e0d9194a1..95ef8b13730f 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -241,4 +241,15 @@ config CXL_PMU
>
> If unsure say 'm'.
>
> +config HX_ARM_NI_PMU
> + tristate "HX ARM NI-700 PMU"
> + depends on PPC_HX_C2000 && 64BIT
1. There is no PPC_HX_C2000.
2. Nothing justified dependency on 64bit. Drop or explain. Your previous
message did not provide real rationale.
3. Your indentation here is entirely mismatched. Read the coding style.
> + default y
> + help
> + Support for NI-700(Network-on-chip Interconnect) PMUs, which
> + provide monitoring of transactions passing through between
> + CMN and other buses or periapherals.
> +
> +source "drivers/perf/hisilicon/Kconfig"
> +
> endmenu
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index a06338e3401c..ec8b9c08577d 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -27,3 +27,4 @@ obj-$(CONFIG_DWC_PCIE_PMU) += dwc_pcie_pmu.o
> obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu/
> obj-$(CONFIG_MESON_DDR_PMU) += amlogic/
> obj-$(CONFIG_CXL_PMU) += cxl_pmu.o
> +obj-$(CONFIG_HX_ARM_NI_PMU) += hx_arm_ni.o
> diff --git a/drivers/perf/hx_arm_ni.c b/drivers/perf/hx_arm_ni.c
> new file mode 100644
> index 000000000000..619e3b789dda
> --- /dev/null
> +++ b/drivers/perf/hx_arm_ni.c
> @@ -0,0 +1,1284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HX ARM-NI-700 uncore PMU support
> + *
> + * This code is based on uncore PMUs arm_smmuv3_pmu and arm-cmn.
> + *
> + * One ni-700 can have many clock domains. Each of them has only one PMU.
> + * Here one PMU corresponds to one 'struct ni_pmu' instance.
> + *
> + * PMU name will be ni_pmu_N_M, which N means different NI-700s and M means
> + * different PMU in one NI-700. If only one NI-700 found in NI-700, name
> + * will be ni_pmu_N.
> + *
> + * Node interface event name will be xxni_N_eventname, such as
> + * asni_0_rdreq_any. There are many kinds of type of nodes in one clock
> + * domain. Also means that there are many kinds of that in one PMU. So we
> + * distinguish them by xxni string. Besides, maybe there are many nodes
> + * have same type. So we have number N in event name.
> + * By ni_pmu_0_0/asni_0_rdreq_any/, we can pinpoint accurate bus traffic.
> + *
> + * Example1: perf stat -a -e ni_pmu_0_0/asni_0_rdreq_any/,ni_pmu_0_0/cycles/
> + * Example2: perf stat -a -e ni_pmu_0_0/asni,id=0,event=0x0/
> + *
> + * TODO: Secure or non-secure attribute in all event omitted now.
> + *
> + */
> +
> +#define dev_fmt(fmt) "ni-700 pmu: " fmt
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/cpumask.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/msi.h>
> +#include <linux/of.h>
> +#include <linux/perf_event.h>
> +#include <linux/platform_device.h>
> +#include <linux/smp.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <linux/build_bug.h>
> +
> +/* number of counters in one ni pmu */
> +#define NI_PMU_COUNTER_NUM 8
> +
> +/* node type values */
> +enum ni_node_type {
> + NI_BASE = 0x0,
> + NI_VD,
> + NI_PD,
> + NI_CD,
> + NI_ASNI = 0x4,
> + NI_AMNI,
> + NI_PMU,
> + NI_HSNI,
> + NI_HMNI,
> + NI_PMNI = 0x9,
> +};
> +
> +/* event format */
> +/**
That's not kerneldoc.
You must test your code with W=1 build, sparse and smatch.
> + * config:
> + * 0-5 31 32-47 48-63
> + * event cycles node_type node_id
> + *
> + */
> +#define NI_EVENT_FORMAT_EVENT GENMASK_ULL(5, 0)
> +#define NI_EVENT_FORMAT_CYCLES (1ULL << 31)
> +#define NI_EVENT_FORMAT_NODETYPE GENMASK_ULL(32 + NI_PMNI, 32)
> +#define NI_EVENT_FORMAT_ASNI BIT(32 + NI_ASNI)
> +#define NI_EVENT_FORMAT_AMNI BIT(32 + NI_AMNI)
> +#define NI_EVENT_FORMAT_HSNI BIT(32 + NI_HSNI)
> +#define NI_EVENT_FORMAT_HMNI BIT(32 + NI_HMNI)
> +#define NI_EVENT_FORMAT_PMNI BIT(32 + NI_PMNI)
> +#define NI_EVENT_FORMAT_NODEID GENMASK_ULL(63, 48)
> +
..
> +
> +static ssize_t ni_event_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct ni_event_attr *eattr;
> +
> + eattr = to_ni_event_attr(attr);
> +
> + if (eattr->ev_desc)
> + return sysfs_emit(buf,
> + "%s,id=0x%x,event=0x%llx\n",
> + ni_node_name[eattr->node->type],
> + eattr->node->id,
> + eattr->ev_desc->eventid);
> +
> + return sysfs_emit(buf, "cycles\n");
> +}
> +
> +struct ni_format_attr {
> + struct device_attribute attr;
> + u64 field;
> +};
Declarations go to the top of the file.
> +
..
> +
> +static bool is_event_supported(u64 eventid, enum ni_node_type type)
> +{
> + int num;
> + int idx;
> + struct ni_event_desc **descs;
> +
> + num = ni_ev_desc_array_size(type, &descs);
> +
> + for (idx = 0; idx < num; idx++)
> + if (eventid == descs[idx]->eventid)
> + break;
> +
> + return idx == num ? false : true;
> +}
> +
> +static enum ni_node_type ni_event_config_nodetype(u64 config)
> +{
> + u64 nodetype = _ni_event_config_nodetype(config);
> + unsigned long lo = __ffs(nodetype), hi = __fls(nodetype);
> +
> + if (!nodetype || lo != hi)
> + return 0;
> +
> + return lo;
> +
Redundant blank line. Clean it up from the code.
> +}
> +
..
> +
> +static irqreturn_t ni_pmu_handle_irq(int irq_num, void *data)
> +{
> + struct ni_pmu *ni_pmu = data;
> + int idx, ret = IRQ_NONE;
> +
> + if (ni_pmu->ni->irq_num != 1)
> + return _ni_pmu_handle_irq(ni_pmu);
> +
> + for (idx = 0; idx < ni_pmu->ni->pmu_num; idx++)
> + ret |= _ni_pmu_handle_irq(ni_pmu->ni->ni_pmus[idx]);
> +
> + return ret;
> +}
> +
> +static int ni_hp_state;
Drop. No file-scope variables. And for 100% no file scope variables
hidden in the middle of something else.
> +static int ni_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> + struct global_ni *ni;
> + unsigned int target;
> + int idx;
> +
> +
> + ni = hlist_entry_safe(node, struct global_ni, node);
> + if (cpu != ni->on_cpu)
> + return 0;
> +
> +
> + target = cpumask_any_but(cpu_online_mask, cpu);
> + if (target >= nr_cpu_ids)
> + return 0;
> +
> +
> + for (idx = 0; idx < ni->pmu_num; idx++) {
> + perf_pmu_migrate_context(&ni->ni_pmus[idx]->pmu, cpu, target);
> +#ifndef CONFIG_PPC_HX_C2000
Drop, it does not exist.
> + WARN_ON(irq_set_affinity(ni->ni_pmus[idx]->irq, cpumask_of(target)));
> +#endif
> + }
> +
> + ni->on_cpu = target;
> +
> + return 0;
> +}
> +
> +static u32 ni_child_number_total(void __iomem *periphbase,
> + void __iomem *from, enum ni_node_type type)
> +{
> + enum ni_node_type node_type;
> + int total, idx;
> + void __iomem *child_base;
> +
> + node_type = ni_node_type(from);
> +
> + if (node_type == type)
> + return 1;
> +
> + if (node_type >= NI_ASNI)
> + return 0;
> +
> + total = 0;
> + for (idx = 0; idx < ni_child_number(from); idx++) {
> + child_base = ni_child_pointer(periphbase, from, idx);
> + total += ni_child_number_total(periphbase, child_base, type);
> + }
> +
> + return total;
> +}
> +
> +static void ni_pmu_reset(struct ni_pmu *ni_pmu)
> +{
> + ni_pmu_disable(&ni_pmu->pmu);
> +
> +#define clear_reg(name) \
> + writel(readl(ni_pmu_offset(ni_pmu, name)), ni_pmu_offset(ni_pmu, name))
> +
> + clear_reg(pmcntenclr);
> + clear_reg(pmintenclr);
> + clear_reg(pmovsclr);
> +
> + writel_relaxed(NI_PMU_PMCR_RST_CYC_CNTR & NI_PMU_PMCR_RST_EV_CNTR,
> + ni_pmu_offset(ni_pmu, pmcr));
> +}
> +
> +static int ni_pmu_irq_setup(struct ni_pmu *ni_pmu, int irq_idx)
> +{
> + int err;
> + unsigned long flags = IRQF_NOBALANCING | IRQF_SHARED | IRQF_NO_THREAD;
> +
> + ni_pmu->irq = platform_get_irq(to_platform_device(ni_pmu->dev), irq_idx);
> + if (ni_pmu->irq < 0)
> + return ni_pmu->irq;
> +
> + err = devm_request_irq(ni_pmu->dev, ni_pmu->irq, ni_pmu_handle_irq,
> + flags, dev_name(ni_pmu->dev), ni_pmu);
> + if (err)
> + return err;
> +
> +#ifndef CONFIG_PPC_HX_C2000
Drop, it does not exist.
> + err = irq_set_affinity(ni_pmu->irq, cpumask_of(ni_pmu->ni->on_cpu));
> + if (err)
> + return err;
> +#endif
> +
> + return 0;
> +}
..
> +static int ni_discovery(struct global_ni *ni)
> +{
> + u32 vd_idx, pd_idx, cd_idx, nd_idx, num_idx = 0;
> + void __iomem *vd, *pd, *cd, *nd, **cd_arrays;
> + int num;
> + struct ni_pmu *ni_pmu;
> + struct ni_node node;
> + void __iomem *pbase;
> + struct device *dev = ni->dev;
> +
> + pbase = ni->base;
> +
> + cd_arrays = devm_kmalloc(dev, ni->cd_num * sizeof(typeof(cd)), GFP_KERNEL);
> +
> + /* Step1: Get all clock domains. */
> + for (vd_idx = 0; vd_idx < ni_child_number(ni->base); vd_idx++) {
> + vd = ni_child_pointer(pbase, ni->base, vd_idx);
> +
> + for (pd_idx = 0; pd_idx < ni_child_number(vd); pd_idx++) {
> + pd = ni_child_pointer(pbase, vd, pd_idx);
> +
> + dev_dbg(dev, "The %dth power domain has %d clock domain",
> + pd_idx,
> + ni_child_number(pd));
> +
> + for (cd_idx = 0; cd_idx < ni_child_number(pd); cd_idx++) {
> + cd_arrays[num_idx++] =
> + ni_child_pointer(pbase, pd, cd_idx);
> + }
> + }
> + }
> +
> + /* Step2: Traverse all clock domains. */
> + for (cd_idx = 0; cd_idx < ni->cd_num; cd_idx++) {
> + cd = cd_arrays[cd_idx];
> +
> + num = ni_child_number(cd);
> + dev_dbg(dev, "The %dth clock domain has %d child nodes:", cd_idx, num);
> +
> + /* Omit pmu node */
> + ni_pmu = devm_kzalloc(dev, struct_size(ni_pmu, ev_src_nodes, num - 1),
> + GFP_KERNEL);
> + ni_pmu->ev_src_num = num - 1;
> +
> + if (!ni_pmu)
> + return -ENOMEM;
> +
> + num_idx = 0;
> + for (nd_idx = 0; nd_idx < num; nd_idx++) {
> + nd = ni_child_pointer(pbase, cd, nd_idx);
> +
> + node.base = nd;
> + node.node_type = ni_node_node_type(nd);
> +
> + if (unlikely(ni_node_type(nd) == NI_PMU))
> + ni_pmu->pmu_node = node;
> + else
> + ni_pmu->ev_src_nodes[num_idx++] = node;
> + dev_dbg(dev, " name: %s id: %d", ni_node_name[node.type], node.id);
> + }
> +
> + ni_pmu->dev = dev;
> + ni_pmu->ni = ni;
> + ni->ni_pmus[cd_idx] = ni_pmu;
> + }
> +
> + devm_kfree(dev, cd_arrays);
Why? If it is not device-lifetime then allocate with usual way.
> +
> + return 0;
> +}
> +
> +static int ni_pmu_probe(struct platform_device *pdev)
> +{
> + int ret, cd_num, idx, irq_num, irq_idx;
> + void __iomem *periphbase;
> + struct global_ni *ni;
> + struct device *dev = &pdev->dev;
> + char *name;
> + static int id;
> + struct ni_pmu *ni_pmu;
> +
> + BUILD_BUG_ON(sizeof(struct ni_hw_perf_event) >
> + offsetof(struct hw_perf_event, target));
> +#define NI_PMU_REG_MAP_SIZE 0xE08
> + BUILD_BUG_ON(sizeof(struct ni_pmu_reg_map) != NI_PMU_REG_MAP_SIZE);
> +
> + periphbase = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(periphbase)) {
> + dev_err_probe(dev, PTR_ERR(periphbase), "Couldn't get ioremap\n");
> + return PTR_ERR(periphbase);
I wrote you the syntax last time:
return dev_err_probe
> + }
> +
..
> +
> +static const struct of_device_id ni_pmu_of_match[] = {
> + { .compatible = "hx,c2000-arm-ni" },
Don't send undocumented compatibles.
Best regards,
Krzysztof