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.

Sign up for GitHub

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

Jump to bottom

cpu/stm32: Fix stm clock configuration #18797

Merged
merged 6 commits into from
Nov 15, 2022
Merged

cpu/stm32: Fix stm clock configuration #18797

merged 6 commits into from
Nov 15, 2022

Conversation

MrKevinWeiss
Copy link
Contributor

Contribution description

This splits the stm32 clock tree in Kconfig making it a bit easier to look at and manage.
It exposes the clock speeds allowing enough information to match the make resolution.
It also introduces some HAVE_ symbols to make it a bit easier to manage multiple CPU_SERIES and/ors.

There is a small fix to the make resolution for the stm32f303*, some boards were not running at the max possible clock speed.

Testing procedure

Green murdock with NIGHTLY=1

Self testing based off #18771 with

export TEST_BOARDS=$(FEATURES_REQUIRED=cpu_stm32 make -C tests/pkg_tinyusb_cdc_msc info-boards-supported --no-print-directory)
for board in $TEST_BOARDS; do echo $board && /bin/bash -c "source .murdock; JOBS=8 compile tests/pkg_tinyusb_cdc_msc ${board}:gnu" | grep mismatch; done;

as well (since this should fix the USB errors)

Issues/PRs references

Follow up #18771

@github-actions github-actions bot added Area: boards Area: Board ports Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 25, 2022
@MrKevinWeiss
Copy link
Contributor Author

Note that I still have to clean up commits...

Teufelchen1
Teufelchen1 reviewed Oct 25, 2022
View reviewed changes
boards/nucleo-wl55jc/Kconfig Outdated
@@ -22,10 +22,16 @@ config BOARD_NUCLEO_WL55JC
select HAS_PERIPH_TIMER
select HAS_PERIPH_UART

select BOARD_HAS_HSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Misses # Clock configuration comment as seen on other boards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as it is defined in the source "$(RIOTBOARD)/common/nucleo64/Kconfig", maybe that gets more explicit later...

Teufelchen1
Teufelchen1 reviewed Oct 25, 2022
View reviewed changes
cpu/stm32/dist/clk_conf/clk_conf.c Outdated
@@ -454,7 +454,8 @@ int main(int argc, char **argv)
printf("#define CLOCK_CORECLOCK (%uU)\n", coreclock);
printf("/* 0: no external high speed crystal available\n"
" * else: actual crystal frequency [in Hz] */\n"
"#define CLOCK_HSE (%uU)\n", pll_src ? pll_in : 0);
"#ifndef CONFIG_CLOCK_HSE
#define CONFIG_CLOCK_HSE (%uU)\n", pll_src ? pll_in : 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting looks odd?

Teufelchen1
Teufelchen1 reviewed Oct 25, 2022
View reviewed changes
cpu/stm32/stm32_line.mk Outdated
@@ -56,6 +56,7 @@ ifeq (F,$(STM32_TYPE))
ifneq (, $(filter $(STM32_ROMSIZE), 6 8))
CPU_LINE = STM32F$(STM32_MODEL)x8
else ifneq (,$(filter $(STM32_ROMSIZE), B C))
$(info ************ TEST VERSION ************)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's its purpose?

@MrKevinWeiss
Copy link
Contributor Author

Commits have been cleaned up, probably supersedes #18771, all in one PR splitting is a bit of a pain.

@MrKevinWeiss
Copy link
Contributor Author

Note: I should not make the CLOCK_HSE value selectable and remove the CLOCK_HSE_*MHZ options.

@MrKevinWeiss
Copy link
Contributor Author

If the nightly murdock results are good I would remove the REMOVEME commit and say it is ready...

@MrKevinWeiss
Copy link
Contributor Author

Anyone else willing to review? This is needed to fix nightlies? if @aabadie doesn't have any strong opinions then maybe we can move with this?

@MrKevinWeiss MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 1, 2022
@MrKevinWeiss MrKevinWeiss mentioned this pull request Nov 1, 2022
Closed
@riot-ci
Copy link

riot-ci commented Nov 1, 2022 •
edited
Loading

Murdock results

✔️ PASSED

464f57b boards: Remove PLL overrides in kconfig

Success Failures Total Runtime
117848 0 117848 01h:35m:51s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@MrKevinWeiss
Copy link
Contributor Author

So this indicates that it does fix the intended failures on the nightlies. Can I squash and someone take a look?

@leandrolanzieri
Copy link
Contributor

Go ahead

@MrKevinWeiss
treewide/stm32: Make CLOCK_HS* configurable
17cce01
@MrKevinWeiss
cpu/stm: Fix clock settings F303*
641d539
@MrKevinWeiss
boards/stm32f7508-dk: Add STM32_ETH in kconfig
fe5892d
@MrKevinWeiss
cpu/stm32/Kconfig: Fix help indent
ba4843b
@MrKevinWeiss
cpu/stm32/kconfig: rework clock tree
28aed3c 
This splits up the clock configs.
It allows CPU_FAM based file sourcing and also common CPU_FAMs.
The dependancies are also included in wildcards would be used for the CPU_FAM macro.
This should be much more readable.
This also takes into account the HSE speeds in order to match the make/header resolution.
Some hidden symbols were added to make sorting many CPU_SERIES dependencies easier.
@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Nov 3, 2022
@MrKevinWeiss
boards: Remove PLL overrides in kconfig
464f57b 
Since we know the HSE speed, manual overrides are not needed anymore
@MrKevinWeiss
Copy link
Contributor Author

All passing...

@MrKevinWeiss
Copy link
Contributor Author

Now that #18787 is merged, this is the only remaining fix needed for nightlies.

maribu
maribu approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. Changes look good to me.

Testing doesn't scale to all of the STM32 boards, so I kind of fear this will break some board. But just letting this sitting here for all eternity while nightlies remain failing is not really an option. So let's merge this and fix the fallout later when the bug reports come in.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 15, 2022
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 15, 2022
@MrKevinWeiss
Copy link
Contributor Author

Thanks, this is mostly just matching the kconfig to the header dependency resolution, so low risk from the "production" side. I noticed a few things that were corrected though.

I also tested against all stm boards with the compile_like_murdock.py script that is PRed as well. Not all apps but the ones that would be affected by USB and non-USB clock contraints.

@maribu maribu merged commit 35149bd into RIOT-OS:master Nov 15, 2022
@MrKevinWeiss MrKevinWeiss deleted the pr/fixstmclk branch November 15, 2022 12:05
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Teufelchen1 Teufelchen1 Teufelchen1 left review comments

@maribu maribu maribu approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
Release 2023.01
Development

Successfully merging this pull request may close these issues.
7 participants
@MrKevinWeiss @riot-ci @leandrolanzieri @maribu @Teufelchen1 @aabadie @kaspar030