Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f67bf3b6-c8ba-4134-5772-ce750f4bb4dd@arm.com>
Date:   Thu, 29 Sep 2022 09:29:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.13.1
Subject: Re: [PATCH v5 1/2] perf: arm_cspmu: Add support for ARM CoreSight PMU
 driver
To:     Besar Wicaksono <bwicaksono@nvidia.com>, robin.murphy@arm.com,
        catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com
Cc:     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,
        treding@nvidia.com, jonathanh@nvidia.com, vsethi@nvidia.com,
        mathieu.poirier@linaro.org, mike.leach@linaro.org,
        leo.yan@linaro.org
References: <20220928201830.45637-1-bwicaksono@nvidia.com>
 <20220928201830.45637-2-bwicaksono@nvidia.com>
From:   Suzuki K Poulose <suzuki.poulose@arm.com>
In-Reply-To: <20220928201830.45637-2-bwicaksono@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Besar

On 28/09/2022 21:18, 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
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Besar Wicaksono <bwicaksono@nvidia.com>

Apologies for this late comment. Please find it below.

> ---
>   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 | 1276 ++++++++++++++++++++++++++++
>   drivers/perf/arm_cspmu/arm_cspmu.h |  151 ++++
>   7 files changed, 1450 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/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 7d1105343bc2..ee31c9159a5b 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1212,6 +1212,7 @@ CONFIG_PHY_UNIPHIER_USB3=y
>   CONFIG_PHY_TEGRA_XUSB=y
>   CONFIG_PHY_AM654_SERDES=m
>   CONFIG_PHY_J721E_WIZ=m
> +CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU=y
>   CONFIG_ARM_SMMU_V3_PMU=m
>   CONFIG_FSL_IMX8_DDR_PMU=m
>   CONFIG_QCOM_L2_PMU=y
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 1e2d69453771..c94d3601eb48 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -192,4 +192,6 @@ config MARVELL_CN10K_DDR_PMU
>   	  Enable perf support for Marvell DDR Performance monitoring
>   	  event on CN10K platform.
>   
> +source "drivers/perf/arm_cspmu/Kconfig"
> +
>   endmenu
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 57a279c61df5..3bc9323f0965 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
>   obj-$(CONFIG_MARVELL_CN10K_TAD_PMU) += marvell_cn10k_tad_pmu.o
>   obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
>   obj-$(CONFIG_APPLE_M1_CPU_PMU) += apple_m1_cpu_pmu.o
> +obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu/
> diff --git a/drivers/perf/arm_cspmu/Kconfig b/drivers/perf/arm_cspmu/Kconfig
> new file mode 100644
> index 000000000000..c2c56ecafccb
> --- /dev/null
> +++ b/drivers/perf/arm_cspmu/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
> +
> +config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
> +	tristate "ARM Coresight Architecture PMU"
> +	depends on ACPI
> +	depends on ACPI_APMT || COMPILE_TEST
> +	help
> +	  Provides support for performance monitoring unit (PMU) devices
> +	  based on ARM CoreSight PMU architecture. Note that this PMU
> +	  architecture does not have relationship with the ARM CoreSight
> +	  Self-Hosted Tracing.
> diff --git a/drivers/perf/arm_cspmu/Makefile b/drivers/perf/arm_cspmu/Makefile
> new file mode 100644
> index 000000000000..cdc3455f74d8
> --- /dev/null
> +++ b/drivers/perf/arm_cspmu/Makefile
> @@ -0,0 +1,6 @@
> +# Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
> +#
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += \
> +	arm_cspmu.o
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> new file mode 100644
> index 000000000000..0fa5f29bf1c6
> --- /dev/null
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -0,0 +1,1276 @@


> +/* Default event list. */
> +static struct attribute *arm_cspmu_event_attrs[] = {
> +	ARM_CSPMU_EVENT_ATTR(cycles, ARM_CSPMU_EVT_CYCLES_DEFAULT),
> +	NULL,
> +};
> +
> +static struct attribute **
> +arm_cspmu_get_event_attrs(const struct arm_cspmu *cspmu)
> +{
> +	return arm_cspmu_event_attrs;
> +}

This would make all the "PMU" instances (which don't implement the
callback) share a non-const array. Could we instead return a copy of
the attrs to avoid drivers messing up with the array ?
The array could be allocated via devm_ on the specific device, thus
it gets cleaned up on the device tear down ?

...

> +static struct attribute **
> +arm_cspmu_get_format_attrs(const struct arm_cspmu *cspmu)
> +{
> +	return arm_cspmu_format_attrs;
> +}
> +

Same as above

Suzuki