SPI and deep sleep fixes for FUTURE_SEQUANA target. #8905
Conversation
- fix to allow using only a subset of pins for SPI communication - added missing conter increment in spi_master_block_write()
|
@ARMmbed/mbed-os-hal Please review |
| @@ -374,7 +374,7 @@ static cy_en_syspm_status_t serial_pm_callback(cy_stc_syspm_callback_params_t *p | |||
|
|
|||
| case CY_SYSPM_CHECK_FAIL: | |||
| /* Enable the UART to operate */ | |||
| Cy_SCB_UART_Enable(obj->base); | |||
| //Cy_SCB_UART_Enable(obj->base); | |||
There was a problem hiding this comment.
This is the point #4 in description.
There was a problem hiding this comment.
Ups... you're right, it doesn't make sense.
| @@ -384,7 +384,7 @@ static cy_en_syspm_status_t serial_pm_callback(cy_stc_syspm_callback_params_t *p | |||
|
|
|||
| case CY_SYSPM_AFTER_TRANSITION: | |||
| /* Enable the UART to operate */ | |||
| Cy_SCB_UART_Enable(obj->base); | |||
| //Cy_SCB_UART_Enable(obj->base); | |||
| @@ -26,16 +29,34 @@ void hal_sleep(void) | |||
| Cy_SysPm_Sleep(CY_SYSPM_WAIT_FOR_INTERRUPT); | |||
| } | |||
|
|
|||
|
|
|||
| #define DEEP_SLEEP_ENTER_TIMEOUT_US 260000 | |||
There was a problem hiding this comment.
This potentially violates this requirement:
* * the wake-up time should be less than 10 ms - Verified by deepsleep_lpticker_test().
There was a problem hiding this comment.
My understanding is that this requirement assumes the hardware is in a state ready to enter deep sleep mode when the hal_deepsleep() function is called. But this seems not always to be satisfied and my code tries to implement a workaround. Ideally, HAL API should be able to return an error code when it is not able to enter deep sleep mode as requested.
There was a problem hiding this comment.
If not able to enter deepsleep, it should return immediately
| static volatile uint32_t in_progress = 0; | ||
|
|
||
| // SysPm API is not re-entrant, so make sure not to enter it multiple times. | ||
| if (core_util_atomic_incr_u32(&in_progress, 1) == 1) { |
There was a problem hiding this comment.
hal_deepsleep() does not need to be re-entrant. It's called from a critical section (not doing that yields undefined behaviour as it doesn't protect against early Iinterrupts.
There was a problem hiding this comment.
Actually, HAL requirements are not well documented in Mbed.
I can remove it, should I?
There was a problem hiding this comment.
I'll go further - you must ;)
Agreed that we should review doc
|
@lrusinowicz please take a look at Don's review comments |
|
@lrusinowicz will you able to address deep sleep comments today? This is scheduled for rc3 that we would like to finalize today |
- fixed feature rename (from DEVICE_LOWPOWERTIMER to DEVICE_LPTICKER) in all missed places - disabled power manager callback for serial driver
lrusinowicz
force-pushed
the
spi_fixes
branch
from
e609154 to
ea5dcca
Compare
|
I have edited my commits to address the comments and removed the changes in sleep_API.c. Comments has pointed I went completely wrong with these, so it should be OK now. |
|
@donatieng could you re-review please? |
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
Proper handling of (NC) in the SPI initialization to allow only a subset SPI pins to be used for communication (e.g. no SS pin or tx-only SPI mode).
DEVICE_LOWPOWERTIMER was renamed into DEVICE_LPTICKER in one of the prevoius Mbed OS releases. This was missed in a few places in Sequana code.
Better handling of deep sleep entry in hal_deepsleep()
Cypress Power Manager deep sleep callback removed from HAL serial driver. Callbacks are not strictly necessary, serial works anyway, but having them breaks assumed timing dependencies in HAL sleep tests - serial callback handling delays deep sleep mode entry more than allowed in these tests.
Pull request type