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

Designate LPSPI hardware chip select #131

Open
mciantyre opened this issue Mar 16, 2023 · 12 comments · May be fixed by #180
Open

Designate LPSPI hardware chip select #131

mciantyre opened this issue Mar 16, 2023 · 12 comments · May be fixed by #180
Labels
help wanted Extra attention is needed

Comments

@mciantyre
Copy link
Member

We're starting to add support for more hardware-managed peripheral chip selects (PCS) in the LPSPI driver (imxrt-rs/imxrt-iomuxc#34). This issue tracks a feature to control which PCS output is used by the LPSPI driver.

I think the baseline API should assume that a user called Lpspi::without_pins() to construct their driver. The other initialization paths assume that users supply the first PCS output, PCS0. Extending those resource-checked initialization paths could happen here, but I consider that a nice to have. Essentially, the feature shouldn't require that we're tracking the user's PCSn pin in the type system.

I'll leave some tips and suggestions as to what might work. No preference for making this a (non)breaking change.

@mciantyre
Copy link
Member Author

Tips, pointers, suggestions:

  • Chapter 48 in the 1060 reference manual (RM) covers the the LPSPI peripheral. The peripheral is the same across all supported MCUs.

  • The PCS for the next transaction is configured in the transfer command word. See table 48-3 in the RM.

  • There's two driver behaviors to consider. They're briefly covered in the module front-matter.

    • Users who prefer the higher-level API (transfer, write) expect a setting like set_bit_order and (a hypothetical) set_pcs to be "sticky." They set it once, and then all subsequent I/O uses that settings.
    • Users who prefer the lower-level Transaction API want to set the bit order and PCS themselves, so that it takes effect only for the (next) transaction.

    The new feature should support both of these users. To make this all concrete, study the way the driver manages bit_order. I would expect a similar approach to work for the PCSn selection.

@teburd
Copy link
Member

teburd commented Mar 16, 2023

Some maybe random thoughts on this, I haven't and can't really look too deeply but...

There might be some advantage to tracking a peripheral controlled chip select (pcsX) vs having a gpio controlled chip select in some manner, perhaps using a trait marker.

A SPI peripheral is a bus with potentially many devices hanging off it. Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange. IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature.

I know some folks will even use the SPI peripheral to try and hack up neoled control signaling in which case cs may not even be used!

There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

@mciantyre
Copy link
Member Author

mciantyre commented Mar 16, 2023

Good thoughts. I'm happy to help with you / others with that deeper look.

Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange.

Lpspi::without_pins() helps us with that, at least today. (I'm using this to bypass the perpetually-incomplete imxrt-iomuxc package.) Nevertheless, we can easily drop the chip select requirement in new as a breaking change.

For readers using imxrt-hal 0.5, I'd expect Lpspi::without_pins() to work for that CS-less neoled control and also more advanced software-managed control.

IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature. [...] There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

I like this. If we're going this route, we could start future proofing for embedded-hal 1.0.The blocking SPI traits in alpha-9 differentiate "no chip select" upport from "hardware-managed chip select" support; I like the latter. I also appreciate how we can always get software-managed chip selects for free through embedded-hal-bus. It's worthy to break our LPSPI API for these goals.


I have no immediate plan to write any of these ideas into code myself. I'm happy to help others put this into the HAL. Leave questions and comments here if you'd like more insight / mentorship.

@ameliamariecollins
Copy link

These things are import to me:

  • That lpspi4's initialization doesn't assume I want to initialize every chip select — I may want those pins for other functions.
  • That desired chip selects are associated with the hardware SPI, naturally and intuitively and simply doing the right thing by default.
  • That we also make sure that this is compatible with tasks under RTIC, where another task my steal in and need to use another peripheral on the same SPI. Also, this presents a Rust problem: does the interrupting task need an impossible second &mut?

Perhaps we may even think of some representation of devices on the SPI bus. In my case, an ST7789V uses Pcs0, and the SD card on the display module uses Pcs1.

@ameliamariecollins
Copy link

So the question is where the split happens in the representation. If we have one lpspi object, the split may happens by calling a method that switches to a desired Pcs — but this invites trouble when pre-empted, because even if some intervening code is equipped to store the previous Pcsx selection and restore it, things can go wrong, leaving the hardware device in the wrong state.

Perhaps there's a representation of a combination of SPI bus and a particular chip select, and someone owns the &mut to that. We're attempting to deterministically abstract a tricky situation, so I want to help the user by sweeping the details under the rug and presenting the cleanest possible API at the top level.

@ameliamariecollins
Copy link

Good thoughts. I'm happy to help with you / others with that deeper look.

Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange.

Lpspi::without_pins() helps us with that, at least today. (I'm using this to bypass the perpetually-incomplete imxrt-iomuxc package.) Nevertheless, we can easily drop the chip select requirement in new as a breaking change.

For readers using imxrt-hal 0.5, I'd expect Lpspi::without_pins() to work for that CS-less neoled control and also more advanced software-managed control.

IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature. [...] There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

I like this. If we're going this route, we could start future proofing for embedded-hal 1.0.The blocking SPI traits in alpha-9 differentiate "no chip select" upport from "hardware-managed chip select" support; I like the latter. I also appreciate how we can always get software-managed chip selects for free through embedded-hal-bus. It's worthy to break our LPSPI API for these goals.

I have no immediate plan to write any of these ideas into code myself. I'm happy to help others put this into the HAL. Leave questions and comments here if you'd like more insight / mentorship.

As you can see, I'm already agonizing over the possibilities. I will gladly add such a system to HAL (with a bit of help) when the time comes. First, we need to figure out what the shape of this should be.

@ameliamariecollins
Copy link

ameliamariecollins commented Mar 17, 2023

I know some folks will even use the SPI peripheral to try and hack up neoled control signaling in which case cs may not even be used!

My APA102Cs and SK9822s feel seen! 😃

@ameliamariecollins
Copy link

Some maybe random thoughts on this, I haven't and can't really look too deeply but...

There might be some advantage to tracking a peripheral controlled chip select (pcsX) vs having a gpio controlled chip select in some manner, perhaps using a trait marker.

A SPI peripheral is a bus with potentially many devices hanging off it. Having the chip select as part of the driver initialization (LPSPI::new()) seems a bit strange. IMO this really should be done as part of the transaction setup. E.g. spi.with_cs(...).transact(...) or something to that nature.

I know some folks will even use the SPI peripheral to try and hack up neoled control signaling in which case cs may not even be used!

There's also I think some proposals I saw to improve the SPI embedded-hal traits which might be worth tracking, dealing with shared ownership of a bus.

I like spi_thingy::with_cs() a lot. It would take a Pin and initialize it. In this case, it would return some object that was comped from the SPI hardware device, and the CS pin it initialized. Reads and writes would always assert the CS pin for safety (and other CS pins would need to be de-asserted), and would be atomic to ensure the device wouldn't be interrupted. Plus then you could have the regular spi::new() great a CS-less SPI for situations where someone wanted to control CS themselves, or if no CS was needed.

So I guess there's a CSPinGroup somewhere in this, that knows all the in-use PcsX pins for a SPI bus and some SPI device has a way to get at their pin and can select()/deselect() it. select() would first deselect other pins, then select the desired pin.

@mciantyre
Copy link
Member Author

If you're curious about how RTIC manages resources, it's documented here. RTIC uses granular critical sections to ensure mutual exclusion of a resource. Here's some pseudo-code to show how a single SPI bus could be shared across two tasks, each using blocking I/O operations.

#[shared]
struct Shared {
    lpspi4: Lpspi<MyPins, 4>,
    // ...
}

#[local]
struct Local {
    pcs0: Pcs0Pin,
    pcs2: Pcs2Pin,
    // ...
}

#[task(shared = [lpspi4], local = [pcs2], priority = 2)]
fn task_a(cx: task_a::Context) {
    let mut buffer = [0; 16];
    cx.shared.lpspi4.lock(|lpspi4| {
        lpspi4.set_chip_select(&mut cx.local.pcs2);
        lpspi4.transfer(&mut buffer).unwrap();
    });
    handle_spi_data(&buffer);
}

#[task(shared = [lpspi4], local = [pcs0], priority = 1)]
fn task_b(cx: task_b::Context) {
    cx.shared.lpspi4.lock(|lpspi4| {
        lpspi4.set_chip_select(&mut cx.local.pcs0);
        lpspi4.write(&[3, 4, 5, 6, 7]).unwrap();
    });
}

Task A uses PCS2 to interact with one device, and task B uses PCS0 to interact with another device. RTIC prevents wrong peripheral states by requiring a lock to access the LPSPI4 bus.

This pseudo-code only has a single object for the SPI bus, or what the latest embedded-hal expresses with *Bus traits. We're not splitting the bus into devices with chip selects, called SpiDevice in the latest embedded-hal. We could support that splitting, and I think it would work in RTIC. But if we're optimizing for RTIC, I'm curious to see what that split device objects look like. We need to manage the locking ourself if we're attempting a SpiDevice implementation.

@ameliamariecollins
Copy link

I like this approach. I was looking through the HAL code earlier today. Perhaps a new Lpspi has no initial CS pin, but when set_chip_select is called, it looks it up in a list of CS pins. If it's not there, it configures it and adds it to the list. The select method can be sure to deselect the other CS lines so it doesn't leave two devices selected with their SDOs fighting each other.

@mciantyre
Copy link
Member Author

mciantyre commented Mar 20, 2023

Sounds great. Feel free to remove the PCS0 requirement from new and from the struct Pins.

I'm interested to see what that list of CS pins looks like. Some maybe gotch-yas and considerations:

  • The standard imxrt_iomuxc API uses unique types for pad objects. If we're taking ownership of PCSx objects while keeping the strong types, we would need to reflect this type list in the public API. It might become a complicated type state machine. Nevertheless, we have ways to erase pad types, and other workarounds, to reduce this complexity.
  • Configuring a pad for LPSPI PCSx functions is relatively cheap; just a write or two to a peripheral register. I don't see much harm in having a code path that re-configures the pin again and again.

In the pseudo-code above, set_chip_select is called with an imxrt_iomuxc pad object that implements a LPSPIn PCSx function. The user implicitly manages the collection of CS pins so we don't have to.

@ameliamariecollins
Copy link

ameliamariecollins commented Mar 27, 2023

I'm having trouble budgeting time for this project right now. Can you mark this feature as "Needs Help"?

Many thanks.

@teburd teburd added the help wanted Extra attention is needed label Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants