Change in blobs[master]: 3rdparty/blobs/mainboard/teleplatforms/D4E4S16P8: Create new folder f...

[Angel Pons (Code Review)]

Attention is currently required from: Дмитрий Понаморев, Kyösti Mälkki.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/blobs/+/57196 )

Change subject: 3rdparty/blobs/mainboard/teleplatforms/D4E4S16P8: Create new folder for teleplatforms/D4E4S16P8 CRB.
......................................................................

Patch Set 4:

(1 comment)

Patchset:

PS4: 
I see you've included FSP binaries. I suppose they're just the DNV-NS FSP from https://github.com/intel/FSP with UPD settings applied using BCT. This is not how coreboot does it because it's hard to keep track of the settings each board uses. Moreover, if a new FSP version gets released, one needs to transfer the UPD settings from the old FSP binaries to the new ones.

The preferred method to set FSP UPDs is to do so at runtime in coreboot code. The values can come from various places, such as:

- Kconfig options, e.g. `soc/intel/denverton_ns/romstage.c`:
   m_cfg->PcdEnableIQAT = CONFIG(IQAT_ENABLE);

- Devicetree devices' on/off state, e.g. `soc/intel/skylake/chip.c`:
   params->PchHdaEnable = is_devfn_enabled(PCH_DEVFN_HDA);

- Devicetree chip "registers", e.g. `soc/intel/skylake/chip.c`:
   params->SsicPortEnable = config->SsicPortEnable;

- Mainboard code, e.g. the `mainboard_silicon_init_params()` function.

Compared to other Intel FSP platforms, only a few DNV-NS UPDs are hooked up to coreboot code. This is most likely because upstream DNV-NS development is basically non-existent. The only upstream DNV-NS development I'm aware of is scaleway/tagada and your teleplatforms/D4E4S16P8.

Exposing FSP UPDs as devicetree "registers" as-is, while common, is subpar: besides the ugly CamelCase names (coreboot uses snake_case), FSP UPDs often depend on other UPDs, can be cumbersome to use (e.g. individual UPDs instead of an array) and can also have other pitfalls. Also, unspecified devicetree "registers" default to 0, which can be a valid value for an UPD but not a reasonable default. For example, a 0 in the `PcieClkSrcUsage` CFL FSP UPD means "clock source used for RP #1", but it would make more sense to default to 0xff aka "clock source is unused". There's some code to remap the values, but it's ugly: CB:49172 touches that code.

IMHO, the best way to deal with cumbersome FSP UPDs is to "hide" them behind the devicetree: provide easy-to-use devicetree settings and calculate the corresponding UPD values in SoC code. That being said, I'm aware that implementing this takes a significant amount of time, so I'm fine if you prefer to directly set FSP UPDs from mainboard code.

In any case, here's an example of what could be done to improve FSP integration. DNV-NS FSP-S UPDs for PCIe root port settings (de-emphasis, link speed, ASPM, lane reversal) depend on PCIe controller bifurcation settings, which depends on whether the PCIe controllers are enabled. Moreover, all of this depends on FIA MUX configuration done by FSP-M. It doesn't make sense to specify de-emphasis, link speed, ASPM or lane reversal for a disabled and/or unavailable PCIe root port, nor does specifying bifurcation settings for a disabled PCIe controller.

Another annoying thing is that FSP UPDs aren't arrays, but it doesn't matter much because it makes more sense to group the root port settings (de-emphasis, link speed, ASPM, lane reversal) on a per-port basis, e.g. using a struct type. Still, the devicetree can use an array of structs to expose these settings (array index would be root port number). Furthermore, coreboot can skip programming the settings for disabled root ports into the FSP UPDs.