Maxim Polyakov 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 11:
(19 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:
I think the file goes up and down a lot w.r.t. tabulation... It happens.
Most likely I just forgot to add a tab after the label was removed.
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 2815: 0xa19f
0xa19a (last digit is wrong)
Fixed
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 […]
I think it would be better not to change the macro style for PCIE IDs to avoid confusion. All these IDs starts with PCIE1
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?
Alternative SKU sounds even more mysterious than SUPER SKU :) But since I have already added SUPER, I will add all the others
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 ?
Ok, fixed
https://review.coreboot.org/c/coreboot/+/35030/10/src/include/device/pci_ids... PS10, Line 3244: #define PCI_DEVICE_ID_INTEL_LWB_P2SB 0xa1a0 : #define PCI_DEVICE_ID_INTEL_LWB_P2SB_SUPER 0xa220 Should I add this ID to https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/... Skylake doesn't use the p2sb driver
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 2717: _PROD
I'd also drop the _PROD here and in the following lines
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 2805: _PROD
same here and following
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 2951: _PROD
same
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 2974: _PROD
same
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 3080: _PROD
same
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 3232: _PROD
same
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 3243: _PROD
same
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 3264: _PROD
same
Done
https://review.coreboot.org/c/coreboot/+/35030/8/src/include/device/pci_ids.... PS8, Line 3277: _PROD
same
Done
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 81: PCI_DEVICE_ID_INTEL_LWB_SSATA_AHCI_SUPER, Should I add PCI_DEVICE_ID_INTEL_LWB_(S)SATA_RAID_* here?
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
I have added alternate IDs
https://review.coreboot.org/c/coreboot/+/35030/10/src/soc/intel/common/block... File src/soc/intel/common/block/smbus/smbus.c:
https://review.coreboot.org/c/coreboot/+/35030/10/src/soc/intel/common/block... PS10, Line 96: PCI_DEVICE_ID_INTEL_KBP_H_SMBUS renamed to PCI_DEVICE_ID_INTEL_KBP_H_LWB_SMBUS
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)
Fixed