[coreboot] [PATCH] flashrom - board enable - reconstruct table.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon Apr 20 15:25:33 CEST 2009
On 20.04.2009 14:28, Luc Verhaegen wrote:
> On Mon, Apr 20, 2009 at 01:17:43PM +0200, Carl-Daniel Hailfinger wrote:
>
>> On 20.04.2009 12:12, Luc Verhaegen wrote:
>>
>>> Flashrom: board_enables: reconstruct table.
>>>
>>> This patch restores the pciid based board matching table. It makes this
>>> table readable and hackable again, and the only disadvantage is that the
>>> right margin is way beyond the rather dogmatic 80. All 0x0000 pci ids have
>>> been string replaced by 0 to more easily spot missing ids, and extra
>>> comments have been added to explain how the various entries are used.
>>>
>>> Signed-Off-By: Luc Verhaegen <libv at skynet.be>
>>>
>>>
>> The extra comments are good, but "coreboot bios" in such a text really
>> hurts. Please change the word BIOS to all-caps and don't refer to
>> coreboot as a BIOS. Thanks.
>>
>
> These are the lines in question:
>
> * The coreboot ids are used two fold. When a coreboot bios is installed, it
> * uniquely matches the coreboot board identification string. When a legacy
> * bios is installed and when autodetection is not possible, these ids can be
> * used to identify the board through a command line argument.
>
> It is clear and concise.
>
> Let's take the other option: "when coreboot is installed" What does that
> mean? Did the user check out the tree, did he install some of the utils,
> or did he install a coreboot bios image into his rom?
>
Coreboot is NOT a BIOS. If you really insist calling coreboot a BIOS,
why not call it LinuxBIOS?
> Your statement is more about politics than anything else.
>
There was almost universal agreement about the name change from
LinuxBIOS to coreboot (well, I was unhappy initially, but I soon found
out that the absence of BIOS in the new name is really beneficial). One
of the reasons for the name change was that coreboot is NOT a BIOS nor
should it ever be called one.
Feel free to propose calling coreboot a BIOS in a separate RFC on this list.
>> The documentation part can be committed after one more iteration.
>>
>
> I would never have altered these comments if it wasn't for the previous
> flamewar. For those just tuning in, this is the thread in the
> mailarchives:
> http://www.coreboot.org/pipermail/coreboot/2009-January/044086.html
>
Ah yes, I forgot. In
http://www.coreboot.org/pipermail/coreboot/2009-January/044129.html you
promised to "create all board enable functions and table entries for the
foreseeable future for everyone who asks once we switch back to the old
layout [...] next to keeping X from degrading even further." Was that
irony or a real promise?
>> The rest of the patch makes board entries unreadable.
>>
>
> We had this one before. It makes it unreadable for you. But not for
> everyone.
Well, more flashrom developers on this list favour the multiline
solution than those opposed to it. That means I'm part of the majority.
And I do help people on IRC add board enables.
>> In the past few weeks, we had quite a few people in #coreboot who wanted
>> to add support for their boards. The presence of struct member names
>> helped immensely to explain how to add board enables. Some even figured
>> it out from similar entries. Lowering the barrier for possible future
>> developers is very important.
>>
>
> What makes you so certain that it is this change that made people add 4
> entries in 3 months? Couldn't that be because flashrom just is getting
> used more? And besides, this growth is not significantly faster than
> before.
>
That's a straw man. I didn't claim any of the points you try to question
above.
>> Since IIRC you said some time ago that the multiline layout hinders your
>> workflow, feel free to submit any new board enables in the single-line
>> layout. I'll fix them up.
>>
>
> Why is my workflow relevant?
If your workflow is irrelevant, why would we want to accommodate it?
> Do you really think that i have all my
> editors open at 160 chars all the time? My you are referring to my
> statements about being able to oversee everything more quickly with a
> condensed table. This is just common sense, everybody will have to
> acknowledge that one.
>
Unless you get to define everbody's common sense (which is inherently
subjective), it is unlikely that everybody (in the strict sense) will
have to acknowledge your view.
After all, I'm a person (and thus part of the "everybody" group of
people) and I disagree with you. You could claim that I don't have
common sense, but I hope you won't do that.
> [...]
> What will you do when it reaches a hundred or so entries? Will you be
> happy with a 1400line mess?
Is any table with 1400 lines a mess?
> Do you think that this table will stop
> growing at some point?
>
Why should it? Hardware will probably have quirks for the foreseeable
future.
> How many actual board enable functions will there be with multiple
> entries in the table? How much space will the actual enables take
> compared to the table?
>
I expect at least half a dozen entries with flash translation chips
because these boards exist out there.
> How will people tell whether something similar has already been added?
>
Like they do now.
> Will you impose some entry ordering scheme (which, in light of likely
> future table growth, is actually a useful change)?
>
Your patch doesn't touch that issue, so the point is orthogonal to our
discussion.
> It rapidly becomes non-obvious when you don't have a tight table. And i
> know what you are going to say here: just make sure that they are added
> in whatever order already. And my answer to that is: you can't. Left or
> right, entries will be added that aren't ordered, and afterwards no-one
> will spot them being unordered anymore. In a tight table, you spot them
> right away and can fix it easily on the next iteration.
>
See above. There are lots of possible ordering schemes, but deciding
which makes sense is a different matter.
> It really is creating a situation where you are unable to see the forest
> through the trees.
>
Grep works nicely.
> Also, you originally suggested to remove lines like = 0x0000; or = NULL;
> This is a very dangerous path. People will only see the last 2-3
> entries, and never look beyond. Remove a line, and it will not be
> thought of for the next entry. This is simply human behaviour.
>
So you suggest to keep these lines. Fine.
> What you then end up with is a whole range of boards where just a single
> pair of main ids, the coreboot ids, the string and the enable will be
> created. No autodetection will be provided any more, as the trivial
> addition of yet another argument-specified-only board is of course
> obvious for the person who added the board enable to begin with.
>
> The end result then is that all users will just randomly go through the
> board names for ages, all the time, some of which will work, as the
> amount of pci devices really isn't that great. This is quite a dangerous
> situation and a really bad user experience.
>
Adding unique subsystem IDs to that struct is hard. First, I know for a
fact that my laptop and a related model have identical subsystem IDs.
That alone is proof that subsystem IDs are not always unique. Unless you
know all subsystem IDs of all boards from a given manufacturer, you
can't say for sure whether the board enable you're adding subsystem IDs
for is actually _not_ going to trigger on another board.
To make this even more fun, which subsystem ID will you use
> With a condensed table, people are forced to pad, and will put more
> thought into their entries and will feel obliged to add subsystem ids.
>
You must be joking. If you can _prove_ that, you'll probably get a PhD
in psychology right away.
If pressing tab once really forces people to think for minutes about
something that is neither easy nor obvious, I want a tab key in every
conversation.
> This in turn leads to more autodetection and less people blindly forcing
> boardnames, and in general a better user experience.
>
> So maybe making it "easier" for people to "understand" what the
> different entries are for (which actually boils down to being able to
> ignore how the entries are used), is not a goal that should be pursued
> above other goals. For existing developers, at least those willing to
> not religiously stick to dogmas when it makes sense, the condensed table
> is the better option.
>
Let me modify one word of your statement to express my opinion:
For existing developers, at least those willing to
not religiously stick to dogmas when it makes sense, the multiline table
is the better option.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list