Fix HSEM Implementation

[Legorooj]

This PR fixes the HSEM implementation for STM32 MCUs. The current implementation does not appear to work on wb55 boards at minimum, and as such needs testing where possible.

List of chips that the current implementation is confirmed to not function correctly on:

• WB55
• WL5x
• WLEx
• h723,h725,h730,h733,h735
• h742,h743,h753,h750
• h745,h755,h747,h757
• h7a3,h7b3,h7b0

Chips that the new implementation is confirmed to work on:

• WB55
• WL5x
• WLEx
• h723,h725,h730,h733,h735
• h742,h743,h753,h750
• h745,h755,h747,h757
• h7a3,h7b3,h7b0

This implementation should provide full functionality on all STM MCUs which have a Hardware Semaphore feature.

[Dirbaio]

thanks for working on this! :)

With the trusted label CI will run HIL tests. I can also give you a token so you can cargo run in the HIL test farm locally so you don't have to wait for CI to build stuff. Ping me on Matrix if you want it.

[Dirbaio]

Have you seen #4106 ? seems you guys are working on the same thing.

[Legorooj]

@Dirbaio ha, no, I hadn't, though looking now, that PR will only fix the WB55 where as this one aims to fix the implementation for all of the MCUs supported.

[ckrenslehner]

I did not see it, sorry. I am fine with any implementation :-)

[ckrenslehner]

Is there a reason why you moved your hsem code out into a separate file and rename the mod.rs into old.rs?

[ckrenslehner]

@Dirbaio ha, no, I hadn't, though looking now, that PR will only fix the WB55 where as this one aims to fix the implementation for all of the MCUs supported.

This was not my intention. I kept the values for the h7 mcus as they were, but I moved the RLR values into a separate enum. Looking at your code, you added more h7 variants, though. :-)
e3034a7#diff-e2b9c0e56c77ee17673e3aa131d3a98e608d3fd12a0c57eed4108b3a86107bcbR73

This is out of inspiration of the stm32-data crate, but I am not sure if this is worth the hassle to really move such things into the data crate and add mcu specific hsem yaml files

[Legorooj]

Is there a reason why you moved your hsem code out into a separate file and rename the mod.rs into old.rs?

HSEM doesn't need multiple files, therefore hsem/mod.rs is slightly more confusing than hsem.rs in my opinion. Additionally, there's no consistency within Embassy for one or the other of these, as there are multiple modules in both patterns. If @Dirbaio has a preference, I am happy to make it match, but if not, then a single hsem.rs file is clearer imo.

[Legorooj]

I'd like input regarding the H7 variants for HSEM @Dirbaio and @ckrenslehner. I cannot seem to get them working, no matter what I try the results do not match what the datasheets say they should. I've even checked against STM's official HAL & my implementation semantically matches. As such, which do you think is the better option:

1. Commit current implementation (a little housekeeping is needed before this happens). H7 MCUs with HSEM should just fail, I could also maybe add a note in the docs for h7 that it is known to have issues.
2. Remove support for H7 CPUs entirely, via features.
3. If you have suggestions?

[ckrenslehner]

Can you explain more about the datasheet mismatch or what exactly is not working? I probably have no access to a H7 so unfortunately I cannot test myself

[ckrenslehner]

Looking through the manual of the h7 and your code, it should work.
As commented, the onestep should IMO check the uppermost bit for the lock status.

Have you tested running some cube code on a h7 target? Unfortunately, I do not have a h7 to test with

[Legorooj]

Apologies for the delay on response.

Have you tested running some cube code on a h7 target? Unfortunately, I do not have a h7 to test with

Yes, see the latest ci/build. I've got a bunch of debug statements in there, you should be able to read those and track my confusion!

Looking through the manual of the h7 and your code, it should work. As commented, the onestep should IMO check the uppermost bit for the lock status.

To which comment are you referring?

[ckrenslehner]

Should the match condition really be (_, true, 0)?
reg.lock() returns true if locked and false if not locked, right?

[ckrenslehner]

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);
}

HSEM_R_LOCK = (0x1UL << (31U))
LL_HSEM_COREID = (0x4U << (8U))

So the top 1 is checked which corresponds to reg.lock()

[Legorooj]

Should the match condition really be (_, true, 0)? reg.lock() returns true if locked and false if not locked, right?

You would think that, however the datasheet itself doesn't mention it, and my WB55 board returns reg.lock() == false even though the lock is successful - I know this because I need to lock HSEM 5 in order for Wireless & USB stacks to not fight over clocks, and they both function perfectly.

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.

[ckrenslehner]

Okay.. thats odd, because I can report the opposite.
I was making my HSEM PR only, because I want to fully add the low-power feature for the stm32wb55. Unfortunately I did not see your PR (sorry again).

See #4116.
This code does successfully lock and unlock Sem3 and Sem4. And the match expression required lock to be true.

I actually had to change the original match (from the code ours PRs are based on) because there was false which did not work for me.

See
https://github.com/embassy-rs/embassy/pull/4106/files#diff-e2b9c0e56c77ee17673e3aa131d3a98e608d3fd12a0c57eed4108b3a86107bcbL109-L112

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?

[Legorooj]

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?

[ckrenslehner]

wb55rg too. as mentioned, I could provide you a small sample application where it enters stop mode via locking/unlocking the semaphores

[ckrenslehner]

I added a low_power example to the stm32wb55 showing the behavior of the hsem with trace output.
See this commit

You can run it on your own by executing cargo r --bin low_power --features low-power in the stm32wb examples dir

My output shows this:

29.429840 TRACE Polling Sem3 until granted
└─ embassy_stm32::rcc::low_power::stm32wb_configure_clocks_enter_stop_mode::{closure#0} @ C:\Users\ChristianKrenslehner\Software\fork\embassy\embassy-stm32\src\rcc\low_power.rs:15
29.429840 TRACE RLR (bits: 0x80000400) - lock: true, coreid: 4, procid: 0
└─ embassy_stm32::hsem::{impl#4}::one_step_lock @ C:\Users\ChristianKrenslehner\Software\fork\embassy\embassy-stm32\src\hsem\mod.rs:206
29.429840 TRACE Locked successfully (coreid: 4, procid: 0)
└─ embassy_stm32::hsem::{impl#4}::one_step_lock @ C:\Users\ChristianKrenslehner\Software\fork\embassy\embassy-stm32\src\hsem\mod.rs:220
29.429840 TRACE Sem3 granted
└─ embassy_stm32::rcc::low_power::stm32wb_configure_clocks_enter_stop_mode::{closure#0} @ C:\Users\ChristianKrenslehner\Software\fork\embassy\embassy-stm32\src\rcc\low_power.rs:17

As you can see, the topmost bit is set when the semaphore is locked successfully :-)

[ckrenslehner]

@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.

[ckrenslehner]

Not part of your PR, but shoud be make process_id and Option? It feels a little awkward to just pass 0 if you locked it with one_step_lock before. Maybe we could pass None instead and set it to 0 inside in such a case

[Legorooj]

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 process_id to enter - e.g. if you used one_step_lock - the default should be 0".

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 0 is the correct value.

[ckrenslehner]

Yeah thats true

[ckrenslehner]

Does it make sense to introduce a semaphore ID enum for MCUs as here in my PR?
Maybe it saves others some time when looking for possible ID values

[Legorooj]

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.

[ckrenslehner]

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

[ckrenslehner]

@Legorooj if you find the time, you could try adding true in the match (to check for the uppermust lock bit) and a full RLR content log to your code and push it.
If I understand correctly, you can run your code on the test-cluster this way?

So maybe we gain further evidence why this PR might not work.

[ckrenslehner]

To which comment are you referring?

Sorry, I thought I submitted the review.
To this comment: #4092 (comment)

My bad, sorry again.

[ckrenslehner]

Yes, see the latest ci/build. I've got a bunch of debug statements in there, you should be able to read those and track my confusion!

Ok, so the actual core ID value is 0 even though it should be 3. That's odd.
Can you print the whole RLR and R register?

I just wonder if there is anything non zero inside

[ckrenslehner]

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

impl crate::rcc::SealedRccPeripheral for peripherals::HSEM {
    fn frequency() -> crate::time::Hertz {
        unsafe {
            unwrap ! (crate :: rcc :: get_freqs () . hclk3 . to_hertz () , "peripheral '{}' is configured to use the '{}' clock, which is not running. \
                    Either enable it in 'config.rcc' or change 'config.rcc.mux' to use another clock" , "HSEM" , "HCLK3")
        }
    }
    const RCC_INFO: crate::rcc::RccInfo = unsafe {
        crate::rcc::RccInfo::new(
            Some((12u8, 19u8)),
            (20u8, 19u8),
            None,
            #[cfg(feature = "low-power")]
            crate::rcc::StopMode::Stop1,
        )
    };
}

[Legorooj]

Good thought, I'll explore this tomorrow if I have time.