[coreboot] Unifying IO accessor macros

[Aaron Durbin]

On Wed, Feb 18, 2015 at 9:16 AM, Aaron Durbin <adurbin at google.com> wrote:
> 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.


Hit "send" accidentally. Anyway, I'd like to hear why it's OK to
purposefully keep this barrier in place.

Thanks.

-Aaron