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

[Subrata Banik (Code Review)]

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

Subrata Banik 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/1b98d7da_01dfb0a7?usp... :
PS10, Line 205: &
> Good point. The code says "if `dw1` and `dw0` don't have any one-bits in common", which is rather odd.
>
> As per the surrounding code, looks like this is a 64-bit register (a memory address?), and a zero address would be invalid. If this is correct, then the check should be:
>    if (!dw1 && !dw0) {
>
> This would be equivalent, but I feel it's harder to understand so I prefer the former:
>    if (!(dw1 | dw0)) {

If the idea is to check the zero based value, then please be explicit and don't use bit wise operator.

```
if (!dw1 && !dw0) {
```

https://review.coreboot.org/c/coreboot/+/82136/comment/825b6d82_01ccc0c1?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 being assigned to any other resource ?

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

https://review.coreboot.org/c/coreboot/+/82136/comment/3f554cee_7f447cd5?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 ?
```
uint8_t t_bir_q = cpu_cl_devsc_cap.discovery_data.fields.t_bir_q;

const uint8_t BAR_OFFSET = PCI_BASE_ADDRESS_0;

if (t_bir_q >= TEL_DVSEC_TBIR_BAR0 && t_bir_q <= TEL_DVSEC_TBIR_BAR1) {
    uint8_t bar_index = t_bir_q - TEL_DVSEC_TBIR_BAR0;

pci_write_config32(SA_DEV_TMT, BAR_OFFSET + bar_index, cpu_bar_addr);
} else {
    printk(BIOS_DEBUG, "Invalid TEL_DVSEC_TBIR_BARx: %d\n", t_bir_q); 
}

```