Basic SDRAM FMC driver #194
Conversation
|
Thanks for the PR. I was planning to add FSMC support at some point and since they're very similar we should probably try to add them in one go and in a consolidated way. I need to look at your proposal at more detail but I'll add a few things I've noticed right away. |
| $gpioX | ||
| .$pXY | ||
| .into_alternate_af12() | ||
| .set_speed(crate::gpio::Speed::High) |
There was a problem hiding this comment.
Why only High instead of VeryHigh?
There was a problem hiding this comment.
Primarily because it's unnecessary. Using a higher Speed means using more GPIO drivers (more drive strength), which means more potential EMI emissions and power consumption (best practice is to use the lowest Speed suitable for the application). The FMC runs the memory at half of HCLK, which puts you in the ballpark of 90MHz SDCLK frequency on a maxed out F429 running at 180MHz. Speed::High the lowest setting which reliably gives us the bandwidth we need (good for 50MHz - 100MHz).
| sdnwe: SDNWE, | ||
| ) { | ||
| let peripherals = unsafe { crate::stm32::Peripherals::steal() }; | ||
| let gpiod = peripherals.GPIOD.split(); |
There was a problem hiding this comment.
Those operations are risky as they do RMW operations. At the very least we need to somehow protect these. Also I think those GPIOs don't belong into something called SDRAMConfig as the GPIOs are completely independent of what exactly you're attaching to your parallel bus.
Probably better to move the GPIO configuration out into it's own thing. Also an interesting approach to explore would be to have the traditional new function where you have to pass in all the correct pins and a (unsafe) new_unchecked function which assumes that the application has properly set up everything.
There was a problem hiding this comment.
Those operations are risky as they do RMW operations. At the very least we need to somehow protect these.
Agree with the risk, I was hoping it's mitigated by the fact that the operations they do are idempotent (i.e., they never disable any GPIO peripherals or pins, only additively enable them), and banking on the fact that the RegisterBlocks are guarded by VolatileCells in the upstream API
Also I think those GPIOs don't belong into something called SDRAMConfig as the GPIOs are completely independent of what exactly you're attaching to your parallel bus. Probably better to move the GPIO configuration out into it's own thing.
Do you mean the GPIOs which are passed into configure, or generally the notion of impl SDRAMConfig being ultimately responsible for setting up the GPIOs. If the former -- those GPIOs are necessary for SDRAM functionality. The are passed into SDRAMConfig.configure(...) which consumes the SDRAMConfig and returns an SDRAM. This is deliberately done so that at least ownership of the swappable SDRAM control pins is taken in this case. If the latter, it's only done that way to separate out the logic of the different steps (i.e., so we don't have a gigantic configure() function)
Also an interesting approach to explore would be to have the traditional new function where you have to pass in all the correct pins and a (unsafe) new_unchecked function which assumes that the application has properly set up everything.
A new function was my initial approach, but was abandoned due to the potential combinatorial explosion in future support for all device families and FMC configurations (i.e., device family <-> gpio ports mappings/AFs <-> possible FMC configurations w/r/t bus widths and other pins). OTOH, I can totally see a separate FMCAddressBus, FMCDataBus, etc. set of traits which /could/ encapsulate the different non-swappable pins, but again, their construction would involve significant boilerplate and doesn't seem to me to bring much value unless the rest of FMC's more sophisticated capabilities are supported (e.g., NOR/PSRAM mux, etc.)
There was a problem hiding this comment.
Agree with the risk, I was hoping it's mitigated by the fact that the operations they do are idempotent (i.e., they never disable any GPIO peripherals or pins, only additively enable them), and banking on the fact that the RegisterBlocks are guarded by VolatileCells in the upstream API
VolatileCell is not providing any protection against racy RMW operations.
Do you mean the GPIOs which are passed into configure, or generally the notion of impl SDRAMConfig being ultimately responsible for setting up the GPIOs.
Both. The pins used by FMC/FSMC are the same, independent of the mode or devices connected to the bus. Probably we should even separate the bus setup altogether from the configuration of the channels.
A new function was my initial approach, but was abandoned due to the potential combinatorial explosion in future support for all device families and FMC configurations (i.e., device family <-> gpio ports mappings/AFs <-> possible FMC configurations w/r/t bus widths and other pins).
It's not too bad, really. We'll have to deal with that anyway at some point.
I can test e.g. with a STM32F413H Disco (FSMC) and would certainly like to see the display work with this driver. I do have it running (in a hackish way) and can certainly provide some details about that.
There was a problem hiding this comment.
VolatileCell is not providing any protection against racy RMW operations.
Someone would be using this code with ownership of the peripheral register blocks anyway when doing their GPIO setup, and the VolatileCell guards against races due to the compiler reordering the accesses. I suspect we might not be on the same page with what "racy" means, could you elaborate?
Both. The pins used by FMC/FSMC are the same, independent of the mode or devices connected to the bus. Probably we should even separate the bus setup altogether from the configuration of the channels.
I just started doing this, but this does mean a separate type for each possible width of address bus (+3 types for data buses). To enforce stuff like valid values of Column vs Row bits, how do you feel about requiring const_generics to support that?
There was a problem hiding this comment.
Someone would be using this code with ownership of the peripheral register blocks anyway when doing their GPIO setup
How do you mean? You're conjuring the PeripheralBlocks out of thin air with your steal(). Stealing != owning.
I just started doing this, but this does mean a separate type for each possible width of address bus (+3 types for data buses).
Not necessarily. Depending on how we want to achieve that we could do a lot of different things, including automatically generating enums with all possible variants or specififying the maximum possible and allowing dummy pins.
how do you feel about requiring const_generics to support that?
Anything not requiring nightly is fair game. 😅
There was a problem hiding this comment.
#[inline] has no effect in dev mode. Need to go #[inline(always)] for that.
There was a problem hiding this comment.
Just peppered #[inline(always)] everywhere, same result.
There was a problem hiding this comment.
I've pushed an fmc-support-owned branch if you want to see if you can make it fit. I'm going to make this PR rely less on unsafe in the interim w/r/t owning FMC and taking ownership of as many pins as possible.
There was a problem hiding this comment.
Need to go deeper into the rabbit hole and figure out where the bloat is coming from. It's unfortunate that dev mode is pretty much unusable for embedded but it's also not worth working around it in weird ways because it's quite likely that you'll hit the next road block right around the corner (usually in the form of code executing so slow due a massive amount of silly function calls that all essential timing goals are missed).
There was a problem hiding this comment.
I've pushed an
fmc-support-ownedbranch if you want to see if you can make it fit.
Making code work in dev mode is a tertiary goal for me at best. As I mentioned before I'd be okay with having two ways of initialising, one "regular" new() with all pins and one new_unchecked(), e.g. like done here: https://github.com/stm32-rs/stm32h7xx-hal/blob/master/src/spi.rs. Though I will insist on that being unsafe because I very much disagree on the unchecked postfix providing a clear enough message about the real unsafety (and that unsafe is only applicable to memory safety) of this.
|
Thanks for taking a look at this and giving valuable feedback! I've replied to a few of these with my motivations/thought process, and will fix the remainder later tonight. Happy to keep discussing and refining this until its something everyone's OK with! I originally wanted to kickstart a discussion of how an FMC API would work as an issue, but it seemed more fruitful to start with an MVP and refine it with something that can be tested for ergonomics and functionality -- hence the PR. |
Co-authored-by: Daniel Egger <daniel@eggers-club.de>
Co-authored-by: Daniel Egger <daniel@eggers-club.de>
|
@iostat I absolutely prefer the MVP approach! |
|
I've started a crate to provide FMC/FSMC drivers that can be used for all the stm32 parts. It currently only supports some F7, some H7, and F469/F479, and also only has SDRAM support so far. Would you be interested in combining this PR into the crate? I think some of the tasks would be:
Let me know your thoughts. I will be adding more support for the H7, but happy to help here as well. |
297: Add support for some LCD controllers using the FMC/FSMC r=therealprof a=samcrow # Introduction This pull request adds code that uses the Flexible Memory Controller or Flexible Static Memory Controller to interface with some types of LCD controllers. We have [another FMC pull request](#194) for non-LCD applications that has been inactive for a while. I'm hoping that this pull request will be easier to review because it does not try to implement everything that the hardware supports. # Features * Controls up to 4 independent LCDs * Compatible with LCD controllers that use the Intel 8080 protocol with a 16-bit bidirectional data bus * Includes an example that works on the STM32F412G-DISCO board * Provides a low-level interface that can be used to write drivers for specific LCD controllers # Limitations * No support for bus widths other than 16 bits * I have only tested this code with Sitronix ST7789-series LCD controllers. Are there other LCD controllers that use an Intel 8080-like interface that this code cannot support? * The code uses some trait trickery to ensure that the client code can only access the display(s) that correspond to the chip select pin(s) in use. However, this may make the API too difficult for people to understand. # Future work * Support an 8-bit data bus (in addition to the currently supported 16-bit bus) * Potentially support an 18-bit data bus, on microcontroller models that have more than 16 data pins Co-authored-by: Sam Crow <scrow@eng.ucsd.edu>
|
Closing in favor of stm32-fmc. |
Title says it all. This is a basic driver allowing one to use the FMC peripheral to drive an SDRAM module which can then be accessed directly in the memory space of the MCU. It /should/ work on F427, F429, F437, and F439 devices. However, I only have a STM32F429I-Discovery board to test it against. Mostly ported from the STM32 HAL/LL libraries. Included is a simple RAM test in
examples/targeting the STM32F429I-Discovery boardIt'd be nice if this could be expanded to support NOR/PSRAM and all the other goodies that the FMC offers, but I don't have any hardware to test it with. I believe the 429I-EVAL includes a NOR Flash and SDRAM on a single bus, so that may be interesting to ultimately target.
One particular divergence from the usual patterns in this module is that we
Peripherals::steal()to get access to the GPIOs to set up their AF/Speed/Pullup. This is mainly due to the sheer number of pins that would otherwise have to be passed by the user, which seemed inconvenient at best. The majority of the FMC pins aren't swappable, so it made the most sense ergonomically to just manipulate them directly usingsteal(). The few pins which /are/ swappable (and whose usage of swapping affects behavior) are captured in the type system.