coreboot-gerrit October 2019

coreboot-gerrit@coreboot.org

1 participants
1921 discussions

Start a nNew thread

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Subrata Banik (Code Review) 31 Oct '19

31 Oct '19

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Nico Huber (Code Review) 31 Oct '19

31 Oct '19

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 ) Change subject: soc/intel/tigerlake: Do initial SoC commit ...................................................................... Patch Set 10: > > That doesn't mean we shouldn't review the Tiger Lake addition. And > > I think we all agreed already that a 8k LOC patch isn't reviewable. > > And of course, if we had more common code already, it would be much > > easier... and we would have a better code base if 3ee54bb hadn't > > slipped in. Let's not repeat that error. > > Sure, i believe Arthur has done great work of taking pain to review this CL. we have address majority of his feedback and few more is there. will continue to address also will see if we can submit this CL into 4 part > > 1. Bootblock (soc/intel/tigerlake/bootblock/) > 2. romstage (soc/intel/tigerlake/romstage/) > 3. ramstage (soc/intel/tigerlake/) > 4. acpi (soc/intel/tigerlake/acpi/) I still don't think "this CL" is a good idea, even if split up. Any commit history that first copies stale code and then adapts it in later commits (instead of adding the adapted code step by step) will lose lots of information. For instance, I assume some developers have already looked at the copied code and identified chunks that can stay the same. A copy first, adapt later commit history will not contain this information. It would contain details about what was changed, but not about what wasn't. OTOH, if the commit that adds (pieces of) unchanged code targets a single topic, and the commit message states that this was confirmed to work on the new platform, nobody will have to doubt if that was already checked. And the information will be in the Git history and can help future maintenance. -- To view, visit https://review.coreboot.org/c/coreboot/+/36087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id95e2fa9b7a7c6b3b9233d2c438b25a6c4904bbb Gerrit-Change-Number: 36087 Gerrit-PatchSet: 10 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Thu, 31 Oct 2019 14:54:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE
by Kyösti Mälkki (Code Review) 31 Oct '19

31 Oct '19

Hello Patrick Rudolph, Vanny E, Huang Jin, Philipp Deppenwiese, build bot (Jenkins), David Guckian, Patrick Georgi, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/33928 to look at the new patch set (#12). Change subject: cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE ...................................................................... cpu/x86/tsc: Replace TSC_CONSTANT_RATE with UNKNOWN_TSC_RATE Implementations with LAPIC_MONOTONIC_TIMER typically will not have tsc_freq_mhz() implemented and default to UNKNOWN_TSC_RATE. Implementations of tsc_freq_mhz() already cache the value in .bss. For via/epia-m850, switch romstage udelay() from UDELAY_IO to UDELAY_TSC. Change-Id: I1690cb80295d6b006b75ed69edea28899b674b68 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/arch/x86/cpu.c M src/arch/x86/timestamp.c M src/cpu/intel/fsp_model_406dx/Kconfig M src/cpu/intel/haswell/Kconfig M src/cpu/intel/model_1067x/Kconfig M src/cpu/intel/model_106cx/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_6ex/Kconfig M src/cpu/intel/model_6fx/Kconfig M src/cpu/intel/slot_1/Kconfig M src/cpu/intel/socket_mPGA604/Kconfig M src/cpu/qemu-x86/Kconfig M src/cpu/via/nano/Kconfig M src/cpu/x86/Kconfig M src/cpu/x86/tsc/Makefile.inc M src/cpu/x86/tsc/delay_tsc.c M src/drivers/pc80/pc/i8254.c M src/include/cpu/x86/tsc.h M src/northbridge/via/vx900/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/baytrail/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/denverton_ns/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/fsp_broadwell_de/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/quark/Kconfig M src/soc/intel/skylake/Kconfig 32 files changed, 43 insertions(+), 54 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/33928/12 -- To view, visit https://review.coreboot.org/c/coreboot/+/33928 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1690cb80295d6b006b75ed69edea28899b674b68 Gerrit-Change-Number: 33928 Gerrit-PatchSet: 12 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: David Guckian <david.guckian(a)intel.com> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Vanny E <vanessa.f.eusebio(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Subrata Banik (Code Review) 31 Oct '19

31 Oct '19

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Patrick Rudolph (Code Review) 31 Oct '19

31 Oct '19

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 ) Change subject: soc/intel/tigerlake: Do initial SoC commit ...................................................................... Patch Set 10: (1 comment) https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/lo… File src/soc/intel/tigerlake/lockdown.c: https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/lo… PS8, Line 20: > sounds good. i can write a document with those document number and chapter with file details. […] No, Inline comments only. -- To view, visit https://review.coreboot.org/c/coreboot/+/36087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id95e2fa9b7a7c6b3b9233d2c438b25a6c4904bbb Gerrit-Change-Number: 36087 Gerrit-PatchSet: 10 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Thu, 31 Oct 2019 14:34:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro(a)das-labor.org> Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Arthur Heymans (Code Review) 31 Oct '19

31 Oct '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 ) Change subject: soc/intel/tigerlake: Do initial SoC commit ...................................................................... Patch Set 10: (1 comment) https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/f… File src/soc/intel/tigerlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/f… PS10, Line 42: __weak > yes, because its not mandatory to have anyway mainboard override atleast few CRB i have seen where default UPDs are good for mainboard config Even then I don't think it's bad to have the mainboard explicitly do nothing. We have those callbacks that are empty but need a definition to please the linker else too in the codebase. For things as crucial as GPIO's I think its not bad for developers to be conscious about this. -- To view, visit https://review.coreboot.org/c/coreboot/+/36087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id95e2fa9b7a7c6b3b9233d2c438b25a6c4904bbb Gerrit-Change-Number: 36087 Gerrit-PatchSet: 10 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Thu, 31 Oct 2019 14:34:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Subrata Banik (Code Review) 31 Oct '19

31 Oct '19

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Subrata Banik (Code Review) 31 Oct '19

31 Oct '19

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 ) Change subject: soc/intel/tigerlake: Do initial SoC commit ...................................................................... Patch Set 10: (6 comments) https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/K… File src/soc/intel/tigerlake/Kconfig: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/K… PS10, Line 215: ENABLE_DISPLAY_OVER_EXT_PCIE_GFX > This seems to (partially) reinvent CONFIG_ONBOARD_VGA_IS_PRIMARY. […] oh yes, this has added to enable dGPU over PCIE usecase on icelake RVP which might be applicable for TGL as well. let me check if we can remove for now in TGL https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/b… File src/soc/intel/tigerlake/bootblock/report_platform.c: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/b… PS10, Line 29: : static struct { : u32 cpuid; : const char *name; : } cpu_table[] = { : { CPUID_ICELAKE_A0, "Icelake A0" }, : { CPUID_ICELAKE_B0, "Icelake B0" }, : }; : : static struct { : u16 mchid; : const char *name; : } mch_table[] = { : { PCI_DEVICE_ID_INTEL_ICL_ID_U, "Icelake-U" }, : { PCI_DEVICE_ID_INTEL_ICL_ID_U_2_2, "Icelake-U-2-2" }, : { PCI_DEVICE_ID_INTEL_ICL_ID_Y, "Icelake-Y" }, : { PCI_DEVICE_ID_INTEL_ICL_ID_Y_2, "Icelake-Y-2" }, : }; : : static struct { : u16 espiid; : const char *name; : } pch_table[] = { : { PCI_DEVICE_ID_INTEL_ICL_BASE_U_ESPI, "Icelake-U Base" }, : { PCI_DEVICE_ID_INTEL_ICL_BASE_Y_ESPI, "Icelake-Y Base" }, : { PCI_DEVICE_ID_INTEL_ICL_U_PREMIUM_ESPI, "Icelake-U Premium" }, : { PCI_DEVICE_ID_INTEL_ICL_U_SUPER_U_ESPI, "Icelake-U Super" }, : { PCI_DEVICE_ID_INTEL_ICL_U_SUPER_U_ESPI_REV0, "Icelake-U Super REV0" }, : { PCI_DEVICE_ID_INTEL_ICL_SUPER_Y_ESPI, "Icelake-Y Super" }, : { PCI_DEVICE_ID_INTEL_ICL_Y_PREMIUM_ESPI, "Icelake-Y Premium" }, : }; : : static struct { : u16 igdid; : const char *name; : } igd_table[] = { : { PCI_DEVICE_ID_INTEL_ICL_GT0_ULT, "Icelake ULT GT0" }, : { PCI_DEVICE_ID_INTEL_ICL_GT0_5_ULT, "Icelake ULT GT0.5" }, : { PCI_DEVICE_ID_INTEL_ICL_GT1_ULT, "Icelake U GT1" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_0, "Icelake Y GT2" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_1, "Icelake Y GT2_1" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_1, "Icelake U GT2_1" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_2, "Icelake Y GT2_2" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_2, "Icelake U GT2_2" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_3, "Icelake Y GT2_3" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_3, "Icelake U GT2_3" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_4, "Icelake Y GT2_4" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_4, "Icelake U GT2_4" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULX_5, "Icelake Y GT2_5" }, : { PCI_DEVICE_ID_INTEL_ICL_GT2_ULT_5, "Icelake U GT2_5" }, : { PCI_DEVICE_ID_INTEL_ICL_GT3_ULT, "Icelake U GT3" }, > Does this need updating? Yes, this CL will care of updating https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/f… File src/soc/intel/tigerlake/finalize.c: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/f… PS10, Line 118: : BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, soc_finalize, NULL); : BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_EXIT, soc_finalize, NULL); > Any reason to not make this a '. […] this is good feedback to handle this in .final = but only problem is that, this file is more miscellaneous IP/controller programming so might not be bonded with any IP. So may be chip.c ops.final i can try to see if we are meeting all requirement correctly. if yes then we can change https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/f… File src/soc/intel/tigerlake/fsp_params.c: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/f… PS10, Line 42: __weak > I'd not make this callback weak. […] yes, because its not mandatory to have anyway mainboard override atleast few CRB i have seen where default UPDs are good for mainboard config https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/i… File src/soc/intel/tigerlake/include/soc/gpio.h: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/i… PS10, Line 19: #include <soc/gpio_defs.h> : #include <intelblocks/gpio.h> : : #define CROS_GPIO_DEVICE_NAME "INT3455:00" > Is this file needed? Can headers be included directly? i believe this file is kind of wrapper for other mainboard and common code files to get gpio related API. soc/gpio.h is the expectation from other files https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/r… File src/soc/intel/tigerlake/romstage/fsp_params.c: https://review.coreboot.org/c/coreboot/+/36087/10/src/soc/intel/tigerlake/r… PS10, Line 26: __weak > Try to avoid __weak functions like that. […] true. as i have mentioned in soc weak implementation, its just to make sure incase mainboard doesn't have any override and mainboard_memory_init_params() is calling from fsp driver common code -- To view, visit https://review.coreboot.org/c/coreboot/+/36087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id95e2fa9b7a7c6b3b9233d2c438b25a6c4904bbb Gerrit-Change-Number: 36087 Gerrit-PatchSet: 10 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Thu, 31 Oct 2019 14:27:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Subrata Banik (Code Review) 31 Oct '19

31 Oct '19

1 0

Change in coreboot[master]: soc/intel/tigerlake: Do initial SoC commit
by Subrata Banik (Code Review) 31 Oct '19

31 Oct '19

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 ) Change subject: soc/intel/tigerlake: Do initial SoC commit ...................................................................... Patch Set 10: (1 comment) > Patch Set 9: > > > Sorry, but all companies and developers have to go through the process of submitting quality software stated in the coreboot trademark license agreement as well. Be prepared that this code only goes into the tree if the common code stuff is done before. > > I have to side with Intel on the matter of common code. While I > totally agree that we shouldn't duplicate too much code, we never > made maximizing common code a requirement for new platforms. And > it seems harsh to arbitrarily add that requirement when people feel > time pressure. Thanks Nico for understanding the problem what we are dealing with against timeline. > > That doesn't mean we shouldn't review the Tiger Lake addition. And > I think we all agreed already that a 8k LOC patch isn't reviewable. > And of course, if we had more common code already, it would be much > easier... and we would have a better code base if 3ee54bb hadn't > slipped in. Let's not repeat that error. Sure, i believe Arthur has done great work of taking pain to review this CL. we have address majority of his feedback and few more is there. will continue to address also will see if we can submit this CL into 4 part 1. Bootblock (soc/intel/tigerlake/bootblock/) 2. romstage (soc/intel/tigerlake/romstage/) 3. ramstage (soc/intel/tigerlake/) 4. acpi (soc/intel/tigerlake/acpi/) Also i was just looking at CLOC inside TGL directory with this newer patchset. Looks like we have only ~4K LOC inside each soc but that doesn't mean we can't do further common code. common SMM is something we are doing and common report platform implementation also there. Let me revive those CL. But i will hope to get your support after i split this CL into 4 CL as mentioned above for further review and possibility merge by this week (please) src/soc/intel/tigerlake$ cloc . 73 text files. 73 unique files. 14 files ignored. http://cloc.sourceforge.net v 1.60 T=0.28 s (211.0 files/s, 21877.3 lines/s) ------------------------------------------------------------------------------- Language files blank comment code ------------------------------------------------------------------------------- C 30 460 957 2228 C/C++ Header 28 285 610 1559 Pascal 1 1 0 18 ------------------------------------------------------------------------------- SUM: 59 746 1567 3805 ------------------------------------------------------------------------------- https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/lo… File src/soc/intel/tigerlake/lockdown.c: https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/lo… PS8, Line 20: > You can still reference the document number and chapter, even if it's not published yet. […] sounds good. i can write a document with those document number and chapter with file details. Will that help? -- To view, visit https://review.coreboot.org/c/coreboot/+/36087 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id95e2fa9b7a7c6b3b9233d2c438b25a6c4904bbb Gerrit-Change-Number: 36087 Gerrit-PatchSet: 10 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Michael Niewöhner Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Thu, 31 Oct 2019 13:58:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <siro(a)das-labor.org> Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2019