RE: [PATCH v4 1/2] perf: arm_cspmu: Add support for ARM CoreSight PMU driver

Hi Will,

Thanks for the comment. Please see my response inline.

-----Original Message-----
From: Will Deacon <will@kernel.org>
Sent: Thursday, September 22, 2022 8:53 AM
To: Besar Wicaksono <redacted>
Cc: suzuki.poulose@arm.com; robin.murphy@arm.com;
catalin.marinas@arm.com; mark.rutland@arm.com; linux-arm-
kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
tegra@vger.kernel.org; sudeep.holla@arm.com;
thanu.rangarajan@arm.com; Michael.Williams@arm.com; Thierry Reding
[off-list ref]; Jonathan Hunter [off-list ref]; Vikram
Sethi [off-list ref]; mathieu.poirier@linaro.org;
mike.leach@linaro.org; leo.yan@linaro.org
Subject: Re: [PATCH v4 1/2] perf: arm_cspmu: Add support for ARM
CoreSight PMU driver

External email: Use caution opening links or attachments


On Sun, Aug 14, 2022 at 01:23:50PM -0500, Besar Wicaksono wrote:

quoted

Add support for ARM CoreSight PMU driver framework and interfaces.
The driver provides generic implementation to operate uncore PMU based
on ARM CoreSight PMU architecture. The driver also provides interface
to get vendor/implementation specific information, for example event
attributes and formating.

The specification used in this implementation can be found below:
 * ACPI Arm Performance Monitoring Unit table:
        https://developer.arm.com/documentation/den0117/latest
 * ARM Coresight PMU architecture:
        https://developer.arm.com/documentation/ihi0091/latest

Signed-off-by: Besar Wicaksono <redacted>
---
 arch/arm64/configs/defconfig       |    1 +
 drivers/perf/Kconfig               |    2 +
 drivers/perf/Makefile              |    1 +
 drivers/perf/arm_cspmu/Kconfig     |   13 +
 drivers/perf/arm_cspmu/Makefile    |    6 +
 drivers/perf/arm_cspmu/arm_cspmu.c | 1262

++++++++++++++++++++++++++++

quoted

 drivers/perf/arm_cspmu/arm_cspmu.h |  151 ++++
 7 files changed, 1436 insertions(+)
 create mode 100644 drivers/perf/arm_cspmu/Kconfig
 create mode 100644 drivers/perf/arm_cspmu/Makefile
 create mode 100644 drivers/perf/arm_cspmu/arm_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/arm_cspmu.h

[...]

quoted

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c

b/drivers/perf/arm_cspmu/arm_cspmu.c

quoted

new file mode 100644
index 000000000000..410876f86eb0

--- /dev/null
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c

[...]

quoted

+/*
+ * Read 64-bit register as a pair of 32-bit registers using hi-lo-hi sequence.
+ */
+static u64 read_reg64_hilohi(const void __iomem *addr)
+{
+     u32 val_lo, val_hi;
+     u64 val;
+
+     /* Use high-low-high sequence to avoid tearing */
+     do {
+             val_hi = readl(addr + 4);
+             val_lo = readl(addr);
+     } while (val_hi != readl(addr + 4));

Hmm, we probably want a timeout or something in here so we don't lock
up the CPU if the device goes wonky.

This function is used to read the counter register. The perf driver APIs
(read, stop) that use this function do not return an error code. I am
not sure if we can just break the loop and return 0. Any suggestions ?
Is triggering a panic acceptable ?

With that, how about adding this a helper to
include/linux/io-64-nonatomic-*o.h so other folks can reuse it?

quoted

+/* Check if PMU supports 64-bit single copy atomic. */
+static inline bool supports_64bit_atomics(const struct arm_cspmu

*cspmu)

quoted

+{
+     return CHECK_APMT_FLAG(cspmu->apmt_node->flags, ATOMIC,

SUPP);

quoted

+}

Is this just there because the architecture permits it, or are folks
actually hanging these things off 32-bit MMIO buses on arm64 SoCs?

Yes, the PMU spec permits a system that needs to break 64-bit access into
a pair of 32-bit accesses.

quoted

+static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
+{
+     int irq, ret;
+     struct device *dev;
+     struct platform_device *pdev;
+     struct acpi_apmt_node *apmt_node;
+
+     dev = cspmu->dev;
+     pdev = to_platform_device(dev);
+     apmt_node = cspmu->apmt_node;
+
+     /* Skip IRQ request if the PMU does not support overflow interrupt. */
+     if (apmt_node->ovflw_irq == 0)
+             return 0;

Set PERF_PMU_CAP_NO_INTERRUPT?

Thanks, I will apply it on the next version.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel