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

[Will Deacon]

On Sun, Aug 14, 2022 at 01:23:50PM -0500, Besar Wicaksono wrote:
> 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 <bwicaksono@xxxxxxxxxx>
> ---
>  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 ++++++++++++++++++++++++++++
>  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

[...]

> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> new file mode 100644
> index 000000000000..410876f86eb0
> --- /dev/null
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c

[...]

> +/*
> + * 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.

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

> +/* Check if PMU supports 64-bit single copy atomic. */
> +static inline bool supports_64bit_atomics(const struct arm_cspmu *cspmu)
> +{
> +	return CHECK_APMT_FLAG(cspmu->apmt_node->flags, ATOMIC, SUPP);
> +}

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

> +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?

Will