Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32346 )
Change subject: Add support for the Fujitsu E900 with autoport generated sources ......................................................................
Patch Set 1:
(21 comments)
Looks like it's missing all the SuperIO-related stuff, plus PCIe ports, plus libgfxinit.
https://review.coreboot.org/#/c/32346/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32346/1//COMMIT_MSG@10 PS1, Line 10: PS/2 does not work yet.
Likely a Super I/O issue.
It definitely is.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/K... File src/mainboard/fujitsu/esprimo_e900/Kconfig:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/K... PS1, Line 38: # FIXME: check this How to check if this is correct: suspend/resume should work.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/M... File src/mainboard/fujitsu/esprimo_e900/Makefile.inc:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/M... PS1, Line 2: You may want to hook up libgfxinit.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/a... File src/mainboard/fujitsu/esprimo_e900/acpi_tables.c:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/a... PS1, Line 30: : // the lid is open by default. : gnvs->lids = 1; Which lid? I think this isn't a laptop.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/b... File src/mainboard/fujitsu/esprimo_e900/board_info.txt:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/b... PS1, Line 4: FIXME: check category, , put ROM package, ROM socketed, Release year FIXME
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... File src/mainboard/fujitsu/esprimo_e900/devicetree.cb:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 3: register "gfx.link_frequency_270_mhz" = "0" Can be removed
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 5: register "gfx.use_spread_spectrum_clock" = "0" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "4" : register "gpu_dp_d_hotplug" = "4" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "4" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000" Can be removed
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 25: device lapic 0x0 on : end : device lapic 0xacac off : end Please put the "end" on the same line
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 31: : Add this line here:
subsystemid 0x1734 0x11ba inherit
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 37: register "gen3_dec" = "0x00000000" : register "gen4_dec" = "0x00000000" Can be removed
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 40: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Can be removed
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 49: device pci 16.1 off # Management Engine Interface 2 : end Please move the "end" to the previous line
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 59: subsystemid 0x1734 0x11ba After doing what the comment on line 32 says, lines like this one with the same subsystemid can be removed.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 64: device pci 1c.0 off # PCIe Port #1 : end : device pci 1c.1 off # PCIe Port #2 : end : device pci 1c.2 off # PCIe Port #3 : end : device pci 1c.3 off # PCIe Port #4 : end : device pci 1c.4 off # PCIe Port #5 : end : device pci 1c.5 off # PCIe Port #6 : end : device pci 1c.6 off # PCIe Port #7 : end : device pci 1c.7 off # PCIe Port #8 : end Does that board have any PCIe ports? It's weird none are enabled.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 86: device pci 1f.0 on # LPC bridge PCI-LPC bridge There should be something about the SuperIO here.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 100: Host bridge Duplicated comment
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 100: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x1734 0x11b9 : end : device pci 01.0 off # PCIe Bridge for discrete graphics : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x1734 0x11b9 : end This block can be moved above the "chip southbridge/intel/bd82x6x" line
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... File src/mainboard/fujitsu/esprimo_e900/dsdt.asl:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/d... PS1, Line 15: /* Some generic macros */ Please remove this comment.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/m... File src/mainboard/fujitsu/esprimo_e900/mainboard.c:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/m... PS1, Line 8: GMA_INT15_ACTIVE_LFP_INT_LVDS This seems wrong.
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/r... File src/mainboard/fujitsu/esprimo_e900/romstage.c:
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/r... PS1, Line 39: pci_write_config32(PCI_DEV(0, 0x1f, 0), 0x8c, 0x00000000); : pci_write_config32(PCI_DEV(0, 0x1f, 0), 0x90, 0x00000000); These can be removed
https://review.coreboot.org/#/c/32346/1/src/mainboard/fujitsu/esprimo_e900/r... PS1, Line 73: /* FIXME: Put proper SPD map here. */ How many RAM slots does this board have?