Lpc2000

John,

Hope this spoilt child will not be grounded for critiquing on dad's
work :)

It certainly dismisses the myth that one can disable interrupts while
in 16-bit mode without interworking!

I hope you will not mind if I critique your design with the view of
improving it becomes a file.  I do not wish to kill your efforts by
coming up with yet another interface design.

I do apologise before hand if in aiming to be complete I appear to be
nit picking.  It is occupational hazard that I have learnt to live with :)

I feel your design can be made more compiler friendly in relation to
optimisation if you had used inline functions.

I trust my comments will be received in a constructive mode.

Regards,

Jaya

--- In lpc2000@yahoogroups.com, "John Heenan" <l10@...> wrote:
>
> I have extended and tidied up the examples I have just posted to the 
> thread. I will probably post the code to the file section soon.
> 
> John Heenan
> 
> //header file interwork.h
> #ifndef INTERWORK_H_
> #define INTERWORK_H_

Probably your intention was to demonstrate interworking.  When you
make it a file, perhaps you could reflect on the purpose of this
interface, which is not interworking.

> //MAKE SURE A DEFAULT ISR HANDLER IS ASSIGNED TO VICDefVectAddr
> //THE DEFAULT ISR HANDLER CAN BE EMPTY BUT MUST BE PRESENT

I do feel this comment is out of place.  Certainly it applies to LPC
but is not something generic that I think warrants such a comment if
we look at the portability requirement some here appear preoccupied with.

> 
> #ifdef THUMB_INTERWORK

Appears to violate a rule in my KISS book that says:

  "Avoid defining a new conditional for one that exists."

Why not use the __thumb__ token that is already defined by GCC when
you invoke it with -mthumb option to produce 16-bit code?

>  #define INTDISABLE cpsr_if_disable()
>  #define INTENABLE  cpsr_if_enable()
>  #define SLOWINTDISABLE cpsr_i_disable()
>  #define SLOWINTENABLE  cpsr_i_enable()
>  //volatile definition forces stack storage and is not required
>  //as optimisation sees variable is used by a function and so keeps

I think the comment is not clear.  I do not understand what you are
getting here.  Perhaps you can rephrase it to make it clearer.

>  #define INTVAR int cpsr_prior

Appears to violate two rules in my KISS book:

  "Avoid defining a new interface for one that exists."

and

  "Avoid using macros to alters syntax or semantics."

In relation to the latter rule I am assuming we are taking about a
fresh design, not retrofitting header to existing code.

>  #define INTGETDISABLE cpsr_prior=cpsr_ifget_disable()
>  #define INTRESTORE cpsr_c_restore(cpsr_prior)
> 
>  void cpsr_if_disable(void); //works with thumb
>  void cpsr_if_enable(void);
>  void cpsr_i_disable(void);
>  void cpsr_i_enable(void);
>  int cpsr_ifget_disable(void);
>  void cpsr_c_restore(int cpsr_prior);
> #else
>  #define INTDISABLE INTIF_DISABLE
>  #define INTENABLE  INTIF_ENABLE
>  #define SLOWINTDISABLE INTI_DISABLE
>  #define SLOWINTENABLE  INTI_ENABLE
>  #define INTVAR volatile int cpsr_prior
>  #define INTGETDISABLE INTIFGET_DISABLE(cpsr_prior)
>  #define INTRESTORE INTIF_RESTORE(cpsr_prior)
> #endif
> 
> /*
> Usage example
>   INTDISABLE
>   
>   INTENABLE
> 
> 
>   INTVAR;
>   INTGETDISABLE;
>   
>   
>   INTRESTORE;
>   
>   INTGETDISABLE;
>   
>   
>   INTRESTORE;
> */
> 
> //not OK for thumb code
> #define INTIF_ENABLE   asm volatile ( \
>    "mrs r3,cpsr     \n\t" \
>    "bic r3,r3,#0xC0 \n\t" \
>    "msr cpsr_c,r3   \n\t" \
>    :                      \
>    :                      \
>    : "r3"                 \
> )

I prefer to avoid having to name registers explicitly so as not to
impose additional constraints on register optimisation.

Although not naming a register appears to adds one extra instruction
(which can be avoided if we were to design the interface differently),
it can save two move instructions where it is instantiated inline.

> 
> #define INTIF_DISABLE  asm volatile ( \
>    "mrs r3,cpsr		\n\t" \
>    "orr r3,r3,#0x80 \n\t" /* I bit. separate disablement: see user 
> manual */ \
>    "msr cpsr_c,r3   \n\t" \
>    "orr r3,r3,#0x40 \n\t" /* F bit */ \
>    "msr cpsr_c,r3   \n\t" \
>    :                      \
>    :                      \
>    : "r3"                 \
> )

Same issue here with naming register.  More such examples below.

> 
> #define INTI_ENABLE   asm volatile ( \
>    "mrs r3,cpsr     \n\t" \
>    "bic r3,r3,#0x80 \n\t" \
>    "msr cpsr_c,r3   \n\t" \
>    :                      \
>    :                      \
>    : "r3"                 \
> )
> 
> #define INTI_DISABLE  asm volatile ( \
>    "mrs r3,cpsr     \n\t" \
>    "orr r3,r3,#0x80 \n\t" \
>    "msr cpsr_c,r3   \n\t" \
>    :                      \
>    :                      \
>    : "r3"                 \
> )
>                 	
>                 
> #define INTIFGET_DISABLE(A) asm volatile ( \
>    "mrs %0, cpsr       \n\t" \
>    "orr r3, %0, #0x80  \n\t"  /* I bit. separate disablement: see 
> user manual */ \
>    "msr cpsr_c, r3     \n\t" \
>    "orr r3, r3, #0x40  \n\t"  /* F bit */ \
>    "msr cpsr_c, r3     \n\t" \
>     : "=r"(A)                \
>     :                        \
>     : "r3"                   \
> )                                         
> 
> #define INTIF_RESTORE(A) asm volatile ( \
>    "msr cpsr_c, %0 \n\t" \
>     :                    \
>     : "r"(A)             \
> )
> 
> #endif // INTERWORK_H_
> 
> //##########################
> 
> //module file interwork.c compiled as ARM assembler

I recommend naming the interworking options needed if compiled code is
to work when called from 16-bit mode -- caller or callee interworking?

> #include "interwork.h"
> void cpsr_if_disable(void)
> {
>  INTIF_DISABLE;
> }
> 
> void cpsr_if_enable(void)
> {
>   INTIF_ENABLE;
> }
> 
> void cpsr_i_disable(void)
> {
>   INTI_DISABLE;
> }
> 
> void cpsr_i_enable(void)
> {
>   INTI_ENABLE;
> }
> 
> int cpsr_ifget_disable(void)
> {
>   unsigned int cpsr_prior;
>   INTIFGET_DISABLE(cpsr_prior);
>   return cpsr_prior;  
> //assembler generated
> // mrs r0, cpsr       
> // orr r3, r0, #0x80  
> // msr cpsr_c, r3     
> // orr r3, r3, #0x40  
> // msr cpsr_c, r3     
> // bx	lr
> }
> 
> void cpsr_c_restore(int cpsr_prior)
> {
>   INTIF_RESTORE(cpsr_prior);	
> //assembler generated
> // msr cpsr_c, r0  
> // bx	lr
> }
> 
> --- In lpc2000@yahoogroups.com, "John Heenan" <l10@> wrote:
> >
> > Despised by the 'beautiful people' but practical, macros make it is 
> > easy to retarget ARM inline assembler to work with C code compiled 
> as 
> > Thumb assembler.

The macro feature is powerful, so powerful that it can be used to
obfuscate code as much as to make it clearer.

Macros in themselves are not ugly, but it certainly can be used in
very ugly ways.  Thus I do not despise the macro feature itself.

The most powerful use of macro is in retrofitting through making
minimal yet effective changes in the header.

> > 
> > Following are expamples for interrupt disable/enable for GCC. The 
> > intention is simply to force the macros through functions compiled 
> as 
> > ARM code so as the compiler will force correct usage of bx to 
> switch 
> > between ARM and Thumb modes, when compiling Thumb code. The beauty 
> of 
> > the macro system is that all ARM generated code can use pure inline 
> > assembler through
>