Re: [coreboot] Inconsistent low-level arch functions between ARM and x86

[David Hendricks]

On Dec 29, 2013 7:17 PM, "ron minnich" <[e-mail]> 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 <[e-mail]> 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