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