-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HID: ft260: fix wake up device from power saving mode #7
HID: ft260: fix wake up device from power saving mode #7
Conversation
Hi @enrikb, I cannot reproduce the issue in my VM (Virtual box Version 6.1.32, Ubuntu 22.04.1 LTS, kernel 5.15.0-52)
Can you add the issued commands and logs? Thanks, |
Hi @MichaelZaidman,
To get more insight, I hooked a scope to the bus. The signal has no issues. What is interesting: the FT260 blindly sends all the bytes requested even though no single ACK can be seen. (Of course, it is expected in this case that no ACK can be seen, there is still no device at 0x8. But is it normal that the I2C controller keeps on transmitting after a NACK?) Anyway, the FT260 behavior of sending all data in spite of NACKs could explain different timing depending on the amount of data written. After applying the change in this PR, I can easily run a test loop like this:
Without the change, the same tests usually fails after just a few rounds (single digit number). You may have noticed that I'm only writing a single byte in the |
Hi @enrikb, Thanks for the details. I run the 100 iterations loop on HW and VM setups with the UMFT260EV1A evaluation module. Strange, but I cannot reproduce the issue you reported. The test timing on my VM resembles yours, but the test always succeeds.
Generally, the i2c slave device is allowed not to acknowledge the address byte. In contrast, for the SMBus, it is the requirement. Since the ft260 is an i2c bus controller, it is less restrictive with this. I have no scope at the moment to try to reproduce the experiment, but from the log you shared, I see that you sent one data byte only. So should we expect the controller to report NACK on this byte only? Does it make sense? Initially, I rejected the FTDI's suggestion of querying the status twice in the ft260_xfer_status() due to the expected performance penalty. "The workaround of querying the status twice will work, but it may degrade the performance since every status query takes 200us on average", was my response. But we can improve it in the way shown below. Since need_wakeup_at is accessed in an atomic way, it is safe to access it from different execution contexts. The rationale for doing it this way is to minimize the overhead associated with the extra ft260_xfer_status call and make it more generic by covering all xfer cases. What do you think?
|
Hi @MichaelZaidman, |
JFTR, I have dumped a few waveforms from the scope: Write to non-existing device: Even though no single ACK can be seen, the FT260 continues writing all bytes. The protocol decoder shows '?' for the missing ACK (i.e. NACK). detail of the above clearly showing NACKs both on address and first byte To confirm plausible test setup, write address 0 to the EEPROM on the FTDI eval board: The ACKs from the EEPROM are clearly visible due to slightly different level. No more '?' from the protocol decoder. |
506ac7f
to
16f86ea
Compare
I changed the PR following your suggestion. |
Hi @enrikb, Thanks for the wave forms.
I can explain the Regarding the rest of the bytes, the I2C spec says: Thanks, |
Hi @MichaelZaidman, |
Hi @enrikb,
No, it is correct when "If a slave-receiver does acknowledge the slave address", but in our case, there is no device at the slave address. The controller cannot know it since, according to the spec, the data line should be left in HIGH, which is also valid for ghost devices. The related quotes from I2C spec: "When a slave doesn’t acknowledge the slave address (for example, it’s unable to receive or transmit because it’s performing some real-time function), the data line must be left HIGH by the slave". "If a slave-receiver does acknowledge the slave address To summarize:
|
"The slave leaves the data line HIGH and the master generates a Yes, exactly. And the FT260, as far as I can see, continues to send data before generating a STOP or repeated START. |
Perhaps, I was not clear. It is for the REAL devices that acknowledged the address first. It is not for the GHOST devices, as in our case. |
Hi @enrikb,
Thanks for this. I did more testing. It works. We can see multiple wake-up messages during the first write into EEPROM since the status is polled in the loop before the
I suggest replacing the
with
So the log will look like this
Thanks, |
c354762
to
029ae8f
Compare
Hi @MichaelZaidman, log message changed in the latest force push (the one before only rebases to the updated main). I'm also checking the return code of the 'wakeup' status request now as the result will be used in the log. This way, the log also nicely confirms (again) the bogus first status, as can be seen in your dump. |
Hi @enrikb, I fixed the check patch warnings and modified the code by gathering the wake-up handling in a single place. It implies an extra status query every 5 seconds during the I2C traffic but removes jiffies calculation per I2C transfer request. What do you think?
|
During testing from within a KVM based virtual machine, it was found that the wakeup implementation was not working in all cases. By requesting status two times in ft260_xfer_status() after the chip has been idle more than 5s the fix seems to be more reliable. Co-developed-by: Michael Zaidman <michael.zaidman@gmail.com> Signed-off-by: Enrik Berkhan <Enrik.Berkhan@inka.de>
029ae8f
to
912ff6f
Compare
Thanks @MichaelZaidman, |
Thanks @enrikb! Merged. |
During testing from within a KVM based virtual machine, it was found that the wakeup implementation was not working in all cases.
By moving the extra ft260_xfer_status() to
ft260_hid_output_report_check_status(), it can be placed after sending the output report. This change seems to fix the issue or at least to be more stable. No more bogus responses after power save have been noticed during my tests, both on real hardware and through the VM.
I tested both with debug messages enabled and disabled.
If this is still not enough for other hardware / timing constellations, adding a small additional delay for the wake-up case should be considered.
Signed-off-by: Enrik Berkhan Enrik.Berkhan@inka.de