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 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46260/10/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/rtd3/rtd3.c:
https://review.coreboot.org/c/coreboot/+/46260/10/src/soc/intel/common/block... PS10, Line 30: /* : * Delay up to wait_ms until provided register matches expected value. : * : * Local7 = segments : * While (Local7 > 0) : * { : * If (name == val) { : * Break : * } : * Sleep (wait_ms_segment) : * Local7++ : * } : */ : static void pcie_rtd3_acpi_delay(uint32_t wait_ms, const char *name, uint64_t val) : { : uint32_t wait_ms_segment = 1; : uint32_t segments = wait_ms; : : if (wait_ms > 32) { : wait_ms_segment = 16; : segments = wait_ms / 16; : } : : acpigen_write_store_int_to_op(segments, LOCAL7_OP); : acpigen_emit_byte(WHILE_OP); : acpigen_write_len_f(); : acpigen_emit_byte(LGREATER_OP); : acpigen_emit_byte(LOCAL7_OP); : acpigen_emit_byte(ZERO_OP); : if (name) { : acpigen_write_if_lequal_namestr_int(name, val); : acpigen_emit_byte(BREAK_OP); : acpigen_pop_len(); /* If */ : } : acpigen_write_sleep(wait_ms_segment); : acpigen_emit_byte(DECREMENT_OP); : acpigen_emit_byte(LOCAL7_OP); : acpigen_pop_len(); /* While */ : }
Seems like this might be common enough to go in acipgen, wdyt?
Ya I can do that.
https://review.coreboot.org/c/coreboot/+/46260/10/src/soc/intel/common/block... PS10, Line 114: /* Assert power gpio. */ : acpigen_enable_tx_gpio(power_gpio); : : /* Enable CLKSRC for root port. */ : pmc_ipc_acpi_set_pci_clock(pcie_rp, clock_pin, true); : : /* De-assert reset gpio. */ : if (reset_gpio->pin_count) : acpigen_disable_tx_gpio(reset_gpio);
It's surprising to me that there isn't some kind of minimum time in reset here (I see the ASL didn't […]
It does seems like there might be some sequencing violations without any delays. I can add some config options to add delays.
https://review.coreboot.org/c/coreboot/+/46260/10/src/soc/intel/common/block... PS10, Line 266: RTD3 Looks like this has to be named PXSX for the kernel to recognize it. (that seems like a completely unnecessary implementation detail and there is no reason it couldn't look for any device name as long as it had _ADR=0)