Change in coreboot[master]: soc/intel/skylake: lock TCO base address on pch finalize

[Michael Niewöhner (Code Review)]

Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/skylake: lock TCO base address on pch finalize
......................................................................

soc/intel/skylake: lock TCO base address on pch finalize

This is a follow-up on I5bd95b3580adc0f4cffa667f8979b7cf08925720 which
already locks TCO itself but does not lock the base address.

Signed-off-by: Michael Niewöhner 
Change-Id: Idab9419487e6e4cbdecd2efaa4772ff4960c9055
---
M src/soc/intel/common/block/smbus/tco.c
1 file changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35525/1

diff --git a/src/soc/intel/common/block/smbus/tco.c b/src/soc/intel/common/block/smbus/tco.c
index 1a215eb..a3ebe24 100644
--- a/src/soc/intel/common/block/smbus/tco.c
+++ b/src/soc/intel/common/block/smbus/tco.c
@@ -34,6 +34,7 @@
 #define TCOBASE		0x50
 #define TCOCTL		0x54
 #define  TCO_BASE_EN		(1 << 8)
+#define  TCO_BASE_LOCK		(1 << 0)

/* Get base address of TCO I/O registers. */
 static uint16_t tco_get_bar(void)
@@ -61,8 +62,13 @@

void tco_lockdown(void)
 {
+	uint32_t reg32;
    uint16_t tcocnt;

+	/* TCO base address lockdown */
+	reg32 = pci_read_config32(dev, TCOCTL);
+	pci_write_config32(dev, TCOCTL, reg32 | TCO_BASE_LOCK);
+
    /* TCO Lock down */
    tcocnt = tco_read_reg(TCO1_CNT);
    tcocnt |= TCO_LOCK;

[Michael Niewöhner (Code Review)]

Hello Patrick Rudolph, Subrata Banik, build bot (Jenkins), Subrata Banik, Patrick Georgi,

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

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

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

Change subject: soc/intel/skylake: lock TCO base address on pch finalize
......................................................................

soc/intel/skylake: lock TCO base address on pch finalize

This is a follow-up on I5bd95b3580adc0f4cffa667f8979b7cf08925720 which
already locks TCO itself but does not lock the base address.

Signed-off-by: Michael Niewöhner 
Change-Id: Idab9419487e6e4cbdecd2efaa4772ff4960c9055
---
M src/soc/intel/common/block/smbus/tco.c
1 file changed, 13 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/35525/2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/skylake: lock TCO base address on pch finalize
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG@7 
PS2, Line 7: soc/intel/skylake
This is applying the logic for not just skylake but all Intel SoCs using this common code.

https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG@7 
PS2, Line 7: soc/intel/skylake: lock TCO base address on pch finalize
Just curious: What is the motivation behind this locking? Is this following any particular recommendation?

https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... 
PS2, Line 67: #if defined(__SIMPLE_DEVICE__)
            : 	int devfn = PCH_DEVFN_SMBUS;
            : 	pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn));
            : #else
            : 	struct device *dev;
            : 	dev = PCH_DEV_SMBUS;
            : #endif
            : 
            : 	/* TCO base address lockdown */
            : 	reg32 = pci_read_config32(dev, TCOCTL);
            : 	pci_write_config32(dev, TCOCTL, reg32 | TCO_BASE_LOCK);
I think at minimum this should be guarded by SOC_INTEL_COMMON_BLOCK_TCO_ENABLE_THROUGH_SMBUS. It might be better to just create a separate function to do this: static void tco_lockdown_bar()

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/skylake: lock TCO base address on pch finalize
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG@7 
PS2, Line 7: soc/intel/skylake: lock TCO base address on pch finalize
> Just curious: What is the motivation behind this locking? Is this following any particular recommend […]

from security perspective there is absolutely no reason for leaving it unlocked. see https://bugzilla.tianocore.org/show_bug.cgi?id=1457

https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG@7 
PS2, Line 7: soc/intel/skylake
> This is applying the logic for not just skylake but all Intel SoCs using this common code.

indeed, the prefix is wrong and will be corrected

https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... 
PS2, Line 67: if defined(__SIMPLE_DEVICE__)
I guess _SIMPLE_DEVICE_ is wrong here... will rework that

https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... 
PS2, Line 67: #if defined(__SIMPLE_DEVICE__)
            : 	int devfn = PCH_DEVFN_SMBUS;
            : 	pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn));
            : #else
            : 	struct device *dev;
            : 	dev = PCH_DEV_SMBUS;
            : #endif
            : 
            : 	/* TCO base address lockdown */
            : 	reg32 = pci_read_config32(dev, TCOCTL);
            : 	pci_write_config32(dev, TCOCTL, reg32 | TCO_BASE_LOCK);
> this should be guarded by SOC_INTEL_COMMON_BLOCK_TCO_ENABLE_THROUGH_SMBUS

no, it shouldn't. The base address should be locked in any case; maybe we should guard it by COREBOOT_LOCKDOWN

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/skylake: lock TCO base address on pch finalize
......................................................................

Patch Set 2: Code-Review-1

Need to do some rework and some more fixes (e.g. I found that the TCO BILD is not set, but the datasheet says this is needed for BILD.....)

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Michael Niewöhner (Code Review)]

Attention is currently required from: Furquan Shaikh, Angel Pons.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3:

(1 comment)

This change is ready for review.

File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/comment/982b2214_185eaf74 
PS2, Line 67: if defined(__SIMPLE_DEVICE__)
> nice, thx for that hint!

Done

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Angel Pons (Code Review)]

Attention is currently required from: Furquan Shaikh, Michael Niewöhner.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

Patchset:

PS3: 
LGTM, but I haven't read the datasheets yet

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Angel Pons (Code Review)]

Attention is currently required from: Marc Jones, Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Michael Niewöhner.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3:

(1 comment)

File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/comment/50c04eed_ae1ba8f3 
PS3, Line 59: 	pci_or_config32(dev, TCOCTL, TCO_BASE_LOCK);
Gemini Lake does not seem to have this register

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Michael Niewöhner (Code Review)]

Attention is currently required from: Marc Jones, Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3:

(1 comment)

File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/comment/bd58fe76_43ba063b 
PS3, Line 59: 	pci_or_config32(dev, TCOCTL, TCO_BASE_LOCK);
> Gemini Lake does not seem to have this register

glk fsp sets it

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Michael Niewöhner (Code Review)]

Attention is currently required from: Marc Jones, Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3:

(1 comment)

File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/comment/dca9d373_b4a655e4 
PS3, Line 59: 	pci_or_config32(dev, TCOCTL, TCO_BASE_LOCK);
> glk fsp sets it

apl datasheet also says "reserved" while apl fsp sets the bit

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Angel Pons (Code Review)]

Attention is currently required from: Marc Jones, Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Michael Niewöhner.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

File src/soc/intel/common/block/smbus/tco.c:

https://review.coreboot.org/c/coreboot/+/35525/comment/92e81e02_a095b522 
PS3, Line 59: 	pci_or_config32(dev, TCOCTL, TCO_BASE_LOCK);
> apl datasheet also says "reserved" while apl fsp sets the bit

OK, looks like the bit exists.

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Marc Jones (Code Review)]

Attention is currently required from: Furquan Shaikh, Jonathan Zhang, Jay Talbott, Stefan Reinauer, Michael Niewöhner.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/common/smbus: lock TCO base address on PCH finalize

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/common/smbus: lock TCO base address on PCH finalize
......................................................................

soc/intel/common/smbus: lock TCO base address on PCH finalize

Signed-off-by: Michael Niewöhner 
Change-Id: Idab9419487e6e4cbdecd2efaa4772ff4960c9055
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35525
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Marc Jones 
---
M src/soc/intel/common/block/smbus/tco.c
1 file changed, 5 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marc Jones: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/common/block/smbus/tco.c b/src/soc/intel/common/block/smbus/tco.c
index 8cde3bf..518541b 100644
--- a/src/soc/intel/common/block/smbus/tco.c
+++ b/src/soc/intel/common/block/smbus/tco.c
@@ -24,6 +24,7 @@
 #define TCOBASE		0x50
 #define TCOCTL		0x54
 #define  TCO_BASE_EN		(1 << 8)
+#define  TCO_BASE_LOCK		(1 << 0)

/* Get base address of TCO I/O registers. */
 static uint16_t tco_get_bar(void)
@@ -52,6 +53,10 @@
 void tco_lockdown(void)
 {
    uint16_t tcocnt;
+	const pci_devfn_t dev = PCH_DEV_SMBUS;
+
+	/* TCO base address lockdown */
+	pci_or_config32(dev, TCOCTL, TCO_BASE_LOCK);

/* TCO Lock down */
    tcocnt = tco_read_reg(TCO1_CNT);