On Dec 29, 2013 7:17 PM, "ron minnich" rminnich@gmail.com wrote:
The functions are wrong in two ways.
on the x86 side, the addr should be a void *. Had we done this long ago we would have caught some bugs.
On the ARM side, the addr should be first, not last, to be consistent with other "addr"s in coreboot (e.g. pci space). So we'd like to fix the argument order being backwards too.
Yep. I think we had caught this a while back, but left it as-is to be consistent with the u-boot macros. But since we've corrected a few things along the way, such as giving them explicit sizes and doing type checking, I suppose we may as well fix the argument ordering to be consistent with the rest of coreboot.
It's been on my fix it list for months, it's pretty easy with coccinnelle, I just have not had time.
ron
On Sun, Dec 29, 2013 at 5:16 PM, mrnuke mr.nuke.me@gmail.com wrote:
So, I found this:
ARM: static inline void write32(uint32_t val, void *addr) x86: static inline void write32(unsigned long addr, uint32_t value)
Now, isn't a 4-byte memory write a 4-byte memory write independent of architecture? Forget about the fact that the x86 version takes an
unsigned
long instead of a void * for the address; the ARM and x86 variants have
their
arguments mixed up.
I actually came across this when trying to use our 8250 memmapped UART
code on
an ARM chip. The original code was written for x86, and hence, it can't
work
on ARM (yet).
I propose that these simple memory accessors be unified. Since they
refer to a
memory address, I propose void * for the address parameter. I don't
care what
order the parameters come in as long it's consistent between arches. Any takers?
Alex
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot