Change in coreboot[master]: driver/usb/acpi: Add power resources for devices on USB ports

[Karthik Ramasubramanian (Code Review)]

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
> > >     end
> > >
> > > Each "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?