Frans Hendriks 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:
(15 comments)
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. […]
Set to force 8MiB size. Just tested and noticed that 8MiB is default even without this setting. Will be removed in new patch
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.
Will look into this.
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?
Copy/paste from Intel Strago. This file in not included in build. Will remove it in new patch
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?
The board contains a ITE8528. UART0 of this device is enabled and used as console (output)
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?
It is a Q7 module. Where can I find possible catagories?
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. […]
Should be disabled.
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. […]
Carrier did not have SATA ports (at that stage). Meanwhile we have mod the HW to test with SATA port. Will enable in new patch
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.
DPTF should be disabled
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.
PUNIT device is by code. Should be disabled here also.
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.
Since SATA is tested now. Will enabled SATA
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. […]
Will update in new patch
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
Will update in new patch.
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?
No. Will remove it in new patch
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: […]
Will implement this
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. […]
Copy/Paste from Intel Strago