Change in coreboot[master]: include/spi-generic: moving common flash timouts

[Uwe Poeche (Code Review)]

Uwe Poeche has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: moving common flash timouts
......................................................................

include/spi-generic: moving common flash timouts

This patch moves SPI_FLASH Timeouts from spi/spi_flash_internal.h for
SPI SW-sequencing in
include/spi-generic.h to provide also for SPI HW-sequencing.

tested on siemens/bdx1 and checked if all includes of
spi_flash_internal.h on other places provide an include of
spi-generic.h before

Change-Id: I837f1a027b836996bc42389bdf7dbab7f0e9db09
Signed-off-by: Uwe Poeche 
---
M src/drivers/spi/spi_flash_internal.h
M src/include/spi-generic.h
2 files changed, 9 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34345/1

diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h
index 4a9e289..95c51a8 100644
--- a/src/drivers/spi/spi_flash_internal.h
+++ b/src/drivers/spi/spi_flash_internal.h
@@ -7,15 +7,6 @@
 #ifndef SPI_FLASH_INTERNAL_H
 #define SPI_FLASH_INTERNAL_H

-/* Common parameters -- kind of high, but they should only occur when there
- * is a problem (and well your system already is broken), so err on the side
- * of caution in case we're dealing with slower SPI buses and/or processors.
- */
-#define CONF_SYS_HZ 100
-#define SPI_FLASH_PROG_TIMEOUT		(2 * CONF_SYS_HZ)
-#define SPI_FLASH_PAGE_ERASE_TIMEOUT	(5 * CONF_SYS_HZ)
-#define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
-
 /* Common commands */
 #define CMD_READ_ID			0x9f

diff --git a/src/include/spi-generic.h b/src/include/spi-generic.h
index d0f957f..93704f8 100644
--- a/src/include/spi-generic.h
+++ b/src/include/spi-generic.h
@@ -16,6 +16,15 @@
 #ifndef _SPI_GENERIC_H_
 #define _SPI_GENERIC_H_

+/* Common parameters -- kind of high, but they should only occur when there
+ * is a problem (and well your system already is broken), so err on the side
+ * of caution in case we're dealing with slower SPI buses and/or processors.
+ */
+#define CONF_SYS_HZ 100
+#define SPI_FLASH_PROG_TIMEOUT		(2 * CONF_SYS_HZ)
+#define SPI_FLASH_PAGE_ERASE_TIMEOUT	(5 * CONF_SYS_HZ)
+#define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
+
 #include <commonlib/region.h>
 #include <stdint.h>
 #include <stddef.h>

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: moving common flash timouts
......................................................................

Patch Set 1:

(3 comments)

Thanks.

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG@9 
PS1, Line 9: Timeouts
time-outs

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG@10 
PS1, Line 10: in
to

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG@11 
PS1, Line 11: include/spi-generic.h to provide also for SPI HW-sequencing.
Some parts of this should fit on the line above.

[Uwe Poeche (Code Review)]

Hello David Hendricks, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: include/spi-generic: moving common flash timouts
......................................................................

include/spi-generic: moving common flash timouts

This patch moves SPI_FLASH time-outs from spi/spi_flash_internal.h for
SPI SW-sequencing to include/spi-generic.h to provide also for
SPI HW-sequencing.

tested on siemens/bdx1 and checked if all includes of
spi_flash_internal.h on other places provide an include of
spi-generic.h before

Change-Id: I837f1a027b836996bc42389bdf7dbab7f0e9db09
Signed-off-by: Uwe Poeche 
---
M src/drivers/spi/spi_flash_internal.h
M src/include/spi-generic.h
2 files changed, 9 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34345/2

[Uwe Poeche (Code Review)]

Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: moving common flash timouts
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG@9 
PS1, Line 9: Timeouts
> time-outs

Done

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG@10 
PS1, Line 10: in
> to

Done

https://review.coreboot.org/c/coreboot/+/34345/1//COMMIT_MSG@11 
PS1, Line 11: include/spi-generic.h to provide also for SPI HW-sequencing.
> Some parts of this should fit on the line above.

Done

[Furquan Shaikh (Code Review)]

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

Change subject: include/spi-generic: moving common flash timouts
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34345/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34345/2//COMMIT_MSG@7 
PS2, Line 7: timouts
timeouts

Change in coreboot[master]: include/spi-generic: moving common flash timeouts

[Uwe Poeche (Code Review)]

Hello Julius Werner, David Hendricks, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: include/spi-generic: moving common flash timeouts
......................................................................

include/spi-generic: moving common flash timeouts

This patch moves SPI_FLASH time-outs from spi/spi_flash_internal.h for
SPI SW-sequencing to include/spi-generic.h to provide also for
SPI HW-sequencing.

tested on siemens/bdx1 and checked if all includes of
spi_flash_internal.h on other places provide an include of
spi-generic.h before

Change-Id: I837f1a027b836996bc42389bdf7dbab7f0e9db09
Signed-off-by: Uwe Poeche 
---
M src/drivers/spi/spi_flash_internal.h
M src/include/spi-generic.h
2 files changed, 9 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34345/3

Change in coreboot[master]: include/spi-generic: moving common flash timeouts

[Uwe Poeche (Code Review)]

Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: moving common flash timeouts
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34345/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34345/2//COMMIT_MSG@7 
PS2, Line 7: timouts
> timeouts

Done

Change in coreboot[master]: include/spi-generic: moving common flash timeouts

[Julius Werner (Code Review)]

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

Change subject: include/spi-generic: moving common flash timeouts
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h 
File src/include/spi-generic.h:

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h@2... 
PS3, Line 26: #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
nit: While we're here, do we maybe want to clean these up a little? Does anyone understand what CONF_SYS_HZ is supposed to be? I don't. Looks like these numbers turn out to be milliseconds, so it's probably clearer to just write 200, 500 and 1000 directly (and ideally rename the constants to end in _MS, or at least put a comment about the unit here).

Change in coreboot[master]: include/spi-generic: moving common flash timeouts

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: moving common flash timeouts
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34345/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34345/3//COMMIT_MSG@7 
PS3, Line 7: moving
Please use imperative tense: 'move'

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h 
File src/include/spi-generic.h:

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h@2... 
PS3, Line 26: #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
> nit: While we're here, do we maybe want to clean these up a little? Does anyone understand what CONF […]

Hz is a unit. Specifically, it is 1/second (the inverse of a second). If the "100" actually means milliseconds, then Hz as unit is completely wrong.

Change in coreboot[master]: include/spi-generic: moving common flash timeouts

[Werner Zeh (Code Review)]

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

Change subject: include/spi-generic: moving common flash timeouts
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h 
File src/include/spi-generic.h:

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h@2... 
PS3, Line 26: #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
> Hz is a unit. Specifically, it is 1/second (the inverse of a second). […]

Yes, we stumbled over this _HZ naming already.
I guess we should change the defines into something like ..._TIMEOUT_MS and drop the define for CONF_SYS_HZ 100 completely. It seems not to be used elsewhere anyway.

Change in coreboot[master]: include/spi-generic: move common flash timeouts

[Uwe Poeche (Code Review)]

Hello Julius Werner, David Hendricks, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: include/spi-generic: move common flash timeouts
......................................................................

include/spi-generic: move common flash timeouts

This patch moves SPI_FLASH time-outs from spi/spi_flash_internal.h for
SPI SW-sequencing to include/spi-generic.h to provide also for
SPI HW-sequencing.

tested on siemens/bdx1 and checked if all includes of
spi_flash_internal.h on other places provide an include of
spi-generic.h before

Change-Id: I837f1a027b836996bc42389bdf7dbab7f0e9db09
Signed-off-by: Uwe Poeche 
---
M src/drivers/spi/spi_flash_internal.h
M src/include/spi-generic.h
2 files changed, 9 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34345/4

Change in coreboot[master]: include/spi-generic: move common flash timeouts

[Uwe Poeche (Code Review)]

Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: move common flash timeouts
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34345/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34345/3//COMMIT_MSG@7 
PS3, Line 7: moving
> Please use imperative tense: 'move'

Done

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h 
File src/include/spi-generic.h:

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h@2... 
PS3, Line 26: #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
> Yes, we stumbled over this _HZ naming already. […]

We push that in an following commit.

Change in coreboot[master]: include/spi-generic: move common flash timeouts

[Uwe Poeche (Code Review)]

Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: move common flash timeouts
......................................................................

Patch Set 5:

(1 comment)

^

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h 
File src/include/spi-generic.h:

https://review.coreboot.org/c/coreboot/+/34345/3/src/include/spi-generic.h@2... 
PS3, Line 26: #define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
> We push that in an following commit.

Ack

Change in coreboot[master]: include/spi-generic: move common flash timeouts

[Werner Zeh (Code Review)]

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

Change subject: include/spi-generic: move common flash timeouts
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: include/spi-generic: move common flash timeouts

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34345 )

Change subject: include/spi-generic: move common flash timeouts
......................................................................

include/spi-generic: move common flash timeouts

This patch moves SPI_FLASH time-outs from spi/spi_flash_internal.h for
SPI SW-sequencing to include/spi-generic.h to provide also for
SPI HW-sequencing.

tested on siemens/bdx1 and checked if all includes of
spi_flash_internal.h on other places provide an include of
spi-generic.h before

Change-Id: I837f1a027b836996bc42389bdf7dbab7f0e9db09
Signed-off-by: Uwe Poeche 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34345
Tested-by: build bot (Jenkins) 
Reviewed-by: Werner Zeh 
---
M src/drivers/spi/spi_flash_internal.h
M src/include/spi-generic.h
2 files changed, 9 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Werner Zeh: Looks good to me, approved

diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h
index 4a9e289..95c51a8 100644
--- a/src/drivers/spi/spi_flash_internal.h
+++ b/src/drivers/spi/spi_flash_internal.h
@@ -7,15 +7,6 @@
 #ifndef SPI_FLASH_INTERNAL_H
 #define SPI_FLASH_INTERNAL_H

-/* Common parameters -- kind of high, but they should only occur when there
- * is a problem (and well your system already is broken), so err on the side
- * of caution in case we're dealing with slower SPI buses and/or processors.
- */
-#define CONF_SYS_HZ 100
-#define SPI_FLASH_PROG_TIMEOUT		(2 * CONF_SYS_HZ)
-#define SPI_FLASH_PAGE_ERASE_TIMEOUT	(5 * CONF_SYS_HZ)
-#define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
-
 /* Common commands */
 #define CMD_READ_ID			0x9f

diff --git a/src/include/spi-generic.h b/src/include/spi-generic.h
index d0f957f..93704f8 100644
--- a/src/include/spi-generic.h
+++ b/src/include/spi-generic.h
@@ -16,6 +16,15 @@
 #ifndef _SPI_GENERIC_H_
 #define _SPI_GENERIC_H_

+/* Common parameters -- kind of high, but they should only occur when there
+ * is a problem (and well your system already is broken), so err on the side
+ * of caution in case we're dealing with slower SPI buses and/or processors.
+ */
+#define CONF_SYS_HZ 100
+#define SPI_FLASH_PROG_TIMEOUT		(2 * CONF_SYS_HZ)
+#define SPI_FLASH_PAGE_ERASE_TIMEOUT	(5 * CONF_SYS_HZ)
+#define SPI_FLASH_SECTOR_ERASE_TIMEOUT	(10 * CONF_SYS_HZ)
+
 #include <commonlib/region.h>
 #include <stdint.h>
 #include <stddef.h>