Re: [PATCH v6 3/5] i2c: add support for HiSilicon I2C controller
On 2021/4/8 7:04, Wolfram Sang wrote: > >> Reason for temp variable is for me it's confusing to see statement like >> "rate_khz = rate_khz / 1000". > > Yes. And with this clearer calculation, we can maybe skip the HZ_PER_KHZ > define completely and just use plain '1000' as a factor/divider because > it then becomes obvious. I still find the define more confusing than > helpful TBH. But I'll leave the final decision to Yicong Yang. > HZ_PER_KHZ macro are defined separately in other places of the kernel. Andy suggested to have this defined and used so that one day we can factor this macro out to the public. :)
[PATCH v7 1/5] i2c: core: add managed function for adding i2c adapters
Some I2C controller drivers will only unregister the I2C
adapter in their .remove() callback, which can be done
by simply using a managed variant to add the I2C adapter.
So add the managed functions for adding the I2C adapter.
Reviewed-by: Andy Shevchenko
Reviewed-by: Dmitry Osipenko
Signed-off-by: Yicong Yang
---
drivers/i2c/i2c-core-base.c | 26 ++
include/linux/i2c.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf72..de9402c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1703,6 +1703,32 @@ void i2c_del_adapter(struct i2c_adapter *adap)
}
EXPORT_SYMBOL(i2c_del_adapter);
+static void devm_i2c_del_adapter(void *adapter)
+{
+ i2c_del_adapter(adapter);
+}
+
+/**
+ * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter()
+ * @dev: managing device for adding this I2C adapter
+ * @adapter: the adapter to add
+ * Context: can sleep
+ *
+ * Add adapter with dynamic bus number, same with i2c_add_adapter()
+ * but the adapter will be auto deleted on driver detach.
+ */
+int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
+{
+ int ret;
+
+ ret = i2c_add_adapter(adapter);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, devm_i2c_del_adapter, adapter);
+}
+EXPORT_SYMBOL_GPL(devm_i2c_add_adapter);
+
static void i2c_parse_timing(struct device *dev, char *prop_name, u32
*cur_val_p,
u32 def_val, bool use_def)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5662265..10bd0b0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -844,6 +844,7 @@ static inline void i2c_mark_adapter_resumed(struct
i2c_adapter *adap)
*/
#if IS_ENABLED(CONFIG_I2C)
int i2c_add_adapter(struct i2c_adapter *adap);
+int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter);
void i2c_del_adapter(struct i2c_adapter *adap);
int i2c_add_numbered_adapter(struct i2c_adapter *adap);
--
2.8.1
[PATCH v7 2/5] i2c: core: add api to provide frequency mode strings
Some I2C drivers like Designware and HiSilicon will print the
bus frequency mode information, so add a public one that everyone
can make use of.
Tested-by: Jarkko Nikula
Reviewed-by: Jarkko Nikula
Reviewed-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
drivers/i2c/i2c-core-base.c | 21 +
include/linux/i2c.h | 3 +++
2 files changed, 24 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index de9402c..775b8cc 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -76,6 +76,27 @@ void i2c_transfer_trace_unreg(void)
static_branch_dec(&i2c_trace_msg_key);
}
+const char *i2c_freq_mode_string(u32 bus_freq_hz)
+{
+ switch (bus_freq_hz) {
+ case I2C_MAX_STANDARD_MODE_FREQ:
+ return "Standard Mode (100 kHz)";
+ case I2C_MAX_FAST_MODE_FREQ:
+ return "Fast Mode (400 kHz)";
+ case I2C_MAX_FAST_MODE_PLUS_FREQ:
+ return "Fast Mode Plus (1.0 MHz)";
+ case I2C_MAX_TURBO_MODE_FREQ:
+ return "Turbo Mode (1.4 MHz)";
+ case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+ return "High Speed Mode (3.4 MHz)";
+ case I2C_MAX_ULTRA_FAST_MODE_FREQ:
+ return "Ultra Fast Mode (5.0 MHz)";
+ default:
+ return "Unknown Mode";
+ }
+}
+EXPORT_SYMBOL_GPL(i2c_freq_mode_string);
+
const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 10bd0b0..0813be1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -51,6 +51,9 @@ struct module;
struct property_entry;
#if IS_ENABLED(CONFIG_I2C)
+/* Return the Frequency mode string based on the bus frequency */
+const char *i2c_freq_mode_string(u32 bus_freq_hz);
+
/*
* The master routines are the ones normally used to transmit data to devices
* on a bus (or read from them). Apart from two basic transfer functions to
--
2.8.1
[PATCH v7 0/5] Add support for HiSilicon I2C controller
Add driver and MAINTAINERS for HiSilicon I2C controller on Kunpeng SoC. Also provide the devm_*() variants for adding the I2C adapters. Add a public api to provide I2C frequency mode strings and convert designware driver to use it. Change since v6: - make the i2c_freq_mode_string() exported rather than inline - addressed the comments from Wolfram and Jarkko for the driver Change since v5: - address the comment from Dmitry and add his Reviewed-by - address the comment from Jarkko and add his Reviewed-by and Tested-by - add Jarkko's Acked-by for designware patch Link: https://lore.kernel.org/linux-i2c/1617113966-40498-1-git-send-email-yangyic...@hisilicon.com/ Change since v4: - and Andy's Reviewed-by - attach Andy's patch of switch designware driver to use i2c_freq_mode_string() Link: https://lore.kernel.org/linux-i2c/1617109549-4013-1-git-send-email-yangyic...@hisilicon.com/ Link: https://lore.kernel.org/linux-i2c/20210330134633.29889-1-andriy.shevche...@linux.intel.com/ Change since v3: - split the bus mode string api to I2C as suggested by Andy - simplify the devm variants and change the export format - address the comments of the HiSilicon I2C driver from Andy and Dmitry, thanks! Link: https://lore.kernel.org/linux-i2c/1616411413-7177-1-git-send-email-yangyic...@hisilicon.com/ Change since v2: - handle -EPROBE_DEFER case when get irq number by platform_get_irq() Link: https://lore.kernel.org/linux-i2c/1615296137-14558-1-git-send-email-yangyic...@hisilicon.com/ Change since v1: - fix compile test error on 32bit arch, reported by intel lkp robot: 64 bit division without using kernel wrapper in probe function. Link:https://lore.kernel.org/linux-i2c/1615016946-55670-1-git-send-email-yangyic...@hisilicon.com/ Andy Shevchenko (1): i2c: designware: Switch over to i2c_freq_mode_string() Yicong Yang (4): i2c: core: add managed function for adding i2c adapters i2c: core: add api to provide frequency mode strings i2c: add support for HiSilicon I2C controller MAINTAINERS: Add maintainer for HiSilicon I2C driver MAINTAINERS| 7 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile| 1 + drivers/i2c/busses/i2c-designware-master.c | 20 +- drivers/i2c/busses/i2c-hisi.c | 504 + drivers/i2c/i2c-core-base.c| 47 +++ include/linux/i2c.h| 4 + 7 files changed, 577 insertions(+), 16 deletions(-) create mode 100644 drivers/i2c/busses/i2c-hisi.c -- 2.8.1
[PATCH v7 3/5] i2c: add support for HiSilicon I2C controller
Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides the access to the i2c busses, which connects to the eeprom, rtc, etc. The driver works with IRQ mode, and supports basic I2C features and 10bit address. The DMA is not supported. Reviewed-by: Andy Shevchenko Reviewed-by: Dmitry Osipenko Signed-off-by: Yicong Yang --- drivers/i2c/busses/Kconfig| 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-hisi.c | 504 ++ 3 files changed, 515 insertions(+) create mode 100644 drivers/i2c/busses/i2c-hisi.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 05ebf75..eddf7bf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -645,6 +645,16 @@ config I2C_HIGHLANDER This driver can also be built as a module. If so, the module will be called i2c-highlander. +config I2C_HISI + tristate "HiSilicon I2C controller" + depends on ARM64 || COMPILE_TEST + help + Say Y here if you want to have Hisilicon I2C controller support + available on the Kunpeng Server. + + This driver can also be built as a module. If so, the module + will be called i2c-hisi. + config I2C_IBM_IIC tristate "IBM PPC 4xx on-chip I2C interface" depends on 4xx diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 615f35e..e1c9292 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o +obj-$(CONFIG_I2C_HISI) += i2c-hisi.o obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c new file mode 100644 index 000..acf3948 --- /dev/null +++ b/drivers/i2c/busses/i2c-hisi.c @@ -0,0 +1,504 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * HiSilicon I2C Controller Driver for Kunpeng SoC + * + * Copyright (c) 2021 HiSilicon Technologies Co., Ltd. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HISI_I2C_FRAME_CTRL0x +#define HISI_I2C_FRAME_CTRL_SPEED_MODE GENMASK(1, 0) +#define HISI_I2C_FRAME_CTRL_ADDR_TEN BIT(2) +#define HISI_I2C_SLV_ADDR 0x0004 +#define HISI_I2C_SLV_ADDR_VALGENMASK(9, 0) +#define HISI_I2C_SLV_ADDR_GC_S_MODE BIT(10) +#define HISI_I2C_SLV_ADDR_GC_S_ENBIT(11) +#define HISI_I2C_CMD_TXDATA0x0008 +#define HISI_I2C_CMD_TXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_CMD_TXDATA_RW BIT(8) +#define HISI_I2C_CMD_TXDATA_P_EN BIT(9) +#define HISI_I2C_CMD_TXDATA_SR_ENBIT(10) +#define HISI_I2C_RXDATA0x000c +#define HISI_I2C_RXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_SS_SCL_HCNT 0x0010 +#define HISI_I2C_SS_SCL_LCNT 0x0014 +#define HISI_I2C_FS_SCL_HCNT 0x0018 +#define HISI_I2C_FS_SCL_LCNT 0x001c +#define HISI_I2C_HS_SCL_HCNT 0x0020 +#define HISI_I2C_HS_SCL_LCNT 0x0024 +#define HISI_I2C_FIFO_CTRL 0x0028 +#define HISI_I2C_FIFO_RX_CLR BIT(0) +#define HISI_I2C_FIFO_TX_CLR BIT(1) +#define HISI_I2C_FIFO_RX_AF_THRESH GENMASK(7, 2) +#define HISI_I2C_FIFO_TX_AE_THRESH GENMASK(13, 8) +#define HISI_I2C_FIFO_STATE0x002c +#define HISI_I2C_FIFO_STATE_RX_RERR BIT(0) +#define HISI_I2C_FIFO_STATE_RX_WERR BIT(1) +#define HISI_I2C_FIFO_STATE_RX_EMPTY BIT(3) +#define HISI_I2C_FIFO_STATE_TX_RERR BIT(6) +#define HISI_I2C_FIFO_STATE_TX_WERR BIT(7) +#define HISI_I2C_FIFO_STATE_TX_FULL BIT(11) +#define HISI_I2C_SDA_HOLD 0x0030 +#define HISI_I2C_SDA_HOLD_TX GENMASK(15, 0) +#define HISI_I2C_SDA_HOLD_RX GENMASK(23, 16) +#define HISI_I2C_FS_SPK_LEN0x0038 +#define HISI_I2C_FS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_HS_SPK_LEN0x003c +#define HISI_I2C_HS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_INT_MSTAT 0x0044 +#define HISI_I2C_INT_CLR 0x0048 +#define HISI_I2C_INT_MASK 0x004C +#define HISI_I2C_TRANS_STATE 0x0050 +#define HISI_I2C_TRANS_ERR 0x0054 +#define HISI_I2C_VERSION 0x0058 + +#define HISI_I2C_INT_ALL GENMASK(4, 0) +#define HISI_I2C_INT_TRANS_CPLTBIT(0) +#define HISI_I2C_INT_TRANS_ERR BIT(1) +#define HISI_I2C_INT_FIFO_ERR BIT(2) +#define HISI_I2C_INT_RX_FULL BIT(3) +#define HISI_I2C_INT_TX_EMPTY BIT(4) +#define HISI_I2C_INT_ERR \ + (HISI_I2C_INT_TRANS_ERR | HISI_I2C_INT_FIFO_ERR) + +#define HISI_I2C_STD_SPEED_M
[PATCH v7 5/5] i2c: designware: Switch over to i2c_freq_mode_string()
From: Andy Shevchenko
Use generic i2c_freq_mode_string() helper to print chosen bus speed.
Acked-by: Jarkko Nikula
Signed-off-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
drivers/i2c/busses/i2c-designware-master.c | 20
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c
b/drivers/i2c/busses/i2c-designware-master.c
index dd27b9d..b64c4c8 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -35,10 +35,10 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev
*dev)
static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
{
- const char *mode_str, *fp_str = "";
u32 comp_param1;
u32 sda_falling_time, scl_falling_time;
struct i2c_timings *t = &dev->timings;
+ const char *fp_str = "";
u32 ic_clk;
int ret;
@@ -153,22 +153,10 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev
*dev)
ret = i2c_dw_set_sda_hold(dev);
if (ret)
- goto out;
-
- switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) {
- case DW_IC_CON_SPEED_STD:
- mode_str = "Standard Mode";
- break;
- case DW_IC_CON_SPEED_HIGH:
- mode_str = "High Speed Mode";
- break;
- default:
- mode_str = "Fast Mode";
- }
- dev_dbg(dev->dev, "Bus speed: %s%s\n", mode_str, fp_str);
+ return ret;
-out:
- return ret;
+ dev_dbg(dev->dev, "Bus speed: %s\n",
i2c_freq_mode_string(t->bus_freq_hz));
+ return 0;
}
/**
--
2.8.1
[PATCH v7 4/5] MAINTAINERS: Add maintainer for HiSilicon I2C driver
Add maintainer for HiSilicon I2C driver. Signed-off-by: Yicong Yang --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8d23b0e..da2754a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8040,6 +8040,13 @@ F: drivers/crypto/hisilicon/hpre/hpre.h F: drivers/crypto/hisilicon/hpre/hpre_crypto.c F: drivers/crypto/hisilicon/hpre/hpre_main.c +HISILICON I2C CONTROLLER DRIVER +M: Yicong Yang +L: linux-...@vger.kernel.org +S: Maintained +W: https://www.hisilicon.com +F: drivers/i2c/busses/i2c-hisi.c + HISILICON LPC BUS DRIVER M: john.ga...@huawei.com S: Maintained -- 2.8.1
Re: [PATCH 3/4] docs: Add documentation for HiSilicon PTT device driver
On 2021/4/8 2:55, Bjorn Helgaas wrote:
> Move important info in the subject earlier, e.g.,
>
> docs: Add HiSilicon PTT device documentation
>
> On Tue, Apr 06, 2021 at 08:45:53PM +0800, Yicong Yang wrote:
>> Document the introduction and usage of HiSilicon PTT device driver.
>>
>> Signed-off-by: Yicong Yang
>> ---
>> Documentation/trace/hisi-ptt.rst | 316
>> +++
>> 1 file changed, 316 insertions(+)
>> create mode 100644 Documentation/trace/hisi-ptt.rst
>>
>> diff --git a/Documentation/trace/hisi-ptt.rst
>> b/Documentation/trace/hisi-ptt.rst
>> new file mode 100644
>> index 000..215676f
>> --- /dev/null
>> +++ b/Documentation/trace/hisi-ptt.rst
>> @@ -0,0 +1,316 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +==
>> +HiSilicon PCIe Tune and Trace device
>> +==
>> +
>> +Introduction
>> +
>> +
>> +HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex
>> +integrated Endpoint (RCiEP) device, providing the capability
>> +to dynamically monitor and tune the PCIe link's events (tune),
>> +and trace the TLP headers (trace). The two functions are independent,
>> +but is recommended to use them together to analyze and enhance the
>> +PCIe link's performance.
>
>> +On Kunpeng 930 SoC, the PCIe root complex is composed of several
>> +PCIe cores.
>> +Each core is composed of several root ports, RCiEPs, and one
>> +PTT device, like below. The PTT device is capable of tuning and
>> +tracing the link of the PCIe core.
>
> s/root complex/Root Complex/ to match spec, diagram, RCiEP above
> s/root ports/Root Ports/ to match spec, etc (also below)
>
thanks. will fix here and below in this doc.
> Can you connect "Kunpeng 930" to something in the kernel tree?
> "git grep -i kunpeng" shows nothing that's obviously relevant.
> I assume there's a related driver in drivers/pci/controller/?
>
Kunpeng 930 is the product name of Hip09 platform. The PCIe
controller uses the generic PCIe driver based on ACPI.
> Is this one paragraph or two? If one, reflow. If two, add blank line
> between.
>
will reflow here and below. it's one paragraph.
> IIUC, the diagram below shows two PCIe cores, each with three Root
> Ports and a PTT RCiEP. Your text mentions "RCiEPs, and one PTT" which
> suggests RCiEPs in addition to the PTT, but the diagram doesn't show
> any, and if there are other RCiEPs, they don't seem relevant to this
> doc. Maybe something like this?
>
> Each PCIe core includes several Root Ports and a PTT RCiEP ...
>
will fix.
>> +::
>> + +--Core 0---+
>> + | | [ PTT ] |
>> + | | [Root Port]---[Endpoint]
>> + | | [Root Port]---[Endpoint]
>> + | | [Root Port]---[Endpoint]
>> +Root Complex |--Core 1---+
>> + | | [ PTT ] |
>> + | | [Root Port]---[ Switch ]---[Endpoint]
>> + | | [Root Port]---[Endpoint] `-[Endpoint]
>> + | | [Root Port]---[Endpoint]
>> + +---+
>> +
>> +The PTT device driver cannot be loaded if debugfs is not mounted.
>> +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt
>> +as its root directory, with name of its BDF number.
>> +::
>> +
>> +/sys/kernel/debug/hisi_ptt/::.
>> +
>> +Tune
>> +
>> +
>> +PTT tune is designed for monitoring and adjusting PCIe link
>> parameters(events).
>
> Add a space before "(".
>
will add here and below.
>> +Currently we support events in 4 classes. The scope of the events
>> +covers the PCIe core with which the PTT device belongs to.
>
> ... the PCIe core to which the PTT device belongs.
will fix.
>> +
>> +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and
>> +mostly a simple open/read/write/close cycle will be used to tune
>> +the event.
>> +::
>> +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune
>> +$ ls
>> +qos_tx_cplqos_tx_npqos_tx_p
>> +tx_path_rx_req_alloc_buf_level
>> +tx_path_tx_req_alloc_buf_level
>> +$ cat qos_tx_dp
>> +1
>> +$ echo 2 > qos_tx_dp
>> +$ cat qos_tx_dp
>> +2
>> +
>> +Current value(numerical value) of the event can be simply
Re: [PATCH 0/4] Add support for HiSilicon PCIe Tune and Trace device
On 2021/4/7 18:25, Greg KH wrote: > On Wed, Apr 07, 2021 at 06:03:11PM +0800, Yicong Yang wrote: >> On 2021/4/6 21:49, Greg KH wrote: >>> On Tue, Apr 06, 2021 at 08:45:50PM +0800, Yicong Yang wrote: >>>> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex >>>> integrated Endpoint(RCiEP) device, providing the capability >>>> to dynamically monitor and tune the PCIe traffic(tune), >>>> and trace the TLP headers(trace). The driver exposes the user >>>> interface through debugfs, so no need for extra user space tools. >>>> The usage is described in the document. >>> >>> Why use debugfs and not the existing perf tools for debugging? >>> >> >> The perf doesn't match our device as we've analyzed. >> >> For the tune function it doesn't do the sampling at all. >> User specifys one link parameter and reads its current value or set >> the desired one. The process is static. We didn't find a >> way to adapt to perf. >> >> For the trace function, we may barely adapt to the perf framework >> but it doesn't seems like a better choice. We have our own format >> of data and don't need perf doing the parsing, and we'll get extra >> information added by perf as well. The settings through perf tools >> won't satisfy our needs, we cannot present available settings >> (filter BDF number, TLP types, buffer controls) to >> the user and user cannot set in a friendly way. For example, >> we cannot count on perf to decode the usual format BDF number like >> ::., which user can use filter the TLP >> headers. > > Please work with the perf developers to come up with a solution. I find > it hard to believe that your hardware is so different than all the other > hardware that perf currently supports. I would need their agreement > that you can not use perf before accepting this patchset. > Sure. I'll resend this series with more detailed information and with perf list and developers cc'ed to collect more suggestions on this device and driver. Thanks, Yicong
Re: [PATCH 1/4] driver core: Use subdir-ccflags-* to inherit debug flag
On 2021/2/10 19:42, Daniel Thompson wrote:
> On Mon, Feb 08, 2021 at 09:09:20PM +0800, Yicong Yang wrote:
>> On 2021/2/8 18:47, Greg KH wrote:
>>> On Mon, Feb 08, 2021 at 06:44:52PM +0800, Yicong Yang wrote:
>>>> On 2021/2/5 17:53, Greg KH wrote:
>>>>> What does this offer in benefit of the existing way? What is it fixing?
>>>>> Why do this "churn"?
>>>>
>>>> currently we have added ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG in the
>>>> Makefile
>>>> of driver/base and driver/base/power, but not in the subdirectory
>>>> driver/base/firmware_loader. we cannot turn the debug on for subdirectory
>>>> firmware_loader if we config DEBUG_DRIVER and there is no kconfig option
>>>> for the it.
>>>
>>> Is that necessary? Does that directory need it?
>>
>> there are several debug prints in firmware_loader/main.c:
>>
>> ./main.c:207: pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name,
>> fw_priv);
>> ./main.c:245: pr_debug("batched request - sharing the same
>> struct fw_priv and lookup for multiple requests\n");
>>
>
> Even if these are not in scope for CONFIG_DEBUG_DRVIER there is a
> config option that would allow you to observe them without changing
> any code (CONFIG_DYNAMIC_DEBUG).
>
yes. they're two mechanisms of debug. i think it's the right thing to make
both work properly.
>
> Daniel.
>
> .
>
Re: [PATCH] PCI: Use subdir-ccflags-* to inherit debug flag
On 2021/2/10 5:25, Bjorn Helgaas wrote:
> [+cc Masahiro, Michal, linux-kbuild, linux-kernel]
>
> On Thu, Feb 04, 2021 at 07:30:15PM +0800, Yicong Yang wrote:
>> From: Junhao He
>>
>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
>> settings from Kconfig when traversing subdirectories.
>>
>> Signed-off-by: Junhao He
>> Signed-off-by: Yicong Yang
>
> I applied this with Krzysztof's reviewed-by and the commit log below
> to pci/misc for v5.12, thanks!
>
> Feel free to copy or improve the commit log for use elsewhere.
>
thanks for improving the commit. i admit that i didn't make the it
clear enough. it's much better now.
Thanks,
Yicong
>> ---
>> drivers/pci/Makefile | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 11cc794..d62c4ac 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
>> obj-y += controller/
>> obj-y += switch/
>>
>> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>
> commit e8e9aababe60 ("PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci
> hierarchy")
> Author: Junhao He
> Date: Thu Feb 4 19:30:15 2021 +0800
>
> PCI: Apply CONFIG_PCI_DEBUG to entire drivers/pci hierarchy
>
> CONFIG_PCI_DEBUG=y adds -DDEBUG to CFLAGS, which enables things like
> pr_debug() and dev_dbg() (and hence pci_dbg()). Previously we added
> -DDEBUG for files in drivers/pci/, but not files in subdirectories of
> drivers/pci/.
>
> Add -DDEBUG to CFLAGS for all files below drivers/pci/ so CONFIG_PCI_DEBUG
> applies to the entire hierarchy.
>
> [bhelgaas: commit log]
> Link:
> https://lore.kernel.org/r/1612438215-33105-1-git-send-email-yangyic...@hisilicon.com
> Signed-off-by: Junhao He
> Signed-off-by: Yicong Yang
> Signed-off-by: Bjorn Helgaas
> Reviewed-by: Krzysztof Wilczyński
>
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 11cc79411e2d..d62c4ac4ae1b 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
> obj-y+= controller/
> obj-y+= switch/
>
> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>
> .
>
Re: [PATCH v2 2/4] hwmon: Use subdir-ccflags-* to inherit debug flag
On 2021/2/9 23:06, Guenter Roeck wrote: > On Tue, Feb 09, 2021 at 07:08:17PM +0800, Yicong Yang wrote: >> From: Junhao He >> >> We use ccflags-$(CONFIG_HWMON_DEBUG_CHIP) for the debug >> message in drivers/hwmon, but the DEBUG flag will not pass to >> the subdirectory. >> >> Considering CONFIG_HWMON_DEBUG_CHIP intends to have DEBUG >> recursively in driver/hwmon. It will be clearer >> to use subdir-ccflags-* instead of ccflags-* to inherit >> the debug settings from Kconfig when traversing subdirectories, >> and it will avoid omittance of DEBUG define when debug messages >> added in the subdirectories. >> > > The above paragraph doesn't add clarity and may as well be dropped. > On the other side, the commit message still doesn't mention that > pr_debug depends on DEBUG, which I am sure many people don't know > or remember. This is the prime reason why this patch is acceptable, > so it most definitely needs to be mentioned here. sorry, i didn't realize that you mean this. will impove this in the next version after the lunar new year holiday over. Thanks, Yicong > > Guenter > >> Suggested-by: Bjorn Helgaas >> Signed-off-by: Junhao He >> Signed-off-by: Yicong Yang >> --- >> drivers/hwmon/Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index 09a86c5..1c0c089 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -201,5 +201,5 @@ obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o >> obj-$(CONFIG_SENSORS_OCC) += occ/ >> obj-$(CONFIG_PMBUS) += pmbus/ >> >> -ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG >> +subdir-ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG >> >> -- >> 2.8.1 >> > > . >
Re: [PATCH] PCI: Use subdir-ccflags-* to inherit debug flag
On 2021/2/5 0:10, Bjorn Helgaas wrote:
> [+cc Masahiro, Michal, linux-kbuild, linux-kernel]
>
> On Thu, Feb 04, 2021 at 07:30:15PM +0800, Yicong Yang wrote:
>> From: Junhao He
>>
>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
>> settings from Kconfig when traversing subdirectories.
>
> So I guess the current behavior is:
>
> If CONFIG_PCI_DEBUG=y, add -DDEBUG to CFLAGS in the current
> directory, but not in any subdirectories
>
> and the behavior after this patch is:
>
> If CONFIG_PCI_DEBUG=y, add -DDEBUG to CFLAGS in the current
> directory and any subdirectories
>
> Is that right? That makes sense to me. I wonder if any other places
> have this issue?
that's right. we didn't check other places, but some have individual config
in their sub-directory as you mentioned below.
>
> 'git grep "^ccflags.*-DDEBUG"' finds a few cases where subdirectories
> use their own debug config options, e.g.,
>
> drivers/i2c/Makefile:ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
> drivers/i2c/algos/Makefile:ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG
> drivers/i2c/busses/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> drivers/i2c/muxes/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
>
> But some have subdirectories that look like they probably should be
> included by using subdir-ccflags, e.g.,
>
> drivers/base/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> drivers/base/power/Makefile:ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> # drivers/base/{firmware_loader,regmap,test}/ not included
>
> drivers/hwmon/Makefile:ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> # drivers/hwmon/{occ,pmbus}/ not included
>
> drivers/pps/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> drivers/pps/clients/Makefile:ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> # drivers/pps/generators/ not included
>
> There are many more places that add -DDEBUG to ccflags-y that *don't*
> have subdirectories.
>
> I wonder the default should be that we use subdir-ccflags all the
> time, and use ccflags only when we actually want different
> CONFIG_*_DEBUG options for subdirectories.
agree. if there is no debug config in the sub-directory, the
config should be inherited from its parent directory using subdir-ccflags.
we can post a separate serial to issue other places.
Thanks,
Yicong
>
>> Signed-off-by: Junhao He
>> Signed-off-by: Yicong Yang
>> ---
>> drivers/pci/Makefile | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
>> index 11cc794..d62c4ac 100644
>> --- a/drivers/pci/Makefile
>> +++ b/drivers/pci/Makefile
>> @@ -36,4 +36,4 @@ obj-$(CONFIG_PCI_ENDPOINT) += endpoint/
>> obj-y += controller/
>> obj-y += switch/
>>
>> -ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> +subdir-ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
>> --
>> 2.8.1
>>
>
> .
>
[PATCH 2/4] hwmon: Use subdir-ccflags-* to inherit debug flag
From: Junhao He Use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/hwmon/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 09a86c5..1c0c089 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -201,5 +201,5 @@ obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o obj-$(CONFIG_SENSORS_OCC) += occ/ obj-$(CONFIG_PMBUS)+= pmbus/ -ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG +subdir-ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG -- 2.8.1
[PATCH 3/4] pps: Use subdir-ccflags-* to inherit debug flag
From: Junhao He Use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/pps/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile index ceaf65c..7a2d3f7 100644 --- a/drivers/pps/Makefile +++ b/drivers/pps/Makefile @@ -8,4 +8,4 @@ pps_core-$(CONFIG_NTP_PPS) += kc.o obj-$(CONFIG_PPS) := pps_core.o obj-y += clients/ generators/ -ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG +subdir-ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG -- 2.8.1
[PATCH 1/4] driver core: Use subdir-ccflags-* to inherit debug flag
From: Junhao He Use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/base/Makefile | 2 +- drivers/base/power/Makefile | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 5e7bf96..c6bdf19 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -27,5 +27,5 @@ obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o obj-y += test/ -ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG +subdir-ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile index 8fdd007..2990167 100644 --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -5,5 +5,3 @@ obj-$(CONFIG_PM_TRACE_RTC) += trace.o obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o obj-$(CONFIG_HAVE_CLK) += clock_ops.o obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o - -ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG -- 2.8.1
[PATCH 0/4] Use subdir-ccflags-* to inherit debug flag
Few drivers use ccflags-* in their top directory to enable -DDEBUG, but don't have config options to enable debug in the sub-directories. They should want the subdirectories inherit the debug flag from the top. Use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. We primarily find this issue when debugging PCIe and other drivers may also have this issues. Previous discussion can be find at https://lore.kernel.org/linux-pci/1612438215-33105-1-git-send-email-yangyic...@hisilicon.com/ Junhao He (4): driver core: Use subdir-ccflags-* to inherit debug flag hwmon: Use subdir-ccflags-* to inherit debug flag pps: Use subdir-ccflags-* to inherit debug flag staging: comedi: Use subdir-ccflags-* to inherit debug flag drivers/base/Makefile | 2 +- drivers/base/power/Makefile | 2 -- drivers/hwmon/Makefile| 2 +- drivers/pps/Makefile | 2 +- drivers/staging/comedi/Makefile | 2 +- drivers/staging/comedi/drivers/Makefile | 1 - drivers/staging/comedi/drivers/tests/Makefile | 2 -- drivers/staging/comedi/kcomedilib/Makefile| 2 -- 8 files changed, 4 insertions(+), 11 deletions(-) -- 2.8.1
[PATCH 4/4] staging: comedi: Use subdir-ccflags-* to inherit debug flag
From: Junhao He Use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/staging/comedi/Makefile | 2 +- drivers/staging/comedi/drivers/Makefile | 1 - drivers/staging/comedi/drivers/tests/Makefile | 2 -- drivers/staging/comedi/kcomedilib/Makefile| 2 -- 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/staging/comedi/Makefile b/drivers/staging/comedi/Makefile index 072ed83..f51cc14 100644 --- a/drivers/staging/comedi/Makefile +++ b/drivers/staging/comedi/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG +subdir-ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG comedi-y := comedi_fops.o range.o drivers.o \ comedi_buf.o diff --git a/drivers/staging/comedi/drivers/Makefile b/drivers/staging/comedi/drivers/Makefile index b24ac00..7cafc36 100644 --- a/drivers/staging/comedi/drivers/Makefile +++ b/drivers/staging/comedi/drivers/Makefile @@ -1,7 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for individual comedi drivers # -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG # Comedi "helper" modules obj-$(CONFIG_COMEDI_8254) += comedi_8254.o diff --git a/drivers/staging/comedi/drivers/tests/Makefile b/drivers/staging/comedi/drivers/tests/Makefile index b5d8e13..44ac13d 100644 --- a/drivers/staging/comedi/drivers/tests/Makefile +++ b/drivers/staging/comedi/drivers/tests/Makefile @@ -1,7 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for comedi drivers unit tests # -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG - obj-$(CONFIG_COMEDI_TESTS) += example_test.o ni_routes_test.o CFLAGS_ni_routes_test.o:= -DDEBUG diff --git a/drivers/staging/comedi/kcomedilib/Makefile b/drivers/staging/comedi/kcomedilib/Makefile index 8031142..9f20318 100644 --- a/drivers/staging/comedi/kcomedilib/Makefile +++ b/drivers/staging/comedi/kcomedilib/Makefile @@ -1,6 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG - obj-$(CONFIG_COMEDI_KCOMEDILIB)+= kcomedilib.o kcomedilib-objs := kcomedilib_main.o -- 2.8.1
[PATCH v2 4/4] staging: comedi: Use subdir-ccflags-* to inherit debug flag
From: Junhao He As CONFIG_COMEDI_DEBUG intends to have the DEBUG flag recursively under drivers/staging/comedi, use subdir-ccflags-* instead of ccflags-* will make it clearer as the DEBUG flag will be inherited when traversing subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/staging/comedi/Makefile | 2 +- drivers/staging/comedi/drivers/Makefile | 1 - drivers/staging/comedi/drivers/tests/Makefile | 2 -- drivers/staging/comedi/kcomedilib/Makefile| 2 -- 4 files changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/staging/comedi/Makefile b/drivers/staging/comedi/Makefile index 072ed83..f51cc14 100644 --- a/drivers/staging/comedi/Makefile +++ b/drivers/staging/comedi/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG +subdir-ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG comedi-y := comedi_fops.o range.o drivers.o \ comedi_buf.o diff --git a/drivers/staging/comedi/drivers/Makefile b/drivers/staging/comedi/drivers/Makefile index b24ac00..7cafc36 100644 --- a/drivers/staging/comedi/drivers/Makefile +++ b/drivers/staging/comedi/drivers/Makefile @@ -1,7 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for individual comedi drivers # -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG # Comedi "helper" modules obj-$(CONFIG_COMEDI_8254) += comedi_8254.o diff --git a/drivers/staging/comedi/drivers/tests/Makefile b/drivers/staging/comedi/drivers/tests/Makefile index b5d8e13..44ac13d 100644 --- a/drivers/staging/comedi/drivers/tests/Makefile +++ b/drivers/staging/comedi/drivers/tests/Makefile @@ -1,7 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for comedi drivers unit tests # -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG - obj-$(CONFIG_COMEDI_TESTS) += example_test.o ni_routes_test.o CFLAGS_ni_routes_test.o:= -DDEBUG diff --git a/drivers/staging/comedi/kcomedilib/Makefile b/drivers/staging/comedi/kcomedilib/Makefile index 8031142..9f20318 100644 --- a/drivers/staging/comedi/kcomedilib/Makefile +++ b/drivers/staging/comedi/kcomedilib/Makefile @@ -1,6 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 -ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG - obj-$(CONFIG_COMEDI_KCOMEDILIB)+= kcomedilib.o kcomedilib-objs := kcomedilib_main.o -- 2.8.1
[PATCH v2 3/4] pps: Use subdir-ccflags-* to inherit debug flag
From: Junhao He We use ccflags-$(CONFIG_PPS_DEBUG) for the debug message in drivers/pps, but the DEBUG flag will not pass to the subdirectory. Considering CONFIG_HWMON_DEBUG_CHIP intends to turn on debug recursively under driver/pps, so it will be clearer to use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/pps/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile index ceaf65c..7a2d3f7 100644 --- a/drivers/pps/Makefile +++ b/drivers/pps/Makefile @@ -8,4 +8,4 @@ pps_core-$(CONFIG_NTP_PPS) += kc.o obj-$(CONFIG_PPS) := pps_core.o obj-y += clients/ generators/ -ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG +subdir-ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG -- 2.8.1
[PATCH v2 0/4] Use subdir-ccflags-* to inherit debug flag
Few drivers use ccflags-* in their top directory to enable -DDEBUG, but don't have config options to enable debug in the sub-directories, or they use per subdirectory ccflags-* to have DEBUG with the same kconfig option. Considering they intends to enable debug for all the files under the directory with the same kconfig option, it will be clearer to use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories. We primarily find this issue when debugging PCIe and thought other drivers may also have this issues. Previous discussion can be find at https://lore.kernel.org/linux-pci/1612438215-33105-1-git-send-email-yangyic...@hisilicon.com/ Change since v1: - reword the commits to illustrate the reasons of the change and the benefits. v1: https://lore.kernel.org/lkml/1612518255-23052-1-git-send-email-yangyic...@hisilicon.com/ Junhao He (4): driver core: Use subdir-ccflags-* to inherit debug flag hwmon: Use subdir-ccflags-* to inherit debug flag pps: Use subdir-ccflags-* to inherit debug flag staging: comedi: Use subdir-ccflags-* to inherit debug flag drivers/base/Makefile | 2 +- drivers/base/power/Makefile | 2 -- drivers/hwmon/Makefile| 2 +- drivers/pps/Makefile | 2 +- drivers/staging/comedi/Makefile | 2 +- drivers/staging/comedi/drivers/Makefile | 1 - drivers/staging/comedi/drivers/tests/Makefile | 2 -- drivers/staging/comedi/kcomedilib/Makefile| 2 -- 8 files changed, 4 insertions(+), 11 deletions(-) -- 2.8.1
[PATCH v2 2/4] hwmon: Use subdir-ccflags-* to inherit debug flag
From: Junhao He We use ccflags-$(CONFIG_HWMON_DEBUG_CHIP) for the debug message in drivers/hwmon, but the DEBUG flag will not pass to the subdirectory. Considering CONFIG_HWMON_DEBUG_CHIP intends to have DEBUG recursively in driver/hwmon. It will be clearer to use subdir-ccflags-* instead of ccflags-* to inherit the debug settings from Kconfig when traversing subdirectories, and it will avoid omittance of DEBUG define when debug messages added in the subdirectories. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/hwmon/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 09a86c5..1c0c089 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -201,5 +201,5 @@ obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o obj-$(CONFIG_SENSORS_OCC) += occ/ obj-$(CONFIG_PMBUS)+= pmbus/ -ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG +subdir-ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG -- 2.8.1
[PATCH v2 1/4] driver core: Use subdir-ccflags-* to inherit debug flag
From: Junhao He Currently we can turn on the debug message in the top directory driver/base and subdirectory driver/base/power with kconfig option CONFIG_DEBUG_DRIVER. But the DEBUG flags will not pass to subdirectory drvier/base/firmware_loader which the ccflags-$(CONFIG_DEBUG_DRIVER) is missing and there is no kconfig option to turn on the debug message for it. Use subdir-ccflags-* for the DEBUG flag in the top directory will fix this. Considering CONFIG_DEBUG_DRIVER intends to turn on the debug recursively, use subdir-cclags-* will be clearer and avoid omittance of DEBUG define in the subdirectory. Suggested-by: Bjorn Helgaas Signed-off-by: Junhao He Signed-off-by: Yicong Yang --- drivers/base/Makefile | 2 +- drivers/base/power/Makefile | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 5e7bf96..c6bdf19 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -27,5 +27,5 @@ obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o obj-y += test/ -ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG +subdir-ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile index 8fdd007..2990167 100644 --- a/drivers/base/power/Makefile +++ b/drivers/base/power/Makefile @@ -5,5 +5,3 @@ obj-$(CONFIG_PM_TRACE_RTC) += trace.o obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o obj-$(CONFIG_HAVE_CLK) += clock_ops.o obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o - -ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG -- 2.8.1
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan
>
> If there are non-hotplug capable devices connected to a given
> port, then during the fatal error recovery(triggered by DPC or
> AER), after calling reset_link() function, we cannot rely on
> hotplug handler to detach and re-enumerate the device drivers
> in the affected bus. Instead, we will have to let the error
> recovery handler call report_slot_reset() for all devices in
> the bus to notify about the reset operation. Although this is
> only required for non hot-plug capable devices, doing it for
> hotplug capable devices should not affect the functionality.
>
> Along with above issue, this fix also applicable to following
> issue.
>
> Commit 6d2c89441571 ("PCI/ERR: Update error status after
> reset_link()") added support to store status of reset_link()
> call. Although this fixed the error recovery issue observed if
> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded the status
> result from report_frozen_detected. This can cause a failure to
> recover if _NEED_RESET is returned by report_frozen_detected and
> report_slot_reset is not invoked.
>
> Such an event can be induced for testing purposes by reducing the
> Max_Payload_Size of a PCIe bridge to less than that of a device
> downstream from the bridge, and then initiating I/O through the
> device, resulting in oversize transactions. In the presence of DPC,
> this results in a containment event and attempted reset and recovery
> via pcie_do_recovery. After 6d2c89441571 report_slot_reset is not
> invoked, and the device does not recover.
>
> [original patch is from jay.vosbu...@canonical.com]
> [original patch link
> https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
> Signed-off-by: Jay Vosburgh
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
> ---
> drivers/pci/pcie/err.c | 19 +++
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
> index 14bb8f54723e..db80e1ecb2dc 100644
> --- a/drivers/pci/pcie/err.c
> +++ b/drivers/pci/pcie/err.c
> @@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
> pci_dbg(dev, "broadcast error_detected message\n");
> if (state == pci_channel_io_frozen) {
> pci_walk_bus(bus, report_frozen_detected, &status);
> - status = reset_link(dev);
> - if (status != PCI_ERS_RESULT_RECOVERED) {
> + status = PCI_ERS_RESULT_NEED_RESET;
> + } else {
> + pci_walk_bus(bus, report_normal_detected, &status);
> + }
> +
> + if (status == PCI_ERS_RESULT_NEED_RESET) {
> + if (reset_link) {
> + if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED)
we'll call reset_link() only if link is frozen. so it may have problem here.
Thanks,
Yicong
> + status = PCI_ERS_RESULT_DISCONNECT;
> + } else {
> + if (pci_bus_error_reset(dev))
> + status = PCI_ERS_RESULT_DISCONNECT;
> + }
> +
> + if (status == PCI_ERS_RESULT_DISCONNECT) {
> pci_warn(dev, "link reset failed\n");
> goto failed;
> }
> - } else {
> - pci_walk_bus(bus, report_normal_detected, &status);
> }
>
> if (status == PCI_ERS_RESULT_CAN_RECOVER) {
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 5/20/20 1:28 AM, Yicong Yang wrote:
>> On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan
>>>
>>>
>>> If there are non-hotplug capable devices connected to a given
>>> port, then during the fatal error recovery(triggered by DPC or
>>> AER), after calling reset_link() function, we cannot rely on
>>> hotplug handler to detach and re-enumerate the device drivers
>>> in the affected bus. Instead, we will have to let the error
>>> recovery handler call report_slot_reset() for all devices in
>>> the bus to notify about the reset operation. Although this is
>>> only required for non hot-plug capable devices, doing it for
>>> hotplug capable devices should not affect the functionality.
>>>
>>> Along with above issue, this fix also applicable to following
>>> issue.
>>>
>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>> reset_link()") added support to store status of reset_link()
>>> call. Although this fixed the error recovery issue observed if
>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded the status
>>> result from report_frozen_detected. This can cause a failure to
>>> recover if _NEED_RESET is returned by report_frozen_detected and
>>> report_slot_reset is not invoked.
>>>
>>> Such an event can be induced for testing purposes by reducing the
>>> Max_Payload_Size of a PCIe bridge to less than that of a device
>>> downstream from the bridge, and then initiating I/O through the
>>> device, resulting in oversize transactions. In the presence of DPC,
>>> this results in a containment event and attempted reset and recovery
>>> via pcie_do_recovery. After 6d2c89441571 report_slot_reset is not
>>> invoked, and the device does not recover.
>>>
>>> [original patch is from jay.vosbu...@canonical.com]
>>> [original patch link
>>> https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>> Signed-off-by: Jay Vosburgh
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>
>>> ---
>>> drivers/pci/pcie/err.c | 19 +++
>>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>> index 14bb8f54723e..db80e1ecb2dc 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>> pci_dbg(dev, "broadcast error_detected message\n");
>>> if (state == pci_channel_io_frozen) {
>>> pci_walk_bus(bus, report_frozen_detected, &status);
>>> -status = reset_link(dev);
>>> -if (status != PCI_ERS_RESULT_RECOVERED) {
>>> +status = PCI_ERS_RESULT_NEED_RESET;
>>> +} else {
>>> +pci_walk_bus(bus, report_normal_detected, &status);
>>> +}
>>> +
>>> +if (status == PCI_ERS_RESULT_NEED_RESET) {
>>> +if (reset_link) {
>>> +if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED)
>>
>> we'll call reset_link() only if link is frozen. so it may have problem here.
> you mean before this change right?
> After this change, reset_link() will be called as long as status is
> PCI_ERS_RESULT_NEED_RESET.
Yes. I think we should reset the link only if the io is blocked as before.
There's
no reason to reset a normal link.
Furthermore, PCI_ERS_RESULT_NEED_RESET means device driver requires a slot
reset rather
than a link reset, so it maybe improper to use it to judge whether a link reset
is needed.
We decide whether to do a link reset only by the io state.
Thanks,
Yicong
>>
>> Thanks,
>> Yicong
>>
>>
>>> +status = PCI_ERS_RESULT_DISCONNECT;
>>> +} else {
>>> +if (pci_bus_error_reset(dev))
>>> +status = PCI_ERS_RESULT_DISCONNECT;
>>> +}
>>> +
>>> +if (status == PCI_ERS_RESULT_DISCONNECT) {
>>> pci_warn(dev, "link reset failed\n");
>>> goto failed;
>>> }
>>> -} else {
>>> -pci_walk_bus(bus, report_normal_detected, &status);
>>> }
>>> if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>>
> .
>
Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support
On 2021/3/18 17:24, Michael Walle wrote:
> Add support to show the name and JEDEC identifier as well as to dump the
> SFDP table. Not all flashes list their SFDP table contents in their
> datasheet. So having that is useful. It might also be helpful in bug
> reports from users.
>
> The idea behind the sysfs module is also to have raw access to the SPI
> NOR flash device registers, which can also be useful for debugging.
Hi Michael,
I like the idea to dump the sfdp data,it will make debug easier. should it go
in debugfs?
we already have debugfs files for partname and partid of the flash.
>
> Signed-off-by: Michael Walle
> ---
> drivers/mtd/spi-nor/Makefile | 2 +-
> drivers/mtd/spi-nor/core.c | 5 +++
> drivers/mtd/spi-nor/core.h | 3 ++
> drivers/mtd/spi-nor/sysfs.c | 86
> 4 files changed, 95 insertions(+), 1 deletion(-)
> create mode 100644 drivers/mtd/spi-nor/sysfs.c
>
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..aff308f75987 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
>
> -spi-nor-objs := core.o sfdp.o
> +spi-nor-objs := core.o sfdp.o sysfs.o
> spi-nor-objs += atmel.o
> spi-nor-objs += catalyst.o
> spi-nor-objs += eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a315cb1c4db..2eaf4ba8c0f3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem *spimem)
> if (ret)
> return ret;
>
> + ret = spi_nor_sysfs_create(nor);
> + if (ret)
> + return ret;
> +
> return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> data ? data->nr_parts : 0);
> }
> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
> struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>
> spi_nor_restore(nor);
> + spi_nor_sysfs_remove(nor);
>
> /* Clean up MTD stuff. */
> return mtd_device_unregister(&nor->mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 668f22011b1d..dd592f7b62d1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused
> *mtd_to_spi_nor(struct mtd_info *mtd)
> return mtd->priv;
> }
>
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
> #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index ..0de031e246c5
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + return sprintf(buf, "%s\n", nor->info->name);
perhaps sysfs_emit() instead if we go sysfs? as suggested by [1].
[1] Documentation/filesystems/sysfs.rst:line 246
Thanks,
Yicong
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> + &dev_attr_name.attr,
> + &dev_attr_jedec_id.attr,
> + NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf,
> + loff_t off, size_t count)
> +{
> + struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> + struct sfdp *sfdp = nor->sfdp;
> + size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> + return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
> +sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> + &bin_attr_sfdp,
> + NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> +
[PATCH v5 1/5] i2c: core: add managed function for adding i2c adapters
Some I2C controller drivers will only unregister the I2C
adapter in their .remove() callback, which can be done
by simply using a managed variant to add the I2C adapter.
So add the managed functions for adding the I2C adapter.
Reviewed-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
drivers/i2c/i2c-core-base.c | 26 ++
include/linux/i2c.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf72..de9402c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1703,6 +1703,32 @@ void i2c_del_adapter(struct i2c_adapter *adap)
}
EXPORT_SYMBOL(i2c_del_adapter);
+static void devm_i2c_del_adapter(void *adapter)
+{
+ i2c_del_adapter(adapter);
+}
+
+/**
+ * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter()
+ * @dev: managing device for adding this I2C adapter
+ * @adapter: the adapter to add
+ * Context: can sleep
+ *
+ * Add adapter with dynamic bus number, same with i2c_add_adapter()
+ * but the adapter will be auto deleted on driver detach.
+ */
+int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
+{
+ int ret;
+
+ ret = i2c_add_adapter(adapter);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, devm_i2c_del_adapter, adapter);
+}
+EXPORT_SYMBOL_GPL(devm_i2c_add_adapter);
+
static void i2c_parse_timing(struct device *dev, char *prop_name, u32
*cur_val_p,
u32 def_val, bool use_def)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5662265..10bd0b0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -844,6 +844,7 @@ static inline void i2c_mark_adapter_resumed(struct
i2c_adapter *adap)
*/
#if IS_ENABLED(CONFIG_I2C)
int i2c_add_adapter(struct i2c_adapter *adap);
+int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter);
void i2c_del_adapter(struct i2c_adapter *adap);
int i2c_add_numbered_adapter(struct i2c_adapter *adap);
--
2.8.1
[PATCH v5 3/5] i2c: add support for HiSilicon I2C controller
Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides the access to the i2c busses, which connects to the eeprom, rtc, etc. The driver works with IRQ mode, and supports basic I2C features and 10bit address. The DMA is not supported. Reviewed-by: Andy Shevchenko Signed-off-by: Yicong Yang --- drivers/i2c/busses/Kconfig| 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-hisi.c | 506 ++ 3 files changed, 517 insertions(+) create mode 100644 drivers/i2c/busses/i2c-hisi.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 05ebf75..eddf7bf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -645,6 +645,16 @@ config I2C_HIGHLANDER This driver can also be built as a module. If so, the module will be called i2c-highlander. +config I2C_HISI + tristate "HiSilicon I2C controller" + depends on ARM64 || COMPILE_TEST + help + Say Y here if you want to have Hisilicon I2C controller support + available on the Kunpeng Server. + + This driver can also be built as a module. If so, the module + will be called i2c-hisi. + config I2C_IBM_IIC tristate "IBM PPC 4xx on-chip I2C interface" depends on 4xx diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 615f35e..e1c9292 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o +obj-$(CONFIG_I2C_HISI) += i2c-hisi.o obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c new file mode 100644 index 000..e717734 --- /dev/null +++ b/drivers/i2c/busses/i2c-hisi.c @@ -0,0 +1,506 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * HiSilicon I2C Controller Driver for Kunpeng SoC + * + * Copyright (c) 2021 HiSilicon Limited. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HISI_I2C_FRAME_CTRL0x +#define HISI_I2C_FRAME_CTRL_SPEED_MODE GENMASK(1, 0) +#define HISI_I2C_FRAME_CTRL_ADDR_TEN BIT(2) +#define HISI_I2C_SLV_ADDR 0x0004 +#define HISI_I2C_SLV_ADDR_VALGENMASK(9, 0) +#define HISI_I2C_SLV_ADDR_GC_S_MODE BIT(10) +#define HISI_I2C_SLV_ADDR_GC_S_ENBIT(11) +#define HISI_I2C_CMD_TXDATA0x0008 +#define HISI_I2C_CMD_TXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_CMD_TXDATA_RW BIT(8) +#define HISI_I2C_CMD_TXDATA_P_EN BIT(9) +#define HISI_I2C_CMD_TXDATA_SR_ENBIT(10) +#define HISI_I2C_RXDATA0x000c +#define HISI_I2C_RXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_SS_SCL_HCNT 0x0010 +#define HISI_I2C_SS_SCL_LCNT 0x0014 +#define HISI_I2C_FS_SCL_HCNT 0x0018 +#define HISI_I2C_FS_SCL_LCNT 0x001c +#define HISI_I2C_HS_SCL_HCNT 0x0020 +#define HISI_I2C_HS_SCL_LCNT 0x0024 +#define HISI_I2C_FIFO_CTRL 0x0028 +#define HISI_I2C_FIFO_RX_CLR BIT(0) +#define HISI_I2C_FIFO_TX_CLR BIT(1) +#define HISI_I2C_FIFO_RX_AF_THRESH GENMASK(7, 2) +#define HISI_I2C_FIFO_TX_AE_THRESH GENMASK(13, 8) +#define HISI_I2C_FIFO_STATE0x002c +#define HISI_I2C_FIFO_STATE_RX_RERR BIT(0) +#define HISI_I2C_FIFO_STATE_RX_WERR BIT(1) +#define HISI_I2C_FIFO_STATE_RX_EMPTY BIT(3) +#define HISI_I2C_FIFO_STATE_TX_RERR BIT(6) +#define HISI_I2C_FIFO_STATE_TX_WERR BIT(7) +#define HISI_I2C_FIFO_STATE_TX_FULL BIT(11) +#define HISI_I2C_SDA_HOLD 0x0030 +#define HISI_I2C_SDA_HOLD_TX GENMASK(15, 0) +#define HISI_I2C_SDA_HOLD_RX GENMASK(23, 16) +#define HISI_I2C_FS_SPK_LEN0x0038 +#define HISI_I2C_FS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_HS_SPK_LEN0x003c +#define HISI_I2C_HS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_INT_MSTAT 0x0044 +#define HISI_I2C_INT_CLR 0x0048 +#define HISI_I2C_INT_MASK 0x004C +#define HISI_I2C_TRANS_STATE 0x0050 +#define HISI_I2C_TRANS_ERR 0x0054 +#define HISI_I2C_VERSION 0x0058 + +#define HISI_I2C_INT_ALL GENMASK(4, 0) +#define HISI_I2C_INT_TRANS_CPLTBIT(0) +#define HISI_I2C_INT_TRANS_ERR BIT(1) +#define HISI_I2C_INT_FIFO_ERR BIT(2) +#define HISI_I2C_INT_RX_FULL BIT(3) +#define HISI_I2C_INT_TX_EMPTY BIT(4) +#define HISI_I2C_INT_ERR \ + (HISI_I2C_INT_TRANS_ERR | HISI_I2C_INT_FIFO_ERR) + +#define HISI_I2C_STD_SPEED_MODE0 +#define HISI_I2C_FAST_SPEED_M
[PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string()
From: Andy Shevchenko
Use generic i2c_freq_mode_string() helper to print chosen bus speed.
Signed-off-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
drivers/i2c/busses/i2c-designware-master.c | 20
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c
b/drivers/i2c/busses/i2c-designware-master.c
index dd27b9d..b64c4c8 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -35,10 +35,10 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev
*dev)
static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
{
- const char *mode_str, *fp_str = "";
u32 comp_param1;
u32 sda_falling_time, scl_falling_time;
struct i2c_timings *t = &dev->timings;
+ const char *fp_str = "";
u32 ic_clk;
int ret;
@@ -153,22 +153,10 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev
*dev)
ret = i2c_dw_set_sda_hold(dev);
if (ret)
- goto out;
-
- switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) {
- case DW_IC_CON_SPEED_STD:
- mode_str = "Standard Mode";
- break;
- case DW_IC_CON_SPEED_HIGH:
- mode_str = "High Speed Mode";
- break;
- default:
- mode_str = "Fast Mode";
- }
- dev_dbg(dev->dev, "Bus speed: %s%s\n", mode_str, fp_str);
+ return ret;
-out:
- return ret;
+ dev_dbg(dev->dev, "Bus speed: %s\n",
i2c_freq_mode_string(t->bus_freq_hz));
+ return 0;
}
/**
--
2.8.1
[PATCH v5 4/5] MAINTAINERS: Add maintainer for HiSilicon I2C driver
Add maintainer for HiSilicon I2C driver. Signed-off-by: Yicong Yang --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8d23b0e..da2754a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8040,6 +8040,13 @@ F: drivers/crypto/hisilicon/hpre/hpre.h F: drivers/crypto/hisilicon/hpre/hpre_crypto.c F: drivers/crypto/hisilicon/hpre/hpre_main.c +HISILICON I2C CONTROLLER DRIVER +M: Yicong Yang +L: linux-...@vger.kernel.org +S: Maintained +W: https://www.hisilicon.com +F: drivers/i2c/busses/i2c-hisi.c + HISILICON LPC BUS DRIVER M: john.ga...@huawei.com S: Maintained -- 2.8.1
[PATCH v5 0/5] Add support for HiSilicon I2C controller
Add driver and MAINTAINERS for HiSilicon I2C controller on Kunpeng SoC. Also provide the devm_*() variants for adding the I2C adapters. Add a public api to provide I2C frequency mode strings and convert designware driver to use it. Change since v4: - and Andy's review-by - attach Andy's patch of switch designware driver to use i2c_freq_mode_string() Link: https://lore.kernel.org/linux-i2c/1617109549-4013-1-git-send-email-yangyic...@hisilicon.com/ Link: https://lore.kernel.org/linux-i2c/20210330134633.29889-1-andriy.shevche...@linux.intel.com/ Change since v3: - split the bus mode string api to I2C as suggested by Andy - simplify the devm variants and change the export format - address the comments of the HiSilicon I2C driver from Andy and Dmitry, thanks! Link: https://lore.kernel.org/linux-i2c/1616411413-7177-1-git-send-email-yangyic...@hisilicon.com/ Change since v2: - handle -EPROBE_DEFER case when get irq number by platform_get_irq() Link: https://lore.kernel.org/linux-i2c/1615296137-14558-1-git-send-email-yangyic...@hisilicon.com/ Change since v1: - fix compile test error on 32bit arch, reported by intel lkp robot: 64 bit division without using kernel wrapper in probe function. Link:https://lore.kernel.org/linux-i2c/1615016946-55670-1-git-send-email-yangyic...@hisilicon.com/ Andy Shevchenko (1): i2c: designware: Switch over to i2c_freq_mode_string() Yicong Yang (4): i2c: core: add managed function for adding i2c adapters i2c: core: add api to provide frequency mode strings i2c: add support for HiSilicon I2C controller MAINTAINERS: Add maintainer for HiSilicon I2C driver MAINTAINERS| 7 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile| 1 + drivers/i2c/busses/i2c-designware-master.c | 20 +- drivers/i2c/busses/i2c-hisi.c | 506 + drivers/i2c/i2c-core-base.c| 26 ++ include/linux/i2c.h| 21 ++ 7 files changed, 575 insertions(+), 16 deletions(-) create mode 100644 drivers/i2c/busses/i2c-hisi.c -- 2.8.1
[PATCH v5 2/5] i2c: core: add api to provide frequency mode strings
Some I2C drivers like Designware and HiSilicon will print the
bus frequency mode information, so add a public one that everyone
can make use of.
Reviewed-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
include/linux/i2c.h | 20
1 file changed, 20 insertions(+)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 10bd0b0..6837e64 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
#define I2C_MAX_HIGH_SPEED_MODE_FREQ 340
#define I2C_MAX_ULTRA_FAST_MODE_FREQ 500
+static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
+{
+ switch (bus_freq_hz) {
+ case I2C_MAX_STANDARD_MODE_FREQ:
+ return "Standard Mode(100KHz)";
+ case I2C_MAX_FAST_MODE_FREQ:
+ return "Fast Mode(400KHz)";
+ case I2C_MAX_FAST_MODE_PLUS_FREQ:
+ return "Fast Mode Plus(1.0MHz)";
+ case I2C_MAX_TURBO_MODE_FREQ:
+ return "Turbo Mode(1.4MHz)";
+ case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+ return "High Speed Mode(3.4MHz)";
+ case I2C_MAX_ULTRA_FAST_MODE_FREQ:
+ return "Ultra Fast Mode(5.0MHz)";
+ default:
+ return "Unknown Mode";
+ }
+}
+
struct module;
struct property_entry;
--
2.8.1
Re: [PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string()
On 2021/3/31 18:37, Andy Shevchenko wrote: > On Tue, Mar 30, 2021 at 10:19:26PM +0800, Yicong Yang wrote: >> From: Andy Shevchenko >> >> Use generic i2c_freq_mode_string() helper to print chosen bus speed. > > Since it will be a new version (based on Jarkko's comments), I guess you may > add his Ack here that he gave against standalone submission of this patch. > > What Bary reported I will fix separately. > i'll addresse the comments and update the series with Jarkko's acked-by added. Thanks for reminding me!
Re: [PATCH v5 2/5] i2c: core: add api to provide frequency mode strings
On 2021/3/31 16:35, Jarkko Nikula wrote:
> Hi
>
> On 3/30/21 5:19 PM, Yicong Yang wrote:
>> Some I2C drivers like Designware and HiSilicon will print the
>> bus frequency mode information, so add a public one that everyone
>> can make use of.
>>
>> Reviewed-by: Andy Shevchenko
>> Signed-off-by: Yicong Yang
>> ---
>> include/linux/i2c.h | 20
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 10bd0b0..6837e64 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>> #define I2C_MAX_HIGH_SPEED_MODE_FREQ 340
>> #define I2C_MAX_ULTRA_FAST_MODE_FREQ 500
>> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
>> +{
>> + switch (bus_freq_hz) {
>> + case I2C_MAX_STANDARD_MODE_FREQ:
>> + return "Standard Mode(100KHz)";
>> + case I2C_MAX_FAST_MODE_FREQ:
>> + return "Fast Mode(400KHz)";
>> + case I2C_MAX_FAST_MODE_PLUS_FREQ:
>> + return "Fast Mode Plus(1.0MHz)";
>> + case I2C_MAX_TURBO_MODE_FREQ:
>> + return "Turbo Mode(1.4MHz)";
>> + case I2C_MAX_HIGH_SPEED_MODE_FREQ:
>> + return "High Speed Mode(3.4MHz)";
>> + case I2C_MAX_ULTRA_FAST_MODE_FREQ:
>> + return "Ultra Fast Mode(5.0MHz)";
>> + default:
>> + return "Unknown Mode";
>> + }
>
> A few minor nits here:
> - KHz -> kHz
> - Space between text and opening parenthesis: "Mode(" -> "Mode ("
> - Space between number and unit: (1.0MHz) -> (1.0 MHz)
will address. thanks!
>
> With those changes you may add my
>
> Reviewed-by: Jarkko Nikula
> Tested-by: Jarkko Nikula
> .
[PATCH v6 1/5] i2c: core: add managed function for adding i2c adapters
Some I2C controller drivers will only unregister the I2C
adapter in their .remove() callback, which can be done
by simply using a managed variant to add the I2C adapter.
So add the managed functions for adding the I2C adapter.
Reviewed-by: Andy Shevchenko
Reviewed-by: Dmitry Osipenko
Signed-off-by: Yicong Yang
---
drivers/i2c/i2c-core-base.c | 26 ++
include/linux/i2c.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf72..de9402c 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1703,6 +1703,32 @@ void i2c_del_adapter(struct i2c_adapter *adap)
}
EXPORT_SYMBOL(i2c_del_adapter);
+static void devm_i2c_del_adapter(void *adapter)
+{
+ i2c_del_adapter(adapter);
+}
+
+/**
+ * devm_i2c_add_adapter - device-managed variant of i2c_add_adapter()
+ * @dev: managing device for adding this I2C adapter
+ * @adapter: the adapter to add
+ * Context: can sleep
+ *
+ * Add adapter with dynamic bus number, same with i2c_add_adapter()
+ * but the adapter will be auto deleted on driver detach.
+ */
+int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
+{
+ int ret;
+
+ ret = i2c_add_adapter(adapter);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, devm_i2c_del_adapter, adapter);
+}
+EXPORT_SYMBOL_GPL(devm_i2c_add_adapter);
+
static void i2c_parse_timing(struct device *dev, char *prop_name, u32
*cur_val_p,
u32 def_val, bool use_def)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5662265..10bd0b0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -844,6 +844,7 @@ static inline void i2c_mark_adapter_resumed(struct
i2c_adapter *adap)
*/
#if IS_ENABLED(CONFIG_I2C)
int i2c_add_adapter(struct i2c_adapter *adap);
+int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter);
void i2c_del_adapter(struct i2c_adapter *adap);
int i2c_add_numbered_adapter(struct i2c_adapter *adap);
--
2.8.1
[PATCH v6 5/5] i2c: designware: Switch over to i2c_freq_mode_string()
From: Andy Shevchenko
Use generic i2c_freq_mode_string() helper to print chosen bus speed.
Acked-by: Jarkko Nikula
Signed-off-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
drivers/i2c/busses/i2c-designware-master.c | 20
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-master.c
b/drivers/i2c/busses/i2c-designware-master.c
index dd27b9d..b64c4c8 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -35,10 +35,10 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev
*dev)
static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
{
- const char *mode_str, *fp_str = "";
u32 comp_param1;
u32 sda_falling_time, scl_falling_time;
struct i2c_timings *t = &dev->timings;
+ const char *fp_str = "";
u32 ic_clk;
int ret;
@@ -153,22 +153,10 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev
*dev)
ret = i2c_dw_set_sda_hold(dev);
if (ret)
- goto out;
-
- switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) {
- case DW_IC_CON_SPEED_STD:
- mode_str = "Standard Mode";
- break;
- case DW_IC_CON_SPEED_HIGH:
- mode_str = "High Speed Mode";
- break;
- default:
- mode_str = "Fast Mode";
- }
- dev_dbg(dev->dev, "Bus speed: %s%s\n", mode_str, fp_str);
+ return ret;
-out:
- return ret;
+ dev_dbg(dev->dev, "Bus speed: %s\n",
i2c_freq_mode_string(t->bus_freq_hz));
+ return 0;
}
/**
--
2.8.1
[PATCH v6 4/5] MAINTAINERS: Add maintainer for HiSilicon I2C driver
Add maintainer for HiSilicon I2C driver. Signed-off-by: Yicong Yang --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8d23b0e..da2754a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8040,6 +8040,13 @@ F: drivers/crypto/hisilicon/hpre/hpre.h F: drivers/crypto/hisilicon/hpre/hpre_crypto.c F: drivers/crypto/hisilicon/hpre/hpre_main.c +HISILICON I2C CONTROLLER DRIVER +M: Yicong Yang +L: linux-...@vger.kernel.org +S: Maintained +W: https://www.hisilicon.com +F: drivers/i2c/busses/i2c-hisi.c + HISILICON LPC BUS DRIVER M: john.ga...@huawei.com S: Maintained -- 2.8.1
[PATCH v6 0/5] Add support for HiSilicon I2C controller
Add driver and MAINTAINERS for HiSilicon I2C controller on Kunpeng SoC. Also provide the devm_*() variants for adding the I2C adapters. Add a public api to provide I2C frequency mode strings and convert designware driver to use it. Change since v5: - address the comment from Dmitry and add his Reviewed-by - address the comment from Jarkko and add his Reviewed-by and Tested-by - add Jarkko's Acked-by for designware patch Link: https://lore.kernel.org/linux-i2c/1617113966-40498-1-git-send-email-yangyic...@hisilicon.com/ Change since v4: - and Andy's Reviewed-by - attach Andy's patch of switch designware driver to use i2c_freq_mode_string() Link: https://lore.kernel.org/linux-i2c/1617109549-4013-1-git-send-email-yangyic...@hisilicon.com/ Link: https://lore.kernel.org/linux-i2c/20210330134633.29889-1-andriy.shevche...@linux.intel.com/ Change since v3: - split the bus mode string api to I2C as suggested by Andy - simplify the devm variants and change the export format - address the comments of the HiSilicon I2C driver from Andy and Dmitry, thanks! Link: https://lore.kernel.org/linux-i2c/1616411413-7177-1-git-send-email-yangyic...@hisilicon.com/ Change since v2: - handle -EPROBE_DEFER case when get irq number by platform_get_irq() Link: https://lore.kernel.org/linux-i2c/1615296137-14558-1-git-send-email-yangyic...@hisilicon.com/ Change since v1: - fix compile test error on 32bit arch, reported by intel lkp robot: 64 bit division without using kernel wrapper in probe function. Link:https://lore.kernel.org/linux-i2c/1615016946-55670-1-git-send-email-yangyic...@hisilicon.com/ Andy Shevchenko (1): i2c: designware: Switch over to i2c_freq_mode_string() Yicong Yang (4): i2c: core: add managed function for adding i2c adapters i2c: core: add api to provide frequency mode strings i2c: add support for HiSilicon I2C controller MAINTAINERS: Add maintainer for HiSilicon I2C driver MAINTAINERS| 7 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile| 1 + drivers/i2c/busses/i2c-designware-master.c | 20 +- drivers/i2c/busses/i2c-hisi.c | 505 + drivers/i2c/i2c-core-base.c| 26 ++ include/linux/i2c.h| 21 ++ 7 files changed, 574 insertions(+), 16 deletions(-) create mode 100644 drivers/i2c/busses/i2c-hisi.c -- 2.8.1
[PATCH v6 3/5] i2c: add support for HiSilicon I2C controller
Add HiSilicon I2C controller driver for the Kunpeng SoC. It provides the access to the i2c busses, which connects to the eeprom, rtc, etc. The driver works with IRQ mode, and supports basic I2C features and 10bit address. The DMA is not supported. Reviewed-by: Andy Shevchenko Reviewed-by: Dmitry Osipenko Signed-off-by: Yicong Yang --- drivers/i2c/busses/Kconfig| 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-hisi.c | 505 ++ 3 files changed, 516 insertions(+) create mode 100644 drivers/i2c/busses/i2c-hisi.c diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 05ebf75..eddf7bf 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -645,6 +645,16 @@ config I2C_HIGHLANDER This driver can also be built as a module. If so, the module will be called i2c-highlander. +config I2C_HISI + tristate "HiSilicon I2C controller" + depends on ARM64 || COMPILE_TEST + help + Say Y here if you want to have Hisilicon I2C controller support + available on the Kunpeng Server. + + This driver can also be built as a module. If so, the module + will be called i2c-hisi. + config I2C_IBM_IIC tristate "IBM PPC 4xx on-chip I2C interface" depends on 4xx diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 615f35e..e1c9292 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o +obj-$(CONFIG_I2C_HISI) += i2c-hisi.o obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o obj-$(CONFIG_I2C_IMG) += i2c-img-scb.o diff --git a/drivers/i2c/busses/i2c-hisi.c b/drivers/i2c/busses/i2c-hisi.c new file mode 100644 index 000..4284935 --- /dev/null +++ b/drivers/i2c/busses/i2c-hisi.c @@ -0,0 +1,505 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * HiSilicon I2C Controller Driver for Kunpeng SoC + * + * Copyright (c) 2021 HiSilicon Technologies Co., Ltd. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HISI_I2C_FRAME_CTRL0x +#define HISI_I2C_FRAME_CTRL_SPEED_MODE GENMASK(1, 0) +#define HISI_I2C_FRAME_CTRL_ADDR_TEN BIT(2) +#define HISI_I2C_SLV_ADDR 0x0004 +#define HISI_I2C_SLV_ADDR_VALGENMASK(9, 0) +#define HISI_I2C_SLV_ADDR_GC_S_MODE BIT(10) +#define HISI_I2C_SLV_ADDR_GC_S_ENBIT(11) +#define HISI_I2C_CMD_TXDATA0x0008 +#define HISI_I2C_CMD_TXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_CMD_TXDATA_RW BIT(8) +#define HISI_I2C_CMD_TXDATA_P_EN BIT(9) +#define HISI_I2C_CMD_TXDATA_SR_ENBIT(10) +#define HISI_I2C_RXDATA0x000c +#define HISI_I2C_RXDATA_DATA GENMASK(7, 0) +#define HISI_I2C_SS_SCL_HCNT 0x0010 +#define HISI_I2C_SS_SCL_LCNT 0x0014 +#define HISI_I2C_FS_SCL_HCNT 0x0018 +#define HISI_I2C_FS_SCL_LCNT 0x001c +#define HISI_I2C_HS_SCL_HCNT 0x0020 +#define HISI_I2C_HS_SCL_LCNT 0x0024 +#define HISI_I2C_FIFO_CTRL 0x0028 +#define HISI_I2C_FIFO_RX_CLR BIT(0) +#define HISI_I2C_FIFO_TX_CLR BIT(1) +#define HISI_I2C_FIFO_RX_AF_THRESH GENMASK(7, 2) +#define HISI_I2C_FIFO_TX_AE_THRESH GENMASK(13, 8) +#define HISI_I2C_FIFO_STATE0x002c +#define HISI_I2C_FIFO_STATE_RX_RERR BIT(0) +#define HISI_I2C_FIFO_STATE_RX_WERR BIT(1) +#define HISI_I2C_FIFO_STATE_RX_EMPTY BIT(3) +#define HISI_I2C_FIFO_STATE_TX_RERR BIT(6) +#define HISI_I2C_FIFO_STATE_TX_WERR BIT(7) +#define HISI_I2C_FIFO_STATE_TX_FULL BIT(11) +#define HISI_I2C_SDA_HOLD 0x0030 +#define HISI_I2C_SDA_HOLD_TX GENMASK(15, 0) +#define HISI_I2C_SDA_HOLD_RX GENMASK(23, 16) +#define HISI_I2C_FS_SPK_LEN0x0038 +#define HISI_I2C_FS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_HS_SPK_LEN0x003c +#define HISI_I2C_HS_SPK_LEN_CNT GENMASK(7, 0) +#define HISI_I2C_INT_MSTAT 0x0044 +#define HISI_I2C_INT_CLR 0x0048 +#define HISI_I2C_INT_MASK 0x004C +#define HISI_I2C_TRANS_STATE 0x0050 +#define HISI_I2C_TRANS_ERR 0x0054 +#define HISI_I2C_VERSION 0x0058 + +#define HISI_I2C_INT_ALL GENMASK(4, 0) +#define HISI_I2C_INT_TRANS_CPLTBIT(0) +#define HISI_I2C_INT_TRANS_ERR BIT(1) +#define HISI_I2C_INT_FIFO_ERR BIT(2) +#define HISI_I2C_INT_RX_FULL BIT(3) +#define HISI_I2C_INT_TX_EMPTY BIT(4) +#define HISI_I2C_INT_ERR \ + (HISI_I2C_INT_TRANS_ERR | HISI_I2C_INT_FIFO_ERR) + +#define HISI_I2C_STD_SPEED_M
[PATCH v6 2/5] i2c: core: add api to provide frequency mode strings
Some I2C drivers like Designware and HiSilicon will print the
bus frequency mode information, so add a public one that everyone
can make use of.
Tested-by: Jarkko Nikula
Reviewed-by: Jarkko Nikula
Reviewed-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
include/linux/i2c.h | 20
1 file changed, 20 insertions(+)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 10bd0b0..7268180 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
#define I2C_MAX_HIGH_SPEED_MODE_FREQ 340
#define I2C_MAX_ULTRA_FAST_MODE_FREQ 500
+static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
+{
+ switch (bus_freq_hz) {
+ case I2C_MAX_STANDARD_MODE_FREQ:
+ return "Standard Mode (100 kHz)";
+ case I2C_MAX_FAST_MODE_FREQ:
+ return "Fast Mode (400 kHz)";
+ case I2C_MAX_FAST_MODE_PLUS_FREQ:
+ return "Fast Mode Plus (1.0 MHz)";
+ case I2C_MAX_TURBO_MODE_FREQ:
+ return "Turbo Mode (1.4 MHz)";
+ case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+ return "High Speed Mode (3.4 MHz)";
+ case I2C_MAX_ULTRA_FAST_MODE_FREQ:
+ return "Ultra Fast Mode (5.0 MHz)";
+ default:
+ return "Unknown Mode";
+ }
+}
+
struct module;
struct property_entry;
--
2.8.1
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On 2021/4/15 16:18, Yicong Yang wrote:
> On 2021/4/15 2:06, Geert Uytterhoeven wrote:
>> Hi Yicong,
>>
>> On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang
>> wrote:
>>> On 2021/4/13 20:26, Geert Uytterhoeven wrote:
>>>> The HiSilicon Kunpeng I2C controller is only present on HiSilicon
>>>> Kunpeng SoCs, and its driver relies on ACPI to probe for its presence.
>>>> Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user
>>>> about this driver when configuring a kernel without Hisilicon platform
>>>> or ACPI firmware support.
>>>
>>> this is a public IP which doesn't specifically depend on ARCH_HISI. I'm
>>> not sure all the platform this IP on has ARCH_HISI configured. The driver
>>> will not be compiled by default config. This is not correct to have
>>> this dependence.
>>
>> Thanks for your answer!
>>
>> I guess it's still fine to add a dependency on ACPI?
>
> yes. currently we only use this driver through ACPI. So at least
> for this driver, it make sense to keep the dependency.
>
sorry. i was a little mess about this. I dropped this in [1].
so just keep it as is.
[1] https://lore.kernel.org/linux-i2c/ygmntyt2iz72w...@smile.fi.intel.com/
>>
>> Thanks again!
>>
>>>> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller")
>>>> Signed-off-by: Geert Uytterhoeven
>>>> ---
>>>> drivers/i2c/busses/Kconfig | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>>> index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644
>>>> --- a/drivers/i2c/busses/Kconfig
>>>> +++ b/drivers/i2c/busses/Kconfig
>>>> @@ -647,7 +647,7 @@ config I2C_HIGHLANDER
>>>>
>>>> config I2C_HISI
>>>> tristate "HiSilicon I2C controller"
>>>> - depends on ARM64 || COMPILE_TEST
>>>> + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST
>>>> help
>>>> Say Y here if you want to have Hisilicon I2C controller support
>>>> available on the Kunpeng Server.
>> \
>> Gr{oetje,eeting}s,
>>
>> Geert
>>
>
>
> .
>
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On 2021/4/13 20:26, Geert Uytterhoeven wrote:
> The HiSilicon Kunpeng I2C controller is only present on HiSilicon
> Kunpeng SoCs, and its driver relies on ACPI to probe for its presence.
> Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user
> about this driver when configuring a kernel without Hisilicon platform
> or ACPI firmware support.
>
this is a public IP which doesn't specifically depend on ARCH_HISI. I'm
not sure all the platform this IP on has ARCH_HISI configured. The driver
will not be compiled by default config. This is not correct to have
this dependence.
> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller")
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -647,7 +647,7 @@ config I2C_HIGHLANDER
>
> config I2C_HISI
> tristate "HiSilicon I2C controller"
> - depends on ARM64 || COMPILE_TEST
> + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST
> help
> Say Y here if you want to have Hisilicon I2C controller support
> available on the Kunpeng Server.
>
Re: [PATCH] i2c: I2C_HISI should depend on ARCH_HISI && ACPI
On 2021/4/15 2:06, Geert Uytterhoeven wrote:
> Hi Yicong,
>
> On Wed, Apr 14, 2021 at 11:24 AM Yicong Yang wrote:
>> On 2021/4/13 20:26, Geert Uytterhoeven wrote:
>>> The HiSilicon Kunpeng I2C controller is only present on HiSilicon
>>> Kunpeng SoCs, and its driver relies on ACPI to probe for its presence.
>>> Hence add dependencies on ARCH_HISI and ACPI, to prevent asking the user
>>> about this driver when configuring a kernel without Hisilicon platform
>>> or ACPI firmware support.
>>
>> this is a public IP which doesn't specifically depend on ARCH_HISI. I'm
>> not sure all the platform this IP on has ARCH_HISI configured. The driver
>> will not be compiled by default config. This is not correct to have
>> this dependence.
>
> Thanks for your answer!
>
> I guess it's still fine to add a dependency on ACPI?
yes. currently we only use this driver through ACPI. So at least
for this driver, it make sense to keep the dependency.
>
> Thanks again!
>
>>> Fixes: d62fbdb99a85730a ("i2c: add support for HiSilicon I2C controller")
>>> Signed-off-by: Geert Uytterhoeven
>>> ---
>>> drivers/i2c/busses/Kconfig | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>> index b5b4e0d0ff4dd0bc..3ead6d9e130b2ebc 100644
>>> --- a/drivers/i2c/busses/Kconfig
>>> +++ b/drivers/i2c/busses/Kconfig
>>> @@ -647,7 +647,7 @@ config I2C_HIGHLANDER
>>>
>>> config I2C_HISI
>>> tristate "HiSilicon I2C controller"
>>> - depends on ARM64 || COMPILE_TEST
>>> + depends on (ARM64 && ARCH_HISI && ACPI) || COMPILE_TEST
>>> help
>>> Say Y here if you want to have Hisilicon I2C controller support
>>> available on the Kunpeng Server.
> \
> Gr{oetje,eeting}s,
>
> Geert
>
[PATCH 2/4] hwtracing: Add tune function support for HiSilicon PCIe Tune and Trace device
Add tune function for the HiSilicon Tune and Trace device. The interface
of tune is also exposed through debugfs.
Signed-off-by: Yicong Yang
---
drivers/hwtracing/hisilicon/hisi_ptt.c | 187 +
1 file changed, 187 insertions(+)
diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c
b/drivers/hwtracing/hisilicon/hisi_ptt.c
index a1feece..2e3631b 100644
--- a/drivers/hwtracing/hisilicon/hisi_ptt.c
+++ b/drivers/hwtracing/hisilicon/hisi_ptt.c
@@ -72,6 +72,7 @@ struct hisi_ptt_debugfs_file_desc {
struct hisi_ptt *hisi_ptt;
const char *name;
const struct file_operations *fops;
+ struct hisi_ptt_tune_event_desc *private;
};
#define PTT_FILE_INIT(__name, __fops) \
@@ -85,6 +86,15 @@ struct hisi_ptt_trace_event_desc {
#define PTT_TRACE_EVENT_INIT(__name, __event_code) \
{ .name = __name, .event_code = __event_code }
+struct hisi_ptt_tune_event_desc {
+ const char *name;
+ u32 event_code;
+ int (*set)(struct hisi_ptt *hisi_ptt, void *data);
+ int (*get)(struct hisi_ptt *hisi_ptt, void *data);
+};
+#define PTT_TUNE_EVENT_INIT(__name, __event_code, __set, __get)\
+ { .name = __name, .event_code = __event_code, .set = __set, .get =
__get }
+
enum hisi_ptt_trace_rxtx {
HISI_PTT_TRACE_RX = 0,
HISI_PTT_TRACE_TX,
@@ -217,6 +227,8 @@ struct hisi_ptt {
bool is_port;
u16 val;
} cur_filter;
+
+ u32 tune_event;
};
static int hisi_ptt_write_to_buffer(void *buf, size_t len, loff_t *pos,
@@ -234,6 +246,131 @@ static int hisi_ptt_write_to_buffer(void *buf, size_t
len, loff_t *pos,
return 0;
}
+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, void *data)
+{
+ u32 *val = data, 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,
+ hisi_ptt->tune_event);
+ writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
+ /* Write all 1 to indicates it's the read process */
+ writel(HISI_PTT_TUNING_DATA_VAL_MASK,
+ hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
+
+ if (hisi_ptt_wait_tuning_finish(hisi_ptt)) {
+ pci_err(hisi_ptt->pdev, "failed to read tune data, device
timeout.\n");
+ return -ETIMEDOUT;
+ }
+
+ *val = readl(hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
+ *val &= HISI_PTT_TUNING_DATA_VAL_MASK;
+
+ return 0;
+}
+
+static int hisi_ptt_tune_data_set(struct hisi_ptt *hisi_ptt, void *data)
+{
+ u32 *val = data, 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,
+ hisi_ptt->tune_event);
+ writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
+ *val &= HISI_PTT_TUNING_DATA_VAL_MASK;
+ writel(*val, hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
+
+ if (hisi_ptt_wait_tuning_finish(hisi_ptt)) {
+ pci_err(hisi_ptt->pdev, "failed to write tune data, device
timeout.\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static ssize_t hisi_ptt_tune_common_read(struct file *filp, char __user *buf,
+size_t count, loff_t *pos)
+{
+ struct hisi_ptt_debugfs_file_desc *desc = filp->private_data;
+ struct hisi_ptt *hisi_ptt = desc->hisi_ptt;
+ struct hisi_ptt_tune_event_desc *e_desc = desc->private;
+ char tbuf[HISI_PTT_CTRL_STR_LEN];
+ int len, ret;
+ u32 val;
+
+ if (!mutex_trylock(&hisi_ptt->mutex))
+ return -EBUSY;
+
+ hisi_ptt->tune_event = e_desc->event_code;
+ ret = e_desc->get(hisi_ptt, &val);
+ mutex_unlock(&hisi_ptt->mutex);
+ if (ret)
+ return ret;
+
+ len = snprintf(tbuf, HISI_PTT_CTRL_STR_LEN, "%d\n", val);
+
+ return simple_read_from_buffer(buf, count, pos, tbuf, len);
+}
+
+static ssize_t hisi_ptt_tune_common_write(struct file *filp,
+ const char __user *buf,
+ size_t count, loff_t *pos)
+{
+ struct hisi_ptt_debugfs_file_desc *desc = filp->private_data;
+
[PATCH 1/4] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex integrated Endpoint(RCiEP) device, providing the capability to dynamically monitor and tune the PCIe traffic(tune), and trace the TLP headers(trace). Add the driver for the device to enable the trace function. The driver will create debugfs directory for each PTT device, and users can operate the device through the files under its directory. Signed-off-by: Yicong Yang --- drivers/Makefile |1 + drivers/hwtracing/Kconfig |2 + drivers/hwtracing/hisilicon/Kconfig|8 + drivers/hwtracing/hisilicon/Makefile |2 + drivers/hwtracing/hisilicon/hisi_ptt.c | 1449 5 files changed, 1462 insertions(+) create mode 100644 drivers/hwtracing/hisilicon/Kconfig create mode 100644 drivers/hwtracing/hisilicon/Makefile create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c diff --git a/drivers/Makefile b/drivers/Makefile index 6fba7da..9a0f76d 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -174,6 +174,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf/ obj-$(CONFIG_RAS) += ras/ obj-$(CONFIG_USB4) += thunderbolt/ obj-$(CONFIG_CORESIGHT)+= hwtracing/coresight/ +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ obj-y += hwtracing/intel_th/ obj-$(CONFIG_STM) += hwtracing/stm/ obj-$(CONFIG_ANDROID) += android/ diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig index 1308583..e3796b1 100644 --- a/drivers/hwtracing/Kconfig +++ b/drivers/hwtracing/Kconfig @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" source "drivers/hwtracing/intel_th/Kconfig" +source "drivers/hwtracing/hisilicon/Kconfig" + endmenu diff --git a/drivers/hwtracing/hisilicon/Kconfig b/drivers/hwtracing/hisilicon/Kconfig new file mode 100644 index 000..e4c2379 --- /dev/null +++ b/drivers/hwtracing/hisilicon/Kconfig @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0-only +config HISI_PTT + tristate "HiSilicon PCIe Tune and Trace Device" + depends on (ARM64 && PCI && HAS_DMA && HAS_IOMEM && DEBUG_FS) || COMPILE_TEST + help + HiSilicon PCIe Tune and Trace Device exist as a PCIe iEP + device, provides support for PCIe traffic tuning and + tracing TLP headers to the memory. diff --git a/drivers/hwtracing/hisilicon/Makefile b/drivers/hwtracing/hisilicon/Makefile new file mode 100644 index 000..908c09a --- /dev/null +++ b/drivers/hwtracing/hisilicon/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c b/drivers/hwtracing/hisilicon/hisi_ptt.c new file mode 100644 index 000..a1feece --- /dev/null +++ b/drivers/hwtracing/hisilicon/hisi_ptt.c @@ -0,0 +1,1449 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for HiSilicon PCIe tune and trace device + * + * Copyright (c) 2021 HiSilicon Limited. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HISI_PTT_CTRL_STR_LEN 40 +#define HISI_PTT_DEFAULT_TRACE_BUF_CNT 64 +#define HISI_PTT_FILTER_UPDATE_FIFO_SIZE 16 + +#define HISI_PTT_RESET_WAIT_MS 1000UL +#define HISI_PTT_WORK_DELAY_MS 100UL +#define HISI_PTT_WAIT_TIMEOUT_US 100UL +#define HISI_PTT_WAIT_POLL_INTERVAL_US 100UL + +#define HISI_PTT_IRQ_NUMS 1 +#define HISI_PTT_DMA_IRQ 0 +#define HISI_PTT_DMA_NUMS 4 + +#define HISI_PTT_TUNING_CTRL 0x +#define HISI_PTT_TUNING_CTRL_CODEGENMASK(15, 0) +#define HISI_PTT_TUNING_CTRL_SUB GENMASK(23, 16) +#define HISI_PTT_TUNING_CTRL_CHN GENMASK(31, 24) +#define HISI_PTT_TUNING_DATA 0x0004 +#define HISI_PTT_TUNING_DATA_VAL_MASKGENMASK(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 +#define HISI_PTT_TRACE_ADDR_STRIDE 0x8 +#define HISI_PTT_TRACE_CTRL0x0850 +#define HISI_PTT_TRACE_CTRL_EN BIT(0) +#define HISI_PTT_TRACE_CTRL_RST BIT(1) +#define HISI_PTT_TRACE_CTRL_RXTX_SEL GENMASK(3, 2) +#define HISI_PTT_TRACE_CTRL_TYPE_SEL GENMASK(7, 4) +#define HISI_PTT_TRACE_CTRL_DATA_FORMAT BIT(14) +#define HISI_PTT_TRACE_CTRL_FILTER_MODE BIT(15) +#define HISI_PTT_TRACE_CTRL_TARGET_SEL GENMASK(31, 16) +#define HISI_PTT_TRACE_INT_STAT0x0890 +#define HISI_PTT_TRACE_INT_STAT_MASK GENMASK(3, 0) +#define HISI_PTT_TRACE_INT_MASK_REG0x0894 +#define HISI_PTT_TUNING_INT_STAT 0x0898 +#define HISI_PTT_TUNING_INT_STAT_MASKBIT(0) +#define HISI_PTT_TUNING_INT_MASK 0x089c +#define HISI_PTT_TRACE_WR_STS
[PATCH 3/4] docs: Add documentation for HiSilicon PTT device driver
Document the introduction and usage of HiSilicon PTT device driver. Signed-off-by: Yicong Yang --- Documentation/trace/hisi-ptt.rst | 316 +++ 1 file changed, 316 insertions(+) create mode 100644 Documentation/trace/hisi-ptt.rst diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst new file mode 100644 index 000..215676f --- /dev/null +++ b/Documentation/trace/hisi-ptt.rst @@ -0,0 +1,316 @@ +.. SPDX-License-Identifier: GPL-2.0 + +== +HiSilicon PCIe Tune and Trace device +== + +Introduction + + +HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex +integrated Endpoint (RCiEP) device, providing the capability +to dynamically monitor and tune the PCIe link's events (tune), +and trace the TLP headers (trace). The two functions are independent, +but is recommended to use them together to analyze and enhance the +PCIe link's performance. + +On Kunpeng 930 SoC, the PCIe root complex is composed of several +PCIe cores. +Each core is composed of several root ports, RCiEPs, and one +PTT device, like below. The PTT device is capable of tuning and +tracing the link of the PCIe core. +:: + +--Core 0---+ + | | [ PTT ] | + | | [Root Port]---[Endpoint] + | | [Root Port]---[Endpoint] + | | [Root Port]---[Endpoint] +Root Complex |--Core 1---+ + | | [ PTT ] | + | | [Root Port]---[ Switch ]---[Endpoint] + | | [Root Port]---[Endpoint] `-[Endpoint] + | | [Root Port]---[Endpoint] + +---+ + +The PTT device driver cannot be loaded if debugfs is not mounted. +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt +as its root directory, with name of its BDF number. +:: + +/sys/kernel/debug/hisi_ptt/::. + +Tune + + +PTT tune is designed for monitoring and adjusting PCIe link parameters(events). +Currently we support events in 4 classes. The scope of the events +covers the PCIe core with which the PTT device belongs to. + +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and +mostly a simple open/read/write/close cycle will be used to tune +the event. +:: +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune +$ ls +qos_tx_cplqos_tx_npqos_tx_p +tx_path_rx_req_alloc_buf_level +tx_path_tx_req_alloc_buf_level +$ cat qos_tx_dp +1 +$ echo 2 > qos_tx_dp +$ cat qos_tx_dp +2 + +Current value(numerical value) of the event can be simply read +from the file, and the desired value written to the file to tune. +Tuning multiple events at the same time is not permitted, which means +you cannot read or write more than one tune file at one time. + +1. Tx path QoS control + + +Following files are provided to tune the QoS of the tx path of the PCIe core. + +- qos_tx_cpl: weight of tx completion TLPs +- qos_tx_np: weight of tx non-posted TLPs +- qos_tx_p: weight of tx posted TLPs + +The weight influences the proportion of certain packets on the PCIe link. +For example, for the storage scenario, increase the proportion +of the completion packets on the link to enhance the performance as +more completions are consumed. + +The available tune data of these events is [0, 1, 2]. +Writing a negative value will return an error, and out of range +values will be converted to 2. Note that the event value just +indicates a probable level, but is not precise. + +2. Tx path buffer control +- + +Following files are provided to tune the buffer of tx path of the PCIe core. + +- tx_path_rx_req_alloc_buf_level: watermark of RX requested +- tx_path_tx_req_alloc_buf_level: watermark of TX requested + +These events influence the watermark of the buffer allocated for each +type. RX means the inbound while Tx means outbound. For a busy +direction, you should increase the related buffer watermark to enhance +the performance. + +The available tune data of above events is [0, 1, 2]. +Writing a negative value will return an error, and out of range +values will be converted to 2. Note that the event value just +indicates a probable level, but is not precise. + +Trace += + +PTT trace is designed for dumping the TLP headers to the memory, which +can be used to analyze the transactions and usage condition of the PCIe +Link. You can chose to filter the traced headers by either requester ID, +or those downstream of a set of root ports on the same core of the PTT +device. It's also support to trace the headers of certain type and of +certain direction. + +In order to start trace, you need to configure the parameters first. +The parameters files is provided under $(PTT root dir)/$(BDF)/trace. +:: +$ cd /sys/kernel/debug/his
[PATCH 0/4] Add support for HiSilicon PCIe Tune and Trace device
HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex integrated Endpoint(RCiEP) device, providing the capability to dynamically monitor and tune the PCIe traffic(tune), and trace the TLP headers(trace). The driver exposes the user interface through debugfs, so no need for extra user space tools. The usage is described in the document. Yicong Yang (4): hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device hwtracing: Add tune function support for HiSilicon PCIe Tune and Trace device docs: Add documentation for HiSilicon PTT device driver MAINTAINERS: Add maintainer for HiSilicon PTT driver Documentation/trace/hisi-ptt.rst | 316 ++ MAINTAINERS|7 + drivers/Makefile |1 + drivers/hwtracing/Kconfig |2 + drivers/hwtracing/hisilicon/Kconfig|8 + drivers/hwtracing/hisilicon/Makefile |2 + drivers/hwtracing/hisilicon/hisi_ptt.c | 1636 7 files changed, 1972 insertions(+) create mode 100644 Documentation/trace/hisi-ptt.rst create mode 100644 drivers/hwtracing/hisilicon/Kconfig create mode 100644 drivers/hwtracing/hisilicon/Makefile create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c -- 2.8.1
[PATCH 4/4] MAINTAINERS: Add maintainer for HiSilicon PTT driver
Add maintainer for driver and documentation of HiSilicon PTT device. Signed-off-by: Yicong Yang --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8d23b0e..61b793b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8078,6 +8078,13 @@ W: http://www.hisilicon.com F: Documentation/admin-guide/perf/hisi-pmu.rst F: drivers/perf/hisilicon +HISILICON PTT DRIVER +M: Yicong Yang +L: linux-kernel@vger.kernel.org +S: Maintained +F: Documentation/trace/hisi-ptt.rst +F: drivers/hwtracing/hisilicon/ + HISILICON QM AND ZIP Controller DRIVER M: Zhou Wang L: linux-cry...@vger.kernel.org -- 2.8.1
Re: [PATCH v6 3/5] i2c: add support for HiSilicon I2C controller
On 2021/4/7 4:02, Wolfram Sang wrote:
>
> Only super minor stuff. Thanks to all the contributors and reviewers!
>
>> +#define HZ_PER_KHZ 1000
>
> KHZ_PER_HZ?
that doesn't match what we want. we want the count of HZs per one KHZ.
>
>> +ret = devm_i2c_add_adapter(dev, adapter);
>> +if (ret) {
>> +dev_err(dev, "failed to add i2c adapter, ret = %d\n", ret);
>
> No need to print that. The core prints messages on failures.
>
i check the code and find a little difference. the print in the core doesn't
have the device prefix so it may not tell us which device's registeration fails.
but considering we have device name embedded in the adapter name for this
driver, it's ok to remove this message.
Re: [PATCH v6 2/5] i2c: core: add api to provide frequency mode strings
On 2021/4/7 3:54, Wolfram Sang wrote:
>
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 10bd0b0..7268180 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>> #define I2C_MAX_HIGH_SPEED_MODE_FREQ340
>> #define I2C_MAX_ULTRA_FAST_MODE_FREQ500
>>
>> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
>> +{
>> +switch (bus_freq_hz) {
>> +case I2C_MAX_STANDARD_MODE_FREQ:
>> +return "Standard Mode (100 kHz)";
>> +case I2C_MAX_FAST_MODE_FREQ:
>> +return "Fast Mode (400 kHz)";
>> +case I2C_MAX_FAST_MODE_PLUS_FREQ:
>> +return "Fast Mode Plus (1.0 MHz)";
>> +case I2C_MAX_TURBO_MODE_FREQ:
>> +return "Turbo Mode (1.4 MHz)";
>> +case I2C_MAX_HIGH_SPEED_MODE_FREQ:
>> +return "High Speed Mode (3.4 MHz)";
>> +case I2C_MAX_ULTRA_FAST_MODE_FREQ:
>> +return "Ultra Fast Mode (5.0 MHz)";
>> +default:
>> +return "Unknown Mode";
>> +}
>> +}
>
> Any reason ehy this is an inline function? My gut feeling says it would
> be better added to the core?
>
it's not a complicated function so i didn't think it'll make much difference,
so i just put it in the header along with the coresponding macro definitions.
do you want me to move it to the core?
Thanks
Re: [PATCH 0/4] Add support for HiSilicon PCIe Tune and Trace device
On 2021/4/6 21:49, Greg KH wrote: > On Tue, Apr 06, 2021 at 08:45:50PM +0800, Yicong Yang wrote: >> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex >> integrated Endpoint(RCiEP) device, providing the capability >> to dynamically monitor and tune the PCIe traffic(tune), >> and trace the TLP headers(trace). The driver exposes the user >> interface through debugfs, so no need for extra user space tools. >> The usage is described in the document. > > Why use debugfs and not the existing perf tools for debugging? > The perf doesn't match our device as we've analyzed. For the tune function it doesn't do the sampling at all. User specifys one link parameter and reads its current value or set the desired one. The process is static. We didn't find a way to adapt to perf. For the trace function, we may barely adapt to the perf framework but it doesn't seems like a better choice. We have our own format of data and don't need perf doing the parsing, and we'll get extra information added by perf as well. The settings through perf tools won't satisfy our needs, we cannot present available settings (filter BDF number, TLP types, buffer controls) to the user and user cannot set in a friendly way. For example, we cannot count on perf to decode the usual format BDF number like ::., which user can use filter the TLP headers. So we intended to make the operation of this driver a bit like ftrace. user sets the control settings through control files and get the result through files as well. No additional tools is necessay. A user space tool is necessary if we use a character device or misc device for implementing this. The trace data maybe hundreds of megabytes, and debugfs file can just satisfy it. Thanks, Yicong
Re: [PATCH v6 3/5] i2c: add support for HiSilicon I2C controller
On 2021/4/7 16:32, Jarkko Nikula wrote:
> Hi
>
> On 3/31/21 4:36 PM, Yicong Yang wrote:
>> + ret = device_property_read_u64(dev, "clk_rate", &ctlr->clk_rate_khz);
>> + if (ret) {
>> + dev_err(dev, "failed to get clock frequency, ret = %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ctlr->clk_rate_khz = DIV_ROUND_UP_ULL(ctlr->clk_rate_khz, HZ_PER_KHZ);
>> +
>
> I'd use a temp variable here for reading the "clk_rate" property in Hertz and
> calculating the derived kHz value from it. As a bonus allow to use u32 for
> clk_rate_khz instead of u64. u32 will still provide plenty of headroom :-)
>
> Reason for temp variable is for me it's confusing to see statement like
> "rate_khz = rate_khz / 1000".
>
I can get this addressed in the updated version. Thanks for the suggestion!
Re: [PATCH v6 2/5] i2c: core: add api to provide frequency mode strings
On 2021/4/7 18:08, Andy Shevchenko wrote:
> On Wed, Apr 07, 2021 at 04:29:29PM +0800, Yicong Yang wrote:
>> On 2021/4/7 3:54, Wolfram Sang wrote:
>>>
>>>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>>>> index 10bd0b0..7268180 100644
>>>> --- a/include/linux/i2c.h
>>>> +++ b/include/linux/i2c.h
>>>> @@ -47,6 +47,26 @@ typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
>>>> #define I2C_MAX_HIGH_SPEED_MODE_FREQ 340
>>>> #define I2C_MAX_ULTRA_FAST_MODE_FREQ 500
>>>>
>>>> +static inline const char *i2c_freq_mode_string(u32 bus_freq_hz)
>>>> +{
>>>> + switch (bus_freq_hz) {
>>>> + case I2C_MAX_STANDARD_MODE_FREQ:
>>>> + return "Standard Mode (100 kHz)";
>>>> + case I2C_MAX_FAST_MODE_FREQ:
>>>> + return "Fast Mode (400 kHz)";
>>>> + case I2C_MAX_FAST_MODE_PLUS_FREQ:
>>>> + return "Fast Mode Plus (1.0 MHz)";
>>>> + case I2C_MAX_TURBO_MODE_FREQ:
>>>> + return "Turbo Mode (1.4 MHz)";
>>>> + case I2C_MAX_HIGH_SPEED_MODE_FREQ:
>>>> + return "High Speed Mode (3.4 MHz)";
>>>> + case I2C_MAX_ULTRA_FAST_MODE_FREQ:
>>>> + return "Ultra Fast Mode (5.0 MHz)";
>>>> + default:
>>>> + return "Unknown Mode";
>>>> + }
>>>> +}
>>>
>>> Any reason ehy this is an inline function? My gut feeling says it would
>>> be better added to the core?
>>>
>>
>> it's not a complicated function so i didn't think it'll make much difference,
>> so i just put it in the header along with the coresponding macro definitions.
>> do you want me to move it to the core?
>
> I guess exporting will save few dozens of bytes if the function is used more
> than once. (All strings will be duplicated or multiplied in that case)
>
yes, that's one concern. since we don't need this to perform fast, an inline
one maybe unnecessary.
[PATCH RESEND 4/4] MAINTAINERS: Add maintainer for HiSilicon PTT driver
Add maintainer for driver and documentation of HiSilicon PTT device. Signed-off-by: Yicong Yang --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 7fdc513..647f0bf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8084,6 +8084,13 @@ W: http://www.hisilicon.com F: Documentation/admin-guide/perf/hisi-pmu.rst F: drivers/perf/hisilicon +HISILICON PTT DRIVER +M: Yicong Yang +L: linux-kernel@vger.kernel.org +S: Maintained +F: Documentation/trace/hisi-ptt.rst +F: drivers/hwtracing/hisilicon/ + HISILICON QM AND ZIP Controller DRIVER M: Zhou Wang L: linux-cry...@vger.kernel.org -- 2.8.1
[PATCH RESEND 2/4] hwtracing: Add tune function support for HiSilicon PCIe Tune and Trace device
Add tune function for the HiSilicon Tune and Trace device. The interface
of tune is also exposed through debugfs.
Signed-off-by: Yicong Yang
---
drivers/hwtracing/hisilicon/hisi_ptt.c | 187 +
1 file changed, 187 insertions(+)
diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c
b/drivers/hwtracing/hisilicon/hisi_ptt.c
index a1feece..2e3631b 100644
--- a/drivers/hwtracing/hisilicon/hisi_ptt.c
+++ b/drivers/hwtracing/hisilicon/hisi_ptt.c
@@ -72,6 +72,7 @@ struct hisi_ptt_debugfs_file_desc {
struct hisi_ptt *hisi_ptt;
const char *name;
const struct file_operations *fops;
+ struct hisi_ptt_tune_event_desc *private;
};
#define PTT_FILE_INIT(__name, __fops) \
@@ -85,6 +86,15 @@ struct hisi_ptt_trace_event_desc {
#define PTT_TRACE_EVENT_INIT(__name, __event_code) \
{ .name = __name, .event_code = __event_code }
+struct hisi_ptt_tune_event_desc {
+ const char *name;
+ u32 event_code;
+ int (*set)(struct hisi_ptt *hisi_ptt, void *data);
+ int (*get)(struct hisi_ptt *hisi_ptt, void *data);
+};
+#define PTT_TUNE_EVENT_INIT(__name, __event_code, __set, __get)\
+ { .name = __name, .event_code = __event_code, .set = __set, .get =
__get }
+
enum hisi_ptt_trace_rxtx {
HISI_PTT_TRACE_RX = 0,
HISI_PTT_TRACE_TX,
@@ -217,6 +227,8 @@ struct hisi_ptt {
bool is_port;
u16 val;
} cur_filter;
+
+ u32 tune_event;
};
static int hisi_ptt_write_to_buffer(void *buf, size_t len, loff_t *pos,
@@ -234,6 +246,131 @@ static int hisi_ptt_write_to_buffer(void *buf, size_t
len, loff_t *pos,
return 0;
}
+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, void *data)
+{
+ u32 *val = data, 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,
+ hisi_ptt->tune_event);
+ writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
+ /* Write all 1 to indicates it's the read process */
+ writel(HISI_PTT_TUNING_DATA_VAL_MASK,
+ hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
+
+ if (hisi_ptt_wait_tuning_finish(hisi_ptt)) {
+ pci_err(hisi_ptt->pdev, "failed to read tune data, device
timeout.\n");
+ return -ETIMEDOUT;
+ }
+
+ *val = readl(hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
+ *val &= HISI_PTT_TUNING_DATA_VAL_MASK;
+
+ return 0;
+}
+
+static int hisi_ptt_tune_data_set(struct hisi_ptt *hisi_ptt, void *data)
+{
+ u32 *val = data, 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,
+ hisi_ptt->tune_event);
+ writel(reg, hisi_ptt->iobase + HISI_PTT_TUNING_CTRL);
+ *val &= HISI_PTT_TUNING_DATA_VAL_MASK;
+ writel(*val, hisi_ptt->iobase + HISI_PTT_TUNING_DATA);
+
+ if (hisi_ptt_wait_tuning_finish(hisi_ptt)) {
+ pci_err(hisi_ptt->pdev, "failed to write tune data, device
timeout.\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static ssize_t hisi_ptt_tune_common_read(struct file *filp, char __user *buf,
+size_t count, loff_t *pos)
+{
+ struct hisi_ptt_debugfs_file_desc *desc = filp->private_data;
+ struct hisi_ptt *hisi_ptt = desc->hisi_ptt;
+ struct hisi_ptt_tune_event_desc *e_desc = desc->private;
+ char tbuf[HISI_PTT_CTRL_STR_LEN];
+ int len, ret;
+ u32 val;
+
+ if (!mutex_trylock(&hisi_ptt->mutex))
+ return -EBUSY;
+
+ hisi_ptt->tune_event = e_desc->event_code;
+ ret = e_desc->get(hisi_ptt, &val);
+ mutex_unlock(&hisi_ptt->mutex);
+ if (ret)
+ return ret;
+
+ len = snprintf(tbuf, HISI_PTT_CTRL_STR_LEN, "%d\n", val);
+
+ return simple_read_from_buffer(buf, count, pos, tbuf, len);
+}
+
+static ssize_t hisi_ptt_tune_common_write(struct file *filp,
+ const char __user *buf,
+ size_t count, loff_t *pos)
+{
+ struct hisi_ptt_debugfs_file_desc *desc = filp->private_data;
+
[PATCH RESEND 0/4] Add support for HiSilicon PCIe Tune and Trace device
[RESEND with perf and coresight folks Cc'ed] HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex integrated Endpoint (RCiEP) device, providing the capability to dynamically monitor and tune the PCIe traffic (tune), and trace the TLP headers (trace). PTT tune is designed for monitoring and adjusting PCIe link parameters. We provide several parameters of the PCIe link. Through the driver, user can adjust the value of certain parameter to affect the PCIe link for the purpose of enhancing the performance in certian situation. PTT trace is designed for dumping the TLP headers to the memory, which can be used to analyze the transactions and usage condition of the PCIe Link. Users can choose filters to trace headers, by either requester ID, or those downstream of a set of Root Ports on the same core of the PTT device. It's also supported to trace the headers of certain type and of certain direction. We use debugfs to expose the interface. For tune, one parameter is a debugfs file and user can set/get the value by reading/writing the file. For trace, we have several control files for the user to configure the trace parameters like filters, TLP type and format, the desired trace data size and so on. There is one data file for dumping the traced data to the user. The traced data maybe hundreds of megabytes so sysfs cannot support. The reason for debugfs rather than character device is that we don't want to have additional userspace tools. The operation through debugfs is easier and a bit like ftrace. The reason for not using perf is because there is no current support for uncore tracing in the perf facilities. We have our own format of data and don't need perf doing the parsing. The setting through perf tools doesn't seem to be friendly as well. For example, we cannot count on perf to decode the usual format BDF number like ::., which user can use to filter the TLP headers through the PTT device. A similar approach for implementing this function is ETM, which use sysfs for configuring and a character device for dumping data. Greg has some comments on our implementation and doesn't advocate to build driver on debugfs [1]. So I resend this series to collect more feedbacks on the implementation of this driver. Hi perf and ETM related experts, is it suggested to adapt this driver to perf? Or is the debugfs approach acceptable? Otherwise use sysfs + character device like ETM and use perf tools for decoding it? Any comments is welcomed. [1] https://lore.kernel.org/linux-pci/1617713154-35533-1-git-send-email-yangyic...@hisilicon.com/ Yicong Yang (4): hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device hwtracing: Add tune function support for HiSilicon PCIe Tune and Trace device docs: Add HiSilicon PTT device driver documentation MAINTAINERS: Add maintainer for HiSilicon PTT driver Documentation/trace/hisi-ptt.rst | 326 +++ MAINTAINERS|7 + drivers/Makefile |1 + drivers/hwtracing/Kconfig |2 + drivers/hwtracing/hisilicon/Kconfig| 11 + drivers/hwtracing/hisilicon/Makefile |2 + drivers/hwtracing/hisilicon/hisi_ptt.c | 1636 7 files changed, 1985 insertions(+) create mode 100644 Documentation/trace/hisi-ptt.rst create mode 100644 drivers/hwtracing/hisilicon/Kconfig create mode 100644 drivers/hwtracing/hisilicon/Makefile create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c -- 2.8.1
[PATCH RESEND 1/4] hwtracing: Add trace function support for HiSilicon PCIe Tune and Trace device
HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex integrated Endpoint(RCiEP) device, providing the capability to dynamically monitor and tune the PCIe traffic(tune), and trace the TLP headers(trace). Add the driver for the device to enable the trace function. The driver will create debugfs directory for each PTT device, and users can operate the device through the files under its directory. Signed-off-by: Yicong Yang --- drivers/Makefile |1 + drivers/hwtracing/Kconfig |2 + drivers/hwtracing/hisilicon/Kconfig| 11 + drivers/hwtracing/hisilicon/Makefile |2 + drivers/hwtracing/hisilicon/hisi_ptt.c | 1449 5 files changed, 1465 insertions(+) create mode 100644 drivers/hwtracing/hisilicon/Kconfig create mode 100644 drivers/hwtracing/hisilicon/Makefile create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c diff --git a/drivers/Makefile b/drivers/Makefile index 6fba7da..9a0f76d 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -174,6 +174,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf/ obj-$(CONFIG_RAS) += ras/ obj-$(CONFIG_USB4) += thunderbolt/ obj-$(CONFIG_CORESIGHT)+= hwtracing/coresight/ +obj-$(CONFIG_HISI_PTT) += hwtracing/hisilicon/ obj-y += hwtracing/intel_th/ obj-$(CONFIG_STM) += hwtracing/stm/ obj-$(CONFIG_ANDROID) += android/ diff --git a/drivers/hwtracing/Kconfig b/drivers/hwtracing/Kconfig index 1308583..e3796b1 100644 --- a/drivers/hwtracing/Kconfig +++ b/drivers/hwtracing/Kconfig @@ -5,4 +5,6 @@ source "drivers/hwtracing/stm/Kconfig" source "drivers/hwtracing/intel_th/Kconfig" +source "drivers/hwtracing/hisilicon/Kconfig" + endmenu diff --git a/drivers/hwtracing/hisilicon/Kconfig b/drivers/hwtracing/hisilicon/Kconfig new file mode 100644 index 000..e169771 --- /dev/null +++ b/drivers/hwtracing/hisilicon/Kconfig @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0-only +config HISI_PTT + tristate "HiSilicon PCIe Tune and Trace Device" + depends on ARM64 + depends on PCI && PCI_MSI + depends on HAS_DMA && HAS_IOMEM + depends on DEBUG_FS + help + HiSilicon PCIe Tune and Trace Device exist as a PCIe iEP + device, provides support for PCIe traffic tuning and + tracing TLP headers to the memory. diff --git a/drivers/hwtracing/hisilicon/Makefile b/drivers/hwtracing/hisilicon/Makefile new file mode 100644 index 000..908c09a --- /dev/null +++ b/drivers/hwtracing/hisilicon/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_HISI_PTT) += hisi_ptt.o diff --git a/drivers/hwtracing/hisilicon/hisi_ptt.c b/drivers/hwtracing/hisilicon/hisi_ptt.c new file mode 100644 index 000..a1feece --- /dev/null +++ b/drivers/hwtracing/hisilicon/hisi_ptt.c @@ -0,0 +1,1449 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for HiSilicon PCIe tune and trace device + * + * Copyright (c) 2021 HiSilicon Limited. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define HISI_PTT_CTRL_STR_LEN 40 +#define HISI_PTT_DEFAULT_TRACE_BUF_CNT 64 +#define HISI_PTT_FILTER_UPDATE_FIFO_SIZE 16 + +#define HISI_PTT_RESET_WAIT_MS 1000UL +#define HISI_PTT_WORK_DELAY_MS 100UL +#define HISI_PTT_WAIT_TIMEOUT_US 100UL +#define HISI_PTT_WAIT_POLL_INTERVAL_US 100UL + +#define HISI_PTT_IRQ_NUMS 1 +#define HISI_PTT_DMA_IRQ 0 +#define HISI_PTT_DMA_NUMS 4 + +#define HISI_PTT_TUNING_CTRL 0x +#define HISI_PTT_TUNING_CTRL_CODEGENMASK(15, 0) +#define HISI_PTT_TUNING_CTRL_SUB GENMASK(23, 16) +#define HISI_PTT_TUNING_CTRL_CHN GENMASK(31, 24) +#define HISI_PTT_TUNING_DATA 0x0004 +#define HISI_PTT_TUNING_DATA_VAL_MASKGENMASK(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 +#define HISI_PTT_TRACE_ADDR_STRIDE 0x8 +#define HISI_PTT_TRACE_CTRL0x0850 +#define HISI_PTT_TRACE_CTRL_EN BIT(0) +#define HISI_PTT_TRACE_CTRL_RST BIT(1) +#define HISI_PTT_TRACE_CTRL_RXTX_SEL GENMASK(3, 2) +#define HISI_PTT_TRACE_CTRL_TYPE_SEL GENMASK(7, 4) +#define HISI_PTT_TRACE_CTRL_DATA_FORMAT BIT(14) +#define HISI_PTT_TRACE_CTRL_FILTER_MODE BIT(15) +#define HISI_PTT_TRACE_CTRL_TARGET_SEL GENMASK(31, 16) +#define HISI_PTT_TRACE_INT_STAT0x0890 +#define HISI_PTT_TRACE_INT_STAT_MASK GENMASK(3, 0) +#define HISI_PTT_TRACE_INT_MASK_REG0x0894 +#define HISI_PTT_TUNING_INT_STAT 0x0898 +#define HISI_PTT_TUNING_INT_STAT_MASKBIT(0) +#define HISI_PTT_TUNING_INT_MASK 0x089c +#define HISI_PTT_TRACE_WR_ST
[PATCH RESEND 3/4] docs: Add HiSilicon PTT device driver documentation
Document the introduction and usage of HiSilicon PTT device driver. Signed-off-by: Yicong Yang --- Documentation/trace/hisi-ptt.rst | 326 +++ 1 file changed, 326 insertions(+) create mode 100644 Documentation/trace/hisi-ptt.rst diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst new file mode 100644 index 000..f093846 --- /dev/null +++ b/Documentation/trace/hisi-ptt.rst @@ -0,0 +1,326 @@ +.. SPDX-License-Identifier: GPL-2.0 + +== +HiSilicon PCIe Tune and Trace device +== + +Introduction + + +HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex +integrated Endpoint (RCiEP) device, providing the capability +to dynamically monitor and tune the PCIe link's events (tune), +and trace the TLP headers (trace). The two functions are independent, +but is recommended to use them together to analyze and enhance the +PCIe link's performance. + +On Kunpeng 930 SoC, the PCIe Root Complex is composed of several +PCIe cores. Each PCIe core includes several Root Ports and a PTT +RCiEP, like below. The PTT device is capable of tuning and +tracing the link of the PCIe core. +:: + +--Core 0---+ + | | [ PTT ] | + | | [Root Port]---[Endpoint] + | | [Root Port]---[Endpoint] + | | [Root Port]---[Endpoint] +Root Complex |--Core 1---+ + | | [ PTT ] | + | | [Root Port]---[ Switch ]---[Endpoint] + | | [Root Port]---[Endpoint] `-[Endpoint] + | | [Root Port]---[Endpoint] + +---+ + +The PTT device driver cannot be loaded if debugfs is not mounted. +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt +as its root directory, with name of its BDF number. +:: + +/sys/kernel/debug/hisi_ptt/::. + +Tune + + +PTT tune is designed for monitoring and adjusting PCIe link parameters (events). +Currently we support events in 4 classes. The scope of the events +covers the PCIe core to which the PTT device belongs. + +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and +mostly a simple open/read/write/close cycle will be used to tune +the event. +:: +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune +$ ls +qos_tx_cplqos_tx_npqos_tx_p +tx_path_rx_req_alloc_buf_level +tx_path_tx_req_alloc_buf_level +$ cat qos_tx_dp +1 +$ echo 2 > qos_tx_dp +$ cat qos_tx_dp +2 + +Current value (numerical value) of the event can be simply read +from the file, and the desired value written to the file to tune. + +1. Tx path QoS control + + +The following files are provided to tune the QoS of the tx path of +the PCIe core. + +- qos_tx_cpl: weight of Tx completion TLPs +- qos_tx_np: weight of Tx non-posted TLPs +- qos_tx_p: weight of Tx posted TLPs + +The weight influences the proportion of certain packets on the PCIe link. +For example, for the storage scenario, increase the proportion +of the completion packets on the link to enhance the performance as +more completions are consumed. + +The available tune data of these events is [0, 1, 2]. +Writing a negative value will return an error, and out of range +values will be converted to 2. Note that the event value just +indicates a probable level, but is not precise. + +2. Tx path buffer control +- + +Following files are provided to tune the buffer of tx path of the PCIe core. + +- tx_path_rx_req_alloc_buf_level: watermark of Rx requested +- tx_path_tx_req_alloc_buf_level: watermark of Tx requested + +These events influence the watermark of the buffer allocated for each +type. Rx means the inbound while Tx means outbound. The packets will +be stored in the buffer first and then posted either when the watermark +reached or when timed out. For a busy direction, you should increase +the related buffer watermark to avoid frequently posting and thus +enhance the performance. In most cases just keep the default value. + +The available tune data of above events is [0, 1, 2]. +Writing a negative value will return an error, and out of range +values will be converted to 2. Note that the event value just +indicates a probable level, but is not precise. + +Trace += + +PTT trace is designed for dumping the TLP headers to the memory, which +can be used to analyze the transactions and usage condition of the PCIe +Link. You can choose to filter the traced headers by either requester ID, +or those downstream of a set of Root Ports on the same core of the PTT +device. It's also supported to trace the headers of certain type and of +certain direction. + +In order to start trace, you need to configure the parameters first. +The parameters files are provided under $(PTT root
Re: [PATCH RESEND 0/4] Add support for HiSilicon PCIe Tune and Trace device
On 2021/4/17 21:56, Alexander Shishkin wrote: > Yicong Yang writes: > >> The reason for not using perf is because there is no current support >> for uncore tracing in the perf facilities. > > Not unless you count > > $ perf list|grep -ic uncore > 77 > these are uncore events probably do not support sampling. I tried on x86: # ./perf record -e uncore_imc_0/cas_count_read/ Error: The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (uncore_imc_0/cas_count_read/). /bin/dmesg | grep -i perf may provide additional information. For HiSilicon uncore PMUs, we don't support uncore sampling: 'The current driver does not support sampling. So "perf record" is unsupported. ' [1] and also in another PMU: 'PMU doesn't support process specific events and cannot be used in sampling mode.' [2] [1] Documentation/admin-guide/perf/hisi-pmu.rst [2] Documentation/admin-guide/perf/arm_dsu_pmu.rst >> We have our own format >> of data and don't need perf doing the parsing. > > Perf has AUX buffers, which are used for all kinds of own formats. > ok. we thought perf will break the data format but AUX buffers seems won't. do we need to add full support for tracing as well as parsing or it's ok for not parsing it through perf? >> A similar approach for implementing this function is ETM, which use >> sysfs for configuring and a character device for dumping data. > > And also perf. One reason ETM has a sysfs interface is because the > driver predates perf's AUX buffers. Can't say if it's the only > reason. I'm assuming you're talking about Coresight ETM. > got it. thanks. >> Greg has some comments on our implementation and doesn't advocate >> to build driver on debugfs [1]. So I resend this series to >> collect more feedbacks on the implementation of this driver. >> >> Hi perf and ETM related experts, is it suggested to adapt this driver >> to perf? Or is the debugfs approach acceptable? Otherwise use > > Aside from the above, I don't think the use of debugfs for kernel ABIs > is ever encouraged. > ok. thanks for the suggestions. Regards, Yicong > Regards, > -- > Ale > > . >
Re: [PATCH RESEND 3/4] docs: Add HiSilicon PTT device driver documentation
On 2021/4/19 17:07, Daniel Thompson wrote: > On Sat, Apr 17, 2021 at 06:17:10PM +0800, Yicong Yang wrote: >> Document the introduction and usage of HiSilicon PTT device driver. >> >> Signed-off-by: Yicong Yang >> --- >> Documentation/trace/hisi-ptt.rst | 326 >> +++ >> 1 file changed, 326 insertions(+) >> create mode 100644 Documentation/trace/hisi-ptt.rst >> >> diff --git a/Documentation/trace/hisi-ptt.rst >> b/Documentation/trace/hisi-ptt.rst >> new file mode 100644 >> index 000..f093846 >> --- /dev/null >> +++ b/Documentation/trace/hisi-ptt.rst >> @@ -0,0 +1,326 @@ >> [...] >> +On Kunpeng 930 SoC, the PCIe Root Complex is composed of several >> +PCIe cores. Each PCIe core includes several Root Ports and a PTT >> +RCiEP, like below. The PTT device is capable of tuning and >> +tracing the link of the PCIe core. >> +:: >> + +--Core 0---+ >> + | | [ PTT ] | >> + | | [Root Port]---[Endpoint] >> + | | [Root Port]---[Endpoint] >> + | | [Root Port]---[Endpoint] >> +Root Complex |--Core 1---+ >> + | | [ PTT ] | >> + | | [Root Port]---[ Switch ]---[Endpoint] >> + | | [Root Port]---[Endpoint] `-[Endpoint] >> + | | [Root Port]---[Endpoint] >> + +---+ >> + >> +The PTT device driver cannot be loaded if debugfs is not mounted. > > This can't be right can it? Obviously debugfs must be enabled but why > mounted? > just mention the limit as I'm not sure it's always be mounted. > >> +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt >> +as its root directory, with name of its BDF number. >> +:: >> + >> +/sys/kernel/debug/hisi_ptt/::. >> + >> +Tune >> + >> + >> +PTT tune is designed for monitoring and adjusting PCIe link parameters >> (events). >> +Currently we support events in 4 classes. The scope of the events >> +covers the PCIe core to which the PTT device belongs. >> + >> +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and >> +mostly a simple open/read/write/close cycle will be used to tune >> +the event. >> +:: >> +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune >> +$ ls >> +qos_tx_cplqos_tx_npqos_tx_p >> +tx_path_rx_req_alloc_buf_level >> +tx_path_tx_req_alloc_buf_level >> +$ cat qos_tx_dp >> +1 >> +$ echo 2 > qos_tx_dp >> +$ cat qos_tx_dp >> +2 >> + >> +Current value (numerical value) of the event can be simply read >> +from the file, and the desired value written to the file to tune. > > I saw that this RFC asks about whether debugfs is an appropriate > interface for the *tracing* capability of the platform. Have similar > questions been raised about the tuning interfaces? > yes. as well. > It looks to me like tuning could be handled entirely using sysfs > attributes. I think trying to handle these mostly decoupled feature > in the same place is likely to be a mistake. > Tuning and tracing are two separate functions and it does make sense to decouple them. Thanks for the advice, we can make tuning using sysfs attributes as debugfs is not encouraged. Regards, Yicong > > Daniel. > > . >
Re: [PATCH RESEND 0/4] Add support for HiSilicon PCIe Tune and Trace device
On 2021/4/19 19:17, Suzuki K Poulose wrote: > On 17/04/2021 11:17, Yicong Yang wrote: >> [RESEND with perf and coresight folks Cc'ed] >> >> HiSilicon PCIe tune and trace device (PTT) is a PCIe Root Complex >> integrated Endpoint (RCiEP) device, providing the capability >> to dynamically monitor and tune the PCIe traffic (tune), >> and trace the TLP headers (trace). >> >> PTT tune is designed for monitoring and adjusting PCIe link parameters. >> We provide several parameters of the PCIe link. Through the driver, >> user can adjust the value of certain parameter to affect the PCIe link >> for the purpose of enhancing the performance in certian situation. > > ... > >> >> The reason for not using perf is because there is no current support >> for uncore tracing in the perf facilities. We have our own format >> of data and don't need perf doing the parsing. The setting through >> perf tools doesn't seem to be friendly as well. For example, >> we cannot count on perf to decode the usual format BDF number like >> ::., which user can use to filter the TLP >> headers through the PTT device. >> >> A similar approach for implementing this function is ETM, which use >> sysfs for configuring and a character device for dumping data. >> >> Greg has some comments on our implementation and doesn't advocate >> to build driver on debugfs [1]. So I resend this series to >> collect more feedbacks on the implementation of this driver. >> >> Hi perf and ETM related experts, is it suggested to adapt this driver >> to perf? Or is the debugfs approach acceptable? Otherwise use >> sysfs + character device like ETM and use perf tools for decoding it? >> Any comments is welcomed. > > Please use perf. Debugfs / sysfs is not the right place for these things. > ok. > Also, please move your driver to drivers/perf/ > Does it make sense as it's a tuning and tracing device, and doesn't have counters nor do the sampling like usual PMU device under drivers/perf/. > As Alex mentioned, the ETM drivers were initially developed when the AUX > buffer was not available. The sysfs interface is there only for the backward > compatibility and for bring up ( due to the nature of the > connections between the CoreSight components and sometimes the missing > engineering spec). > got it. thanks for the explanation. Regards, Yicong > Suzuki > > .
Re: [PATCH v7 2/5] i2c: core: add api to provide frequency mode strings
On 2021/4/9 19:40, Andy Shevchenko wrote:
> On Fri, Apr 9, 2021 at 2:37 PM Wolfram Sang wrote:
>>
>>
>>> Can we add this later if needed?
>>> Because in such case additionally printing bus_freq_hz will be fine, no?
>>
>> Yes, we can do that.
>>
>>> But putting max to each frequency representation in the list of strings
>>> sounds
>>> good to me.
>>
>> It is not important to me if we are going to change that later anyhow.
>> I'll leave it to you guys.
>
> Thanks, I think the series is okay to go as is.
>
sorry for the late reply. we can have this series applied if possible,
or you may apply the changed patch below (please let me know if you
want the whole series updated).
I didn't realize this, sorry. our two users don't have this situation.
thanks Wolfram and Andy!
Yicong.
>From 14da3be8d85536c16adbc4006fc12c6837ef7474 Mon Sep 17 00:00:00 2001
From: Yicong Yang
Date: Sat, 27 Mar 2021 11:48:25 +0800
Subject: [PATCH] i2c: core: add api to provide frequency mode strings
Some I2C drivers like Designware and HiSilicon will print the
bus frequency mode information, so add a public one that everyone
can make use of.
Tested-by: Jarkko Nikula
Reviewed-by: Jarkko Nikula
Reviewed-by: Andy Shevchenko
Signed-off-by: Yicong Yang
---
drivers/i2c/i2c-core-base.c | 19 +++
include/linux/i2c.h | 3 +++
2 files changed, 22 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index de9402c..53836b5 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -76,6 +76,25 @@ void i2c_transfer_trace_unreg(void)
static_branch_dec(&i2c_trace_msg_key);
}
+const char *i2c_freq_mode_string(u32 bus_freq_hz)
+{
+ if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ)
+ return "Standard Mode (max 100 kHz)";
+ else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ)
+ return "Fast Mode (max 400 kHz)";
+ else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ)
+ return "Fast Mode Plus (max 1.0 MHz)";
+ else if (bus_freq_hz <= I2C_MAX_TURBO_MODE_FREQ)
+ return "Turbo Mode (max 1.4 MHz)";
+ else if (bus_freq_hz <= I2C_MAX_HIGH_SPEED_MODE_FREQ)
+ return "High Speed Mode (max 3.4 MHz)";
+ else if (bus_freq_hz <= I2C_MAX_ULTRA_FAST_MODE_FREQ)
+ return "Ultra Fast Mode (max 5.0 MHz)";
+ else
+ return "Unknown Mode";
+}
+EXPORT_SYMBOL_GPL(i2c_freq_mode_string);
+
const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
const struct i2c_client *client)
{
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 10bd0b0..0813be1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -51,6 +51,9 @@ struct module;
struct property_entry;
#if IS_ENABLED(CONFIG_I2C)
+/* Return the Frequency mode string based on the bus frequency */
+const char *i2c_freq_mode_string(u32 bus_freq_hz);
+
/*
* The master routines are the ones normally used to transmit data to devices
* on a bus (or read from them). Apart from two basic transfer functions to
--
2.8.1
Re: [PATCH 3/4] docs: Add documentation for HiSilicon PTT device driver
On 2021/4/9 0:57, Bjorn Helgaas wrote: > On Thu, Apr 08, 2021 at 09:22:52PM +0800, Yicong Yang wrote: >> On 2021/4/8 2:55, Bjorn Helgaas wrote: >>> On Tue, Apr 06, 2021 at 08:45:53PM +0800, Yicong Yang wrote: > >>>> +On Kunpeng 930 SoC, the PCIe root complex is composed of several >>>> +PCIe cores. >> >>> Can you connect "Kunpeng 930" to something in the kernel tree? >>> "git grep -i kunpeng" shows nothing that's obviously relevant. >>> I assume there's a related driver in drivers/pci/controller/? >> >> Kunpeng 930 is the product name of Hip09 platform. The PCIe >> controller uses the generic PCIe driver based on ACPI. > > I guess I'm just looking for a hint to help users know when to enable > the Kconfig for this. Maybe the "HiSilicon" in the Kconfig help is > enough? Maybe "Kunpeng 930" is not even necessary? If "Kunpeng 930" > *is* necessary, there should be some way to relate it to something > else. > since it's added in Kunpeng 930. otherwise users maybe confused why they don't find it on Kunpeng 920 (Hip 08) or older platforms. The Kunpeng is the product name, users should have known it when they have such a platform. >>>> +from the file, and the desired value written to the file to tune. >>> >>>> +Tuning multiple events at the same time is not permitted, which means >>>> +you cannot read or write more than one tune file at one time. >>> >>> I think this is obvious from the model, so the sentence doesn't really >>> add anything. Each event is a separate file, and it's obvious that >>> there's no way to write to multiple files simultaneously. >> >> from the usage we shown below this situation won't happen. I just worry >> that users may have a program to open multiple files at the same time and >> read/write simultaneously, so add this line here to mention the restriction. > > How is this possible? I don't think "writing multiple files > simultaneously" is even possible in the Linux syscall model. I don't > think a user will do anything differently after reading "you cannot > read or write more than one tune file at one time." > then I'll remove this line. thanks. >>>> +- tx_path_rx_req_alloc_buf_level: watermark of RX requested >>>> +- tx_path_tx_req_alloc_buf_level: watermark of TX requested >>>> + >>>> +These events influence the watermark of the buffer allocated for each >>>> +type. RX means the inbound while Tx means outbound. For a busy >>>> +direction, you should increase the related buffer watermark to enhance >>>> +the performance. >>> >>> Based on what you have written here, I would just write 2 to both >>> files to enhance the performance in both directions. But obviously >>> there must be some tradeoff here, e.g., increasing Rx performance >>> comes at the cost of Tx performane. >> >> the Rx buffer and Tx buffer are separate, so they won't influence >> each other. > > Why would I write anything other than 2 to these files? That's the > question I think this paragraph should answer. > In most cases just keep the normal level. the data in the buffer will be posted when reaching the watermark or timed out. for some situation you have an idle traffic but you want a quick response, then set the watermark in lower level. >>>> +9. data_format >>>> +-- >>>> + >>>> +File to indicate the format of the traced TLP headers. User can also >>>> +specify the desired format of traced TLP headers. Available formats >>>> +are 4DW, 8DW which indicates the length of each TLP headers traced. >>>> +:: >>>> +$ cat data_format >>>> +[4DW]8DW >>>> +$ echo 8 > data_format >>>> +$ cat data_format >>>> +4DW [8DW] >>>> + >>>> +The traced TLP header format is different from the PCIe standard. >>> >>> I'm confused. Below you say the fields of the traced TLP header are >>> defined by the PCIe spec. But here you say the format is *different*. >>> What exactly is different? >> >> For the Request Header Format for 64-bit addressing of Memory, defind in >> PCIe spec 4.0, Figure 2-15, the 1st DW is like: >> >> Byte 0 > [Fmt] [Type] [T9] [Tc] [T8] [Attr] [LN] [TH] ... [Length] >> >> some are recorded in our traced header like below, which some are not. >> that's what I me
[PATCH] fs: export vfs_stat() and vfs_fstatat()
The public function vfs_stat() and vfs_fstatat() are
unexported after moving out of line in
commit 09f1bde4017e ("fs: move vfs_fstatat out of line"),
which will prevent the using in kernel modules.
So make them exported.
Fixes: 09f1bde4017e ("fs: move vfs_fstatat out of line")
Reported-by: Yang Shen
Signed-off-by: Yicong Yang
---
fs/stat.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/stat.c b/fs/stat.c
index dacecdd..7d690c6 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -147,6 +147,7 @@ int vfs_fstat(int fd, struct kstat *stat)
fdput(f);
return error;
}
+EXPORT_SYMBOL(vfs_fstat);
/**
* vfs_statx - Get basic and extra attributes by filename
@@ -207,6 +208,7 @@ int vfs_fstatat(int dfd, const char __user *filename,
return vfs_statx(dfd, filename, flags | AT_NO_AUTOMOUNT,
stat, STATX_BASIC_STATS);
}
+EXPORT_SYMBOL(vfs_fstatat);
#ifdef __ARCH_WANT_OLD_STAT
--
2.8.1
Re: [PATCH] fs: export vfs_stat() and vfs_fstatat()
Hi Christoph,
On 2020/11/26 15:18, Christoph Hellwig wrote:
> On Thu, Nov 26, 2020 at 03:15:48PM +0800, Yicong Yang wrote:
>> The public function vfs_stat() and vfs_fstatat() are
>> unexported after moving out of line in
>> commit 09f1bde4017e ("fs: move vfs_fstatat out of line"),
>> which will prevent the using in kernel modules.
>> So make them exported.
> And why would you want to use them in kernel module? Please explain
> that in the patch that exports them, and please send that patch in the
> same series as the patches adding the users.
we're using it in the modules for testing our crypto driver on our CI system.
is it mandatory to upstream it if we want to use this function?
Thanks,
Yicong
> .
>
Re: [PATCH] fs: export vfs_stat() and vfs_fstatat()
On 2020/11/26 17:15, Christoph Hellwig wrote:
> On Thu, Nov 26, 2020 at 05:08:26PM +0800, Yicong Yang wrote:
>>> And why would you want to use them in kernel module? Please explain
>>> that in the patch that exports them, and please send that patch in the
>>> same series as the patches adding the users.
>> we're using it in the modules for testing our crypto driver on our CI system.
>> is it mandatory to upstream it if we want to use this function?
> Yes. And chances are that you do not actaully need these functions
> either, but to suggest a better placement I need to actually see the
> code.
> .
Sorry for not describing the issues I met correctly in the commit message.
Actually we're using inline function vfs_stat() for getting the
attributes, which calls vfs_fstatat():
static inline int vfs_stat(const char __user *filename, struct kstat *stat)
{
return vfs_fstatat(AT_FDCWD, filename, stat, 0);
}
after the vfs_fstatat is out-of-line it will make the moduler user of vfs_stat()
broken:
[ 5328.903677] crypto_zip_perf_test: Unknown symbol vfs_fstatat (err -2)
so the simplest way i think is directly export the vfs_fstatat().
Thanks,
Yicong
>
Re: [PATCH] fs: export vfs_stat() and vfs_fstatat()
On 2020/11/26 17:50, Christoph Hellwig wrote: > On Thu, Nov 26, 2020 at 05:48:25PM +0800, Yicong Yang wrote: >> Sorry for not describing the issues I met correctly in the commit message. >> Actually we're using inline function vfs_stat() for getting the >> attributes, which calls vfs_fstatat(): > Again, there generally isn't much need to look at the stat data > for an in-kernel caller. But without you submitting the code I can't > really help you anyway. > . sure. we'll check to see whether it's necessary or there is other way. many thanks!
[PATCH] sched/topology: Fix a typo in pr_err()
Fix a typo 'borken' to 'broken' in pr_err().
Signed-off-by: Yicong Yang
---
kernel/sched/topology.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 09d3504..c42e388 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1906,7 +1906,7 @@ static struct sched_domain *build_sched_domain(struct
sched_domain_topology_leve
if (!cpumask_subset(sched_domain_span(child),
sched_domain_span(sd))) {
- pr_err("BUG: arch topology borken\n");
+ pr_err("BUG: arch topology broken\n");
#ifdef CONFIG_SCHED_DEBUG
pr_err(" the %s domain not a subset of the %s
domain\n",
child->name, sd->name);
--
2.8.1
Re: [PATCH] sched/topology: Fix a typo in pr_err()
On 2021/3/8 19:32, Peter Zijlstra wrote: > On Mon, Mar 08, 2021 at 05:24:56PM +0800, Yicong Yang wrote: >> Fix a typo 'borken' to 'broken' in pr_err(). > > It was not a typo.. > got it already. terribly sorry to bother and please ignore this. > . >
Re: [PATCH 1/4] driver core: Use subdir-ccflags-* to inherit debug flag
Hi Greg, On 2021/2/5 17:53, Greg KH wrote: > On Fri, Feb 05, 2021 at 05:44:12PM +0800, Yicong Yang wrote: >> From: Junhao He >> >> Use subdir-ccflags-* instead of ccflags-* to inherit the debug >> settings from Kconfig when traversing subdirectories. > > That says what you do, but not _why_ you are doing it. i'll illustrate the reason and reword the commit. > > What does this offer in benefit of the existing way? What is it fixing? > Why do this "churn"? currently we have added ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG in the Makefile of driver/base and driver/base/power, but not in the subdirectory driver/base/firmware_loader. we cannot turn the debug on for subdirectory firmware_loader if we config DEBUG_DRIVER and there is no kconfig option for the it. as there is no other debug config in the subdirectory of driver/base, CONFIG_DEBUG_DRIVER may also mean turn on the debug in the subdirectories, so use subdir-ccflags-* instead for the -DDEBUG will allow us to enable debug for all the subdirectories. Thanks, Yicong > > thanks, > > greg k-h > > . >
Re: [PATCH 2/4] hwmon: Use subdir-ccflags-* to inherit debug flag
On 2021/2/6 4:08, Bjorn Helgaas wrote: > On Fri, Feb 05, 2021 at 10:28:32AM -0800, Guenter Roeck wrote: >> On Fri, Feb 05, 2021 at 05:44:13PM +0800, Yicong Yang wrote: >>> From: Junhao He >>> >>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug >>> settings from Kconfig when traversing subdirectories. >>> >>> Suggested-by: Bjorn Helgaas >>> Signed-off-by: Junhao He >>> Signed-off-by: Yicong Yang >> >> What problem does this fix ? Maybe I am missing it, but I don't see >> DEBUG being used in a subdirectory of drivers/hwmon. > > It's my fault for raising this question [1]. Yicong fixed a real > problem in drivers/pci, where we are currently using > > ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG > > so CONFIG_PCI_DEBUG=y turns on debug in drivers/pci, but not in the > subdirectories. That's surprising to users. > > So my question was whether we should default to using subdir-ccflags > for -DDEBUG in general, and only use ccflags when we have > subdirectories that have their own debug options, e.g., > > drivers/i2c/Makefile:ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG > drivers/i2c/algos/Makefile:ccflags-$(CONFIG_I2C_DEBUG_ALGO) := -DDEBUG > drivers/i2c/busses/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > drivers/i2c/muxes/Makefile:ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > > I mentioned drivers/hwmon along with a few others that have > subdirectories, do not have per-subdirectory debug options, and use > ccflags. I didn't try to determine whether those subdirectories > currently use -DDEBUG. > > In the case of drivers/hwmon, several drivers do use pr_debug(), > and CONFIG_HWMON_DEBUG_CHIP=y turns those on. But if somebody > were to add pr_debug() to drivers/hwmon/occ/common.c, for example, > CONFIG_HWMON_DEBUG_CHIP=y would *not* turn it on. That sounds > surprising to me, but if that's what you intend, that's totally fine. i thought CONFIG_HWMON_DEBUG_CHIP=y means to enable debug including the subdirectories, so use subdir-ccflags-* will make sure the debug message on in the subdirectories, if there will be. please let me know if i understand wrong. Thanks, Yicong > > [1] https://lore.kernel.org/r/20210204161048.GA68790@bjorn-Precision-5520 > >>> --- >>> drivers/hwmon/Makefile | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >>> index 09a86c5..1c0c089 100644 >>> --- a/drivers/hwmon/Makefile >>> +++ b/drivers/hwmon/Makefile >>> @@ -201,5 +201,5 @@ obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o >>> obj-$(CONFIG_SENSORS_OCC) += occ/ >>> obj-$(CONFIG_PMBUS)+= pmbus/ >>> >>> -ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG >>> +subdir-ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG >>> >>> -- >>> 2.8.1 >>> > > . >
Re: [PATCH 1/4] driver core: Use subdir-ccflags-* to inherit debug flag
On 2021/2/8 18:47, Greg KH wrote:
> On Mon, Feb 08, 2021 at 06:44:52PM +0800, Yicong Yang wrote:
>> Hi Greg,
>>
>> On 2021/2/5 17:53, Greg KH wrote:
>>> On Fri, Feb 05, 2021 at 05:44:12PM +0800, Yicong Yang wrote:
>>>> From: Junhao He
>>>>
>>>> Use subdir-ccflags-* instead of ccflags-* to inherit the debug
>>>> settings from Kconfig when traversing subdirectories.
>>>
>>> That says what you do, but not _why_ you are doing it.
>>
>> i'll illustrate the reason and reword the commit.
>>
>>>
>>> What does this offer in benefit of the existing way? What is it fixing?
>>> Why do this "churn"?
>>
>> currently we have added ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG in the
>> Makefile
>> of driver/base and driver/base/power, but not in the subdirectory
>> driver/base/firmware_loader. we cannot turn the debug on for subdirectory
>> firmware_loader if we config DEBUG_DRIVER and there is no kconfig option
>> for the it.
>
> Is that necessary? Does that directory need it?
there are several debug prints in firmware_loader/main.c:
./main.c:207: pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name, fw_priv);
./main.c:245: pr_debug("batched request - sharing the same
struct fw_priv and lookup for multiple requests\n");
./main.c:269: pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
./main.c:594: pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
./main.c:605: pr_debug("%s: fw_name-%s devm-%p released\n",
./main.c:1175: pr_debug("%s: %s\n", __func__, fw_name);
./main.c:1181: pr_debug("%s: %s ret=%d\n", __func__, fw_name, ret);
./main.c:1214: pr_debug("%s: %s\n", __func__, fw_name);
./main.c:1272: pr_debug("%s: fw: %s\n", __func__, name);
./main.c:1389: pr_debug("%s\n", __func__);
./main.c:1415: pr_debug("%s\n", __func__);
>
> thanks,
>
> greg k-h
>
> .
>
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
Hi,
On 2020/5/27 9:31, Kuppuswamy, Sathyanarayanan wrote:
> Hi,
>
> On 5/21/20 7:56 PM, Yicong Yang wrote:
>>
>>
>> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>>
>>>
>>> On 5/21/20 3:58 AM, Yicong Yang wrote:
>>>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>>>
>>>>>
>>>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>>>> On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
>>>>>>> From: Kuppuswamy Sathyanarayanan
>>>>>>>
>>>>>>>
>>>>>>> If there are non-hotplug capable devices connected to a given
>>>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>>>> in the affected bus. Instead, we will have to let the error
>>>>>>> recovery handler call report_slot_reset() for all devices in
>>>>>>> the bus to notify about the reset operation. Although this is
>>>>>>> only required for non hot-plug capable devices, doing it for
>>>>>>> hotplug capable devices should not affect the functionality.
>>>>>>>
>>>>>>> Along with above issue, this fix also applicable to following
>>>>>>> issue.
>>>>>>>
>>>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>>> reset_link()") added support to store status of reset_link()
>>>>>>> call. Although this fixed the error recovery issue observed if
>>>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded the status
>>>>>>> result from report_frozen_detected. This can cause a failure to
>>>>>>> recover if _NEED_RESET is returned by report_frozen_detected and
>>>>>>> report_slot_reset is not invoked.
>>>>>>>
>>>>>>> Such an event can be induced for testing purposes by reducing the
>>>>>>> Max_Payload_Size of a PCIe bridge to less than that of a device
>>>>>>> downstream from the bridge, and then initiating I/O through the
>>>>>>> device, resulting in oversize transactions. In the presence of DPC,
>>>>>>> this results in a containment event and attempted reset and recovery
>>>>>>> via pcie_do_recovery. After 6d2c89441571 report_slot_reset is not
>>>>>>> invoked, and the device does not recover.
>>>>>>>
>>>>>>> [original patch is from jay.vosbu...@canonical.com]
>>>>>>> [original patch link
>>>>>>> https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>>>> Signed-off-by: Jay Vosburgh
>>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>>>>>
>>>>>>> ---
>>>>>>> drivers/pci/pcie/err.c | 19 +++
>>>>>>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>>>> index 14bb8f54723e..db80e1ecb2dc 100644
>>>>>>> --- a/drivers/pci/pcie/err.c
>>>>>>> +++ b/drivers/pci/pcie/err.c
>>>>>>> @@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev
>>>>>>> *dev,
>>>>>>> pci_dbg(dev, "broadcast error_detected message\n");
>>>>>>> if (state == pci_channel_io_frozen) {
>>>>>>> pci_walk_bus(bus, report_frozen_detected, &status);
>>>>>>> -status = reset_link(dev);
>>>>>>> -if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>>>> +status = PCI_ERS_RESULT_NEED_RESET;
>>>>>>> +} else {
>>>>>>> +pci_walk_bus(bus, report_normal_detected, &status);
>>>>>>> +}
>>>>>>> +
>>>>>>> +if (status == PCI_ERS_RESULT_NEED_RESET) {
>>>>&
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On 2020/5/27 12:04, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 5/26/20 8:50 PM, Yicong Yang wrote:
>> Hi,
>>
>>
>> On 2020/5/27 9:31, Kuppuswamy, Sathyanarayanan wrote:
>>> Hi,
>>>
>>> On 5/21/20 7:56 PM, Yicong Yang wrote:
>>>>
>>>>
>>>> On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>>>>>
>>>>>
>>>>> On 5/21/20 3:58 AM, Yicong Yang wrote:
>>>>>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>>>>>> On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
>>>>>>>>> From: Kuppuswamy Sathyanarayanan
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If there are non-hotplug capable devices connected to a given
>>>>>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>>>>>> in the affected bus. Instead, we will have to let the error
>>>>>>>>> recovery handler call report_slot_reset() for all devices in
>>>>>>>>> the bus to notify about the reset operation. Although this is
>>>>>>>>> only required for non hot-plug capable devices, doing it for
>>>>>>>>> hotplug capable devices should not affect the functionality.
>>>>>>>>>
>>>>>>>>> Along with above issue, this fix also applicable to following
>>>>>>>>> issue.
>>>>>>>>>
>>>>>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>>>>> reset_link()") added support to store status of reset_link()
>>>>>>>>> call. Although this fixed the error recovery issue observed if
>>>>>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded the status
>>>>>>>>> result from report_frozen_detected. This can cause a failure to
>>>>>>>>> recover if _NEED_RESET is returned by report_frozen_detected and
>>>>>>>>> report_slot_reset is not invoked.
>>>>>>>>>
>>>>>>>>> Such an event can be induced for testing purposes by reducing the
>>>>>>>>> Max_Payload_Size of a PCIe bridge to less than that of a device
>>>>>>>>> downstream from the bridge, and then initiating I/O through the
>>>>>>>>> device, resulting in oversize transactions. In the presence of DPC,
>>>>>>>>> this results in a containment event and attempted reset and recovery
>>>>>>>>> via pcie_do_recovery. After 6d2c89441571 report_slot_reset is not
>>>>>>>>> invoked, and the device does not recover.
>>>>>>>>>
>>>>>>>>> [original patch is from jay.vosbu...@canonical.com]
>>>>>>>>> [original patch link
>>>>>>>>> https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after
>>>>>>>>> reset_link()")
>>>>>>>>> Signed-off-by: Jay Vosburgh
>>>>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> drivers/pci/pcie/err.c | 19 +++
>>>>>>>>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>>>>>> index 14bb8f54723e..db80e1ecb2dc 100644
>>>>>>>>> --- a/drivers/pci/pcie/err.c
>>>>>>>>> +++ b/drivers/pci/pcie/err.c
>>>>>>>>> @@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct
>>>>>>>>> pci_dev *dev,
>>>>>>>>> pci_dbg(dev, "broadcast error_detected mess
[RFC PATCH] hwtracing: Add HiSilicon PCIe Tune and Trace device driver
HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex integrated Endpoint(RCiEP) device, providing the capability to dynamically monitor and tune the PCIe traffic parameters(tune), and trace the TLP headers to the memory(trace). Add the driver for the device to enable its functions. The driver will create debugfs directory for each PTT device, and users can operate the device through the files under its directory. RFC: - The hardware interface is not yet finalized. - The interface to the users is through debugfs, and the usage will be further illustrated in the document. - The driver is intended to be put under drivers/hwtracing, where we think best match the device's function. Signed-off-by: Yicong Yang --- Documentation/trace/hisi-ptt.rst | 272 drivers/hwtracing/Kconfig |2 + drivers/hwtracing/hisilicon/Kconfig|8 + drivers/hwtracing/hisilicon/Makefile |2 + drivers/hwtracing/hisilicon/hisi_ptt.c | 1172 5 files changed, 1456 insertions(+) create mode 100644 Documentation/trace/hisi-ptt.rst create mode 100644 drivers/hwtracing/hisilicon/Kconfig create mode 100644 drivers/hwtracing/hisilicon/Makefile create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c diff --git a/Documentation/trace/hisi-ptt.rst b/Documentation/trace/hisi-ptt.rst new file mode 100644 index 000..c99fbf9 --- /dev/null +++ b/Documentation/trace/hisi-ptt.rst @@ -0,0 +1,272 @@ +.. SPDX-License-Identifier: GPL-2.0 + +== +HiSilicon PCIe Tune and Trace device +== + +Introduction + + +HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex +integrated Endpoint(RCiEP) device, providing the capability +to dynamically monitor and tune the PCIe link's events(tune), +and trace the TLP headers to the memory(trace). The two functions +are inpendent, but is recommended to use them together to analyze +and enhance the PCIe link's performance. + +On Hip09, the PCIe root complex is composed of several PCIe cores. +And each core is composed of several root ports, RCiEPs, and one +PTT device, like below. The PTT device is capable of tuning and +tracing the link on and downstream the PCIe core. +:: + +--Core 0---+ + | | [ PTT ] | + | | [Root Port]---[Endpoint] + | | [Root Port]---[Endpoint] + | | [Root Port]---[Endpoint] +Root Complex |--Core 1---+ + | | [ PTT ] | + | | [Root Port]---[ Switch ]---[Endpoint] + | | [Root Port]---[Endpoint] `-[Endpoint] + | | [Root Port]---[Endpoint] + +---+ + +The PTT device driver cannot be loaded if debugfs is not mounted. +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt +as its root directory, with name of its BDF number. +:: + +/sys/kernel/debug/hisi_ptt/::. + +Tune + + +PTT tune is designed for monitoring and adjusting PCIe link parameters(events). +Currently we support events 4 classes. The scope of the events +covers the PCIe core with which the PTT device belongs to. + +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and +mostly this will be a simple open/read/write/close cycle to tune +the event. +:: +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune +$ ls +buf_rx_cpld buf_rx_pd dllp_link_ack_freq link_credit_rx_cplh +link_credit_rx_ph qos_tx_dp qos_tx_dp +$ cat qos_tx_dp +100 +$ echo 50 > qos_tx_dp +$ cat qos_tx_dp +50 + +Current value(numerical value) of the event can be get by simply +read the file, and write the desired value to the file to tune. +Tune multiple events at the same time is not permitted, which means +you cannot read or write more than one tune file at one time. + +1. Link credit control +-- + +Following files are provided for tune the link credit events of the PCIe core. +PCIe link uses credit to control the flow, refer to the PCIe Spec for further +information. + +- link_credit_rx_nph: rx non-posted request headers' credit +- link_credit_rx_npd: rx non-posted request data payload's credit +- link_credit_rx_ph: rx posted request headers' credit +- link_credit_rx_pd: rx posted request data payload's credit +- link_credit_rx_cplh: rx completion headers' credit +- link_credit_rx_cpld: rx completion data payload's credit + +Note that the event value is not accurate but a probable one to indicate +the level of each event, for example, perhaps 100 for high level, +50 for median and 0 for low. + +2. Link DLLP control + + +Following files are provided for tune the link events of DLLP of the PCIe core. + +- dllp_link_ack_freq: frequency of DLLP ACKs +- dllp_link_updat
[PATCH] libfs: fix error cast of negative value in simple_attr_write()
The attr->set() receive a value of u64, but we use simple_strtoll()
for doing the conversion. It will lead to the error cast if user inputs
a negative value.
Use kstrtoull() instead to resolve this issue, -EINVAL will be returned
if a negative value is input.
Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
Signed-off-by: Yicong Yang
---
fs/libfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index e0d42e9..803c439 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -975,7 +975,9 @@ ssize_t simple_attr_write(struct file *file, const char
__user *buf,
goto out;
attr->set_buf[size] = '\0';
- val = simple_strtoll(attr->set_buf, NULL, 0);
+ ret = kstrtoull(attr->set_buf, 0, &val);
+ if (ret)
+ goto out;
ret = attr->set(attr->data, val);
if (ret == 0)
ret = len; /* on success, claim we got the whole input */
--
2.8.1
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On 2020/5/22 3:31, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 5/21/20 3:58 AM, Yicong Yang wrote:
>> On 2020/5/21 1:04, Kuppuswamy, Sathyanarayanan wrote:
>>>
>>>
>>> On 5/20/20 1:28 AM, Yicong Yang wrote:
>>>> On 2020/5/7 11:32, sathyanarayanan.kuppusw...@linux.intel.com wrote:
>>>>> From: Kuppuswamy Sathyanarayanan
>>>>>
>>>>>
>>>>> If there are non-hotplug capable devices connected to a given
>>>>> port, then during the fatal error recovery(triggered by DPC or
>>>>> AER), after calling reset_link() function, we cannot rely on
>>>>> hotplug handler to detach and re-enumerate the device drivers
>>>>> in the affected bus. Instead, we will have to let the error
>>>>> recovery handler call report_slot_reset() for all devices in
>>>>> the bus to notify about the reset operation. Although this is
>>>>> only required for non hot-plug capable devices, doing it for
>>>>> hotplug capable devices should not affect the functionality.
>>>>>
>>>>> Along with above issue, this fix also applicable to following
>>>>> issue.
>>>>>
>>>>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>>>>> reset_link()") added support to store status of reset_link()
>>>>> call. Although this fixed the error recovery issue observed if
>>>>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>>>>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded the status
>>>>> result from report_frozen_detected. This can cause a failure to
>>>>> recover if _NEED_RESET is returned by report_frozen_detected and
>>>>> report_slot_reset is not invoked.
>>>>>
>>>>> Such an event can be induced for testing purposes by reducing the
>>>>> Max_Payload_Size of a PCIe bridge to less than that of a device
>>>>> downstream from the bridge, and then initiating I/O through the
>>>>> device, resulting in oversize transactions. In the presence of DPC,
>>>>> this results in a containment event and attempted reset and recovery
>>>>> via pcie_do_recovery. After 6d2c89441571 report_slot_reset is not
>>>>> invoked, and the device does not recover.
>>>>>
>>>>> [original patch is from jay.vosbu...@canonical.com]
>>>>> [original patch link
>>>>> https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>>>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>>>> Signed-off-by: Jay Vosburgh
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>>>
>>>>> ---
>>>>>drivers/pci/pcie/err.c | 19 +++
>>>>>1 file changed, 15 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>>> index 14bb8f54723e..db80e1ecb2dc 100644
>>>>> --- a/drivers/pci/pcie/err.c
>>>>> +++ b/drivers/pci/pcie/err.c
>>>>> @@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev
>>>>> *dev,
>>>>>pci_dbg(dev, "broadcast error_detected message\n");
>>>>>if (state == pci_channel_io_frozen) {
>>>>>pci_walk_bus(bus, report_frozen_detected, &status);
>>>>> -status = reset_link(dev);
>>>>> -if (status != PCI_ERS_RESULT_RECOVERED) {
>>>>> +status = PCI_ERS_RESULT_NEED_RESET;
>>>>> +} else {
>>>>> +pci_walk_bus(bus, report_normal_detected, &status);
>>>>> +}
>>>>> +
>>>>> +if (status == PCI_ERS_RESULT_NEED_RESET) {
>>>>> +if (reset_link) {
>>>>> +if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED)
>>>>
>>>> we'll call reset_link() only if link is frozen. so it may have problem
>>>> here.
>>> you mean before this change right?
>>> After this change, reset_link() will be called as long as status is
>>> PCI_ERS_RESULT_NEED_RESET.
>>
>> Yes. I think we should reset the link only if the io is blocked as before.
>> There's
>> no reason to reset a normal link.
> Currently, only AER and DPC driver uses pcie_do_recovery() call. So the
> possible reset_link options are dpc_rese
[PATCH] PCI: Factor functions of PCI function reset
Previosly we use pci_probe_reset_function() to probe whehter a function
can be reset and use __pci_reset_function_locked() to perform a function
reset. These two functions have lots of common lines.
Factor the two functions and reduce the redundancy.
Signed-off-by: Yicong Yang
---
drivers/pci/pci.c | 61 -
drivers/pci/pci.h | 2 +-
drivers/pci/probe.c | 2 +-
3 files changed, 20 insertions(+), 45 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e39c549..e3e5f0f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5006,9 +5006,11 @@ static void pci_dev_restore(struct pci_dev *dev)
}
/**
- * __pci_reset_function_locked - reset a PCI device function while holding
- * the @dev mutex lock.
+ * pci_probe_reset_function - check whether the device can be safely reset
+ *or reset a PCI device function while holding
+ *the @dev mutex lock.
* @dev: PCI device to reset
+ * @probe: Probe or not whether the device can be reset.
*
* Some devices allow an individual function to be reset without affecting
* other functions in the same device. The PCI device must be responsive
@@ -5022,10 +5024,10 @@ static void pci_dev_restore(struct pci_dev *dev)
* device including MSI, bus mastering, BARs, decoding IO and memory spaces,
* etc.
*
- * Returns 0 if the device function was successfully reset or negative if the
- * device doesn't support resetting a single function.
+ * Returns 0 if the device function can be reset or was successfully reset.
+ * negative if the device doesn't support resetting a single function.
*/
-int __pci_reset_function_locked(struct pci_dev *dev)
+int pci_probe_reset_function(struct pci_dev *dev, int probe)
{
int rc;
@@ -5039,61 +5041,34 @@ int __pci_reset_function_locked(struct pci_dev *dev)
* other error, we're also finished: this indicates that further
* reset mechanisms might be broken on the device.
*/
- rc = pci_dev_specific_reset(dev, 0);
+ rc = pci_dev_specific_reset(dev, probe);
if (rc != -ENOTTY)
return rc;
if (pcie_has_flr(dev)) {
+ if (probe)
+ return 0;
rc = pcie_flr(dev);
if (rc != -ENOTTY)
return rc;
}
- rc = pci_af_flr(dev, 0);
+ rc = pci_af_flr(dev, probe);
if (rc != -ENOTTY)
return rc;
- rc = pci_pm_reset(dev, 0);
+ rc = pci_pm_reset(dev, probe);
if (rc != -ENOTTY)
return rc;
- rc = pci_dev_reset_slot_function(dev, 0);
+ rc = pci_dev_reset_slot_function(dev, probe);
if (rc != -ENOTTY)
return rc;
- return pci_parent_bus_reset(dev, 0);
+
+ return pci_parent_bus_reset(dev, probe);
}
-EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
-/**
- * pci_probe_reset_function - check whether the device can be safely reset
- * @dev: PCI device to reset
- *
- * Some devices allow an individual function to be reset without affecting
- * other functions in the same device. The PCI device must be responsive
- * to PCI config space in order to use this function.
- *
- * Returns 0 if the device function can be reset or negative if the
- * device doesn't support resetting a single function.
- */
-int pci_probe_reset_function(struct pci_dev *dev)
+int __pci_reset_function_locked(struct pci_dev *dev)
{
- int rc;
-
- might_sleep();
-
- rc = pci_dev_specific_reset(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- if (pcie_has_flr(dev))
- return 0;
- rc = pci_af_flr(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_pm_reset(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_dev_reset_slot_function(dev, 1);
- if (rc != -ENOTTY)
- return rc;
-
- return pci_parent_bus_reset(dev, 1);
+ return pci_probe_reset_function(dev, 0);
}
+EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
/**
* pci_reset_function - quiesce and reset a PCI device function
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fa12f7c..73740dd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -39,7 +39,7 @@ enum pci_mmap_api {
int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
enum pci_mmap_api mmap_api);
-int pci_probe_reset_function(struct pci_dev *dev);
+int pci_probe_reset_function(struct pci_dev *dev, int probe);
int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
int pci_bus_error_reset(struct pci_dev *dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 03d3712..793cc8a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2403,7 +2403,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
pcie_rep
[PATCH] PCI: Fix race condition between block_cfg_access and msi_enabled/msix_enabled
Previously we use bit field for block_cfg_access and
msi_enabled/msix_enabled, which is non-atomic and they may race
each other as they share the same memory region. A race condition
is met between driver bind vs FLR through sysfs:
for driver bind side in thread 1:
...
device_lock()
...
->probe()
pci_alloc_irq_vectors_affinity()
__pci_enable_msi_range()
msi_capability_init()
dev->msi_enabled=1 <---set here
request_irq(pci_irq_vector(),...)
when echo 1 > reset in thread 2:
pci_reset_function()
pci_dev_lock()
pci_cfg_access_lock()
dev->block_cfg_access=1 <---may overwrite msi_enabled bit
device_lock()
The msi_enabled bit may be overwritten to 0 and will trigger the WARN
assert in pci_irq_vector(). A similar issue has been addressed in
commit 44bda4b7d26e ("PCI: Fix is_added/is_busmaster race condition").
Move the block_cfg_access to the pci_dev->priv_flags and use atomic
bit operations to avoid the race condition.
Signed-off-by: Yicong Yang
---
drivers/pci/access.c | 20 ++--
drivers/pci/pci.h| 11 +++
include/linux/pci.h | 1 -
3 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 4693569..5826962 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -208,9 +208,9 @@ static noinline void pci_wait_cfg(struct pci_dev *dev)
{
do {
raw_spin_unlock_irq(&pci_lock);
- wait_event(pci_cfg_wait, !dev->block_cfg_access);
+ wait_event(pci_cfg_wait, !pci_dev_is_cfg_access_blocked(dev));
raw_spin_lock_irq(&pci_lock);
- } while (dev->block_cfg_access);
+ } while (pci_dev_is_cfg_access_blocked(dev));
}
/* Returns 0 on success, negative values indicate error. */
@@ -223,7 +223,7 @@ int pci_user_read_config_##size
\
if (PCI_##size##_BAD) \
return -EINVAL; \
raw_spin_lock_irq(&pci_lock); \
- if (unlikely(dev->block_cfg_access))\
+ if (unlikely(pci_dev_is_cfg_access_blocked(dev)))
\
pci_wait_cfg(dev); \
ret = dev->bus->ops->read(dev->bus, dev->devfn, \
pos, sizeof(type), &data); \
@@ -242,7 +242,7 @@ int pci_user_write_config_##size
\
if (PCI_##size##_BAD) \
return -EINVAL; \
raw_spin_lock_irq(&pci_lock); \
- if (unlikely(dev->block_cfg_access))\
+ if (unlikely(pci_dev_is_cfg_access_blocked(dev)))
\
pci_wait_cfg(dev); \
ret = dev->bus->ops->write(dev->bus, dev->devfn,\
pos, sizeof(type), val);\
@@ -271,9 +271,9 @@ void pci_cfg_access_lock(struct pci_dev *dev)
might_sleep();
raw_spin_lock_irq(&pci_lock);
- if (dev->block_cfg_access)
+ if (pci_dev_is_cfg_access_blocked(dev))
pci_wait_cfg(dev);
- dev->block_cfg_access = 1;
+ pci_dev_block_cfg_access(dev, true);
raw_spin_unlock_irq(&pci_lock);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_lock);
@@ -292,10 +292,10 @@ bool pci_cfg_access_trylock(struct pci_dev *dev)
bool locked = true;
raw_spin_lock_irqsave(&pci_lock, flags);
- if (dev->block_cfg_access)
+ if (pci_dev_is_cfg_access_blocked(dev))
locked = false;
else
- dev->block_cfg_access = 1;
+ pci_dev_block_cfg_access(dev, true);
raw_spin_unlock_irqrestore(&pci_lock, flags);
return locked;
@@ -318,9 +318,9 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
* This indicates a problem in the caller, but we don't need
* to kill them, unlike a double-block above.
*/
- WARN_ON(!dev->block_cfg_access);
+ WARN_ON(!pci_dev_is_cfg_access_blocked(dev));
- dev->block_cfg_access = 0;
+ pci_dev_block_cfg_access(dev, false);
raw_spin_unlock_irqrestore(&pci_lock, flags);
wake_up_all(&pci_cfg_wait);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 73740dd..1cf3122 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -410,6 +410,7 @@ static inline bool pci_dev_is_disconnected(const struct
pci_dev *dev)
/* pci_dev priv_flags */
#define PCI_DEV_ADDED 0
+#define PCI_DEV_BLOCK_CFG_ACCESS 1
Re: [PATCH] PCI: Factor functions of PCI function reset
+cc linux-pci as I forgot to
On 2020/9/7 19:16, Yicong Yang wrote:
> Previosly we use pci_probe_reset_function() to probe whehter a function
> can be reset and use __pci_reset_function_locked() to perform a function
> reset. These two functions have lots of common lines.
>
> Factor the two functions and reduce the redundancy.
>
> Signed-off-by: Yicong Yang
> ---
> drivers/pci/pci.c | 61
> -
> drivers/pci/pci.h | 2 +-
> drivers/pci/probe.c | 2 +-
> 3 files changed, 20 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e39c549..e3e5f0f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5006,9 +5006,11 @@ static void pci_dev_restore(struct pci_dev *dev)
> }
>
> /**
> - * __pci_reset_function_locked - reset a PCI device function while holding
> - * the @dev mutex lock.
> + * pci_probe_reset_function - check whether the device can be safely reset
> + *or reset a PCI device function while holding
> + *the @dev mutex lock.
> * @dev: PCI device to reset
> + * @probe: Probe or not whether the device can be reset.
> *
> * Some devices allow an individual function to be reset without affecting
> * other functions in the same device. The PCI device must be responsive
> @@ -5022,10 +5024,10 @@ static void pci_dev_restore(struct pci_dev *dev)
> * device including MSI, bus mastering, BARs, decoding IO and memory spaces,
> * etc.
> *
> - * Returns 0 if the device function was successfully reset or negative if the
> - * device doesn't support resetting a single function.
> + * Returns 0 if the device function can be reset or was successfully reset.
> + * negative if the device doesn't support resetting a single function.
> */
> -int __pci_reset_function_locked(struct pci_dev *dev)
> +int pci_probe_reset_function(struct pci_dev *dev, int probe)
> {
> int rc;
>
> @@ -5039,61 +5041,34 @@ int __pci_reset_function_locked(struct pci_dev *dev)
>* other error, we're also finished: this indicates that further
>* reset mechanisms might be broken on the device.
>*/
> - rc = pci_dev_specific_reset(dev, 0);
> + rc = pci_dev_specific_reset(dev, probe);
> if (rc != -ENOTTY)
> return rc;
> if (pcie_has_flr(dev)) {
> + if (probe)
> + return 0;
> rc = pcie_flr(dev);
> if (rc != -ENOTTY)
> return rc;
> }
> - rc = pci_af_flr(dev, 0);
> + rc = pci_af_flr(dev, probe);
> if (rc != -ENOTTY)
> return rc;
> - rc = pci_pm_reset(dev, 0);
> + rc = pci_pm_reset(dev, probe);
> if (rc != -ENOTTY)
> return rc;
> - rc = pci_dev_reset_slot_function(dev, 0);
> + rc = pci_dev_reset_slot_function(dev, probe);
> if (rc != -ENOTTY)
> return rc;
> - return pci_parent_bus_reset(dev, 0);
> +
> + return pci_parent_bus_reset(dev, probe);
> }
> -EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
>
> -/**
> - * pci_probe_reset_function - check whether the device can be safely reset
> - * @dev: PCI device to reset
> - *
> - * Some devices allow an individual function to be reset without affecting
> - * other functions in the same device. The PCI device must be responsive
> - * to PCI config space in order to use this function.
> - *
> - * Returns 0 if the device function can be reset or negative if the
> - * device doesn't support resetting a single function.
> - */
> -int pci_probe_reset_function(struct pci_dev *dev)
> +int __pci_reset_function_locked(struct pci_dev *dev)
> {
> - int rc;
> -
> - might_sleep();
> -
> - rc = pci_dev_specific_reset(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> - if (pcie_has_flr(dev))
> - return 0;
> - rc = pci_af_flr(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pci_pm_reset(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> - rc = pci_dev_reset_slot_function(dev, 1);
> - if (rc != -ENOTTY)
> - return rc;
> -
> - return pci_parent_bus_reset(dev, 1);
> + return pci_probe_reset_function(dev, 0);
> }
> +EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
>
> /**
> * pci_reset_function - quiesce and reset a PCI device function
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fa12f7c..73740dd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -39,7 +39,7 @@ enum pci_mm
Re: [PATCH] PCI: Fix race condition between block_cfg_access and msi_enabled/msix_enabled
+cc linux-pci as I forgot.
On 2020/9/7 19:06, Yicong Yang wrote:
> Previously we use bit field for block_cfg_access and
> msi_enabled/msix_enabled, which is non-atomic and they may race
> each other as they share the same memory region. A race condition
> is met between driver bind vs FLR through sysfs:
>
> for driver bind side in thread 1:
> ...
> device_lock()
> ...
> ->probe()
> pci_alloc_irq_vectors_affinity()
> __pci_enable_msi_range()
> msi_capability_init()
> dev->msi_enabled=1 <---set here
> request_irq(pci_irq_vector(),...)
>
> when echo 1 > reset in thread 2:
> pci_reset_function()
> pci_dev_lock()
> pci_cfg_access_lock()
> dev->block_cfg_access=1 <---may overwrite msi_enabled bit
> device_lock()
>
> The msi_enabled bit may be overwritten to 0 and will trigger the WARN
> assert in pci_irq_vector(). A similar issue has been addressed in
> commit 44bda4b7d26e ("PCI: Fix is_added/is_busmaster race condition").
>
> Move the block_cfg_access to the pci_dev->priv_flags and use atomic
> bit operations to avoid the race condition.
>
> Signed-off-by: Yicong Yang
> ---
> drivers/pci/access.c | 20 ++--
> drivers/pci/pci.h| 11 +++
> include/linux/pci.h | 1 -
> 3 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 4693569..5826962 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -208,9 +208,9 @@ static noinline void pci_wait_cfg(struct pci_dev *dev)
> {
> do {
> raw_spin_unlock_irq(&pci_lock);
> - wait_event(pci_cfg_wait, !dev->block_cfg_access);
> + wait_event(pci_cfg_wait, !pci_dev_is_cfg_access_blocked(dev));
> raw_spin_lock_irq(&pci_lock);
> - } while (dev->block_cfg_access);
> + } while (pci_dev_is_cfg_access_blocked(dev));
> }
>
> /* Returns 0 on success, negative values indicate error. */
> @@ -223,7 +223,7 @@ int pci_user_read_config_##size
> \
> if (PCI_##size##_BAD) \
> return -EINVAL; \
> raw_spin_lock_irq(&pci_lock); \
> - if (unlikely(dev->block_cfg_access))\
> + if (unlikely(pci_dev_is_cfg_access_blocked(dev)))
> \
> pci_wait_cfg(dev); \
> ret = dev->bus->ops->read(dev->bus, dev->devfn, \
> pos, sizeof(type), &data); \
> @@ -242,7 +242,7 @@ int pci_user_write_config_##size
> \
> if (PCI_##size##_BAD) \
> return -EINVAL; \
> raw_spin_lock_irq(&pci_lock); \
> - if (unlikely(dev->block_cfg_access))\
> + if (unlikely(pci_dev_is_cfg_access_blocked(dev)))
> \
> pci_wait_cfg(dev); \
> ret = dev->bus->ops->write(dev->bus, dev->devfn,\
> pos, sizeof(type), val);\
> @@ -271,9 +271,9 @@ void pci_cfg_access_lock(struct pci_dev *dev)
> might_sleep();
>
> raw_spin_lock_irq(&pci_lock);
> - if (dev->block_cfg_access)
> + if (pci_dev_is_cfg_access_blocked(dev))
> pci_wait_cfg(dev);
> - dev->block_cfg_access = 1;
> + pci_dev_block_cfg_access(dev, true);
> raw_spin_unlock_irq(&pci_lock);
> }
> EXPORT_SYMBOL_GPL(pci_cfg_access_lock);
> @@ -292,10 +292,10 @@ bool pci_cfg_access_trylock(struct pci_dev *dev)
> bool locked = true;
>
> raw_spin_lock_irqsave(&pci_lock, flags);
> - if (dev->block_cfg_access)
> + if (pci_dev_is_cfg_access_blocked(dev))
> locked = false;
> else
> - dev->block_cfg_access = 1;
> + pci_dev_block_cfg_access(dev, true);
> raw_spin_unlock_irqrestore(&pci_lock, flags);
>
> return locked;
> @@ -318,9 +318,9 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>* This indicates a problem in the caller, but we don't need
>* to kill them, unlike a double-block above.
>*/
> - WARN_ON(!dev->block_cfg_access);
> + WARN_ON(!pci_dev_is_cfg_access_blocked(dev));
>
Re: [RESEND PATCH] libfs: fix error cast of negative value in simple_attr_write()
Hi, Thanks for reviewing this. On 2020/11/11 3:18, Andrew Morton wrote: > On Tue, 10 Nov 2020 17:25:24 +0800 Yicong Yang > wrote: > >> The attr->set() receive a value of u64, but simple_strtoll() is used >> for doing the conversion. It will lead to the error cast if user inputs >> a negative value. >> >> Use kstrtoull() instead of simple_strtoll() to convert a string got >> from the user to an unsigned value. The former will return '-EINVAL' if >> it gets a negetive value, but the latter can't handle the situation >> correctly. >> >> ... >> >> --- a/fs/libfs.c >> +++ b/fs/libfs.c >> @@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char >> __user *buf, >> goto out; >> >> attr->set_buf[size] = '\0'; >> -val = simple_strtoll(attr->set_buf, NULL, 0); >> +ret = kstrtoull(attr->set_buf, 0, &val); >> +if (ret) >> +goto out; >> ret = attr->set(attr->data, val); >> if (ret == 0) >> ret = len; /* on success, claim we got the whole input */ > kstrtoull() takes an `unsigned long long *', but `val' is a u64. > > I think this probably works OK on all architectures (ie, no 64-bit > architectures are using `unsigned long' for u64). But perhaps `val' > should have type `unsigned long long'? the attr->set() takes 'val' as u64, so maybe we can stay it unchanged here if it works well. Thanks, Yicong > . >
[PATCH v2] libfs: fix error cast of negative value in simple_attr_write()
The attr->set() receive a value of u64, but simple_strtoll() is used
for doing the conversion. It will lead to the error cast if user inputs
a negative value.
Use kstrtoull() instead of simple_strtoll() to convert a string got
from the user to an unsigned value. The former will return '-EINVAL' if
it gets a negetive value, but the latter can't handle the situation
correctly.
Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
Signed-off-by: Yicong Yang
---
Change since v1:
- address the compile warning for non-64 bit platform
fs/libfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index fc34361..3a0d99c 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char
__user *buf,
goto out;
attr->set_buf[size] = '\0';
- val = simple_strtoll(attr->set_buf, NULL, 0);
+ ret = kstrtoull(attr->set_buf, 0, (unsigned long long *)&val);
+ if (ret)
+ goto out;
ret = attr->set(attr->data, val);
if (ret == 0)
ret = len; /* on success, claim we got the whole input */
--
2.8.1
[PATCH v3] libfs: fix error cast of negative value in simple_attr_write()
The attr->set() receive a value of u64, but simple_strtoll() is used
for doing the conversion. It will lead to the error cast if user inputs
a negative value.
Use kstrtoull() instead of simple_strtoll() to convert a string got
from the user to an unsigned value. The former will return '-EINVAL' if
it gets a negetive value, but the latter can't handle the situation
correctly. Make 'val' unsigned long long as what kstrtoull() takes, this
will eliminate the compile warning on no 64-bit architectures.
Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
Signed-off-by: Yicong Yang
---
Change since v1:
- address the compile warning for non-64 bit platform.
Change since v2:
Link:
https://lore.kernel.org/linux-fsdevel/1605000324-7428-1-git-send-email-yangyic...@hisilicon.com/
- make 'val' unsigned long long and mentioned in the commit
Link:
https://lore.kernel.org/linux-fsdevel/1605261369-551-1-git-send-email-yangyic...@hisilicon.com/
fs/libfs.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index fc34361..7124c2e 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -959,7 +959,7 @@ ssize_t simple_attr_write(struct file *file, const char
__user *buf,
size_t len, loff_t *ppos)
{
struct simple_attr *attr;
- u64 val;
+ unsigned long long val;
size_t size;
ssize_t ret;
@@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char
__user *buf,
goto out;
attr->set_buf[size] = '\0';
- val = simple_strtoll(attr->set_buf, NULL, 0);
+ ret = kstrtoull(attr->set_buf, 0, &val);
+ if (ret)
+ goto out;
ret = attr->set(attr->data, val);
if (ret == 0)
ret = len; /* on success, claim we got the whole input */
--
2.8.1
[RESEND PATCH] libfs: fix error cast of negative value in simple_attr_write()
The attr->set() receive a value of u64, but simple_strtoll() is used
for doing the conversion. It will lead to the error cast if user inputs
a negative value.
Use kstrtoull() instead of simple_strtoll() to convert a string got
from the user to an unsigned value. The former will return '-EINVAL' if
it gets a negetive value, but the latter can't handle the situation
correctly.
Fixes: f7b88631a897 ("fs/libfs.c: fix simple_attr_write() on 32bit machines")
Signed-off-by: Yicong Yang
---
fs/libfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index fc34361..2dcf40e 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -977,7 +977,9 @@ ssize_t simple_attr_write(struct file *file, const char
__user *buf,
goto out;
attr->set_buf[size] = '\0';
- val = simple_strtoll(attr->set_buf, NULL, 0);
+ ret = kstrtoull(attr->set_buf, 0, &val);
+ if (ret)
+ goto out;
ret = attr->set(attr->data, val);
if (ret == 0)
ret = len; /* on success, claim we got the whole input */
--
2.8.1
Re: [PATCH v12 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe controller errors
Hi Bjorn,
Thanks for the comments.
On 2020/7/15 5:10, Bjorn Helgaas wrote:
> [+cc Lorenzo]
>
> On Mon, Jul 13, 2020 at 03:10:19PM +0100, Shiju Jose wrote:
>> From: Yicong Yang
>>
>> The HiSilicon HIP PCIe controller is capable of handling errors
>> on root port and perform port reset separately at each root port.
> s/perform/performing/ (to match "handling")
>
>> The driver placed in the drivers/pci/controller/ because the
>> HIP PCIe controller does not use DWC ip.
> s/ip/IP/
will fix these.
> +#define HISI_PCIE_LOCAL_VALID_ERR_MISC 9
> +
> +static guid_t hisi_pcie_sec_guid =
> + GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
> + 0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
> +
> +/*
> + * We pass core id and core port id to the ACPI reset method to identify
> + * certain root port to reset, while the firmware reports sockets port
> + * id which occurs an error. Use the macros here to do the conversion
> Maybe:
>
> Firmware reports the socket port ID where the error occurred. These
> macros convert that to the core ID and core port ID required by the
> ACPI reset method.
>
> But even that doesn't quite make sense because you apparently get two
> values (edata->core_id, edata->port_id) from firmware.
will reword the comments.
Actually We have got the socket_id from the firmware, and we use it to find the
correct
error handler device on the same socket in hisi_pcie_notify_error(). As for
port id and core port id, the driver got the port id indexed per socket, but
the firmware
needs the port id indexed per core to locate the right register, so we need
these macros
to do the conversion.
>> + */
>> +#define HISI_PCIE_CORE_ID(v) ((v) >> 3)
>> +#define HISI_PCIE_PORT_ID(core, v) (((v) >> 1) + ((core) << 3))
>> +#define HISI_PCIE_CORE_PORT_ID(v)(((v) & 7) << 1)
> These would make more sense reordered and with HISI_PCIE_PORT_ID()
> rewritten like this:
>
> #define HISI_PCIE_PORT_ID(core, v) (((core) << 3) | ((v) >> 1))
> #define HISI_PCIE_CORE_ID(v) ((v) >> 3)
> #define HISI_PCIE_CORE_PORT_ID(v)(((v) & 7) << 1)
will reorder these.
Regards,
Yicong
>
>> +
>> +struct hisi_pcie_error_data {
>> +u64 val_bits;
>> +u8 version;
>> +u8 soc_id;
>> +u8 socket_id;
>> +u8 nimbus_id;
>> +u8 sub_module_id;
>> +u8 core_id;
>> +u8 port_id;
>> +u8 err_severity;
>> +u16 err_type;
>> +u8 reserv[2];
>> +u32 err_misc[HISI_PCIE_ERR_MISC_REGS];
>> +};
>> +
>> +struct hisi_pcie_error_private {
>> +struct notifier_block nb;
>> +struct device *dev;
>> +};
>> +
>> +enum hisi_pcie_submodule_id {
>> +HISI_PCIE_SUB_MODULE_ID_AP,
>> +HISI_PCIE_SUB_MODULE_ID_TL,
>> +HISI_PCIE_SUB_MODULE_ID_MAC,
>> +HISI_PCIE_SUB_MODULE_ID_DL,
>> +HISI_PCIE_SUB_MODULE_ID_SDI,
>> +};
>> +
>> +static const char * const hisi_pcie_sub_module[] = {
>> +[HISI_PCIE_SUB_MODULE_ID_AP]= "AP Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_TL]= "TL Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_MAC] = "MAC Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_DL]= "DL Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_SDI] = "SDI Layer",
>> +};
>> +
>> +enum hisi_pcie_err_severity {
>> +HISI_PCIE_ERR_SEV_RECOVERABLE,
>> +HISI_PCIE_ERR_SEV_FATAL,
>> +HISI_PCIE_ERR_SEV_CORRECTED,
>> +HISI_PCIE_ERR_SEV_NONE,
>> +};
>> +
>> +static const char * const hisi_pcie_error_sev[] = {
>> +[HISI_PCIE_ERR_SEV_RECOVERABLE] = "recoverable",
>> +[HISI_PCIE_ERR_SEV_FATAL] = "fatal",
>> +[HISI_PCIE_ERR_SEV_CORRECTED] = "corrected",
>> +[HISI_PCIE_ERR_SEV_NONE]= "none",
>> +};
>> +
>> +static const char *hisi_pcie_get_string(const char * const *array,
>> +size_t n, u32 id)
>> +{
>> +u32 index;
>> +
>> +for (index = 0; index < n; index++) {
>> +if (index == id && array[index])
>> +return array[index];
>> +}
>> +
>> +return "unknown";
>> +}
>> +
>> +static int hisi_pcie_port_reset(struct platform_device *pdev,
>> +u32 chip_id, u32 port_id)
>> +{
>> +str
Re: [RFC PATCH] hwtracing: Add HiSilicon PCIe Tune and Trace device driver
On 2020/7/17 5:31, Bjorn Helgaas wrote:
> On Thu, Jul 16, 2020 at 05:06:19PM +0800, Yicong Yang wrote:
>> On 2020/7/11 7:09, Bjorn Helgaas wrote:
>>> On Sat, Jun 13, 2020 at 05:32:13PM +0800, Yicong Yang wrote:
>>>> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex
>>>> integrated Endpoint(RCiEP) device, providing the capability
>>>> to dynamically monitor and tune the PCIe traffic parameters(tune),
>>>> and trace the TLP headers to the memory(trace).
>>>>
>>>> Add the driver for the device to enable its functions. The driver
>>>> will create debugfs directory for each PTT device, and users can
>>>> operate the device through the files under its directory.
>>>> +Tune
>>>> +
>>>> +
>>>> +PTT tune is designed for monitoring and adjusting PCIe link
>>>> parameters(events).
>>>> +Currently we support events 4 classes. The scope of the events
>>>> +covers the PCIe core with which the PTT device belongs to.
>>> All of these look like things that have the potential to break the
>>> PCIe protocol and cause errors like timeouts, receiver overflows, etc.
>>> That's OK for a debug/analysis situation, but it should taint the
>>> kernel somehow because I don't want to debug problems like that if
>>> they're caused by users tweaking things.
>>>
>>> That might even be a reason *not* to merge the tune side of this. I
>>> can see how it might be useful for you internally, but it's not
>>> obvious to me how it will benefit other users. Maybe that part should
>>> be an out-of-tree module?
>> All the tuning values are not accurate, but abstracted to several
>> _levels_ of each events. The levels are delicately designed to
>> guarantee by the hardware that they are always valid and will not
>> break the PCIe link. The possible level values exposed to the users
>> is tested and safe and other values will not be accepted.
>>
>> The final tuning events is not settled and we'll not exposed the
>> events which will may lead to the link broken. Furthermore, maybe we
>> could default disable the tune events' level adjustment and make
>> them readonly. The user can enable the full tune function by a
>> module parameters or in the BIOS, and a warning message will be
>> displayed.
>>
>> The tune part is beneficial for the users and not only for our
>> internal use. We intends to provide a way to tune the link
>> depending on the downstream components and link configuration. For
>> example, users can tune the data path QoS level to get better
>> performance according to the link width is x8 or x16, or according
>> to the endpoints' class is a network card or a nvme disk. It will
>> make our controller adapt to different condition with high
>> performance, so we hope this feature to be merged.
> OK. This driver itself is outside my area, so I guess merging it is
> up to Alexander.
>
> Do you have any measurements of performance improvements? I think it
> would be good to have real numbers showing that this is useful.
>
> You mentioned a warning message, so I assume you'll add some kind of
> dmesg logging when tuning happens?
>
> Is this protected so it's only usable by root or other appropriate
> privileged user?
We haven't got measurement statistic currently as the device is still in
progress. We can measure the improvements when it's finalized.
I suppose to add some info/warning messages in dmesg log when tune happens.
The whole PTT functions are accessible only by root.
>
>>>> + * The PTT can designate function for trace.
>>>> + * Add the root port's subordinates in the list as we
>>>> + * can specify certain function.
>>>> + */
>>>> + child_bus = tpdev->subordinate;
>>>> + list_for_each_entry(tpdev, &child_bus->devices, bus_list) {
>>> *This* looks like a potential problem with hotplug. How do you deal
>>> with devices being added/removed after this loop?
>> Yes. I have considered the add/remove situation but not intend to address it
>> in this RFC and assume the topology is static after probing.
>> I will manage the situation in next version.
> What happens if a device is added or removed after boot? If the only
> limitation is that you can't tune or trace a hot-added device, that's
> fine. (I mean, it's really *not* fine because it's a poor user
> experience, but at least it's just a u
Re: [RFC PATCH] hwtracing: Add HiSilicon PCIe Tune and Trace device driver
Hi Bjorn,
Thanks for reviewing this. Some replies below.
Thanks,
Yicong
On 2020/7/11 7:09, Bjorn Helgaas wrote:
> On Sat, Jun 13, 2020 at 05:32:13PM +0800, Yicong Yang wrote:
>> HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex
>> integrated Endpoint(RCiEP) device, providing the capability
>> to dynamically monitor and tune the PCIe traffic parameters(tune),
>> and trace the TLP headers to the memory(trace).
>>
>> Add the driver for the device to enable its functions. The driver
>> will create debugfs directory for each PTT device, and users can
>> operate the device through the files under its directory.
>>
>> RFC:
>> - The hardware interface is not yet finalized.
>> - The interface to the users is through debugfs, and the usage will
>> be further illustrated in the document.
>> - The driver is intended to be put under drivers/hwtracing, where
>> we think best match the device's function.
>>
>> Signed-off-by: Yicong Yang
>> ---
>> Documentation/trace/hisi-ptt.rst | 272
>> drivers/hwtracing/Kconfig |2 +
>> drivers/hwtracing/hisilicon/Kconfig|8 +
>> drivers/hwtracing/hisilicon/Makefile |2 +
>> drivers/hwtracing/hisilicon/hisi_ptt.c | 1172
>>
>> 5 files changed, 1456 insertions(+)
>> create mode 100644 Documentation/trace/hisi-ptt.rst
>> create mode 100644 drivers/hwtracing/hisilicon/Kconfig
>> create mode 100644 drivers/hwtracing/hisilicon/Makefile
>> create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c
>>
>> diff --git a/Documentation/trace/hisi-ptt.rst
>> b/Documentation/trace/hisi-ptt.rst
>> new file mode 100644
>> index 000..c99fbf9
>> --- /dev/null
>> +++ b/Documentation/trace/hisi-ptt.rst
>> @@ -0,0 +1,272 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +==
>> +HiSilicon PCIe Tune and Trace device
>> +==
>> +
>> +Introduction
>> +
>> +
>> +HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex
>> +integrated Endpoint(RCiEP) device, providing the capability
>> +to dynamically monitor and tune the PCIe link's events(tune),
>> +and trace the TLP headers to the memory(trace). The two functions
>> +are inpendent, but is recommended to use them together to analyze
>> +and enhance the PCIe link's performance.
> Add a space before "(". Many occurrences above and below, too.
>
> s/inpendent/independent/
>
> What does "TLP headers to the memory" mean? TLPs can go either
> upstream (i.e., device DMA to system memory) or downstream (i.e., CPU
> MMIO writes to device). It sounds like maybe you can trace only the
> upstream ones heading toward system memory?
It's capable of tracing TLPs both upstream and downstream. I'll modify
the statement here.
>
>> +On Hip09, the PCIe root complex is composed of several PCIe cores.
>> +And each core is composed of several root ports, RCiEPs, and one
>> +PTT device, like below. The PTT device is capable of tuning and
>> +tracing the link on and downstream the PCIe core.
>> +::
>> + +--Core 0---+
>> + | | [ PTT ] |
>> + | | [Root Port]---[Endpoint]
>> + | | [Root Port]---[Endpoint]
>> + | | [Root Port]---[Endpoint]
>> +Root Complex |--Core 1---+
>> + | | [ PTT ] |
>> + | | [Root Port]---[ Switch ]---[Endpoint]
>> + | | [Root Port]---[Endpoint] `-[Endpoint]
>> + | | [Root Port]---[Endpoint]
>> + +---+
> I guess this means each PTT can tune/trace all the Root Ports in the
> same PCIe core?
Yes.
>> +The PTT device driver cannot be loaded if debugfs is not mounted.
>> +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt
>> +as its root directory, with name of its BDF number.
>> +::
>> +
>> +/sys/kernel/debug/hisi_ptt/::.
>> +
>> +Tune
>> +
>> +
>> +PTT tune is designed for monitoring and adjusting PCIe link
>> parameters(events).
>> +Currently we support events 4 classes. The scope of the events
>> +covers the PCIe core with which the PTT device belongs to.
> All of these look like things that have the potential to break the
> PCIe protocol and cause errors like timeouts, receiver overflows, etc.
> That's OK for a d
Re: [PATCH -next] i2c: efm32: Use devm_platform_get_and_ioremap_resource()
On 2020/9/18 16:25, Wang ShaoBo wrote:
> Make use of devm_platform_get_and_ioremap_resource() provided by
> driver core platform instead of duplicated analogue. dev_err() is
> removed because it has been done in devm_ioremap_resource().
>
> Signed-off-by: Wang ShaoBo
> ---
> drivers/i2c/busses/i2c-efm32.c | 12 +++-
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-efm32.c b/drivers/i2c/busses/i2c-efm32.c
> index 838ce0947191..f6e13ceeb2b3 100644
> --- a/drivers/i2c/busses/i2c-efm32.c
> +++ b/drivers/i2c/busses/i2c-efm32.c
> @@ -332,21 +332,15 @@ static int efm32_i2c_probe(struct platform_device *pdev)
> return ret;
> }
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "failed to determine base address\n");
> - return -ENODEV;
> - }
> + ddata->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> + if (IS_ERR(ddata->base))
> + return PTR_ERR(ddata->base);
>
> if (resource_size(res) < 0x42) {
res is not assigned. should it be removed?
> dev_err(&pdev->dev, "memory resource too small\n");
> return -EINVAL;
> }
>
> - ddata->base = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(ddata->base))
> - return PTR_ERR(ddata->base);
> -
> ret = platform_get_irq(pdev, 0);
> if (ret <= 0) {
> if (!ret)
Re: [PATCH v2 2/2] PCI/ERR: Add reset support for non fatal errors
Hi Sathy, one minor comments below. On 2020/6/5 5:50, sathyanarayanan.kuppusw...@linux.intel.com wrote: > From: Kuppuswamy Sathyanarayanan > > PCI_ERS_RESULT_NEED_RESET error status implies the device is > requesting a slot reset and a notification about slot reset > completion via ->slot_reset() callback. > > But in non-fatal errors case, if report_error_detected() or > report_mmio_enabled() functions requests PCI_ERS_RESULT_NEED_RESET > then current pcie_do_recovery() implementation does not do the > requested explicit slot reset, instead just calls the ->slot_reset() > callback on all affected devices. Notifying about the slot reset > completion without resetting it incorrect. So add this support. > > Signed-off-by: Kuppuswamy Sathyanarayanan > > --- > drivers/pci/pcie/err.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c > index 5fe8561c7185..94d1c2ff7b40 100644 > --- a/drivers/pci/pcie/err.c > +++ b/drivers/pci/pcie/err.c > @@ -206,6 +206,9 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, >* functions to reset slot before calling >* drivers' slot_reset callbacks? >*/ > + if (state != pci_channel_io_frozen) > + pci_reset_bus(dev); > + If it's the implementation to reset the slot, should we remove the TODO comments? JYI. Thanks, Yicong > status = PCI_ERS_RESULT_RECOVERED; > pci_dbg(dev, "broadcast slot_reset message\n"); > pci_walk_bus(bus, report_slot_reset, &status);
Re: [PATCH v2 1/2] PCI/ERR: Fix fatal error recovery for non-hotplug capable devices
Hi Jay,
I've tested the patches on my board, and they work well.
Thanks,
Yicong
On 2020/6/25 2:52, Jay Vosburgh wrote:
> Jay Vosburgh wrote:
>
>> sathyanarayanan.kuppusw...@linux.intel.com wrote:
>>
>> From: Kuppuswamy Sathyanarayanan
>>> Fatal (DPC) error recovery is currently broken for non-hotplug
>>> capable devices. With current implementation, after successful
>>> fatal error recovery, non-hotplug capable device state won't be
>>> restored properly. You can find related issues in following links.
>>>
>>> https://lkml.org/lkml/2020/5/27/290
>>> https://lore.kernel.org/linux-pci/12115.1588207324@famine/
>>> https://lkml.org/lkml/2020/3/28/328
>>>
>>> Current fatal error recovery implementation relies on hotplug handler
>>> for detaching/re-enumerating the affected devices/drivers on DLLSC
>>> state changes. So when dealing with non-hotplug capable devices,
>>> recovery code does not restore the state of the affected devices
>>> correctly. Correct implementation should call report_slot_reset()
>>> function after resetting the link to restore the state of the
>>> device/driver.
>>>
>>> So use PCI_ERS_RESULT_NEED_RESET as error status for successful
>>> reset_link() operation and use PCI_ERS_RESULT_DISCONNECT for failure
>>> case. PCI_ERS_RESULT_NEED_RESET error state will ensure slot_reset()
>>> is called after reset link operation which will also fix the above
>>> mentioned issue.
>>>
>>> [original patch is from jay.vosbu...@canonical.com]
>>> [original patch link
>>> https://lore.kernel.org/linux-pci/12115.1588207324@famine/]
>>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>>> Signed-off-by: Jay Vosburgh
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>>
>> I've tested this patch set on one of our test machines, and it
>> resolves the issue. I plan to test with other systems tomorrow.
> I've done testing on two different systems that exhibit the
> original issue and this patch set appears to behave as expected.
>
> Has anyone else (Yicong?) had an opportunity to test this?
>
> Can this be considered for acceptance, or is additional feedback
> or review needed?
>
> -J
>
>>> ---
>>> drivers/pci/pcie/err.c | 24 ++--
>>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>> index 14bb8f54723e..5fe8561c7185 100644
>>> --- a/drivers/pci/pcie/err.c
>>> +++ b/drivers/pci/pcie/err.c
>>> @@ -165,8 +165,28 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>> pci_dbg(dev, "broadcast error_detected message\n");
>>> if (state == pci_channel_io_frozen) {
>>> pci_walk_bus(bus, report_frozen_detected, &status);
>>> - status = reset_link(dev);
>>> - if (status != PCI_ERS_RESULT_RECOVERED) {
>>> + /*
>>> +* After resetting the link using reset_link() call, the
>>> +* possible value of error status is either
>>> +* PCI_ERS_RESULT_DISCONNECT (failure case) or
>>> +* PCI_ERS_RESULT_NEED_RESET (success case).
>>> +* So ignore the return value of report_error_detected()
>>> +* call for fatal errors. Instead use
>>> +* PCI_ERS_RESULT_NEED_RESET as initial status value.
>>> +*
>>> +* Ignoring the status return value of report_error_detected()
>>> +* call will also help in case of EDR mode based error
>>> +* recovery. In EDR mode AER and DPC Capabilities are owned by
>>> +* firmware and hence report_error_detected() call will possibly
>>> +* return PCI_ERS_RESULT_NO_AER_DRIVER. So if we don't ignore
>>> +* the return value of report_error_detected() then
>>> +* pcie_do_recovery() would report incorrect status after
>>> +* successful recovery. Ignoring PCI_ERS_RESULT_NO_AER_DRIVER
>>> +* in non EDR case should not have any functional impact.
>>> +*/
>>> + status = PCI_ERS_RESULT_NEED_RESET;
>>> + if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED) {
>>> + status = PCI_ERS_RESULT_DISCONNECT;
>>> pci_warn(dev, "link reset failed\n");
>>> goto failed;
>>> }
>>> --
>>> 2.17.1
> ---
> -Jay Vosburgh, jay.vosbu...@canonical.com
> .
>
Re: [PATCH v1 1/1] PCI/ERR: Handle fatal error recovery for non-hotplug capable devices
On 2020/5/13 3:20, Jay Vosburgh wrote:
> sathyanarayanan.kuppusw...@linux.intel.com wrote:
>
>> From: Kuppuswamy Sathyanarayanan
>>
>> If there are non-hotplug capable devices connected to a given
>> port, then during the fatal error recovery(triggered by DPC or
>> AER), after calling reset_link() function, we cannot rely on
>> hotplug handler to detach and re-enumerate the device drivers
>> in the affected bus. Instead, we will have to let the error
>> recovery handler call report_slot_reset() for all devices in
>> the bus to notify about the reset operation. Although this is
>> only required for non hot-plug capable devices, doing it for
>> hotplug capable devices should not affect the functionality.
> Yicong,
>
> Does the patch below also resolve the issue for you, as with
> your changed version of my original patch?
Yes. It works.
>
> -J
>
>> Along with above issue, this fix also applicable to following
>> issue.
>>
>> Commit 6d2c89441571 ("PCI/ERR: Update error status after
>> reset_link()") added support to store status of reset_link()
>> call. Although this fixed the error recovery issue observed if
>> the initial value of error status is PCI_ERS_RESULT_DISCONNECT
>> or PCI_ERS_RESULT_NO_AER_DRIVER, it also discarded the status
>> result from report_frozen_detected. This can cause a failure to
>> recover if _NEED_RESET is returned by report_frozen_detected and
>> report_slot_reset is not invoked.
>>
>> Such an event can be induced for testing purposes by reducing the
>> Max_Payload_Size of a PCIe bridge to less than that of a device
>> downstream from the bridge, and then initiating I/O through the
>> device, resulting in oversize transactions. In the presence of DPC,
>> this results in a containment event and attempted reset and recovery
>> via pcie_do_recovery. After 6d2c89441571 report_slot_reset is not
>> invoked, and the device does not recover.
>>
>> [original patch is from jay.vosbu...@canonical.com]
>> [original patch link
>> https://lore.kernel.org/linux-pci/18609.1588812972@famine/]
>> Fixes: 6d2c89441571 ("PCI/ERR: Update error status after reset_link()")
>> Signed-off-by: Jay Vosburgh
>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>
>> ---
>> drivers/pci/pcie/err.c | 19 +++
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>> index 14bb8f54723e..db80e1ecb2dc 100644
>> --- a/drivers/pci/pcie/err.c
>> +++ b/drivers/pci/pcie/err.c
>> @@ -165,13 +165,24 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>> pci_dbg(dev, "broadcast error_detected message\n");
>> if (state == pci_channel_io_frozen) {
>> pci_walk_bus(bus, report_frozen_detected, &status);
>> -status = reset_link(dev);
>> -if (status != PCI_ERS_RESULT_RECOVERED) {
>> +status = PCI_ERS_RESULT_NEED_RESET;
>> +} else {
>> +pci_walk_bus(bus, report_normal_detected, &status);
>> +}
>> +
>> +if (status == PCI_ERS_RESULT_NEED_RESET) {
>> +if (reset_link) {
>> +if (reset_link(dev) != PCI_ERS_RESULT_RECOVERED)
>> +status = PCI_ERS_RESULT_DISCONNECT;
>> +} else {
>> +if (pci_bus_error_reset(dev))
>> +status = PCI_ERS_RESULT_DISCONNECT;
>> +}
>> +
>> +if (status == PCI_ERS_RESULT_DISCONNECT) {
>> pci_warn(dev, "link reset failed\n");
>> goto failed;
>> }
>> -} else {
>> -pci_walk_bus(bus, report_normal_detected, &status);
>> }
>>
>> if (status == PCI_ERS_RESULT_CAN_RECOVER) {
>> --
>> 2.17.1
>>
> ---
> -Jay Vosburgh, jay.vosbu...@canonical.com
> .
>
Re: [RFC PATCH] hwtracing: Add HiSilicon PCIe Tune and Trace device driver
Hi all, As it's a new tracing device for PCIe traffic, any comments about the driver or the user interface is appreciated. Thanks. On 2020/6/13 17:32, Yicong Yang wrote: > HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex > integrated Endpoint(RCiEP) device, providing the capability > to dynamically monitor and tune the PCIe traffic parameters(tune), > and trace the TLP headers to the memory(trace). > > Add the driver for the device to enable its functions. The driver > will create debugfs directory for each PTT device, and users can > operate the device through the files under its directory. > > RFC: > - The hardware interface is not yet finalized. > - The interface to the users is through debugfs, and the usage will > be further illustrated in the document. > - The driver is intended to be put under drivers/hwtracing, where > we think best match the device's function. > > Signed-off-by: Yicong Yang > --- > Documentation/trace/hisi-ptt.rst | 272 > drivers/hwtracing/Kconfig |2 + > drivers/hwtracing/hisilicon/Kconfig|8 + > drivers/hwtracing/hisilicon/Makefile |2 + > drivers/hwtracing/hisilicon/hisi_ptt.c | 1172 > > 5 files changed, 1456 insertions(+) > create mode 100644 Documentation/trace/hisi-ptt.rst > create mode 100644 drivers/hwtracing/hisilicon/Kconfig > create mode 100644 drivers/hwtracing/hisilicon/Makefile > create mode 100644 drivers/hwtracing/hisilicon/hisi_ptt.c > > diff --git a/Documentation/trace/hisi-ptt.rst > b/Documentation/trace/hisi-ptt.rst > new file mode 100644 > index 000..c99fbf9 > --- /dev/null > +++ b/Documentation/trace/hisi-ptt.rst > @@ -0,0 +1,272 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +== > +HiSilicon PCIe Tune and Trace device > +== > + > +Introduction > + > + > +HiSilicon PCIe tune and trace device(PTT) is a PCIe Root Complex > +integrated Endpoint(RCiEP) device, providing the capability > +to dynamically monitor and tune the PCIe link's events(tune), > +and trace the TLP headers to the memory(trace). The two functions > +are inpendent, but is recommended to use them together to analyze > +and enhance the PCIe link's performance. > + > +On Hip09, the PCIe root complex is composed of several PCIe cores. > +And each core is composed of several root ports, RCiEPs, and one > +PTT device, like below. The PTT device is capable of tuning and > +tracing the link on and downstream the PCIe core. > +:: > + +--Core 0---+ > + | | [ PTT ] | > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > + | | [Root Port]---[Endpoint] > +Root Complex |--Core 1---+ > + | | [ PTT ] | > + | | [Root Port]---[ Switch ]---[Endpoint] > + | | [Root Port]---[Endpoint] `-[Endpoint] > + | | [Root Port]---[Endpoint] > + +---+ > + > +The PTT device driver cannot be loaded if debugfs is not mounted. > +Each PTT device will be presented under /sys/kernel/debugfs/hisi_ptt > +as its root directory, with name of its BDF number. > +:: > + > +/sys/kernel/debug/hisi_ptt/::. > + > +Tune > + > + > +PTT tune is designed for monitoring and adjusting PCIe link > parameters(events). > +Currently we support events 4 classes. The scope of the events > +covers the PCIe core with which the PTT device belongs to. > + > +Each event is presented as a file under $(PTT root dir)/$(BDF)/tune, and > +mostly this will be a simple open/read/write/close cycle to tune > +the event. > +:: > +$ cd /sys/kernel/debug/hisi_ptt/$(BDF)/tune > +$ ls > +buf_rx_cpld buf_rx_pd dllp_link_ack_freq link_credit_rx_cplh > +link_credit_rx_ph qos_tx_dp qos_tx_dp > +$ cat qos_tx_dp > +100 > +$ echo 50 > qos_tx_dp > +$ cat qos_tx_dp > +50 > + > +Current value(numerical value) of the event can be get by simply > +read the file, and write the desired value to the file to tune. > +Tune multiple events at the same time is not permitted, which means > +you cannot read or write more than one tune file at one time. > + > +1. Link credit control > +-- > + > +Following files are provided for tune the link credit events of the PCIe > core. > +PCIe link uses credit to control the flow, refer to the PCIe Spec for further > +information. > + > +- link_credit_rx_nph: r
