Change in coreboot[master]: sb/intel/lynxpoint/lpc.c: Relocate lock bit write

[Angel Pons (Code Review)]

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

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

sb/intel/lynxpoint/lpc.c: Relocate lock bit write

This lock bit can be set later, and should also be set for Lynxpoint-H.

Change-Id: I5c32127f2b4cfdfeb0e30a64e5bdda89958933cb
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/lpc.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/47036/1

diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c
index 6b47b8a..10e498a 100644
--- a/src/southbridge/intel/lynxpoint/lpc.c
+++ b/src/southbridge/intel/lynxpoint/lpc.c
@@ -364,9 +364,6 @@
    if (RCBA32(FD) & PCH_DISABLE_ADSPD)
    	RCBA32_OR(0x2b1c, (1 << 29));

-	/* Lock */
-	RCBA32_OR(0x3a6c, 0x00000001);
-
    /* Set RCBA 0x33D4 after other setup */
    RCBA32_OR(0x33d4, 0x2fff2fb1);

@@ -772,6 +769,9 @@
 {
    spi_finalize_ops();

+	/* Lock */
+	RCBA32_OR(0x3a6c, 0x00000001);
+
    if (acpi_is_wakeup_s3() || CONFIG(INTEL_CHIPSET_LOCKDOWN))
    	apm_control(APM_CNT_FINALIZE);
 }

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47036 )

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/47036/comment/68156a0c_f3a381ed 
PS7, Line 9: This lock bit can be set later
Many others that are set early potentially too. Why make an
exception for this one?

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47036 )

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

Patch Set 7:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/47036/comment/73d46084_940b1974 
PS7, Line 9: This lock bit can be set later
> Many others that are set early potentially too. Why make an […]

The BS/BWGs suggest doing this register write after other steps are done. Reference code also defers this write to happen in PchInitBeforeBoot instead.

Also, it is currently set in a LPT-LP specific function, and I'd rather move the write elsewhere than duplicate it. This also eases unifying with Broadwell, which does the lock bit write in a dedicated finalisation function after `spi_finalize_ops()`.

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

sb/intel/lynxpoint/lpc.c: Relocate lock bit write

This lock bit can be set later, and should also be set for LynxPoint-H.

Tested on Asrock B85M Pro4 (LynxPoint-H), the lock bit is now set.

Change-Id: I5c32127f2b4cfdfeb0e30a64e5bdda89958933cb
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/lpc.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/47036/8

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47036 )

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

Patch Set 8: Code-Review+2

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/47036/comment/a6e58295_95ac6f1f 
PS7, Line 9: This lock bit can be set later
> This also eases unifying with Broadwell, which does the lock bit write in a dedicated finalisation function after `spi_finalize_ops()`.

That's very valueable information (i.e. we basically tested it already),
how about mentioning it in the commit message?

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47036 )

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

Patch Set 8:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/47036/comment/d00f2d60_97e3aebc 
PS7, Line 9: This lock bit can be set later
> > This also eases unifying with Broadwell, which does the lock bit write in a dedicated finalisation […]

Wait, I thought I had written a sentence there...

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Hello build bot (Jenkins), Nico Huber, Patrick Rudolph,

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

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

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

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

sb/intel/lynxpoint/lpc.c: Relocate lock bit write

This lock bit can be set later, and should also be set for LynxPoint-H.
This eases merging with Broadwell, which already sets this lock bit
after `spi_finalize_ops()` in a dedicated finalisation function.

Tested on Asrock B85M Pro4 (LynxPoint-H), the lock bit is now set.

Change-Id: I5c32127f2b4cfdfeb0e30a64e5bdda89958933cb
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/lynxpoint/lpc.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/47036/9

[Angel Pons (Code Review)]

Attention is currently required from: Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47036 )

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

Patch Set 9:

(1 comment)

Commit Message:

https://review.coreboot.org/c/coreboot/+/47036/comment/81039c91_906e6f35 
PS7, Line 9: This lock bit can be set later
> Wait, I thought I had written a sentence there...

Done

[Patrick Georgi (Code Review)]

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

Change subject: sb/intel/lynxpoint/lpc.c: Relocate lock bit write
......................................................................

sb/intel/lynxpoint/lpc.c: Relocate lock bit write

This lock bit can be set later, and should also be set for LynxPoint-H.
This eases merging with Broadwell, which already sets this lock bit
after `spi_finalize_ops()` in a dedicated finalisation function.

Tested on Asrock B85M Pro4 (LynxPoint-H), the lock bit is now set.

Change-Id: I5c32127f2b4cfdfeb0e30a64e5bdda89958933cb
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47036
Reviewed-by: Nico Huber 
Tested-by: build bot (Jenkins) 
---
M src/southbridge/intel/lynxpoint/lpc.c
1 file changed, 3 insertions(+), 3 deletions(-)

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

diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c
index cf80e64..55157a8 100644
--- a/src/southbridge/intel/lynxpoint/lpc.c
+++ b/src/southbridge/intel/lynxpoint/lpc.c
@@ -485,9 +485,6 @@
    if (RCBA32(FD) & PCH_DISABLE_ADSPD)
    	RCBA32_OR(0x2b1c, (1 << 29));

-	/* Lock */
-	RCBA32_OR(0x3a6c, 0x00000001);
-
    /* Set RCBA 0x33D4 after other setup */
    RCBA32_OR(0x33d4, 0x2fff2fb1);

@@ -809,6 +806,9 @@
 {
    spi_finalize_ops();

+	/* Lock */
+	RCBA32_OR(0x3a6c, 0x00000001);
+
    if (acpi_is_wakeup_s3() || CONFIG(INTEL_CHIPSET_LOCKDOWN))
    	apm_control(APM_CNT_FINALIZE);
 }