New subject: Unifying IO accessor macros

17 Feb 2015

      On Tuesday, February 17, 2015 03:12:35 PM Julius Werner wrote:
...
I'd like to propose an API change/cleanup for a long-standing problem
with architecture-independent code that people have hit and privately
discussed/cursed multiple times already, but no one really had time to
make the big change/fix yet. (Some earlier discussion among Chromium
people about this is available at http://crbug.com/451388 )
For doing MMIO accesses to registers, x86 code has traditionally used
a function with a signature like this:
void write32(unsigned long addr, u32 value);  // equivalents for
read32(), write8(), etc.
While ARM (and based on that, ARM64) has been brought up with both of these:
void write32(u32 value, void *addr);
void writel(u32 value, void *addr);
There's two problems with this: first, the order of arguments (value
and address) is switched around between the two architectures, meaning
it is currently impossible to write any architecture-independent MMIO
code (which blocks important features like generalizing
src/lib/uart8250mem.c). Second, the x86 version is not type safe (it
takes addresses as integers instead of pointers), making it extremely
easy to accidentally swap address and value (especially due to all
this architecture confusion about which order is correct). Changing
the addresses to a pointer type will generate obvious compiler errors
for most of those screw-ups, which I think is a good thing that x86
could benefit from as well.
The x86 was recently fixed to take in void *. The order of arguments was 
discussed before, and the agreement is to use write*(addr, val);
This has the advantage of being consistent with other accessor functions, such 
as PCI, and follows the logical C ordering of the assignment operator.
destination = value;
...
I want to unify all architectures under a single, type safe version of
these macros that will remove the confusion and allow writing
architecture-independent MMIO code in the future. For this, I plan to:

Switch all existing uses of write32() (and friends) in ARM(64) code

to writel().
Please don't. Always use functions that indicate bitness.
...

Remove write32() from all architectures but x86.

See above point.
...

Add writel(u32 value, void *addr) which is compatible with other

architectures to x86.
What is writel? Is it 32 bits? 64 bits? This C70's mentality has to go away. 
We've worked pretty hard to go into the 21st century, with stdint types, and 
other "duh! This just makes sense" changes.
...

Pronounce write32(unsigned long addr, u32 value) deprecated for x86

and slowly phase it out over time (at least from common/recent code).
Already done. It's void * for the addr arg... just where we want it. And it 
indicates bitness, just like we want it.
...
After this we'll be converged on writel(u32 value, void *addr) as the
Please don't use AT&T-centric syntax. It's inconsistent with every other 
accessor in coreboot.
...
one and only way to do it across all architectures going forward. I
know that there are probably people who prefer the name write32()
aesthetically, or who have a strong preference for the
It's part of the critical system guidelines to use types that indicate 
bitness.
...
first-address-then-value parameter order. I personally don't really
Again, this matches the logical  order of the C programming language.
Maybe we want to switch coreboot to assembly? JK, JK.
...
like the way writel() looks myself, but at the end of the day it
doesn't make a huge difference. I think this is the best solution
But it does. You see, bitness is not obviuous from the likes of long, long 
long, int short, word, dword, etc. It's also compiler dependent, just adding 
to the overall confusion. Then what do you do for write64()... writell()? 
That's easily mistakeable for writel().
...
because it has strong practical advantages and will allow us to solve
the issue with the least amount of changes: most ARM(64) code already
uses writel(), libpayload uses writel() across all architectures, and
All it takes is a trivial semantic patch or short sed line to switch this. 
This is hardly any effort at all when compared to the efforts of integrating 
code from other codebases.
...
it's the standard for U-Boot and a majority of the Linux kernel which
We got it right. And consistent. Using codebases that are either as old as 
linux, or got it wrong, like uboot, is really not a fair comparison.
...
are popular import sources. Also, the amount of existing x86 code is
probably too large to ever change (and appropriately test) it all...
so it's useful to pick a solution that can leave the old untyped
write32() in place on x86 as a deprecated fallback.
Huh? It's almost trivial to do with a semantic patch.
...
Since the changes required for this would mostly affect ARM(64) SoCs
and boards, which are currently much more developed (and in flux) in
the Chromium tree, I'm planning to only change this there for now and
wait until it gets merged into coreboot mainline in the right order as
part of the current upstreaming process. I still wanted to float the
idea here first to give everyone a chance to chime in before it's all
said and done.
You're free to continue to provide writel() and friends as a fallback for ARM 
code picked from other codebases, but please consider that it's wrong.
Alex

Re: [coreboot] Unifying IO accessor macros