Re: [PATCH v9 3/5] perf/marvell: Odyssey DDR Performance monitor support

[Gowthami Thiagarajan]

From: Will Deacon <will@xxxxxxxxxx> 
Sent: Thursday, October 24, 2024 5:38 PM
To: Gowthami Thiagarajan <gthiagarajan@xxxxxxxxxxx>
Cc: mark.rutland@xxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Bharat Bhushan <bbhushan2@xxxxxxxxxxx>; George Cherian <gcherian@xxxxxxxxxxx>; Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; jonathan.cameron@xxxxxxxxxx
Subject: [EXTERNAL] Re: [PATCH v9 3/5] perf/marvell: Odyssey DDR Performance monitor support Wed, Oct 16, 2024 at 01: 31: 51PM +0530, Gowthami Thiagarajan wrote: > Odyssey DRAM Subsystem supports eight counters for monitoring performance > and software can program those counters to monitor any of the defined > performance

On Wed, Oct 16, 2024 at 01:31:51PM +0530, Gowthami Thiagarajan wrote:
> Odyssey DRAM Subsystem supports eight counters for monitoring performance
> and software can program those counters to monitor any of the defined
> performance events. Supported performance events include those counted
> at the interface between the DDR controller and the PHY, interface between
> the DDR Controller and the CHI interconnect, or within the DDR Controller.
> 
> Additionally DSS also supports two fixed performance event counters, one
> for ddr reads and the other for ddr writes.
> 
> Signed-off-by: Gowthami Thiagarajan <mailto:gthiagarajan@xxxxxxxxxxx>
> ---
>  Documentation/admin-guide/perf/index.rst      |   1 +
>  .../admin-guide/perf/mrvl-odyssey-ddr-pmu.rst |  80 ++++++
>  drivers/perf/marvell_cn10k_ddr_pmu.c          | 261 +++++++++++++++++-
>  3 files changed, 339 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/admin-guide/perf/mrvl-odyssey-ddr-pmu.rst

[...]

> @@ -297,20 +405,27 @@ static ktime_t cn10k_ddr_pmu_timer_period(void)
>  	return ms_to_ktime((u64)cn10k_ddr_pmu_poll_period_sec * USEC_PER_SEC);
>  }
>  
> -static int ddr_perf_get_event_bitmap(int eventid, u64 *event_bitmap)
> +static int ddr_perf_get_event_bitmap(int eventid, u64 *event_bitmap,
> +				     struct cn10k_ddr_pmu *ddr_pmu)
>  {
>  	switch (eventid) {
>  	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
>  	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
>  		*event_bitmap = (1ULL << (eventid - 1));
>  		break;
> +	case EVENT_DFI_PARITY_POISON ...EVENT_DFI_CMD_IS_RETRY:
> +		if (ddr_pmu->p_data->is_ody)
> +			*event_bitmap = (1ULL << (eventid - 1));
> +		else
> +			goto err;
> +		break;

You could tidy this up a little with a fallthrough:
Sure. Will make this change in the next version.

	int err = 0;

	switch (eventid) {
	case EVENT_DFI_PARITY_POISON ...EVENT_DFI_CMD_IS_RETRY:
		if (!ddr_pmu->p_data->is_ody) {
			err = -EINVAL;
			break;
		}
		fallthrough;
	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
		*event_bitmap = (1ULL << (eventid - 1));
		break;
	default:
		err = -EINVAL;
	}

	if (err) {
		pr_err("%s Invalid eventid %d\n", __func__, eventid);
		return err;
	}

>  static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
>  {
>  	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
> +	u64 ctrl_reg = pmu->p_data->cnt_op_mode_ctrl;
>  	struct hw_perf_event *hwc = &event->hw;
> +	bool is_ody = pmu->p_data->is_ody;
>  	int counter = hwc->idx;
>  
>  	local64_set(&hwc->prev_count, 0);
>  
>  	cn10k_ddr_perf_counter_enable(pmu, counter, true);
> +	if (is_ody) {
> +	/* Setup the PMU counter to work in manual mode */
> +		writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, pmu->base +

Existing typo: OP_MODE_CTRL_VAL_MANNUAL

I guess you could fix that in one of the earlier refactoring patches, if
you wanted to.
 
Yes. Will fix the typo along with refactor.

> +			       DDRC_PERF_REG(ctrl_reg, counter));
> +
> +		cn10k_ddr_perf_counter_start(pmu, counter);
> +	}

Why not put this inside cn10k_ddr_perf_counter_enable()?
Thanks. Can be made inside the counter_enable.

>  
>  	hwc->state = 0;
>  }
> @@ -486,7 +630,7 @@ static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
>  	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
>  		/* Generic counters, configure event id */
>  		reg_offset = DDRC_PERF_CFG(p_data->cfg_base, counter);
> -		ret = ddr_perf_get_event_bitmap(config, &val);
> +		ret = ddr_perf_get_event_bitmap(config, &val, pmu);
>  		if (ret)
>  			return ret;
>  
> @@ -511,10 +655,14 @@ static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
>  {
>  	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
>  	struct hw_perf_event *hwc = &event->hw;
> +	bool is_ody = pmu->p_data->is_ody;
>  	int counter = hwc->idx;
>  
>  	cn10k_ddr_perf_counter_enable(pmu, counter, false);
>  
> +	if (is_ody)
> +		cn10k_ddr_perf_counter_stop(pmu, counter);

Same here.
Will make cn10k_ddr_perf_counter_stop/start inside the cn10k_ddr_perf_counter_enable.
-Gowthami

Will