ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

NVIC->STIR should be SCB->STIR #404

Closed salkinium closed 6 years ago

salkinium commented 6 years ago

Hiiiii,

I was trying to place the core peripherals as objects into my linkerscript for better debugging like so:

MEMORY
{
    PERIPHERALS_CORE (rx)   : ORIGIN = 0xE0000000, LENGTH = 0x1FFFFFFF
}
SECTIONS
{
    .peripherals_core (NOLOAD):
    {
        . = 0xe100; KEEP(*(.peri_NVIC));
        . = 0xed00; KEEP(*(.peri_SCB));
    } > PERIPHERALS_CORE
}

When I got a bunch of collisions:

cannot move location counter backwards (from 00000000e000ef04 to 00000000e000ed00)

So I did a bit of research, and the culprit is the fact that you place the STIR register of the SCB in the NVIC type:

typedef struct
{
// ...
  __IOM uint8_t  IP[240U];               /*!< Offset: 0x300 (R/W)  Interrupt Priority Register (8Bit wide) */
        uint32_t RESERVED5[644U];
-  __OM  uint32_t STIR;                   /*!< Offset: 0xE00 ( /W)  Software Trigger Interrupt Register */
}  NVIC_Type;

Now, I understand the sentiment here, and yes it does sorta make sense to group the software trigger register with the NVIC, but that's not what the documentation says! For example for the Cortex-M4 TRM Section 4.1 (or any other Technical Reference Manual, Section on System Control):

screen shot 2018-08-13 at 23 46 37

The fix is to move the STIR register to the SCB_Type:

typedef struct
{
// ... 
  __IOM uint32_t CPACR;                  /*!< Offset: 0x088 (R/W)  Coprocessor Access Control Register */
+        uint32_t RESERVED1[29U];
+  __OM  uint32_t STIR;                   /*!< Offset: 0x100 ( /W)  Software Trigger Interrupt Register */
} SCB_Type;

Are we gonna fix this, considering this might break a lot of code? You don't have a __NVIC_TriggerIRQ() function that shims this register either.

cc @JonatanAntoni @ReinhardKeil

Also: Happy Issue #404!1!!

ReinhardKeil commented 6 years ago

Hi Niklas,

The definition of the NVIC structure has some history and IMHO we should not change it. What was the motivation to add peripherals to the linker script?

Reinhard

salkinium commented 6 years ago

When debugging with GDB, it can sometimes be difficult to access peripherals, due to the CMSIS implementation with CPP defines and GCC optimization flags (even with -g3). Depending on these circumstances, GDB may only "see" a peripheral in a certain context, which can make the debugging experience quite frustrating.

To aid this, we generate a linkerscript fragment and source file, which place the peripherals as real objects in the devices memory space. You can then access these peripherals inside GDB at any time, regardless of context and build profile setting:

(gdb) p/x *GPIOB
$1 = {
  MODER = 0xaa0280,
  OTYPER = 0x300,
  OSPEEDR = 0x2a00c0,
  PUPDR = 0x400100,
  IDR = 0xfd0,
  ODR = 0x100,
  BSRR = 0x0,
  LCKR = 0x0,
  AFR = {0x0, 0x7744}
}
salkinium commented 6 years ago

The concept is taken from here: https://github.com/maximevince/ucregview

salkinium commented 6 years ago

I've solved this differently now, so I don't get the linkerscript errors anymore. Nonetheless, this is still a bug, since the ARM documentation is correct by definition, not CMSIS. I'm still closing this issue, since I don't think it can be fixed anymore in CMSISv5. Maybe CMSISv6 ;-)

(gdb) p/x *NVIC
$1 = {
  ISER = {0x80000000, 0x81, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  RESERVED0 = {0x0 <repeats 24 times>},
  ICER = {0x80000000, 0x81, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  RSERVED1 = {0x0 <repeats 24 times>},
  ISPR = {0x800040, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  RESERVED2 = {0x0 <repeats 24 times>},
  ICPR = {0x800040, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  RESERVED3 = {0x0 <repeats 24 times>},
  IABR = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  RESERVED4 = {0x0 <repeats 56 times>},
  IP = {0xf0 <repeats 31 times>, 0xa0, 0xa0, 0xf0, 0xf0, 0xf0, 0xf0, 0xf0, 0xf0, 0xc0, 0xf0 <repeats 53 times>,
    0x0 <repeats 147 times>},
  RESERVED5 = {0x0 <repeats 516 times>, 0x410fc241, 0x400000, 0x8000000, 0xfa050300, 0x0, 0x310, 0x0, 0x0,
    0xf0000000, 0x0, 0x0, 0x0, 0x9, 0xe000edf8, 0xe000edf8, 0x0, 0x30, 0x200, 0x100000, 0x0, 0x100030, 0x0,
    0x1000000, 0x0, 0x1101110, 0x2112000, 0x21232231, 0x1111131, 0x1310132, 0x0, 0x0, 0x0, 0x0, 0x0, 0xf00000, 0x0,
    0x800, 0x0 <repeats 23 times>, 0x30003, 0x0, 0x0, 0x1000000, 0x0 <repeats 64 times>},
  STIR = 0x0
}

Reading the entire NVIC struct also reads the entire SCB as a side-effect (RESERVED5):

However, considering that accessing the reserved values may not be legal (and may cause Bus Faults), I think this approach of using the CMSIS struct for a "native" peripheral debug is not a good idea anyways. I see why uVision has it's own peripheral memory viewer via the SVD files, I'd be inclined to implement something similar as a GDB Python plugin instead of this.