[coreboot-gerrit] [XL] Change in coreboot[main]: soc/ptl/acpi: Add SoC ACPI directory for Panther Lake

[Subrata Banik (Code Review)]

Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Hannah Williams, Jamie Ryu, 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/ptl/acpi: Add SoC ACPI directory for Panther Lake
......................................................................

Patch Set 23:

(20 comments)

File src/soc/intel/pantherlake/acpi/dptf.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/822145ad_5251a1a7?usp... :
PS23, Line 5: INTC1041
what is the source of this HID ?

Also, why overriding the default value "INT34xx" ?

File src/soc/intel/pantherlake/acpi/pcie.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/45327088_df3e6881?usp... :
PS23, Line 237: Name (_ADR, 0x001D0000)
i really don't understand what is the source of truth that your are following ?

As per EDS, 
RP09 - 0:06:0
RP10 - 0:06:1
RP11 - 0:06:2
RP12 - 0:06:3

You have listed RP09 as 0:0x1D:0.

I'm very disappointed with the code review :-(

https://review.coreboot.org/c/coreboot/+/83772/comment/7e1f4c66_fbc652a9?usp... :
PS23, Line 254: Name (_ADR, 0x001D0001)
```suggestion
    Name (_ADR, 0x00060001)
```

File src/soc/intel/pantherlake/acpi/serialio.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/3700eca5_ed1486a4?usp... :
PS23, Line 47: Device (I3C0)
             : {
             : 	Name (_ADR, 0x00110000)
             : 	Name (_DDN, "Serial IO I3C Controller 0")
             : }
             : 
             : Device (I3C1)
             : {
             : 	Name (_ADR, 0x00110002)
             : 	Name (_DDN, "Serial IO I3C Controller 1")
             : }
do we have any usage for I3C debugging ? I don't believe so

https://review.coreboot.org/c/coreboot/+/83772/comment/47d0bd10_2db50d90?usp... :
PS23, Line 59: 
empty line

File src/soc/intel/pantherlake/acpi/southbridge.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/3d9202f8_b48077e9?usp... :
PS23, Line 10: 
you still need this IMO because of 2nd P2SB device

```
/* IOE PCR access */
#if CONFIG(SOC_INTEL_COMMON_BLOCK_IOE_P2SB)
#include <soc/intel/common/acpi/ioe_pcr.asl>
#endif
```

https://review.coreboot.org/c/coreboot/+/83772/comment/f8c36b29_3a83c06a?usp... :
PS23, Line 11: /* PCH clock */
missing

```
/* PCIE src clock control */
#include <soc/intel/common/acpi/pcie_clk.asl>
```

https://review.coreboot.org/c/coreboot/+/83772/comment/ed78be7e_969260ce?usp... :
PS23, Line 47: /* UFS 0:17:0 */
             : #include "ufs.asl"
based on my understanding, PTL-U only has UFS controller hence, not sure if keeping ufs.asl unguarded make sense or not.

moreover, while including ufs.asl for any SOC platform, you need to define `UFS_ACPI_DEVICE` (which I don't see as part of this CL). Can you please double confirm ?

File src/soc/intel/pantherlake/acpi/tcss.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/209fee1a_87f8c069?usp... :
PS23, Line 396: 	/*OperationRegion (IOMR, SystemMemory, IOM_BASE_ADDR, 0x100)*/
???

https://review.coreboot.org/c/coreboot/+/83772/comment/a0a92c13_86e3b770?usp... :
PS23, Line 397: 0xE0800000
what is this hardcoded value ? IOM_BASE_ADDR is what you need here

```
#define IOM_BASE_ADDR		0x4010800000
#define IOM_BASE_SIZE		0x10000
#define IOM_BASE_ADDR_MAX	((IOM_BASE_ADDR + IOM_BASE_SIZE) - 1)
```

https://review.coreboot.org/c/coreboot/+/83772/comment/cd4657b8_a985cab1?usp... :
PS23, Line 512: 				If (_SB.PCI0.TDM1.IF30 != 1) {
              : 					Return
              : 				}
              : 
as i have doubted previously as well, you are pushing some stale code w/o bothering looking into what is there in upstream MTL and copied from there.

https://review.coreboot.org/c/coreboot/+/77456

we have dropped IF30 sometime now whereelse IF30 still exists in your code.

https://review.coreboot.org/c/coreboot/+/83772/comment/0e1e38d7_b9ad1571?usp... :
PS23, Line 727: 			If (TRE0 == 1) {
again same stale code

https://review.coreboot.org/c/coreboot/+/81842

File src/soc/intel/pantherlake/acpi/tcss_dma.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/cb685742_08f11fd5?usp... :
PS23, Line 9: 	Offset (0x85),
            : 	PMEE, 1,        /* 8, PME_EN */
can someone from Intel double check the spec, I don't see this TBT DMA device in EDS

https://review.coreboot.org/c/coreboot/+/83772/comment/5baa75c3_4a9374c2?usp... :
PS23, Line 15: 	IF30,  1,	/* ITBT FW Version Bit30 */
don't need

File src/soc/intel/pantherlake/acpi/tcss_pcierp.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/677b809a_ca58bb74?usp... :
PS23, Line 12: 	Offset(0x51),
why ? as there is no bit-field

https://review.coreboot.org/c/coreboot/+/83772/comment/335699cf_fcadbc75?usp... :
PS23, Line 28:                 
tab?

https://review.coreboot.org/c/coreboot/+/83772/comment/7cdb330c_763402c6?usp... :
PS23, Line 33:                
please try to be consistent with space and tab

https://review.coreboot.org/c/coreboot/+/83772/comment/0ea8d2dd_e77ec5b5?usp... :
PS23, Line 56: 0xBAC
use macro?

File src/soc/intel/pantherlake/acpi/tcss_xhci.asl:

https://review.coreboot.org/c/coreboot/+/83772/comment/6c60cbdb_678e7174?usp... :
PS23, Line 166: 2
0 aka _SB.PCI0.TDM1.0

https://review.coreboot.org/c/coreboot/+/83772/comment/93c14ba9_4c181e7a?usp... :
PS23, Line 183: 3
1 aka _SB.PCI0.TDM1.1