[coreboot-gerrit] Change in coreboot[master]: soc/intel/alderlake: Add option to make MRC log silent

[Nico Huber (Code Review)]

Attention is currently required from:  Felix Singer, Subrata Banik, Tim Wawrzynczak, Angel Pons, Arthur Heymans, Nick Vaccaro, Andrey Petrov, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60441 )

Change subject: soc/intel/alderlake: Add option to make MRC log silent
......................................................................

Patch Set 2:

(1 comment)

Patchset:

PS2:
> This is what my understanding is:
> coreboot and FSP MRC log level are different for example: typical log level for coreboot is 8 where else for MRC the most verbose level is 5.
>
> Hence, the UPD assignment like below will set the wrong value to UPD. Isn't it?
> m_cfg->SerialDebugMrcLevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL;

Yes, that's why I said "map".
> Most of the time, we want to see FSP debug logs but not that verbose MRC debug log which takes several minutes to boot the platform hence, we might still prefer to have different log levels between coreboot and FSP. And for that matter inside FSP even there are 2 different log level (there is one more UPD PcdSerialDebugLevel which controls overall FSP entire debug log level). But the intention is to just make FSP-M silent without additional MRC training prints.

Why disable error messages then?

Anyway, for verbose training output there is config
DEBUG_RAM_SETUP.
> May be we can drop `SOC_INTEL_ALDERLAKE_MRC_DEBUG_CONSENT` config and assign the UPD like this ?
>
> if (CONFIG(SILENT_FSP_M_DEBUG_MESSAGE))
>      m_cfg->SerialDebugMrcLevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL_0;
> else
>      m_cfg->SerialDebugMrcLevel = CONFIG_DEFAULT_CONSOLE_LOGLEVEL_3;

This would only set it to 1 in odd cases? I don't follow. Did
you mean constant 0 / 3? Also, the console log level is a runtime
setting. It's not easy to query right now, but the API could be
changed.

Maybe something like this (with to-be-written get_max_console_log_level()):

int fspm_debug_level;
  switch (get_max_console_log_level(CONSOLE_LOG_ALL)) {
  case BIOS_EMERG: case BIOS_ALERT: case BIOS_CRIT: case BIOS_ERR:
          fspm_debug_level = 1;
          break;
  case BIOS_WARNING:
          fspm_debug_level = 2;
          break;
  case BIOS_NOTICE:
          fspm_debug_level = 3;
          break;
  case BIOS_INFO:
          fspm_debug_level = 4;
          break;
  case BIOS_DEBUG: case BIOS_SPEW:
          fspm_debug_level = 5;
          break;
  default:
          fspm_debug_level = 0;
          break;
  }
  if (!CONFIG(DEBUG_RAM_SETUP))
          fspm_debug_level = MIN(fspm_debug_level, 2);
  m_cfg->SerialDebugMrcLevel = fspm_debug_level;
> in that way, we can reduce to only 1 new Kconfig rather SoC wise Kconfig. WDYT?

I think this isn't FSP specific. It doesn't matter if one chose
to use a blob instead of writing native code. coreboot already
provides all the needed options.