Derek Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43061 )
Change subject: soc/intel/tigerlake: Update Tiger Lake SA IDs ......................................................................
Patch Set 13:
(20 comments)
Patch Set 2:
(1 comment)
Is there an updated sightings document that lists the most current PCI IDs?
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG@2 PS1, Line 2: derek.huang
Please use *Derek Huang*. […]
Done
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG@7 PS1, Line 7: Tigerlake
Tiger Lake
Done
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG@9 PS1, Line 9: update
updates
Done
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG@9 PS1, Line 9: Tigerlake
Tiger Lake
Done
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG@13 PS1, Line 13:
Please mention that the ID of the one device is changed. […]
Done
https://review.coreboot.org/c/coreboot/+/43061/1//COMMIT_MSG@14 PS1, Line 14: derek.huang derek.huang@intel.corp-partner.google.com
why not @intel. […]
Ack
https://review.coreboot.org/c/coreboot/+/43061/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43061/2//COMMIT_MSG@12 PS2, Line 12: 9A02h
I did not find any code change in your CL for 9a02. […]
Done
https://review.coreboot.org/c/coreboot/+/43061/2//COMMIT_MSG@14 PS2, Line 14: Signed-off-by: derek.huang derek.huang@intel.corp-partner.google.com
remove extra signed-off-by tag
Done
https://review.coreboot.org/c/coreboot/+/43061/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43061/8//COMMIT_MSG@11 PS8, Line 11: TGL-UP4(Y) (4+2): Change PCI_DEVICE_ID_INTEL_TGL_ID_Y to 0x9A12h
Append this accordingly *_Y_4_2 as per code changes.
Done
https://review.coreboot.org/c/coreboot/+/43061/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43061/10//COMMIT_MSG@10 PS10, Line 10: #626936 and doc #630389
I am unable to find these docs anywhere even on RDC, but if UP4 is the -Y and UP3 is -U, then it agr […]
Done
https://review.coreboot.org/c/coreboot/+/43061/12/src/include/device/pci_ids... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/43061/12/src/include/device/pci_ids... PS12, Line 3524: #define PCI_DEVICE_ID_INTEL_TGL_ID_U_4_2 0x9A14 : #define PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2 0x9A04 : #define PCI_DEVICE_ID_INTEL_TGL_ID_Y_4_2 0x9A12 : #define PCI_DEVICE_ID_INTEL_TGL_ID_Y_2_2 0x9A02
might as well apply _2_4 before _4_4 ordering here as well for consistency.
Done. I guess you mean _2_2 before _4_2
https://review.coreboot.org/c/coreboot/+/43061/10/src/include/device/pci_ids... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/43061/10/src/include/device/pci_ids... PS10, Line 3525: #define PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2 0x9A04
Please order 4_2 after 2_2.
Done
https://review.coreboot.org/c/coreboot/+/43061/10/src/include/device/pci_ids... PS10, Line 3527: #define PCI_DEVICE_ID_INTEL_TGL_ID_Y_2_2 0x9A02
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/43061/2/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/43061/2/src/include/device/pci_ids.... PS2, Line 3527: 0x9A10
any reason for removing this 0x9A10 entry which is for Y segment SKU ?
Done
https://review.coreboot.org/c/coreboot/+/43061/2/src/include/device/pci_ids.... PS2, Line 3525: #define PCI_DEVICE_ID_INTEL_TGL_ID_Y 0x9A12 : #define PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2 0x9A04
could you swap these two lines so that the _Y/_U IDs are grouped?
Done
https://review.coreboot.org/c/coreboot/+/43061/4/src/soc/intel/tigerlake/boo... File src/soc/intel/tigerlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/43061/4/src/soc/intel/tigerlake/boo... PS4, Line 36: Tigerlake-U-4-3e
any reason for removing this ?
Please check the changes in src/include/device/pci_ids.h in the old code, PCI_DEVICE_ID_INTEL_TGL_ID_U_1 is defined as 0x9A12, but 0x9A12 is the ID for TGL-Y(4+2). So PCI_DEVICE_ID_INTEL_TGL_ID_U_1 is replaced by PCI_DEVICE_ID_INTEL_TGL_ID_Y
https://review.coreboot.org/c/coreboot/+/43061/5/src/soc/intel/tigerlake/boo... File src/soc/intel/tigerlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/43061/5/src/soc/intel/tigerlake/boo... PS5, Line 35: PCI_DEVICE_ID_INTEL_TGL_ID_U
Would you mind to append this *_4_2 to match with name string. […]
Done
https://review.coreboot.org/c/coreboot/+/43061/5/src/soc/intel/tigerlake/boo... PS5, Line 37: PCI_DEVICE_ID_INTEL_TGL_ID_Y
same comment as above.
Done
https://review.coreboot.org/c/coreboot/+/43061/12/src/soc/intel/tigerlake/bo... File src/soc/intel/tigerlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/43061/12/src/soc/intel/tigerlake/bo... PS12, Line 35: { PCI_DEVICE_ID_INTEL_TGL_ID_U_4_2, "Tigerlake-U-4-2" }, : { PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2, "Tigerlake-U-2-2" }, : { PCI_DEVICE_ID_INTEL_TGL_ID_Y_4_2, "Tigerlake-Y-4-2" }, : { PCI_DEVICE_ID_INTEL_TGL_ID_Y_2_2, "Tigerlake-Y-2-2" },
also reorder -2-2 before -4-2 here.
Done
https://review.coreboot.org/c/coreboot/+/43061/12/src/soc/intel/tigerlake/sy... File src/soc/intel/tigerlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/43061/12/src/soc/intel/tigerlake/sy... PS12, Line 81: case PCI_DEVICE_ID_INTEL_TGL_ID_U_4_2: : soc_config = &config->power_limits_config[POWER_LIMITS_U_4_CORE]; : break; : case PCI_DEVICE_ID_INTEL_TGL_ID_U_2_2: : soc_config = &config->power_limits_config[POWER_LIMITS_U_2_CORE]; : break;
_2_2 before _4_2 here as well.
Done