Change in coreboot[master]: soc/intel/cnl: Enable HECI3 depending on devicetree

[Felix Singer (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

soc/intel/cnl: Enable HECI3 depending on devicetree

Currently HECI3 gets enabled by the option Heci3Enabled, but this
duplicates the devicetree on/off options. Therefore depend on the
devicetree for enablement of the HECI3 controller.

All corresponding mainboards were checked if the devicetree
configuration matches the Heci3Enabled setting.

Change-Id: Ic7d52096aee225c2ced1e1bc29ca850fe5073edc
Signed-off-by: Felix Singer 
---
M src/mainboard/purism/librem_whl/devicetree.cb
M src/mainboard/system76/lemp9/devicetree.cb
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
4 files changed, 4 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/44579/1

diff --git a/src/mainboard/purism/librem_whl/devicetree.cb b/src/mainboard/purism/librem_whl/devicetree.cb
index c122bb8..2f4bca3 100644
--- a/src/mainboard/purism/librem_whl/devicetree.cb
+++ b/src/mainboard/purism/librem_whl/devicetree.cb
@@ -277,9 +277,7 @@
    	device pci 16.1 off end # Management Engine Interface 2
    	device pci 16.2 off end # Management Engine IDE-R
    	device pci 16.3 off end # Management Engine KT Redirection
-		device pci 16.4 off	# Management Engine Interface 3
-			register "Heci3Enabled" = "0"
-		end
+		device pci 16.4 off end # Management Engine Interface 3
    	device pci 16.5 off end # Management Engine Interface 4
    	device pci 17.0 on  end # SATA
    	device pci 19.0 off end # I2C #4
diff --git a/src/mainboard/system76/lemp9/devicetree.cb b/src/mainboard/system76/lemp9/devicetree.cb
index 9cdeeab..ff088c9 100644
--- a/src/mainboard/system76/lemp9/devicetree.cb
+++ b/src/mainboard/system76/lemp9/devicetree.cb
@@ -201,9 +201,7 @@
    	device pci 16.1 off end # Management Engine Interface 2
    	device pci 16.2 off end # Management Engine IDE-R
    	device pci 16.3 off end # Management Engine KT Redirection
-		device pci 16.4 off     # Management Engine Interface 3
-			register "Heci3Enabled" = "0"
-		end
+		device pci 16.4 off end # Management Engine Interface 3
    	device pci 16.5 off end # Management Engine Interface 4
    	device pci 17.0 on  end # SATA
    	device pci 19.0 off end # I2C #4
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index 2923efc..1b92f0a 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -226,7 +226,6 @@
    uint8_t PchIshEnable;

/* Heci related */
-	uint8_t Heci3Enabled;
    uint8_t DisableHeciRetry;

/* Gfx related */
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 0779ce2..e7d4fb2 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -523,7 +523,8 @@
    else
    	params->ScsUfsEnabled = dev->enabled;

-	params->Heci3Enabled = config->Heci3Enabled;
+	dev = pcidev_path_on_root(PCH_DEVFN_CSE_3);
+	params->Heci3Enabled = is_dev_enabled(dev);
 #if !CONFIG(HECI_DISABLE_USING_SMM)
    dev = pcidev_path_on_root(PCH_DEVFN_CSE);
    params->Heci1Disabled = !is_dev_enabled(dev);

[Felix Singer (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Set Ready For Review

[Nico Huber (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 4:

What is Heci3? Is it enabled by any devicetree? is it even useful
with coreboot? If not, why not always disable it?

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 4:
> Patch Set 4:
>
> What is Heci3? Is it enabled by any devicetree? is it even useful
> with coreboot? If not, why not always disable it?

Heci3 is another ME interface (there are four, actually. FSP only has options for two (three?))

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 4: Code-Review+2

[Michael Niewöhner (Code Review)]

Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44579 )

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

soc/intel/cnl: Enable HECI3 depending on devicetree

Currently HECI3 gets enabled by the option Heci3Enabled, but this
duplicates the devicetree on/off options. Therefore depend on the
devicetree for enablement of the HECI3 controller.

All corresponding mainboards were checked if the devicetree
configuration matches the Heci3Enabled setting, and divergent
devicetrees were adjusted.

Change-Id: Ic7d52096aee225c2ced1e1bc29ca850fe5073edc
Signed-off-by: Felix Singer 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44579
Reviewed-by: Michael Niewöhner
Tested-by: build bot (Jenkins) 
---
M src/mainboard/prodrive/hermes/devicetree.cb
M src/mainboard/purism/librem_whl/devicetree.cb
M src/mainboard/system76/lemp9/devicetree.cb
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
5 files changed, 5 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Michael Niewöhner: Looks good to me, approved

diff --git a/src/mainboard/prodrive/hermes/devicetree.cb b/src/mainboard/prodrive/hermes/devicetree.cb
index 69323f2..5615554 100644
--- a/src/mainboard/prodrive/hermes/devicetree.cb
+++ b/src/mainboard/prodrive/hermes/devicetree.cb
@@ -28,7 +28,7 @@
    		register "HeciEnabled" = "1"
    	end
    	device pci 16.1 on  end # Management Engine Interface 2
-		device pci 16.4 on  end # Management Engine Interface 3
+		device pci 16.4 off end # Management Engine Interface 3
    	device pci 17.0 on  end # SATA
    	device pci 1d.6 on	# PCIe root port 15
    		device pci 00.0 on      # Aspeed PCI Bridge
diff --git a/src/mainboard/purism/librem_whl/devicetree.cb b/src/mainboard/purism/librem_whl/devicetree.cb
index fc3f418..b85e10f 100644
--- a/src/mainboard/purism/librem_whl/devicetree.cb
+++ b/src/mainboard/purism/librem_whl/devicetree.cb
@@ -277,9 +277,7 @@
    	device pci 16.1 off end # Management Engine Interface 2
    	device pci 16.2 off end # Management Engine IDE-R
    	device pci 16.3 off end # Management Engine KT Redirection
-		device pci 16.4 off	# Management Engine Interface 3
-			register "Heci3Enabled" = "0"
-		end
+		device pci 16.4 off end # Management Engine Interface 3
    	device pci 16.5 off end # Management Engine Interface 4
    	device pci 17.0 on  end # SATA
    	device pci 19.0 off end # I2C #4
diff --git a/src/mainboard/system76/lemp9/devicetree.cb b/src/mainboard/system76/lemp9/devicetree.cb
index c07b687..e9e3aa3 100644
--- a/src/mainboard/system76/lemp9/devicetree.cb
+++ b/src/mainboard/system76/lemp9/devicetree.cb
@@ -201,9 +201,7 @@
    	device pci 16.1 off end # Management Engine Interface 2
    	device pci 16.2 off end # Management Engine IDE-R
    	device pci 16.3 off end # Management Engine KT Redirection
-		device pci 16.4 off     # Management Engine Interface 3
-			register "Heci3Enabled" = "0"
-		end
+		device pci 16.4 off end # Management Engine Interface 3
    	device pci 16.5 off end # Management Engine Interface 4
    	device pci 17.0 on  end # SATA
    	device pci 19.0 off end # I2C #4
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index e5ceac9..9e7aa45 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -228,7 +228,6 @@
    uint8_t PchIshEnable;

/* Heci related */
-	uint8_t Heci3Enabled;
    uint8_t DisableHeciRetry;

/* Gfx related */
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 51ed2a8..615a94f 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -525,7 +525,8 @@
    else
    	params->ScsUfsEnabled = dev->enabled;

-	params->Heci3Enabled = config->Heci3Enabled;
+	dev = pcidev_path_on_root(PCH_DEVFN_CSE_3);
+	params->Heci3Enabled = is_dev_enabled(dev);
 #if !CONFIG(HECI_DISABLE_USING_SMM)
    dev = pcidev_path_on_root(PCH_DEVFN_CSE);
    params->Heci1Disabled = !is_dev_enabled(dev);

[Nico Huber (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> > What is Heci3? Is it enabled by any devicetree? is it even useful
> > with coreboot? If not, why not always disable it?
>
> Heci3 is another ME interface (there are four, actually. FSP only has options for two (three?))

I guess I'll place my comments inline in the future so
they get answered before merge... Again, is enabling
Heci3 useful with coreboot? Doesn't it require additional
configuration/software?

[Felix Singer (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
> > > What is Heci3? Is it enabled by any devicetree? is it even useful
> > > with coreboot? If not, why not always disable it?
> >
> > Heci3 is another ME interface (there are four, actually. FSP only has options for two (three?))
>
> I guess I'll place my comments inline in the future so
> they get answered before merge... Again, is enabling
> Heci3 useful with coreboot? Doesn't it require additional
> configuration/software?

I agree with Nico that questions should be resolved before submitting the patch.

Anyway, I don't know what this device is used for but I think we should give the possibility to enable it. There might be users outside of this tree. Always disabling it makes me feel we withhold something from the user.

[Angel Pons (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > > What is Heci3? Is it enabled by any devicetree? is it even useful
> > > > with coreboot? If not, why not always disable it?
> > >
> > > Heci3 is another ME interface (there are four, actually. FSP only has options for two (three?))
> >
> > I guess I'll place my comments inline in the future so
> > they get answered before merge... Again, is enabling
> > Heci3 useful with coreboot? Doesn't it require additional
> > configuration/software?
>
> I agree with Nico that questions should be resolved before submitting the patch.
>
> Anyway, I don't know what this device is used for but I think we should give the possibility to enable it. There might be users outside of this tree. Always disabling it makes me feel we withhold something from the user.

I consider that the limits of what a user can do is limited to that which won't result in a dirty image (anything past that threshold would be hacking/development, and I wouldn't expect most people to do that). Changing a setting on the devicetree will make the coreboot image dirty.

If you want the user to have control over this setting, it needs to be a Kconfig option. However, I firmly believe this should *NOT* be user-configurable. Not all coreboot developers know what HECI 3 is useful for (e.g. me), so having it as a devicetree option seems reasonable.

[Nico Huber (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Not all coreboot developers know what HECI 3 is useful for (e.g. me), so having it as a devicetree option seems reasonable.

That's what help texts are there for.

Also, worth adding, it seems that actually *no* coreboot developer
knew what `Heci3Enabled` was there for ;)

Shortly before grepping for it in FSP, I realized the typo: why is
there a `d`? Usually these options are called `*Enable`. It turned
out that this UPD never qualified to be set from a devicetree. One
is supposed to set it to a value from a HOB on the resume path. No
idea what happens if one sets it to 1 unconditionally. And I doubt
anybody ever tried: In the coreboot history it was never set to 1.

You are maintaining ghosts here, folks :)

[Michael Niewöhner (Code Review)]

Michael Niewöhner has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/44579 )

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
> > Not all coreboot developers know what HECI 3 is useful for (e.g. me), so having it as a devicetree option seems reasonable.
>
> That's what help texts are there for.
>
> Also, worth adding, it seems that actually *no* coreboot developer
> knew what `Heci3Enabled` was there for ;)
>
> Shortly before grepping for it in FSP, I realized the typo: why is
> there a `d`? Usually these options are called `*Enable`. It turned
> out that this UPD never qualified to be set from a devicetree. One
> is supposed to set it to a value from a HOB on the resume path. No
> idea what happens if one sets it to 1 unconditionally. And I doubt
> anybody ever tried: In the coreboot history it was never set to 1.
>
> You are maintaining ghosts here, folks :)

Meh. Thank you!

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Not all coreboot developers know what HECI 3 is useful for (e.g. me), so having it as a devicetree option seems reasonable.
> >
> > That's what help texts are there for.
> >
> > Also, worth adding, it seems that actually *no* coreboot developer
> > knew what `Heci3Enabled` was there for ;)
> >
> > Shortly before grepping for it in FSP, I realized the typo: why is
> > there a `d`? Usually these options are called `*Enable`. It turned
> > out that this UPD never qualified to be set from a devicetree. One
> > is supposed to set it to a value from a HOB on the resume path. No
> > idea what happens if one sets it to 1 unconditionally. And I doubt
> > anybody ever tried: In the coreboot history it was never set to 1.
> >
> > You are maintaining ghosts here, folks :)
>
> Meh. Thank you!

Oh, I just found this in the bsf:
"The HECI3 state from Mbp for reference in S3 path or when MbpHob is not installed. 0: disable, 1: enable"; the interesting part is "or when MbpHob is not installed."

[Nico Huber (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> > > Patch Set 5:
> > > > Not all coreboot developers know what HECI 3 is useful for (e.g. me), so having it as a devicetree option seems reasonable.
> > >
> > > That's what help texts are there for.
> > >
> > > Also, worth adding, it seems that actually *no* coreboot developer
> > > knew what `Heci3Enabled` was there for ;)
> > >
> > > Shortly before grepping for it in FSP, I realized the typo: why is
> > > there a `d`? Usually these options are called `*Enable`. It turned
> > > out that this UPD never qualified to be set from a devicetree. One
> > > is supposed to set it to a value from a HOB on the resume path. No
> > > idea what happens if one sets it to 1 unconditionally. And I doubt
> > > anybody ever tried: In the coreboot history it was never set to 1.
> > >
> > > You are maintaining ghosts here, folks :)
> >
> > Meh. Thank you!
>
> Oh, I just found this in the bsf:
> "The HECI3 state from Mbp for reference in S3 path or when MbpHob is not installed. 0: disable, 1: enable"; the interesting part is "or when MbpHob is not installed."

Yes, bascically that's when `SkipMbpHob` is set or something goes wrong
in FSP. Funnily, we set it on SKL and ICL. But I don't think it matters.
In that case, we would have to perform the capability check that FSP
would do otherwise, before setting `Heci3Enabled`... It's really nothing
that we should prepare for just in case somebody might want to use it...
sometime...

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:

Subrata, can you give us a little insight on what HECI 3 is useful for?

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44579 )

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
>
> Subrata, can you give us a little insight on what HECI 3 is useful for?

HECI 3 is Function 4 of same ME device so you can say its Interface #3 of ME device

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
>
> Subrata, can you give us a little insight on what HECI 3 is useful for?

Ah, found it... HECI3 is the "Integrated Touch" ("iTouch") interface. Currently, there is no device using it.

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/cnl: Enable HECI3 depending on devicetree
......................................................................

Patch Set 5:
> Patch Set 5:
> > Patch Set 5:
> >
> > Subrata, can you give us a little insight on what HECI 3 is useful for?
>
> HECI 3 is Function 4 of same ME device so you can say its Interface #3 of ME device

Sure, the main part of that question was "what HECI 3 is useful for" :P But thanks, anyway