Messages in this thread | | | From | Gowthami Thiagarajan <> | Subject | RE: [EXT] Re: [PATCH v3 1/2] perf/marvell: Odyssey DDR Performance monitor support | Date | Tue, 27 Feb 2024 13:46:51 +0000 |
| |
Hi Jonathan,
Please find the response inline.
> -----Original Message----- > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Sent: Monday, January 29, 2024 5:35 PM > To: Gowthami Thiagarajan <gthiagarajan@marvell.com> > Cc: will@kernel.org; mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>; Bharat Bhushan > <bbhushan2@marvell.com>; George Cherian <gcherian@marvell.com> > Subject: [EXT] Re: [PATCH v3 1/2] perf/marvell: Odyssey DDR Performance monitor support > > External Email > > ---------------------------------------------------------------------- > On Mon, 22 Jan 2024 18:19:32 +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> > Hi Gowthami, > > A quick drive by review. > Questions like whether this patch should be split are down to the perf maintainers. > I would ask for it, but not my area of the kernel ;) > > Jonathan > > > > +#define VERSION_V1 1 > > +#define VERSION_V2 2 > > Prefix these defines + perhaps make them an enum? [Gowthami] Ack. > > > + > > struct cn10k_ddr_pmu { > > struct pmu pmu; > > void __iomem *base; > > + struct ddr_pmu_platform_data *p_data; > > const both because it should be and to avoid casting away the const. [Gowthami] Ack. > > > > + int version; > > unsigned int cpu; > > struct device *dev; > > int active_events; > > @@ -134,6 +160,54 @@ 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; > > + u64 ddrc_perf_cfg_base; > > + u64 ddrc_perf_cnt_op_mode_ctrl; > > Good to name these in a fashion that makes it clear what they are. > Some are values, some are register address offsets I think? > > Shy is ddrc_perf prefix useful in here? [Gowthami] Yes. Some are values and most of them point to register offsets. Just to match with the register defined values named it so. > > > > + 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; > > +}; > > + > > +static const struct ddr_pmu_platform_data cn10k_ddr_pmu_pdata = { > > + .counter_overflow_val = BIT_ULL(48), > > + .counter_max_val = GENMASK_ULL(48, 0), > > + .ddrc_perf_cnt_base = CN10K_DDRC_PERF_CNT_VALUE_BASE, > > + .ddrc_perf_cfg_base = CN10K_DDRC_PERF_CFG_BASE, > > + .ddrc_perf_cnt_op_mode_ctrl = CN10K_DDRC_PERF_CNT_OP_MODE_CTRL, > > + .ddrc_perf_cnt_start_op_ctrl = CN10K_DDRC_PERF_CNT_START_OP_CTRL, > > + .ddrc_perf_cnt_end_op_ctrl = CN10K_DDRC_PERF_CNT_END_OP_CTRL, > > + .ddrc_perf_cnt_end_status = CN10K_DDRC_PERF_CNT_END_STATUS, > > + .ddrc_perf_cnt_freerun_en = CN10K_DDRC_PERF_CNT_FREERUN_EN, > > + .ddrc_perf_cnt_freerun_ctrl = CN10K_DDRC_PERF_CNT_FREERUN_CTRL, > > + .ddrc_perf_cnt_freerun_clr = 0, > > + .ddrc_perf_cnt_value_wr_op = CN10K_DDRC_PERF_CNT_VALUE_WR_OP, > > + .ddrc_perf_cnt_value_rd_op = CN10K_DDRC_PERF_CNT_VALUE_RD_OP, > > +}; > > + > > +static const struct ddr_pmu_platform_data odyssey_ddr_pmu_pdata = { > > + .counter_overflow_val = 0, > > + .counter_max_val = GENMASK_ULL(63, 0), > > + .ddrc_perf_cnt_base = ODY_DDRC_PERF_CNT_VALUE_BASE, > > + .ddrc_perf_cfg_base = ODY_DDRC_PERF_CFG_BASE, > > + .ddrc_perf_cnt_op_mode_ctrl = ODY_DDRC_PERF_CNT_OP_MODE_CTRL, > > + .ddrc_perf_cnt_start_op_ctrl = ODY_DDRC_PERF_CNT_START_OP_CTRL, > > + .ddrc_perf_cnt_end_op_ctrl = ODY_DDRC_PERF_CNT_END_OP_CTRL, > > + .ddrc_perf_cnt_end_status = ODY_DDRC_PERF_CNT_END_STATUS, > > + .ddrc_perf_cnt_freerun_en = 0, > > + .ddrc_perf_cnt_freerun_ctrl = ODY_DDRC_PERF_CNT_FREERUN_CTRL, > > + .ddrc_perf_cnt_freerun_clr = ODY_DDRC_PERF_CNT_FREERUN_CLR, > > + .ddrc_perf_cnt_value_wr_op = ODY_DDRC_PERF_CNT_VALUE_WR_OP, > > + .ddrc_perf_cnt_value_rd_op = ODY_DDRC_PERF_CNT_VALUE_RD_OP, > > +}; > > > ... > > > -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) > > { > > + int ret = 0; > > + > > 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 == VERSION_V2) { > > + *event_bitmap = (1ULL << (eventid - 1)); > > + } else { > > + pr_err("%s Invalid eventid %d\n", __func__, eventid); > > + ret = -EINVAL; > > + } > > + break; > > case EVENT_OP_IS_ENTER_SELFREF: > > case EVENT_OP_IS_ENTER_POWERDOWN: > > case EVENT_OP_IS_ENTER_MPSM: > > @@ -280,10 +451,10 @@ static int ddr_perf_get_event_bitmap(int eventid, u64 *event_bitmap) > > break; > > default: > > pr_err("%s Invalid eventid %d\n", __func__, eventid); > > - return -EINVAL; > > + ret = -EINVAL; > > } > > > > - return 0; > > + return ret; > Why? Just return in the various paths above. > > Direct returns make for easier to review code as you can follow a particular path through > more quickly. [Gowthami] Ack. > > > } > > > static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu) > > { > > struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu); > > + struct ddr_pmu_platform_data *p_data = ddr_pmu->p_data; > > > > writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base + > > - DDRC_PERF_CNT_END_OP_CTRL); > > + p_data->ddrc_perf_cnt_end_op_ctrl); > > } > > > > static void cn10k_ddr_perf_event_update_all(struct cn10k_ddr_pmu *pmu) > > @@ -549,6 +778,7 @@ static void cn10k_ddr_perf_event_update_all(struct cn10k_ddr_pmu *pmu) > > > > static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu) > > { > > + struct ddr_pmu_platform_data *p_data = pmu->p_data; > > struct perf_event *event; > > struct hw_perf_event *hwc; > > u64 prev_count, new_count; > > @@ -561,7 +791,8 @@ static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu) > > prev_count = local64_read(&hwc->prev_count); > > new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx); > > > > - /* Overflow condition is when new count less than > > + /* > > + * Overflow condition is when new count less than > > * previous count > > */ > > if (new_count < prev_count) > > @@ -574,7 +805,8 @@ static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu) > > prev_count = local64_read(&hwc->prev_count); > > new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx); > > > > - /* Overflow condition is when new count less than > > + /* > > + * Overflow condition is when new count less than > > Good to fix this, but not in a patch doing anything meaningful. If you want > to make comment syntax changes - separate patch. [Gowthami] Ack. > > > * previous count > > */ > > if (new_count < prev_count) > > @@ -586,11 +818,23 @@ static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu) > > continue; > > > > value = cn10k_ddr_perf_read_counter(pmu, i); > > - if (value == DDRC_PERF_CNT_MAX_VALUE) { > > + if (value == p_data->counter_max_val) { > > pr_info("Counter-(%d) reached max value\n", i); > > - cn10k_ddr_perf_event_update_all(pmu); > > - cn10k_ddr_perf_pmu_disable(&pmu->pmu); > > - cn10k_ddr_perf_pmu_enable(&pmu->pmu); > > + /* > > + * As separate control register is added for each > > + * counter in odyssey, no need to update all > > + * the events > > + */ > > + if (pmu->version == VERSION_V2) { > > This sort of version difference is often better handled via a callback > in the your pdata structure. Makes it easy to add a new one for v3 :) > [Gowthami] Agree. Will make the change in the next version. > > + cn10k_ddr_perf_event_update(pmu->events[i]); > > + cn10k_ddr_perf_counter_stop(pmu, i); > > + cn10k_ddr_perf_counter_start(pmu, i); > > + > > + } else { > > + cn10k_ddr_perf_event_update_all(pmu); > > + cn10k_ddr_perf_pmu_disable(&pmu->pmu); > > + cn10k_ddr_perf_pmu_enable(&pmu->pmu); > > + } > > } > > } > > > > @@ -631,7 +875,10 @@ static int cn10k_ddr_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node) > > > > static int cn10k_ddr_perf_probe(struct platform_device *pdev) > > { > > + struct ddr_pmu_platform_data *pltfm_data; > > + struct device *dev = &pdev->dev; > > struct cn10k_ddr_pmu *ddr_pmu; > > + const char *compatible; > > struct resource *res; > > void __iomem *base; > > char *name; > > @@ -642,6 +889,14 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev) > > return -ENOMEM; > > > > ddr_pmu->dev = &pdev->dev; > > + > > + pltfm_data = (struct ddr_pmu_platform_data *) > > + device_get_match_data(&pdev->dev); > > Shouldn't need the cast as it's a const void * > and you should not need to modify it in here (so make your > data types > const struct ddr_pmu_platform * > [Gowthami] Ack > > > > + if (!pltfm_data) { > > + dev_err(&pdev->dev, "Error: No device match data found\n"); > > + return -ENODEV; > > + } > > + ddr_pmu->p_data = pltfm_data; > > platform_set_drvdata(pdev, ddr_pmu); > > > > base = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > > @@ -650,25 +905,59 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev) > > > > ddr_pmu->base = base; > > > > - /* Setup the PMU counter to work in manual mode */ > > - writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base + > > - DDRC_PERF_CNT_OP_MODE_CTRL); > > - > > - ddr_pmu->pmu = (struct pmu) { > > - .module = THIS_MODULE, > > - .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > > - .task_ctx_nr = perf_invalid_context, > > - .attr_groups = cn10k_attr_groups, > > - .event_init = cn10k_ddr_perf_event_init, > > - .add = cn10k_ddr_perf_event_add, > > - .del = cn10k_ddr_perf_event_del, > > - .start = cn10k_ddr_perf_event_start, > > - .stop = cn10k_ddr_perf_event_stop, > > - .read = cn10k_ddr_perf_event_update, > > - .pmu_enable = cn10k_ddr_perf_pmu_enable, > > - .pmu_disable = cn10k_ddr_perf_pmu_disable, > > - }; > > + ret = device_property_read_string(dev, "compatible", &compatible); > > + if (ret) { > > + pr_err("compatible property not found\n"); > > + return ret; > > + } > > > > + if ((strncmp("marvell,cn10k-ddr-pmu", compatible, > > + strlen(compatible)) == 0)) > > Why not just embed this in your pdata structure? > Even better would be add data to reflect the actual differences > rather than relying on a 'version' number. It tends to be more > extensible as new implementations surface to encode each difference > as data in such a structure. Otherwise, in the long run you > end up with big switch statements for the many different versions > which just provide some per version constants. That's messy. [Gowthami] Ack. > > > > + ddr_pmu->version = VERSION_V1; > > + else > > + ddr_pmu->version = VERSION_V2; > > + > > + if (ddr_pmu->version == VERSION_V1) { > > + ddr_pmu->pmu = (struct pmu) { > > + .module = THIS_MODULE, > > + .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > > + .task_ctx_nr = perf_invalid_context, > > + .attr_groups = cn10k_attr_groups, > > + .event_init = cn10k_ddr_perf_event_init, > > + .add = cn10k_ddr_perf_event_add, > > + .del = cn10k_ddr_perf_event_del, > > + .start = cn10k_ddr_perf_event_start, > > + .stop = cn10k_ddr_perf_event_stop, > > + .read = cn10k_ddr_perf_event_update, > > + .pmu_enable = cn10k_ddr_perf_pmu_enable, > > + .pmu_disable = cn10k_ddr_perf_pmu_disable, > > + }; > > + > > + /* > > + * As we have separate control registers for each counter in Odyssey, > > + * setting up the mode will be done when we enable each counter > > + * > > Trivial: Odd formatting. I'd drop the blank commented line and add a full stop. [Gowthami] Ack > > > + */ > > + > > + /* Setup the PMU counter to work in manual mode */ > > + writeq(OP_MODE_CTRL_VAL_MANUAL, ddr_pmu->base + > > + (ddr_pmu->p_data->ddrc_perf_cnt_op_mode_ctrl)); > > + } else { > > + ddr_pmu->pmu = (struct pmu) { > > + .module = THIS_MODULE, > > + .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > > + .task_ctx_nr = perf_invalid_context, > > + .attr_groups = odyssey_attr_groups, > > + .event_init = cn10k_ddr_perf_event_init, > > + .add = cn10k_ddr_perf_event_add, > > + .del = cn10k_ddr_perf_event_del, > > + .start = cn10k_ddr_perf_event_start, > > + .stop = cn10k_ddr_perf_event_stop, > > + .read = cn10k_ddr_perf_event_update, > > + .pmu_enable = NULL, > > + .pmu_disable = NULL, > > No need to set these to NULL. Not providing them has same result and > I don't think there is any particular value wrt to 'documentation' of > setting them explicitly. If there is a reason this needs calling out > I'd expect a comment explaining why. [Gowthami] There is no specific reason. Will remove it in the next version. > > Ideal patch series structure for changes like this patch makes would be: > 1) Refactor to pull out the pdata - no functional change. > 2) Patch adding the support for the new device. > > Result is easier to review than the combination of the two changes. [Gowthami] Agree. Will split the patch as suggested.
Thanks, Gowthami > > > > + }; > > + }
| |