Project

General

Profile

Actions

Bug #370

open

Fix AMD programmer (sb600spi.c) for Promontory and chips >16MB size

Added by Anastasia Klimchuk over 2 years ago. Updated 5 months ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
Programmer
Target version:
Start date:
04/28/2022
Due date:
% Done:

0%

Estimated time:
Affected versions:
Needs backport to:
Affected hardware:
Affected OS:

Description

Original patch is linked
Specific questions:
1) API violations
2) Soft bricks machines with >16MiB flash
3) Blocking other cleanup work

Actions #1

Updated by Edward . over 2 years ago

  • Assignee set to Edward .
Actions #2

Updated by Felix Singer over 2 years ago

Shouldn't this go to the bug tracker?

Actions #3

Updated by Edward . over 2 years ago

  • Related links updated (diff)
Actions #4

Updated by Felix Singer about 2 years ago

  • Tracker changed from Feature to Bug
  • Target version changed from 1.3 to main
Actions #5

Updated by Anastasia Klimchuk about 2 years ago

  • Parent task deleted (#353)

Promontory support is dropped from 1.3.x as https://review.coreboot.org/c/flashrom/+/68824

I am unlinking this bug from parent 353, keeping the same assignee. The work under this bug continues on master.

Actions #6

Updated by Anastasia Klimchuk 6 months ago

Many thanks to Felix Held for providing detailed information about the bug and overall state of sb600spi programmer. I am updating the ticket with details from Felix.

For the SPI controller on newer AMD SoCs, 'promontory' is a misnomer, since the SPI100 controller is part of the integrated FCH inside the newer SoCs and Promontory is basically an external IO extender that connects via PCIe to the SoC and provides PCIe, SATA and USB3 ports. Like most if not all laptop platforms, none of the Chromebooks contains a promontory chip.

The main issues are the following.

One problem is that the code accesses registers and bits that aren't implemented on newer platforms. Splitting the legacy pre-SPI100 code and the SPI100 code into two programmers might be something to look into, but not sure how different the old pre-SPI100 and SPI100 controllers are. Also not sure when the SPI100 controller was introduced; it was quite some time before family 17h though. Yangtze should already have the SPI100, but this needs to be checked in the documentation.

Next problem is that flashrom unconditionally uses or at least used the 16MByte ROM2 MMIO mapping of the SPI flash right below 4GB to read it, but doesn't check if the flash isn't larger than 16MByte. So when the SPI flash is larger, flashrom will access MMIO that it shouldn't, which might cause the system to crash. Using the SPI100 FIFO allows accessing larger SPI flash chips, but it's slower, since that won't be using the dual/quad SPI modes and their typically higher frequencies. A guess is that using the MMIO mapping was probably a speed optimization, but it's missing a check if it can be used or if it's needed to fall back to the slower FIFO access path.
It needs to be checked in documentation whether there is anything to allow reading larger flash chips via MMIO.

An idea about patch 65239:
either a SPI ROM size check should be done for the MMIO path or the ROM3 mapping should be used for larger than 16MByte flashes. Knowing if we're dealing with a larger than 16MByte SPI flash requires the SPI flash to already be probed which probably isn't the case when the SPI host controller is probed. So not sure how to do that. Maybe check some data structure about the flash after it got probed and use that info when trying to read it?

Actions #7

Updated by Anastasia Klimchuk 5 months ago

  • Subject changed from Fix sb600spi to Fix AMD programmer (sb600spi.c) for Promontory and chips >16MB size
  • Category changed from Release prep to Programmer
  • Assignee deleted (Edward .)
Actions

Also available in: Atom PDF