| Message ID | 20191029070314.16719-3-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/4] docs/perf: Add explanation for DDR_CAP_AXI_ID_FILTER_ENHANCED quirk | expand |
On Tue, Oct 29, 2019 at 07:06:19AM +0000, Joakim Zhang wrote: > With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter > which only can get bursts from DDR transaction, i.e. DDR read/write > requests. > > This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW > supports AXI ID filter which can get bursts and bytes from DDR > transaction at the same time. We hope PMU always return bytes in the > driver due to it is more meaningful for users. > > DDR_CAP_AXI_ID_ENHANCED_FILTER is based on DDR_CAP_AXI_ID_FILTER and > extend it a bit. So need select both above two qiurks together when > HW supports enhanced AXI ID filter. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > Changelog: > V1->V2: > * use ddr_perf_is_filtered() helper to simply the code. > * improve the commit message. > --- > drivers/perf/fsl_imx8_ddr_perf.c | 55 ++++++++++++++++++++------------ > 1 file changed, 34 insertions(+), 21 deletions(-) > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c > index ce7345745b42..17c817d89222 100644 > --- a/drivers/perf/fsl_imx8_ddr_perf.c > +++ b/drivers/perf/fsl_imx8_ddr_perf.c > @@ -45,7 +45,8 @@ > static DEFINE_IDA(ddr_ida); > > /* DDR Perf hardware feature */ > -#define DDR_CAP_AXI_ID_FILTER 0x1 /* support AXI ID filter */ > +#define DDR_CAP_AXI_ID_FILTER BIT(1) /* support AXI ID filter */ > +#define DDR_CAP_AXI_ID_FILTER_ENHANCED BIT(2) /* support enhanced AXI ID filter */ I still don't understand why you don't do something like this: #define DDR_CAP_AXI_ID_FILTER 0x1 /* support AXI ID filter */ #define DDR_CAP_AXI_ID_FILTER_ENHANCED 0x3 /* support enhanced AXI ID filter */ static bool ddr_perf_is_enhanced_filtered(struct perf_event *event) { unsigned int filt; struct ddr_pmu *pmu = to_ddr_pmu(event->pmu); filt = pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED; return (filt == DDR_CAP_AXI_ID_FILTER_ENHANCED) && ddr_perf_is_filtered(event); } and then: > + /* > + * return bytes instead of bursts from ddr transaction for > + * axid-read and axid-write event if PMU core supports enhanced > + * filter. > + */ > + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && > + (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) && > + ddr_perf_is_filtered(event)) { static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter) { struct perf_event *event = pmu->events[counter]; void __iomem *base = pmu->base; base += ddr_perf_is_enhanced_filtered(event) ? COUNTER_DPCR1 : COUNTER_READ; return readl_relaxed(base + counter * 4); } In patch 4 you can then do: .quirks = DDR_CAP_AXI_ID_FILTER_ENHANCED; Make sense? Will
> -----Original Message----- > From: Will Deacon <will@kernel.org> > Sent: 2019年10月31日 23:39 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: mark.rutland@arm.com; robin.murphy@arm.com; dl-linux-imx > <linux-imx@nxp.com>; Frank Li <frank.li@nxp.com>; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH V2 3/4] perf/imx_ddr: Add enhanced AXI ID filter support > > On Tue, Oct 29, 2019 at 07:06:19AM +0000, Joakim Zhang wrote: > > With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter > > which only can get bursts from DDR transaction, i.e. DDR read/write > > requests. > > > > This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW > > supports AXI ID filter which can get bursts and bytes from DDR > > transaction at the same time. We hope PMU always return bytes in the > > driver due to it is more meaningful for users. > > > > DDR_CAP_AXI_ID_ENHANCED_FILTER is based on DDR_CAP_AXI_ID_FILTER > and > > extend it a bit. So need select both above two qiurks together when HW > > supports enhanced AXI ID filter. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > Changelog: > > V1->V2: > > * use ddr_perf_is_filtered() helper to simply the code. > > * improve the commit message. > > --- > > drivers/perf/fsl_imx8_ddr_perf.c | 55 > > ++++++++++++++++++++------------ > > 1 file changed, 34 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c > > b/drivers/perf/fsl_imx8_ddr_perf.c > > index ce7345745b42..17c817d89222 100644 > > --- a/drivers/perf/fsl_imx8_ddr_perf.c > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c > > @@ -45,7 +45,8 @@ > > static DEFINE_IDA(ddr_ida); > > > > /* DDR Perf hardware feature */ > > -#define DDR_CAP_AXI_ID_FILTER 0x1 /* support AXI ID > filter */ > > +#define DDR_CAP_AXI_ID_FILTER BIT(1) /* support AXI ID > filter */ > > +#define DDR_CAP_AXI_ID_FILTER_ENHANCED BIT(2) /* > support enhanced AXI ID filter */ > > I still don't understand why you don't do something like this: This could be better as user is aware that enhanced filter is based on previous filter. > #define DDR_CAP_AXI_ID_FILTER 0x1 /* support AXI ID filter */ > #define DDR_CAP_AXI_ID_FILTER_ENHANCED 0x3 /* support enhanced AXI > ID filter */ > > > static bool ddr_perf_is_enhanced_filtered(struct perf_event *event) { > unsigned int filt; > struct ddr_pmu *pmu = to_ddr_pmu(event->pmu); > > filt = pmu->devtype_data->quirks & > DDR_CAP_AXI_ID_FILTER_ENHANCED; > return (filt == DDR_CAP_AXI_ID_FILTER_ENHANCED) && > ddr_perf_is_filtered(event); > } > > > and then: > > > + /* > > + * return bytes instead of bursts from ddr transaction for > > + * axid-read and axid-write event if PMU core supports enhanced > > + * filter. > > + */ > > + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && > > + (pmu->devtype_data->quirks & > DDR_CAP_AXI_ID_FILTER_ENHANCED) && > > + ddr_perf_is_filtered(event)) { > > static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter) { > struct perf_event *event = pmu->events[counter]; > void __iomem *base = pmu->base; > > base += ddr_perf_is_enhanced_filtered(event) ? COUNTER_DPCR1 : > COUNTER_READ; > return readl_relaxed(base + counter * 4); } > > > In patch 4 you can then do: > > .quirks = DDR_CAP_AXI_ID_FILTER_ENHANCED; > > Make sense? Great, this makes code more modular. Will, thanks for your perfect coding! Best Regards, Joakim Zhang > Will
diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c index ce7345745b42..17c817d89222 100644 --- a/drivers/perf/fsl_imx8_ddr_perf.c +++ b/drivers/perf/fsl_imx8_ddr_perf.c @@ -45,7 +45,8 @@ static DEFINE_IDA(ddr_ida); /* DDR Perf hardware feature */ -#define DDR_CAP_AXI_ID_FILTER 0x1 /* support AXI ID filter */ +#define DDR_CAP_AXI_ID_FILTER BIT(1) /* support AXI ID filter */ +#define DDR_CAP_AXI_ID_FILTER_ENHANCED BIT(2) /* support enhanced AXI ID filter */ struct fsl_ddr_devtype_data { unsigned int quirks; /* quirks needed for different DDR Perf core */ @@ -178,6 +179,26 @@ static const struct attribute_group *attr_groups[] = { NULL, }; +static bool ddr_perf_is_filtered(struct perf_event *event) +{ + return event->attr.config == 0x41 || event->attr.config == 0x42; +} + +static u32 ddr_perf_filter_val(struct perf_event *event) +{ + return event->attr.config1; +} + +static bool ddr_perf_filters_compatible(struct perf_event *a, + struct perf_event *b) +{ + if (!ddr_perf_is_filtered(a)) + return true; + if (!ddr_perf_is_filtered(b)) + return true; + return ddr_perf_filter_val(a) == ddr_perf_filter_val(b); +} + static u32 ddr_perf_alloc_counter(struct ddr_pmu *pmu, int event) { int i; @@ -209,27 +230,19 @@ static void ddr_perf_free_counter(struct ddr_pmu *pmu, int counter) static u32 ddr_perf_read_counter(struct ddr_pmu *pmu, int counter) { - return readl_relaxed(pmu->base + COUNTER_READ + counter * 4); -} + struct perf_event *event = pmu->events[counter]; -static bool ddr_perf_is_filtered(struct perf_event *event) -{ - return event->attr.config == 0x41 || event->attr.config == 0x42; -} - -static u32 ddr_perf_filter_val(struct perf_event *event) -{ - return event->attr.config1; -} - -static bool ddr_perf_filters_compatible(struct perf_event *a, - struct perf_event *b) -{ - if (!ddr_perf_is_filtered(a)) - return true; - if (!ddr_perf_is_filtered(b)) - return true; - return ddr_perf_filter_val(a) == ddr_perf_filter_val(b); + /* + * return bytes instead of bursts from ddr transaction for + * axid-read and axid-write event if PMU core supports enhanced + * filter. + */ + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && + (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED) && + ddr_perf_is_filtered(event)) { + return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter * 4); + } else + return readl_relaxed(pmu->base + COUNTER_READ + counter * 4); } static int ddr_perf_event_init(struct perf_event *event)
With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter which only can get bursts from DDR transaction, i.e. DDR read/write requests. This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW supports AXI ID filter which can get bursts and bytes from DDR transaction at the same time. We hope PMU always return bytes in the driver due to it is more meaningful for users. DDR_CAP_AXI_ID_ENHANCED_FILTER is based on DDR_CAP_AXI_ID_FILTER and extend it a bit. So need select both above two qiurks together when HW supports enhanced AXI ID filter. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- Changelog: V1->V2: * use ddr_perf_is_filtered() helper to simply the code. * improve the commit message. --- drivers/perf/fsl_imx8_ddr_perf.c | 55 ++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 21 deletions(-)