ColdFire I2C implementing I2C idle [PATCH]

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

If I2SR[IBB] is set when the I2C bus module is enabled, execute the following code sequence before proceeding with normal initialization code. This issues a STOP command to the slave device, placing it in idle state as if it were just power-cycled on.

   I2CR = 0x0
   I2CR = 0xA
   dummy read of I2DR
   I2SR = 0x0
   I2CR = 0x0

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/i2c/fsl_i2c.c +10 -0

index 2241990..bce750c 100644
@@ -251,12 +251,25 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
+static __inline__ int i2c_set_idle(void)
+{
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN | I2C_CR_MSTA, &i2c_dev[i2c_bus_num]->cr);
+    readb(&i2c_dev[i2c_bus_num]->dr);
+    writeb(0, &i2c_dev[i2c_bus_num]->sr);
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+}
+
 static int
 i2c_wait4bus(void)

Hello Michael,

Thanks for posting your patches in plain text.

Quoted 41 lines · Michael Durrant

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

If I2SR[IBB] is set when the I2C bus module is enabled, execute the following code sequence before proceeding with normal initialization code. This issues a STOP command to the slave device, placing it in idle state as if it were just power-cycled on.

   I2CR = 0x0
   I2CR = 0xA
   dummy read of I2DR
   I2SR = 0x0
   I2CR = 0x0

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/i2c/fsl_i2c.c +10 -0

index 2241990..bce750c 100644
@@ -251,12 +251,25 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
+static __inline__ int i2c_set_idle(void)
+{
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN | I2C_CR_MSTA, &i2c_dev[i2c_bus_num]->cr);
+    readb(&i2c_dev[i2c_bus_num]->dr);
+    writeb(0, &i2c_dev[i2c_bus_num]->sr);
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+}
+
 static int
 i2c_wait4bus(void)

drivers/i2c/fsl_i2c.c i2c_init(int speed, int slaveadd) · line 275

Quoted 10 lines

drivers/i2c/fsl_i2c.c +3 -0

 {
     unsigned long long timeval = get_ticks();
     const unsigned long long timeout = usec2ticks(CONFIG_I2C_MBB_TIMEOUT);
 
+    if (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB)
+        i2c_set_idle();
+
     while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) {
         if ((get_ticks() - timeval) > timeout)
             return -1;

Hmm.. I can;t find this for example in the MPC8360ERM.pdf, which uses also this driver ... So I vote for activating this only, if this driver is used for __M68K__ ...

Also, you write, that this sequence is necessary before normal initialization code, but i2c_wait4bus() is called from i2c_read() and i2c_write(), so the I2C bus is long ago initialized ... or what do you mean with "normal initialization code" ?

Also, whats with multimaster systems? Your code maybe cuts a running data transmission. The MPC8360ERM.pdf manual says "check the MBB bit, if the bus is free, before switching to master mode", thats what actual code did ... so, I want this only activated, if __M68K__ is defined ...

Quoted 65 lines

Hello Michael,

Thanks for posting your patches in plain text.

Quoted 41 lines · Michael Durrant

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

If I2SR[IBB] is set when the I2C bus module is enabled, execute the following code sequence before proceeding with normal initialization code. This issues a STOP command to the slave device, placing it in idle state as if it were just power-cycled on.

   I2CR = 0x0
   I2CR = 0xA
   dummy read of I2DR
   I2SR = 0x0
   I2CR = 0x0

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/i2c/fsl_i2c.c +10 -0

index 2241990..bce750c 100644
@@ -251,12 +251,25 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
+static __inline__ int i2c_set_idle(void)
+{
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN | I2C_CR_MSTA, &i2c_dev[i2c_bus_num]->cr);
+    readb(&i2c_dev[i2c_bus_num]->dr);
+    writeb(0, &i2c_dev[i2c_bus_num]->sr);
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+}
+
 static int
 i2c_wait4bus(void)

drivers/i2c/fsl_i2c.c i2c_init(int speed, int slaveadd) · line 275

Quoted 10 lines

drivers/i2c/fsl_i2c.c +3 -0

 {
     unsigned long long timeval = get_ticks();
     const unsigned long long timeout = usec2ticks(CONFIG_I2C_MBB_TIMEOUT);
 
+    if (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB)
+        i2c_set_idle();
+
     while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) {
         if ((get_ticks() - timeval) > timeout)
             return -1;

Hmm.. I can;t find this for example in the MPC8360ERM.pdf, which uses also this driver ... So I vote for activating this only, if this driver is used for __M68K__ ...

Also, you write, that this sequence is necessary before normal initialization code, but i2c_wait4bus() is called from i2c_read() and i2c_write(), so the I2C bus is long ago initialized ... or what do you mean with "normal initialization code" ?

Quoted 5 lines

Also, whats with multimaster systems? Your code maybe cuts a running data transmission. The MPC8360ERM.pdf manual says "check the MBB bit, if the bus is free, before switching to master mode", thats what actual code did ... so, I want this only activated, if __M68K__ is defined ...

All true. This cannot go in as is. What you need is a I2C reset sequence as the above isn't enough in the general case. Michael, have a look at the kernel driver, it has some fixup code you could borrow.

Hello Joakim,

Quoted 71 lines · Joakim Tjernlund

Quoted 65 lines

Hello Michael,

Thanks for posting your patches in plain text.

Quoted 41 lines · Michael Durrant

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

If I2SR[IBB] is set when the I2C bus module is enabled, execute the following code sequence before proceeding with normal initialization code. This issues a STOP command to the slave device, placing it in idle state as if it were just power-cycled on.

   I2CR = 0x0
   I2CR = 0xA
   dummy read of I2DR
   I2SR = 0x0
   I2CR = 0x0

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/i2c/fsl_i2c.c +10 -0

index 2241990..bce750c 100644
@@ -251,12 +251,25 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
+static __inline__ int i2c_set_idle(void)
+{
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN | I2C_CR_MSTA, &i2c_dev[i2c_bus_num]->cr);
+    readb(&i2c_dev[i2c_bus_num]->dr);
+    writeb(0, &i2c_dev[i2c_bus_num]->sr);
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+}
+
 static int
 i2c_wait4bus(void)

drivers/i2c/fsl_i2c.c i2c_init(int speed, int slaveadd) · line 275

Quoted 10 lines

drivers/i2c/fsl_i2c.c +3 -0

 {
     unsigned long long timeval = get_ticks();
     const unsigned long long timeout = usec2ticks(CONFIG_I2C_MBB_TIMEOUT);
 
+    if (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB)
+        i2c_set_idle();
+
     while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) {
         if ((get_ticks() - timeval) > timeout)
             return -1;

Hmm.. I can;t find this for example in the MPC8360ERM.pdf, which uses also this driver ... So I vote for activating this only, if this driver is used for __M68K__ ...

Also, you write, that this sequence is necessary before normal initialization code, but i2c_wait4bus() is called from i2c_read() and i2c_write(), so the I2C bus is long ago initialized ... or what do you mean with "normal initialization code" ?

Quoted 5 lines

Also, whats with multimaster systems? Your code maybe cuts a running data transmission. The MPC8360ERM.pdf manual says "check the MBB bit, if the bus is free, before switching to master mode", thats what actual code did ... so, I want this only activated, if __M68K__ is defined ...

Quoted 3 lines

All true. This cannot go in as is. What you need is a I2C reset sequence as the above isn't enough in the general case. Michael, have a look at the kernel driver, it has some fixup code you could borrow.

Michael: Maybe you take a look in u-boot:board/keymile/common/common.c i2c_init_board(), there is a I2C reset sequence for 83xx based boards from keymile, which use this i2c driver.

Maybe its time to move this code in the i2c driver?

Quoted 76 lines · Heiko Schocher

Hello Joakim,

Quoted 73 lines · Joakim Tjernlund

Quoted 65 lines

Hello Michael,

Thanks for posting your patches in plain text.

Quoted 41 lines · Michael Durrant

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

If I2SR[IBB] is set when the I2C bus module is enabled, execute the following code sequence before proceeding with normal initialization code. This issues a STOP command to the slave device, placing it in idle state as if it were just power-cycled on.

   I2CR = 0x0
   I2CR = 0xA
   dummy read of I2DR
   I2SR = 0x0
   I2CR = 0x0

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/i2c/fsl_i2c.c +10 -0

index 2241990..bce750c 100644
@@ -251,12 +251,25 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
+static __inline__ int i2c_set_idle(void)
+{
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN | I2C_CR_MSTA, &i2c_dev[i2c_bus_num]->cr);
+    readb(&i2c_dev[i2c_bus_num]->dr);
+    writeb(0, &i2c_dev[i2c_bus_num]->sr);
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+}
+
 static int
 i2c_wait4bus(void)

drivers/i2c/fsl_i2c.c i2c_init(int speed, int slaveadd) · line 275

Quoted 10 lines

drivers/i2c/fsl_i2c.c +3 -0

 {
     unsigned long long timeval = get_ticks();
     const unsigned long long timeout = usec2ticks(CONFIG_I2C_MBB_TIMEOUT);
 
+    if (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB)
+        i2c_set_idle();
+
     while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) {
         if ((get_ticks() - timeval) > timeout)
             return -1;

Hmm.. I can;t find this for example in the MPC8360ERM.pdf, which uses also this driver ... So I vote for activating this only, if this driver is used for __M68K__ ...

Also, you write, that this sequence is necessary before normal initialization code, but i2c_wait4bus() is called from i2c_read() and i2c_write(), so the I2C bus is long ago initialized ... or what do you mean with "normal initialization code" ?

Quoted 5 lines

Also, whats with multimaster systems? Your code maybe cuts a running data transmission. The MPC8360ERM.pdf manual says "check the MBB bit, if the bus is free, before switching to master mode", thats what actual code did ... so, I want this only activated, if __M68K__ is defined ...

All true. This cannot go in as is. What you need is a I2C reset sequence as the above isn't enough in the general case. Michael, have a look at the kernel driver, it has some fixup code you could borrow.

Quoted 5 lines

Michael: Maybe you take a look in u-boot:board/keymile/common/common.c i2c_init_board(), there is a I2C reset sequence for 83xx based boards from keymile, which use this i2c driver.

Maybe its time to move this code in the i2c driver?

this code looks good except I do not see how the special case of multi-master systems you mentioned is handled.

I think for multi-master a timeout would have to be implemented to wait for a possible other master transfer to finish before initiating the abort sequence, or is this too much time spent during init?

Hello Wolfgang,

Quoted 89 lines · Wolfgang Wegner

Quoted 87 lines · Heiko Schocher

Hello Joakim,

Quoted 73 lines · Joakim Tjernlund

Quoted 67 lines

Hello Michael,

Thanks for posting your patches in plain text.

Quoted 42 lines · Michael Durrant

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

If I2SR[IBB] is set when the I2C bus module is enabled, execute the following code sequence before proceeding with normal initialization code. This issues a STOP command to the slave device, placing it in idle state as if it were just power-cycled on.

   I2CR = 0x0
   I2CR = 0xA
   dummy read of I2DR
   I2SR = 0x0
   I2CR = 0x0

Signed-off-by David Wu <[email protected]>

Signed-off-by Michael Durrant <[email protected]>

---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

drivers/i2c/fsl_i2c.c +10 -0

index 2241990..bce750c 100644
@@ -251,12 +251,25 @@ i2c_init(int speed, int slaveadd)
 #endif
 }
 
+static __inline__ int i2c_set_idle(void)
+{
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN | I2C_CR_MSTA, &i2c_dev[i2c_bus_num]->cr);
+    readb(&i2c_dev[i2c_bus_num]->dr);
+    writeb(0, &i2c_dev[i2c_bus_num]->sr);
+    writeb(0, &i2c_dev[i2c_bus_num]->cr);
+    writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
+}
+
 static int
 i2c_wait4bus(void)

drivers/i2c/fsl_i2c.c i2c_init(int speed, int slaveadd) · line 275

Quoted 10 lines

drivers/i2c/fsl_i2c.c +3 -0

 {
     unsigned long long timeval = get_ticks();
     const unsigned long long timeout = usec2ticks(CONFIG_I2C_MBB_TIMEOUT);
 
+    if (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB)
+        i2c_set_idle();
+
     while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) {
         if ((get_ticks() - timeval) > timeout)
             return -1;

Hmm.. I can;t find this for example in the MPC8360ERM.pdf, which uses also this driver ... So I vote for activating this only, if this driver is used for __M68K__ ...

Also, you write, that this sequence is necessary before normal initialization code, but i2c_wait4bus() is called from i2c_read() and i2c_write(), so the I2C bus is long ago initialized ... or what do you mean with "normal initialization code" ?

Quoted 6 lines

Also, whats with multimaster systems? Your code maybe cuts a running data transmission. The MPC8360ERM.pdf manual says "check the MBB bit, if the bus is free, before switching to master mode", thats what actual code did ... so, I want this only activated, if __M68K__ is defined ...

Quoted 4 lines

All true. This cannot go in as is. What you need is a I2C reset sequence as the above isn't enough in the general case. Michael, have a look at the kernel driver, it has some fixup code you could borrow.

Michael: Maybe you take a look in u-boot:board/keymile/common/common.c i2c_init_board(), there is a I2C reset sequence for 83xx based boards from keymile, which use this i2c driver.

Maybe its time to move this code in the i2c driver?

Quoted 3 lines

this code looks good except I do not see how the special case of multi-master systems you mentioned is handled.

I have no experience with multimaster systems, and I think, this special case is not yet factored in code.

Quoted 4 lines

I think for multi-master a timeout would have to be implemented to wait for a possible other master transfer to finish before initiating the abort sequence, or is this too much time spent during init?

Or, it could be detected? If BB Bit is set and the SCL pin doesn;t alter, the I2C bus must be blocked ... just an idea.

bye
Heiko

Quoted 18 lines · Heiko Schocher

Hello Wolfgang,

Quoted 14 lines · Wolfgang Wegner

Quoted 12 lines · Heiko Schocher

Hello Joakim,

Quoted 8 lines · Joakim Tjernlund

Quoted 8 lines

Hello Michael,

Thanks for posting your patches in plain text.

Quoted 2 lines · Michael Durrant

drivers_i2c_fsl_i2c.patch
- need to set I2C to be idle according to the MCF5282 user's manual

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 1 line

Hmm.. I can;t find this for example in the MPC8360ERM.pdf, which uses also this driver ... So I vote for activating this only, if this driver is used for __M68K__ ...

Also, you write, that this sequence is necessary before normal initialization code, but i2c_wait4bus() is called from i2c_read() and i2c_write(), so the I2C bus is long ago initialized ... or what do you mean with "normal initialization code" ?

Quoted 6 lines

Also, whats with multimaster systems? Your code maybe cuts a running data transmission. The MPC8360ERM.pdf manual says "check the MBB bit, if the bus is free, before switching to master mode", thats what actual code did ... so, I want this only activated, if __M68K__ is defined ...

All true. This cannot go in as is. What you need is a I2C reset sequence as the above isn't enough in the general case. Michael, have a look at the kernel driver, it has some fixup code you could borrow.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Michael: Maybe you take a look in u-boot:board/keymile/common/common.c i2c_init_board(), there is a I2C reset sequence for 83xx based boards from keymile, which use this i2c driver.

Maybe its time to move this code in the i2c driver?

Will do ... David Wu emailed me a few comments as well that I include at the end.
For now I agree that we should protect non ColdFire V2/V3 users with the __M68K__ definition

Quoted 11 lines

Quoted 3 lines

this code looks good except I do not see how the special case of multi-master systems you mentioned is handled.

I have no experience with multimaster systems, and I think, this special case is not yet factored in code.

Quoted 4 lines

I think for multi-master a timeout would have to be implemented to wait for a possible other master transfer to finish before initiating the abort sequence, or is this too much time spent during init?

Quoted 6 lines

Or, it could be detected? If BB Bit is set and the SCL pin doesn;t alter, the I2C bus must be blocked ... just an idea.

bye
Heiko

David Wu's thoughts follow:

1. agree to add a protection using #if defined(__M68K__) at some point this i2c_set_idle() has to be called when this I2C
master
is enabled. In one master environment it can be in
- i2c_set_bus_speed() where the I2C is enabled for that bus, or
- i2c_init() or,
- board specific routine
- please suggest the right location.

2. since only the master can do a read and write call, when the bus is busy there is no need to wait until it is free which may never be free if we don't force a STOP.

3. In a multi-master environment, there should be an additional condition as to when one master can read/write, still i2c_wait4bus() may timeout.