Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
mainboard/google/puff: Enable net driver on pcie ep
Let coreboot know there is a NIC device on the end so that the mac from vpd is set at early boot.
Properly configure the link-leds in devicetree s.t. valid values are written out to the register at initialization.
Finally let the linker trim unused net driver symbols when unused in devicetree rather than being overly zealous in the Kconfig.
BUG=b:146592075 BRANCH=none TEST=Boot to kernel. Insert mac address into VPD vpd -s ethernet_mac=<address> reboot the system. Ensure we have ip address and corresponding mac address with ifconfig. Ensure ethernet controller shows up with lspci.
Change-Id: I76ce6d8a5a26842fcb2544ee96567fe0da8603b1 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38003/1
diff --git a/src/mainboard/google/hatch/Kconfig b/src/mainboard/google/hatch/Kconfig index 3427ced..783ec73 100644 --- a/src/mainboard/google/hatch/Kconfig +++ b/src/mainboard/google/hatch/Kconfig @@ -22,8 +22,8 @@ select MB_HAS_ACTIVE_HIGH_SD_PWR_ENABLE select SOC_INTEL_COMETLAKE select SYSTEM_TYPE_LAPTOP - select RT8168_GET_MAC_FROM_VPD if BOARD_GOOGLE_PUFF - select RT8168_SET_LED_MODE if BOARD_GOOGLE_PUFF + select RT8168_GET_MAC_FROM_VPD + select RT8168_SET_LED_MODE
if BOARD_GOOGLE_BASEBOARD_HATCH
diff --git a/src/mainboard/google/hatch/variants/puff/overridetree.cb b/src/mainboard/google/hatch/variants/puff/overridetree.cb index 342994d..0c6b3b0 100644 --- a/src/mainboard/google/hatch/variants/puff/overridetree.cb +++ b/src/mainboard/google/hatch/variants/puff/overridetree.cb @@ -180,7 +180,12 @@ end #I2C #4 device pci 1a.0 on end # eMMC device pci 1c.0 on end # FSP requires func0 be enabled. - device pci 1c.6 on end # RTL8111H Ethernet NIC (becomes RP1). + device pci 1c.6 on + chip drivers/net + register "customized_leds" = "0x05af" + device pci 00.0 on end + end + end # RTL8111H Ethernet NIC (becomes RP1). device pci 1e.3 off end # GSPI #1 end
Hello Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38003
to look at the new patch set (#2).
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
mainboard/google/puff: Enable net driver on pcie ep
Let coreboot know there is a NIC device on the end so that the mac from vpd is set at early boot.
Properly configure the link-leds in devicetree s.t. valid values are written out to the register at initialization.
Finally let the linker trim unused net driver symbols when unused in devicetree rather than being overly zealous in the Kconfig.
BUG=b:146592075,146999042,146999043 BRANCH=none TEST=Boot to kernel. Insert mac address into VPD vpd -s ethernet_mac=<address> reboot the system. Ensure we have ip address and corresponding mac address with ifconfig. Ensure ethernet controller shows up with lspci.
Change-Id: I76ce6d8a5a26842fcb2544ee96567fe0da8603b1 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38003/2
Hello Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38003
to look at the new patch set (#3).
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
mainboard/google/puff: Enable net driver on pcie ep
Let coreboot know there is a NIC device on the end so that the mac from vpd is set at early boot.
Properly configure the link-leds in devicetree s.t. valid values are written out to the register at initialization.
Finally let the linker trim unused net driver symbols when unused in devicetree rather than being overly zealous in the Kconfig.
BUG=b:146592075,146999042,146999043 BRANCH=none TEST=Boot to kernel. Insert mac address into VPD vpd -s ethernet_mac=<address> reboot the system. Ensure we have ip address and corresponding mac address with ifconfig. Ensure ethernet controller shows up with lspci.
Change-Id: I76ce6d8a5a26842fcb2544ee96567fe0da8603b1 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/Kconfig M src/mainboard/google/hatch/variants/puff/overridetree.cb 2 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38003/3
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/38003/3/src/mainboard/google/hatch/... PS3, Line 25: if BOARD_GOOGLE_PUFF Why did you remove this check? Not all hatch variants have NIC.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38003/3//COMMIT_MSG@7 PS3, Line 7: mainboard/google/puff: Enable net driver on pcie ep AFAICT, the driver is already enabled for puff, so that part is a no=op for puff.
The functional part of this patch is the devicetree part that sets up the LEDs and enables device 0.0; so I recommend changing the commit message subject to reflect that.
Further, so as to unblock this important change, move the KConfig part that enables the NIC driver for all hatch to a different CL which can have its own discussion.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38003/3//COMMIT_MSG@7 PS3, Line 7: mainboard/google/puff: Enable net driver on pcie ep
AFAICT, the driver is already enabled for puff, so that part is a no=op for puff. […]
A few points here so I will enumerate them:
Point i) && iii) are essentially the same point about the kconfig part. This has been explicitly mentioned at the commit message about the rational behind this. This follows on from advice and agreement from previous patches talking with Furquan that we should practically leave the linker to do it's job and not make huge combinatoric messes in kconfigs when it pays no dividends. See discussion on https://review.coreboot.org/c/coreboot/+/37909
Fur point ii) I don't understand your criticism as that is precisely what the commit short line says as well as the very first and second paragraph of the commit msg body says. If the short msg is somehow unclear, please elaborate with an alternative.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
https://review.coreboot.org/c/coreboot/+/38003/3/src/mainboard/google/hatch/... PS3, Line 25: if BOARD_GOOGLE_PUFF
Why did you remove this check? Not all hatch variants have NIC.
Because the linker will remove the driver symbols if it has not been instantiated in the devicetree of the variant. There is no need for kconfig combinatoric games. Please see the discussion on https://review.coreboot.org/c/coreboot/+/37909 for the background.
Hello Rizwan Qureshi, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38003
to look at the new patch set (#4).
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
mainboard/google/puff: Enable net driver on pcie ep
Let coreboot know there is a NIC device on the end so that the mac from vpd is set at early boot.
Properly configure the link-leds in devicetree s.t. valid values are written out to the register at initialization.
BUG=b:146592075,146999042,146999043 BRANCH=none TEST=Boot to kernel. Insert mac address into VPD vpd -s ethernet_mac=<address> reboot the system. Ensure we have ip address and corresponding mac address with ifconfig. Ensure ethernet controller shows up with lspci.
Change-Id: I76ce6d8a5a26842fcb2544ee96567fe0da8603b1 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/overridetree.cb 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38003/4
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38003/3//COMMIT_MSG@7 PS3, Line 7: mainboard/google/puff: Enable net driver on pcie ep
A few points here so I will enumerate them: […]
The Kconfig patch was split out into https://review.coreboot.org/c/coreboot/+/38098/ as you requested.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 4: Code-Review+2
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
mainboard/google/puff: Enable net driver on pcie ep
Let coreboot know there is a NIC device on the end so that the mac from vpd is set at early boot.
Properly configure the link-leds in devicetree s.t. valid values are written out to the register at initialization.
BUG=b:146592075,146999042,146999043 BRANCH=none TEST=Boot to kernel. Insert mac address into VPD vpd -s ethernet_mac=<address> reboot the system. Ensure we have ip address and corresponding mac address with ifconfig. Ensure ethernet controller shows up with lspci.
Change-Id: I76ce6d8a5a26842fcb2544ee96567fe0da8603b1 Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38003 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Daniel Kurtz djkurtz@google.com --- M src/mainboard/google/hatch/variants/puff/overridetree.cb 1 file changed, 6 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Daniel Kurtz: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/puff/overridetree.cb b/src/mainboard/google/hatch/variants/puff/overridetree.cb index 342994d..4385e98 100644 --- a/src/mainboard/google/hatch/variants/puff/overridetree.cb +++ b/src/mainboard/google/hatch/variants/puff/overridetree.cb @@ -179,7 +179,12 @@ end end #I2C #4 device pci 1a.0 on end # eMMC - device pci 1c.0 on end # FSP requires func0 be enabled. + device pci 1c.0 on + chip drivers/net + register "customized_leds" = "0x05af" + device pci 00.0 on end + end + end # FSP requires func0 be enabled. device pci 1c.6 on end # RTL8111H Ethernet NIC (becomes RP1). device pci 1e.3 off end # GSPI #1 end
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... PS5, Line 183: chip drivers/net : register "customized_leds" = "0x05af" : device pci 00.0 on end : end This should not be done. Instead, we should get this change in: https://review.coreboot.org/c/coreboot/+/36651/
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... PS5, Line 183: chip drivers/net : register "customized_leds" = "0x05af" : device pci 00.0 on end : end
This should not be done.
This = adding chip to func0 instead of the actual function.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... PS5, Line 183: chip drivers/net : register "customized_leds" = "0x05af" : device pci 00.0 on end : end
This should not be done. […]
What is https://review.coreboot.org/c/coreboot/+/36651 all about? Looks unfinished currently?
Yes, see my prior change put it on port7 however it didn't work which why I quested this a lot with other folks.
In any case I believe we found the underlying reason in that a clk was off in ME, specifically ClkoutSRC0, which took us some time to trace down. I'll try moving it back again once that change is merged.
The NIC issue is still WIP however thanks for the comment.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38003 )
Change subject: mainboard/google/puff: Enable net driver on pcie ep ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38003/5/src/mainboard/google/hatch/... PS5, Line 183: chip drivers/net : register "customized_leds" = "0x05af" : device pci 00.0 on end : end
What is https://review.coreboot.org/c/coreboot/+/36651 all about? Looks unfinished currently?
It is expected to enable support for devicetree update when root ports get swapped by FSP such that the mainboard can continue adding chips/devices to the root port on which the devices really live on. Having that change in will enable you to add this chip and device for NIC under 1c.6 instead of adding it to 1c.0.
-
Daniel Kurtz (Code Review) -
Edward O'Callaghan (Code Review)
-
Furquan Shaikh (Code Review)
-
Rizwan Qureshi (Code Review)