[Bug 1001275] Cortex-M (armV7) architecture endian instructions / Applied on lwIP

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001275

--- Comment #3 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-17 21:25:47 BST ---
Hi Sergei, thank you for comments.

(In reply to comment #2)
> Hi Ijlija,
> 
> Thank you for your attention to low-memory footprint eCos targets and your
> ideas around, i.e. how to utilize every bit on such CPUs :-)

And spare some clock cycles :)

> 
> My $.02.
> 
> Instead hal_arch_instr.h I would create <arch>_regs.h header file to define
> a few instructions in assembler.  In your case it is cortexm_regs.h.
> 
> NOTE: There are the precendents of such definitions in other architecture
>       trees.
> 

Right! Actually I was looking for such a header but obviously not hard enough.
I would have been happier if file name was hal_regs.h, but let's not break the
habit, and use cortexm_regs.h

> I mean (snippets not tested!)
> 
> #ifndef CYGONCE_HAL_CORTEXM_REGS_H
> #define CYGONCE_HAL_CORTEXM_REGS_H
> //==================================================== ...
> //
> //      cortexm_regs.h
> //
> //      Cortex-M CPU definitions
> //
> //      ...
> 
> #include <pkgconf/hal.h>
> 
> #include <cyg/hal/var_regs.h>
> 
> // Macro to embed rev instructions in C code
> #define CYGARC_REV(_origin_, _swapped_)        \
>     asm volatile( "rev %0, %1\n"        \
>           : "=r" (_swapped_)         \
>           : "r" (_origin_)        \
>         );
> 
> #define CYGARC_REV16(_origin_, _swapped_)    \
>     asm volatile( "rev16 %0, %1\n"        \
>           : "=r" (_swapped_)         \
>           : "r" (_origin_)        \
>         );


//  We could also define the functions, now using macros.
//  In context with this please look at discussion
//  regarding lwIP below.
//  Regarding functions I have one dillema:
//  Could we rename cygarc_arm_rev*() functions into cygarc_byteswap*()?

#define CYGARC_HAL_INSTR_ATTR \
   __externC __attribute__(( always_inline )) inline

// This is an optional name for cygarc_arm_rev()
// CYGARC_HAL_INSTR_ATTR cyg_uint32 cygarc_byteswap(cyg_uint32 original){

CYGARC_HAL_INSTR_ATTR cyg_uint32 cygarc_arm_rev(cyg_uint32 original){
    cyg_uint32 swapped;
    CYGARC_REV(swapped, original);
    return swapped;
}

CYGARC_HAL_INSTR_ATTR cyg_uint32 cygarc_arm_rev16(cyg_uint32 original){
    cyg_uint32 swapped;
    CYGARC_REV16(swapped, original);
    return swapped;
}

CYGARC_HAL_INSTR_ATTR cyg_int32 cygarc_arm_revsh(cyg_int32 original){
    cyg_int32 swapped;
    CYGARC_REVSH(swapped, original);
    return swapped;
}

// Platform supports endian operations.
#define CYGARC_BYTESWAP 1
#define CYGARC_BYTESWAP32(__val) cygarc_arm_rev(__val)
#define CYGARC_BYTESWAP16(__val) ((cyg_uint16)cygarc_arm_rev16(__val))

> // ---------------------------------------------------- ...
> // End of cortexm_regs.h
> #endif // ifdef CYGONCE_HAL_CORTEXM_REGS_H
> 

> 
> And then I would add only needed inline functions and declarations in some
> header file (again, not tested)
> 
> #include <cyg/hal/cortexm_regs.h>
> 
> #define LWIP_PLATFORM_HTONL( _hostlong_ )  hal_htol( _hostlong_ )
> #define LWIP_PLATFORM_HTONS( _hostshort_ ) hal_htos( _hostshort_ )


I think that it wouldn't be appropriate to use ARM-related mnemonics
(CYGARC_REV) in lwIP subtree. Also we should avoid, putting lwIP - related
names into HAL.
For this reason I have defined CYGARC_BUTESWAPxx() - architecture neutral macro
name that could be used in other architectures as appropriate.

> 
> static inline cyg_uint32 hal_htonl(cyg_uint32 hostslong)
> {
>     cyg_uint32      netlong;
>     CYGARC_REV(hostlong, netlong);
>     return netlong;
> }
> 
> static inline cyg_uint16 hal_htons(cyg_uint16 hostshort)
> {
>     cyg_uint16      netshort;
>     CYGARC_REV16(hostshort, netshort);
>     return netshort;
> }
> 
> 
> But, I would resist to put them in hal_arch.h ...  And it is mine only. By
> other hand lwipopts.h includes that header only. I thought it's pitty that we
> have hal_io.h, but not hal_net.h. Thus, we need more expert views on code's
> arrangement.

I think that hal_arch.h isn't bad, after all cortexm_regs.h belong to hal and
is architecture specific.

I am not able to run the code at present but it builds without warnings.
Disassembly looks as expected.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.