Change in coreboot[master]: nb/intel/haswell/early_init.c: Remove invalid register writes

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

nb/intel/haswell/early_init.c: Remove invalid register writes

MRC overwrites the value of SSKPD and register 0x6120 is reserved.

Tested on Asrock B85M Pro4, still boots.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46683/1

diff --git a/src/northbridge/intel/haswell/early_init.c b/src/northbridge/intel/haswell/early_init.c
index 79cc277..a0e4211 100644
--- a/src/northbridge/intel/haswell/early_init.c
+++ b/src/northbridge/intel/haswell/early_init.c
@@ -146,16 +146,6 @@
    reg32 = MCHBAR32(SAPMCTL);
    MCHBAR32(SAPMCTL) = reg32 | 1;

-	/* GPU RC6 workaround for sighting 366252 */
-	reg32 = MCHBAR32(SSKPD + 4);
-	reg32 |= (1UL << 31);
-	MCHBAR32(SSKPD + 4) = reg32;
-
-	/* VLW (Virtual Legacy Wire?) */
-	reg32 = MCHBAR32(0x6120);
-	reg32 &= ~(1 << 0);
-	MCHBAR32(0x6120) = reg32;
-
    reg32 = MCHBAR32(INTRDIRCTL);
    reg32 |= (1 << 4) | (1 << 5);
    MCHBAR32(INTRDIRCTL) = reg32;

[Patrick Rudolph (Code Review)]

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

nb/intel/haswell/early_init.c: Remove invalid register writes

MRC overwrites the value of SSKPD and register 0x6120 is reserved.

Tested on Asrock B85M Pro4, still boots.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46683/4

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46683 )

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 4: Code-Review-1

I would like to talk about this before it goes in... For instance,
it's possible that the bit in SSKPD gives the MRC a hint what to
do. Also, reserved often just means "reserved from your eyes".

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46683 )

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG@9 
PS4, Line 9: 0x6120
I can't find anything about it. Where did you read that it's reserved?

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG@9 
PS4, Line 9: 0x6120
> I can't find anything about it. […]

It does not appear anywhere in reference code, and was dropped from Broadwell in CL:199364

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

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

nb/intel/haswell/early_init.c: Remove invalid register writes

MRC overwrites the value of SSKPD and register 0x6120 is reserved. These
workarounds were copied from Sandy Bridge, and got dropped on Broadwell.

Tested on Asrock B85M Pro4, still boots.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46683/5

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 5:

(1 comment)
> Patch Set 4: Code-Review-1
>
> I would like to talk about this before it goes in... For instance,
> it's possible that the bit in SSKPD gives the MRC a hint what to
> do. Also, reserved often just means "reserved from your eyes".

Sighting 366252 first appeared in sandybridge, and the code was copy-pasted from Haswell.

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG@9 
PS4, Line 9: 0x6120
> It does not appear anywhere in reference code, and was dropped from Broadwell in CL:199364

I've updated the commit message.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46683 )

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG@9 
PS4, Line 9: 0x6120
> I've updated the commit message.

Please mention as many details as possible. For instance that the MRC
was checked and we know that nothing consumes the SSKPD value. Also that
no documentation mentions MCHBAR+0x6120. (Not the same as reserved.)

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

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

nb/intel/haswell/early_init.c: Remove invalid register writes

MRC does not use the value of SSKPD, and will overwrite it with constant
values at the end of memory initialisation. Since coreboot does not rely
on this particular bit's value, it is safe to drop the writes to set it.

MCHBAR register 0x6120 is undocumented. It is nowhere to be found in any
documentation or code I have access to; not even for Sandy/Ivy Bridge,
the platform where this mysterious register write originally came from.

These workarounds were copied from Sandy Bridge, but do not apply to
Haswell. They were dropped on Broadwell, so drop them for Haswell too.

Tested on Asrock B85M Pro4, still boots.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/46683/6

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46683 )

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

Patch Set 6: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46683/4//COMMIT_MSG@9 
PS4, Line 9: 0x6120
> Please mention as many details as possible. For instance that the MRC […]

Thanks

[Angel Pons (Code Review)]

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

Change subject: nb/intel/haswell/early_init.c: Remove invalid register writes
......................................................................

nb/intel/haswell/early_init.c: Remove invalid register writes

MRC does not use the value of SSKPD, and will overwrite it with constant
values at the end of memory initialisation. Since coreboot does not rely
on this particular bit's value, it is safe to drop the writes to set it.

MCHBAR register 0x6120 is undocumented. It is nowhere to be found in any
documentation or code I have access to; not even for Sandy/Ivy Bridge,
the platform where this mysterious register write originally came from.

These workarounds were copied from Sandy Bridge, but do not apply to
Haswell. They were dropped on Broadwell, so drop them for Haswell too.

Tested on Asrock B85M Pro4, still boots.

Change-Id: I21d9656a7595d47ac8648c08d223b7cbafd213c3
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46683
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/northbridge/intel/haswell/early_init.c
1 file changed, 0 insertions(+), 10 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/northbridge/intel/haswell/early_init.c b/src/northbridge/intel/haswell/early_init.c
index 79cc277..a0e4211 100644
--- a/src/northbridge/intel/haswell/early_init.c
+++ b/src/northbridge/intel/haswell/early_init.c
@@ -146,16 +146,6 @@
    reg32 = MCHBAR32(SAPMCTL);
    MCHBAR32(SAPMCTL) = reg32 | 1;

-	/* GPU RC6 workaround for sighting 366252 */
-	reg32 = MCHBAR32(SSKPD + 4);
-	reg32 |= (1UL << 31);
-	MCHBAR32(SSKPD + 4) = reg32;
-
-	/* VLW (Virtual Legacy Wire?) */
-	reg32 = MCHBAR32(0x6120);
-	reg32 &= ~(1 << 0);
-	MCHBAR32(0x6120) = reg32;
-
    reg32 = MCHBAR32(INTRDIRCTL);
    reg32 |= (1 << 4) | (1 << 5);
    MCHBAR32(INTRDIRCTL) = reg32;