Duncan Laurie 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 12:
(8 comments)
only minor stuff
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 6: * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. Change to SPDX format?
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 163: */
96 columns
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 203: * 0xFE - Command timeout One of the calls to DSGS is doing "DSGS() & 0x2" which would return true with these failure return values.
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 381: ( nit: space before (
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/tcss_xhci.asl:
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 6: * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. change to SPDX style
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 16: BASE(_ADR), 0x100 Is there any downside to taking the whole 256 byte config space? Could this instead just expose one byte by setting the base to BASE(_ADR) + 0x74?
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 34: PMEE Are we sure this is posted write and doesn't need read back?
(this was an issue with the emmc controller on kabylake)
https://review.coreboot.org/c/coreboot/+/39785/12/src/soc/intel/tigerlake/ac... PS12, Line 150: Method (_PS0, 0, Serialized) : { : } : : Method (_PS2, 0, Serialized) : { : } : : Method (_PS3, 0, Serialized) : { : } do these empty methods need to exist?