| Submitter | Wagner, Helge (GE Intelligent Platforms) |
|---|---|
| Date | 2010-08-05 14:43:26 |
| Message ID | <A3EBBCEF5B62BB44AA29816B1814095A05372289@BUDMLVEM06.e2k.ad.ge.com> |
| Download | mbox | patch |
| Permalink | /patch/1724/ |
| State | Accepted |
| Commit | 1137 |
| Headers | show |
Comments
2010/8/5 Wagner, Helge (GE Intelligent Platforms) <Helge.Wagner@ge.com> > I have added support for some new intel chipsets. > If only we could have saved you from doing this twice: http://www.flashrom.org/pipermail/flashrom/2010-April/thread.html#2896 http://patchwork.coreboot.org/patch/1208/ > > (At least) for the QM57 which i have tested an additional patch was > needed as some reserved bits in the "Software Sequencing Flash Control > Register" (SSFC) needs to be programmed to 1 in the QM57. > > Signed-off-by: Helge Wagner <helge.wagner@ge.com> > > diff -urN flashrom-0.9.2/chipset_enable.c flashrom/chipset_enable.c > --- flashrom-0.9.2/chipset_enable.c 2010-08-01 01:16:09.000000000 > +0200 > +++ flashrom/chipset_enable.c 2010-08-05 13:28:29.000000000 +0200 > @@ -1011,7 +1011,21 @@ > {0x1166, 0x0205, OK, "Broadcom", "HT-1000", > enable_flash_ht1000}, > {0x8086, 0x3b00, NT, "Intel", "3400 Desktop", > enable_flash_ich10}, > {0x8086, 0x3b01, NT, "Intel", "3400 Mobile", > enable_flash_ich10}, > + {0x8086, 0x3b02, NT, "Intel", "P55", > enable_flash_ich10}, > + {0x8086, 0x3b03, NT, "Intel", "PM55", > enable_flash_ich10}, > + {0x8086, 0x3b06, NT, "Intel", "H55", > enable_flash_ich10}, > + {0x8086, 0x3b07, OK, "Intel", "QM57", > enable_flash_ich10}, > + {0x8086, 0x3b08, NT, "Intel", "H57", > enable_flash_ich10}, > + {0x8086, 0x3b09, NT, "Intel", "HM55", > enable_flash_ich10}, > + {0x8086, 0x3b0a, NT, "Intel", "Q57", > enable_flash_ich10}, > + {0x8086, 0x3b0b, NT, "Intel", "HM57", > enable_flash_ich10}, > {0x8086, 0x3b0d, NT, "Intel", "3400 Mobile SFF", > enable_flash_ich10}, > + {0x8086, 0x3b0e, NT, "Intel", "B55", > enable_flash_ich10}, > See below > + {0x8086, 0x3b0f, NT, "Intel", "QS57", > enable_flash_ich10}, > + {0x8086, 0x3b12, NT, "Intel", "3400", > enable_flash_ich10}, > + {0x8086, 0x3b14, NT, "Intel", "3420", > enable_flash_ich10}, > + {0x8086, 0x3b16, NT, "Intel", "3450", > enable_flash_ich10}, > + {0x8086, 0x3b1e, NT, "Intel", "B55", > enable_flash_ich10}, > Thanks for finding/adding "B55": the friendly name for this chipset is empty in forementioned patch (see patchwork). Do you perchance have a verbose log of flashrom (flashrom -V) from that chipset ? {0x8086, 0x7198, OK, "Intel", "440MX", > enable_flash_piix4}, > {0x8086, 0x25a1, OK, "Intel", "6300ESB", > enable_flash_ich_4e}, > {0x8086, 0x2670, OK, "Intel", "631xESB/632xESB/3100", > enable_flash_ich_dc}, > diff -urN flashrom-0.9.2/ichspi.c flashrom/ichspi.c > --- flashrom-0.9.2/ichspi.c 2010-07-28 00:41:39.000000000 +0200 > +++ flashrom/ichspi.c 2010-08-05 13:30:32.000000000 +0200 > @@ -560,7 +560,9 @@ > } > > /* Assemble SSFS + SSFC */ > - temp32 = 0; > + /* keep reserved bits (23-19,7,0) */ > + temp32 = REGREAD32(ICH9_REG_SSFS); > + temp32 &= 0xF8008100; > > /* clear error status registers */ > temp32 |= (SSFS_CDS + SSFS_FCERR); > > > > _______________________________________________ > flashrom mailing list > flashrom@flashrom.org > http://www.flashrom.org/mailman/listinfo/flashrom >
>If only we could have saved you from doing this twice But why are these chipsets still not integrated? >Do you perchance have a verbose log of flashrom (flashrom -V) from that chipset ? No, sorry! The only chipset i have here is the QM57. That's why i have marked that (and only that) as tested.
Hi Helge, On 05.08.2010 16:43, Wagner, Helge (GE Intelligent Platforms) wrote: > I have added support for some new intel chipsets. > Thanks for your patch! > (At least) for the QM57 which i have tested an additional patch was > needed as some reserved bits in the "Software Sequencing Flash Control > Register" (SSFC) needs to be programmed to 1 in the QM57. > Ouch! Do you see a hang without that part of the patch? We've been chasing a hang on some ICH10 family boards, and your patch may actually fix it. I noticed that you keep the reserved bits as is, but should we set them to 1 instead even if they are not set? > Signed-off-by: Helge Wagner <helge.wagner@ge.com> > Looks good, but it seems your mailer mangled the patch (inserted linebreaks). Could you please resend it as attachment? > diff -urN flashrom-0.9.2/chipset_enable.c flashrom/chipset_enable.c > --- flashrom-0.9.2/chipset_enable.c 2010-08-01 01:16:09.000000000 > +0200 > +++ flashrom/chipset_enable.c 2010-08-05 13:28:29.000000000 +0200 > @@ -1011,7 +1011,21 @@ > {0x1166, 0x0205, OK, "Broadcom", "HT-1000", > enable_flash_ht1000}, > {0x8086, 0x3b00, NT, "Intel", "3400 Desktop", > enable_flash_ich10}, > {0x8086, 0x3b01, NT, "Intel", "3400 Mobile", > enable_flash_ich10}, > + {0x8086, 0x3b02, NT, "Intel", "P55", > enable_flash_ich10}, > + {0x8086, 0x3b03, NT, "Intel", "PM55", > enable_flash_ich10}, > + {0x8086, 0x3b06, NT, "Intel", "H55", > enable_flash_ich10}, > + {0x8086, 0x3b07, OK, "Intel", "QM57", > enable_flash_ich10}, > + {0x8086, 0x3b08, NT, "Intel", "H57", > enable_flash_ich10}, > + {0x8086, 0x3b09, NT, "Intel", "HM55", > enable_flash_ich10}, > + {0x8086, 0x3b0a, NT, "Intel", "Q57", > enable_flash_ich10}, > + {0x8086, 0x3b0b, NT, "Intel", "HM57", > enable_flash_ich10}, > {0x8086, 0x3b0d, NT, "Intel", "3400 Mobile SFF", > enable_flash_ich10}, > + {0x8086, 0x3b0e, NT, "Intel", "B55", > enable_flash_ich10}, > + {0x8086, 0x3b0f, NT, "Intel", "QS57", > enable_flash_ich10}, > + {0x8086, 0x3b12, NT, "Intel", "3400", > enable_flash_ich10}, > + {0x8086, 0x3b14, NT, "Intel", "3420", > enable_flash_ich10}, > + {0x8086, 0x3b16, NT, "Intel", "3450", > enable_flash_ich10}, > + {0x8086, 0x3b1e, NT, "Intel", "B55", > enable_flash_ich10}, > {0x8086, 0x7198, OK, "Intel", "440MX", > enable_flash_piix4}, > {0x8086, 0x25a1, OK, "Intel", "6300ESB", > enable_flash_ich_4e}, > {0x8086, 0x2670, OK, "Intel", "631xESB/632xESB/3100", > enable_flash_ich_dc}, > diff -urN flashrom-0.9.2/ichspi.c flashrom/ichspi.c > --- flashrom-0.9.2/ichspi.c 2010-07-28 00:41:39.000000000 +0200 > +++ flashrom/ichspi.c 2010-08-05 13:30:32.000000000 +0200 > @@ -560,7 +560,9 @@ > } > > /* Assemble SSFS + SSFC */ > - temp32 = 0; > + /* keep reserved bits (23-19,7,0) */ > + temp32 = REGREAD32(ICH9_REG_SSFS); > + temp32 &= 0xF8008100; > > /* clear error status registers */ > temp32 |= (SSFS_CDS + SSFS_FCERR); > Regards, Carl-Daniel
Hi Carl-Daniel, >Ouch! Do you see a hang without that part of the patch? Without the patch our boards are going into S5 some seconds after i started flashrom. >I noticed that you keep the reserved bits as is, but should we set them to 1 instead even if they are not set? The default value of these bits is still correct, so not toughing them would be the best. Else you would have to differentiate the chipsets as some chipsets needs different values (e.g. Bits 23-19 should be 0 on ICH9, while they should be 1 on QM57). >Looks good, but it seems your mailer mangled the patch (inserted linebreaks). Could you please resend it as attachment? Please find the patch attached. Regards, Helge -----Original Message----- From: Carl-Daniel Hailfinger [mailto:c-d.hailfinger.devel.2006@gmx.net] Sent: Donnerstag, 5. August 2010 23:46 To: Wagner, Helge (GE Intelligent Platforms) Cc: flashrom@flashrom.org Subject: Re: [flashrom] New Intel Chipsets Hi Helge, On 05.08.2010 16:43, Wagner, Helge (GE Intelligent Platforms) wrote: > I have added support for some new intel chipsets. > Thanks for your patch! > (At least) for the QM57 which i have tested an additional patch was > needed as some reserved bits in the "Software Sequencing Flash Control > Register" (SSFC) needs to be programmed to 1 in the QM57. > Ouch! Do you see a hang without that part of the patch? We've been chasing a hang on some ICH10 family boards, and your patch may actually fix it. I noticed that you keep the reserved bits as is, but should we set them to 1 instead even if they are not set? > Signed-off-by: Helge Wagner <helge.wagner@ge.com> > Looks good, but it seems your mailer mangled the patch (inserted linebreaks). Could you please resend it as attachment? > diff -urN flashrom-0.9.2/chipset_enable.c flashrom/chipset_enable.c > --- flashrom-0.9.2/chipset_enable.c 2010-08-01 01:16:09.000000000 > +0200 > +++ flashrom/chipset_enable.c 2010-08-05 13:28:29.000000000 +0200 > @@ -1011,7 +1011,21 @@ > {0x1166, 0x0205, OK, "Broadcom", "HT-1000", enable_flash_ht1000}, > {0x8086, 0x3b00, NT, "Intel", "3400 Desktop", enable_flash_ich10}, > {0x8086, 0x3b01, NT, "Intel", "3400 Mobile", enable_flash_ich10}, > + {0x8086, 0x3b02, NT, "Intel", "P55", > enable_flash_ich10}, > + {0x8086, 0x3b03, NT, "Intel", "PM55", > enable_flash_ich10}, > + {0x8086, 0x3b06, NT, "Intel", "H55", > enable_flash_ich10}, > + {0x8086, 0x3b07, OK, "Intel", "QM57", > enable_flash_ich10}, > + {0x8086, 0x3b08, NT, "Intel", "H57", > enable_flash_ich10}, > + {0x8086, 0x3b09, NT, "Intel", "HM55", > enable_flash_ich10}, > + {0x8086, 0x3b0a, NT, "Intel", "Q57", > enable_flash_ich10}, > + {0x8086, 0x3b0b, NT, "Intel", "HM57", > enable_flash_ich10}, > {0x8086, 0x3b0d, NT, "Intel", "3400 Mobile SFF", > enable_flash_ich10}, > + {0x8086, 0x3b0e, NT, "Intel", "B55", > enable_flash_ich10}, > + {0x8086, 0x3b0f, NT, "Intel", "QS57", > enable_flash_ich10}, > + {0x8086, 0x3b12, NT, "Intel", "3400", > enable_flash_ich10}, > + {0x8086, 0x3b14, NT, "Intel", "3420", > enable_flash_ich10}, > + {0x8086, 0x3b16, NT, "Intel", "3450", > enable_flash_ich10}, > + {0x8086, 0x3b1e, NT, "Intel", "B55", > enable_flash_ich10}, > {0x8086, 0x7198, OK, "Intel", "440MX", enable_flash_piix4}, > {0x8086, 0x25a1, OK, "Intel", "6300ESB", enable_flash_ich_4e}, > {0x8086, 0x2670, OK, "Intel", "631xESB/632xESB/3100", > enable_flash_ich_dc}, diff -urN flashrom-0.9.2/ichspi.c > flashrom/ichspi.c > --- flashrom-0.9.2/ichspi.c 2010-07-28 00:41:39.000000000 +0200 > +++ flashrom/ichspi.c 2010-08-05 13:30:32.000000000 +0200 > @@ -560,7 +560,9 @@ > } > > /* Assemble SSFS + SSFC */ > - temp32 = 0; > + /* keep reserved bits (23-19,7,0) */ > + temp32 = REGREAD32(ICH9_REG_SSFS); > + temp32 &= 0xF8008100; > > /* clear error status registers */ > temp32 |= (SSFS_CDS + SSFS_FCERR); > Regards, Carl-Daniel
Am Freitag, den 06.08.2010, 08:52 +0200 schrieb Wagner, Helge (GE Intelligent Platforms): > >Ouch! Do you see a hang without that part of the patch? > Without the patch our boards are going into S5 some seconds after i > started flashrom. OK, this is probably the same issue that causes other boards to freeze. This patch has one minor issue: flash_enable_ich_dc_spi decodes the BIOS type select straps. They are valid for ICH7. ICH8, 9, 10 nearly have the same assignment, but accept 0 as alias for 1 (SPI) instead of declaring it as "reserved". But 5 Series/3400 OTOH has a completely different different assignment with IIRC SPI on 3, which we do not respect. I don't really want to push you into fixing that, but just raise awareness, nevertheless I would really appreciate a patch fixing that, too. I am really happy that there is a patch that makes the 5 Series chipsets usable in flashrom. So even with this quirk, the patch is in my oppinion a worthwile addition to flashrom and thus Acked-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> The Intel PCI IDs can be verified for example using the Intel INF Update 9.1.2.1007. Regards, Michael Karcher
Hi Michael, >This patch has one minor issue: flash_enable_ich_dc_spi decodes the BIOS type select straps. The function is called "enable_flash_ich_dc_spi" (for those that try to find it in the source). In this function the BIOS type select strap is not honored at all except for ICH7: ... if (ich_generation == 7) { if(bbs == ICH_STRAP_LPC) { /* No further SPI initialization required */ return ret; } else /* Disable LPC/FWH if strapped to PCI or SPI */ buses_supported = 0; } ... So it should all be ok already for new chipsets. Do you agree? Regards, Helge -----Original Message----- From: Michael Karcher [mailto:flashrom@mkarcher.dialup.fu-berlin.de] Sent: Montag, 9. August 2010 00:56 To: Wagner, Helge (GE Intelligent Platforms) Cc: Carl-Daniel Hailfinger; flashrom@flashrom.org Subject: Re: [flashrom] New Intel Chipsets Am Freitag, den 06.08.2010, 08:52 +0200 schrieb Wagner, Helge (GE Intelligent Platforms): > >Ouch! Do you see a hang without that part of the patch? > Without the patch our boards are going into S5 some seconds after i > started flashrom. OK, this is probably the same issue that causes other boards to freeze. This patch has one minor issue: flash_enable_ich_dc_spi decodes the BIOS type select straps. They are valid for ICH7. ICH8, 9, 10 nearly have the same assignment, but accept 0 as alias for 1 (SPI) instead of declaring it as "reserved". But 5 Series/3400 OTOH has a completely different different assignment with IIRC SPI on 3, which we do not respect. I don't really want to push you into fixing that, but just raise awareness, nevertheless I would really appreciate a patch fixing that, too. I am really happy that there is a patch that makes the 5 Series chipsets usable in flashrom. So even with this quirk, the patch is in my oppinion a worthwile addition to flashrom and thus Acked-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> The Intel PCI IDs can be verified for example using the Intel INF Update 9.1.2.1007. Regards, Michael Karcher
Am Montag, den 09.08.2010, 09:55 +0200 schrieb Wagner, Helge (GE Intelligent Platforms): > Hi Michael, > > >This patch has one minor issue: flash_enable_ich_dc_spi decodes the > BIOS type select straps. > > The function is called "enable_flash_ich_dc_spi" (for those that try to > find it in the source). > > In this function the BIOS type select strap is not honored at all except > for ICH7: > ... > if (ich_generation == 7) { > if(bbs == ICH_STRAP_LPC) { > /* No further SPI initialization required */ > return ret; > } > else > /* Disable LPC/FWH if strapped to PCI or SPI */ > buses_supported = 0; > } > ... > So it should all be ok already for new chipsets. Do you agree? The state it is now (that the bbs is ignored for newer chipsets) is becasue we were already aware of the problem. Still the debugging print in the beginning is wrong, especially for 5 Series/3400 series. I'm going to commit your patch as it does not break anything, and adds working support for the new chipsets. We are checking bbs for ICH7 because that chipset supports *either* LPC/FWH *or* the SPI interface and never both interfaces run at the same time. This is different for later chipsets where the SPI chip can be addressed through the software sequencing registers even if the system boots from LPC flash. Regards, Michael Karcher
Am Montag, den 09.08.2010, 00:56 +0200 schrieb Michael Karcher: > Am Freitag, den 06.08.2010, 08:52 +0200 schrieb Wagner, Helge (GE > Intelligent Platforms): > > >Ouch! Do you see a hang without that part of the patch? > > Without the patch our boards are going into S5 some seconds after i > > started flashrom. > OK, this is probably the same issue that causes other boards to freeze. > Acked-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> committed in r1137, sorry for the delay. Regards, Michael Karcher
Patch
diff -urN flashrom-0.9.2/ichspi.c flashrom/ichspi.c --- flashrom-0.9.2/ichspi.c 2010-07-28 00:41:39.000000000 +0200 +++ flashrom/ichspi.c 2010-08-05 13:30:32.000000000 +0200 @@ -560,7 +560,9 @@ } /* Assemble SSFS + SSFC */ - temp32 = 0; + /* keep reserved bits (23-19,7,0) */ + temp32 = REGREAD32(ICH9_REG_SSFS); + temp32 &= 0xF8008100; /* clear error status registers */ temp32 |= (SSFS_CDS + SSFS_FCERR);
I have added support for some new intel chipsets. (At least) for the QM57 which i have tested an additional patch was needed as some reserved bits in the "Software Sequencing Flash Control Register" (SSFC) needs to be programmed to 1 in the QM57. Signed-off-by: Helge Wagner <helge.wagner@ge.com> diff -urN flashrom-0.9.2/chipset_enable.c flashrom/chipset_enable.c --- flashrom-0.9.2/chipset_enable.c 2010-08-01 01:16:09.000000000 +0200 +++ flashrom/chipset_enable.c 2010-08-05 13:28:29.000000000 +0200 @@ -1011,7 +1011,21 @@ {0x1166, 0x0205, OK, "Broadcom", "HT-1000", enable_flash_ht1000}, {0x8086, 0x3b00, NT, "Intel", "3400 Desktop", enable_flash_ich10}, {0x8086, 0x3b01, NT, "Intel", "3400 Mobile", enable_flash_ich10}, + {0x8086, 0x3b02, NT, "Intel", "P55", enable_flash_ich10}, + {0x8086, 0x3b03, NT, "Intel", "PM55", enable_flash_ich10}, + {0x8086, 0x3b06, NT, "Intel", "H55", enable_flash_ich10}, + {0x8086, 0x3b07, OK, "Intel", "QM57", enable_flash_ich10}, + {0x8086, 0x3b08, NT, "Intel", "H57", enable_flash_ich10}, + {0x8086, 0x3b09, NT, "Intel", "HM55", enable_flash_ich10}, + {0x8086, 0x3b0a, NT, "Intel", "Q57", enable_flash_ich10}, + {0x8086, 0x3b0b, NT, "Intel", "HM57", enable_flash_ich10}, {0x8086, 0x3b0d, NT, "Intel", "3400 Mobile SFF", enable_flash_ich10}, + {0x8086, 0x3b0e, NT, "Intel", "B55", enable_flash_ich10}, + {0x8086, 0x3b0f, NT, "Intel", "QS57", enable_flash_ich10}, + {0x8086, 0x3b12, NT, "Intel", "3400", enable_flash_ich10}, + {0x8086, 0x3b14, NT, "Intel", "3420", enable_flash_ich10}, + {0x8086, 0x3b16, NT, "Intel", "3450", enable_flash_ich10}, + {0x8086, 0x3b1e, NT, "Intel", "B55", enable_flash_ich10}, {0x8086, 0x7198, OK, "Intel", "440MX", enable_flash_piix4}, {0x8086, 0x25a1, OK, "Intel", "6300ESB", enable_flash_ich_4e}, {0x8086, 0x2670, OK, "Intel", "631xESB/632xESB/3100", enable_flash_ich_dc},