[coreboot-gerrit] [S] Change in coreboot[main]: arch/riscv/pmp: Add print macro

[Angel Pons (Code Review)]

Attention is currently required from: Arthur Heymans, Maximilian Brune, Philipp Hug, Ron Minnich.

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

Change subject: arch/riscv/pmp: Add print macro
......................................................................

Patch Set 2: Code-Review+1

(5 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/83056/comment/b97fc144_09119c45?usp... :
PS2, Line 7: arch/riscv/pmp: Add print macro
"Add print macro" is not very informative. How about:
> arch/riscv/pmp: Only print registers from one HART

Or:
> arch/riscv/pmp: Avoid race conditions in logging

File src/arch/riscv/pmp.c:

https://review.coreboot.org/c/coreboot/+/83056/comment/24209667_7f771577?usp... :
PS2, Line 14: #define PMP_PRINTK(level, fmt, ...) { \
            : 	if (read_csr(mhartid) == CONFIG_RISCV_WORKING_HARTID) \
            : 		printk(level, fmt, __VA_ARGS__); \
            : 	}
To avoid what https://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html describes, I would do the following:

```
#define PMP_PRINTK(level, fmt, ...) \
    if (read_csr(mhartid) == CONFIG_RISCV_WORKING_HARTID) \
    	printk(level, fmt, __VA_ARGS__)
```

Or:

```
#define PMP_PRINTK(level, fmt, ...) \
    do { \
    	if (read_csr(mhartid) == CONFIG_RISCV_WORKING_HARTID) \
    		printk(level, fmt, __VA_ARGS__); \
    } while (0)
```

https://review.coreboot.org/c/coreboot/+/83056/comment/4a16b879_4f806a55?usp... :
PS2, Line 179: 
             : 	switch (idx) {
             : 	case 0:
             : 		write_csr(pmpaddr0, val);
             : 		break;
             : 	case 1:
             : 		write_csr(pmpaddr1, val);
             : 		break;
             : 	case 2:
             : 		write_csr(pmpaddr2, val);
             : 		break;
             : 	case 3:
             : 		write_csr(pmpaddr3, val);
             : 		break;
             : 	case 4:
             : 		write_csr(pmpaddr4, val);
             : 		break;
             : 	case 5:
             : 		write_csr(pmpaddr5, val);
             : 		break;
             : 	case 6:
             : 		write_csr(pmpaddr6, val);
             : 		break;
             : 	case 7:
             : 		write_csr(pmpaddr7, val);
             : 		break;
             : 	case 8:
             : 		write_csr(pmpaddr8, val);
             : 		break;
             : 	case 9:
             : 		write_csr(pmpaddr9, val);
             : 		break;
             : 	case 10:
             : 		write_csr(pmpaddr10, val);
             : 		break;
             : 	case 11:
             : 		write_csr(pmpaddr11, val);
             : 		break;
             : 	case 12:
             : 		write_csr(pmpaddr12, val);
             : 		break;
             : 	case 13:
             : 		write_csr(pmpaddr13, val);
             : 		break;
             : 	case 14:
             : 		write_csr(pmpaddr14, val);
             : 		break;
             : 	case 15:
             : 		write_csr(pmpaddr15, val);
             : 		break;
             : 	}
Not for this patch, but... Why is `pmpaddr` not an array?????

https://review.coreboot.org/c/coreboot/+/83056/comment/c0eed743_3018d2f6?usp... :
PS2, Line 231: 		PMP_PRINTK(BIOS_INFO, "%s(%d, %lx) = %lx\n", __func__, idx, val, read_pmpaddr(idx));
nit: drop one tab

https://review.coreboot.org/c/coreboot/+/83056/comment/62fb233e_f8d0ddd1?usp... :
PS2, Line 234: if (read_csr(mhartid) == CONFIG_RISCV_WORKING_HARTID)
Seems redundant