Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: Update SPI from 66MHz, 112 to 100MHz, 122 ......................................................................
Update SPI from 66MHz, 112 to 100MHz, 122
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I14f96e3c085126c70e64ef3a3f5b7b54ce6cbffe --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/43306/1
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree.cb b/src/mainboard/google/zork/variants/baseboard/devicetree.cb index 9db0d23..ca33bbd 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree.cb @@ -140,11 +140,11 @@
# SPI Configuration register "common_config.spi_config" = "{ - .normal_speed = SPI_SPEED_66M, /* MHz */ - .fast_speed = SPI_SPEED_66M, /* MHz */ + .normal_speed = SPI_SPEED_100M, /* MHz */ + .fast_speed = SPI_SPEED_100M, /* MHz */ .altio_speed = SPI_SPEED_66M, /* MHz */ .tpm_speed = SPI_SPEED_66M, /* MHz */ - .read_mode = SPI_READ_MODE_DUAL112, + .read_mode = SPI_READ_MODE_DUAL122, }"
# eSPI Configuration
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43306
to look at the new patch set (#2).
Change subject: mb/google/zork: Update SPI mode to100MHz, 122 ......................................................................
mb/google/zork: Update SPI mode to100MHz, 122
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I14f96e3c085126c70e64ef3a3f5b7b54ce6cbffe --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/43306/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43306
to look at the new patch set (#3).
Change subject: mb/google/zork: Update SPI mode to 100MHz, 122 ......................................................................
mb/google/zork: Update SPI mode to 100MHz, 122
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I14f96e3c085126c70e64ef3a3f5b7b54ce6cbffe --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/43306/3
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 122 ......................................................................
Patch Set 3: Code-Review+1
So 100MHZ is working now? Is there a bug?
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 122 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43306/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43306/3//COMMIT_MSG@7 PS3, Line 7: mb/google/zork: Update SPI mode to 100MHz, 122 bug & test to make sure it's working? Did you try this on Dalboz+Ezk+Trembyle?
b/157452205
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Eric Peers, Rob Barnes, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43306
to look at the new patch set (#4).
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
mb/google/zork: Update SPI mode to 100MHz, 1-2-2
Change SPI speed from 66MHz, mode 1-1-2 to 100MHz mode 1-2-2.
“1-2-2" means command, address and data are transmitted through 1 wire, 2 wire and 2 wire, respectively.
BUG=b:160603142 TEST=Boot on trembyle, verify register settings.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I14f96e3c085126c70e64ef3a3f5b7b54ce6cbffe --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/43306/4
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43306/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43306/3//COMMIT_MSG@7 PS3, Line 7: mb/google/zork: Update SPI mode to 100MHz, 122
bug & test to make sure it's working? Did you try this on Dalboz+Ezk+Trembyle? […]
I tested on Trembyle and Dalboz. The ODM devices should have followed the reference design, so it should work on them, but I haven't tested all the OEM designs.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
mb/google/zork: Update SPI mode to 100MHz, 1-2-2
Change SPI speed from 66MHz, mode 1-1-2 to 100MHz mode 1-2-2.
“1-2-2" means command, address and data are transmitted through 1 wire, 2 wire and 2 wire, respectively.
BUG=b:160603142 TEST=Boot on trembyle, verify register settings.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I14f96e3c085126c70e64ef3a3f5b7b54ce6cbffe Reviewed-on: https://review.coreboot.org/c/coreboot/+/43306 Reviewed-by: Felix Held felix-coreboot@felixheld.de Reviewed-by: Rob Barnes robbarnes@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/baseboard/devicetree.cb 1 file changed, 3 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Rob Barnes: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree.cb b/src/mainboard/google/zork/variants/baseboard/devicetree.cb index 9db0d23..ca33bbd 100644 --- a/src/mainboard/google/zork/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/zork/variants/baseboard/devicetree.cb @@ -140,11 +140,11 @@
# SPI Configuration register "common_config.spi_config" = "{ - .normal_speed = SPI_SPEED_66M, /* MHz */ - .fast_speed = SPI_SPEED_66M, /* MHz */ + .normal_speed = SPI_SPEED_100M, /* MHz */ + .fast_speed = SPI_SPEED_100M, /* MHz */ .altio_speed = SPI_SPEED_66M, /* MHz */ .tpm_speed = SPI_SPEED_66M, /* MHz */ - .read_mode = SPI_READ_MODE_DUAL112, + .read_mode = SPI_READ_MODE_DUAL122, }"
# eSPI Configuration
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 143: .normal_speed = SPI_SPEED_100M, /* MHz */ It used to be that the SPI controllers normal speed register was applied to most of the commands other than memory-mapped code fetches. Those would use the Fast Read command format enabled with read_mode below.
Looking at a somewhat common W25Q128FW (1.8V 128 Mbit datasheet rev K) there are commands listed with max frequency <100MHz. Infact, I don't remember seeing any SPI NOR flash that supports >55 MHz for a Read Data instruction without dummy cycles.
Something similar once broke 'flashrom -p internal' on older discrete FCH.
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 144: .fast_speed = SPI_SPEED_100M, /* MHz */ You may want to double-check that alternative parts in the BOMs can all accept this. If I read the Winbond datasheet correctly, maximum clock for that particular part with DUAL122 would be 80MHz.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 143: .normal_speed = SPI_SPEED_100M, /* MHz */
It used to be that the SPI controllers normal speed register was applied to most of the commands oth […]
Personally, I thought this should be 33, but it's been working at 66 and it's working at 100. I'm not sure what that means. Maybe it'll break when we fix whatever the issue is that's killing the bandwidth, but it's working so far.
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 144: .fast_speed = SPI_SPEED_100M, /* MHz */
You may want to double-check that alternative parts in the BOMs can all accept this. […]
I think that if the alternate SPI chips don't support the same speeds, the OEMs need to use different chips that do.
The EEs are going to recheck the timings with this fix in.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 143: .normal_speed = SPI_SPEED_100M, /* MHz */
Personally, I thought this should be 33, but it's been working at 66 and it's working at 100. […]
I checked a bit deeper, I believe 100M is desired here for faster Page Program command and the implicit Read Data command (via memory-mapped translation) speed is further capped with the spi-read-mode configuration.
In coreboot proper, the problematic case of normal_speed>=50MHz and expicit use of Read Data command 0x03 (CMD_READ_ARRAY_SLOW) seems to be guarded by Kconfig SPI_FLASH_NO_FAST_READ nowadays.
In flashrom/spi25.c:spi_nbyte_read() there is reference to JEDEC_READ command 0x03 but my first impression is that it's not on the execution path with -p internal.
I tracked down the old flashrom breakage, I have fixed it in CB:5089 and it was a case of attempting Normal Read with 66MHz..
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 144: .fast_speed = SPI_SPEED_100M, /* MHz */
I think that if the alternate SPI chips don't support the same speeds, the OEMs need to use differen […]
Thanks. Seems like these latest SPI flash parts are not as interchangeable as they used to be. Apparently Micron parts can do 100M+ in DUAL_122 with some (vendor-specific?) dummy-cycle configurations applied. I only now came across the EFS SPI patches.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/43306/5/src/mainboard/google/zork/v... PS5, Line 143: .normal_speed = SPI_SPEED_100M, /* MHz */
I checked a bit deeper, I believe 100M is desired here for faster Page Program command […]
I'm rather certain that the speed setting for the normal read mode shouldn't be 100MHz. Difference between normal read mode and fast read mode is that they use different commands and that the fast mode sends a dummy byte after the address, so it can operate at a higher frequency and throughput, but the latency of the flash isn't an issue.
Since the mode is set to SPI_READ_MODE_DUAL122, the system should always use the fast mode, so this won't cause any issue, but it's still very likely wrong.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43306 )
Change subject: mb/google/zork: Update SPI mode to 100MHz, 1-2-2 ......................................................................
Patch Set 5:
Effectively reverted with commit 13ec6a03a.
-
Eric Peers (Code Review) -
Felix Held (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Rob Barnes (Code Review)