Re: [flashrom] [PATCH 1/6] sbxxx: Add detection for the remaining AMD chipset families.

[Carl-Daniel Hailfinger]

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
>
> 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?
> 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 [e-mail]\n",
> 		   rev);
> 	amd_gen = CHIPSET_SB89XX;

We could error out here as well. The next release is a few weeks away
anyway.
> }
> } 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 [e-mail] 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.
> 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?
> *
> *  <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.
> */
> 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?
>  */
> 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 "?".
> * 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.
> */
> 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.
> 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?
> 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) {

Regards,
Carl-Daniel

[PATCH 1/6] sbxxx: Add detection for the remaining AMD chipset families.

[Stefan Tauner]

On Wed, 14 Aug 2013 08:42:15 +0200
Carl-Daniel Hailfinger  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
> >
> > 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 [e-mail]\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 [e-mail] 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?