| Message ID | 20220124131118.17887-5-yangyicong@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Add support for HiSilicon PCIe Tune and Trace device | expand |
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; > +} > +
On 2022/2/7 19:49, Jonathan Cameron wrote: > 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> > Thanks for the comments. > >> --- >> 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. > yes. The valid data field is [0,15]. But all 1 is used here to indicate that it's a read process rather than a write process. >> + >> + 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. > sure. will use field ops here and below. Thanks. >> + >> + 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; >> +} >> + > > > . >
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); + + 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; + + 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; + writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_DATA); + + if (hisi_ptt_wait_tuning_finish(hisi_ptt)) + return -ETIMEDOUT; + + return 0; +} + +static ssize_t hisi_ptt_tune_attr_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); + struct dev_ext_attribute *ext_attr; + struct hisi_ptt_tune_desc *desc; + int ret; + u16 val; + + ext_attr = container_of(attr, struct dev_ext_attribute, attr); + desc = ext_attr->var; + + if (!mutex_trylock(&hisi_ptt->mutex)) + return -EBUSY; + + ret = hisi_ptt_tune_data_get(hisi_ptt, desc->event_code, &val); + + mutex_unlock(&hisi_ptt->mutex); + return ret ? ret : sysfs_emit(buf, "%u\n", val); +} + +static ssize_t hisi_ptt_tune_attr_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct hisi_ptt *hisi_ptt = to_hisi_ptt(dev_get_drvdata(dev)); + struct dev_ext_attribute *ext_attr; + struct hisi_ptt_tune_desc *desc; + int ret; + u16 val; + + ext_attr = container_of(attr, struct dev_ext_attribute, attr); + desc = ext_attr->var; + + if (kstrtou16(buf, 10, &val)) + return -EINVAL; + + if (!mutex_trylock(&hisi_ptt->mutex)) + return -EBUSY; + + ret = hisi_ptt_tune_data_set(hisi_ptt, desc->event_code, val); + + mutex_unlock(&hisi_ptt->mutex); + return ret ? ret : count; +} + +#define HISI_PTT_TUNE_ATTR(_name, _val, _show, _store) \ + static struct hisi_ptt_tune_desc _name##_desc = { \ + .name = #_name, \ + .event_code = _val, \ + }; \ + static struct dev_ext_attribute hisi_ptt_##_name##_attr = { \ + .attr = __ATTR(_name, 0600, _show, _store), \ + .var = &_name##_desc, \ + } + +#define HISI_PTT_TUNE_ATTR_COMMON(_name, _val) \ + HISI_PTT_TUNE_ATTR(_name, _val, \ + hisi_ptt_tune_attr_show, \ + hisi_ptt_tune_attr_store) + +/* + * The value of the tuning event are composed of two parts: main event code in bit[0,15] and + * subevent code in bit[16,23]. For example, qox_tx_cpl is a subevent of 'Tx path QoS control' + * which for tuning the weight of Tx completion TLPs. See hisi_ptt.rst documentation for + * more information. + */ +#define HISI_PTT_TUNE_QOS_TX_CPL (0x4 | (3 << 16)) +#define HISI_PTT_TUNE_QOS_TX_NP (0x4 | (4 << 16)) +#define HISI_PTT_TUNE_QOS_TX_P (0x4 | (5 << 16)) +#define HISI_PTT_TUNE_TX_PATH_IOB_RX_REQ_ALLOC_BUF_LEVEL (0x5 | (6 << 16)) +#define HISI_PTT_TUNE_TX_PATH_TX_REQ_ALLOC_BUF_LEVEL (0x5 | (7 << 16)) + +HISI_PTT_TUNE_ATTR_COMMON(qos_tx_cpl, + HISI_PTT_TUNE_QOS_TX_CPL); +HISI_PTT_TUNE_ATTR_COMMON(qos_tx_np, + HISI_PTT_TUNE_QOS_TX_NP); +HISI_PTT_TUNE_ATTR_COMMON(qos_tx_p, + HISI_PTT_TUNE_QOS_TX_P); +HISI_PTT_TUNE_ATTR_COMMON(tx_path_iob_rx_req_alloc_buf_level, + HISI_PTT_TUNE_TX_PATH_IOB_RX_REQ_ALLOC_BUF_LEVEL); +HISI_PTT_TUNE_ATTR_COMMON(tx_path_tx_req_alloc_buf_level, + HISI_PTT_TUNE_TX_PATH_TX_REQ_ALLOC_BUF_LEVEL); + +static struct attribute *hisi_ptt_tune_attrs[] = { + &hisi_ptt_qos_tx_cpl_attr.attr.attr, + &hisi_ptt_qos_tx_np_attr.attr.attr, + &hisi_ptt_qos_tx_p_attr.attr.attr, + &hisi_ptt_tx_path_iob_rx_req_alloc_buf_level_attr.attr.attr, + &hisi_ptt_tx_path_tx_req_alloc_buf_level_attr.attr.attr, + NULL, +}; + +static struct attribute_group hisi_ptt_tune_group = { + .attrs = hisi_ptt_tune_attrs, + .name = "tune", +}; + static u16 hisi_ptt_get_filter_val(struct pci_dev *pdev) { if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) @@ -516,6 +669,7 @@ static struct attribute_group hisi_ptt_pmu_filter_group = { static const struct attribute_group *hisi_ptt_pmu_groups[] = { &hisi_ptt_pmu_format_group, &hisi_ptt_pmu_filter_group, + &hisi_ptt_tune_group, NULL }; diff --git a/drivers/hwtracing/ptt/hisi_ptt.h b/drivers/hwtracing/ptt/hisi_ptt.h index 151e9e544653..c350f6d91c69 100644 --- a/drivers/hwtracing/ptt/hisi_ptt.h +++ b/drivers/hwtracing/ptt/hisi_ptt.h @@ -23,6 +23,11 @@ /* * The definition of the device registers and register fields. */ +#define HISI_PTT_TUNING_CTRL 0x0000 +#define HISI_PTT_TUNING_CTRL_CODE GENMASK(15, 0) +#define HISI_PTT_TUNING_CTRL_SUB GENMASK(23, 16) +#define HISI_PTT_TUNING_DATA 0x0004 +#define HISI_PTT_TUNING_DATA_VAL_MASK GENMASK(15, 0) #define HISI_PTT_TRACE_ADDR_SIZE 0x0800 #define HISI_PTT_TRACE_ADDR_BASE_LO_0 0x0810 #define HISI_PTT_TRACE_ADDR_BASE_HI_0 0x0814 @@ -38,6 +43,8 @@ #define HISI_PTT_TRACE_INT_STAT 0x0890 #define HISI_PTT_TRACE_INT_STAT_MASK GENMASK(3, 0) #define HISI_PTT_TRACE_INT_MASK 0x0894 +#define HISI_PTT_TUNING_INT_STAT 0x0898 +#define HISI_PTT_TUNING_INT_STAT_MASK BIT(0) #define HISI_PTT_TRACE_WR_STS 0x08a0 #define HISI_PTT_TRACE_WR_STS_WRITE GENMASK(27, 0) #define HISI_PTT_TRACE_WR_STS_BUFFER GENMASK(29, 28) @@ -71,6 +78,18 @@ enum hisi_ptt_trace_status { HISI_PTT_TRACE_STATUS_ON, }; +/** + * struct hisi_ptt_tune_desc - describe tune event for PTT tune + * @hisi_ptt: PTT device this tune event belongs to + * @name: name of this event + * @event_code: code of the event + */ +struct hisi_ptt_tune_desc { + struct hisi_ptt *hisi_ptt; + const char *name; + u32 event_code; +}; + /** * struct hisi_ptt_dma_buffer - describe a single trace buffer of PTT trace. * The detail of the data format is described
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> --- drivers/hwtracing/ptt/hisi_ptt.c | 154 +++++++++++++++++++++++++++++++ drivers/hwtracing/ptt/hisi_ptt.h | 19 ++++ 2 files changed, 173 insertions(+)