[coreboot-gerrit] Change in coreboot[master]: fw_config: Add firmware configuration interface

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41209 )

Change subject: fw_config: Add firmware configuration interface
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41209/2/src/include/fw_config.h 
File src/include/fw_config.h:

https://review.coreboot.org/c/coreboot/+/41209/2/src/include/fw_config.h@9 
PS2, Line 9: 
I did initially pursue a macro based implementation.  It had a number of positives like you've noted but also a number of negatives:

- mainboards themselves can't include code to static.c currently (had to hack sconfig and it was not clean; furquan says I should be able to add a chip.h in mainboard but I had issues when I tried that)  this could be fixed of course.
- I want a string for each field and option for console output anyway
- lots of macro expansion which usually generates complaints in reviews

The string approach was very flexible and it didn't seem like a significant size hit (32 options per field is a bit extreme and could be lowered) because the real target is ramstage, but I do agree it is more than we might want in bootblock on some systems.

At the end of the day I'd like for the table itself to be generated from our own database of these fields and options, so maybe it doesn't need to be in code form directly.

I did try to build off the cmos.layout format/parser in order to take advantage of having this in a separate file.  But that is tightly bound to the nvramtool and not very flexible.

I also considered extending the sconfig/devicetree to support defining the table directly in devicetree and have sconfig generate the underlying macros.  It should be possible to add more tokens to devicetree parsing and do something like:

fw_config mainboard/google/volteer
  field AUDIO 8 10
    option NONE 0 end
    option MAX98357_ALC5682I_I2S 1 end
    option MAX98373_ALC5682I_I2S 2 end
    option MAX98373_ALC5682_SNDW 3 end
  end
end
(or similar)

And then sconfig would generate all the #defines that go together with the accessor macros that you noted above.

I'll do some more investigation here tomorrow...