[U-Boot] [PATCH 07/30] riscv: set -march and -mabi based on the Kconfig configuration

[Auer, Lukas]

Hi Bin,

On Mon, 2018-10-22 at 15:21 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Sat, Oct 20, 2018 at 6:09 AM Lukas Auer
> <lukas.auer at aisec.fraunhofer.de> wrote:
> > 
> > Use the new Kconfig entries to construct the ISA string for the
> > -march
> > compiler flag. The -mabi compiler flag is selected based on the
> > base
> > integer instruction set.
> > 
> > With this change, the C (compressed instructions) ISA extension is
> > now
> > enabled for all boards with CONFIG_RISCV_ISA_C set. Buildman
> > reports a
> > decrease in binary size of 71590 bytes.
> > 
> > Signed-off-by: Lukas Auer <lukas.auer at aisec.fraunhofer.de>
> > ---
> > 
> >  arch/riscv/Makefile  | 13 +++++++++++++
> >  arch/riscv/config.mk |  4 ----
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 8fb6a889d8..6fb292d0b4 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -3,6 +3,19 @@
> >  # Copyright (C) 2017 Andes Technology Corporation.
> >  # Rick Chen, Andes Technology Corporation <rick at andestech.com>
> > 
> > +riscv-march-$(CONFIG_ARCH_RV32I)       := rv32im
> > +riscv-march-$(CONFIG_ARCH_RV64I)       := rv64im
> > +riscv-march-$(CONFIG_RISCV_ISA_A)      := $(riscv-march-y)a
> > +riscv-march-$(CONFIG_RISCV_ISA_C)      := $(riscv-march-y)c
> > +
> > +riscv-mabi-$(CONFIG_ARCH_RV64I)        := lp64
> > +riscv-mabi-$(CONFIG_ARCH_RV32I)        := ilp32
> > +
> > +arch-y := -march=$(riscv-march-y) -mabi=$(riscv-mabi-y)
> > +
> > +PLATFORM_CPPFLAGS      += $(arch-y)
> > +CFLAGS_EFI             += $(arch-y)
> > +
> 
> The concept of this patch is good. However the usage of := is a bit
> odd, since it makes people think the latter will override the former
> one, however it is not.
> 
> Can we get rid of these riscv-mach-xxx, instead using something like
> this:
> 
> ifeq ($(CONFIG_RISCV_ISA_A),y)
>         ARCH_A = a
> endif
> ifeq ($(CONFIG_RISCV_ISA_C),y)
>         ARCH_C = c
> endif
> 
> ifeq ($(CONFIG_ARCH_RV32I),y)
>         BITS = 32
>         ABI_I = i
> endif
> ifeq ($(CONFIG_ARCH_RV64I),y)
>         BITS = 64
> endif
> 
> PLATFORM_CPPFLAGS += -march=rv$(BITS)im$(ARCH_A)$(ARCH_C)
> -mabi=$(ABI_I)lp$(BITS)
> 

That makes sense. Yes I can change it to something like that. 
One small change I would do is to try and keep any constant ISA / ABI
strings (so rv and im) out of PLATFORM_CPPFLAGS to keep the
configuration more in one place. So something like this. What do you
think?

ifeq ($(CONFIG_ARCH_RV32I),y)
        ARCH_BASE = rv32im
        ABI = ilp32
endif
ifeq ($(CONFIG_ARCH_RV64I),y)
        ARCH_BASE = rv64im
        ABI = lp64
endif

PLATFORM_CPPFLAGS += -march=$(ARCH_BASE)$(ARCH_A)$(ARCH_C)
-mabi=$(ABI)

Thanks,
Lukas

> >  head-y := arch/riscv/cpu/start.o
> > 
> >  libs-y += arch/riscv/cpu/
> > diff --git a/arch/riscv/config.mk b/arch/riscv/config.mk
> > index ed9eb0c24c..9088b9ef2c 100644
> > --- a/arch/riscv/config.mk
> > +++ b/arch/riscv/config.mk
> > @@ -14,16 +14,12 @@
> >  64bit-emul             := elf64lriscv
> > 
> >  ifdef CONFIG_32BIT
> > -PLATFORM_CPPFLAGS      += -march=rv32ima -mabi=ilp32
> >  PLATFORM_LDFLAGS       += -m $(32bit-emul)
> > -CFLAGS_EFI             += -march=rv32ima -mabi=ilp32
> >  EFI_LDS                        := elf_riscv32_efi.lds
> >  endif
> > 
> >  ifdef CONFIG_64BIT
> > -PLATFORM_CPPFLAGS      += -march=rv64ima -mabi=lp64
> >  PLATFORM_LDFLAGS       += -m $(64bit-emul)
> > -CFLAGS_EFI             += -march=rv64ima -mabi=lp64
> >  EFI_LDS                        := elf_riscv64_efi.lds
> >  endif
> > 
> > --
> 
> Regards,
> Bin