-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix HSEM Implementation #4092
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
base: main
Are you sure you want to change the base?
Fix HSEM Implementation #4092
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
//! Hardware Semaphore (HSEM) | ||
const HSEM: crate::pac::hsem::Hsem = crate::pac::HSEM; | ||
|
||
/// Locking the HardwareSemaphore failed. | ||
#[derive(Debug)] | ||
pub struct HsemLockFailed; | ||
|
||
/// CPU Core. | ||
/// | ||
/// The enum values are identical to the bus master IDs defined for each chip family. | ||
/// | ||
/// On some chips, the Reference Manual calls this value MASTERID instead of COREID. | ||
#[repr(u8)] | ||
#[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
pub enum CoreId { | ||
#[cfg(any(stm32h7a3, stm32h7b3, stm32h7b0))] | ||
/// Cortex-M7, core 1. MASTERID = 1 | ||
Core0 = 1, | ||
|
||
#[cfg(any( | ||
stm32h723, stm32h725, stm32h730, stm32h733, stm32h735, stm32h742, stm32h743, stm32h745, stm32h747, stm32h750, | ||
stm32h753, stm32h755, stm32h757, | ||
))] | ||
/// Cortex-M7, core 1. MASTERID = 3 | ||
Core0 = 3, | ||
|
||
#[cfg(any(stm32wb, stm32wl))] | ||
/// Cortex-M4, core 1 | ||
Core0 = 4, | ||
|
||
#[cfg(not(any(stm32wb, stm32wl, stm32h7a3, stm32h7b3, stm32h7b0)))] | ||
/// Cortex-M4, core 2 | ||
Core1 = 1, | ||
|
||
#[cfg(any(stm32wb, stm32wl))] | ||
/// Cortex M0+, core 2 | ||
Core1 = 8, | ||
} | ||
|
||
impl CoreId { | ||
#[cfg(any( | ||
all(stm32wl, not(feature = "_core-cm0p")), | ||
all(not(stm32wl), any(feature = "_core-cm7", not(feature = "_core-cm4"))), | ||
))] | ||
const fn current() -> CoreId { | ||
CoreId::Core0 | ||
} | ||
|
||
#[cfg(any(all(not(stm32wl), feature = "_core-cm4"), all(stm32wl, feature = "_core-cm0p")))] | ||
const fn current() -> CoreId { | ||
CoreId::Core1 | ||
} | ||
|
||
fn as_index(&self) -> usize { | ||
match self { | ||
CoreId::Core0 => 0, | ||
#[cfg(not(any(stm32h7a3, stm32h7b3, stm32h7b0)))] | ||
CoreId::Core1 => 1, | ||
} | ||
} | ||
} | ||
|
||
/// TODO | ||
pub struct HardwareSemaphore<'d> { | ||
_peri: crate::Peri<'d, crate::peripherals::HSEM>, | ||
} | ||
|
||
impl<'d> HardwareSemaphore<'d> { | ||
/// Create a new HardwareSemaphore instance. | ||
pub fn new(peri: crate::Peri<'d, crate::peripherals::HSEM>) -> Self { | ||
HardwareSemaphore { _peri: peri } | ||
} | ||
|
||
/// Locks the semaphore via the 2-step (write) lock procedure | ||
/// | ||
/// The two-step procedure consists of a write to lock the semaphore, followed by a read | ||
/// to check if the lock has been successful, carried out from the HSEM_Rx retgister. | ||
pub fn two_step_lock(&mut self, sem_id: u8, process_id: u8) -> Result<(), HsemLockFailed> { | ||
HSEM.r(sem_id as usize).write(|w| { | ||
w.set_procid(process_id); | ||
w.set_coreid(CoreId::current() as u8); | ||
w.set_lock(true); | ||
}); | ||
|
||
let reg = HSEM.r(sem_id as usize).read(); | ||
|
||
match ( | ||
reg.lock(), | ||
reg.coreid() == CoreId::current() as u8, | ||
reg.procid() == process_id, | ||
) { | ||
(true, true, true) => Ok(()), | ||
v => { | ||
error!("{}: {}", v, CoreId::current() as u8); | ||
Err(HsemLockFailed) | ||
} | ||
} | ||
} | ||
|
||
/// Locks the semaphore via the 1-step (read) lock procedure | ||
/// | ||
/// The one-step procedure consists of a read to lock and check the semaphore in a single | ||
/// step, carried out from the HSEM_RLRx register. | ||
pub fn one_step_lock(&mut self, sem_id: u8) -> Result<(), HsemLockFailed> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to introduce a semaphore ID enum for MCUs as here in my PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did think about this, and I really like what you did with your PR, and meant to bring it up as a discussion point, because I feel if we could find a reference of different SEMIDs and their meanings for all the chips, it would be a good idea, however if not, it might be better to just link to the relevant documentation for each chip we can find docs for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the stm32wb is can be found in this page 21. I suppose there are similar documents for the other MCUs. I can look if I find something There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Legorooj if you find the time, you could try adding So maybe we gain further evidence why this PR might not work. |
||
let reg = HSEM.rlr(sem_id as usize).read(); | ||
|
||
match (reg.lock(), reg.coreid() == CoreId::current() as u8, reg.procid()) { | ||
(_, true, 0) => Ok(()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the match condition really be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the wb55 hal drivers there is the following code: /**
* @brief Get the lock with 1-step lock.
* @rmtoll RLR LOCK LL_HSEM_1StepLock
* @rmtoll RLR COREID LL_HSEM_1StepLock
* @rmtoll RLR PROCID LL_HSEM_1StepLock
* @param HSEMx HSEM Instance.
* @param Semaphore Semaphore number. Value between Min_Data=0 and Max_Data=31
* @retval 1 lock fail, 0 lock successful or already locked by same core
*/
__STATIC_INLINE uint32_t LL_HSEM_1StepLock(HSEM_TypeDef *HSEMx, uint32_t Semaphore)
{
return ((HSEMx->RLR[Semaphore] != (HSEM_R_LOCK | LL_HSEM_COREID)) ? 1UL : 0UL);
}
So the top There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You would think that, however the datasheet itself doesn't mention it, and my WB55 board returns The same issue appeared to occur when I ran it through CI too, on one of the early runs. Feel free to see if you get different behaviour on your WB55. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay.. thats odd, because I can report the opposite. See #4116. I actually had to change the original match (from the code ours PRs are based on) because there was You can test it yourself using the low-power executor if you want to. If it helps I can provide you a little example project. Further looking at the lock algorithm for entering the low power mode of this document page 23. There is clearly a "check if locked" step. So there needs to be an indication in the RLR which actually signals a successful lock.. How would you know if sem is granted otherwise? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By manually checking if it's locked, I guess. Which wb55 are you using, exactly? All my tests have been on wb55rg, maybe there's a difference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wb55rg too. as mentioned, I could provide you a small sample application where it enters stop mode via locking/unlocking the semaphores There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a You can run it on your own by executing My output shows this:
As you can see, the topmost bit is set when the semaphore is locked successfully :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Legorooj if I can provide further help, ping me :-) You mentioned your successful use of USB. I wonder if you implemented the algorithm of page 26. Inside this flow chart you are also supposed to poll Sem5 until granted. |
||
v => { | ||
error!("{}: Current: {} Actual: {}", v, CoreId::current() as u8, reg.coreid()); | ||
Err(HsemLockFailed) | ||
} | ||
} | ||
} | ||
|
||
/// Unlocks the semaphore | ||
/// | ||
/// Unlocking a semaphore is a protected process, to prevent accidental clearing by an AHB | ||
/// bus core ID or by a process not having the semaphore lock right. | ||
pub fn unlock(&mut self, sem_id: u8, process_id: u8) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not part of your PR, but shoud be make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I understand & would normally agree with this, considering it is an embedded context I wonder whether it would not be better to simply specify in the doc comment that "if you are not sure which I say this because someone learning to program these devices in Rust will either be coming from a C/C++ background, or be a beginner having to heavily rely on C/C++ resources, which would specify that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah thats true |
||
HSEM.r(sem_id as usize).write(|w| { | ||
w.set_procid(process_id); | ||
w.set_coreid(CoreId::current() as u8); | ||
w.set_lock(false); | ||
}); | ||
} | ||
|
||
/// Unlocks all semahpores. | ||
/// | ||
/// All semaphores locked by a COREID can be unlocked at once by using the HSEM_CR register. | ||
/// Write COREID and correct KEY value in HSEM_CR. All locked semaphores with a matching | ||
/// COREID are unlocked, and may generate an interrupt if enabled. | ||
pub fn unlock_all(&mut self, key: u16, core_id: u8) { | ||
HSEM.cr().write(|w| { | ||
w.set_key(key); | ||
w.set_coreid(core_id); | ||
}) | ||
} | ||
|
||
/// Checks if the semaphore is locked. | ||
pub fn is_semaphore_locked(&self, sem_id: u8) -> bool { | ||
HSEM.r(sem_id as usize).read().lock() | ||
} | ||
|
||
/// Sets the clear (unlock) key | ||
pub fn set_clear_key(&mut self, key: u16) { | ||
HSEM.keyr().modify(|w| w.set_key(key)); | ||
} | ||
|
||
/// Gets the clear (unlock) key | ||
pub fn get_clear_key(&mut self) -> u16 { | ||
HSEM.keyr().read().key() | ||
} | ||
|
||
/// Sets the interrupt enable bit for the semaphore. | ||
pub fn enable_interrupt(&mut self, core_id: CoreId, sem_x: usize, enable: bool) { | ||
HSEM.ier(core_id.as_index()).modify(|w| w.set_ise(sem_x, enable)); | ||
} | ||
|
||
/// Clears the interrupt flag for the semaphore. | ||
pub fn clear_interrupt(&mut self, core_id: CoreId, sem_x: usize) { | ||
HSEM.icr(core_id.as_index()).write(|w| w.set_isc(sem_x, false)); | ||
} | ||
|
||
/// Gets the interrupt flag for the semaphore. | ||
pub fn is_interrupt_active(&mut self, core_id: CoreId, sem_x: usize) -> bool { | ||
HSEM.isr(core_id.as_index()).read().isf(sem_x) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// required-features: hsem | ||
#![no_std] | ||
#![no_main] | ||
|
||
#[path = "../common.rs"] | ||
mod common; | ||
|
||
use common::*; | ||
use embassy_executor::Spawner; | ||
use embassy_stm32::hsem::HardwareSemaphore; | ||
|
||
#[embassy_executor::main] | ||
async fn main(_spawner: Spawner) { | ||
let p: embassy_stm32::Peripherals = init(); | ||
|
||
let mut hsem = HardwareSemaphore::new(p.HSEM); | ||
|
||
if hsem.is_semaphore_locked(5) { | ||
defmt::panic!("Semaphore 5 already locked!") | ||
} | ||
|
||
hsem.one_step_lock(5).unwrap(); | ||
hsem.two_step_lock(1, 0).unwrap(); | ||
|
||
info!("Test OK"); | ||
cortex_m::asm::bkpt(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to call
rcc::enable_and_reset::<T>();
here as well to make sure, that the AHB clock is enabled?In this case for the HSEM of course.
At least there is some hsem related code in
_generated.rs
.Here for the stm32wb55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought, I'll explore this tomorrow if I have time.