Re: [PATCH v8 4/6] perf/marvell: Odyssey DDR Performance monitor support

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

On Thu, 19 Sep 2024 13:17:15 +0530
Gowthami Thiagarajan <gthiagarajan@marvell.com> 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 <gthiagarajan@marvell.com>
Follow on comments. Given I'm late to the game and none of this
is critical, you can ignore if maintainers think current code
is fine.

Jonathan

> ---
>  Documentation/admin-guide/perf/index.rst      |   1 +
>  .../admin-guide/perf/mrvl-odyssey-ddr-pmu.rst |  80 ++++++
>  drivers/perf/marvell_cn10k_ddr_pmu.c          | 257 +++++++++++++++++-
>  3 files changed, 335 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/admin-guide/perf/mrvl-odyssey-ddr-pmu.rst
> 
> diff --git a/Documentation/admin-guide/perf/index.rst b/Documentation/admin-guide/perf/index.rst
> index 7eb3dcd6f4da..d673ccfea903 100644
> --- a/Documentation/admin-guide/perf/index.rst
> +++ b/Documentation/admin-guide/perf/index.rst
> @@ -14,6 +14,7 @@ Performance monitor support
>     qcom_l2_pmu
>     qcom_l3_pmu
>     starfive_starlink_pmu
> +   mrvl-odyssey-ddr-pmu
>     arm-ccn
>     arm-cmn
>     xgene-pmu
> diff --git a/Documentation/admin-guide/perf/mrvl-odyssey-ddr-pmu.rst b/Documentation/admin-guide/perf/mrvl-odyssey-ddr-pmu.rst
> new file mode 100644
> index 000000000000..2e817593a4d9
> --- /dev/null
> +++ b/Documentation/admin-guide/perf/mrvl-odyssey-ddr-pmu.rst
...

> diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
> index 65422fd5ddd2..95818bc035e4 100644
> --- a/drivers/perf/marvell_cn10k_ddr_pmu.c
> +++ b/drivers/perf/marvell_cn10k_ddr_pmu.c

> -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->version == DDR_PMU_V2)
> +			*event_bitmap = (1ULL << (eventid - 1));
> +		else
> +			goto err;
> +		break;
>  	case EVENT_OP_IS_ENTER_SELFREF:
>  	case EVENT_OP_IS_ENTER_POWERDOWN:
>  	case EVENT_OP_IS_ENTER_MPSM:
>  		*event_bitmap = (0xFULL << (eventid - 1));
>  		break;
>  	default:
> -		pr_err("%s Invalid eventid %d\n", __func__, eventid);
> +err:		pr_err("%s Invalid eventid %d\n", __func__, eventid);

Hmm. Not pretty. I'd print that the event is not supported prior to
v2 in where you have goto err above.

>  		return -EINVAL;
>  	}
>  
c 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->ddrc_perf_cnt_op_mode_ctrl;
>  	struct hw_perf_event *hwc = &event->hw;
>  	int counter = hwc->idx;
>  
>  	local64_set(&hwc->prev_count, 0);
>  
>  	cn10k_ddr_perf_counter_enable(pmu, counter, true);
> +	if (pmu->version == DDR_PMU_V2) {
As below.  Just use a flag for whether to do this.
That flag can give it a clear name rather than basing it on 
a magic version number.

> +	/* Setup the PMU counter to work in manual mode */
> +		writeq_relaxed(OP_MODE_CTRL_VAL_MANUAL, pmu->base +
> +			       DDRC_PERF_REG(ctrl_reg, counter));
> +
> +		cn10k_ddr_perf_counter_start(pmu, counter);
> +	}
>  
>  	hwc->state = 0;
>  }
> @@ -495,7 +636,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->ddrc_perf_cfg_base, counter);
> -		ret = ddr_perf_get_event_bitmap(config, &val);
> +		ret = ddr_perf_get_event_bitmap(config, &val, pmu);
>  		if (ret)
>  			return ret;
>  
> @@ -524,6 +665,9 @@ static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
>  
>  	cn10k_ddr_perf_counter_enable(pmu, counter, false);
>  
> +	if (pmu->version == DDR_PMU_V2)
> +		cn10k_ddr_perf_counter_stop(pmu, counter);
Use a flag in pdata to decide if this needs doing, not a version check.
Versions are just not flexible enough once a significant number of
them exist and there is very little cost in avoiding them in the first place.

Also, use device names not v1 and v2.


Jonathan

> +
>  	if (flags & PERF_EF_UPDATE)
>  		cn10k_ddr_perf_event_update(event);
>  
> @@ -640,6 +784,66 @@ static void ddr_pmu_overflow_hander(struct cn10k_ddr_pmu *pmu, int evt_idx)
>  	cn10k_ddr_perf_pmu_enable(&pmu->pmu);
>  }
>