Re: [PATCH v8 1/6] perf/marvell: Refactor to extract platform data - no functional change

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 19 Sep 2024 13:17:12 +0530
Gowthami Thiagarajan <gthiagarajan@marvell.com> wrote:

> This commit introduces a refactor to the Marvell DDR pmu driver,
> specifically targeting the extraction of platform data
> (referred to as "pdata") from the existing driver. The purpose of
> this refactor is to prepare for the upcoming support of the next
> version of the Performance Monitoring Unit (PMU) in this
> driver.
> 
> No functional changes are introduced in this refactor. Its sole
> purpose is to improve code organization and pave the way for
> future enhancements to the driver.
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
A few drive by comments as I was curious.

Jonathan

> ---
>  drivers/perf/marvell_cn10k_ddr_pmu.c | 127 +++++++++++++++++++--------
>  1 file changed, 92 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> index 94f1ebcd2a27..e33d383aa6d2 100644
> --- a/drivers/perf/marvell_cn10k_ddr_pmu.c
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
> @@ -1,7 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
> -/* Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
> +/*
> + * Marvell CN10K DRAM Subsystem (DSS) Performance Monitor Driver
>   *
> - * Copyright (C) 2021 Marvell.
> + * Copyright (C) 2024 Marvell.

Maybe 2021-2024 is appropriate?


>   */
>  
>  #include <linux/init.h>
> @@ -14,24 +15,24 @@
>  #include <linux/platform_device.h>
>  
>  /* Performance Counters Operating Mode Control Registers */
> -#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
> -#define OP_MODE_CTRL_VAL_MANNUAL	0x1
> +#define CN10K_DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
> +#define OP_MODE_CTRL_VAL_MANUAL	0x1

Typo fix probably belongs in a separate patch.


>  /* 8 Generic event counter + 2 fixed event counters */
>  #define DDRC_PERF_NUM_GEN_COUNTERS	8
> @@ -42,17 +43,19 @@
>  					 DDRC_PERF_NUM_FIX_COUNTERS)
>  
>  /* Generic event counter registers */
> -#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
> +#define DDRC_PERF_CFG(base, n)		((base) + 8 * (n))
>  #define EVENT_ENABLE			BIT_ULL(63)
>  
>  /* Two dedicated event counters for DDR reads and writes */
>  #define EVENT_DDR_READS			101
>  #define EVENT_DDR_WRITES		100
>  
> +#define DDRC_PERF_REG(base, n)		((base) + 8 * (n))
>  /*
>   * programmable events IDs in programmable event counters.
>   * DO NOT change these event-id numbers, they are used to
>   * program event bitmap in h/w.
> + *

Stray change. Drop this additional line.

>   */
>  #define EVENT_OP_IS_ZQLATCH			55
>  #define EVENT_OP_IS_ZQSTART			54
> @@ -63,8 +66,8 @@
>  #define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
>  #define EVENT_BSM_STARVATION			48
>  #define EVENT_BSM_ALLOC				47
> -#define EVENT_LPR_REQ_WITH_NOCREDIT		46
> -#define EVENT_HPR_REQ_WITH_NOCREDIT		45
> +#define EVENT_RETRY_FIFO_FULL_OR_LPR_REQ_NOCRED	46
> +#define EVENT_DFI_OR_HPR_REQ_NOCRED		45
Is this reflecting a 'fix' of the naming, or the broadening
of this event for a different IP?
If it is a 'fix' then do it first, if it is an either or
kind of thing then maybe additional define for the other use is
appropriate?

Pulling it out to a patch where you can explain the change
is probably a good idea rather than burying it in here.


>  
> -/* Fixed event counter value register */
> -#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
> -#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
>  #define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
>  #define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
>  
> +/* Fixed event counter value register */
> +#define CN10K_DDRC_PERF_CNT_VALUE_WR_OP		0x80D0
> +#define CN10K_DDRC_PERF_CNT_VALUE_RD_OP		0x80D8
> +
>  struct cn10k_ddr_pmu {
>  	struct pmu pmu;
>  	void __iomem *base;
> +	const struct ddr_pmu_platform_data *p_data;
>  	unsigned int cpu;
>  	struct	device *dev;
>  	int active_events;
> @@ -134,6 +138,22 @@ struct cn10k_ddr_pmu {
>  
>  #define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
>  
> +struct ddr_pmu_platform_data {
> +	u64 counter_overflow_val;
> +	u64 counter_max_val;
> +	u64 ddrc_perf_cnt_base;

As this is in a structure that tells you it's about ddrc pmu
can you drop the ddrc_perf_ prefix?

> +	u64 ddrc_perf_cfg_base;
> +	u64 ddrc_perf_cnt_op_mode_ctrl;
> +	u64 ddrc_perf_cnt_start_op_ctrl;
> +	u64 ddrc_perf_cnt_end_op_ctrl;
> +	u64 ddrc_perf_cnt_end_status;
> +	u64 ddrc_perf_cnt_freerun_en;
> +	u64 ddrc_perf_cnt_freerun_ctrl;
> +	u64 ddrc_perf_cnt_freerun_clr;
> +	u64 ddrc_perf_cnt_value_wr_op;
> +	u64 ddrc_perf_cnt_value_rd_op;
> +};