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

FR: Allow generating a Clocks from the current RCC register values #215

Open
BryanKadzban opened this issue Aug 3, 2024 · 2 comments
Open

Comments

@BryanKadzban
Copy link
Contributor

Background:

Several peripheral init methods require a const ref to a Clocks struct, to ensure the clocks have been set up and to figure out what frequencies to use as their input calculations. (For example, SPI requires knowing the frequency of the bus that it's on, to figure out which divisor to use to generate the frequency that the user requested for the SPI clock pin.)

But the only way to get a Clocks struct is to call .freeze() on the RCC CFGR, consuming it, and .freeze() tries to set a bunch of RCC registers to actuate the requested clock configuration.

The consumption-of-self is correct here, because those RCC registers can't be safely changed after the clocks are initialized. (Or, more correctly, the microcontroller allows the registers to change, but you have to go through a very different process to change them than you went through to set them up in the first place. Rerunning the setup code potentially disables the core's main clock, for example. So you can't just re-run .freeze() and get a different clock configuration.)

And that's all well and good.

The problem is, in my case, I run a firmware image under a bootloader, and the bootloader sets up the clock related registers before it jumps to the firmware's first instruction. So I can't call .freeze() even if I set up the same requested clock configuration, as I don't want to risk locking up the core by disabling its (PLL-derived) clock.

This means I can't use the SPI wrapper.

That's not the end of the world -- I could just use the SPI registers directly I suppose -- but the way the HAL already correctly and safely implements DMA is something I would really like to be able to use.

So, the feature request:

I'd like to be able to generate a Clocks struct off the actual RCC register contents, read-only. This might need to be unsafe, to avoid using both it and the .freeze() API and create two separate Clocks instances, but I actually think that if this new API consumes an RCC RegisterBlock, it might be OK. Or maybe it can consume a CFGR just like .freeze() does, so you can only ever use one of them? That seems best now that I say it.

WDYT?

@maximeborges
Copy link
Member

Hey, I was having a look at that last Monday but was too busy since.
My idea was to consume RCC and generate its content from the current values of the registers (something like fn from_current_config(self, hse_freq: Option<Hertz>) -> Rcc), so you can still apply some modification to the Rcc struct at that point, and then you would be able to call freeze() on it and passe the Clock around without any issue..
My current code is a bit too WIP right now, but what do you think about going that way

@BryanKadzban
Copy link
Contributor Author

So, another method on RccExt, then?

I think that works as long as the caller doesn't modify the resulting Rcc struct in a way that causes .freeze() to break the clocks when applying the settings back to the registers. It's also a little wasteful to read regs and then write them back to the same values, but that's not as big of an issue as it could be (the parts of this firmware that need to use this new API don't need to be tiny).

I have a WIP git change as well locally, with the initial .adopt_registers() idea (consuming a CFGR). Haven't committed or pushed it yet, but it does compile, at least.

But I'd be up for either approach, sure. LMK what you think about the .freeze() concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants