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

Full RCC support for STM32F107 #3779

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

algesten
Copy link

@algesten algesten commented Jan 16, 2025

STM32F107 has a two more PLLs than F013, which means using f013.rs config options doesn't let us configure the entire RCC.

My use case is to use STM32F107 for ethernet with an external HSE.

This PR adds a special case for F107 with a fuller RCC config.

@algesten algesten marked this pull request as ready for review January 16, 2025 11:16
@Dirbaio
Copy link
Member

Dirbaio commented Jan 16, 2025

instead of adding a new f107.rs file, could you add support for the PLLs in f013.rs using cfg's like cfg(stm32f107)?

(We used to have code duplication like this in RCC a while ago and it proved to be an unmaintainable mess. Hard to keep them in sync with fixes and features. the file you've added is already out of sync, for example it does .unwrap() instead of unwrap!(..) and is missing the asserts for min/max frequencies)

@algesten
Copy link
Author

@Dirbaio sure. Is there some thinking of how these are bundled together? Like, why is f107 sorted under f013? Like is there some arch concept that certain groups of STM32 belongs together?

@Dirbaio
Copy link
Member

Dirbaio commented Jan 16, 2025

Yes, they're grouped by "what STM32 families are more similar". ST did some design for the F1, then F107, F3, F0 are incremental improvements on top of that but the "overall structure" stays very similar. OTOH they designed the PLLs very differently for F2, then F4, F7 are incremental improvements on top of that. Etc etc.

(f013 means "STM32F0 + STM32F1 + STM32F3", not "STM32F013")

@algesten
Copy link
Author

@Dirbaio thanks for explaining.

I have pushed an update where I try to merge the changes into f013.rs. Hopefully it's closer to what you want.

PllSource::HSE => (Pllsrc::HSE_DIV_PREDIV, unwrap!(hse)),
#[cfg(stm32f107)]
PllSource::HSE => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit of a lie, if you set PllSource::HSE the source can end up being either HSE or PLL2. I'd suggest adding a new variant PllSource::PLL2 instead, and removing the prediv1_src field from config.

let in_freq = src_freq / pll.prediv;

#[cfg(stm32f107)]
let in_freq = src_freq / (pll.prediv.to_bits() + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be needed, / pll.prediv does the right thing automatically (it's a Div impl generated in build.rs, it's not dividing by the bit value)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, for this to work add Prediv1, Prediv2 here.

"Pllp" | "Pllq" | "Pllr" | "Pllm" | "Plln" => true,
. Then you can directly divide instead of having to do .to_bits().

#[cfg(stm32f107)]
let pll2freq = config.pll2.map(|pll2| {
let prediv2 = config.prediv2.unwrap_or(PllPreDiv::DIV1);
let in_freq = hse.unwrap() / (prediv2.to_bits() + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as prediv1, divide directly instead of doing .to_bits(). (see below)

#[cfg(stm32f107)]
pub prediv1_src: Option<PreDiv1Src>,
#[cfg(stm32f107)]
pub prediv2: Option<PllPreDiv>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this sholdn't be Option, because you can't "not enable prediv2".

#[cfg(stm32f107)]
pub i2s2_src: Option<I2s2src>,
#[cfg(stm32f107)]
pub i2s3_src: Option<I2s2src>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for these.

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

Successfully merging this pull request may close these issues.

2 participants