Add support for SMBus

[alistair23]

Pull Request Overview

Add some functions to support SMBus read/write operations. Although SMBus is similar to I2C it has a few differences, mostly in regards to frequency, ACK/NACK, timings and ARP.

Some I2C hardware exposes timing options that can be tweaked to better match the SMBus timing requirements. This is why we are adding two new functions. It tells the driver to tweak the timing to match the SMBus spec.

This PR adds new functions to the I2C HIL and also extends the virtual_i2c.rs to Mux SMBus devices with I2C devices.

Testing Strategy

With some patches on top of this I can communicate with an SMBus IR temperature sensor. Once this is merged I'll send a PR to add the sensor.

TODO or Help Wanted

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make format.
• Fixed errors surfaced by make clippy.

[bradjc]

Overall seems good.

Do we need the default implementation? I think it would be simpler to remove that, and then we don't need the new error.

[alistair23]

The idea with the default implementation is that we can do something like this:

match self.smbus_dev.smbus_write_read(buf, 1, 1) {
    Ok(_) => {}
    Err(e) => {
        if e.0 == Error::NotSupported {
            // SMBus operations aren't supported. Try normal I2C
            self.smbus_dev.write_read(e.1, 1, 1)
        }
    }
}

[bradjc]

I think it is misleading to mark an object as impl SMBus and then have it just give you errors. Are you expecting a capsule to do the fallback? Could there just be a capsule shim that converts I2C to SMBus if you need this use case?

[alistair23]

Yeah, I agree that is confusing. I originally had the functions as part of the I2C traits to not imply that we support SMBus.

I'm expecting the virtual_smbus or something similar to handle the fallback. The reason is that some I2C modules don't expose anything to software to correctly handle SMBus. In that case I think we should just fall-back to the standard I2C commands (some SMBus devices are lenient with the spec and should work).

In saying that most Tock I2C devices use 400kHz and SMBus is 100kHz, so it most likely won't work without the hardware having at least some implementation for these new functions.

[alistair23]

I pushed an update that fleshes this out more.

[bradjc]

The MuxI2C changes look pretty good, but I still don't see where the NotSupported case would happen.

[hudson-ayers]

Personally, I think I am okay with the HIL changes, but may need to think about them a little more.

However I don't really like adding a new SMBusDevice struct that is exactly identical to the I2CDevice struct, and implements the same traits with identical code...could we just modify the I2CDevice to have a SMBusMode boolean? Or a mode field that is an enum for either I2CMode or SMBusMode, and that defaults to I2CMode?

That would also help resolve the issue of how the current implementation always prioritizes I2C transactions over SMBus transactions

[hudson-ayers]

Would it be possible to separate the I2CMaster and SMBusMaster HILs, and then have the virtualI2C capsule look like this:

pub struct MuxI2C<'a> {
    i2c: &'a dyn hil::i2c::I2CMaster,
    smbus: Option<&'a dyn hil::smbus::SMBusMaster>,
    devices: List<'a, I2CDevice<'a>>,
    i2c_devices: List<'a, I2CDevice<'a>>,
    smbus_devices: List<'a, SMBusDevice<'a>>,
    enabled: Cell<usize>,
    inflight: OptionalCell<&'a I2CDevice<'a>>,
    i2c_inflight: OptionalCell<&'a I2CDevice<'a>>,
    smbus_inflight: OptionalCell<&'a SMBusDevice<'a>>,
    deferred_caller: &'a DynamicDeferredCall,
    handle: OptionalCell<DeferredCallHandle>,
}

Then any board supporting smbus could just pass the same trait object for the i2c and smbus fields, while boards not supporting smbus would pass None for the smbus field. Ideally, the i2c component could check at compile-time whether the smbus trait was implemented and pass the appropriate value to the MuxI2C::new().

[alistair23]

The problem with that is that we then can't mux the i2c and smbus devices. How do we ensure that only a single operation is going on?

[hudson-ayers]

The problem with that is that we then can't mux the i2c and smbus devices. How do we ensure that only a single operation is going on?

In this scenario virtual_i2c.rs is still responsible for muxing both i2c and smbus -- it just lets the SMBus HIL be completely seperate from the I2C HIL. But MuxI2C::do_next_op(), for example, would remain basically as it is now, but lines like self.i2c.smbus_read() would be replaced with self.smbus.unwrap().read().

I can try to prototype this if that doesn't make sense

[hudson-ayers]

https://github.com/hudson-ayers/tock/tree/smbus is what I have in mind. It seperates the SMBusMaster HIL from the I2CMaster, and removes the separate SMBusDevice struct in favor of adding a boolean to the I2CDevice struct.

It requires a lot less changes to the virtual_i2c capsule and I think lets you do everything that the current implementation does, but I am not totally familiar with SMBus so I could be wrong. Let me know what you think @alistair23

[alistair23]

I have pushed an update that follows what Hudson did and separates the SMBusMaster trait.

I prefer the separate SMBusDevice so I have left that in. Let me know what people think. If we think we should squash the device and use a bool I can do that instead.

[hudson-ayers]

Tested the Imix app with this branch, which reads from multiple i2c sensors in quick succession, and it works.

[brghena]

bors r+

[bors]

Build succeeded:

• ci-build (macos-latest)
• ci-build (ubuntu-latest)
• ci-format (macos-latest)
• ci-format (ubuntu-latest)
• ci-qemu
• ci-tests (macos-latest)
• ci-tests (ubuntu-latest)