Change in coreboot[master]: drivers/sn65dsi86: Switch EDID reading to use "indirect mode"

[Douglas Anderson (Code Review)]

Attention is currently required from: Vinod Polimera, Julius Werner.
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52959 )

Change subject: drivers/sn65dsi86: Switch EDID reading to use "indirect mode"
......................................................................

Patch Set 4: Code-Review-1

(4 comments)

Patchset:

PS4: 
I think there are still problems. Please yell if you think I got any of my analysis wrong.

File src/drivers/ti/sn65dsi86bridge/sn65dsi86bridge.c:

https://review.coreboot.org/c/coreboot/+/52959/comment/ab176843_cc7e282b 
PS4, Line 189: 		[I2C_RAW_WRITE] = I2C_OVER_AUX_WRITE_MOT_1,
             : 		[I2C_RAW_READ] = I2C_OVER_AUX_READ_MOT_1,
I haven't fully analyzed the side effects, but I _think_ that always using MOT=1 here isn't quite right. MOT=Middle-of-Transaction and I believe it says not to send the "stop" bit. I think you're essentially always leaving the panel hanging at the end of your i2c transac...

For the kernel this is all abstracted out by the generic I2C-over-aux framework which presumably handles it.

For the "Example of Indirect I2C Read of the EDID." in the TI datasheet, they seem to handle it by adding an extra zero-byte command (with MOT=0) at the end. For read that's:
```
18. Program the AUX_CMD = 0x1, AUX_ADDR[7:0] = 0x50, and AUX_LENGTH = 0x00.
```
For write, that's:
```
9. Program the AUX_CMD = 0x0, AUX_ADDR[7:0] = 0x30, and AUX_LENGTH = 0x00.
```

Maybe you're handling this in some other way and I just missed it.

https://review.coreboot.org/c/coreboot/+/52959/comment/1cb68a7f_f8491612 
PS4, Line 200: i2c_writeb(bus, chip, SN_AUX_CMD_REG, (cmd[request] << 4))
I don't know if it maters, but kernel driver and datasheet show doing this _first_, before the setting of the address and length. Maybe move it first just to be consistent?

https://review.coreboot.org/c/coreboot/+/52959/comment/998550d4_d8f0f050 
PS4, Line 220: 
In the kernel driver, we check a whole bunch of extra status things here (timeout, short, and failed). I don't think we've ever actually seen them in practice but I think they're important. Specifically your code is assuming that the 100 ms timeout above will handle everything but I don't think it will. Specifically see this bit:
> Once the requested Read or Write completes, the SN65DSI86 will clear the SEND bit and if an error occurred, the SN65DSI86 will set the NAT_I2C_FAILED flag. The NAT_I2C_FAILED flag will get set if for some reason the slave NACK the I2C Address. If the Slave NACK without completing the entire request AUX_LENGTH, the SN65DSI86 will set the AUX_SHORT flag and update the AUX_LENGTH register with the amount of data completed and then clear the SEND bit

So the SEND bit could be cleared but then we might have a problem.

Presumably for most of this you just want to treat it as an error. The bridge chip does a whole lot of retries on its own for you so pretty much anything it reports back to you means we've already tried a bunch and failed. In theory you could _try_ to handle the SHORT case but I think treating it as an error is likely fine too. Since I don't think we've ever seen this in practice, the current kernel code is just our best guess about how to handle it (the kernel driver leverages the generic kernel framework for most of this).