Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45972 )
Change subject: soc/intel/alderlake/acpi: Add SoC ACPI directory for ADL ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/camera_clock_ctl.asl:
PS2:
I think this is going to be useful for other SoCs as well (e.g. […]
i do expect to have some changes in this file based on ADL GPIO PIN. but we could evaluate the scope of common code in MTL time may be?
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 20: Method (OFST, 0x1, NotSerialized)
IMHO, this method is unnecessary. The calculation could be done directly in RAOW.
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 26: OR
Or
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 27: source and destination
Clock source select
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 31: 0x3
This is the number of arguments. It should be written in decimal.
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 44: * Arg0: Clock source select(0: IMGCLKOUT_0, 1: IMGCLKOUT_1, 2: IMGCLKOUT_2, 3: IMGCLKOUT_3, : * 4: IMGCLKOUT_4, 5: IMGCLKOUT_5)
I'd rather specify the range of valid values: […]
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 46: Select 24MHz / 19.2 MHz
Frequency select
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/dptf.asl:
PS2:
This just gave me an idea: Why not have each SoC define its ACPI device IDs in a file? e.g. include/soc/acpi_def.h
Very nice idea, please pick IA-common code, i can take the validation part if you wish for any SoC/platform for this effort
I think _ADR defines using the PCI_DEV macro should work too
_ADR might only need Device and Function hence we need to leave Bus calculation from PCI_DEV macro. may be need some new macro. i have attempted something for ish.asl 😊
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/ipu.asl:
PS2:
These and other files seem to be great candidates for common ACPI.
Ack
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/pch_hda.asl:
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 53: If (LEqual (Arg2, One))
old syntax
Ack