[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Thomas Chou]

We should not set SDA after TRISTATE, as it results in contention.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/i2c/soft_i2c.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
index 847db76..344b7f8 100644
--- a/drivers/i2c/soft_i2c.c
+++ b/drivers/i2c/soft_i2c.c
@@ -305,8 +305,8 @@ static uchar read_byte(int ack)
 	/*
 	 * Read 8 bits, MSB first.
 	 */
-	I2C_TRISTATE;
 	I2C_SDA(1);
+	I2C_TRISTATE;
 	data = 0;
 	for(j = 0; j < 8; j++) {
 		I2C_SCL(0);
-- 
1.7.1

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Andrew Dyer]

On Tue, Jul 6, 2010 at 1:14 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
> We should not set SDA after TRISTATE, as it results in contention.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
> ?drivers/i2c/soft_i2c.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
> index 847db76..344b7f8 100644
> --- a/drivers/i2c/soft_i2c.c
> +++ b/drivers/i2c/soft_i2c.c
> @@ -305,8 +305,8 @@ static uchar read_byte(int ack)
> ? ? ? ?/*
> ? ? ? ? * Read 8 bits, MSB first.
> ? ? ? ? */
> - ? ? ? I2C_TRISTATE;
> ? ? ? ?I2C_SDA(1);
> + ? ? ? I2C_TRISTATE;
> ? ? ? ?data = 0;
> ? ? ? ?for(j = 0; j < 8; j++) {
> ? ? ? ? ? ? ? ?I2C_SCL(0);
> --

NAK.

I2C_TRISTATE is supposed to be persistent until I2C_ACTIVE is called,
so in the original code it should still be in effect when I2C_SDA(1)
is executed and there should be no contention.  This patch causes the
code to actively drive SDA high at the same time the addressed device
might be driving it low, causing contention until the I2C_TRISTATE
takes effect.

In some sense the code is misleadingly written, as it is not allowed
in the spec to actively drive a '1' on the bus, a chip is only
supposed to drive 'z' or '0', and the platform is supposed to provide
the pullup current.  This comes more into play if the i2c bus supports
clock stretching or arbitration among multiple masters.

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Reinhard Meyer]

Andrew Dyer wrote:
> NAK.
>
> I2C_TRISTATE is supposed to be persistent until I2C_ACTIVE is called,
> so in the original code it should still be in effect when I2C_SDA(1)
> is executed and there should be no contention.  This patch causes the
> code to actively drive SDA high at the same time the addressed device
> might be driving it low, causing contention until the I2C_TRISTATE
> takes effect.
>   
Right.
Whenever possible by the hardware, I make I2C_SDA/SCL(1) do a tri-state and
I2C_TRISTATE and I2C_ACTIVE are empty.
> In some sense the code is misleadingly written, as it is not allowed
> in the spec to actively drive a '1' on the bus, a chip is only
> supposed to drive 'z' or '0', and the platform is supposed to provide
> the pullup current.  This comes more into play if the i2c bus supports
> clock stretching or arbitration among multiple masters.
>   
I might point out that the soft I2C as it is does not support clock 
stretching. Unless you
add the wait for SCL high into I2C_SCL(1) yourself.

Reinhard

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Mike Frysinger]

On Wednesday, July 07, 2010 00:45:42 Andrew Dyer wrote:
> On Tue, Jul 6, 2010 at 1:14 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
> > We should not set SDA after TRISTATE, as it results in contention.
> > 
> > Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> > ---
> >  drivers/i2c/soft_i2c.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
> > index 847db76..344b7f8 100644
> > --- a/drivers/i2c/soft_i2c.c
> > +++ b/drivers/i2c/soft_i2c.c
> > @@ -305,8 +305,8 @@ static uchar read_byte(int ack)
> >        /*
> >         * Read 8 bits, MSB first.
> >         */
> > -       I2C_TRISTATE;
> >        I2C_SDA(1);
> > +       I2C_TRISTATE;
> >        data = 0;
> >        for(j = 0; j < 8; j++) {
> >                I2C_SCL(0);
> > --
> 
> I2C_TRISTATE is supposed to be persistent until I2C_ACTIVE is called,
> so in the original code it should still be in effect when I2C_SDA(1)
> is executed and there should be no contention.  This patch causes the
> code to actively drive SDA high at the same time the addressed device
> might be driving it low, causing contention until the I2C_TRISTATE
> takes effect.
> 
> In some sense the code is misleadingly written, as it is not allowed
> in the spec to actively drive a '1' on the bus, a chip is only
> supposed to drive 'z' or '0', and the platform is supposed to provide
> the pullup current.  This comes more into play if the i2c bus supports
> clock stretching or arbitration among multiple masters.

how do you propose we get i2c gpio working ?  it works fine under Linux.  we 
cannot tristate a pin (set the gpio to an input) and then turn around and 
attempt to drive it (set the gpio to an output with a specific value).  that 
is what the code currently does.

our end goal is simple: have i2c gpio bitbanging work under u-boot like under 
linux.  we dont really care about the exact way we get there.  this patch was 
just one idea.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100711/63b1adc8/attachment.pgp 

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Andrew Dyer]

On Sun, Jul 11, 2010 at 5:55 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday, July 07, 2010 00:45:42 Andrew Dyer wrote:
>> On Tue, Jul 6, 2010 at 1:14 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
>> > We should not set SDA after TRISTATE, as it results in contention.
>> >
>> > Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>> > ---
>> > ?drivers/i2c/soft_i2c.c | ? ?2 +-
>> > ?1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
>> > index 847db76..344b7f8 100644
>> > --- a/drivers/i2c/soft_i2c.c
>> > +++ b/drivers/i2c/soft_i2c.c
>> > @@ -305,8 +305,8 @@ static uchar read_byte(int ack)
>> > ? ? ? ?/*
>> > ? ? ? ? * Read 8 bits, MSB first.
>> > ? ? ? ? */
>> > - ? ? ? I2C_TRISTATE;
>> > ? ? ? ?I2C_SDA(1);
>> > + ? ? ? I2C_TRISTATE;
>> > ? ? ? ?data = 0;
>> > ? ? ? ?for(j = 0; j < 8; j++) {
>> > ? ? ? ? ? ? ? ?I2C_SCL(0);
>> > --
>>
>> I2C_TRISTATE is supposed to be persistent until I2C_ACTIVE is called,
>> so in the original code it should still be in effect when I2C_SDA(1)
>> is executed and there should be no contention. ?This patch causes the
>> code to actively drive SDA high at the same time the addressed device
>> might be driving it low, causing contention until the I2C_TRISTATE
>> takes effect.
>>
>> In some sense the code is misleadingly written, as it is not allowed
>> in the spec to actively drive a '1' on the bus, a chip is only
>> supposed to drive 'z' or '0', and the platform is supposed to provide
>> the pullup current. ?This comes more into play if the i2c bus supports
>> clock stretching or arbitration among multiple masters.
>
> how do you propose we get i2c gpio working ? ?it works fine under Linux. ?we
> cannot tristate a pin (set the gpio to an input) and then turn around and
> attempt to drive it (set the gpio to an output with a specific value). ?that
> is what the code currently does.
>
> our end goal is simple: have i2c gpio bitbanging work under u-boot like under
> linux. ?we dont really care about the exact way we get there. ?this patch was
> just one idea.
> -mike

If you make sure that I2C_SDA(1)/I2C_SCL(1) makes the pins tristate
and I2C_SDA(0)/I2C_SCL(0) means driven actively low, you can define
I2C_ACTIVE and I2C_TRISTATE as nothing, and it should all work out
right.  The trick is in making sure that the order of operations on
the IO buffer data/enables doesn't cause the output to glitch,
especially on the clock line.

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Thomas Chou]

Reinhard Meyer wrote:
> Whenever possible by the hardware, I make I2C_SDA/SCL(1) do a tri-state and
> I2C_TRISTATE and I2C_ACTIVE are empty.

Dear Mike,

I traced the i2c-gpio.c of linux and realized that there are potential 
bus contention with the current soft_i2c.c if the ports are not 
open-drained.

Reinhard suggested a solution, which was similar to what linux driver 
does. So I would withdraw my SDA patch.

For our i2c gpio framework, I added these changes and tested on my 
boards. Please check if it works on yours.

# ifndef I2C_ACTIVE
#  define I2C_ACTIVE do {} while (0)
# endif

# ifndef I2C_TRISTATE
#  define I2C_TRISTATE do {} while (0)
# endif

# ifndef I2C_SDA
#  define I2C_SDA(bit) \
	if (bit) {						\
		gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);	\
	} else {						\
		gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
	}
# endif

I didn't tristate SCL(1) because it cannot be tristated on some nios2 
boards. As soft_i2c of u-boot didn't support clock stretching, it 
shouldn't matter.

Best regards,
Thomas

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Reinhard Meyer]

Thomas Chou wrote:
> Reinhard Meyer wrote:
>   
>> Whenever possible by the hardware, I make I2C_SDA/SCL(1) do a tri-state and
>> I2C_TRISTATE and I2C_ACTIVE are empty.
>>     
>
> Dear Mike,
>
> I traced the i2c-gpio.c of linux and realized that there are potential 
> bus contention with the current soft_i2c.c if the ports are not 
> open-drained.
>
> Reinhard suggested a solution, which was similar to what linux driver 
> does. So I would withdraw my SDA patch.
>
> For our i2c gpio framework, I added these changes and tested on my 
> boards. Please check if it works on yours.
>
> # ifndef I2C_ACTIVE
> #  define I2C_ACTIVE do {} while (0)
> # endif
>
> # ifndef I2C_TRISTATE
> #  define I2C_TRISTATE do {} while (0)
> # endif
>
> # ifndef I2C_SDA
> #  define I2C_SDA(bit) \
> 	if (bit) {						\
> 		gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);	\
> 	} else {						\
> 		gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
> 	}
> # endif
>
> I didn't tristate SCL(1) because it cannot be tristated on some nios2 
> boards. As soft_i2c of u-boot didn't support clock stretching, it 
> shouldn't matter.
>   
Its even simpler, provided the  hardware can do open collector. Here was 
my solution for AVR32AP7000:

int board_early_init_f(void)
{
    ...
#ifdef CONFIG_CMD_I2C
    /* set SCL and SDA to open drain gpio */
    portmux_select_gpio(PORTMUX_PORT_A,(1<<SDA_PIN),
        PORTMUX_DIR_OUTPUT|PORTMUX_INIT_LOW|PORTMUX_OPEN_DRAIN);
    portmux_select_gpio(PORTMUX_PORT_A,(1<<SCL_PIN),
        PORTMUX_DIR_OUTPUT|PORTMUX_INIT_LOW|PORTMUX_OPEN_DRAIN);
    /* initialize i2c. note: params ignored in SOFT I2C */
    i2c_init(0, 0);
#endif
    ...
}


/* I2C access functions */
#ifdef CONFIG_CMD_I2C

int iic_read(void)
{
    return pio_get_input_value(GPIO_PIN_PA(SDA_PIN));
}

void iic_sda(int bit)
{
    pio_set_output_value(GPIO_PIN_PA(SDA_PIN),bit);
}

void iic_scl(int bit)
{
    pio_set_output_value(GPIO_PIN_PA(SCL_PIN),bit);
}

#endif /* CONFIG_CMD_I2C */


I realize now, that in the init I should set the initial port value to 
high, but AP7000 is history for me.
I did make the access functions real functions and let the macros call them:

#define SDA_PIN                6
#define SCL_PIN                7
#define I2C_SOFT_DECLARATIONS        int iic_read(void);\
                    void iic_sda(int);\
                    void iic_scl(int);
#define I2C_ACTIVE
#define I2C_TRISTATE
#define I2C_READ            iic_read()
#define I2C_SDA(bit)            iic_sda(bit)
#define I2C_SCL(bit)            iic_scl(bit)
#define I2C_DELAY            udelay(3)

THAT, of course is a personal preference. As simple as the functions 
are, they could be inline or the macro itself ;)

Reinhard

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Thomas Chou]

Reinhard Meyer wrote:
> Its even simpler, provided the  hardware can do open collector. Here was 
> my solution for AVR32AP7000:
> 

Then we could add a config for open drain similar to that of linux driver.

# ifndef I2C_SDA
#  ifdef CONFIG_SOFT_I2C_GPIO_SDA_IS_OPEN_DRAIN
#   define I2C_SDA(bit) gpio_set_value(CONFIG_SOFT_I2C_GPIO_SDA, bit)
#  else
#   define I2C_SDA(bit) \
     if (bit) {                        \
         gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);    \
     } else {                        \
         gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
     }
#  endif
# endif

BTW, will you be interested in using common gpio framework for avr32 family?

Cheers,
Thomas

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Reinhard Meyer]

Thomas Chou schrieb:
> Reinhard Meyer wrote:
>> Its even simpler, provided the  hardware can do open collector. Here
>> was my solution for AVR32AP7000:
>>
> 
> Then we could add a config for open drain similar to that of linux driver.
> 
> # ifndef I2C_SDA
> #  ifdef CONFIG_SOFT_I2C_GPIO_SDA_IS_OPEN_DRAIN
> #   define I2C_SDA(bit) gpio_set_value(CONFIG_SOFT_I2C_GPIO_SDA, bit)
> #  else
> #   define I2C_SDA(bit) \
>     if (bit) {                        \
>         gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);    \
>     } else {                        \
>         gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
>     }
> #  endif
> # endif

Why would we need that? Since the defines reside in the board/project specific
include/configs/*.h file, it is well known if the hardware has open drain or
not.

> 
> BTW, will you be interested in using common gpio framework for avr32
> family?

No, AVR32AP7000 is dead for me. Working on AT91SAM9xxx now, which already uses gpio framework...

Reinhard

-------------- next part --------------
A non-text attachment was scrubbed...
Name: reinhard_meyer.vcf
Type: text/x-vcard
Size: 370 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100712/6db4bf2c/attachment.vcf 

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Thomas Chou]

Reinhard Meyer wrote:
> Thomas Chou schrieb:
>> Reinhard Meyer wrote:
>>> Its even simpler, provided the  hardware can do open collector. Here
>>> was my solution for AVR32AP7000:
>>>
>> Then we could add a config for open drain similar to that of linux driver.
>>
>> # ifndef I2C_SDA
>> #  ifdef CONFIG_SOFT_I2C_GPIO_SDA_IS_OPEN_DRAIN
>> #   define I2C_SDA(bit) gpio_set_value(CONFIG_SOFT_I2C_GPIO_SDA, bit)
>> #  else
>> #   define I2C_SDA(bit) \
>>     if (bit) {                        \
>>         gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);    \
>>     } else {                        \
>>         gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
>>     }
>> #  endif
>> # endif
> 
> Why would we need that? Since the defines reside in the board/project specific
> include/configs/*.h file, it is well known if the hardware has open drain or
> not.

We want to use common gpio framework and unify the soft i2c access. 
Please see the patches from Mike Frysinger,
[07/05] i2c: soft_i2c: add simple GPIO implementation

> 
>> BTW, will you be interested in using common gpio framework for avr32
>> family?
> 
> No, AVR32AP7000 is dead for me. Working on AT91SAM9xxx now, which already uses gpio framework...
> 

Yet, it is still at91 specific. I mean a common gpio framework like that 
of linux.

- Thomas

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Reinhard Meyer]

Thomas Chou schrieb:
> Reinhard Meyer wrote:
>> Thomas Chou schrieb:
>>> Reinhard Meyer wrote:
>>>> Its even simpler, provided the  hardware can do open collector. Here
>>>> was my solution for AVR32AP7000:
>>>>
>>> Then we could add a config for open drain similar to that of linux driver.
>>>
>>> # ifndef I2C_SDA
>>> #  ifdef CONFIG_SOFT_I2C_GPIO_SDA_IS_OPEN_DRAIN
>>> #   define I2C_SDA(bit) gpio_set_value(CONFIG_SOFT_I2C_GPIO_SDA, bit)
>>> #  else
>>> #   define I2C_SDA(bit) \
>>>     if (bit) {                        \
>>>         gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);    \
>>>     } else {                        \
>>>         gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
>>>     }
>>> #  endif
>>> # endif
>> Why would we need that? Since the defines reside in the board/project specific
>> include/configs/*.h file, it is well known if the hardware has open drain or
>> not.
> 
> We want to use common gpio framework and unify the soft i2c access. 
> Please see the patches from Mike Frysinger,
> [07/05] i2c: soft_i2c: add simple GPIO implementation
>
So, above #-code would be in soft_i2c.c in the future? Then I agree the
define would be useful. However Soft I2C should always allow for some quite
uncommon ways to handle the SDA and SCL lines, not everytime the hardware
is like simple GPIO... As I see, as long as I define alle I2C_ related things
myself, this is still the case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reinhard_meyer.vcf
Type: text/x-vcard
Size: 370 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100714/adf14b80/attachment.vcf 

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Mike Frysinger]

On Monday, July 12, 2010 00:14:39 Thomas Chou wrote:
> Reinhard Meyer wrote:
> > Whenever possible by the hardware, I make I2C_SDA/SCL(1) do a tri-state
> > and I2C_TRISTATE and I2C_ACTIVE are empty.
> 
> Dear Mike,
> 
> I traced the i2c-gpio.c of linux and realized that there are potential
> bus contention with the current soft_i2c.c if the ports are not
> open-drained.
> 
> Reinhard suggested a solution, which was similar to what linux driver
> does. So I would withdraw my SDA patch.
> 
> For our i2c gpio framework, I added these changes and tested on my
> boards. Please check if it works on yours.
> 
> # ifndef I2C_ACTIVE
> #  define I2C_ACTIVE do {} while (0)
> # endif
> 
> # ifndef I2C_TRISTATE
> #  define I2C_TRISTATE do {} while (0)
> # endif
> 
> # ifndef I2C_SDA
> #  define I2C_SDA(bit) \
> 	if (bit) {						\
> 		gpio_direction_input(CONFIG_SOFT_I2C_GPIO_SDA);	\
> 	} else {						\
> 		gpio_direction_output(CONFIG_SOFT_I2C_GPIO_SDA, 0);\
> 	}
> # endif
> 
> I didn't tristate SCL(1) because it cannot be tristated on some nios2
> boards. As soft_i2c of u-boot didn't support clock stretching, it
> shouldn't matter.

this sort of fixed things for me.  the problem i was having is that repeated 
start is not the default behavior for the soft i2c bus master.  i thought that 
this would be the default, but there doesnt seem to be any documentation on 
the expected standard behavior out of the box.

once i enabled that for all Blackfin boards, soft i2c worked for me.  so i 
posted an updated patch ... if you need some open drain behavior, i think it 
best to post a patch on top of that.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100721/86af7efa/attachment.pgp 

[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()

[Thomas Chou]

Mike Frysinger wrote:
> once i enabled that for all Blackfin boards, soft i2c worked for me.  so i 
> posted an updated patch ... if you need some open drain behavior, i think it 
> best to post a patch on top of that.
> -mike

Hi Mike,

Thanks. I tested the v2 patch, and it works well on my nios2 boards.

As I didn't use open drain on my boards, I would rather leave the add-on 
to those who really use it.

Best regards,
Thomas