Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 23 May 2022 11:37:33 -0500
From:   Rob Herring <robh@kernel.org>
To:     Tanmay Jagdale <tanmay@marvell.com>
Cc:     will@kernel.org, mark.rutland@arm.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        sgoutham@marvell.com, lcherian@marvell.com, bbhushan2@marvell.com,
        amitsinght@marvell.com
Subject: Re: [PATCH] perf/marvell_cn10k: Add MPAM support for TAD PMU
Message-ID: <20220523163733.GA1712487-robh@kernel.org>
References: <20220523145738.2750368-1-tanmay@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220523145738.2750368-1-tanmay@marvell.com>
Precedence: bulk

On Mon, May 23, 2022 at 08:27:38PM +0530, Tanmay Jagdale wrote:
> The TAD PMU supports following counters that can be filtered by MPAM
> partition id.

How are you setting the PARTID? There's no support yet in the kernel to 
set it. 

>     - (0x1a) tad_alloc_dtg : Allocations to DTG.
>     - (0x1b) tad_alloc_ltg : Allocations to LTG.
>     - (0x1c) tad_alloc_any : Total allocations to DTG/LTG.
>     - (0x1d) tad_hit_dtg   : DTG hits.
>     - (0x1e) tad_hit_ltg   : LTG hits.
>     - (0x1f) tad_hit_any   : Hit in LTG/DTG.
>     - (0x20) tad_tag_rd    : Total tag reads.
> 
> Add a new 'partid' attribute of 16-bits to get the partition id
> passed from perf tool. This value would be stored in config1 field
> of perf_event_attr structure.
> 
> Example:
> perf stat -e tad/tad_alloc_any,partid=0x12/ <program>

How would userspace get the 0x12 value?

> 
> - Drop read of TAD_PRF since we don't have to preserve any
>   bit fields and always write an updated value.
> - Update register offsets of TAD_PRF and TAD_PFC.
> 
> Signed-off-by: Tanmay Jagdale <tanmay@marvell.com>
> ---
>  drivers/perf/marvell_cn10k_tad_pmu.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
> index 282d3a071a67..f552e6bffcac 100644
> --- a/drivers/perf/marvell_cn10k_tad_pmu.c
> +++ b/drivers/perf/marvell_cn10k_tad_pmu.c
> @@ -18,10 +18,12 @@
>  #include <linux/perf_event.h>
>  #include <linux/platform_device.h>
>  
> -#define TAD_PFC_OFFSET		0x0
> +#define TAD_PFC_OFFSET		0x800
>  #define TAD_PFC(counter)	(TAD_PFC_OFFSET | (counter << 3))
> -#define TAD_PRF_OFFSET		0x100
> +#define TAD_PRF_OFFSET		0x900
>  #define TAD_PRF(counter)	(TAD_PRF_OFFSET | (counter << 3))
> +#define TAD_PRF_MATCH_PARTID	(1 << 8)
> +#define TAD_PRF_PARTID_NS	(1 << 10)
>  #define TAD_PRF_CNTSEL_MASK	0xFF
>  #define TAD_MAX_COUNTERS	8

Does this h/w block follow the MPAM specification or just uses PARTID in 
its own way?

Rob