Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46260 )
Change subject: soc/intel/common: Add PCIe Runtime D3 driver for ACPI ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/rtd3/chip.h:
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... PS15, Line 33: OS to apply security restrictions.
Are these security restrictions specific to D3?
This ties into external devices in general but there are some devices like SD-express which need both the external property and the other RTD3 properties like hotplug.
What makes it difficult to split into separate drivers is the limitation of only one _DSD in the device. Even with separate UUID for this property it needs to be under the _DSD package, and currently that means it has to be written out at the same time as any other properties.
I did write a separate 'drivers/generic/external' driver for supplying this property separately but have not uploaded it since I don't have a use case yet.
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... PS15, Line 44: Disable the ACPI-driven L23
Under what conditions would a mainboard want to disable this?
I've been asking myself (and Intel) the same question. They have said that SD doesn't need it, but I am finding that s0ix testing is more stable if I do the full flow with L23 and clock disable. I put this in to make the driver more flexible in case we do find a situation where the GPIO controls are needed but the L23 flow is not.
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... PS15, Line 148: config->enable_gpio.pin_count
Isn't enable gpio mandatory as per https://review.coreboot. […]
Ah I need to fix the comment in chip.h as I did end up making it optional.
On volteer the later revisions of the RTS5261 daughter board repurposed the power enable GPIO as a presence input from the SD-express device, so we don't actually have power control but we still need to be able to (pretend to?) do RTD3 on that port with just placing the device in reset.
I suspect this isn't something we would ship a product with (would probably want HW to add an enable GPIO) but it is useful for supporting the different DBs for volteer.
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... PS15, Line 199: !config->enable_gpio.pin_count
Is it okay not to provide the enable GPIO?
Ack
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... PS15, Line 241: PCIE_HOTPLUG_IN_D3_UUID
Is this true for all PCIe devices? Or would it be only for external facing ports? Given that there i […]
This is the property that actually tells the kernel to run the _ON/_OFF methods on a hotplug root port (or force it on the kernel command line with pcie_port_pm=force).
It seems to have originally came about with thunderbolt but seems to now extend to any RTD3 capable port.
There is the separate question of whether devices like regular (non-express) SD should be hotplug ports at all, but I found that when I did not enable the root port for hotplug with a regular GL9755 it would have issues on resume when the device lost power in D3. So the two seem to go together if we are looking to do power control for getting into deeper S0ix substates.
https://review.coreboot.org/c/coreboot/+/46260/15/src/soc/intel/common/block... PS15, Line 281: PXSX
Is this not present in kernel v5.4 in chromium tree? I was not able to find it. […]
It is still pending backport into 5.4, waiting for this patch to land in coreboot: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2... (not sure why though, it doesn't do anything by itself)
As for the naming, I have only seen this specific name required for NVMe but Intel does use it for devices attached to thunderbolt ports as well.