Skoll RC has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40813 )
Change subject: mb/ongy/h61m-s1: Add new mainboard ......................................................................
Patch Set 15: Code-Review+1
(19 comments)
Done with most of comments, I just need some answers for some files and I think it starts to be good
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG@7 PS14, Line 7: mb/ongy/h61m-s1: Add new mainboard
Add an empty line between the commit summary and the commit message: […]
Done
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG@15 PS14, Line 15: Integrated VGA and HDMI works
Was this with libgfxinit? (the default graphics init for most Intel iGPUs)
Yes, I mentioned it in a new version of this file
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG@17 PS14, Line 17: Seabios
SeaBIOS
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/Kconfig:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 26: config VGA_BIOS_FILE : string : default "pci8086,0112.rom" : : config VGA_BIOS_ID : string : default "8086,0112"
Remove this, it depends on the installed CPU
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 34: onfig DRAM_RESET_GATE_GPIO : int : default 60
This is not correct. GPIO60 is configured as native mode on your board. […]
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 8: /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0;
This does nothing. […]
So must I remove all line and let a blank function or revmove everthing an let an empty file?
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 5: register "gfx" = "GMA_STATIC_DISPLAYS(0)" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "4" : register "gpu_dp_d_hotplug" = "4" : register "gpu_panel_power_cycle_delay" = "4"
These settings aren't needed for a desktop board
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 10: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 18: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 22: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 25: register "docking_supported" = "0"
This defaults to zero already. […]
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 27: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }"
This defaults to zero already. […]
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 64: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x8086 0x0100 : end : device pci 01.0 on # PEG : subsystemid 0x8086 0x0101 : end : device pci 02.0 on # iGPU : subsystemid 0x8086 0x2010 : end
Please move these entries above the southbridge block. That is, before this line: […]
Done
https://review.coreboot.org/c/coreboot/+/40813/10/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40813/10/src/mainboard/ongy/h61m-s1... PS10, Line 4: #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
It's only used on Lenovo mainboards. […]
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 4: #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
Unused, can remove
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 22: g
Capitalize: "Global"
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 29: #include <drivers/intel/gma/acpi/default_brightness_levels.asl>
Not needed for a desktop board without an integrated panel
Done
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 10: private package GMA.Mainboard is
Almost all the gma_mainboard.ads files in coreboot are indented with three spaces, instead of tabs. […]
Done (I think :-) )
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/mainboard.c:
PS14:
If you don't plan on using the VGA BIOS, I would remove this and the `select INTEL_INT15` line from […]
You mean remove everything? My first build was with libgfxinit indeed