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