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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
