[XL] Change in coreboot[main]: soc/intel/ptl: Do initial Panther Lake SoC commit till ramstage

[Subrata Banik (Code Review)]

Attention is currently required from: Cliff Huang, Kapil Porwal, Pranava Y N, Saurabh Mishra.

Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83798?usp=email )

Change subject: soc/intel/ptl: Do initial Panther Lake SoC commit till ramstage
......................................................................

Patch Set 55:

(24 comments)

File src/soc/intel/pantherlake/acpi.c:

https://review.coreboot.org/c/coreboot/+/83798/comment/c9d9de06_b89fb271?usp... :
PS55, Line 124: 		printk(BIOS_ERR, "Error: Configuration could not be retrieved.\n");
shouldn't we return when we have detected a NULL pointer? you have identified a NULL pointer and next line#126, you have de-refer to fetch `s0ix_enable`. how is that possible ?

additionally, this is single line statement hence, you don't need brace around

https://review.coreboot.org/c/coreboot/+/83798/comment/0dafd437_dea832ff?usp... :
PS55, Line 148: 	}
same

https://review.coreboot.org/c/coreboot/+/83798/comment/fe8b4de5_4a960d79?usp... :
PS55, Line 161: 	}
same

https://review.coreboot.org/c/coreboot/+/83798/comment/f0ee9690_b744f1f3?usp... :
PS55, Line 233: vtd_engine_enabled
this looks like a `bool` value that meant to check if VTD is enabled or not ? if yes, then perform below logic

```
const bool vtd_engine_enabled = MCHBAR32(GFXVTBAR) & VTBAR_ENABLED; 
const uint64_t gfxvtbar = MCHBAR64(GFXVTBAR) & VTBAR_MASK;
```

https://review.coreboot.org/c/coreboot/+/83798/comment/f57f8c40_72b287e2?usp... :
PS55, Line 235: DEBUG_WILL
what you mean by `DEBUG_WILL` is this kind of macro ?

https://review.coreboot.org/c/coreboot/+/83798/comment/3a5486e0_a381243b?usp... :
PS55, Line 238: PCI_DEVFN_IGD
we kept IGD default enabled. does this still make sense ?

https://review.coreboot.org/c/coreboot/+/83798/comment/8cee8b95_2fe1eb28?usp... :
PS55, Line 238: 0x1
are you checking BIT0? if yes, then what BIT-0 refers to here ?

refer to my feedback above

https://review.coreboot.org/c/coreboot/+/83798/comment/ce159de4_51d6046b?usp... :
PS55, Line 240: DEBUG_WILL
same?

https://review.coreboot.org/c/coreboot/+/83798/comment/512fe5ab_5547ec5d?usp... :
PS55, Line 241: GFXVT_BASE_ADDRESS
why are you  not passing `MCHBAR32(GFXVTBAR)` aka gfxvtbar

https://review.coreboot.org/c/coreboot/+/83798/comment/4d4ab024_f922bcfd?usp... :
PS55, Line 247: (is_devfn_enabled(PCI_DEVFN_DPTF) ||
              : 		is_devfn_enabled(PCI_DEVFN_IPU) ||
              : 		is_devfn_enabled(PCI_DEVFN_NPU) ||
              : 		is_devfn_enabled(PCI_DEVFN_IAA))
why we need to check these logic twice?

one here and one at line#258-#265?

just once makes sense to me. Just check if BIT(1) is set?

https://review.coreboot.org/c/coreboot/+/83798/comment/0b455056_a96ff997?usp... :
PS55, Line 251: 0x2
what is BIT(1)? need macro

https://review.coreboot.org/c/coreboot/+/83798/comment/e6002d46_02edef40?usp... :
PS55, Line 330: 0x7
what is this magic value ? can you please explain?

https://review.coreboot.org/c/coreboot/+/83798/comment/121f4e90_a0ecc353?usp... :
PS55, Line 347: 	}
why using NULL pointer w/o returning?

File src/soc/intel/pantherlake/chip.h:

https://review.coreboot.org/c/coreboot/+/83798/comment/8330a623_a98ee3f4?usp... :
PS55, Line 46: 	PTL_U_404_4_15W_CORE, /* 4 P-cores +0 E-cores + 4 LP E-cores + 4Xe25W*/
one space after `+`

https://review.coreboot.org/c/coreboot/+/83798/comment/20e53648_6cb5ac17?usp... :
PS55, Line 46: 4Xe25W
this is 25W?

https://review.coreboot.org/c/coreboot/+/83798/comment/a770c792_1b886a77?usp... :
PS55, Line 49: PTL_H_484_45W_CORE
n

https://review.coreboot.org/c/coreboot/+/83798/comment/b72a63b6_71bb9759?usp... :
PS55, Line 68: PCI_DID_INTEL_PTL_H_ID_2
as per EDS this is 45W SOC part bt you have added this as 25W SKU

File src/soc/intel/pantherlake/chip.h:

https://review.coreboot.org/c/coreboot/+/83798/comment/9f2b3d0b_38b5af58?usp... :
PS50, Line 43: 	PTL_U_404_15W_CORE,
             : 	PTL_H_484_25W_CORE,
             : 	PTL_H_484_45W_CORE,
> Hi Subrata, 404 means =(4 P-cores +0 E-cores + 4 LP
> E-cores) & 484 mean =(4 P-cores +8 E-cores + 4 LP Ecores).
>
> Is it Ok to carry forwaed this naming?
> About keeping comment, i have add them. Please let me know if this works.

please add those details as part of the comment then only have such magic numbers in macro name else drop it.

File src/soc/intel/pantherlake/chip.c:

https://review.coreboot.org/c/coreboot/+/83798/comment/5596a056_46458902?usp... :
PS55, Line 167: 	}
same , please return if NULL

https://review.coreboot.org/c/coreboot/+/83798/comment/ef43637c_9d770b1a?usp... :
PS55, Line 256: soc
pcd as well ?

File src/soc/intel/pantherlake/cpu.c:

https://review.coreboot.org/c/coreboot/+/83798/comment/0d8a3287_a6f36046?usp... :
PS55, Line 129: 	}
same

https://review.coreboot.org/c/coreboot/+/83798/comment/810ca143_b850e8ae?usp... :
PS55, Line 171: 	}
same

File src/soc/intel/pantherlake/include/soc/p2sb.h:

https://review.coreboot.org/c/coreboot/+/83798/comment/15acc40a_c3d7b30b?usp... :
PS55, Line 12:  soc_
pcd

File src/soc/intel/pantherlake/p2sb.c:

https://review.coreboot.org/c/coreboot/+/83798/comment/889ff557_bca7a431?usp... :
PS55, Line 47:  soc_
pcd