Messages in this thread | | | Date | Mon, 7 Feb 2022 11:49:11 +0000 | From | Jonathan Cameron <> | Subject | Re: [PATCH v3 4/8] hisi_ptt: Add tune function support for HiSilicon PCIe Tune and Trace device |
| |
On Mon, 24 Jan 2022 21:11:14 +0800 Yicong Yang <yangyicong@hisilicon.com> wrote:
> Add tune function for the HiSilicon Tune and Trace device. The interface > of tune is exposed through sysfs attributes of PTT PMU device. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
A few trivial things inline, but looks good in general to me. With those tidied up Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> --- > drivers/hwtracing/ptt/hisi_ptt.c | 154 +++++++++++++++++++++++++++++++ > drivers/hwtracing/ptt/hisi_ptt.h | 19 ++++ > 2 files changed, 173 insertions(+) > > diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c > index 2994354e690b..b11e702eb506 100644 > --- a/drivers/hwtracing/ptt/hisi_ptt.c > +++ b/drivers/hwtracing/ptt/hisi_ptt.c > @@ -21,6 +21,159 @@ > > #include "hisi_ptt.h" > > +static int hisi_ptt_wait_tuning_finish(struct hisi_ptt *hisi_ptt) > +{ > + u32 val; > + > + return readl_poll_timeout(hisi_ptt->iobase + HISI_PTT_TUNING_INT_STAT, > + val, !(val & HISI_PTT_TUNING_INT_STAT_MASK), > + HISI_PTT_WAIT_POLL_INTERVAL_US, > + HISI_PTT_WAIT_TIMEOUT_US); > +} > + > +static int hisi_ptt_tune_data_get(struct hisi_ptt *hisi_ptt, > + u32 event, u16 *data) > +{ > + u32 reg; > + > + reg = readl(hisi_ptt->iobase + HISI_PTT_TUNING_CTRL); > + reg &= ~(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB); > + reg |= FIELD_PREP(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB, > + event); > + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL); > + > + /* Write all 1 to indicates it's the read process */ > + writel(~0UL, hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
Just to check, this is includes the bits above the DATA_VAL_MASK? Fine if so, just seems odd to define a field but then write parts of the register that aren't part of that field.
> + > + if (hisi_ptt_wait_tuning_finish(hisi_ptt)) > + return -ETIMEDOUT; > + > + reg = readl(hisi_ptt->iobase + HISI_PTT_TUNING_DATA); > + reg &= HISI_PTT_TUNING_DATA_VAL_MASK; > + *data = (u16)reg;
As below, prefer a FIELD_GET() for this.
> + > + return 0; > +} > + > +static int hisi_ptt_tune_data_set(struct hisi_ptt *hisi_ptt, > + u32 event, u16 data) > +{ > + u32 reg; > + > + reg = readl(hisi_ptt->iobase + HISI_PTT_TUNING_CTRL); > + reg &= ~(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB); > + reg |= FIELD_PREP(HISI_PTT_TUNING_CTRL_CODE | HISI_PTT_TUNING_CTRL_SUB, > + event); > + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL); > + > + reg = data; Given you defined HISI_PTT_TUNING_DATA_VAL_MASK why not use it here
writel(FIELD_PREP(..), ...)?
> + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_DATA); > + > + if (hisi_ptt_wait_tuning_finish(hisi_ptt)) > + return -ETIMEDOUT; > + > + return 0; > +} > +
| |