Attention is currently required from: Felix Singer, Raul Rangel, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52493 )
Change subject: [RFC] device: Introduce new method for setting device states ......................................................................
Patch Set 21:
(1 comment)
File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/52493/comment/4678949f_407d3846 PS13, Line 461: if (!xdci_can_enable()) : params->XdciEnable = 0;
I had the idea that we could set related UPDs in that function, too, like UPDs that depend on the enabled state of the device e.g. PchHdaEnable, PchHdaVcType etc. or ScsEmmcHs400Enabled, PchScsEmmcHs400DllDataValid, PchScsEmmcHs400RxStrobeDll1.
This way, these UPDs would be grouped together in these functions (on function per device). However, we would have to pass that FSP*_UPD pointer around, which is pretty ugly.
For everything there is a pattern. Usually one would pass a void pointer through the generic API.
However, I have to say that this might be going in the wrong direction. At some point, we'd have to ask: why a table? It seems to make sense if it's about one thing common to all entries, e.g. the enable state. If we allow just everything, we can as well call the functions directly (with some alignment maybe?).
pch_enable_dev (¶ms->PchHdaEnable, PCH_DEVFN_HDA); pch_enable_dev (¶ms->ShowSpiController, PCH_DEVFN_SPI); pch_enable_emmc(params, PCH_DEVFN_EMMC); pch_enable_dci (...
That's a table too of sorts.