Unify differences of I2S hardware via metadata, add C5 and C61 I2S support#5483
Unify differences of I2S hardware via metadata, add C5 and C61 I2S support#5483bugadani merged 9 commits intoesp-rs:mainfrom
Conversation
playfulFence
changed the title
|
/hil full --tests i2s |
|
Triggered full HIL run for #5483. Run: https://github.com/esp-rs/esp-hal/actions/runs/25173460536 Status update: ❌ HIL (full) run failed (conclusion: failure). |
|
HIL failure in CI doesn't seem to be related to the changes, runners might have some problems 🤔 |
| #[cfg(not(i2s_bck_divider_in_conf))] | ||
| self.regs().rx_conf1().modify(|_, w| unsafe { | ||
| w.rx_bck_div_num() | ||
| .bits((clock_settings.bclk_divider - 1) as u8) | ||
| }); | ||
| #[cfg(i2s_bck_divider_in_conf)] | ||
| self.regs().rx_conf().modify(|_, w| unsafe { | ||
| w.rx_bck_div_num() | ||
| .bits((clock_settings.bclk_divider - 1) as u8) | ||
| }); |
There was a problem hiding this comment.
Could this be a PAC change?
There was a problem hiding this comment.
No, unfortunately, these seem to be different HW layouts really 😞
| #[serde(default)] | ||
| /// Whether MSB shift fields live in the main TX/RX_CONF registers. | ||
| /// Another option is CONF1 registers. | ||
| msb_shift_in_conf: bool, |
There was a problem hiding this comment.
These warrant a v3, perhaps.
There was a problem hiding this comment.
Meeeh 🤔
I think version covers pretty much enough of HW differences... These look more like weird field-placement quirk inside the v2 family (directly related to the earlier review though), I don't think it warrants a separate v3 tbh...
What might make sense though is to unite bck_divider_in_conf and msb_shift_in_conf as they describe the same chips from v2 gen
There was a problem hiding this comment.
I tend to agree with @bugadani -the TRMs at least show even more than three variants of I2S_DATE_REG values
| - C5 and C61: Enable RTC timekeeping (#5449) | ||
| - C5 and C61: I2S support (#5483) | ||
| - More I2S flags for HW unification (#5483) |
There was a problem hiding this comment.
I don't think we need to maintain a changelog for esp-metadata. This is purely intended to generate esp-metadata-generated, really, and we can consider stopping to release it, even.
There was a problem hiding this comment.
Do you want me to revert these 2 entries or let's roll with them one last time?
There was a problem hiding this comment.
We can keep them but the changelog is full of holes already :)
playfulFence
force-pushed
the
unify_periphs
branch
from
78f9e0f to
b6367f8
Compare
| #[cfg_attr(not(feature = "unstable"), allow(unused))] | ||
| pub(crate) mod constants { | ||
| /// The clock frequency for the I2S peripheral in Hertz. | ||
| pub const I2S_SCLK: u32 = 160_000_000; |
There was a problem hiding this comment.
I wonder if these constants should go into esp-metadata (for the I2S driver)
There was a problem hiding this comment.
Not SCLK, we should be able to query this from the clock tree.
playfulFence
force-pushed
the
unify_periphs
branch
from
b6367f8 to
ec0e8f6
Compare
| "dma_channel" => { | ||
| cfg(any(esp32, esp32s2)) => "DMA_I2S0", | ||
| cfg(not(any(esp32, esp32s2))) => "DMA_CH0" | ||
| cfg(i2s_version = "1") => "DMA_I2S0", |
There was a problem hiding this comment.
maybe checking dma_kind = "pdma" / dma_kind = "gdma" would be more "correct"
There was a problem hiding this comment.
it won't scale well for P4, unfortunately
There was a problem hiding this comment.
To clarify, this works here, but since the P4 has two independent GDMAs with different peripheral assignment, the same scheme won't work for SPI and others - so we'll need something else, going forward.
bjoernQ
left a comment
bjoernQ
left a comment
There was a problem hiding this comment.
I guess this is a good start for now - we'll see how things work out with the next chip I guess
github-actions
Bot
added
the
merge-conflict
|
New commits in main have made this PR unmergeable. Please resolve the conflicts. |
playfulFence
force-pushed
the
unify_periphs
branch
from
a7b2871 to
1c19985
Compare
github-actions
Bot
removed
the
merge-conflict
3 participants
closes #5482
closes #5172
closes #5415
Testing
HIL tests