[S] Change in coreboot[main]: mb/google/brox: Fix CPU crashlog device MMIO memory access

[Angel Pons (Code Review)]

Attention is currently required from: Benjamin Doron, Dinesh Gehlot, Kane Chen, Kapil Porwal, Karthik Ramasubramanian, Nick Vaccaro, Paul Menzel, Sowmya Aralguppe, Subrata Banik.

Angel Pons has posted comments on this change by Sowmya Aralguppe. ( https://review.coreboot.org/c/coreboot/+/82136?usp=email )

Change subject: mb/google/brox: Fix CPU crashlog device MMIO memory access
......................................................................

Patch Set 10:

(3 comments)

File src/soc/intel/alderlake/crashlog.c:

https://review.coreboot.org/c/coreboot/+/82136/comment/ac0df1e4_414ad3de?usp... :
PS10, Line 205: &
> I am reading the value in the CRASHLOG_HEADER and CRASHLOG_CAPABILITY_STATUS registers and populatin […]

Ah, it's two separate registers... Later stored as a single 64-bit value for reasons I cannot comprehend. Oh, well.

In that case, can we *not* name these variables `dw0` and `dw1`, please? `cl_header` and `cl_cap_status` would make the code substantially easier to follow.

https://review.coreboot.org/c/coreboot/+/82136/comment/1aa5aef0_b317da92?usp... :
PS10, Line 215: 
              : 	for (int i = 0; i < cpu_cl_disc_tab.header.fields.count; i++) {
              : 		cur_offset = 16 + 8*i;
              : 		cpu_cl_disc_tab.buffers[i].data = ((u64)read32((u32 *)(disc_tab_addr +
              : 						cur_offset)) + ((u64)read32((u32 *)
              : 						(disc_tab_addr + cur_offset + 4)) << 32));
This code makes little sense. Why is it using arrays of 64-bit values to store stuff that gets read as 32-bit values?

```suggestion
    for (int i = 0; i < cpu_cl_disc_tab.header.fields.count; i++) {
    	const uintptr_t cur_addr = (uintptr_t)disc_tab_addr + 16 + 8 * i;
    	cpu_cl_disc_tab.buffers[i].data = (u64)read32p(cur_addr) | (u64)read32p(cur_addr + 4) << 32);
```

https://review.coreboot.org/c/coreboot/+/82136/comment/4635d739_eaed2bd2?usp... :
PS10, Line 248: 	if (cpu_cl_devsc_cap.discovery_data.fields.t_bir_q == TEL_DVSEC_TBIR_BAR0) {
              : 		pci_write_config32(SA_DEV_TMT, PCI_BASE_ADDRESS_0, cpu_bar_addr);
              : 	} else if (cpu_cl_devsc_cap.discovery_data.fields.t_bir_q == TEL_DVSEC_TBIR_BAR1) {
              : 		pci_write_config32(SA_DEV_TMT, PCI_BASE_ADDRESS_1, cpu_bar_addr);
              : 	} else {
              : 		printk(BIOS_DEBUG, "invalid discovery data t_bir_q: 0x%x\n",
              : 			cpu_cl_devsc_cap.discovery_data.fields.t_bir_q);
              : 		return false;
              : 	}
> The intention here is to write the temp address (cpu_bar_addr) in both PCI_BASE_ADDRESS_0 and PCI_BA […]

If you assign the same base address to both BARs, don't you end up with a resource conflict?