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

Derive SAI blocks for 1176 from SAI1 #45

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

SpinFast
Copy link
Contributor

@SpinFast SpinFast commented Dec 17, 2023

All SAI instances can derive their register blocks from SAI1. While not
every instance has multiple tx/rx lines and fifos the register layout
can be reused for all SAI instances. The mcux sdk actually does this
by casting the instance address to I2S_Type* which is a common struct
with a common register layout.

@mciantyre
Copy link
Member

When I locally generate the RAL, I don't see any changes to the Rust code. There's a CI job that confirms this, too. If the SVDs were the root cause, I would have expected there to be a change in the Rust source code as of these SVD updates.

I'm not sure this will fix the issue. Could you check my notes here? All links are as of the 0.5.3 tag.

The 1176 (CM7) has two different SAI modules: sai, and sai1.

imxrt-ral/src/imxrt1176_cm7.rs

Lines 4542 to 4629 in bb0088d

pub mod sai {
#[doc = "SAI"]
pub const SAI2: *const RegisterBlock = 0x4040_8000 as *const RegisterBlock;
#[doc = "SAI"]
pub const SAI3: *const RegisterBlock = 0x4040_c000 as *const RegisterBlock;
#[doc = "SAI"]
pub const SAI4: *const RegisterBlock = 0x40c4_0000 as *const RegisterBlock;
#[path = "blocks/imxrt1176_cm4/sai.rs"]
mod blocks;
pub use blocks::*;
pub type Instance<const N: u8> = crate::Instance<RegisterBlock, N>;
pub type SAI2 = Instance<2>;
impl crate::private::Sealed for SAI2 {}
impl crate::Valid for SAI2 {}
impl SAI2 {
#[doc = r" Acquire a vaild, but possibly aliased, instance."]
#[doc = r""]
#[doc = r" # Safety"]
#[doc = r""]
#[doc = r" See [the struct-level safety documentation](crate::Instance)."]
#[inline]
pub const unsafe fn instance() -> Self {
Instance::new(SAI2)
}
}
pub type SAI3 = Instance<3>;
impl crate::private::Sealed for SAI3 {}
impl crate::Valid for SAI3 {}
impl SAI3 {
#[doc = r" Acquire a vaild, but possibly aliased, instance."]
#[doc = r""]
#[doc = r" # Safety"]
#[doc = r""]
#[doc = r" See [the struct-level safety documentation](crate::Instance)."]
#[inline]
pub const unsafe fn instance() -> Self {
Instance::new(SAI3)
}
}
pub type SAI4 = Instance<4>;
impl crate::private::Sealed for SAI4 {}
impl crate::Valid for SAI4 {}
impl SAI4 {
#[doc = r" Acquire a vaild, but possibly aliased, instance."]
#[doc = r""]
#[doc = r" # Safety"]
#[doc = r""]
#[doc = r" See [the struct-level safety documentation](crate::Instance)."]
#[inline]
pub const unsafe fn instance() -> Self {
Instance::new(SAI4)
}
}
#[doc = r" Returns the instance number `N` for a peripheral instance."]
pub fn number(rb: *const RegisterBlock) -> Option<u8> {
[(SAI2, 2), (SAI3, 3), (SAI4, 4)]
.into_iter()
.find(|(ptr, _)| core::ptr::eq(rb, *ptr))
.map(|(_, inst)| inst)
}
}
#[path = "."]
pub mod sai1 {
#[doc = "SAI"]
pub const SAI1: *const RegisterBlock = 0x4040_4000 as *const RegisterBlock;
#[path = "blocks/imxrt1176_cm4/sai1.rs"]
mod blocks;
pub use blocks::*;
pub type Instance<const N: u8> = crate::Instance<RegisterBlock, N>;
pub type SAI1 = Instance<{ crate::SOLE_INSTANCE }>;
impl crate::private::Sealed for SAI1 {}
impl crate::Valid for SAI1 {}
impl SAI1 {
#[doc = r" Acquire a vaild, but possibly aliased, instance."]
#[doc = r""]
#[doc = r" # Safety"]
#[doc = r""]
#[doc = r" See [the struct-level safety documentation](crate::Instance)."]
#[inline]
pub const unsafe fn instance() -> Self {
Instance::new(SAI1)
}
}
#[doc = r" Returns the instance number `N` for a peripheral instance."]
pub fn number(rb: *const RegisterBlock) -> Option<u8> {
core::ptr::eq(rb, SAI1).then_some(0)
}
}

The sai-without-1 does not have an array for TDR. It's just TDR0.

pub TDR0: crate::RWRegister<u32>,

The sai1 does have an array for TDR.

pub TDR: [crate::RWRegister<u32>; 4usize],

@mciantyre
Copy link
Member

mciantyre commented Dec 18, 2023

Here's a few "don't want to deal with this" ideas that would be OK with me.

Within the HAL, and when the 1170 feature is active, remap ral::sai1 to ral::sai. That might allow the driver to build for 1170 MCUs, but it only supports the sole multi-channel SAI instance. We could consider some tricks; when the 1170 build is active, provide an unsafe constructor to go from single-channel SAI instances to multi-channel SAI instances. The user promises "I won't touch those extra channels." We pointer cast register blocks inside the HAL. Not great as is, but could be a start for some kind of type- or runtime-safer approach.

Within the HAL, don't export SAI as a common driver. Pick your favorite MCU families and export it when that feature is active.

@SpinFast
Copy link
Contributor Author

I'd like to fix it, 1176 would be cool to have at some point perhaps.

I'll see what I can do to correct the svd with patching methods

@SpinFast
Copy link
Contributor Author

From what I can tell other parts provide a single block that has registers as arrays regardless of whether the instance has the full set of channels. That works for me. I tried patching it but svdtools is I don't think as helpful here as I would like. I'll consider the other options tomorrow.

@SpinFast SpinFast changed the title Update imxrt1176 svd files Derive SAI blocks for 1176 from SAI1 Dec 19, 2023
@SpinFast
Copy link
Contributor Author

The svdtools patch now generates ral stuff that looks a lot closer to the other parts as the registerblock gets reused for all instances

Copy link
Member

@mciantyre mciantyre 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 digging into this. I'll include this in a future breaking release (0.6) of imxrt-ral.

I'm pondering ways to hack something like this within the 0.5 series, but I haven't seen the path yet. If we figured this out, we could release the HAL's SAI driver within imxrt-hal 0.5 series and still fully support the 1170. Of course, we could always release the SAI driver in the HAL's 0.5 series without immediately supporting all MCUs.

It's also OK to update the HAL's dependency on the RAL to realize this change for SAI development.

All SAI instances can derive their register blocks from SAI1. While not
every instance has multiple tx/rx lines and fifos the register layout
can be reused for all SAI instances. The mcux sdk actually does this
by casting the instance address to I2S_Type* which is a common struct
with a common register layout.
@mciantyre mciantyre merged commit a2383c5 into imxrt-rs:master Jan 6, 2024
20 checks passed
@SpinFast SpinFast deleted the imxrt1170_updates branch February 6, 2024 19:52
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.

2 participants