Re: [PATCH 03/19] LoongArch: Add build infrastructure

[Huacai Chen <chenhuacai@gmail.com>]

Hi, Arnd,

On Tue, Jul 6, 2021 at 6:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
>  Tue, Jul 6, 2021 at 6:18 AM Huacai Chen <chenhuacai@loongson.cn> wrote:
>
> > --- /dev/null
> > +++ b/arch/loongarch/Kbuild
> > @@ -0,0 +1,21 @@
> > +# Fail on warnings - also for files referenced in subdirs
> > +# -Werror can be disabled for specific files using:
> > +# CFLAGS_<file.o> := -Wno-error
> > +ifeq ($(W),)
> > +subdir-ccflags-y := -Werror
> > +endif
>
> This tends to break building with 'make W=12' or similar, I would recommend not
> adding -Werror. It is a good idea though to have your CI systems build
> with -Werror
> enabled on the command line though.
If we use W=???, corresponding flags will be used; if we don't use
W=???, -Werrer flag is used here. So it seems not break the building.

>
> > +# platform specific definitions
> > +include arch/loongarch/Kbuild.platforms
> > +obj-y := $(platform-y)
> > +
>
> I would recommend not planning on having platform specific files. The way
> we do it on modern platforms (arm64, rv64) is to have all the device drivers
> in the driver subsystems instead.
>
> > +       select GENERIC_IOMAP
>
> GENERIC_IOMAP is one of those you probably don't want here. If PCI I/O
> space is mapped into MMIO registers, just do what arm64 has and make
> ioport_map() return a pointer, then make ioread32() an alias for readl().
OK, I will try.

>
> > +       select HAVE_CBPF_JIT if !64BIT
> > +       select HAVE_EBPF_JIT if 64BIT
>
> These look like you incorrectly copied them from MIPS. I don't see any EBPF_JIT
> implemementation (I may have missed that, as I'm still starting), but
> you probably
> don't want the CBPF version for 32-bit anyway.
OK, they should be removed at this time.

>
> > +       select HAVE_IDE
>
> drivers/ide/ was just removed,so this line can go as well.
>
> > +       select VIRT_TO_BUS
>
> Remove this: if you still require a driver that fails to use the dma-mapping
> API, fix the driver instead so it can work with IOMMUs.
OK, they will be removed in the next version.

>
> > +menu "Machine selection"
> > +
> > +choice
> > +       prompt "System type"
> > +       default MACH_LOONGSON64
> > +
> > +config MACH_LOONGSON64
> > +       bool "Loongson 64-bit family of machines"
>
> There should generally not be a 'choice' menu to select between
> different machine types, but instead the kernel should be able
> to run on all platforms that have the same instruction set.
>
> Maybe just make this a silent option and use 'select
> MACH_LOONGSON64' so you can pick up the device drivers
> that mips uses.
OK, thanks.

>
> > +       select ARCH_MIGHT_HAVE_PC_PARPORT
> > +       select ARCH_MIGHT_HAVE_PC_SERIO
> > +       select GENERIC_ISA_DMA
> > +       select ISA
>
> Do you actually have ISA-style add-on cards? If not, then
> remove the ISA options. If you do, which drivers are they?
Yes, there is an LPC controller in LS7A, and superio is connected on
it for laptops.

>
> > +config CPU_LOONGSON64
> > +       bool "Loongson 64-bit CPU"
> > +       depends on SYS_HAS_CPU_LOONGSON64
> > +       select CPU_SUPPORTS_64BIT_KERNEL
> > +       select GPIOLIB
> > +       select SWIOTLB
> > +       select ARCH_SUPPORTS_ATOMIC_RMW
> > +       help
> > +         The Loongson 64-bit processor implements the LoongArch64 (the 64-bit
> > +         version of LoongArch) instruction set.
> > +
> > +endchoice
> > +
> > +config SYS_HAS_CPU_LOONGSON64
> > +       bool
> > +
> > +endmenu
> > +
> > +config SYS_SUPPORTS_32BIT_KERNEL
> > +       bool
> > +config SYS_SUPPORTS_64BIT_KERNEL
> > +       bool
> > +config CPU_SUPPORTS_32BIT_KERNEL
> > +       bool
> > +config CPU_SUPPORTS_64BIT_KERNEL
> > +       bool
>
> Same for the CPUs: I would suggest you keep this simple until you get
> to the point of actually having multiple CPUs that you need to distinguish.
>
> Different 64-bit CPUs are hopefully going to be compatible with one
> another, so they should not be listed as mutually exclusive in a
> 'choice' statement. The complexity with two levels of 32/64 support
> is probably not going to be helpful here either.
>
> The 'select' statements that you have under CPU_LOONGSON64
> ook like they should just be in the top level.
>
> GPIOLIB and SWIOTLB could just be left user selectable, if turning
> them off results only in run-time loss of functionality but not a
> build failure.
>
> > +menu "Kernel type"
> > +
> > +choice
> > +       prompt "Kernel code model"
> > +       help
> > +         You should only select this option if you have a workload that
> > +         actually benefits from 64-bit processing or if your machine has
> > +         large memory.  You will only be presented a single option in this
> > +         menu if your system does not support both 32-bit and 64-bit kernels.
> > +
> > +config 32BIT
> > +       bool "32-bit kernel"
> > +       depends on CPU_SUPPORTS_32BIT_KERNEL && SYS_SUPPORTS_32BIT_KERNEL
> > +       help
> > +         Select this option if you want to build a 32-bit kernel.
> > +
> > +config 64BIT
> > +       bool "64-bit kernel"
> > +       depends on CPU_SUPPORTS_64BIT_KERNEL && SYS_SUPPORTS_64BIT_KERNEL
> > +       help
> > +         Select this option if you want to build a 64-bit kernel.
> > +
> > +endchoice
>
> Since you don't support any 32-bit target at the moment, just make CONFIG_64BIT
> a default-on statement, and make it user-selectable only when you add a 32-bit
> target.
OK.

Huacai
>
> > +choice
> > +       prompt "Kernel page size"
> > +       default PAGE_SIZE_16KB
> > +
> > +config PAGE_SIZE_4KB
> > +       bool "4kB"
> > +       help
> > +         This option select the standard 4kB Linux page size.
> > +
> > +config PAGE_SIZE_16KB
> > +       bool "16kB"
> > +       help
> > +         This option select the standard 16kB Linux page size.
> > +
> > +config PAGE_SIZE_64KB
> > +       bool "64kB"
> > +       help
> > +         This option select the standard 64kB Linux page size.
> > +
> > +endchoice
> > +
> > +choice
> > +       prompt "Virtual memory address space bits"
> > +       default VA_BITS_40
> > +       help
> > +         Allows choosing one of multiple possible virtual memory
> > +         address space bits for applications. The level of page
> > +         translation table is determined by a combination of page
> > +         size and virtual memory address space bits.
> > +
> > +config VA_BITS_40
> > +       bool "40-bits"
> > +       depends on 64BIT
> > +       help
> > +         Support a maximum at least 40 bits of application virtual memory.
> > +
> > +config VA_BITS_48
> > +       bool "48-bits"
> > +       depends on 64BIT
> > +       help
> > +         Support a maximum at least 48 bits of application virtual memory.
> > +
> > +endchoice
>
> Isn't the size of the address space tied to the page size?
>
>          Arnd