Change in coreboot[master]: soc/amd/common/lpc: Add decode disable function

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35270 )

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

soc/amd/common/lpc: Add decode disable function

It is already trivial to set D14F3x44 to 0, but add a function to wipe
both that and the settings in D14F3x48, along with x48's associated
addresses.

Change-Id: Ibec25562b2a1568681aea7caf86f00094c436a50
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/lpc_util.c
2 files changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35270/1

diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h
index 6b6745d..0f4f616 100644
--- a/src/soc/amd/common/block/include/amdblocks/lpc.h
+++ b/src/soc/amd/common/block/include/amdblocks/lpc.h
@@ -68,6 +68,7 @@
 #define   DECODE_IO_PORT_ENABLE2	BIT(18)
 #define   DECODE_IO_PORT_ENABLE1	BIT(17)
 #define   DECODE_IO_PORT_ENABLE0	BIT(16)
+#define   LPC_SYNC_TIMEOUT_COUNT_MASK	0xff
 #define   LPC_SYNC_TIMEOUT_COUNT_ENABLE	BIT(7)
 #define   LPC_DECODE_RTC_IO_ENABLE	BIT(6)
 #define   DECODE_MEM_PORT_ENABLE0	BIT(5)
@@ -134,6 +135,7 @@
 #define   PREFETCH_EN_SPI_FROM_HOST	BIT(0)
 #define   T_START_ENH			BIT(3)

+void lpc_disable_decodes(void);
 /* LPC is typically enabled very early, but this function is last opportunity */
 void soc_late_lpc_bridge_enable(void);
 void lpc_enable_port80(void);
diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c
index 008d14c..485eeda 100644
--- a/src/soc/amd/common/block/lpc/lpc_util.c
+++ b/src/soc/amd/common/block/lpc/lpc_util.c
@@ -170,6 +170,21 @@
    pci_write_config32(_LPCB_DEV, LPC_IO_PORT_DECODE_ENABLE, decodes);
 }

+void lpc_disable_decodes(void)
+{
+	uint32_t reg;
+
+	lpc_enable_decode(0);
+	reg = pci_read_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE);
+	reg &= LPC_SYNC_TIMEOUT_COUNT_MASK + LPC_SYNC_TIMEOUT_COUNT_ENABLE;
+	pci_write_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE, reg);
+
+	/* D14F3x48 enables ranges configured in additional registers */
+	pci_write_config32(_LPCB_DEV, LPC_MEM_PORT1, 0);
+	pci_write_config32(_LPCB_DEV, LPC_MEM_PORT0, 0);
+	pci_write_config32(_LPCB_DEV, LPC_WIDEIO2_GENERIC_PORT, 0);
+}
+
 uintptr_t lpc_spibase(void)
 {
    u32 base, enables;

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
PS1, Line 179: 	reg &= LPC_SYNC_TIMEOUT_COUNT_MASK + LPC_SYNC_TIMEOUT_COUNT_ENABLE;
This looks weird. If I got it right, the end value to be AND is 0x00000017 Is this really what you want?

[Marshall Dawson (Code Review)]

Hello Richard Spiegel, build bot (Jenkins), Martin Roth,

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

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

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

soc/amd/common/lpc: Add decode disable function

It is already trivial to set D14F3x44 to 0, but add a function to wipe
both that and the settings in D14F3x48, along with x48's associated
addresses.

Change-Id: Ibec25562b2a1568681aea7caf86f00094c436a50
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/lpc_util.c
2 files changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35270/2

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
PS1, Line 179: 	reg &= LPC_SYNC_TIMEOUT_COUNT_MASK + LPC_SYNC_TIMEOUT_COUNT_ENABLE;
> This looks weird. […]

I goofed the mask; see the update to lpc.h.  This line is to clear the register contents while preserving the count and enable.  So AND with 0000ff80.

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
PS2, Line 179: +
Now it makes more sense, though also this should really be an OR instead of a PLUS. Though the end result is the same, it makes for clear understanding of your intention.

[Marshall Dawson (Code Review)]

Hello Richard Spiegel, build bot (Jenkins), Martin Roth,

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

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

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

soc/amd/common/lpc: Add decode disable function

It is already trivial to set D14F3x44 to 0, but add a function to wipe
both that and the settings in D14F3x48, along with x48's associated
addresses.

Change-Id: Ibec25562b2a1568681aea7caf86f00094c436a50
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/lpc_util.c
2 files changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35270/3

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
PS2, Line 179: +
> Now it makes more sense, though also this should really be an OR instead of a PLUS. […]

Done

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35270 )

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
PS1, Line 173: void lpc_disable_decodes(void)
what does it do? Please add a comment.

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/1/src/soc/amd/common/block/lp... 
PS1, Line 173: void lpc_disable_decodes(void)
> what does it do? Please add a comment.

Done

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
PS2, Line 179: +
> Now it makes more sense, though also this should really be an OR instead of a PLUS. […]

It seems like some other reviews can tend toward + at times, but I'm OK either way.  Certainly with values containing "COUNT", arithmetic look weird.

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
PS2, Line 179: +
> It seems like some other reviews can tend toward + at times, but I'm OK either way. […]

To me it's more that we are talking bits, and the results are different if the same bit is added twice or OR-ed twice

[Marshall Dawson (Code Review)]

Hello Richard Spiegel, build bot (Jenkins), Martin Roth,

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

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

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

soc/amd/common/lpc: Add decode disable function

It is already trivial to set D14F3x44 to 0, but add a function to wipe
both that and the settings in D14F3x48, along with x48's associated
addresses.

Change-Id: Ibec25562b2a1568681aea7caf86f00094c436a50
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/lpc_util.c
2 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/35270/5

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
File src/soc/amd/common/block/lpc/lpc_util.c:

https://review.coreboot.org/c/coreboot/+/35270/2/src/soc/amd/common/block/lp... 
PS2, Line 179: +
> To me it's more that we are talking bits, and the results are different if the same bit is added twi […]

Oops, looks like I didn't get my online correction made locally, so I overwrote it with last night's update.  I didn't intend to revert it.

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

Patch Set 5: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35270 )

Change subject: soc/amd/common/lpc: Add decode disable function
......................................................................

soc/amd/common/lpc: Add decode disable function

It is already trivial to set D14F3x44 to 0, but add a function to wipe
both that and the settings in D14F3x48, along with x48's associated
addresses.

Change-Id: Ibec25562b2a1568681aea7caf86f00094c436a50
Signed-off-by: Marshall Dawson 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35270
Tested-by: build bot (Jenkins) 
Reviewed-by: Richard Spiegel 
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/lpc_util.c
2 files changed, 23 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h
index 6b6745d..f956ba3 100644
--- a/src/soc/amd/common/block/include/amdblocks/lpc.h
+++ b/src/soc/amd/common/block/include/amdblocks/lpc.h
@@ -68,6 +68,7 @@
 #define   DECODE_IO_PORT_ENABLE2	BIT(18)
 #define   DECODE_IO_PORT_ENABLE1	BIT(17)
 #define   DECODE_IO_PORT_ENABLE0	BIT(16)
+#define   LPC_SYNC_TIMEOUT_COUNT_MASK	(0xff << 8)
 #define   LPC_SYNC_TIMEOUT_COUNT_ENABLE	BIT(7)
 #define   LPC_DECODE_RTC_IO_ENABLE	BIT(6)
 #define   DECODE_MEM_PORT_ENABLE0	BIT(5)
@@ -134,6 +135,9 @@
 #define   PREFETCH_EN_SPI_FROM_HOST	BIT(0)
 #define   T_START_ENH			BIT(3)

+/* Clear all decoding to the LPC bus and erase any range registers associated
+ * with the enable bits. */
+void lpc_disable_decodes(void);
 /* LPC is typically enabled very early, but this function is last opportunity */
 void soc_late_lpc_bridge_enable(void);
 void lpc_enable_port80(void);
diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c
index 008d14c..1d46acb 100644
--- a/src/soc/amd/common/block/lpc/lpc_util.c
+++ b/src/soc/amd/common/block/lpc/lpc_util.c
@@ -170,6 +170,25 @@
    pci_write_config32(_LPCB_DEV, LPC_IO_PORT_DECODE_ENABLE, decodes);
 }

+/*
+ * Clear all decoding to the LPC bus and erase any range registers associated
+ * with the enable bits.
+ */
+void lpc_disable_decodes(void)
+{
+	uint32_t reg;
+
+	lpc_enable_decode(0);
+	reg = pci_read_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE);
+	reg &= LPC_SYNC_TIMEOUT_COUNT_MASK | LPC_SYNC_TIMEOUT_COUNT_ENABLE;
+	pci_write_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE, reg);
+
+	/* D14F3x48 enables ranges configured in additional registers */
+	pci_write_config32(_LPCB_DEV, LPC_MEM_PORT1, 0);
+	pci_write_config32(_LPCB_DEV, LPC_MEM_PORT0, 0);
+	pci_write_config32(_LPCB_DEV, LPC_WIDEIO2_GENERIC_PORT, 0);
+}
+
 uintptr_t lpc_spibase(void)
 {
    u32 base, enables;