Attention is currently required from: Christian Walter, Maximilian Brune, Lean Sheng Tan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68224 )
Change subject: mb/prodrive/atlas: Print HSID ......................................................................
Patch Set 1: Code-Review+1
(5 comments)
File src/mainboard/prodrive/atlas/gpio.c:
https://review.coreboot.org/c/coreboot/+/68224/comment/65d21445_e9f783b8 PS1, Line 10: PAD_CFG_GPI(GPP_A8, NONE, RSMRST), /* HSID_0 */ We should probably configure the GPIOs early in bootblock, as in the future we might need to read the HSID in pre-RAM.
File src/mainboard/prodrive/atlas/mainboard.c:
https://review.coreboot.org/c/coreboot/+/68224/comment/1ae93db6_dedae1bd PS1, Line 7: #include <arch/io.h> nit: alphabetical order?
https://review.coreboot.org/c/coreboot/+/68224/comment/25786754_b0bafd74 PS1, Line 11: static void print_hsid(void) We probably want to have a `uint8_t get_hsid()` function instead, in case we need to deal with revision-specific differences.
``` uint8_t get_hsid(void) { const gpio_t hsid_gpios[] = { GPP_A8, GPP_F19, GPP_H23, GPP_H19, }; return gpio_base2_value(hsid_gpios, ARRAY_SIZE(hsid_gpios)); }
static void mainboard_init(void *chip_info) { configure_gpio_pads(); printk(BIOS_INFO, "HSID: 0x%x\n", get_hsid()); } ```
https://review.coreboot.org/c/coreboot/+/68224/comment/6816a11f_966d0d6e PS1, Line 14: < This is not a shift! 😄
Also, there's `gpio_base2_value()`
https://review.coreboot.org/c/coreboot/+/68224/comment/5f0f9e1c_d695f24e PS1, Line 17: 1 No need to restrict the print width, it may actually make bugs harder to diagnose.