Re: [PATCH 2/7] RISC-V: arch/riscv Makefile and Kconfigs

[Arnd Bergmann <arnd@arndb.de>]

On Tue, May 23, 2017 at 2:41 AM, Palmer Dabbelt <palmer@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
>
> 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

This doesn't seem to belong here: There is no reason for riscv
to be different from all other architectures. Is something wrong
with the top-level .gitignore? If so, we should just fix it there.

> 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

You normally don't want GENERIC_PCI_IOMAP, unless your
inb()/outb() uses other instructions than your readl()/writel()

> +config MMU
> +       def_bool y

Just a general question: has there been any interest in a no-MMU
version?

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

Are you required to use 64-bit addressing for RAM on 32-bit
architectures though? Using 32-bit dma_addr_t and phys_addr_t
when possible makes some code noticeably more efficient.

> +config PGTABLE_LEVELS
> +       int
> +       default 3 if 64BIT
> +       default 2

With 2-level page tables, you usually can't address much more
than 32-bit physical memory anyway, so I'd guess that most
32-bit chips would actually put their RAM under the 4GB boundary.

> +config RV_ATOMIC
> +       bool "Use atomic memory instructions (RV32A or RV64A)"
> +       default y
> +
> +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.

Just express this in Kconfig terms to prevent misconfiguration:

config RV_SYSRISCV_ATOMIC
       bool "Include support for atomic operation syscalls" if RV_ATOMIC
       default !RV_ATOMIC

I wonder what the cost would be of always providing the syscalls
for compatibility. This is also something worth putting into a VDSO
instead of exposing the syscall:

That way, user space that is built with -mno-atomic can call into
the vdso, which depending on the hardware support will perform
the atomic operation directly or enter the syscall.

> +config PCI_DOMAINS
> +       def_bool PCI
> +
> +config PCI_DOMAINS_GENERIC
> +       def_bool PCI
> +
> +config PCI_SYSCALL
> +       def_bool PCI

I don't think you want PCI_SYSCALL

        Arnd