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

Add basic esp32p4 support #2466

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

Conversation

playfulFence
Copy link
Contributor

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

For now, there're still some TODOs left. They'll be incrementally removed as p4 support grows. For example, for now some of checks in CI are off, some code sections and so on.

related to #2285

Testing

advanced_serial example, some more basic tests like:

//% CHIPS: esp32p4

#![no_std]
#![no_main]

use esp_backtrace as _;
use esp_hal::prelude::*;

#[entry]
fn main() -> ! {
    esp_println::println!("Hello world, from ESP32-P4 :)");

    loop {}
}

@jessebraham
Copy link
Member

I don't think we want to include this in this release, so converting to draft for now.

@@ -285,21 +310,36 @@ pub struct ChannelRxImpl<C: GdmaChannel>(C);
impl<C: GdmaChannel> crate::private::Sealed for ChannelRxImpl<C> {}

impl<C: GdmaChannel> ChannelRxImpl<C> {
#[cfg(not(esp32p4))]
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I'd prefer not touching DMA for the P4.

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 think there's nothing critical to have minor changes in terms of registers/fields management there, I think it can stay there

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's nothing critical to have minor changes in terms of registers/fields management there, I think it can stay there

The P4 doesn't even have a single unified GDMA, it has two different ones. I doubt we'll use any of these additions in this form.

use crate::peripherals::SYSTEM;

/// Peripherals which can be enabled via `PeripheralClockControl`.
///
/// This enum represents various hardware peripherals that can be enabled
/// by the system's clock control. Depending on the target device, different
/// peripherals will be available for enabling.
// FIXME: This enum needs to be public because it's exposed via a bunch of traits, but it's not
// useful to users.
// NOTE: This enum needs to be public because it's exposed via a bunch of
Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo this, FIXME is actually highlighted if you use the Todo Tree vscode plugin.

Copy link
Member

@jessebraham jessebraham Nov 6, 2024

Choose a reason for hiding this comment

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

FIXME/TODO/NOTE are all pretty standard, plugin I use at least highlights all of those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, this change has no relevance for P4, and a FIXME better signals that something needs to be changed around these parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be done

esp-metadata/devices/esp32p4.toml Outdated Show resolved Hide resolved
examples/src/bin/advanced_serial.rs Outdated Show resolved Hide resolved
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks! I have some initial comments, mostly around the maintainability of adding more support in the future - we don't want to spend a bunch of time working on something only to realise the function that should be doing something is stubbed out to do nothing :D.

I'll dig out my P4 rev1 board and try the examples soon.

esp-hal/src/clock/mod.rs Outdated Show resolved Hide resolved
@@ -94,24 +94,41 @@ static RX_WAKERS: [AtomicWaker; CHANNEL_COUNT] = [const { AtomicWaker::new() };
impl<C: GdmaChannel> crate::private::Sealed for ChannelTxImpl<C> {}

impl<C: GdmaChannel> ChannelTxImpl<C> {
#[cfg(not(esp32p4))]
Copy link
Member

Choose a reason for hiding this comment

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

Can we just cfg away DMA changes (i.e remove gdma from metadata)? We won't have a need for it for some time, and DMA on P4 is going to be complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if that works: #2466 (comment)

gdma will be temporarily removed from metadata

esp-hal/src/interrupt/riscv.rs Show resolved Hide resolved
esp-hal/src/soc/esp32p4/gpio.rs Show resolved Hide resolved
esp-hal/src/soc/esp32p4/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32p4/mod.rs Show resolved Hide resolved
esp-hal/src/system.rs Show resolved Hide resolved
esp-hal/src/uart.rs Show resolved Hide resolved
@playfulFence
Copy link
Contributor Author

Regarding dma stuff: currently we have a lot of drivers which are tightly coupled to dma. UART is one of them, create_peripheral macro as well, so as soon as you exclude gdma from metadata, you'll immediately get an unknown amount of errors.

@bugadani
Copy link
Contributor

bugadani commented Nov 6, 2024

Yeah let's have those errors, I'll move PeripheralMarker to a better place. Alternatively, you could define an axi_gdma and ahb_gdma symbol pair, and cfg-enable dma for those without gdma/pdma parts.

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.

4 participants