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.
Alex
On Tue, Feb 17, 2015 at 10:44 PM, Alexandru Gagniuc mr.nuke.me@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.
-
Aaron Durbin -
Alexandru Gagniuc