Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Hannah Williams, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Wonkyu Kim.
Subrata Banik has posted comments on this change by Ravishankar Sarawadi. ( https://review.coreboot.org/c/coreboot/+/83772?usp=email )
Change subject: soc/intel/ptl: Add SoC ACPI directory for Panther Lake ......................................................................
Patch Set 104:
(13 comments)
Patchset:
PS104: me
File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/83772/comment/3cbd670c_58857b78?usp... : PS104, Line 284: Name (_UID, 1) please submit this CL separately
File src/soc/intel/pantherlake/acpi/camera_clock_ctl.asl:
https://review.coreboot.org/c/coreboot/+/83772/comment/42f01c84_df3ae1c5?usp... : PS104, Line 3: #define R_ICLK_PCR_CAMERA1 0x8000 : #define B_ICLK_PCR_FREQUENCY 0x1 : #define B_ICLK_PCR_REQUEST 0x2 : can you please point me at the EDS section to compare the bit-field?
I could see different macros are being used across different SoC.
``` grep -rsn "B_ICLK_PCR_FREQUENCY" src/soc/intel/ src/soc/intel/pantherlake/acpi/camera_clock_ctl.asl:4:#define B_ICLK_PCR_FREQUENCY 0x1 src/soc/intel/pantherlake/acpi/camera_clock_ctl.asl:40: RAOW (Arg0, ~B_ICLK_PCR_FREQUENCY, Arg1) src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl:4:#define B_ICLK_PCR_FREQUENCY 0x1 src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl:51: RAOW (Arg0, ~B_ICLK_PCR_FREQUENCY, Arg1) src/soc/intel/alderlake/acpi/camera_clock_ctl.asl:4:#define B_ICLK_PCR_FREQUENCY 0x1 src/soc/intel/alderlake/acpi/camera_clock_ctl.asl:40: RAOW (Arg0, ~B_ICLK_PCR_FREQUENCY, Arg1) src/soc/intel/meteorlake/acpi/camera_clock_ctl.asl:4:#define B_ICLK_PCR_FREQUENCY 0x3 src/soc/intel/meteorlake/acpi/camera_clock_ctl.asl:44: RAOW (Arg0, ~B_ICLK_PCR_FREQUENCY, Arg1) src/soc/intel/jasperlake/acpi/camera_clock_ctl.asl:4:#define B_ICLK_PCR_FREQUENCY 0x1 src/soc/intel/jasperlake/acpi/camera_clock_ctl.asl:52: RAOW (Arg0, ~B_ICLK_PCR_FREQUENCY, Arg1)
```
File src/soc/intel/pantherlake/acpi/tcss.asl:
https://review.coreboot.org/c/coreboot/+/83772/comment/ad95da09_c55ace64?usp... : PS104, Line 396: /*OperationRegion (IOMR, SystemMemory, IOM_BASE_ADDR, 0x100)*/ do we need this ?
https://review.coreboot.org/c/coreboot/+/83772/comment/67fddb7d_aa9886dd?usp... : PS104, Line 779: If (TRE2 == 1) { same
https://review.coreboot.org/c/coreboot/+/83772/comment/25ddaab3_eb511bdd?usp... : PS104, Line 809: If (TRE3 == 1) { again same problem, this PTL code is so old that you need to rebase to apply latest code changes.
Refer to https://review.coreboot.org/c/coreboot/+/81842
File src/soc/intel/pantherlake/acpi/tcss_dma.asl:
https://review.coreboot.org/c/coreboot/+/83772/comment/4f6e70fc_feb606c9?usp... : PS104, Line 13: 0xC8 use smallcase `0xc8`
https://review.coreboot.org/c/coreboot/+/83772/comment/202d20f1_14744141?usp... : PS104, Line 22: Offset (0xFF), : DMAD, 8 /* 31:24 DMA Active Delay */ can you please point me at EDS chapter ?
File src/soc/intel/pantherlake/acpi/tcss_pcierp.asl:
https://review.coreboot.org/c/coreboot/+/83772/comment/efb47b3f_b82eede4?usp... : PS23, Line 56: 0xBAC
Subrata, I browsed across the ASL files and check how the Field under OperationRegion are written, the Offset always uses with direct value instead of macros. I assume you mean all the hard-coded offset value and not just this particular one. Unless the same offset has been used in multiple places, I'd rather leave it as it for consistency reason. In addition, the ASL will look closer to its converted .dsl file from the platform for debug/comparison purpose. Will it be okay?
a macro is desired to explain the purpose, preferably register name.
for example:
1. 0xBB2 <---- Root Port Power Gating Enable = TCSS_CFG_RPPGEN_FROM_CCFG
File src/soc/intel/pantherlake/acpi/tcss_pcierp.asl:
https://review.coreboot.org/c/coreboot/+/83772/comment/f7052859_78a8b331?usp... : PS104, Line 22: Offset (0x5B), we don't need this line
https://review.coreboot.org/c/coreboot/+/83772/comment/0b95114b_57d8ddbb?usp... : PS104, Line 15: Offset(0x5A), /* SLSTS[7:0] - Slot Status Register */ : ABPX, 1, /* 0, Attention Button Pressed */ : , 2, : PDCX, 1, /* 3, Presence Detect Changed */ : , 2, : PDSX, 1, /* 6, Presence Detect State */ : , 1, : Offset (0x5B), : DLSC, 1, /* 8, Data Link Layer State Changed */ why not this? ``` Offset(0x5A), /* SLSTS[7:0] - Slot Status Register */ ABPX, 1, /* 0, Attention Button Pressed */ , 2, PDCX, 1, /* 3, Presence Detect Changed */ , 2, PDSX, 1, /* 6, Presence Detect State */ , 1, DLSC, 1, /* 8, Data Link Layer State Changed */ ```
https://review.coreboot.org/c/coreboot/+/83772/comment/d0dd931b_5b9706c7?usp... : PS104, Line 24: Offset(0x60), /* RSTS - Root Status Register */ : Offset(0x62), why not
``` Offset(0x60), /* RSTS - Root Status Register */ , 16, ```
https://review.coreboot.org/c/coreboot/+/83772/comment/18de563d_a1fcef0c?usp... : PS104, Line 32: Offset (0x404), : LSOE, 1, : LNSE, 1, i don't see the register definition is part of EDS