On Wednesday, February 18, 2015 05:14:15 PM ron minnich wrote:
> Somebody make a decision and go for it. Just make sure it's type safe so we
> can catch it when I get it backwards.
write[8|16|32|64](void *addr, uint[8|16|32|64]_t val);
There. It's done :)
> Sorry I have been absent, somehow I got unsubscribed and did not notice for
> a while ...
>
Same thing happened to me.
Alex
On Wednesday, February 18, 2015 08:23:08 AM Vadim Bendebury wrote:
> On Wed, Feb 18, 2015 at 7:16 AM, Aaron Durbin via coreboot
>
> > 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.
>
> This is a great reason to keep those accessors, IMO it tramps other
> considerations voiced on this thread. Let's be consistent with other
> software systems.
>
Following that reasoning, we should switch to EFI style so we can better
integrate vendorcode:
read32 -> LibVendorMemRead(..., AccessWidth32, ...)
Doing what other software stacks do without a real reason, and just because
it's trendy...
Alex
On Wednesday, February 18, 2015 04:41:13 PM ron minnich wrote:
> Having spent 15 or so years on Plan 9 and Linux, the only conclusion I can
> make is that nothing is universal.
>
> Plan 9 is this:
> outl(int port, ulong value)
>
Finally an implementation that makes sense. Those out[lbw]() accessors always
get me.
> Linux got it backwards,
>
Thank you!
Alex
Once the order is the same on all architectures, we could change it
to match Linux at any point. I think its important to stop holding
up architectural unification in the name of the perfect solution.
The Coccinelle to switch the ordering is much safer to do after you have
the ordering consistent and both arguments to the write functions are
types which are incompatible with each other.
Personally I am in the camp of having the same order as Linux, but there
is a lot of ARM work held up because they are *different*. This is the
problem I am trying to solve. Lets make the best community decision
regardless of our personal opinions.
Kevin
> On Feb 18, 2015, at 8:40 AM, Aaron Durbin via coreboot <coreboot(a)coreboot.org> wrote:
>
> On Wed, Feb 18, 2015 at 10:35 AM, Patrick Georgi <pgeorgi(a)google.com <mailto:pgeorgi@google.com>> wrote:
>> 2015-02-18 17:23 GMT+01:00 Vadim Bendebury <vbendeb(a)chromium.org>:
>>>> 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.
>>>>
>> Since all imported code requires some rework before it works for us,
>> and redoing that particular issue is a matter of a simple spatch
>> (http://coccinelle.lip6.fr/), I'm not sure that's actually that big a
>> concern.
>>
>> @@
>> expression a,v;
>> @@
>> -writel(v,a)
>> +write32(a, v)
>
> Can you provide the spatch utility and script as well as the
> documentation so that every potential contributor can perform the
> necessary steps? Or we could be done w/ it once and for all and fix
> the existing coreboot semantics. Then there's not another step for
> anyone in perpetuity. Also keep in mind it's not just software going
> into coreboot but reusing software within coreboot as well. It's a 2
> way street.
>
> I did volunteer to do the big change, and if people want I will. Let me know.
>
> -Aaron
>
> --
> coreboot mailing list: coreboot(a)coreboot.org <mailto:coreboot@coreboot.org>
> http://www.coreboot.org/mailman/listinfo/coreboot <http://www.coreboot.org/mailman/listinfo/coreboot>
Having spent 15 or so years on Plan 9 and Linux, the only conclusion I can
make is that nothing is universal.
Plan 9 is this:
outl(int port, ulong value)
And I used software that came long before Linux (back in the 70s, if you
want to know) that followed that form too. I think Linux got it backwards,
but it's the standard, so I guess that's what people want.
I don't really care at this point, as long as whatever we use is defined
with a correct prototype, so we don't get errors when people from other
projects also get it "backwards", like me. It's just not that big a deal.
Let's stop fussing about it.
ron
On Wed, Feb 18, 2015 at 10:35 AM, Patrick Georgi <pgeorgi(a)google.com> wrote:
> 2015-02-18 17:23 GMT+01:00 Vadim Bendebury <vbendeb(a)chromium.org>:
>>> 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.
>>>
> Since all imported code requires some rework before it works for us,
> and redoing that particular issue is a matter of a simple spatch
> (http://coccinelle.lip6.fr/), I'm not sure that's actually that big a
> concern.
>
> @@
> expression a,v;
> @@
> -writel(v,a)
> +write32(a, v)
Can you provide the spatch utility and script as well as the
documentation so that every potential contributor can perform the
necessary steps? Or we could be done w/ it once and for all and fix
the existing coreboot semantics. Then there's not another step for
anyone in perpetuity. Also keep in mind it's not just software going
into coreboot but reusing software within coreboot as well. It's a 2
way street.
I did volunteer to do the big change, and if people want I will. Let me know.
-Aaron
2015-02-18 17:23 GMT+01:00 Vadim Bendebury <vbendeb(a)chromium.org>:
>> 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.
>>
Since all imported code requires some rework before it works for us,
and redoing that particular issue is a matter of a simple spatch
(http://coccinelle.lip6.fr/), I'm not sure that's actually that big a
concern.
@@
expression a,v;
@@
-writel(v,a)
+write32(a, v)
Patrick
--
Google Germany GmbH
ABC-Str. 19
20354 Hamburg
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
On Wed, Feb 18, 2015 at 7:16 AM, Aaron Durbin via coreboot
<coreboot(a)coreboot.org> wrote:
> On Tue, Feb 17, 2015 at 10:44 PM, Alexandru Gagniuc
>
> 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.
>
This is a great reason to keep those accessors, IMO it tramps other
considerations voiced on this thread. Let's be consistent with other
software systems.
--vb
> Alex, you've clearly stated your opinion you've not justified a reason
> for keeping the barrier.
>
> --
> coreboot mailing list: coreboot(a)coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
On Wed, Feb 18, 2015 at 9:16 AM, Aaron Durbin <adurbin(a)google.com> wrote:
> On Tue, Feb 17, 2015 at 10:44 PM, Alexandru Gagniuc
> <mr.nuke.me(a)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
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
