Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Felix Singer, Jakub Czapiga, Kapil Porwal, Saurabh Mishra, Tarun.
Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83354?usp=email )
Change subject: soc/intel/ptl: Do initial Panther Lake SoC commit till bootblock ......................................................................
Patch Set 14:
(20 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83354/comment/68184929_684a9083?usp... : PS8, Line 44: 0xfa000000
Due to PTL using config blocks (intermediality, UPD based FSP will be relased soon) to update to FSP-M/S, hence the size of DCACHE was increased. This setting is reverted back to previous program settings, and validated with LNL-M SOC SKU (FSP-Debug/Non-debug both).
I don't think PTL-FSP should be using config block, we have dropped that feature from PTL hence, this argument doesn't make any sense to me.
https://review.coreboot.org/c/coreboot/+/83354/comment/477b7a54_5840107a?usp... : PS8, Line 47: 0x200000
Due to PTL using config blocks (intermediality, UPD based FSP will be relased soon) to update to FSP-M/S, hence the size of DCACHE was increased. This setting is reverted back to previous program settings, and validated with LNL-M SOC SKU (FSP-Debug/Non-debug both).
same justification, please restore the changes w/o getting impacted by FSP config block which is non-POR for PTL
https://review.coreboot.org/c/coreboot/+/83354/comment/32c33095_9a6fe5f2?usp... : PS8, Line 63: 0x80100
Due to PTL using config blocks (intermediality, UPD based FSP will be relased soon) to update to FSP-M/S, hence the size of DCACHE was increased. This setting is reverted back to previous program settings, and validated with LNL-M SOC SKU (FSP-Debug/Non-debug both).
same justification
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83354/comment/a91cd180_735258d9?usp... : PS14, Line 25: select HAVE_X86_64_SUPPORT please use the order ?
https://review.coreboot.org/c/coreboot/+/83354/comment/c26b09a4_5f151821?usp... : PS14, Line 32: Choose this option if your mainboard has a PTL-U (15W) : or PTL-H 4Xe3 / 12Xe3 (25W) SoC. Choose this option if your mainboard has a PTL-UH SoC.
https://review.coreboot.org/c/coreboot/+/83354/comment/85796e91_cf3d5b09?usp... : PS14, Line 33: PTL-H 4Xe3 this is pure PTL-H and not part of the PTL-UH
https://review.coreboot.org/c/coreboot/+/83354/comment/e75115b1_caeb3e6a?usp... : PS14, Line 67: ~1KiB) 32KiB?
File src/soc/intel/pantherlake/espi.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/cf053ca4_29240850?usp... : PS14, Line 27: #if ENV_RAMSTAGE I believe this is not required. CB:83637
File src/soc/intel/pantherlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/28519cd3_a77ae3f0?usp... : PS14, Line 59: /* need white space around
File src/soc/intel/pantherlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/075fc440_7243ab93?usp... : PS8, Line 37: VTD_BASE_ADDRESS
Acknowledged
I'm unable to follow why you have marked this `ack` when I'm expecting little more clarification about what is DMI3BAR ?
https://review.coreboot.org/c/coreboot/+/83354/comment/2ec14909_f8e454a6?usp... : PS8, Line 37: fc800000
Acknowledged
why you have marked it `ack` when nothing has been changed ?
https://review.coreboot.org/c/coreboot/+/83354/comment/4c8944ca_7d8ef968?usp... : PS8, Line 74: 0x3fff0800000
IOM should be anywhere of size 64KB.
what you mean by anywhere, this number has to be aligned between FSP and coreboot. How about coreboot programs something differently than what FSP does?
File src/soc/intel/pantherlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/132a2d20_d1d64e43?usp... : PS14, Line 174: need two more space
File src/soc/intel/pantherlake/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/09480f09_425f22aa?usp... : PS14, Line 6: /* : * Port ids : */ why multiline ?
File src/soc/intel/pantherlake/include/soc/soc_info.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/593ecaf7_7129106f?usp... : PS14, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ please give one new line
https://review.coreboot.org/c/coreboot/+/83354/comment/ab2fda0f_8606b0a9?usp... : PS14, Line 5: enum { : NOT_DETECTED = 0, : PTLU, : PTLH, : }; why needed ?
File src/soc/intel/pantherlake/soc_info.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/ef1bbed0_c76f1255?usp... : PS14, Line 2: ``` #define __SIMPLE_DEVICE__ ```
to avoid #if/#else in line 14-20
https://review.coreboot.org/c/coreboot/+/83354/comment/546f3027_0261b076?usp... : PS14, Line 10: get_soctype why do we need this ? which problem this new API will solve. ideally an API should have its own comment block for caller to know when to use it
https://review.coreboot.org/c/coreboot/+/83354/comment/4979cdad_fd0141da?usp... : PS14, Line 69: printk(BIOS_DEBUG, "soc_info: max_pcie_clock:%d\n", pcie_clock); why we need these prints ?
https://review.coreboot.org/c/coreboot/+/83354/comment/3037dc59_1baa3c63?usp... : PS14, Line 92: } can you follow mtl soc_info.c? I guess your PTL code base is very old (atleast two quarter old because https://review.coreboot.org/c/coreboot/+/81111 actually did the code refactoring which is not present in your PTL code). Your soc_info.c code is actually the older version of MTL soc_info.c before https://review.coreboot.org/c/coreboot/+/81111 refactoring.
Please don't try to push older code base in form of PTL (it only makes things difficult and add up more work for us later).