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

Jonathan Cameron
2024-09-24T16:18:13Z

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;
> +};