[M] Change in coreboot[main]: soc/intel/alderlake: Add function to force disable memory channels

[Angel Pons (Code Review)]

Attention is currently required from: Christian Walter, Dinesh Gehlot, Jamie Chen, Jayvik Desai, Jeremy Soller, Kapil Porwal, Lean Sheng Tan, Michał Kopeć, Michał Żygowski, Nick Vaccaro, Paul Menzel, Piotr Król, Sean Rhodes, Tim Crawford.

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

Change subject: soc/intel/alderlake: Add function to force disable memory channels
......................................................................

Patch Set 6: Code-Review+1

(7 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85529/comment/aa44d63d_90c519b8?usp... :
PS3, Line 8:
> DMI is only a way to ensure that the intended channels were disabled. […]

Looking at DMI info is a simple way to show what happens when using the channel disable mask.

File src/soc/intel/alderlake/include/soc/meminit.h:

https://review.coreboot.org/c/coreboot/+/85529/comment/6d13e061_43c75074?usp... :
PS6, Line 120: mb_fill_override_channel_mask
> you need to write a API expectation in comment section for mb users to follow

+1

nit: could we also rename the function?

```suggestion
uint8_t mb_get_channel_disable_mask(void);
```

File src/soc/intel/alderlake/meminit.c:

https://review.coreboot.org/c/coreboot/+/85529/comment/9ec2248f_e342aff6?usp... :
PS1, Line 264: void memcfg_init(FSPM_UPD *memupd, const struct mb_cfg *mb_cfg,
             : 		 const struct mem_spd *spd_info, bool half_populated,
             : 		 uint32_t ch_disable_mask)
> Uploaded a new patch.

Looks good to me.

https://review.coreboot.org/c/coreboot/+/85529/comment/ee027c1a_33575f7a?usp... :
PS1, Line 315: 	mem_init_override_channel_mask(mem_cfg, ch_disable_mask);
> > Could this be called at the very end of the function? If so, you could export this function (make  […]

Ack

File src/soc/intel/alderlake/meminit.c:

https://review.coreboot.org/c/coreboot/+/85529/comment/124b029e_4cb5706c?usp... :
PS6, Line 260: if ((ch_disable_mask & BIT(0)) &&
             : 		!((ch_disable_mask & 0xf) == (BIT(0) | BIT(1)))
             : 		) {
I'm confused about this check. If Mc0Ch0 is disabled, but Mc0Ch0 + Mc0Ch1 are *not* the only disabled channels on Mc0, the configuration is invalid. For the following `ch_disable_mask ---> valid` (considering Mc0 only):

- Ch0, Ch1           ---> valid
- Ch0, Ch1,      Ch3 ---> invalid
-           Ch2, Ch3 ---> valid
- Ch0,      Ch2      ---> invalid
-      Ch1,      Ch3 ---> valid
-           Ch2      ---> valid

https://review.coreboot.org/c/coreboot/+/85529/comment/79682a1d_f8bfd998?usp... :
PS6, Line 269:  {
> you don't need braces for single statement

Sharing some thoughts on this, but I don't have a strong opinion.

IIRC the coding style doesn't enforce a preference. I prefer using braces myself but I don't mind either approach during reviews unless it's error-prone, e.g.:

```
while (true)
    if (true)
    	for (int i = 0; i < 10; i++)
    		foo();
    bar(); // some compilers might warn about this likely being a bug
```
There have been bugs in the past caused by missing braces, which is why I prefer using them.

https://review.coreboot.org/c/coreboot/+/85529/comment/884f32c4_ef4221d8?usp... :
PS6, Line 325: 	mem_init_override_channel_mask(mem_cfg, mb_fill_override_channel_mask());
nit: You can call `mb_fill_override_channel_mask()` inside `mem_init_override_channel_mask()`