Change in ...coreboot[master]: soc/amd/stoneyridge: Correct bugs in lpc.c

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

soc/amd/stoneyridge: Correct bugs in lpc.c

Remove the bridge enable step of accessing D14F0x64.  This method for
enabling the bridge appears to be last present in the SB700 device.
Beginning in the SB800 (and all FCH, SoC devices), the enable is in
PMxEC[0].  Since the bridge is enabled in bootblock to allow port 80h,
there is no need to maintain it in ramstage.

Correct the device used for misc. configuration of the LPC bridge.
The #defined value removed is 14.0 but the settings are in 14.3.

TEST=Boot Grunt, check console and dmesg for errors and warnings
BUG=b:131862871

Change-Id: I078be974dc3c78c94cb7c0832518f21bac029ff2
Signed-off-by: Marshall Dawson 
---
M src/soc/amd/stoneyridge/lpc.c
1 file changed, 13 insertions(+), 26 deletions(-)

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

diff --git a/src/soc/amd/stoneyridge/lpc.c b/src/soc/amd/stoneyridge/lpc.c
index 9a8c8ef..a838146 100644
--- a/src/soc/amd/stoneyridge/lpc.c
+++ b/src/soc/amd/stoneyridge/lpc.c
@@ -35,31 +35,22 @@
 static void lpc_init(struct device *dev)
 {
    u8 byte;
-	u32 dword;
-
-	/*
-	 * Enable the LPC Controller
-	 * SMBus register 0x64 is not defined in public datasheet.
-	 */
-	dword = pci_read_config32(SOC_SMBUS_DEV, 0x64);
-	dword |= 1 << 20;
-	pci_write_config32(SOC_SMBUS_DEV, 0x64, dword);

/* Initialize isa dma */
    isa_dma_init();

/* Enable DMA transaction on the LPC bus */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_PCI_CONTROL);
+	byte = pci_read_config8(dev, LPC_PCI_CONTROL);
    byte |= LEGACY_DMA_EN;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_PCI_CONTROL, byte);
+	pci_write_config8(dev, LPC_PCI_CONTROL, byte);

/* Disable the timeout mechanism on LPC */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_IO_OR_MEM_DECODE_ENABLE);
+	byte = pci_read_config8(dev, LPC_IO_OR_MEM_DECODE_ENABLE);
    byte &= ~LPC_SYNC_TIMEOUT_COUNT_ENABLE;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_IO_OR_MEM_DECODE_ENABLE, byte);
+	pci_write_config8(dev, LPC_IO_OR_MEM_DECODE_ENABLE, byte);

/* Disable LPC MSI Capability */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_MISC_CONTROL_BITS);
+	byte = pci_read_config8(dev, LPC_MISC_CONTROL_BITS);
    /* BIT 1 is not defined in public datasheet. */
    byte &= ~(1 << 1);

@@ -69,15 +60,15 @@
     * interrupt and visit LPC.
     */
    byte &= ~LPC_NOHOG;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_MISC_CONTROL_BITS, byte);
+	pci_write_config8(dev, LPC_MISC_CONTROL_BITS, byte);

/*
     * Enable hand-instance of the pulse generator and SPI
     * controller prefetch of flash.
     */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_HOST_CONTROL);
+	byte = pci_read_config8(dev, LPC_HOST_CONTROL);
    byte |= PREFETCH_EN_SPI_FROM_HOST | T_START_ENH;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_HOST_CONTROL, byte);
+	pci_write_config8(dev, LPC_HOST_CONTROL, byte);

cmos_check_update_date();

@@ -161,9 +152,8 @@
    pci_dev_set_resources(dev);
 }

-static void set_child_resource(struct device *child,
-				u32 *reg,
-				u32 *reg_x)
+static void set_child_resource(struct device *dev, struct device *child,
+				u32 *reg, u32 *reg_x)
 {
    struct resource *res;
    u32 base, end;
@@ -272,7 +262,7 @@
    		wideio_index = sb_set_wideio_range(base, res->size);
    		if (wideio_index != WIDEIO_RANGE_ERROR) {
    			/* preserve wide IO related bits. */
-				*reg_x = pci_read_config32(SOC_LPC_DEV,
+				*reg_x = pci_read_config32(dev,
    				LPC_IO_OR_MEM_DECODE_ENABLE);

printk(BIOS_DEBUG,
@@ -308,11 +298,8 @@
    	for (child = link->children; child;
    	     child = child->sibling) {
    		if (child->enabled
-			    && (child->path.type == DEVICE_PATH_PNP)) {
-				set_child_resource(child,
-						&reg,
-						&reg_x);
-			}
+			    && (child->path.type == DEVICE_PATH_PNP))
+				set_child_resource(dev, child, &reg, &reg_x);
    	}
    }
    pci_write_config32(dev, LPC_IO_PORT_DECODE_ENABLE, reg);

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

Patch Set 1:

Comparing against Hudson code, I do agree with your changes. I wander why no side effect was found before. Maybe some of the code (with exception of SPI prefetch) could simply be removed (modern OS would re-initialize them as needed)?

I'll await an answer, otherwise I would approve it (if you say leave it there, I'll accept and approve).

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

Patch Set 1:

SPI prefetch is set up in bootblock already.  My guess is we _probably_ haven't seen any side effects -- the EC is the only device on LPC and it may not be trying to take any actions that would be a problem.  I don't believe an OS would reinitialize these items.

[Martin Roth (Code Review)]

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

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32551/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32551/1//COMMIT_MSG@12 
PS1, Line 12: Since the bridge is enabled in bootblock to allow port 80h,
            : there is no need to maintain it in ramstage.
I'm ok with that for Stoney, although adding a comment to that effect would probably be good.
For Picasso, are we just going to assume that the ABL has already enabled it?

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32551/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32551/1//COMMIT_MSG@12 
PS1, Line 12: Since the bridge is enabled in bootblock to allow port 80h,
            : there is no need to maintain it in ramstage.
> I'm ok with that for Stoney, although adding a comment to that effect would probably be good.

I'm in the process of relocating this file to common/blocks, and intend to add an empty weak late_lpc_bridge_enable() with comment.  Since the method for enabling has shifted over time, that seems like the best approach to me.
> For Picasso, are we just going to assume that the ABL has already enabled it?

We should consider that -- seems like a reasonable assumption it'll already be enabled, but I'll need to verify.  It looks like I'd left it like ST, enabled in sb_lpc_port80().

[Richard Spiegel (Code Review)]

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

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

Patch Set 1: Code-Review+2

I see, so this is just 1 step on something much bigger. By the way, I tested your changes (including the SPI prefetch). It boots fine, and as you predicted, the boot time did not change.

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32551 )

Change subject: soc/amd/stoneyridge: Correct bugs in lpc.c
......................................................................

soc/amd/stoneyridge: Correct bugs in lpc.c

Remove the bridge enable step of accessing D14F0x64.  This method for
enabling the bridge appears to be last present in the SB700 device.
Beginning in the SB800 (and all FCH, SoC devices), the enable is in
PMxEC[0].  Since the bridge is enabled in bootblock to allow port 80h,
there is no need to maintain it in ramstage.

Correct the device used for misc. configuration of the LPC bridge.
The #defined value removed is 14.0 but the settings are in 14.3.

TEST=Boot Grunt, check console and dmesg for errors and warnings
BUG=b:131862871

Change-Id: I078be974dc3c78c94cb7c0832518f21bac029ff2
Signed-off-by: Marshall Dawson 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32551
Tested-by: build bot (Jenkins) 
Reviewed-by: Richard Spiegel 
---
M src/soc/amd/stoneyridge/lpc.c
1 file changed, 13 insertions(+), 26 deletions(-)

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

diff --git a/src/soc/amd/stoneyridge/lpc.c b/src/soc/amd/stoneyridge/lpc.c
index 9a8c8ef..a838146 100644
--- a/src/soc/amd/stoneyridge/lpc.c
+++ b/src/soc/amd/stoneyridge/lpc.c
@@ -35,31 +35,22 @@
 static void lpc_init(struct device *dev)
 {
    u8 byte;
-	u32 dword;
-
-	/*
-	 * Enable the LPC Controller
-	 * SMBus register 0x64 is not defined in public datasheet.
-	 */
-	dword = pci_read_config32(SOC_SMBUS_DEV, 0x64);
-	dword |= 1 << 20;
-	pci_write_config32(SOC_SMBUS_DEV, 0x64, dword);

/* Initialize isa dma */
    isa_dma_init();

/* Enable DMA transaction on the LPC bus */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_PCI_CONTROL);
+	byte = pci_read_config8(dev, LPC_PCI_CONTROL);
    byte |= LEGACY_DMA_EN;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_PCI_CONTROL, byte);
+	pci_write_config8(dev, LPC_PCI_CONTROL, byte);

/* Disable the timeout mechanism on LPC */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_IO_OR_MEM_DECODE_ENABLE);
+	byte = pci_read_config8(dev, LPC_IO_OR_MEM_DECODE_ENABLE);
    byte &= ~LPC_SYNC_TIMEOUT_COUNT_ENABLE;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_IO_OR_MEM_DECODE_ENABLE, byte);
+	pci_write_config8(dev, LPC_IO_OR_MEM_DECODE_ENABLE, byte);

/* Disable LPC MSI Capability */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_MISC_CONTROL_BITS);
+	byte = pci_read_config8(dev, LPC_MISC_CONTROL_BITS);
    /* BIT 1 is not defined in public datasheet. */
    byte &= ~(1 << 1);

@@ -69,15 +60,15 @@
     * interrupt and visit LPC.
     */
    byte &= ~LPC_NOHOG;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_MISC_CONTROL_BITS, byte);
+	pci_write_config8(dev, LPC_MISC_CONTROL_BITS, byte);

/*
     * Enable hand-instance of the pulse generator and SPI
     * controller prefetch of flash.
     */
-	byte = pci_read_config8(SOC_SMBUS_DEV, LPC_HOST_CONTROL);
+	byte = pci_read_config8(dev, LPC_HOST_CONTROL);
    byte |= PREFETCH_EN_SPI_FROM_HOST | T_START_ENH;
-	pci_write_config8(SOC_SMBUS_DEV, LPC_HOST_CONTROL, byte);
+	pci_write_config8(dev, LPC_HOST_CONTROL, byte);

cmos_check_update_date();

@@ -161,9 +152,8 @@
    pci_dev_set_resources(dev);
 }

-static void set_child_resource(struct device *child,
-				u32 *reg,
-				u32 *reg_x)
+static void set_child_resource(struct device *dev, struct device *child,
+				u32 *reg, u32 *reg_x)
 {
    struct resource *res;
    u32 base, end;
@@ -272,7 +262,7 @@
    		wideio_index = sb_set_wideio_range(base, res->size);
    		if (wideio_index != WIDEIO_RANGE_ERROR) {
    			/* preserve wide IO related bits. */
-				*reg_x = pci_read_config32(SOC_LPC_DEV,
+				*reg_x = pci_read_config32(dev,
    				LPC_IO_OR_MEM_DECODE_ENABLE);

printk(BIOS_DEBUG,
@@ -308,11 +298,8 @@
    	for (child = link->children; child;
    	     child = child->sibling) {
    		if (child->enabled
-			    && (child->path.type == DEVICE_PATH_PNP)) {
-				set_child_resource(child,
-						&reg,
-						&reg_x);
-			}
+			    && (child->path.type == DEVICE_PATH_PNP))
+				set_child_resource(dev, child, &reg, &reg_x);
    	}
    }
    pci_write_config32(dev, LPC_IO_PORT_DECODE_ENABLE, reg);