[XL] Change in coreboot[main]: soc/intel/snowridge: Add support for Intel Atom Snow Ridge SoC

[Name of user not set (Code Review)]

Attention is currently required from: Angel Pons, Elyes Haouas, Felix Held, Felix Singer, Frans Hendriks, Jérémy Compostella, Maximilian Brune, Patrick Rudolph, Paul Menzel, Shuo Liu.

[e-mail] has posted comments on this change by [e-mail]. ( https://review.coreboot.org/c/coreboot/+/83321?usp=email )

Change subject: soc/intel/snowridge: Add support for Intel Atom Snow Ridge SoC
......................................................................

Patch Set 63:

(12 comments)

File src/soc/intel/snowridge/Kconfig:

https://review.coreboot.org/c/coreboot/+/83321/comment/7600e5a9_2a17ef32?usp... :
PS28, Line 39: 	select FSP_CAR
> Does native CAR work?

As Vasiliy replied to you in another comment, native CAR doesn't works on Snow Ridge.

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

https://review.coreboot.org/c/coreboot/+/83321/comment/3e267b04_eeeb7607?usp... :
PS60, Line 115: 	intel_write_pci_PRT(scope, pin_irq_map, map_count, &pirq_map);
> The scope could be obtained from domain's device path, e.g. […]

Done

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

https://review.coreboot.org/c/coreboot/+/83321/comment/441356a6_519f544b?usp... :
PS33, Line 235:
> Are RP04, RP05, RP06, RP07 not implemented in Snow Ridge?

Yes in Snow Ridge there are 2 Root Port clusters, cluster 0 and cluster 2. Cluster 0 contains Root Port 0~4 and cluster 2 contains Root Port 8~11. In the latest patch the ACPI PRT method for Root Port is generated dynamically.

File src/soc/intel/snowridge/acpi/southcluster.asl:

https://review.coreboot.org/c/coreboot/+/83321/comment/ca9aee79_cfdec085?usp... :
PS28, Line 13: // SATA Controller 0
             : 	Device (SAT0)
             : 	{
             : 		Name (_ADR, 0x00070000)
             : 		Name (_DDN, "SATA Controller 0")
             : 	}
             : 
             : 	// SATA Controller 2
             : 	Device (SAT2)
             : 	{
             : 		Name (_ADR, 0x000e0000)
             : 		Name (_DDN, "SATA Controller 2")
             : 	}
             :
> No SATA Controller 1?

Yes there are only SATA controller 0 and SATA controller 2 in Snow Ridge. They are only place holders thus I deleted them in the latest patch.

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

https://review.coreboot.org/c/coreboot/+/83321/comment/997b32a4_b4fb5643?usp... :
PS28, Line 229: 		die("Please add domain 0 to device tree!\n");
> Should be able to guarantee this using a chipset devicetree, btw.

In the latest patch the device tree is moved to `src/soc/intel/snowridge/chipset.cb`, mainboard only contains an empty device tree to pass compilation.

File src/soc/intel/snowridge/common/fsp_hob.h:

https://review.coreboot.org/c/coreboot/+/83321/comment/dfddb3d1_90d0f101?usp... :
PS28, Line 10: extern const uint8_t fsp_hob_fia_override_status_guid[16];
> Could use `guid_t` I believe: https://github. […]

Done

File src/soc/intel/snowridge/common/hob_display.c:

https://review.coreboot.org/c/coreboot/+/83321/comment/97c02057_13647006?usp... :
PS28, Line 12: 	const void *guid;
> Would be nice to use `guid_t` (well, `const guid_t *guid`): https://github. […]

Done

File src/soc/intel/snowridge/common/kti_cache.c:

PS28:
> Does KTI refer to UPI (high-speed CPU interconnect links on server platforms), or is it something el […]

yes, in FSP output its call KTI. I think KTI is code name, and UPI is official name.

File src/soc/intel/snowridge/common/reset.c:

https://review.coreboot.org/c/coreboot/+/83321/comment/be381012_780f9544?usp... :
PS28, Line 26: 	do_full_reset();
> Does the CSE or SPS need to be notified about the global reset? This feels odd

Actually I'm not very familiar with this. I checked other `do_global_reset()` implementations, they called `cse_request_global_reset()` in `src/soc/intel/common/block/cse/cse.c`, but on Snow Ridge, we didn't use that common code.

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

https://review.coreboot.org/c/coreboot/+/83321/comment/79a760d7_c7ad3111?usp... :
PS28, Line 72: /**
             : 	 * Skip Pre MP init MTRR programming as MTRRs are mirrored from BSP that are set prior to
             : 	 * ramstage. Real MTRRs programming are being done after resource allocation.
             : 	 */
> nit: reflow comment to not exceed 96 characters per line […]

Done

File src/soc/intel/snowridge/heci.c:

https://review.coreboot.org/c/coreboot/+/83321/comment/5dd22fc8_795d46c5?usp... :
PS28, Line 25: 	res->flags |= IORESOURCE_MEM;
> Wouldn't `IORESOURCE_MEM` be set already? Or is it mutuallyy exclusive with `IORESOURCE_PCI64`?

`IORESOURCE_MEM` is removed.

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

https://review.coreboot.org/c/coreboot/+/83321/comment/e40688ed_3bfe5a0e?usp... :
PS28, Line 13: @sa
> Does this mean "see also"? Not sure

yes that's a conventional usage of Doxygen.