From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22845 invoked by alias); 18 Aug 2011 21:14:47 -0000 Received: (qmail 22795 invoked by uid 22791); 18 Aug 2011 21:14:46 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00 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; Thu, 18 Aug 2011 21:14:29 +0000 Received: from localhost (hagrid.ecoscentric.com [127.0.0.1]) by mail.ecoscentric.com (Postfix) with ESMTP id 3531E2F78016 for ; Thu, 18 Aug 2011 22:14:28 +0100 (BST) Received: from mail.ecoscentric.com ([127.0.0.1]) by localhost (hagrid.ecoscentric.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6vjtyMntdCDv; Thu, 18 Aug 2011 22:14:25 +0100 (BST) From: bugzilla-daemon@bugs.ecos.sourceware.org To: ecos-patches@ecos.sourceware.org Subject: [Bug 1001275] Cortex-M (armV7) architecture endian instructions / Applied on lwIP X-Bugzilla-Reason: CC 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: sergei.gavrikov@gmail.com 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: References: X-Bugzilla-URL: http://bugs.ecos.sourceware.org/ Auto-Submitted: auto-generated Content-Type: text/plain; charset="UTF-8" MIME-Version: 1.0 Date: Thu, 18 Aug 2011 21:14:00 -0000 Message-Id: <20110818211425.932A32F78010@mail.ecoscentric.com> Mailing-List: contact ecos-patches-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: ecos-patches-owner@ecos.sourceware.org X-SW-Source: 2011-08/txt/msg00005.txt.bz2 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 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 _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 > > > > #include > > > > // 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 > > > > #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) _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 on the CC list for the bug.