On Mon, May 22, 2017 at 9:49 PM, Palmer Dabbelt <pal...@dabbelt.com> wrote: > On Mon, 22 May 2017 18:27:21 PDT (-0700), o...@lixom.net wrote: >> Hi, >> >> >> On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt <pal...@dabbelt.com> wrote: >>> --- >>> arch/riscv/.gitignore | 35 ++++ >>> arch/riscv/Kconfig | 300 >>> +++++++++++++++++++++++++++++++++++ >>> arch/riscv/Makefile | 64 ++++++++ >>> arch/riscv/configs/riscv32_spike | 47 ++++++ >>> arch/riscv/configs/riscv64_freedom-u | 52 ++++++ >>> arch/riscv/configs/riscv64_qemu | 64 ++++++++ >>> arch/riscv/configs/riscv64_spike | 45 ++++++ >>> 7 files changed, 607 insertions(+) >>> create mode 100644 arch/riscv/.gitignore >>> create mode 100644 arch/riscv/Kconfig >>> create mode 100644 arch/riscv/Makefile >>> create mode 100644 arch/riscv/configs/riscv32_spike >>> create mode 100644 arch/riscv/configs/riscv64_freedom-u >>> create mode 100644 arch/riscv/configs/riscv64_qemu >>> create mode 100644 arch/riscv/configs/riscv64_spike >> >> Nearly all other platforms have _defconfig in the config names. It >> might get a bit excessive to prepend riscv{32,64} to all of them >> though. Most other platforms have shortened it to, for example, >> spike_defconfig, spike64_defconfig, qemu_defconfig, >> freedom-u_defconfig. >> >> Not going to argue too much about the color of the shed here, but >> using the _defconfig naming is recommended. > > Works for me > <https://github.com/riscv/riscv-linux/commit/b1165397ba6cb54f23910537c4bf4c3488ef9aad> > > I'll squash all the CR comments into a v2. > >> >>> >>> diff --git a/arch/riscv/.gitignore b/arch/riscv/.gitignore >>> new file mode 100644 >>> index 000000000000..376d06eb5d52 >>> --- /dev/null >>> +++ b/arch/riscv/.gitignore >>> @@ -0,0 +1,35 @@ >>> +# Now un-ignore all files. >>> +!* >>> + >>> +# But then re-ignore the files listed in the Linux .gitignore >>> +# Normal rules >>> +# >>> +.* >>> +*.o >>> +*.o.* >>> +*.a >>> +*.s >>> +*.ko >>> +*.so >>> +*.so.dbg >>> +*.mod.c >>> +*.i >>> +*.lst >>> +*.symtypes >>> +*.order >>> +modules.builtin >>> +*.elf >>> +*.bin >>> +*.gz >>> +*.bz2 >>> +*.lzma >>> +*.xz >>> +*.lzo >>> +*.patch >>> +*.gcno >> >> I don't think you need to do any of this, just inherit the global one >> (by not having one here)? > > Sorry, that's a holdover from how we used to manage our out-of-tree port and > can just be deleted. > > > https://github.com/riscv/riscv-linux/commit/68032fb592297331a2f2caf246968da7b70373fe > >>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig >>> new file mode 100644 >>> index 000000000000..510ead1d3343 >>> --- /dev/null >>> +++ b/arch/riscv/Kconfig >>> +config CPU_RV_GENERIC >>> + bool "Generic RISC-V" >>> + select CPU_SUPPORTS_32BIT_KERNEL >>> + select CPU_SUPPORTS_64BIT_KERNEL >> >> Is this even needed at this point? If the only CPU you can pick >> supports this, you might as well not make it an option (CPU_RV_GENERIC >> that is), and just make CPU_SUPPORTS_{32,64}BIT_KERNEL 'def_bool y' >> for now. > > I think this is actually broken in the opposite direction: we only support > 32-bit kernels on RV32 and 64-bit kernels on RV64, not both at the same time. > I don't really think it makes sense to build a kernel that supports both > 32-bit > and 64-bit on RISC-V (as they're different base ISAs), so I think it'd be > better to just have a "base ISA" configuration. > > > https://github.com/riscv/riscv-linux/commit/695d428d65bf6fe1382d34393e9d07f40b74e1b2 > > We'll think on this a bit more and get something saner.
Sure, sounds good overall though. >>> +config SBI_CONSOLE >>> + tristate "SBI console support" >>> + select TTY >>> + default y >> >> Usually you end up having a DRIVER_FOO and DRIVER_FOO_CONSOLE option >> to enable registering it as console. >> >> Also, unless there's strong reason to keep it under arch/, it should >> probably go under drivers/tty/. > > In this case "SBI" is the "Supervisor Binary Interface". This is a set of > routines that are provided by the platform for OS use that do things like > writing to the boot console or TLB shootdowns. The SBI is part of the RISC-V > ISA, so there isn't a config option for turning it off. SBI_CONSOLE > enables/disables the SBI's console support, so I think this option is sane. > > It's in arch/riscv because the SBI is part of the RISC-V ISA -- essentially > there's special SBI instructions that mean "write some register to the > console" > (there's some implementation tricks behind this, so it's really just a > specification). That said, I'm fine moving this to drivers. The same is true for some other drivers. Actually, I wonder if it might be just as easy to implement a sbi backend for hvc -- see hvc_udbg.c for an example where, on power, you have a simple get/put char hypervisor call in a very similar manner. Either way (keeping discrete sbi driver or implementing hvc backend), moving to drivers/tty is the right thing here -- we've worked hard on ARM to get rid of random drivers under arch/ and it'd be nice to not see new ones intoduced here. > >>> +config RVC >>> + bool "Use compressed instructions (RV32C or RV64C)" >>> + default n >> >> What does "use" here mean? Use during build, or allow userspace to use them? >> >>> + >>> +config RV_ATOMIC >>> + bool "Use atomic memory instructions (RV32A or RV64A)" >>> + default y >> >> Same for this. > > These mean "tell the compiler that it can emit these instructions when > building > Linux". Userspace applications can still use these instructions either way. > How does "Emit compressed instructions when building Linux" sound? > > > https://github.com/riscv/riscv-linux/commit/d6e65bd8b7dcfa72578d62e5eb367f680b55f5a8 Sounds good, you can still reference the ISA extensions if you want though. >> >>> +config RV_SYSRISCV_ATOMIC >>> + bool "Include support for atomic operation syscalls" >>> + default n >>> + help >>> + If atomic memory instructions are present, i.e., >>> + CONFIG_RV_ATOMIC, this includes support for the syscall that >>> + provides atomic accesses. This is only useful to run >>> + binaries that require atomic access but were compiled with >>> + -mno-atomic. >>> + >>> + If CONFIG_RV_ATOMIC is unset, this option is mandatory. >> >> If it's mandatory then Kconfig language should make it so. > > I'm not sure what you mean by this. We have > > config RISCV > ... > select RV_SYSRISCV_ATOMIC if !RV_ATOMIC > > Should this constraint just live within "config RV_SYSRISCV_ATOMIC"? It seems > cleaner to have the constraints next to the config definitions. Ah, I just missed the select. -Olof