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

Add mechanism to configure UART source clock #1416

Merged
merged 8 commits into from
Apr 17, 2024
Merged

Add mechanism to configure UART source clock #1416

merged 8 commits into from
Apr 17, 2024

Conversation

playfulFence
Copy link
Contributor

closes #1181

@playfulFence playfulFence self-assigned this Apr 9, 2024
SergioGasquez
SergioGasquez reviewed Apr 10, 2024
View reviewed changes
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Changes mostly LGTM! Just left a few comments, thanks for taking care of this!

esp-hal/src/uart.rs Outdated Show resolved Hide resolved
esp-hal/src/uart.rs Outdated Show resolved Hide resolved
esp-hal/src/uart.rs Outdated Show resolved Hide resolved
esp-hal/src/uart.rs Show resolved Hide resolved
@playfulFence playfulFence force-pushed the features/uart_clockconf branch 2 times, most recently from 46af4e9 to 93984f6 Compare April 10, 2024 12:24
JurajSadel
JurajSadel reviewed Apr 10, 2024
View reviewed changes
esp-hal/src/uart.rs Outdated Show resolved Hide resolved
@playfulFence playfulFence force-pushed the features/uart_clockconf branch 3 times, most recently from b55d22a to 2426e2a Compare April 10, 2024 15:31
jessebraham
jessebraham requested changes Apr 10, 2024
View reviewed changes
esp-hal/src/uart.rs Show resolved Hide resolved
@SergioGasquez
Copy link
Member

Resolved the conflicts with main to make it mergeable in case it gets the required approvals before next week when @playfulFence starts working again.

@playfulFence
Copy link
Contributor Author

It's rebased now

cc: @SergioGasquez

@jessebraham jessebraham self-assigned this Apr 11, 2024
@playfulFence
Copy link
Contributor Author

Rebased again

JurajSadel
JurajSadel approved these changes Apr 12, 2024
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 thanks for working on this!

bjoernQ
bjoernQ reviewed Apr 16, 2024
View reviewed changes
esp-hal/src/soc/esp32/mod.rs
@@ -28,6 +28,8 @@ pub(crate) mod constants {

pub const SOC_DRAM_LOW: u32 = 0x3FFA_E000;
pub const SOC_DRAM_HIGH: u32 = 0x4000_0000;

pub const REF_TICK: fugit::HertzU32 = fugit::HertzU32::MHz(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if these shouldn't go into clock::Clocks - on the other hand we already have a few frequencies here.

Most probably okay for now and we should rework the whole clocks thing since it's not very maintainable as it looks now 🤷‍♂️

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 thought about that too, but then I saw that it's used like twice in the whole driver (including this PR), so it was decided to leave it in the soc module for now. However, yes, after clocks thing is reworked, I guess we should consider having all the clock frequencies there

PS: Second place where REF_TICK freq is used:
https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/clock/clocks_ll/esp32.rs#L6

@SergioGasquez SergioGasquez mentioned this pull request Apr 16, 2024
Closed
SergioGasquez
SergioGasquez approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this!

jessebraham
jessebraham approved these changes Apr 17, 2024
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.

LGTM, thank you for this!

@jessebraham jessebraham added this pull request to the merge queue Apr 17, 2024
Merged via the queue into esp-rs:main with commit 6f91367 Apr 17, 2024
22 checks passed
@playfulFence playfulFence mentioned this pull request Jul 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjoernQ bjoernQ bjoernQ left review comments

@JurajSadel JurajSadel JurajSadel approved these changes

@SergioGasquez SergioGasquez SergioGasquez approved these changes

@jessebraham jessebraham jessebraham approved these changes

Assignees

@jessebraham jessebraham

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a mechanism to configure the source clock for UART
5 participants
@playfulFence @SergioGasquez @jessebraham @bjoernQ @JurajSadel