This is the mail archive of the ecos-bugs@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

--- Comment #4 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-18 22:14:20 BST ---
(In reply to comment #3 from IK)
> (In reply to comment #2 from SG)
> > 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.

Then my question, Where did you plan to define and implement them?

> //  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*()?

IMHO, as hal_endian.h contains

  #ifndef CYG_SWAP16
  # define CYG_SWAP16(_x_)
  ...

  #ifndef CYG_SWAP32
  # define CYG_SWAP32(_x_)
  ...

And you would just define own versions (see below).

> #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;
> }

[snip]

Ilija, Do you really need in C90 inline attribute? Do you really need
extern inlines. AFAIK, extern + inline is almost macro and what is about

  #ifndef CYGARC_SWAP16(_x_)
   #define CYGARC_SWAP16(_x_) cygarc_swap16(_x_)
   static __inline__ cygarc_swap16(_original_)
   {
       cyg_uint16      _swapped_;
       CYGARC_REV16(_orginal_, _swapped_);
       return _swapped_;
   }
  ...

  etc.

> // Platform supports endian operations.
> #define CYGARC_BYTESWAP 1

OK, or may be CYGARC_SWAPXX.

> #define CYGARC_BYTESWAP32(__val) cygarc_arm_rev(__val)
> #define CYGARC_BYTESWAP16(__val) ((cyg_uint16)cygarc_arm_rev16(__val))

I would prefer those short names or may be even CYG_SWAP16, CYG_SWAP32;
also I would not put the above definitions in cortexm_regs.h.  In my
opinion CYGARC_SWAP16, CYGARC_SWAP32 can be placed in hal_arch.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.

Agreed. But, I told about some invented `hal_net.h' thought. Above and
below was for such a header. See below.

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

Now you see. That was just thoughts.

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

Ilija, IMHO

1) <hal>_regs.h is for registers, opcodes, mnemonics, and asm inlines
   definitions. So, let's put a few CYGARC_REV* defines there for now.

2) We can define CYGARC_SWAPxx or CYG_SWAPxx macros and put there static
   inlines for them in hal_arch.h.

   I would avoid C90 inline attributes (always_inline) in hal_arch.h as
   it is GCC4-ish and I would quote here "one never knows what the next
   annoying compiler will demand" :-)

   Look, I configured/build eCos for stm3210e (default+lwip) and I've
   got

   % grep hal_arch.h --include=*.d -r . | wc -l
   107

   The hal_arch.h is a very important header and I would not risk to add
   more compiler dependences there.

   If it (always_inline) uses itself only for one module it is not big
   deal I even know where is such an application in eCos. That is eCos
   `framebuf' package. Look, please, at framebuf.inl. But only one C
   source (linear.c) uses those "always inlined" functions and by the
   way they declared as static.

3) If we will define SWAP16/32 in hal_arch.h then lwiopts.h can get
   almost yours (just an example)

  #ifdef CYGARC_SWAPXX
  # define LWIP_PLATFORM_BYTESWAP 1
  # define LWIP_PLATFORM_HTONS(__val) CYGARC_SWAP16(__val)
  # define LWIP_PLATFORM_HTONL(__val) CYGARC_SWAP32(__val)
  #endif

Well, it is my view only. I hope that I did not miss something.

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

It's good to know. Thank you for feedback.

Sergei

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]