firmware/i2c: rewrite I2C implementation

[jordens]

• Never drive SDL or SDA high. They are specified to be open
  collector/drain and pulled up by resistive pullups. Driving
  high fails miserably in a multi-master topology (e.g. with
  a USB I2C interface). It would only ever be implemented to
  speed up the bus actively but that's tricky and completely
  unnecessary here.
• Make the handover states between the I2C protocol phases (start, stop,
  restart, write, read) well defined. Add comments stressing those
  pre/postconditions.
• Add checks for SDA arbitration failures and stuck SCL.
• Remove wrong, misleading or redundant comments.

ARTIQ Pull Request

Description of Changes

Related Issue

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
✓	🔨 Refactoring
	📜 Docs

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

• Use correct spelling and grammar.
• Update RELEASE_NOTES.md if there are noteworthy changes, especially if there are changes to existing APIs.
• Close/update issues.
• Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

• Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
• Test your changes or have someone test them. Mention what was tested and how.
• Add and check docstrings and comments
• Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Documentation Changes

• Check, test, and update the documentation in doc/. Build documentation (cd doc/manual/; make html) to ensure no errors.

Git Logistics

• Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
• Write short & meaningful commit messages. Review each commit for messages (git show). Format:

  topic: description. < 50 characters total.
  
  Longer description. < 70 characters per line
  

Licensing

See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.

[sbourdeauducq]

* Never drive SDL or SDA high.

I believe only SCL was driven high (because this was never meant to be used with multiple masters) - correct?

[sbourdeauducq]

With this new code, what happens if the FPGA talks on the I2C bus at the same time as another master?

[jordens]

Neither SCL nor SDA should ever be driven high on a normal i2c bus. Single master or not. It just becomes worse with multiple masters.
The same thing will happen as before. One of them will loose arbitration and fail it's transaction. But with the new code at least there is no doubly driven line anymore. And additionally the new code will recognize a couple possible cases of arbitration failure.

[sbourdeauducq]

Neither SCL nor SDA should ever be driven high on a normal i2c bus. Single master or not. It just becomes worse with multiple masters.

What is the issue with driving SCL high in a single-master system?

One of them will loose arbitration and fail it's transaction.

And it seems the possible failure modes include silent corruption of a transaction's data... Multi-master I2C doesn't sound like such a great idea.

[sbourdeauducq]

https://www2.renesas.cn/cn/zh/doc/products/mpumcu/apn/002/rej06b0634_h8sap.pdf has some details about multi-mastering that seems somewhat reliable, though it sounds complicated and I remain a bit skeptical of multi-master I2C. Do we trust other masters to implement something like that correctly?

[jordens]

• It's the responsibility of the original implementation/implementer to argue its correctness. By demanding proof how the clearly faulty old implementation is problematic you reverse that responsibility.
• The old implementation violates the I2C protocol.
• For your amusement and doing the work that the reviewer or original implementer should have been done long ago: It already blows up if any slave decides to do clock stretching. The new implementation doesn't support clock stretching either but it prevents doubly driven signals and potential damage.
• I don't understand how your complaints about multi-master affect the merits or validity of this PR. The old implementation prevented multi-master from ever working as demonstrated. The new one enables it for the dominant use cases.

I am in the uncomfortable position of having to defend a supposedly advantageous, correct, and compliant PR because of what appears to be you wanting to save face. This has a chilling effect.
Since there are no arguments against the PR, I'll either close it or merge it.

[sbourdeauducq]

The current implementation is intended for simple single-master situations without clock stretching. It is a simple design that doesn't pretend to support any advanced I2C features. It is something we added because Daniel wanted some basic expanders supported on the KC705, and it was later reused for other simple devices like the Si5324 and EEPROMs - none of these are mastering the bus or stretching the clock. For these purposes, as far as I can tell, it is adequate and not "fundamentally broken" or "faulty".
I'm trying to understand what you want to achieve here. If it is simply a step towards supporting all the bells and whistles of I2C (full multi-master and clock stretching support will take a lot more work throughout the ARTIQ codebase and testing of third-party devices and software, as we see), and you think that supporting these are a good idea, then just say that.

[marmeladapk]

@sbourdeauducq if my understanding of this code is correct then this PR will make Artiq support more edge cases with I2C. It will save us (either someone from your team or from ours) time in the future when we encounter some chip with more fancy I2C implementation.

[jordens]

Driving SCL high is fundamentally broken and faulty. It isn't and wasn't adequate by any definition. I don't understand why you claim that. Everybody makes mistakes. You won't lose credibility by admitting that the old code was wrong.

This PR fixes a serious bug on all our multi-master implementations. That's all Kaslis, Sayma, Metlino. You don't need to wait for a clock stretching chip or a corruption-free multi-master protocol to see that.

[sbourdeauducq]

Driving SCL high is fundamentally broken and faulty. It isn't and wasn't adequate by any definition. I don't understand why you claim that.

Simply because, for regular unidirectional signals (again: this code was intended for simple situations), push-pull is the default choice and more common than open drain and pull-up. Call it a mistake if you wish...

[marmeladapk]

@sbourdeauducq Driving SCL high also broke FTDI I2C on Kasli v2.0 when FPGA was programmed. (FTDI wasn't able to drive SCL low so it had low level of around 1,5 V). Now it's fixed.

[hartytp]

:(

[sbourdeauducq]

Okay, but I don't know why you want those complicated and fragile I2C multi-mastering schemes. For accessing I2C on a EEM crate, we could have done it through the FPGA like we currently do it for the SPI flash. And the MMC on µTCA does not really need to access the same I2C bus as the FPGA (or even better: there should be no MMC at all).

[marmeladapk]

@sbourdeauducq Previous code didn't allow to use I2C over USB when ARTIQ was flashed. I consider this as a bug, since everything else on the board suggests that it should be possible.

[sbourdeauducq]

With the current code it will only work "most of the time". If by chance you access the I2C bus at the same time as ARTIQ, transfers will get corrupted.

The ARTIQ I2C implementation was never meant to support I2C multi-mastering, and I do not see why it should, because I2C multi-mastering:

• is a lot more complicated to implement correctly than a "I2C proxy bitstream" single-master scheme, similar to the one we use for SPI flash, which would give a similar level of access to the I2C buses of a crate from USB.
• is inherently more fragile: it only takes one non-compliant master for the whole bus to break, or for data to get intermittently and silently corrupted. And I2C multi-mastering is not a trivial protocol as I mentioned above, so we should expect problems.
• does not provide any clear advantages (is there a case for e.g. flashing I2C EEPROMs without interrupting the operation of ARTIQ?)