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

C6 PMU startup configuration #974

Merged
merged 11 commits into from
Dec 15, 2023
Merged

C6 PMU startup configuration #974

merged 11 commits into from
Dec 15, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 26, 2023 •
edited
Loading

This PR implements PMU setup based on esp-idf v5.1.2.

I implemented PMU setup because I wasn't sure if this was the cause of my MCU not waking up. Well it isn't, but why not add the code to esp-hal :)

I wonder if much of this code should just be ran before main(), these are pre-main in esp-idf, too.

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@bugadani bugadani force-pushed the pmu branch 2 times, most recently from 6d8cd05 to 40d6931 Compare November 26, 2023 16:15
@bugadani bugadani marked this pull request as ready for review November 26, 2023 16:34
@bugadani bugadani force-pushed the pmu branch 3 times, most recently from b7dca07 to 3536d51 Compare November 27, 2023 09:33
bugadani
bugadani commented Nov 27, 2023
View reviewed changes
esp-hal-common/src/rtc_cntl/rtc/esp32c6.rs
macro_rules! hp_system_init {
($state:ident => $s:ident) => {
paste::paste! {
unsafe {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry for these macros. esp-idf just indexes into a register group with 3 (HP) or 2 (LP) groups. Unfortunately, in the PAC, the register field names are different for each power state(?) (they contain a bunch of the register's name for each field) and transmuting them would just be super confusing.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 27, 2023

From skimming over the code everything looks nice and tidy - probably not useful to compare it line by line with the C code.
Probably we "just" need to check if things still work with this and it should be fine - ideally also esp-wifi should still work when calling Rtc::new (I think there is at least a problem with the original ESP32 currently - reminds me I need to double check and create an issue for that)

@bugadani
Copy link
Contributor Author

bugadani commented Nov 27, 2023 •
edited
Loading

probably not useful to compare it line by line with the C code

Honestly, I've gone through this enough times and I still think there might be mistakes somewhere. But each register modification is hidden behind 2 levels of indirection and it's extremely annoying to compare.

Also, it is somewhat interesting that this PR tries to set the same values as esp-idf, but registers still end up with slightly different values in them, so there may be unintended differences.

I think it might be useful for someone to familiarize themselves with the basic flow, that way they can tell if the config values at least correspond to the values in esp-idf.

@bugadani bugadani force-pushed the pmu branch 5 times, most recently from e61908c to 1349c41 Compare December 5, 2023 17:22
@bugadani bugadani force-pushed the pmu branch 3 times, most recently from dff2bef to 0474bee Compare December 13, 2023 10:23
@bugadani
Copy link
Contributor Author

bugadani commented Dec 14, 2023 •
edited
Loading

So now that a new release cycle is going on, can we try and get this merged, or should I consider the PR in a permanent limbo until documentation becomes available?

@jessebraham @MabezDev @bjoernQ

@jessebraham
Copy link
Member

So now that a new release cycle is going on, can we try and get this merged, or should I consider the PR in a permanent limbo until documentation becomes available?

I have no problem merging this, just haven't had the time to review it.

@SergioGasquez @JurajSadel can one or both of you please make some time to look over this? The draft chapter for the C6's PMU is available in our internal chat in the Documentation channel.

@SergioGasquez
Copy link
Member

I tested a few examples and esp-wifi examples, and they were all working fine! Gone through the changes quickly, and they look good to me, probably tomorrow morning I'll take a deeper look at the changes!

@jessebraham jessebraham mentioned this pull request Dec 14, 2023
Closed
4 tasks
JurajSadel
JurajSadel reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

I went line-by-line with ESP-IDF so, hopefully, we haven't missed anything important here, haha
I left a comment where I'm not sure what we want to achieve, otherwise LGTM!

JurajSadel
JurajSadel approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

jessebraham
jessebraham approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this!

Also thank you for reviewing @bjoernQ and @JurajSadel, and thanks for testing @SergioGasquez!

@jessebraham jessebraham added this pull request to the merge queue Dec 15, 2023
Merged via the queue into esp-rs:main with commit 260c882 Dec 15, 2023
@bugadani bugadani deleted the pmu branch December 15, 2023 16:21
Volkalex28 pushed a commit to Volkalex28/esp-hal that referenced this pull request Feb 6, 2024
@bugadani @Volkalex28
C6 PMU startup configuration (esp-rs#974)
47593c4 
* Implement a bunch of missing startup code

* Extract peripheral address retrieval

* Clean up manual register manipulation

* Add missing PMU related setup

* Changelog

* Clean up revision check

* Fix build

* Add note about PMU setup code source

* Use macros to deduplicate hp/lp system setup

* Clean up a bit

* Initialize the correct register in modem_clock_hal_select_wifi_lpclk_source
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessebraham jessebraham jessebraham approved these changes

@JurajSadel JurajSadel JurajSadel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bugadani @bjoernQ @jessebraham @SergioGasquez @JurajSadel