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

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