Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38341 )
Change subject: soc/intel/tigerlake: update pci dev definition ......................................................................
Patch Set 6:
(9 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
Ack
https://review.coreboot.org/c/coreboot/+/38341/5//COMMIT_MSG@9 PS5, Line 9: Update
nit: This change updates
Ack
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 108: case PCH_DEVFN_GSPI3: return "SPI3";
I believe this device should be added to serialio.asl as well.
Yes, it's in serialio.asl : https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac...
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. […]
Ack
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.
Ack
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. […]
This is based on or order of Device id(0x10, 0x11, etc)
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.
It just added dummy number(unused device) for avoid compile failure and it'll not be used in finalize.c Compile failure happen because this is used in SOC_INTEL_COMMON_BLOCK_THERMAL code and SOC_INTEL_COMMON_BLOCK_THERMAL is selected in SOC_INTEL_COMMON_PCH_BASE. We're trying to find better way to clean this.
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.
Ack
https://review.coreboot.org/c/coreboot/+/38341/5/src/soc/intel/tigerlake/inc... PS5, Line 89: PCH_DEV_CNViWIFI
same here.
Ack