The Latest AMD NAVI GPU card has an integrated Type-C controller and
Designware I2C with PCI Interface. The Type-C controller can be
accessed over I2C. The client driver is part of the USB Type-C UCSI
driver.
Also, there exists a couple of notable IP limitations that are dealt as
workarounds:
- I2C transaction work on a polling mode as IP does not generate
interrupt.
- I2C read command sent twice to address the IP issues.
- AMD NAVI GPU based products are already in the commercial market,
hence some of the I2C parameters are statically programmed as it can
not be part of ACPI table.
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]>
---
Changes in v3:
- Fixes runtime PM issue.
- Addressed review comments were given by Jarkko and Andy.
Changes in v2:
- Utilized existing functionality of i2c_dw_xfer_init to configure I2C
bus.
- Removed i2c_dw_populate_client and rewrrient navi_amd_register_client
to deduplicate from existing drivers.
- Addressed review comments were given by Andy.
drivers/i2c/busses/i2c-designware-common.c | 3 +
drivers/i2c/busses/i2c-designware-core.h | 3 +
drivers/i2c/busses/i2c-designware-master.c | 151 +++++++++++++++++++++
drivers/i2c/busses/i2c-designware-pcidrv.c | 57 ++++++++
4 files changed, 214 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 3c19aada4b30..d14ab2d2f23b 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 & MODEL_AMD_NAVI_GPU)
+ 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 5392b82f68a4..f65a15bac862 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -293,9 +293,12 @@ struct dw_i2c_dev {
#define ACCESS_INTR_MASK BIT(0)
#define ACCESS_NO_IRQ_SUSPEND BIT(1)
+#define MODEL_AMD_NAVI_GPU 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 dd27b9dbe931..60d9eae9df51 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -23,6 +23,10 @@
#include "i2c-designware-core.h"
+#define AMD_TIMEOUT_MIN_MSEC 10000
+#define AMD_TIMEOUT_MAX_MSEC 11000
+#define AMD_MASTERCFG_MASK GENMASK(15, 0)
+
static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
{
/* Configure Tx/Rx FIFO threshold levels */
@@ -259,6 +263,127 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
regmap_write(dev->map, DW_IC_INTR_MASK, DW_IC_INTR_MASTER_MASK);
}
+static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read_poll_timeout(dev->map, DW_IC_INTR_STAT, val,
+ !(val & DW_IC_INTR_STOP_DET),
+ 1100, 20000);
+ if (ret) {
+ dev_err(dev->dev, "i2c timeout error %d\n", ret);
+ return -ETIMEDOUT;
+ }
+
+ regmap_read(dev->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 *dev)
+{
+ int status;
+
+ status = i2c_dw_wait_bus_not_busy(dev);
+ if (status)
+ return -ETIMEDOUT;
+
+ return i2c_dw_check_stopbit(dev);
+}
+
+/*
+ * Initiate and continue master read/write transaction with polling
+ * based transfer routine afterward write messages into the tx buffer.
+ */
+static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
+{
+ struct dw_i2c_dev *dev = 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;
+ /*
+ * 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.
+ */
+ regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
+
+ ret = i2c_dw_acquire_lock(dev);
+ if (ret) {
+ dev_err(dev->dev, "Failed to get bus ownership\n");
+ return ret;
+ }
+
+ dev->msgs = msgs;
+ dev->msgs_num = num_msgs;
+ i2c_dw_xfer_init(dev);
+ i2c_dw_disable_int(dev);
+
+ /* 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(dev->map, DW_IC_TX_TL, buf_len - 1);
+ /*
+ * Initiate the i2c read/write transaction 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 the same command twice. */
+ regmap_write(dev->map, DW_IC_DATA_CMD, 0x100);
+ regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | cmd);
+ if (cmd) {
+ regmap_write(dev->map, DW_IC_TX_TL, 2 * (buf_len - 1));
+ regmap_write(dev->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(dev);
+ if (status) {
+ ret = -ETIMEDOUT;
+ goto lock_release;
+ }
+ for (data_idx = 0; data_idx < buf_len; data_idx++) {
+ regmap_read(dev->map, DW_IC_DATA_CMD, &val);
+ tx_buf[data_idx] = val;
+ }
+ status = i2c_dw_check_stopbit(dev);
+ if (status) {
+ ret = -ETIMEDOUT;
+ goto lock_release;
+ }
+ }
+ } else {
+ regmap_write(dev->map, DW_IC_DATA_CMD, *tx_buf++ | cmd);
+ usleep_range(AMD_TIMEOUT_MIN_MSEC, AMD_TIMEOUT_MAX_MSEC);
+ }
+ }
+ status = i2c_dw_check_stopbit(dev);
+ if (status) {
+ ret = -ETIMEDOUT;
+ goto lock_release;
+ }
+ }
+
+lock_release:
+ i2c_dw_release_lock(dev);
+
+ return ret;
+}
+
/*
* Initiate (and continue) low level master read/write transaction.
* This function is only called from i2c_dw_isr, and pumping i2c_msg
@@ -461,6 +586,15 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
pm_runtime_get_sync(dev->dev);
+ /*
+ * 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.
+ */
+ if (dev->flags & MODEL_AMD_NAVI_GPU) {
+ ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
+ goto done_nolock;
+ }
if (dev_WARN_ONCE(dev->dev, dev->suspended, "Transfer while suspended\n")) {
ret = -ESHUTDOWN;
@@ -738,6 +872,20 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
return 0;
}
+static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev)
+{
+ struct i2c_adapter *adap = &dev->adapter;
+ int ret;
+
+ pm_runtime_get_noresume(dev->dev);
+ ret = i2c_add_numbered_adapter(adap);
+ if (ret)
+ dev_err(dev->dev, "Failed to add adapter : %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+
+ return ret;
+}
+
int i2c_dw_probe_master(struct dw_i2c_dev *dev)
{
struct i2c_adapter *adap = &dev->adapter;
@@ -774,6 +922,9 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
adap->dev.parent = dev->dev;
i2c_set_adapdata(adap, dev);
+ if (dev->flags & MODEL_AMD_NAVI_GPU)
+ return amd_i2c_adap_quirk(dev);
+
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..e237e9ac1eef 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_HZ 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_HZ;
+}
+
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,31 @@ static int mfld_setup(struct pci_dev *pdev, struct dw_pci_controller *c)
return -ENODEV;
}
+static int navi_amd_register_client(struct dw_i2c_dev *dev)
+{
+ struct i2c_board_info info;
+
+ memset(&info, 0, sizeof(struct i2c_board_info));
+ strscpy(info.type, "ccgx-ucsi", I2C_NAME_SIZE);
+ info.addr = 0x08;
+ info.irq = dev->irq;
+
+ dev->slave = i2c_new_client_device(&dev->adapter, &info);
+ if (!dev->slave)
+ 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 |= MODEL_AMD_NAVI_GPU;
+ 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 +194,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 +319,14 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
return r;
}
+ if (dev->flags & MODEL_AMD_NAVI_GPU)
+ r = navi_amd_register_client(dev);
+
+ if (r) {
+ dev_err(dev->dev, "register client failed with %d\n", r);
+ return r;
+ }
+
pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_put_autosuspend(&pdev->dev);
@@ -337,6 +390,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
On Tue, Mar 30, 2021 at 12:17:15PM +0530, Sanket Goswami wrote:
Thanks for an update, my comments below.
> The Latest AMD NAVI GPU card has an integrated Type-C controller and
> Designware I2C with PCI Interface. The Type-C controller can be
> accessed over I2C.
Entire controller? You probably have to rephrase this to make sure
that everybody understands what you are talking about (presumable it's
"USB PD for Type-C" rather than "Type-C controller").
> The client driver is part of the USB Type-C UCSI
> driver.
> Also, there exists a couple of notable IP limitations that are dealt as
> workarounds:
> - I2C transaction work on a polling mode as IP does not generate
> interrupt.
> - I2C read command sent twice to address the IP issues.
> - AMD NAVI GPU based products are already in the commercial market,
> hence some of the I2C parameters are statically programmed as it can
> not be part of ACPI table.
...
> + if (dev->flags & MODEL_AMD_NAVI_GPU)
if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
> + map_cfg.max_register = AMD_UCSI_INTR_REG;
...
> #define ACCESS_INTR_MASK BIT(0)
> #define ACCESS_NO_IRQ_SUSPEND BIT(1)
>
> +#define MODEL_AMD_NAVI_GPU BIT(2)
This bit number doesn't fit MODEL_MASK.
> #define MODEL_MSCC_OCELOT BIT(8)
> #define MODEL_BAIKAL_BT1 BIT(9)
> #define MODEL_MASK GENMASK(11, 8)
+ blank line here.
> +#define AMD_UCSI_INTR_EN 0xd
> +#define AMD_UCSI_INTR_REG 0x474
Not sure I understood where is what. 0xd is a value or register offset?
Needs to be register offset first, followed by values. Also add a comment
to explain what is this register for.
...
> +#define AMD_TIMEOUT_MIN_MSEC 10000
> +#define AMD_TIMEOUT_MAX_MSEC 11000
MSEC -> MS
So, it's 10 and 11 seconds?! Needs a very good comment about such a long
delays.
...
> +static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
> +{
> + u32 val;
> + int ret;
> +
> + ret = regmap_read_poll_timeout(dev->map, DW_IC_INTR_STAT, val,
> + !(val & DW_IC_INTR_STOP_DET),
> + 1100, 20000);
> + if (ret) {
> + dev_err(dev->dev, "i2c timeout error %d\n", ret);
> + return -ETIMEDOUT;
Why shadowed error code?
> + }
> + regmap_read(dev->map, DW_IC_CLR_INTR, &val);
So, if above stops the polling, the condition is that bit is cleared.
How this makes it set again? Elaborate, please, why do you need the below
conditional and under which circumstances it may become true.
> + if (val & DW_IC_INTR_STOP_DET)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +static int i2c_dw_status(struct dw_i2c_dev *dev)
> +{
> + int status;
> +
> + status = i2c_dw_wait_bus_not_busy(dev);
> + if (status)
> + return -ETIMEDOUT;
Why you always shadow error codes?
> + return i2c_dw_check_stopbit(dev);
> +}
> +
> +/*
> + * Initiate and continue master read/write transaction with polling
> + * based transfer routine afterward write messages into the tx buffer.
tx -> Tx
> + */
> +static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
> +{
> + struct dw_i2c_dev *dev = 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;
+ blank line.
> + /*
> + * 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.
> + */
> + regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
> + ret = i2c_dw_acquire_lock(dev);
Do you need this? Does AMD have that ugly ACPI based mutex?
> + if (ret) {
> + dev_err(dev->dev, "Failed to get bus ownership\n");
> + return ret;
> + }
> +
> + dev->msgs = msgs;
> + dev->msgs_num = num_msgs;
> + i2c_dw_xfer_init(dev);
> + i2c_dw_disable_int(dev);
> +
> + /* 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(dev->map, DW_IC_TX_TL, buf_len - 1);
> + /*
> + * Initiate the i2c read/write transaction 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 the same command twice. */
> + regmap_write(dev->map, DW_IC_DATA_CMD, 0x100);
> + regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | cmd);
> + if (cmd) {
> + regmap_write(dev->map, DW_IC_TX_TL, 2 * (buf_len - 1));
> + regmap_write(dev->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(dev);
> + if (status) {
> + ret = -ETIMEDOUT;
???
> + goto lock_release;
> + }
> + for (data_idx = 0; data_idx < buf_len; data_idx++) {
> + regmap_read(dev->map, DW_IC_DATA_CMD, &val);
> + tx_buf[data_idx] = val;
> + }
> + status = i2c_dw_check_stopbit(dev);
> + if (status) {
> + ret = -ETIMEDOUT;
???
> + goto lock_release;
> + }
> + }
> + } else {
> + regmap_write(dev->map, DW_IC_DATA_CMD, *tx_buf++ | cmd);
> + usleep_range(AMD_TIMEOUT_MIN_MSEC, AMD_TIMEOUT_MAX_MSEC);
> + }
> + }
> + status = i2c_dw_check_stopbit(dev);
> + if (status) {
> + ret = -ETIMEDOUT;
???
> + goto lock_release;
> + }
> + }
> +lock_release:
> + i2c_dw_release_lock(dev);
So you need this? Why?
> + return ret;
> +}
...
> pm_runtime_get_sync(dev->dev);
+ Blank line.
> + /*
> + * 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.
> + */
> + if (dev->flags & MODEL_AMD_NAVI_GPU) {
> + ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
> + goto done_nolock;
> + }
...
> +static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev)
> +{
> + struct i2c_adapter *adap = &dev->adapter;
> + int ret;
> +
> + pm_runtime_get_noresume(dev->dev);
> + ret = i2c_add_numbered_adapter(adap);
> + if (ret)
> + dev_err(dev->dev, "Failed to add adapter : %d\n", ret);
Why is this under PM hooks?
> + pm_runtime_put_noidle(dev->dev);
> +
> + return ret;
> +}
> int i2c_dw_probe_master(struct dw_i2c_dev *dev)
> {
> struct i2c_adapter *adap = &dev->adapter;
> @@ -774,6 +922,9 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
> adap->dev.parent = dev->dev;
> i2c_set_adapdata(adap, dev);
>
> + if (dev->flags & MODEL_AMD_NAVI_GPU)
> + return amd_i2c_adap_quirk(dev);
> +
> if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
> irq_flags = IRQF_NO_SUSPEND;
> } else {
...
> +#define AMD_CLK_RATE_HZ 100000
Shouldn't others be also defined like this?
...
Add a TODO line here to point out that nVidia driver and this should be
deduplicated on how they instantiate a USB PD slave device.
> +static int navi_amd_register_client(struct dw_i2c_dev *dev)
> +{
> + struct i2c_board_info info;
> +
> + memset(&info, 0, sizeof(struct i2c_board_info));
> + strscpy(info.type, "ccgx-ucsi", I2C_NAME_SIZE);
> + info.addr = 0x08;
> + info.irq = dev->irq;
> +
> + dev->slave = i2c_new_client_device(&dev->adapter, &info);
> + if (!dev->slave)
> + return -ENODEV;
> +
> + return 0;
> +}
...
> + if (dev->flags & MODEL_AMD_NAVI_GPU)
> + r = navi_amd_register_client(dev);
> + if (r) {
> + dev_err(dev->dev, "register client failed with %d\n", r);
> + return r;
> + }
This seems to be on a wrong scope.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks for the review.
On 30-Mar-21 15:21, Andy Shevchenko wrote:
> [CAUTION: External Email]
>
> On Tue, Mar 30, 2021 at 12:17:15PM +0530, Sanket Goswami wrote:
>
> Thanks for an update, my comments below.
>
>> The Latest AMD NAVI GPU card has an integrated Type-C controller and
>> Designware I2C with PCI Interface. The Type-C controller can be
>> accessed over I2C.
>
> Entire controller? You probably have to rephrase this to make sure
> that everybody understands what you are talking about (presumable it's
> "USB PD for Type-C" rather than "Type-C controller").
>
>> The client driver is part of the USB Type-C UCSI
>> driver.
>
>> Also, there exists a couple of notable IP limitations that are dealt as
>> workarounds:
>> - I2C transaction work on a polling mode as IP does not generate
>> interrupt.
>> - I2C read command sent twice to address the IP issues.
>> - AMD NAVI GPU based products are already in the commercial market,
>> hence some of the I2C parameters are statically programmed as it can
>> not be part of ACPI table.
>
> ...
>
>> + if (dev->flags & MODEL_AMD_NAVI_GPU)
>
> if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU)
>
>> + map_cfg.max_register = AMD_UCSI_INTR_REG;
>
> ...
>
>> #define ACCESS_INTR_MASK BIT(0)
>> #define ACCESS_NO_IRQ_SUSPEND BIT(1)
>>
>> +#define MODEL_AMD_NAVI_GPU BIT(2)
>
> This bit number doesn't fit MODEL_MASK.
>
>> #define MODEL_MSCC_OCELOT BIT(8)
>> #define MODEL_BAIKAL_BT1 BIT(9)
>> #define MODEL_MASK GENMASK(11, 8)
>
> + blank line here.
>
>> +#define AMD_UCSI_INTR_EN 0xd
>> +#define AMD_UCSI_INTR_REG 0x474
>
> Not sure I understood where is what. 0xd is a value or register offset?
> Needs to be register offset first, followed by values. Also add a comment
> to explain what is this register for.
>
> ...
>
>> +#define AMD_TIMEOUT_MIN_MSEC 10000
>> +#define AMD_TIMEOUT_MAX_MSEC 11000
>
> MSEC -> MS
>
> So, it's 10 and 11 seconds?! Needs a very good comment about such a long
> delays.
>
> ...
>
>> +static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
>> +{
>> + u32 val;
>> + int ret;
>> +
>> + ret = regmap_read_poll_timeout(dev->map, DW_IC_INTR_STAT, val,
>> + !(val & DW_IC_INTR_STOP_DET),
>> + 1100, 20000);
>> + if (ret) {
>> + dev_err(dev->dev, "i2c timeout error %d\n", ret);
>
>> + return -ETIMEDOUT;
>
> Why shadowed error code?
>
>> + }
>
>> + regmap_read(dev->map, DW_IC_CLR_INTR, &val);
>
> So, if above stops the polling, the condition is that bit is cleared.
> How this makes it set again? Elaborate, please, why do you need the below
> conditional and under which circumstances it may become true.
>
>> + if (val & DW_IC_INTR_STOP_DET)
>> + return -ETIMEDOUT;
>> +
>> + return 0;
>> +}
>> +
>> +static int i2c_dw_status(struct dw_i2c_dev *dev)
>> +{
>> + int status;
>> +
>> + status = i2c_dw_wait_bus_not_busy(dev);
>> + if (status)
>
>> + return -ETIMEDOUT;
>
> Why you always shadow error codes?
>
>> + return i2c_dw_check_stopbit(dev);
>> +}
>> +
>> +/*
>> + * Initiate and continue master read/write transaction with polling
>> + * based transfer routine afterward write messages into the tx buffer.
>
> tx -> Tx
>
>> + */
>> +static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, int num_msgs)
>> +{
>> + struct dw_i2c_dev *dev = 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;
>
> + blank line.
>
>> + /*
>> + * 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.
>> + */
>> + regmap_write(dev->map, AMD_UCSI_INTR_REG, AMD_UCSI_INTR_EN);
>
>> + ret = i2c_dw_acquire_lock(dev);
>
> Do you need this? Does AMD have that ugly ACPI based mutex?
>
>> + if (ret) {
>> + dev_err(dev->dev, "Failed to get bus ownership\n");
>> + return ret;
>> + }
>> +
>> + dev->msgs = msgs;
>> + dev->msgs_num = num_msgs;
>> + i2c_dw_xfer_init(dev);
>> + i2c_dw_disable_int(dev);
>> +
>> + /* 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(dev->map, DW_IC_TX_TL, buf_len - 1);
>> + /*
>> + * Initiate the i2c read/write transaction 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 the same command twice. */
>> + regmap_write(dev->map, DW_IC_DATA_CMD, 0x100);
>> + regmap_write(dev->map, DW_IC_DATA_CMD, 0x100 | cmd);
>> + if (cmd) {
>> + regmap_write(dev->map, DW_IC_TX_TL, 2 * (buf_len - 1));
>> + regmap_write(dev->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(dev);
>> + if (status) {
>
>> + ret = -ETIMEDOUT;
>
> ???
>
>> + goto lock_release;
>> + }
>> + for (data_idx = 0; data_idx < buf_len; data_idx++) {
>> + regmap_read(dev->map, DW_IC_DATA_CMD, &val);
>> + tx_buf[data_idx] = val;
>> + }
>> + status = i2c_dw_check_stopbit(dev);
>> + if (status) {
>
>> + ret = -ETIMEDOUT;
>
> ???
>
>> + goto lock_release;
>> + }
>> + }
>> + } else {
>> + regmap_write(dev->map, DW_IC_DATA_CMD, *tx_buf++ | cmd);
>> + usleep_range(AMD_TIMEOUT_MIN_MSEC, AMD_TIMEOUT_MAX_MSEC);
>> + }
>> + }
>> + status = i2c_dw_check_stopbit(dev);
>> + if (status) {
>
>> + ret = -ETIMEDOUT;
>
> ???
>
>> + goto lock_release;
>> + }
>> + }
>
>> +lock_release:
>> + i2c_dw_release_lock(dev);
>
> So you need this? Why?
>
>> + return ret;
>> +}
>
> ...
>
>> pm_runtime_get_sync(dev->dev);
>
> + Blank line.
>
>> + /*
>> + * 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.
>> + */
>> + if (dev->flags & MODEL_AMD_NAVI_GPU) {
>> + ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
>> + goto done_nolock;
>> + }
>
> ...
>
>> +static int amd_i2c_adap_quirk(struct dw_i2c_dev *dev)
>> +{
>> + struct i2c_adapter *adap = &dev->adapter;
>> + int ret;
>> +
>> + pm_runtime_get_noresume(dev->dev);
>> + ret = i2c_add_numbered_adapter(adap);
>
>> + if (ret)
>> + dev_err(dev->dev, "Failed to add adapter : %d\n", ret);
>
> Why is this under PM hooks?
As driver supports PM runtime, PM runtime related hooks/calls are maintained
as per the existing driver implementation.
>
>> + pm_runtime_put_noidle(dev->dev);
>> +
>> + return ret;
>> +}
>
>> int i2c_dw_probe_master(struct dw_i2c_dev *dev)
>> {
>> struct i2c_adapter *adap = &dev->adapter;
>> @@ -774,6 +922,9 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
>> adap->dev.parent = dev->dev;
>> i2c_set_adapdata(adap, dev);
>>
>> + if (dev->flags & MODEL_AMD_NAVI_GPU)
>> + return amd_i2c_adap_quirk(dev);
>> +
>> if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
>> irq_flags = IRQF_NO_SUSPEND;
>> } else {
>
> ...
>
>> +#define AMD_CLK_RATE_HZ 100000
>
> Shouldn't others be also defined like this?
>
> ...
>
> Add a TODO line here to point out that nVidia driver and this should be
> deduplicated on how they instantiate a USB PD slave device.
>
>> +static int navi_amd_register_client(struct dw_i2c_dev *dev)
>> +{
>> + struct i2c_board_info info;
>> +
>> + memset(&info, 0, sizeof(struct i2c_board_info));
>> + strscpy(info.type, "ccgx-ucsi", I2C_NAME_SIZE);
>> + info.addr = 0x08;
>> + info.irq = dev->irq;
>> +
>> + dev->slave = i2c_new_client_device(&dev->adapter, &info);
>> + if (!dev->slave)
>> + return -ENODEV;
>> +
>> + return 0;
>> +}
>
> ...
>
>> + if (dev->flags & MODEL_AMD_NAVI_GPU)
>> + r = navi_amd_register_client(dev);
>
>> + if (r) {
>> + dev_err(dev->dev, "register client failed with %d\n", r);
>> + return r;
>> + }
>
> This seems to be on a wrong scope.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Remaining review comments will be addressed as part of v4.
Thanks,
Sanket