Skip to content

PSoC 6: rework sleep overrides by Cypress's debug macro #13976

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 17, 2020
Merged

PSoC 6: rework sleep overrides by Cypress's debug macro #13976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f0c98d
PSoC 6: remove debug sleep lock and hook from mbed_sdk_init
LDong-Arm Nov 27, 2020
b1da342
PSoC 6: enable/disable sleep modes based on CY_CFG_PWR_SYS_IDLE_MODE
LDong-Arm Nov 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions targets/TARGET_Cypress/TARGET_PSOC6/cy_sleep_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,16 @@

void hal_sleep(void)
{
// Noop, if the idle mode is active
#if !defined(CY_CFG_PWR_SYS_IDLE_MODE) || (CY_CFG_PWR_SYS_IDLE_MODE != CY_CFG_PWR_MODE_ACTIVE)
cyhal_syspm_sleep();
#endif
Copy link
Contributor Author

@LDong-Arm LDong-Arm Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: As the macro CY_CFG_PWR_SYS_IDLE_MODE is Cypress-specific, maybe it makes more sense to have that defined in cyhal_syspm_sleep() (Cypress SDK code) instead of the Mbed OS HAL implementation? In that case we wouldn't need any tricks at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ARMmbed/team-cypress Or, could you confirm if the macro is still being used by Cypress's debug tool as of today?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion.
@kyle-cypress please review this proposal and let us know if you'd rather change your SDK.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should change cyhal_syspm_sleep. The intent of CY_CFG_PWR_SYS_IDLE_MODE is to control how the RTOS idle task behaves, not to completely disable the sleep driver (even if the two wind up being very similar in practice).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current proposal is good and easy to follow so I would go with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of CY_CFG_PWR_SYS_IDLE_MODE is to control how the RTOS idle task behaves,

Well, as explained in the description, the idle overriding is a feature we provide to the application. Overriding it by drivers/BSPs would create a conflict if the application uses it.

}

void hal_deepsleep(void)
{
#if !defined(CY_CFG_PWR_SYS_IDLE_MODE) || (CY_CFG_PWR_SYS_IDLE_MODE == CY_CFG_PWR_MODE_DEEPSLEEP)

#if DEVICE_LPTICKER
// A running timer will block DeepSleep, which would normally be
// good because we don't want the timer to accidentally
Expand All @@ -41,9 +46,14 @@ void hal_deepsleep(void)
cy_us_ticker_stop();
cyhal_syspm_deepsleep();
cy_us_ticker_start();
#else
#else // DEVICE_LPTICKER
cyhal_syspm_sleep();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it is adding a lot of complexity to the hal_deepsleep implementation. Wouldn't it be simpler to keep the call to sleep_manager_lock_deep_sleep and let the sleep manager take care of figuring out whether to call hal_sleep or hal_deepsleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sleep_manager_lock_deep_sleep() as used before is fine in my opinion. I changed hal_deepsleep() for consistency with the macro-checks in hal_sleep(), but if it's overly complex I can revert this bit.

#endif // DEVICE_LPTICKER

#elif CY_CFG_PWR_SYS_IDLE_MODE == CY_CFG_PWR_MODE_SLEEP
cyhal_syspm_sleep();
#endif /* DEVICE_LPTICKER */
#endif // CY_CFG_PWR_SYS_IDLE_MODE == CY_CFG_PWR_MODE_ACTIVE
// Noop, if the idle mode is active
}

#endif /* DEVICE_SLEEP */
31 changes: 0 additions & 31 deletions targets/TARGET_Cypress/TARGET_PSOC6/mbed_overrides.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include "cyhal_hwmgr.h"
#include "cybsp.h"
#include "cy_mbed_post_init.h"
#include "mbed_power_mgmt.h"
#include "mbed_error.h"
#if MBED_CONF_RTOS_PRESENT
#include "rtos_idle.h"
Expand All @@ -35,22 +34,6 @@
#include "cy_serial_flash_qspi.h"
#endif /* defined(MBED_CONF_TARGET_XIP_ENABLE) */


#if (defined(CY_CFG_PWR_SYS_IDLE_MODE) && (CY_CFG_PWR_SYS_IDLE_MODE == CY_CFG_PWR_MODE_ACTIVE))
/*******************************************************************************
* Function Name: active_idle_hook
****************************************************************************//**
*
* Empty idle hook function to prevent the system entering sleep mode
* automatically any time the system is idle.
*
*******************************************************************************/
static void active_idle_hook(void)
{
/* Do nothing, so the rtos_idle_loop() performs while(1) */
}
#endif

MBED_WEAK void cy_mbed_post_bsp_init_hook(void)
{
/* By default, do nothing */
Expand Down Expand Up @@ -102,18 +85,4 @@ void mbed_sdk_init(void)
/* Enable global interrupts (disabled in CM4 startup assembly) */
__enable_irq();
#endif

#if defined (CY_CFG_PWR_SYS_IDLE_MODE)
/* Configure the lowest power state the system is allowed to enter
* based on the System Idle Power Mode parameter value in the Device
* Configurator. The default value is system deep sleep.
*/
#if (CY_CFG_PWR_SYS_IDLE_MODE == CY_CFG_PWR_MODE_ACTIVE)
rtos_attach_idle_hook(&active_idle_hook);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing this entirely and adding extra checks to hal_sleep to turn it into a no-op, could this be replaced by a call to sleep_manager_lock_sleep? That would be simpler and I believe it would produce more correct results from the sleep/deepsleep statistics mechanism when that is enabled.

Copy link
Contributor Author

@LDong-Arm LDong-Arm Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mbed OS only supports locking deep sleep, not sleep. This was probably the reason an idle hook was used instead. The locking of deep sleep ensures all features work:

mbed-os/platform/include/platform/mbed_power_mgmt.h

Lines 45 to 47 in e77b1d8

* Use locking/unlocking deepsleep for drivers that depend on features that
* are not allowed (=disabled) during the deepsleep. For instance, high frequency
* clocks.

I think in most use cases, the need to lock regular sleep is comparatively low.

@evedon @kjbracey-arm In terms of functionality/actual use cases, does it make sense to add a lock for regular sleep?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see why anyone would lock sleep, as it is very shallow mode (core system clock is disabled).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @0xc0170 said, locking normal sleep makes little sense in actual use cases, so we may not add that to the API.

#elif (CY_CFG_PWR_SYS_IDLE_MODE == CY_CFG_PWR_MODE_SLEEP)
sleep_manager_lock_deep_sleep();
#else
/* Deep sleep is default state */
#endif
#endif
}