[PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[Huangzheng Lai]

We found that when the interrupt bit of the I2C controller is cleared,
the ack/nack bit is also cleared at the same time. After clearing the
interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
when nack cannot be recognized. To solve this problem, we used a global
variable to record ack/nack information before clearing the interrupt
bit instead of a local variable.

Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index aa602958d4fd..dec627ef408c 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -85,6 +85,7 @@ struct sprd_i2c {
 	struct clk *clk;
 	u32 src_clk;
 	u32 bus_freq;
+	bool ack_flag;
 	struct completion complete;
 	struct reset_control *rst;
 	u8 *buf;
@@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
 {
 	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
 
+	i2c_dev->ack_flag = 0;
 	writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
 }
 
@@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 {
 	struct sprd_i2c *i2c_dev = dev_id;
 	struct i2c_msg *msg = i2c_dev->msg;
-	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
 	u32 i2c_tran;
 
 	if (msg->flags & I2C_M_RD)
@@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 	 * For reading data, ack is always true, if i2c_tran is not 0 which
 	 * means we still need to contine to read data from slave.
 	 */
-	if (i2c_tran && ack) {
+	if (i2c_tran && i2c_dev->ack_flag) {
 		sprd_i2c_data_transfer(i2c_dev);
 		return IRQ_HANDLED;
 	}
@@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 	 * If we did not get one ACK from slave when writing data, we should
 	 * return -EIO to notify users.
 	 */
-	if (!ack)
+	if (!i2c_dev->ack_flag)
 		i2c_dev->err = -EIO;
 	else if (msg->flags & I2C_M_RD && i2c_dev->count)
 		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
@@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
 {
 	struct sprd_i2c *i2c_dev = dev_id;
 	struct i2c_msg *msg = i2c_dev->msg;
-	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
 	u32 i2c_tran;
 
 	if (msg->flags & I2C_M_RD)
@@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
 	 * means we can read all data in one time, then we can finish this
 	 * transmission too.
 	 */
-	if (!i2c_tran || !ack) {
+	i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
+	if (!i2c_tran || !i2c_dev->ack_flag) {
 		sprd_i2c_clear_start(i2c_dev);
 		sprd_i2c_clear_irq(i2c_dev);
 	}
-- 
2.17.1

[PATCH V2 0/7] i2c: sprd: Modification of UNISOC Platform I2C Driver

[Huangzheng Lai]

Recently, some bugs have been discovered during use, patch3 and 
patch5-6 are bug fixes. Also, this patchset add new features:
patch1 allows I2C to use more frequencies for communication,
patch2 allows I2C to use 'reset framework' for reset, and patch4 allows
I2C controller to dynamically switch frequencies during use.

change in V2
-Using 'I2C' instead of 'IIC' in the patch set.
-Using imperative form in patch subject.
-Use 'switch case' instead of 'else if' in PATCH 1/7.
-Modify if (i2c_dev->rst != NULL) to if (i2c_dev->rst) in PATCH 2/7.
-Modify some dev_err() to dev_warn() or dev_dbg().
-Clear i2c_dev->ack_flag in sprd_i2c_clear_ack() in PATCH 3/7.
-Modify the indentation format of the code in PATCH 4/7.
-Move sprd_i2c_enable() above its caller in PATCH 5/7.
-Remove 'Set I2C_RX_ACK when clear irq' commit.
-Add Fixes tags. 

Huangzheng Lai (7):
  i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies
  i2c: sprd: Add I2C driver to use 'reset framework' function
  i2c: sprd: Use global variables to record I2C ack/nack status instead
    of local variables
  i2c: sprd: Add I2C controller driver to support dynamic switching of
    400K/1M/3.4M frequency
  i2c: sprd: Configure the enable bit of the I2C controller before each
    transmission initiation
  i2c: sprd: Increase the waiting time for I2C transmission to avoid
    system crash issues
  i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits

 drivers/i2c/busses/i2c-sprd.c | 166 ++++++++++++++++++++++------------
 1 file changed, 106 insertions(+), 60 deletions(-)

-- 
2.17.1

[PATCH V2 1/7] i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies

[Huangzheng Lai]

Add support for 1Mhz and 3.4Mhz frequency configuration.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
Acked-by: Andi Shyti <andi.shyti@kernel.org>
---
 drivers/i2c/busses/i2c-sprd.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index ffc54fbf814d..b44916c6741d 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -343,10 +343,23 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
 	writel(div1, i2c_dev->base + ADDR_DVD1);
 
 	/* Start hold timing = hold time(us) * source clock */
-	if (freq == I2C_MAX_FAST_MODE_FREQ)
-		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
-	else if (freq == I2C_MAX_STANDARD_MODE_FREQ)
+	switch (freq) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
 		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	case I2C_MAX_FAST_MODE_FREQ:
+		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	default:
+		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
+		break;
+	}
 }
 
 static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
@@ -519,9 +532,11 @@ static int sprd_i2c_probe(struct platform_device *pdev)
 	if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
 		i2c_dev->bus_freq = prop;
 
-	/* We only support 100k and 400k now, otherwise will return error. */
+	/* We only support 100k\400k\1m\3.4m now, otherwise will return error. */
 	if (i2c_dev->bus_freq != I2C_MAX_STANDARD_MODE_FREQ &&
-	    i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ)
+			i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ &&
+			i2c_dev->bus_freq != I2C_MAX_FAST_MODE_PLUS_FREQ &&
+			i2c_dev->bus_freq != I2C_MAX_HIGH_SPEED_MODE_FREQ)
 		return -EINVAL;
 
 	ret = sprd_i2c_clk_init(i2c_dev);
-- 
2.17.1

[PATCH V2 2/7] i2c: sprd: Add I2C driver to use 'reset framework' function

[Huangzheng Lai]

Add the 'reset framework' function for I2C drivers, which
resets the I2C controller when a timeout exception occurs.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index b44916c6741d..aa602958d4fd 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -17,6 +17,7 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 
 #define I2C_CTL			0x00
 #define I2C_ADDR_CFG		0x04
@@ -85,6 +86,7 @@ struct sprd_i2c {
 	u32 src_clk;
 	u32 bus_freq;
 	struct completion complete;
+	struct reset_control *rst;
 	u8 *buf;
 	u32 count;
 	int irq;
@@ -278,9 +280,17 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 
 	time_left = wait_for_completion_timeout(&i2c_dev->complete,
 				msecs_to_jiffies(I2C_XFER_TIMEOUT));
-	if (!time_left)
+	if (!time_left) {
+		dev_err(i2c_dev->dev, "transfer timeout, I2C_STATUS = 0x%x\n",
+			readl(i2c_dev->base + I2C_STATUS));
+		if (i2c_dev->rst) {
+			int ret = reset_control_reset(i2c_dev->rst);
+
+			if (ret < 0)
+				dev_warn(i2c_dev->dev, "i2c soft reset failed, ret = %d\n", ret);
+		}
 		return -ETIMEDOUT;
-
+	}
 	return i2c_dev->err;
 }
 
@@ -544,6 +554,11 @@ static int sprd_i2c_probe(struct platform_device *pdev)
 		return ret;
 
 	platform_set_drvdata(pdev, i2c_dev);
+	i2c_dev->rst = devm_reset_control_get(i2c_dev->dev, "i2c_rst");
+	if (IS_ERR(i2c_dev->rst)) {
+		dev_dbg(i2c_dev->dev, "reset control not configured!\n");
+		i2c_dev->rst = NULL;
+	}
 
 	ret = clk_prepare_enable(i2c_dev->clk);
 	if (ret)
-- 
2.17.1

[PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[Huangzheng Lai]

We found that when the interrupt bit of the I2C controller is cleared,
the ack/nack bit is also cleared at the same time. After clearing the
interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
when nack cannot be recognized. To solve this problem, we used a global
variable to record ack/nack information before clearing the interrupt
bit instead of a local variable.

Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index aa602958d4fd..dec627ef408c 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -85,6 +85,7 @@ struct sprd_i2c {
 	struct clk *clk;
 	u32 src_clk;
 	u32 bus_freq;
+	bool ack_flag;
 	struct completion complete;
 	struct reset_control *rst;
 	u8 *buf;
@@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
 {
 	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
 
+	i2c_dev->ack_flag = 0;
 	writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
 }
 
@@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 {
 	struct sprd_i2c *i2c_dev = dev_id;
 	struct i2c_msg *msg = i2c_dev->msg;
-	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
 	u32 i2c_tran;
 
 	if (msg->flags & I2C_M_RD)
@@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 	 * For reading data, ack is always true, if i2c_tran is not 0 which
 	 * means we still need to contine to read data from slave.
 	 */
-	if (i2c_tran && ack) {
+	if (i2c_tran && i2c_dev->ack_flag) {
 		sprd_i2c_data_transfer(i2c_dev);
 		return IRQ_HANDLED;
 	}
@@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 	 * If we did not get one ACK from slave when writing data, we should
 	 * return -EIO to notify users.
 	 */
-	if (!ack)
+	if (!i2c_dev->ack_flag)
 		i2c_dev->err = -EIO;
 	else if (msg->flags & I2C_M_RD && i2c_dev->count)
 		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
@@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
 {
 	struct sprd_i2c *i2c_dev = dev_id;
 	struct i2c_msg *msg = i2c_dev->msg;
-	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
 	u32 i2c_tran;
 
 	if (msg->flags & I2C_M_RD)
@@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
 	 * means we can read all data in one time, then we can finish this
 	 * transmission too.
 	 */
-	if (!i2c_tran || !ack) {
+	i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
+	if (!i2c_tran || !i2c_dev->ack_flag) {
 		sprd_i2c_clear_start(i2c_dev);
 		sprd_i2c_clear_irq(i2c_dev);
 	}
-- 
2.17.1

[PATCH V2 4/7] i2c: sprd: Add I2C controller driver to support dynamic switching of 400K/1M/3.4M frequency

[Huangzheng Lai]

When I2C-slaves supporting different frequencies use the same I2C
controller, the I2C controller usually only operates at lower frequencies.
In order to improve the performance of I2C-slaves transmission supporting
faster frequencies, we dynamically configure the I2C operating frequency
based on the value of the input parameter msg ->flag.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 101 +++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 44 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index dec627ef408c..f1f7fad42ecd 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -75,7 +75,14 @@
 #define SPRD_I2C_PM_TIMEOUT	1000
 /* timeout (ms) for transfer message */
 #define I2C_XFER_TIMEOUT	1000
-
+/* dynamic modify clk_freq flag  */
+#define I2C_3M4_FLAG		0x0100
+#define I2C_1M_FLAG		0x0080
+#define I2C_400K_FLAG		0x0040
+
+#define I2C_FREQ_400K		400000
+#define I2C_FREQ_1M		1000000
+#define I2C_FREQ_3_4M		3400000
 /* SPRD i2c data structure */
 struct sprd_i2c {
 	struct i2c_adapter adap;
@@ -94,6 +101,49 @@ struct sprd_i2c {
 	int err;
 };
 
+static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
+{
+	u32 apb_clk = i2c_dev->src_clk;
+	/*
+	 * From I2C databook, the prescale calculation formula:
+	 * prescale = freq_i2c / (4 * freq_scl) - 1;
+	 */
+	u32 i2c_dvd = apb_clk / (4 * freq) - 1;
+	/*
+	 * From I2C databook, the high period of SCL clock is recommended as
+	 * 40% (2/5), and the low period of SCL clock is recommended as 60%
+	 * (3/5), then the formula should be:
+	 * high = (prescale * 2 * 2) / 5
+	 * low = (prescale * 2 * 3) / 5
+	 */
+	u32 high = ((i2c_dvd << 1) * 2) / 5;
+	u32 low = ((i2c_dvd << 1) * 3) / 5;
+	u32 div0 = I2C_ADDR_DVD0_CALC(high, low);
+	u32 div1 = I2C_ADDR_DVD1_CALC(high, low);
+
+	writel(div0, i2c_dev->base + ADDR_DVD0);
+	writel(div1, i2c_dev->base + ADDR_DVD1);
+
+	/* Start hold timing = hold time(us) * source clock */
+	switch (freq) {
+	case I2C_MAX_STANDARD_MODE_FREQ:
+		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	case I2C_MAX_FAST_MODE_FREQ:
+		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	case I2C_MAX_FAST_MODE_PLUS_FREQ:
+		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
+		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+		break;
+	default:
+		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
+		break;
+	}
+}
+
 static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
 {
 	writel(count, i2c_dev->base + I2C_COUNT);
@@ -269,6 +319,12 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 		sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
 	}
 
+	if (msg->flags & I2C_400K_FLAG)
+		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_400K);
+	else if (msg->flags & I2C_1M_FLAG)
+		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_1M);
+	else if (msg->flags & I2C_3M4_FLAG)
+		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_3_4M);
 	/*
 	 * We should enable rx fifo full interrupt to get data when receiving
 	 * full data.
@@ -331,49 +387,6 @@ static const struct i2c_algorithm sprd_i2c_algo = {
 	.functionality = sprd_i2c_func,
 };
 
-static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
-{
-	u32 apb_clk = i2c_dev->src_clk;
-	/*
-	 * From I2C databook, the prescale calculation formula:
-	 * prescale = freq_i2c / (4 * freq_scl) - 1;
-	 */
-	u32 i2c_dvd = apb_clk / (4 * freq) - 1;
-	/*
-	 * From I2C databook, the high period of SCL clock is recommended as
-	 * 40% (2/5), and the low period of SCL clock is recommended as 60%
-	 * (3/5), then the formula should be:
-	 * high = (prescale * 2 * 2) / 5
-	 * low = (prescale * 2 * 3) / 5
-	 */
-	u32 high = ((i2c_dvd << 1) * 2) / 5;
-	u32 low = ((i2c_dvd << 1) * 3) / 5;
-	u32 div0 = I2C_ADDR_DVD0_CALC(high, low);
-	u32 div1 = I2C_ADDR_DVD1_CALC(high, low);
-
-	writel(div0, i2c_dev->base + ADDR_DVD0);
-	writel(div1, i2c_dev->base + ADDR_DVD1);
-
-	/* Start hold timing = hold time(us) * source clock */
-	switch (freq) {
-	case I2C_MAX_STANDARD_MODE_FREQ:
-		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
-		break;
-	case I2C_MAX_FAST_MODE_FREQ:
-		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
-		break;
-	case I2C_MAX_FAST_MODE_PLUS_FREQ:
-		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
-		break;
-	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
-		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
-		break;
-	default:
-		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
-		break;
-	}
-}
-
 static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
 {
 	u32 tmp = I2C_DVD_OPT;
-- 
2.17.1

[PATCH V2 5/7] i2c: sprd: Configure the enable bit of the I2C controller before each transmission initiation

[Huangzheng Lai]

When a timeout exception occurs in the I2C driver, the I2C controller
will be reset, and after resetting, control bits such as I2C_EN and
I2C_INT_EN will be reset to 0, and the I2C master cannot initiate
Transmission unless sprd_i2c_enable() is executed. To address this issue,
this patch places sprd_i2c_enable() before each transmission initiation
to ensure that the necessary control bits of the I2C controller are
configured.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 38 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index f1f7fad42ecd..b65729ba7d5a 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -352,6 +352,23 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 	return i2c_dev->err;
 }
 
+static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
+{
+	u32 tmp = I2C_DVD_OPT;
+
+	writel(tmp, i2c_dev->base + I2C_CTL);
+
+	sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD);
+	sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD);
+
+	sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq);
+	sprd_i2c_reset_fifo(i2c_dev);
+	sprd_i2c_clear_irq(i2c_dev);
+
+	tmp = readl(i2c_dev->base + I2C_CTL);
+	writel(tmp | I2C_EN | I2C_INT_EN, i2c_dev->base + I2C_CTL);
+}
+
 static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
 				struct i2c_msg *msgs, int num)
 {
@@ -362,6 +379,8 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
 	if (ret < 0)
 		return ret;
 
+	sprd_i2c_enable(i2c_dev);
+
 	for (im = 0; im < num - 1; im++) {
 		ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], 0);
 		if (ret)
@@ -387,23 +406,6 @@ static const struct i2c_algorithm sprd_i2c_algo = {
 	.functionality = sprd_i2c_func,
 };
 
-static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
-{
-	u32 tmp = I2C_DVD_OPT;
-
-	writel(tmp, i2c_dev->base + I2C_CTL);
-
-	sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD);
-	sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD);
-
-	sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq);
-	sprd_i2c_reset_fifo(i2c_dev);
-	sprd_i2c_clear_irq(i2c_dev);
-
-	tmp = readl(i2c_dev->base + I2C_CTL);
-	writel(tmp | I2C_EN | I2C_INT_EN, i2c_dev->base + I2C_CTL);
-}
-
 static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
 {
 	struct sprd_i2c *i2c_dev = dev_id;
@@ -669,8 +671,6 @@ static int __maybe_unused sprd_i2c_runtime_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	sprd_i2c_enable(i2c_dev);
-
 	return 0;
 }
 
-- 
2.17.1

[PATCH V2 6/7] i2c: sprd: Increase the waiting time for I2C transmission to avoid system crash issues

[Huangzheng Lai]

Due to the relatively low priority of the isr_thread, when the CPU
load is high, the execution of sprd_i2c_isr_thread will be delayed.
After the waiting time is exceeded, the I2C driver will perform
operations such as disabling the I2C controller. Later, when
sprd_i2c_isr_thread is called by the CPU, there will be kernel panic
caused by illegal access to the IIC register. After pressure testing,
we found that increasing the IIC waiting time to 10 seconds can
avoid this problem.

Fixes: 0b884fe71f9e ("i2c: sprd: use a specific timeout to avoid system hang up issue")
Cc: <stable@vger.kernel.org> # v5.11+
Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index b65729ba7d5a..dbdac89ad482 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -74,7 +74,7 @@
 /* timeout (ms) for pm runtime autosuspend */
 #define SPRD_I2C_PM_TIMEOUT	1000
 /* timeout (ms) for transfer message */
-#define I2C_XFER_TIMEOUT	1000
+#define I2C_XFER_TIMEOUT	10000
 /* dynamic modify clk_freq flag  */
 #define I2C_3M4_FLAG		0x0100
 #define I2C_1M_FLAG		0x0080
-- 
2.17.1

[PATCH V2 7/7] i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits

[Huangzheng Lai]

The new I2C IP version on the UNISOC platform has added I2C_NACK_EN and
I2C_TRANS_EN control bits. To ensure that the I2C controller can initiate
transmission smoothly, these two bits need to be configured.

Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
---
 drivers/i2c/busses/i2c-sprd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index dbdac89ad482..431c0db84d22 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -33,6 +33,8 @@
 #define ADDR_RST		0x2c
 
 /* I2C_CTL */
+#define I2C_NACK_EN		BIT(22)
+#define I2C_TRANS_EN		BIT(21)
 #define STP_EN			BIT(20)
 #define FIFO_AF_LVL_MASK	GENMASK(19, 16)
 #define FIFO_AF_LVL		16
@@ -366,7 +368,7 @@ static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
 	sprd_i2c_clear_irq(i2c_dev);
 
 	tmp = readl(i2c_dev->base + I2C_CTL);
-	writel(tmp | I2C_EN | I2C_INT_EN, i2c_dev->base + I2C_CTL);
+	writel(tmp | I2C_EN | I2C_INT_EN | I2C_NACK_EN | I2C_TRANS_EN, i2c_dev->base + I2C_CTL);
 }
 
 static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
-- 
2.17.1

Re: [PATCH V2 1/7] i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> Add support for 1Mhz and 3.4Mhz frequency configuration.
> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> Acked-by: Andi Shyti <andi.shyti@kernel.org>
> ---
>   drivers/i2c/busses/i2c-sprd.c | 25 ++++++++++++++++++++-----
>   1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index ffc54fbf814d..b44916c6741d 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -343,10 +343,23 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
>   	writel(div1, i2c_dev->base + ADDR_DVD1);
>   
>   	/* Start hold timing = hold time(us) * source clock */
> -	if (freq == I2C_MAX_FAST_MODE_FREQ)
> -		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> -	else if (freq == I2C_MAX_STANDARD_MODE_FREQ)
> +	switch (freq) {
> +	case I2C_MAX_STANDARD_MODE_FREQ:
>   		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_FAST_MODE_FREQ:
> +		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	default:
> +		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
> +		break;
> +	}
>   }
>   
>   static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
> @@ -519,9 +532,11 @@ static int sprd_i2c_probe(struct platform_device *pdev)
>   	if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
>   		i2c_dev->bus_freq = prop;
>   
> -	/* We only support 100k and 400k now, otherwise will return error. */
> +	/* We only support 100k\400k\1m\3.4m now, otherwise will return error. */
>   	if (i2c_dev->bus_freq != I2C_MAX_STANDARD_MODE_FREQ &&
> -	    i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ)
> +			i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ &&
> +			i2c_dev->bus_freq != I2C_MAX_FAST_MODE_PLUS_FREQ &&
> +			i2c_dev->bus_freq != I2C_MAX_HIGH_SPEED_MODE_FREQ)

Can you keep the same alignment format as the previous code? Otherwise 
look good to me.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Re: [PATCH V2 2/7] i2c: sprd: Add I2C driver to use 'reset framework' function

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> Add the 'reset framework' function for I2C drivers, which
> resets the I2C controller when a timeout exception occurs.

Can you explain the situations that can lead to a timeout explicitly?

> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>   drivers/i2c/busses/i2c-sprd.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index b44916c6741d..aa602958d4fd 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -17,6 +17,7 @@
>   #include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>   
>   #define I2C_CTL			0x00
>   #define I2C_ADDR_CFG		0x04
> @@ -85,6 +86,7 @@ struct sprd_i2c {
>   	u32 src_clk;
>   	u32 bus_freq;
>   	struct completion complete;
> +	struct reset_control *rst;
>   	u8 *buf;
>   	u32 count;
>   	int irq;
> @@ -278,9 +280,17 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
>   
>   	time_left = wait_for_completion_timeout(&i2c_dev->complete,
>   				msecs_to_jiffies(I2C_XFER_TIMEOUT));
> -	if (!time_left)
> +	if (!time_left) {
> +		dev_err(i2c_dev->dev, "transfer timeout, I2C_STATUS = 0x%x\n",
> +			readl(i2c_dev->base + I2C_STATUS));
> +		if (i2c_dev->rst) {
> +			int ret = reset_control_reset(i2c_dev->rst);
> +
> +			if (ret < 0)
> +				dev_warn(i2c_dev->dev, "i2c soft reset failed, ret = %d\n", ret);
> +		}
>   		return -ETIMEDOUT;
> -
> +	}
>   	return i2c_dev->err;
>   }
>   
> @@ -544,6 +554,11 @@ static int sprd_i2c_probe(struct platform_device *pdev)
>   		return ret;
>   
>   	platform_set_drvdata(pdev, i2c_dev);
> +	i2c_dev->rst = devm_reset_control_get(i2c_dev->dev, "i2c_rst");

IMO, you need update the device binding file firstly.

> +	if (IS_ERR(i2c_dev->rst)) {
> +		dev_dbg(i2c_dev->dev, "reset control not configured!\n");
> +		i2c_dev->rst = NULL;
> +	}
>   
>   	ret = clk_prepare_enable(i2c_dev->clk);
>   	if (ret)

Re: [PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> We found that when the interrupt bit of the I2C controller is cleared,
> the ack/nack bit is also cleared at the same time. After clearing the
> interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> when nack cannot be recognized. To solve this problem, we used a global

This is a hardware bug?

> variable to record ack/nack information before clearing the interrupt
> bit instead of a local variable.
> 
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>   drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index aa602958d4fd..dec627ef408c 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -85,6 +85,7 @@ struct sprd_i2c {
>   	struct clk *clk;
>   	u32 src_clk;
>   	u32 bus_freq;
> +	bool ack_flag;
>   	struct completion complete;
>   	struct reset_control *rst;
>   	u8 *buf;
> @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
>   {
>   	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
>   
> +	i2c_dev->ack_flag = 0;
>   	writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
>   }
>   
> @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>   {
>   	struct sprd_i2c *i2c_dev = dev_id;
>   	struct i2c_msg *msg = i2c_dev->msg;
> -	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);

Before this patch, we will re-read the ack bit form the register, but 
now we just read it in sprd_i2c_isr(). Is it possible that we will miss 
the ack bit?

>   	u32 i2c_tran;
>   
>   	if (msg->flags & I2C_M_RD)
> @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>   	 * For reading data, ack is always true, if i2c_tran is not 0 which
>   	 * means we still need to contine to read data from slave.
>   	 */
> -	if (i2c_tran && ack) {
> +	if (i2c_tran && i2c_dev->ack_flag) {
>   		sprd_i2c_data_transfer(i2c_dev);
>   		return IRQ_HANDLED;
>   	}
> @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>   	 * If we did not get one ACK from slave when writing data, we should
>   	 * return -EIO to notify users.
>   	 */
> -	if (!ack)
> +	if (!i2c_dev->ack_flag)
>   		i2c_dev->err = -EIO;
>   	else if (msg->flags & I2C_M_RD && i2c_dev->count)
>   		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>   {
>   	struct sprd_i2c *i2c_dev = dev_id;
>   	struct i2c_msg *msg = i2c_dev->msg;
> -	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>   	u32 i2c_tran;
>   
>   	if (msg->flags & I2C_M_RD)
> @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>   	 * means we can read all data in one time, then we can finish this
>   	 * transmission too.
>   	 */
> -	if (!i2c_tran || !ack) {
> +	i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> +	if (!i2c_tran || !i2c_dev->ack_flag) {
>   		sprd_i2c_clear_start(i2c_dev);
>   		sprd_i2c_clear_irq(i2c_dev);
>   	}

Re: [PATCH V2 4/7] i2c: sprd: Add I2C controller driver to support dynamic switching of 400K/1M/3.4M frequency

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> When I2C-slaves supporting different frequencies use the same I2C
> controller, the I2C controller usually only operates at lower frequencies.
> In order to improve the performance of I2C-slaves transmission supporting
> faster frequencies, we dynamically configure the I2C operating frequency
> based on the value of the input parameter msg ->flag.

I am not sure if this is suitable to expand the msg->flag. Andi, how do 
you think? Thanks.

> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>   drivers/i2c/busses/i2c-sprd.c | 101 +++++++++++++++++++---------------
>   1 file changed, 57 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index dec627ef408c..f1f7fad42ecd 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -75,7 +75,14 @@
>   #define SPRD_I2C_PM_TIMEOUT	1000
>   /* timeout (ms) for transfer message */
>   #define I2C_XFER_TIMEOUT	1000
> -
> +/* dynamic modify clk_freq flag  */
> +#define I2C_3M4_FLAG		0x0100
> +#define I2C_1M_FLAG		0x0080
> +#define I2C_400K_FLAG		0x0040
> +
> +#define I2C_FREQ_400K		400000
> +#define I2C_FREQ_1M		1000000
> +#define I2C_FREQ_3_4M		3400000
>   /* SPRD i2c data structure */
>   struct sprd_i2c {
>   	struct i2c_adapter adap;
> @@ -94,6 +101,49 @@ struct sprd_i2c {
>   	int err;
>   };
>   
> +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
> +{
> +	u32 apb_clk = i2c_dev->src_clk;
> +	/*
> +	 * From I2C databook, the prescale calculation formula:
> +	 * prescale = freq_i2c / (4 * freq_scl) - 1;
> +	 */
> +	u32 i2c_dvd = apb_clk / (4 * freq) - 1;
> +	/*
> +	 * From I2C databook, the high period of SCL clock is recommended as
> +	 * 40% (2/5), and the low period of SCL clock is recommended as 60%
> +	 * (3/5), then the formula should be:
> +	 * high = (prescale * 2 * 2) / 5
> +	 * low = (prescale * 2 * 3) / 5
> +	 */
> +	u32 high = ((i2c_dvd << 1) * 2) / 5;
> +	u32 low = ((i2c_dvd << 1) * 3) / 5;
> +	u32 div0 = I2C_ADDR_DVD0_CALC(high, low);
> +	u32 div1 = I2C_ADDR_DVD1_CALC(high, low);
> +
> +	writel(div0, i2c_dev->base + ADDR_DVD0);
> +	writel(div1, i2c_dev->base + ADDR_DVD1);
> +
> +	/* Start hold timing = hold time(us) * source clock */
> +	switch (freq) {
> +	case I2C_MAX_STANDARD_MODE_FREQ:
> +		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_FAST_MODE_FREQ:
> +		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	default:
> +		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
> +		break;
> +	}
> +}
> +
>   static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
>   {
>   	writel(count, i2c_dev->base + I2C_COUNT);
> @@ -269,6 +319,12 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
>   		sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
>   	}
>   
> +	if (msg->flags & I2C_400K_FLAG)
> +		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_400K);
> +	else if (msg->flags & I2C_1M_FLAG)
> +		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_1M);
> +	else if (msg->flags & I2C_3M4_FLAG)
> +		sprd_i2c_set_clk(i2c_dev, I2C_FREQ_3_4M);
>   	/*
>   	 * We should enable rx fifo full interrupt to get data when receiving
>   	 * full data.
> @@ -331,49 +387,6 @@ static const struct i2c_algorithm sprd_i2c_algo = {
>   	.functionality = sprd_i2c_func,
>   };
>   
> -static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
> -{
> -	u32 apb_clk = i2c_dev->src_clk;
> -	/*
> -	 * From I2C databook, the prescale calculation formula:
> -	 * prescale = freq_i2c / (4 * freq_scl) - 1;
> -	 */
> -	u32 i2c_dvd = apb_clk / (4 * freq) - 1;
> -	/*
> -	 * From I2C databook, the high period of SCL clock is recommended as
> -	 * 40% (2/5), and the low period of SCL clock is recommended as 60%
> -	 * (3/5), then the formula should be:
> -	 * high = (prescale * 2 * 2) / 5
> -	 * low = (prescale * 2 * 3) / 5
> -	 */
> -	u32 high = ((i2c_dvd << 1) * 2) / 5;
> -	u32 low = ((i2c_dvd << 1) * 3) / 5;
> -	u32 div0 = I2C_ADDR_DVD0_CALC(high, low);
> -	u32 div1 = I2C_ADDR_DVD1_CALC(high, low);
> -
> -	writel(div0, i2c_dev->base + ADDR_DVD0);
> -	writel(div1, i2c_dev->base + ADDR_DVD1);
> -
> -	/* Start hold timing = hold time(us) * source clock */
> -	switch (freq) {
> -	case I2C_MAX_STANDARD_MODE_FREQ:
> -		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> -		break;
> -	case I2C_MAX_FAST_MODE_FREQ:
> -		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> -		break;
> -	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> -		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> -		break;
> -	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
> -		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> -		break;
> -	default:
> -		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
> -		break;
> -	}
> -}
> -
>   static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
>   {
>   	u32 tmp = I2C_DVD_OPT;

Re: [PATCH V2 6/7] i2c: sprd: Increase the waiting time for I2C transmission to avoid system crash issues

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> Due to the relatively low priority of the isr_thread, when the CPU
> load is high, the execution of sprd_i2c_isr_thread will be delayed.
> After the waiting time is exceeded, the I2C driver will perform
> operations such as disabling the I2C controller. Later, when
> sprd_i2c_isr_thread is called by the CPU, there will be kernel panic
> caused by illegal access to the IIC register. After pressure testing,
> we found that increasing the IIC waiting time to 10 seconds can
> avoid this problem.
> 
> Fixes: 0b884fe71f9e ("i2c: sprd: use a specific timeout to avoid system hang up issue")
> Cc: <stable@vger.kernel.org> # v5.11+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Looks reasonable to me.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   drivers/i2c/busses/i2c-sprd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index b65729ba7d5a..dbdac89ad482 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -74,7 +74,7 @@
>   /* timeout (ms) for pm runtime autosuspend */
>   #define SPRD_I2C_PM_TIMEOUT	1000
>   /* timeout (ms) for transfer message */
> -#define I2C_XFER_TIMEOUT	1000
> +#define I2C_XFER_TIMEOUT	10000
>   /* dynamic modify clk_freq flag  */
>   #define I2C_3M4_FLAG		0x0100
>   #define I2C_1M_FLAG		0x0080

Re: [PATCH V2 7/7] i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> The new I2C IP version on the UNISOC platform has added I2C_NACK_EN and
> I2C_TRANS_EN control bits. To ensure that the I2C controller can initiate
> transmission smoothly, these two bits need to be configured.

What is the side impact for old hardwares that does not support these 2 
bits?

> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>   drivers/i2c/busses/i2c-sprd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index dbdac89ad482..431c0db84d22 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -33,6 +33,8 @@
>   #define ADDR_RST		0x2c
>   
>   /* I2C_CTL */
> +#define I2C_NACK_EN		BIT(22)
> +#define I2C_TRANS_EN		BIT(21)
>   #define STP_EN			BIT(20)
>   #define FIFO_AF_LVL_MASK	GENMASK(19, 16)
>   #define FIFO_AF_LVL		16
> @@ -366,7 +368,7 @@ static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
>   	sprd_i2c_clear_irq(i2c_dev);
>   
>   	tmp = readl(i2c_dev->base + I2C_CTL);
> -	writel(tmp | I2C_EN | I2C_INT_EN, i2c_dev->base + I2C_CTL);
> +	writel(tmp | I2C_EN | I2C_INT_EN | I2C_NACK_EN | I2C_TRANS_EN, i2c_dev->base + I2C_CTL);
>   }
>   
>   static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,

Re: [PATCH V2 0/7] i2c: sprd: Modification of UNISOC Platform I2C Driver

[Baolin Wang]

On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> Recently, some bugs have been discovered during use, patch3 and
> patch5-6 are bug fixes. Also, this patchset add new features:
> patch1 allows I2C to use more frequencies for communication,
> patch2 allows I2C to use 'reset framework' for reset, and patch4 allows
> I2C controller to dynamically switch frequencies during use.

I suggest separating bugfix patches from feature patches to ensure that 
bugfix patches are reviewed and merged as soon as possible.

> 
> change in V2
> -Using 'I2C' instead of 'IIC' in the patch set.
> -Using imperative form in patch subject.
> -Use 'switch case' instead of 'else if' in PATCH 1/7.
> -Modify if (i2c_dev->rst != NULL) to if (i2c_dev->rst) in PATCH 2/7.
> -Modify some dev_err() to dev_warn() or dev_dbg().
> -Clear i2c_dev->ack_flag in sprd_i2c_clear_ack() in PATCH 3/7.
> -Modify the indentation format of the code in PATCH 4/7.
> -Move sprd_i2c_enable() above its caller in PATCH 5/7.
> -Remove 'Set I2C_RX_ACK when clear irq' commit.
> -Add Fixes tags.
> 
> Huangzheng Lai (7):
>    i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies
>    i2c: sprd: Add I2C driver to use 'reset framework' function
>    i2c: sprd: Use global variables to record I2C ack/nack status instead
>      of local variables
>    i2c: sprd: Add I2C controller driver to support dynamic switching of
>      400K/1M/3.4M frequency
>    i2c: sprd: Configure the enable bit of the I2C controller before each
>      transmission initiation
>    i2c: sprd: Increase the waiting time for I2C transmission to avoid
>      system crash issues
>    i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits
> 
>   drivers/i2c/busses/i2c-sprd.c | 166 ++++++++++++++++++++++------------
>   1 file changed, 106 insertions(+), 60 deletions(-)
> 

Re: [PATCH V2 1/7] i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies

[Andi Shyti]

Hi Huangzheng,

On Mon, Oct 23, 2023 at 04:11:52PM +0800, Huangzheng Lai wrote:
> Add support for 1Mhz and 3.4Mhz frequency configuration.
> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> Acked-by: Andi Shyti <andi.shyti@kernel.org>

you can make this:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

> ---
>  drivers/i2c/busses/i2c-sprd.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index ffc54fbf814d..b44916c6741d 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -343,10 +343,23 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
>  	writel(div1, i2c_dev->base + ADDR_DVD1);
>  
>  	/* Start hold timing = hold time(us) * source clock */
> -	if (freq == I2C_MAX_FAST_MODE_FREQ)
> -		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> -	else if (freq == I2C_MAX_STANDARD_MODE_FREQ)
> +	switch (freq) {
> +	case I2C_MAX_STANDARD_MODE_FREQ:
>  		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_FAST_MODE_FREQ:
> +		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_FAST_MODE_PLUS_FREQ:
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;
> +	case I2C_MAX_HIGH_SPEED_MODE_FREQ:
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +		break;

so these were defined but not used.

> +	default:
> +		dev_err(i2c_dev->dev, "Unsupported frequency: %d\n", freq);
> +		break;
> +	}
>  }
>  
>  static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
> @@ -519,9 +532,11 @@ static int sprd_i2c_probe(struct platform_device *pdev)
>  	if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
>  		i2c_dev->bus_freq = prop;
>  
> -	/* We only support 100k and 400k now, otherwise will return error. */
> +	/* We only support 100k\400k\1m\3.4m now, otherwise will return error. */
>  	if (i2c_dev->bus_freq != I2C_MAX_STANDARD_MODE_FREQ &&
> -	    i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ)
> +			i2c_dev->bus_freq != I2C_MAX_FAST_MODE_FREQ &&
> +			i2c_dev->bus_freq != I2C_MAX_FAST_MODE_PLUS_FREQ &&
> +			i2c_dev->bus_freq != I2C_MAX_HIGH_SPEED_MODE_FREQ)
>  		return -EINVAL;

This is a bit of a weird management of the bus_freq because it
goes like this:

	i2c_dev->bus_freq = I2C_MAX_STANDARD_MODE_FREQ;
	...
	if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
		i2c_dev->bus_freq = prop;

	if (...)
		return -EINVAL;

A follow-up cleanup can be removing the first assignement and
instead of returning -EINVAL, we can keep going:

	if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
		i2c_dev->bus_freq = prop;

	if (...) {
		dev_warn("...");
		i2c_dev->bus_freq = I2C_MAX_STANDARD_MODE_FREQ;
	}

But this is topic for a different patch.

Andi

>  
>  	ret = sprd_i2c_clk_init(i2c_dev);
> -- 
> 2.17.1
> 

Re: [PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[Andi Shyti]

Hi Huangzheng,

On Mon, Oct 23, 2023 at 04:11:54PM +0800, Huangzheng Lai wrote:
> We found that when the interrupt bit of the I2C controller is cleared,
> the ack/nack bit is also cleared at the same time. After clearing the
> interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> when nack cannot be recognized. To solve this problem, we used a global
> variable to record ack/nack information before clearing the interrupt
> bit instead of a local variable.
> 
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index aa602958d4fd..dec627ef408c 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -85,6 +85,7 @@ struct sprd_i2c {
>  	struct clk *clk;
>  	u32 src_clk;
>  	u32 bus_freq;
> +	bool ack_flag;

So that you are telling me that this is not racy because we won't
receive any irq until we pull the ack down. Am I understanding
correctly?

But if this patch is fixing an unstable ack flag, how can I
believe this won't end up into a race?

>  	struct completion complete;
>  	struct reset_control *rst;
>  	u8 *buf;
> @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
>  {
>  	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
>  
> +	i2c_dev->ack_flag = 0;
>  	writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
>  }
>  
> @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>  {
>  	struct sprd_i2c *i2c_dev = dev_id;
>  	struct i2c_msg *msg = i2c_dev->msg;
> -	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);

Where exactly did you see the ack going to '0', here in the
thread or in handler?

>  	u32 i2c_tran;
>  
>  	if (msg->flags & I2C_M_RD)
> @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>  	 * For reading data, ack is always true, if i2c_tran is not 0 which
>  	 * means we still need to contine to read data from slave.
>  	 */
> -	if (i2c_tran && ack) {
> +	if (i2c_tran && i2c_dev->ack_flag) {
>  		sprd_i2c_data_transfer(i2c_dev);
>  		return IRQ_HANDLED;
>  	}
> @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
>  	 * If we did not get one ACK from slave when writing data, we should
>  	 * return -EIO to notify users.
>  	 */
> -	if (!ack)
> +	if (!i2c_dev->ack_flag)
>  		i2c_dev->err = -EIO;
>  	else if (msg->flags & I2C_M_RD && i2c_dev->count)
>  		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>  {
>  	struct sprd_i2c *i2c_dev = dev_id;
>  	struct i2c_msg *msg = i2c_dev->msg;
> -	bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>  	u32 i2c_tran;
>  
>  	if (msg->flags & I2C_M_RD)
> @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
>  	 * means we can read all data in one time, then we can finish this
>  	 * transmission too.
>  	 */
> -	if (!i2c_tran || !ack) {
> +	i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> +	if (!i2c_tran || !i2c_dev->ack_flag) {
>  		sprd_i2c_clear_start(i2c_dev);

this clear_start() is called both here and in the thread, why?

Andi

>  		sprd_i2c_clear_irq(i2c_dev);
>  	}
> -- 
> 2.17.1
> 

Re: [PATCH V2 4/7] i2c: sprd: Add I2C controller driver to support dynamic switching of 400K/1M/3.4M frequency

[Andi Shyti]

Hi Huangzheng,

On Mon, Oct 23, 2023 at 04:11:55PM +0800, Huangzheng Lai wrote:
> When I2C-slaves supporting different frequencies use the same I2C
> controller, the I2C controller usually only operates at lower frequencies.
> In order to improve the performance of I2C-slaves transmission supporting
> faster frequencies, we dynamically configure the I2C operating frequency
> based on the value of the input parameter msg ->flag.
> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 101 +++++++++++++++++++---------------
>  1 file changed, 57 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index dec627ef408c..f1f7fad42ecd 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -75,7 +75,14 @@
>  #define SPRD_I2C_PM_TIMEOUT	1000
>  /* timeout (ms) for transfer message */
>  #define I2C_XFER_TIMEOUT	1000
> -
> +/* dynamic modify clk_freq flag  */
> +#define I2C_3M4_FLAG		0x0100
> +#define I2C_1M_FLAG		0x0080
> +#define I2C_400K_FLAG		0x0040
> +
> +#define I2C_FREQ_400K		400000
> +#define I2C_FREQ_1M		1000000
> +#define I2C_FREQ_3_4M		3400000

Why are you redefining these values?

You could use the defines you already have or, if you really want
a different name you could do:

#define I2C_FREQ_3_4M		I2C_MAX_HIGH_SPEED_MODE_FREQ

Rest looks good.

Andi

Re: [PATCH V2 5/7] i2c: sprd: Configure the enable bit of the I2C controller before each transmission initiation

[Andi Shyti]

On Mon, Oct 23, 2023 at 04:11:56PM +0800, Huangzheng Lai wrote:
> When a timeout exception occurs in the I2C driver, the I2C controller
> will be reset, and after resetting, control bits such as I2C_EN and
> I2C_INT_EN will be reset to 0, and the I2C master cannot initiate
> Transmission unless sprd_i2c_enable() is executed. To address this issue,
> this patch places sprd_i2c_enable() before each transmission initiation
> to ensure that the necessary control bits of the I2C controller are
> configured.

where is the timeout occurring? Is it a sporadic failure?

> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Would be nice if any from Orson, Boulin or Chunyan could take a
look here.

Andi

Re: [PATCH V2 6/7] i2c: sprd: Increase the waiting time for I2C transmission to avoid system crash issues

[Andi Shyti]

Hi Huangzheng,

On Mon, Oct 23, 2023 at 04:11:57PM +0800, Huangzheng Lai wrote:
> Due to the relatively low priority of the isr_thread, when the CPU
> load is high, the execution of sprd_i2c_isr_thread will be delayed.
> After the waiting time is exceeded, the I2C driver will perform
> operations such as disabling the I2C controller. Later, when
> sprd_i2c_isr_thread is called by the CPU, there will be kernel panic
> caused by illegal access to the IIC register. After pressure testing,
> we found that increasing the IIC waiting time to 10 seconds can
> avoid this problem.
> 
> Fixes: 0b884fe71f9e ("i2c: sprd: use a specific timeout to avoid system hang up issue")
> Cc: <stable@vger.kernel.org> # v5.11+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Acked-by: Andi Shyti <andi.shyti@kernel.org>

Andi

> ---
>  drivers/i2c/busses/i2c-sprd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index b65729ba7d5a..dbdac89ad482 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -74,7 +74,7 @@
>  /* timeout (ms) for pm runtime autosuspend */
>  #define SPRD_I2C_PM_TIMEOUT	1000
>  /* timeout (ms) for transfer message */
> -#define I2C_XFER_TIMEOUT	1000
> +#define I2C_XFER_TIMEOUT	10000
>  /* dynamic modify clk_freq flag  */
>  #define I2C_3M4_FLAG		0x0100
>  #define I2C_1M_FLAG		0x0080
> -- 
> 2.17.1
> 

Re: [PATCH V2 0/7] i2c: sprd: Modification of UNISOC Platform I2C Driver

[Andi Shyti]

Hi Huangzheng,

could you please use the [PATCH RESEND...] prefix when sending
the patch as it is?

Thanks,
Andi

On Mon, Oct 23, 2023 at 04:11:51PM +0800, Huangzheng Lai wrote:
> Recently, some bugs have been discovered during use, patch3 and 
> patch5-6 are bug fixes. Also, this patchset add new features:
> patch1 allows I2C to use more frequencies for communication,
> patch2 allows I2C to use 'reset framework' for reset, and patch4 allows
> I2C controller to dynamically switch frequencies during use.
> 
> change in V2
> -Using 'I2C' instead of 'IIC' in the patch set.
> -Using imperative form in patch subject.
> -Use 'switch case' instead of 'else if' in PATCH 1/7.
> -Modify if (i2c_dev->rst != NULL) to if (i2c_dev->rst) in PATCH 2/7.
> -Modify some dev_err() to dev_warn() or dev_dbg().
> -Clear i2c_dev->ack_flag in sprd_i2c_clear_ack() in PATCH 3/7.
> -Modify the indentation format of the code in PATCH 4/7.
> -Move sprd_i2c_enable() above its caller in PATCH 5/7.
> -Remove 'Set I2C_RX_ACK when clear irq' commit.
> -Add Fixes tags. 
> 
> Huangzheng Lai (7):
>   i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies
>   i2c: sprd: Add I2C driver to use 'reset framework' function
>   i2c: sprd: Use global variables to record I2C ack/nack status instead
>     of local variables
>   i2c: sprd: Add I2C controller driver to support dynamic switching of
>     400K/1M/3.4M frequency
>   i2c: sprd: Configure the enable bit of the I2C controller before each
>     transmission initiation
>   i2c: sprd: Increase the waiting time for I2C transmission to avoid
>     system crash issues
>   i2c: sprd: Add I2C_NACK_EN and I2C_TRANS_EN control bits
> 
>  drivers/i2c/busses/i2c-sprd.c | 166 ++++++++++++++++++++++------------
>  1 file changed, 106 insertions(+), 60 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH V2 2/7] i2c: sprd: Add I2C driver to use 'reset framework' function

[Krzysztof Kozlowski]

On 23/10/2023 10:11, Huangzheng Lai wrote:
> Add the 'reset framework' function for I2C drivers, which
> resets the I2C controller when a timeout exception occurs.
> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 19 +++++++++++++++++--


>  		return -ETIMEDOUT;
> -
> +	}
>  	return i2c_dev->err;
>  }
>  
> @@ -544,6 +554,11 @@ static int sprd_i2c_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, i2c_dev);
> +	i2c_dev->rst = devm_reset_control_get(i2c_dev->dev, "i2c_rst");

NAK

This is not documented in the bindings. You cannot sneak properties
without their documentation. Also, you cannot sneak new properties to
bindings conversion patches!

Send proper patchset following submission rules, so first documentation,
then driver implementing new feature. One patchset.

Best regards,
Krzysztof

Re: [PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[Krzysztof Kozlowski]

On 23/10/2023 10:11, Huangzheng Lai wrote:
> We found that when the interrupt bit of the I2C controller is cleared,
> the ack/nack bit is also cleared at the same time. After clearing the
> interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> when nack cannot be recognized. To solve this problem, we used a global
> variable to record ack/nack information before clearing the interrupt
> bit instead of a local variable.
> 
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Fixes must be send independent of features.

Best regards,
Krzysztof

Re: [PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[huangzheng lai]

On Mon, Oct 23, 2023 at 7:32 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 10/23/2023 4:11 PM, Huangzheng Lai wrote:
> > We found that when the interrupt bit of the I2C controller is cleared,
> > the ack/nack bit is also cleared at the same time. After clearing the
> > interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> > obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> > when nack cannot be recognized. To solve this problem, we used a global
>
> This is a hardware bug?
>

Yes.

> > variable to record ack/nack information before clearing the interrupt
> > bit instead of a local variable.
> >
> > Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> > Cc: <stable@vger.kernel.org> # v4.14+
> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> > ---
> >   drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> > index aa602958d4fd..dec627ef408c 100644
> > --- a/drivers/i2c/busses/i2c-sprd.c
> > +++ b/drivers/i2c/busses/i2c-sprd.c
> > @@ -85,6 +85,7 @@ struct sprd_i2c {
> >       struct clk *clk;
> >       u32 src_clk;
> >       u32 bus_freq;
> > +     bool ack_flag;
> >       struct completion complete;
> >       struct reset_control *rst;
> >       u8 *buf;
> > @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
> >   {
> >       u32 tmp = readl(i2c_dev->base + I2C_STATUS);
> >
> > +     i2c_dev->ack_flag = 0;
> >       writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
> >   }
> >
> > @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >   {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>
> Before this patch, we will re-read the ack bit form the register, but
> now we just read it in sprd_i2c_isr(). Is it possible that we will miss
> the ack bit?
>

Yes, we will miss the 'ack' bit after clear 'irq' bit.

> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * For reading data, ack is always true, if i2c_tran is not 0 which
> >        * means we still need to contine to read data from slave.
> >        */
> > -     if (i2c_tran && ack) {
> > +     if (i2c_tran && i2c_dev->ack_flag) {
> >               sprd_i2c_data_transfer(i2c_dev);
> >               return IRQ_HANDLED;
> >       }
> > @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * If we did not get one ACK from slave when writing data, we should
> >        * return -EIO to notify users.
> >        */
> > -     if (!ack)
> > +     if (!i2c_dev->ack_flag)
> >               i2c_dev->err = -EIO;
> >       else if (msg->flags & I2C_M_RD && i2c_dev->count)
> >               sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> > @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >   {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >        * means we can read all data in one time, then we can finish this
> >        * transmission too.
> >        */
> > -     if (!i2c_tran || !ack) {
> > +     i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> > +     if (!i2c_tran || !i2c_dev->ack_flag) {
> >               sprd_i2c_clear_start(i2c_dev);
> >               sprd_i2c_clear_irq(i2c_dev);
> >       }

Re: [PATCH V2 3/7] i2c: sprd: Use global variables to record I2C ack/nack status instead of local variables

[huangzheng lai]

Hi Andi,


On Wed, Oct 25, 2023 at 5:20 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Huangzheng,
>
> On Mon, Oct 23, 2023 at 04:11:54PM +0800, Huangzheng Lai wrote:
> > We found that when the interrupt bit of the I2C controller is cleared,
> > the ack/nack bit is also cleared at the same time. After clearing the
> > interrupt bit in sprd_i2c_isr(), incorrect ack/nack information will be
> > obtained in sprd_i2c_isr_thread(), resulting in incorrect communication
> > when nack cannot be recognized. To solve this problem, we used a global
> > variable to record ack/nack information before clearing the interrupt
> > bit instead of a local variable.
> >
> > Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> > Cc: <stable@vger.kernel.org> # v4.14+
> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> > ---
> >  drivers/i2c/busses/i2c-sprd.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> > index aa602958d4fd..dec627ef408c 100644
> > --- a/drivers/i2c/busses/i2c-sprd.c
> > +++ b/drivers/i2c/busses/i2c-sprd.c
> > @@ -85,6 +85,7 @@ struct sprd_i2c {
> >       struct clk *clk;
> >       u32 src_clk;
> >       u32 bus_freq;
> > +     bool ack_flag;
>
> So that you are telling me that this is not racy because we won't
> receive any irq until we pull the ack down. Am I understanding
> correctly?
>
> But if this patch is fixing an unstable ack flag, how can I
> believe this won't end up into a race?
>

In fact, the ack flag is not unstable. However, the ack state on the
hardware will
reset as the interrupt state is cleared, nd there are some unexpected coupling
relationships on the hardware.We need to obtain and save the ack flag before
clearing the interrupt state.

> >       struct completion complete;
> >       struct reset_control *rst;
> >       u8 *buf;
> > @@ -119,6 +120,7 @@ static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
> >  {
> >       u32 tmp = readl(i2c_dev->base + I2C_STATUS);
> >
> > +     i2c_dev->ack_flag = 0;
> >       writel(tmp & ~I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
> >  }
> >
> > @@ -393,7 +395,6 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >  {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
>
> Where exactly did you see the ack going to '0', here in the
> thread or in handler?
>

After function sprd_i2c_irq() is executed, the ack state bit in the
hardware will be reset to the default ack.
When the slave nack, the ack flag obtained in the thread is incorrect,
which can cause
the i2c driver to mistakenly believe that the transmission was successful.

> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -409,7 +410,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * For reading data, ack is always true, if i2c_tran is not 0 which
> >        * means we still need to contine to read data from slave.
> >        */
> > -     if (i2c_tran && ack) {
> > +     if (i2c_tran && i2c_dev->ack_flag) {
> >               sprd_i2c_data_transfer(i2c_dev);
> >               return IRQ_HANDLED;
> >       }
> > @@ -420,7 +421,7 @@ static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> >        * If we did not get one ACK from slave when writing data, we should
> >        * return -EIO to notify users.
> >        */
> > -     if (!ack)
> > +     if (!i2c_dev->ack_flag)
> >               i2c_dev->err = -EIO;
> >       else if (msg->flags & I2C_M_RD && i2c_dev->count)
> >               sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
> > @@ -437,7 +438,6 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >  {
> >       struct sprd_i2c *i2c_dev = dev_id;
> >       struct i2c_msg *msg = i2c_dev->msg;
> > -     bool ack = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> >       u32 i2c_tran;
> >
> >       if (msg->flags & I2C_M_RD)
> > @@ -456,7 +456,8 @@ static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
> >        * means we can read all data in one time, then we can finish this
> >        * transmission too.
> >        */
> > -     if (!i2c_tran || !ack) {
> > +     i2c_dev->ack_flag = !(readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK);
> > +     if (!i2c_tran || !i2c_dev->ack_flag) {
> >               sprd_i2c_clear_start(i2c_dev);
>
> this clear_start() is called both here and in the thread, why?
>

When it is determined here that there is no remaining i2c data or slave nack,
clear_start() is called for stopping i2c controller transmission.
In the thread, clear_start() is called because all i2c data
reads/writes are completed.

> Andi
>
> >               sprd_i2c_clear_irq(i2c_dev);
> >       }
> > --
> > 2.17.1
> >