Change in ...coreboot[master]: google/kahlee: No need to guard with HAVE_ACPI_RESUME

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31317

Change subject: google/kahlee: No need to guard with HAVE_ACPI_RESUME
......................................................................

google/kahlee: No need to guard with HAVE_ACPI_RESUME

Change-Id: Ie8748ccddc8e31569c58deba5d08c98a04326fa8
Signed-off-by: Kyösti Mälkki 
---
M src/mainboard/google/kahlee/mainboard.c
M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/kahlee/variants/baseboard/mainboard.c
3 files changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31317/1

diff --git a/src/mainboard/google/kahlee/mainboard.c b/src/mainboard/google/kahlee/mainboard.c
index ebdcc93..ed48593 100644
--- a/src/mainboard/google/kahlee/mainboard.c
+++ b/src/mainboard/google/kahlee/mainboard.c
@@ -195,12 +195,10 @@
    return variant_get_ehci_oc_map(map);
 }

-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void mainboard_suspend_resume(void)
 {
    variant_mainboard_suspend_resume();
 }
-#endif

struct chip_operations mainboard_ops = {
    .init = mainboard_init,
@@ -214,11 +212,9 @@
    return 0;
 }

-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void __weak variant_mainboard_suspend_resume(void)
 {
 }
-#endif

const char *smbios_mainboard_sku(void)
 {
diff --git a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
index da65bd8..e3cde3e 100644
--- a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
+++ b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
@@ -31,8 +31,6 @@
 const struct soc_amd_gpio *variant_romstage_gpio_table(size_t *size);
 const struct soc_amd_gpio *variant_gpio_table(size_t *size);
 void variant_romstage_entry(int s3_resume);
-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void variant_mainboard_suspend_resume(void);
-#endif

#endif /* __BASEBOARD_VARIANTS_H__ */
diff --git a/src/mainboard/google/kahlee/variants/baseboard/mainboard.c b/src/mainboard/google/kahlee/variants/baseboard/mainboard.c
index d1a3492..71aca73 100644
--- a/src/mainboard/google/kahlee/variants/baseboard/mainboard.c
+++ b/src/mainboard/google/kahlee/variants/baseboard/mainboard.c
@@ -32,13 +32,11 @@
    return sku;
 }

-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void variant_mainboard_suspend_resume(void)
 {
    /* Enable backlight - GPIO 133 active low */
    gpio_set(GPIO_133, 0);
 }
-#endif

void board_bh720(struct device *dev)
 {

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: No need to guard with HAVE_ACPI_RESUME
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/#/c/31317/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31317/1//COMMIT_MSG@7 
PS1, Line 7: google/kahlee: No need to guard with HAVE_ACPI_RESUME
Maybe:
> Remove unneeded HAVE_ACPI_RESUME guard

https://review.coreboot.org/#/c/31317/1//COMMIT_MSG@8 
PS1, Line 8: 
Add a small note, why it’s not needed?

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: No need to guard with HAVE_ACPI_RESUME
......................................................................

Patch Set 1: Code-Review-1

For the same reason as the previous review, I would prefer you abandon this unless you first block the capability to enter S3. Also, I should soon release a new board using a variation of stoneyridge code (to support merlin falcon). S3 is a lot more complicated then you imagine, and some of these code was not part of original S3, but fix to S3 bugs. However, AMD gardenia (as far as I remember) do not support S3 and don't have this code. If you play with S3 control without first being sure that S3 entry is avoided when S3 is disabled, all sort of bad things could happen. True, all kahlee based platforms will have S3 enabled... but what if someone wants to create new board, based on kahlee (want some of kahlee characteristics but not S3)?
Though not as serious as one within agesawrapper, I would not play with S3 code. The amount of bugs we had to fix on S3 code is not worth risking creating new bugs.
If you want to continue with this, at least add a message to this effect warning any OEM using kahlee folder as a base for a new project on what is needed if he/she does not want S3.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: No need to guard with HAVE_ACPI_RESUME
......................................................................

Patch Set 1:

Richard, you most likely want to check with Martin or Marshall for my background around AGESA and S3 before commenting further...

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: No need to guard with HAVE_ACPI_RESUME
......................................................................

Patch Set 1: -Code-Review
> Patch Set 1:
>
> Richard, you most likely want to check with Martin or Marshall for my background around AGESA and S3 before commenting further...

I'm still not comfortable on playing with S3. I agree that if ASL code does not advertise S3 then you are probably correct removing these. But it'll add unneeded extra code when S3 is disabled. At least add the message requested by Paul.

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: No need to guard with HAVE_ACPI_RESUME
......................................................................

Patch Set 1:

(1 comment)

I'm good with the change itself.  Only a suggestion in the commit message.

https://review.coreboot.org/#/c/31317/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31317/1//COMMIT_MSG@8 
PS1, Line 8:
> Add a small note, why it’s not needed?

I'm on the same page as Paul, here.  A quick sentence that explains why it's unnecessary will help educate others, as they browse changes, to understand why something's not necessary.

Change in ...coreboot[master]: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard

[Kyösti Mälkki (Code Review)]

Hello Marshall Dawson, Richard Spiegel, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31317

to look at the new patch set (#2).

Change subject: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard
......................................................................

google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard

We leave it to linker garbage collection to drop
unreferenced code and symbols from final object files.
Function declarations and definitions are to be guarded
with preprocessor directives only as a last resort.

Change-Id: Ie8748ccddc8e31569c58deba5d08c98a04326fa8
Signed-off-by: Kyösti Mälkki 
---
M src/mainboard/google/kahlee/mainboard.c
M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/kahlee/variants/baseboard/mainboard.c
3 files changed, 0 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31317/2

Change in ...coreboot[master]: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard
......................................................................

Patch Set 2: Code-Review+2

The way I was looking at it is that the mainboard's source is doing a check for a Kconfig symbol, yet the Kconfig file selects the symbol.

Change in ...coreboot[master]: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31317 )

Change subject: google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard
......................................................................

google/kahlee: Remove unneeded HAVE_ACPI_RESUME guard

We leave it to linker garbage collection to drop
unreferenced code and symbols from final object files.
Function declarations and definitions are to be guarded
with preprocessor directives only as a last resort.

Change-Id: Ie8748ccddc8e31569c58deba5d08c98a04326fa8
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/31317
Tested-by: build bot (Jenkins) 
Reviewed-by: Richard Spiegel 
Reviewed-by: Marshall Dawson 
---
M src/mainboard/google/kahlee/mainboard.c
M src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/kahlee/variants/baseboard/mainboard.c
3 files changed, 0 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marshall Dawson: Looks good to me, approved
  Richard Spiegel: Looks good to me, approved

diff --git a/src/mainboard/google/kahlee/mainboard.c b/src/mainboard/google/kahlee/mainboard.c
index ebdcc93..ed48593 100644
--- a/src/mainboard/google/kahlee/mainboard.c
+++ b/src/mainboard/google/kahlee/mainboard.c
@@ -195,12 +195,10 @@
    return variant_get_ehci_oc_map(map);
 }

-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void mainboard_suspend_resume(void)
 {
    variant_mainboard_suspend_resume();
 }
-#endif

struct chip_operations mainboard_ops = {
    .init = mainboard_init,
@@ -214,11 +212,9 @@
    return 0;
 }

-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void __weak variant_mainboard_suspend_resume(void)
 {
 }
-#endif

const char *smbios_mainboard_sku(void)
 {
diff --git a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
index da65bd8..e3cde3e 100644
--- a/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
+++ b/src/mainboard/google/kahlee/variants/baseboard/include/baseboard/variants.h
@@ -31,8 +31,6 @@
 const struct soc_amd_gpio *variant_romstage_gpio_table(size_t *size);
 const struct soc_amd_gpio *variant_gpio_table(size_t *size);
 void variant_romstage_entry(int s3_resume);
-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void variant_mainboard_suspend_resume(void);
-#endif

#endif /* __BASEBOARD_VARIANTS_H__ */
diff --git a/src/mainboard/google/kahlee/variants/baseboard/mainboard.c b/src/mainboard/google/kahlee/variants/baseboard/mainboard.c
index d1a3492..71aca73 100644
--- a/src/mainboard/google/kahlee/variants/baseboard/mainboard.c
+++ b/src/mainboard/google/kahlee/variants/baseboard/mainboard.c
@@ -32,13 +32,11 @@
    return sku;
 }

-#if IS_ENABLED(CONFIG_HAVE_ACPI_RESUME)
 void variant_mainboard_suspend_resume(void)
 {
    /* Enable backlight - GPIO 133 active low */
    gpio_set(GPIO_133, 0);
 }
-#endif

void board_bh720(struct device *dev)
 {