[coreboot-gerrit] [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, Jérémy Compostella, 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 71:

(25 comments)

File src/soc/intel/pantherlake/Makefile.mk:

https://review.coreboot.org/c/coreboot/+/83798/comment/44828877_eab3f450?usp... :
PS71, Line 48: ramstage-y += soc_info.c
can you please follow the order ?

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

https://review.coreboot.org/c/coreboot/+/83798/comment/cc846f9d_99bd296f?usp... :
PS71, Line 126: 	if (!initialized) {
why not bail out early ? w/o need to give one extra tab?

```
if (initialized)
  return map;

config_t *config = config_of_soc();
if (config == NULL) {
    printk(BIOS_ERR, "Error: Configuration could not be retrieved.\n");
    return NULL;
}
....
....
```

https://review.coreboot.org/c/coreboot/+/83798/comment/0fcc849f_23c9ff99?usp... :
PS71, Line 251: 			
I believe it would fit in line above?

https://review.coreboot.org/c/coreboot/+/83798/comment/fe5d1e44_facc44f7?usp... :
PS71, Line 262: 
why empty line?

https://review.coreboot.org/c/coreboot/+/83798/comment/4238e4f3_443b30fe?usp... :
PS71, Line 280: 
same

https://review.coreboot.org/c/coreboot/+/83798/comment/8e550794_00c0ec63?usp... :
PS71, Line 286: V_P2SB_CFG_IBDF_FUNC
this would fit into line above

https://review.coreboot.org/c/coreboot/+/83798/comment/f7e6764d_f7d0d0e2?usp... :
PS71, Line 287: 	current += acpi_create_dmar_ds_msi_hpet(current,
              : 			0, V_P2SB_CFG_HBDF_BUS, V_P2SB_CFG_HBDF_DEV,
              : 			V_P2SB_CFG_HBDF_FUNC);
try to reflow the line till 96 char

https://review.coreboot.org/c/coreboot/+/83798/comment/8bd14b8a_df8e6460?usp... :
PS71, Line 296: 		current += acpi_create_dmar_rmrr(current, 0,
              : 				sa_get_gsm_base(), sa_get_tolud_base() - 1);
              : 		current += acpi_create_dmar_ds_pci(current, 0, PCI_DEV_SLOT_IGD, 0);
in all previous code you have given one empty line between `acpi_create_dmar_drhd` and `acpi_create_dmar_ds_pci`. now this line not following the same practice. try to be consistent in you have added one empty line for better code readability then use here too ?

https://review.coreboot.org/c/coreboot/+/83798/comment/9f552263_021753b0?usp... :
PS71, Line 303: 
why empty line

https://review.coreboot.org/c/coreboot/+/83798/comment/ee610d4e_c6ade57f?usp... :
PS71, Line 305: 
              : 		current += acpi_create_dmar_satc(current, ATC_REQUIRED, 0);
              : 			current += acpi_create_dmar_ds_pci(current, 0, PCI_DEV_SLOT_IGD, 0);
same feedback as above

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

https://review.coreboot.org/c/coreboot/+/83798/comment/2408a0ec_67ccdd13?usp... :
PS71, Line 60: 	TDP_45W = 45
nit

```
TDP_45W = 45,
```

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

https://review.coreboot.org/c/coreboot/+/83798/comment/15af1fdb_f98bf372?usp... :
PS71, Line 80:             
please use consistent tab

https://review.coreboot.org/c/coreboot/+/83798/comment/8360cfeb_09153f97?usp... :
PS71, Line 202: 	if (CONFIG(SOC_INTEL_CSE_SEND_EOP_EARLY) ||
              : 	    CONFIG(SOC_INTEL_CSE_SEND_EOP_ASYNC)) {
I hope it would fit within char < 96

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

https://review.coreboot.org/c/coreboot/+/83798/comment/a009a3db_05d6844d?usp... :
PS71, Line 140: 
should we be adding GPE1 as well here?

@[e-mail]

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

https://review.coreboot.org/c/coreboot/+/83798/comment/ba53ae8a_e9d06ba8?usp... :
PS71, Line 7: #define C1_LATENCY	1
            : #define C6_LATENCY	127
            : #define C7_LATENCY	253
            : #define C8_LATENCY	260
            : #define C9_LATENCY	487
            : #define C10_LATENCY	1048
same

https://review.coreboot.org/c/coreboot/+/83798/comment/23f33192_b191c1d0?usp... :
PS71, Line 15: #define C1_POWER	0x3e8
             : #define C6_POWER	0x15e
             : #define C7_POWER	0xc8
             : #define C8_POWER	0xc8
             : #define C9_POWER	0xc8
             : #define C10_POWER	0xc8
please confirm the applicability and authenticity of these values  for PTL SoC.

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

https://review.coreboot.org/c/coreboot/+/83798/comment/fbc3d7a2_b31c1bfa?usp... :
PS71, Line 6: 
drop one empty line

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

https://review.coreboot.org/c/coreboot/+/83798/comment/724201d4_ad698da1?usp... :
PS71, Line 23: DMI
```
/* Add Dummy Entry to cater common/block/acpi/acpi/northbridge.asl */
```

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

https://review.coreboot.org/c/coreboot/+/83798/comment/878a5db3_c392c15e?usp... :
PS71, Line 29: reg
can you please check if the `reg` is non-zero ?

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

https://review.coreboot.org/c/coreboot/+/83798/comment/8602614e_08b1dbf2?usp... :
PS71, Line 110: 
should we add GPE1 as well here

@[e-mail]

https://review.coreboot.org/c/coreboot/+/83798/comment/b557beab_73ba2c5b?usp... :
PS71, Line 152: 
please add a NULL check

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

https://review.coreboot.org/c/coreboot/+/83798/comment/2dc4699b_bf1b3e66?usp... :
PS71, Line 19: 
             : 	for (uint8_t i = 0; i < MAX_TYPE_C_PORTS; i++) {
```
    for (uint8_t i = 0, int ec_port = 0; i < MAX_TYPE_C_PORTS; i++) {
```

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

https://review.coreboot.org/c/coreboot/+/83798/comment/b070fb08_eb02ef6b?usp... :
PS71, Line 38: 0x40000000,
can we use a macro ?

if this is PCI device (like audio controller), then try to use pre-defined macro

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

https://review.coreboot.org/c/coreboot/+/83798/comment/5e65de34_d2e146b5?usp... :
PS71, Line 33: // PCH_PRESERVERD covers:
             : 		// TraceHub SW BAR, PMC MBAR, SPI BAR0, SerialIo BAR in ACPI mode
             : 		// eSPI LGMR BAR, eSPI2 SEGMR BAR, TraceHub MTB BAR, TraceHub FW BAR
             : 		// IOE PMC BAR, Tracehub RTIT BAR (SOC), HECI{1,2,3} BAR0
             : 		// see fsp/ClientOneSiliconPkg/Fru/MtlSoc/Include/PchReservedResources.h
I understand that you are concerned about the accessibility of this code. I will remove the comment so that it is no longer visible to everyone.

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

https://review.coreboot.org/c/coreboot/+/83798/comment/892df576_4a3257fc?usp... :
PS48, Line 6: const struct soc_tcss_ops tcss_ops = {
> Hi Subrata,we will be requiring this, since TCSS I/O Manager is above 4GB.
>
> const struct soc_tcss_ops tcss_ops = {
>         .configure_aux_bias_pads = ioe_tcss_configure_aux_bias_pads_sbi,
>         .valid_tbt_auth = ioe_tcss_valid_tbt_auth,
> };

if you need this then please try to implement it w/o keeping skeleton code ?