[U-Boot] ColdFire I2C implementing I2C idle [PATCH]

[U-Boot] ColdFire I2C implementing I2C idle [PATCH]

[Michael Durrant]

Signed-off-by: David Wu <davidwu@arcturusnetworks.com>
Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>


Patch created against u-boot-2009.11 release

drivers_i2c_fsl_i2c.patch
   - need to set I2C to be idle acoording 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

--
Michael Durrant
mdurrant at arcturusnetworks.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: drivers_i2c_fsl_i2c.patch
Type: application/octet-stream
Size: 1005 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100119/6b191970/attachment.obj 

[U-Boot] ColdFire I2C implementing I2C idle [PATCH]

[Heiko Schocher]

Hello Michael,

Michael Durrant wrote:
> Signed-off-by: David Wu <davidwu@arcturusnetworks.com>
> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>

Can you please use git-format-patch for creating this patch,
and post it here again not attached, just as plain text?

(Same for your other 3 patches)

Thanks.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[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 <davidwu@arcturusnetworks.com>
Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
---
 drivers/i2c/fsl_i2c.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 2241990..bce750c 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -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)
 {
     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;
-- 1.4.3.4

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Heiko Schocher]

Hello Michael,

Thanks for posting your patches in plain text.

Michael Durrant wrote:
> 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 <davidwu@arcturusnetworks.com>
> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
> ---
>  drivers/i2c/fsl_i2c.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index 2241990..bce750c 100644
> --- a/drivers/i2c/fsl_i2c.c
> +++ b/drivers/i2c/fsl_i2c.c
> @@ -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)
>  {
>      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 ...

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Joakim Tjernlund]

>
> Hello Michael,
>
> Thanks for posting your patches in plain text.
>
> Michael Durrant wrote:
> > 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 <davidwu@arcturusnetworks.com>
> > Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
> > ---
> >  drivers/i2c/fsl_i2c.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> > index 2241990..bce750c 100644
> > --- a/drivers/i2c/fsl_i2c.c
> > +++ b/drivers/i2c/fsl_i2c.c
> > @@ -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)
> >  {
> >      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 ...

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.

   Jocke

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Heiko Schocher]

Hello Joakim,

Joakim Tjernlund wrote:
>> Hello Michael,
>>
>> Thanks for posting your patches in plain text.
>>
>> Michael Durrant wrote:
>>> 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 <davidwu@arcturusnetworks.com>
>>> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
>>> ---
>>>  drivers/i2c/fsl_i2c.c |   13 +++++++++++++
>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
>>> index 2241990..bce750c 100644
>>> --- a/drivers/i2c/fsl_i2c.c
>>> +++ b/drivers/i2c/fsl_i2c.c
>>> @@ -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)
>>>  {
>>>      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 ...
> 
> 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?

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Wolfgang Wegner]

Hello Heiko,

On Thu, Jan 21, 2010 at 09:04:29AM +0100, Heiko Schocher wrote:
> Hello Joakim,
> 
> Joakim Tjernlund wrote:
> >> Hello Michael,
> >>
> >> Thanks for posting your patches in plain text.
> >>
> >> Michael Durrant wrote:
> >>> 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 <davidwu@arcturusnetworks.com>
> >>> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
> >>> ---
> >>>  drivers/i2c/fsl_i2c.c |   13 +++++++++++++
> >>>  1 files changed, 13 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> >>> index 2241990..bce750c 100644
> >>> --- a/drivers/i2c/fsl_i2c.c
> >>> +++ b/drivers/i2c/fsl_i2c.c
> >>> @@ -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)
> >>>  {
> >>>      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 ...
> > 
> > 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?

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?

Or am I missing something obvious?

(Sorry, no PPC experience here, we just had the exact same problem
with the old uclinux I2C driver on coldfire, where I currently use a
quickly hacked own driver in favor of the completely broken original
driver which did not even return an error on NACK...)

Regards,
Wolfgang

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Wegner wrote:
> On Thu, Jan 21, 2010 at 09:04:29AM +0100, Heiko Schocher wrote:
>> Hello Joakim,
>>
>> Joakim Tjernlund wrote:
>>>> Hello Michael,
>>>>
>>>> Thanks for posting your patches in plain text.
>>>>
>>>> Michael Durrant wrote:
>>>>> 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 <davidwu@arcturusnetworks.com>
>>>>> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
>>>>> ---
>>>>>  drivers/i2c/fsl_i2c.c |   13 +++++++++++++
>>>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
>>>>> index 2241990..bce750c 100644
>>>>> --- a/drivers/i2c/fsl_i2c.c
>>>>> +++ b/drivers/i2c/fsl_i2c.c
>>>>> @@ -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)
>>>>>  {
>>>>>      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 ...
>>> 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?
> 
> 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.

> 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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Michael Durrant]

Heiko Schocher wrote:
> Hello Wolfgang,
>
> Wolfgang Wegner wrote:
>   
>> On Thu, Jan 21, 2010 at 09:04:29AM +0100, Heiko Schocher wrote:
>>     
>>> Hello Joakim,
>>>
>>> Joakim Tjernlund wrote:
>>>       
>>>>> Hello Michael,
>>>>>
>>>>> Thanks for posting your patches in plain text.
>>>>>
>>>>> Michael Durrant wrote:
>>>>>           
>>>>>> 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 <davidwu@arcturusnetworks.com>
>>>>>> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
>>>>>> ---
>>>>>>  drivers/i2c/fsl_i2c.c |   13 +++++++++++++
>>>>>>  1 files changed, 13 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
>>>>>> index 2241990..bce750c 100644
>>>>>> --- a/drivers/i2c/fsl_i2c.c
>>>>>> +++ b/drivers/i2c/fsl_i2c.c
>>>>>> @@ -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)
>>>>>>  {
>>>>>>      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 ...
>>>>>           
>>>> 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?
>>>       

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

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

  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.

  --
  Regards,
  David Wu


Thanks for the great feedback. 

Regards,
Michael Durrant

[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Heiko Schocher]

Hello Michael,

Michael Durrant wrote:
> Heiko Schocher wrote:
>> Hello Wolfgang,
>>
>> Wolfgang Wegner wrote:
>>   
>>> On Thu, Jan 21, 2010 at 09:04:29AM +0100, Heiko Schocher wrote:
>>>     
>>>> Hello Joakim,
>>>>
>>>> Joakim Tjernlund wrote:
>>>>       
>>>>>> Hello Michael,
>>>>>>
>>>>>> Thanks for posting your patches in plain text.
>>>>>>
>>>>>> Michael Durrant wrote:
>>>>>>           
>>>>>>> drivers_i2c_fsl_i2c.patch
>>>>>>>    - need to set I2C to be idle according to the MCF5282 user's manual
[...]
>>>>>>>             
>>>>>> 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 ...
>>>>>>           
>>>>> 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?
>>>>       
> 
> 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
> 
>>> 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.
>>
>>   
>>> 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
>>   
> 
>   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.

You can use the CONFIG_SYS_I2C_INIT_BOARD define, and then define
i2c_init_board() in board specific code. i2c_init_board() is called
from i2c_init(), so there is no __M68K__ specific code necessary
in this driver.

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

Ok.

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

Ok.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany