Skip to content
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

[rtl] use past SDA in twd FSM transition #1165

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

LukasP46
Copy link
Contributor

Since the SDA has to have a valid value only during high SCL, I had the case where with the fall of SCL the SDA also changed and thus a wrong state transition happened. But the protocol should guarantee a valid input during high SCL, so this should be a valid fix?

The image shows such a transition, which would fail with the old implementation.

image

@stnolting
Copy link
Owner

You are right. SDA gets tested here on the falling edge of SCL which is not compliant to the spec (I think)...

However, checking smp.sda_sreg(2) might not be a good idea as this represents the state of SDA from one (processor clock!) cycle before.

@stnolting
Copy link
Owner

I've modified your PR so that ACK/NACK gets sampled on the rising edge of SCL, but evaluated at the falling edge of SCL (= the end of the ACK/NACK bit phase). This seems to look good - at least with my very simplem test case.

Feel free to revert my changes if this does not work for your test case.

Could you then please provide a minimal test case to reproduce this ACK "race condition"?

@stnolting stnolting marked this pull request as draft January 21, 2025 20:43
@LukasP46
Copy link
Contributor Author

However, checking smp.sda_sreg(2) might not be a good idea as this represents the state of SDA from one (processor clock!) cycle before.

Spec wise this would be fine "The data on the SDA line must be stable during the HIGH period of the clock." (3.1.3 UM10204
I2C-bus specification and user manual
) and even one clock period before the fall should be fine.

But it is not so transparent because it depends on the system clock. So your solution is cleaner and only costs one more bit on the ctrl register...

Feel free to revert my changes if this does not work for your test case.

Works just fine, thanks a lot!

Could you then please provide a minimal test case to reproduce this ACK "race condition"?

Unfortunately I use parts of a library I cannot share, but the tricky part was that the SDA has a transition to '1' at the very same moment the SCL falls.

@LukasP46
Copy link
Contributor Author

Unfortunately I use parts of a library I cannot share, but the tricky part was that the SDA has a transition to '1' at the very same moment the SCL falls.

Content wise I only read two bytes, so

  1. I send the device ID
  2. then I read one byte
  3. send an ACK (here was the error)
  4. then I read one byte and
  5. then I don't send an ACK
  6. but a stop after the ACK check period.

"I" is the I2C Host which just uses the Testbench TWD ports.

@stnolting stnolting marked this pull request as ready for review January 22, 2025 17:48
@stnolting
Copy link
Owner

Spec wise this would be fine "The data on the SDA line must be stable during the HIGH period of the clock." (3.1.3 UM10204 I2C-bus specification and user manual) and even one clock period before the fall should be fine.

This should be the case now.

Works just fine, thanks a lot!

Great to hear! Thank you for testing!

@stnolting stnolting merged commit 51fd684 into stnolting:main Jan 22, 2025
2 checks passed
@LukasP46 LukasP46 deleted the twd-2nd-fix branch January 24, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants