search

openbmc / qemu

Official QEMU mirror
Other
20 stars 22 forks source link

I2C command handling is too simplistic #5

Closed legoater closed 7 years ago

legoater commented 7 years ago

The I2C driver for mainline Linux has a more precise use of the irq status when issuing commands. The qemu model does not handle them properly and devices probing timeouts.

aspeed_i2c_bus_handle_cmd() needs a fix.

shenki commented 7 years ago

This is still an issue with v6 of the i2c patch set:

[    5.520000] aspeed-i2c-bus 1e78a0c0.i2c-bus: i2c bus 2 registered, irq 24
[    5.520000] aspeed-i2c-bus 1e78a100.i2c-bus: i2c bus 3 registered, irq 25
[    5.520000] aspeed-i2c-bus 1e78a140.i2c-bus: i2c bus 4 registered, irq 26
[    5.520000] aspeed-i2c-bus 1e78a180.i2c-bus: i2c bus 5 registered, irq 27
[   11.080000] rtc-rv8803 11-0032: Unable to read register 0x0e
[   11.080000] rtc-rv8803: probe of 11-0032 failed with error -110
[   11.080000] aspeed-i2c-bus 1e78a400.i2c-bus: i2c bus 11 registered, irq 28
[   11.080000] aspeed-i2c-bus 1e78a440.i2c-bus: i2c bus 12 registered, irq 29
[   11.080000] Driver for 1-wire Dallas network protocol.
[   16.160000] lm75: probe of 4-004a failed with error -110
[   21.200000] lm75: probe of 5-004a failed with error -110
ozbenh commented 7 years ago

Some of that might be a problem with the model, but it could also be that qemu exposes a bug in the patch... For example, Brendan patch still has an issue where it clears the status after starting the next command phase instead of before. If the command actually takes the time it should, we may never hit the bug, but if qemu completes it immediately, we will hit the bug as linux will clear the status of the next command instead.

So something to keep an eye on.

bjh83 commented 7 years ago

Noted, I am going to try your recommended change in the I2C driver and see if it fixes this (I plan on putting it in the next revision anyway).

I will try to get to it in the next week.

ozbenh commented 7 years ago

Heh I didn't say it was the problem :-)

I just wanted Cedric to know in case it is so he doesn't spend days trying to figure it out.

legoater commented 7 years ago

I am hitting this problem. The previous driver was reseting the irq status register and using a completion to start the next command. What's the plan for the current driver ?

ozbenh commented 7 years ago

On Tue, 2017-05-09 at 06:33 -0700, Cédric Le Goater wrote:

I am hitting this problem. The previous driver was reseting the irq status register and using a completion to start the next command.

What's the plan for the driver ?

I think Brendan plans to fix that in his next drop.

Cheers, Ben.

legoater commented 7 years ago

I now have a fix for QEMU, which was not handling the I2C commands correctly, but I also need this for Linux : https://github.com/legoater/linux/commit/c1e752bc64acaa4cbdc13f9d52895f3dd01cdf22

@bjh83 : is that ok for you ?

bjh83 commented 7 years ago

@legoater I will fix it in my next revision.

legoater commented 7 years ago

Fixed in mainline commits :

http://git.qemu.org/?p=qemu.git;a=commit;h=4960f084cfb4 "aspeed/i2c: introduce a state machine" http://git.qemu.org/?p=qemu.git;a=commit;h=d0efdc168640 "aspeed/i2c: handle LAST command under the RX command" http://git.qemu.org/?p=qemu.git;a=commit;h=ddabca757a4e "aspeed/i2c: improve command handling"