This is the mail archive of the
ecos-patches@sourceware.org
mailing list for the eCos project.
[Bug 1001275] Cortex-M (armV7) architecture endian instructions / Applied on lwIP
- From: bugzilla-daemon at bugs dot ecos dot sourceware dot org
- To: ecos-patches at ecos dot sourceware dot org
- Date: Fri, 19 Aug 2011 09:40:46 +0100
- Subject: [Bug 1001275] Cortex-M (armV7) architecture endian instructions / Applied on lwIP
- Auto-submitted: auto-generated
- References: <bug-1001275-104@http.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 #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 on the CC list for the bug.