Change in coreboot[master]: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing

[Raul Rangel (Code Review)]

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40951 )

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing

If a Picasso platform wants to use GPIO 67 it must disable ROM sharing.
Otherwise ROM access is incredibly slow.

BUG=b:153502861
TEST=Build trembyle

Signed-off-by: Raul E Rangel 
Change-Id: Ia9ab3803a2f56f68c1164bd241fc3917a3ffcf2b
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/Kconfig
M src/soc/amd/common/block/lpc/lpc_util.c
3 files changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/40951/1

diff --git a/src/soc/amd/common/block/include/amdblocks/lpc.h b/src/soc/amd/common/block/include/amdblocks/lpc.h
index dc33073..1d74823 100644
--- a/src/soc/amd/common/block/include/amdblocks/lpc.h
+++ b/src/soc/amd/common/block/include/amdblocks/lpc.h
@@ -10,6 +10,8 @@
 /* PCI registers for D14F3 */
 #define LPC_PCI_CONTROL			0x40
 #define   LEGACY_DMA_EN			BIT(2)
+#define   VW_ROM_SHARING_EN		BIT(3)
+#define   EXT_ROM_SHARING_EN		BIT(4)

#define LPC_IO_PORT_DECODE_ENABLE	0x44
 #define   DECODE_ENABLE_PARALLEL_PORT0	BIT(0)
@@ -148,6 +150,7 @@
 void lpc_tpm_decode_spi(void);
 void lpc_enable_rom(void);
 void lpc_enable_spi_prefetch(void);
+void lpc_disable_spi_rom_sharing(void);

/**
  * @brief Find the size of a particular wide IO
diff --git a/src/soc/amd/common/block/lpc/Kconfig b/src/soc/amd/common/block/lpc/Kconfig
index b0d59a5..3cfbfe5d 100644
--- a/src/soc/amd/common/block/lpc/Kconfig
+++ b/src/soc/amd/common/block/lpc/Kconfig
@@ -3,3 +3,9 @@
    default n
    help
      Select this option to use the traditional LPC-ISA bridge at D14F3.
+
+config PROVIDES_ROM_SHARING
+	bool
+	default n
+	help
+	  Select this option if the LPC bridge supports ROM sharing.
diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c
index 571c6fe..ce0f473 100644
--- a/src/soc/amd/common/block/lpc/lpc_util.c
+++ b/src/soc/amd/common/block/lpc/lpc_util.c
@@ -300,6 +300,18 @@
    pci_write_config32(_LPCB_DEV, LPC_ROM_DMA_EC_HOST_CONTROL, dword);
 }

+#if CONFIG(PROVIDES_ROM_SHARING)
+void lpc_disable_spi_rom_sharing(void)
+{
+	u8 byte;
+
+	byte = pci_read_config8(_LPCB_DEV, LPC_PCI_CONTROL);
+	byte &= ~VW_ROM_SHARING_EN;
+	byte &= ~EXT_ROM_SHARING_EN;
+	pci_write_config8(_LPCB_DEV, LPC_PCI_CONTROL, byte);
+}
+#endif
+
 uintptr_t lpc_get_spibase(void)
 {
    u32 base;

[Furquan Shaikh (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Patch Set 1: Code-Review+2

[Aaron Durbin (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40951/1/src/soc/amd/common/block/lp... 
PS1, Line 306: 	u8 byte;
I think I'd prefer this:

if (!CONFIG(PROVIDES_ROM_SHARING))
  dead_code();

That way we aren't guarding valid code with macros and we're getting link time errors.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/40951 )

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Removed Code-Review+2 by Furquan Shaikh

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40951/1/src/soc/amd/common/block/lp... 
PS1, Line 311: 	pci_write_config8(_LPCB_DEV, LPC_PCI_CONTROL, byte);
Can `pci_update_config8()` be used?

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,

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

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

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing

If a Picasso platform wants to use GPIO 67 it must disable ROM sharing.
Otherwise ROM access is incredibly slow.

BUG=b:153502861
TEST=Build trembyle

Signed-off-by: Raul E Rangel 
Change-Id: Ia9ab3803a2f56f68c1164bd241fc3917a3ffcf2b
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/Kconfig
M src/soc/amd/common/block/lpc/lpc_util.c
3 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/40951/2

[Raul Rangel (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Uploaded patch set 2.

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/40951/1/src/soc/amd/common/block/lp... 
PS1, Line 306: 	u8 byte;
> I think I'd prefer this: […]

Cool. Didn't know about that.

https://review.coreboot.org/c/coreboot/+/40951/1/src/soc/amd/common/block/lp... 
PS1, Line 311: 	pci_write_config8(_LPCB_DEV, LPC_PCI_CONTROL, byte);
> Can `pci_update_config8()` be used?

It could, though I don't think it's any easier to read.

pci_update_config8(_LPCB_DEV, LPC_PCI_CONTROL,
       ~(VW_ROM_SHARING_EN | EXT_ROM_SHARING_EN), 0);

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40951 )

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Patch Set 2:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/40951/2/src/soc/amd/common/block/lp... 
PS2, Line 309:   		dead_code();
code indent should use tabs where possible

https://review.coreboot.org/c/coreboot/+/40951/2/src/soc/amd/common/block/lp... 
PS2, Line 309:   		dead_code();
please, no space before tabs

https://review.coreboot.org/c/coreboot/+/40951/2/src/soc/amd/common/block/lp... 
PS2, Line 309:   		dead_code();
please, no spaces at the start of a line

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Aaron Durbin,

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

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

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing

If a Picasso platform wants to use GPIO 67 it must disable ROM sharing.
Otherwise ROM access is incredibly slow.

BUG=b:153502861
TEST=Build trembyle

Signed-off-by: Raul E Rangel 
Change-Id: Ia9ab3803a2f56f68c1164bd241fc3917a3ffcf2b
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/Kconfig
M src/soc/amd/common/block/lpc/lpc_util.c
3 files changed, 23 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/40951/3

[Raul Rangel (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Uploaded patch set 3.

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40951/1/src/soc/amd/common/block/lp... 
PS1, Line 311: 	pci_write_config8(_LPCB_DEV, LPC_PCI_CONTROL, byte);
> It could, though I don't think it's any easier to read. […]

Ack

[Furquan Shaikh (Code Review)]

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

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40951 )

Change subject: soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing
......................................................................

soc/amd/common/block/lpc: Add lpc_disable_spi_rom_sharing

If a Picasso platform wants to use GPIO 67 it must disable ROM sharing.
Otherwise ROM access is incredibly slow.

BUG=b:153502861
TEST=Build trembyle

Signed-off-by: Raul E Rangel 
Change-Id: Ia9ab3803a2f56f68c1164bd241fc3917a3ffcf2b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40951
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Menzel 
Reviewed-by: Furquan Shaikh 
---
M src/soc/amd/common/block/include/amdblocks/lpc.h
M src/soc/amd/common/block/lpc/Kconfig
M src/soc/amd/common/block/lpc/lpc_util.c
3 files changed, 23 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Furquan Shaikh: 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 dc33073..1d74823 100644
--- a/src/soc/amd/common/block/include/amdblocks/lpc.h
+++ b/src/soc/amd/common/block/include/amdblocks/lpc.h
@@ -10,6 +10,8 @@
 /* PCI registers for D14F3 */
 #define LPC_PCI_CONTROL			0x40
 #define   LEGACY_DMA_EN			BIT(2)
+#define   VW_ROM_SHARING_EN		BIT(3)
+#define   EXT_ROM_SHARING_EN		BIT(4)

#define LPC_IO_PORT_DECODE_ENABLE	0x44
 #define   DECODE_ENABLE_PARALLEL_PORT0	BIT(0)
@@ -148,6 +150,7 @@
 void lpc_tpm_decode_spi(void);
 void lpc_enable_rom(void);
 void lpc_enable_spi_prefetch(void);
+void lpc_disable_spi_rom_sharing(void);

/**
  * @brief Find the size of a particular wide IO
diff --git a/src/soc/amd/common/block/lpc/Kconfig b/src/soc/amd/common/block/lpc/Kconfig
index b0d59a5..3cfbfe5d 100644
--- a/src/soc/amd/common/block/lpc/Kconfig
+++ b/src/soc/amd/common/block/lpc/Kconfig
@@ -3,3 +3,9 @@
    default n
    help
      Select this option to use the traditional LPC-ISA bridge at D14F3.
+
+config PROVIDES_ROM_SHARING
+	bool
+	default n
+	help
+	  Select this option if the LPC bridge supports ROM sharing.
diff --git a/src/soc/amd/common/block/lpc/lpc_util.c b/src/soc/amd/common/block/lpc/lpc_util.c
index 571c6fe..45b252f 100644
--- a/src/soc/amd/common/block/lpc/lpc_util.c
+++ b/src/soc/amd/common/block/lpc/lpc_util.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /* This file is part of the coreboot project. */

+#include <assert.h>
 #include <stdint.h>
 #include <device/device.h>
 #include <device/pci_ops.h>
@@ -300,6 +301,19 @@
    pci_write_config32(_LPCB_DEV, LPC_ROM_DMA_EC_HOST_CONTROL, dword);
 }

+void lpc_disable_spi_rom_sharing(void)
+{
+	u8 byte;
+
+	if (!CONFIG(PROVIDES_ROM_SHARING))
+		dead_code();
+
+	byte = pci_read_config8(_LPCB_DEV, LPC_PCI_CONTROL);
+	byte &= ~VW_ROM_SHARING_EN;
+	byte &= ~EXT_ROM_SHARING_EN;
+	pci_write_config8(_LPCB_DEV, LPC_PCI_CONTROL, byte);
+}
+
 uintptr_t lpc_get_spibase(void)
 {
    u32 base;