RE: [PATCH V1 RESEND 1/3] perf/imx_ddr: Add system PMU identifier for userspace

[Joakim Zhang <qiangqing.zhang@nxp.com>]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2020年5月20日 2:51
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: john.garry@huawei.com; will@kernel.org; mark.rutland@arm.com;
> shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V1 RESEND 1/3] perf/imx_ddr: Add system PMU identifier
> for userspace
> 
> On Tue, May 12, 2020 at 03:31:13PM +0800, Joakim Zhang wrote:
> > The DDR Perf for i.MX8 is a system PMU whose axi id would different
> > from SoC to SoC. Need expose system PMU identifier for userspace which
> > refer to /sys/bus/event_source/devices/<PMU DEVICE>/identifier.
> 
> Why not just expose the AXI ID if that's what's different?

Hi Rob,

Each master has their own AXI ID, such as USB, GPU, VPU etc, it is various from different SoCs. We want to add system PMU support in perf tool, so we want to expose something from perf driver to identify each SoC.
When we know which SoC it is, we can get each master AXI ID. If this patch can be accepted, /sys/bus/event_source/devices/<PMU DEVICE>/identifier could be a common interface for all system PMUs.

I will change to add a property to identify SoC, to see if it is better. Thanks.

Best Regards,
Joakim Zhang
> >
> > Reviewed-by: John Garry <john.garry@huawei.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/perf/fsl_imx8_ddr_perf.c | 45
> > +++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c
> > b/drivers/perf/fsl_imx8_ddr_perf.c
> > index 95dca2cb5265..88addbffbbd0 100644
> > --- a/drivers/perf/fsl_imx8_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > @@ -50,21 +50,38 @@ static DEFINE_IDA(ddr_ida);
> >
> >  struct fsl_ddr_devtype_data {
> >  	unsigned int quirks;    /* quirks needed for different DDR Perf core */
> > +	const char *identifier;	/* system PMU identifier for userspace */
> >  };
> >
> > -static const struct fsl_ddr_devtype_data imx8_devtype_data;
> > +static const struct fsl_ddr_devtype_data imx8_devtype_data = {
> > +	.identifier = "i.MX8",
> > +};
> > +
> > +static const struct fsl_ddr_devtype_data imx8mq_devtype_data = {
> > +	.quirks = DDR_CAP_AXI_ID_FILTER,
> > +	.identifier = "i.MX8MQ",
> > +};
> > +
> > +static const struct fsl_ddr_devtype_data imx8mm_devtype_data = {
> > +	.quirks = DDR_CAP_AXI_ID_FILTER,
> > +	.identifier = "i.MX8MM",
> > +};
> >
> > -static const struct fsl_ddr_devtype_data imx8m_devtype_data = {
> > +static const struct fsl_ddr_devtype_data imx8mn_devtype_data = {
> >  	.quirks = DDR_CAP_AXI_ID_FILTER,
> > +	.identifier = "i.MX8MN",
> >  };
> >
> >  static const struct fsl_ddr_devtype_data imx8mp_devtype_data = {
> >  	.quirks = DDR_CAP_AXI_ID_FILTER_ENHANCED,
> > +	.identifier = "i.MX8MP",
> >  };
> >
> >  static const struct of_device_id imx_ddr_pmu_dt_ids[] = {
> >  	{ .compatible = "fsl,imx8-ddr-pmu", .data = &imx8_devtype_data},
> > -	{ .compatible = "fsl,imx8m-ddr-pmu", .data = &imx8m_devtype_data},
> 
> You need to keep the old one for compatibility.
> 
> > +	{ .compatible = "fsl,imx8mq-ddr-pmu", .data = &imx8mq_devtype_data},
> > +	{ .compatible = "fsl,imx8mm-ddr-pmu", .data = &imx8mm_devtype_data},
> > +	{ .compatible = "fsl,imx8mn-ddr-pmu", .data = &imx8mn_devtype_data},
> >  	{ .compatible = "fsl,imx8mp-ddr-pmu", .data = &imx8mp_devtype_data},
> >  	{ /* sentinel */ }
> >  };
> > @@ -84,6 +101,27 @@ struct ddr_pmu {
> >  	int id;
> >  };
> >
> > +static ssize_t ddr_perf_identifier_show(struct device *dev,
> > +					struct device_attribute *attr,
> > +					char *page)
> > +{
> > +	struct ddr_pmu *pmu = dev_get_drvdata(dev);
> > +
> > +	return sprintf(page, "%s\n", pmu->devtype_data->identifier);
> 
> Why do we need yet another way to identify the SoC from userspace?
> 
> > +}
> > +
> > +static struct device_attribute ddr_perf_identifier_attr =
> > +	__ATTR(identifier, 0444, ddr_perf_identifier_show, NULL);
> 
> sysfs attributes are supposed to be documented.
> 
> > +
> > +static struct attribute *ddr_perf_identifier_attrs[] = {
> > +	&ddr_perf_identifier_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group ddr_perf_identifier_attr_group = {
> > +	.attrs = ddr_perf_identifier_attrs,
> > +};
> > +
> >  enum ddr_perf_filter_capabilities {
> >  	PERF_CAP_AXI_ID_FILTER = 0,
> >  	PERF_CAP_AXI_ID_FILTER_ENHANCED,
> > @@ -237,6 +275,7 @@ static const struct attribute_group *attr_groups[] = {
> >  	&ddr_perf_format_attr_group,
> >  	&ddr_perf_cpumask_attr_group,
> >  	&ddr_perf_filter_cap_attr_group,
> > +	&ddr_perf_identifier_attr_group,
> >  	NULL,
> >  };
> >
> > --
> > 2.17.1
> >