[L] Change in coreboot[main]: soc/intel/common/block/imc: Add Integrated Memory Controller driver

[Name of user not set (Code Review)]

Attention is currently required from: Felix Singer, Jérémy Compostella, Shuo Liu.

[e-mail] has posted comments on this change by [e-mail]. ( https://review.coreboot.org/c/coreboot/+/83320?usp=email )

Change subject: soc/intel/common/block/imc: Add Integrated Memory Controller driver
......................................................................

Patch Set 17:

(9 comments)

File src/soc/intel/common/block/imc/imc.c:

https://review.coreboot.org/c/coreboot/+/83320/comment/e87d16bb_eafb9308?usp... :
PS3, Line 57: void imc_smbus_spd_init(pci_devfn_t dev)
> imc_spd_smbus_init?

Done

File src/soc/intel/common/block/include/intelblocks/imc.h:

https://review.coreboot.org/c/coreboot/+/83320/comment/d3286162_fd69107b?usp... :
PS15, Line 10:
> can these be static? or to static what ever could be?

I moved it to an internal header imclib.h.

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

https://review.coreboot.org/c/coreboot/+/83320/comment/7b3ef5c4_30eccf36?usp... :
PS3, Line 10: {
> sure, let us remove the weak here

Done

https://review.coreboot.org/c/coreboot/+/83320/comment/e0a94c66_afef34f9?usp... :
PS3, Line 96: 		if (!CONFIG(SOC_INTEL_COMMON_BLOCK_IMC) && blk->addr_map[i] == 0) {
> add a common block here might be helpful

Done

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

https://review.coreboot.org/c/coreboot/+/83320/comment/91a70186_19a102bd?usp... :
PS7, Line 38: static void spd_read(u8 *spd, u8 addr)
> maybe this can be moved to a generic spd.c, which is implemented either by smbuslib, or the imc/spd. […]

This function is not the actual SPD IO function, it calls spd_read_byte(), spd_read_word() and spd_write_byte() inside.

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

https://review.coreboot.org/c/coreboot/+/83320/comment/f62d2f6d_d4b64011?usp... :
PS15, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
> maybe you can add a separate patch to rename smbuslib.c to spdlib.c, and to rename spd. […]

Please review https://review.coreboot.org/c/coreboot/+/84201.

https://review.coreboot.org/c/coreboot/+/83320/comment/516a73fc_b389c957?usp... :
PS15, Line 9: __weak int spd_read_byte(u8 slave_addr, u8 bus_addr)
> no need to have week here

Done

https://review.coreboot.org/c/coreboot/+/83320/comment/7bb8c67f_c5bd16c9?usp... :
PS15, Line 70: 	if (i2c_eeprom_read(addr, 0, SPD_PAGE_LEN, spd) < 0) {
> if (!IMC && ... […]

Done

File src/southbridge/intel/common/smbus.c:

https://review.coreboot.org/c/coreboot/+/83320/comment/81b57d65_0eb04de6?usp... :
PS15, Line 465: 	if (CONFIG(SOUTHBRIDGE_INTEL_I82371EB) || CONFIG(SOUTHBRIDGE_INTEL_I82801DX) ||
> this is for pch smbus and we should not change here.

Done