Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40851 )
Change subject: soc/intel/elkhartlake: Add Elkhart Lake SoC support ......................................................................
Patch Set 1:
(15 comments)
Thank you for starting this integration. Since this patch is meant to be a copy of Jasperlake I understand that there are a bunch of JSL references still in the code. I have marked a few places where it could be corrected already in this patch but feel free to do this in a follow up commit.
Regarding the license header: I would take care of SPDX in this patch already so that you have a clean starting point in this field.
And for all the mentioned PCI device paths in the comments: consider them just as a remark so that you can find them easier in the follow up patches.
https://review.coreboot.org/c/coreboot/+/40851/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40851/1//COMMIT_MSG@14 PS1, Line 14: difference differences
https://review.coreboot.org/c/coreboot/+/40851/1//COMMIT_MSG@25 PS1, Line 25: port are "Number of USB ports is different"
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/K... File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/K... PS1, Line 160: JSL Since you have already modified Kconfig for EHL: mind to change it here as well?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/camera_clock_ctl.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 1: /* Switch to SPDX format here?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/ish.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 4: 0:12.0 Is this still valid for EHL?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/pch_glan.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 4: 0:1f.6 Is this still true for Elkhartlake? If not please note it down for the follow up patches to correct.
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/pch_hda.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 4: Device 31, Function 3 Still valid for EHL?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 1: /* : * This file is part of the coreboot project. : * : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; either version 2 of the License, or : * (at your option) any later version. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ Is there a reason why you stick here with the old header instead of SPDX like the other .asl-files?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/pcie.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 238: 0x001D0000 I guess this is not valid, too for EHL (or does EHL have a PCIe root complex on 00:1d.n)?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/southbridge.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 1: /* : * This file is part of the coreboot project. : * : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; either version 2 of the License, or : * (at your option) any later version. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ Same here, please switch to SPDX format.
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 51: ISH 0:12.0 Does this still apply for EHL?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... File src/soc/intel/elkhartlake/acpi/xhci.asl:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/a... PS1, Line 28: Root Hub for Jasperlake PCH Mind to rename it to Elkhartlake?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/b... File src/soc/intel/elkhartlake/bootblock/cpu.c:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/b... PS1, Line 10: Jasperlake Elkhartlake
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/r... File src/soc/intel/elkhartlake/romstage/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/r... PS1, Line 1: # : # This file is part of the coreboot project. : # : # : # This program is free software; you can redistribute it and/or modify : # it under the terms of the GNU General Public License as published by : # the Free Software Foundation; version 2 of the License. : # : # This program is distributed in the hope that it will be useful, : # but WITHOUT ANY WARRANTY; without even the implied warranty of : # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : # GNU General Public License for more details. : # Switch to SPDX format here as well?
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/s... File src/soc/intel/elkhartlake/spi.c:
https://review.coreboot.org/c/coreboot/+/40851/1/src/soc/intel/elkhartlake/s... PS1, Line 1: /* SPDX format here as well?