Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32188 )
Change subject: WIP: Power resource stub ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
(21 comments)
This CL should be broken down into multiple smaller CLs:
- Adding probed property to sconfig to support probed devices
- Adding power resource device to device tree
- Adding power resource driver for ACPI
- Adding helper function for enabling power in bootblock
- Moving all mainboards using power resource to the newly added device
- Get rid of power resource properties from I2C generic and SPI devices.
- Get rid of probed property
If power is enabled in bootblock, and probing is done in ramstage, when is the appropriate time to deassert reset?
So you think it would be appropriate to assert enable GPIOs first, then sleep() until the max() of the enable times has elapsed, and then deassert resets?
Yes. grepping for enable_delay_ms in coreboot and ignoring the values of 1:
git grep ""enable_delay_ms"" | grep -v "1" src/mainboard/google/octopus/variants/bobba/overridetree.cb: register "enable_delay_ms" = "50" src/mainboard/google/octopus/variants/phaser/overridetree.cb: register "enable_delay_ms" = "50" src/mainboard/google/poppy/variants/baseboard/devicetree.cb: register "enable_delay_ms" = "250" src/mainboard/google/poppy/variants/nami/devicetree.cb: register "enable_delay_ms" = "5" src/mainboard/google/reef/variants/coral/devicetree.cb: register "enable_delay_ms" = "50" src/mainboard/google/reef/variants/snappy/devicetree.cb: register "enable_delay_ms" = "5" src/mainboard/google/reef/variants/snappy/devicetree.cb: register "enable_delay_ms" = "50" src/mainboard/google/sarien/variants/sarien/devicetree.cb: register "enable_delay_ms" = "5"
Looking closely at each of these, I think most of them are incorrect configurations. Same device is being used on other mainboards with enable delay of 1. I understand it is difficult to predict what a device in future would require for enable delay, but sprinkling the logic in more places seems to make it just more complicated than necessary. Thoughts?