Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AH2ALB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AH2ALB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AH2ALB clocks

Gate the AH2ALB clocks to save power. This was previously
performed within the AGESA FSP but this change relocates it
into coreboot.

BUG=b:154144239
TEST=Check AH2ALB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/southbridge.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/1

diff --git a/src/soc/amd/picasso/include/soc/southbridge.h b/src/soc/amd/picasso/include/soc/southbridge.h
index e9f7e2e..191904f 100644
--- a/src/soc/amd/picasso/include/soc/southbridge.h
+++ b/src/soc/amd/picasso/include/soc/southbridge.h
@@ -205,6 +205,13 @@
 #define FCH_AOAC_DEV_UART3		26
 #define FCH_AOAC_DEV_ESPI		27

+/* FCH AH2ALB Registers */
+#define AH2ALB_BASE			0xFEDC0000ul
+#define AH2ALB_CONTROL_CLK_OFFSET	0x10
+#define   AH2ALB_CLK_GATE_EN		BIT(1)
+#define AH2ALB_CONTROL_HCLK_OFFSET	0x30
+#define   AH2ALB_HCLK_GATE_EN		BIT(1)
+
 /* Bit definitions for Device D3 Control AOACx0000[40...7E] step 2 */
 #define   FCH_AOAC_TARGET_DEVICE_STATE (BIT(0) + BIT(1))
 #define     FCH_AOAC_D0_UNINITIALIZED	0
diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c
index 45f66d6..d222e9d 100644
--- a/src/soc/amd/picasso/southbridge.c
+++ b/src/soc/amd/picasso/southbridge.c
@@ -328,11 +328,24 @@

BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, set_nvs_sws, NULL);

+static void ah2alb_clock_gate(void)
+{
+	uint8_t ah2alb_val;
+
+	ah2alb_val = read8((void *)(AH2ALB_BASE + AH2ALB_CONTROL_CLK_OFFSET));
+	ah2alb_val |= AH2ALB_CLK_GATE_EN;
+	write8((void *)(AH2ALB_BASE + AH2ALB_CONTROL_CLK_OFFSET), ah2alb_val);
+	ah2alb_val = read8((void *)(AH2ALB_BASE + AH2ALB_CONTROL_HCLK_OFFSET));
+	ah2alb_val |= AH2ALB_HCLK_GATE_EN;
+	write8((void *)(AH2ALB_BASE + AH2ALB_CONTROL_HCLK_OFFSET), ah2alb_val);
+}
+
 void southbridge_init(void *chip_info)
 {
    i2c_soc_init();
    sb_init_acpi_ports();
    acpi_clear_pm1_status();
+	ah2alb_clock_gate();
 }

static void set_sb_final_nvs(void)

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AH2ALB clocks
......................................................................

Patch Set 1:

I am hoping this is the right place to put something like this

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AH2ALB clocks
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/42829/1//COMMIT_MSG@9 
PS1, Line 9: AH2ALB
What is this block? And should we always unconditionally gate the clocks?

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AH2ALB clocks
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/42829/1//COMMIT_MSG@9 
PS1, Line 9: AH2ALB
> What is this block? And should we always unconditionally gate the clocks?

A-Link to AHB bridge, part of the AMBA fabric.

Looks like this was spelled different in the FSP vs the PPR. I will fix to clarify as we should follow the PPR.

My understanding is these are always gated in Raven/Picasso.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the AL2AHB clocks to save power. This was previously
performed within the AGESA FSP but this change relocates it
into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/southbridge.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/2

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/42829/1//COMMIT_MSG@9 
PS1, Line 9: AH2ALB
> A-Link to AHB bridge, part of the AMBA fabric. […]

OK. Can you please add that information to the commit description as well as a comment on the function so there's more context?

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the A-Link to AHB Bridge clocks to save power. These
are internal clocks part and are unneeded for Raven/Picasso.
This was previously performed within the AGESA FSP but this
change relocates it into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/southbridge.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/3

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/42829/1//COMMIT_MSG@9 
PS1, Line 9: AH2ALB
> OK. […]

Sure, will do that in future changes too.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the A-Link to AHB Bridge clocks to save power. These
are internal clocks and are unneeded for Raven/Picasso.
This was previously performed within the AGESA FSP but this
change relocates it into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/southbridge.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/4

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
PS4, Line 339: 	al2ahb_val = read8((void *)(AL2AHB_BASE + AL2AHB_CONTROL_CLK_OFFSET));
please cast through uintptr_t

You probably wanta  local variable dealing with the base to use to add offsets.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/southbridge.h:

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/include... 
PS4, Line 209: 0xFEDC0000ul
nit, lower case.

Oh, this is already defined in iomap.h as ALINK_AHB_ADDRESS.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the A-Link to AHB Bridge clocks to save power. These
are internal clocks and are unneeded for Raven/Picasso.
This was previously performed within the AGESA FSP but this
change relocates it into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/southbridge.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/5

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Marshall Dawson, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the A-Link to AHB Bridge clocks to save power. These
are internal clocks and are unneeded for Raven/Picasso.
This was previously performed within the AGESA FSP but this
change relocates it into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/6

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
PS4, Line 339: 	al2ahb_val = read8((void *)(AL2AHB_BASE + AL2AHB_CONTROL_CLK_OFFSET));
> please cast through uintptr_t […]

Check the latest patch to make sure I did it right. Is this part of C99? Unfortunately there was no equivalent I knew of when working with UEFI.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Raul Rangel (Code Review)]

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/southbridge.h:

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/include... 
PS4, Line 209: 0xFEDC0000ul
> nit, lower case. […]

Done

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
PS4, Line 339: 	al2ahb_val = read8((void *)(AL2AHB_BASE + AL2AHB_CONTROL_CLK_OFFSET));
> Check the latest patch to make sure I did it right. […]

It's fine now. In short it's an issue of literals defaulting to ints in C. It's to handle lp64  (longs pointers are 64-bit) vs ilp32 (integers longs pointers are 32-bit). For forward pointing code uintptr_t is the size of a pointer for unsigned int. Therefore it will work regardless of pointer size.

UEFI likes to use UINTN for posix type uintptr_t from my recollection.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/4/src/soc/amd/picasso/southbr... 
PS4, Line 339: 	al2ahb_val = read8((void *)(AL2AHB_BASE + AL2AHB_CONTROL_CLK_OFFSET));
> It's fine now. In short it's an issue of literals defaulting to ints in C. […]

Thanks for the explanation. Makes sense

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
PS6, Line 340: AL2AHB_CONTROL_CLK_OFFSET
Just curious: Is this different than enabling the clock gating via FCH::Misc and FCH:PM as detailed in section 15.1.6.1 of PPR?

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
PS6, Line 340: AL2AHB_CONTROL_CLK_OFFSET
> Just curious: Is this different than enabling the clock gating via FCH::Misc and FCH:PM as detailed  […]

I suspect it is different (hence the change) but I have yet to hear back from the designers. Will send them a reminder today.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
PS6, Line 340: AL2AHB_CONTROL_CLK_OFFSET
> I suspect it is different (hence the change) but I have yet to hear back from the designers. […]

Heard back this AM. Those other clock gating bits are unrelated.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/include... 
PS6, Line 19: #define   AL2AHB_CLK_GATE_EN		BIT(1)
using BIT() needs to have types.h included in this file. I'd prefer (1 << 1) instead of BIT(1) though which is more in line with the coreboot code base

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6: Code-Review+1

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 6:

Matt, you need to resolve all the comments.

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Marshall Dawson, Angel Pons, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the A-Link to AHB Bridge clocks to save power. These
are internal clocks and are unneeded for Raven/Picasso.
This was previously performed within the AGESA FSP but this
change relocates it into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
---
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/42829/7

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Matt Papageorge (Code Review)]

Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/include... 
File src/soc/amd/picasso/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/include... 
PS6, Line 19: #define   AL2AHB_CLK_GATE_EN		BIT(1)
> using BIT() needs to have types.h included in this file. […]

Done

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Felix Held (Code Review)]

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Angel Pons (Code Review)]

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

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
File src/soc/amd/picasso/southbridge.c:

https://review.coreboot.org/c/coreboot/+/42829/6/src/soc/amd/picasso/southbr... 
PS6, Line 340: AL2AHB_CONTROL_CLK_OFFSET
> Heard back this AM. Those other clock gating bits are unrelated.

Thanks Matt!

Change in coreboot[master]: soc/amd/picasso/sb: Gate FCH AL2AHB clocks

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42829 )

Change subject: soc/amd/picasso/sb: Gate FCH AL2AHB clocks
......................................................................

soc/amd/picasso/sb: Gate FCH AL2AHB clocks

Gate the A-Link to AHB Bridge clocks to save power. These
are internal clocks and are unneeded for Raven/Picasso.
This was previously performed within the AGESA FSP but this
change relocates it into coreboot.

BUG=b:154144239
TEST=Check AL2AHB clock gate bits at the end of POST before and
after change with HDT.

Change-Id: Ifcbc144a8769f8ea440cdd560bab146bf5058cf9
Signed-off-by: Matt Papageorge 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42829
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
Reviewed-by: Angel Pons 
Reviewed-by: Furquan Shaikh 
---
M src/soc/amd/picasso/include/soc/iomap.h
M src/soc/amd/picasso/southbridge.c
2 files changed, 23 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/amd/picasso/include/soc/iomap.h b/src/soc/amd/picasso/include/soc/iomap.h
index cb845c4..6b9ad2a 100644
--- a/src/soc/amd/picasso/include/soc/iomap.h
+++ b/src/soc/amd/picasso/include/soc/iomap.h
@@ -12,7 +12,12 @@
 #endif
 #define HPET_BASE_ADDRESS		0xfed00000

+/* FCH AL2AHB Registers */
 #define ALINK_AHB_ADDRESS		0xfedc0000
+#define AL2AHB_CONTROL_CLK_OFFSET	0x10
+#define   AL2AHB_CLK_GATE_EN		(1 << 1)
+#define AL2AHB_CONTROL_HCLK_OFFSET	0x30
+#define   AL2AHB_HCLK_GATE_EN		(1 << 1)

/* Reserved				0xfecd1000-0xfedc3fff */

diff --git a/src/soc/amd/picasso/southbridge.c b/src/soc/amd/picasso/southbridge.c
index cb22195..4cd24dd 100644
--- a/src/soc/amd/picasso/southbridge.c
+++ b/src/soc/amd/picasso/southbridge.c
@@ -328,11 +328,29 @@

BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, set_nvs_sws, NULL);

+/*
+ * A-Link to AHB bridge, part of the AMBA fabric. These are internal clocks
+ * and unneeded for Raven/Picasso so gate them to save power.
+ */
+static void al2ahb_clock_gate(void)
+{
+	uint8_t al2ahb_val;
+	uintptr_t al2ahb_base = ALINK_AHB_ADDRESS;
+
+	al2ahb_val = read8((void *)(al2ahb_base + AL2AHB_CONTROL_CLK_OFFSET));
+	al2ahb_val |= AL2AHB_CLK_GATE_EN;
+	write8((void *)(al2ahb_base + AL2AHB_CONTROL_CLK_OFFSET), al2ahb_val);
+	al2ahb_val = read8((void *)(al2ahb_base + AL2AHB_CONTROL_HCLK_OFFSET));
+	al2ahb_val |= AL2AHB_HCLK_GATE_EN;
+	write8((void *)(al2ahb_base + AL2AHB_CONTROL_HCLK_OFFSET), al2ahb_val);
+}
+
 void southbridge_init(void *chip_info)
 {
    i2c_soc_init();
    sb_init_acpi_ports();
    acpi_clear_pm1_status();
+	al2ahb_clock_gate();
 }

static void set_sb_final_nvs(void)