View Change

3 comments:

• File src/soc/intel/common/basecode/bootblock/bootblock.c:

  • Patch Set #37, Line 39: bootblock_soc_early_init(void)

    This reminds me on UEFI implementations that use function pointers everywhere […]

    I think this is just to allow arbitrary function names. To allow that, I would just use thunk functions inside SoC-specific code:

     void bootblock_soc_early_init(void)
     {
             bootblock_my_ultra_awesome_soc_early_init();
     }

    In the above example, things can be simplified by a rename. However, when multiple SoCs are supported with the same code, things get more complex. For instance, soc/intel/cannonlake supports Cannonlake, Coffeelake, Whiskeylake and Cometlake. The code on that SoC could be something like this:

     void bootblock_soc_early_init(void)
     {
             switch (get_soc_generation()) {
             case SOC_GENERATION_CANNONLAKE:
                      bootblock_cnl_early_init();
                      break;
             case SOC_GENERATION_COFFEELAKE:
             case SOC_GENERATION_WHISKEYLAKE:
                      bootblock_cfl_whl_early_init();
                      break;
             case SOC_GENERATION_COMETLAKE:
                      bootblock_cml_early_init();
                      break;
             default:
                      die("%s: Unsupported SoC generation!", __func__);
             }
     }

    This is a good idea if the same coreboot binary has to support more than one generation of SoC, as the specific generation can only be determined at runtime.

    However, if only one generation is supported, it is known at build-time, so things can be improved even further using something like the variants mechanism for the mainboards. This allows building only the necessary files, and enables build-time checking.

    Using build-time checking, the Makefiles sanity-check the selected Kconfig options and abort the build process if they are invalid. Also, if a SoC generation has not implemented the required functions, it will result in a linker error. And thanks to Jenkins, this will be checked on every change, so accidentally breaking something is harder :)

  • Patch Set #37, Line 46: die("%s has not been implemented by SoC", __func__);

    With build-time checking, these errors would just be linker errors.

  • Patch Set #37, Line 102: (CONFIG(PAGING_IN_CACHE_AS_RAM))

    Why is there APL/GLK specific code here? Wasn't the idea to make it more flexible?

    I think APL/GLK differ too much from the big core platforms to have common code everywhere. If the next little core SoCs also need this, I would then consider an abstraction, but not now.

To view, visit change 18457. To unsubscribe, or for help writing mail filters, visit settings.