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.

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

Sign conversion warnings #74

Open
HTRamsey opened this issue Oct 27, 2023 · 5 comments
Open

Sign conversion warnings #74

HTRamsey opened this issue Oct 27, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@HTRamsey
Copy link
Contributor

When using gcc 11.3 & 12.3, there are sign conversion warnings in armv7m_cachel1.h with most usages of __SCB_DCACHE_LINE_SIZE and __SCB_ICACHE_LINE_SIZE in that file.

@JonatanAntoni
Copy link
Member

Hi @holden-zenith,

Thanks for pointing this out. The cache maintenance functions convert forth and back between signed and unsigned which is probably causing your warnings. Would you like to contribute reworked versions of the functions that work for you?

Thanks,
Jonatan

@ReinhardKeil
Copy link
Contributor

I think the issue is the definition

__STATIC_FORCEINLINE void SCB_InvalidateICache_by_Addr (volatile void *addr, int32_t isize)
{
  #if defined (__ICACHE_PRESENT) && (__ICACHE_PRESENT == 1U)
    if ( isize > 0 ) {
       int32_t op_size = isize + (((uint32_t)addr) & (__SCB_ICACHE_LINE_SIZE - 1U));
      uint32_t op_addr = (uint32_t)addr /* & ~(__SCB_ICACHE_LINE_SIZE - 1U) */;

      __DSB();

      do {
        SCB->ICIMVAU = op_addr;             /* register accepts only 32byte aligned values, only bits 31..5 are valid */
        op_addr += __SCB_ICACHE_LINE_SIZE;
        op_size -= __SCB_ICACHE_LINE_SIZE;
      } while ( op_size > 0 );

      __DSB();
      __ISB();
    }
  #endif
}

See: https://arm-software.github.io/CMSIS_6/latest/Core/html/group__Icache__functions__m7.html#gaeb1a2bf181afcfb837ce0502e6bfa4fb

Changing this as below should solve the problem. But perhaps there is a timing reason for the previous code:

__STATIC_FORCEINLINE void SCB_InvalidateICache_by_Addr (volatile void *addr, uint32_t isize)
{
  #if defined (__ICACHE_PRESENT) && (__ICACHE_PRESENT == 1U)
    if ( isize > 0 ) {
      uint32_t op_size = isize + (((uint32_t)addr) & (__SCB_ICACHE_LINE_SIZE - 1U));
      uint32_t op_addr = (uint32_t)addr /* & ~(__SCB_ICACHE_LINE_SIZE - 1U) */;

      __DSB();

      while ( op_size >=  __SCB_ICACHE_LINE_SIZE ) {
        SCB->ICIMVAU = op_addr;             /* register accepts only 32byte aligned values, only bits 31..5 are valid */
        op_addr += __SCB_ICACHE_LINE_SIZE;
        op_size -= __SCB_ICACHE_LINE_SIZE;
      };

      __DSB();
      __ISB();
    }
  #endif
}

@JonatanAntoni
Copy link
Member

@holden-zenith, can you confirm the reworked code provided by Reinhard solves the issue your you while it is still performing correctly?

@HTRamsey
Copy link
Contributor Author

HTRamsey commented Nov 6, 2023

@JonatanAntoni well it definitely resolves the warning in that function which would also need to be applied to the others, but changing the do while to a while will obviously change the functionality depending on isize. Are you supposed to manually verify isize is >= __SCB_ICACHE_LINE_SIZE before calling this?

@JonatanAntoni
Copy link
Member

JonatanAntoni commented Nov 6, 2023

Good spot. Basically, the idea of the loop is that at least isize bytes gets invalidated. Effectively, cache maintenance can only happen in junks of __SCB_ICACHE_LINE_SIZE. So, an alternative implementation might be calculating the number of cache lines to be invalidated and use a for-loop based on the address only.
E.g.

__STATIC_FORCEINLINE void SCB_InvalidateICache_by_Addr (volatile void *addr, uint32_t isize)
{
  #if defined (__ICACHE_PRESENT) && (__ICACHE_PRESENT == 1U)
    const uint32_t op_start_addr = (uint32_t)addr /* & ~(__SCB_ICACHE_LINE_SIZE - 1U) */;
    const uint32_t op_end_addr = (uint32_t)addr + isize; 

    __DSB();

    for (uint32_t op_addr = op_start_addr; op_addr < op_end_addr; op_addr += __SCB_ICACHE_LINE_SIZE) {
      SCB->ICIMVAU = op_addr;             /* register accepts only 32byte aligned values, only bits 31..5 are valid */
    };

    __DSB();
    __ISB();
  #endif
}

A problem could result from an integer overflow in op_end_addr calculation.

@JonatanAntoni JonatanAntoni self-assigned this Jan 19, 2024
@JonatanAntoni JonatanAntoni added the bug Something isn't working label Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants