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

Jonathan Cameron
2024-09-24T16:28:57Z

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