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 <davi...@arcturusnetworks.com> >>>>>> Signed-off-by: Michael Durrant <mdurr...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot