i2c: imx: fix missing stop condition in single-master mode

PATCH In review Stefan Eichenberger 4 participants 6 messages Dec 16, 2024 – Dec 19, 2024

Subsystems (3)

Arm i.MX and Layerscape I2C subsystem host drivers Freescale i.MX I2C driver

lore

Share & tools

lore

Stefan Eichenberger

author

2024-12-16 15:18 UTC #0 lore raw

PATCH 1/1 i2c: imx: fix missing stop condition in single-master mode

From: Stefan Eichenberger <[email protected]>

A regression was introduced with the implementation of single-master mode, preventing proper stop conditions from being generated. Devices that require a valid stop condition, such as EEPROMs, fail to function correctly as a result.

The issue only affects devices with the single-master property enabled.

This commit resolves the issue by re-enabling I2C bus busy bit (IBB) polling for single-master mode when generating a stop condition. The fix further ensures that the i2c_imx->stopped flag is cleared at the start of each transfer, allowing the stop condition to be correctly generated in i2c_imx_stop().

According to the reference manual (IMX8MMRM, Rev. 2, 09/2019, page 5270), polling the IBB bit to determine if the bus is free is only necessary in multi-master mode. Consequently, the IBB bit is not polled for the start condition in single-master mode.

Fixes 6692694aca86 ("i2c: imx: do not poll for bus busy in single master mode")

Signed-off-by Stefan Eichenberger <[email protected]>

---
 drivers/i2c/busses/i2c-imx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/i2c/busses/i2c-imx.c +3 -5

index f751d231ded8..cbf66a69e20b 100644
@@ -534,20 +534,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
-
-	if (!i2c_imx->multi_master)
-		return 0;
+	bool multi_master = i2c_imx->multi_master;
 
 	while (1) {
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 
 		/* check for arbitration lost */
-		if (temp & I2SR_IAL) {
+		if (multi_master && (temp & I2SR_IAL)) {
 			i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
 			return -EAGAIN;
 		}
 
-		if (for_busy && (temp & I2SR_IBB)) {
+		if (for_busy && (!multi_master || (temp & I2SR_IBB))) {
 			i2c_imx->stopped = 0;
 			break;
 		}

Signature · 1 line

2.45.2

Frank Li

maintainer Reviewed-by on v1 1/1

2024-12-16 16:35 UTC #1 lore raw

Quoted 31 lines · Stefan Eichenberger

From: Stefan Eichenberger <[email protected]>
A regression was introduced with the implementation of single-master mode, preventing proper stop conditions from being generated. Devices that require a valid stop condition, such as EEPROMs, fail to function correctly as a result.
The issue only affects devices with the single-master property enabled.
This commit resolves the issue by re-enabling I2C bus busy bit (IBB) polling for single-master mode when generating a stop condition. The fix further ensures that the i2c_imx->stopped flag is cleared at the start of each transfer, allowing the stop condition to be correctly generated in i2c_imx_stop().
According to the reference manual (IMX8MMRM, Rev. 2, 09/2019, page 5270), polling the IBB bit to determine if the bus is free is only necessary in multi-master mode. Consequently, the IBB bit is not polled for the start condition in single-master mode.
Fixes 6692694aca86 ("i2c: imx: do not poll for bus busy in single master mode")
Signed-off-by Stefan Eichenberger <[email protected]>
---
 drivers/i2c/busses/i2c-imx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-imx.c +0 -0
index f751d231ded8..cbf66a69e20b 100644

drivers/i2c/busses/i2c-imx.c static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a · line 537

Quoted 7 lines

drivers/i2c/busses/i2c-imx.c +1 -3

@@ -534,20 +534,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
-
-	if (!i2c_imx->multi_master)
-		return 0;
+	bool multi_master = i2c_imx->multi_master;

Move "bool multi_master = i2c_imx->multi_master;" before "unsigned long orig_jiffies = jiffies;" to keep reverse christmas tree order.

Reviewed-by Frank Li <[email protected]>

Quoted 19 lines

drivers/i2c/busses/i2c-imx.c +2 -3

 	while (1) {
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 
 		/* check for arbitration lost */
-		if (temp & I2SR_IAL) {
+		if (multi_master && (temp & I2SR_IAL)) {
 			i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
 			return -EAGAIN;
 		}
 
-		if (for_busy && (temp & I2SR_IBB)) {
+		if (for_busy && (!multi_master || (temp & I2SR_IBB))) {
 			i2c_imx->stopped = 0;
 			break;
 		}
--

2.45.2

Andi Shyti

maintainer

2024-12-18 18:00 UTC #2 lore raw

Hi,

...

Quoted 8 lines

drivers/i2c/busses/i2c-imx.c +0 -0

index f751d231ded8..cbf66a69e20b 100644
@@ -534,20 +534,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;

Quoted 8 lines

drivers/i2c/busses/i2c-imx.c static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a · line 537
Quoted 4 lines
drivers/i2c/busses/i2c-imx.c +1 -3
-
-	if (!i2c_imx->multi_master)
-		return 0;
+	bool multi_master = i2c_imx->multi_master;
Move "bool multi_master = i2c_imx->multi_master;" before "unsigned long orig_jiffies = jiffies;" to keep reverse christmas tree order.

I will take care of it.

Thanks, Stefan.

Andi

Francesco Dolcini

Reviewed-by on v1 1/1

2024-12-16 18:42 UTC #3 lore raw

In reply to Stefan Eichenberger

Quoted 15 lines · Stefan Eichenberger

From: Stefan Eichenberger <[email protected]>
A regression was introduced with the implementation of single-master mode, preventing proper stop conditions from being generated. Devices that require a valid stop condition, such as EEPROMs, fail to function correctly as a result.
The issue only affects devices with the single-master property enabled.
This commit resolves the issue by re-enabling I2C bus busy bit (IBB) polling for single-master mode when generating a stop condition. The fix further ensures that the i2c_imx->stopped flag is cleared at the start of each transfer, allowing the stop condition to be correctly generated in i2c_imx_stop().

Quoted 7 lines

According to the reference manual (IMX8MMRM, Rev. 2, 09/2019, page 5270), polling the IBB bit to determine if the bus is free is only necessary in multi-master mode. Consequently, the IBB bit is not polled for the start condition in single-master mode.
Fixes 6692694aca86 ("i2c: imx: do not poll for bus busy in single master mode")
Signed-off-by Stefan Eichenberger <[email protected]>

Reviewed-by Francesco Dolcini <[email protected]>

Andi Shyti

maintainer

2024-12-18 18:16 UTC #4 lore raw

In reply to Stefan Eichenberger

Hi Stefan,

Quoted 31 lines · Stefan Eichenberger

From: Stefan Eichenberger <[email protected]>
A regression was introduced with the implementation of single-master mode, preventing proper stop conditions from being generated. Devices that require a valid stop condition, such as EEPROMs, fail to function correctly as a result.
The issue only affects devices with the single-master property enabled.
This commit resolves the issue by re-enabling I2C bus busy bit (IBB) polling for single-master mode when generating a stop condition. The fix further ensures that the i2c_imx->stopped flag is cleared at the start of each transfer, allowing the stop condition to be correctly generated in i2c_imx_stop().
According to the reference manual (IMX8MMRM, Rev. 2, 09/2019, page 5270), polling the IBB bit to determine if the bus is free is only necessary in multi-master mode. Consequently, the IBB bit is not polled for the start condition in single-master mode.
Fixes 6692694aca86 ("i2c: imx: do not poll for bus busy in single master mode")
Signed-off-by Stefan Eichenberger <[email protected]>
---
 drivers/i2c/busses/i2c-imx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-imx.c +0 -0
index f751d231ded8..cbf66a69e20b 100644

drivers/i2c/busses/i2c-imx.c static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a · line 537

Quoted 7 lines

drivers/i2c/busses/i2c-imx.c +1 -3

@@ -534,20 +534,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
-
-	if (!i2c_imx->multi_master)
-		return 0;
+	bool multi_master = i2c_imx->multi_master;

with this small adjustment, I have applied your patch to i2c/i2c-host-fixes.

The idea behind the change is that variables are sorted by length, forming a kind of reversed Christmas tree shape. It's not a strict rule—some communities enforce it, others don't, and some follow entirely different conventions.

Since it feels somewhat arbitrary to me, I don't enforce it strictly. However, I personally try to adhere to the reversed Christmas tree rule whenever possible.

Andi

Quoted 19 lines

drivers/i2c/busses/i2c-imx.c +2 -2

 	while (1) {
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
 
 		/* check for arbitration lost */
-		if (temp & I2SR_IAL) {
+		if (multi_master && (temp & I2SR_IAL)) {
 			i2c_imx_clear_irq(i2c_imx, I2SR_IAL);
 			return -EAGAIN;
 		}
 
-		if (for_busy && (temp & I2SR_IBB)) {
+		if (for_busy && (!multi_master || (temp & I2SR_IBB))) {
 			i2c_imx->stopped = 0;
 			break;
 		}

Signature · 1 line

2.45.2

Stefan Eichenberger

author

2024-12-19 09:19 UTC #5 lore raw

Hi Andi,

Quoted 45 lines · Andi Shyti

Hi Stefan,
Quoted 31 lines · Stefan Eichenberger
From: Stefan Eichenberger <[email protected]>
A regression was introduced with the implementation of single-master mode, preventing proper stop conditions from being generated. Devices that require a valid stop condition, such as EEPROMs, fail to function correctly as a result.
The issue only affects devices with the single-master property enabled.
This commit resolves the issue by re-enabling I2C bus busy bit (IBB) polling for single-master mode when generating a stop condition. The fix further ensures that the i2c_imx->stopped flag is cleared at the start of each transfer, allowing the stop condition to be correctly generated in i2c_imx_stop().
According to the reference manual (IMX8MMRM, Rev. 2, 09/2019, page 5270), polling the IBB bit to determine if the bus is free is only necessary in multi-master mode. Consequently, the IBB bit is not polled for the start condition in single-master mode.
Fixes 6692694aca86 ("i2c: imx: do not poll for bus busy in single master mode")
Signed-off-by Stefan Eichenberger <[email protected]>
---
 drivers/i2c/busses/i2c-imx.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/i2c/busses/i2c-imx.c +0 -0
index f751d231ded8..cbf66a69e20b 100644
drivers/i2c/busses/i2c-imx.c static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a · line 537
Quoted 7 lines
drivers/i2c/busses/i2c-imx.c +1 -3
@@ -534,20 +534,18 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool a
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
-
-	if (!i2c_imx->multi_master)
-		return 0;
+	bool multi_master = i2c_imx->multi_master;
with this small adjustment, I have applied your patch to i2c/i2c-host-fixes.

Quoted 8 lines

The idea behind the change is that variables are sorted by length, forming a kind of reversed Christmas tree shape. It's not a strict rule—some communities enforce it, others don't, and some follow entirely different conventions.
Since it feels somewhat arbitrary to me, I don't enforce it strictly. However, I personally try to adhere to the reversed Christmas tree rule whenever possible.

Thanks a lot for doing the change and for the explanation. I will keep it in mind for future patches.

Best regards,
Stefan