Interrupt masking patch for Cortex-M3 ports

clang didn’t play nice with the inline assembly implementation of vPortClearInterruptMask.  It had incorrect constraints, which confused things.  I don’t think it liked the __attribute__((naked)), either.  Anyway, here’s the patch that fixes it – it’s “more correct” for gcc, too:
From 57f7991e165724df2488dc4f17d297e908dcf714 Mon Sep 17 00:00:00 2001
From: Carl Norum <carl@lytro.com>
Date: Thu, 9 May 2013 13:38:13 -0700
Subject: [PATCH 1/2] Fix inline assembly operand semantics in
 vPortClearInterruptMask.
---
 freertos/port/port.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/freertos/port/port.c b/freertos/port/port.c
index fc917d9..350eb28 100644
--- a/freertos/port/port.c
+++ b/freertos/port/port.c
@@ -300,13 +300,9 @@ __attribute__(( naked )) void vPortClearInterruptMask( unsigned long ulNewMaskVa
 {
    __asm volatile                                                  
    (                                                               
-       "   msr basepri, r0                                     n" 
-       "   bx lr                                               n" 
-       :::"r0"                                                     
+       "   msr basepri, %0                                     n" 
+       ::"r"(ulNewMaskValue):                                      
    );
-
-   /* Just to avoid compiler warnings. */
-   ( void ) ulNewMaskValue;
 }
 /*-----------------------------------------------------------*/

-- 
1.8.2
There was some unnecessary “funny business” going on in ulPortSetInterruptMask, too, thought it does work as is in my testing. Here’s a second patch that cleans that up, in case someone out there finds it useful:
From 929631c7e8593eef122b34b23a4c2a86db2ef635 Mon Sep 17 00:00:00 2001
From: Carl Norum <carl@lytro.com>
Date: Thu, 9 May 2013 13:41:28 -0700
Subject: [PATCH 2/2] Fix inline assembly operand semantics for
 ulPortSetInterruptMask.
---
 freertos/port/port.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/freertos/port/port.c b/freertos/port/port.c
index 350eb28..f7415b3 100644
--- a/freertos/port/port.c
+++ b/freertos/port/port.c
@@ -281,18 +281,18 @@ void vPortExitCritical( void )

 __attribute__(( naked )) unsigned long ulPortSetInterruptMask( void )
 {
+    unsigned long basepri;
    __asm volatile                                                      
    (                                                                   
-       "   mrs r0, basepri                                         n" 
-       "   mov r1, %0                                              n" 
+       "   mrs %0, basepri                                         n" 
+       "   mov r1, %1                                              n" 
        "   msr basepri, r1                                         n" 
-       "   bx lr                                                   n" 
-       :: "i" ( configMAX_SYSCALL_INTERRUPT_PRIORITY ) : "r0", "r1"    
+       :"=r"(basepri): "i" ( configMAX_SYSCALL_INTERRUPT_PRIORITY ) : "r1" 
    );

    /* This return will not be reached but is necessary to prevent compiler
    warnings. */
-   return 0;
+   return basepri;
 }
 /*-----------------------------------------------------------*/

-- 
1.8.2
If this forum isn’t the right place for patches, please let me know and I can forward them on to the right people. Thanks,
  • Carl Norum
    Lytro, Inc.
    carl@lytro.com

Interrupt masking patch for Cortex-M3 ports

Whoops – I included an early set of patches by accident.  These should probably not have the “__attribute__(( naked ))” included anymore.  New patches: vPortClearInterruptMask:
From 963241d50e572b5ec7a83dad90b7279e142b339b Mon Sep 17 00:00:00 2001
From: Carl Norum <carl@lytro.com>
Date: Thu, 9 May 2013 13:38:13 -0700
Subject: [PATCH 1/2] Fix inline assembly operand semantics in
 vPortClearInterruptMask.
---
 freertos/port/port.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/freertos/port/port.c b/freertos/port/port.c
index fc917d9..acac8c8 100644
--- a/freertos/port/port.c
+++ b/freertos/port/port.c
@@ -296,17 +296,13 @@ __attribute__(( naked )) unsigned long ulPortSetInterruptMask( void )
 }
 /*-----------------------------------------------------------*/

-__attribute__(( naked )) void vPortClearInterruptMask( unsigned long ulNewMaskValue )
+void vPortClearInterruptMask( unsigned long ulNewMaskValue )
 {
    __asm volatile                                                  
    (                                                               
-       "   msr basepri, r0                                     n" 
-       "   bx lr                                               n" 
-       :::"r0"                                                     
+       "   msr basepri, %0                                     n" 
+       ::"r"(ulNewMaskValue):                                      
    );
-
-   /* Just to avoid compiler warnings. */
-   ( void ) ulNewMaskValue;
 }
 /*-----------------------------------------------------------*/

-- 
1.8.2
and for ulPortSetInterruptMask:
From fbea744cb4f1fe65ba1f608266b09da4f4ab34be Mon Sep 17 00:00:00 2001
From: Carl Norum <carl@lytro.com>
Date: Thu, 9 May 2013 13:41:28 -0700
Subject: [PATCH 2/2] Fix inline assembly operand semantics for
 ulPortSetInterruptMask.
---
 freertos/port/port.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/freertos/port/port.c b/freertos/port/port.c
index acac8c8..c2fa998 100644
--- a/freertos/port/port.c
+++ b/freertos/port/port.c
@@ -279,20 +279,20 @@ void vPortExitCritical( void )
 }
 /*-----------------------------------------------------------*/

-__attribute__(( naked )) unsigned long ulPortSetInterruptMask( void )
+unsigned long ulPortSetInterruptMask( void )
 {
+    unsigned long basepri;
    __asm volatile                                                      
    (                                                                   
-       "   mrs r0, basepri                                         n" 
-       "   mov r1, %0                                              n" 
+       "   mrs %0, basepri                                         n" 
+       "   mov r1, %1                                              n" 
        "   msr basepri, r1                                         n" 
-       "   bx lr                                                   n" 
-       :: "i" ( configMAX_SYSCALL_INTERRUPT_PRIORITY ) : "r0", "r1"    
+       :"=r"(basepri): "i" ( configMAX_SYSCALL_INTERRUPT_PRIORITY ) : "r1" 
    );

    /* This return will not be reached but is necessary to prevent compiler
    warnings. */
-   return 0;
+   return basepri;
 }
 /*-----------------------------------------------------------*/

-- 
1.8.2

Interrupt masking patch for Cortex-M3 ports

Thanks for your input.  In my experimentation I had to remove the naked from ulPortSetInterruptMask()  which increased the number of instructions required for the function (more so at low compiler optimisation levels) which I don’t think is a good thing for a function called so often.  The same is not true for the ulPortClearInterruptMask() function though.  I think there are other optimisations that can be made here too, particularly changing the implementation to make more use of inlined asm code as that could be done with very few instructions and be very beneficial. Regards.

Interrupt masking patch for Cortex-M3 ports

I think clang was just ignoring the ‘naked’ declaration entirely.  It was including its own return instructions, for example.  But anyway, you have these
(void) ulNewMaskValue
; and
return 0;
lines that aren’t really necessary if you spec the asm operands correctly – my patches should do that for all (gcc inline assembly compatible) compilers.  Nothing like semantic correctness to make users happy!

Interrupt masking patch for Cortex-M3 ports

I just noticed my patches kept included some now-unnecessary comments.  Do you want me to clean that up and resubmit?