Change in ...coreboot[master]: arch/arm: Make ARM specific options depend on select ARCH_ARM
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31313
Change subject: arch/arm: Make ARM specific options depend on select ARCH_ARM ......................................................................
arch/arm: Make ARM specific options depend on select ARCH_ARM
Also don't define the default as this result in spurious lines in the .config.
Change-Id: I1ed4a71599641db606510e5304b9f0acf9b7eb88 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/soc/nvidia/tegra124/Kconfig M src/soc/nvidia/tegra210/Kconfig M src/soc/qualcomm/ipq40xx/Kconfig M src/soc/qualcomm/ipq806x/Kconfig M src/soc/rockchip/rk3288/Kconfig M src/soc/samsung/exynos5250/Kconfig M src/soc/samsung/exynos5420/Kconfig 8 files changed, 11 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/31313/1
diff --git a/src/arch/arm/Kconfig b/src/arch/arm/Kconfig index 6d9cc78..14d054a 100644 --- a/src/arch/arm/Kconfig +++ b/src/arch/arm/Kconfig @@ -1,11 +1,11 @@ config ARCH_ARM bool - default n + +if ARCH_ARM
config ARCH_BOOTBLOCK_ARM bool default n - select ARCH_ARM select C_ENVIRONMENT_BOOTBLOCK
config ARCH_VERSTAGE_ARM @@ -26,3 +26,5 @@ config ARM_LPAE bool default n + +endif #if ARCH_ARM diff --git a/src/soc/nvidia/tegra124/Kconfig b/src/soc/nvidia/tegra124/Kconfig index eecb59b..e9e43a5 100644 --- a/src/soc/nvidia/tegra124/Kconfig +++ b/src/soc/nvidia/tegra124/Kconfig @@ -1,6 +1,7 @@ config SOC_NVIDIA_TEGRA124 bool default n + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV4 select BOOTBLOCK_CUSTOM select ARCH_VERSTAGE_ARMV7 diff --git a/src/soc/nvidia/tegra210/Kconfig b/src/soc/nvidia/tegra210/Kconfig index 8883baa..52118c3 100644 --- a/src/soc/nvidia/tegra210/Kconfig +++ b/src/soc/nvidia/tegra210/Kconfig @@ -1,6 +1,7 @@ config SOC_NVIDIA_TEGRA210 bool default n + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV4 select BOOTBLOCK_CUSTOM select ARCH_VERSTAGE_ARMV4 diff --git a/src/soc/qualcomm/ipq40xx/Kconfig b/src/soc/qualcomm/ipq40xx/Kconfig index 72e05fa..7a0648c 100644 --- a/src/soc/qualcomm/ipq40xx/Kconfig +++ b/src/soc/qualcomm/ipq40xx/Kconfig @@ -1,6 +1,7 @@ config SOC_QC_IPQ40XX bool default n + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV7 select ARCH_VERSTAGE_ARMV7 select ARCH_ROMSTAGE_ARMV7 diff --git a/src/soc/qualcomm/ipq806x/Kconfig b/src/soc/qualcomm/ipq806x/Kconfig index 0b112d9..3bea529 100644 --- a/src/soc/qualcomm/ipq806x/Kconfig +++ b/src/soc/qualcomm/ipq806x/Kconfig @@ -1,6 +1,7 @@ config SOC_QC_IPQ806X bool default n + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV7 select ARCH_VERSTAGE_ARMV7 select ARCH_ROMSTAGE_ARMV7 diff --git a/src/soc/rockchip/rk3288/Kconfig b/src/soc/rockchip/rk3288/Kconfig index 53666c2..654374c 100644 --- a/src/soc/rockchip/rk3288/Kconfig +++ b/src/soc/rockchip/rk3288/Kconfig @@ -16,6 +16,7 @@ config SOC_ROCKCHIP_RK3288 bool default n + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV7 select ARCH_VERSTAGE_ARMV7 select ARCH_ROMSTAGE_ARMV7 diff --git a/src/soc/samsung/exynos5250/Kconfig b/src/soc/samsung/exynos5250/Kconfig index 5b39724..149d703 100644 --- a/src/soc/samsung/exynos5250/Kconfig +++ b/src/soc/samsung/exynos5250/Kconfig @@ -1,4 +1,5 @@ config CPU_SAMSUNG_EXYNOS5250 + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV7 select ARCH_VERSTAGE_ARMV7 select ARCH_ROMSTAGE_ARMV7 diff --git a/src/soc/samsung/exynos5420/Kconfig b/src/soc/samsung/exynos5420/Kconfig index 7b87650..4ffbf7f 100644 --- a/src/soc/samsung/exynos5420/Kconfig +++ b/src/soc/samsung/exynos5420/Kconfig @@ -1,4 +1,5 @@ config CPU_SAMSUNG_EXYNOS5420 + select ARCH_ARM select ARCH_BOOTBLOCK_ARMV7 select ARCH_VERSTAGE_ARMV7 select ARCH_ROMSTAGE_ARMV7
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31313
to look at the new patch set (#2).
Change subject: arch/arm: Make ARM specific options depend on select ARCH_ARM ......................................................................
arch/arm: Make ARM specific options depend on select ARCH_ARM
Also don't define the default as this result in spurious lines in the .config.
Change-Id: I1ed4a71599641db606510e5304b9f0acf9b7eb88 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/cpu/allwinner/a10/Kconfig M src/cpu/armltd/cortex-a9/Kconfig M src/cpu/ti/am335x/Kconfig M src/soc/nvidia/tegra124/Kconfig M src/soc/nvidia/tegra210/Kconfig M src/soc/qualcomm/ipq40xx/Kconfig M src/soc/qualcomm/ipq806x/Kconfig M src/soc/rockchip/rk3288/Kconfig M src/soc/samsung/exynos5250/Kconfig M src/soc/samsung/exynos5420/Kconfig 11 files changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/31313/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM specific options depend on select ARCH_ARM ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/31313/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31313/2//COMMIT_MSG@7 PS2, Line 7: select selected? (Otherwise it’s ambiguous.)
Hello Julius Werner, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31313
to look at the new patch set (#4).
Change subject: arch/arm: Make ARM specific options depend on ARCH_ARM ......................................................................
arch/arm: Make ARM specific options depend on ARCH_ARM
Also don't define the default as this result in spurious lines in the .config.
TEST: The generated config.h remains the same for all boards.
Change-Id: I1ed4a71599641db606510e5304b9f0acf9b7eb88 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/cpu/allwinner/a10/Kconfig M src/cpu/armltd/cortex-a9/Kconfig M src/cpu/ti/am335x/Kconfig M src/soc/nvidia/tegra124/Kconfig M src/soc/nvidia/tegra210/Kconfig M src/soc/qualcomm/ipq40xx/Kconfig M src/soc/qualcomm/ipq806x/Kconfig M src/soc/rockchip/rk3288/Kconfig M src/soc/samsung/exynos5250/Kconfig M src/soc/samsung/exynos5420/Kconfig 11 files changed, 14 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/31313/4
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM specific options depend on ARCH_ARM ......................................................................
Patch Set 4: Code-Review+1
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM specific options depend on ARCH_ARM ......................................................................
Patch Set 4: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM specific options depend on ARCH_ARM ......................................................................
Patch Set 4: Code-Review-1
Same comment as for CB:31315
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM specific options depend on ARCH_ARM ......................................................................
Patch Set 4: -Code-Review
I've already merged a couple of these patches, but I'd like to get Arthur's response to Julius's comment.
Hello HAOUAS Elyes, Julius Werner, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31313
to look at the new patch set (#5).
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
arch/arm: Make ARM stages select ARCH_ARM
This removes the need to select ARCH_ARM in SOC Kconfig
Also don't define the default as this result in spurious lines in the .config.
Change-Id: I1ed4a71599641db606510e5304b9f0acf9b7eb88 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/arm/Kconfig M src/arch/arm/armv4/Kconfig M src/arch/arm/armv7/Kconfig M src/mainboard/google/nyan_blaze/Kconfig 4 files changed, 19 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/31313/5
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
Patch Set 6: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
This no longer does the thing the original patch tried to do? But LGTM anyway.
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig File src/arch/arm/armv4/Kconfig:
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig@... PS6, Line 2: bool This is functionally equivalent, right? Any reason we're changing it? Personally, I find "def_bool n" more descriptive that having to know off-hand that the default default value is n.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig File src/arch/arm/armv4/Kconfig:
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig@... PS6, Line 2: bool
This is functionally equivalent, right? Any reason we're changing it? Personally, I find "def_bool n […]
The only thing I can think of is that if this comes before another location that wants to set the default, they can't be changed if a default is specified here. These Kconfig files should be loaded practically last though, so that shouldn't be the case.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig File src/arch/arm/armv4/Kconfig:
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig@... PS6, Line 2: bool After writing that comment I noticed this in the commit message which is probably the justification for this:
Also don't define the default as this result in spurious lines in the
.config.
Although I'm still not sure what the problem is, I was hoping Arthur could elaborate a little. But if there's a real problem with def_bool, I'm fine writing it this way instead.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/31313/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31313/2//COMMIT_MSG@7 PS2, Line 7: select
selected? (Otherwise it’s ambiguous. […]
Ack
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig File src/arch/arm/armv4/Kconfig:
https://review.coreboot.org/c/coreboot/+/31313/6/src/arch/arm/armv4/Kconfig@... PS6, Line 2: bool
After writing that comment I noticed this in the commit message which is probably the justification […]
Done
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31313 )
Change subject: arch/arm: Make ARM stages select ARCH_ARM ......................................................................
arch/arm: Make ARM stages select ARCH_ARM
This removes the need to select ARCH_ARM in SOC Kconfig
Also don't define the default as this result in spurious lines in the .config.
Change-Id: I1ed4a71599641db606510e5304b9f0acf9b7eb88 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/31313 Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/arm/Kconfig M src/arch/arm/armv4/Kconfig M src/arch/arm/armv7/Kconfig M src/mainboard/google/nyan_blaze/Kconfig 4 files changed, 19 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, but someone else must approve Julius Werner: Looks good to me, approved
diff --git a/src/arch/arm/Kconfig b/src/arch/arm/Kconfig index 6d9cc78..47c333b 100644 --- a/src/arch/arm/Kconfig +++ b/src/arch/arm/Kconfig @@ -1,24 +1,22 @@ config ARCH_ARM bool - default n
config ARCH_BOOTBLOCK_ARM bool - default n select ARCH_ARM select C_ENVIRONMENT_BOOTBLOCK
config ARCH_VERSTAGE_ARM bool - default n + select ARCH_ARM
config ARCH_ROMSTAGE_ARM bool - default n + select ARCH_ARM
config ARCH_RAMSTAGE_ARM bool - default n + select ARCH_ARM
source src/arch/arm/armv4/Kconfig source src/arch/arm/armv7/Kconfig diff --git a/src/arch/arm/armv4/Kconfig b/src/arch/arm/armv4/Kconfig index aa5fab9..0b50165 100644 --- a/src/arch/arm/armv4/Kconfig +++ b/src/arch/arm/armv4/Kconfig @@ -1,15 +1,15 @@ config ARCH_BOOTBLOCK_ARMV4 - def_bool n + bool select ARCH_BOOTBLOCK_ARM
config ARCH_VERSTAGE_ARMV4 - def_bool n + bool select ARCH_VERSTAGE_ARM
config ARCH_ROMSTAGE_ARMV4 - def_bool n + bool select ARCH_ROMSTAGE_ARM
config ARCH_RAMSTAGE_ARMV4 - def_bool n + bool select ARCH_RAMSTAGE_ARM diff --git a/src/arch/arm/armv7/Kconfig b/src/arch/arm/armv7/Kconfig index 3734426..6d5fb9e 100644 --- a/src/arch/arm/armv7/Kconfig +++ b/src/arch/arm/armv7/Kconfig @@ -1,37 +1,39 @@ config ARCH_BOOTBLOCK_ARMV7 - def_bool n + bool select ARCH_BOOTBLOCK_ARM
config ARCH_VERSTAGE_ARMV7 - def_bool n + bool select ARCH_VERSTAGE_ARM
config ARCH_ROMSTAGE_ARMV7 - def_bool n + bool select ARCH_ROMSTAGE_ARM
config ARCH_RAMSTAGE_ARMV7 - def_bool n + bool select ARCH_RAMSTAGE_ARM + config ARCH_BOOTBLOCK_ARMV7_M - def_bool n + bool select ARCH_BOOTBLOCK_ARM + config ARCH_VERSTAGE_ARMV7_M - def_bool n + bool select ARCH_VERSTAGE_ARM
config ARCH_BOOTBLOCK_ARMV7_R - def_bool n + bool select ARCH_BOOTBLOCK_ARM
config ARCH_VERSTAGE_ARMV7_R - def_bool n + bool select ARCH_VERSTAGE_ARM
config ARCH_ROMSTAGE_ARMV7_R - def_bool n + bool select ARCH_ROMSTAGE_ARM
config ARCH_RAMSTAGE_ARMV7_R - def_bool n + bool select ARCH_RAMSTAGE_ARM diff --git a/src/mainboard/google/nyan_blaze/Kconfig b/src/mainboard/google/nyan_blaze/Kconfig index a5b1747..1e91da9 100644 --- a/src/mainboard/google/nyan_blaze/Kconfig +++ b/src/mainboard/google/nyan_blaze/Kconfig @@ -17,7 +17,6 @@
config BOARD_SPECIFIC_OPTIONS def_bool y - select ARCH_ARM select COMMON_CBFS_SPI_WRAPPER select EC_GOOGLE_CHROMEEC select EC_GOOGLE_CHROMEEC_SPI
-
Arthur Heymans (Code Review) -
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)