Change in coreboot[master]: mb/teleplatforms/D4E4S16P8: Add new CRB teleplatforms/D4E4S16P8
Attention is currently required from: Mariusz Szafrański, Paul Menzel, Suresh Bellampalli, Vanessa Eusebio, Michal Motyl, Kyösti Mälkki, Patrick Rudolph, King Sumo. Dmitry Ponamorev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57194 )
Change subject: mb/teleplatforms/D4E4S16P8: Add new CRB teleplatforms/D4E4S16P8 ......................................................................
Patch Set 20:
(8 comments)
Patchset:
PS20: Correct config file to remove all links to 3rdparty/blobs.
File configs/config.teleplatforms.D4E4S16P8:
PS18:
This config will not build until 3rdparty/blobs is merged
What solutions can there be? Remove all links to 3rdparty/blobs?
File src/mainboard/teleplatforms/D4E4S16P8/Kconfig:
https://review.coreboot.org/c/coreboot/+/57194/comment/eb5f452b_38d88f6a PS17, Line 14: ##
I'll adjust the licenses to one-liners.
Done
File src/mainboard/teleplatforms/D4E4S16P8/Kconfig:
https://review.coreboot.org/c/coreboot/+/57194/comment/e7fda71f_9c2e1a3d PS19, Line 31: default "1.0.12"
This should be explicitly set only for a release to avoid mistaking development builds as releases. […]
Done
File src/mainboard/teleplatforms/D4E4S16P8/gpio.h:
https://review.coreboot.org/c/coreboot/+/57194/comment/cc14d3f7_a341668b PS18, Line 9: const struct dnv_pad_config D4E4S16P8_gpio_table[] = {
No data, only prototypes and defines in .h files.
The same as with hsio.h. This is the definition of an array of structures and its elements. Since the data of these structures does not change, it is okay to define them here?
File src/mainboard/teleplatforms/D4E4S16P8/hsio.h:
https://review.coreboot.org/c/coreboot/+/57194/comment/6a2621cf_2ae7d48c PS18, Line 9: const BL_HSIO_INFORMATION D4E4S16P8_hsio_config[] = {
No data, only prototypes and defines in .h files.
I don't quite understand what the problem is. This is the definition of an array of structures and its elements. Since the data of these structures does not change, it is okay to define them here?
File src/mainboard/teleplatforms/D4E4S16P8/ramstage.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/cbd7e72c_e1fbacff PS18, Line 28: if (tmp == 0x3f/*?*/ || (tmp < 0x20 || tmp > 0x7f)) {
I'll recude this to BIOS_SPEW. […]
BIOS_SPEW is ok. The main purpose is for the serial number to be visible in the output of the dmidecode function.
File src/mainboard/teleplatforms/D4E4S16P8/smbus.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/743dd689_ec6e03e2 PS17, Line 81: .device = 0x19df, /* Denverton SMBus0 "Legacy" device id */
This file is not specific to your board, I'll remove it here. […]
Without this function, I will not be able to read the serial number of the motherboard (smbios_mainboard_serial_number in D4E4S16P8/ramstage.c). Functions soc/intel didn't work for this. This is not critical - you can remove it.
-
Dmitry Ponamorev (Code Review)