Change in coreboot[master]: [RFC] device: Introduce new method for setting device states
Attention is currently required from: Felix Singer, Nico Huber, Raul Rangel, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held. Tim Wawrzynczak 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 22:
(5 comments)
Patchset:
PS22:
I'm still interested in this, anyone else ? […]
Thanks Felix for your patience. Remember: perfect is the enemy of done! I'll stop complaining about names 😋
File src/include/device/devenmap.h:
https://review.coreboot.org/c/coreboot/+/52493/comment/75b14325_353b244f PS22, Line 12: option
Well, that be more specific to FSP then. […]
Ack
https://review.coreboot.org/c/coreboot/+/52493/comment/c4bfaf7f_7c51fe93 PS22, Line 13: pci_devfn_t
I think I tried booting with this and it worked,
Yes, it should still work, it's just semantically incorrect. A `pci_devfn_t` is intended to be an offset you apply to MMCONFIG (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...) to read a PCI config register.
The devicetree uses a byte to encode the slot (device) and function number (typically typed with `unsigned int`), but I seem to recall unfortunately there are incorrect references to `pci_devfn_t` for these byte-encoded values still lingering that have yet to be cleaned up.
Hmmm, maybe we should have a `pci_dev_t` for `PCI_DEV()` and use `pci_devfn_t` with `PCI_DEVFN()`
The types/naming situation are very confusing right now 😞 that's an interesting idea, that sounds like a nearly impossible task at this point
https://review.coreboot.org/c/coreboot/+/52493/comment/786280ec_8349145e PS22, Line 14: hook
The whole thing is about enabling/disabling something. […]
Ack
https://review.coreboot.org/c/coreboot/+/52493/comment/6fd4c680_6f60b633 PS22, Line 18: set_dev_state_by_devicetree
See my answer about the option name.
Ack
-
Tim Wawrzynczak (Code Review)