Michael has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44167 )
Change subject: mb/acer/m3800: add Acer Aspire M3800 desktop ......................................................................
Patch Set 2:
(12 comments)
https://review.coreboot.org/c/coreboot/+/44167/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44167/1//COMMIT_MSG@30 PS1, Line 30: Core2
Core 2
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... File src/mainboard/acer/m3800/Kconfig:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... PS1, Line 28: config MMCONF_BASE_ADDRESS : hex : default 0xe0000000
Shouldn't be needed
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... PS1, Line 40: config MAX_CPUS : int : default 4
Was moved to common Kconfig
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... File src/mainboard/acer/m3800/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... PS1, Line 2: M3800
Also add the mainboard codename?
Good idea, changed it to "M3800 / G43T-AM3". Could leave out the spaces?
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... File src/mainboard/acer/m3800/cmos.layout:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... PS1, Line 6: # ----------------------------------------------------------------- : # Status Register A : # ----------------------------------------------------------------- : # Status Register B : # ----------------------------------------------------------------- : # Status Register C : #96 4 r 0 status_c_rsvd : #100 1 r 0 uf_flag : #101 1 r 0 af_flag : #102 1 r 0 pf_flag : #103 1 r 0 irqf_flag : # ----------------------------------------------------------------- : # Status Register D : #104 7 r 0 status_d_rsvd : #111 1 r 0 valid_cmos_ram : # ----------------------------------------------------------------- : # Diagnostic Status Register : #112 8 r 0 diag_rsvd1
I'd drop this
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... PS1, Line 27: #120 264 r 0 unused
Also drop commented-out unused entries please
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... PS1, Line 90: 11 10 160M : 11 11 224M : 11 12 352M
Argh! alignment!
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/de... File src/mainboard/acer/m3800/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/de... PS1, Line 144: device pci 1f.1 on end # PATA/IDE
Really?
Certainly not. Looking at cbmem I can see "PCI: Static device PCI: 00:1f.1 not found, disabling it". Line removed.
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... File src/mainboard/acer/m3800/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... PS1, Line 10: 0x20090419 // OEM revision I copied this from the Intel DG43GT. Should I put the "Release Date" from dmidecode there?
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... PS1, Line 17: Scope (_SB) { : Device (PCI0) : {
I use `Device (_SB. […]
Neat, I like that. On the other hand the verbosity seems useful when you're not familiar with ASL yet (like me). What weighs more?
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... PS1, Line 22: #include <drivers/intel/gma/acpi/default_brightness_levels.asl>
Not necessary anymore
Done
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/gp... File src/mainboard/acer/m3800/gpio.c:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/gp... PS1, Line 87: static const struct pch_gpio_set3 pch_gpio_set3_mode = { }; : : static const struct pch_gpio_set3 pch_gpio_set3_direction = { }; : : static const struct pch_gpio_set3 pch_gpio_set3_level = { };
not needed for ICH
Done