search

Nuclei-Software / nuclei-sdk

Nuclei RISC-V Software Development Kit
https://doc.nucleisys.com/nuclei_sdk
Apache License 2.0
117 stars 50 forks source link

SoC/gd32vf103: Fix deep sleep behavior in pmu_to_deepsleepmode #42

Closed gsauthof closed 2 years ago

gsauthof commented 2 years ago

While trying to make the MCU enter deep sleep mode I noticed that pmu_to_deepsleepmode() doesn't work, i.e. that deep sleep mode isn't entered.

I thus reviewed the code and noticed that the custom SLEEPVALUE RISC-V control and status register (CSR) isn't set as described in the GD32VF103 manual.

This change fixes this function and while at it I reviewed and fixed the other sleep/standby functions, as well.

As mentioned by the manual, when the MCU enters deep sleep mode the system clock is reset to IRC8M - and stays that way when it wakes up again. Thus, with the default system initialization (which sets the system clock to PLL 108 MHz) it's imperative to explicitly reconfigure the system clock to the old state.

Otherwise, peripherals like USART don't work anymore after leaving deep sleep.

In order to simplify this common use case I also changed the visibility of system_clock_config() from static to global such that user code can simply call it after pmu_to_deepsleepmode().

Ideally, pmu_to_deepsleepmode() would save the current clock configuration before entering deep sleep mode and if (and only if the old clock configuration wasn't already IRC8M) restore all the bits and pieces. However, this takes more effort to implement and perhaps is a little bit tricky to support all possible configurations.

I thought about changing system_clock_config() to weak linkage, i.e. to allow users to provide their own system clock initialization function in case they want to clock the MCU differently (e.g. just 8 MHz to save power).

I'm not sure, perhaps there is a better way for such customization.

fanghuaqi commented 2 years ago

Hi @gsauthof , I have checked in some source code to set SYSTEM_CLOCK and clock source using make variable SYSCLK and CLKSRC, see changes below:

Maybe it could match the requirements to change the system clock frequency during startup process, and you can revert changes in commit 634ffc5

Thanks

gsauthof commented 2 years ago

Thank you for the changes and the pointers. I think this is sufficient for customization and with that in place there is no need to make system_clock_config() weak.

I've replied to your review comment, above. Short summary: I think the extern change is still useful, as it allows for convenient system clock re-initialization after wake up from deep sleep.

fanghuaqi commented 2 years ago

Hi @gsauthof , maybe SystemInit function is enough for you in this case, it will do system init. Could you take a look at it?

gsauthof commented 2 years ago

Yes, I've looked at SystemInit(). The thing is that it does too much, i.e. these steps are redundant after a deep-sleep:

https://github.com/Nuclei-Software/nuclei-sdk/blob/1058d50cf6d01dcc6043d8ec895a0cba31d98081/SoC/gd32vf103/Common/Source/system_gd32vf103.c#L221-L244

I did a small test where my code stores the values of RCU_CTL, RCU_CFG0, RCU_CFG1 and RCU_INT before going into deep sleep, and after wake-up from deep-sleep it a) calls SystemInit() or b) calls system_clock_config(). It then compares current values of these registers with the backed up ones.

The results are: In both cases the register comparison doesn't find any differences.

This shows that the extra initialization done in SystemInit() isn't required after waking up from deep sleep.

FWIW, my test program also writes some stuff to UART which continues to work after wake-up in both cases. (it doesn't work if I leave out the system clock initialization, as expected)