Change in coreboot[master]: soc/amd/picasso: use FADT devicetree configuration options

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

soc/amd/picasso: use FADT devicetree configuration options

Two of the items in the FADT ACPI table frequently need to be updated by
the platform, so let's make it easy to update them by putting them into
the devicetree.

- fadt_boot_arch 0="legacy free" which while reasonable, probably isn't
what will be wanted by most platforms, so this should generally get
updated in the platform code.
- fadt_flags 0 is definitely NOT a reasonable default, so supply a
better default value of what was previously being set.

The fadt_pm_profile option is removed from this patch. See commit
56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
field.

Change-Id: I6e8d0c60cadfdd24b6926703b252abbc56d436de
Signed-off-by: Martin Roth 
Signed-off-by: Felix Held 
Reviewed-on: https://chromium-review.googlesource.com/1944624
---
M src/soc/amd/picasso/acpi.c
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/43418/1

diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c
index 3c22f1d..2eff4fe 100644
--- a/src/soc/amd/picasso/acpi.c
+++ b/src/soc/amd/picasso/acpi.c
@@ -88,6 +88,8 @@
  */
 void acpi_fill_fadt(acpi_fadt_t *fadt)
 {
+	const struct soc_amd_picasso_config *cfg = config_of_soc();
+
    printk(BIOS_DEBUG, "pm_base: 0x%04x\n", PICASSO_ACPI_IO_BASE);

fadt->sci_int = 9;		/* IRQ 09 - ACPI SCI */
@@ -124,9 +126,12 @@
    fadt->day_alrm = 0;	/* 0x7d these have to be */
    fadt->mon_alrm = 0;	/* 0x7e added to cmos.layout */
    fadt->century = 0;	/* 0x7f to make rtc alarm work */
-	fadt->iapc_boot_arch = ACPI_FADT_LEGACY_DEVICES | ACPI_FADT_8042;
+	fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */
    fadt->res2 = 0;		/* reserved, MUST be 0 ACPI 3.0 */
-	fadt->flags = ACPI_FADT_WBINVD | /* See table 5-10 ACPI 3.0a spec */
+	if (cfg->fadt_flags)
+		fadt->flags = cfg->fadt_flags;
+	else
+		fadt->flags = ACPI_FADT_WBINVD | /* See table 5-10 ACPI 3.0a spec */
    			ACPI_FADT_C1_SUPPORTED |
    			ACPI_FADT_SLEEP_BUTTON |
    			ACPI_FADT_S4_RTC_WAKE |

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(2 comments)

I do not like the inherent redundancy of specifying such options as-is in the devicetree.

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 129: 	fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */
What controls 8042 timer enablement? Maybe hook things up so that this is automatically configured depending on whether the legacy timer is enabled?

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 132: 		fadt->flags = cfg->fadt_flags;
I don't think repeating all the FADT flags on each devicetree is a good idea. Sure, I understand there will be very few users of this SoC, but it's error-prone. For example, ACPI_FADT_WBINVD is mandatory, ACPI_FADT_RESET_REGISTER depends on options set up below (so it should always be set) and most of the other options seem to be fixed. Maybe do like some Intel southbridges do with the `docking_supported` options instead?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
This was done in the parent commit? CB:43417

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
> This was done in the parent commit? CB:43417

The devicetree option got removed from picasso in the parent commit, but the commit I referenced removed the overriding of the FADT entry. That I dropped the fadt_pm_profile here is a difference to the downstream patch

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
> The devicetree option got removed from picasso in the parent commit, but the commit I referenced rem […]

I'm afraid I don't follow 😕

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@7 
PS1, Line 7: soc/amd/picasso: use FADT devicetree configuration options
Would be great to also read, that the defaults are changed. (I f I am not mistaken.)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
Please use:
> commit 56da63c3dc (sb,soc/amd, ACPI: Do not override FADT preferred_pm_profile)

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
> Please use: […]

the referenced commit removed the preferred_pm_profile override, so that the option selected by SYSTEM_TYPE_* is used.

i consider using short hashes a rather bad idea; much bigger chance of collisions. if we have more bits, we shouldn't throw them away; sha1 already doesn't have that many bits. this usually gets converted into a clickable link, but the line break in between probably prevents that. i can add the summary line there, but don't see that much added value in that and i haven't seen that being done in other patches

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
> the referenced commit removed the preferred_pm_profile override, so that the option selected by SYST […]

I found a bug in CB:1055 where the SHA in gerrit doesn't match the SHA in the actual git repo. One case. Still, if you can write "commit foobarbaz" in a single line, it should become a clickable link.

Change in coreboot[master]: [WIP] soc/amd/picasso: use FADT devicetree configuration options

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Angel Pons,

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

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

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

Change subject: [WIP] soc/amd/picasso: use FADT devicetree configuration options
......................................................................

[WIP] soc/amd/picasso: use FADT devicetree configuration options

Two of the items in the FADT ACPI table frequently need to be updated by
the platform, so let's make it easy to update them by putting them into
the devicetree.

- fadt_boot_arch 0="legacy free" which while reasonable, probably isn't
what will be wanted by most platforms, so this should generally get
updated in the platform code.
- fadt_flags 0 is definitely NOT a reasonable default, so supply a
better default value of what was previously being set.

This patch changes the default for fadt_boot_arch.

commit 56da63c3dc3f50cfac541c779b608e1bae9e635c removed overriding
fadt_pm_profile, so this is removed from the original version of this
patch from ChromeOS downstream.

Change-Id: I6e8d0c60cadfdd24b6926703b252abbc56d436de
Signed-off-by: Martin Roth 
Signed-off-by: Felix Held 
Reviewed-on: https://chromium-review.googlesource.com/1944624
---
M src/soc/amd/picasso/acpi.c
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/43418/2

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Angel Pons,

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

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

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

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

soc/amd/picasso: use FADT devicetree configuration options

Two of the items in the FADT ACPI table frequently need to be updated by
the platform, so let's make it easy to update them by putting them into
the devicetree.

- fadt_boot_arch 0="legacy free" which while reasonable, probably isn't
what will be wanted by most platforms, so this should generally get
updated in the platform code.
- fadt_flags 0 is definitely NOT a reasonable default, so supply a
better default value of what was previously being set.

This patch changes the default for fadt_boot_arch.

commit 56da63c3dc3f50cfac541c779b608e1bae9e635c removed overriding
fadt_pm_profile, so this is removed from the original version of this
patch from ChromeOS downstream.

Change-Id: I6e8d0c60cadfdd24b6926703b252abbc56d436de
Signed-off-by: Martin Roth 
Signed-off-by: Felix Held 
Reviewed-on: https://chromium-review.googlesource.com/1944624
---
M src/soc/amd/picasso/acpi.c
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/43418/3

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@7 
PS1, Line 7: soc/amd/picasso: use FADT devicetree configuration options
> Would be great to also read, that the defaults are changed. (I f I am not mistaken. […]

Done

https://review.coreboot.org/c/coreboot/+/43418/1//COMMIT_MSG@19 
PS1, Line 19: The fadt_pm_profile option is removed from this patch. See commit
            : 56da63c3dc3f50cfac541c779b608e1bae9e635c which removed overriding that
            : field.
> I found a bug in CB:1055 where the SHA in gerrit doesn't match the SHA in the actual git repo. […]

I tried to clarify the last paragraph in more recent versions of the commit message

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 132: 		fadt->flags = cfg->fadt_flags;
> I don't think repeating all the FADT flags on each devicetree is a good idea. […]

it might be a good idea to set all options that are mandatory here and only bitwise or that with the additional settings from the devicetree

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 129: 	fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */
> What controls 8042 timer enablement? Maybe hook things up so that this is automatically configured d […]

Hrm, instead of allowing one to arbitrarily override this, maybe use an `enable_8042` option?

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 132: 		fadt->flags = cfg->fadt_flags;
> it might be a good idea to set all options that are mandatory here and only bitwise or that with the […]

Or even better: have devicetree options to enable such options instead of directly poking FADT. I had an insight recently: the devicetree should be as abstract as possible. That is, hide implementation details and provide high-level settings. Yes, the `register` keyword needs to be killed with fire.

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Angel Pons,

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

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

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

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

soc/amd/picasso: use FADT devicetree configuration options

Two of the items in the FADT ACPI table frequently are partially board-
specific, so let's make it easy to update them via devicetree settings.

- fadt_boot_arch 0="legacy free" which while reasonable, probably isn't
what will be wanted by most mainboards, so this should generally get
updated in the specific devicetree.
- In fadt_flags all chipset-specific flags get set while the mainboard
has to set all other flags that it needs to have set.

This patch changes the default for fadt_boot_arch.

Change-Id: I6e8d0c60cadfdd24b6926703b252abbc56d436de
Signed-off-by: Martin Roth 
Signed-off-by: Felix Held 
---
M src/soc/amd/picasso/acpi.c
1 file changed, 12 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/43418/5

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 5: Code-Review+2

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Angel Pons,

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

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

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

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

soc/amd/picasso: use FADT devicetree configuration options

Two of the items in the FADT ACPI table frequently are partially board-
specific, so let's make it easy to update them via devicetree settings.

- fadt_boot_arch 0="legacy free" which while reasonable, probably isn't
what will be wanted by most mainboards, so this should generally get
updated in the specific devicetree.
- In fadt_flags all chipset-specific flags get set while the mainboard
has to set all other flags that it needs to have set.

This patch changes the default for fadt_boot_arch.

Change-Id: I6e8d0c60cadfdd24b6926703b252abbc56d436de
Signed-off-by: Martin Roth 
Signed-off-by: Felix Held 
---
M src/soc/amd/picasso/acpi.c
1 file changed, 12 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/43418/6

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 6: Code-Review+2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 129: 	fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */
> Hrm, instead of allowing one to arbitrarily override this, maybe use an `enable_8042` option?

hm, good question. are you ok with getting this merged in its current state that at least properly sorts out the fadt_flags handling and I'll try to have a look into the fadt_boot_arch in a follow-up?

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 132: 		fadt->flags = cfg->fadt_flags;
> Or even better: have devicetree options to enable such options instead of directly poking FADT. […]

I added all mandatory and soc-specific options directly here and just add the board specific ones in the devicetree. while this might not be perfect, it's still much better than the old version of this patch

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

Patch Set 6: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/43418/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 129: 	fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */
> hm, good question. […]

Sounds good

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43418 )

Change subject: soc/amd/picasso: use FADT devicetree configuration options
......................................................................

soc/amd/picasso: use FADT devicetree configuration options

Two of the items in the FADT ACPI table frequently are partially board-
specific, so let's make it easy to update them via devicetree settings.

- fadt_boot_arch 0="legacy free" which while reasonable, probably isn't
what will be wanted by most mainboards, so this should generally get
updated in the specific devicetree.
- In fadt_flags all chipset-specific flags get set while the mainboard
has to set all other flags that it needs to have set.

This patch changes the default for fadt_boot_arch.

Change-Id: I6e8d0c60cadfdd24b6926703b252abbc56d436de
Signed-off-by: Martin Roth 
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43418
Tested-by: build bot (Jenkins) 
Reviewed-by: Raul Rangel 
Reviewed-by: Angel Pons 
---
M src/soc/amd/picasso/acpi.c
1 file changed, 12 insertions(+), 10 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Raul Rangel: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c
index da6bc94..1b9c0ca 100644
--- a/src/soc/amd/picasso/acpi.c
+++ b/src/soc/amd/picasso/acpi.c
@@ -88,6 +88,8 @@
  */
 void acpi_fill_fadt(acpi_fadt_t *fadt)
 {
+	const struct soc_amd_picasso_config *cfg = config_of_soc();
+
    printk(BIOS_DEBUG, "pm_base: 0x%04x\n", PICASSO_ACPI_IO_BASE);

fadt->sci_int = 9;		/* IRQ 09 - ACPI SCI */
@@ -115,17 +117,17 @@
    fadt->day_alrm = 0x0d;
    fadt->mon_alrm = 0;
    fadt->century = 0x32;
-	fadt->iapc_boot_arch = ACPI_FADT_LEGACY_DEVICES | ACPI_FADT_8042;
+	fadt->iapc_boot_arch = cfg->fadt_boot_arch; /* legacy free default */
    fadt->res2 = 0;		/* reserved, MUST be 0 ACPI 3.0 */
-	fadt->flags |= ACPI_FADT_WBINVD | /* See table 5-10 ACPI 3.0a spec */
-				ACPI_FADT_C1_SUPPORTED |
-				ACPI_FADT_SLEEP_BUTTON |
-				ACPI_FADT_S4_RTC_WAKE |
-				ACPI_FADT_32BIT_TIMER |
-				ACPI_FADT_PCI_EXPRESS_WAKE |
-				ACPI_FADT_PLATFORM_CLOCK |
-				ACPI_FADT_S4_RTC_VALID |
-				ACPI_FADT_REMOTE_POWER_ON;
+	fadt->flags |=	ACPI_FADT_WBINVD | /* See table 5-34 ACPI 6.3 spec */
+			ACPI_FADT_C1_SUPPORTED |
+			ACPI_FADT_S4_RTC_WAKE |
+			ACPI_FADT_32BIT_TIMER |
+			ACPI_FADT_PCI_EXPRESS_WAKE |
+			ACPI_FADT_PLATFORM_CLOCK |
+			ACPI_FADT_S4_RTC_VALID |
+			ACPI_FADT_REMOTE_POWER_ON;
+	fadt->flags |= cfg->fadt_flags; /* additional board-specific flags */

fadt->ARM_boot_arch = 0;	/* MUST be 0 ACPI 3.0 */
    fadt->FADT_MinorVersion = 0;	/* MUST be 0 ACPI 3.0 */