[coreboot] Unifying IO accessor macros

Aaron Durbin adurbin at google.com
Wed Feb 18 16:16:07 CET 2015


On Tue, Feb 17, 2015 at 10:44 PM, Alexandru Gagniuc
<mr.nuke.me at gmail.com> wrote:
> On Tuesday, February 17, 2015 04:32:18 PM Julius Werner wrote:
>> > 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;
>>
>> Oh, wow... that merged literally just this weekend. Talk about bad
>> timing. I wasn't aware this was going on (and none of the reviewers
>> bothered to tell me -.- ).
>>
> Well, I tried pinging you on IRC a few times, but I didn't realize you prefer
> ML over IRC. Sorry about that. To be fair, Ron did warn me about you being on
> the same tracks.
>
>> Guess that brings me back to the drawing board. I don't generally
>> disagree with your reasonings about why you consider certain patterns
>> better than others... at most I'd say that I don't think it's all that
>> important at the end of the day (writel() has always referred to 32
>> bits wherever I've seen it yet, and 64 could easily be writeq() if we
>> ever need it), and that doing things the way people are used to from
>> other/bigger projects (like Linux) might ultimately be more useful to
>> us than doing what we consider "the right way".
>>
> Well, we're firmware. We're special. We used to do hardware. Now we mostly
> deal with hardhardhardhardhardhardware. Every little imperfection propagates
> faster than light and multiplies exponentially. Linux doesn't deal with that.
> U-boot doesn't deal with that. Not quite a fair comparison.
>
>> But I really don't have a strong opinion in either direction, I just
>> care about unifying this and making sure there is a single, consistent
>> pattern everywhere. If most people here think write32(void *addr, u32
>> value) is the right way to go, I can instead switch ARM(64) over to
>> that. It would be a pretty massive change, but since all those boards
>> are Chromebooks for now I'm in a reasonably good position to ensure
>> everything stays working.
>>
> This reverse order hit me like a brick wall when I first started doing ARM on
> coreboot. But I was angry at ARM code for having gotten it wrong. I also hated
> uboot's setbits_le32(), but hey, at least it indicates bitness -- compare to
> setbits_lel().
>
>> One issue we're left with is libpayload, which I think should
>> definitely use the same pattern as coreboot. Right now it uses
>> writel(v,a) on all architectures (typed on ARM and untyped on x86). I
>> can change the library, but all external payloads will need to change
>> their call sites themselves. Are we okay with that?
>
> Personally, I'm okay with unifying the order/type of arguments. As long as the
> the payloads that we have in-tree are updated, the external ones shouldn't be
> that hard to do at a later time.
>


As I have noted on http://review.coreboot.org/#/c/7924/ it's very
short sighted to go this route. In assembling a coreboot stack (which
includes libpayload and the payload itself) the code usually comes
from different software systems. Those include libpayload, linux
kernel, u-boot, etc. They all have the write(val, addr) semantics. I
see no good reason to artificially erect an ever present barrier for
integrating code into a coreboot system.

Alex, you've clearly stated your opinion you've not justified a reason
for keeping the barrier.



More information about the coreboot mailing list