Attention is currently required from: Dmitry Ponamorev, Mariusz Szafrański, Suresh Bellampalli, Vanessa Eusebio, Michal Motyl, Kyösti Mälkki, Patrick Rudolph, King Sumo. Angel Pons 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 21: Code-Review+1
(15 comments)
File src/mainboard/teleplatforms/D4E4S16P8/Kconfig:
https://review.coreboot.org/c/coreboot/+/57194/comment/5dc2d82e_0c73d136 PS21, Line 17: string Not needed since CB:56553
https://review.coreboot.org/c/coreboot/+/57194/comment/75843fb4_7f737f20 PS21, Line 21: string Not needed since CB:56554
https://review.coreboot.org/c/coreboot/+/57194/comment/062583d3_edf7b5f8 PS21, Line 24: config MAINBOARD_VENDOR : string : default "teleplatforms" Already set in `src/mainboard/teleplatforms/Kconfig`
File src/mainboard/teleplatforms/D4E4S16P8/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/57194/comment/927b4144_27861e89 PS21, Line 5: Device (PWRB) : { : Name(_HID, EisaId("PNP0C0C")) : : // Wake : Name(_PRW, Package(){0x1d, 0x05}) : } This shouldn't be needed, see CB:27272
File src/mainboard/teleplatforms/D4E4S16P8/acpi/mainboard_pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/57194/comment/07d8f4cb_ab20fbbb PS21, Line 3: board specific Hmmm, doesn't seem to be that board-specific: the other two DNV-NS boards (scaleway/tagada and intel/harcuvar) have the exact same file.
File src/mainboard/teleplatforms/D4E4S16P8/acpi/thermal.asl:
PS21: If there are no plans to add more stuff here, I'd drop the file.
File src/mainboard/teleplatforms/D4E4S16P8/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/e6f90832_756324b7 PS21, Line 13: /* Disable USB ports in S5 */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0; These values are only used with mainboard-specific SMI handlers and/or ACPI. They are otherwise meaningless and this could be dropped.
https://review.coreboot.org/c/coreboot/+/57194/comment/32db178a_0b3195ad PS21, Line 17: /* TPM Present */ : gnvs->tpmp = 0; Doesn't seem to be used anywhere in coreboot. Default is already zero, so I'd drop this.
File src/mainboard/teleplatforms/D4E4S16P8/ramstage.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/b72e6c7d_7e30ea0c PS21, Line 26: 0x56 EEPROM_ADDR
https://review.coreboot.org/c/coreboot/+/57194/comment/2b7b0b9d_caa80b50 PS21, Line 26: SMBUS_IO_BASE Why not `smbus_base()`?
https://review.coreboot.org/c/coreboot/+/57194/comment/4fd11e75_825809dc PS21, Line 28: tmp == 0x3f/*?*/ nit: you can use a character literal in the comparison:
tmp == `?`
https://review.coreboot.org/c/coreboot/+/57194/comment/b82a4521_85210708 PS21, Line 28: (tmp < 0x20 || tmp > 0x7f) 0x7f is DEL, which isn't printable. If you exclude it, the resulting check is the complement (negation) of `isprint()` from `src/include/ctype.h`.
https://review.coreboot.org/c/coreboot/+/57194/comment/50107993_dccbb59f PS21, Line 30: memset(serial, 0, sizeof(serial)); : memcpy(serial, ERROR_STRING, sizeof(ERROR_STRING)); : return serial; Why not simply `return ERROR_STRING;` instead?
https://review.coreboot.org/c/coreboot/+/57194/comment/034ea3b3_97ec4496 PS21, Line 43: unsigned int smbios_cpu_get_max_speed_mhz(void) : { : return 2400; : } : : unsigned int smbios_cpu_get_current_speed_mhz(void) : { : return 2400; : } Hmmm, Intel says the Atom C3758 has a clock speed of 2200 MHz: https://ark.intel.com/content/www/us/en/ark/products/97926/intel-atom-proces...
File src/mainboard/teleplatforms/D4E4S16P8/romstage.c:
https://review.coreboot.org/c/coreboot/+/57194/comment/9c08cb8b_6c6d9583 PS20, Line 437: {SOUTH_GROUP0_CTBTRIGINOUT,
Except for this, this is all copy-paste from intel/harcuvar. Some like eMMC below seems wrong. […]
Would be interesting to use the coreboot GPIO configuration mechanism instead, which scaleway/tagada uses. IMHO, it's significantly more readable.