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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Sep 12 17:44:06 CEST 2013


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 at 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 at 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 at 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 at 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 at gmx.net>
> repost follows later.

Thanks, repost seen and acked.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list