[PATCH 0/8] i2c: sprd: Modification of UNIOC Platform IIC Driver

[PATCH 0/8] i2c: sprd: Modification of UNIOC Platform IIC Driver

[Huangzheng Lai]

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

Huangzheng Lai (8):
  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 IIC ack/nack status instead
    of local variables
  i2c: sprd: Add IIC controller driver to support dynamic switching of
    400K/1M/3.4M frequency
  i2c: sprd: Configure the enable bit of the IIC controller before each
    transmission initiation
  i2c: sprd: Add additional IIC control bit configuration to adapt to
    the new IP version of the UNISOC platform
  i2c: sprd: Set I2C_RX_ACK when clear irq
  i2c: sprd: Increase the waiting time for IIC transmission to avoid
    system crash issues

 drivers/i2c/busses/i2c-sprd.c | 70 +++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 15 deletions(-)

-- 
2.17.1

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

[Huangzheng Lai]

This patch adds I2C controller driver support for 1Mhz and 3.4Mhz
frequency configurations.

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

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index ffc54fbf814d..acc2a4d4eeae 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -347,6 +347,10 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
 		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
 	else if (freq == I2C_MAX_STANDARD_MODE_FREQ)
 		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
+	else if (freq == I2C_MAX_FAST_MODE_PLUS_FREQ)
+		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+	else if (freq == I2C_MAX_HIGH_SPEED_MODE_FREQ)
+		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
 }
 
 static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
@@ -519,9 +523,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 2/8] i2c: sprd: Add I2C driver to use 'reset framework' function

[Huangzheng Lai]

This patch adds 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 acc2a4d4eeae..066b3a9c30c8 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;
@@ -247,6 +249,7 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
 {
 	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
 	unsigned long time_left;
+	int ret;
 
 	i2c_dev->msg = msg;
 	i2c_dev->buf = msg->buf;
@@ -278,9 +281,16 @@ 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 != NULL) {
+			ret = reset_control_reset(i2c_dev->rst);
+			if (ret < 0)
+				dev_err(i2c_dev->dev, "i2c soft reset failed, ret = %d\n", ret);
+		}
 		return -ETIMEDOUT;
-
+	}
 	return i2c_dev->err;
 }
 
@@ -535,6 +545,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_err(i2c_dev->dev, "can't get i2c reset node\n");
+		i2c_dev->rst = NULL;
+	}
 
 	ret = clk_prepare_enable(i2c_dev->clk);
 	if (ret)
-- 
2.17.1

[PATCH 3/8] i2c: sprd: Use global variables to record IIC ack/nack status instead of local variables

[Huangzheng Lai]

We found that when the interrupt bit of the IIC 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.

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

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 066b3a9c30c8..549b60dd3273 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;
@@ -384,7 +385,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)
@@ -400,7 +400,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;
 	}
@@ -411,7 +411,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);
@@ -428,7 +428,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)
@@ -447,7 +446,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 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

[Huangzheng Lai]

When IIC-slaves supporting different frequencies use the same IIC
controller, the IIC controller usually only operates at lower frequencies.
In order to improve the performance of IIC-slaves transmission supporting
faster frequencies, we dynamically configure the IIC 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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 549b60dd3273..02c11a9ff2da 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,8 @@ struct sprd_i2c {
 	int err;
 };
 
+static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq);
+
 static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
 {
 	writel(count, i2c_dev->base + I2C_COUNT);
@@ -269,6 +278,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.
-- 
2.17.1

[PATCH 5/8] i2c: sprd: Configure the enable bit of the IIC controller before each transmission initiation

[Huangzheng Lai]

When a timeout exception occurs in the IIC driver, the IIC controller
will be reset, and after resetting, control bits such as I2C_EN and
I2C_INT_EN will be reset to 0, and the IIC 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 IIC controller are
configured.

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

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 02c11a9ff2da..7314c897525d 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -310,6 +310,8 @@ 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);
+
 static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
 				struct i2c_msg *msgs, int num)
 {
@@ -320,6 +322,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)
@@ -661,8 +665,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 6/8] i2c: sprd: Add additional IIC control bit configuration to adapt to the new IP version of the UNISOC platform

[Huangzheng Lai]

The new IIC IP version on the UNISOC platform has added I2C_NACK_EN and
I2C_TRANS_EN control bits. To ensure that the IIC 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 7314c897525d..d867389c7f17 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
@@ -397,7 +399,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 irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
-- 
2.17.1

[PATCH 7/8] i2c: sprd: Set I2C_RX_ACK when clear irq

[Huangzheng Lai]

We found that when clearing the I2C_TX_ACK bit, the I2C_MODE bit will
also be cleared to 0. When the IIC master reads data, this situation
will cause the FIFO of the IIC to be empty after clearing the interrupt.
To address this issue, when clearing interrupts, set I2C_RX_ACK bit to 1,
as writing 1 to this bit will not take effect.

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 d867389c7f17..6f65f28ea69d 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -138,7 +138,7 @@ static void sprd_i2c_clear_irq(struct sprd_i2c *i2c_dev)
 {
 	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
 
-	writel(tmp & ~I2C_INT, i2c_dev->base + I2C_STATUS);
+	writel((tmp & ~I2C_INT) | I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
 }
 
 static void sprd_i2c_reset_fifo(struct sprd_i2c *i2c_dev)
-- 
2.17.1

[PATCH 8/8] i2c: sprd: Increase the waiting time for IIC 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 IIC driver will perform
operations such as disabling the IIC 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.

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 6f65f28ea69d..3c7af04fa177 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -76,7 +76,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 1/8] i2c: sprd: Add configurations that support 1Mhz and 3.4Mhz frequencies

[Yann Sionneau]

Le 17/08/2023 à 11:45, Huangzheng Lai a écrit :
> This patch adds I2C controller driver support for 1Mhz and 3.4Mhz
> frequency configurations.
> @@ -347,6 +347,10 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
>   		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
>   	else if (freq == I2C_MAX_STANDARD_MODE_FREQ)
>   		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> +	else if (freq == I2C_MAX_FAST_MODE_PLUS_FREQ)
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +	else if (freq == I2C_MAX_HIGH_SPEED_MODE_FREQ)
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
>   }

Maybe using a switch case could be beneficial to readability instead of 
a series of if / else if ?

-- 

Yann

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

[Yann Sionneau]

Le 17/08/2023 à 11:45, Huangzheng Lai a écrit :

> This patch adds the 'reset framework' function for I2C drivers, which
> resets the I2C controller when a timeout exception occurs.
...
> +		if (i2c_dev->rst != NULL) {
if (i2c_dev->rst) {
> +			ret = reset_control_reset(i2c_dev->rst);
> +			if (ret < 0)
> +				dev_err(i2c_dev->dev, "i2c soft reset failed, ret = %d\n", ret);
> +		}

-- 

Yann

Re: [PATCH 0/8] i2c: sprd: Modification of UNIOC Platform IIC Driver

[Andi Shyti]

Hi Orson,

On Thu, Aug 17, 2023 at 05:45:12PM +0800, Huangzheng Lai wrote:
> Recently, some bugs have been discovered during use, and patch3 
> and patch5-8 are bug fixes. Also, this patchset add new features: 
> patch1 allows IIC to use more frequencies for communication, 
> patch2 allows IIC to use 'reset framework' for reset, and patch4 allows 
> IIC controller to dynamically switch frequencies during use.
> 
> Huangzheng Lai (8):
>   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 IIC ack/nack status instead
>     of local variables
>   i2c: sprd: Add IIC controller driver to support dynamic switching of
>     400K/1M/3.4M frequency
>   i2c: sprd: Configure the enable bit of the IIC controller before each
>     transmission initiation
>   i2c: sprd: Add additional IIC control bit configuration to adapt to
>     the new IP version of the UNISOC platform
>   i2c: sprd: Set I2C_RX_ACK when clear irq
>   i2c: sprd: Increase the waiting time for IIC transmission to avoid
>     system crash issues

there are some changes here that require device knowledge... can
you please take a look here?

Andi

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

[Chunyan Zhang]

On Thu, 17 Aug 2023 at 17:46, Huangzheng Lai <Huangzheng.Lai@unisoc.com> wrote:
>
> We found that when the interrupt bit of the IIC 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.
>
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 066b3a9c30c8..549b60dd3273 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;
> @@ -384,7 +385,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)
> @@ -400,7 +400,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;
>         }
> @@ -411,7 +411,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);
> @@ -428,7 +428,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)
> @@ -447,7 +446,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);

Do we need clear i2c_dev->ack_flag in sprd_i2c_clear_ack()?

> +       if (!i2c_tran || !i2c_dev->ack_flag) {
>                 sprd_i2c_clear_start(i2c_dev);
>                 sprd_i2c_clear_irq(i2c_dev);
>         }
> --
> 2.17.1
>

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

[Chunyan Zhang]

On Thu, 17 Aug 2023 at 17:46, Huangzheng Lai <Huangzheng.Lai@unisoc.com> wrote:
>
> When IIC-slaves supporting different frequencies use the same IIC

%s/I2C/IIC

> controller, the IIC controller usually only operates at lower frequencies.
> In order to improve the performance of IIC-slaves transmission supporting
> faster frequencies, we dynamically configure the IIC 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 | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 549b60dd3273..02c11a9ff2da 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 <space> I2C_3M4_FLAG <tab> 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

ditto

>  /* SPRD i2c data structure */
>  struct sprd_i2c {
>         struct i2c_adapter adap;
> @@ -94,6 +101,8 @@ struct sprd_i2c {
>         int err;
>  };
>
> +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq);

I would suggest moving this whole function above its caller.

> +
>  static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
>  {
>         writel(count, i2c_dev->base + I2C_COUNT);
> @@ -269,6 +278,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)

Where does this flag set? I'm not sure we can use msg->flags like
this. I don't know i2c very well.

Thanks,
Chunyan

> +               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.
> --
> 2.17.1
>

Re: [PATCH 6/8] i2c: sprd: Add additional IIC control bit configuration to adapt to the new IP version of the UNISOC platform

[Chunyan Zhang]

On Thu, 17 Aug 2023 at 17:46, Huangzheng Lai <Huangzheng.Lai@unisoc.com> wrote:
>
> The new IIC IP version on the UNISOC platform has added I2C_NACK_EN and
> I2C_TRANS_EN control bits. To ensure that the IIC controller can initiate
> transmission smoothly, these two bits need to be configured.
>
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

I would suggest changing the subject to "i2c: sprd: Add I2C_NACK_EN
and I2C_TRANS_EN control bits".
> ---
>  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 7314c897525d..d867389c7f17 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
> @@ -397,7 +399,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 irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
> --
> 2.17.1
>

Re: [PATCH 5/8] i2c: sprd: Configure the enable bit of the IIC controller before each transmission initiation

[Chunyan Zhang]

On Thu, 17 Aug 2023 at 17:46, Huangzheng Lai <Huangzheng.Lai@unisoc.com> wrote:
>
> When a timeout exception occurs in the IIC driver, the IIC controller
> will be reset, and after resetting, control bits such as I2C_EN and
> I2C_INT_EN will be reset to 0, and the IIC 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 IIC controller are
> configured.
>
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 02c11a9ff2da..7314c897525d 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -310,6 +310,8 @@ 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);

Can we move this whole function above its caller?

> +
>  static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
>                                 struct i2c_msg *msgs, int num)
>  {
> @@ -320,6 +322,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)
> @@ -661,8 +665,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
>

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

[Andi Shyti]

Hi Huangzheng,

On Thu, Aug 17, 2023 at 05:45:13PM +0800, Huangzheng Lai wrote:
> This patch adds I2C controller driver support for 1Mhz and 3.4Mhz
> frequency configurations.

please avoid the "This patch adds..." form, it can be sometimes
misleading... Use always the imperative form.

"Add support for 1Mhz and 3.5Mhz frequency configuration"

> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
> ---
>  drivers/i2c/busses/i2c-sprd.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index ffc54fbf814d..acc2a4d4eeae 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -347,6 +347,10 @@ static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, u32 freq)
>  		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
>  	else if (freq == I2C_MAX_STANDARD_MODE_FREQ)
>  		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> +	else if (freq == I2C_MAX_FAST_MODE_PLUS_FREQ)
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +	else if (freq == I2C_MAX_HIGH_SPEED_MODE_FREQ)
> +		writel((8 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);

if you want you can queue up a patch that makes this train of
"else if" in a switch case.

>  }
>  
>  static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
> @@ -519,9 +523,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)

We finally make use of these defines :)

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

Thanks,
Andi

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

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

[Andi Shyti]

Hi Huangzheng,

On Thu, Aug 17, 2023 at 05:45:14PM +0800, Huangzheng Lai wrote:
> This patch adds the 'reset framework' function for I2C drivers, which
> resets the I2C controller when a timeout exception occurs.

as in the earlier patch, please use the imperative form.

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

[...]

> @@ -247,6 +249,7 @@ static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
>  {
>  	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
>  	unsigned long time_left;
> +	int ret;

please move this declaration...

>  
>  	i2c_dev->msg = msg;
>  	i2c_dev->buf = msg->buf;
> @@ -278,9 +281,16 @@ 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 != NULL) {

... here.

> +			ret = reset_control_reset(i2c_dev->rst);
> +			if (ret < 0)
> +				dev_err(i2c_dev->dev, "i2c soft reset failed, ret = %d\n", ret);

dev_warn()

> +		}
>  		return -ETIMEDOUT;
> -
> +	}
>  	return i2c_dev->err;
>  }
>  
> @@ -535,6 +545,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_err(i2c_dev->dev, "can't get i2c reset node\n");

if the i2c_rst is optional then this is not an error and it
should use dev_dbg(); right?

In that case please reword the message to "reset control not
configured".

Andi

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

[Andi Shyti]

Hi Huangzheng,

On Thu, Aug 17, 2023 at 05:45:15PM +0800, Huangzheng Lai wrote:
> We found that when the interrupt bit of the IIC 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.
> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Is this a fix? Then please consider adding

Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
Cc: <stable@vger.kernel.org> # v4.14+

> ---
>  drivers/i2c/busses/i2c-sprd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 066b3a9c30c8..549b60dd3273 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;

smells a bit racy... however we are in the same interrupt cycle.

Do you think we might need a spinlock around here?

>  	struct completion complete;
>  	struct reset_control *rst;
>  	u8 *buf;
> @@ -384,7 +385,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)
> @@ -400,7 +400,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;
>  	}
> @@ -411,7 +411,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);
> @@ -428,7 +428,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)
> @@ -447,7 +446,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);

there is a question from Chunyan here.

I like more

	val = readl(...);
	i2c_dev->ack_flag = !(val & I2C_RX_ACK);

a matter of taste, your choice.

Andi

> +	if (!i2c_tran || !i2c_dev->ack_flag) {
>  		sprd_i2c_clear_start(i2c_dev);
>  		sprd_i2c_clear_irq(i2c_dev);
>  	}
> -- 
> 2.17.1
> 

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

[Andi Shyti]

Hi Chunyan,

[...]

> > When IIC-slaves supporting different frequencies use the same IIC
> 
> %s/I2C/IIC

[...]

> >  #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 <space> I2C_3M4_FLAG <tab> 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
> 
> ditto

why should he use IIC instead of I2C. The file's defines start
with I2C, for consistency he should use the same prefix.

Andi

Re: [PATCH 7/8] i2c: sprd: Set I2C_RX_ACK when clear irq

[Andi Shyti]

Hi Huangzheng,

On Thu, Aug 17, 2023 at 05:45:19PM +0800, Huangzheng Lai wrote:
> We found that when clearing the I2C_TX_ACK bit, the I2C_MODE bit will

where is the I2C_TX_ACK bit cleared?

> also be cleared to 0. When the IIC master reads data, this situation
> will cause the FIFO of the IIC to be empty after clearing the interrupt.

Still I am not understanding the situation. In the clear_irq you
don't seem to do anything with the TX_ACK.

> To address this issue, when clearing interrupts, set I2C_RX_ACK bit to 1,
> as writing 1 to this bit will not take effect.

writing 1 to which bit?

Andi

> 
> 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 d867389c7f17..6f65f28ea69d 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -138,7 +138,7 @@ static void sprd_i2c_clear_irq(struct sprd_i2c *i2c_dev)
>  {
>  	u32 tmp = readl(i2c_dev->base + I2C_STATUS);
>  
> -	writel(tmp & ~I2C_INT, i2c_dev->base + I2C_STATUS);
> +	writel((tmp & ~I2C_INT) | I2C_RX_ACK, i2c_dev->base + I2C_STATUS);
>  }
>  
>  static void sprd_i2c_reset_fifo(struct sprd_i2c *i2c_dev)
> -- 
> 2.17.1
> 

Re: [PATCH 8/8] i2c: sprd: Increase the waiting time for IIC transmission to avoid system crash issues

[Andi Shyti]

Hi Huangzheng,

On Thu, Aug 17, 2023 at 05:45:20PM +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 IIC driver will perform
> operations such as disabling the IIC 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.
> 
> Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>

Is this a fix? Then please consider adding:

0b884fe71f9e ("i2c: sprd: use a specific timeout to avoid system hang up issue")
Cc: <stable@vger.kernel.org> # v5.11+

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 6f65f28ea69d..3c7af04fa177 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -76,7 +76,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 4/8] i2c: sprd: Add IIC controller driver to support dynamic switching of 400K/1M/3.4M frequency

[Chunyan Zhang]

Hi Andi,

On Sun, 3 Sept 2023 at 05:08, Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Chunyan,
>
> [...]
>
> > > When IIC-slaves supporting different frequencies use the same IIC
> >
> > %s/I2C/IIC

I just noticed that this was the reverse, I meant "%s/IIC/I2C" :)
That's saying we should use I2C in the whole driver.

> [...]
>
> > >  #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 <space> I2C_3M4_FLAG <tab> 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
> >
> > ditto

I meant "#define <space> I2C_FREQ_3_4M <tab> 3400000"

>
> why should he use IIC instead of I2C. The file's defines start
> with I2C, for consistency he should use the same prefix.
>

Yes, I agree.

Thanks,
Chunyan

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

[Andi Shyti]

Hi Chunyan,

> > > >  #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 <space> I2C_3M4_FLAG <tab> 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
> > >
> > > ditto
> 
> I meant "#define <space> I2C_FREQ_3_4M <tab> 3400000"

right! Agree with your comment!

> >
> > why should he use IIC instead of I2C. The file's defines start
> > with I2C, for consistency he should use the same prefix.
> >
> 
> Yes, I agree.

Thanks,
Andi

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

[Chunyan Zhang]

On Wed, 13 Sept 2023 at 15:12, huangzheng lai <laihuangzheng@gmail.com> wrote:
>
> Hi Chunyan，
>
> I don't think it's necessary to clear  i2c_dev->ack_flag in sprd_i2c_clear_ack() . Because every time a new interrupt is triggered, it will retrieve the value of i2c_dev->ack_flag in sprd_i2c_isr and then use it in sprd_i2c_isr_thread.

You're assuming that ack_flag is and will be used in sprd_i2c_isr_thread() only.

If ack_flag is used to represent the ack/nack bit of interrupt status
register, I think we should clear it as well when clearing ack/nack
bit.

BTW, please make sure your email is plain-text mode so that people can
receive from the mail list, and do not top-post again.

Thanks,
Chunyan

>
> On Sun, Sep 3, 2023 at 5:05 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>>
>> Hi Huangzheng,
>>
>> On Thu, Aug 17, 2023 at 05:45:15PM +0800, Huangzheng Lai wrote:
>> > We found that when the interrupt bit of the IIC 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.
>> >
>> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
>>
>> Is this a fix? Then please consider adding
>>
>> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
>> Cc: <stable@vger.kernel.org> # v4.14+
>>
>> > ---
>> >  drivers/i2c/busses/i2c-sprd.c | 10 +++++-----
>> >  1 file changed, 5 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
>> > index 066b3a9c30c8..549b60dd3273 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;
>>
>> smells a bit racy... however we are in the same interrupt cycle.
>>
>> Do you think we might need a spinlock around here?
>>
>> >       struct completion complete;
>> >       struct reset_control *rst;
>> >       u8 *buf;
>> > @@ -384,7 +385,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)
>> > @@ -400,7 +400,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;
>> >       }
>> > @@ -411,7 +411,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);
>> > @@ -428,7 +428,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)
>> > @@ -447,7 +446,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);
>>
>> there is a question from Chunyan here.
>>
>> I like more
>>
>>         val = readl(...);
>>         i2c_dev->ack_flag = !(val & I2C_RX_ACK);
>>
>> a matter of taste, your choice.
>>
>> Andi
>>
>> > +     if (!i2c_tran || !i2c_dev->ack_flag) {
>> >               sprd_i2c_clear_start(i2c_dev);
>> >               sprd_i2c_clear_irq(i2c_dev);
>> >       }
>> > --
>> > 2.17.1
>> >

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

[huangzheng lai]

Hi Andi,

On Sun, Sep 3, 2023 at 5:05 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Huangzheng,
>
> On Thu, Aug 17, 2023 at 05:45:15PM +0800, Huangzheng Lai wrote:
> > We found that when the interrupt bit of the IIC 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.
> >
> > Signed-off-by: Huangzheng Lai <Huangzheng.Lai@unisoc.com>
>
> Is this a fix? Then please consider adding
>
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Cc: <stable@vger.kernel.org> # v4.14+

Thank you for your prompt. In the next version of the patch, I will
add the fixes tag.

>
> > ---
> >  drivers/i2c/busses/i2c-sprd.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> > index 066b3a9c30c8..549b60dd3273 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;
>
> smells a bit racy... however we are in the same interrupt cycle.
>
> Do you think we might need a spinlock around here?

The fifo empty and full interrupt enable will be turned off in
sprd_i2c_isr(), and will not be reset until sprd_i2c_isr_thread()
finishes processing, depending on the situation. Apart from these two
interrupt types, there are only two types left: transmission
completion and transmission failure. Both interrupts need to be re
initiated for transmission to occur, and transmission will not be re
initiated until the current data processing is completed.

Thanks,
Huangzheng