Attention is currently required from: Mario Scheithauer. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63863 )
Change subject: soc/intel/elkhartlake: Provide ability to update TSN GbE MAC addresses ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
File src/soc/intel/elkhartlake/tsn_gbe.c:
https://review.coreboot.org/c/coreboot/+/63863/comment/fcd14049_452b683e PS2, Line 11: adr_to_set Maybe just call it 'mac' here to shorten down the name a bit?
https://review.coreboot.org/c/coreboot/+/63863/comment/ebd33131_e7c0760d PS2, Line 16: printk(BIOS_ERR, "TSN GbE: No valid MAC address found\n"); I wouldn't print that with ERROR level as there are valid cases where the mainboard code does not provide an address. I guess INFO would be better here, or?
https://review.coreboot.org/c/coreboot/+/63863/comment/e3e14826_2117a78b PS2, Line 25: *mac_p = (*mac_p & 0xFFFF0000) | (adr_to_set[5] << 8) | adr_to_set[4]; How about uing clrsetbits32() from device/mmio.h here? Could look like: clrsetbits32((void *)(io_mem_base + TSN_MAC_ADDRESS0_HIGH_OFFSET), 0xffff, ((adr_to_set[5] << 8) | adr_to_set[4]));
And if you shorten down the register and bar pointer name a bit and rename 'adr_to_set' to 'mac' this could fit on one line.