Iru Cai (vimacs) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40300 )
Change subject: mainboard: add Dell Latitude E7240 ......................................................................
Patch Set 2:
(10 comments)
Recreate the code with the logs dumped from an i5 laptop (previously it's using an i3 CPU). The GPIO and USB settings have changed.
https://review.coreboot.org/c/coreboot/+/40300/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40300/1//COMMIT_MSG@12 PS1, Line 12: It just boots from USB with SeaBIOS and runs for more than one hour.
Wow. […]
Ack
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 9: /* 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;
Already zero
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 17: // the lid is open by default.
That looks like old autoport: […]
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 23: end
Pick these up and place them on the previous line?
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 33: register "gen4_dec" = "0x00000000"
Already zero, can be dropped
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 46: subsystemid 0x1028 0x05ca
Inherit this?
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 100: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x1028 0x05ca : end : device pci 01.0 off # PCIe Bridge for discrete graphics : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x1028 0x05ca : end : device pci 03.0 on # Mini-HD audio Unsupported PCI device 8086:0a0c : subsystemid 0x1028 0x05ca : end
Move these above the "chip southbridge/intel/lynxpoint" entry?
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 1: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB
Missing license header, and these are likely not being used
Done
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/ec.c:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 77: static int write_send(u8 *sdata, u8 c, unsigned count)
Please address
Now this file is moved to ec/ in another patch.
https://review.coreboot.org/c/coreboot/+/40300/2/src/superio/smsc/Makefile.i... File src/superio/smsc/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40300/2/src/superio/smsc/Makefile.i... PS2, Line 16: subdirs-y += ece5048 gosh, I'm not going to add this yet...
-
Iru Cai (vimacs) (Code Review)