* [U-Boot] (no subject) @ 2009-06-04 10:27 Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 1/3] ARM: remove unused bit operations Daniel Mack 0 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 10:27 UTC (permalink / raw) To: u-boot The following patch series is needed to build U-Boot for ARM platforms with CONFIG_CMD_UBIFS set. The UBIFS layer uses bit operation functions which are currently only implemented for PPC, and the ARM bit operation definitions are unused and wrong. [PATCH 1/3] ARM: remove unused bit operations [PATCH 2/3] Add generic bit operations [PATCH 3/3] ARM: add unaligned macros ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/3] ARM: remove unused bit operations 2009-06-04 10:27 [U-Boot] (no subject) Daniel Mack @ 2009-06-04 10:27 ` Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 2/3] Add generic " Daniel Mack 0 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 10:27 UTC (permalink / raw) To: u-boot Remove include/asm-arm/bitops.h a bunch of 'external' marked functions from include/asm-arm/bitops.h. They are not implemented anywhere in the sources, so this forward declaration is wrong. Also remove the functions __set_bit, __clear_bit, __change_bit, __test_and_set_bit, __test_and_clear_bit and __test_and_change_bit. All these functions can be implemented in a generic fashion which will be done in the next patch. Signed-off-by: Daniel Mack <daniel@caiaq.de> --- include/asm-arm/bitops.h | 70 ---------------------------------------------- 1 files changed, 0 insertions(+), 70 deletions(-) diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h index 4b8bab2..3ffd4d5 100644 --- a/include/asm-arm/bitops.h +++ b/include/asm-arm/bitops.h @@ -20,76 +20,6 @@ #define smp_mb__before_clear_bit() do { } while (0) #define smp_mb__after_clear_bit() do { } while (0) -/* - * Function prototypes to keep gcc -Wall happy. - */ -extern void set_bit(int nr, volatile void * addr); - -static inline void __set_bit(int nr, volatile void *addr) -{ - ((unsigned char *) addr)[nr >> 3] |= (1U << (nr & 7)); -} - -extern void clear_bit(int nr, volatile void * addr); - -static inline void __clear_bit(int nr, volatile void *addr) -{ - ((unsigned char *) addr)[nr >> 3] &= ~(1U << (nr & 7)); -} - -extern void change_bit(int nr, volatile void * addr); - -static inline void __change_bit(int nr, volatile void *addr) -{ - ((unsigned char *) addr)[nr >> 3] ^= (1U << (nr & 7)); -} - -extern int test_and_set_bit(int nr, volatile void * addr); - -static inline int __test_and_set_bit(int nr, volatile void *addr) -{ - unsigned int mask = 1 << (nr & 7); - unsigned int oldval; - - oldval = ((unsigned char *) addr)[nr >> 3]; - ((unsigned char *) addr)[nr >> 3] = oldval | mask; - return oldval & mask; -} - -extern int test_and_clear_bit(int nr, volatile void * addr); - -static inline int __test_and_clear_bit(int nr, volatile void *addr) -{ - unsigned int mask = 1 << (nr & 7); - unsigned int oldval; - - oldval = ((unsigned char *) addr)[nr >> 3]; - ((unsigned char *) addr)[nr >> 3] = oldval & ~mask; - return oldval & mask; -} - -extern int test_and_change_bit(int nr, volatile void * addr); - -static inline int __test_and_change_bit(int nr, volatile void *addr) -{ - unsigned int mask = 1 << (nr & 7); - unsigned int oldval; - - oldval = ((unsigned char *) addr)[nr >> 3]; - ((unsigned char *) addr)[nr >> 3] = oldval ^ mask; - return oldval & mask; -} - -extern int find_first_zero_bit(void * addr, unsigned size); -extern int find_next_zero_bit(void * addr, int size, int offset); - -/* - * This routine doesn't need to be atomic. - */ -static inline int test_bit(int nr, const void * addr) -{ - return ((unsigned char *) addr)[nr >> 3] & (1U << (nr & 7)); -} /* * ffz = Find First Zero in word. Undefined if no zero exists, -- 1.6.3.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 10:27 ` [U-Boot] [PATCH 1/3] ARM: remove unused bit operations Daniel Mack @ 2009-06-04 10:27 ` Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 3/3] ARM: add unaligned macros Daniel Mack ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Daniel Mack @ 2009-06-04 10:27 UTC (permalink / raw) To: u-boot This adds generic bit operations for all platforms and enables includes the implementations from asm-arm. Code taken from Linux kernel. Signed-off-by: Daniel Mack <daniel@caiaq.de> --- include/asm-arm/bitops.h | 2 + include/asm-generic/bitops.h | 151 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 0 deletions(-) create mode 100644 include/asm-generic/bitops.h diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h index 3ffd4d5..97abea6 100644 --- a/include/asm-arm/bitops.h +++ b/include/asm-arm/bitops.h @@ -15,6 +15,8 @@ #ifndef __ASM_ARM_BITOPS_H #define __ASM_ARM_BITOPS_H +#include "asm-generic/bitops.h" + #ifdef __KERNEL__ #define smp_mb__before_clear_bit() do { } while (0) diff --git a/include/asm-generic/bitops.h b/include/asm-generic/bitops.h new file mode 100644 index 0000000..088e5da --- /dev/null +++ b/include/asm-generic/bitops.h @@ -0,0 +1,151 @@ +#ifndef _ASM_GENERIC_BITOPS_H_ +#define _ASM_GENERIC_BITOPS_H_ + +#include <asm/types.h> + +#define BIT(nr) (1UL << (nr)) +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) +#define BITS_PER_BYTE 8 +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) + +/** + * set_bit - Set a bit in memory + * @nr: the bit to set + * @addr: the address to start counting from + * + * Unlike set_bit(), this function is non-atomic and may be reordered. + * If it's called on the same region of memory simultaneously, the effect + * may be that only one operation succeeds. + */ +static inline void set_bit(int nr, volatile unsigned long *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); + + *p |= mask; +} + +static inline void clear_bit(int nr, volatile unsigned long *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); + + *p &= ~mask; +} + +/** + * change_bit - Toggle a bit in memory + * @nr: the bit to change + * @addr: the address to start counting from + * + * Unlike change_bit(), this function is non-atomic and may be reordered. + * If it's called on the same region of memory simultaneously, the effect + * may be that only one operation succeeds. + */ +static inline void change_bit(int nr, volatile unsigned long *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); + + *p ^= mask; +} + +/** + * test_and_set_bit - Set a bit and return its old value + * @nr: Bit to set + * @addr: Address to count from + * + * This operation is non-atomic and can be reordered. + * If two examples of this operation race, one can appear to succeed + * but actually fail. You must protect multiple accesses with a lock. + */ +static inline int test_and_set_bit(int nr, volatile unsigned long *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); + unsigned long old = *p; + + *p = old | mask; + return (old & mask) != 0; +} + +/** + * test_and_clear_bit - Clear a bit and return its old value + * @nr: Bit to clear + * @addr: Address to count from + * + * This operation is non-atomic and can be reordered. + * If two examples of this operation race, one can appear to succeed + * but actually fail. You must protect multiple accesses with a lock. + */ +static inline int test_and_clear_bit(int nr, volatile unsigned long *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); + unsigned long old = *p; + + *p = old & ~mask; + return (old & mask) != 0; +} + +/* WARNING: non atomic and it can be reordered! */ +static inline int test_and_change_bit(int nr, + volatile unsigned long *addr) +{ + unsigned long mask = BIT_MASK(nr); + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); + unsigned long old = *p; + + *p = old ^ mask; + return (old & mask) != 0; +} + +/** + * test_bit - Determine whether a bit is set + * @nr: bit number to test + * @addr: Address to start counting from + */ +static inline int test_bit(int nr, const volatile unsigned long *addr) +{ + return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1))); +} + +/** + * fls - find last (most-significant) bit set + * @x: the word to search + * + * This is defined the same way as ffs. + * Note fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32. + */ + +static inline int fls(int x) +{ + int r = 32; + + if (!x) + return 0; + if (!(x & 0xffff0000u)) { + x <<= 16; + r -= 16; + } + if (!(x & 0xff000000u)) { + x <<= 8; + r -= 8; + } + if (!(x & 0xf0000000u)) { + x <<= 4; + r -= 4; + } + if (!(x & 0xc0000000u)) { + x <<= 2; + r -= 2; + } + if (!(x & 0x80000000u)) { + x <<= 1; + r -= 1; + } + return r; +} + +#endif /* _ASM_GENERIC_BITOPS_H_ */ -- 1.6.3.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 3/3] ARM: add unaligned macros 2009-06-04 10:27 ` [U-Boot] [PATCH 2/3] Add generic " Daniel Mack @ 2009-06-04 10:27 ` Daniel Mack 2009-06-04 17:42 ` Daniel Mack 2009-06-04 11:45 ` [U-Boot] [PATCH 2/3] Add generic bit operations Wolfgang Denk ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 10:27 UTC (permalink / raw) To: u-boot Signed-off-by: Daniel Mack <daniel@caiaq.de> --- include/asm-arm/unaligned.h | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-) create mode 100644 include/asm-arm/unaligned.h diff --git a/include/asm-arm/unaligned.h b/include/asm-arm/unaligned.h new file mode 100644 index 0000000..dd7d852 --- /dev/null +++ b/include/asm-arm/unaligned.h @@ -0,0 +1,14 @@ +#ifndef _ASM_ARM_UNALIGNED_H +#define _ASM_ARM_UNALIGNED_H + +#ifdef __KERNEL__ + +#include <linux/unaligned/access_ok.h> +#include <linux/unaligned/generic.h> + +#define get_unaligned __get_unaligned_le +#define put_unaligned __put_unaligned_le + +#endif /* __KERNEL__ */ +#endif /* _ASM_ARM_UNALIGNED_H */ + -- 1.6.3.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 3/3] ARM: add unaligned macros 2009-06-04 10:27 ` [U-Boot] [PATCH 3/3] ARM: add unaligned macros Daniel Mack @ 2009-06-04 17:42 ` Daniel Mack 2009-06-04 19:03 ` Wolfgang Denk 0 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 17:42 UTC (permalink / raw) To: u-boot On Thu, Jun 04, 2009 at 12:27:21PM +0200, Daniel Mack wrote: > --- > include/asm-arm/unaligned.h | 14 ++++++++++++++ > 1 files changed, 14 insertions(+), 0 deletions(-) > create mode 100644 include/asm-arm/unaligned.h This one was too easy, updated patch below. With that one applied, the lzo1x decompressor and the whole ubifs works fine on an ARM PXA3xx. Daniel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 3/3] ARM: add unaligned macros 2009-06-04 17:42 ` Daniel Mack @ 2009-06-04 19:03 ` Wolfgang Denk 2009-06-04 19:23 ` Daniel Mack 0 siblings, 1 reply; 18+ messages in thread From: Wolfgang Denk @ 2009-06-04 19:03 UTC (permalink / raw) To: u-boot Dear Daniel Mack, In message <20090604174208.GM26688@buzzloop.caiaq.de> you wrote: > > +static inline u16 get_unaligned_le16(const void *p) > +{ > + return __get_unaligned_cpu16((const u8 *)p); > +} > + > +static inline u32 get_unaligned_le32(const void *p) > +{ > + return __get_unaligned_cpu32((const u8 *)p); > +} > + > +static inline u64 get_unaligned_le64(const void *p) > +{ > + return __get_unaligned_cpu64((const u8 *)p); > +} Are these 3 really all "u8" pointers, or is this a copy & paste error? Is there any guarantee that such macros are never used on device registers and the like? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "It is better to have tried and failed than to have failed to try, but the result's the same." - Mike Dennison ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 3/3] ARM: add unaligned macros 2009-06-04 19:03 ` Wolfgang Denk @ 2009-06-04 19:23 ` Daniel Mack 2009-06-05 3:21 ` Stefan Roese 0 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 19:23 UTC (permalink / raw) To: u-boot On Thu, Jun 04, 2009 at 09:03:47PM +0200, Wolfgang Denk wrote: > > +static inline u16 get_unaligned_le16(const void *p) > > +{ > > + return __get_unaligned_cpu16((const u8 *)p); > > +} > > + > > +static inline u32 get_unaligned_le32(const void *p) > > +{ > > + return __get_unaligned_cpu32((const u8 *)p); > > +} > > + > > +static inline u64 get_unaligned_le64(const void *p) > > +{ > > + return __get_unaligned_cpu64((const u8 *)p); > > +} > > Are these 3 really all "u8" pointers, or is this a copy & paste error? Yes, this is how it came from the Linux kernel and my tests show that these access methods work well. > Is there any guarantee that such macros are never used on device > registers and the like? Well - how can I guarantee that? Anyway - the functions can be enhanced later to make them work with different types of memories. For now, they implement a working set of functions to allow ubifs (and probably other code as well) to be compiled and ran on ARMs. Daniel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 3/3] ARM: add unaligned macros 2009-06-04 19:23 ` Daniel Mack @ 2009-06-05 3:21 ` Stefan Roese 0 siblings, 0 replies; 18+ messages in thread From: Stefan Roese @ 2009-06-05 3:21 UTC (permalink / raw) To: u-boot On Thursday 04 June 2009 21:23:31 Daniel Mack wrote: > > Is there any guarantee that such macros are never used on device > > registers and the like? > > Well - how can I guarantee that? Anyway - the functions can be enhanced > later to make them work with different types of memories. For now, they > implement a working set of functions to allow ubifs (and probably other > code as well) to be compiled and ran on ARMs. Yes. I suggest that we just document that these functions (and the set_bit()... ones) don't implement any memory barriers/sync operations and therefore should be handled with care when used on IO registers etc (on platforms that need such barriers like PPC). Best regards, Stefan ===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de ===================================================================== ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 10:27 ` [U-Boot] [PATCH 2/3] Add generic " Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 3/3] ARM: add unaligned macros Daniel Mack @ 2009-06-04 11:45 ` Wolfgang Denk 2009-06-04 11:48 ` Daniel Mack 2009-06-04 11:47 ` Wolfgang Denk 2009-06-05 20:44 ` Jean-Christophe PLAGNIOL-VILLARD 3 siblings, 1 reply; 18+ messages in thread From: Wolfgang Denk @ 2009-06-04 11:45 UTC (permalink / raw) To: u-boot Dear Daniel Mack, In message <1244111241-32735-3-git-send-email-daniel@caiaq.de> you wrote: > This adds generic bit operations for all platforms and enables includes > the implementations from asm-arm. Be careful. I am not sure if a generic definition is even possible. In any case, it is extremely risky to use in the wrong way. It's nothing but faux ami. My recommendation is NEVER to use any code based on bit numbers (even if this is what some chip documentation may refer to) but ONLY use appropriate masks instead. > new file mode 100644 > index 0000000..088e5da > --- /dev/null > +++ b/include/asm-generic/bitops.h > @@ -0,0 +1,151 @@ > +#ifndef _ASM_GENERIC_BITOPS_H_ > +#define _ASM_GENERIC_BITOPS_H_ > + > +#include <asm/types.h> > + > +#define BIT(nr) (1UL << (nr)) > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > +#define BITS_PER_BYTE 8 > +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) You see, this is plain wrong on PowerPC. On PowerPC, bit number 0 is the most significant bit, not the least significant one as you assume here. So using this well-intended code on a PowerPC system will most likely get you in trouble. [And you cannot even use a generic definition for PowerPC, as some- thing like "#define BIT(nr) (0x80000000 >> (nr))" will only work on 32 bit systems but be wrong on 64 bit ones.] Let's get rid of this stuff, it is confusing. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "The one charm of marriage is that it makes a life of deception a neccessity." - Oscar Wilde ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 11:45 ` [U-Boot] [PATCH 2/3] Add generic bit operations Wolfgang Denk @ 2009-06-04 11:48 ` Daniel Mack 2009-06-04 11:58 ` Wolfgang Denk 0 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 11:48 UTC (permalink / raw) To: u-boot On Thu, Jun 04, 2009 at 01:45:22PM +0200, Wolfgang Denk wrote: > > +#define BIT(nr) (1UL << (nr)) > > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > +#define BITS_PER_BYTE 8 > > +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > > You see, this is plain wrong on PowerPC. > > On PowerPC, bit number 0 is the most significant bit, not the least > significant one as you assume here. So using this well-intended code > on a PowerPC system will most likely get you in trouble. Well, the idea is to let those platforms use the generic operations that do not bring their owm ones. The code above is not on use by PPC, so it doesn't harm either. > [And you cannot even use a generic definition for PowerPC, as some- > thing like "#define BIT(nr) (0x80000000 >> (nr))" will only work on > 32 bit systems but be wrong on 64 bit ones.] That is why PPC implements that in its own fashion. Fair enough. > Let's get rid of this stuff, it is confusing. Hmm, and how? Daniel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 11:48 ` Daniel Mack @ 2009-06-04 11:58 ` Wolfgang Denk 0 siblings, 0 replies; 18+ messages in thread From: Wolfgang Denk @ 2009-06-04 11:58 UTC (permalink / raw) To: u-boot Dear Daniel Mack, In message <20090604114818.GH26688@buzzloop.caiaq.de> you wrote: > > > > +#define BIT(nr) (1UL << (nr)) > > > +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) > > > +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > > +#define BITS_PER_BYTE 8 > > > +#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long)) > > > > You see, this is plain wrong on PowerPC. > > > > On PowerPC, bit number 0 is the most significant bit, not the least > > significant one as you assume here. So using this well-intended code > > on a PowerPC system will most likely get you in trouble. > > Well, the idea is to let those platforms use the generic operations that > do not bring their owm ones. The code above is not on use by PPC, so it > doesn't harm either. But it is not a generic operation. The notion of "bit number" is a generic concept, but here we have a machine dependent implementation that sails under the name "asm-generic/bitops.h". But it is NOT generic. > > Let's get rid of this stuff, it is confusing. > > Hmm, and how? Just don't use it. Use masks instead of bit numbers. What's wrong with using 0x00008000 instead of BIT(15) (which would be 0x00010000 on 32 bit Power systems). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Prof: So the American government went to IBM to come up with a data encryption standard and they came up with ... Student: EBCDIC! ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 10:27 ` [U-Boot] [PATCH 2/3] Add generic " Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 3/3] ARM: add unaligned macros Daniel Mack 2009-06-04 11:45 ` [U-Boot] [PATCH 2/3] Add generic bit operations Wolfgang Denk @ 2009-06-04 11:47 ` Wolfgang Denk 2009-06-04 11:54 ` Daniel Mack 2009-06-05 20:44 ` Jean-Christophe PLAGNIOL-VILLARD 3 siblings, 1 reply; 18+ messages in thread From: Wolfgang Denk @ 2009-06-04 11:47 UTC (permalink / raw) To: u-boot Dear Daniel Mack, In message <1244111241-32735-3-git-send-email-daniel@caiaq.de> you wrote: > This adds generic bit operations for all platforms and enables includes > the implementations from asm-arm. I should have read the patch to end... > +static inline void set_bit(int nr, volatile unsigned long *addr) > +{ > + unsigned long mask = BIT_MASK(nr); > + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); > + > + *p |= mask; > +} > + > +static inline void clear_bit(int nr, volatile unsigned long *addr) > +{ > + unsigned long mask = BIT_MASK(nr); > + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); > + > + *p &= ~mask; > +} Such code has no chance of being accepted anyway. It tries to be generic and does not care if you use it on memory or device addresses - but for device addresses, the use of proper I/O accessor functions is mandatory. Full NAK. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de I really hate this damned machine It never does quite what I want I wish that they would sell it. But only what I tell it. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 11:47 ` Wolfgang Denk @ 2009-06-04 11:54 ` Daniel Mack [not found] ` <20090604115922.E6893832E416@gemini.denx.de> 0 siblings, 1 reply; 18+ messages in thread From: Daniel Mack @ 2009-06-04 11:54 UTC (permalink / raw) To: u-boot On Thu, Jun 04, 2009 at 01:47:17PM +0200, Wolfgang Denk wrote: > > +static inline void clear_bit(int nr, volatile unsigned long *addr) > > +{ > > + unsigned long mask = BIT_MASK(nr); > > + unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr); > > + > > + *p &= ~mask; > > +} > > Such code has no chance of being accepted anyway. > > It tries to be generic and does not care if you use it on memory or > device addresses - but for device addresses, the use of proper I/O > accessor functions is mandatory. And the functions I removed from asm-arm/bitops.h did that? ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <20090604115922.E6893832E416@gemini.denx.de>]
* [U-Boot] [PATCH 2/3] Add generic bit operations [not found] ` <20090604115922.E6893832E416@gemini.denx.de> @ 2009-06-04 18:00 ` Daniel Mack 2009-06-04 18:18 ` Stefan Roese 2009-06-04 19:06 ` Wolfgang Denk 0 siblings, 2 replies; 18+ messages in thread From: Daniel Mack @ 2009-06-04 18:00 UTC (permalink / raw) To: u-boot On Thu, Jun 04, 2009 at 01:59:22PM +0200, Wolfgang Denk wrote: > > And the functions I removed from asm-arm/bitops.h did that? > > No. Let's be happy that we have eliminated some poor code, and if we > add a replacement, let's make sure not to repeat the mistakes of the > past again. Ok. I just saw ubifs implementing its own set_bit() functions and considered that the wrong place for such functions to live in. ext2 seems to do the same things, also minix. Platforms which want to enable support for these filesystems have to do an evil #define to map ext2_set_bit() to the platform specific version. Which is all bogus, you might agree. And because of that situation, ubifs can't currently build for anything else than ppc (according to 'git grep -w fls include/asm*'). Hence I thought it might be a good idea (or at least a good start thereof) to put the functions where I believe they belong to - a 'generic' place. Anyway - applying the first patch of this series would at least prevent others from being mislead by dead and wrong code. Daniel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 18:00 ` Daniel Mack @ 2009-06-04 18:18 ` Stefan Roese 2009-06-04 19:06 ` Wolfgang Denk 1 sibling, 0 replies; 18+ messages in thread From: Stefan Roese @ 2009-06-04 18:18 UTC (permalink / raw) To: u-boot On Thursday 04 June 2009 20:00:42 Daniel Mack wrote: > On Thu, Jun 04, 2009 at 01:59:22PM +0200, Wolfgang Denk wrote: > > > And the functions I removed from asm-arm/bitops.h did that? > > > > No. Let's be happy that we have eliminated some poor code, and if we > > add a replacement, let's make sure not to repeat the mistakes of the > > past again. > > Ok. I just saw ubifs implementing its own set_bit() functions and > considered that the wrong place for such functions to live in. ext2 > seems to do the same things, also minix. Platforms which want to > enable support for these filesystems have to do an evil #define to > map ext2_set_bit() to the platform specific version. Which is all > bogus, you might agree. Ack. It would a good thing to have those functions available for all architectures so that common code like UBIFS etc can use it. Please note that UBIFS uses these bit functions for memory operations only (not IO access). So we don't have to use accessor functions etc here. The "standard" Linux implementation should do. > And because of that situation, ubifs can't > currently build for anything else than ppc > (according to 'git grep -w fls include/asm*'). > > Hence I thought it might be a good idea (or at least a good start > thereof) to put the functions where I believe they belong to - a > 'generic' place. Yes, I do like this approach. And from my point of view, we should continue this way. Best regards, Stefan ===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de ===================================================================== ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 18:00 ` Daniel Mack 2009-06-04 18:18 ` Stefan Roese @ 2009-06-04 19:06 ` Wolfgang Denk 1 sibling, 0 replies; 18+ messages in thread From: Wolfgang Denk @ 2009-06-04 19:06 UTC (permalink / raw) To: u-boot Dear Daniel Mack, In message <20090604180042.GN26688@buzzloop.caiaq.de> you wrote: > > Ok. I just saw ubifs implementing its own set_bit() functions and > considered that the wrong place for such functions to live in. ext2 > seems to do the same things, also minix. Platforms which want to > enable support for these filesystems have to do an evil #define to > map ext2_set_bit() to the platform specific version. Which is all > bogus, you might agree. And because of that situation, ubifs can't > currently build for anything else than ppc > (according to 'git grep -w fls include/asm*'). Yes, we have a mess here, and we need toclean this up ASAP. I'm trying to find out if there is any agreement what a portable set of I/O and bit macro definitions should look like. > Anyway - applying the first patch of this series would at least prevent > others from being mislead by dead and wrong code. ACK on that. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de There are three ways to get something done: do it yourself, hire someone, or forbid your kids to do it. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-04 10:27 ` [U-Boot] [PATCH 2/3] Add generic " Daniel Mack ` (2 preceding siblings ...) 2009-06-04 11:47 ` Wolfgang Denk @ 2009-06-05 20:44 ` Jean-Christophe PLAGNIOL-VILLARD 2009-06-07 22:41 ` Daniel Mack 3 siblings, 1 reply; 18+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-06-05 20:44 UTC (permalink / raw) To: u-boot On 12:27 Thu 04 Jun , Daniel Mack wrote: > This adds generic bit operations for all platforms and enables includes > the implementations from asm-arm. > > Code taken from Linux kernel. > the __set_bit, __clear_bit, __change_bit & co generic is used on arm ok but be aware that we use it in U-Boot so NACK and please do not rename __xxx by xxx they do not mean the same think the __xxx can be re-ordered and the xxx not IMHO please keep the linux naming to simplify think in the futur and please specify which file it's import and which kernel version Best Regards, J. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/3] Add generic bit operations 2009-06-05 20:44 ` Jean-Christophe PLAGNIOL-VILLARD @ 2009-06-07 22:41 ` Daniel Mack 0 siblings, 0 replies; 18+ messages in thread From: Daniel Mack @ 2009-06-07 22:41 UTC (permalink / raw) To: u-boot On Fri, Jun 05, 2009 at 10:44:21PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > This adds generic bit operations for all platforms and enables includes > > the implementations from asm-arm. > > > > Code taken from Linux kernel. > > > the __set_bit, __clear_bit, __change_bit & co generic is used on arm ok Do you mean it worked before? Or are you referring to the version I posted which you think is ok? > but be aware that we use it in U-Boot so NACK You use what? The functions I removed? I doubt that. The functions marked 'external' have no implementation, and I also don't believe the static inlines did the right thing (note the shift operations). > and please do not rename __xxx by xxx they do not mean the same think > > the __xxx can be re-ordered and the xxx not Sorry, I don't get it - what's your point about the status quo in the sources and about the patches? Daniel ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2009-06-07 22:41 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-04 10:27 [U-Boot] (no subject) Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 1/3] ARM: remove unused bit operations Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 2/3] Add generic " Daniel Mack 2009-06-04 10:27 ` [U-Boot] [PATCH 3/3] ARM: add unaligned macros Daniel Mack 2009-06-04 17:42 ` Daniel Mack 2009-06-04 19:03 ` Wolfgang Denk 2009-06-04 19:23 ` Daniel Mack 2009-06-05 3:21 ` Stefan Roese 2009-06-04 11:45 ` [U-Boot] [PATCH 2/3] Add generic bit operations Wolfgang Denk 2009-06-04 11:48 ` Daniel Mack 2009-06-04 11:58 ` Wolfgang Denk 2009-06-04 11:47 ` Wolfgang Denk 2009-06-04 11:54 ` Daniel Mack [not found] ` <20090604115922.E6893832E416@gemini.denx.de> 2009-06-04 18:00 ` Daniel Mack 2009-06-04 18:18 ` Stefan Roese 2009-06-04 19:06 ` Wolfgang Denk 2009-06-05 20:44 ` Jean-Christophe PLAGNIOL-VILLARD 2009-06-07 22:41 ` Daniel Mack
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.