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

[bugzilla-daemon@bugs.ecos.sourceware.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 #9 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-22 13:09:41 BST ---
(In reply to comment #8)
> Sergei
> 
> I did testing and I have working code, but I would postpone patch
> generation because of the question below.
> 
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > Personally I would like CYGARC_SWAPxx(), but since CYG_SWAPxx() are
> > > already present in hal_endian.h let's use them. It will propagate REV
> > > instructions in other parts of code that may benefit (i guess it is
> > > safe).
> 
> hal_endian.h has a provision CYGBLD_HAL_ENDIAN_H for include of
> architecture specific CYGARC_SWAPxx(). This is what I put in
> hal_cortexm.cdl:
> 
>     define_proc {
>         puts $::cdl_system_header "#define CYGBLD_HAL_ENDIAN_H \
>                 <cyg/hal/cortexm_endian.h>"
>     }
> 

I like it, this is a great find!

If I understood you correctly you again returned back to CYGARC prefix
for macros. If it is not a typo then I would add below those definitions
(CYGARC_SWAPxx macros) in ``cortexm_endian.h'' the same check and far
definitions (overrides)

#ifdef CYGBLD_HAL_CORTEXM_FAST_SWAP_OPS
# define CYG_SWAP16(x) CYGARC_SWAP16(x)
# define CYG_SWAP32(x) CYGARC_SWAP32(x)
#endif

and add such CDL option in Cortex-M config file. Well, used name of the
option is for demo only. Thus, users can rid fast inlines if they want.

> Now cortexm_endian.h is a new header that contains the CYGARC_SWAPxx()
> macros and corresponding inline functions. I added this file in order
> to avoid include of complete hal_arch.h
> 
> For me it is OK to add this header in arch/include but there may be a
> better proposal. Please comment.
> 
> Ilija

So, with your proposal we can leave hal_arch.h without any changes. Did
you mean it? I read so and I guess that we have to manage the definition
of LWIP_PLATFORM_BYTESWAP from configtool. In ideal world this value must
be defined from lwip config file, e.g.

to add into lwip_net.cdl:

cdl_option CYGBLD_LWIP_HTONS_HTONL_INLINED {
  default_value 0
}

to add into lwipopts.h:

#if CYGPKG_LWIP_HTONS_HTONL_INLINED
# include <cyg/hal/hal_endian.h>
# define LWIP_PLATFORM_BYTESWAP 1
# define LWIP_PLATFORM_HTONS(__val) CYG_SWAP16(__val)
# define LWIP_PLATFORM_HTONL(__val) CYG_SWAP32(__val)
#endif

What do you think?

Sergei

-- 
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.