[U-Boot] [PATCH] i2c: fix SDA contention in read_byte()
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);
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.
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
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
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
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
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
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
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.
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
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
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
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.
participants (4)
-
Andrew Dyer -
Mike Frysinger -
Reinhard Meyer -
Thomas Chou