Tim Wawrzynczak 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:
(7 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 266: RTD3
Looks like this has to be named PXSX for the kernel to recognize it. […]
agreed
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?
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 + toggling _ON / _OFF is enough?
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?
https://review.coreboot.org/c/coreboot/+/46260/13/src/soc/intel/common/block... PS13, Line 157: PCIE_PXCS_ADDRESS
Obviously I was not thinking clearly when I did this, it should just be using PCI config space (or a […]
I'd just use PCI_Config if possible, that's what it's for 😊
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
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