Attention is currently required from: Felix Singer, Paul Menzel, Tim Wawrzynczak. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59548 )
Change subject: mb/clevo/tgl-u: add new board L14xMU ......................................................................
Patch Set 13:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59548/comment/de119df2_bace8c3d PS8, Line 56:
Too make it crystal clear, maybe mention “vendor firmware”.
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/59548/comment/c517ca15_45e2f3ce PS12, Line 7: mb: add new board clevo/tgl-u/l140mu
Why not our usual scheme? Suggestion: […]
"our"? CB:45624 ;)
Done, I don't care
File src/mainboard/clevo/tgl-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/59548/comment/b400c0a5_c7dc8672 PS12, Line 48: config VGA_BIOS_ID : default "8086,9b41"
This is only needed when an option rom is used. It's not the case here, so remove it.
true, thanks
https://review.coreboot.org/c/coreboot/+/59548/comment/32dc48d8_7a65a573 PS12, Line 51: config TPM_PIRQ : default 0x77 if BOARD_CLEVO_L140MU # GPP_C9_IRQ
I would move this to the other options on the top, so that the variant specific options are not scat […]
I don't agree on this one; it doesn't really matter. also, l140cu is the same ;)
https://review.coreboot.org/c/coreboot/+/59548/comment/37d597cf_679b6a70 PS12, Line 57: config VARIANT_DIR : default "l140mu" if BOARD_CLEVO_L140MU
Same here.
this file is mostly a copy of l140cu^^ I agree that it fits better near the mainboard dir, though. -> Done
File src/mainboard/clevo/tgl-u/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/59548/comment/c2bdd78f_f84f2152 PS12, Line 4: bool "L140MU / L141MU / L142MU"
What are the differences between these variants?
just the color, same as for L140CU
File src/mainboard/clevo/tgl-u/variants/l140mu/gpio_early.c:
https://review.coreboot.org/c/coreboot/+/59548/comment/18aa2367_4389dc0e PS12, Line 10: PAD_NC(GPP_C22, UP_20K), : PA
Not needed here. Please remove.
first, it's the same as l140cu here. also, the default is rts/cts, so it makes sense to set them to NC. if you really insist on removing them, reopen please
-
Michael Niewöhner (Code Review)