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