feat(d1): implement I2C driver #103

hawkw · 2023-06-25T18:36:29Z

Motivation

The Beepy keyboard is driven by firmware (called i2c_puppet) running
on an RP2040 MCU, which can be communicated with over the I²C
bus. In order to use the keyboard, we therefore must implement a driver
for the Allwinner D1's I²C controller.

Closes #101
Closes #102

Solution

This branch adds a definition for an I²C driver service to
kernel::services, and an implementation of the I²C driver
service for the Allwinner D1's TWI hardware.

I²C Service

The cross-platform kernel cannot implement the server for the
I²C driver, because the driver implementation is
platform-specific; but we can provide an abstraction for interfacing
with drivers for a platform's I²C hardware. The
I²C service provides an I2cClient type, which is used to
initiate bus operations an I²C target device at a provided
address. Once a bus transaction is initiated, a Transaction type is
performed, which can be used to perform multiple read and write
operations with the target device. When the Transaction is completed,
a STOP condition is sent on the bus; until then, completing a read or
write will instead send a repeated-START. This interface permits the
common I²C pattern of writing a register address to read
from, and then reading the contents of that register from the target
within the same transaction.

In addition, the transaction interface can be used to implement the
embedded_hal_async crate's I2c trait for the I2cClient type. This
provides a higher level interface for I²C operations, and
allows our I²C driver to be used with third-party libraries
implemented against the embedded_hal_async interface.

Because of mnemOS' message-passing design, the I2cService operates
with owned buffers, rather than borrowed buffers, so a FixedVec<u8> is
used as the buffer type for both read and write operations. This means
that we must allocate when performing I²C operations. To
reduce the amount of allocation necessary, all Transaction methods
return the buffer that was passed in, allowing the buffer to be reused
for multiple operations. To facilitate this, the Transaction::read and
Transaction::write methods also take a len parameter indicating the
actual number of bytes to write from the buffer/read into the buffer,
rather than always writing the entire buffer contents or filling the
entire buffer with bytes. This way, we can size the buffer to the
largest buffer required for a sequence of operations, but perform
smaller reads and writes using the same FixedVec<u8>, avoiding
reallocations. The implementation of
embedded_hal_async::i2c::I2c::transaction will allocate a single
buffer large enough for the largest operation in the transaction, and
reuse that buffer for every operation.

TWI driver

This branch adds an implementation of a driver for controlling the
I²C hardware on the D1, which the D1 manual calls a TWI
(Two-Wire Interface), likely due to I²C being a trademark of
Phillips Semiconductor. The TWI hardware can be used in one of two
modes: "TWI engine" mode, where individual bytes are written/read from
the I²C bus in an interrupt handler, and "TWI driver" mode,
where the TWI hardware can operate at the level of I²C
register read/writes using a DMA buffer. This branch only implements a
driver for the TWI engine mode, since it can model all forms of
I²C operations. In the future, we will likely want to
opportunistically use the offload capabilities of the TWI driver when
the I²C transaction has the correct shape for offloading, but
this branch just implements the simpler TWI engine mode.

The TWI hardware is a bit difficult to use correctly, so implementing
this was a bit of a struggle. In particular, it turns out that the
generation of I²C clock pulses occurs when the TWI_CNTR
register, which controls the TWI, is written to.

The driver works by sharing state between a driver task and an ISR,
since the TWI engine mode is interrupt driven. The shared state is
"locked" by disabling TWI interrupts temporarily while the driver task
is writing to the shared state, and resuming interrupts when a write to
the shared state completes. In theory, this driver is also safe for use
on multi-core hardware, since a single daemon task is responsible for
all the non-ISR writes to this shared state, but this doesn't actually
matter, because the D1 is an inherently single-core CPU.

The D1 has four separate TWI controllers, TWI0, TWI1, TWI2, and
TWI3. The pin mapping for the 40-pin Pi header I2C0 pins differs
between the MangoPi MQ Pro and the Lichee RV Dock. On the MQ Pro, TWI0
is used, with SCL on pin PG12 and SDA on pin PG13. On the Lichee
RV Dock, TWI2 is used instead, with SCL on pin PB0 and SDA on
pin PB1. Therefore, the respective pin mappings are set up by the
individual board support targets, rather than in mnemos_d1_core.

Validation

I've validated the implementation by testing with the Beepy's
i2c_puppet device. Now, when running on the Beepy, we spawn a task
that performs a simple read operation to get i2c_puppet's firmware
version, and then sends commands to i2c_puppet to turn the Beepy's RGB
LED green. This works :)

Future Work

Some things this branch doesn't do, but that we will want to do in the
future, include:

Implement support for the "TWI driver" DMA offload mode, as
discussed above
Implement 10-bit I²C addresses. The current
implementation only supports 7-bit address mode. (platform(d1): implement 10-bit I2C addresses #143)
Potentially, add a driver for MnemOS operating as the
I²C bus target device rather than as the bus controller?
I'm not sure if this is actually something we would ever need,
though...
Implement a proper driver for i2c_puppet, so that we can use the
keyboard.
A potential optimization involving an I2cClient reusing the same
allocated buffer across multiple embedded_hal_async I2c::transactions
could be worth implementing.

netlify · 2023-06-25T18:36:33Z

✅ Deploy Preview for merry-scone-cc7a60 ready!

Name	Link
🔨 Latest commit	`ebc6764`
🔍 Latest deploy log	https://app.netlify.com/sites/merry-scone-cc7a60/deploys/64adbe7fea38d70008f772f1
😎 Deploy Preview	https://deploy-preview-103--merry-scone-cc7a60.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jamesmunns · 2023-06-25T20:53:13Z

source/kernel/src/registry/mod.rs

@@ -27,13 +27,15 @@ pub mod known_uuids {
        pub const SERIAL_MUX: Uuid = uuid!("54c983fa-736f-4223-b90d-c4360a308647");
        pub const SIMPLE_SERIAL_PORT: Uuid = uuid!("f06aac01-2773-4266-8681-583ffe756554");
        pub const EMB_DISPLAY: Uuid = uuid!("b54db574-3eb7-4c89-8bfb-1a20890be68e");
+        pub const I2C: Uuid = uuid!("011ebd3e-1b14-4bfd-b581-6138239b82f3");


Doesn't need to be fixed in this PR, but we probably want to think about including date/version values here. Eventually we should care about breaking interface changes.

yup, good idea!

platforms/allwinner-d1/boards/lichee-rv/src/twi.rs

Currently, the behavior of the `OwnedPortChunk::decode` function is incorrect. The returned `chunk` in the `OwnedPortChunk` is constructed by allocating a new `Vec` from the input, decoding a `PortChunk` from it, and then shrinking the `Vec` to the size of the `chunk` component of the returned borrowed `PortChunk`. This does the wrong thing. `Vec::shrink_to` shrinks the `Vec` from the *end*, so we are shrinking it down to the *first* `chunk.len()` bytes. The port number is at the beginning of the `Vec`, rather than the end, so instead of dropping the port number and keeping the data, we are dropping the last two bytes of the data and producing a buffer that contains `[port[0], port[1], data[0], ... data[len(data) - 2]]`. This means the data is silently corrupted. This branch fixes this behavior by instead constructing the owned chunk using `pc.chunk.to_vec()`. This has the unfortunate consequence of meaning that we allocate twice, rather than once, but it means that we now return the correct data when using `OwnedPortChunk`. Alternatively, we could use `Vec::remove` twice to remove the first two bytes of the `Vec`, but that's two _O_(_n_) operations, which is probably at least as bad as allocating. We could, potentially, change the `OwnedPortChunk::decode` operation to not just call `PortChunk::decode`, but that would be more complicated. Adding a failing test for this is as simple as adding a round-tripping test for `OwnedPortChunk` as well as `PortChunk`, which we didn't have previously. I've also changed the round-tripping tests to use `proptest` to generate arbitrary port numbers and chunks to round trip, because...why not?

platforms/allwinner-d1/boards/src/beepy.rs

platforms/allwinner-d1/core/src/drivers/twi.rs

jamesmunns · 2023-07-11T20:12:24Z

platforms/allwinner-d1/core/src/drivers/twi.rs

+
+impl Drop for TwiDataGuard<'_> {
+    fn drop(&mut self) {
+        self.twi.twi_cntr.modify(|_r, w| w.int_en().high());


Should we check if the int wasn't enabled before locking? not sure if this matters, but locking and unlocking could indirectly enable the interrupt even if you weren't expecting it.

theoretically yeah, we probably should — its not actually an issue here because the guard type is only used in one place, but it would be more correct to do that.

really, the MutexGuard-like behavior is not actually necessary any longer. at one point, i was actually locking and dropping the guard in several places. now, the only time we actually unmask the interrupt is inside of wait_for_irq, which guarantees that the driver task is not writing to the shared state just because it's yielded to wait for the IRQ. so, we could refactor this to remove the guard-like behavior...

platforms/allwinner-d1/core/src/drivers/twi.rs

platforms/allwinner-d1/core/src/lib.rs

source/alloc/src/heap.rs

source/kernel/src/services/i2c.rs

jamesmunns

Overall looks awesome, left a bunch of nits, feel free to take them or leave them :)

This branch adds support for the 10-bit I2C address mode. This was pretty trivial to add but I had punted on it in PR #103 in the interest of getting the minimum necessary I2C stuff working. I couldn't easily test this because I don't have any I2C devices on hand that require the 10-bit address mode, but it should work! :) Closes #143

jamesmunns reviewed Jun 25, 2023

View reviewed changes

hawkw force-pushed the eliza/i2c-2 branch from f6872f4 to 9c5baa9 Compare June 26, 2023 16:55

jamesmunns reviewed Jun 29, 2023

View reviewed changes

platforms/allwinner-d1/boards/lichee-rv/src/twi.rs Outdated Show resolved Hide resolved

hawkw force-pushed the eliza/i2c-2 branch 2 times, most recently from 8b1c6ff to c9653f3 Compare July 4, 2023 17:29

hawkw added 24 commits July 4, 2023 11:36

test(sermux): test OwnedPortChunk roundtrip

1ab7b48

test(sermux): add property tests for roundtripping

23203b2

wip: owned read buf

1a10159

lol wip

cf827bd

wip

35f353b

WIP: rough draft of a bad i2c api

ec2c24e

update

066ef28

wip

20742fc

separate mq pro and lichee rv twi mappings

072689c

wip

008cdaa

wip

89a1285

wip

dfc66da

WIP twi engine

56d6750

oops

0898b01

wip: total twi driver rewrite

b47c7a3

blerrrghhh

017f255

BLEH

785dbbd

maybe progress

5cc6ac6

ghghghghglgllghh

a1c58c6

actually set bus speed

015e860

welp

9611301

whghghghgh

d5fb490

rm uart crap, etc

e382772

hawkw added 2 commits July 11, 2023 11:01

rustfmt

622f279

more dox

23b3381

hawkw marked this pull request as ready for review July 11, 2023 18:28

hawkw added this to the beepberry computer v0.1 milestone Jul 11, 2023

This was referenced Jul 11, 2023

platform(d1): implement 10-bit I2C addresses #143

Closed

port bbq10kbd to embedded-hal-async #144

Closed

hawkw requested a review from jamesmunns July 11, 2023 19:51

hawkw added 2 commits July 11, 2023 12:59

remove readbuf code i didn't end up using

211db2a

rename to just i2c0

cc9189c