[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 at arcturusnetworks.com>
>>>>>> Signed-off-by: Michael Durrant <mdurrant at 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