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 9:
(19 comments)
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 46: , 8, : PBSS, 1, // Power Button Status : Offset(0x40), // 0x40, General Purpose Event Control : , 17, : GPEC, 1, // Software GPE Control
this looks unused?
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 74: ToInteger
comment above says this should be an integer?
Looks redundant as Arg3 is referred to xhci variable DCPM which already has definition of integer value 4.
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 135: _SB.PCI0.
already in _SB. […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 141: MBAR
does this work to re-use the same field name as above? can they just be merged or is the ByteAcc vs […]
It seems valid to refer to same filed name. DWordAcc validates index values of 0, 4, 8, etc. The above BMAR ByteAcc has 64 bytes of RBAR which is referred by IOMA method execution. It seems fine to keep two access type to differentiate RBAR and TCD3 bit access effectively.
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 263: LEqual (Local0, 100)
Local0 == 100
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 283: And (^RBAR, 0xFFFFFFFFFFFFFFFE)
Does (^RBAR & ~1) work?
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 287: dynamically
Nothing seems dynamic about this method
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 312: {
nit: CR before {
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 314: LEqual (TD3C, 1)
TD3C == 1
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 317: Store (0, TD3C)
TD3C = 0
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 318: Store (0, Local0)
Local0 = 0
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 318: /
nit: space after //
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 321: ///TCSS_IOM_ACK_TIMEOUT_IN_MS))) {
left over?
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 330: LEqual
maybe us LNotEqual so the if clause isn't empty? (or the ASL2. […]
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 348: } Else { : /* Drop this method due to it is already exit D3 cold */ : Return : }
this is unnecessary
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 379: PowerResource
this PowerResource is for _SB. […]
D3C is referred by xhci _PR0/_PR3 methods as _SB.PCI0.D3C.
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 408: (
nit: space before (
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 415:
nit: extra tab
Ack
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/39785/7/src/soc/intel/tigerlake/acp... PS7, Line 31: (
nit: space after (
Ack