[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