Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31471
Change subject: sb/intel/i82371eb/fadt: Fix compilation on x86_64 ......................................................................
sb/intel/i82371eb/fadt: Fix compilation on x86_64
Change-Id: I8997910ff003a4d0c97656cb1e9a4342230ac51a Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/southbridge/intel/i82371eb/fadt.c 1 file changed, 6 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/31471/1
diff --git a/src/southbridge/intel/i82371eb/fadt.c b/src/southbridge/intel/i82371eb/fadt.c index 41ad31c..4526675 100644 --- a/src/southbridge/intel/i82371eb/fadt.c +++ b/src/southbridge/intel/i82371eb/fadt.c @@ -46,8 +46,8 @@ memcpy(header->asl_compiler_id, ASLC, 4); header->asl_compiler_revision = 42;
- fadt->firmware_ctrl = (u32)facs; - fadt->dsdt = (u32)dsdt; + fadt->firmware_ctrl = (uintptr_t)facs; + fadt->dsdt = (uintptr_t)dsdt; fadt->preferred_pm_profile = 0; /* unspecified */ fadt->sci_int = 9; fadt->smi_cmd = 0; /* smi command port */ @@ -153,10 +153,10 @@ fadt->reset_reg.addrh = 0x0; fadt->reset_value = 0;
- fadt->x_firmware_ctl_l = (u32)facs; - fadt->x_firmware_ctl_h = 0; - fadt->x_dsdt_l = (u32)dsdt; - fadt->x_dsdt_h = 0; + fadt->x_firmware_ctl_l = (uintptr_t)facs; + fadt->x_firmware_ctl_h = ((uintptr_t)facs) >> 32; + fadt->x_dsdt_l = (uintptr_t)dsdt; + fadt->x_dsdt_h = ((uintptr_t)dsdt) >> 32;
fadt->x_pm1a_evt_blk.space_id = 1; fadt->x_pm1a_evt_blk.bit_width = fadt->pm1_evt_len * 8;
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31471
to look at the new patch set (#2).
Change subject: sb/intel/i82371eb/fadt: Fix compilation on x86_64 ......................................................................
sb/intel/i82371eb/fadt: Fix compilation on x86_64
Change-Id: I8997910ff003a4d0c97656cb1e9a4342230ac51a Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/southbridge/intel/i82371eb/fadt.c 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/31471/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31471 )
Change subject: sb/intel/i82371eb/fadt: Fix compilation on x86_64 ......................................................................
Patch Set 2: Code-Review+2
(2 comments)
Let's make qemu a clean sample of x86_64 casts and fix rest of the tree to match.
https://review.coreboot.org/#/c/31471/2/src/southbridge/intel/i82371eb/fadt.... File src/southbridge/intel/i82371eb/fadt.c:
https://review.coreboot.org/#/c/31471/2/src/southbridge/intel/i82371eb/fadt.... PS2, Line 50: fadt->dsdt = (uintptr_t)dsdt; We have pretty much the same in mainboard directories. I feel uinptr_t is the correct one to use, but I can see unsigned long elsewhere.
https://review.coreboot.org/#/c/31471/2/src/southbridge/intel/i82371eb/fadt.... PS2, Line 159: fadt->x_dsdt_h = 0; I was going to suggest what you already tried in patchset #1. Once we have some agreement on what to do with -Wshift-xxx we can return to this and probably all similar cases under mainboard/ as well.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31471 )
Change subject: sb/intel/i82371eb/fadt: Fix compilation on x86_64 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31471/2/src/southbridge/intel/i82371eb/fadt.... File src/southbridge/intel/i82371eb/fadt.c:
https://review.coreboot.org/#/c/31471/2/src/southbridge/intel/i82371eb/fadt.... PS2, Line 50: fadt->dsdt = (uintptr_t)dsdt;
We have pretty much the same in mainboard directories. […]
It depends on the destination. Some fields are 32bit only. We need to add asserts on x64 to make sure it never truncates, but that is a future cleanup.
https://review.coreboot.org/#/c/31471/2/src/southbridge/intel/i82371eb/fadt.... PS2, Line 159: fadt->x_dsdt_h = 0;
I was going to suggest what you already tried in patchset #1. […]
yes, but that's future cleanup. ATM it works for all architectures as everything is assumed to be below 4GiB.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31471 )
Change subject: sb/intel/i82371eb/fadt: Fix compilation on x86_64 ......................................................................
sb/intel/i82371eb/fadt: Fix compilation on x86_64
Change-Id: I8997910ff003a4d0c97656cb1e9a4342230ac51a Signed-off-by: Patrick Rudolph siro@das-labor.org Reviewed-on: https://review.coreboot.org/c/31471 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/intel/i82371eb/fadt.c 1 file changed, 4 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/southbridge/intel/i82371eb/fadt.c b/src/southbridge/intel/i82371eb/fadt.c index 41ad31c..2a3a18a 100644 --- a/src/southbridge/intel/i82371eb/fadt.c +++ b/src/southbridge/intel/i82371eb/fadt.c @@ -46,8 +46,8 @@ memcpy(header->asl_compiler_id, ASLC, 4); header->asl_compiler_revision = 42;
- fadt->firmware_ctrl = (u32)facs; - fadt->dsdt = (u32)dsdt; + fadt->firmware_ctrl = (uintptr_t)facs; + fadt->dsdt = (uintptr_t)dsdt; fadt->preferred_pm_profile = 0; /* unspecified */ fadt->sci_int = 9; fadt->smi_cmd = 0; /* smi command port */ @@ -153,9 +153,9 @@ fadt->reset_reg.addrh = 0x0; fadt->reset_value = 0;
- fadt->x_firmware_ctl_l = (u32)facs; + fadt->x_firmware_ctl_l = (uintptr_t)facs; fadt->x_firmware_ctl_h = 0; - fadt->x_dsdt_l = (u32)dsdt; + fadt->x_dsdt_l = (uintptr_t)dsdt; fadt->x_dsdt_h = 0;
fadt->x_pm1a_evt_blk.space_id = 1;
-
Kyösti Mälkki (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)