| Message ID | 20210506205419.26294-1-zev@bewilderbeest.net |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: aspeed: disable additional device addresses on ast2[56]xx | expand |
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>
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>
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!
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. */
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(-)