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

Calculate STM32 clocks from the roots #1137

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Mar 5, 2024

There are hardcoded clock constants in each of **/board.hpp althought the required information to calculate the truth is available!

@TomSaw TomSaw mentioned this pull request Mar 5, 2024
@TomSaw TomSaw force-pushed the fix/calc_clocks_from_the_roots branch from 0752317 to 70fa1ce Compare March 7, 2024 16:48
@salkinium salkinium added this to the 2024q1 milestone Mar 8, 2024
@TomSaw TomSaw force-pushed the fix/calc_clocks_from_the_roots branch 2 times, most recently from 13a306f to 4de3164 Compare March 8, 2024 11:32
Copy link
Contributor Author

@TomSaw TomSaw left a comment

Choose a reason for hiding this comment

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

Request for a 1th review @salkinium.

@TomSaw TomSaw force-pushed the fix/calc_clocks_from_the_roots branch from 4de3164 to a22d568 Compare March 8, 2024 15:33
Copy link
Contributor Author

@TomSaw TomSaw left a comment

Choose a reason for hiding this comment

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

You are now empowered to control the clocks from PLL factors and prescalers for blue-pill and f411ve-disco.
Before diving deeper, please confirm my design decisions. Further improvements may be added to the Rcc namespace but let's be happy with pll's and prescalers for now.

Rcc::AhbPrescaler does not behave as expected

When selecting anything but Div1 or Div2 for Rcc::AhbPrescaler, the F103-blue-bill refuses to work. f411-disco keeps working but the Timer frequencies (scoped in hardware) don't scale like expected. I've matched the various intermediate clocks using static_assert with what is shown in CubeMX for the same configuration and seems to be all fine 🤨.

Propably, another side effect enters the scene when going beyond Div1, Div2 with Rcc::AhbPrescaler.

Because all the boards use an uninspired Div1 the glitch shall be tolerated for now.
I'll add a comment at least.

src/modm/board/disco_f411ve/board.hpp Show resolved Hide resolved
Comment on lines +30 to +42
template<Rcc::Apb1Prescaler Prescaler>
static consteval int
prescalerToValue()
{
switch (Prescaler)
{
case Rcc::Apb1Prescaler::Div1: return 1;
case Rcc::Apb1Prescaler::Div2: return 2;
case Rcc::Apb1Prescaler::Div4: return 4;
case Rcc::Apb1Prescaler::Div8: return 8;
case Rcc::Apb1Prescaler::Div16: return 16;
};
}
Copy link
Contributor Author

@TomSaw TomSaw Mar 8, 2024

Choose a reason for hiding this comment

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

To associate prescaler values to their registers, i've borrowed this construct from modm::platform::GeneralPurposeTimer::signalToChannel<...>(...) but using a switch operator.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but I'd rather use a normal constexpr function like uint8_t Rcc::Apb1Div(Rcc::Apb1Prescaler pre) {...}, then it can also be used at runtime if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course!

@salkinium
Copy link
Member

Hm, so what I would want is to move the common pattern of

// Enable clock source
Rcc::enableExternalCrystal();
// Enable PLL
Rcc::enablePll(Rcc::PllSource::ExternalCrystal, pllFactors);
Rcc::setFlashLatency<Frequency>();
Rcc::enableSystemClock(Rcc::SystemClockSource::Pll);
// Set Bus Prescaler
Rcc::setAhbPrescaler(Rcc::AhbPrescaler::Div1);
Rcc::setApb1Prescaler(Rcc::Apb1Prescaler::Div2);
Rcc::setApb2Prescaler(Rcc::Apb2Prescaler::Div1);

into a helper class defined in modm and configured by the user:

template< OneConfigStruct? >
struct BaseSystemClock
{
// Computes the basic frequencies with static_assert < max freq

// Adds all the peripheral frequencies via modm-devices?

// has a basic enable() function that does the above always correctly
};

The BSP would then inherit from that for their SystemClock and perhaps overwrite the one of the other thing for customization.

// simple case
using SystemClock = BaseSystemClock< ConfigStruct >;
// advances case
struct SystemClock : public BaseSystemClock< ConfigStruct >;
{
// customize
};

This would probably cover 90% of use-cases.
Maaaany (8) years ago I tried something like that before, but I did it way too complicated. So please do it simpler if you're interested in that.

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 11, 2024

That't some good guidiance! You're way more convenient with the STM architecture than me, so i blindly follow your suggestions in this topic.

// simple case
using SystemClock = BaseSystemClock< ConfigStruct >;
// advances case
struct SystemClock : public BaseSystemClock< ConfigStruct >;
{
// customize
};

Thats simple and users can opt in to vary their clocks per application.
I go for this.

@salkinium salkinium removed this from the 2024q1 milestone Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants