diff mbox series

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

Message ID 20230817094520.21286-4-Huangzheng.Lai@unisoc.com
State Superseded
Headers show
Series i2c: sprd: Modification of UNIOC Platform IIC Driver | expand

Commit Message

Huangzheng Lai Aug. 17, 2023, 9:45 a.m. UTC 
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(-)

Comments

Chunyan Zhang Aug. 31, 2023, 7:04 a.m. UTC | #1 
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
>
Andi Shyti Sept. 2, 2023, 9:05 p.m. UTC | #2 
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
>
Chunyan Zhang Sept. 13, 2023, 8:28 a.m. UTC | #3 
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
>> >
huangzheng lai Sept. 13, 2023, 12:28 p.m. UTC | #4 
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
diff mbox series

Patch

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);
 	}