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

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

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 marked this pull request as ready for review November 26, 2023 16:34
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

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
Copy link
Contributor Author

bugadani commented Dec 14, 2023

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!

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!

esp-hal-common/src/rtc_cntl/rtc/esp32c6.rs Show resolved Hide resolved
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!

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
17 checks passed
@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
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants