Change in coreboot[master]: soc/intel/icelake: Remove redundent dependency check in cpu.c
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundent dependency check in cpu.c ......................................................................
soc/intel/icelake: Remove redundent dependency check in cpu.c
This patch removes redundent dependency check related to CONFIG_BOOT_DEVICE_SPI_FLASH kconfig inside bootblock/cpu.c.
CONFIG_BOOT_DEVICE_MEMORY_MAPPED is enable if CONFIG_BOOT_DEVICE_SPI_FLASH is selected.
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/36431/1
diff --git a/src/soc/intel/icelake/bootblock/cpu.c b/src/soc/intel/icelake/bootblock/cpu.c index e23286c..4b092e2 100644 --- a/src/soc/intel/icelake/bootblock/cpu.c +++ b/src/soc/intel/icelake/bootblock/cpu.c @@ -20,7 +20,6 @@ void bootblock_cpu_init(void) { /* Temporarily cache the memory-mapped boot media. */ - if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED) && - CONFIG(BOOT_DEVICE_SPI_FLASH)) + if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) fast_spi_cache_bios_region(); }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundent dependency check in cpu.c ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/36431/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36431/1//COMMIT_MSG@7 PS1, Line 7: redundent nit: redundant
https://review.coreboot.org/c/coreboot/+/36431/1//COMMIT_MSG@9 PS1, Line 9: redundent redundant
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundent dependency check in cpu.c ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36431/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36431/1//COMMIT_MSG@7 PS1, Line 7: redundent
nit: redundant
Done
https://review.coreboot.org/c/coreboot/+/36431/1//COMMIT_MSG@9 PS1, Line 9: redundent
redundant
Done
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36431
to look at the new patch set (#2).
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
soc/intel/icelake: Remove redundant dependency check in cpu.c
This patch removes redundant dependency check related to CONFIG_BOOT_DEVICE_SPI_FLASH kconfig inside bootblock/cpu.c.
CONFIG_BOOT_DEVICE_MEMORY_MAPPED is enable if CONFIG_BOOT_DEVICE_SPI_FLASH is selected.
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/36431/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... File src/soc/intel/icelake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... PS2, Line 23: if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED)) I don't think this is correct. The call below *assumes* spi flash, but the check is removed for it. It's very much conceivable (and happened on glk/apl) that first part of the boot was memory mapped by way of CSE even if using emmc device.
Yes, we have defaults in Kconfig that if it's x86 and boot device is spi flash then we say boot device is memory mapped. But it does not follow that if boot device is memory mapped then the boot device's media is spi flash.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
Patch Set 2: -Code-Review
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... File src/soc/intel/icelake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... PS2, Line 23: if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED))
I don't think this is correct. The call below *assumes* spi flash, but the check is removed for it. […]
Agree but as boot from block device is not POR on core platform hence can't we assume that core platforms do have SPI mapped memory access enable (unlike APL/GLK) and this is not common code so we can make the assumption specific for ICL right ?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... File src/soc/intel/icelake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... PS2, Line 23: if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED))
Agree but as boot from block device is not POR on core platform hence can't we assume that core plat […]
If you are going to make that assumption then why have the BOOT_DEVICE_MEMORY_MAPPED check as well? If you are assuming it's always memory mapped and spi flash boot media, then it should be an unconditional call, right?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... File src/soc/intel/icelake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... PS2, Line 23: if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED))
If you are going to make that assumption then why have the BOOT_DEVICE_MEMORY_MAPPED check as well? […]
yes, i was thinking that, shouldn't we make that call implicit of any check as we know its SPI memory mapped for sure ?
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36431
to look at the new patch set (#3).
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
soc/intel/icelake: Remove redundant dependency check in cpu.c
This patch removes redundant dependency check related to CONFIG_BOOT_DEVICE_SPI_FLASH kconfig inside bootblock/cpu.c.
CONFIG_BOOT_DEVICE_MEMORY_MAPPED is enable if CONFIG_BOOT_DEVICE_SPI_FLASH is selected.
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/36431/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Remove redundant dependency check in cpu.c ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... File src/soc/intel/icelake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... PS2, Line 23: if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED))
yes, i was thinking that, shouldn't we make that call implicit of any check as we know its SPI memor […]
I was noting that if you are going to make that argument about what icelake supports, etc, take the argument to the end. Add a comment at least to the assumptions one is making. I personally don't think the Kconfig checks are bad. If someone decides they want to combine things into common code that may be an issue as the same assumptions don't hold, but software can always be edited/changed.
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36431
to look at the new patch set (#4).
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally
Icelake platform doesn't support booting from any other media (like eMMC on APL/GLK platform) than only booting from SPI device and on IA platform SPI is memory mapped hence enabling temporarily cacheing on memory-mapped spi boot media.
This assumption will not hold good for APL/GLK platform where boot from eMMC is also possible options.
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 10 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/36431/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... File src/soc/intel/icelake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/36431/2/src/soc/intel/icelake/bootb... PS2, Line 23: if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED))
I was noting that if you are going to make that argument about what icelake supports, etc, take the […]
Done
Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36431
to look at the new patch set (#5).
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally
Icelake platform doesn't support booting from any other media (like eMMC on APL/GLK platform) than only booting from SPI device and on IA platform SPI is memory mapped hence enabling temporarily cacheing on memory-mapped spi boot media.
This assumption will not hold good for APL/GLK platform where boot from eMMC is also possible options.
Also removed inclusion of unused header in cpu.c file
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 10 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/36431/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
Patch Set 5: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36431/5//COMMIT_MSG@14 PS5, Line 14: This assumption will not hold good for APL/GLK platform where boot : from eMMC is also possible options. This is sufficiently clear for the previous sentences.
Hello Aaron Durbin, Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36431
to look at the new patch set (#6).
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally
Icelake platform doesn't support booting from any other media (like eMMC on APL/GLK platform) than only booting from SPI device and on IA platform SPI is memory mapped hence enabling temporarily cacheing on memory-mapped spi boot media.
Also removed inclusion of unused header in cpu.c file
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 10 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/36431/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36431/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36431/5//COMMIT_MSG@14 PS5, Line 14: This assumption will not hold good for APL/GLK platform where boot : from eMMC is also possible options.
This is sufficiently clear for the previous sentences.
Done
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36431 )
Change subject: soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally ......................................................................
soc/intel/icelake: Enable caching on SPI memory-mapped boot device unconditionally
Icelake platform doesn't support booting from any other media (like eMMC on APL/GLK platform) than only booting from SPI device and on IA platform SPI is memory mapped hence enabling temporarily cacheing on memory-mapped spi boot media.
Also removed inclusion of unused header in cpu.c file
TEST=Able to build and boot ICL DE board.
Change-Id: I46d9ec054c4804ca756f2101085a55e91b5cc6f0 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36431 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/icelake/bootblock/cpu.c 1 file changed, 10 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/icelake/bootblock/cpu.c b/src/soc/intel/icelake/bootblock/cpu.c index e23286c..da7b7ea 100644 --- a/src/soc/intel/icelake/bootblock/cpu.c +++ b/src/soc/intel/icelake/bootblock/cpu.c @@ -13,14 +13,19 @@ * GNU General Public License for more details. */
-#include <intelblocks/cpulib.h> #include <intelblocks/fast_spi.h> #include <soc/bootblock.h>
void bootblock_cpu_init(void) { - /* Temporarily cache the memory-mapped boot media. */ - if (CONFIG(BOOT_DEVICE_MEMORY_MAPPED) && - CONFIG(BOOT_DEVICE_SPI_FLASH)) - fast_spi_cache_bios_region(); + /* + * Icelake platform doesn't support booting from any other media + * (like eMMC on APL/GLK platform) than only booting from SPI device + * and on IA platform SPI is memory mapped hence enabling temporarily + * cacheing on memory-mapped spi boot media. + * + * This assumption will not hold good for APL/GLK platform where boot + * from eMMC is also possible options. + */ + fast_spi_cache_bios_region(); }
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Subrata Banik (Code Review)