Change in ...coreboot[master]: cbfs_spi: Enable speed logging by default for BIOS_DEBUG

[Julius Werner (Code Review)]

Hello Aaron Durbin, Furquan Shaikh,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/33281

to review the following change.

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

cbfs_spi: Enable speed logging by default for BIOS_DEBUG

The SPI transfer speed logging in cbfs_spi is super useful, doesn't get
in the way (just adding one line per stage, essentially) and should have
no notable overhead. Let's enable it by default for the BIOS_DEBUG log
level rather than having to recompile to get it.

Change-Id: I03c77938afe01fdcecf917e8c4c25cc29cdc764e
Signed-off-by: Julius Werner 
---
M src/drivers/spi/cbfs_spi.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/33281/1

diff --git a/src/drivers/spi/cbfs_spi.c b/src/drivers/spi/cbfs_spi.c
index e311752..175af25 100644
--- a/src/drivers/spi/cbfs_spi.c
+++ b/src/drivers/spi/cbfs_spi.c
@@ -31,21 +31,18 @@
 static bool spi_flash_init_done;

/*
- * Set this to 1 to debug SPI speed, 0 to disable it
- * The format is:
+ * SPI speed logging for big transfers available with BIOS_DEBUG. The format is:
  *
- * read SPI 62854 7db7: 10416 us, 3089 KB/s, 24.712 Mbps
+ * read SPI 0x62854 0x7db7: 10416 us, 3089 KB/s, 24.712 Mbps
  *
  * The important number is the last one. It should roughly match your SPI
  * clock. If it doesn't, your driver might need a little tuning.
  */
-#define SPI_SPEED_DEBUG		0
-
 static ssize_t spi_readat(const struct region_device *rd, void *b,
    			size_t offset, size_t size)
 {
    struct stopwatch sw;
-	bool show = SPI_SPEED_DEBUG && size >= 4 * KiB;
+	bool show = size >= 4 * KiB && console_log_level(BIOS_DEBUG);

if (show)
    	stopwatch_init(&sw);

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 1: Code-Review+2

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 1:

This seems reasonable, but why is it super-useful?

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 1:
> This seems reasonable, but why is it super-useful?

I just found it very useful testing the Dual SPI patch and thought "why don't we have this on all the time?" It's probably something you want to look at at least once for every board bring-up.

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 1: Code-Review+1

Fair enough. There might be a code-size issue though.

I could +2 on Google acct but I see that Aaron already has

[Julius Werner (Code Review)]

Hello Aaron Durbin, Simon Glass, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33281

to look at the new patch set (#2).

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

cbfs_spi: Enable speed logging by default for BIOS_DEBUG

The SPI transfer speed logging in cbfs_spi is super useful, doesn't get
in the way (just adding one line per stage, essentially) and should have
no notable overhead. Let's enable it by default for the BIOS_DEBUG log
level rather than having to recompile to get it.

Also fix an issue with building this code on MIPS due to lack of 64-bit
division primitives. (This means MIPS and arm32 board may display
incorrect results when reading more than 4MB in a single transfer, which
sounds very unlikely.)

Change-Id: I03c77938afe01fdcecf917e8c4c25cc29cdc764e
Signed-off-by: Julius Werner 
---
M src/drivers/spi/cbfs_spi.c
1 file changed, 4 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/33281/2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 2: Code-Review+2

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

Patch Set 2: Code-Review+2

[Julius Werner (Code Review)]

Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33281 )

Change subject: cbfs_spi: Enable speed logging by default for BIOS_DEBUG
......................................................................

cbfs_spi: Enable speed logging by default for BIOS_DEBUG

The SPI transfer speed logging in cbfs_spi is super useful, doesn't get
in the way (just adding one line per stage, essentially) and should have
no notable overhead. Let's enable it by default for the BIOS_DEBUG log
level rather than having to recompile to get it.

Also fix an issue with building this code on MIPS due to lack of 64-bit
division primitives. (This means MIPS and arm32 board may display
incorrect results when reading more than 4MB in a single transfer, which
sounds very unlikely.)

Change-Id: I03c77938afe01fdcecf917e8c4c25cc29cdc764e
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33281
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
Reviewed-by: Simon Glass 
Reviewed-by: Furquan Shaikh 
---
M src/drivers/spi/cbfs_spi.c
1 file changed, 4 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Simon Glass: Looks good to me, approved

diff --git a/src/drivers/spi/cbfs_spi.c b/src/drivers/spi/cbfs_spi.c
index e311752..ad282c6 100644
--- a/src/drivers/spi/cbfs_spi.c
+++ b/src/drivers/spi/cbfs_spi.c
@@ -31,21 +31,18 @@
 static bool spi_flash_init_done;

/*
- * Set this to 1 to debug SPI speed, 0 to disable it
- * The format is:
+ * SPI speed logging for big transfers available with BIOS_DEBUG. The format is:
  *
- * read SPI 62854 7db7: 10416 us, 3089 KB/s, 24.712 Mbps
+ * read SPI 0x62854 0x7db7: 10416 us, 3089 KB/s, 24.712 Mbps
  *
  * The important number is the last one. It should roughly match your SPI
  * clock. If it doesn't, your driver might need a little tuning.
  */
-#define SPI_SPEED_DEBUG		0
-
 static ssize_t spi_readat(const struct region_device *rd, void *b,
    			size_t offset, size_t size)
 {
    struct stopwatch sw;
-	bool show = SPI_SPEED_DEBUG && size >= 4 * KiB;
+	bool show = size >= 4 * KiB && console_log_level(BIOS_DEBUG);

if (show)
    	stopwatch_init(&sw);
@@ -58,7 +55,7 @@
    	u64 speed;	/* KiB/s */
    	int bps;	/* Bits per second */

-		speed = (u64)size * 1000 / usecs;
+		speed = size * 1000 / usecs;
    	bps = speed * 8;

printk(BIOS_DEBUG, "read SPI %#zx %#zx: %ld us, %lld KB/s, %d.%03d Mbps\n",