Skip to contentSkip to conversation

Fix the I2C on SAMA5D3

PATCH New Alan Ott 3 participants 9 messages Nov 29, 2017 – Dec 7, 2017

Subsystems (2)
Arm/Microchip (AT91) SoC support I2C
lore
Share & tools
lore

Alan Ott

author
#0 lore raw

cover 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(-)
Signature · 1 line
2.9.3
author
#1 lore raw

PATCH 1/2 i2c: at91_i2c: Wait for TXRDY after sending the first byte

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 <[email protected]>
---
 drivers/i2c/at91_i2c.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/i2c/at91_i2c.c +2 -0
index d394044..20d0929 100644
@@ -72,6 +72,8 @@ static int at91_i2c_xfer_msg(struct at91_i2c_bus *bus, struct i2c_msg *msg)
 
 	} else {
 		writel(msg->buf[0], &reg->thr);
+		ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);
+
 		for (i = 1; !ret && (i < msg->len); i++) {
 			writel(msg->buf[i], &reg->thr);
 			ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);
Signature · 1 line
2.9.3
#2 lore raw

PATCH 1/2 i2c: at91_i2c: Wait for TXRDY after sending the first byte

Quoted 8 lines · 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 <[email protected]>

---

Acked-by   Wenyou Yang <[email protected]>

Thank you for your patch.

Quoted 8 lines
  drivers/i2c/at91_i2c.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/i2c/at91_i2c.c +0 -0
index d394044..20d0929 100644

drivers/i2c/at91_i2c.c static int at91_i2c_xfer_msg(struct at91_i2c_bus *bus, struct i2c_msg *msg) · line 79

Quoted 8 lines
drivers/i2c/at91_i2c.c +2 -0
@@ -72,6 +72,8 @@ static int at91_i2c_xfer_msg(struct at91_i2c_bus *bus, struct i2c_msg *msg)
 
  	} else {
  		writel(msg->buf[0], &reg->thr);
+		ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);
+
  		for (i = 1; !ret && (i < msg->len); i++) {
  			writel(msg->buf[i], &reg->thr);
  			ret = at91_wait_for_xfer(bus, TWI_SR_TXRDY);

Best Regards,
Wenyou Yang

#3 lore raw

PATCH 1/2 i2c: at91_i2c: Wait for TXRDY after sending the first byte

In reply to Alan Ott

Hello Alan,

Am 29.11.2017 um 04:25 schrieb Alan Ott:

Quoted 10 lines

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 <[email protected]>
---
  drivers/i2c/at91_i2c.c | 2 ++
  1 file changed, 2 insertions(+)

Thanks!

Reviewed-by Heiko Schocher <[email protected]>

bye,
Heiko

Signature · 3 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de
#4 lore raw

PATCH 1/2 i2c: at91_i2c: Wait for TXRDY after sending the first byte

In reply to Alan Ott

Hello Alan,

Am 29.11.2017 um 04:25 schrieb Alan Ott:

Quoted 10 lines

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 <[email protected]>
---
  drivers/i2c/at91_i2c.c | 2 ++
  1 file changed, 2 insertions(+)

Applied to u-boot-i2c master

Thanks!

bye,
Heiko

Signature · 3 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de
author
#5 lore raw

PATCH 2/2 i2c: at91_i2c: remove the .probe_chip function

In reply to 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 <[email protected]>
---
 drivers/i2c/at91_i2c.c | 22 ----------------------
 1 file changed, 22 deletions(-)
drivers/i2c/at91_i2c.c +0 -22
index 20d0929..7917ca1 100644
@@ -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, &reg->cr);
-
-	at91_calc_i2c_clock(dev, bus->clock_frequency);
-
-	writel(bus->cwgr_val, &reg->cwgr);
-	writel(TWI_CR_MSEN, &reg->cr);
-	writel(TWI_CR_SVDIS, &reg->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,
 };
Signature · 1 line
2.9.3
#6 lore raw

PATCH 2/2 i2c: at91_i2c: remove the .probe_chip function

Quoted 9 lines · 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.

Quoted 6 lines

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 <[email protected]>

---

Acked-by Wenyou Yang <[email protected]>

Thank you for your patch.

Quoted 36 lines
  drivers/i2c/at91_i2c.c | 22 ----------------------
  1 file changed, 22 deletions(-)
drivers/i2c/at91_i2c.c +0 -21
index 20d0929..7917ca1 100644
@@ -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, &reg->cr);
-
-	at91_calc_i2c_clock(dev, bus->clock_frequency);
-
-	writel(bus->cwgr_val, &reg->cwgr);
-	writel(TWI_CR_MSEN, &reg->cr);
-	writel(TWI_CR_SVDIS, &reg->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);

drivers/i2c/at91_i2c.c static int at91_i2c_ofdata_to_platdata(struct udevice *dev) · line 240

Quoted 7 lines
drivers/i2c/at91_i2c.c +0 -1
@@ -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

#7 lore raw

PATCH 2/2 i2c: at91_i2c: remove the .probe_chip function

In reply to Alan Ott

Hello Alan,

Am 29.11.2017 um 04:25 schrieb Alan Ott:

Quoted 9 lines

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.

Quoted 8 lines

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 <[email protected]>
---
  drivers/i2c/at91_i2c.c | 22 ----------------------
  1 file changed, 22 deletions(-)

Thanks!

Reviewed-by Heiko Schocher <[email protected]>

bye,
Heiko

Signature · 3 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de
#8 lore raw

PATCH 2/2 i2c: at91_i2c: remove the .probe_chip function

In reply to Alan Ott

Hello Alan,

Am 29.11.2017 um 04:25 schrieb Alan Ott:

Quoted 9 lines

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.

Quoted 8 lines

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 <[email protected]>
---
  drivers/i2c/at91_i2c.c | 22 ----------------------
  1 file changed, 22 deletions(-)

Applied to u-boot-i2c master

Thanks!

bye,
Heiko

Signature · 3 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de