[coreboot-gerrit] [L] Change in coreboot[main]: mainboard: add Dell Latitude E7240

[Iru Cai (Code Review)]

Attention is currently required from: Arthur Heymans, Nicholas Chin.

Iru Cai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79746?usp=email )

Change subject: mainboard: add Dell Latitude E7240
......................................................................

Patch Set 3:

(13 comments)

Patchset:

PS2:
> I noticed that your old MEC5055 patch in CB:44975 which your old E7240 port used sent a 0xAB command […]

I didn't know what the effect of this command is, just found it in the vendor firmware.

File Documentation/mainboard/dell/e7240.md:

https://review.coreboot.org/c/coreboot/+/79746/comment/a39fe40b_163b08c2 :
PS2, Line 29: a programmer
> Maybe make this more clear by using "an external programmer"

Done

https://review.coreboot.org/c/coreboot/+/79746/comment/0254b434_b58567c1 :
PS2, Line 41: Schematic of this laptop can be found on [Lab One]. The board name is Compal LA-9431P.
> It's not a formally written policy but it's probably not good idea for coreboot to link to documents […]

removed

https://review.coreboot.org/c/coreboot/+/79746/comment/e4b0df7f_33a82d11 :
PS2, Line 81: 
            : [Lab One]: https://www.laboneinside.com/dell-latitude-e7240-schematic-diagram/
            :
> See prior comment about the schematic link

Done

File src/mainboard/dell/e7240/Kconfig:

https://review.coreboot.org/c/coreboot/+/79746/comment/a99c069c_cea00ea9 :
PS2, Line 5: BOARD_ROMSIZE_KB_8192
> Is this correct? The E7240 appears to have 2 flash chips, an 8MiB and a 4MiB

I can see one chip in my board, another place that can solder a flash chip is empty. The image is uploaded.

https://review.coreboot.org/c/coreboot/+/79746/comment/bb436704_fd6441d0 :
PS2, Line 17: string
> Not necessary, as the type is already defined in `mb/Kconfig`

Done

https://review.coreboot.org/c/coreboot/+/79746/comment/9e0c3720_a0d699b5 :
PS2, Line 21: string
> Same here, can be removed

Done

https://review.coreboot.org/c/coreboot/+/79746/comment/93e93223_c4556ff6 :
PS2, Line 24: config VGA_BIOS_FILE
            : 	string
            : 	default "pci8086,0a16.rom"
> Remove, see commit 05ae8f2ff3 (mainboard: Drop invalid `VGA_BIOS_FILE` defaults).

Done

https://review.coreboot.org/c/coreboot/+/79746/comment/08238363_cc4e1ab0 :
PS2, Line 29: string
> Can be removed

Done

https://review.coreboot.org/c/coreboot/+/79746/comment/e059493f_a1710b2e :
PS2, Line 33: int
> Can be removed

Done

File src/mainboard/dell/e7240/Makefile.inc:

PS2:
> Missing SPDX header. […]

Done

File src/mainboard/dell/e7240/bootblock.c:

https://review.coreboot.org/c/coreboot/+/79746/comment/1130749a_aa4d837d :
PS2, Line 13: pci_write_config32(PCH_LPC_DEV, LPC_GEN4_DEC, 0x007c0901);
            : 	mec5035_early_init();
            : 	pci_write_config32(PCH_LPC_DEV, LPC_GEN4_DEC, 0);
> Any reason to explicitly set and unset LPC_GEN4_DEC here? The devicetree entries for these are set u […]

Done

File src/mainboard/dell/e7240/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/79746/comment/5e463dbf_bc32e8ed :
PS2, Line 66: device pci 1f.0 on  end # LPC bridge
> Did you try adding the EC here like the dell/e6400? That seems to call a function that initialized t […]

Done