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?
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 :)
}- } 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
@@ -334,6 +359,12 @@ int sb600_probe_spi(struct pci_dev *dev) sb600_spibar += tmp & 0xfff;
determine_generation(dev);
- if (amd_gen == CHIPSET_AMD_UNKNOWN) {
/* FIXME: Eventually this should be a fatal error, but until we are certain to have covered all* IDs actually found in the wild, try to mimmick the previous behavior. */Given that we're now post-0.9.7, we can make this a fatal error. Besides that, the current patch doesn't even set CHIPSET_AMD_UNKNOWN for some combinations with unknown SMBus PCI revision ID.
Sure, this was targeted to be in 0.9.7 (and see above).
msg_pwarn("Could not determine chipset generation. Assuming SB6xx.");amd_gen = CHIPSET_SB6XX;}
if (amd_gen == CHIPSET_YANGTZE) { msg_perr("SPI on Kabini/Temash and newer chipsets are not yet supported.\n"
@@ -341,35 +372,83 @@ 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.
- /* 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.
** <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.
*/
- 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.Are these bits required to be set even for post-SB7000 chipsets?
That's left as an exercise to the reader. :P 5 is reserved anywhere but on sb7x/sp5100, the other bits change semantics in sb8xx...
*/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)
* bit 6xx 7xx/SP5100 8xx 9xx hudson1 hudson2+ yangtze* 17 rsvd <- <- ? <- ? <-Again the table I don't understand. Maybe you should add an explanation which specifies the meaning of "?".
Given the explanation above, would you mind formulating it? IMHO it improves such things quite a bit if another mind puts it into words than the one inventing it.
* 18 rsvd <- fastReadEnable<1> ? <- ? SpiReadMode[0]<1>* 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]<1>* 31 rsvd <- SpiBusy ? <- ? <-** <1> see handle_speedWhich handle_speed? Can't find it in svn HEAD nor in this patch.
Added later in the patch set. Do you really want me to change those lines again in the followup patch? I had the impression you hate that :)
*/
- 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.
- 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?