Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29470 )
Change subject: src/mainboard/portwell/m107: Do initial mainboard commit ......................................................................
Patch Set 8:
(23 comments)
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... File Documentation/mainboard/portwell/pq7-m107.md:
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... PS8, Line 5: ## Required blobs How about microcode state? Does the board run with microcode generated from tree?
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... PS8, Line 11: You may also consider committing a miniconfig to the configs/ directory.
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... PS8, Line 39: The main SPI flash can be accessed using [flashrom]. Please clean up the obsolete whitespaces at the end of lines.
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... PS8, Line 43: The flash chip is a 64 MiB socketed DIP-8 chip. Specifically, it's a W25Q64FW is a 64 Megabit flash = 8MiB. Please correct.
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... PS8, Line 56: - flashing coreboot How did You verify Your patch then if not even tested flashing coreboot?
https://review.coreboot.org/#/c/29470/8/Documentation/mainboard/portwell/pq7... PS8, Line 59: - Full Embedded controller support Embedded Controller
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/Kconfig File src/mainboard/portwell/m107/Kconfig:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/Kconfig@... PS8, Line 55: config ROM_SIZE Already set to 8MiB in the board specific options. Any reasoning for duplicating the setting?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/Kconfig@... PS8, Line 64: config SPI_FLASH_INCLUDE_ALL_DRIVERS Would be great to have all these optimizations in a miniconfig file.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/Makefile... File src/mainboard/portwell/m107/Makefile.inc:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/Makefile... PS8, Line 26: ## Please select GENERIC_SPD_BIN option in the board specific options and define SPD_SOURCES variable with the name of SPD file without spd.hex extension. And also get rid of CONFIG_MAINBOARD_SPD0_FILE_NAME in Kconfig file then. It will do the same magic as You have written here.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/acpi/dpt... File src/mainboard/portwell/m107/acpi/dptf.asl:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/acpi/dpt... PS8, Line 1: /* Is this ASL file included anywhere? If not, what is the reasoning of committing the file?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/acpi/sup... File src/mainboard/portwell/m107/acpi/superio.asl:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/acpi/sup... PS8, Line 21: Device (COM1) { Why it is indented?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/board_in... File src/mainboard/portwell/m107/board_info.txt:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/board_in... PS8, Line 3: Category: misc Isn't it SBC?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/devicetr... File src/mainboard/portwell/m107/devicetree.cb:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/devicetr... PS8, Line 27: register "PcdEnableLpe" = "1" LPE is enabled here, but not described in domain 0. What is the intention of setting it to enabled?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/devicetr... PS8, Line 43: register "PcdEnableSata" = "0" # Disable SATA The product specification on Portwell site says there are 2x SATA ports. Does carrier board not have the SATA ports?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/devicetr... PS8, Line 75: register "DptfDisable" = "1" Here DPTF is disabled so there is less sense to have the DPTF ASL code in the mainboard tree.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/devicetr... PS8, Line 99: device pci 0b.0 on end # 8086 22DC - PUNIT/DPTF Now the DPTF is on? I am lost. What is Your intention? Please elaborate.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/devicetr... PS8, Line 102: device pci 14.0 on end # 8086 22b5 - USB XHCI - Only 1 USB controller at a time Maybe consider declaring SATA controller as disabled here explicitly.
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/dsdt.asl File src/mainboard/portwell/m107/dsdt.asl:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/dsdt.asl... PS8, Line 36: #include <acpi/cpu.asl> Intel board should have unified CPU ASL code as introduced by the patch https://review.coreboot.org/c/coreboot/+/29894:
<cpu/intel/common/acpi/cpu.asl>
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/fadt.c File src/mainboard/portwell/m107/fadt.c:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/fadt.c@3... PS8, Line 36: fadt->model = 1; model field is no longer used AFAIK
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/gpio.c File src/mainboard/portwell/m107/gpio.c:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/gpio.c@1... PS8, Line 18: #include "irqroute.h" Is this include needed?
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/gpio.c@2... PS8, Line 258: Empty line, please remove
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/romstage... File src/mainboard/portwell/m107/romstage.c:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/romstage... PS8, Line 33: ps->spd_data_ch0 = cbfs_boot_map_with_leak(buf, CBFS_TYPE_SPD, NULL); With GENERIC_SPD_BIN option the function will be: cbfs_boot_map_with_leak("spd.bin", CBFS_TYPE_SPD, NULL);
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/smihandl... File src/mainboard/portwell/m107/smihandler.c:
https://review.coreboot.org/#/c/29470/8/src/mainboard/portwell/m107/smihandl... PS8, Line 61: case ACPI_S3: The board does not seem to support S3 sleep state, only S4. Why this case exists then?
-
Michał Żygowski (Code Review)