[Bug 1001275] New: Cortex-M (armV7) architecture endian instructions / lwIP

[Bug 1001275] New: Cortex-M (armV7) architecture endian instructions / lwIP

[bugzilla-daemon]

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

           Summary: Cortex-M (armV7) architecture endian instructions /
                    lwIP
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Cortex-M
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: low
         Component: Patches and contributions
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: ilijak@siva.com.mk
                CC: ecos-patches@ecos.sourceware.org
             Class: Advice Request


Created an attachment (id=1299)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1299)
Cortex-M endian instructions

This patch contains C function wrappers for Cortex-M byte swapping
instructions. A new file hal_arch_instr.inl is proposed as a place to
accommodate specific architecture instructions such as presented rev rev16 and
revsh. Other instructions may be added in future.

Note: Incentive for this patch was speeding-up of endian operations such as
htonl() and htons(). A patch for lwIP htonl()/htons() is submitted in comment
#1.

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

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

[bugzilla-daemon]

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

--- Comment #1 from Ilija Kocho <ilijak@siva.com.mk> 2011-07-03 18:52:09 BST ---
Created an attachment (id=1300)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1300)
lwip_htonl_htons.diff

This patch provides Cortex-M architecture endian optimization to lWIP.

Tested on LPC-1766-STK and STM32E EVAL with ENC424 (Bug 1000910).

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

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

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Cortex-M (armV7)            |Cortex-M (armV7)
                   |architecture endian         |architecture endian
                   |instructions / lwIP         |instructions / Applied on
                   |                            |lwIP

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

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

[bugzilla-daemon]

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

--- Comment #2 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-15 21:17:36 BST ---
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 :-)

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.

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_)        \
        );

// ---------------------------------------------------- ...
// 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_ )

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.

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.

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

[bugzilla-daemon]

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 the assignee for the bug.

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

[bugzilla-daemon]

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.

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

[bugzilla-daemon]

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

--- Comment #5 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-19 09:40:37 BST ---
(In reply to comment #4)
> (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?

The idea was to have CMSIS like functions
http://www.arm.com/products/processors/cortex-m/cortex-microcontroller-software-interface-standard.php
in cortexm_regs.h, but I changed my mind so pse forget.

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

Agree.

[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

If I reconsider, not really. These functions are single instruction and
compiler should be crazy not to inline.

[snip]

> 
> > // 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;

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

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

I am thinking this way too, only I didn't want to do lot's of changes in
hal_arch.h. Now, especially if we use CYG_SWAPxx() it shouldn't be a problem.

[snip]

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

IMHO this would need further consideration since as it is supposed to propagate
through other architectures too. Likewise, I would be happy to have hal_devs.h,
(actually var_devs.h) for HAL defines borrowed to devices (pin configurations,
clock sources, power management etc).

[snip]

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

Agree.

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

Agree.

> 
>    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" :-)

As mentioned before it should be safe to omit them, I'll re-confirm during my
tests.

> 

[snip]

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

Clear.

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

Thanks again for considerations. At present I am away, but today I return home
and I am going to test during weekend. FAOD, unless you object I intend to use
CYG_SWAPxx() naming.

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #6 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-19 16:09:22 BST ---
(In reply to comment #5)
> (In reply to comment #4)

[ snip many lines where we came to consensus, see 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).

> > 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.
> 
> I am thinking this way too, only I didn't want to do lot's of changes
> in hal_arch.h. Now, especially if we use CYG_SWAPxx() it shouldn't be
> a problem.

So, we will not reinvent the weel and will stay on CYG_SWAPxx().

> > Well, it is my view only. I hope that I did not miss something.
> 
> Thanks again for considerations. At present I am away, but today I
> return home and I am going to test during weekend. FAOD, unless you
> object I intend to use CYG_SWAPxx() naming.

I vote for CYG_SWAPxx() too. I'm sorry but this week I'll be AFK.

CYA

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

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

[bugzilla-daemon]

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

--- Comment #7 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-19 16:18:17 BST ---
(In reply to comment #5)

> I vote for CYG_SWAPxx() too. I'm sorry but this week I'll be AFK.

s/week/weekend/

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

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

[bugzilla-daemon]

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

--- Comment #8 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-21 14:46:27 BST ---
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>"
    }

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

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

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

[bugzilla-daemon]

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 the assignee for the bug.

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

[bugzilla-daemon]

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

--- Comment #10 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-22 14:01:20 BST ---
(In reply to comment #9)
Ilija, to synchronize our heads :-) I did mean/understand what

on the arch's side we plan to add two new header files:

  - cortextm_regs.h     // CYGARC_REV16(), CYGARC_REV32(), etc.
  - cortexm_endian.h    // CYGARC_SWAP16(), CYGARC_SWAP32(), and
                        // by cond. CYG_SWAP16(), CYG_SWAP32()
and patch one

  - hal_cortexm.cdl     // 1) define_proc ... CYGBLD_HAL_ENDIAN_H
                        // 2) CYGBLD_HAL_CORTEXM_FAST_SWAPXX_INST
                        // NOTE: it is a draft name (to discuss)

on the lwip's side to patch

  - lwip_net.cdl        // CYGBLD_LWIP_HTOS_HTONL_INLINED
  - lwipopts.h          // check above and set if needed
                        // LWIP_PLATFORM_BYTESWAP, etc.

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.

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

[bugzilla-daemon]

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

--- Comment #11 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-22 14:23:22 BST ---
(In reply to comment #9)
> (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)

It is a my mistake:( sorry, CYG_SWAPxx() it should be.

[snip (as it is consequence to my error)]

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

Here is cortexm_endian.h snippet:

#include <cyg/hal/cortexm_regs.h>

static __inline__ cyg_uint32 cyg_hal_swap32(cyg_uint32 original)
{
    cyg_uint32 swapped;
    CYGARC_REV(swapped, original);
    return swapped;
}

static __inline__ cyg_uint16 cyg_hal_swap16(cyg_uint16 original)
{
    cyg_uint16 swapped;
    CYGARC_REV16(swapped, original);
    return swapped;
}

#define CYG_SWAP32(__val) cyg_hal_swap32(__val)
#define CYG_SWAP16(__val) cyg_hal_swap16(__val)

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

I can see 2 options:

Option 1. We include cortexm_endian.h in hal_arch.h (that's what i have
tested).
here are all changes to hal_arch.h:

diff -u -r1.3 hal_arch.h
--- hal_arch.h    13 Feb 2009 17:04:18 -0000    1.3
+++ hal_arch.h    22 Aug 2011 12:45:24 -0000
@@ -55,6 +55,9 @@

 #include <cyg/hal/var_arch.h>

+#include <cyg/hal/cortexm_regs.h>
+#include <cyg/hal/cortexm_endian.h>
+ 


Option 2. Probably we can include hal_endian.h in lwipots.h (not tested).

My vote is for Option 1. since it would propagate cortexm_endian.h everywhere
(for better, or for worse) Cortex-M is used.

> 
> to add into lwip_net.cdl:
> 
> cdl_option CYGBLD_LWIP_HTONS_HTONL_INLINED {
>   default_value 0
> }
> 

In either case we don't need CDL. Following is what I have tested (a patch to
your input) :

> to add into lwipopts.h:
> 
> #if CYGPKG_LWIP_HTONS_HTONL_INLINED
-#if CYGPKG_LWIP_HTONS_HTONL_INLINED

> # include <cyg/hal/hal_endian.h>
+#ifdef CYG_SWAP32
> # 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?

If you agree with adding cortexm_endian.h and #include it in hal_arch.h I could
submit patches for review.

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #12 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-22 14:35:54 BST ---
(In reply to comment #10)
> (In reply to comment #9)
> Ilija, to synchronize our heads :-) I did mean/understand what

Just to sync with my comment #11

> 
> on the arch's side we plan to add two new header files:
> 
>   - cortexm_regs.h     // CYGARC_REV16(), CYGARC_REV32(), etc.
    - cortexm_endian.h   // CYG_SWAP16(), CYG_SWAP32()

> and patch one
> 
>   - hal_cortexm.cdl     // 1) define_proc ... CYGBLD_HAL_ENDIAN_H

We don't need following (see comment #11)

>                         // 2) CYGBLD_HAL_CORTEXM_FAST_SWAPXX_INST
>                         // NOTE: it is a draft name (to discuss)
> 
> on the lwip's side to patch
> 

Not this (comment #11)

>   - lwip_net.cdl        // CYGBLD_LWIP_HTOS_HTONL_INLINED

Only this:

>   - lwipopts.h          // check above and set if needed
>                         // LWIP_PLATFORM_BYTESWAP, etc.
> 
> 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.

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

[bugzilla-daemon]

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

--- Comment #13 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-22 15:24:02 BST ---
(In reply to comment #11)
> (In reply to comment #9)

[snip]

> > 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)
> 
> It is a my mistake:( sorry, CYG_SWAPxx() it should be.
> 
> [snip (as it is consequence to my error)]

I see, however your "mistake" would provide us two versions of
CYG_SWAPxx() macros (one from hal_endian.h and another from
cortex_endian.h if it's needed). Ok, let's forget it.

> Here is cortexm_endian.h snippet:
> 
> #include <cyg/hal/cortexm_regs.h>
> 
> static __inline__ cyg_uint32 cyg_hal_swap32(cyg_uint32 original)
> {
>     cyg_uint32 swapped;
>     CYGARC_REV(swapped, original);
>     return swapped;
> }
> 
> static __inline__ cyg_uint16 cyg_hal_swap16(cyg_uint16 original)
> {
>     cyg_uint16 swapped;
>     CYGARC_REV16(swapped, original);
>     return swapped;
> }
> 
> #define CYG_SWAP32(__val) cyg_hal_swap32(__val)
> #define CYG_SWAP16(__val) cyg_hal_swap16(__val)

Looks okay for me.

> > 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.
> 
> I can see 2 options:
> 
> Option 1. We include cortexm_endian.h in hal_arch.h (that's what i
> have tested).  here are all changes to hal_arch.h:
> 
> diff -u -r1.3 hal_arch.h
> --- hal_arch.h	13 Feb 2009 17:04:18 -0000	1.3
> +++ hal_arch.h	22 Aug 2011 12:45:24 -0000
> @@ -55,6 +55,9 @@
>  
>  #include <cyg/hal/var_arch.h>
>  
> +#include <cyg/hal/cortexm_regs.h>
> +#include <cyg/hal/cortexm_endian.h>
> + 
>  
> Option 2. Probably we can include hal_endian.h in lwipots.h (not tested).
> 
> My vote is for Option 1. since it would propagate cortexm_endian.h
> everywhere (for better, or for worse) Cortex-M is used.

In opposite I would avoid from "Option 1". Usually <hal>_regs.h, and
hal_endian.h include inself from hal_io.h, and hal_misc.c. I liked you
find, because I thought, Great! We do not need hack hal_arch.h :-) I
would vote for "Option 2" and that is what I did mean in my comment 9.

> > to add into lwip_net.cdl:
> > 
> > cdl_option CYGBLD_LWIP_HTONS_HTONL_INLINED {
> >   default_value 0
> > }
> > 
> 
> In either case we don't need CDL. Following is what I have tested (a
> patch to your input) :
> 
> > to add into lwipopts.h:
> > 
> > #if CYGPKG_LWIP_HTONS_HTONL_INLINED
> -#if CYGPKG_LWIP_HTONS_HTONL_INLINED
> 
> > # include <cyg/hal/hal_endian.h>
> +#ifdef CYG_SWAP32
> > # 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?

I see. However, in such case you make lwip stack use only the inlined
versions for htonX(s). I have a doubt about such a forcing. What is a
bad with CDL option?

> If you agree with adding cortexm_endian.h and #include it in
> hal_arch.h I could submit patches for review.
> 
> Ilija

It seems to me that you can send cortexm_{regs,endian.h} for now (arch's
side) and the lwip's side needs further discussion.

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.
>From ecos-bugs-return-8229-listarch-ecos-bugs=sources.redhat.com@sourceware.org Mon Aug 22 16:45:28 2011
Return-Path: <ecos-bugs-return-8229-listarch-ecos-bugs=sources.redhat.com@sourceware.org>
Delivered-To: listarch-ecos-bugs@sources.redhat.com
Received: (qmail 572 invoked by alias); 22 Aug 2011 16:45:24 -0000
Received: (qmail 526 invoked by uid 22791); 22 Aug 2011 16:45:18 -0000
X-SWARE-Spam-Status: No, hits=-2.1 required=5.0
	tests=AWL,BAYES_00,RP_MATCHES_RCVD
X-Spam-Check-By: sourceware.org
Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197)
    by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 22 Aug 2011 16:44:52 +0000
Received: by mail.ecoscentric.com (Postfix, from userid 48)
	id 53B512F78001; Mon, 22 Aug 2011 17:44:51 +0100 (BST)
X-Original-To: unassigned@bugs.ecos.sourceware.org
Delivered-To: unassigned@bugs.ecos.sourceware.org
From: bugzilla-daemon@bugs.ecos.sourceware.org
To: unassigned@bugs.ecos.sourceware.org
Subject: [Bug 1001275] Cortex-M (armV7) architecture endian instructions /
 Applied on lwIP
X-Bugzilla-Reason: AssignedTo
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: eCos
X-Bugzilla-Component: Patches and contributions
X-Bugzilla-Keywords:
X-Bugzilla-Severity: enhancement
X-Bugzilla-Who: ilijak@siva.com.mk
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: low
X-Bugzilla-Assigned-To: unassigned@bugs.ecos.sourceware.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Changed-Fields: Attachment #1299 is obsolete
In-Reply-To: <bug-1001275-777@http.bugs.ecos.sourceware.org/>
References: <bug-1001275-777@http.bugs.ecos.sourceware.org/>
X-Bugzilla-URL: http://bugs.ecos.sourceware.org/
Auto-Submitted: auto-generated
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Date: Mon, 22 Aug 2011 16:45:00 -0000
Message-Id: <20110822164446.7B6FC2F78004@mail.ecoscentric.com>
Mailing-List: contact ecos-bugs-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <ecos-bugs.sourceware.org>
List-Subscribe: <mailto:ecos-bugs-subscribe@sourceware.org>
List-Post: <mailto:ecos-bugs@sourceware.org>
List-Help: <mailto:ecos-bugs-help@sourceware.org>, <http://sourceware.org/lists.html#faqs>
Sender: ecos-bugs-owner@sourceware.org
Delivered-To: mailing list ecos-bugs@sourceware.org
X-SW-Source: 2011/txt/msg01139.txt.bz2
Content-length: 4096

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1299|0                           |1
        is obsolete|                            |

--- Comment #14 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-22 17:44:42 BST ---
Created an attachment (id\x1345)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id\x1345)
Cortex-M architecture endian.

(In reply to comment #13)

Here I submit Cortex-M architectural patches. I took the liberty to put/leave
some additional instructions  in cortexm_regs.h (that I am using for Kinetis
port). They are CPSID and CPSIE. I hope they will be accepted but if reviewers
find that they are too off-topic they may be removed.

> (In reply to comment #11)
> In opposite I would avoid from "Option 1". Usually <hal>_regs.h, and
> hal_endian.h include inself from hal_io.h, and hal_misc.c. I liked you
> find, because I thought, Great! We do not need hack hal_arch.h :-) I
> would vote for "Option 2" and that is what I did mean in my comment 9.

Now I agree. Patch follows the Option 2.

>
> > > to add into lwip_net.cdl:
> > >
> > > cdl_option CYGBLD_LWIP_HTONS_HTONL_INLINED {
> > >   default_value 0
> > > }
> > >
> >
> > In either case we don't need CDL. Following is what I have tested (a
> > patch to your input) :
> >
> > > to add into lwipopts.h:
> > >
> > > #if CYGPKG_LWIP_HTONS_HTONL_INLINED
> > -#if CYGPKG_LWIP_HTONS_HTONL_INLINED
> >
> > > # include <cyg/hal/hal_endian.h>
> > +#ifdef CYG_SWAP32
> > > # 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?
>
> I see. However, in such case you make lwip stack use only the inlined
> versions for htonX(s). I have a doubt about such a forcing. What is a
> bad with CDL option?

Sergei, now I see, you want to provide the user with choice between standard
and /architecture optimized/ endian functions.

It makes sense, but IMHO that, when available, /architecture optimized/
functions should be enabled by default.

It is likely that we shall need to add CDL options in both lwIP (bool) and in
architecture's CDL (provider of default value for lwIP).
For arch name I would propose something like CYGxxx_HAL_ARCH_ENDIAN where xxx
could be {PKG, OPT, BLD, INT}, and for lwIP - CYGxxx_LWIP_ENDIAN_BY_HAL.

At the end, for reference, here are lwip modifications (as are now) with which
I have tested. Please note that now I am using CYG_CPU_TO_BExx macros, also
defined in hal_endian.h and they consider existence of big endians.

diff -u -r1.8 lwipopts.h
--- current/include/lwipopts.h    31 Jan 2011 21:52:32 -0000    1.8
+++ current/include/lwipopts.h    22 Aug 2011 16:23:38 -0000
@@ -35,6 +35,7 @@
 #include <pkgconf/net_lwip.h>

 #include <cyg/hal/hal_arch.h>
+#include <cyg/hal/hal_endian.h>

 //-------------------------------------------------------------------
 // Platform specific locking
@@ -44,6 +45,16 @@
 #define NO_SYS                      defined(CYGFUN_LWIP_MODE_SIMPLE)

 //-------------------------------------------------------------------
+// Architecture specific options
+//--------------------------------------------------------------------
+
+#if defined(CYG_CPU_TO_BE16) && defined(CYG_CPU_TO_BE32)
+# define LWIP_PLATFORM_BYTESWAP 1
+# define LWIP_PLATFORM_HTONS(__val) CYG_CPU_TO_BE16(__val)
+# define LWIP_PLATFORM_HTONL(__val) CYG_CPU_TO_BE32(__val)
+#endif // defined(CYG_CPU_TO_BE16(__val)) && defined(CYG_CPU_TO_BE32(__val))
+
+//-----------------------------------------------------------------
 // Memory options
 //----------------------------------------------------------------

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

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

[bugzilla-daemon]

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

--- Comment #15 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-22 19:26:17 BST ---
(In reply to comment #14)

> Created an attachment (id=1345)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1345)
> Cortex-M architecture endian.

Ilija, thank you. The patch looks good for me, minors:

  hal_cortexm.cdl: a few trailing whitespaces.
  cortexm_regs.h: s/CYGONCE_HALCORTEXM_REGS_H/CYGONCE_CORTEXM_REGS_H/

No need to repost the patch. I'll fix that on commit.

> (In reply to comment #13)
> Here I submit Cortex-M architectural patches. I took the liberty to
> put/leave some additional instructions  in cortexm_regs.h (that I am
> using for Kinetis port). They are CPSID and CPSIE. I hope they will be
> accepted but if reviewers find that they are too off-topic they may be
> removed.

As for me I have no objection to include CPSID/CPSIE in cortexm_regs.h,
more that you test the macros on real hardware what is much appreciated.

> > (In reply to comment #11)
> > In opposite I would avoid from "Option 1". Usually <hal>_regs.h, and
> > hal_endian.h include inself from hal_io.h, and hal_misc.c. I liked you
> > find, because I thought, Great! We do not need hack hal_arch.h :-) I
> > would vote for "Option 2" and that is what I did mean in my comment 9.
> 
> Now I agree. Patch follows the Option 2.

(snip snippets)

> > I see. However, in such case you make lwip stack use only the
> > inlined versions for htonX(s). I have a doubt about such a forcing.
> > What is a bad with CDL option?
> 
> Sergei, now I see, you want to provide the user with choice between
> standard and /architecture optimized/ endian functions.

Yep.

> It makes sense, but IMHO that, when available, /architecture optimized/
> functions should be enabled by default.

Agreed.

> It is likely that we shall need to add CDL options in both lwIP (bool)
> and in architecture's CDL (provider of default value for lwIP).  For
> arch name I would propose something like CYGxxx_HAL_ARCH_ENDIAN where
> xxx could be {PKG, OPT, BLD, INT}, and for lwIP -
> CYGxxx_LWIP_ENDIAN_BY_HAL.

Agreed. IMHO, you offered clear names for us (who reads this thread),
however, I would stick on a set from PKG, or BLD either IMP.

[OFF-TOPIC]

My recent find. CYGOPT_* names use itself in eCos config files more than
100 times! It seems to mee I used such names too :-) and reading this
http://ecos.sourceware.org/docs-latest/cdl-guide/language.naming.html
chapter I believe in that what to use CYGOPT is some kind of tautology.

> At the end, for reference, here are lwip modifications (as are now)
> with which I have tested. Please note that now I am using
> CYG_CPU_TO_BExx macros, also defined in hal_endian.h and they consider
> existence of big endians.
> 
> diff -u -r1.8 lwipopts.h
> --- current/include/lwipopts.h	31 Jan 2011 21:52:32 -0000	1.8
> +++ current/include/lwipopts.h	22 Aug 2011 16:23:38 -0000
> @@ -35,6 +35,7 @@
>  #include <pkgconf/net_lwip.h>
>  
>  #include <cyg/hal/hal_arch.h>
> +#include <cyg/hal/hal_endian.h>
>  
>  //-------------------------------------------------------------------
>  // Platform specific locking
> @@ -44,6 +45,16 @@
>  #define NO_SYS                      defined(CYGFUN_LWIP_MODE_SIMPLE)
>  
>  //-------------------------------------------------------------------
> +// Architecture specific options
> +//--------------------------------------------------------------------
> +
> +#if defined(CYG_CPU_TO_BE16) && defined(CYG_CPU_TO_BE32)
> +# define LWIP_PLATFORM_BYTESWAP 1
> +# define LWIP_PLATFORM_HTONS(__val) CYG_CPU_TO_BE16(__val)
> +# define LWIP_PLATFORM_HTONL(__val) CYG_CPU_TO_BE32(__val)
> +#endif // defined(CYG_CPU_TO_BE16(__val)) && defined(CYG_CPU_TO_BE32(__val))
> +
> +//-----------------------------------------------------------------
>  // Memory options
>  //----------------------------------------------------------------

Ilija, it's even more proper check. Once again thank you for your tests.
If you plan to check proposed CYGxxx_LWIP_ENDIAN_BY_HAL in lwipopts.h

#ifdef CYGxxx_LWIP_ENDIAN_BY_HAL
# include <cyg/hal/hal_endian.h>
#endif

I will very appreciate your final patches for lwipopts.h, and config
files.

Thank you,
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.
>From ecos-bugs-return-8231-listarch-ecos-bugs=sources.redhat.com@sourceware.org Tue Aug 23 13:27:54 2011
Return-Path: <ecos-bugs-return-8231-listarch-ecos-bugs=sources.redhat.com@sourceware.org>
Delivered-To: listarch-ecos-bugs@sources.redhat.com
Received: (qmail 9404 invoked by alias); 23 Aug 2011 13:27:51 -0000
Received: (qmail 9388 invoked by uid 22791); 23 Aug 2011 13:27:50 -0000
X-SWARE-Spam-Status: No, hits=-2.1 required=5.0
	tests=AWL,BAYES_00,RP_MATCHES_RCVD
X-Spam-Check-By: sourceware.org
Received: from hagrid.ecoscentric.com (HELO mail.ecoscentric.com) (212.13.207.197)
    by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 23 Aug 2011 13:27:36 +0000
Received: by mail.ecoscentric.com (Postfix, from userid 48)
	id CA27E2F78004; Tue, 23 Aug 2011 14:27:34 +0100 (BST)
X-Original-To: unassigned@bugs.ecos.sourceware.org
Delivered-To: unassigned@bugs.ecos.sourceware.org
From: bugzilla-daemon@bugs.ecos.sourceware.org
To: unassigned@bugs.ecos.sourceware.org
Subject: [Bug 1001275] Cortex-M (armV7) architecture endian instructions /
 Applied on lwIP
X-Bugzilla-Reason: AssignedTo
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: eCos
X-Bugzilla-Component: Patches and contributions
X-Bugzilla-Keywords:
X-Bugzilla-Severity: enhancement
X-Bugzilla-Who: ilijak@siva.com.mk
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: low
X-Bugzilla-Assigned-To: unassigned@bugs.ecos.sourceware.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Changed-Fields:
In-Reply-To: <bug-1001275-777@http.bugs.ecos.sourceware.org/>
References: <bug-1001275-777@http.bugs.ecos.sourceware.org/>
X-Bugzilla-URL: http://bugs.ecos.sourceware.org/
Auto-Submitted: auto-generated
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Date: Tue, 23 Aug 2011 13:27:00 -0000
Message-Id: <20110823132727.BD3062F7800A@mail.ecoscentric.com>
Mailing-List: contact ecos-bugs-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <ecos-bugs.sourceware.org>
List-Subscribe: <mailto:ecos-bugs-subscribe@sourceware.org>
List-Post: <mailto:ecos-bugs@sourceware.org>
List-Help: <mailto:ecos-bugs-help@sourceware.org>, <http://sourceware.org/lists.html#faqs>
Sender: ecos-bugs-owner@sourceware.org
Delivered-To: mailing list ecos-bugs@sourceware.org
X-SW-Source: 2011/txt/msg01141.txt.bz2
Content-length: 2398

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

--- Comment #16 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 14:27:21 BST ---
(In reply to comment #15)

> I will very appreciate your final patches for lwipopts.h, and config
> files.

Here's what I got today.

Cortex-M CDL: Disabling CYGIMP_HAL_ARCH_ENDIAN also suppresses generation of
cortexm_endian.h so we have somplete global wipe-out.

--- ecos/packages/hal/cortexm/arch/current/cdl/hal_cortexm.cdl    2011-04-06
18:54:16.000000000 +0200
+++ ecos_kin/packages/hal/cortexm/arch/current/cdl/hal_cortexm.cdl
2011-08-23 14:54:14.000000000 +0200
@@ -138,6 +138,19 @@
              Higher priorities are reserved for the DEBUG and SVC traps."
     }

+    cdl_option CYGIMP_HAL_ARCH_ENDIAN {
+        display "Architecture optimized endian functions"
+        flavor bool
+        default_value 1
+        description "..."
+
+        define_proc {
+            puts $::cdl_system_header \
+           "#define CYGBLD_HAL_ENDIAN_H <cyg/hal/cortexm_endian.h>"
+        }
+    }
+
     cdl_option CYGBLD_LINKER_SCRIPT {
         display "Linker script"
         flavor data

lwIP CDL (lwipopt.h changed accordingly):

--- ecos/packages/net/lwip_tcpip/current/cdl/lwip_net.cdl    2011-03-17
12:40:35.000000000 +0100
+++ ecos_kin/packages/net/lwip_tcpip/current/cdl/lwip_net.cdl    2011-08-23
13:06:35.000000000 +0200
@@ -127,6 +127,17 @@
         compile         ecos/sio.c
     }

+    cdl_option CYGIMP_LWIP_ENDIAN_BY_HAL {
+        display "HAL optimized endian functions"
+        flavor bool
+        active_if CYGIMP_HAL_ARCH_ENDIAN
+        default_value CYGIMP_HAL_ARCH_ENDIAN
+
+        description "..."
+    }
+
     cdl_component CYGPKG_LWIP_APIS {
         display         "APIs"
         flavor          none

I tested following cases:
  1. CYGIMP_HAL_ARCH_ENDIAN absent (what we have in other architectures)
  2. CYGIMP_HAL_ARCH_ENDIAN disabled (global disable)
  3. CYGIMP_HAL_ARCH_ENDIAN enabled and CYGIMP_LWIP_ENDIAN_BY_HAL disabled
  4  Both CYGIMP_HAL_ARCH_ENDIAN and CYGIMP_LWIP_ENDIAN_BY_HAL enabled

If you are happy, I would submit.

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #17 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 14:31:51 BST ---
(In reply to comment #16)
> (In reply to comment #15)

Correction:

> Cortex-M CDL: Disabling CYGIMP_HAL_ARCH_ENDIAN also suppresses generation of
> cortexm_endian.h so we have somplete global wipe-out.

I mean: ... suppresses generation of cortexm_endian.h entry in system header
file ...

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #18 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-23 15:18:22 BST ---
(In reply to comment #16)
> (In reply to comment #15)
> 
> > I will very appreciate your final patches for lwipopts.h, and config
> > files.
> 
> Here's what I got today.
> 
> Cortex-M CDL: Disabling CYGIMP_HAL_ARCH_ENDIAN also suppresses
> generation of cortexm_endian.h so we have somplete global wipe-out.
> 
> --- ecos/packages/hal/cortexm/arch/current/cdl/hal_cortexm.cdl  2011-04-06
> 18:54:16.000000000 +0200
> +++ ecos_kin/packages/hal/cortexm/arch/current/cdl/hal_cortexm.cdl
> 2011-08-23 14:54:14.000000000 +0200
> @@ -138,6 +138,19 @@
>               Higher priorities are reserved for the DEBUG and SVC traps."
>      }
>      
> +    cdl_option CYGIMP_HAL_ARCH_ENDIAN {
> +        display "Architecture optimized endian functions"
> +        flavor bool
> +        default_value 1
> +        description "..."
> +        
> +        define_proc {
> +            puts $::cdl_system_header \
> +           "#define CYGBLD_HAL_ENDIAN_H <cyg/hal/cortexm_endian.h>"
> +        }        
> +    }
> +    
>      cdl_option CYGBLD_LINKER_SCRIPT {
>          display "Linker script"
>          flavor data
> 
> lwIP CDL (lwipopt.h changed accordingly):
> 
> --- ecos/packages/net/lwip_tcpip/current/cdl/lwip_net.cdl       2011-03-17
> 12:40:35.000000000 +0100
> +++ ecos_kin/packages/net/lwip_tcpip/current/cdl/lwip_net.cdl   2011-08-23
> 13:06:35.000000000 +0200
> @@ -127,6 +127,17 @@
>          compile         ecos/sio.c
>      }
>      
> +    cdl_option CYGIMP_LWIP_ENDIAN_BY_HAL {
> +        display "HAL optimized endian functions"
> +        flavor bool
> +        active_if CYGIMP_HAL_ARCH_ENDIAN
> +        default_value CYGIMP_HAL_ARCH_ENDIAN
> +        
> +        description "..."
> +    }
> +    
>      cdl_component CYGPKG_LWIP_APIS {
>          display         "APIs"
>          flavor          none

Ilija, for me these CDLs look great! IMHO, you picked clear names.

> I tested following cases:
>   1. CYGIMP_HAL_ARCH_ENDIAN absent (what we have in other architectures)
>   2. CYGIMP_HAL_ARCH_ENDIAN disabled (global disable)
>   3. CYGIMP_HAL_ARCH_ENDIAN enabled and CYGIMP_LWIP_ENDIAN_BY_HAL disabled
>   4  Both CYGIMP_HAL_ARCH_ENDIAN and CYGIMP_LWIP_ENDIAN_BY_HAL enabled

Many thanks for testing.

> If you are happy, I would submit.

Of course, please, submit your patch(es).

Thank you,
Sergei

> 
> Ilija

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

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

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1345|0                           |1
        is obsolete|                            |

--- Comment #19 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 17:13:27 BST ---
Created an attachment (id=1348)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1348)
Cortex-M architecture endian 110823

Here I re-submit Cortex-M arch integral patch, headers+CDL.

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

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

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1300|0                           |1
        is obsolete|                            |

--- Comment #20 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 17:16:48 BST ---
Created an attachment (id=1349)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1349)
lwIP htonl htons 110823

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

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

[bugzilla-daemon]

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

--- Comment #21 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 17:19:19 BST ---
Created an attachment (id=1350)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1350)
Cortex-M ChangeLog 110823

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

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

[bugzilla-daemon]

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

--- Comment #22 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 17:21:12 BST ---
Created an attachment (id=1351)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1351)
lwIP ChangeLog 110823

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

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

[bugzilla-daemon]

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

--- Comment #23 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-23 20:27:36 BST ---
(In reply to comment #19)
> Created an attachment (id=1348)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1348)
> Cortex-M architecture endian 110823
> 
> Here I re-submit Cortex-M arch integral patch, headers+CDL.

Regarding attachment 1348

hal_cortexm.cdl (CYGIMP_HAL_ARCH_ENDIAN): reformatted according CDL
coding style, fixed a typo in description (s/bute/byte) and I would
have said

+        default_value 1
+        description   "
+            Cortex-M architecture implements instructions for endian
+            manipulation (byte swapping).  If enabled, this feature
+            can produce shorter and faster code for that."

cortexm_regs.h:

Ilija, I missed that you have used such an order for arguments in REV
macros, for example

+// Reverse word
+#define CYGARC_REV(_swapped_,_original_) \
+        __asm__ volatile ("rev %0, %1\n" : "=r"(_swapped_) : "r"(_original_))
+

It looks clear for ARM assembler gurus (dst <- src), and for C-guys it
looks a bit puzzled, (IMO) they are accustomed to same

#define HAL_READ_UINT32( _register_, _value_ ) \

Still, I would use prototypes where result returns in the second
argument. And if you have no objections I would use multiline variants
for those macros.

#define CYGARC_REV( _origin_, _swapped_ )       \
    asm volatile( "rev %0, %1\n"                \
                  : "=r" (_swapped_)            \
                  : "r" (_origin_)              \
                );

You will not be against it (re-ordering the arguments)?

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

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

[bugzilla-daemon]

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

--- Comment #24 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-23 21:14:19 BST ---
(In reply to comment #23)
> (In reply to comment #19)
> Regarding attachment 1348 [details]
> 
> hal_cortexm.cdl (CYGIMP_HAL_ARCH_ENDIAN): reformatted according CDL
> coding style, fixed a typo in description (s/bute/byte) and I would
> have said
> 
> +        default_value 1
> +        description   "
> +            Cortex-M architecture implements instructions for endian
> +            manipulation (byte swapping).  If enabled, this feature
> +            can produce shorter and faster code for that."

OK. Thanks.

> 
> cortexm_regs.h:
> 
> Ilija, I missed that you have used such an order for arguments in REV
> macros, for example
> 
> +// Reverse word
> +#define CYGARC_REV(_swapped_,_original_) \
> +        __asm__ volatile ("rev %0, %1\n" : "=r"(_swapped_) : "r"(_original_))
> +
> 
> It looks clear for ARM assembler gurus (dst <- src), and for C-guys it
> looks a bit puzzled, (IMO) they are accustomed to same

I would insist to keep present order. Here are some arguments:

These macros shall be used mainly by people who are familiar with assembler and
they would be surprised if order is not as in assembler.

Also there is a long tradition the destination to be first argument in C
functions. The origins date back to big-bang, look for rationale in K&R book.
Examples strcpy(), strcat(), memcpy()...

> #define HAL_READ_UINT32( _register_, _value_ ) \

What about HAL_WRITE_UINT32(__register__, value)?

> 
> Still, I would use prototypes where result returns in the second
> argument. And if you have no objections I would use multiline variants
> for those macros.
> 
> #define CYGARC_REV( _origin_, _swapped_ )       \
>     asm volatile( "rev %0, %1\n"                \
>                   : "=r" (_swapped_)            \
>                   : "r" (_origin_)              \
>                 );

I wouldn't object, only I don't see the need for it. Present layout doesn't
stretch out of limits. As far as I have seen other architectures (coldfire,
ppc) use same layout. 

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #25 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-23 21:46:54 BST ---
(In reply to comment #24)

[snip]

> > cortexm_regs.h:
> > 
> > Ilija, I missed that you have used such an order for arguments in REV
> > macros, for example
> > 
> > +// Reverse word
> > +#define CYGARC_REV(_swapped_,_original_) \
> > +        __asm__ volatile ("rev %0, %1\n" : "=r"(_swapped_) : "r"(_original_))
> > +
> > 
> > It looks clear for ARM assembler gurus (dst <- src), and for C-guys
> > it looks a bit puzzled, (IMO) they are accustomed to same

[snip]

> I would insist to keep present order. Here are some arguments:
> 
> These macros shall be used mainly by people who are familiar with
> assembler and they would be surprised if order is not as in assembler.
> 
> Also there is a long tradition the destination to be first argument in
> C functions. The origins date back to big-bang, look for rationale in
> K&R book.  Examples strcpy(), strcat(), memcpy()...
>  

Ilija, thank you for your comments. I have to agree with such arguments
:-)

> > Still, I would use prototypes where result returns in the second
> > argument. And if you have no objections I would use multiline
> > variants for those macros.
> > 
> > #define CYGARC_REV( _origin_, _swapped_ )       \
> >     asm volatile( "rev %0, %1\n"                \
> >                   : "=r" (_swapped_)            \
> >                   : "r" (_origin_)              \
> >                 );
> 
> I wouldn't object, only I don't see the need for it. Present layout
> doesn't stretch out of limits. As far as I have seen other
> architectures (coldfire, ppc) use same layout.

I see. It seems it's a matter of taste, as for me I try to avoid the
1-liners near to 78th position.

So, I plan to check-in the sent patches with some minor clean-ups, of
course, if no one else has objections.

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.

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

[bugzilla-daemon]

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

--- Comment #26 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-24 07:51:12 BST ---
(In reply to comment #25)
> (In reply to comment #24)

> 
> I see. It seems it's a matter of taste, as for me I try to avoid the
> 1-liners near to 78th position.

Replacing __asm__ with asm may help little-bit. Also _original_ with _origin_ .

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

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

[bugzilla-daemon]

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

--- Comment #27 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-24 08:07:51 BST ---
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> 
> > I see. It seems it's a matter of taste, as for me I try to avoid the
> > 1-liners near to 78th position.
> 
> Replacing __asm__ with asm may help little-bit. Also _original_ with
> _origin_ .

Yes, to use ``_origin_`` is quite enougth.

Ilija, if you test your patches with existsing target in eCos AnonCVS,
could you, please, attach the used ECM?

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

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

[bugzilla-daemon]

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

--- Comment #28 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-24 14:34:06 BST ---
Created an attachment (id=1352)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1352)
ECM: Endian test target LPC-1766-STK

(In reply to comment #27)

> Ilija, if you test your patches with existsing target in eCos AnonCVS,
> could you, please, attach the used ECM?

I am sorry for delay, my last test were on target yet to come to AnonCVS. Now I
repeated the tests on LPC-1766-STK (ECM attached).

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #29 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-24 14:46:50 BST ---
(In reply to comment #28)
> Created an attachment (id=1352)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1352)
> ECM: Endian test target LPC-1766-STK
> 
> (In reply to comment #27)
> 
> > Ilija, if you test your patches with existsing target in eCos
> > AnonCVS, could you, please, attach the used ECM?
> 
> I am sorry for delay, my last test were on target yet to come to
> AnonCVS. Now I repeated the tests on LPC-1766-STK (ECM attached).

Thank you for ECM. This evening I want to test a few builds and if all
will pass smoothly, I'll check-in the patches. Once again thank you.

Sergei

> Ilija

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

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

[bugzilla-daemon]

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

--- Comment #30 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-24 17:42:58 BST ---
(In reply to comment #29)

> Thank you for ECM. This evening I want to test a few builds and if all
> will pass smoothly, I'll check-in the patches. Once again thank you.
> 

Sergei,
Thank you for your concern

I found a typo in Attachment 1349, a comment reflects older version.

Patch snippet:

-#endif // defined(CYG_CPU_TO_BE16(__val)) && defined(CYG_CPU_TO_BE32(__val))
+#endif // defined(CYGIMP_LWIP_ENDIAN_BY_HAL) && CYGIMP_LWIP_ENDIAN_BY_HAL

I guess you can fix this manually.

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #31 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-24 18:01:32 BST ---
(In reply to comment #30)

...

> I found a typo in Attachment 1349, a comment reflects older version.
> 
> Patch snippet:
> 
> -#endif // defined(CYG_CPU_TO_BE16(__val)) && defined(CYG_CPU_TO_BE32(__val))
> +#endif // defined(CYGIMP_LWIP_ENDIAN_BY_HAL) && CYGIMP_LWIP_ENDIAN_BY_HAL
> 
> I guess you can fix this manually.

Ilija, I have removed the comment yesterday :-) If idefs are not nested
and "wraps" about 1-5 lines of code I prefer to do not duplicate its
condition/check in a comment, especially if the condition is too long.
IMO, it looks clear and it's easier to read such blocks.

In any case, thank you for report.

Sergei
> 
> Ilija

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

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

[bugzilla-daemon]

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

--- Comment #32 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-24 18:55:27 BST ---
(In reply to comment #29)

> > AnonCVS. Now I repeated the tests on LPC-1766-STK (ECM attached).
>  
> Thank you for ECM. This evening I want to test a few builds and if all
> will pass smoothly, I'll check-in the patches. Once again thank you.

Ilija,

Your mini config has saved my time. I repeated that 4-items test matrix
(built eCos tests for hal/common, net/lwip_tcp) with different endian
options. Everything went fine.

Unfortunately, I have not Cortex-M targets and it was very helpful that
you have spent your time on testing.

Now your code in CVS.

Thank you very much for your work. Thank you for your patience, thank
you for your cooperation.

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.

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

[bugzilla-daemon]

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

--- Comment #33 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-25 09:01:40 BST ---
(In reply to comment #32)
> (In reply to comment #29)
> 
> > > AnonCVS. Now I repeated the tests on LPC-1766-STK (ECM attached).

[snip]

> 
> Unfortunately, I have not Cortex-M targets and it was very helpful that
> you have spent your time on testing.

If you need to put your hands on some specific device we have in our lab
(LPC-17xx, STM32, Kinetis) I can provide you remote access to our dev/eval
boards - through PEEDI, (invitation valid to eCos maintainers and
contributors).

> 
> Now your code in CVS.
> 
> Thank you very much for your work. Thank you for your patience, thank
> you for your cooperation.

Thank you for review and good advices. I'm glad we augmented eCos for Cortex-M
in a non-intrusive way.

OFF-TOPIC: There are few my bugs that have been checked in, but not yet
formally resolved in Bugzilla. Can I resolve them?

Ilija

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

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

[bugzilla-daemon]

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

--- Comment #34 from Sergei Gavrikov <sergei.gavrikov@gmail.com> 2011-08-25 09:19:53 BST ---
(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #29)
> > 
> > > > AnonCVS. Now I repeated the tests on LPC-1766-STK (ECM
> > > > attached).
> 
> [snip]
> 
> > Unfortunately, I have not Cortex-M targets and it was very helpful
> > that you have spent your time on testing.
> 
> If you need to put your hands on some specific device we have in our
> lab (LPC- 17xx, STM32, Kinetis) I can provide you remote access to our
> dev/eval boards - through PEEDI, (invitation valid to eCos maintainers
> and contributors).

It is tempting offer. I'll have it in mind. Thank you.

> > Now your code in CVS.
> > 
> > Thank you very much for your work. Thank you for your patience,
> > thank you for your cooperation.
> 
> Thank you for review and good advices. I'm glad we augmented eCos for
> Cortex-M in a non-intrusive way.
> 
> OFF-TOPIC: There are few my bugs that have been checked in, but not
> yet formally resolved in Bugzilla. Can I resolve them?

Sure! Please, reduce our backlog.

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.

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

[bugzilla-daemon]

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

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |CURRENTRELEASE

--- Comment #35 from Ilija Kocho <ilijak@siva.com.mk> 2011-08-25 15:03:30 BST ---
Checked out and re-rested. Compiles and works as expected.

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