| Submitter | Stefan Reinauer |
|---|---|
| Date | 2010-02-04 00:51:58 |
| Message ID | <4B6A1A2E.9090004@coresystems.de> |
| Download | mbox | patch |
| Permalink | /patch/871/ |
| State | Bitrotted |
| Headers | show |
Comments
On Thu, Feb 04, 2010 at 01:51:58AM +0100, Stefan Reinauer wrote: > See patch. This is what I had to do to get the combo working again. > * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 > * get Winbond W39V080FA working again. this was apparently broken in r868 > * tested SST49LF008A and Winbond W39V080FA > > Signed-off-by: Stefan Reinauer <stepan@coresystems.de> > Index: board_enable.c > =================================================================== > --- board_enable.c (revision 889) > +++ board_enable.c (working copy) > @@ -1221,7 +1221,7 @@ > {0x1166, 0x0205, 0x1014, 0x0347, 0x1002, 0x515E, 0x1014, 0x0325, NULL, NULL, NULL, "IBM", "x3455", 0, board_ibm_x3455}, > {0x1039, 0x5513, 0x8086, 0xd61f, 0x1039, 0x6330, 0x8086, 0xd61f, NULL, NULL, NULL, "Intel", "D201GLY", 0, wbsio_check_for_spi}, > {0x1022, 0x7468, 0, 0, 0, 0, 0, 0, NULL, "iwill", "dk8_htx", "IWILL", "DK8-HTX", 0, w83627hf_gpio24_raise_2e}, > - {0x8086, 0x27A0, 0, 0, 0x8086, 0x27b8, 0, 0, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, > + {0x8086, 0x27A0, 0x8086, 0x27a0, 0x8086, 0x27b8, 0x8086, 0x27b8, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, > {0x8086, 0x2411, 0x8086, 0x2411, 0x8086, 0x7125, 0x0e11, 0xb165, NULL, NULL, NULL, "Mitac", "6513WU", 0, board_mitac_6513wu}, > {0x13f6, 0x0111, 0x1462, 0x5900, 0x1106, 0x3177, 0x1106, 0, NULL, NULL, NULL, "MSI", "MS-6590 (KT4 Ultra)", 0, board_msi_kt4v}, > {0x1106, 0x3149, 0x1462, 0x7094, 0x10ec, 0x8167, 0x1462, 0x094c, NULL, NULL, NULL, "MSI", "MS-6702E (K8T Neo2-F)", 0, w83627thf_gpio4_4_raise_2e}, Nack. Those are bogus subsystem ids. Luc Verhaegen.
On 04.02.2010 01:51, Stefan Reinauer wrote: > * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 > Just a short question: The Kontron 986LCD-M appeared to be running coreboot. Is that correct? Quoting from the failure log you sent earlier: > flashrom v0.9.1-r2816 > Found candidate at: 00000500-00000510 > Found coreboot table at 0x00000500. > Found candidate at: 00000000-000009e4 > Found coreboot table at 0x00000000. > coreboot table found at 0x7f70e000. > coreboot header(24) checksum: 19e5 table(2532) checksum: 4a78 entries: 17 > Vendor ID: KONTRON, part ID: 986LCD-M > Found chipset "Intel ICH7/ICH7R", enabling flash write... > [...] > OK. > Disabling flash write protection for board "Kontron 986LCD-M"... > Intel ICH LPC Bridge: Raising GPIO34. > Intel ICH LPC Bridge: Raising GPIO35. > OK. > Calibrating delay loop... 491M loops per second, 100 myus = 206 us. OK. > Apparently it finds the Kontron board just fine if the board is running coreboot. From the above, I guess that Kontron detection fails if the board is running classic BIOS. In that case, a verbose run of flashrom (probe is enough) on a 986LCD-M with factory BIOS should give us the DMI strings we need to match without problems even if coreboot is not running. > * get Winbond W39V080FA working again. this was apparently broken in r868 > Thanks for the patch! Sean Nelson is currently working on a patch which handles locking in a generic way, and I heard his patch is mostly finished. Your patch is a good interim solution, but we'd have to revert it once Sean's patch goes in (which may be even this week). Not sure how to proceed. > * tested SST49LF008A and Winbond W39V080FA > Thanks. Given that we're currently rewriting probe to be more robust (reset before and after probe), and Sean's locking patch is just around the corner, everything will need to be retested. I plan to commit your patch if those rewrites are not committed until early next week. Regards, Carl-Daniel
On 2/4/10 2:55 AM, Luc Verhaegen wrote: >> Index: board_enable.c >> =================================================================== >> --- board_enable.c (revision 889) >> +++ board_enable.c (working copy) >> @@ -1221,7 +1221,7 @@ >> {0x1166, 0x0205, 0x1014, 0x0347, 0x1002, 0x515E, 0x1014, 0x0325, NULL, NULL, NULL, "IBM", "x3455", 0, board_ibm_x3455}, >> {0x1039, 0x5513, 0x8086, 0xd61f, 0x1039, 0x6330, 0x8086, 0xd61f, NULL, NULL, NULL, "Intel", "D201GLY", 0, wbsio_check_for_spi}, >> {0x1022, 0x7468, 0, 0, 0, 0, 0, 0, NULL, "iwill", "dk8_htx", "IWILL", "DK8-HTX", 0, w83627hf_gpio24_raise_2e}, >> - {0x8086, 0x27A0, 0, 0, 0x8086, 0x27b8, 0, 0, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, >> + {0x8086, 0x27A0, 0x8086, 0x27a0, 0x8086, 0x27b8, 0x8086, 0x27b8, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, >> {0x8086, 0x2411, 0x8086, 0x2411, 0x8086, 0x7125, 0x0e11, 0xb165, NULL, NULL, NULL, "Mitac", "6513WU", 0, board_mitac_6513wu}, >> {0x13f6, 0x0111, 0x1462, 0x5900, 0x1106, 0x3177, 0x1106, 0, NULL, NULL, NULL, "MSI", "MS-6590 (KT4 Ultra)", 0, board_msi_kt4v}, >> {0x1106, 0x3149, 0x1462, 0x7094, 0x10ec, 0x8167, 0x1462, 0x094c, NULL, NULL, NULL, "MSI", "MS-6702E (K8T Neo2-F)", 0, w83627thf_gpio4_4_raise_2e}, >> > Nack. Those are bogus subsystem ids. > > Sorry to say that these are the subsystem IDs that are set by the BIOS of that machine. The old entry simply does not work. So do you have a suggestion to make, beyond Nack?
On 2/4/10 3:07 AM, Carl-Daniel Hailfinger wrote: > On 04.02.2010 01:51, Stefan Reinauer wrote: > >> * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 >> >> > Just a short question: The Kontron 986LCD-M appeared to be running > coreboot. Is that correct? Quoting from the failure log you sent earlier: > No. The run that requires the IDs are with vendor BIOS. coreboot can detect the board through coreboot table. The subsystem IDs are only required when running flashrom on a not yet migrated board. I tried not changing the subsystem IDs and simply adding a DMI match string, but that won't work because the DMI matching code is never executed because the subsystem vendor ID matching code already decided it's not a kontron board > Apparently it finds the Kontron board just fine if the board is running > coreboot. > > From the above, I guess that Kontron detection fails if the board is > running classic BIOS. In that case, a verbose run of flashrom (probe is > enough) on a 986LCD-M with factory BIOS should give us the DMI strings > we need to match without problems even if coreboot is not running. > See above. >> * tested SST49LF008A and Winbond W39V080FA >> >> > Thanks. Given that we're currently rewriting probe to be more robust > (reset before and after probe) Playing the devils advocate ;-) So then probing will fail if reset fails, and if probe fails. Now probe fails only if probe fails. So the code gets more robust by adding more potential failure points? ;-) > and Sean's locking patch is just around > the corner, everything will need to be retested. I plan to commit your > patch if those rewrites are not committed until early next week. > Don't worry committing the stuff. The patches are out there for those who need them, that'll be fine until flashrom has a decent locking framework. Stefan
On Thu, Feb 04, 2010 at 03:18:08AM +0100, Stefan Reinauer wrote: > On 2/4/10 2:55 AM, Luc Verhaegen wrote: > >> Index: board_enable.c > >> =================================================================== > >> --- board_enable.c (revision 889) > >> +++ board_enable.c (working copy) > >> @@ -1221,7 +1221,7 @@ > >> {0x1166, 0x0205, 0x1014, 0x0347, 0x1002, 0x515E, 0x1014, 0x0325, NULL, NULL, NULL, "IBM", "x3455", 0, board_ibm_x3455}, > >> {0x1039, 0x5513, 0x8086, 0xd61f, 0x1039, 0x6330, 0x8086, 0xd61f, NULL, NULL, NULL, "Intel", "D201GLY", 0, wbsio_check_for_spi}, > >> {0x1022, 0x7468, 0, 0, 0, 0, 0, 0, NULL, "iwill", "dk8_htx", "IWILL", "DK8-HTX", 0, w83627hf_gpio24_raise_2e}, > >> - {0x8086, 0x27A0, 0, 0, 0x8086, 0x27b8, 0, 0, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, > >> + {0x8086, 0x27A0, 0x8086, 0x27a0, 0x8086, 0x27b8, 0x8086, 0x27b8, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, > >> {0x8086, 0x2411, 0x8086, 0x2411, 0x8086, 0x7125, 0x0e11, 0xb165, NULL, NULL, NULL, "Mitac", "6513WU", 0, board_mitac_6513wu}, > >> {0x13f6, 0x0111, 0x1462, 0x5900, 0x1106, 0x3177, 0x1106, 0, NULL, NULL, NULL, "MSI", "MS-6590 (KT4 Ultra)", 0, board_msi_kt4v}, > >> {0x1106, 0x3149, 0x1462, 0x7094, 0x10ec, 0x8167, 0x1462, 0x094c, NULL, NULL, NULL, "MSI", "MS-6702E (K8T Neo2-F)", 0, w83627thf_gpio4_4_raise_2e}, > >> > > Nack. Those are bogus subsystem ids. > > > > > > Sorry to say that these are the subsystem IDs that are set by the BIOS > of that machine. The old entry simply does not work. So do you have a > suggestion to make, beyond Nack? If the PCI subsystem ids are bogus, autodetection per pci-ids will mean that this board enable gets run on other, equally bogus, boards as well. We have DMI, please provide the output of flashrom -V so we can try to salvage things through DMI. If not, this board has to be -m, or coreboot ids only. Luc Verhaegen.
On Thu, Feb 04, 2010 at 03:30:13AM +0100, Stefan Reinauer wrote: > On 2/4/10 3:07 AM, Carl-Daniel Hailfinger wrote: > > On 04.02.2010 01:51, Stefan Reinauer wrote: > > > >> * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 > >> > >> > > Just a short question: The Kontron 986LCD-M appeared to be running > > coreboot. Is that correct? Quoting from the failure log you sent earlier: > > > No. The run that requires the IDs are with vendor BIOS. coreboot can > detect the board through coreboot table. > > The subsystem IDs are only required when running flashrom on a not yet > migrated board. > > I tried not changing the subsystem IDs and simply adding a DMI match > string, but that won't work because the DMI matching code is never > executed because the subsystem vendor ID matching code already decided > it's not a kontron board If DMI can be used successfully (could be that kontron also buggered that up), then the combination of subsystem ids and dmi will make a unique match. As things are, it will not be unique. Add DMI, and it will be unique. How did you handle this board in the past? It always used to be a -m board as far as i can tell: /* Note: There are >= 2 version of the Kontron 986LCD-M/mITX! */ - {0x8086, 0x27b8, 0, 0, 0, 0, 0, 0, "kontron", "986lcd-m", "Kontron", "986LCD-M", board_kontron_986lcd_m}, - {0x10ec, 0x8168, 0x10ec, 0x8168, 0x104c, 0x8023, 0x104c, 0x8019, "kontron", "986lcd-m", "Kontron", "986LCD-M", board_kontron_986lcd_m}, + {0x8086, 0x27A0, 0, 0, 0x8086, 0x27b8, 0, 0, "kontron", "986lcd-m", "Kontron", "986LCD-M", board_kontron_986lcd_m}, Luc Verhaegen.
Short summary: The DMI non-match behaviour you're seeing is either by design or by accident. Not sure yet. There are patches floating around which would allow a DMI match without subsystem IDs. Will ping you once this is resolved. On 04.02.2010 03:30, Stefan Reinauer wrote: > On 2/4/10 3:07 AM, Carl-Daniel Hailfinger wrote: > >> On 04.02.2010 01:51, Stefan Reinauer wrote: >> >> >>> * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 >>> >>> >>> >> Just a short question: The Kontron 986LCD-M appeared to be running >> coreboot. Is that correct? Quoting from the failure log you sent earlier: >> >> > No. The run that requires the IDs are with vendor BIOS. coreboot can > detect the board through coreboot table. > > The subsystem IDs are only required when running flashrom on a not yet > migrated board. > > I tried not changing the subsystem IDs and simply adding a DMI match > string, but that won't work because the DMI matching code is never > executed because the subsystem vendor ID matching code already decided > it's not a kontron board > Ah, right. I'm not 100% familiar with the code. Looking at it again, the rules are: - Provide subsystem IDs even if they are bogus (e.g. copy of chipset vendor/device ID) - Since the above will match too many boards, DMI will be consulted as well. - DMI without subsystem IDs is too dangerous (multiple matches), so subsystem IDs are required even if they are bogus. Given that there seems to be some confusion about whether requiring subsystem IDs even in the presence of DMI is a good idea, the rules may change. Side note: the coreboot ID matching code needs some revamping to be more readable (and with DMI, some abuses can be eliminated). >> Apparently it finds the Kontron board just fine if the board is running >> coreboot. >> >> From the above, I guess that Kontron detection fails if the board is >> running classic BIOS. In that case, a verbose run of flashrom (probe is >> enough) on a 986LCD-M with factory BIOS should give us the DMI strings >> we need to match without problems even if coreboot is not running. >> >> > See above. > Unless I'm mistaken, the flashrom log you sent was from a board with coreboot, but I need a flashrom log from a board with factory BIOS. Short summary: - With the current DMI matching code, we need the sort-of-bogus subsystem IDs rejected by Luc. - The subsystem requirement may change tomorrow. - Additionally, that entry needs a DMI string as well. Caveat: If subsystem IDs differ between vendor BIOS and coreboot, we need two entries with the current policy. >>> * tested SST49LF008A and Winbond W39V080FA >>> >>> >> Thanks. Given that we're currently rewriting probe to be more robust >> (reset before and after probe) >> > > Playing the devils advocate ;-) So then probing will fail if reset > fails, and if probe fails. > Now probe fails only if probe fails. So the code gets more robust by > adding more potential failure points? ;-) > Yes and no. The problem is that sometimes a chip may be in an undefined state because of previous probe sequences it didn't understand (happened on some Alix board with an Amic chip). Resetting it before probing could fix that case. Fortunately, the variability of reset sequences is much lower than the variability of probe sequences (I think only 3 reset sequences exist in total). Besides that, probe_xxx() right now is probe/reset, and afterwards it will be reset/probe/reset. Regards, Carl-Daniel
On 04.02.2010 04:42, Carl-Daniel Hailfinger wrote: > Short summary: > The DMI non-match behaviour you're seeing is either by design or by > accident. Not sure yet. There are patches floating around which would > allow a DMI match without subsystem IDs. > Will ping you once this is resolved. > Resolved for now. > On 04.02.2010 03:30, Stefan Reinauer wrote: > >> On 2/4/10 3:07 AM, Carl-Daniel Hailfinger wrote: >> >>> On 04.02.2010 01:51, Stefan Reinauer wrote: >>> >>> >>>> * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 >>>> >>> Just a short question: The Kontron 986LCD-M appeared to be running >>> coreboot. Is that correct? Quoting from the failure log you sent earlier: >>> >> No. The run that requires the IDs are with vendor BIOS. coreboot can >> detect the board through coreboot table. >> >> The subsystem IDs are only required when running flashrom on a not yet >> migrated board. >> >> I tried not changing the subsystem IDs and simply adding a DMI match >> string, but that won't work because the DMI matching code is never >> executed because the subsystem vendor ID matching code already decided >> it's not a kontron board >> > > Ah, right. I'm not 100% familiar with the code. Looking at it again, the > rules are: > - Provide subsystem IDs even if they are bogus (e.g. copy of chipset > vendor/device ID) > - Since the above will match too many boards, DMI will be consulted as well. > - DMI without subsystem IDs is too dangerous (multiple matches), so > subsystem IDs are required even if they are bogus. > Subsystem IDs for the first device are required for DMI to match, subsystem IDs for the second device are optional AFAICS. > Unless I'm mistaken, the flashrom log you sent was from a board with > coreboot, but I need a flashrom log from a board with factory BIOS. > > Short summary: > - With the current DMI matching code, we need the sort-of-bogus > subsystem IDs rejected by Luc. > - The subsystem requirement may change tomorrow. > - Additionally, that entry needs a DMI string as well. > > Caveat: If subsystem IDs differ between vendor BIOS and coreboot, we > need two entries with the current policy. > I must have missed the lspci of the Kontron 986LCD-M with factory BIOS. Anyway, if you resend your mainboard patch with one small change (added DMI match) and if it works, I'll ack. Regards, Carl-Daniel
Hi Stefan, W39V080FA writing should be fixed in r907 (and any later version). The DMI code has been fixed, and it now should work reliably. I'd like to know which changes are still needed from your side, and then make sure they get merged in a way that keeps existing BIOS and coreboot versions of the board working. Regards, Carl-Daniel
Patch
Index: jedec.c =================================================================== --- jedec.c (revision 889) +++ jedec.c (working copy) @@ -29,6 +29,9 @@ #define MASK_2AA 0x7ff #define MASK_AAA 0xfff +int unlock_winbond_fwhub(struct flashchip *flash); +static int unlocked = 0; + /* Check one byte for odd parity */ uint8_t oddparity(uint8_t val) { @@ -480,6 +483,19 @@ return erase_sector_jedec_common(flash, page, size, mask); } +int unlock_and_erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size) +{ + int mask; + + if (!unlocked) { + unlock_winbond_fwhub(flash); + unlocked=1; + } + + mask = getaddrmask(flash); + return erase_sector_jedec_common(flash, page, size, mask); +} + int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int size) { int mask; Index: flashchips.c =================================================================== --- flashchips.c (revision 889) +++ flashchips.c (working copy) @@ -4286,7 +4286,7 @@ .total_size = 1024, .page_size = 64 * 1024, .feature_bits = FEATURE_REGISTERMAP | FEATURE_EITHER_RESET, - .tested = TEST_OK_PRW, + .tested = TEST_OK_PREW, .probe = probe_sst_fwhub, .probe_timing = 1, /* 150 ns | routine is wrapper to probe_jedec (sst_fwhub.c) */ .erase = NULL, @@ -6116,7 +6116,7 @@ .total_size = 1024, .page_size = 64 * 1024, .feature_bits = FEATURE_REGISTERMAP | FEATURE_EITHER_RESET, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = probe_jedec, .probe_timing = TIMING_FIXME, .erase = NULL, /* Was erase_winbond_fwhub */ @@ -6124,7 +6124,7 @@ { { .eraseblocks = { {64 * 1024, 16}, }, - .block_erase = erase_sector_jedec, + .block_erase = unlock_and_erase_sector_jedec, }, { .eraseblocks = { {1024 * 1024, 1} }, .block_erase = erase_chip_block_jedec, Index: chipdrivers.h =================================================================== --- chipdrivers.h (revision 889) +++ chipdrivers.h (working copy) @@ -86,6 +86,7 @@ int write_jedec(struct flashchip *flash, uint8_t *buf); int write_jedec_1(struct flashchip *flash, uint8_t *buf); int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize); +int unlock_and_erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size); int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, chipaddr dst, unsigned int page_size, unsigned int mask); Index: board_enable.c =================================================================== --- board_enable.c (revision 889) +++ board_enable.c (working copy) @@ -1221,7 +1221,7 @@ {0x1166, 0x0205, 0x1014, 0x0347, 0x1002, 0x515E, 0x1014, 0x0325, NULL, NULL, NULL, "IBM", "x3455", 0, board_ibm_x3455}, {0x1039, 0x5513, 0x8086, 0xd61f, 0x1039, 0x6330, 0x8086, 0xd61f, NULL, NULL, NULL, "Intel", "D201GLY", 0, wbsio_check_for_spi}, {0x1022, 0x7468, 0, 0, 0, 0, 0, 0, NULL, "iwill", "dk8_htx", "IWILL", "DK8-HTX", 0, w83627hf_gpio24_raise_2e}, - {0x8086, 0x27A0, 0, 0, 0x8086, 0x27b8, 0, 0, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, + {0x8086, 0x27A0, 0x8086, 0x27a0, 0x8086, 0x27b8, 0x8086, 0x27b8, NULL, "kontron", "986lcd-m", "Kontron", "986LCD-M", 0, board_kontron_986lcd_m}, {0x8086, 0x2411, 0x8086, 0x2411, 0x8086, 0x7125, 0x0e11, 0xb165, NULL, NULL, NULL, "Mitac", "6513WU", 0, board_mitac_6513wu}, {0x13f6, 0x0111, 0x1462, 0x5900, 0x1106, 0x3177, 0x1106, 0, NULL, NULL, NULL, "MSI", "MS-6590 (KT4 Ultra)", 0, board_msi_kt4v}, {0x1106, 0x3149, 0x1462, 0x7094, 0x10ec, 0x8167, 0x1462, 0x094c, NULL, NULL, NULL, "MSI", "MS-6702E (K8T Neo2-F)", 0, w83627thf_gpio4_4_raise_2e},
See patch. This is what I had to do to get the combo working again. * get Kontron 986LCD-M recognized by ID detection again. This was apparently broken in r796 * get Winbond W39V080FA working again. this was apparently broken in r868 * tested SST49LF008A and Winbond W39V080FA Signed-off-by: Stefan Reinauer <stepan@coresystems.de>