John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39785 )
Change subject: soc/intel/tigerlake: Configure TCSS xHCI power management ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39785/3/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/39785/3/src/soc/intel/tigerlake/acp... PS3, Line 40: /* : * Procedure: GPRW : * : * Description: Generic Wake up Control Method to detect the Max Sleep State : * available in ASL Name scope and return the package compatible with _PRW format. : * Input: Arg0 = bit offset within GPE register space device event will be triggered to. : * Arg1 = Max Sleep state, device can resume the System from. : * If Arg1 = 0, Update Arg1 with Max _Sx state enabled in the System. : * Output: _PRW package : */ : Name (PRWP, Package(){ Zero, Zero }) // _PRW Package : Name (SS1, 0) : Name (SS2, 0) : Name (SS3, 1) : Name (SS4, 1) : : Method(GPRW, 2) : { : Store (Arg0, Index(PRWP, 0)) // copy GPE# : /* : * SS1-SS4 - Setup sleep states : */ : Store (ShiftLeft(SS1, 1), Local0) // S1 : Or (Local0, ShiftLeft(SS2, 2), Local0) // S2 : Or (Local0, ShiftLeft(SS3, 3), Local0) // S3 : Or (Local0, ShiftLeft(SS4, 4), Local0) // S4 : /* : * Local0 has a bit mask of enabled Sx(1 based) : * bit mask of enabled in setup sleep states(1 based) : */ : If (And (ShiftLeft(1, Arg1), Local0)) : { : /* : * Requested wake up value (Arg1) is present in Sx list of available : * Sleep states : */ : Store (Arg1, Index(PRWP, 1)) // copy Sx# : } : Else : { : /* : * Not available -> match Wake up value to the higher Sx state : */ : ShiftRight(Local0, 1, Local0) : FindSetLeftBit(Local0, Index(PRWP, 1)) // Arg1 == Min Sx : } : : Return (PRWP) : }
I feel like this function is unnecessary? Maybe as part of the full reference code where PRW is som […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/3/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/39785/3/src/soc/intel/tigerlake/acp... PS3, Line 145: /* : * Method for creating Type C _PLD buffers : * _PLD contains lots of data, but for purpose of internal validation we only care : * about ports visibility and pairing (this requires group position). So these are : * only those 2 configurable parameters. : */ : Method (TPLD, 2, Serialized) : { : /* : * Arg0: Visible : * Arg1: Group position : */ : Name (PCKG, Package() { Buffer(0x10) {} } ) : CreateField (DerefOf (Index(PCKG, 0)), 0, 7, REV) : Store (1, REV) : CreateField (DerefOf (Index(PCKG, 0)), 64, 1, VISI) : Store (Arg0, VISI) : CreateField (DerefOf (Index(PCKG, 0)), 87, 8, GPOS) : Store (Arg1, GPOS) : : /* For USB type C, Standard values */ : CreateField (DerefOf (Index(PCKG, 0)), 74, 4, SHAP) // Shape set to Oval : Store (1, SHAP) : CreateField (DerefOf (Index(PCKG, 0)), 32, 16, WID) // Width of the : // connector, 8.34mm : Store (8, WID) : CreateField (DerefOf (Index(PCKG, 0)), 48, 16, HGT) // Height of the : // connector, 2.56mm : Store (3, HGT) : return (PCKG) : } : : /* : * Method for creating Type C _UPC buffers : * Similar to _PLD, for internal testing we only care about 1 parameter. : */ : Method (TUPC, 2, Serialized) : { : /* : * Arg0: Connectable : * Arg1: Type : * Type: : * 0x08: Type C connector - USB2-only : * 0x09: Type C connector - USB2 and SS with Switch : * 0x0A: Type C connector - USB2 and SS without Switch : */ : Name (PCKG, Package(4) { 1, 0x00, 0, 0 } ) : Store (Arg0, Index(PCKG, 0)) : Store (Arg1, Index(PCKG, 1)) : return (PCKG) : }
these don't look to be used. we generate the _PLD and _UPC from devicetree.
Ack