Latest AMDGPU NAVI cards have USB Type-C interface which can be accessed
over I2C. The Type-C controller has integrated designware i2c which is
exposed as a PCI device to the AMD platform.
Also there exists couple of notable IP problems that are dealt as a
workaround:
- I2C transactions work on a polling mode as IP does not generate
interrupt.
- I2C reads commands are sent twice to address the IP issues.
Reviewed-by: Shyam Sundar S K <[email protected]>
Co-developed-by: Nehal Bakulchandra Shah <[email protected]>
Signed-off-by: Nehal Bakulchandra Shah <[email protected]>
Signed-off-by: Sanket Goswami <[email protected]>
---
drivers/i2c/busses/i2c-designware-common.c | 3 +
drivers/i2c/busses/i2c-designware-core.h | 3 +
drivers/i2c/busses/i2c-designware-master.c | 175 +++++++++++++++++++++
drivers/i2c/busses/i2c-designware-pcidrv.c | 56 +++++++
4 files changed, 237 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 3c19aada4b30..50883a70b482 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -150,6 +150,9 @@ int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
reg = readl(dev->base + DW_IC_COMP_TYPE);
i2c_dw_release_lock(dev);
+ if (dev->flags & AMD_NON_INTR_MODE)
+ map_cfg.max_register = AMD_UCSI_INTR_REG;
+
if (reg == swab32(DW_IC_COMP_TYPE_VALUE)) {
map_cfg.reg_read = dw_reg_read_swab;
map_cfg.reg_write = dw_reg_write_swab;
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 85307cfa7109..0cce4144b9ac 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -291,9 +291,12 @@ struct dw_i2c_dev {
#define ACCESS_INTR_MASK BIT(0)
#define ACCESS_NO_IRQ_SUSPEND BIT(1)
+#define AMD_NON_INTR_MODE BIT(2)
#define MODEL_MSCC_OCELOT BIT(8)
#define MODEL_BAIKAL_BT1 BIT(9)
#define MODEL_MASK GENMASK(11, 8)
+#define AMD_UCSI_INTR_EN 0xD
+#define AMD_UCSI_INTR_REG 0x474
int i2c_dw_init_regmap(struct dw_i2c_dev *dev);
u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index d6425ad6e6a3..35c894a01381 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -22,6 +22,10 @@
#include <linux/reset.h>
#include "i2c-designware-core.h"
+#define AMD_TIMEOUT_MSEC_MIN 10000
+#define AMD_TIMEOUT_MSEC_MAX 11000
+#define AMD_FIFO_SIZE 0x20
+#define AMD_MASTERCFG_MASK GENMASK(15, 0)
static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
{
@@ -33,6 +37,154 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
regmap_write(dev->map, DW_IC_CON, dev->master_cfg);
}
+static void i2c_dw_configure_bus(struct dw_i2c_dev *i2cd)
+{
+ int master_cfg;
+ u16 icon;
+ u32 reg;
+
+ /* First disable the controller */
+ __i2c_dw_disable_nowait(i2cd);
+ master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE | DW_IC_CON_SPEED_STD;
+
+ /* Clear all the interrupts */
+ regmap_read(i2cd->map, DW_IC_CLR_INTR, ®);
+ regmap_write(i2cd->map, DW_IC_INTR_MASK, STATUS_IDLE);
+
+ icon = master_cfg & AMD_MASTERCFG_MASK;
+ icon &= ~DW_IC_CON_10BITADDR_MASTER;
+ icon = icon | DW_IC_CON_SPEED_STD;
+
+ /* Configure the master */
+ regmap_write(i2cd->map, DW_IC_CON, icon);
+ /* Configure the FIFO */
+ regmap_write(i2cd->map, DW_IC_TX_TL, AMD_FIFO_SIZE);
+ regmap_write(i2cd->map, DW_IC_RX_TL, AMD_FIFO_SIZE);
+
+ /* Setup 100k Speed */
+ regmap_write(i2cd->map, DW_IC_SS_SCL_HCNT, i2cd->ss_hcnt);
+ regmap_write(i2cd->map, DW_IC_SS_SCL_LCNT, i2cd->ss_lcnt);
+ regmap_write(i2cd->map, DW_IC_TAR, 0x08);
+
+ /* Now enable the controller */
+ __i2c_dw_enable(i2cd);
+}
+
+static int i2c_dw_check_stopbit(struct dw_i2c_dev *i2cd)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read_poll_timeout(i2cd->map, DW_IC_INTR_STAT, val,
+ !(val & DW_IC_INTR_STOP_DET),
+ 1100, 20000);
+ if (ret) {
+ dev_err(i2cd->dev, "i2c timeout error %d\n", ret);
+ return -ETIMEDOUT;
+ }
+
+ regmap_read(i2cd->map, DW_IC_CLR_INTR, &val);
+ if (val & DW_IC_INTR_STOP_DET)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static int i2c_dw_status(struct dw_i2c_dev *i2cd)
+{
+ int status;
+
+ status = i2c_dw_wait_bus_not_busy(i2cd);
+ if (status)
+ return -ETIMEDOUT;
+
+ status = i2c_dw_check_stopbit(i2cd);
+ if (status)
+ return -ETIMEDOUT;
+
+ return status;
+}
+
+/* Initiate and continue master read/write transaction with polling
+ * based transfer routine and write messages into the tx buffer.
+ */
+static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
+{
+ struct dw_i2c_dev *i2cd = i2c_get_adapdata(adap);
+ int msg_wrt_idx, msg_itr_lmt, buf_len, data_idx;
+ int cmd = 0, ret, status;
+ u8 *tx_buf;
+ u32 val;
+
+ ret = i2c_dw_acquire_lock(i2cd);
+ if (ret) {
+ dev_err(i2cd->dev, "Failed to get bus ownership\n");
+ return ret;
+ }
+
+ i2c_dw_configure_bus(i2cd);
+
+ /* Initiate messages read/write transaction */
+ for (msg_wrt_idx = 0; msg_wrt_idx < num_msgs; msg_wrt_idx++) {
+ tx_buf = msgs[msg_wrt_idx].buf;
+ buf_len = msgs[msg_wrt_idx].len;
+
+ if (!(msgs[msg_wrt_idx].flags & I2C_M_RD))
+ regmap_write(i2cd->map, DW_IC_TX_TL, buf_len - 1);
+
+ /* Initiate the i2c read/write transition of buffer length,
+ * and poll for bus busy status. For the last message transfer,
+ * update the command with stopbit enable.
+ */
+ for (msg_itr_lmt = buf_len; msg_itr_lmt > 0; msg_itr_lmt--) {
+ if (msg_wrt_idx == num_msgs - 1 && msg_itr_lmt == 1)
+ cmd |= BIT(9);
+
+ if (msgs[msg_wrt_idx].flags & I2C_M_RD) {
+ /* Due to hardware bug, need to write command two times. */
+ regmap_write(i2cd->map, DW_IC_DATA_CMD, 0x100);
+ regmap_write(i2cd->map, DW_IC_DATA_CMD, 0x100 | cmd);
+ if (cmd) {
+ regmap_write(i2cd->map, DW_IC_TX_TL, 2 * (buf_len - 1));
+ regmap_write(i2cd->map, DW_IC_RX_TL, 2 * (buf_len - 1));
+
+ /* Need to check the stop bit. However, it cannot be
+ * detected from the registers so we check it always
+ * when read/write the last byte.
+ */
+ status = i2c_dw_status(i2cd);
+ if (status) {
+ ret = -ETIMEDOUT;
+ goto lock_release;
+ }
+ for (data_idx = 0; data_idx < buf_len; data_idx++) {
+ regmap_read(i2cd->map, DW_IC_DATA_CMD, &val);
+ tx_buf[data_idx] = val;
+ }
+ status = i2c_dw_check_stopbit(i2cd);
+ if (status) {
+ ret = -ETIMEDOUT;
+ goto lock_release;
+ }
+ }
+ } else {
+ regmap_write(i2cd->map, DW_IC_DATA_CMD, *tx_buf++ | cmd);
+ usleep_range(AMD_TIMEOUT_MSEC_MIN, AMD_TIMEOUT_MSEC_MAX);
+ }
+ }
+ status = i2c_dw_check_stopbit(i2cd);
+ if (status) {
+ ret = -ETIMEDOUT;
+ goto lock_release;
+ }
+ }
+
+lock_release:
+ i2c_dw_release_lock(i2cd);
+
+ return ret;
+}
+
static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
{
const char *mode_str, *fp_str = "";
@@ -208,6 +360,9 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev)
if (dev->sda_hold_time)
regmap_write(dev->map, DW_IC_SDA_HOLD, dev->sda_hold_time);
+ /* Enable ucsi interrupt */
+ if (dev->flags & AMD_NON_INTR_MODE)
+ regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
i2c_dw_configure_fifo_master(dev);
i2c_dw_release_lock(dev);
@@ -462,6 +617,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
pm_runtime_get_sync(dev->dev);
+ if (dev->flags & AMD_NON_INTR_MODE)
+ return amd_i2c_dw_master_xfer(adap, msgs, num);
+
if (dev_WARN_ONCE(dev->dev, dev->suspended, "Transfer while suspended\n")) {
ret = -ESHUTDOWN;
goto done_nolock;
@@ -738,6 +896,19 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
return 0;
}
+int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
+{
+ struct i2c_adapter *amdap = &amdev->adapter;
+ int ret;
+
+ if (!(amdev->flags & AMD_NON_INTR_MODE))
+ return -ENODEV;
+
+ return i2c_add_numbered_adapter(amdap);
+
+ return ret;
+}
+
int i2c_dw_probe_master(struct dw_i2c_dev *dev)
{
struct i2c_adapter *adap = &dev->adapter;
@@ -774,6 +945,10 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
adap->dev.parent = dev->dev;
i2c_set_adapdata(adap, dev);
+ ret = amd_i2c_adap_quirk(dev);
+ if (ret != -ENODEV)
+ return ret;
+
if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
irq_flags = IRQF_NO_SUSPEND;
} else {
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 55c83a7a24f3..69ea6f206ab2 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -26,6 +26,7 @@
#include "i2c-designware-core.h"
#define DRIVER_NAME "i2c-designware-pci"
+#define AMD_CLK_RATE 100000
enum dw_pci_ctl_id_t {
medfield,
@@ -34,6 +35,7 @@ enum dw_pci_ctl_id_t {
cherrytrail,
haswell,
elkhartlake,
+ navi_amd,
};
struct dw_scl_sda_cfg {
@@ -78,11 +80,23 @@ static struct dw_scl_sda_cfg hsw_config = {
.sda_hold = 0x9,
};
+/* NAVI-AMD HCNT/LCNT/SDA hold time */
+static struct dw_scl_sda_cfg navi_amd_config = {
+ .ss_hcnt = 0x1ae,
+ .ss_lcnt = 0x23a,
+ .sda_hold = 0x9,
+};
+
static u32 mfld_get_clk_rate_khz(struct dw_i2c_dev *dev)
{
return 25000;
}
+static u32 navi_amd_get_clk_rate_khz(struct dw_i2c_dev *dev)
+{
+ return AMD_CLK_RATE;
+}
+
static int mfld_setup(struct pci_dev *pdev, struct dw_pci_controller *c)
{
struct dw_i2c_dev *dev = dev_get_drvdata(&pdev->dev);
@@ -104,6 +118,35 @@ static int mfld_setup(struct pci_dev *pdev, struct dw_pci_controller *c)
return -ENODEV;
}
+static int i2c_dw_populate_client(struct dw_i2c_dev *i2cd)
+{
+ struct i2c_board_info *i2c_dw_ccgx_ucsi;
+ struct i2c_client *ccgx_client;
+
+ i2c_dw_ccgx_ucsi = devm_kzalloc(i2cd->dev, sizeof(*i2c_dw_ccgx_ucsi), GFP_KERNEL);
+ if (!i2c_dw_ccgx_ucsi)
+ return -ENOMEM;
+
+ strscpy(i2c_dw_ccgx_ucsi->type, "ccgx-ucsi", sizeof(i2c_dw_ccgx_ucsi->type));
+ i2c_dw_ccgx_ucsi->addr = 0x08;
+ i2c_dw_ccgx_ucsi->irq = i2cd->irq;
+
+ ccgx_client = i2c_new_client_device(&i2cd->adapter, i2c_dw_ccgx_ucsi);
+ if (!ccgx_client)
+ return -ENODEV;
+
+ return 0;
+}
+
+static int navi_amd_setup(struct pci_dev *pdev, struct dw_pci_controller *c)
+{
+ struct dw_i2c_dev *dev = dev_get_drvdata(&pdev->dev);
+
+ dev->flags |= AMD_NON_INTR_MODE;
+ dev->timings.bus_freq_hz = I2C_MAX_STANDARD_MODE_FREQ;
+ return 0;
+}
+
static int mrfld_setup(struct pci_dev *pdev, struct dw_pci_controller *c)
{
/*
@@ -155,6 +198,12 @@ static struct dw_pci_controller dw_pci_controllers[] = {
.bus_num = -1,
.get_clk_rate_khz = ehl_get_clk_rate_khz,
},
+ [navi_amd] = {
+ .bus_num = -1,
+ .scl_sda_cfg = &navi_amd_config,
+ .setup = navi_amd_setup,
+ .get_clk_rate_khz = navi_amd_get_clk_rate_khz,
+ },
};
#ifdef CONFIG_PM
@@ -274,6 +323,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
return r;
}
+ if (dev->flags & AMD_NON_INTR_MODE)
+ i2c_dw_populate_client(dev);
+
pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_put_autosuspend(&pdev->dev);
@@ -337,6 +389,10 @@ static const struct pci_device_id i2_designware_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x4bbe), elkhartlake },
{ PCI_VDEVICE(INTEL, 0x4bbf), elkhartlake },
{ PCI_VDEVICE(INTEL, 0x4bc0), elkhartlake },
+ { PCI_VDEVICE(ATI, 0x7314), navi_amd },
+ { PCI_VDEVICE(ATI, 0x73a4), navi_amd },
+ { PCI_VDEVICE(ATI, 0x73e4), navi_amd },
+ { PCI_VDEVICE(ATI, 0x73c4), navi_amd },
{ 0,}
};
MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
--
2.25.1
Hi Sanket,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.12-rc2 next-20210309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sanket-Goswami/i2c-add-i2c-bus-driver-for-amd-navi-gpu/20210309-213535
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: powerpc64-randconfig-m031-20210309 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/f1d0b9c128d45f6d55f3d3f864db2e7ebf42adc5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sanket-Goswami/i2c-add-i2c-bus-driver-for-amd-navi-gpu/20210309-213535
git checkout f1d0b9c128d45f6d55f3d3f864db2e7ebf42adc5
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-designware-master.c:899:5: warning: no previous prototype for 'amd_i2c_adap_quirk' [-Wmissing-prototypes]
899 | int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
| ^~~~~~~~~~~~~~~~~~
vim +/amd_i2c_adap_quirk +899 drivers/i2c/busses/i2c-designware-master.c
898
> 899 int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
900 {
901 struct i2c_adapter *amdap = &amdev->adapter;
902 int ret;
903
904 if (!(amdev->flags & AMD_NON_INTR_MODE))
905 return -ENODEV;
906
907 return i2c_add_numbered_adapter(amdap);
908
909 return ret;
910 }
911
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
i2c: -> i2c: designware:
add i2c bus driver -> add a driver
amd -> AMD
gpu -> GPU
in the subject
> Latest AMDGPU NAVI cards have USB Type-C interface which can be accessed
> over I2C.
I didn't get this. You mean that USB traffic over I?C? This sounds insane
for a least. Maybe something else is there and description is not fully
correct?
> The Type-C controller has integrated designware i2c which is
DesignWare
> exposed as a PCI device to the AMD platform.
>
> Also there exists couple of notable IP problems that are dealt as a
> workaround:
> - I2C transactions work on a polling mode as IP does not generate
> interrupt.
>
> - I2C reads commands are sent twice to address the IP issues.
Please, read this article: https://chris.beams.io/posts/git-commit/
...
> +#define AMD_UCSI_INTR_EN 0xD
Why it's capitalized?
...
> #include "i2c-designware-core.h"
+ Blank line
> +#define AMD_TIMEOUT_MSEC_MIN 10000
> +#define AMD_TIMEOUT_MSEC_MAX 11000
Use unit suffix in the definitions.
...
> +static void i2c_dw_configure_bus(struct dw_i2c_dev *i2cd)
Why all this is customized? Why FIFO can't be autodetected?
...
> +/* Initiate and continue master read/write transaction with polling
> + * based transfer routine and write messages into the tx buffer.
> + */
Wrong multi-line comment style.
...
> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
Why do you need a custom function for that? Can it be generic and not AMD
specific?
...
> + /* Enable ucsi interrupt */
> + if (dev->flags & AMD_NON_INTR_MODE)
> + regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
This is looks like a hack. Why is it here?
...
> + if (dev->flags & AMD_NON_INTR_MODE)
> + return amd_i2c_dw_master_xfer(adap, msgs, num);
Ditto.
...
> +int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
> +{
> + struct i2c_adapter *amdap = &amdev->adapter;
> + int ret;
See below.
> + if (!(amdev->flags & AMD_NON_INTR_MODE))
> + return -ENODEV;
> + return i2c_add_numbered_adapter(amdap);
> +
> + return ret;
What the second one does?
> +}
...
> + ret = amd_i2c_adap_quirk(dev);
> + if (ret != -ENODEV)
This is ugly. Can we run quirk if and only if we have an AMD chip?
> + return ret;
...
> #define DRIVER_NAME "i2c-designware-pci"
> +#define AMD_CLK_RATE 100000
Add units.
...
> +/* NAVI-AMD HCNT/LCNT/SDA hold time */
> +static struct dw_scl_sda_cfg navi_amd_config = {
> + .ss_hcnt = 0x1ae,
> + .ss_lcnt = 0x23a,
> + .sda_hold = 0x9,
> +};
(1)
...
> +static int i2c_dw_populate_client(struct dw_i2c_dev *i2cd)
> +{
> + struct i2c_board_info *i2c_dw_ccgx_ucsi;
> + struct i2c_client *ccgx_client;
> +
> + i2c_dw_ccgx_ucsi = devm_kzalloc(i2cd->dev, sizeof(*i2c_dw_ccgx_ucsi), GFP_KERNEL);
> + if (!i2c_dw_ccgx_ucsi)
> + return -ENOMEM;
> +
> + strscpy(i2c_dw_ccgx_ucsi->type, "ccgx-ucsi", sizeof(i2c_dw_ccgx_ucsi->type));
> + i2c_dw_ccgx_ucsi->addr = 0x08;
> + i2c_dw_ccgx_ucsi->irq = i2cd->irq;
> +
> + ccgx_client = i2c_new_client_device(&i2cd->adapter, i2c_dw_ccgx_ucsi);
> + if (!ccgx_client)
> + return -ENODEV;
> +
> + return 0;
> +}
This is the same as in nVidia GPU I?C driver. Can you do a preparatory changes
to deduplicate this?
Also why (1) and this can't be instantiated from ACPI / DT?
--
With Best Regards,
Andy Shevchenko
Hi Sanket,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.12-rc2 next-20210309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sanket-Goswami/i2c-add-i2c-bus-driver-for-amd-navi-gpu/20210309-213535
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm-randconfig-r033-20210308 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 820f508b08d7c94b2dd7847e9710d2bc36d3dd45)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/f1d0b9c128d45f6d55f3d3f864db2e7ebf42adc5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sanket-Goswami/i2c-add-i2c-bus-driver-for-amd-navi-gpu/20210309-213535
git checkout f1d0b9c128d45f6d55f3d3f864db2e7ebf42adc5
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-designware-master.c:899:5: warning: no previous prototype for function 'amd_i2c_adap_quirk' [-Wmissing-prototypes]
int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
^
drivers/i2c/busses/i2c-designware-master.c:899:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
^
static
1 warning generated.
vim +/amd_i2c_adap_quirk +899 drivers/i2c/busses/i2c-designware-master.c
898
> 899 int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
900 {
901 struct i2c_adapter *amdap = &amdev->adapter;
902 int ret;
903
904 if (!(amdev->flags & AMD_NON_INTR_MODE))
905 return -ENODEV;
906
907 return i2c_add_numbered_adapter(amdap);
908
909 return ret;
910 }
911
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Sanket,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v5.12-rc2 next-20210309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Sanket-Goswami/i2c-add-i2c-bus-driver-for-amd-navi-gpu/20210309-213535
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: riscv-randconfig-r003-20210309 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 820f508b08d7c94b2dd7847e9710d2bc36d3dd45)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/f1d0b9c128d45f6d55f3d3f864db2e7ebf42adc5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sanket-Goswami/i2c-add-i2c-bus-driver-for-amd-navi-gpu/20210309-213535
git checkout f1d0b9c128d45f6d55f3d3f864db2e7ebf42adc5
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/i2c/busses/i2c-designware-master.c:899:5: warning: no previous prototype for function 'amd_i2c_adap_quirk' [-Wmissing-prototypes]
int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
^
drivers/i2c/busses/i2c-designware-master.c:899:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
^
static
1 warning generated.
vim +/amd_i2c_adap_quirk +899 drivers/i2c/busses/i2c-designware-master.c
898
> 899 int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
900 {
901 struct i2c_adapter *amdap = &amdev->adapter;
902 int ret;
903
904 if (!(amdev->flags & AMD_NON_INTR_MODE))
905 return -ENODEV;
906
907 return i2c_add_numbered_adapter(amdap);
908
909 return ret;
910 }
911
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Andy,
On 09-Mar-21 19:56, Andy Shevchenko wrote:
> [CAUTION: External Email]
>
> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
>
> i2c: -> i2c: designware:
> add i2c bus driver -> add a driver
> amd -> AMD
> gpu -> GPU
>
> in the subject
>
>> Latest AMDGPU NAVI cards have USB Type-C interface which can be accessed
>> over I2C.
>
> I didn't get this. You mean that USB traffic over I²C? This sounds insane
> for a least. Maybe something else is there and description is not fully
> correct?
>
>> The Type-C controller has integrated designware i2c which is
>
> DesignWare
>
>> exposed as a PCI device to the AMD platform.
>>
>> Also there exists couple of notable IP problems that are dealt as a
>> workaround:
>> - I2C transactions work on a polling mode as IP does not generate
>> interrupt.
>>
>> - I2C reads commands are sent twice to address the IP issues.
>
> Please, read this article: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchris.beams.io%2Fposts%2Fgit-commit%2F&data=04%7C01%7CSanket.Goswami%40amd.com%7C70d1c562d0c04fc2b76508d8e3074c97%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637508967775355658%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=yLiOkOtqCwGQAJgpYast8cMfCY4I9R74ElWm9FDNFNQ%3D&reserved=0
>
> ...
>
>> +#define AMD_UCSI_INTR_EN 0xD
>
> Why it's capitalized?
>
> ...
>
>> #include "i2c-designware-core.h"
>
> + Blank line
>
>> +#define AMD_TIMEOUT_MSEC_MIN 10000
>> +#define AMD_TIMEOUT_MSEC_MAX 11000
>
> Use unit suffix in the definitions.
>
> ...
>
>> +static void i2c_dw_configure_bus(struct dw_i2c_dev *i2cd)
>
> Why all this is customized? Why FIFO can't be autodetected?
Addressed in v2.
>
> ...
>
>> +/* Initiate and continue master read/write transaction with polling
>> + * based transfer routine and write messages into the tx buffer.
>> + */
>
> Wrong multi-line comment style.
>
> ...
>
>> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
>
> Why do you need a custom function for that? Can it be generic and not AMD
> specific?
This function takes care of AMD Specific bus configuration for the transfer and
also addresses the IP issue of i2c transactions hence it is kept as custom.
>
> ...
>
>> + /* Enable ucsi interrupt */
>> + if (dev->flags & AMD_NON_INTR_MODE)
>> + regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
>
> This is looks like a hack. Why is it here?
In order to enable the interrupt for UCSI i.e. AMD NAVI GPU card,
it is mandatory to set the right value in specific register
(offset:0x474) as per the hardware IP specification.
>
> ...
>
>> + if (dev->flags & AMD_NON_INTR_MODE)
>> + return amd_i2c_dw_master_xfer(adap, msgs, num);
>
> Ditto.
Initiate I2C message transfer when AMD NAVI GPU card is enabled,
As it is polling based transfer mechanism, which does not support
interrupt based functionalities of existing DesignWare driver.
> ...
>
>> +int amd_i2c_adap_quirk(struct dw_i2c_dev *amdev)
>> +{
>> + struct i2c_adapter *amdap = &amdev->adapter;
>
>> + int ret;
>
> See below.
>
>> + if (!(amdev->flags & AMD_NON_INTR_MODE))
>> + return -ENODEV;
>
>> + return i2c_add_numbered_adapter(amdap);
>> +
>> + return ret;
>
> What the second one does?
>
>> +}
>
> ...
>
>> + ret = amd_i2c_adap_quirk(dev);
>> + if (ret != -ENODEV)
>
> This is ugly. Can we run quirk if and only if we have an AMD chip?
>
>> + return ret;
>
> ...
>
>> #define DRIVER_NAME "i2c-designware-pci"
>> +#define AMD_CLK_RATE 100000
>
> Add units.
>
> ...
>
>> +/* NAVI-AMD HCNT/LCNT/SDA hold time */
>> +static struct dw_scl_sda_cfg navi_amd_config = {
>> + .ss_hcnt = 0x1ae,
>> + .ss_lcnt = 0x23a,
>> + .sda_hold = 0x9,
>> +};
>
> (1)
>
> ...
>
>> +static int i2c_dw_populate_client(struct dw_i2c_dev *i2cd)
>> +{
>> + struct i2c_board_info *i2c_dw_ccgx_ucsi;
>> + struct i2c_client *ccgx_client;
>> +
>> + i2c_dw_ccgx_ucsi = devm_kzalloc(i2cd->dev, sizeof(*i2c_dw_ccgx_ucsi), GFP_KERNEL);
>> + if (!i2c_dw_ccgx_ucsi)
>> + return -ENOMEM;
>> +
>> + strscpy(i2c_dw_ccgx_ucsi->type, "ccgx-ucsi", sizeof(i2c_dw_ccgx_ucsi->type));
>> + i2c_dw_ccgx_ucsi->addr = 0x08;
>> + i2c_dw_ccgx_ucsi->irq = i2cd->irq;
>> +
>> + ccgx_client = i2c_new_client_device(&i2cd->adapter, i2c_dw_ccgx_ucsi);
>> + if (!ccgx_client)
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>
> This is the same as in nVidia GPU I²C driver. Can you do a preparatory changes
> to deduplicate this?
Addressed in v2.
>
> Also why (1) and this can't be instantiated from ACPI / DT?
It is in line with the existing PCIe-based DesignWare driver,
A similar approach is used by the various vendors.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Sanket
On Mon, Mar 22, 2021 at 10:26:55PM +0530, Goswami, Sanket wrote:
> On 09-Mar-21 19:56, Andy Shevchenko wrote:
> > On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
...
> >> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
> >
> > Why do you need a custom function for that? Can it be generic and not AMD
> > specific?
> This function takes care of AMD Specific bus configuration for the transfer and
> also addresses the IP issue of i2c transactions hence it is kept as custom.
Can you a bit elaborate this? IT would be nice to have a comment in the code
explaining what is the difference with a generic approach.
...
> >> + /* Enable ucsi interrupt */
> >> + if (dev->flags & AMD_NON_INTR_MODE)
> >> + regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
> >
> > This is looks like a hack. Why is it here?
> In order to enable the interrupt for UCSI i.e. AMD NAVI GPU card,
> it is mandatory to set the right value in specific register
> (offset:0x474) as per the hardware IP specification.
Why it can not be done outside of this function?
...
> >> + if (dev->flags & AMD_NON_INTR_MODE)
> >> + return amd_i2c_dw_master_xfer(adap, msgs, num);
> >
> > Ditto.
> Initiate I2C message transfer when AMD NAVI GPU card is enabled,
> As it is polling based transfer mechanism, which does not support
> interrupt based functionalities of existing DesignWare driver.
Ditto.
And I think I already have told you that I prefer to see rather MODEL_ quirk.
...
> > Also why (1) and this can't be instantiated from ACPI / DT?
> It is in line with the existing PCIe-based DesignWare driver,
> A similar approach is used by the various vendors.
Here is no answer to the question. What prevents you to fix your ACPI tables or
DT?
We already got rid of FIFO hard coded values, timings are harder to achieve,
but we expect that new firmwares will provide values in the ACPI tables.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 25-Mar-21 22:35, Andy Shevchenko wrote:
> [CAUTION: External Email]
>
> On Mon, Mar 22, 2021 at 10:26:55PM +0530, Goswami, Sanket wrote:
>> On 09-Mar-21 19:56, Andy Shevchenko wrote:
>>> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
>
> ...
>
>>>> +static int amd_i2c_dw_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
>>>
>>> Why do you need a custom function for that? Can it be generic and not AMD
>>> specific?
>> This function takes care of AMD Specific bus configuration for the transfer and
>> also addresses the IP issue of i2c transactions hence it is kept as custom.
>
> Can you a bit elaborate this? IT would be nice to have a comment in the code
> explaining what is the difference with a generic approach.
This will be addressed in v3.
>
> ...
>
>>>> + /* Enable ucsi interrupt */
>>>> + if (dev->flags & AMD_NON_INTR_MODE)
>>>> + regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
>>>
>>> This is looks like a hack. Why is it here?
>> In order to enable the interrupt for UCSI i.e. AMD NAVI GPU card,
>> it is mandatory to set the right value in specific register
>> (offset:0x474) as per the hardware IP specification.
>
> Why it can not be done outside of this function?
This will be addressed in v3.
>
> ...
>
>>>> + if (dev->flags & AMD_NON_INTR_MODE)
>>>> + return amd_i2c_dw_master_xfer(adap, msgs, num);
>>>
>>> Ditto.
>> Initiate I2C message transfer when AMD NAVI GPU card is enabled,
>> As it is polling based transfer mechanism, which does not support
>> interrupt based functionalities of existing DesignWare driver.
>
> Ditto.
>
> And I think I already have told you that I prefer to see rather MODEL_ quirk.
I did not find MODEL_ quirk reference in the PCI device tree, It is actually
used in platform device tree which is completely different from our PCI
based configuration, can you please provide some reference of MODEL_ quirk
which will be part of the PCI device tree?
>
> ...
>
>>> Also why (1) and this can't be instantiated from ACPI / DT?
>> It is in line with the existing PCIe-based DesignWare driver,
>> A similar approach is used by the various vendors.
>
> Here is no answer to the question. What prevents you to fix your ACPI tables or
> DT?
>
> We already got rid of FIFO hard coded values, timings are harder to achieve,
> but we expect that new firmwares will provide values in the ACPI tables.
AMD NAVI GPU card is the PCI initiated driver, not ACPI initiated, and also
It does not contain a corresponding ACPI match table. Moreover, AMD NAVI GPU
based products are already in the commercial market hence going by this
approach will break the functionalities for the same.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Sanket
On Fri, Mar 26, 2021 at 03:53:34PM +0530, Goswami, Sanket wrote:
> On 25-Mar-21 22:35, Andy Shevchenko wrote:
> > On Mon, Mar 22, 2021 at 10:26:55PM +0530, Goswami, Sanket wrote:
> >> On 09-Mar-21 19:56, Andy Shevchenko wrote:
> >>> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
...
> > And I think I already have told you that I prefer to see rather MODEL_ quirk.
>
> I did not find MODEL_ quirk reference in the PCI device tree, It is actually
> used in platform device tree which is completely different from our PCI
> based configuration, can you please provide some reference of MODEL_ quirk
> which will be part of the PCI device tree?
I meant the name of new definition for quirk.
...
> >>> Also why (1) and this can't be instantiated from ACPI / DT?
> >> It is in line with the existing PCIe-based DesignWare driver,
> >> A similar approach is used by the various vendors.
> >
> > Here is no answer to the question. What prevents you to fix your ACPI tables or
> > DT?
> >
> > We already got rid of FIFO hard coded values, timings are harder to achieve,
> > but we expect that new firmwares will provide values in the ACPI tables.
>
> AMD NAVI GPU card is the PCI initiated driver, not ACPI initiated,
Which doesn't prevent to have an ACPI companion (via description in the
tables).
> and also
> It does not contain a corresponding ACPI match table.
Yes, that's what should be done in the firmware.
At least for the new version of firmware consider to add proper data into the
tables.
> Moreover, AMD NAVI GPU
> based products are already in the commercial market hence going by this
> approach will break the functionalities for the same.
This is quite bad and unfortunate. So, you have to elaborate this in the commit
message.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks for the review.
On 26-Mar-21 16:10, Andy Shevchenko wrote:
> [CAUTION: External Email]
>
> On Fri, Mar 26, 2021 at 03:53:34PM +0530, Goswami, Sanket wrote:
>> On 25-Mar-21 22:35, Andy Shevchenko wrote:
>>> On Mon, Mar 22, 2021 at 10:26:55PM +0530, Goswami, Sanket wrote:
>>>> On 09-Mar-21 19:56, Andy Shevchenko wrote:
>>>>> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
>
> ...
>
>>> And I think I already have told you that I prefer to see rather MODEL_ quirk.
>>
>> I did not find MODEL_ quirk reference in the PCI device tree, It is actually
>> used in platform device tree which is completely different from our PCI
>> based configuration, can you please provide some reference of MODEL_ quirk
>> which will be part of the PCI device tree?
>
> I meant the name of new definition for quirk.
Can you please elaborate this? i am not able to comprehend.
> ...
>
>>>>> Also why (1) and this can't be instantiated from ACPI / DT?
>>>> It is in line with the existing PCIe-based DesignWare driver,
>>>> A similar approach is used by the various vendors.
>>>
>>> Here is no answer to the question. What prevents you to fix your ACPI tables or
>>> DT?
>>>
>>> We already got rid of FIFO hard coded values, timings are harder to achieve,
>>> but we expect that new firmwares will provide values in the ACPI tables.
>>
>> AMD NAVI GPU card is the PCI initiated driver, not ACPI initiated,
>
> Which doesn't prevent to have an ACPI companion (via description in the
> tables).
>
>> and also
>> It does not contain a corresponding ACPI match table.
>
> Yes, that's what should be done in the firmware.
> At least for the new version of firmware consider to add proper data into the
> tables.
>
>> Moreover, AMD NAVI GPU
>> based products are already in the commercial market hence going by this
>> approach will break the functionalities for the same.
>
> This is quite bad and unfortunate. So, you have to elaborate this in the commit
> message.
Sure, will take care of this as part of commit message of v3.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Sanket
On Mon, Mar 29, 2021 at 11:25:58AM +0530, Goswami, Sanket wrote:
> On 26-Mar-21 16:10, Andy Shevchenko wrote:
> > On Fri, Mar 26, 2021 at 03:53:34PM +0530, Goswami, Sanket wrote:
> >> On 25-Mar-21 22:35, Andy Shevchenko wrote:
> >>> On Mon, Mar 22, 2021 at 10:26:55PM +0530, Goswami, Sanket wrote:
> >>>> On 09-Mar-21 19:56, Andy Shevchenko wrote:
> >>>>> On Tue, Mar 09, 2021 at 07:01:47PM +0530, Sanket Goswami wrote:
...
> >>> And I think I already have told you that I prefer to see rather MODEL_ quirk.
> >>
> >> I did not find MODEL_ quirk reference in the PCI device tree, It is actually
> >> used in platform device tree which is completely different from our PCI
> >> based configuration, can you please provide some reference of MODEL_ quirk
> >> which will be part of the PCI device tree?
> >
> > I meant the name of new definition for quirk.
>
> Can you please elaborate this? i am not able to comprehend.
Define in the i2c-designware-core.h something like
#define MODEL_AMD_BLA_BLA_BLA BIT(FOO)
--
With Best Regards,
Andy Shevchenko