[PATCH v2] i2c: aspeed: disable additional device addresses on ast2[56]xx

[PATCH v2] i2c: aspeed: disable additional device addresses on ast2[56]xx

[Zev Weiss]

The ast25xx and ast26xx have, respectively, two and three configurable
slave device addresses to the ast24xx's one.  We only support using
one at a time, but the others may come up in an indeterminate state
depending on hardware/bootloader behavior, so we need to make sure we
disable them so as to avoid ending up with phantom devices on the bus.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---

Changes since v1 [0]:
 - reduced to simplified approach suggested by Joel

[0] https://lore.kernel.org/linux-arm-kernel/20200915184525.29665-1-zev@bewilderbeest.net/

 drivers/i2c/busses/i2c-aspeed.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 724bf30600d6..67e8b97c0c95 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -727,10 +727,14 @@ static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
 {
 	u32 addr_reg_val, func_ctrl_reg_val;
 
-	/* Set slave addr. */
-	addr_reg_val = readl(bus->base + ASPEED_I2C_DEV_ADDR_REG);
-	addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
-	addr_reg_val |= slave_addr & ASPEED_I2CD_DEV_ADDR_MASK;
+	/*
+	 * Set slave addr.  Reserved bits can all safely be written with zeros
+	 * on all of ast2[456]00, so zero everything else to ensure we only
+	 * enable a single slave address (ast2500 has two, ast2600 has three,
+	 * the enable bits for which are also in this register) so that we don't
+	 * end up with additional phantom devices responding on the bus.
+	 */
+	addr_reg_val = slave_addr & ASPEED_I2CD_DEV_ADDR_MASK;
 	writel(addr_reg_val, bus->base + ASPEED_I2C_DEV_ADDR_REG);
 
 	/* Turn on slave mode. */
-- 
2.31.1

Re: [PATCH v2] i2c: aspeed: disable additional device addresses on ast2[56]xx

[Brendan Higgins]

On Thu, May 6, 2021 at 1:54 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> The ast25xx and ast26xx have, respectively, two and three configurable
> slave device addresses to the ast24xx's one.  We only support using
> one at a time, but the others may come up in an indeterminate state
> depending on hardware/bootloader behavior, so we need to make sure we
> disable them so as to avoid ending up with phantom devices on the bus.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

Looks great! No concerns from me.

Nevertheless, I am not in a position to test this at this time. Joel,
or Andrew could one of you (or someone else on the mailing list) test
this?

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH v2] i2c: aspeed: disable additional device addresses on ast2[56]xx

[Joel Stanley]

On Fri, 7 May 2021 at 19:57, Brendan Higgins <brendanhiggins@google.com> wrote:
>
> On Thu, May 6, 2021 at 1:54 PM Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> > The ast25xx and ast26xx have, respectively, two and three configurable
> > slave device addresses to the ast24xx's one.  We only support using
> > one at a time, but the others may come up in an indeterminate state
> > depending on hardware/bootloader behavior, so we need to make sure we
> > disable them so as to avoid ending up with phantom devices on the bus.
> >
> > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>
> Looks great! No concerns from me.
>
> Nevertheless, I am not in a position to test this at this time. Joel,
> or Andrew could one of you (or someone else on the mailing list) test
> this?

I tried testing this by connecting I2C1 and I2C2 (schematic numbering)
on the AST2600A2 EVB. I got it working, but only when I read a single
byte. There's more about that below, but it's a different issue
unrelated to this patch.

Reviewed-by: Joel Stanley <joel@jms.id.au>
Tested-by: Joel Stanley <joel@jms.id.au>

--
On to the bug I saw:

When reading one byte, it works:

root@ast2600a2:~# dd status=none
if=/sys/bus/i2c/devices/i2c-2/2-0064/eeprom  count=1 bs=1| hexdump -C
00000000  48                                                |H|

If I try to read more than one byte:

root@ast2600a2:~# dd status=none
if=/sys/bus/i2c/devices/i2c-2/2-0064/eeprom  count=1 bs=2 | hexdump -C
[ 1568.320096] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1568.328385] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1568.339106] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1568.347423] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1568.358112] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1568.366430] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
dd: error reading '/sys/bus/i2c/devices/i2c-2/2-0064/eeprom':
Connection timed out


If I then go back to reading one byte, I get the error the first time
but the data does come out:

root@ast2600a2:~# dd status=none
if=/sys/bus/i2c/devices/i2c-2/2-0064/eeprom  count=1 bs=1 | hexdump -C
[ 1593.306360] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1593.315191] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
00000000  48                                                |H|
00000001

Subsequent reads work as expected.

With further debugging turned on, this is the log of doing a two byte read:

[ 1781.027360] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.027552] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xec0b0000
[ 1781.027751] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.027906] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000001, cmd 0xea250000
[ 1781.028069] aspeed-i2c-bus 1e78a180.i2c-bus: received error
interrupt: 0x00000008
[ 1781.029683] aspeed-i2c-bus 1e78a180.i2c-bus: SDA hung (state
a050000), attempting recovery
[ 1781.029760] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xd2b10004
[ 1781.029773] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1781.038729] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000020, cmd 0x0a050000
[ 1781.038761] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1781.046932] aspeed-i2c-bus 1e78a100.i2c-bus: received error
interrupt: 0x00000020
[ 1781.047022] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.047195] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xec0b0000
[ 1781.047404] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.047556] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000001, cmd 0xea250000
[ 1781.047756] aspeed-i2c-bus 1e78a180.i2c-bus: received error
interrupt: 0x00000008
[ 1781.049367] aspeed-i2c-bus 1e78a180.i2c-bus: SDA hung (state
a050000), attempting recovery
[ 1781.049421] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xd2b10004
[ 1781.049435] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1781.058322] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000020, cmd 0x0a050000
[ 1781.058351] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1781.066522] aspeed-i2c-bus 1e78a100.i2c-bus: received error
interrupt: 0x00000020
[ 1781.066609] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.066789] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xec0b0000
[ 1781.066998] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.067148] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000001, cmd 0xea250000
[ 1781.067313] aspeed-i2c-bus 1e78a180.i2c-bus: received error
interrupt: 0x00000008
[ 1781.068968] aspeed-i2c-bus 1e78a180.i2c-bus: SDA hung (state
a050000), attempting recovery
[ 1781.069083] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xd2b10004
[ 1781.069100] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1781.077903] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000020, cmd 0x0a050000
[ 1781.077934] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1781.086106] aspeed-i2c-bus 1e78a100.i2c-bus: received error
interrupt: 0x00000020
[ 1781.086186] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.086371] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xec0b0000
[ 1781.086579] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1781.086730] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000001, cmd 0xea250000
[ 1781.086897] aspeed-i2c-bus 1e78a180.i2c-bus: received error
interrupt: 0x00000008

This is the result of the first single byte read after the error state:

[ 1710.150555] aspeed-i2c-bus 1e78a180.i2c-bus: SDA hung (state
a050000), attempting recovery
[ 1710.150627] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xd2350004
[ 1710.150646] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1710.159468] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000020, cmd 0x0a050000
[ 1710.159500] aspeed-i2c-bus 1e78a100.i2c-bus: Expected ACK after
processed read.
[ 1710.167681] aspeed-i2c-bus 1e78a100.i2c-bus: received error
interrupt: 0x00000020
[ 1710.167793] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1710.167971] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000004, cmd 0xec0b0000
[ 1710.168178] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000084, cmd 0xec0b0000
[ 1710.168335] aspeed-i2c-bus 1e78a100.i2c-bus: slave irq status
0x00000002, cmd 0x0a070000

This is unrelated to your change, but should be investigated by anyone
looking at slave support on the ast2600.

Lets go ahead with merging this change.


>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH v2] i2c: aspeed: disable additional device addresses on ast2[56]xx

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Thu, May 06, 2021 at 03:54:19PM -0500, Zev Weiss wrote:
> The ast25xx and ast26xx have, respectively, two and three configurable
> slave device addresses to the ast24xx's one.  We only support using
> one at a time, but the others may come up in an indeterminate state
> depending on hardware/bootloader behavior, so we need to make sure we
> disable them so as to avoid ending up with phantom devices on the bus.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]