ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30348 )
Change subject: riscv: create Kconfig architecture features for new parts ......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/#/c/30348/1/3rdparty/blobs File 3rdparty/blobs:
https://review.coreboot.org/#/c/30348/1/3rdparty/blobs@1 PS1, Line 1: Subproject commit dd00ad1260ef1dc0ba8c55c06ab10c7639dc3eb1
This slipped in by accident, I guess
fixed.
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Kconfig File src/arch/riscv/Kconfig:
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Kconfig@14 PS1, Line 14: config ARCH_RISCV_M
Please add descriptions to make it clear what these options mean, either as comments or with "help" […]
Done
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Kconfig@15 PS1, Line 15: bool
ARCH_RISCV_M seems unnecessary, because M-mode is mandated (RISC-V Priv. Arch. spec. 1. […]
There is a SOC in the works that won't have M.
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Kconfig@22 PS1, Line 22: config ARCH_RISCV_H
H-mode has been dropped, it's gone in the Privileged Architecture spec 1.10.
Done
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Kconfig@26 PS1, Line 26: config ARCH_RISCV_U
I'm not sure about the purpose of ARCH_RISCV_S and ARCH_RISCV_U. […]
They are required for an SOC I will be pushing but in general for, e.g., rv32 that don't have those modes. Leaving them out was a mistake I made at the start.
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Kconfig@30 PS1, Line 30: config ARCH_RISCV_RV64 That's seeming less likely all the time but we'll see.
https://review.coreboot.org/#/c/30348/2/src/arch/riscv/Kconfig File src/arch/riscv/Kconfig:
https://review.coreboot.org/#/c/30348/2/src/arch/riscv/Kconfig@14 PS2, Line 14: config ARCH_RISCV_M : # Whether a SOC implements M mode. : # M mode is the most privileged mode, it is : # the equivalent in some ways of x86 SMM mode : # save that in M mode it is impossible to turn : # on paging. : # While the spec requires it, there is at least : # one implementation that will not have it due : # to security concerns. : bool : default y
Maybe add another config option to allow this to be disabled via a select: […]
excellent idea.
https://review.coreboot.org/#/c/30348/2/src/arch/riscv/Kconfig@40 PS2, Line 40: config ARCH_RISCV_RV32 : bool : default n
maybe default this to y unless 64-bit is selected?
let's not. I don't want people to pick it by accident
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Makefile.inc File src/arch/riscv/Makefile.inc:
https://review.coreboot.org/#/c/30348/1/src/arch/riscv/Makefile.inc@40 PS1, Line 40: $(error "You need to select ARCH_RISCV_RV64 or ARCH_RISCV_RV32")
You're in the if branch for clang analyzer, here. […]
i can't figure out this makefile. But I'll give it a try.