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

[permalink] [raw]

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

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

Signed-off-by: Huangzheng Lai <[email protected]>
---
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

2023-10-23 08:13:48

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-10-23 08:14:06

[permalink] [raw]

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

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: <[email protected]> # v4.14+
Signed-off-by: Huangzheng Lai <[email protected]>
---
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

2023-10-23 08:14:17

[permalink] [raw]

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

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: <[email protected]> # v5.11+
Signed-off-by: Huangzheng Lai <[email protected]>
---
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

2023-10-23 08:14:39

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-10-23 11:27:09

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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)

2023-10-23 11:32:31

[permalink] [raw]

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

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: <[email protected]> # v4.14+
> Signed-off-by: Huangzheng Lai <[email protected]>
> ---
> 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);
> }

2023-10-23 11:37:23

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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;

2023-10-23 11:39:27

[permalink] [raw]

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

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: <[email protected]> # v5.11+
> Signed-off-by: Huangzheng Lai <[email protected]>

Looks reasonable to me.
Reviewed-by: Baolin Wang <[email protected]>

> ---
> 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

2023-10-23 11:44:33

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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,

2023-10-23 11:45:23

[permalink] [raw]

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

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(-)
>

2023-10-24 21:20:34

[permalink] [raw]

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

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: <[email protected]> # v4.14+
> Signed-off-by: Huangzheng Lai <[email protected]>
> ---
> 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
>

2023-10-24 21:29:06

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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

2023-10-24 21:38:55

[permalink] [raw]

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

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: <[email protected]> # v5.11+
> Signed-off-by: Huangzheng Lai <[email protected]>

Acked-by: Andi Shyti <[email protected]>

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
>

2023-10-24 21:40:37