[U-Boot] [PATCH 0/2] Fix the I2C on SAMA5D3
Two patches to fix the I2C on SAMA5D3. The first fixes a race condition in pushing TX data to the hardware, and the second removes an errant probe_chip function which prevents devices from being probed properly
These have been tested on master and on 2017.07
Alan Ott (2): i2c: at91_i2c: Wait for TXRDY after sending the first byte i2c: at91_i2c: remove the .probe_chip function
drivers/i2c/at91_i2c.c | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-)
The driver must wait for TXRDY after each byte is pushed into the i2c FIFO before pushing the next byte. Previously this was not done for the first byte, causing a race condition with zeros sometimes being sent for the next byte (which is typically the first actual data byte).
Signed-off-by: Alan Ott alan@softiron.com --- drivers/i2c/at91_i2c.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/at91_i2c.c b/drivers/i2c/at91_i2c.c index d394044..20d0929 100644 --- a/drivers/i2c/at91_i2c.c +++ b/drivers/i2c/at91_i2c.c @@ -72,6 +72,8 @@ static int at91_i2c_xfer_msg(struct at91_i2c_bus *bus, struct i2c_msg *msg)
} else { writel(msg->buf[0], ®->thr); + ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY); + for (i = 1; !ret && (i < msg->len); i++) { writel(msg->buf[i], ®->thr); ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);
On 2017/11/29 11:25, Alan Ott wrote:
The driver must wait for TXRDY after each byte is pushed into the i2c FIFO before pushing the next byte. Previously this was not done for the first byte, causing a race condition with zeros sometimes being sent for the next byte (which is typically the first actual data byte).
Signed-off-by: Alan Ott alan@softiron.com
Acked-by: Wenyou Yang wenyou.yang@microchip.com
Thank you for your patch.
drivers/i2c/at91_i2c.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/at91_i2c.c b/drivers/i2c/at91_i2c.c index d394044..20d0929 100644 --- a/drivers/i2c/at91_i2c.c +++ b/drivers/i2c/at91_i2c.c @@ -72,6 +72,8 @@ static int at91_i2c_xfer_msg(struct at91_i2c_bus *bus, struct i2c_msg *msg)
} else { writel(msg->buf[0], ®->thr);
ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);- for (i = 1; !ret && (i < msg->len); i++) { writel(msg->buf[i], ®->thr); ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);
Best Regards, Wenyou Yang
Hello Alan,
Am 29.11.2017 um 04:25 schrieb Alan Ott:
The driver must wait for TXRDY after each byte is pushed into the i2c FIFO before pushing the next byte. Previously this was not done for the first byte, causing a race condition with zeros sometimes being sent for the next byte (which is typically the first actual data byte).
Signed-off-by: Alan Ott alan@softiron.com
drivers/i2c/at91_i2c.c | 2 ++ 1 file changed, 2 insertions(+)
Thanks!
Reviewed-by: Heiko Schocher hs@denx.de
bye, Heiko
Hello Alan,
Am 29.11.2017 um 04:25 schrieb Alan Ott:
The driver must wait for TXRDY after each byte is pushed into the i2c FIFO before pushing the next byte. Previously this was not done for the first byte, causing a race condition with zeros sometimes being sent for the next byte (which is typically the first actual data byte).
Signed-off-by: Alan Ott alan@softiron.com
drivers/i2c/at91_i2c.c | 2 ++ 1 file changed, 2 insertions(+)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
The .probe_chip function is supposed to probe an i2c device on the bus to determine whether a device is answering to a particular address. at91_i2c_probe_chip() did not do anything resembling this and always returned 0.
It looks as though at91_i2c_probe_chip() was intended to be a .probe function for the controller, as it was copied-and-pasted to become at91_i2c_probe() in 0bc8f640a4d7ed.
Removing the at91_i2c_probe_chip() function makes the higher layer (i2c_probe_chip()) try a zero-length read transfer to test for the presence of a device instead, which does work.
Signed-off-by: Alan Ott alan@softiron.com --- drivers/i2c/at91_i2c.c | 22 ---------------------- 1 file changed, 22 deletions(-)
diff --git a/drivers/i2c/at91_i2c.c b/drivers/i2c/at91_i2c.c index 20d0929..7917ca1 100644 --- a/drivers/i2c/at91_i2c.c +++ b/drivers/i2c/at91_i2c.c @@ -201,27 +201,6 @@ static int at91_i2c_enable_clk(struct udevice *dev) return 0; }
-static int at91_i2c_probe_chip(struct udevice *dev, uint chip, uint chip_flags) -{ - struct at91_i2c_bus *bus = dev_get_priv(dev); - struct at91_i2c_regs *reg = bus->regs; - int ret; - - ret = at91_i2c_enable_clk(dev); - if (ret) - return ret; - - writel(TWI_CR_SWRST, ®->cr); - - at91_calc_i2c_clock(dev, bus->clock_frequency); - - writel(bus->cwgr_val, ®->cwgr); - writel(TWI_CR_MSEN, ®->cr); - writel(TWI_CR_SVDIS, ®->cr); - - return 0; -} - static int at91_i2c_set_bus_speed(struct udevice *dev, unsigned int speed) { struct at91_i2c_bus *bus = dev_get_priv(dev); @@ -256,7 +235,6 @@ static int at91_i2c_ofdata_to_platdata(struct udevice *dev)
static const struct dm_i2c_ops at91_i2c_ops = { .xfer = at91_i2c_xfer, - .probe_chip = at91_i2c_probe_chip, .set_bus_speed = at91_i2c_set_bus_speed, .get_bus_speed = at91_i2c_get_bus_speed, };
On 2017/11/29 11:25, Alan Ott wrote:
The .probe_chip function is supposed to probe an i2c device on the bus to determine whether a device is answering to a particular address. at91_i2c_probe_chip() did not do anything resembling this and always returned 0.
It looks as though at91_i2c_probe_chip() was intended to be a .probe function for the controller, as it was copied-and-pasted to become at91_i2c_probe() in 0bc8f640a4d7ed.
Removing the at91_i2c_probe_chip() function makes the higher layer (i2c_probe_chip()) try a zero-length read transfer to test for the presence of a device instead, which does work.
Signed-off-by: Alan Ott alan@softiron.com
Acked-by: Wenyou Yang wenyou.yang@microchip.com
Thank you for your patch.
drivers/i2c/at91_i2c.c | 22 ---------------------- 1 file changed, 22 deletions(-)
diff --git a/drivers/i2c/at91_i2c.c b/drivers/i2c/at91_i2c.c index 20d0929..7917ca1 100644 --- a/drivers/i2c/at91_i2c.c +++ b/drivers/i2c/at91_i2c.c @@ -201,27 +201,6 @@ static int at91_i2c_enable_clk(struct udevice *dev) return 0; }
-static int at91_i2c_probe_chip(struct udevice *dev, uint chip, uint chip_flags) -{
- struct at91_i2c_bus *bus = dev_get_priv(dev);
- struct at91_i2c_regs *reg = bus->regs;
- int ret;
- ret = at91_i2c_enable_clk(dev);
- if (ret)
return ret;- writel(TWI_CR_SWRST, ®->cr);
- at91_calc_i2c_clock(dev, bus->clock_frequency);
- writel(bus->cwgr_val, ®->cwgr);
- writel(TWI_CR_MSEN, ®->cr);
- writel(TWI_CR_SVDIS, ®->cr);
- return 0;
-}
- static int at91_i2c_set_bus_speed(struct udevice *dev, unsigned int speed) { struct at91_i2c_bus *bus = dev_get_priv(dev);
@@ -256,7 +235,6 @@ static int at91_i2c_ofdata_to_platdata(struct udevice *dev)
static const struct dm_i2c_ops at91_i2c_ops = { .xfer = at91_i2c_xfer,
- .probe_chip = at91_i2c_probe_chip, .set_bus_speed = at91_i2c_set_bus_speed, .get_bus_speed = at91_i2c_get_bus_speed, };
Best Regards, Wenyou Yang
Hello Alan,
Am 29.11.2017 um 04:25 schrieb Alan Ott:
The .probe_chip function is supposed to probe an i2c device on the bus to determine whether a device is answering to a particular address. at91_i2c_probe_chip() did not do anything resembling this and always returned 0.
It looks as though at91_i2c_probe_chip() was intended to be a .probe function for the controller, as it was copied-and-pasted to become at91_i2c_probe() in 0bc8f640a4d7ed.
Removing the at91_i2c_probe_chip() function makes the higher layer (i2c_probe_chip()) try a zero-length read transfer to test for the presence of a device instead, which does work.
Signed-off-by: Alan Ott alan@softiron.com
drivers/i2c/at91_i2c.c | 22 ---------------------- 1 file changed, 22 deletions(-)
Thanks!
Reviewed-by: Heiko Schocher hs@denx.de
bye, Heiko
Hello Alan,
Am 29.11.2017 um 04:25 schrieb Alan Ott:
The .probe_chip function is supposed to probe an i2c device on the bus to determine whether a device is answering to a particular address. at91_i2c_probe_chip() did not do anything resembling this and always returned 0.
It looks as though at91_i2c_probe_chip() was intended to be a .probe function for the controller, as it was copied-and-pasted to become at91_i2c_probe() in 0bc8f640a4d7ed.
Removing the at91_i2c_probe_chip() function makes the higher layer (i2c_probe_chip()) try a zero-length read transfer to test for the presence of a device instead, which does work.
Signed-off-by: Alan Ott alan@softiron.com
drivers/i2c/at91_i2c.c | 22 ---------------------- 1 file changed, 22 deletions(-)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
participants (3)
-
Alan Ott -
Heiko Schocher -
Yang, Wenyou