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

[Marshall Dawson (Code Review)]

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

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

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

The LPC-ISA bridge supports two ranges for SuperIO control registers.
Add a generic function to allow a mainboard to enable the appropriate
range.  Provide #define values that are more descriptive than the
register's field names.

Change-Id: Ic5445cfc137604cb1bb3ee3ea4c3a4ebdb9a9cab
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/71/35271/1

diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h
index 0f4f616..4b20ba7 100644
--- a/src/soc/amd/common/block/include/amdblocks/lpc.h
+++ b/src/soc/amd/common/block/include/amdblocks/lpc.h
@@ -75,6 +75,8 @@
 #define   LPC_WIDEIO0_ENABLE		BIT(2)
 #define   DECODE_ALTERNATE_SIO_ENABLE	BIT(1)
 #define   DECODE_SIO_ENABLE		BIT(0)
+#define     DECODE_SIO_4E4F		1
+#define     DECODE_SIO_2E2F		0
 #define   WIDEIO_RANGE_ERROR		-1

/* Assuming word access to higher word (register 0x4a) */
@@ -141,6 +143,8 @@
 void lpc_enable_port80(void);
 void lpc_enable_pci_port80(void);
 void lpc_enable_decode(uint32_t decodes);
+/* base = index/data I/O to enable.  0: 2e/2f, 1: 4e/4f */
+void lpc_enable_sio_decode(int base);
 uintptr_t lpc_spibase(void);
 void lpc_tpm_decode(void);
 void lpc_tpm_decode_spi(void);
diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c
index 485eeda..27a5f4e 100644
--- a/src/soc/amd/common/block/lpc/lpc_util.c
+++ b/src/soc/amd/common/block/lpc/lpc_util.c
@@ -165,6 +165,19 @@
    pci_write_config8(_LPCB_DEV, LPC_IO_OR_MEM_DEC_EN_HIGH, byte);
 }

+/* base = index/data I/O to enable.  0: 2e/2f, 1: 4e/4f */
+void lpc_enable_sio_decode(int base)
+{
+	uint32_t decodes;
+
+	if (base > 1)
+		return;
+
+	decodes = pci_read_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE);
+	decodes |= 1 << base;
+	pci_write_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE, decodes);
+}
+
 void lpc_enable_decode(uint32_t decodes)
 {
    pci_write_config32(_LPCB_DEV, LPC_IO_PORT_DECODE_ENABLE, decodes);

[Richard Spiegel (Code Review)]

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

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/in... 
File src/soc/amd/common/block/include/amdblocks/lpc.h:

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/in... 
PS1, Line 78: #define     DECODE_SIO_4E4F		1
Reading the code is obvious, but not here. Could you please add a comment indicating that these 2 values refer to the bit that needs to be set (from DECODE_ALTERNATE_SIO_ENABLE and DECODE_SIO_ENABLE)? Or maybe (changing the code) use:
DECODE_SIO_BOTH        (DECODE_SIO_ENABLE | DECODE_ALTERNATE_SIO_ENABLE)
DECODE_SIO_4E4f        DECODE_ALTERNATE_SIO_ENABLE
DECODE_SIO_2E2F        DECODE_SIO_ENABLE
DECODE_SIO_MASK        DECODE_SIO_BOTH

[Marshall Dawson (Code Review)]

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

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/in... 
File src/soc/amd/common/block/include/amdblocks/lpc.h:

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/in... 
PS1, Line 78: #define     DECODE_SIO_4E4F		1
> Reading the code is obvious, but not here. […]

I'm OK with redefining the 4e4f and 2e2f values.

I don't see the point in doing both, and given the work for eSPI, I want to start being more judicious about simply turning on as much decoding as possible.  Is there a real-world scenario for that?  And if so, the caller could simply make two calls, or the common code could be updated then.

Similarly, do we need a mask?  It seems like the most common scenario is enabling bits, not disabling.  I have a disable function that clears all LPC decoding, but that's really because the PSP is leaving stuff turned on (at least with the APCB we're currently using).

[Richard Spiegel (Code Review)]

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

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/in... 
File src/soc/amd/common/block/include/amdblocks/lpc.h:

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/in... 
PS1, Line 78: #define     DECODE_SIO_4E4F		1
> I'm OK with redefining the 4e4f and 2e2f values. […]

At least add the comment indicating that these 2 values refer to the bit that needs to be set (from DECODE_ALTERNATE_SIO_ENABLE and DECODE_SIO_ENABLE)

[Patrick Rudolph (Code Review)]

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

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

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35271/1/src/soc/amd/common/block/lp... 
PS1, Line 169: void lpc_enable_sio_decode(int base)
const bool

[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/+/35271

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

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

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

The LPC-ISA bridge supports two ranges for SuperIO control registers.
Add a generic function to allow a mainboard to enable the appropriate
range.  Provide #define values that are more descriptive than the
register's field names.

Change-Id: Ic5445cfc137604cb1bb3ee3ea4c3a4ebdb9a9cab
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(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/35271/2

[Richard Spiegel (Code Review)]

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

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

Patch Set 2:

(5 comments)

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
File src/soc/amd/common/block/include/amdblocks/lpc.h:

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
PS2, Line 78: 1
true

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
PS2, Line 78: #define     LPC_SELECT_SIO_4E4F		1
Add a comment explaining why these 2 defines are Boolean

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
PS2, Line 79: 0
false

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

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/lp... 
PS2, Line 168: addr
Maybe for clarity, is_it_uart1

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/lp... 
PS2, Line 174: LPC_SELECT_SIO_2E2F
Making the input a bool, the check against a value is no longer needed and makes no sense.
enable = addr ? DECODE_SIO_ENABLE : DECODE_ALTERNATE_SIO_ENABLE

[Marshall Dawson (Code Review)]

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

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

Patch Set 3:

(5 comments)

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
File src/soc/amd/common/block/include/amdblocks/lpc.h:

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
PS2, Line 78: #define     LPC_SELECT_SIO_4E4F		1
> Add a comment explaining why these 2 defines are Boolean

I don't feel like a selection from among two addresses lends itself very well to the bool idea of true and false.  I want the mb consumer of this only to consider using the right define, not get caught up in whether it's true or false.

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
PS2, Line 78: 1
> true

Same as above.

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/in... 
PS2, Line 79: 0
> false

Same as above.

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

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/lp... 
PS2, Line 168: addr
> Maybe for clarity, is_it_uart1

I don't think that adds much clarity, given what I have below.  The bool already should enforce effectively only one of two choices.

https://review.coreboot.org/c/coreboot/+/35271/2/src/soc/amd/common/block/lp... 
PS2, Line 174: LPC_SELECT_SIO_2E2F
> Making the input a bool, the check against a value is no longer needed and makes no sense. […]

I don't feel like a selection of addresses lends itself well to true/false.

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35271 )

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

Patch Set 3: Code-Review+2

[Martin Roth (Code Review)]

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

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

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

The LPC-ISA bridge supports two ranges for SuperIO control registers.
Add a generic function to allow a mainboard to enable the appropriate
range.  Provide #define values that are more descriptive than the
register's field names.

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

Approvals:
  build bot (Jenkins): Verified
  Martin Roth: 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 11880eb..2874c18 100644
--- a/src/soc/amd/common/block/include/amdblocks/lpc.h
+++ b/src/soc/amd/common/block/include/amdblocks/lpc.h
@@ -75,6 +75,8 @@
 #define   LPC_WIDEIO0_ENABLE		BIT(2)
 #define   DECODE_ALTERNATE_SIO_ENABLE	BIT(1)
 #define   DECODE_SIO_ENABLE		BIT(0)
+#define     LPC_SELECT_SIO_4E4F		1
+#define     LPC_SELECT_SIO_2E2F		0
 #define   WIDEIO_RANGE_ERROR		-1

/* Assuming word access to higher word (register 0x4a) */
@@ -151,6 +153,8 @@
 void lpc_enable_port80(void);
 void lpc_enable_pci_port80(void);
 void lpc_enable_decode(uint32_t decodes);
+/* addr = index/data to enable:  LPC_SELECT_SIO_2E2F or LPC_SELECT_SIO_4E4F */
+void lpc_enable_sio_decode(const bool addr);
 uintptr_t lpc_spibase(void);
 void lpc_tpm_decode(void);
 void lpc_tpm_decode_spi(void);
diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c
index 1d46acb..cdf36b2 100644
--- a/src/soc/amd/common/block/lpc/lpc_util.c
+++ b/src/soc/amd/common/block/lpc/lpc_util.c
@@ -165,6 +165,18 @@
    pci_write_config8(_LPCB_DEV, LPC_IO_OR_MEM_DEC_EN_HIGH, byte);
 }

+void lpc_enable_sio_decode(const bool addr)
+{
+	uint32_t decodes;
+	uint32_t enable;
+
+	decodes = pci_read_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE);
+	enable = addr == LPC_SELECT_SIO_2E2F ?
+			DECODE_SIO_ENABLE : DECODE_ALTERNATE_SIO_ENABLE;
+	decodes |= enable;
+	pci_write_config32(_LPCB_DEV, LPC_IO_OR_MEM_DECODE_ENABLE, decodes);
+}
+
 void lpc_enable_decode(uint32_t decodes)
 {
    pci_write_config32(_LPCB_DEV, LPC_IO_PORT_DECODE_ENABLE, decodes);