Skip to contentSkip to conversation

i2c: fix SDA contention in read_byte()

PATCH New Thomas Chou 4 participants 13 messages Jul 6, 2010 – Jul 22, 2010 I2C

lore
Share & tools
lore

Thomas Chou

author
#0 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

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

Signed-off-by Thomas Chou <[email protected]>
---
 drivers/i2c/soft_i2c.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/i2c/soft_i2c.c +1 -1
index 847db76..344b7f8 100644
@@ -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);
Signature · 1 line
1.7.1
NAK
#1 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 14 lines · Thomas Chou

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

Signed-off-by Thomas Chou <[email protected]>

---
?drivers/i2c/soft_i2c.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

drivers/i2c/soft_i2c.c +0 -0
index 847db76..344b7f8 100644
@@ -305,8 +305,8 @@ static uchar read_byte(int ack)

? ? ? ?/*
? ? ? ? * Read 8 bits, MSB first.

Quoted 8 lines

? ? ? ? */
- ? ? ? 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.

#2 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 9 lines · Andrew Dyer

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.

Quoted 6 lines

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

author
#3 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 2 lines · Reinhard Meyer

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

#4 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 35 lines · Thomas Chou
Quoted 3 lines · Reinhard Meyer

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
Quoted 4 lines

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

author
#5 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 3 lines · Reinhard Meyer

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

#6 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 7 lines · Thomas Chou
Quoted 3 lines · Reinhard Meyer

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.

Quoted 12 lines
# 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.

Quoted 3 lines

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

author
#7 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 20 lines · Reinhard Meyer
Quoted 6 lines · Thomas Chou
Quoted 3 lines · Reinhard Meyer

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.

Quoted 12 lines
# 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
Quoted 3 lines

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

Quoted 6 lines
Quoted 3 lines

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

#8 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 20 lines · Thomas Chou
Quoted 19 lines · Reinhard Meyer
Quoted 18 lines · Thomas Chou
Quoted 3 lines · Reinhard Meyer

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
Quoted 8 lines
Quoted 3 lines

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

#9 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

In reply to Thomas Chou
Quoted 33 lines · On Monday, July 12, 2010 00:14:39 Thomas Chou wrote:
Quoted 2 lines · Reinhard Meyer

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
Quoted 3 lines

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

author
#10 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 4 lines · Mike Frysinger

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

#11 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

In reply to Andrew Dyer
Quoted 31 lines · On Wednesday, July 07, 2010 00:45:42 Andrew Dyer wrote:
Quoted 12 lines · Thomas Chou

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

Signed-off-by Thomas Chou <[email protected]>
---
 drivers/i2c/soft_i2c.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/i2c/soft_i2c.c +0 -0
index 847db76..344b7f8 100644

drivers/i2c/soft_i2c.c static uchar read_byte(int ack)

Quoted 10 lines
drivers/i2c/soft_i2c.c +1 -2
@@ -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.

Quoted 5 lines

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

#12 lore raw

PATCH 1/1 i2c: fix SDA contention in read_byte()

Quoted 43 lines · Mike Frysinger
Quoted 31 lines · On Wednesday, July 07, 2010 00:45:42 Andrew Dyer wrote:
Quoted 14 lines · Thomas Chou

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

Signed-off-by Thomas Chou <[email protected]>

---
?drivers/i2c/soft_i2c.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

drivers/i2c/soft_i2c.c +0 -0
index 847db76..344b7f8 100644
@@ -305,8 +305,8 @@ static uchar read_byte(int ack)

? ? ? ?/*
? ? ? ? * Read 8 bits, MSB first.

Quoted 8 lines

? ? ? ? */
- ? ? ? 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.

Quoted 5 lines

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.

Quoted 4 lines

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.