[coreboot-gerrit] [M] Change in coreboot[main]: soc/intel/xeon_sp/gnr: Add dimm_slot configuration

[Angel Pons (Code Review)]

Attention is currently required from: Arthur Heymans, Christian Walter, Johnny Lin, Jonathan Zhang, Patrick Rudolph, Shuo Liu, Tim Chu.

Angel Pons has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/83326?usp=email )

Change subject: soc/intel/xeon_sp/gnr: Add dimm_slot configuration
......................................................................

Patch Set 4:

(7 comments)

File src/mainboard/intel/avenuecity_crb/config/dimm_slot.c:

https://review.coreboot.org/c/coreboot/+/83326/comment/399d3b5a_e75b85af?usp... :
PS4, Line 10: 	_DIMM_SLOT_CFG_STRUCT(0, 0, 0, "CPU0_DIMM_A1", "BANK 0", "CPU0_DIMM_A1_AssetTag"),
Maybe add a `TODO: add the rest of DIMM slots` comment?

File src/mainboard/intel/avenuecity_crb/ramstage.c:

https://review.coreboot.org/c/coreboot/+/83326/comment/e683546d_58a3eed0?usp... :
PS4, Line 16: for (int i = 0; i < size; i++)
            : 		if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i],
            : 		    dimm->soc_num, dimm->channel_num, dimm->dimm_num)) {
            : 			const char *locator = dimm_slot_config_table[i].dev_locator;
            : 			t->device_locator = smbios_add_string(t->eos, locator);
            : 			locator = dimm_slot_config_table[i].bank_locator;
            : 			t->bank_locator = smbios_add_string(t->eos, locator);
            : 		}
I'd use braces:

```suggestion
    for (int i = 0; i < size; i++) {
    	if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i],
    	    dimm->soc_num, dimm->channel_num, dimm->dimm_num)) {
    		const char *locator = dimm_slot_config_table[i].dev_locator;
    		t->device_locator = smbios_add_string(t->eos, locator);
    		locator = dimm_slot_config_table[i].bank_locator;
    		t->bank_locator = smbios_add_string(t->eos, locator);
    	}
    }
```

https://review.coreboot.org/c/coreboot/+/83326/comment/902daed1_c329cab9?usp... :
PS4, Line 31: for (int i = 0; i < size; i++)
            : 		if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i],
            : 		    dimm->soc_num, dimm->channel_num, dimm->dimm_num)) {
            : 			const char *asset_tag = dimm_slot_config_table[i].asset_tag;
            : 			t->asset_tag = smbios_add_string(t->eos, asset_tag);
            : 		}
Same here:

```suggestion
    for (int i = 0; i < size; i++) {
    	if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i],
    	    dimm->soc_num, dimm->channel_num, dimm->dimm_num)) {
    		const char *asset_tag = dimm_slot_config_table[i].asset_tag;
    		t->asset_tag = smbios_add_string(t->eos, asset_tag);
    	}
    }
```

File src/mainboard/intel/avenuecity_crb/romstage.c:

https://review.coreboot.org/c/coreboot/+/83326/comment/fd2f1ff3_b2fa16d5?usp... :
PS4, Line 50: for (int i = 0; i < size; i++)
            : 		if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], socket, channel, dimm))
            : 			return true;
Same here:

```suggestion
    for (int i = 0; i < size; i++) {
    	if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], socket, channel, dimm))
    		return true;
    }
```

File src/mainboard/intel/beechnutcity_crb/config/dimm_slot.c:

https://review.coreboot.org/c/coreboot/+/83326/comment/e0444e4f_82484a57?usp... :
PS4, Line 10: 	_DIMM_SLOT_CFG_STRUCT(0, 0, 0, "CPU0_DIMM_A1", "BANK 0", "CPU0_DIMM_A1_AssetTag"),
Maybe add a `TODO: add the rest of DIMM slots` comment?

File src/mainboard/intel/beechnutcity_crb/romstage.c:

https://review.coreboot.org/c/coreboot/+/83326/comment/02f70249_7d5179b3?usp... :
PS4, Line 50: for (int i = 0; i < size; i++)
            : 		if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], socket, channel, dimm))
            : 			return true;
```suggestion
    for (int i = 0; i < size; i++) {
    	if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], socket, channel, dimm))
    		return true;
    }
```

File src/soc/intel/xeon_sp/gnr/include/soc/dimm_slot.h:

https://review.coreboot.org/c/coreboot/+/83326/comment/43ff0cf2_919d6295?usp... :
PS4, Line 21: #define _DIMM_SLOT_CFG_STRUCT(s, c, d, dl, bl, at) {\
            : 	.socket       = s,\
            : 	.channel      = c,\
            : 	.dimm         = d,\
            : 	.dev_locator  = dl,\
            : 	.bank_locator = bl,\
            : 	.asset_tag    = at\
            : }
I don't see why this macro would be needed. These two lines are equivalent:

```
_DIMM_SLOT_CFG_STRUCT(0, 0, 0, "CPU0_DIMM_A1", "BANK 0", "CPU0_DIMM_A1_AssetTag"),
 { 0, 0, 0, "CPU0_DIMM_A1", "BANK 0", "CPU0_DIMM_A1_AssetTag" },
```