Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84008?usp=email )
Change subject: libpayload: Use defined length movs
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
File payloads/libpayload/arch/x86/pt.S:
https://review.coreboot.org/c/coreboot/+/84008/comment/aa7cc9dc_665f95e8?us… :
PS3, Line 132: mov %eax, (%rsi, %rcx, 8)
: mov %ebx, 4(%rsi, %rcx, 8)
> Why are these ones fine?
I guess because the actual problematic lines were the `mov $0...` ones
because the size is not implied (it is for the other cases by the register
size). Is that right Arthur? If we change the `mov %eax,...` above for
consistency, I'd say we should change these too.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84008?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I2fabe7fbe3f8afac5c1128debf2e09a484f26fc5
Gerrit-Change-Number: 84008
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:59:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Attention is currently required from: Angel Pons, Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84007?usp=email )
Change subject: libpayload: Use unsigned integet for PDG_MASK
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
File payloads/libpayload/arch/arm/virtual.c:
https://review.coreboot.org/c/coreboot/+/84007/comment/921f3371_4cd9547d?us… :
PS3, Line 82: return 1 << PAGE_SHIFT;
> Same "problem" here
I don't think it is. Shifting a signed but positive integer is defined.
The problem above, I guess, is that `~0` (to be read as `~(int)0`)
is negative.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84007?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ic4ce64207393ec4a8d6b188b35e0f436342826de
Gerrit-Change-Number: 84007
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:54:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Attention is currently required from: Arthur Heymans, Julius Werner.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84005?usp=email )
Change subject: arch/arm: Fix building with LTO
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS4:
Now it's done for all the stages, but there's a lonely comment
only for ramstage... Maybe it's time for a loop? That would
allow to keep everything together (with the comment).
--
To view, visit https://review.coreboot.org/c/coreboot/+/84005?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I3d89c093cee2636e648987a06afb0d325b1d96ff
Gerrit-Change-Number: 84005
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:47:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Arthur Heymans, Marvin Drees, Patrick Rudolph.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84003?usp=email )
Change subject: Add initial experimental LTO support
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84003?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ieb9204777fd349542744a8946e2207731c37969c
Gerrit-Change-Number: 84003
Gerrit-PatchSet: 6
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Marvin Drees <marvin.drees(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Marvin Drees <marvin.drees(a)9elements.com>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:42:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/69747?usp=email )
(
25 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: arch/arm: Add a few ARM targets as supported by CLANG
......................................................................
arch/arm: Add a few ARM targets as supported by CLANG
Some targets cannot be supported by clang as clang generates slightly
larger binaries which the hardware won't accept. This is usually the
case with CONFIG_CHROMEOS.
Change-Id: I88cf8ce16fb6c61c19d615e396f5871179b06fc8
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/69747
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/arch/arm/Kconfig
M src/soc/nvidia/tegra124/Kconfig
M src/soc/qualcomm/ipq40xx/Kconfig
M src/soc/qualcomm/ipq806x/Kconfig
M src/soc/rockchip/rk3288/Kconfig
5 files changed, 8 insertions(+), 1 deletion(-)
Approvals:
Nico Huber: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/src/arch/arm/Kconfig b/src/arch/arm/Kconfig
index 0829dcb..64fe915 100644
--- a/src/arch/arm/Kconfig
+++ b/src/arch/arm/Kconfig
@@ -2,7 +2,6 @@
config ARCH_ARM
bool
- select CLANG_UNSUPPORTED
config ARCH_BOOTBLOCK_ARM
bool
diff --git a/src/soc/nvidia/tegra124/Kconfig b/src/soc/nvidia/tegra124/Kconfig
index 88877e9..5f967d2 100644
--- a/src/soc/nvidia/tegra124/Kconfig
+++ b/src/soc/nvidia/tegra124/Kconfig
@@ -14,6 +14,8 @@
select MAINBOARD_HAS_NATIVE_VGA_INIT
select MAINBOARD_FORCE_NATIVE_VGA_INIT
select HAVE_LINEAR_FRAMEBUFFER
+ # clang creates larger binaries that may not fit
+ select CLANG_UNSUPPORTED if CHROMEOS
if SOC_NVIDIA_TEGRA124
diff --git a/src/soc/qualcomm/ipq40xx/Kconfig b/src/soc/qualcomm/ipq40xx/Kconfig
index 98c3a8e..e32f228 100644
--- a/src/soc/qualcomm/ipq40xx/Kconfig
+++ b/src/soc/qualcomm/ipq40xx/Kconfig
@@ -9,6 +9,8 @@
select ARCH_RAMSTAGE_ARMV7
select HAVE_UART_SPECIAL
select GENERIC_GPIO_LIB
+# clang creates larger binaries that may not fit
+ select CLANG_UNSUPPORTED if CHROMEOS
if SOC_QC_IPQ40XX
diff --git a/src/soc/qualcomm/ipq806x/Kconfig b/src/soc/qualcomm/ipq806x/Kconfig
index b9e47e5..22a2155 100644
--- a/src/soc/qualcomm/ipq806x/Kconfig
+++ b/src/soc/qualcomm/ipq806x/Kconfig
@@ -10,6 +10,8 @@
select HAVE_UART_SPECIAL
select GENERIC_GPIO_LIB
select NO_MONOTONIC_TIMER
+# clang creates larger binaries that may not fit
+ select CLANG_UNSUPPORTED if CHROMEOS
if SOC_QC_IPQ806X
diff --git a/src/soc/rockchip/rk3288/Kconfig b/src/soc/rockchip/rk3288/Kconfig
index a51df2a..7036bbb 100644
--- a/src/soc/rockchip/rk3288/Kconfig
+++ b/src/soc/rockchip/rk3288/Kconfig
@@ -18,6 +18,8 @@
select NO_BOOTBLOCK_CONSOLE
select NO_FMAP_CACHE
select NO_CBFS_MCACHE
+# clang creates larger binaries that may not fit
+ select CLANG_UNSUPPORTED if CHROMEOS
if SOC_ROCKCHIP_RK3288
--
To view, visit https://review.coreboot.org/c/coreboot/+/69747?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I88cf8ce16fb6c61c19d615e396f5871179b06fc8
Gerrit-Change-Number: 69747
Gerrit-PatchSet: 27
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Julius Werner, Paul Menzel.
Hello Julius Werner, Martin L Roth, Nico Huber, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/69747?usp=email
to look at the new patch set (#26).
Change subject: arch/arm: Add a few ARM targets as supported by CLANG
......................................................................
arch/arm: Add a few ARM targets as supported by CLANG
Some targets cannot be supported by clang as clang generates slightly
larger binaries which the hardware won't accept. This is usually the
case with CONFIG_CHROMEOS.
Change-Id: I88cf8ce16fb6c61c19d615e396f5871179b06fc8
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
---
M src/arch/arm/Kconfig
M src/soc/nvidia/tegra124/Kconfig
M src/soc/qualcomm/ipq40xx/Kconfig
M src/soc/qualcomm/ipq806x/Kconfig
M src/soc/rockchip/rk3288/Kconfig
5 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/69747/26
--
To view, visit https://review.coreboot.org/c/coreboot/+/69747?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I88cf8ce16fb6c61c19d615e396f5871179b06fc8
Gerrit-Change-Number: 69747
Gerrit-PatchSet: 26
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Attention is currently required from: Julius Werner, Paul Menzel.
Arthur Heymans has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/69747?usp=email )
Change subject: arch/arm: Add a few ARM targets as supported by CLANG
......................................................................
Patch Set 25:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/69747/comment/aa555361_08612b77?us… :
PS25, Line 7: arch/arm: Add a few ARM targets as supported by CLANG
> Mention that it’s for non ChromeOS case?
Done
https://review.coreboot.org/c/coreboot/+/69747/comment/9efec586_cd2a424c?us… :
PS25, Line 11:
> Some numbers would be nice.
Binaries fail to link. It is possible to artificially change the linker script to have some insight in this. Clangs LTO is quite good and should allow to fix this.
--
To view, visit https://review.coreboot.org/c/coreboot/+/69747?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I88cf8ce16fb6c61c19d615e396f5871179b06fc8
Gerrit-Change-Number: 69747
Gerrit-PatchSet: 25
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:39:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Attention is currently required from: Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84056?usp=email )
Change subject: drivers/intel/gma: Fix mismatching types for fb_add_framebuffer_info
......................................................................
Patch Set 6: Code-Review+1
Copied votes on follow-up patch sets have been updated:
* Code-Review+1 has been copied to patch set 7 (copy condition: "changekind:NO_CHANGE OR changekind:NO_CODE_CHANGE OR changekind:TRIVIAL_REBASE OR is:MIN").
(1 comment)
Patchset:
PS6:
I don't mind doing this, but want to provide some context / alternatives:
Pretending it's `size_t` apparently was a simplification to avoid adding
a lot of details just for the null-check. So if it's just a null-check we
want, and we want to do it without tricks, we could also move it to C.
Could be something like
```
int fb_add_framebuffer_info_simple(
uintptr_t fb_addr, uint32_t x_res, uint32_t y_res,
uint32_t bytes_per_line, uint8_t bits_per_pixel)
{
return fb_add_framebuffer_info(fb_addr, x_res, y_res, bytes_per_line, bits_per_pixel) != NULL;
}
```
And call that from Ada.
OTOH, if we keep the current API and add all the definitions in Ada (never
know, might be useful in the future), then we have to keep C and Ada in
sync and should at least keep some comments around that remind us.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84056?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I2d5a9a86dbb91a5505891a30c6e9072b1b4dfc92
Gerrit-Change-Number: 84056
Gerrit-PatchSet: 6
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:38:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Arthur Heymans has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84012?usp=email )
Change subject: libpayload: Allow LTO with clang
......................................................................
Patch Set 3:
(1 comment)
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/84012/comment/ee6d7cad_58c8174d?us… :
PS1, Line 89: final binary size, but may increase compilation time.
> > > > How well is this tested with clang? Should we leave a warning? […]
Done. Will attempt upstream but patching clang works for now.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84012?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I41bb613de5d16ca180dd684a0bec4840d9119e6f
Gerrit-Change-Number: 84012
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:33:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Attention is currently required from: Angel Pons, Nico Huber.
Arthur Heymans has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84011?usp=email )
Change subject: nvramcui: Fix main function signature
......................................................................
Patch Set 3:
(1 comment)
File payloads/nvramcui/nvramcui.c:
https://review.coreboot.org/c/coreboot/+/84011/comment/491d440e_50234768?us… :
PS1, Line 162: *argv
> I've seen both, I prefer `char *argv[]` because it better conveys intent (`argv` is an array of strings)
It is but this patch is about just matching signatures. Fixing this in libpayload would a different topic which may involve downstream changes.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84011?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ia0c50224bd70503e884573fedf3bf33c134bba00
Gerrit-Change-Number: 84011
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 23 Aug 2024 10:32:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>