On Wed, 14 Aug 2013 08:42:15 +0200 Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> > > --- > > 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 [email protected]\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 [email protected] 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_speed > > Which 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? -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
