[coreboot-gerrit] Change in coreboot[master]: soc/intel/elkhartlake: Enable PCH GBE

[Werner Zeh (Code Review)]

Attention is currently required from: Maulik V Vaghela, Mario Scheithauer, Subrata Banik, Lean Sheng Tan, Patrick Rudolph.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55355 )

Change subject: soc/intel/elkhartlake: Enable PCH GBE
......................................................................

Patch Set 4:

(2 comments)

File src/soc/intel/elkhartlake/chip.h:

https://review.coreboot.org/c/coreboot/+/55355/comment/1d045297_d17d3d09 
PS4, Line 386: Tsn_2_5_Gbps,
             : 		Tsn_1_Gbps,
For EHL we now know that 38,4 MHz XTAL is fixed. Can't we now use something like this?
enum {
    Tsn_2_5_Gbps = 2
    Tsn_1_Gbps = 3
} PchTsnGbeLinkSpeed;

Later then, when it is set up in fsp_params.c, you could simply do this:

'params->PchTsnGbeLinkSpeed = config->PchTsnGbeLinkSpeed);'

and everything would match as needed.

File src/soc/intel/elkhartlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/55355/comment/c9625fc9_bc10d4bf 
PS4, Line 380: params->PchTsnGbeLinkSpeed = 0;
             : 			printk(BIOS_ERR, "ERROR: Unsupported CPU XTAL for GBE config.\n"
             : 				"Disabling PCH GBE.\n");
Setting params->PchTsnGbeLinkSpeed = 0 does not really disable the TSN controller, it just sets the link speed to 1 Gbps@24MHz according to the comment above.
If this is not allowed in this context I would rather do the following above:

params->PchTsnEnable = is_dev_enabled(dev) && (CONFIG_CPU_XTAL_HZ == 38400000);

Or, if you want the error log message:
params->PchTsnEnable = is_dev_enabled(dev);
if (params->PchTsnEnable && CONFIG_CPU_XTAL_HZ != 38400000) {
    printk (BIOS_ERR, "Error...");
} else {
    params->PchTsnGbeLinkSpeed = config->PchTsnGbeLinkSpeed;
    params->PchTsnGbeSgmiiEnable = config->PchTsnGbeSgmiiEnable;
    params->PchTsnGbeMultiVcEnable = config->PchTsnGbeMultiVcEnable;
}