[U-Boot] ColdFire I2C implementing I2C idle [PATCH] -- resent in git format

[Wolfgang Wegner]

Hello Heiko,

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?

this code looks good except I do not see how the special case of
multi-master systems you mentioned is handled.
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 am I missing something obvious?

(Sorry, no PPC experience here, we just had the exact same problem
with the old uclinux I2C driver on coldfire, where I currently use a
quickly hacked own driver in favor of the completely broken original
driver which did not even return an error on NACK...)

Regards,
Wolfgang