Skip to content
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

ARMv8M: Ensure SysTick timer is disabled when modifying SysTick regs #660

Closed
wants to merge 2 commits into from
Closed

ARMv8M: Ensure SysTick timer is disabled when modifying SysTick regs #660

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7acd88f
portable/ARMv8M: Ensure SysTick timer is disabled when modifying SysT…
paulbartell Apr 6, 2023
897e9a5
Address PR comments.
paulbartell Apr 10, 2023
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
97 changes: 72 additions & 25 deletions portable/ARMv8M/non_secure/port.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
#if ( configUSE_TICKLESS_IDLE == 1 )
__attribute__( ( weak ) ) void vPortSuppressTicksAndSleep( TickType_t xExpectedIdleTime )
{
uint32_t ulReloadValue, ulCompleteTickPeriods, ulCompletedSysTickDecrements, ulSysTickDecrementsLeft;
uint32_t ulCtrlRegValue, ulReloadValue, ulCompleteTickPeriods, ulCompletedSysTickDecrements, ulSysTickDecrementsLeft;
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable names should be written out in full, not abbreviated (as per the other variables declared here).

TickType_t xModifiableIdleTime;

/* Make sure the SysTick reload value does not overflow the counter. */
Expand Down Expand Up @@ -422,7 +422,7 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
* is accounted for as best it can be, but using the tickless mode will
* inevitably result in some tiny drift of the time maintained by the
* kernel with respect to calendar time. */
portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT );
portNVIC_SYSTICK_CTRL_REG &= ~( portNVIC_SYSTICK_ENABLE_BIT );
Copy link
Contributor

Choose a reason for hiding this comment

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

The original code:

  • writes 0 to the ENABLE bit, disabling the timer, which is the desired behaviour.
  • writes 0 to the systick interrupt enable bit, preventing a systick interrupt from becoming pended should the systick value count down to zero.
  • writes 0 to the COUNTFLAG bit, which leaves the COUNTFLAG unchanged, which is necessary as it is checked later to determine if the systick value counted to zero while its interrupt was disabled.
  • writes 0 to the CLKSOURCE bit, the effect of which is implementation defined, so should be reconsidered.

The new code:

  • performs a read-modify-write operation, so reads the COUNTFLAG, potentially clearing it, which I belive introduced a bug as the COUNTFLAG is checked later in the code.

Therefore I think the original code is (more) correct, and the updated code is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RichardBarry The remaining bits of SYSTICK_CTRL are labeled RES0. According to the arm glossary it is implementation-defined whether or not writes or zero or one to these bits have any side effects.

Given this, the read-modify-write operations better comply with ARM's recommendations in the Armv8-M Architecture Reference Manual and Glossary.


/* Use the SysTick current-value register to determine the number of
* SysTick decrements remaining until the next tick interrupt. If the
Expand Down Expand Up @@ -458,8 +458,7 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
/* Set the new reload value. */
portNVIC_SYSTICK_LOAD_REG = ulReloadValue;

/* Clear the SysTick count flag and set the count value back to
* zero. */
/* Clear the SysTick count flag and set the count value back to 0. */
portNVIC_SYSTICK_CURRENT_VALUE_REG = 0UL;

/* Restart SysTick. */
Expand Down Expand Up @@ -497,17 +496,29 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
__asm volatile ( "dsb" );
__asm volatile ( "isb" );

/* Disable the SysTick clock without reading the
* portNVIC_SYSTICK_CTRL_REG register to ensure the
* portNVIC_SYSTICK_COUNT_FLAG_BIT is not cleared if it is set. Again,
* the time the SysTick is stopped for is accounted for as best it can
/*
* Read the contents of the portNVIC_SYSTICK_CTRL_REG register into
* ulCtrlRegValue to ensure portNVIC_SYSTICK_COUNT_FLAG_BIT is taken
* into account later on.
*/
ulCtrlRegValue = portNVIC_SYSTICK_CTRL_REG;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a race condition here? Reading portNVIC_DYDTICK_CTRL_REG clears the portNVIC_SYSTICK_COUNT_FLAG_BIT, which might have otherwise been set between this line executing and the second line down where the saved value is written back to the control register.

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 race condition is addressed on line 511.


/* Disable the SysTick timer. */
ulCtrlRegValue &= ~( portNVIC_SYSTICK_ENABLE_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Update ulCtrlRegValue */
ulCtrlRegValue |= portNVIC_SYSTICK_CTRL_REG;
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 value of the register is re-read to prevent a race condition while portNVIC_SYSTICK_ENABLE_BIT is still set.


/*
* The time the SysTick is stopped for is accounted for as best it can
* be, but using the tickless mode will inevitably result in some tiny
* drift of the time maintained by the kernel with respect to calendar
* time*/
portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT );
* time.
*/

/* Determine whether the SysTick has already counted to zero. */
if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
if( ( ulCtrlRegValue & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
{
uint32_t ulCalculatedLoadValue;

Expand Down Expand Up @@ -568,34 +579,61 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
portNVIC_SYSTICK_LOAD_REG = ( ( ulCompleteTickPeriods + 1UL ) * ulTimerCountsForOneTick ) - ulCompletedSysTickDecrements;
}

/* Restart SysTick so it runs from portNVIC_SYSTICK_LOAD_REG again,
/*
* Restart SysTick so it runs from portNVIC_SYSTICK_LOAD_REG again,
* then set portNVIC_SYSTICK_LOAD_REG back to its standard value. If
* the SysTick is not using the core clock, temporarily configure it to
* use the core clock. This configuration forces the SysTick to load
* use the core clock. This configuration forces the SysTick to load
* from portNVIC_SYSTICK_LOAD_REG immediately instead of at the next
* cycle of the other clock. Then portNVIC_SYSTICK_LOAD_REG is ready
* to receive the standard value immediately. */
* cycle of the external clock. Then portNVIC_SYSTICK_LOAD_REG is ready
Copy link
Contributor

Choose a reason for hiding this comment

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

I belive the other clock can also be internal - if a peripheral time is considered internal - but I may be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would you prefer the term "implementation-defined clock?"

Copy link
Contributor Author

@paulbartell paulbartell Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The ARM documentation refers to this as the "IMPLEMENTATION DEFINED external reference clock."

* to receive the standard value immediately.
*/

portNVIC_SYSTICK_CURRENT_VALUE_REG = 0UL;
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT | portNVIC_SYSTICK_INT_BIT | portNVIC_SYSTICK_ENABLE_BIT;

#if ( portNVIC_SYSTICK_CLK_BIT_CONFIG == portNVIC_SYSTICK_CLK_BIT )
{
portNVIC_SYSTICK_LOAD_REG = ulTimerCountsForOneTick - 1UL;

/* Enable SysTick */
portNVIC_SYSTICK_CTRL_REG |= portNVIC_SYSTICK_ENABLE_BIT;
}
#else
{
/* The temporary usage of the core clock has served its purpose,
* as described above. Resume usage of the other clock. */
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT | portNVIC_SYSTICK_INT_BIT;
/* Temporarily switch to core clock */
ulCtrlRegValue |= portNVIC_SYSTICK_CLK_BIT;
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Enable SysTick */
ulCtrlRegValue |= portNVIC_SYSTICK_ENABLE_BIT;
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Read portNVIC_SYSTICK_CTRL_REG into ulCtrlRegValue */
ulCtrlRegValue = portNVIC_SYSTICK_CTRL_REG;

if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
/* Disable SysTick */
ulCtrlRegValue &= ~( portNVIC_SYSTICK_ENABLE_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Update ulCtrlRegValue */
ulCtrlRegValue |= portNVIC_SYSTICK_CTRL_REG;

/* Switch back to external clock. */
ulCtrlRegValue &= ~( portNVIC_SYSTICK_CLK_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

if( ( ulCtrlRegValue & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
{
/* The partial tick period already ended. Be sure the SysTick
* counts it only once. */
portNVIC_SYSTICK_CURRENT_VALUE_REG = 0;
}

portNVIC_SYSTICK_LOAD_REG = ulTimerCountsForOneTick - 1UL;
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT | portNVIC_SYSTICK_ENABLE_BIT;

/* Enable SysTick */
ulCtrlRegValue |= ( portNVIC_SYSTICK_ENABLE_BIT | portNVIC_SYSTICK_INT_BIT );
Copy link
Contributor

Choose a reason for hiding this comment

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

Code sequence needs considering carefully as it's potentially using an outdated ulCtrlRegValue value.

portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;
}
#endif /* portNVIC_SYSTICK_CLK_BIT_CONFIG */

Expand All @@ -620,13 +658,22 @@ __attribute__( ( weak ) ) void vPortSetupTimerInterrupt( void ) /* PRIVILEGED_FU
}
#endif /* configUSE_TICKLESS_IDLE */

/* Stop and reset the SysTick. */
portNVIC_SYSTICK_CTRL_REG = 0UL;
/* Stop the SysTick timer, clear the value. */
portNVIC_SYSTICK_CTRL_REG &= ~( portNVIC_SYSTICK_ENABLE_BIT );
portNVIC_SYSTICK_CURRENT_VALUE_REG = 0UL;

/* Configure SysTick to interrupt at the requested rate. */
/* Configure SysTick to interrupt at the requested rate with the request clock source. */
portNVIC_SYSTICK_LOAD_REG = ( configSYSTICK_CLOCK_HZ / configTICK_RATE_HZ ) - 1UL;
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT | portNVIC_SYSTICK_ENABLE_BIT;

/* Set or clear the portNVIC_SYSTICK_CLK_BIT as required. */

#if ( portNVIC_SYSTICK_CLK_BIT_CONFIG == portNVIC_SYSTICK_CLK_BIT )
paulbartell marked this conversation as resolved.
Show resolved Hide resolved
portNVIC_SYSTICK_CTRL_REG |= portNVIC_SYSTICK_CLK_BIT;
#else /* portNVIC_SYSTICK_CLK_BIT_CONFIG == 0 */
portNVIC_SYSTICK_CTRL_REG &= ~( portNVIC_SYSTICK_CLK_BIT_CONFIG );
#endif /* portNVIC_SYSTICK_CLK_BIT_CONFIG */

portNVIC_SYSTICK_CTRL_REG |= ( portNVIC_SYSTICK_ENABLE_BIT | portNVIC_SYSTICK_INT_BIT );
}
/*-----------------------------------------------------------*/

Expand Down
97 changes: 72 additions & 25 deletions portable/GCC/ARM_CM23/non_secure/port.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
#if ( configUSE_TICKLESS_IDLE == 1 )
__attribute__( ( weak ) ) void vPortSuppressTicksAndSleep( TickType_t xExpectedIdleTime )
{
uint32_t ulReloadValue, ulCompleteTickPeriods, ulCompletedSysTickDecrements, ulSysTickDecrementsLeft;
uint32_t ulCtrlRegValue, ulReloadValue, ulCompleteTickPeriods, ulCompletedSysTickDecrements, ulSysTickDecrementsLeft;
TickType_t xModifiableIdleTime;

/* Make sure the SysTick reload value does not overflow the counter. */
Expand Down Expand Up @@ -422,7 +422,7 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
* is accounted for as best it can be, but using the tickless mode will
* inevitably result in some tiny drift of the time maintained by the
* kernel with respect to calendar time. */
portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT );
portNVIC_SYSTICK_CTRL_REG &= ~( portNVIC_SYSTICK_ENABLE_BIT );

/* Use the SysTick current-value register to determine the number of
* SysTick decrements remaining until the next tick interrupt. If the
Expand Down Expand Up @@ -458,8 +458,7 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
/* Set the new reload value. */
portNVIC_SYSTICK_LOAD_REG = ulReloadValue;

/* Clear the SysTick count flag and set the count value back to
* zero. */
/* Clear the SysTick count flag and set the count value back to 0. */
portNVIC_SYSTICK_CURRENT_VALUE_REG = 0UL;

/* Restart SysTick. */
Expand Down Expand Up @@ -497,17 +496,29 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
__asm volatile ( "dsb" );
__asm volatile ( "isb" );

/* Disable the SysTick clock without reading the
* portNVIC_SYSTICK_CTRL_REG register to ensure the
* portNVIC_SYSTICK_COUNT_FLAG_BIT is not cleared if it is set. Again,
* the time the SysTick is stopped for is accounted for as best it can
/*
* Read the contents of the portNVIC_SYSTICK_CTRL_REG register into
* ulCtrlRegValue to ensure portNVIC_SYSTICK_COUNT_FLAG_BIT is taken
* into account later on.
*/
ulCtrlRegValue = portNVIC_SYSTICK_CTRL_REG;

/* Disable the SysTick timer. */
ulCtrlRegValue &= ~( portNVIC_SYSTICK_ENABLE_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Update ulCtrlRegValue */
ulCtrlRegValue |= portNVIC_SYSTICK_CTRL_REG;

/*
* The time the SysTick is stopped for is accounted for as best it can
* be, but using the tickless mode will inevitably result in some tiny
* drift of the time maintained by the kernel with respect to calendar
* time*/
portNVIC_SYSTICK_CTRL_REG = ( portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT );
* time.
*/

/* Determine whether the SysTick has already counted to zero. */
if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
if( ( ulCtrlRegValue & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
{
uint32_t ulCalculatedLoadValue;

Expand Down Expand Up @@ -568,34 +579,61 @@ PRIVILEGED_DATA static volatile uint32_t ulCriticalNesting = 0xaaaaaaaaUL;
portNVIC_SYSTICK_LOAD_REG = ( ( ulCompleteTickPeriods + 1UL ) * ulTimerCountsForOneTick ) - ulCompletedSysTickDecrements;
}

/* Restart SysTick so it runs from portNVIC_SYSTICK_LOAD_REG again,
/*
* Restart SysTick so it runs from portNVIC_SYSTICK_LOAD_REG again,
* then set portNVIC_SYSTICK_LOAD_REG back to its standard value. If
* the SysTick is not using the core clock, temporarily configure it to
* use the core clock. This configuration forces the SysTick to load
* use the core clock. This configuration forces the SysTick to load
* from portNVIC_SYSTICK_LOAD_REG immediately instead of at the next
* cycle of the other clock. Then portNVIC_SYSTICK_LOAD_REG is ready
* to receive the standard value immediately. */
* cycle of the external clock. Then portNVIC_SYSTICK_LOAD_REG is ready
* to receive the standard value immediately.
*/

portNVIC_SYSTICK_CURRENT_VALUE_REG = 0UL;
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT | portNVIC_SYSTICK_INT_BIT | portNVIC_SYSTICK_ENABLE_BIT;

#if ( portNVIC_SYSTICK_CLK_BIT_CONFIG == portNVIC_SYSTICK_CLK_BIT )
{
portNVIC_SYSTICK_LOAD_REG = ulTimerCountsForOneTick - 1UL;

/* Enable SysTick */
portNVIC_SYSTICK_CTRL_REG |= portNVIC_SYSTICK_ENABLE_BIT;
}
#else
{
/* The temporary usage of the core clock has served its purpose,
* as described above. Resume usage of the other clock. */
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT | portNVIC_SYSTICK_INT_BIT;
/* Temporarily switch to core clock */
ulCtrlRegValue |= portNVIC_SYSTICK_CLK_BIT;
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Enable SysTick */
ulCtrlRegValue |= portNVIC_SYSTICK_ENABLE_BIT;
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Read portNVIC_SYSTICK_CTRL_REG into ulCtrlRegValue */
ulCtrlRegValue = portNVIC_SYSTICK_CTRL_REG;

if( ( portNVIC_SYSTICK_CTRL_REG & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
/* Disable SysTick */
ulCtrlRegValue &= ~( portNVIC_SYSTICK_ENABLE_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

/* Update ulCtrlRegValue */
ulCtrlRegValue |= portNVIC_SYSTICK_CTRL_REG;

/* Switch back to external clock. */
ulCtrlRegValue &= ~( portNVIC_SYSTICK_CLK_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;

if( ( ulCtrlRegValue & portNVIC_SYSTICK_COUNT_FLAG_BIT ) != 0 )
{
/* The partial tick period already ended. Be sure the SysTick
* counts it only once. */
portNVIC_SYSTICK_CURRENT_VALUE_REG = 0;
}

portNVIC_SYSTICK_LOAD_REG = ulTimerCountsForOneTick - 1UL;
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT | portNVIC_SYSTICK_ENABLE_BIT;

/* Enable SysTick */
ulCtrlRegValue |= ( portNVIC_SYSTICK_ENABLE_BIT | portNVIC_SYSTICK_INT_BIT );
portNVIC_SYSTICK_CTRL_REG = ulCtrlRegValue;
}
#endif /* portNVIC_SYSTICK_CLK_BIT_CONFIG */

Expand All @@ -620,13 +658,22 @@ __attribute__( ( weak ) ) void vPortSetupTimerInterrupt( void ) /* PRIVILEGED_FU
}
#endif /* configUSE_TICKLESS_IDLE */

/* Stop and reset the SysTick. */
portNVIC_SYSTICK_CTRL_REG = 0UL;
/* Stop the SysTick timer, clear the value. */
portNVIC_SYSTICK_CTRL_REG &= ~( portNVIC_SYSTICK_ENABLE_BIT );
portNVIC_SYSTICK_CURRENT_VALUE_REG = 0UL;

/* Configure SysTick to interrupt at the requested rate. */
/* Configure SysTick to interrupt at the requested rate with the request clock source. */
portNVIC_SYSTICK_LOAD_REG = ( configSYSTICK_CLOCK_HZ / configTICK_RATE_HZ ) - 1UL;
portNVIC_SYSTICK_CTRL_REG = portNVIC_SYSTICK_CLK_BIT_CONFIG | portNVIC_SYSTICK_INT_BIT | portNVIC_SYSTICK_ENABLE_BIT;

/* Set or clear the portNVIC_SYSTICK_CLK_BIT as required. */

#if ( portNVIC_SYSTICK_CLK_BIT_CONFIG == portNVIC_SYSTICK_CLK_BIT )
portNVIC_SYSTICK_CTRL_REG |= portNVIC_SYSTICK_CLK_BIT;
#else /* portNVIC_SYSTICK_CLK_BIT_CONFIG == 0 */
portNVIC_SYSTICK_CTRL_REG &= ~( portNVIC_SYSTICK_CLK_BIT_CONFIG );
#endif /* portNVIC_SYSTICK_CLK_BIT_CONFIG */

portNVIC_SYSTICK_CTRL_REG |= ( portNVIC_SYSTICK_ENABLE_BIT | portNVIC_SYSTICK_INT_BIT );
}
/*-----------------------------------------------------------*/

Expand Down
Loading