[coreboot-gerrit] [M] Change in coreboot[main]: soc/common/smbus: Support reading SPD5 hubs for DDR5

[Krystian Hebel (Code Review)]

Attention is currently required from: Angel Pons, Balaji Manigandan, Balaji Manigandan, Eric Lai, Haribalaraman Ramasubramanian, Jeremy Soller, Kapil Porwal, Karthik Ramasubramanian, Kyösti Mälkki, Maulik Vaghela, Meera Ravindranath, Nick Vaccaro, Patrick Rudolph, Paul Menzel, Raj Astekar, Reka Norman, Rizwan Qureshi, Subrata Banik, Subrata Banik, Tarun Tuli, Tim Crawford, Tim Wawrzynczak.

Krystian Hebel has posted comments on this change by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/52731?usp=email )

Change subject: soc/common/smbus: Support reading SPD5 hubs for DDR5
......................................................................

Patch Set 16:

(3 comments)

File src/soc/intel/common/block/smbus/smbuslib.c:

https://review.coreboot.org/c/coreboot/+/52731/comment/eb4cb110_4cfe5467?usp... :
PS16, Line 45: offset
Nit: this name is a bit misleading, it took me a while to understand that this isn't offset on a page, which wouldn't make sense in this function.

In fact, this variable could be skipped by using `SPD5_MEMREG_REG` directly in call to `smbus_write_byte`, just as is done in `reset_page_spd5`.

https://review.coreboot.org/c/coreboot/+/52731/comment/6b32b10e_8b93fb59?usp... :
PS16, Line 95: 	smbus_write_byte(spd_addr, SPD5_MEMREG_REG(SPD5_MR11), 0);
This is basically `switch_page(spd_addr, 0);`, it would be clearer to call it instead of writing the byte directly. Compiler will optimize it anyway.

https://review.coreboot.org/c/coreboot/+/52731/comment/7e0bd178_e13eaf34?usp... :
PS16, Line 126: 		if (spd[SPD_DRAM_TYPE] == SPD_DRAM_DDR4 &&
I think it would be better to move this up a level, and have three branches (DDR5, DDR4 and older) in top-level `if`/`else if`/`else`, even if this means that some parts of the code will have to be repeated. Otherwise this could lead to indefinite nesting level in the future, which will be hard to follow.