Date:   Mon, 22 May 2017 18:27:21 -0700
From:   Olof Johansson <olof@...om.net>
To:     Palmer Dabbelt <palmer@...belt.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>, albert@...ive.com
Subject: Re: [PATCH 2/7] RISC-V: arch/riscv Makefile and Kconfigs

Hi,


On Mon, May 22, 2017 at 5:41 PM, Palmer Dabbelt <palmer@...belt.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.

>
> 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)?

> +
> +include/generated

This is already covered by the global .gitignore.

> +kernel/vmlinux.lds

And if needed this should be added to the arch/riscv/kernel/.gitignore
file instead.


> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> new file mode 100644
> index 000000000000..510ead1d3343
> --- /dev/null
> +++ b/arch/riscv/Kconfig
> @@ -0,0 +1,300 @@
> +#
> +# For a description of the syntax of this configuration file,
> +# see Documentation/kbuild/kconfig-language.txt.
> +#
> +
> +config RISCV
> +       def_bool y
> +       select OF
> +       select OF_EARLY_FLATTREE
> +       select OF_IRQ
> +       select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
> +       select ARCH_WANT_FRAME_POINTERS
> +       select CLONE_BACKWARDS
> +       select COMMON_CLK
> +       select GENERIC_CLOCKEVENTS
> +       select GENERIC_CPU_DEVICES
> +       select GENERIC_IRQ_SHOW
> +       select GENERIC_PCI_IOMAP
> +       select GENERIC_STRNCPY_FROM_USER
> +       select GENERIC_STRNLEN_USER
> +       select GENERIC_SMP_IDLE_THREAD
> +       select GENERIC_ATOMIC64 if !64BIT || !RV_ATOMIC
> +       select ARCH_WANT_OPTIONAL_GPIOLIB
> +       select HAVE_MEMBLOCK
> +       select HAVE_DMA_API_DEBUG
> +       select HAVE_DMA_CONTIGUOUS
> +       select HAVE_GENERIC_DMA_COHERENT
> +       select IRQ_DOMAIN
> +       select NO_BOOTMEM
> +       select RV_ATOMIC if SMP
> +       select RV_SYSRISCV_ATOMIC if !RV_ATOMIC
> +       select SPARSE_IRQ
> +       select SYSCTL_EXCEPTION_TRACE
> +       select HAVE_ARCH_TRACEHOOK
> +       select MODULES_USE_ELF_RELA if MODULES
> +
> +config MMU
> +       def_bool y
> +
> +# even on 32-bit, physical (and DMA) addresses are > 32-bits
> +config ARCH_PHYS_ADDR_T_64BIT
> +       def_bool y
> +
> +config ARCH_DMA_ADDR_T_64BIT
> +       def_bool y
> +
> +config STACKTRACE_SUPPORT
> +       def_bool y
> +
> +config RWSEM_GENERIC_SPINLOCK
> +       def_bool y
> +
> +config GENERIC_BUG
> +       def_bool y
> +       depends on BUG
> +       select GENERIC_BUG_RELATIVE_POINTERS if 64BIT
> +
> +config GENERIC_BUG_RELATIVE_POINTERS
> +       bool
> +
> +config GENERIC_CALIBRATE_DELAY
> +       def_bool y
> +
> +config GENERIC_CSUM
> +       def_bool y
> +
> +config GENERIC_HWEIGHT
> +       def_bool y
> +
> +config PGTABLE_LEVELS
> +       int
> +       default 3 if 64BIT
> +       default 2
> +
> +config HAVE_KPROBES
> +       def_bool n
> +
> +config DMA_NOOP_OPS
> +       def_bool y
> +
> +menu "Platform type"
> +
> +config SMP
> +       bool "Symmetric Multi-Processing"
> +       help
> +         This enables support for systems with more than one CPU.  If
> +         you say N here, the kernel will run on single and
> +         multiprocessor machines, but will use only one CPU of a
> +         multiprocessor machine. If you say Y here, the kernel will run
> +         on many, but not all, single processor machines. On a single
> +         processor machine, the kernel will run faster if you say N
> +         here.
> +
> +         If you don't know what to do here, say N.
> +
> +config NR_CPUS
> +       int "Maximum number of CPUs (2-32)"
> +       range 2 32
> +       depends on SMP
> +       default "8"
> +
> +choice
> +       prompt "CPU selection"
> +       default CPU_RV_GENERIC
> +
> +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.

> +
> +endchoice
> +
> +config PLIC
> +       bool "Platform-Level Interrupt Controller"
> +       default y
> +       help
> +          This enables support for the PLIC chip found in standard RISC-V
> +          systems. The PLIC is the top-most interrupt controller found in
> +          the system, connected directly to the core complex. All other
> +          interrupt sources (MSI, GPIO, etc) are subordinate to the PLIC.
> +
> +          If you don't know what to do here, say Y.
> +
> +config CPU_SUPPORTS_32BIT_KERNEL
> +       bool
> +config CPU_SUPPORTS_64BIT_KERNEL
> +       bool
> +
> +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/.

> +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.

> +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.

> +config RV_PUM
> +       def_bool y
> +       prompt "Protect User Memory" if EXPERT
> +       ---help---
> +         Protect User Memory (PUM) prevents the kernel from inadvertently
> +         accessing user-space memory.  There is a small performance cost
> +         and kernel size increase if this is enabled.
> +
> +         If unsure, say Y.
> +
> +endmenu
> +
> +menu "Kernel type"
> +
> +choice
> +       prompt "Kernel code model"
> +       default 64BIT
> +
> +config 32BIT
> +       bool "32-bit kernel"
> +       depends on CPU_SUPPORTS_32BIT_KERNEL
> +       help
> +         Select this option to build a 32-bit kernel.
> +
> +config 64BIT
> +       bool "64-bit kernel"
> +       depends on CPU_SUPPORTS_64BIT_KERNEL
> +       help
> +         Select this option to build a 64-bit kernel.
> +
> +endchoice
> +
[...]


-Olof

Powered by blists - more mailing lists