Change in coreboot[master]: soc/intel/common/block/gspi: Recalculate BAR after resource allocation

[Jes Klinke (Code Review)]

Hello Jes Klinke,

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

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

to review the following change.

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

soc/intel/common/block/gspi: Recalculate BAR after resource allocation

The base address of the memory mapped I/O registers should not
be cached across resource allocation.  This CL will evict the cached
value upon exiting the BS_DEV_RESOURCES stage.

Change-Id: I81f2b5bfadbf1aaa3b38cca2bcc44ce521666821
Signed-off-by: [e-mail]
---
M src/soc/intel/common/block/gspi/gspi.c
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/44084/1

diff --git a/src/soc/intel/common/block/gspi/gspi.c b/src/soc/intel/common/block/gspi/gspi.c
index 599ab7e..6337226 100644
--- a/src/soc/intel/common/block/gspi/gspi.c
+++ b/src/soc/intel/common/block/gspi/gspi.c
@@ -2,6 +2,7 @@

#include <device/mmio.h>
 #include <assert.h>
+#include <bootstate.h>
 #include <console/console.h>
 #include <delay.h>
 #include <device/device.h>
@@ -257,6 +258,12 @@
    return gspi_base[gspi_bus];
 }

+static void gspi_clear_base_stash(void *unused)
+{
+	memset(gspi_base, 0, sizeof(gspi_base));
+}
+BOOT_STATE_INIT_ENTRY(BS_DEV_RESOURCES, BS_ON_EXIT, gspi_clear_base_stash, NULL);
+
 /* Parameters for GSPI controller operation. */
 struct gspi_ctrlr_params {
    uintptr_t mmio_base;

[Jes Klinke (Code Review)]

Hello Jes Klinke, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

soc/intel/common/block/gspi: Recalculate BAR after resource allocation

The base address of the memory mapped I/O registers should not
be cached across resource allocation.  This CL will evict the cached
value upon exiting the BS_DEV_RESOURCES stage.

See https://review.coreboot.org/c/coreboot/+/43741 for context.

Change-Id: I81f2b5bfadbf1aaa3b38cca2bcc44ce521666821
Signed-off-by: [e-mail]
---
M src/soc/intel/common/block/gspi/gspi.c
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/44084/2

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44084/1//COMMIT_MSG@10 
PS1, Line 10: cached
Why it being cached?

[Jes Klinke (Code Review)]

Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44084 )

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44084/1//COMMIT_MSG@10 
PS1, Line 10: cached
> Why it being cached?

I am not familiar with this code.  I see that the logic in gspi_calc_base_addr() is non-trivial (it is the return value from this method that is being cached), but I do not know how expensive it is to evaluate in practice.

If we remove the caching and call gspi_calc_base_addr() directly in gspi_ctrlr_params_init(), then every CS assert/deassert, and every SPI transaction, will result in a new call to gspi_calc_base_addr().  I assume that the reason the caching was added was that doing without it was found to be too expensive.

Furquan, maybe you have additional insight.

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/44084/1//COMMIT_MSG@10 
PS1, Line 10: cached
> I am not familiar with this code. […]

Ack

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44084/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/gspi/gspi.c:

https://review.coreboot.org/c/coreboot/+/44084/2/src/soc/intel/common/block/... 
PS2, Line 260: 
I think it would be very helpful to have a comment here explaining why the stashed values are being cleared.

[Jes Klinke (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Jes Klinke, Angel Pons, Julius Werner, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

soc/intel/common/block/gspi: Recalculate BAR after resource allocation

The base address of the memory mapped I/O registers should not
be cached across resource allocation.  This CL will evict the cached
value upon exiting the BS_DEV_RESOURCES stage.

Change-Id: I81f2b5bfadbf1aaa3b38cca2bcc44ce521666821
Signed-off-by: [e-mail]
---
M src/soc/intel/common/block/gspi/gspi.c
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/44084/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44084 )

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44084/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/gspi/gspi.c:

https://review.coreboot.org/c/coreboot/+/44084/3/src/soc/intel/common/block/... 
PS3, Line 262:  * PCI ressource allocation will likely change the base address of the mapped
'ressource' may be misspelled - perhaps 'resource'?

[Jes Klinke (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Jes Klinke, Angel Pons, Julius Werner, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

soc/intel/common/block/gspi: Recalculate BAR after resource allocation

The base address of the memory mapped I/O registers should not
be cached across resource allocation.  This CL will evict the cached
value upon exiting the BS_DEV_RESOURCES stage.

Change-Id: I81f2b5bfadbf1aaa3b38cca2bcc44ce521666821
Signed-off-by: [e-mail]
---
M src/soc/intel/common/block/gspi/gspi.c
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/44084/4

[Jes Klinke (Code Review)]

Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44084 )

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 4:

(1 comment)

Added comment, please take another look.

https://review.coreboot.org/c/coreboot/+/44084/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/gspi/gspi.c:

https://review.coreboot.org/c/coreboot/+/44084/2/src/soc/intel/common/block/... 
PS2, Line 260:
> I think it would be very helpful to have a comment here explaining why the stashed values are being  […]

Done

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44084 )

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 5: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 5: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 5: Code-Review+2

[Jes Klinke (Code Review)]

Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44084 )

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 5:

Pardon my ignorance of Coreboot Gerrit.  Do I need to take action to have this patch merged?  It has Verified +1 by the build bot, and several Code-Review +2 already.  Do I need to click a "Submit" button and/or give Code-Review +2 myself, in order for it to move forward?

[Julius Werner (Code Review)]

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

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

Patch Set 5:
> Pardon my ignorance of Coreboot Gerrit.  Do I need to take action to have this patch merged?  It has Verified +1 by the build bot, and several Code-Review +2 already.  Do I need to click a "Submit" button and/or give Code-Review +2 myself, in order for it to move forward?

You need to make sure it has at least one +2, the Verified+1, and the All-Comments-Resolved thumbs-up. Once you have that (which you do for this patch), Patrick will usually come by in a day or two to push it in. (If you're in a hurry you can ask someone else with Submit rights... I can do this one for you.)

[Julius Werner (Code Review)]

Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44084 )

Change subject: soc/intel/common/block/gspi: Recalculate BAR after resource allocation
......................................................................

soc/intel/common/block/gspi: Recalculate BAR after resource allocation

The base address of the memory mapped I/O registers should not
be cached across resource allocation.  This CL will evict the cached
value upon exiting the BS_DEV_RESOURCES stage.

Change-Id: I81f2b5bfadbf1aaa3b38cca2bcc44ce521666821
Signed-off-by: [e-mail]
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44084
Reviewed-by: Karthik Ramasubramanian 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/common/block/gspi/gspi.c
1 file changed, 12 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Angel Pons: Looks good to me, approved
  Karthik Ramasubramanian: Looks good to me, approved

diff --git a/src/soc/intel/common/block/gspi/gspi.c b/src/soc/intel/common/block/gspi/gspi.c
index 599ab7e..8363713 100644
--- a/src/soc/intel/common/block/gspi/gspi.c
+++ b/src/soc/intel/common/block/gspi/gspi.c
@@ -2,6 +2,7 @@

#include <device/mmio.h>
 #include <assert.h>
+#include <bootstate.h>
 #include <console/console.h>
 #include <delay.h>
 #include <device/device.h>
@@ -257,6 +258,17 @@
    return gspi_base[gspi_bus];
 }

+/*
+ * PCI resource allocation will likely change the base address of the mapped
+ * I/O registers.  Clearing the cached value after the allocation step will
+ * cause it to be recomputed by gspi_calc_base_addr() on next access.
+ */
+static void gspi_clear_cached_base(void *unused)
+{
+	memset(gspi_base, 0, sizeof(gspi_base));
+}
+BOOT_STATE_INIT_ENTRY(BS_DEV_RESOURCES, BS_ON_EXIT, gspi_clear_cached_base, NULL);
+
 /* Parameters for GSPI controller operation. */
 struct gspi_ctrlr_params {
    uintptr_t mmio_base;