On Tue, 10 Sep 2013 09:31:13 +0200 (CEST) "Carl-Daniel U. Hailfinger" c-d.hailfinger.devel.2006@gmx.net wrote:
[Sorry for breaking the threading. This is meant to be a reply to http://patchwork.coreboot.org/patch/4016/ ] Stefan Tauner wrote on 2013-08-14 17:35:13
On Wed, 14 Aug 2013 08:42:15 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 10.08.2013 03:45 schrieb Stefan Tauner:
Also, correct prettyprinting of the registers of the various families, and abort if SpiAccessMacRomEn or SpiHostAccessRomEn prohibit full access.
Tested reading/writing on ASRock IMB-A180, and chipset detection on one of each affected generation by Chris Goodrich from Sage.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
sb600spi.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 106 insertions(+), 27 deletions(-)
diff --git a/sb600spi.c b/sb600spi.c index cb7c4ac..c9be44c 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -57,7 +57,28 @@ static enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN; static void determine_generation(struct pci_dev *dev) { amd_gen = CHIPSET_AMD_UNKNOWN;
- if (dev->device_id == 0x780e) {
- msg_pdbg2("Trying to determine the generation of the SPI interface... ");
- if (dev->device_id == 0x438d) {
amd_gen = CHIPSET_SB6XX;msg_pdbg("SB6xx detected.\n");- } else if (dev->device_id == 0x439d) {
struct pci_dev *smbus_dev = pci_dev_find(0x1002, 0x4385);Why do we search twice for this SMBus device, once inside sb600_probe_spi and once here? Can't we just pass the smbus device as second parameter to determine_generation?
Yes we could and I thought about this too but came to the conclusion that it is not worth it. Do you think otherwise?
I'd really like to limit the number of times we search for PCI devices. On some machines with lots of PCI devices that can take a lot of time.
I have no idea how you come to that conclusion. This uses 0 bus accesses AFAICS because libpci caches that information, but even if we would have to rescan all devices on the bus it would be rather negligible: it does only apply to very few users and I am not sure if any of them would notice it. It might not be pretty theoretically, but I don't anticipate any problems in reality at all. What do you think is the worst case latency added? A few ms?
That said, I also think such refactoring can be done in a later iteration, and it is also possible to have the generation detection as first command inside the AMD SBx00 SPI probe function, and let it set a static struct *smbus_dev. Your choice when/how to deal with it.
Anyway, most/all of the code above was committed in r1706, so this is mostly an after-the-fact review.
ATM we do only look for the smbus device twice on hudson 2+ and yangtze. With the rebased version of this patch it will be done for all amd chipsets since sb7xx:
if (smbus_dev == NULL)return;uint8_t rev = pci_read_byte(smbus_dev, PCI_REVISION_ID);if (rev >= 0x39 && rev <= 0x3D) {amd_gen = CHIPSET_SB7XX;msg_pdbg("SB7xx/SP5100 detected.\n");} else if (rev >= 0x40 && rev <= 0x42) {amd_gen = CHIPSET_SB89XX;msg_pdbg("SB8xx/SB9xx/Hudson-1 detected.\n");} else {msg_pwarn("SB device found but SMBus revision 0x%02x does not match known values.\n""Assuming SB8xx/SB9xx/Hudson-1. Please send a log to flashrom@flashrom.org\n",rev);amd_gen = CHIPSET_SB89XX;We could error out here as well. The next release is a few weeks away anyway.
No regressions if we can easily avoid them please (I'll break enough stuff unintentionally, rest assured :) It is fairly safe to assume that the code path for sb89xx/hudson will work on such devices if they exist, maybe with some wrong prints if it is sb7xx but nothing we worried about until I provided a solution for it in most cases :)
I'm not convinced that this would likely introduce a regression. For all we know, AMD might use higher SMBus device PCI revision IDs for respins of older chipsets, and there we really want to error out instead of having flashrom deal with bogus errors.
This does only affect pre-hudson2 chipsets. All of them work fine with the existing code for pre-hudson2 chipsets as far as most users are concerned. With my changes applied it would print an additional warning (the one above) and maybe print some verbose messages about registers that do not exist. The latter has been the case since we had sb7xx support and I can't remember anyone complaining (or sending a fix for it). Therefore, I don't see any beneficial effects of bailing out here other than getting notified about different IDs more probably.
- tmp = pci_read_byte(dev, 0xbb);
- /* FIXME: Set bit 3,6,7 if not already set.
- Set bit 5, otherwise SPI accesses are pointless in LPC mode.
- See doc 42413 AMD SB700/710/750 RPR.
- /* Chipset support matrix for SPI Base_Addr (LPC PCI reg 0xa0)
- bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson2+ yangtze
- 3 rsvd <- <- ? <- ? RouteTpm2Spi
- 2 rsvd AbortEnable rsvd ? <- ? <-
- 1 rsvd SpiRomEnable <- ? <- ? <-
- 0 rsvd AltSpiCSEnable<1> rsvd ? <- ? <-
Maybe it's just me, but I have trouble parsing that table. Let's talk about bit 2: What you mean is clear for 6xx (rsvd), 7xx (abortenable), 8xx (abortenable). I'm assuming that this bit is not described for 9xx and hudson1, but why is the hudson2+ entry "?" instead of "<-" ? Or do the "?" entries for 9xx and hudson2+ have a different meaning?
Actually it is the <- that changes meaning, if you will, and the ? are not just "we dont know", as one obviously would to infer. The ? marks values where we do not have a datasheet, but which we assume to be identical to the previous generations. So maybe we should use "<-?" instead? Other suggestions are very welcomed. Now it should also be clear what the <- after a ? means: we have a datasheet and it indicates the same values as the generation *before* the question marks before it. I used this mainly for myself but I would still like to include it in some form.
Let me try to phrase this in a way I can understand: +++++++++++++ How to read this table: "nd" means we have no datasheet for this chipset generation. "ni" means the datasheet for this generation doesn't have any relevant info. "<-" means the bit/register meaning is identical to the next non-ni/non-nd chipset to the left. If a "nd" chipset is between two chipsets with identical meaning, we assume the meaning didn't change twice in between, i.e. the meaning is unchanged for the "nd" chipset. This is a tried and (so far) valid assumption. +++++++++++++ Your "?" would turn into "nd".
That said, keeping your symbols is also an option. My phrasing for that scenario would be: +++++++++++++ How to read this table: "?" means we have no datasheet for this chipset generation or the datasheet doesn't have any relevant info. "<-" means the bit/register meaning is identical to the next non-"?" chipset to the left. "<-" thus never refers to a "?". If a "?" chipset is between two chipsets with identical meaning, we assume the meaning didn't change twice in between, i.e. the meaning is unchanged for the "?" chipset. This is a tried and (so far) valid assumption. +++++++++++++
I have added an explanation to the source.
- <1> It is unknown if that was ever working. On later chipsets the CS config is at memmapped 0x1D.
Heh. Let's find someone with DualBIOS on a SB7xx board. But seriously, I'd assume this works if it's still in the current revision of the register reference guide.
The problem is (IIRC) that it is not too well defined (who would have thought that!) but maybe I am confusing it with Fast Speed.
AFAICS the meaning is well defined, except that vendors apparently didn't use it on the boards we know. A simple test would be to set that bit and check if SPI probe still returns anything. If yes, there are either two flash chips or my understanding of that bit is wrong.
If it is well defined you can surely tell me which pin SPI_CS2# is connected to? Oh actually you can. It is H21 == SPI_CS2#/IMC_GPIO2. My skepticism was rooted in the phrase "When this bit is set, SPI_CS# is routed to a different pin (TBD)" of the RRG. The sb710 databook does actually specify which pin it is...
[…]
*/
- msg_pdbg("(0x%08" PRIx32 ") fastReadEnable=%u, SpiArbEnable=%i, SpiAccessMacRomEn=%i, "
"SpiHostAccessRomEn=%i, ArbWaitCount=%i, ""SpiBridgeDisable=%i, DropOneClkOnRd=%i\n",tmp, (tmp >> 18) & 0x1,(tmp >> 19) & 0x1, (tmp >> 22) & 0x1,(tmp >> 23) & 0x1, (tmp >> 24) & 0x7,(tmp >> 27) & 0x1, (tmp >> 28) & 0x1);
- tmp = mmio_readl(sb600_spibar + 0x00);
- msg_pdbg("(0x%08" PRIx32 ") SpiArbEnable=%i", tmp, (tmp >> 19) & 0x1);
- msg_pdbg(", SpiAccessMacRomEn=%i, SpiHostAccessRomEn=%i, ArbWaitCount=%i",
(tmp >> 22) & 0x1, (tmp >> 23) & 0x1, (tmp >> 24) & 0x7);- if (amd_gen != CHIPSET_YANGTZE)
msg_pdbg(", SpiBridgeDisable=%i", (tmp >> 27) & 0x1);- switch (amd_gen) {
- case CHIPSET_SB7XX:
msg_pdbg(", DropOneClkOnRd/SpiClkGate=%i", (tmp >> 28) & 0x1);Should we check that bit and set it to 0? I haven't yet been able to find a good explanation of what it does, but it seems to be a trick to allow normal read commands at higher speed. I could be mistaken, though.
I would refrain from changing it if we don't really know what it does (and we probably will never do so...). I have read the description a few times... and it does not make a lot of sense to me.
Maybe our friends at AMD/Sage can explain this?
One would have to ask them ;)
- case CHIPSET_SB89XX:
- case CHIPSET_HUDSON234:
msg_pdbg(", SpiBusy=%i", (tmp >> 31) & 0x1);- default: break;
- }
- msg_pdbg("\n");
- if (((tmp >> 22) & 0x1) == 0 || ((tmp >> 23) & 0x1) == 0) {
msg_perr("ERROR: State of SpiAccessMacRomEn or SpiHostAccessRomEn prohibits full access.\n");return ERROR_NONFATAL;- }
- tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; msg_pdbg("NormSpeed is %s MHz\n", speed_names[tmp]);
AFAICS this speed handling stuff is changing in a later patch, correct?
Exactly because this is a good breaking point to avoid a monster patch ;)
Thanks for the review! Apart from the ? explanation and the fatal error, I think this can go in soon?
Yes, looks fine otherwise. Could you please forward-port this patch and repost it? I just want to make sure I didn't overlook anything serious. Otherwise, this is (with the ? explanation and the fatal error)
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
repost follows later.
Also, correct prettyprinting of the registers of the various families, and abort if SpiAccessMacRomEn or SpiHostAccessRomEn prohibit full access.
Tested reading/writing on ASRock IMB-A180, and chipset detection on one of each affected generation by Chris Goodrich from Sage.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- sb600spi.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 110 insertions(+), 27 deletions(-)
diff --git a/sb600spi.c b/sb600spi.c index dcf1b3c..febeabb 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -57,7 +57,28 @@ static enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN; static void determine_generation(struct pci_dev *dev) { amd_gen = CHIPSET_AMD_UNKNOWN; - if (dev->device_id == 0x780e) { + msg_pdbg2("Trying to determine the generation of the SPI interface... "); + if (dev->device_id == 0x438d) { + amd_gen = CHIPSET_SB6XX; + msg_pdbg("SB6xx detected.\n"); + } else if (dev->device_id == 0x439d) { + struct pci_dev *smbus_dev = pci_dev_find(0x1002, 0x4385); + if (smbus_dev == NULL) + return; + uint8_t rev = pci_read_byte(smbus_dev, PCI_REVISION_ID); + if (rev >= 0x39 && rev <= 0x3D) { + amd_gen = CHIPSET_SB7XX; + msg_pdbg("SB7xx/SP5100 detected.\n"); + } else if (rev >= 0x40 && rev <= 0x42) { + amd_gen = CHIPSET_SB89XX; + msg_pdbg("SB8xx/SB9xx/Hudson-1 detected.\n"); + } else { + msg_pwarn("SB device found but SMBus revision 0x%02x does not match known values.\n" + "Assuming SB8xx/SB9xx/Hudson-1. Please send a log to flashrom@flashrom.org\n", + rev); + amd_gen = CHIPSET_SB89XX; + } + } else if (dev->device_id == 0x780e) { /* The PCI ID of the LPC bridge doesn't change between Hudson-2/3/4 and Yangtze (Kabini/Temash) * although they use different SPI interfaces. */ #ifdef USE_YANGTZE_HEURISTICS @@ -94,7 +115,11 @@ static void determine_generation(struct pci_dev *dev) "the output of lspci -nnvx, thanks!.\n", rev); } #endif - } + } else + msg_pwarn("%s: Unknown LPC device %" PRIx16 ":%" PRIx16 ".\n" + "Please report this to flashrom@flashrom.org and include this log and\n" + "the output of lspci -nnvx, thanks!\n", + __func__, dev->vendor_id, dev->device_id); }
static void reset_internal_fifo_pointer(void) @@ -250,7 +275,7 @@ static int sb600_spi_send_command(struct flashctx *flash, unsigned int writecnt, static int sb600_handle_imc(struct pci_dev *dev, bool amd_imc_force) { /* Handle IMC everywhere but sb600 which does not have one. */ - if (dev->device_id == 0x438d) + if (amd_gen == CHIPSET_SB6XX) return 0;
/* TODO: we should not only look at IntegratedImcPresent (LPC Dev 20, Func 3, 40h) but also at @@ -336,6 +361,10 @@ int sb600_probe_spi(struct pci_dev *dev) sb600_spibar += tmp & 0xfff;
determine_generation(dev); + if (amd_gen == CHIPSET_AMD_UNKNOWN) { + msg_perr("Could not determine chipset generation."); + return ERROR_NONFATAL; + }
if (amd_gen == CHIPSET_YANGTZE) { msg_perr("SPI on Kabini/Temash and newer chipsets are not yet supported.\n" @@ -343,35 +372,89 @@ int sb600_probe_spi(struct pci_dev *dev) return ERROR_NONFATAL; }
- tmp = pci_read_long(dev, 0xa0); - msg_pdbg("AltSpiCSEnable=%i, SpiRomEnable=%i, " - "AbortEnable=%i\n", tmp & 0x1, (tmp & 0x2) >> 1, - (tmp & 0x4) >> 2); - tmp = (pci_read_byte(dev, 0xba) & 0x4) >> 2; - msg_pdbg("PrefetchEnSPIFromIMC=%i, ", tmp); - - tmp = pci_read_byte(dev, 0xbb); - /* FIXME: Set bit 3,6,7 if not already set. - * Set bit 5, otherwise SPI accesses are pointless in LPC mode. - * See doc 42413 AMD SB700/710/750 RPR. + /* How to read the following table and similar ones in this file: + * "?" means we have no datasheet for this chipset generation or it doesn't have any relevant info. + * "<-" means the bit/register meaning is identical to the next non-"?" chipset to the left. "<-" thus + * never refers to another "?". + * If a "?" chipset is between two chipsets with identical meaning, we assume the meaning didn't change + * twice in between, i.e. the meaning is unchanged for the "?" chipset. Usually we assume that + * succeeding hardware supports the same functionality as its predecessor unless proven different by + * tests or documentation, hence "?" will often be implemented equally to "<-". + * + * Chipset support matrix for SPI Base_Addr (LPC PCI reg 0xa0) + * bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson2+ yangtze + * 3 rsvd <- <- ? <- ? RouteTpm2Spi + * 2 rsvd AbortEnable rsvd ? <- ? <- + * 1 rsvd SpiRomEnable <- ? <- ? <- + * 0 rsvd AltSpiCSEnable rsvd ? <- ? <- */ - msg_pdbg("PrefetchEnSPIFromHost=%i, SpiOpEnInLpcMode=%i\n", - tmp & 0x1, (tmp & 0x20) >> 5); - tmp = mmio_readl(sb600_spibar); - /* FIXME: If SpiAccessMacRomEn or SpiHostAccessRomEn are zero on - * SB700 or later, reads and writes will be corrupted. Abort in this - * case. Make sure to avoid this check on SB600. + if (amd_gen >= CHIPSET_SB7XX) { + tmp = pci_read_long(dev, 0xa0); + msg_pdbg("SpiRomEnable=%i", (tmp >> 1) & 0x1); + if (amd_gen == CHIPSET_SB7XX) + msg_pdbg(", AltSpiCSEnable=%i, AbortEnable=%i", tmp & 0x1, (tmp >> 2) & 0x1); + + tmp = pci_read_byte(dev, 0xba); + msg_pdbg(", PrefetchEnSPIFromIMC=%i", (tmp & 0x4) >> 2); + + tmp = pci_read_byte(dev, 0xbb); + /* FIXME: Set bit 3,6,7 if not already set. + * Set bit 5, otherwise SPI accesses are pointless in LPC mode. + * See doc 42413 AMD SB700/710/750 RPR. + */ + if (amd_gen == CHIPSET_SB7XX) + msg_pdbg(", SpiOpEnInLpcMode=%i", (tmp >> 5) & 0x1); + msg_pdbg(", PrefetchEnSPIFromHost=%i\n", tmp & 0x1); + } + + /* Chipset support matrix for SPI_Cntrl0 (spibar + 0x0) + * See the chipset support matrix for SPI Base_Addr above for an explanation of the symbols used. + * bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson2+ yangtze + * 17 rsvd <- <- ? <- ? <- + * 18 rsvd <- fastReadEnable<1> ? <- ? SpiReadMode[0] + * 19 SpiArbEnable <- <- ? <- ? <- + * 20 (FifoPtrClr) <- <- ? <- ? <- + * 21 (FifoPtrInc) <- <- ? <- ? IllegalAccess + * 22 SpiAccessMacRomEn <- <- ? <- ? <- + * 23 SpiHostAccessRomEn <- <- ? <- ? <- + * 24:26 ArbWaitCount <- <- ? <- ? <- + * 27 SpiBridgeDisable <- <- ? <- ? rsvd + * 28 rsvd DropOneClkOnRd = SPIClkGate ? <- ? <- + * 29:30 rsvd <- <- ? <- ? SpiReadMode[2:1] + * 31 rsvd <- SpiBusy ? <- ? <- */ - msg_pdbg("(0x%08" PRIx32 ") fastReadEnable=%u, SpiArbEnable=%i, SpiAccessMacRomEn=%i, " - "SpiHostAccessRomEn=%i, ArbWaitCount=%i, " - "SpiBridgeDisable=%i, DropOneClkOnRd=%i\n", - tmp, (tmp >> 18) & 0x1, - (tmp >> 19) & 0x1, (tmp >> 22) & 0x1, - (tmp >> 23) & 0x1, (tmp >> 24) & 0x7, - (tmp >> 27) & 0x1, (tmp >> 28) & 0x1); + tmp = mmio_readl(sb600_spibar + 0x00); + msg_pdbg("(0x%08" PRIx32 ") SpiArbEnable=%i", tmp, (tmp >> 19) & 0x1); + + msg_pdbg(", SpiAccessMacRomEn=%i, SpiHostAccessRomEn=%i, ArbWaitCount=%i", + (tmp >> 22) & 0x1, (tmp >> 23) & 0x1, (tmp >> 24) & 0x7); + + if (amd_gen != CHIPSET_YANGTZE) + msg_pdbg(", SpiBridgeDisable=%i", (tmp >> 27) & 0x1); + + switch (amd_gen) { + case CHIPSET_SB7XX: + msg_pdbg(", DropOneClkOnRd/SpiClkGate=%i", (tmp >> 28) & 0x1); + case CHIPSET_SB89XX: + case CHIPSET_HUDSON234: + msg_pdbg(", SpiBusy=%i", (tmp >> 31) & 0x1); + default: break; + } + msg_pdbg("\n"); + + if (((tmp >> 22) & 0x1) == 0 || ((tmp >> 23) & 0x1) == 0) { + msg_perr("ERROR: State of SpiAccessMacRomEn or SpiHostAccessRomEn prohibits full access.\n"); + return ERROR_NONFATAL; + } + tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; msg_pdbg("NormSpeed is %s MHz\n", speed_names[tmp]);
+ if (amd_gen >= CHIPSET_SB89XX) { + tmp = mmio_readb(sb600_spibar + 0x1D); + msg_pdbg("Using SPI_CS%d\n", tmp & 0x3); + } + /* Look for the SMBus device. */ smbus_dev = pci_dev_find(0x1002, 0x4385); if (!smbus_dev) {
Am 11.09.2013 15:51 schrieb Stefan Tauner:
On Tue, 10 Sep 2013 09:31:13 +0200 (CEST) "Carl-Daniel U. Hailfinger" c-d.hailfinger.devel.2006@gmx.net wrote:
[Sorry for breaking the threading. This is meant to be a reply to http://patchwork.coreboot.org/patch/4016/ ] Stefan Tauner wrote on 2013-08-14 17:35:13
On Wed, 14 Aug 2013 08:42:15 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 10.08.2013 03:45 schrieb Stefan Tauner:
Also, correct prettyprinting of the registers of the various families, and abort if SpiAccessMacRomEn or SpiHostAccessRomEn prohibit full access.
Tested reading/writing on ASRock IMB-A180, and chipset detection on one of each affected generation by Chris Goodrich from Sage.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
sb600spi.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 106 insertions(+), 27 deletions(-)
diff --git a/sb600spi.c b/sb600spi.c index cb7c4ac..c9be44c 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -57,7 +57,28 @@ static enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN; static void determine_generation(struct pci_dev *dev) { amd_gen = CHIPSET_AMD_UNKNOWN;
- if (dev->device_id == 0x780e) {
- msg_pdbg2("Trying to determine the generation of the SPI interface... ");
- if (dev->device_id == 0x438d) {
amd_gen = CHIPSET_SB6XX;msg_pdbg("SB6xx detected.\n");- } else if (dev->device_id == 0x439d) {
struct pci_dev *smbus_dev = pci_dev_find(0x1002, 0x4385);Why do we search twice for this SMBus device, once inside sb600_probe_spi and once here? Can't we just pass the smbus device as second parameter to determine_generation?
Yes we could and I thought about this too but came to the conclusion that it is not worth it. Do you think otherwise?
I'd really like to limit the number of times we search for PCI devices. On some machines with lots of PCI devices that can take a lot of time.
I have no idea how you come to that conclusion. This uses 0 bus accesses AFAICS because libpci caches that information, but even if we would have to rescan all devices on the bus it would be rather negligible: it does only apply to very few users and I am not sure if any of them would notice it. It might not be pretty theoretically, but I don't anticipate any problems in reality at all. What do you think is the worst case latency added? A few ms?
I think I read such a warning about PCI bus scan speed either on the linux kernel mailing list or in the pciutils source. Sorry, it's been a few years, and the only thing I remember about said warning was that on large IBM/SGI machines (dozens of PCI buses, many devices) a full scan could take quite some time. Then again, AFAIK current libpci does cache the bus info and after one full scan (i.e. a search by PCI vendor/device ID), no further slowdown happens. I retract my objection.
That said, I also think such refactoring can be done in a later iteration, and it is also possible to have the generation detection as first command inside the AMD SBx00 SPI probe function, and let it set a static struct *smbus_dev. Your choice when/how to deal with it.
Anyway, most/all of the code above was committed in r1706, so this is mostly an after-the-fact review.
ATM we do only look for the smbus device twice on hudson 2+ and yangtze. With the rebased version of this patch it will be done for all amd chipsets since sb7xx:
if (smbus_dev == NULL)return;uint8_t rev = pci_read_byte(smbus_dev, PCI_REVISION_ID);if (rev >= 0x39 && rev <= 0x3D) {amd_gen = CHIPSET_SB7XX;msg_pdbg("SB7xx/SP5100 detected.\n");} else if (rev >= 0x40 && rev <= 0x42) {amd_gen = CHIPSET_SB89XX;msg_pdbg("SB8xx/SB9xx/Hudson-1 detected.\n");} else {msg_pwarn("SB device found but SMBus revision 0x%02x does not match known values.\n""Assuming SB8xx/SB9xx/Hudson-1. Please send a log to flashrom@flashrom.org\n",rev);amd_gen = CHIPSET_SB89XX;We could error out here as well. The next release is a few weeks away anyway.
No regressions if we can easily avoid them please (I'll break enough stuff unintentionally, rest assured :) It is fairly safe to assume that the code path for sb89xx/hudson will work on such devices if they exist, maybe with some wrong prints if it is sb7xx but nothing we worried about until I provided a solution for it in most cases :)
I'm not convinced that this would likely introduce a regression. For all we know, AMD might use higher SMBus device PCI revision IDs for respins of older chipsets, and there we really want to error out instead of having flashrom deal with bogus errors.
This does only affect pre-hudson2 chipsets. All of them work fine with the existing code for pre-hudson2 chipsets as far as most users are concerned. With my changes applied it would print an additional warning (the one above) and maybe print some verbose messages about registers that do not exist. The latter has been the case since we had sb7xx support and I can't remember anyone complaining (or sending a fix for it). Therefore, I don't see any beneficial effects of bailing out here other than getting notified about different IDs more probably.
Ah OK, I must have misread the code. My fear was that Hudson-2 code might be run by accident on pre-Hudson-2 chipsets if such chipsets had newer SMBus PCI revision IDs.
- tmp = pci_read_byte(dev, 0xbb);
- /* FIXME: Set bit 3,6,7 if not already set.
- Set bit 5, otherwise SPI accesses are pointless in LPC mode.
- See doc 42413 AMD SB700/710/750 RPR.
- /* Chipset support matrix for SPI Base_Addr (LPC PCI reg 0xa0)
- bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson2+ yangtze
- 3 rsvd <- <- ? <- ? RouteTpm2Spi
- 2 rsvd AbortEnable rsvd ? <- ? <-
- 1 rsvd SpiRomEnable <- ? <- ? <-
- 0 rsvd AltSpiCSEnable<1> rsvd ? <- ? <-
Maybe it's just me, but I have trouble parsing that table. [...]
[...] The ? marks values where we do not have a datasheet, but which we assume to be identical to the previous generations. [...]
How to read this table: [...]
I have added an explanation to the source.
Thanks!
- <1> It is unknown if that was ever working. On later chipsets the CS config is at memmapped 0x1D.
Heh. Let's find someone with DualBIOS on a SB7xx board. But seriously, I'd assume this works if it's still in the current revision of the register reference guide.
The problem is (IIRC) that it is not too well defined (who would have thought that!) but maybe I am confusing it with Fast Speed.
AFAICS the meaning is well defined, except that vendors apparently didn't use it on the boards we know. A simple test would be to set that bit and check if SPI probe still returns anything. If yes, there are either two flash chips or my understanding of that bit is wrong.
If it is well defined you can surely tell me which pin SPI_CS2# is connected to? Oh actually you can. It is H21 == SPI_CS2#/IMC_GPIO2. My skepticism was rooted in the phrase "When this bit is set, SPI_CS# is routed to a different pin (TBD)" of the RRG. The sb710 databook does actually specify which pin it is...
Good detective work!
- case CHIPSET_SB89XX:
- case CHIPSET_HUDSON234:
msg_pdbg(", SpiBusy=%i", (tmp >> 31) & 0x1);- default: break;
- }
- msg_pdbg("\n");
- if (((tmp >> 22) & 0x1) == 0 || ((tmp >> 23) & 0x1) == 0) {
msg_perr("ERROR: State of SpiAccessMacRomEn or SpiHostAccessRomEn prohibits full access.\n");return ERROR_NONFATAL;- }
- tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3; msg_pdbg("NormSpeed is %s MHz\n", speed_names[tmp]);
AFAICS this speed handling stuff is changing in a later patch, correct?
Exactly because this is a good breaking point to avoid a monster patch ;)
Thanks for the review! Apart from the ? explanation and the fatal error, I think this can go in soon?
Yes, looks fine otherwise. Could you please forward-port this patch and repost it? I just want to make sure I didn't overlook anything serious. Otherwise, this is (with the ? explanation and the fatal error)
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
repost follows later.
Thanks, repost seen and acked.
Regards, Carl-Daniel
Hello everyone,
we have stumbled upon something in the AMD southbridge docs which we don't understand: SPI controller register SPI_Cntrl0 (address spibar + 0x0), bit 28. In various datasheets it is called either SpiClkGate or DropOneClkOnRd.
What exactly does that bit do? 1. Does it stop the SPI clock for the duration of one clock cycle (i.e. one bit) after sending the last bit on the MOSI line (thereby effectively doubling the time between last bit sent to the flash chip and the first bit from the flash chip)? 2. Does it stop the SPI clock for the duration of 8 clock cycles (i.e. one byte)? 3. Does it send out a dummy byte between last data bit on MOSI and first data bit on MISO to allow easier implementation of SPI FAST READ commands? 4. Does it do something different entirely?
The code in flashrom inside sb600spi.c which we're working on is reproduced below, together with some context:
Am 11.09.2013 15:51 schrieb Stefan Tauner:
On Tue, 10 Sep 2013 09:31:13 +0200 (CEST) "Carl-Daniel U. Hailfinger" c-d.hailfinger.devel.2006@gmx.net wrote:
[Sorry for breaking the threading. This is meant to be a reply to http://patchwork.coreboot.org/patch/4016/ ] Stefan Tauner wrote on 2013-08-14 17:35:13
On Wed, 14 Aug 2013 08:42:15 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 10.08.2013 03:45 schrieb Stefan Tauner:
- tmp = mmio_readl(sb600_spibar + 0x00);
- msg_pdbg("(0x%08" PRIx32 ") SpiArbEnable=%i", tmp, (tmp >> 19) & 0x1);
- msg_pdbg(", SpiAccessMacRomEn=%i, SpiHostAccessRomEn=%i, ArbWaitCount=%i",
(tmp >> 22) & 0x1, (tmp >> 23) & 0x1, (tmp >> 24) & 0x7);- if (amd_gen != CHIPSET_YANGTZE)
msg_pdbg(", SpiBridgeDisable=%i", (tmp >> 27) & 0x1);- switch (amd_gen) {
- case CHIPSET_SB7XX:
msg_pdbg(", DropOneClkOnRd/SpiClkGate=%i", (tmp >> 28) & 0x1);Should we check that bit and set it to 0? I haven't yet been able to find a good explanation of what it does, but it seems to be a trick to allow normal read commands at higher speed. I could be mistaken, though.
I would refrain from changing it if we don't really know what it does (and we probably will never do so...). I have read the description a few times... and it does not make a lot of sense to me.
Maybe our friends at AMD/Sage can explain this?
One would have to ask them ;)
Any insights are appreciated.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Tauner