[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180320131342.GA31542@guoren>
Date: Tue, 20 Mar 2018 21:13:46 +0800
From: Guo Ren <ren_guo@...ky.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: linux-arch <linux-arch@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Jason Cooper <jason@...edaemon.net>,
c-sky_gcc_upstream@...ky.com, gnu-csky@...tor.com,
thomas.petazzoni@...tlin.com, wbx@...ibc-ng.org
Subject: Re: [PATCH 15/19] csky: Build infrastructure
Hi arnd,
On Mon, Mar 19, 2018 at 11:45:23PM +0800, Arnd Bergmann wrote:
> > + select ARCH_WANT_IPC_PARSE_VERSION
>
> Drop ipc_parse_version here, it's only for old architectures.
>
I will remove it.
> > + select HAVE_OPROFILE
> > + select HAVE_PERF_EVENTS
>
> Do you still need oprofile when you have perf?
I'll remove HAVE_OPROFILE and arch/csky/oprofile.
> > + select MODULES_USE_ELF_REL if MODULES
> > + select MODULES_USE_ELF_RELA if MODULES
>
> You should only need one of these two.
Yes, I will keep RELA and current csky-gcc only give .rela section.
> > + select OLD_SIGACTION
> > + select OLD_SIGSUSPEND3
>
> These should not be needed.
Ok, remove them.
> > +config CPU_HAS_TLBI
> > + bool
> > + default n
>
> 'default n' is redundant and can be dropped.
Ok
> > +config GENERIC_CALIBRATE_DELAY
> > + bool
> > + default y
>
> Does your architecture provide a reliable high-reslution clocksource?
> If yes, you
> could use that for the delay, rather than a calibrated loop.
Currently, all boards have clocksource drivers and the reslution is depend on SOC.
I'll try to remove it.
> > + select HIGHMEM
> > + select CPU_HAS_TLBI
> > + select CPU_HAS_CACHEV2
> > +endchoice
>
> Why select 'HIGHMEM' based on the CPU type? You should only need it
> when you have more than 1GB of RAM, and it can be a performance
> problem when it's enabled without need.
OK, I'll give a HIGHMEM option.
> Usually the kernel should allow multiple CPU types to be selected
> together, or ask for a "minimum architecture" level to be selected
> by allow newer cores to be used as a superset.
No, I need keep them seperate.
> > +config CPU_TLB_SIZE
> > + int
> > + default "128" if(CPU_CK610 || CPU_CK807 || CPU_CK810)
> > + default "1024" if(CPU_CK860)
> > +
> > +config L1_CACHE_SHIFT
> > + int
> > + default "4" if(CPU_CK610)
> > + default "5" if(CPU_CK807 || CPU_CK810)
> > + default "6" if(CPU_CK860)
>
> I think you then need to reverse the order of the list here: When e.g. CK860
> and CK810 are both enabled, L1_CACHE_SHIFT should be the largest
> possible size.
No, I use L1_CACHE_SHIFT to determine the size of cache_line.
When I flush cache for a range of memory, I need the size to loop flush cache line.
> > +config SSEG0_BASE
> > + hex "Direct mapping physical address"
> > + default 0x0
> > + help
> > + There are MSAx regs can be used to change the base physical address
> > + of direct mapping. The default base physical address is 0x0.
> > +
> > +config RAM_BASE
> > + hex "DRAM base address offset from SSEG0_BASE, it must be the same with dts memory."
> > + default 0x08000000
>
> To allow one kernel to run on multiple boards, it's better to detect
> these two at runtime.
CK-CPUs have a mips-like direct-mapping, and I use the macros to calculate the virtual-addr
in headers.
> > +config CSKY_NR_IRQS
> > + int "NR_IRQS to max virtual interrupt numbers of the whole system"
> > + range 64 8192
> > + default "128"
> > +endmenu
>
> This should no longer be needed, with the IRQ domain code, any number
> of interrupts
> can be used without noticeable overhead.
Not I use it, some of our users need it to expand the GPIO irqs. Because
they don't use irq domain code properly. I move it to Kconfig.debug, OK?
> Make it either
>
> def_bool y
>
> or
>
> bool
> default y
OK
> > +config CSKY_BUILTIN_DTB
> > + bool "Use kernel builtin dtb"
> > + default n
> > +
> > +config CSKY_BUILTIN_DTB_NAME
> > + string "kernel builtin dtb name"
> > + depends on CSKY_BUILTIN_DTB
>
> It's generally better not to use a builtin dtb, but use the bootloader
> to pass a dtb.
>
> If you need to support existing bootloaders, the best way is to allow
> appending the dtb to the kernel.
Most of our boards use bootloader to pass the dtb, but Hangzhou
Nationalchip want dtb compiled in the vmlinux. So I keep it in
Kconfig.debug.
> > +ifeq ($(VERSION)_$(PATCHLEVEL), 4_9)
> > +COMPAT_KERNEL_4_9 = -DCOMPAT_KERNEL_4_9
> > +endif
>
> Should not be needed
May I keep it? It's a very internal macro for arch/csky and I can
maintain the linux-4.9 together.
> > +KBUILD_CFLAGS += -ffreestanding \
>
> -ffreestanding usually results in worse code and should not be needed
OK, remove it.
> > + -fno-tree-dse \
> > + -pipe \
> > + -Wno-uninitialized \
>
> For -Wno-uninitialized, better fix the bugs properly. Can you explain
> why you want
It will mask some warnings :P, and I will remove it.
> -fno-tree-dse?
This is from "gcc-4.5 compile linux-4.7" and it will cause wrong code without
-fno-tree-dse for list.h. Now we use gcc-6.3, so I will try to remove it.
> > +++ b/arch/csky/abiv1/Makefile
> > @@ -0,0 +1,8 @@
> > +obj-y += src/bswapdi.o
> > +obj-y += src/bswapsi.o
> > +obj-y += src/cacheflush.o
> > +obj-y += src/memcpy.o
> > +obj-y += src/mmap.o
> > +
> > +obj-$(CONFIG_CPU_NEED_SOFTALIGN) += src/alignment.o
>
> Better not use subdirectories like that.
Ok, I will change them like this:
obj-y += bswapdi.o
obj-y += bswapsi.o
...
> Can you explain why you need the alignement fixups?
For abiv1 ck610 couldn't handle the unalignment address access, so we
need soft-alignment exception to fixup. There is no problem in abiv2 cpus.
Best Regards
Guo Ren
Powered by blists - more mailing lists