Hello,
I am working on adding board_enable code for Artec Group DBE61 and DBE62 boards to utils/flashrom to set up everything fully for flash writing to work in all circumstances, but I've hit a problem in the infrastructure code for mainboard selection handling that I'd like to discuss on how to approach.
These boards are based on the AMD Geode line, so the PCI devices that exist for the outside are whatever the VSA exposes, which is typically the very same set of PCI devices that a big number of other Geode based products get exposed (save maybe network adapter and a few other things that can easily overlap with other boards). Therefore, it is undesirable to have the board PCI ID based auto-detection consider a board with Geode PCI devices a DBE61 without the user having said so specifically via -m artecgroup:dbe61 in scenarios when the ROM flash is empty or contains something else than a coreboot image with an appropriate name that would get auto-detected first.
However, the current infrastructure in board_enable.c doesn't allow this scenario, due to the combination of the following in the current code:
a) for a user specified board to be accepted, at least one set of IDs has to match to something that's on board (reasonable safeguard by itself); b) auto-detection is ran whenever a mainboard is not specified - the first board that has all non-NULL IDs existent is deemed the correct board even if the check would succeed for other boards in the list as well
So, as soon as I add a board entry to board_pciid_enables[] that describes one or two Geode VSA exposed PCI devices, I suspect all other Geode products will start getting auto-detected as the board I add, unless they happen to have a coreboot image with a name that is in the struct as well (lb_vendor/lb_name).
As I'm not sure how it's desired to handle this, I'm going to list the various approaches that I can think of, and will wait for comments and suggestions on the matter:
* Allow all NULL PCI IDs, making the board auto-detection skip such entries, and use it only when the coreboot name match succeeds or the user has specified the board with -m (--mainboard) argument. Pros: can leave such hardly identifiable (by PCI devices) out of the auto-detection logic Cons: No safeguarding at all
* Add a field to the struct (either treated as boolean, or a flag), that tells the logic if this entry should be considered in the auto-detection logic or skipped Pros: * can skip boards from PCI based auto-detection logic, requiring user specifying it; * has safeguards still in place (in this specific case will allow to claim it's a DBE61 only on Geode based products) Potential cons: extra struct field
* Make the PCI id based auto-detection logic run over all the struct entries, instead of going with the first that meets all checks. If multiple are found, report ambiguity and require a board name to be specified by the user. This can be considered independently of the above options. Pros: * Doesn't automatically pick the first suitable entry when there is an ambiguity, whereas the pick is made purely on internal struct entry order * Takes care of the possible wrong choice auto-detection in this specific case, as two boards (DBE61 and DBE62) would get added, triggering the ambiguity check. But doesn't help for future cases where two boards with the same IDs don't get introduced at once; also a user friendly ambiguity report would report only the listed entries with the matched IDs, while it could be a completely different board that just isn't listed yet
The patch I would like to see in SVN after this issue is taken care of is attached for reference. Without prior improvements to the general code in board_enable.c, applying this patch would make all Geode based products (that don't expose a separate PCI subsystem) be auto-detected as DBE61 (first in list) - if their ROM doesn't already have a coreboot image with a name that is listed in the struct. If this isn't a problem for the time being, then here's an addition to the commit log:
Signed-off-by: Mart Raudsepp mart.raudsepp@artecdesign.ee
Awaiting for comments and suggestions, Mart Raudsepp, Software Engineer, Artec Design LLC
Hello,
Ühel kenal päeval, R, 2008-02-08 kell 17:54, kirjutas Mart Raudsepp:
- Add a field to the struct (either treated as boolean, or a flag),
that tells the logic if this entry should be considered in the auto-detection logic or skipped Pros: * can skip boards from PCI based auto-detection logic, requiring user specifying it; * has safeguards still in place (in this specific case will allow to claim it's a DBE61 only on Geode based products) Potential cons: extra struct field
As there have been no replies on mailing list on this, I went ahead and made a simple patch for the above quoted approach. I remember that using an extra field was found to be the preferred approach by several people on IRC as well.
Attached you will find that patch, plus an accordingly modified DBE61/DBE62 board enable patch.
Comments or Acks? :)
Mart Raudsepp, Software Engineer, Artec Design LLC
Ühel kenal päeval, E, 2008-02-18 kell 15:44, kirjutas Mart Raudsepp:
- Add a field to the struct (either treated as boolean, or a flag),
that tells the logic if this entry should be considered in the auto-detection logic or skipped Pros: * can skip boards from PCI based auto-detection logic, requiring user specifying it; * has safeguards still in place (in this specific case will allow to claim it's a DBE61 only on Geode based products) Potential cons: extra struct field
As there have been no replies on mailing list on this, I went ahead and made a simple patch for the above quoted approach. I remember that using an extra field was found to be the preferred approach by several people on IRC as well.
Attached you will find that patch, plus an accordingly modified DBE61/DBE62 board enable patch.
Comments or Acks? :)
Luc Verhaegen pointed out that if subsystem IDs are NULL, then the entry already isn't considered in the auto-detection logic. I must have misread the logic and assume that even then the first match is just picked.
So now the attached patch ditches the extra flag adding and just adds board enable for DBE61 and DBE62 without specifying PCI subsytem IDs; and also documents that logic to avoid further confusion.
Mart Raudsepp, Software Engineer, Artec Design LLC
Isn't just about any LX800 board going to put FLASH on the cs5536?
One other option: have the code see how many boards match based on PCI ids. If it is more than one, ask the user which board it is by giving them a selection. That way, code won't stop at first match, and it can detect ambiguities.
ron
On Mon, Feb 18, 2008 at 08:12:30AM -0800, ron minnich wrote:
One other option: have the code see how many boards match based on PCI ids. If it is more than one, ask the user which board it is by giving them a selection. That way, code won't stop at first match, and it can detect ambiguities.
If there is ambiguity I think it's ok to require -m.
Printing all matching boards is a great idea!
//Peter
Ühel kenal päeval, E, 2008-02-18 kell 08:12, kirjutas ron minnich:
Isn't just about any LX800 board going to put FLASH on the cs5536?
You have three places it can be and the DIVIL_BALL_OPTS MSR registry needs to match that. If it's wrong, then the writing happens to the wrong place, which typically fails in our boards (as the "LPC dongle" BIOS emulator doesn't allow flashing over LPC bus, which you don't want anyway). These three locations are:
1) LPC ROM 2) Flash 3) Firmware HUB
The way I'm setting it in board_enable_dbe6x() is making board specific assumptions. If this can be later improved to work for all cs5536 boads, then I would be glad to see this unified.
One other option: have the code see how many boards match based on PCI ids. If it is more than one, ask the user which board it is by giving them a selection. That way, code won't stop at first match, and it can detect ambiguities.
Yup, that idea I also had in my thread starting e-mail as a third option that can be done independently. Would be nice in the long term. Though it doesn't cover cases where you only have one board entry at the time - imagine we would only add DBE61 and no DBE62, then an OLPC would be detected as DBE61 instead of reporting a bogus ambiguity between DBE61 and DBE62.
-- Regards, Mart Raudsepp, Software Engineer, Artec Design LLC
I think the best bet for mainboard guessing is: 1. flashrom should report all matching mainboards 2. flashrom should ALWAYS ask which of the matches to use (even if there is one!)
That's a simple way to do the guessing that ought to avoid accidentally writing the wrong image.
ron
On Mon, Feb 18, 2008 at 11:13:26PM -0800, ron minnich wrote:
I think the best bet for mainboard guessing is:
- flashrom should report all matching mainboards
- flashrom should ALWAYS ask which of the matches to use (even if
there is one!)
As long as there remains a way to make flashrom non-interactive, with a command line flag or so (-m some:board --dont-confirm).
Thanks, Ward.
On Feb 19, 2008 6:12 AM, Ward Vandewege ward@gnu.org wrote:
On Mon, Feb 18, 2008 at 11:13:26PM -0800, ron minnich wrote:
I think the best bet for mainboard guessing is:
- flashrom should report all matching mainboards
- flashrom should ALWAYS ask which of the matches to use (even if
there is one!)
As long as there remains a way to make flashrom non-interactive, with a command line flag or so (-m some:board --dont-confirm).
yes.
flashrom should always err on the side of caution. But a command-line override is fine.
ron
On Tue, Feb 19, 2008 at 08:49:41AM +0200, Mart Raudsepp wrote:
Ühel kenal päeval, E, 2008-02-18 kell 08:12, kirjutas ron minnich:
Isn't just about any LX800 board going to put FLASH on the cs5536?
You have three places it can be and the DIVIL_BALL_OPTS MSR registry needs to match that. If it's wrong, then the writing happens to the wrong place, which typically fails in our boards (as the "LPC dongle" BIOS emulator doesn't allow flashing over LPC bus, which you don't want anyway). These three locations are:
- LPC ROM
- Flash
- Firmware HUB
The way I'm setting it in board_enable_dbe6x() is making board specific assumptions. If this can be later improved to work for all cs5536 boads, then I would be glad to see this unified.
One other option: have the code see how many boards match based on PCI ids. If it is more than one, ask the user which board it is by giving them a selection. That way, code won't stop at first match, and it can detect ambiguities.
Yup, that idea I also had in my thread starting e-mail as a third option that can be done independently. Would be nice in the long term. Though it doesn't cover cases where you only have one board entry at the time - imagine we would only add DBE61 and no DBE62, then an OLPC would be detected as DBE61 instead of reporting a bogus ambiguity between DBE61 and DBE62.
We talked on IRC about using the MAC address of the nic as another piece of information that could be used by flashrom. With that, the OLPC would not be detected; the vendor part of the MAC address would not match.
Thanks, Ward.
Ühel kenal päeval, T, 2008-02-19 kell 09:10, kirjutas Ward Vandewege:
We talked on IRC about using the MAC address of the nic as another piece of information that could be used by flashrom. With that, the OLPC would not be detected; the vendor part of the MAC address would not match.
Maybe the NICs subsystem vendor ID could be used somewhat too. Some ethernet chips allow setting the PCI subsystem vendor and device ID in the EEPROM - is that a proper place to put Artec Group ID in in the first place? Then it would be a simple matter of listing the subsystem IDs for the network card to uniquely identify the board.
Regards, Mart Raudsepp Artec Design LLC
On Mon, Feb 18, 2008 at 04:37:59PM +0200, Mart Raudsepp wrote:
Luc Verhaegen pointed out that if subsystem IDs are NULL, then the entry already isn't considered in the auto-detection logic. I must have misread the logic and assume that even then the first match is just picked.
So now the attached patch ditches the extra flag adding and just adds board enable for DBE61 and DBE62 without specifying PCI subsytem IDs; and also documents that logic to avoid further confusion.
Mart Raudsepp, Software Engineer, Artec Design LLC
commit d42e4c4b6d8ce75429c1899613b704c940c96534 Author: Mart Raudsepp mart.raudsepp@artecdesign.ee Date: Mon Feb 18 16:35:17 2008 +0200
flashrom: Add board_enable for Artec Group DBE61 and DBE62 Also add a comment about NULL subsystem IDs leaving the board entry out of auto-detection logic. Signed-off-by: Mart Raudsepp <mart.raudsepp@artecdesign.ee>diff --git a/board_enable.c b/board_enable.c index 907774d..da5043f 100644 --- a/board_enable.c +++ b/board_enable.c @@ -28,6 +28,7 @@ #include <pci/pci.h> #include <stdint.h> #include <string.h> +#include <fcntl.h> #include "flash.h"
/* @@ -368,12 +369,77 @@ static int board_acorp_6a815epd(const char *name) }
/**
- Suited for Artec Group DBE61 and DBE62.
- */
+static int board_artecgroup_dbe6x(const char *name) +{ +#define DBE6x_MSR_DIVIL_BALL_OPTS 0x51400015 +#define DBE6x_PRI_BOOT_LOC_SHIFT (2) +#define DBE6x_BOOT_OP_LATCHED_SHIFT (8) +#define DBE6x_SEC_BOOT_LOC_SHIFT (10) +#define DBE6x_PRI_BOOT_LOC (3 << DBE6x_PRI_BOOT_LOC_SHIFT) +#define DBE6x_BOOT_OP_LATCHED (3 << DBE6x_BOOT_OP_LATCHED_SHIFT) +#define DBE6x_SEC_BOOT_LOC (3 << DBE6x_SEC_BOOT_LOC_SHIFT) +#define DBE6x_BOOT_LOC_FLASH (2) +#define DBE6x_BOOT_LOC_FWHUB (3)
- unsigned long msr[2];
- int msr_fd;
- unsigned long boot_loc;
- msr_fd = open("/dev/cpu/0/msr", O_RDWR);
- if (msr_fd == -1) {
perror("open /dev/cpu/0/msr");return -1;- }
- if (lseek(msr_fd, DBE6x_MSR_DIVIL_BALL_OPTS, SEEK_SET) == -1) {
perror("lseek");close(msr_fd);return -1;- }
- if (read(msr_fd, (void*) msr, 8) != 8) {
perror("read");close(msr_fd);return -1;- }
- if ((msr[0] & (DBE6x_BOOT_OP_LATCHED)) ==
(DBE6x_BOOT_LOC_FWHUB << DBE6x_BOOT_OP_LATCHED_SHIFT))boot_loc = DBE6x_BOOT_LOC_FWHUB;- else
boot_loc = DBE6x_BOOT_LOC_FLASH;- msr[0] &= ~(DBE6x_PRI_BOOT_LOC | DBE6x_SEC_BOOT_LOC);
- msr[0] |= ((boot_loc << DBE6x_PRI_BOOT_LOC_SHIFT) |
(boot_loc << DBE6x_SEC_BOOT_LOC_SHIFT));- if (lseek(msr_fd, DBE6x_MSR_DIVIL_BALL_OPTS, SEEK_SET) == -1) {
perror("lseek");close(msr_fd);return -1;- }
- if (write(msr_fd, (void*) msr, 8) != 8) {
perror("write");close(msr_fd);return -1;- }
- close(msr_fd);
- return 0;
+}
+/**
- We use 2 sets of IDs here, you're free to choose which is which. This
- is to provide a very high degree of certainty when matching a board on
- the basis of subsystem/card IDs. As not every vendor handles
- subsystem/card IDs in a sane manner.
- Keep the second set NULLed if it should be ignored.
*/
- Keep the subsystem IDs NULLed if they don't identify the board fully.
struct board_pciid_enable { /* Any device, but make it sensible, like the ISA bridge. */ @@ -427,6 +493,10 @@ struct board_pciid_enable board_pciid_enables[] = { "epox", "ep-bx3", "EPoX EP-BX3", board_epox_ep_bx3}, {0x8086, 0x1130, 0x0000, 0x0000, 0x105a, 0x0d30, 0x105a, 0x4d33, "acorp", "6a815epd", "Acorp 6A815EPD", board_acorp_6a815epd},
- {0x1022, 0x2090, 0x0000, 0x0000, 0x1022, 0x2080, 0x0000, 0x0000,
"artecgroup", "dbe61", "Artec Group DBE61", board_artecgroup_dbe6x},- {0x1022, 0x2090, 0x0000, 0x0000, 0x1022, 0x2080, 0x0000, 0x0000,
"artecgroup", "dbe62", "Artec Group DBE62", board_artecgroup_dbe6x},};
Acked-by: Luc Verhaegen libv@skynet.be
* Mart Raudsepp mart.raudsepp@artecdesign.ee [080218 14:44]:
Pros: * can skip boards from PCI based auto-detection logic, requiring user specifying it;
Why is that a pro? This means there are boards that will not work "out of the box" anymore. That looks like a CON right there.
What's the problem with autodetection?
Ühel kenal päeval, E, 2008-02-18 kell 17:07, kirjutas Stefan Reinauer:
- Mart Raudsepp mart.raudsepp@artecdesign.ee [080218 14:44]:
Pros: * can skip boards from PCI based auto-detection logic, requiring user specifying it;
Why is that a pro? This means there are boards that will not work "out of the box" anymore. That looks like a CON right there.
What's the problem with autodetection?
I'll just quote from the original mail that didn't see replies:
These boards are based on the AMD Geode line, so the PCI devices that exist for the outside are whatever the VSA exposes, which is typically the very same set of PCI devices that a big number of other Geode based products get exposed (save maybe network adapter and a few other things that can easily overlap with other boards). Therefore, it is undesirable to have the board PCI ID based auto-detection consider a board with Geode PCI devices a DBE61 without the user having said so specifically via -m artecgroup:dbe61 in scenarios when the ROM flash is empty or contains something else than a coreboot image with an appropriate name that would get auto-detected first.
...
So, as soon as I add a board entry to board_pciid_enables[] that describes one or two Geode VSA exposed PCI devices [with subsystem IDs specified], I suspect all other Geode products will start getting auto-detected as the board I add, unless they happen to have a coreboot image with a name that is in the struct as well (lb_vendor/lb_name).
-- Mart Raudsepp, Software Engineer, Artec Design LLC
* Mart Raudsepp mart.raudsepp@artecdesign.ee [080219 07:41]:
These boards are based on the AMD Geode line, so the PCI devices that exist for the outside are whatever the VSA exposes, which is typically the very same set of PCI devices that a big number of other Geode based products get exposed (save maybe network adapter and a few other things that can easily overlap with other boards). Therefore, it is undesirable to have the board PCI ID based auto-detection consider a board with Geode
Ah ok. That relates only to the PCI ID part. It should be sufficient to leave the PCI IDs set to 0, so they will never match.
-
Luc Verhaegen -
Mart Raudsepp -
Peter Stuge -
ron minnich -
Stefan Reinauer -
Ward Vandewege