[U-Boot] [PATCH 0/2] Fix the I2C on SAMA5D3

[U-Boot] [PATCH 0/2] Fix the I2C on SAMA5D3

[Alan Ott]

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(-)

-- 
2.9.3

[U-Boot] [PATCH 1/2] i2c: at91_i2c: Wait for TXRDY after sending the first byte

[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(+)

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], &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);
-- 
2.9.3

[U-Boot] [PATCH 2/2] i2c: at91_i2c: remove the .probe_chip function

[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(-)

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, &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,
 };
-- 
2.9.3

[U-Boot] [PATCH 1/2] i2c: at91_i2c: Wait for TXRDY after sending the first byte

[Yang, Wenyou]

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], &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

[U-Boot] [PATCH 2/2] i2c: at91_i2c: remove the .probe_chip function

[Yang, Wenyou]

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, &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,
>   };
Best Regards,
Wenyou Yang

[U-Boot] [PATCH 1/2] i2c: at91_i2c: Wait for TXRDY after sending the first byte

[Heiko Schocher]

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
-- 
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

[U-Boot] [PATCH 2/2] i2c: at91_i2c: remove the .probe_chip function

[Heiko Schocher]

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
-- 
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

[U-Boot] [PATCH 1/2] i2c: at91_i2c: Wait for TXRDY after sending the first byte

[Heiko Schocher]

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
-- 
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

[U-Boot] [PATCH 2/2] i2c: at91_i2c: remove the .probe_chip function

[Heiko Schocher]

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
-- 
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