| Message ID | 20190906082356.25485-1-qiangqing.zhang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] perf/imx_ddr: add enhanced AXI ID filter support | expand |
Kindly Ping... Best Regards, Joakim Zhang > -----Original Message----- > From: Joakim Zhang > Sent: 2019年9月6日 16:27 > To: will@kernel.org; mark.rutland@arm.com; robin.murphy@arm.com; Frank > Li <frank.li@nxp.com> > Cc: dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org; > Joakim Zhang <qiangqing.zhang@nxp.com> > Subject: [PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support > > With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter > which only can get bursts of reading/writing DDR, i.e. DDR read/write request. > > This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW > supports AXI ID filter which can get bytes of reading/writing DDR. This feature > is more meaningful due to we always care more about bandwidth. > > Need select both above two qiurks together when HW support enhanced AXI ID > filter. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c > index ce7345745b42..5f70dbfa9607 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 */ > @@ -209,7 +210,15 @@ 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); > + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && > + (pmu->devtype_data->quirks & > DDR_CAP_AXI_ID_FILTER_ENHANCED)) { > + if ((pmu->events[counter]->attr.config == 0x41) || > + (pmu->events[counter]->attr.config == 0x42)) > + return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter > * 4); > + else > + return readl_relaxed(pmu->base + COUNTER_READ + counter * > 4); > + } else > + return readl_relaxed(pmu->base + COUNTER_READ + counter * 4); > } > > static bool ddr_perf_is_filtered(struct perf_event *event) > -- > 2.17.1
On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote: > With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter > which only can get bursts of reading/writing DDR, i.e. DDR read/write > request. > > This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW > supports AXI ID filter which can get bytes of reading/writing DDR. This > feature is more meaningful due to we always care more about bandwidth. > > Need select both above two qiurks together when HW support enhanced AXI > ID filter. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > --- > drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c > index ce7345745b42..5f70dbfa9607 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 */ Is this a deliberate change from the previous code and, if so, why? > +#define DDR_CAP_AXI_ID_FILTER_ENHANCED BIT(2) /* support enhanced AXI ID filter */ What does it mean to have DDR_CAP_AXI_ID_FILTER_ENHANCED but not DDR_CAP_AXI_ID_FILTER? Could we just say that DDR_CAP_AXI_ID_FILTER_ENHANCED implies DDR_CAP_AXI_ID_FILTER? > struct fsl_ddr_devtype_data { > unsigned int quirks; /* quirks needed for different DDR Perf core */ > @@ -209,7 +210,15 @@ 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); > + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && > + (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED)) { > + if ((pmu->events[counter]->attr.config == 0x41) || > + (pmu->events[counter]->attr.config == 0x42)) > + return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter * 4); In which case, this could be slightly simplified. Will
> -----Original Message----- > From: Will Deacon <will@kernel.org> > Sent: 2019年10月28日 22:55 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: mark.rutland@arm.com; robin.murphy@arm.com; Frank Li > <frank.li@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support > > On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote: > > With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter > > which only can get bursts of reading/writing DDR, i.e. DDR read/write > > request. > > > > This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW > > supports AXI ID filter which can get bytes of reading/writing DDR. > > This feature is more meaningful due to we always care more about > bandwidth. > > > > Need select both above two qiurks together when HW support enhanced > > AXI ID filter. > > > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > > --- > > drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c > > b/drivers/perf/fsl_imx8_ddr_perf.c > > index ce7345745b42..5f70dbfa9607 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 */ > > Is this a deliberate change from the previous code and, if so, why? [Joakim] DDR PMU integrated in various SoCs may have different features(extend more features), use BIT(1)/BIT(2)/BIT(3)... instead of 0x1/0x2/0x4..., I think it is more easier. This deliberate change from previous code has no functional effect. > > +#define DDR_CAP_AXI_ID_FILTER_ENHANCED BIT(2) /* > support enhanced AXI ID filter */ > > What does it mean to have DDR_CAP_AXI_ID_FILTER_ENHANCED but not > DDR_CAP_AXI_ID_FILTER? Could we just say that > DDR_CAP_AXI_ID_FILTER_ENHANCED implies DDR_CAP_AXI_ID_FILTER? [Joakim] Yes, DDR_CAP_AXI_ID_FILTER_ENHANCED is based on DDR_CAP_AXI_ID_FILTER and extend it. They are both for event 0x41 and event 0x42. 1)HW(i.MX8M Mini/Quad/Nano) support DDR_CAP_AXI_ID_FILTER, counters only can count DDR burst transaction(read/write requests). 2)HW(i.MX8M Plus) support both DDR_CAP_AXI_ID_FILTER and DDR_CAP_AXI_ID_FILTER_ENHANCED, means that counters can count DDR burst(read/write requests) and data(bytes) transaction at the same time due to it extend another set of data counters(registers address: 0x34,0x38,0x3c). So we can read these counters value to get the *bytes* from DDR transaction. This is more meaningful for users. > > struct fsl_ddr_devtype_data { > > unsigned int quirks; /* quirks needed for different DDR Perf core */ > > @@ -209,7 +210,15 @@ 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); > > + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && > > + (pmu->devtype_data->quirks & > DDR_CAP_AXI_ID_FILTER_ENHANCED)) { > > + if ((pmu->events[counter]->attr.config == 0x41) || > > + (pmu->events[counter]->attr.config == 0x42)) > > + return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter > * 4); > > In which case, this could be slightly simplified. [Joakim] I will use the ddr_perf_is_filtered() helper to simply the code. A v2 is sending, feel free to give comments. Thanks Will. Best Regards, Joakim Zhang > Will
Hi Joakim, Please can you try to configure your email client correctly, so that you don't have to prefix your replies with '[Joakim]'? I use mutt, which works pretty well, but you can look at Documentation/process/email-clients.rst for hints about using other clients. On Tue, Oct 29, 2019 at 07:02:28AM +0000, Joakim Zhang wrote: > > From: Will Deacon <will@kernel.org> > > On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote: > > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c > > > b/drivers/perf/fsl_imx8_ddr_perf.c > > > index ce7345745b42..5f70dbfa9607 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 */ > > > > Is this a deliberate change from the previous code and, if so, why? > > [Joakim] DDR PMU integrated in various SoCs may have different > features(extend more features), use BIT(1)/BIT(2)/BIT(3)... instead of > 0x1/0x2/0x4..., I think it is more easier. This deliberate change from > previous code has no functional effect. My point was that I don't think BIT(1) == 0x1. Will
> -----Original Message----- > From: Will Deacon <will@kernel.org> > Sent: 2019年10月30日 23:37 > To: Joakim Zhang <qiangqing.zhang@nxp.com> > Cc: mark.rutland@arm.com; robin.murphy@arm.com; Frank Li > <frank.li@nxp.com>; dl-linux-imx <linux-imx@nxp.com>; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH 1/2] perf/imx_ddr: add enhanced AXI ID filter support > > Hi Joakim, > > Please can you try to configure your email client correctly, so that you don't > have to prefix your replies with '[Joakim]'? I use mutt, which works pretty well, > but you can look at Documentation/process/email-clients.rst > for hints about using other clients. Hi Will, I use outlook, and the tag [Joakim] added manually by me for clear indication. I will reply this email without it. If you find the incorrect format in replied email, please point to me in detail, as this configuration of my client can work fine in local patch review. Thanks Will. > On Tue, Oct 29, 2019 at 07:02:28AM +0000, Joakim Zhang wrote: > > > From: Will Deacon <will@kernel.org> > > > On Fri, Sep 06, 2019 at 08:26:55AM +0000, Joakim Zhang wrote: > > > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c > > > > b/drivers/perf/fsl_imx8_ddr_perf.c > > > > index ce7345745b42..5f70dbfa9607 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 */ > > > > > > Is this a deliberate change from the previous code and, if so, why? > > > > [Joakim] DDR PMU integrated in various SoCs may have different > > features(extend more features), use BIT(1)/BIT(2)/BIT(3)... instead of > > 0x1/0x2/0x4..., I think it is more easier. This deliberate change > > from previous code has no functional effect. > > My point was that I don't think BIT(1) == 0x1. Sorry for my mistake. I will change into BIT(0) for 0x1 to align to previous code in next version. Could you firstly ignore this and then review others in V2? I will fix all of them together in V3. 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..5f70dbfa9607 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 */ @@ -209,7 +210,15 @@ 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); + if ((pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER) && + (pmu->devtype_data->quirks & DDR_CAP_AXI_ID_FILTER_ENHANCED)) { + if ((pmu->events[counter]->attr.config == 0x41) || + (pmu->events[counter]->attr.config == 0x42)) + return readl_relaxed(pmu->base + COUNTER_DPCR1 + counter * 4); + else + return readl_relaxed(pmu->base + COUNTER_READ + counter * 4); + } else + return readl_relaxed(pmu->base + COUNTER_READ + counter * 4); } static bool ddr_perf_is_filtered(struct perf_event *event)
With DDR_CAP_AXI_ID_FILTER quirk, indicating HW supports AXI ID filter which only can get bursts of reading/writing DDR, i.e. DDR read/write request. This patch add DDR_CAP_AXI_ID_ENHANCED_FILTER quirk, indicating HW supports AXI ID filter which can get bytes of reading/writing DDR. This feature is more meaningful due to we always care more about bandwidth. Need select both above two qiurks together when HW support enhanced AXI ID filter. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> --- drivers/perf/fsl_imx8_ddr_perf.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)