Change in ...coreboot[master]: lib/hardwaremain: Remove unused acpi_is_wakeup() function
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31140
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
lib/hardwaremain: Remove unused acpi_is_wakeup() function
Looks like acpi_is_wakeup() return value is not getting honoured, hence this function is not doing anything rather reading from romstage buffer.
Change-Id: Icc57804074a58315e72e276fcae799febc10612d Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/lib/hardwaremain.c 1 file changed, 1 insertion(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/31140/1
diff --git a/src/lib/hardwaremain.c b/src/lib/hardwaremain.c index 98b8841..3ec7e8e 100644 --- a/src/lib/hardwaremain.c +++ b/src/lib/hardwaremain.c @@ -2,6 +2,7 @@ * This file is part of the coreboot project. * * Copyright (C) 2013 Google, Inc. + * Copyright (C) 2019 Intel Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -469,11 +470,6 @@ timestamp_add_now(TS_START_RAMSTAGE); post_code(POST_ENTRY_RAMSTAGE);
- /* Handoff sleep type from romstage. */ -#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME) - acpi_is_wakeup(); -#endif - exception_init(); threads_initialize();
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1: Code-Review-1
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type global variable, and returns a value based on it. It is necessary to set that global variable correctly (hence a function call is needed), but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with acpi_handoff_wakeup() instead.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type global variable, and returns a value based on it. It is necessary to set that global variable correctly (hence a function call is needed), but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with acpi_handoff_wakeup() instead.
I could see acpi_is_wakeup() function is getting called from some other place as well. i need to see who is calling this.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type global variable, and returns a value based on it. It is necessary to set that global variable correctly (hence a function call is needed), but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with acpi_handoff_wakeup() instead.
I could see acpi_is_wakeup() function is getting called from some other place as well. i need to see who is calling this.
@Angel: Can you please this CL and check if you are seeing any problem.
I don't see any problem with CL over for global variable is getting assigned
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Patch Set 1:
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type global variable, and returns a value based on it. It is necessary to set that global variable correctly (hence a function call is needed), but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with acpi_handoff_wakeup() instead.
I could see acpi_is_wakeup() function is getting called from some other place as well. i need to see who is calling this.
@Angel: Can you please this CL and check if you are seeing any problem.
I don't see any problem with CL over for global variable is getting assigned
Sure, I have a few boards to test this with.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Patch Set 1:
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type global variable, and returns a value based on it. It is necessary
to set
that global variable correctly (hence a function call is
needed),
but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with
acpi_handoff_wakeup()
instead.
I could see acpi_is_wakeup() function is getting called from
some
other place as well. i need to see who is calling this.
@Angel: Can you please this CL and check if you are seeing any
problem.
I don't see any problem with CL over for global variable is
getting assigned
Sure, I have a few boards to test this with.
thanks a lot
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Patch Set 1:
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type
global
variable, and returns a value based on it. It is necessary
to set
that global variable correctly (hence a function call is
needed),
but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with
acpi_handoff_wakeup()
instead.
I could see acpi_is_wakeup() function is getting called from
some
other place as well. i need to see who is calling this.
@Angel: Can you please this CL and check if you are seeing any
problem.
I don't see any problem with CL over for global variable is
getting assigned
Sure, I have a few boards to test this with.
thanks a lot
@Angle, do you have any further observation on this?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c
The acpi_is_wakeup() function sets the acpi_slp_type
global
variable, and returns a value based on it. It is necessary
to set
that global variable correctly (hence a function call is
needed),
but no value needs to be returned.
I would suggest replacing acpi_is_wakeup() with
acpi_handoff_wakeup()
instead.
I could see acpi_is_wakeup() function is getting called from
some
other place as well. i need to see who is calling this.
@Angel: Can you please this CL and check if you are seeing any
problem.
I don't see any problem with CL over for global variable is
getting assigned
Sure, I have a few boards to test this with.
thanks a lot
@Angle, do you have any further observation on this?
Sorry, I have been busy recently. I'll test this today.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
> Refer to: https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/acpi_s3.c > > The acpi_is_wakeup() function sets the acpi_slp_type
global
> variable, and returns a value based on it. It is
necessary
to set
> that global variable correctly (hence a function call
is
needed),
> but no value needs to be returned. > > I would suggest replacing acpi_is_wakeup() with
acpi_handoff_wakeup()
> instead.
I could see acpi_is_wakeup() function is getting called
from
some
other place as well. i need to see who is calling this.
@Angel: Can you please this CL and check if you are seeing
any
problem.
I don't see any problem with CL over for global variable
is
getting assigned
Sure, I have a few boards to test this with.
thanks a lot
@Angle, do you have any further observation on this?
Sorry, I have been busy recently. I'll test this today.
Thanks a lot for your commitment.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1: Code-Review+1
It does not seem to affect resume on an Asrock G41M-S3 mainboard.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
It does not seem to affect resume on an Asrock G41M-S3 mainboard.
Thanks Angle, i have ran more validation. things looks good
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1: Code-Review-2
While it is sort of remainer from the days before variable acpi_slp_type was declared static (and someone evaluated the global variable directly), the call still serves a purpose; it prints into console exactly once, early in ramstage, if we are on normal or S3 boot path.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
While it is sort of remainer from the days before variable acpi_slp_type was declared static (and someone evaluated the global variable directly), the call still serves a purpose; it prints into console exactly once, early in ramstage, if we are on normal or S3 boot path.
without calling this function also i'm seeing normal vs s3 resume print in early ramstage code.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
Patch Set 1:
While it is sort of remainer from the days before variable acpi_slp_type was declared static (and someone evaluated the global variable directly), the call still serves a purpose; it prints into console exactly once, early in ramstage, if we are on normal or S3 boot path.
without calling this function also i'm seeing normal vs s3 resume print in early ramstage code.
My point was the message currently appears at a consistent and convenient location regardless of platform code.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://review.coreboot.org/#/c/31140/1/src/lib/hardwaremain.c File src/lib/hardwaremain.c:
https://review.coreboot.org/#/c/31140/1/src/lib/hardwaremain.c@5 PS1, Line 5: * Copyright (C) 2019 Intel Inc. Claiming copyright with this change does not make sense, since no lines were added that can be copyrighted.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
(1 comment)
IANAL though.
https://review.coreboot.org/#/c/31140/1/src/lib/hardwaremain.c File src/lib/hardwaremain.c:
https://review.coreboot.org/#/c/31140/1/src/lib/hardwaremain.c@5 PS1, Line 5: * Copyright (C) 2019 Intel Inc.
Claiming copyright with this change does not make sense, since no lines were added that can be copyr […]
IANAL, though.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Patch Set 1:
IMHO this should be abandoned.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31140 )
Change subject: lib/hardwaremain: Remove unused acpi_is_wakeup() function ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)
-
Subrata Banik (Code Review)