Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35030 )
Change subject: soc/intel/skylake: Add Lewisburg family PCH support ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 2717: is this a gerrit rendering bug or doe these lines miss one tab?
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 2815: 0xa19f 0xa19a (last digit is wrong)
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 2805: #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP1 0xa190 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP2 0xa191 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP3 0xa192 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP4 0xa193 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP5 0xa194 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP6 0xa195 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP7 0xa196 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP8 0xa197 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP9 0xa198 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP10 0xa199 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP11 0xa19f : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP12 0xa19b : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP13 0xa19c : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP14 0xa19d : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP15 0xa19e : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP16 0xa19f : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP17 0xa1e7 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP18 0xa1e8 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP19 0xa1e9 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP20 0xa1ea : : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP1_SUPER 0xa210 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP2_SUPER 0xa211 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP3_SUPER 0xa212 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP4_SUPER 0xa213 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP5_SUPER 0xa214 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP6_SUPER 0xa215 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP7_SUPER 0xa216 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP8_SUPER 0xa217 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP9_SUPER 0xa218 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP10_SUPER 0xa219 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP11_SUPER 0xa21a : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP12_SUPER 0xa21b : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP13_SUPER 0xa21c : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP14_SUPER 0xa21d : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP15_SUPER 0xa21e : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP16_SUPER 0xa21f : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP17_SUPER 0xa267 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP18_SUPER 0xa268 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP19_SUPER 0xa269 : #define PCI_DEVICE_ID_INTEL_LWB_PCIE_RP20_SUPER 0xa26a should those pcie port names be 0-indexed or 1-indexed? in the datasheet they are 0-indexed, but in the existing code they are 1-indexed
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 2951: #define PCI_DEVICE_ID_INTEL_LWB_SATA_AHCI 0xa182 : #define PCI_DEVICE_ID_INTEL_LWB_SATA_RAID 0xa186 : #define PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI 0xa1d2 : #define PCI_DEVICE_ID_INTEL_LWB_SSATA_RAID 0xa1d6 : #define PCI_DEVICE_ID_INTEL_LWB_SATA_AHCI_SUPER 0xa202 : #define PCI_DEVICE_ID_INTEL_LWB_SATA_RAID_SUPER 0xa206 : #define PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI_SUPER 0xa252 : #define PCI_DEVICE_ID_INTEL_LWB_SSATA_RAID_SUPER 0xa256 there are some alternate PCI IDs for those in the datasheet; should they also be added here?
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 3221: #define PCI_DEVICE_ID_INTEL_KBP_H_SMBUS 0xa1a3 maybe rename this to PCI_DEVICE_ID_INTEL_KBP_H_LWB_SMBUS ?
https://review.coreboot.org/c/coreboot/+/35030/10/src/soc/intel/common/block... File src/soc/intel/common/block/sata/sata.c:
https://review.coreboot.org/c/coreboot/+/35030/10/src/soc/intel/common/block... PS10, Line 78: PCI_DEVICE_ID_INTEL_LWB_SATA_AHCI, : PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI, : PCI_DEVICE_ID_INTEL_LWB_SATA_AHCI_SUPER, : PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI_SUPER, see my comments about the alternate IDs in the datasheet
https://review.coreboot.org/c/coreboot/+/35030/10/src/soc/intel/skylake/boot... File src/soc/intel/skylake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/35030/10/src/soc/intel/skylake/boot... PS10, Line 97: Sku SKU (I'd write this in all upper case, since it's an acronym)