Change in coreboot[master]: nb/intel/haswell: Set up DMI ASPM

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43743 )

Change subject: nb/intel/haswell: Set up DMI ASPM
......................................................................

nb/intel/haswell: Set up DMI ASPM

Tested on Asrock B85M Pro4, still boots.

Change-Id: Ie97ff56eec9f928cfd2d5d43a287f3e0d2fbf3cf
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/haswell/northbridge.c
1 file changed, 38 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/43743/1

diff --git a/src/northbridge/intel/haswell/northbridge.c b/src/northbridge/intel/haswell/northbridge.c
index ef7742e..4311710 100644
--- a/src/northbridge/intel/haswell/northbridge.c
+++ b/src/northbridge/intel/haswell/northbridge.c
@@ -418,10 +418,48 @@
    pci_write_config32(host_dev, DEVEN, deven);
 }

+static void northbridge_dmi_init(void)
+{
+	u32 reg32;
+
+	/* Steps prior to DMI ASPM */
+	reg32 = DMIBAR32(0x98);
+	reg32 |= (1 << 6);
+	DMIBAR32(0x98) = reg32;
+
+	reg32 = DMIBAR32(0x22c);
+	reg32 |= (1 << 31);
+	DMIBAR32(0x22c) = reg32;
+
+	reg32 = DMIBAR32(0x238);
+	reg32 |= (1 << 29);
+	DMIBAR32(0x238) = reg32;
+
+	reg32 = DMIBAR32(0xc28);
+	reg32 &= ~0x1f;
+	reg32 |= 0x13;
+	DMIBAR32(0xc28) = reg32;
+
+	/* Clear error status bits */
+	DMIBAR32(0x1c4) = 0xffffffff;
+	DMIBAR32(0x1d0) = 0xffffffff;
+
+	/* Enable ASPM on SA link, should happen before PCH link */
+	reg32 = DMIBAR32(0x88);
+	reg32 |= (1 << 1) | (1 << 0);
+	DMIBAR32(0x88) = reg32;
+
+	/* Enable DMI IOT */
+	DMIBAR8(0xd34) = 0x44;
+}
+
 static void northbridge_init(struct device *dev)
 {
    u8 bios_reset_cpl, pair;

+	if (!CONFIG(INTEL_LYNXPOINT_LP))
+		northbridge_dmi_init();
+
    /* Enable Power Aware Interrupt Routing. */
    pair = MCHBAR8(INTRDIRCTL);
    pair &= ~0x7;	/* Clear 2:0 */

[Paul Menzel (Code Review)]

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

Change subject: nb/intel/haswell: Set up DMI ASPM
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/43743/1//COMMIT_MSG@8 
PS1, Line 8: 
Can you please elaborate a little, what DMI ASPM is.

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Set up DMI ASPM
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/43743/1//COMMIT_MSG@8 
PS1, Line 8:
> Can you please elaborate a little, what DMI ASPM is.

DMI stands for `Direct Media Interface`, which is the PCIe-like link between the SA (System Agent, aka Northbridge) and the PCH.

ASPM means `Active State Power Management`, and it's a PCIe concept. DMI also has it.

On Haswell-H (e.g. Asrock B85M Pro4) we need to enable ASPM, because we enable it on the PCH but not on the SA. And this is inconsistent (both ends should have the same settings). I have to check if we need to do this on Haswell-LP or not, though.

Given the context (Intel Haswell), the DMI and ASPM acronyms should be clear enough. However, I will clarify the commit summary and message, to explicitly state that we are enabling DMI ASPM, and *why* we are doing so.

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Set up DMI ASPM
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43743/1/src/northbridge/intel/haswe... 
File src/northbridge/intel/haswell/northbridge.c:

https://review.coreboot.org/c/coreboot/+/43743/1/src/northbridge/intel/haswe... 
PS1, Line 452: IOT
I think this has to do with the GDXC thingy, not sure if needed. It's optional.

Change in coreboot[master]: nb/intel/haswell: Enable DMI ASPM

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,

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

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

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

Change subject: nb/intel/haswell: Enable DMI ASPM
......................................................................

nb/intel/haswell: Enable DMI ASPM

On Haswell platforms, the processor and the PCH are two separate dies,
and communicate through a high-speed bus. This is DMI (Direct Media
Interface) on traditional two-package platforms, but single-package
Haswell LP variants use OPI (On-Package Interconnect) instead.

Since OPI is not routed through the mainboard, most link parameters are
static and cannot be changed. OPI self-initializes on boot, anyway.

However, DMI needs to be initialized in firmware. On Haswell, the MRC
initializes the physical DMI link, but things like topology and power
management need to be configured as well. And we don't do that properly.

We enable ASPM on the PCH side of the DMI link, but not on the SA side.
Both sides need to use the same settings, so enable DMI ASPM on the SA.
Clearing the error status bits needs to be done on all Haswell variants.

Tested on Asrock B85M Pro4, still boots.

Change-Id: Ie97ff56eec9f928cfd2d5d43a287f3e0d2fbf3cf
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/northbridge.c
2 files changed, 52 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/43743/2

Change in coreboot[master]: nb/intel/haswell: Enable DMI ASPM

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell: Enable DMI ASPM
......................................................................

Patch Set 2:

(2 comments)

I checked which steps need to be done on which variants, and updated the code accordingly. I also adjusted some registers' width, and put names to them.

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

https://review.coreboot.org/c/coreboot/+/43743/1//COMMIT_MSG@8 
PS1, Line 8:
> DMI stands for `Direct Media Interface`, which is the PCIe-like link between the SA (System Agent, a […]

Done, I hope.

https://review.coreboot.org/c/coreboot/+/43743/1/src/northbridge/intel/haswe... 
File src/northbridge/intel/haswell/northbridge.c:

https://review.coreboot.org/c/coreboot/+/43743/1/src/northbridge/intel/haswe... 
PS1, Line 452: IOT
> I think this has to do with the GDXC thingy, not sure if needed. It's optional.

Dropped as it's not needed.

Change in coreboot[master]: nb/intel/haswell: Enable DMI ASPM

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43743 )

Change subject: nb/intel/haswell: Enable DMI ASPM
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: nb/intel/haswell: Enable DMI ASPM

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43743 )

Change subject: nb/intel/haswell: Enable DMI ASPM
......................................................................

nb/intel/haswell: Enable DMI ASPM

On Haswell platforms, the processor and the PCH are two separate dies,
and communicate through a high-speed bus. This is DMI (Direct Media
Interface) on traditional two-package platforms, but single-package
Haswell LP variants use OPI (On-Package Interconnect) instead.

Since OPI is not routed through the mainboard, most link parameters are
static and cannot be changed. OPI self-initializes on boot, anyway.

However, DMI needs to be initialized in firmware. On Haswell, the MRC
initializes the physical DMI link, but things like topology and power
management need to be configured as well. And we don't do that properly.

We enable ASPM on the PCH side of the DMI link, but not on the SA side.
Both sides need to use the same settings, so enable DMI ASPM on the SA.
Clearing the error status bits needs to be done on all Haswell variants.

Tested on Asrock B85M Pro4, still boots.

Change-Id: Ie97ff56eec9f928cfd2d5d43a287f3e0d2fbf3cf
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43743
Tested-by: build bot (Jenkins) 
Reviewed-by: Arthur Heymans 
---
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/northbridge.c
2 files changed, 52 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Arthur Heymans: Looks good to me, approved

diff --git a/src/northbridge/intel/haswell/haswell.h b/src/northbridge/intel/haswell/haswell.h
index 1255cac..5e28336 100644
--- a/src/northbridge/intel/haswell/haswell.h
+++ b/src/northbridge/intel/haswell/haswell.h
@@ -127,11 +127,21 @@
 #define DMILCTL		0x088	/* 16bit */
 #define DMILSTS		0x08a	/* 16bit */

+#define DMILCTL2	0x098	/* 16bit */
+
 #define DMICTL1		0x0f0	/* 32bit */
 #define DMICTL2		0x0fc	/* 32bit */

+#define DMIUESTS	0x1c4	/* 32bit */
+#define DMICESTS	0x1d0	/* 32bit */
+
 #define DMICC		0x208	/* 32bit */

+#define DMIL0SLAT	0x22c	/* 32bit */
+#define DMILLTC		0x238	/* 32bit */
+
+#define DMI_AFE_PM_TMR	0xc28	/* 32bit */
+
 #define DMIDRCCFG	0xeb4	/* 32bit */

#ifndef __ASSEMBLER__
diff --git a/src/northbridge/intel/haswell/northbridge.c b/src/northbridge/intel/haswell/northbridge.c
index ef7742e..5c9ef74 100644
--- a/src/northbridge/intel/haswell/northbridge.c
+++ b/src/northbridge/intel/haswell/northbridge.c
@@ -418,10 +418,52 @@
    pci_write_config32(host_dev, DEVEN, deven);
 }

+static void northbridge_dmi_init(void)
+{
+	const bool is_haswell_h = !CONFIG(INTEL_LYNXPOINT_LP);
+
+	u16 reg16;
+	u32 reg32;
+
+	/* Steps prior to DMI ASPM */
+	if (is_haswell_h) {
+		/* Configure DMI De-Emphasis */
+		reg16 = DMIBAR16(DMILCTL2);
+		reg16 |= (1 << 6);	/* 0b: -6.0 dB, 1b: -3.5 dB */
+		DMIBAR16(DMILCTL2) = reg16;
+
+		reg32 = DMIBAR32(DMIL0SLAT);
+		reg32 |= (1 << 31);
+		DMIBAR32(DMIL0SLAT) = reg32;
+
+		reg32 = DMIBAR32(DMILLTC);
+		reg32 |= (1 << 29);
+		DMIBAR32(DMILLTC) = reg32;
+
+		reg32 = DMIBAR32(DMI_AFE_PM_TMR);
+		reg32 &= ~0x1f;
+		reg32 |= 0x13;
+		DMIBAR32(DMI_AFE_PM_TMR) = reg32;
+	}
+
+	/* Clear error status bits */
+	DMIBAR32(DMIUESTS) = 0xffffffff;
+	DMIBAR32(DMICESTS) = 0xffffffff;
+
+	if (is_haswell_h) {
+		/* Enable ASPM L0s and L1 on SA link, should happen before PCH link */
+		reg16 = DMIBAR16(DMILCTL);
+		reg16 |= (1 << 1) | (1 << 0);
+		DMIBAR16(DMILCTL) = reg16;
+	}
+}
+
 static void northbridge_init(struct device *dev)
 {
    u8 bios_reset_cpl, pair;

+	northbridge_dmi_init();
+
    /* Enable Power Aware Interrupt Routing. */
    pair = MCHBAR8(INTRDIRCTL);
    pair &= ~0x7;	/* Clear 2:0 */