John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
mb/google/volteer: Include TCSS power management
Include TCSS RTD3 into ACPI DSDT table.
BUG=b:140290596 TEST=Booted to kernel and verified tcss xhci/pcierp/dma power state D3 entry/exit.
Change-Id: Iae31a29eb23f7370737d097dd401f4056b8b7052 Signed-off-by: John Zhao john.zhao@intel.com --- M src/mainboard/google/volteer/dsdt.asl 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/40616/1
diff --git a/src/mainboard/google/volteer/dsdt.asl b/src/mainboard/google/volteer/dsdt.asl index 640f7cd..3cb10af 100644 --- a/src/mainboard/google/volteer/dsdt.asl +++ b/src/mainboard/google/volteer/dsdt.asl @@ -31,6 +31,7 @@ { #include <soc/intel/common/block/acpi/acpi/northbridge.asl> #include <soc/intel/tigerlake/acpi/southbridge.asl> + #include <soc/intel/tigerlake/acpi/tcss.asl> } }
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
northbridge.asl is fine to me.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
northbridge.asl is fine to me.
Sorry, I take it back as northbridge.asl is at src/soc/intel/common/block/acpi. It appears not a proper to include tcss.asl there.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
northbridge.asl is fine to me.
Sorry, I take it back as northbridge.asl is at src/soc/intel/common/block/acpi. It appears not a proper to include tcss.asl there.
It could be good there since new SOCs will also have TCSS.
Perhaps we need a Kconfig option to enable TCSS in an SOC? There might be more code coming here with all the ACPI generation.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
northbridge.asl is fine to me.
Sorry, I take it back as northbridge.asl is at src/soc/intel/common/block/acpi. It appears not a proper to include tcss.asl there.
It could be good there since new SOCs will also have TCSS.
Perhaps we need a Kconfig option to enable TCSS in an SOC? There might be more code coming here with all the ACPI generation.
Kconfig can be updated by adding SOC_INTEL_TIGERLAKE_TCSS. But Northbridge.asl is at common/block/acpi and the platform tcss (like tigerlake) is at soc/tigerlake/acpi/tcss.asl. It seems contradiction to the common code principal if simply including platform tcss into northbridge.asl like #if CONFIG(SOC_INTEL_TIGERLAKE_TCSS) #include <soc/intel/tigerlake/acpi/tcss.asl #endif
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
> Patch Set 1: > > Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard?
It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
northbridge.asl is fine to me.
Sorry, I take it back as northbridge.asl is at src/soc/intel/common/block/acpi. It appears not a proper to include tcss.asl there.
It could be good there since new SOCs will also have TCSS.
Perhaps we need a Kconfig option to enable TCSS in an SOC? There might be more code coming here with all the ACPI generation.
Kconfig can be updated by adding SOC_INTEL_TIGERLAKE_TCSS. But Northbridge.asl is at common/block/acpi and the platform tcss (like tigerlake) is at soc/tigerlake/acpi/tcss.asl. It seems contradiction to the common code principal if simply including platform tcss into northbridge.asl like #if CONFIG(SOC_INTEL_TIGERLAKE_TCSS) #include <soc/intel/tigerlake/acpi/tcss.asl #endif
I guess this is fine in the mainboards for now.
Should there be common TCSS code? i.e. will this TCSS code will be re-usd in future SOCs? We need to start planning now to avoid having to copy this around.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
> Patch Set 1: > > > Patch Set 1: > > > > Can this go in southbridge.asl directly so it doesn't need to be added by each mainboard? > > It might get confusion since tcss is not part of southbridge.
Tcss is located on northbridge, not PCH. It seems meaningful to include tcss inside northbridge.asl. Is it ok?
northbridge.asl is fine to me.
Sorry, I take it back as northbridge.asl is at src/soc/intel/common/block/acpi. It appears not a proper to include tcss.asl there.
It could be good there since new SOCs will also have TCSS.
Perhaps we need a Kconfig option to enable TCSS in an SOC? There might be more code coming here with all the ACPI generation.
Kconfig can be updated by adding SOC_INTEL_TIGERLAKE_TCSS. But Northbridge.asl is at common/block/acpi and the platform tcss (like tigerlake) is at soc/tigerlake/acpi/tcss.asl. It seems contradiction to the common code principal if simply including platform tcss into northbridge.asl like #if CONFIG(SOC_INTEL_TIGERLAKE_TCSS) #include <soc/intel/tigerlake/acpi/tcss.asl #endif
I guess this is fine in the mainboards for now.
Should there be common TCSS code? i.e. will this TCSS code will be re-usd in future SOCs? We need to start planning now to avoid having to copy this around.
Agree.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
Patch Set 1: Code-Review+2
Duncan Laurie has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40616 )
Change subject: mb/google/volteer: Include TCSS power management ......................................................................
mb/google/volteer: Include TCSS power management
Include TCSS RTD3 into ACPI DSDT table.
BUG=b:140290596 TEST=Booted to kernel and verified tcss xhci/pcierp/dma power state D3 entry/exit.
Change-Id: Iae31a29eb23f7370737d097dd401f4056b8b7052 Signed-off-by: John Zhao john.zhao@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40616 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M src/mainboard/google/volteer/dsdt.asl 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/dsdt.asl b/src/mainboard/google/volteer/dsdt.asl index 450835d..e2dcaef 100644 --- a/src/mainboard/google/volteer/dsdt.asl +++ b/src/mainboard/google/volteer/dsdt.asl @@ -31,6 +31,7 @@ { #include <soc/intel/common/block/acpi/acpi/northbridge.asl> #include <soc/intel/tigerlake/acpi/southbridge.asl> + #include <soc/intel/tigerlake/acpi/tcss.asl> } /* Mainboard hooks */ #include "mainboard.asl"
-
Duncan Laurie (Code Review) -
John Zhao (Code Review)