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

[Sowmya Aralguppe (Code Review)]

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

Sowmya Aralguppe 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/a6e67526_6c7c14c4?usp... :
PS10, Line 205: &
> Ah, it's two separate registers... […]

ACK . I will change to meaningful names as suggested. This check was introduced so that it fails early rather than later and both the registers should have non zero values  for crashlog to function properly. yes cl_header and cl_cap_status is packed into 64 bits - cpu_crashlog_header_t in crashlog.h

https://review.coreboot.org/c/coreboot/+/82136/comment/f7bb0af5_fa563b16?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. […]

Here the loop is for number of child crash logs that are connected to the record.
For each child there is a base offset and buffer size .These are 48 bits and hence gets assigned to 64bit values.

Bits	Access	Default	Description
31:0	RW	0	
DATA_BUFFER_ADDRESS

Base address within this BAR for the crashlog data buffer.
47:32	RW	0x700	
DATA_BUFFER_SIZE

https://review.coreboot.org/c/coreboot/+/82136/comment/d6fb65b0_25ede08f?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;
              : 	}
> If you assign the same base address to both BARs, don't you end up with a resource conflict?

Designated Vendor-Specific Extended Capability (DVSEC) specifications for crashlog specifies 98:96 bits as tbir .When tbir is 0 the bar address PM_BAR is read from 0x10 (PCI_BASE_ADDRESS_0) and if its 1 ,it should be read from 0x14 (PCI_BASE_ADDRESS_1).For RPL the PM_BAR is 0x10 by default but to keep it aligned to common code it is good to read tbir and then read from either PCI_BASE_ADDRESS_0 or PCI_BASE_ADDRESS_1.So we write the temporary hard coded bar address to both these offsets.
so to summarize either one of the BAR address is read based on capability reg so there wont be any resource conflict.