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 13:
(9 comments)
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/rtd3/chip.h:
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 23: clock_pin
suggestion: srcclk_pin ?
Done
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 70:
odd alignment?
ya it went beyond 96 chars, such long names..
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 73:
should reset be asserted here first? or do you think the default programmed state of the pin + toggl […]
I think there is an implicit assumption that the pins are configured to retain their state in S0ix in order for this whole thing to work anyway.
I do wonder if the CLKSRC should be enabled before applying power, I followed the previous example code but I have found other examples where the clock was enabled first. (it seems to work either way...)
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 80: pmc_ipc_acpi_set_pci_clock(pcie_rp, config->clock_pin, true); I'm changing the config so if the clock is set to -1 it will not do this step because it doesn't seem to be needed for all devices and it makes it easier to reuse this code.
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 90: pcie_rtd3_acpi_l23_exit(); I'm adding a config option for the L23 entry/exit so it can be reused on devices where this isn't required.
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 140: acpigen_write_if_lequal_op_op(LOCAL0_OP, LOCAL1_OP); : acpigen_write_return_op(ZERO_OP); : acpigen_pop_len(); /* If */ : acpigen_write_else(); : acpigen_write_return_op(ONE_OP); : acpigen_pop_len(); /* Else */
so the kernel is treating the present bit as on/off here?
It should really just use the power enable to determine if the power is on. I had plumbed for the reset GPIO since the PEDET issue was preventing the power GPIO from being used properly but I should just remove it now and rely on power GPIO being present since it is 'required' for using this driver.
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 157: PCIE_PXCS_ADDRESS
I'd just use PCI_Config if possible, that's what it's for 😊
Ya that is what I went with since all the used registers are below 0xff anyway.
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 161: FIELDLIST_NAMESTR("LASX", 1), /* Link Active Status */ : FIELDLIST_OFFSET(PCH_PCIE_PXCS_CFG_SPR), : FIELDLIST_RESERVED(7), : FIELDLIST_NAMESTR("NCB7", 1), : FIELDLIST_OFFSET(PCH_PCIE_PXCS_CFG_RPPGEN), : FIELDLIST_RESERVED(2), : FIELDLIST_NAMESTR("L23E", 1), /* L23_Rdy Entry Request */ : FIELDLIST_NAMESTR("L23R", 1), /* L23_Rdy Detect Transition */
suggestion: macros for the field names so they can be reused above too
I can do that.
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 263: #if CONFIG(HAVE_ACPI_TABLES)
you already have `depends on` this in the Kconfig
Done