Lean Sheng Tan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
soc/intel/elkhartlake: Update Kconfig
Update Kconfig: 1. use FSP2.1 instead of 2.2 2. remove HECI_DISABLE_USING_SMM config 3. update CAR related stack & ram size 4. update FSP heap size 5. set IED region size = 0 as it is not used 6. update SMM TSEG size 7. update RP & I2c max device #s 8. update UART base address
Signed-off-by: Tan, Lean Sheng lean.sheng.tan@intel.com Change-Id: I6a44d357d71be706f402a6b2a4f2d4e7c0eeb4a9 --- M src/soc/intel/elkhartlake/Kconfig 1 file changed, 11 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45078/1
diff --git a/src/soc/intel/elkhartlake/Kconfig b/src/soc/intel/elkhartlake/Kconfig index 166bda0..b5462ff 100644 --- a/src/soc/intel/elkhartlake/Kconfig +++ b/src/soc/intel/elkhartlake/Kconfig @@ -32,7 +32,7 @@ select PARALLEL_MP select PARALLEL_MP_AP_WORK select MICROCODE_BLOB_UNDISCLOSED - select PLATFORM_USES_FSP2_2 + select PLATFORM_USES_FSP2_1 select FSP_PEIM_TO_PEIM_INTERFACE select REG_SCRIPT select PMC_GLOBAL_RESET_ENABLE_LOCK @@ -61,29 +61,27 @@ select UDELAY_TSC select UDK_202005_BINDING select DISPLAY_FSP_VERSION_INFO - select HECI_DISABLE_USING_SMM
config DCACHE_RAM_BASE default 0xfef00000
config DCACHE_RAM_SIZE - default 0x80000 + default 0xc0000 help The size of the cache-as-ram region required during bootblock and/or romstage.
config DCACHE_BSP_STACK_SIZE hex - default 0x30400 + default 0x30000 help The amount of anticipated stack usage in CAR by bootblock and - other stages. In the case of FSP_USES_CB_STACK default value - will be sum of FSP-M stack requirement(192 KiB) and CB romstage - stack requirement(~1KiB). + other stages. In the case of FSP_USES_CB_STACK default value will be + sum of FSP-M stack requirement (128KiB) and CB romstage stack requirement (~1KiB).
config FSP_TEMP_RAM_SIZE hex - default 0x20000 + default 0x40000 help The amount of anticipated heap usage in CAR by FSP. Refer to Platform FSP integration guide document to know @@ -95,7 +93,7 @@
config IED_REGION_SIZE hex - default 0x400000 + default 0x0
config HEAP_SIZE hex @@ -103,7 +101,7 @@
config MAX_ROOT_PORTS int - default 8 + default 7
config MAX_PCIE_CLOCKS int @@ -111,7 +109,7 @@
config SMM_TSEG_SIZE hex - default 0x800000 + default 0x1000000
config SMM_RESERVED_SIZE hex @@ -145,7 +143,7 @@
config SOC_INTEL_I2C_DEV_MAX int - default 6 + default 8
config SOC_INTEL_UART_DEV_MAX int @@ -153,7 +151,7 @@
config CONSOLE_UART_BASE_ADDRESS hex - default 0xfe032000 + default 0xfe042000 depends on INTEL_LPSS_UART_FOR_CONSOLE
# Clock divider parameters for 115200 baud rate
Hello build bot (Jenkins), Maulik V Vaghela, Mario Scheithauer, Subrata Banik, Aamir Bohra, Werner Zeh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45078
to look at the new patch set (#2).
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
soc/intel/elkhartlake: Update Kconfig
Update Kconfig: 1. use FSP2.1 instead of 2.2 2. remove HECI_DISABLE_USING_SMM config 3. update CAR related stack & ram size 4. update FSP heap size 5. set IED region size = 0 as it is not used 6. update SMM TSEG size 7. update RP & I2c max device #s 8. update UART base address
Signed-off-by: Tan, Lean Sheng lean.sheng.tan@intel.com Change-Id: I6a44d357d71be706f402a6b2a4f2d4e7c0eeb4a9 --- M src/soc/intel/elkhartlake/Kconfig 1 file changed, 11 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45078/2
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 2:
(2 comments)
Do you mind to add MAX_CPUS to the Kconfig as on a different patch (CB:45063) we have missed this define for APL and need to fix it now.
https://review.coreboot.org/c/coreboot/+/45078/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/1//COMMIT_MSG@16 PS1, Line 16: I2c I2C
https://review.coreboot.org/c/coreboot/+/45078/2/src/soc/intel/elkhartlake/K... File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/45078/2/src/soc/intel/elkhartlake/K... PS2, Line 76: default 0x30000 This results in 192 kB while the help text is changed to 128 kB. What is the size that needs to be used here, 192k or 128k?
Hello build bot (Jenkins), Maulik V Vaghela, Mario Scheithauer, Subrata Banik, Aamir Bohra, Werner Zeh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45078
to look at the new patch set (#3).
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
soc/intel/elkhartlake: Update Kconfig
Update Kconfig: 1. use FSP2.1 instead of 2.2 2. remove HECI_DISABLE_USING_SMM config 3. update CAR related stack & ram size 4. update FSP heap size 5. set IED region size = 0 as it is not used 6. update SMM TSEG size 7. update RP & I2C max device #s 8. update UART base address
Signed-off-by: Tan, Lean Sheng lean.sheng.tan@intel.com Change-Id: I6a44d357d71be706f402a6b2a4f2d4e7c0eeb4a9 --- M src/soc/intel/elkhartlake/Kconfig 1 file changed, 15 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45078/3
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 3:
(2 comments)
Patch Set 2:
(2 comments)
Do you mind to add MAX_CPUS to the Kconfig as on a different patch (CB:45063) we have missed this define for APL and need to fix it now.
The original implementation is to put it in mainboard folder to accommodate some platforms like cfl which has different max cpus for different boards (S & H), but i think the right way should put in SOC folder. moving it here per requested :)
https://review.coreboot.org/c/coreboot/+/45078/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/1//COMMIT_MSG@16 PS1, Line 16: I2c
I2C
Done
https://review.coreboot.org/c/coreboot/+/45078/2/src/soc/intel/elkhartlake/K... File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/45078/2/src/soc/intel/elkhartlake/K... PS2, Line 76: default 0x30000
This results in 192 kB while the help text is changed to 128 kB. […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@10 PS4, Line 10: 1. use FSP2.1 instead of 2.2 Why? Isn’t it a newer platform?
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@17 PS4, Line 17: 8. update UART base address I’d prefer one commit for each item.
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@10 PS4, Line 10: 1. use FSP2.1 instead of 2.2
Why? Isn’t it a newer platform?
EHL will still only support FSP2.1 because FSP2.2 comes pretty late during our development stage and EHL doesn't support chrome OS hence there is no need to upgrade it for now.
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@17 PS4, Line 17: 8. update UART base address
I’d prefer one commit for each item.
I could do that but not prefer to as that would create 20+ patches just to update kconfig in mainboard & soc 😄 Hence i added detailed changes in the patches. Of cause those kconfig params that tied to other bigger changes i didn't included in and will submitted in other patches. Does this make sense to you?
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 4:
added more changed details in this patch*
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@10 PS4, Line 10: 1. use FSP2.1 instead of 2.2
EHL will still only support FSP2.1 because FSP2. […]
Please add that information to the commit message.
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@17 PS4, Line 17: 8. update UART base address
I could do that but not prefer to as that would create 20+ patches just to update kconfig in mainboa […]
(Wouldn’t it be eight commits?)
Small commits are perfect for git and preferred in my opinion. You already have the git commit message summaries in the enumaration, and could add a short explanation if needed to each commit.
Small commits are preferred, because reverting and bisecting commits is much easier that way.
But I let others comment, and leave this as resolved.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@11 PS4, Line 11: 2. remove HECI_DISABLE_USING_SMM config Why?
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@17 PS4, Line 17: 8. update UART base address
(Wouldn’t it be eight commits?) […]
I also prefer one commit for each of them so that each other is able to follow ;)
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@11 PS4, Line 11: 2. remove HECI_DISABLE_USING_SMM config
Why?
Unlike chromeOS requirements, EHL does not officially support chromeOS and it supports full HECI functions by default.
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@17 PS4, Line 17: 8. update UART base address
I also prefer one commit for each of them so that each other is able to follow ;)
Thanks for your feedback, i understand your concern, but this is only 1 line change for each of them. So this is kept consistent with all Intel projects.
Don't worry, there are few more changes which includes more than one line change, I am planning to upstream them in individual patch :) Please help to review them later. Thanks!
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45078/4//COMMIT_MSG@17 PS4, Line 17: 8. update UART base address
Thanks for your feedback, i understand your concern, but this is only 1 line change for each of them […]
Since there is for now no mainboard that uses this code it is not buildable yet. I guess in this case there is no such strict demand to have 8 commits just for this change. I would like to get that in so that we can eventually get the mainboard code merged, which in turn will finally enable this code to be build-tested at all.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45078 )
Change subject: soc/intel/elkhartlake: Update Kconfig ......................................................................
soc/intel/elkhartlake: Update Kconfig
Update Kconfig: 1. use FSP2.1 instead of 2.2 2. remove HECI_DISABLE_USING_SMM config 3. update CAR related stack & ram size 4. update FSP heap size 5. set IED region size = 0 as it is not used 6. update SMM TSEG size 7. update RP & I2C max device #s 8. update UART base address
Signed-off-by: Tan, Lean Sheng lean.sheng.tan@intel.com Change-Id: I6a44d357d71be706f402a6b2a4f2d4e7c0eeb4a9 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45078 Reviewed-by: Werner Zeh werner.zeh@siemens.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/elkhartlake/Kconfig 1 file changed, 15 insertions(+), 13 deletions(-)
Approvals: build bot (Jenkins): Verified Werner Zeh: Looks good to me, approved
diff --git a/src/soc/intel/elkhartlake/Kconfig b/src/soc/intel/elkhartlake/Kconfig index 89da0be..3031769 100644 --- a/src/soc/intel/elkhartlake/Kconfig +++ b/src/soc/intel/elkhartlake/Kconfig @@ -30,7 +30,7 @@ select PARALLEL_MP select PARALLEL_MP_AP_WORK select MICROCODE_BLOB_UNDISCLOSED - select PLATFORM_USES_FSP2_2 + select PLATFORM_USES_FSP2_1 select FSP_PEIM_TO_PEIM_INTERFACE select REG_SCRIPT select PMC_GLOBAL_RESET_ENABLE_LOCK @@ -61,29 +61,31 @@ select UDELAY_TSC select UDK_202005_BINDING select DISPLAY_FSP_VERSION_INFO - select HECI_DISABLE_USING_SMM + +config MAX_CPUS + int + default 4
config DCACHE_RAM_BASE default 0xfef00000
config DCACHE_RAM_SIZE - default 0x80000 + default 0xc0000 help The size of the cache-as-ram region required during bootblock and/or romstage.
config DCACHE_BSP_STACK_SIZE hex - default 0x30400 + default 0x30000 help The amount of anticipated stack usage in CAR by bootblock and - other stages. In the case of FSP_USES_CB_STACK default value - will be sum of FSP-M stack requirement(192 KiB) and CB romstage - stack requirement(~1KiB). + other stages. In the case of FSP_USES_CB_STACK default value will be + sum of FSP-M stack requirement (192KiB) and CB romstage stack requirement (~1KiB).
config FSP_TEMP_RAM_SIZE hex - default 0x20000 + default 0x40000 help The amount of anticipated heap usage in CAR by FSP. Refer to Platform FSP integration guide document to know @@ -95,7 +97,7 @@
config IED_REGION_SIZE hex - default 0x400000 + default 0x0
config HEAP_SIZE hex @@ -103,7 +105,7 @@
config MAX_ROOT_PORTS int - default 8 + default 7
config MAX_PCIE_CLOCKS int @@ -111,7 +113,7 @@
config SMM_TSEG_SIZE hex - default 0x800000 + default 0x1000000
config SMM_RESERVED_SIZE hex @@ -148,7 +150,7 @@
config SOC_INTEL_I2C_DEV_MAX int - default 6 + default 8
config SOC_INTEL_UART_DEV_MAX int @@ -156,7 +158,7 @@
config CONSOLE_UART_BASE_ADDRESS hex - default 0xfe032000 + default 0xfe042000 depends on INTEL_LPSS_UART_FOR_CONSOLE
# Clock divider parameters for 115200 baud rate
-
Felix Singer (Code Review) -
Lean Sheng Tan (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Werner Zeh (Code Review)