Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46713 )
Change subject: driver/usb/acpi: Add power resources for devices on USB ports ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
Patch Set 2:
(1 comment)
Hmm, this makes me want to attack the power resource problem again... With the devtree alias patches in, expressing power resource dependencies is even cleaner, e.g.,:
chip drivers/generic/power_resource/ register "on" = "{{GPIO_L(GPP_A1), 50}, {GPIO_L(GPIO_B2), 1},}" register "off" = "{{GPIO_L(GPP_B2), 1}, {GPIO(GPP_A1), 10},}" # or even the current interface in generic/2c with the stop_gpio, reset_gpio, etc. # but I kind of like this explicit sequencing you get here. device generic 0 alias i2c1_power_res on end end chip drivers/i2c/generic use i2c1_power_res as power_resource; device i2c 1f on end end chip drivers/spi/generic use i2c1_power_res as power_resource; device spi 0 on end endEach "resource" in the 'on' and 'off' list (i.e., GPIOs) can emit enable/disable methods that use reference counting to keep track of when it's safe to actually assert/deassert the pins.
WDYT? If you don't want to tackle that right now, factoring out the power resource fields into a common `struct power_resource_config` or similar would be better than copy-pasting these fields around into different drivers.
We might want to eventually do something like you recommended i.e. having a separate power resource device especially if multiple devices have to share power resource. Last week you mentioned that this might probably be a use case for the camera device.
One thing that we will probably have to think about some more is -- if a driver wants to expose the GPIOs in _CRS in addition to generating the power resource, then it will require both on/off as well as reset/enable GPIOs to be provided by mainboard in the devicetree entry. It might be helpful to write these thoughts in a doc/bug so that we can capture all scenarios.
About adding a power_resource structure - I think that can be a quick way forward right now especially if Karthik is looking to unblock the mainboard CLs. However, if you plan to refactor and add a new power_resource driver, do you want to wait until then to decide what the structure should really look like?
Just a strawman for the moment 😊
There are other considerations too, like ensuring that _PR0/_PR3 get emitted too. I have some thoughts, I should collect them into a doc.
I will start with aggregating the fields into a common power resource structure and a simple wrapper function to generate the ACPI identifiers. This can be organized something like drivers/power_resource/chip.h drivers/power_resource/power_resource.c
Once we have identified down all the use-cases, I can transform them into a driver covering all the documented use-cases. Does that sound good?
https://review.coreboot.org/c/coreboot/+/46713/2/src/drivers/usb/acpi/chip.h File src/drivers/usb/acpi/chip.h:
https://review.coreboot.org/c/coreboot/+/46713/2/src/drivers/usb/acpi/chip.h... PS2, Line 47: /* Disable reset and enable GPIO export in _CRS */ : bool disable_gpio_export_in_crs;
Thanks for the background, I had been wondering what the deal was with leaving the GPIOs out of _CRS […]
As per the existing use-case, reset_gpio has been already added to _CRS. Should we not expose in the existing use-cases too? I believe BT has been a regular user of the reset GPIO exported by _CRS.
Or did you mean not to expose when the GPIOs are meant specifically for power resource i.e. has_power_resource = 1?