Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/globalnvs.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 30: GNVS
Could you work on aligning those? Every SoC creating nearly identical GNVS is a maintenance burden a […]
https://review.coreboot.org/c/coreboot/+/36457/1
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 59: /* Set flag to enable USB charging in S3 */ : Method (S3UE) : { : Store (One, \S3U0) : } : : /* Set flag to disable USB charging in S3 */ : Method (S3UD) : { : Store (Zero, \S3U0) : } : : /* Set flag to enable USB charging in S5 */ : Method (S5UE) : { : Store (One, \S5U0) : } : : /* Set flag to disable USB charging in S5 */ : Method (S5UD) : { : Store (Zero, \S5U0) : } not required, already deleted in ICL
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/acp... PS8, Line 22: : OperationRegion (APMP, SystemIO, 0xb2, 2) : Field (APMP, ByteAcc, NoLock, Preserve) : { : APMC, 8, // APM command : APMS, 8 // APM status : }
Is this used? This is also very common on Intel SoC.
https://review.coreboot.org/c/coreboot/+/36462/1
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... File src/soc/intel/tigerlake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/boo... PS8, Line 75: reg32 |= PCI_COMMAND_MEMORY;
to many spaces
Done https://review.coreboot.org/c/coreboot/+/36465
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/cpu... File src/soc/intel/tigerlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/36087/8/src/soc/intel/tigerlake/cpu... PS8, Line 130: /* Set PM1 timer IO port and enable*/
missing space
Done https://review.coreboot.org/c/coreboot/+/36465
-
Subrata Banik (Code Review)