[coreboot-gerrit] [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:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/82136/comment/58e25fdc_260d725b?usp... :
PS10, Line 205: &
> > Good point. […]

ACK , using logical operator is the correct thing 
To check if either is 0
1) if (dw1 == 0 || dw2 == 0) or
2)  if (!(dw0 && dw1)) 
IMO ,2 is optimized and hence preferred - please suggest
Also will change the print message from debug to error

https://review.coreboot.org/c/coreboot/+/82136/comment/3b58e179_ea8f4620?usp... :
PS10, Line 245: CPU_TRACE_HUB_BASE_ADDRESS
> wondering why we are using temp base address here ? and how do we know if this temp access is not be […]

We are using temp address because of the Si issue in ADL discussed in Partner isue 
To make sure fixed addresses does not conflict with the temp address - I have referred to FAS 626540 - section 6.5 (as mentioned in iomap.h) 
For dynamic allocation I checked PCIe device allocation in the serial log .
Any workaround that needs to be done to overcome this issue should be in ADL/RPL only and not in the common code - this was the intention

https://review.coreboot.org/c/coreboot/+/82136/comment/8ca51c8a_fa899741?usp... :
PS10, Line 246: 	printk(BIOS_DEBUG, "cpu_bar_addr for Crashlog device : 0x%X\n", cpu_bar_addr);
> this is hardcoded value so why should we print the additional debug msg here ?

ACK - just thought it will helpful for debugging - will remove

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

The intention here is to write the temp address (cpu_bar_addr) in both PCI_BASE_ADDRESS_0 and PCI_BASE ADDRESS_1 so that when it is read back in cl_get_cpu_bar_addr function it reads the hardcoded address