Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38341 )
Change subject: soc/intel/tigerlake: update pci dev definition ......................................................................
Patch Set 5:
(14 comments)
https://review.coreboot.org/c/coreboot/+/38341/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38341/5//COMMIT_MSG@7 PS5, Line 7: u nit: Update
https://review.coreboot.org/c/coreboot/+/38341/5//COMMIT_MSG@9 PS5, Line 9: Update nit: This change updates
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.c:
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 54: case 9: return "HS10"; acpi/xhci.asl lists upto HS12 devices, but there are only devices upto HS10 here.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 63: case 3: return "SS04"; acpi/xhci.asl lists upto SS06 devices, but there are only devices upto SS04 here.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 72: devfn I see a number of devices that do not have their corresponding device in any .asl file. Shouldn't the .asl files be updated as well?
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 74: GFX0 Shouldn't there be a corresponding device in .asl file somewhere?
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 75: ISHB same here.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 108: case PCH_DEVFN_GSPI3: return "SPI3"; I believe this device should be added to serialio.asl as well.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/chi... PS5, Line 116: IGBE Shouldn't this be GLAN as per pch_glan.asl
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... PS5, Line 47: PCH_DEV_SLOT_TBT I don't think this is a PCH device.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... PS5, Line 57: SIO4 It doesn't matter much technically. But I believe SIO4 here refers to Serial I/O 4. Shouldn't this be numbered in order i.e. SIO1 here and then continue with SIO2 and so on.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... PS5, Line 71: #define PCH_DEVFN_THERMAL _PCH_DEVFN(ISH, 1) I don't see this in the EDS.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... PS5, Line 85: iWIFI I think it would be better to keep this as CNVI_WIFI just like CNVI_BT done above in line 61.
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... PS5, Line 89: PCH_DEV_CNViWIFI same here.