Change in coreboot[master]: soc/skylake: Enable DCI depending on devicetree configuration

[Felix Singer (Code Review)]

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43846 )

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

soc/skylake: Enable DCI depending on devicetree configuration

Currently DCI gets enabled by the option PchDciEn, but this
duplicates the devicetree on/off options. Therefore use the
on/off options for the enablement of the DCI controller.

I checked all corresponding mainboards if the devicetree
configuration matches the PchDciEn setting.

Change-Id: Ic816655d87cee9e56520aa30fc3c837b561f24e5
Signed-off-by: Felix Singer 
---
M src/soc/intel/skylake/chip.h
M src/soc/intel/skylake/romstage/romstage.c
2 files changed, 2 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/43846/1

diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h
index 92cd1ba..06dc59d 100644
--- a/src/soc/intel/skylake/chip.h
+++ b/src/soc/intel/skylake/chip.h
@@ -194,9 +194,6 @@
    u32 TraceHubMemReg0Size;
    u32 TraceHubMemReg1Size;

-	/* DCI Enable/Disable */
-	u8 PchDciEn;
-
    /*
     * PCIe Root Port configuration:
     * each element of array corresponds to
diff --git a/src/soc/intel/skylake/romstage/romstage.c b/src/soc/intel/skylake/romstage/romstage.c
index 99f444a..cb3f5b9 100644
--- a/src/soc/intel/skylake/romstage/romstage.c
+++ b/src/soc/intel/skylake/romstage/romstage.c
@@ -291,7 +291,8 @@
    m_t_cfg->DidInitStat = 0x01;

/* DCI and TraceHub configs */
-	m_t_cfg->PchDciEn = config->PchDciEn;
+	dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
+	m_t_cfg->PchDciEn = dev ? dev->enabled : 0;
    m_cfg->EnableTraceHub = config->EnableTraceHub;
    m_cfg->TraceHubMemReg0Size = config->TraceHubMemReg0Size;
    m_cfg->TraceHubMemReg1Size = config->TraceHubMemReg1Size;

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43846 )

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 1: Code-Review-1

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43846 )

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 1:

Some mainboards have turned on DCI in the devicetree, which AFAIK had no effect until now. Thus this patch would enable the DCI controller.

Please have a look at this. I just want to make sure this is really intended and doesn't result in a security issue.

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Michał Żygowski, Patrick Georgi, Martin Roth, Matt DeVillier, David Hendricks, Patrick Rudolph, Piotr Król,

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

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

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

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

soc/skylake: Enable DCI depending on devicetree configuration

Currently DCI gets enabled by the option PchDciEn, but this
duplicates the devicetree on/off options. Therefore use the
on/off options for the enablement of the DCI controller.

I checked all corresponding mainboards if the devicetree
configuration matches the PchDciEn setting.

Change-Id: Ic816655d87cee9e56520aa30fc3c837b561f24e5
Signed-off-by: Felix Singer 
---
M src/soc/intel/skylake/chip.h
M src/soc/intel/skylake/romstage/romstage.c
2 files changed, 3 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/43846/2

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43846 )

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 2: Code-Review-1

[Angel Pons (Code Review)]

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

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/43846/2//COMMIT_MSG@13 
PS2, Line 13: I checked all corresponding mainboards if the devicetree
            : configuration matches the PchDciEn setting.
You didn't, hence the -1? I see lots of "device pci 14.1 on end". Might as well use this comment as a submit gate.

[Michael Niewöhner (Code Review)]

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

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/romst... 
File src/soc/intel/skylake/romstage/romstage.c:

https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/romst... 
PS2, Line 295: 	dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
             : 	m_t_cfg->PchDciEn = dev ? dev->enabled : 0;
             : 
nope, DCI does not depend on xDCI/USBOTG being enabled AFAICT. In fsp, PchDciEn controls the register ECTRL, which enables/disables the dci feature

[Michael Niewöhner (Code Review)]

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

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/chip.... 
PS2, Line 340: dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
             : 	if (dev) {
             : 		if (!xdci_can_enable())
             : 			dev->enabled = 0;
             : 		params->XdciEnable = dev->enabled;
             : 	} else {
             : 		params->XdciEnable = 0;
             : 	}
XdciEnable is controlled here already but this could be simplified...

dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
dev->enabled = dev && dev->enabled && xdci_can_enable();
params->XdciEnable = dev->enabled;

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43846 )

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/43846/2//COMMIT_MSG@13 
PS2, Line 13: I checked all corresponding mainboards if the devicetree
            : configuration matches the PchDciEn setting.
> You didn't, hence the -1? I see lots of "device pci 14.1 on end". […]

That's why the -1: https://review.coreboot.org/c/coreboot/+/43846/2#message-da5583cac770dcfa604...

But I just found out that DCI != xDCI. Looks like xDCI is just "normal" OTG without allowing root access to the CPU. For xDCI there is XdciEnable.

[Michael Niewöhner (Code Review)]

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

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/43846/2//COMMIT_MSG@13 
PS2, Line 13: I checked all corresponding mainboards if the devicetree
            : configuration matches the PchDciEn setting.
> That's why the -1: https://review.coreboot. […]

see https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/chip....

[Angel Pons (Code Review)]

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

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/43846/2/src/soc/intel/skylake/chip.... 
PS2, Line 340: dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
             : 	if (dev) {
             : 		if (!xdci_can_enable())
             : 			dev->enabled = 0;
             : 		params->XdciEnable = dev->enabled;
             : 	} else {
             : 		params->XdciEnable = 0;
             : 	}
> XdciEnable is controlled here already but this could be simplified... […]

No. That will trigger undefined behavior when dev is null.

[Felix Singer (Code Review)]

Felix Singer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43846 )

Change subject: soc/skylake: Enable DCI depending on devicetree configuration
......................................................................

Abandoned

Abandoning this, because I found out DCI != xDCI. See here: https://review.coreboot.org/c/coreboot/+/43846/4#message-5f813788ea0f974b2eb....

Sorry!