Change in coreboot[master]: soc/intel/common: Set controller state to active in GSPI init
Meera Ravindranath has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence, this ensures the controller is up and active.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/1
diff --git a/src/soc/intel/common/block/gspi/gspi.c b/src/soc/intel/common/block/gspi/gspi.c index 17532bf..8bcefe2 100644 --- a/src/soc/intel/common/block/gspi/gspi.c +++ b/src/soc/intel/common/block/gspi/gspi.c @@ -24,6 +24,7 @@ #include <intelblocks/chip.h> #include <intelblocks/gspi.h> #include <intelblocks/spi.h> +#include <intelblocks/lpss.h> #include <soc/iomap.h> #include <soc/pci_devs.h> #include <string.h> @@ -151,6 +152,7 @@ const struct gspi_cfg *cfg = gspi_get_cfg(); int devfn; uintptr_t gspi_base_addr; + const struct device *tree_dev;
assert(gspi_max != 0); if (!cfg) { @@ -172,6 +174,9 @@ devfn = gspi_soc_bus_to_devfn(gspi_bus); gspi_set_base_addr(devfn, NULL, GSPI_BUS_BASE(gspi_base_addr, gspi_bus)); + tree_dev = pcidev_path_on_root(devfn); + /* Ensure controller is in D0 state */ + lpss_set_power_state(tree_dev, STATE_D0); } }
@@ -449,6 +454,9 @@ uint32_t cs_ctrl, sscr0, sscr1, clocks, sitf, sirf, pol; struct gspi_ctrlr_params params, *p = ¶ms;
+ /* Ensure controller is in D0 state */ + lpss_set_power_state((struct device*) dev, STATE_D0); + /* Only chip select 0 is supported. */ if (dev->cs != 0) { printk(BIOS_ERR, "%s: Invalid CS value: cs=%u.\n", __func__,
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 458: lpss_set_power_state((struct device*) dev, STATE_D0); "(foo*)" should be "(foo *)"
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0) we would not need it here. call during the ctrl setup is just fine.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: tab
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 458: lpss_set_power_state((struct device*) dev, STATE_D0);
"(foo*)" should be "(foo *)"
this is wrong. struct device and struct spi_slave is not the same structure hence typecast won't work here
Rather you could so something like below
int devfn = gspi_soc_bus_to_devfn(dev->bus); struct device *device = pcidev_path_on_root(devfn); lpss_set_power_state(device, STATE_D0);
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0)
we would not need it here. call during the ctrl setup is just fine.
gspi_early_bar_init() is getting called from bootblock/pch.c right ? so this is equivalent to lpss_i2c_early_init_bus() what you have called
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179:
tab
yes,here we are just setting the BAR and not doing a controller init/setup.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179:
yes,here we are just setting the BAR and not doing a controller init/setup.
true but point is that if ctrl itself in D3 then what we will achieve by reading BAR and programming that BAR ? Shouldn't we ensure to make controller into D0 and set BAR.
i believe this early BAR programming is required for TPM. We might see TPM communication error if ctrl remains in D3 for *any* reason (although i don't see that reason so far)
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179:
true but point is that if ctrl itself in D3 then what we will achieve by reading BAR and programming […]
the GSPI ctrl setup is called during TPM Init, setting D0 in that call would ensure it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179:
the GSPI ctrl setup is called during TPM Init, setting D0 in that call would ensure it.
yes, make sense, need to remove this code from here. setup will help to fix this problem
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34449/1//COMMIT_MSG@9 PS1, Line 9: Set the controller state to D0 during the GSPI sequence, : this ensures the controller is up and active. Please use the full text width.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34449
to look at the new patch set (#2).
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence, this ensures the controller is up and active.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 2: Code-Review+1
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179:
yes, make sense, need to remove this code from here. […]
Done
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 458: lpss_set_power_state((struct device*) dev, STATE_D0);
this is wrong. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/2/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/2/src/soc/intel/common/block/... PS2, Line 455: devfn = gspi_soc_bus_to_devfn(dev->bus); : device = pcidev_path_on_root(devfn); : : /* Ensure controller is in D0 state */ : lpss_set_power_state(device, STATE_D0); Shouldn't this be done after all the initial checks are done i.e. in line 477?
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34449/2//COMMIT_MSG@11 PS2, Line 11: Can you add BUG,BRANCH,TEST details?
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/2/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/2/src/soc/intel/common/block/... PS2, Line 455: devfn = gspi_soc_bus_to_devfn(dev->bus); : device = pcidev_path_on_root(devfn); : : /* Ensure controller is in D0 state */ : lpss_set_power_state(device, STATE_D0);
Shouldn't this be done after all the initial checks are done i.e. […]
Thank you. Yes, I am considering doing it after the reset i.e line 480. Would that be okay?
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34449
to look at the new patch set (#3).
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence,this ensures the controller is up and active.
BUG=b:135941367 TEST=Verify no timeouts seen during GSPI controller enumeration sequence for CML and ICL platforms.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/3
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 3:
(1 comment)
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34449/2//COMMIT_MSG@11 PS2, Line 11:
Can you add BUG,BRANCH,TEST details?
Done
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34449/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34449/1//COMMIT_MSG@9 PS1, Line 9: Set the controller state to D0 during the GSPI sequence, : this ensures the controller is up and active.
Please use the full text width.
Done
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0);
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0)
gspi_early_bar_init() is getting called from bootblock/pch. […]
Done
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179:
Done
Done
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 179: lpss_set_power_state(tree_dev, STATE_D0);
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/34449/1/src/soc/intel/common/block/... PS1, Line 458: lpss_set_power_state((struct device*) dev, STATE_D0);
Done
Done
https://review.coreboot.org/c/coreboot/+/34449/2/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/2/src/soc/intel/common/block/... PS2, Line 455: devfn = gspi_soc_bus_to_devfn(dev->bus); : device = pcidev_path_on_root(devfn); : : /* Ensure controller is in D0 state */ : lpss_set_power_state(device, STATE_D0);
Thank you. Yes, I am considering doing it after the reset i.e line 480. […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 3: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 479: lpss_set_power_state(device, STATE_D0); It will have to be done before any MMIO addresses are accessed i.e. before gspi_write_mmio_reg above.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 3: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 474: do it here lpss_set_power_state(device, STATE_D0);
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 450: int unsigned int or better uint32_t to be consistent with rest of the definition in the file.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34449
to look at the new patch set (#4).
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence,this ensures the controller is up and active.
BUG=b:135941367 TEST=Verify no timeouts seen during GSPI controller enumeration sequence for CML and ICL platforms.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/4
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/4/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/4/src/soc/intel/common/block/... PS4, Line 25: #include <intelblocks/gspi.h> : #include <intelblocks/spi.h> : #include <intelblocks/lpss.h> nit: can we align order
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34449
to look at the new patch set (#5).
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence,this ensures the controller is up and active.
BUG=b:135941367 TEST=Verify no timeouts seen during GSPI controller enumeration sequence for CML and ICL platforms.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... PS5, Line 450: uint32_t should be int, look at line 153, 173. in this file gspi_soc_bus_to_devfn() return value stored into "int" devfn and its logical as well
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... PS5, Line 450: uint32_t
should be int, look at line 153, 173. […]
Yes, right. @Aamir,can i go ahead with int?
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 474:
do it here […]
Done
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 479: lpss_set_power_state(device, STATE_D0);
It will have to be done before any MMIO addresses are accessed i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/34449/4/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/4/src/soc/intel/common/block/... PS4, Line 25: #include <intelblocks/gspi.h> : #include <intelblocks/spi.h> : #include <intelblocks/lpss.h>
nit: can we align order
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... PS5, Line 450: uint32_t
Yes, right. […]
Yes, please, I was wrong, the function does return signed int. The reason I asked for change was the devfn would be always unsigned for a valid gspi bus. which would be the case for all ctrl setup call.
May be now also add a check for a error in gspi_soc_to_devfn(if it returns -1) and exit the setup call.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34449
to look at the new patch set (#6).
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence,this ensures the controller is up and active.
BUG=b:135941367 TEST=Verify no timeouts seen during GSPI controller enumeration sequence for CML and ICL platforms.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 460: } code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 460: } please, no spaces at the start of a line
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 456: devfn < 0 !devfn
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 460: tab ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 456: devfn < 0
!devfn
looks like it can only return -1 in case of failure hence better to check with == -1 only. although i don't see other caller of gspi_soc_bus_to_devfn() bother about invalid device check.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 456: devfn < 0
looks like it can only return -1 in case of failure hence better to check with == -1 only. […]
IMO, the devfn < 0 would handle that unique error case. for other call, it explicitly runs a loop for 0 to max gspi device, so the bus number would always turn out to be valid.
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 456: devfn < 0
IMO, the devfn < 0 would handle that unique error case. […]
Yes. Line 236 also shows check for devfn < 0.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34449
to look at the new patch set (#7).
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence,this ensures the controller is up and active.
BUG=b:135941367 TEST=Verify no timeouts seen during GSPI controller enumeration sequence for CML and ICL platforms.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/34449/7
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 7: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 7: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 7: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 456: devfn < 0
Yes. Line 236 also shows check for devfn < 0.
Done
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 460: }
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 460:
tab ?
Done
https://review.coreboot.org/c/coreboot/+/34449/6/src/soc/intel/common/block/... PS6, Line 460: }
please, no spaces at the start of a line
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 450: int
unsigned int or better uint32_t to be consistent with rest of the definition in the file.
Done
https://review.coreboot.org/c/coreboot/+/34449/3/src/soc/intel/common/block/... PS3, Line 479: lpss_set_power_state(device, STATE_D0);
Done
Done
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/5/src/soc/intel/common/block/... PS5, Line 450: uint32_t
Yes, please, I was wrong, the function does return signed int. […]
Done
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
soc/intel/common: Set controller state to active in GSPI init
Set the controller state to D0 during the GSPI sequence,this ensures the controller is up and active.
BUG=b:135941367 TEST=Verify no timeouts seen during GSPI controller enumeration sequence for CML and ICL platforms.
Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Change-Id: I2f95059453ca5565a38650b147590ece4d8bf5ed Reviewed-on: https://review.coreboot.org/c/coreboot/+/34449 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Subrata Banik subrata.banik@intel.com Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/common/block/gspi/gspi.c 1 file changed, 15 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Subrata Banik: Looks good to me, approved Aamir Bohra: 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 17532bf..beb12fb 100644 --- a/src/soc/intel/common/block/gspi/gspi.c +++ b/src/soc/intel/common/block/gspi/gspi.c @@ -23,6 +23,7 @@ #include <device/pci_ops.h> #include <intelblocks/chip.h> #include <intelblocks/gspi.h> +#include <intelblocks/lpss.h> #include <intelblocks/spi.h> #include <soc/iomap.h> #include <soc/pci_devs.h> @@ -446,8 +447,19 @@ static int gspi_ctrlr_setup(const struct spi_slave *dev) { struct spi_cfg cfg; + int devfn; uint32_t cs_ctrl, sscr0, sscr1, clocks, sitf, sirf, pol; struct gspi_ctrlr_params params, *p = ¶ms; + const struct device *device; + + devfn = gspi_soc_bus_to_devfn(dev->bus); + if (devfn < 0) { + printk(BIOS_ERR, "%s: No GSPI controller found on SPI bus %u.\n", + __func__, dev->bus); + return -1; + } + + device = pcidev_path_on_root(devfn);
/* Only chip select 0 is supported. */ if (dev->cs != 0) { @@ -466,6 +478,9 @@ return -1; }
+ /* Ensure controller is in D0 state */ + lpss_set_power_state(device, STATE_D0); + /* Take controller out of reset, keeping DMA in reset. */ gspi_write_mmio_reg(p, RESETS, CTRLR_ACTIVE | DMA_RESET);
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/8/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/8/src/soc/intel/common/block/... PS8, Line 462: device = pcidev_path_on_root(devfn); The validity of device needs to be checked, because this GSPI device is not enabled on all SoCs. Actually I am observing a crash due to device being NULL in APL/GLK i.e. the device is passed to lpss_set_power_state which hits pcidev_assert and dies because device is NULL.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34449/8/src/soc/intel/common/block/... File src/soc/intel/common/block/gspi/gspi.c:
https://review.coreboot.org/c/coreboot/+/34449/8/src/soc/intel/common/block/... PS8, Line 455: dev->bus This is not correct. You need to map dev->bus to gspi_bus. This will have to be something like this:
diff --git a/src/soc/intel/common/block/gspi/gspi.c b/src/soc/intel/common/block/gspi/gspi.c index beb12fb231..592071e8ca 100644 --- a/src/soc/intel/common/block/gspi/gspi.c +++ b/src/soc/intel/common/block/gspi/gspi.c @@ -452,15 +452,6 @@ static int gspi_ctrlr_setup(const struct spi_slave *dev) struct gspi_ctrlr_params params, *p = ¶ms; const struct device *device;
- devfn = gspi_soc_bus_to_devfn(dev->bus); - if (devfn < 0) { - printk(BIOS_ERR, "%s: No GSPI controller found on SPI bus %u.\n", - __func__, dev->bus); - return -1; - } - - device = pcidev_path_on_root(devfn); - /* Only chip select 0 is supported. */ if (dev->cs != 0) { printk(BIOS_ERR, "%s: Invalid CS value: cs=%u.\n", __func__, @@ -478,6 +469,15 @@ static int gspi_ctrlr_setup(const struct spi_slave *dev) return -1; }
+ devfn = gspi_soc_bus_to_devfn(p->gspi_bus); + if (devfn < 0) { + printk(BIOS_ERR, "%s: No GSPI controller found on SPI bus %u.\n", + __func__, gspi->bus); + return -1; + } + + device = pcidev_path_on_root(devfn); + /* Ensure controller is in D0 state */ lpss_set_power_state(device, STATE_D0)
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 8:
Acknowledge the issue, Will push a patch ASAP.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34449 )
Change subject: soc/intel/common: Set controller state to active in GSPI init ......................................................................
Patch Set 8:
Patch Set 8:
Acknowledge the issue, Will push a patch ASAP.
CL pushed here: https://review.coreboot.org/c/coreboot/+/34761
-
Aamir Bohra (Code Review) -
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Meera Ravindranath (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)