mbox series

[v2,00/35] Andes(nds32) Linux Kernel Port

Message ID cover.1511785528.git.green.hu@gmail.com
Headers show
Series Andes(nds32) Linux Kernel Port | expand

Message

Greentime Hu Nov. 27, 2017, 12:27 p.m. UTC
This is the 2nd version patchset to add the Linux kernel port for Andes(nds32)
processors. Almost all of the feedback from v1 patchseries has been addressed.
Thanks to everyone who provided feedback on the previous version.


This patchset adds core architecture support to Linux for Andestech's
N13, N15, D15, N10, D10 processor cores.

Based on the 16/32-bit AndeStar RISC-like architecture, we designed the
configurable AndesCore series of embedded processor families. AndesCores
range from highly performance-efficient small-footprint cores for
microcontrollers and deeply-embedded applications to 1GHz+ cores running
Linux, covering general-purpose N-series cores for a wide range of computing
need, DSP-capable D-series cores for digital signal control,
instruction-extensible E-series cores for application-specific acceleration,
and secure S-series cores for best protection of the most valuable.

The patches are based on v4.14-rc8, and can also be found in the
following git tree:
  https://github.com/andestech/linux.git nds32-4.14-rc8-v2

The build script and toolchain repositories are able to be found here:
  https://github.com/andestech/build_script.git

Freely available instruction set and architecture overview documents can
be found on the following page:
  http://www.andestech.com/product.php?cls=9


Vincent Ren-Wei Chen and I will maintain this port. Thanks to everyone who
helped us and contributed to it. :) Any feedback is welcome.


Changes in v2:
 - Set GENERIC_CALIBRATE_DELAY default n
 - Add earlycon support
 - Remove earlyprintk
 - Add CPU_BIG_ENDIAN, CPU_LITTLE_ENDIAN support
 - Refine unalignment access exception handler
 - Add VMSPLIT support
 - Use only one defconfig
 - Change interrupt-cells from 2 to 1
 - Refine andestech cpu names in bindings/nds32/cpus.txt
 - Get clock frequency in dts because fpga bitmap doesn't include this feature
 - Update MAINTAINERS for bindings
 - Remove unused configs in Kconfig
 - Refine device tree scripts
 - Refine coding style
 - Use generic ioremap_nocache
 - Remove L2CC_PA_BASE define and its codes in head.S. It will be moved to bootloader.
 - Set PHYS_OFFSET to 0x0 instead of CONFIG_MEMORY_START
 - Remove unused macros
 - Simplify cpu_cache_* API
 - Change __asm__ __volatile__ to asm volatile
 - Refine uaccess.h
 - Remove unused/deprecated syscall
 - Use generic posix_types.h
 - Remove arch_trace_hardirqs_on/arch_trace_hardirqs_off
 - Fix bug of restart syscall
 - Refine syscall implementations
 - Use IS_ENABLED to replace ifdef as possible
 - Remove device_initcall(nds32_device_probe)
 - Refine vdso implementations
 - Refine copy_from_user()/copy_to_user()/clear_user()/get_user()/memmove()/memcpy()
 - Refine ioremap.c
 - Refine irq-ativic32.c
 - Fix a bug of earlycon.c
 - Export ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_wt
 - Add atcpit100 driver

Greentime Hu (32):
  nds32: Assembly macros and definitions
  nds32: Kernel booting and initialization
  nds32: Exception handling
  nds32: MMU definitions
  nds32: MMU initialization
  nds32: MMU fault handling and page table management
  nds32: Cache and TLB routines
  nds32: Process management
  nds32: IRQ handling
  nds32: Atomic operations
  nds32: Device specific operations
  nds32: DMA mapping API
  nds32: ELF definitions
  nds32: System calls handling
  nds32: VDSO support
  nds32: Signal handling support
  nds32: Library functions
  nds32: Debugging support
  nds32: L2 cache support
  nds32: Loadable modules
  nds32: Generic timers support
  nds32: Device tree support
  nds32: Miscellaneous header files
  nds32: defconfig
  nds32: Build infrastructure
  dt-bindings: interrupt-controller: Andestech Internal Vector
    Interrupt Controller
  irqchip: Andestech Internal Vector Interrupt Controller driver
  MAINTAINERS: Add nds32
  dt-bindings: nds32 CPU Bindings
  net: faraday add nds32 support.
  earlycon: add reg-offset to physical address before mapping
  asm-generic/io.h: move
    ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_wt out of     ifndef
    CONFIG_MMU

Rick Chen (3):
  clocksource/drivers/atcpit100: Add andestech atcpit100 timer
  clocksource/drivers/Kconfig: Support andestech atcpit100 timer
  dt-bindings: timer: Add andestech atcpit100 timer binding doc

 .../interrupt-controller/andestech,ativic32.txt    |   19 +
 Documentation/devicetree/bindings/nds32/cpus.txt   |   32 +
 .../bindings/timer/andestech,atcpit100-timer.txt   |   33 +
 MAINTAINERS                                        |   11 +
 arch/nds32/Kconfig                                 |  107 +++
 arch/nds32/Kconfig.cpu                             |  131 +++
 arch/nds32/Makefile                                |   66 ++
 arch/nds32/boot/Makefile                           |   15 +
 arch/nds32/boot/dts/Makefile                       |    8 +
 arch/nds32/boot/dts/ae3xx.dts                      |   55 ++
 arch/nds32/boot/dts/ag101p.dts                     |   60 ++
 arch/nds32/configs/defconfig                       |  111 +++
 arch/nds32/include/asm/Kbuild                      |   54 ++
 arch/nds32/include/asm/assembler.h                 |   52 ++
 arch/nds32/include/asm/bitfield.h                  |  974 ++++++++++++++++++++
 arch/nds32/include/asm/cache.h                     |   25 +
 arch/nds32/include/asm/cache_info.h                |   26 +
 arch/nds32/include/asm/cacheflush.h                |   57 ++
 arch/nds32/include/asm/current.h                   |   25 +
 arch/nds32/include/asm/delay.h                     |   51 +
 arch/nds32/include/asm/dma-mapping.h               |   27 +
 arch/nds32/include/asm/elf.h                       |  192 ++++
 arch/nds32/include/asm/fixmap.h                    |   42 +
 arch/nds32/include/asm/futex.h                     |  116 +++
 arch/nds32/include/asm/highmem.h                   |   78 ++
 arch/nds32/include/asm/io.h                        |   25 +
 arch/nds32/include/asm/irqflags.h                  |   49 +
 arch/nds32/include/asm/l2_cache.h                  |  155 ++++
 arch/nds32/include/asm/linkage.h                   |   24 +
 arch/nds32/include/asm/memory.h                    |  118 +++
 arch/nds32/include/asm/mmu.h                       |   25 +
 arch/nds32/include/asm/mmu_context.h               |   81 ++
 arch/nds32/include/asm/module.h                    |   24 +
 arch/nds32/include/asm/nds32.h                     |   96 ++
 arch/nds32/include/asm/page.h                      |   78 ++
 arch/nds32/include/asm/pgalloc.h                   |  109 +++
 arch/nds32/include/asm/pgtable.h                   |  426 +++++++++
 arch/nds32/include/asm/proc-fns.h                  |   57 ++
 arch/nds32/include/asm/processor.h                 |  116 +++
 arch/nds32/include/asm/ptrace.h                    |   79 ++
 arch/nds32/include/asm/shmparam.h                  |   32 +
 arch/nds32/include/asm/spinlock.h                  |  184 ++++
 arch/nds32/include/asm/string.h                    |   30 +
 arch/nds32/include/asm/swab.h                      |   48 +
 arch/nds32/include/asm/syscall.h                   |  203 ++++
 arch/nds32/include/asm/syscalls.h                  |   27 +
 arch/nds32/include/asm/thread_info.h               |   91 ++
 arch/nds32/include/asm/tlb.h                       |   41 +
 arch/nds32/include/asm/tlbflush.h                  |   60 ++
 arch/nds32/include/asm/uaccess.h                   |  296 ++++++
 arch/nds32/include/asm/unistd.h                    |   19 +
 arch/nds32/include/asm/vdso.h                      |   35 +
 arch/nds32/include/asm/vdso_datapage.h             |   51 +
 arch/nds32/include/uapi/asm/Kbuild                 |   27 +
 arch/nds32/include/uapi/asm/auxvec.h               |   25 +
 arch/nds32/include/uapi/asm/byteorder.h            |   26 +
 arch/nds32/include/uapi/asm/cachectl.h             |   19 +
 arch/nds32/include/uapi/asm/param.h                |   24 +
 arch/nds32/include/uapi/asm/ptrace.h               |   42 +
 arch/nds32/include/uapi/asm/sigcontext.h           |   73 ++
 arch/nds32/include/uapi/asm/signal.h               |   23 +
 arch/nds32/include/uapi/asm/unistd.h               |   32 +
 arch/nds32/kernel/Makefile                         |   23 +
 arch/nds32/kernel/asm-offsets.c                    |   41 +
 arch/nds32/kernel/atl2c.c                          |   77 ++
 arch/nds32/kernel/cacheinfo.c                      |   62 ++
 arch/nds32/kernel/devtree.c                        |   45 +
 arch/nds32/kernel/dma.c                            |  472 ++++++++++
 arch/nds32/kernel/ex-entry.S                       |  170 ++++
 arch/nds32/kernel/ex-exit.S                        |  206 +++++
 arch/nds32/kernel/ex-scall.S                       |  145 +++
 arch/nds32/kernel/head.S                           |  198 ++++
 arch/nds32/kernel/irq.c                            |   22 +
 arch/nds32/kernel/module.c                         |  299 ++++++
 arch/nds32/kernel/nds32_ksyms.c                    |   44 +
 arch/nds32/kernel/process.c                        |  217 +++++
 arch/nds32/kernel/ptrace.c                         |  325 +++++++
 arch/nds32/kernel/setup.c                          |  407 ++++++++
 arch/nds32/kernel/signal.c                         |  354 +++++++
 arch/nds32/kernel/stacktrace.c                     |   60 ++
 arch/nds32/kernel/sys_nds32.c                      |   74 ++
 arch/nds32/kernel/syscall_table.c                  |   28 +
 arch/nds32/kernel/time.c                           |   22 +
 arch/nds32/kernel/traps.c                          |  441 +++++++++
 arch/nds32/kernel/vdso.c                           |  245 +++++
 arch/nds32/kernel/vdso/Makefile                    |   82 ++
 arch/nds32/kernel/vdso/datapage.S                  |   34 +
 arch/nds32/kernel/vdso/gen_vdso_offsets.sh         |   15 +
 arch/nds32/kernel/vdso/gettimeofday.c              |  266 ++++++
 arch/nds32/kernel/vdso/note.S                      |   29 +
 arch/nds32/kernel/vdso/sigreturn.S                 |   33 +
 arch/nds32/kernel/vdso/vdso.S                      |   33 +
 arch/nds32/kernel/vdso/vdso.lds.S                  |   87 ++
 arch/nds32/kernel/vmlinux.lds.S                    |   70 ++
 arch/nds32/lib/Makefile                            |    3 +
 arch/nds32/lib/clear_user.S                        |   55 ++
 arch/nds32/lib/copy_from_user.S                    |   58 ++
 arch/nds32/lib/copy_page.S                         |   50 +
 arch/nds32/lib/copy_template.S                     |   83 ++
 arch/nds32/lib/copy_to_user.S                      |   58 ++
 arch/nds32/lib/memcpy.S                            |   43 +
 arch/nds32/lib/memmove.S                           |   83 ++
 arch/nds32/lib/memset.S                            |   46 +
 arch/nds32/lib/memzero.S                           |   31 +
 arch/nds32/mm/Makefile                             |    7 +
 arch/nds32/mm/alignment.c                          |  622 +++++++++++++
 arch/nds32/mm/cacheflush.c                         |  331 +++++++
 arch/nds32/mm/extable.c                            |   29 +
 arch/nds32/mm/fault.c                              |  420 +++++++++
 arch/nds32/mm/highmem.c                            |   92 ++
 arch/nds32/mm/init.c                               |  318 +++++++
 arch/nds32/mm/ioremap.c                            |   75 ++
 arch/nds32/mm/mm-nds32.c                           |  103 +++
 arch/nds32/mm/mmap.c                               |   86 ++
 arch/nds32/mm/proc.c                               |  601 ++++++++++++
 arch/nds32/mm/tlb.c                                |   63 ++
 drivers/clocksource/Kconfig                        |    6 +
 drivers/clocksource/Makefile                       |    1 +
 drivers/clocksource/timer-atcpit100.c              |  247 +++++
 drivers/irqchip/Makefile                           |    1 +
 drivers/irqchip/irq-ativic32.c                     |  119 +++
 drivers/net/ethernet/faraday/Kconfig               |    6 +-
 drivers/tty/serial/earlycon.c                      |    3 +-
 include/asm-generic/io.h                           |   18 +-
 124 files changed, 13508 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/andestech,ativic32.txt
 create mode 100644 Documentation/devicetree/bindings/nds32/cpus.txt
 create mode 100644 Documentation/devicetree/bindings/timer/andestech,atcpit100-timer.txt
 create mode 100644 arch/nds32/Kconfig
 create mode 100644 arch/nds32/Kconfig.cpu
 create mode 100644 arch/nds32/Makefile
 create mode 100644 arch/nds32/boot/Makefile
 create mode 100644 arch/nds32/boot/dts/Makefile
 create mode 100644 arch/nds32/boot/dts/ae3xx.dts
 create mode 100644 arch/nds32/boot/dts/ag101p.dts
 create mode 100644 arch/nds32/configs/defconfig
 create mode 100644 arch/nds32/include/asm/Kbuild
 create mode 100644 arch/nds32/include/asm/assembler.h
 create mode 100644 arch/nds32/include/asm/bitfield.h
 create mode 100644 arch/nds32/include/asm/cache.h
 create mode 100644 arch/nds32/include/asm/cache_info.h
 create mode 100644 arch/nds32/include/asm/cacheflush.h
 create mode 100644 arch/nds32/include/asm/current.h
 create mode 100644 arch/nds32/include/asm/delay.h
 create mode 100644 arch/nds32/include/asm/dma-mapping.h
 create mode 100644 arch/nds32/include/asm/elf.h
 create mode 100644 arch/nds32/include/asm/fixmap.h
 create mode 100644 arch/nds32/include/asm/futex.h
 create mode 100644 arch/nds32/include/asm/highmem.h
 create mode 100644 arch/nds32/include/asm/io.h
 create mode 100644 arch/nds32/include/asm/irqflags.h
 create mode 100644 arch/nds32/include/asm/l2_cache.h
 create mode 100644 arch/nds32/include/asm/linkage.h
 create mode 100644 arch/nds32/include/asm/memory.h
 create mode 100644 arch/nds32/include/asm/mmu.h
 create mode 100644 arch/nds32/include/asm/mmu_context.h
 create mode 100644 arch/nds32/include/asm/module.h
 create mode 100644 arch/nds32/include/asm/nds32.h
 create mode 100644 arch/nds32/include/asm/page.h
 create mode 100644 arch/nds32/include/asm/pgalloc.h
 create mode 100644 arch/nds32/include/asm/pgtable.h
 create mode 100644 arch/nds32/include/asm/proc-fns.h
 create mode 100644 arch/nds32/include/asm/processor.h
 create mode 100644 arch/nds32/include/asm/ptrace.h
 create mode 100644 arch/nds32/include/asm/shmparam.h
 create mode 100644 arch/nds32/include/asm/spinlock.h
 create mode 100644 arch/nds32/include/asm/string.h
 create mode 100644 arch/nds32/include/asm/swab.h
 create mode 100644 arch/nds32/include/asm/syscall.h
 create mode 100644 arch/nds32/include/asm/syscalls.h
 create mode 100644 arch/nds32/include/asm/thread_info.h
 create mode 100644 arch/nds32/include/asm/tlb.h
 create mode 100644 arch/nds32/include/asm/tlbflush.h
 create mode 100644 arch/nds32/include/asm/uaccess.h
 create mode 100644 arch/nds32/include/asm/unistd.h
 create mode 100644 arch/nds32/include/asm/vdso.h
 create mode 100644 arch/nds32/include/asm/vdso_datapage.h
 create mode 100644 arch/nds32/include/uapi/asm/Kbuild
 create mode 100644 arch/nds32/include/uapi/asm/auxvec.h
 create mode 100644 arch/nds32/include/uapi/asm/byteorder.h
 create mode 100644 arch/nds32/include/uapi/asm/cachectl.h
 create mode 100644 arch/nds32/include/uapi/asm/param.h
 create mode 100644 arch/nds32/include/uapi/asm/ptrace.h
 create mode 100644 arch/nds32/include/uapi/asm/sigcontext.h
 create mode 100644 arch/nds32/include/uapi/asm/signal.h
 create mode 100644 arch/nds32/include/uapi/asm/unistd.h
 create mode 100644 arch/nds32/kernel/Makefile
 create mode 100644 arch/nds32/kernel/asm-offsets.c
 create mode 100644 arch/nds32/kernel/atl2c.c
 create mode 100644 arch/nds32/kernel/cacheinfo.c
 create mode 100644 arch/nds32/kernel/devtree.c
 create mode 100644 arch/nds32/kernel/dma.c
 create mode 100644 arch/nds32/kernel/ex-entry.S
 create mode 100644 arch/nds32/kernel/ex-exit.S
 create mode 100644 arch/nds32/kernel/ex-scall.S
 create mode 100644 arch/nds32/kernel/head.S
 create mode 100644 arch/nds32/kernel/irq.c
 create mode 100644 arch/nds32/kernel/module.c
 create mode 100644 arch/nds32/kernel/nds32_ksyms.c
 create mode 100644 arch/nds32/kernel/process.c
 create mode 100644 arch/nds32/kernel/ptrace.c
 create mode 100644 arch/nds32/kernel/setup.c
 create mode 100644 arch/nds32/kernel/signal.c
 create mode 100644 arch/nds32/kernel/stacktrace.c
 create mode 100644 arch/nds32/kernel/sys_nds32.c
 create mode 100644 arch/nds32/kernel/syscall_table.c
 create mode 100644 arch/nds32/kernel/time.c
 create mode 100644 arch/nds32/kernel/traps.c
 create mode 100644 arch/nds32/kernel/vdso.c
 create mode 100644 arch/nds32/kernel/vdso/Makefile
 create mode 100644 arch/nds32/kernel/vdso/datapage.S
 create mode 100755 arch/nds32/kernel/vdso/gen_vdso_offsets.sh
 create mode 100644 arch/nds32/kernel/vdso/gettimeofday.c
 create mode 100644 arch/nds32/kernel/vdso/note.S
 create mode 100644 arch/nds32/kernel/vdso/sigreturn.S
 create mode 100644 arch/nds32/kernel/vdso/vdso.S
 create mode 100644 arch/nds32/kernel/vdso/vdso.lds.S
 create mode 100644 arch/nds32/kernel/vmlinux.lds.S
 create mode 100644 arch/nds32/lib/Makefile
 create mode 100644 arch/nds32/lib/clear_user.S
 create mode 100644 arch/nds32/lib/copy_from_user.S
 create mode 100644 arch/nds32/lib/copy_page.S
 create mode 100644 arch/nds32/lib/copy_template.S
 create mode 100644 arch/nds32/lib/copy_to_user.S
 create mode 100644 arch/nds32/lib/memcpy.S
 create mode 100644 arch/nds32/lib/memmove.S
 create mode 100644 arch/nds32/lib/memset.S
 create mode 100644 arch/nds32/lib/memzero.S
 create mode 100644 arch/nds32/mm/Makefile
 create mode 100644 arch/nds32/mm/alignment.c
 create mode 100644 arch/nds32/mm/cacheflush.c
 create mode 100644 arch/nds32/mm/extable.c
 create mode 100644 arch/nds32/mm/fault.c
 create mode 100644 arch/nds32/mm/highmem.c
 create mode 100644 arch/nds32/mm/init.c
 create mode 100644 arch/nds32/mm/ioremap.c
 create mode 100644 arch/nds32/mm/mm-nds32.c
 create mode 100644 arch/nds32/mm/mmap.c
 create mode 100644 arch/nds32/mm/proc.c
 create mode 100644 arch/nds32/mm/tlb.c
 create mode 100644 drivers/clocksource/timer-atcpit100.c
 create mode 100644 drivers/irqchip/irq-ativic32.c

Comments

Mark Rutland Nov. 27, 2017, 1:51 p.m. UTC | #1
Hi,

On Mon, Nov 27, 2017 at 08:27:53PM +0800, Greentime Hu wrote:
> +void do_page_fault(unsigned long entry, unsigned long addr,
> +		   unsigned int error_code, struct pt_regs *regs)
> +{

> +	/*
> +	 * As per x86, we may deadlock here. However, since the kernel only
> +	 * validly references user space from well defined areas of the code,
> +	 * we can bug out early if this is from code which shouldn't.
> +	 */
> +	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
> +		if (!user_mode(regs) &&
> +		    !search_exception_tables(instruction_pointer(regs)))
> +			goto no_context;
> +retry:
> +		down_read(&mm->mmap_sem);
> +	} else {
> +		/*
> +		 * The above down_read_trylock() might have succeeded in which
> +		 * case, we'll have missed the might_sleep() from down_read().
> +		 */
> +		might_sleep();
> +		if (IS_ENABLED(CONFIG_DEBUG_VM)) {
> +			if (!user_mode(regs) &&
> +			    !search_exception_tables(instruction_pointer(regs)))
> +				goto no_context;
> +		}
> +	}

> +	fault = handle_mm_fault(vma, addr, flags);
> +
> +	/*
> +	 * If we need to retry but a fatal signal is pending, handle the
> +	 * signal first. We do not need to release the mmap_sem because it
> +	 * would already be released in __lock_page_or_retry in mm/filemap.c.
> +	 */
> +	if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
> +		return;

I believe you can get stuck in a livelock here (with an unkillable
task), if a uaccess primitive tries to access a region protected by a
userfaultfd. Please see:

  https://lkml.kernel.org/r/1499782590-31366-1-git-send-email-mark.rutland@arm.com

... for details and a test case.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Nov. 27, 2017, 1:57 p.m. UTC | #2
Hi,

On Mon, Nov 27, 2017 at 08:27:57PM +0800, Greentime Hu wrote:
> +static inline void arch_spin_unlock(arch_spinlock_t * lock)
> +{
> +	asm volatile(
> +		"xor	$r15,  $r15, $r15\n"
> +		"swi	$r15, [%0]\n"
> +		:
> +		:"r"(&lock->lock)
> +		:"memory");
> +}

This looks suspicious. There's no clobber for $r15, so isn't this
corrupting a GPR behind the back of the compiler?

Shouldn't there be a tmp variable for the register allocation rather
than hard-coding $r15?

> +static inline void arch_write_unlock(arch_rwlock_t * rw)
> +{
> +	asm volatile(
> +		"xor	$r15, $r15, $r15\n"
> +		"swi	$r15, [%0]\n"
> +		:
> +		:"r"(&rw->lock)
> +		:"memory","$r15");
> +}

This time you have a clobber, but it's still not clear to me why you
don't use a tmp variable and leave the register allocation to the
compiler.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:11 p.m. UTC | #3
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Rick Chen <rickchen36@gmail.com>
>
> Add CLKSRC_ATCPIT100 for Andestech atcpit100 timer selection.
> It often be used in Andestech AE3XX platform.
>
> Signed-off-by: Rick Chen <rickchen36@gmail.com>
> Signed-off-by: Greentime Hu <green.hu@gmail.com>

No need to split out the Makefile patch from the actual driver, they
clearly belong together.
The binding change should be a separate patch, as you did.

It's probably best to separate the driver patches from the
architecture submission
in the future, they can simply get merged by the respective subsystem
maintainers,
with a smaller Cc list.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:14 p.m. UTC | #4
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> It allows some architectures to use this generic macro instead of
> defining theirs.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  include/asm-generic/io.h |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

This looks good, but you probably want to do this earlier in the series, so you
can build on top of this change.

Please keep this patch as part of your series,

Acked-by: Arnd Bergmann <arnd@arndb.de>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:15 p.m. UTC | #5
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> Signed-off-by: Greentime Hu <greentime@andestech.com>

This is missing a patch description.

> diff --git a/drivers/net/ethernet/faraday/Kconfig b/drivers/net/ethernet/faraday/Kconfig
> index 040c7f1..0ae6e9a 100644
> --- a/drivers/net/ethernet/faraday/Kconfig
> +++ b/drivers/net/ethernet/faraday/Kconfig
> @@ -5,7 +5,7 @@
>  config NET_VENDOR_FARADAY
>         bool "Faraday devices"
>         default y
> -       depends on ARM
> +       depends on ARM || NDS32
>         ---help---
>           If you have a network (Ethernet) card belonging to this class, say Y.
>

We probably want to make all three here

depends on ARM || NDS32 || COMPILE_TEST

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:21 p.m. UTC | #6
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
> new file mode 100644
> index 0000000..6b4013f
> --- /dev/null
> +++ b/arch/nds32/Kconfig.cpu
> @@ -0,0 +1,131 @@
> +comment "Processor Features"
> +
> +config CPU_BIG_ENDIAN
> +       bool "Big endian"
> +       def_bool n
> +
> +config CPU_LITTLE_ENDIAN
> +       bool "Little endian"
> +       def_bool y

These must be mutually exclusive, you surely get some build error if you try to
set both here.

You can either make it a 'choice' statement to pick between the two, or you
can make CPU_LITTLE_ENDIAN a silent option like

config CPU_BIG_ENDIAN
       bool "Big endian"

config CPU_LITTLE_ENDIAN
       def_bool !CPU_BIG_ENDIAN

> +config CPU_CACHE_NONALIASING
> +       bool "Non-aliasing cache"
> +       help
> +         If this CPU is using VIPT data cache and its cache way size is larger
> +         than page size, say N. If it is using PIPT data cache, say Y.
> +
> +         If unsure, say Y.

Can you determine this from the CPU type?

> +choice
> +       prompt "Memory split"
> +       depends on MMU
> +       default VMSPLIT_3G
> +       help
> +         Select the desired split between kernel and user memory.
> +
> +         If you are not absolutely sure what you are doing, leave this
> +         option alone!
> +
> +       config VMSPLIT_3G
> +               bool "3G/1G user/kernel split"
> +       config VMSPLIT_3G_OPT
> +               bool "3G/1G user/kernel split (for full 1G low memory)"
> +       config VMSPLIT_2G
> +               bool "2G/2G user/kernel split"
> +       config VMSPLIT_1G
> +               bool "1G/3G user/kernel split"
> +endchoice

I think you mentioned that the 1GB configuration is quite common, how
about making VMSPLIT_3G_OPT the default here?

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:27 p.m. UTC | #7
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> +CONFIG_EXPERT=y

You normally shouldn't need CONFIG_EXPERT in a defconfig file, by definition
this is needed for unusual configurations only.

> +CONFIG_NDS32_BUILTIN_DTB="ae3xx"

Having a built-in DTB also makes no sense for a defconfig, when the idea is that
it can run on as many machines as possible.

> +CONFIG_BLK_DEV_LOOP=y
> +CONFIG_BLK_DEV_RAM=y
> +CONFIG_BLK_DEV_RAM_SIZE=8192

Having ramdisk built-in is rather unusual, most users don't use ramdisks
any more since we don't need it for booting with the initramfs.

> +CONFIG_BRIDGE=y
> +CONFIG_TUN=y

These also look unusual. Would it make sense to have these as loadable
modules, or do you require having everything built-in for your workflow?

> +CONFIG_SOUND=y
> +CONFIG_SND=y
> +# CONFIG_SND_SUPPORT_OLD_API is not set
> +# CONFIG_SND_VERBOSE_PROCFS is not set

It seems the sound subsystem is enabled, but no drivers are selected,
so it won't actually do anything.

> +# CONFIG_USB_SUPPORT is not set
> +CONFIG_MMC=y

Same for MMC. If you are still trying to get the respective device drivers
merged, that doesn't need to stop you from enabling the configurations
here.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:30 p.m. UTC | #8
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> @@ -0,0 +1,55 @@
> +/dts-v1/;
> +/ {
> +       compatible = "nds32 ae3xx";

The compatible string doesn't seem to match the binding, it should always have
vendor prefix.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&intc>;
> +
> +       chosen {
> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
> +               stdout-path = &serial0;
> +       };

I would drop the bootargs here, this is something that should be set by the
bootloader and is up to the user.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:33 p.m. UTC | #9
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> +
> +#define L2C_R_REG(offset)               __raw_readl(atl2c_base + offset)
> +#define L2C_W_REG(offset, value)        __raw_writel(value, atl2c_base + offset)

__raw_readl() is generally not endian-safe, and might  not have the barriers you
require here. Could you use readl/writel here, and only fall back to
readl_relaxed()/writel_relaxed() when you absolutely must avoid the barriers?

> diff --git a/arch/nds32/kernel/atl2c.c b/arch/nds32/kernel/atl2c.c
> new file mode 100644
> index 0000000..dd87fc9
> --- /dev/null
> +++ b/arch/nds32/kernel/atl2c.c
> +#include <linux/compiler.h>
> +#include <linux/of_address.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_platform.h>
> +#include <asm/l2_cache.h>

If this is the only file that includes asm/l2_cache.h, then I'd simply
move the entire
contents in here, rather than having a separate file in the global namespace.

          Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:34 p.m. UTC | #10
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
> +struct user_pt_regs {
> +       long uregs[26];
> +       long fp;
> +       long gp;
> +       long lp;
> +       long sp;
> +       long ipc;
> +#if defined(CONFIG_HWZOL)
> +       long lb;
> +       long le;
> +       long lc;
> +#else
> +       long dummy[3];
> +#endif
> +       long syscallno;
> +};

You cannot reference CONFIG_ symbols in a uapi header, since the
configuration headers
are not available.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:37 p.m. UTC | #11
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> +#ifndef _ASMNDS32_SIGNAL_H
> +#define _ASMNDS32_SIGNAL_H
> +
> +#define SA_RESTORER    0x04000000
> +
> +#include <asm-generic/signal.h>
> +#endif

As documented in asm-generic/signal.h, new architectures should not define
SA_RESTORER.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:46 p.m. UTC | #12
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> diff --git a/arch/nds32/include/asm/syscalls.h b/arch/nds32/include/asm/syscalls.h
> new file mode 100644
> index 0000000..741ccdc
> --- /dev/null
> +++ b/arch/nds32/include/asm/syscalls.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ASM_NDS32_SYSCALLS_H
> +#define __ASM_NDS32_SYSCALLS_H
> +
> +int sys_cacheflush(unsigned long addr, unsigned long len, unsigned int op);
> +long sys_fadvise64_64_wrapper(int fd, int advice, loff_t offset, loff_t len);
> +asmlinkage int sys_syscall(void);
> +asmlinkage int sys_rt_sigreturn_wrapper(void);

You mentioned that sys_syscall() should be removed in this version.

By convention, all syscall declarations should start with 'asmlinkage long',
even though this has no effect in your architecture.

> diff --git a/arch/nds32/include/uapi/asm/unistd.h b/arch/nds32/include/uapi/asm/unistd.h
> new file mode 100644
> index 0000000..2bad1e7
> --- /dev/null
> +++ b/arch/nds32/include/uapi/asm/unistd.h

> +
> +#define __ARCH_WANT_RENAMEAT
> +#define __ARCH_WANT_SYSCALL_OFF_T

These two should not be here.

> +#define __ARCH_WANT_SYNC_FILE_RANGE2

This one is fine though

> +/* Use the standard ABI for syscalls */
> +#include <asm-generic/unistd.h>
> +
> +__SYSCALL(__NR_fadvise64_64, sys_fadvise64_64_wrapper)
> +__SYSCALL(__NR_rt_sigreturn, sys_rt_sigreturn_wrapper)

I think you are overriding the array assignment here, this may cause a
compiler warning when building with "make C=1 V=1" since you have
the same index assigned to two pointers. A better way to do this
is to override the name:

#define sys_rt_sigreturn sys_rt_sigreturn_wrapper
#define sys_fadvise64_64 sys_fadvise64_64_wrapper
#include <asm-generic/unistd.h>


      Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 27, 2017, 2:51 p.m. UTC | #13
On Mon, Nov 27, 2017 at 1:27 PM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch introduces ioremap implementations.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  arch/nds32/include/asm/io.h |   25 +++++++++++++++
>  arch/nds32/mm/ioremap.c     |   75 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 100 insertions(+)
>  create mode 100644 arch/nds32/include/asm/io.h
>  create mode 100644 arch/nds32/mm/ioremap.c
>
> diff --git a/arch/nds32/include/asm/io.h b/arch/nds32/include/asm/io.h
> new file mode 100644
> index 0000000..b83dea1
> --- /dev/null
> +++ b/arch/nds32/include/asm/io.h
> @@ -0,0 +1,25 @@

> +#ifndef __ASM_NDS32_IO_H
> +#define __ASM_NDS32_IO_H
> +
> +#ifdef __KERNEL__
> +void iounmap(void __iomem * addr);
> +#include <asm-generic/io.h>
> +
> +#endif /* __KERNEL__ */
> +#endif /* __ASM_NDS32_IO_H */

Here, you should define a lot of the I/O accessor functions,
dereferencing a pointer
is generally not enough to guarantee an atomic MMIO operation. You need to
force the access to use the correct size to prevent the compiler from issuing
byte-sized operations when it thinks the pointer might be unaligned, and
there should be barriers that ensure a memory access is synchronized with
a DMA that might be triggered by a writel, or claimed to be completed after
a readl. Please see the risc-v header for this, it has many good explanations.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Nov. 27, 2017, 7:07 p.m. UTC | #14
On Mon, Nov 27, 2017 at 6:28 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for device tree.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  arch/nds32/boot/dts/Makefile   |    8 ++++++
>  arch/nds32/boot/dts/ae3xx.dts  |   55 ++++++++++++++++++++++++++++++++++++
>  arch/nds32/boot/dts/ag101p.dts |   60 ++++++++++++++++++++++++++++++++++++++++
>  arch/nds32/kernel/devtree.c    |   45 ++++++++++++++++++++++++++++++
>  4 files changed, 168 insertions(+)
>  create mode 100644 arch/nds32/boot/dts/Makefile
>  create mode 100644 arch/nds32/boot/dts/ae3xx.dts
>  create mode 100644 arch/nds32/boot/dts/ag101p.dts
>  create mode 100644 arch/nds32/kernel/devtree.c
>
> diff --git a/arch/nds32/boot/dts/Makefile b/arch/nds32/boot/dts/Makefile
> new file mode 100644
> index 0000000..d31faa8
> --- /dev/null
> +++ b/arch/nds32/boot/dts/Makefile
> @@ -0,0 +1,8 @@
> +ifneq '$(CONFIG_NDS32_BUILTIN_DTB)' '""'

Built-in dtb's are really for legacy bootloader cases where the
bootloader doesn't understand dtbs. Do you have that here?

Plus, I don't see any code here to handle the built-in dtb.

> +BUILTIN_DTB := $(patsubst "%",%,$(CONFIG_NDS32_BUILTIN_DTB)).dtb.o
> +else
> +BUILTIN_DTB :=
> +endif
> +obj-$(CONFIG_OF) += $(BUILTIN_DTB)
> +
> +clean-files := *.dtb *.dtb.S
> diff --git a/arch/nds32/boot/dts/ae3xx.dts b/arch/nds32/boot/dts/ae3xx.dts
> new file mode 100644
> index 0000000..4181060
> --- /dev/null
> +++ b/arch/nds32/boot/dts/ae3xx.dts
> @@ -0,0 +1,55 @@
> +/dts-v1/;
> +/ {
> +       compatible = "nds32 ae3xx";

This compatible needs to be documented and is not valid. Needs to be
in the form "vendor,board-name" without spaces.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&intc>;
> +
> +       chosen {
> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
> +               stdout-path = &serial0;
> +       };
> +
> +       memory@0 {
> +               device_type = "memory";
> +               reg = <0x00000000 0x40000000>;
> +       };
> +
> +       cpu {
> +               device_type = "cpu";
> +               compatible = "andestech,n13", "andestech,nds32v3";
> +               clock-frequency = <60000000>;
> +       };
> +
> +       intc: interrupt-controller {
> +               compatible = "andestech,ativic32";
> +               #interrupt-cells = <1>;
> +               interrupt-controller;
> +       };
> +
> +       serial0: serial@f0300000 {
> +               compatible = "andestech,uart16550", "ns16550a";
> +               reg = <0xf0300000 0x1000>;
> +               interrupts = <8>;
> +               clock-frequency = <14745600>;
> +               reg-shift = <2>;
> +               reg-offset = <32>;
> +               no-loopback-test = <1>;
> +       };
> +
> +       timer0: timer@f0400000 {
> +               compatible = "andestech,atcpit100";
> +               reg = <0xf0400000 0x1000>;
> +               interrupts = <2>;
> +               clock-frequency = <30000000>;
> +               cycle-count-offset = <0x38>;
> +               cycle-count-down;
> +       };
> +
> +       mac0: mac@e0100000 {

ethernet@...

> +               compatible = "andestech,atmac100";
> +               reg = <0xe0100000 0x1000>;
> +               interrupts = <18>;
> +       };
> +
> +};
> diff --git a/arch/nds32/boot/dts/ag101p.dts b/arch/nds32/boot/dts/ag101p.dts
> new file mode 100644
> index 0000000..f1cb540
> --- /dev/null
> +++ b/arch/nds32/boot/dts/ag101p.dts
> @@ -0,0 +1,60 @@
> +/dts-v1/;
> +/ {
> +       compatible = "nds32 ag101p";

Same here.

> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       interrupt-parent = <&intc>;
> +
> +       chosen {
> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
> +               stdout-path = &serial0;
> +       };
> +
> +       memory@0 {
> +               device_type = "memory";
> +               reg = <0x00000000 0x40000000>;
> +       };
> +
> +       cpu@0 {
> +               device_type = "cpu";
> +               compatible = "andestech,n13";
> +               clock-frequency = <60000000>;
> +               next-level-cache = <&L2>;
> +       };
> +
> +       intc: interrupt-controller {
> +               compatible = "andestech,ativic32";
> +               #interrupt-cells = <2>;
> +               interrupt-controller;
> +       };
> +
> +       serial0: serial@99600000 {
> +               compatible = "andestech,uart16550", "ns16550a";
> +               reg = <0x99600000 0x1000>;
> +               interrupts = <7 4>;
> +               clock-frequency = <14745600>;
> +               reg-shift = <2>;
> +               no-loopback-test = <1>;
> +       };
> +
> +       timer0: timer@98400000 {
> +               compatible = "andestech,atftmr010";
> +               reg = <0x98400000 0x1000>;
> +               interrupts = <19 4>;
> +               clock-frequency = <15000000>;
> +               cycle-count-offset = <0x20>;
> +       };
> +
> +       mac0: mac@90900000 {
> +               compatible = "andestech,atmac100";
> +               reg = <0x90900000 0x1000>;
> +               interrupts = <25 4>;
> +       };
> +
> +       L2: l2-cache {
> +               compatible = "andestech,atl2c";
> +               reg = <0x90f00000 0x1000>;
> +               cache-unified;
> +               cache-level = <2>;
> +       };
> +};
> diff --git a/arch/nds32/kernel/devtree.c b/arch/nds32/kernel/devtree.c
> new file mode 100644
> index 0000000..2af0f1c
> --- /dev/null
> +++ b/arch/nds32/kernel/devtree.c
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/memblock.h>
> +#include <linux/of_fdt.h>
> +#include <linux/bootmem.h>
> +
> +void __init early_init_dt_add_memory_arch(u64 base, u64 size)
> +{
> +       size &= PAGE_MASK;
> +       memblock_add_node(base, size, 0);
> +}
> +
> +void *__init early_init_dt_alloc_memory_arch(u64 size, u64 align)
> +{
> +       return alloc_bootmem_align(size, align);
> +}

You should be able to use the default functions for these 2.

> +
> +void __init early_init_devtree(void *params)
> +{
> +       if (!params || !early_init_dt_scan(params)) {
> +               pr_crit("\n"
> +                       "Error: invalid device tree blob at (virtual address 0x%p)\n"
> +                       "The dtb must be 8-byte aligned and must not exceed 8 KB in size\n"

Why the size limit? That's pretty small for a DT.

> +                       "\nPlease check your bootloader.", params);
> +
> +               while (true)
> +                       cpu_relax();

Might as well use BUG_ON here if you're not going to continue. It's
generally better to WARN and continue on otherwise the messages aren't
visible until the console is up. However, if you have DT errors this
early, there's not much you can really do here.

> +       }
> +
> +       dump_stack_set_arch_desc("%s (DT)", of_flat_dt_get_machine_name());
> +}
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Nov. 27, 2017, 7:14 p.m. UTC | #15
On Mon, Nov 27, 2017 at 1:07 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Mon, Nov 27, 2017 at 6:28 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> This patch adds support for device tree.
>>
>> Signed-off-by: Vincent Chen <vincentc@andestech.com>
>> Signed-off-by: Greentime Hu <greentime@andestech.com>
>> ---
>>  arch/nds32/boot/dts/Makefile   |    8 ++++++
>>  arch/nds32/boot/dts/ae3xx.dts  |   55 ++++++++++++++++++++++++++++++++++++
>>  arch/nds32/boot/dts/ag101p.dts |   60 ++++++++++++++++++++++++++++++++++++++++
>>  arch/nds32/kernel/devtree.c    |   45 ++++++++++++++++++++++++++++++
>>  4 files changed, 168 insertions(+)
>>  create mode 100644 arch/nds32/boot/dts/Makefile
>>  create mode 100644 arch/nds32/boot/dts/ae3xx.dts
>>  create mode 100644 arch/nds32/boot/dts/ag101p.dts
>>  create mode 100644 arch/nds32/kernel/devtree.c
>>
>> diff --git a/arch/nds32/boot/dts/Makefile b/arch/nds32/boot/dts/Makefile
>> new file mode 100644
>> index 0000000..d31faa8
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/Makefile
>> @@ -0,0 +1,8 @@
>> +ifneq '$(CONFIG_NDS32_BUILTIN_DTB)' '""'
>
> Built-in dtb's are really for legacy bootloader cases where the
> bootloader doesn't understand dtbs. Do you have that here?
>
> Plus, I don't see any code here to handle the built-in dtb.

I see it is in patch 2. That means patch 2 won't actually compile. You
should move the devtree.c parts at least and maybe the Makefile
infrastructure part too to patch 2. I don't think you need to split
things up so much, but I'll defer to Arnd on that.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vincent Chen Nov. 28, 2017, 2:18 a.m. UTC | #16
2017-11-27 22:46 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>
>> diff --git a/arch/nds32/include/asm/syscalls.h b/arch/nds32/include/asm/syscalls.h
>> new file mode 100644
>> index 0000000..741ccdc
>> --- /dev/null
>> +++ b/arch/nds32/include/asm/syscalls.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Copyright (C) 2005-2017 Andes Technology Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __ASM_NDS32_SYSCALLS_H
>> +#define __ASM_NDS32_SYSCALLS_H
>> +
>> +int sys_cacheflush(unsigned long addr, unsigned long len, unsigned int op);
>> +long sys_fadvise64_64_wrapper(int fd, int advice, loff_t offset, loff_t len);
>> +asmlinkage int sys_syscall(void);
>> +asmlinkage int sys_rt_sigreturn_wrapper(void);
>
> You mentioned that sys_syscall() should be removed in this version.
>

Sorry, I will remove it in next version patch.

> By convention, all syscall declarations should start with 'asmlinkage long',
> even though this has no effect in your architecture.
>

Thanks.
I will modify it  in next version patch.

>> diff --git a/arch/nds32/include/uapi/asm/unistd.h b/arch/nds32/include/uapi/asm/unistd.h
>> new file mode 100644
>> index 0000000..2bad1e7
>> --- /dev/null
>> +++ b/arch/nds32/include/uapi/asm/unistd.h
>
>> +
>> +#define __ARCH_WANT_RENAMEAT
>> +#define __ARCH_WANT_SYSCALL_OFF_T
>
> These two should not be here.
>

Thanks.
But, I don't know I should move these two macro to which file.
In asm-generic/unistd.h, these two are used to decide whether relative
syscall number is defined or not.
Therefore,  I put these two macros here in order that these two
definitions are available in user space.


>> +#define __ARCH_WANT_SYNC_FILE_RANGE2
>
> This one is fine though
>
>> +/* Use the standard ABI for syscalls */
>> +#include <asm-generic/unistd.h>
>> +
>> +__SYSCALL(__NR_fadvise64_64, sys_fadvise64_64_wrapper)
>> +__SYSCALL(__NR_rt_sigreturn, sys_rt_sigreturn_wrapper)
>
> I think you are overriding the array assignment here, this may cause a
> compiler warning when building with "make C=1 V=1" since you have
> the same index assigned to two pointers. A better way to do this
> is to override the name:
>
> #define sys_rt_sigreturn sys_rt_sigreturn_wrapper
> #define sys_fadvise64_64 sys_fadvise64_64_wrapper
> #include <asm-generic/unistd.h>
>
Thanks,
I will follow your suggestion to modify it in the next version patch.

>
>       Arnd


Vincent
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vincent Chen Nov. 28, 2017, 2:21 a.m. UTC | #17
2017-11-27 22:37 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>
>> +#ifndef _ASMNDS32_SIGNAL_H
>> +#define _ASMNDS32_SIGNAL_H
>> +
>> +#define SA_RESTORER    0x04000000
>> +
>> +#include <asm-generic/signal.h>
>> +#endif
>
> As documented in asm-generic/signal.h, new architectures should not define
> SA_RESTORER.
>

Thanks.
I will remove it in the next version patch.

>         Arnd

Vincent
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vincent Chen Nov. 28, 2017, 2:21 a.m. UTC | #18
2017-11-27 22:34 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> +struct user_pt_regs {
>> +       long uregs[26];
>> +       long fp;
>> +       long gp;
>> +       long lp;
>> +       long sp;
>> +       long ipc;
>> +#if defined(CONFIG_HWZOL)
>> +       long lb;
>> +       long le;
>> +       long lc;
>> +#else
>> +       long dummy[3];
>> +#endif
>> +       long syscallno;
>> +};
>
> You cannot reference CONFIG_ symbols in a uapi header, since the
> configuration headers
> are not available.
>
Thanks
I will modify it in the next version patch.

>        Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 28, 2017, 2:53 a.m. UTC | #19
2017-11-27 22:11 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> From: Rick Chen <rickchen36@gmail.com>
>>
>> Add CLKSRC_ATCPIT100 for Andestech atcpit100 timer selection.
>> It often be used in Andestech AE3XX platform.
>>
>> Signed-off-by: Rick Chen <rickchen36@gmail.com>
>> Signed-off-by: Greentime Hu <green.hu@gmail.com>
>
> No need to split out the Makefile patch from the actual driver, they
> clearly belong together.
> The binding change should be a separate patch, as you did.
>
> It's probably best to separate the driver patches from the
> architecture submission
> in the future, they can simply get merged by the respective subsystem
> maintainers,
> with a smaller Cc list.
>

Hi, Arnd:

Thanks.
We will merge these 2 patches together in the next version patch.
We will sent to clocksource subsystem with a seperate patchset.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 28, 2017, 2:55 a.m. UTC | #20
2017-11-27 22:15 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> Signed-off-by: Greentime Hu <greentime@andestech.com>
>
> This is missing a patch description.
>

Thanks.
Sorry. I miss it. I will add the commit messages in the next version patch.

>> diff --git a/drivers/net/ethernet/faraday/Kconfig b/drivers/net/ethernet/faraday/Kconfig
>> index 040c7f1..0ae6e9a 100644
>> --- a/drivers/net/ethernet/faraday/Kconfig
>> +++ b/drivers/net/ethernet/faraday/Kconfig
>> @@ -5,7 +5,7 @@
>>  config NET_VENDOR_FARADAY
>>         bool "Faraday devices"
>>         default y
>> -       depends on ARM
>> +       depends on ARM || NDS32
>>         ---help---
>>           If you have a network (Ethernet) card belonging to this class, say Y.
>>
>
> We probably want to make all three here
>
> depends on ARM || NDS32 || COMPILE_TEST
>

Thanks.
I will modify it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vincent Chen Nov. 28, 2017, 4:24 a.m. UTC | #21
2017-11-27 21:57 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> Hi,
>
> On Mon, Nov 27, 2017 at 08:27:57PM +0800, Greentime Hu wrote:
>> +static inline void arch_spin_unlock(arch_spinlock_t * lock)
>> +{
>> +     asm volatile(
>> +             "xor    $r15,  $r15, $r15\n"
>> +             "swi    $r15, [%0]\n"
>> +             :
>> +             :"r"(&lock->lock)
>> +             :"memory");
>> +}
>
> This looks suspicious. There's no clobber for $r15, so isn't this
> corrupting a GPR behind the back of the compiler?
>

Thanks.
$r15 is reserved for assembler.
For safety, compiler always put $r15 in clobber register list by default.


> Shouldn't there be a tmp variable for the register allocation rather
> than hard-coding $r15?
>
>> +static inline void arch_write_unlock(arch_rwlock_t * rw)
>> +{
>> +     asm volatile(
>> +             "xor    $r15, $r15, $r15\n"
>> +             "swi    $r15, [%0]\n"
>> +             :
>> +             :"r"(&rw->lock)
>> +             :"memory","$r15");
>> +}
>
> This time you have a clobber, but it's still not clear to me why you
> don't use a tmp variable and leave the register allocation to the
> compiler.
>

Thanks.
When I recheck the code, I found spinlock.h is not needed due that we
do not support SMP.
So, We decide to remove spinlock.h in the next version patch.

Vincent
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 28, 2017, 6:54 a.m. UTC | #22
2017-11-27 22:30 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> @@ -0,0 +1,55 @@
>> +/dts-v1/;
>> +/ {
>> +       compatible = "nds32 ae3xx";
>
> The compatible string doesn't seem to match the binding, it should always have
> vendor prefix.

Sorry I forgot to check this.
I will provide a document in bindings like
"Documentation/devicetree/bindings/nds32/andestech-boards".


>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +       interrupt-parent = <&intc>;
>> +
>> +       chosen {
>> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
>> +               stdout-path = &serial0;
>> +       };
>
> I would drop the bootargs here, this is something that should be set by the
> bootloader and is up to the user.

Thanks
I will drop it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 28, 2017, 9:23 a.m. UTC | #23
On Tue, Nov 28, 2017 at 3:18 AM, Vincent Chen <deanbo422@gmail.com> wrote:
> 2017-11-27 22:46 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

>>> diff --git a/arch/nds32/include/uapi/asm/unistd.h b/arch/nds32/include/uapi/asm/unistd.h
>>> new file mode 100644
>>> index 0000000..2bad1e7
>>> --- /dev/null
>>> +++ b/arch/nds32/include/uapi/asm/unistd.h
>>
>>> +
>>> +#define __ARCH_WANT_RENAMEAT
>>> +#define __ARCH_WANT_SYSCALL_OFF_T
>>
>> These two should not be here.
>>
>
> Thanks.
> But, I don't know I should move these two macro to which file.
> In asm-generic/unistd.h, these two are used to decide whether relative
> syscall number is defined or not.
> Therefore,  I put these two macros here in order that these two
> definitions are available in user space.

What I meant is that they should not be available to user space.

The C libraries implement the user space interfaces based
on the replacement system calls, e.g. an application calling
the glibc stat() function will end up in the sys_stat64() system
call entry point, not the older sys_newstat().

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Nov. 28, 2017, 9:37 a.m. UTC | #24
On 27/11/17 12:28, Greentime Hu wrote:
> From: Greentime Hu <greentime@andestech.com>
> 
> This patch adds the Andestech Internal Vector Interrupt Controller
> driver. You can find the spec here. Ch4.9 of AndeStar SPA V3 Manual.
> http://www.andestech.com/product.php?cls=9
> 
> Signed-off-by: Rick Chen <rick@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
> ---
>  drivers/irqchip/Makefile       |    1 +
>  drivers/irqchip/irq-ativic32.c |  119 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 120 insertions(+)
>  create mode 100644 drivers/irqchip/irq-ativic32.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b842dfd..201ca9f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -80,3 +80,4 @@ obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>  obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
> +obj-$(CONFIG_NDS32)			+= irq-ativic32.o
> diff --git a/drivers/irqchip/irq-ativic32.c b/drivers/irqchip/irq-ativic32.c
> new file mode 100644
> index 0000000..c4d6f86
> --- /dev/null
> +++ b/drivers/irqchip/irq-ativic32.c
> @@ -0,0 +1,119 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip.h>
> +#include <nds32_intrinsic.h>
> +
> +static void ativic32_ack_irq(struct irq_data *data)
> +{
> +	__nds32__mtsr_dsb(1 << data->hwirq, NDS32_SR_INT_PEND2);

Consider writing (1 << data->hwirq) as BIT(data->hwirq).

> +}
> +
> +static void ativic32_mask_irq(struct irq_data *data)
> +{
> +	unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
> +	__nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)), NDS32_SR_INT_MASK2);

Same here.

> +}
> +
> +static void ativic32_mask_ack_irq(struct irq_data *data)
> +{
> +	unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
> +	__nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)), NDS32_SR_INT_MASK2);
> +	__nds32__mtsr_dsb((1 << data->hwirq), NDS32_SR_INT_PEND2);

This is effectively MASK+ACK, so you're better off just writing it as
such. And since there is no advantage in your implementation in having
MASK_ACK over MASK+ACK, I suggest you remove this function completely,
and rely on the core code which will call them in sequence.

> +
> +}
> +
> +static void ativic32_unmask_irq(struct irq_data *data)
> +{
> +	unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
> +	__nds32__mtsr_dsb(int_mask2 | (1 << data->hwirq), NDS32_SR_INT_MASK2);

Same BIT() here.

> +}
> +
> +static struct irq_chip ativic32_chip = {
> +	.name = "ativic32",
> +	.irq_ack = ativic32_ack_irq,
> +	.irq_mask = ativic32_mask_irq,
> +	.irq_mask_ack = ativic32_mask_ack_irq,
> +	.irq_unmask = ativic32_unmask_irq,
> +};
> +
> +static unsigned int __initdata nivic_map[6] = { 6, 2, 10, 16, 24, 32 };
> +
> +static struct irq_domain *root_domain;
> +static int ativic32_irq_domain_map(struct irq_domain *id, unsigned int virq,
> +				  irq_hw_number_t hw)
> +{
> +
> +	unsigned long int_trigger_type;
> +	int_trigger_type = __nds32__mfsr(NDS32_SR_INT_TRIGGER);
> +	if (int_trigger_type & (1 << hw))

And here.

> +		irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
> +	else
> +		irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);

Since you do not express the trigger in DT, you need to tell the core
about it by calling irqd_set_trigger_type() with the right setting.

> +
> +	return 0;
> +}
> +
> +static struct irq_domain_ops ativic32_ops = {
> +	.map = ativic32_irq_domain_map,
> +	.xlate = irq_domain_xlate_onecell
> +};
> +
> +static int get_intr_src(void)

I'm not sure "int" is the best return type here. I suspect something
unsigned would be preferable, or even the irq_hw_number_t type.
> +{
> +	return ((__nds32__mfsr(NDS32_SR_ITYPE)&ITYPE_mskVECTOR) >> ITYPE_offVECTOR)

Spaces around '&'.

> +		- NDS32_VECTOR_offINTERRUPT;
> +}
> +
> +asmlinkage void asm_do_IRQ(struct pt_regs *regs)
> +{
> +	int hwirq = get_intr_src();

irq_hw_number_t.

> +	handle_domain_irq(root_domain, hwirq, regs);
> +}
> +
> +int __init ativic32_init_irq(struct device_node *node, struct device_node *parent)
> +{
> +	unsigned long int_vec_base, nivic;
> +
> +	if (WARN(parent, "non-root ativic32 are not supported"))
> +		return -EINVAL;
> +
> +	int_vec_base = __nds32__mfsr(NDS32_SR_IVB);
> +
> +	if (((int_vec_base & IVB_mskIVIC_VER) >> IVB_offIVIC_VER) == 0)
> +		panic("Unable to use atcivic32 for this cpu.\n");
> +
> +	nivic = (int_vec_base & IVB_mskNIVIC) >> IVB_offNIVIC;
> +	if (nivic >= (sizeof nivic_map / sizeof nivic_map[0]))

This should be:
	if (nivic >= ARRAY_SIZE(NIVIC_MAP))

> +		panic("The number of input for ativic32 is not supported.\n");
> +
> +	nivic = nivic_map[nivic];

I'd rather you use another variable (nr_ints?).

> +
> +	root_domain = irq_domain_add_linear(node, nivic,
> +			&ativic32_ops, NULL);
> +
> +	if (!root_domain)
> +		panic("%s: unable to create IRQ domain\n", node->full_name);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(ativic32, "andestech,ativic32", ativic32_init_irq);
> 

Thanks,

	M.
Greg KH Nov. 28, 2017, 2:25 p.m. UTC | #25
On Mon, Nov 27, 2017 at 08:28:18PM +0800, Greentime Hu wrote:
> From: Greentime Hu <greentime@andestech.com>
> 
> It will get the wrong virtual address because port->mapbase is not added
> the correct reg-offset yet. We have to update it before earlycon_map()
> is called
> ---
>  drivers/tty/serial/earlycon.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

No signed-off-by line?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 5:40 a.m. UTC | #26
2017-11-28 22:25 GMT+08:00 Greg KH <greg@kroah.com>:
> On Mon, Nov 27, 2017 at 08:28:18PM +0800, Greentime Hu wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> It will get the wrong virtual address because port->mapbase is not added
>> the correct reg-offset yet. We have to update it before earlycon_map()
>> is called
>> ---
>>  drivers/tty/serial/earlycon.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> No signed-off-by line?

Sorry I forget it.
Signed-off-by: Greentime Hu <greentime@andestech.com>

I will add it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 7:24 a.m. UTC | #27
Hi, Mark:

2017-11-27 21:51 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> Hi,
>
> On Mon, Nov 27, 2017 at 08:27:53PM +0800, Greentime Hu wrote:
>> +void do_page_fault(unsigned long entry, unsigned long addr,
>> +                unsigned int error_code, struct pt_regs *regs)
>> +{
>
>> +     /*
>> +      * As per x86, we may deadlock here. However, since the kernel only
>> +      * validly references user space from well defined areas of the code,
>> +      * we can bug out early if this is from code which shouldn't.
>> +      */
>> +     if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
>> +             if (!user_mode(regs) &&
>> +                 !search_exception_tables(instruction_pointer(regs)))
>> +                     goto no_context;
>> +retry:
>> +             down_read(&mm->mmap_sem);
>> +     } else {
>> +             /*
>> +              * The above down_read_trylock() might have succeeded in which
>> +              * case, we'll have missed the might_sleep() from down_read().
>> +              */
>> +             might_sleep();
>> +             if (IS_ENABLED(CONFIG_DEBUG_VM)) {
>> +                     if (!user_mode(regs) &&
>> +                         !search_exception_tables(instruction_pointer(regs)))
>> +                             goto no_context;
>> +             }
>> +     }
>
>> +     fault = handle_mm_fault(vma, addr, flags);
>> +
>> +     /*
>> +      * If we need to retry but a fatal signal is pending, handle the
>> +      * signal first. We do not need to release the mmap_sem because it
>> +      * would already be released in __lock_page_or_retry in mm/filemap.c.
>> +      */
>> +     if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
>> +             return;
>
> I believe you can get stuck in a livelock here (with an unkillable
> task), if a uaccess primitive tries to access a region protected by a
> userfaultfd. Please see:
>
>   https://lkml.kernel.org/r/1499782590-31366-1-git-send-email-mark.rutland@arm.com
>
> ... for details and a test case.
>

Thanks for your teatcase and patch. It works.
I will apply it to the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 8:39 a.m. UTC | #28
2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>
>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>> new file mode 100644
>> index 0000000..6b4013f
>> --- /dev/null
>> +++ b/arch/nds32/Kconfig.cpu
>> @@ -0,0 +1,131 @@
>> +comment "Processor Features"
>> +
>> +config CPU_BIG_ENDIAN
>> +       bool "Big endian"
>> +       def_bool n
>> +
>> +config CPU_LITTLE_ENDIAN
>> +       bool "Little endian"
>> +       def_bool y
>
> These must be mutually exclusive, you surely get some build error if you try to
> set both here.
>
> You can either make it a 'choice' statement to pick between the two, or you
> can make CPU_LITTLE_ENDIAN a silent option like
>
> config CPU_BIG_ENDIAN
>        bool "Big endian"
>
> config CPU_LITTLE_ENDIAN
>        def_bool !CPU_BIG_ENDIAN
>

Thanks. I will fix it in the next version patch.

>> +config CPU_CACHE_NONALIASING
>> +       bool "Non-aliasing cache"
>> +       help
>> +         If this CPU is using VIPT data cache and its cache way size is larger
>> +         than page size, say N. If it is using PIPT data cache, say Y.
>> +
>> +         If unsure, say Y.
>
> Can you determine this from the CPU type?

There is no cpu register to determine it. It also depeneds on page
size and way size however page size is configurable by software.
These codes are determined at compile time will be benefit to code
size and performance.
IMHO, I think it would be better to be determined here.

>> +choice
>> +       prompt "Memory split"
>> +       depends on MMU
>> +       default VMSPLIT_3G
>> +       help
>> +         Select the desired split between kernel and user memory.
>> +
>> +         If you are not absolutely sure what you are doing, leave this
>> +         option alone!
>> +
>> +       config VMSPLIT_3G
>> +               bool "3G/1G user/kernel split"
>> +       config VMSPLIT_3G_OPT
>> +               bool "3G/1G user/kernel split (for full 1G low memory)"
>> +       config VMSPLIT_2G
>> +               bool "2G/2G user/kernel split"
>> +       config VMSPLIT_1G
>> +               bool "1G/3G user/kernel split"
>> +endchoice
>
> I think you mentioned that the 1GB configuration is quite common, how
> about making VMSPLIT_3G_OPT the default here?

We use 1GB in our fpga platform, but I am not sure our customer's
config and their user space program may use bigger memory space.
Anyway, we will discuss it to decide the default value.
Thanks for your suggestion.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 29, 2017, 8:58 a.m. UTC | #29
On Wed, Nov 29, 2017 at 9:39 AM, Greentime Hu <green.hu@gmail.com> wrote:
> 2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>>> +config CPU_CACHE_NONALIASING
>>> +       bool "Non-aliasing cache"
>>> +       help
>>> +         If this CPU is using VIPT data cache and its cache way size is larger
>>> +         than page size, say N. If it is using PIPT data cache, say Y.
>>> +
>>> +         If unsure, say Y.
>>
>> Can you determine this from the CPU type?
>
> There is no cpu register to determine it. It also depeneds on page
> size and way size however page size is configurable by software.
> These codes are determined at compile time will be benefit to code
> size and performance.
> IMHO, I think it would be better to be determined here.

I meant determining it at compile time from other Kconfig symbols,
if that's possible. Do the CPU cores each have a fixed way-size?
If they do, it could be done like

menu "CPU selection"

config CPU_N15
      bool "AndesCore N15"
      select CPU_CACHE_NONALIASING

config CPU_N13
      bool "AndesCore N15"
      select CPU_CACHE_NONALIASING if PAGE_SIZE_16K

...

endmenu

and then you can use the same CPU_... symbols to make other decisions
as well, e.g. CPU specific compiler optimizations.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Nov. 29, 2017, 9:10 a.m. UTC | #30
Hi Arnd,

On Wed, Nov 29, 2017 at 9:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Nov 29, 2017 at 9:39 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>>>> +config CPU_CACHE_NONALIASING
>>>> +       bool "Non-aliasing cache"
>>>> +       help
>>>> +         If this CPU is using VIPT data cache and its cache way size is larger
>>>> +         than page size, say N. If it is using PIPT data cache, say Y.
>>>> +
>>>> +         If unsure, say Y.
>>>
>>> Can you determine this from the CPU type?
>>
>> There is no cpu register to determine it. It also depeneds on page
>> size and way size however page size is configurable by software.
>> These codes are determined at compile time will be benefit to code
>> size and performance.
>> IMHO, I think it would be better to be determined here.
>
> I meant determining it at compile time from other Kconfig symbols,
> if that's possible. Do the CPU cores each have a fixed way-size?
> If they do, it could be done like
>
> menu "CPU selection"
>
> config CPU_N15
>       bool "AndesCore N15"
>       select CPU_CACHE_NONALIASING
>
> config CPU_N13
>       bool "AndesCore N15"
>       select CPU_CACHE_NONALIASING if PAGE_SIZE_16K
>
> ...
>
> endmenu
>
> and then you can use the same CPU_... symbols to make other decisions
> as well, e.g. CPU specific compiler optimizations.

Do you want to support multiple CPU types in a single kernel image
(I see no "choice" statement above)?
If yes, you may have a mix of aliasing and non-aliasing caches, so
you may want to invert the logic, and select CPU_CACHE_ALIASING
instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 29, 2017, 9:25 a.m. UTC | #31
On Wed, Nov 29, 2017 at 10:10 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Arnd,
>
> On Wed, Nov 29, 2017 at 9:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Nov 29, 2017 at 9:39 AM, Greentime Hu <green.hu@gmail.com> wrote:
>>> 2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>>>>> +config CPU_CACHE_NONALIASING
>>>>> +       bool "Non-aliasing cache"
>>>>> +       help
>>>>> +         If this CPU is using VIPT data cache and its cache way size is larger
>>>>> +         than page size, say N. If it is using PIPT data cache, say Y.
>>>>> +
>>>>> +         If unsure, say Y.
>>>>
>>>> Can you determine this from the CPU type?
>>>
>>> There is no cpu register to determine it. It also depeneds on page
>>> size and way size however page size is configurable by software.
>>> These codes are determined at compile time will be benefit to code
>>> size and performance.
>>> IMHO, I think it would be better to be determined here.
>>
>> I meant determining it at compile time from other Kconfig symbols,
>> if that's possible. Do the CPU cores each have a fixed way-size?
>> If they do, it could be done like
>>
>> menu "CPU selection"
>>
>> config CPU_N15
>>       bool "AndesCore N15"
>>       select CPU_CACHE_NONALIASING
>>
>> config CPU_N13
>>       bool "AndesCore N15"
>>       select CPU_CACHE_NONALIASING if PAGE_SIZE_16K
>>
>> ...
>>
>> endmenu
>>
>> and then you can use the same CPU_... symbols to make other decisions
>> as well, e.g. CPU specific compiler optimizations.
>
> Do you want to support multiple CPU types in a single kernel image
> (I see no "choice" statement above)?
> If yes, you may have a mix of aliasing and non-aliasing caches, so
> you may want to invert the logic, and select CPU_CACHE_ALIASING
> instead.

Right, my mistake.

         Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 11:39 a.m. UTC | #32
2017-11-29 17:25 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, Nov 29, 2017 at 10:10 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> Hi Arnd,
>>
>> On Wed, Nov 29, 2017 at 9:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Wed, Nov 29, 2017 at 9:39 AM, Greentime Hu <green.hu@gmail.com> wrote:
>>>> 2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>>>>>> +config CPU_CACHE_NONALIASING
>>>>>> +       bool "Non-aliasing cache"
>>>>>> +       help
>>>>>> +         If this CPU is using VIPT data cache and its cache way size is larger
>>>>>> +         than page size, say N. If it is using PIPT data cache, say Y.
>>>>>> +
>>>>>> +         If unsure, say Y.
>>>>>
>>>>> Can you determine this from the CPU type?
>>>>
>>>> There is no cpu register to determine it. It also depeneds on page
>>>> size and way size however page size is configurable by software.
>>>> These codes are determined at compile time will be benefit to code
>>>> size and performance.
>>>> IMHO, I think it would be better to be determined here.
>>>
>>> I meant determining it at compile time from other Kconfig symbols,
>>> if that's possible. Do the CPU cores each have a fixed way-size?
>>> If they do, it could be done like
>>>
>>> menu "CPU selection"
>>>
>>> config CPU_N15
>>>       bool "AndesCore N15"
>>>       select CPU_CACHE_NONALIASING
>>>
>>> config CPU_N13
>>>       bool "AndesCore N15"
>>>       select CPU_CACHE_NONALIASING if PAGE_SIZE_16K
>>>
>>> ...
>>>
>>> endmenu
>>>
>>> and then you can use the same CPU_... symbols to make other decisions
>>> as well, e.g. CPU specific compiler optimizations.
>>
>> Do you want to support multiple CPU types in a single kernel image
>> (I see no "choice" statement above)?
>> If yes, you may have a mix of aliasing and non-aliasing caches, so
>> you may want to invert the logic, and select CPU_CACHE_ALIASING
>> instead.
>
> Right, my mistake.
>

Thanks to Arnd and Geert!

How about this?

choice
        prompt "CPU type"
        default CPU_N13
config CPU_N15
        bool "AndesCore N15"
        select CPU_CACHE_NONALIASING
config CPU_N13
        bool "AndesCore N13"
        select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
config CPU_N10
        bool "AndesCore N10"
        select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
config CPU_D15
        bool "AndesCore D15"
        select CPU_CACHE_NONALIASING
        select HWZOL
config CPU_D10
        bool "AndesCore D10"
        select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
endchoice
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 11:53 a.m. UTC | #33
Hi, Arnd:

2017-11-27 22:33 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> +
>> +#define L2C_R_REG(offset)               __raw_readl(atl2c_base + offset)
>> +#define L2C_W_REG(offset, value)        __raw_writel(value, atl2c_base + offset)
>
> __raw_readl() is generally not endian-safe, and might  not have the barriers you
> require here. Could you use readl/writel here, and only fall back to
> readl_relaxed()/writel_relaxed() when you absolutely must avoid the barriers?

Thanks for your suggestions.
We will changed it to readl/writel

>> diff --git a/arch/nds32/kernel/atl2c.c b/arch/nds32/kernel/atl2c.c
>> new file mode 100644
>> index 0000000..dd87fc9
>> --- /dev/null
>> +++ b/arch/nds32/kernel/atl2c.c
>> +#include <linux/compiler.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/of_platform.h>
>> +#include <asm/l2_cache.h>
>
> If this is the only file that includes asm/l2_cache.h, then I'd simply
> move the entire
> contents in here, rather than having a separate file in the global namespace.
>

arch/nds32/mm/proc.c also includes this file so I will keep it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 29, 2017, 11:57 a.m. UTC | #34
On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
> 2017-11-29 17:25 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, Nov 29, 2017 at 10:10 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> Hi Arnd,
>>>
>>> On Wed, Nov 29, 2017 at 9:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Wed, Nov 29, 2017 at 9:39 AM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>> 2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>>>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>>>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>>>>>>> +config CPU_CACHE_NONALIASING
>>>>>>> +       bool "Non-aliasing cache"
>>>>>>> +       help
>>>>>>> +         If this CPU is using VIPT data cache and its cache way size is larger
>>>>>>> +         than page size, say N. If it is using PIPT data cache, say Y.
>>>>>>> +
>>>>>>> +         If unsure, say Y.
>>>>>>
>>>>>> Can you determine this from the CPU type?
>>>>>
>>>>> There is no cpu register to determine it. It also depeneds on page
>>>>> size and way size however page size is configurable by software.
>>>>> These codes are determined at compile time will be benefit to code
>>>>> size and performance.
>>>>> IMHO, I think it would be better to be determined here.
>>>>
>>>> I meant determining it at compile time from other Kconfig symbols,
>>>> if that's possible. Do the CPU cores each have a fixed way-size?
>>>> If they do, it could be done like
>>>>
>>>> menu "CPU selection"
>>>>
>>>> config CPU_N15
>>>>       bool "AndesCore N15"
>>>>       select CPU_CACHE_NONALIASING
>>>>
>>>> config CPU_N13
>>>>       bool "AndesCore N15"
>>>>       select CPU_CACHE_NONALIASING if PAGE_SIZE_16K
>>>>
>>>> ...
>>>>
>>>> endmenu
>>>>
>>>> and then you can use the same CPU_... symbols to make other decisions
>>>> as well, e.g. CPU specific compiler optimizations.
>>>
>>> Do you want to support multiple CPU types in a single kernel image
>>> (I see no "choice" statement above)?
>>> If yes, you may have a mix of aliasing and non-aliasing caches, so
>>> you may want to invert the logic, and select CPU_CACHE_ALIASING
>>> instead.
>>
>> Right, my mistake.
>>
>
> Thanks to Arnd and Geert!
>
> How about this?
>
> choice
>         prompt "CPU type"
>         default CPU_N13
> config CPU_N15
>         bool "AndesCore N15"
>         select CPU_CACHE_NONALIASING
> config CPU_N13
>         bool "AndesCore N13"
>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
> config CPU_N10
>         bool "AndesCore N10"
>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
> config CPU_D15
>         bool "AndesCore D15"
>         select CPU_CACHE_NONALIASING
>         select HWZOL
> config CPU_D10
>         bool "AndesCore D10"
>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
> endchoice

With a 'choice' statement this works, but I would consider that
suboptimal for another reason: now you cannot build a kernel that
works on e.g. both N13 and N15.

This is what we had on ARM a long time ago and on MIPS not so long
ago, but it's really a burden for testing and distribution once you get
support for more than a handful of machines supported in the mainline
kernel: If each CPU core is mutually incompatible with the other ones,
this means you likely end up having one defconfig per CPU core,
or even one defconfig per SoC or board.

I would always try to get the largest amount of hardware to work
in the same kernel concurrently.

One way of of this would be to define the "CPU type" as the minimum
supported CPU, e.g. selecting D15 would result in a kernel that
only works on D15, while selecting N15 would work on both N15 and
D15, and selecting D10 would work on both D10 and D15.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 2:10 p.m. UTC | #35
2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-29 17:25 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Nov 29, 2017 at 10:10 AM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>> Hi Arnd,
>>>>
>>>> On Wed, Nov 29, 2017 at 9:58 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Wed, Nov 29, 2017 at 9:39 AM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>>> 2017-11-27 22:21 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>>>>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>>>>> diff --git a/arch/nds32/Kconfig.cpu b/arch/nds32/Kconfig.cpu
>>>>>>>> +config CPU_CACHE_NONALIASING
>>>>>>>> +       bool "Non-aliasing cache"
>>>>>>>> +       help
>>>>>>>> +         If this CPU is using VIPT data cache and its cache way size is larger
>>>>>>>> +         than page size, say N. If it is using PIPT data cache, say Y.
>>>>>>>> +
>>>>>>>> +         If unsure, say Y.
>>>>>>>
>>>>>>> Can you determine this from the CPU type?
>>>>>>
>>>>>> There is no cpu register to determine it. It also depeneds on page
>>>>>> size and way size however page size is configurable by software.
>>>>>> These codes are determined at compile time will be benefit to code
>>>>>> size and performance.
>>>>>> IMHO, I think it would be better to be determined here.
>>>>>
>>>>> I meant determining it at compile time from other Kconfig symbols,
>>>>> if that's possible. Do the CPU cores each have a fixed way-size?
>>>>> If they do, it could be done like
>>>>>
>>>>> menu "CPU selection"
>>>>>
>>>>> config CPU_N15
>>>>>       bool "AndesCore N15"
>>>>>       select CPU_CACHE_NONALIASING
>>>>>
>>>>> config CPU_N13
>>>>>       bool "AndesCore N15"
>>>>>       select CPU_CACHE_NONALIASING if PAGE_SIZE_16K
>>>>>
>>>>> ...
>>>>>
>>>>> endmenu
>>>>>
>>>>> and then you can use the same CPU_... symbols to make other decisions
>>>>> as well, e.g. CPU specific compiler optimizations.
>>>>
>>>> Do you want to support multiple CPU types in a single kernel image
>>>> (I see no "choice" statement above)?
>>>> If yes, you may have a mix of aliasing and non-aliasing caches, so
>>>> you may want to invert the logic, and select CPU_CACHE_ALIASING
>>>> instead.
>>>
>>> Right, my mistake.
>>>
>>
>> Thanks to Arnd and Geert!
>>
>> How about this?
>>
>> choice
>>         prompt "CPU type"
>>         default CPU_N13
>> config CPU_N15
>>         bool "AndesCore N15"
>>         select CPU_CACHE_NONALIASING
>> config CPU_N13
>>         bool "AndesCore N13"
>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>> config CPU_N10
>>         bool "AndesCore N10"
>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>> config CPU_D15
>>         bool "AndesCore D15"
>>         select CPU_CACHE_NONALIASING
>>         select HWZOL
>> config CPU_D10
>>         bool "AndesCore D10"
>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>> endchoice
>
> With a 'choice' statement this works, but I would consider that
> suboptimal for another reason: now you cannot build a kernel that
> works on e.g. both N13 and N15.
>
> This is what we had on ARM a long time ago and on MIPS not so long
> ago, but it's really a burden for testing and distribution once you get
> support for more than a handful of machines supported in the mainline
> kernel: If each CPU core is mutually incompatible with the other ones,
> this means you likely end up having one defconfig per CPU core,
> or even one defconfig per SoC or board.
>
> I would always try to get the largest amount of hardware to work
> in the same kernel concurrently.
>
> One way of of this would be to define the "CPU type" as the minimum
> supported CPU, e.g. selecting D15 would result in a kernel that
> only works on D15, while selecting N15 would work on both N15 and
> D15, and selecting D10 would work on both D10 and D15.
>

Hi, Arnd:

Maybe we should keep the original implementation for this reason.
The default value of CPU_CACHE_NONALIASING and ANDES_PAGE_SIZE_8KB is
available for all CPU types for now.
User can use these configs built kernel to boot on almost all nds32 CPUs.

It might be a little bit weird if we config CPU_N10 but run on a N13 CPU.
This might confuse our users.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 29, 2017, 3:23 p.m. UTC | #36
Hi, Marc:

2017-11-28 17:37 GMT+08:00 Marc Zyngier <marc.zyngier@arm.com>:
> On 27/11/17 12:28, Greentime Hu wrote:
>> +static void ativic32_ack_irq(struct irq_data *data)
>> +{
>> +     __nds32__mtsr_dsb(1 << data->hwirq, NDS32_SR_INT_PEND2);
>
> Consider writing (1 << data->hwirq) as BIT(data->hwirq).

Thanks for this suggestion. I will modify it in the next version patch.

>> +}
>> +
>> +static void ativic32_mask_irq(struct irq_data *data)
>> +{
>> +     unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
>> +     __nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)), NDS32_SR_INT_MASK2);
>
> Same here.

Thanks for this suggestion. I will modify it in the next version patch.

>> +}
>> +
>> +static void ativic32_mask_ack_irq(struct irq_data *data)
>> +{
>> +     unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
>> +     __nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)), NDS32_SR_INT_MASK2);
>> +     __nds32__mtsr_dsb((1 << data->hwirq), NDS32_SR_INT_PEND2);
>
> This is effectively MASK+ACK, so you're better off just writing it as
> such. And since there is no advantage in your implementation in having
> MASK_ACK over MASK+ACK, I suggest you remove this function completely,
> and rely on the core code which will call them in sequence.

I think mask_ack is still better than mask + ack because we don't need
to do two function call.
We can save a prologue and a epilogue. It will benefit interrupt latency.

>> +
>> +}
>> +
>> +static void ativic32_unmask_irq(struct irq_data *data)
>> +{
>> +     unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
>> +     __nds32__mtsr_dsb(int_mask2 | (1 << data->hwirq), NDS32_SR_INT_MASK2);
>
> Same BIT() here.
>
Thanks for this suggestion. I will modify it in the next version patch.

>> +}
>> +
>> +static struct irq_chip ativic32_chip = {
>> +     .name = "ativic32",
>> +     .irq_ack = ativic32_ack_irq,
>> +     .irq_mask = ativic32_mask_irq,
>> +     .irq_mask_ack = ativic32_mask_ack_irq,
>> +     .irq_unmask = ativic32_unmask_irq,
>> +};
>> +
>> +static unsigned int __initdata nivic_map[6] = { 6, 2, 10, 16, 24, 32 };
>> +
>> +static struct irq_domain *root_domain;
>> +static int ativic32_irq_domain_map(struct irq_domain *id, unsigned int virq,
>> +                               irq_hw_number_t hw)
>> +{
>> +
>> +     unsigned long int_trigger_type;
>> +     int_trigger_type = __nds32__mfsr(NDS32_SR_INT_TRIGGER);
>> +     if (int_trigger_type & (1 << hw))
>
> And here.
>
Thanks for this suggestion. I will modify it in the next version patch.

>> +             irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
>> +     else
>> +             irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);
>
> Since you do not express the trigger in DT, you need to tell the core
> about it by calling irqd_set_trigger_type() with the right setting.
>

Since the comments say so, I will add ativic32_set_type() for irq_set_type()
in the next version patch.

/*
 * Must only be called inside irq_chip.irq_set_type() functions.
 */
static inline void irqd_set_trigger_type(struct irq_data *d, u32 type)
{
        __irqd_to_state(d) &= ~IRQD_TRIGGER_MASK;
        __irqd_to_state(d) |= type & IRQD_TRIGGER_MASK;
}

It will be like this.
static int ativic32_set_type(struct irq_data *data, unsigned int flow_type)
{
        irqd_set_trigger_type(data, flow_type);
        return IRQ_SET_MASK_OK;
}

>> +
>> +     return 0;
>> +}
>> +
>> +static struct irq_domain_ops ativic32_ops = {
>> +     .map = ativic32_irq_domain_map,
>> +     .xlate = irq_domain_xlate_onecell
>> +};
>> +
>> +static int get_intr_src(void)
>
> I'm not sure "int" is the best return type here. I suspect something
> unsigned would be preferable, or even the irq_hw_number_t type.

Thanks for this suggestion. I will modify it in the next version patch.

>> +{
>> +     return ((__nds32__mfsr(NDS32_SR_ITYPE)&ITYPE_mskVECTOR) >> ITYPE_offVECTOR)
>
> Spaces around '&'.
>
Thanks for this suggestion. I will modify it in the next version patch.

>> +             - NDS32_VECTOR_offINTERRUPT;
>> +}
>> +
>> +asmlinkage void asm_do_IRQ(struct pt_regs *regs)
>> +{
>> +     int hwirq = get_intr_src();
>
> irq_hw_number_t.
>
Thanks for this suggestion. I will modify it in the next version patch.

>> +     handle_domain_irq(root_domain, hwirq, regs);
>> +}
>> +
>> +int __init ativic32_init_irq(struct device_node *node, struct device_node *parent)
>> +{
>> +     unsigned long int_vec_base, nivic;
>> +
>> +     if (WARN(parent, "non-root ativic32 are not supported"))
>> +             return -EINVAL;
>> +
>> +     int_vec_base = __nds32__mfsr(NDS32_SR_IVB);
>> +
>> +     if (((int_vec_base & IVB_mskIVIC_VER) >> IVB_offIVIC_VER) == 0)
>> +             panic("Unable to use atcivic32 for this cpu.\n");
>> +
>> +     nivic = (int_vec_base & IVB_mskNIVIC) >> IVB_offNIVIC;
>> +     if (nivic >= (sizeof nivic_map / sizeof nivic_map[0]))
>
> This should be:
>         if (nivic >= ARRAY_SIZE(NIVIC_MAP))
>
Thanks for this suggestion. I will modify it in the next version patch.

>> +             panic("The number of input for ativic32 is not supported.\n");
>> +
>> +     nivic = nivic_map[nivic];
>
> I'd rather you use another variable (nr_ints?).
>
Thanks for this suggestion. I will modify it in the next version patch.

>> +
>> +     root_domain = irq_domain_add_linear(node, nivic,
>> +                     &ativic32_ops, NULL);
>> +
>> +     if (!root_domain)
>> +             panic("%s: unable to create IRQ domain\n", node->full_name);
>> +
>> +     return 0;
>> +}
>> +IRQCHIP_DECLARE(ativic32, "andestech,ativic32", ativic32_init_irq);
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 29, 2017, 8:27 p.m. UTC | #37
On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>
>>> How about this?
>>>
>>> choice
>>>         prompt "CPU type"
>>>         default CPU_N13
>>> config CPU_N15
>>>         bool "AndesCore N15"
>>>         select CPU_CACHE_NONALIASING
>>> config CPU_N13
>>>         bool "AndesCore N13"
>>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>>> config CPU_N10
>>>         bool "AndesCore N10"
>>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>>> config CPU_D15
>>>         bool "AndesCore D15"
>>>         select CPU_CACHE_NONALIASING
>>>         select HWZOL
>>> config CPU_D10
>>>         bool "AndesCore D10"
>>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>>> endchoice
>>
>> With a 'choice' statement this works, but I would consider that
>> suboptimal for another reason: now you cannot build a kernel that
>> works on e.g. both N13 and N15.
>>
>> This is what we had on ARM a long time ago and on MIPS not so long
>> ago, but it's really a burden for testing and distribution once you get
>> support for more than a handful of machines supported in the mainline
>> kernel: If each CPU core is mutually incompatible with the other ones,
>> this means you likely end up having one defconfig per CPU core,
>> or even one defconfig per SoC or board.
>>
>> I would always try to get the largest amount of hardware to work
>> in the same kernel concurrently.
>>
>> One way of of this would be to define the "CPU type" as the minimum
>> supported CPU, e.g. selecting D15 would result in a kernel that
>> only works on D15, while selecting N15 would work on both N15 and
>> D15, and selecting D10 would work on both D10 and D15.
>>
>
> Hi, Arnd:
>
> Maybe we should keep the original implementation for this reason.
> The default value of CPU_CACHE_NONALIASING and ANDES_PAGE_SIZE_8KB is
> available for all CPU types for now.
> User can use these configs built kernel to boot on almost all nds32 CPUs.
>
> It might be a little bit weird if we config CPU_N10 but run on a N13 CPU.
> This might confuse our users.

I think it really depends on how much flexibility you want to give to users.
The way I suggested first was to allow selecting an arbitrary combination
of CPUs, and then let Kconfig derive the correct set of optimization flags
and other options that work with those. That is probably the easiest for
the users, but can be tricky to get right for all combinations.

When you put them in a sorted list like I mentioned for simplicity, you
could reduce the confusion by naming them differently, e.g.
CONFIG_CPU_N10_OR_NEWER.

Having only the CPU_CACHE_NONALIASING option is fine if you
never need to make any other decisions based on the CPU core
type, but then the help text should describe specifically which cases
are affected (N10/N13/D13 with 4K page size), and you can decide to
hide the option and make it always-on when using 8K page size.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 30, 2017, 5:48 a.m. UTC | #38
2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>>
>>>> How about this?
>>>>
>>>> choice
>>>>         prompt "CPU type"
>>>>         default CPU_N13
>>>> config CPU_N15
>>>>         bool "AndesCore N15"
>>>>         select CPU_CACHE_NONALIASING
>>>> config CPU_N13
>>>>         bool "AndesCore N13"
>>>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>>>> config CPU_N10
>>>>         bool "AndesCore N10"
>>>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>>>> config CPU_D15
>>>>         bool "AndesCore D15"
>>>>         select CPU_CACHE_NONALIASING
>>>>         select HWZOL
>>>> config CPU_D10
>>>>         bool "AndesCore D10"
>>>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>>>> endchoice
>>>
>>> With a 'choice' statement this works, but I would consider that
>>> suboptimal for another reason: now you cannot build a kernel that
>>> works on e.g. both N13 and N15.
>>>
>>> This is what we had on ARM a long time ago and on MIPS not so long
>>> ago, but it's really a burden for testing and distribution once you get
>>> support for more than a handful of machines supported in the mainline
>>> kernel: If each CPU core is mutually incompatible with the other ones,
>>> this means you likely end up having one defconfig per CPU core,
>>> or even one defconfig per SoC or board.
>>>
>>> I would always try to get the largest amount of hardware to work
>>> in the same kernel concurrently.
>>>
>>> One way of of this would be to define the "CPU type" as the minimum
>>> supported CPU, e.g. selecting D15 would result in a kernel that
>>> only works on D15, while selecting N15 would work on both N15 and
>>> D15, and selecting D10 would work on both D10 and D15.
>>>
>>
>> Hi, Arnd:
>>
>> Maybe we should keep the original implementation for this reason.
>> The default value of CPU_CACHE_NONALIASING and ANDES_PAGE_SIZE_8KB is
>> available for all CPU types for now.
>> User can use these configs built kernel to boot on almost all nds32 CPUs.
>>
>> It might be a little bit weird if we config CPU_N10 but run on a N13 CPU.
>> This might confuse our users.
>
> I think it really depends on how much flexibility you want to give to users.
> The way I suggested first was to allow selecting an arbitrary combination
> of CPUs, and then let Kconfig derive the correct set of optimization flags
> and other options that work with those. That is probably the easiest for
> the users, but can be tricky to get right for all combinations.
>
> When you put them in a sorted list like I mentioned for simplicity, you
> could reduce the confusion by naming them differently, e.g.
> CONFIG_CPU_N10_OR_NEWER.
>
> Having only the CPU_CACHE_NONALIASING option is fine if you
> never need to make any other decisions based on the CPU core
> type, but then the help text should describe specifically which cases
> are affected (N10/N13/D13 with 4K page size), and you can decide to
> hide the option and make it always-on when using 8K page size.
>
>        Arnd


Hi, Arnd:

I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
It will be implemented like this.

config HWZOL
        bool "hardware zero overhead loop support"
        depends on CPU_D10 || CPU_D15
        default n
        help
          A set of Zero-Overhead Loop mechanism is provided to reduce the
          instruction fetch and execution overhead of loop-control instructions.
          It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
          You don't need to save these registers if you can make sure your user
          program doesn't use these registers.

          If unsure, say N.

config CPU_CACHE_NONALIASING
        bool "Non-aliasing cache"
        depends on !CPU_N10 && !CPU_D10
        default n
        help
          If this CPU is using VIPT data cache and its cache way size is larger
          than page size, say N. If it is using PIPT data cache, say Y.

          If unsure, say N.

choice
        prompt "CPU type"
        default CPU_V3
config CPU_N15
        bool "AndesCore N15"
        select CPU_CACHE_NONALIASING
config CPU_N13
        bool "AndesCore N13"
        select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
config CPU_N10
        bool "AndesCore N10"
config CPU_D15
        bool "AndesCore D15"
        select CPU_CACHE_NONALIASING
config CPU_D10
        bool "AndesCore D10"
config CPU_V3
        bool "AndesCore v3 compatible"
        select ANDES_PAGE_SIZE_4KB
endchoice
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven Nov. 30, 2017, 7:52 a.m. UTC | #39
On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu@gmail.com> wrote:
> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>> On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
> It will be implemented like this.
>
> config HWZOL
>         bool "hardware zero overhead loop support"
>         depends on CPU_D10 || CPU_D15
>         default n
>         help
>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>           instruction fetch and execution overhead of loop-control instructions.
>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>           You don't need to save these registers if you can make sure your user
>           program doesn't use these registers.
>
>           If unsure, say N.
>
> config CPU_CACHE_NONALIASING
>         bool "Non-aliasing cache"
>         depends on !CPU_N10 && !CPU_D10
>         default n
>         help
>           If this CPU is using VIPT data cache and its cache way size is larger
>           than page size, say N. If it is using PIPT data cache, say Y.
>
>           If unsure, say N.

I still think it will be easier to revert the logic, and have
CPU_CACHE_ALIASING.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 30, 2017, 9:29 a.m. UTC | #40
2017-11-30 15:52 GMT+08:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>>> On Wed, Nov 29, 2017 at 12:39 PM, Greentime Hu <green.hu@gmail.com> wrote:
>> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
>> It will be implemented like this.
>>
>> config HWZOL
>>         bool "hardware zero overhead loop support"
>>         depends on CPU_D10 || CPU_D15
>>         default n
>>         help
>>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>>           instruction fetch and execution overhead of loop-control instructions.
>>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>>           You don't need to save these registers if you can make sure your user
>>           program doesn't use these registers.
>>
>>           If unsure, say N.
>>
>> config CPU_CACHE_NONALIASING
>>         bool "Non-aliasing cache"
>>         depends on !CPU_N10 && !CPU_D10
>>         default n
>>         help
>>           If this CPU is using VIPT data cache and its cache way size is larger
>>           than page size, say N. If it is using PIPT data cache, say Y.
>>
>>           If unsure, say N.
>
> I still think it will be easier to revert the logic, and have
> CPU_CACHE_ALIASING.
>

Thanks Geert

I will implement it like this.

config HWZOL
        bool "hardware zero overhead loop support"
        depends on CPU_D10 || CPU_D15
        default n
        help
          A set of Zero-Overhead Loop mechanism is provided to reduce the
          instruction fetch and execution overhead of loop-control instructions.
          It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
          You don't need to save these registers if you can make sure your user
          program doesn't use these registers.

          If unsure, say N.

config CPU_CACHE_ALIASING
        bool "Aliasing cache"
        depends on CPU_N10 || CPU_D10 || CPU_N13 || CPU_V3
        default y
        help
          If this CPU is using VIPT data cache and its cache way size is larger
          than page size, say Y. If it is using PIPT data cache, say N.

          If unsure, say Y.

choice
        prompt "CPU type"
        default CPU_V3
config CPU_N15
        bool "AndesCore N15"
config CPU_N13
        bool "AndesCore N13"
        select CPU_CACHE_ALIASING if ANDES_PAGE_SIZE_4KB
config CPU_N10
        bool "AndesCore N10"
        select CPU_CACHE_ALIASING
config CPU_D15
        bool "AndesCore D15"
config CPU_D10
        bool "AndesCore D10"
        select CPU_CACHE_ALIASING
config CPU_V3
        bool "AndesCore v3 compatible"
        select ANDES_PAGE_SIZE_8KB
endchoice
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 30, 2017, 9:30 a.m. UTC | #41
On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu@gmail.com> wrote:
> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:

>> When you put them in a sorted list like I mentioned for simplicity, you
>> could reduce the confusion by naming them differently, e.g.
>> CONFIG_CPU_N10_OR_NEWER.
>>
>> Having only the CPU_CACHE_NONALIASING option is fine if you
>> never need to make any other decisions based on the CPU core
>> type, but then the help text should describe specifically which cases
>> are affected (N10/N13/D13 with 4K page size), and you can decide to
>> hide the option and make it always-on when using 8K page size.
>>
>>        Arnd
>
>
> Hi, Arnd:
>
> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
> It will be implemented like this.

I think I'm still a bit confused about the relation between CPU cores
and architecture levels. Is it correct to say that there are orthogonal,
and that you can have e.g. an N10 core implementing either nds32v2
or nds32v3?

There is nothing wrong with that of course, it's just not what I
expected from having worked with other architectures.

I also see that GCC has no pipeline specific optimizations for
specific cores, it just understands the differences between the
architecture levels, so at least today there is way to pass e.g.
"-march=nds32v2 -mtune=d15" to generate code that would
work on both v2 and v3 but be optimized for d15.

> config HWZOL
>         bool "hardware zero overhead loop support"
>         depends on CPU_D10 || CPU_D15
>         default n
>         help
>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>           instruction fetch and execution overhead of loop-control instructions.
>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>           You don't need to save these registers if you can make sure your user
>           program doesn't use these registers.
>
>           If unsure, say N.
>
> config CPU_CACHE_NONALIASING
>         bool "Non-aliasing cache"
>         depends on !CPU_N10 && !CPU_D10
>         default n
>         help
>           If this CPU is using VIPT data cache and its cache way size is larger
>           than page size, say N. If it is using PIPT data cache, say Y.
>
>           If unsure, say N.

This looks ok, yes, but as Geert said, it would seem more intuitive to
write it as

config CPU_CACHE_ALIASING
         bool "Aliasing VIPT cache"
         depends on CPU_N10 || CPU_D10

> choice
>         prompt "CPU type"
>         default CPU_V3
> config CPU_N15
>         bool "AndesCore N15"
>         select CPU_CACHE_NONALIASING
> config CPU_N13
>         bool "AndesCore N13"
>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
> config CPU_N10
>         bool "AndesCore N10"
> config CPU_D15
>         bool "AndesCore D15"
>         select CPU_CACHE_NONALIASING
> config CPU_D10
>         bool "AndesCore D10"
> config CPU_V3
>         bool "AndesCore v3 compatible"
>         select ANDES_PAGE_SIZE_4KB
> endchoice

Two points here:

- Generally you should not mix 'select' and 'depends on' like this.
  Either you make the cache aliasing a user visible option that
  uses 'depends on' with a combination of CPU cores, or you
  make it a hidden option (with no string after the "bool" keyword)
  that always gets selected from the per-cpu options.

- There is a  little-known trick with choice statements that allows
  you to use 'tristate' instead of 'bool' in the choice. In that case,
  you can enable multiple options together as long as all of them
  are 'm'.

         Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Nov. 30, 2017, 10:01 a.m. UTC | #42
2017-11-30 17:30 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Thu, Nov 30, 2017 at 6:48 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> 2017-11-30 4:27 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Nov 29, 2017 at 3:10 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>>> 2017-11-29 19:57 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>
>>> When you put them in a sorted list like I mentioned for simplicity, you
>>> could reduce the confusion by naming them differently, e.g.
>>> CONFIG_CPU_N10_OR_NEWER.
>>>
>>> Having only the CPU_CACHE_NONALIASING option is fine if you
>>> never need to make any other decisions based on the CPU core
>>> type, but then the help text should describe specifically which cases
>>> are affected (N10/N13/D13 with 4K page size), and you can decide to
>>> hide the option and make it always-on when using 8K page size.
>>>
>>>        Arnd
>>
>>
>> Hi, Arnd:
>>
>> I think I can use this name "CPU_V3" for all nds32 v3 compatible cpu.
>> It will be implemented like this.
>
> I think I'm still a bit confused about the relation between CPU cores
> and architecture levels. Is it correct to say that there are orthogonal,
> and that you can have e.g. an N10 core implementing either nds32v2
> or nds32v3?
>

Yup, we did having N10 cores are implementing either nds32v3 or
nds32v2, but nds32v2 are not used anymore.
We can assume every nds32 cores are v3.

> There is nothing wrong with that of course, it's just not what I
> expected from having worked with other architectures.
>
> I also see that GCC has no pipeline specific optimizations for
> specific cores, it just understands the differences between the
> architecture levels, so at least today there is way to pass e.g.
> "-march=nds32v2 -mtune=d15" to generate code that would
> work on both v2 and v3 but be optimized for d15.

Thanks. We will work on that.

>> config HWZOL
>>         bool "hardware zero overhead loop support"
>>         depends on CPU_D10 || CPU_D15
>>         default n
>>         help
>>           A set of Zero-Overhead Loop mechanism is provided to reduce the
>>           instruction fetch and execution overhead of loop-control instructions.
>>           It will save 3 registers($LB, $LC, $LE) for context saving if say Y.
>>           You don't need to save these registers if you can make sure your user
>>           program doesn't use these registers.
>>
>>           If unsure, say N.
>>
>> config CPU_CACHE_NONALIASING
>>         bool "Non-aliasing cache"
>>         depends on !CPU_N10 && !CPU_D10
>>         default n
>>         help
>>           If this CPU is using VIPT data cache and its cache way size is larger
>>           than page size, say N. If it is using PIPT data cache, say Y.
>>
>>           If unsure, say N.
>
> This looks ok, yes, but as Geert said, it would seem more intuitive to
> write it as
>
> config CPU_CACHE_ALIASING
>          bool "Aliasing VIPT cache"
>          depends on CPU_N10 || CPU_D10
>
>> choice
>>         prompt "CPU type"
>>         default CPU_V3
>> config CPU_N15
>>         bool "AndesCore N15"
>>         select CPU_CACHE_NONALIASING
>> config CPU_N13
>>         bool "AndesCore N13"
>>         select CPU_CACHE_NONALIASING if ANDES_PAGE_SIZE_8KB
>> config CPU_N10
>>         bool "AndesCore N10"
>> config CPU_D15
>>         bool "AndesCore D15"
>>         select CPU_CACHE_NONALIASING
>> config CPU_D10
>>         bool "AndesCore D10"
>> config CPU_V3
>>         bool "AndesCore v3 compatible"
>>         select ANDES_PAGE_SIZE_4KB
>> endchoice
>
> Two points here:
>
> - Generally you should not mix 'select' and 'depends on' like this.
>   Either you make the cache aliasing a user visible option that
>   uses 'depends on' with a combination of CPU cores, or you
>   make it a hidden option (with no string after the "bool" keyword)
>   that always gets selected from the per-cpu options.
>
> - There is a  little-known trick with choice statements that allows
>   you to use 'tristate' instead of 'bool' in the choice. In that case,
>   you can enable multiple options together as long as all of them
>   are 'm'.
>
>          Arnd


Thanks.
CPU_CACHE_ALIASING is more intuitive. I will apply it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Nov. 30, 2017, 10:57 a.m. UTC | #43
On Wed, Nov 29 2017 at 11:23:34 pm GMT, Greentime Hu <green.hu@gmail.com> wrote:

Hi Greentime,

>>> +}
>>> +
>>> +static void ativic32_mask_ack_irq(struct irq_data *data)
>>> +{
>>> +     unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
>>> + __nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)),
>>> NDS32_SR_INT_MASK2);
>>> +     __nds32__mtsr_dsb((1 << data->hwirq), NDS32_SR_INT_PEND2);
>>
>> This is effectively MASK+ACK, so you're better off just writing it as
>> such. And since there is no advantage in your implementation in having
>> MASK_ACK over MASK+ACK, I suggest you remove this function completely,
>> and rely on the core code which will call them in sequence.
>
> I think mask_ack is still better than mask + ack because we don't need
> to do two function call.
> We can save a prologue and a epilogue. It will benefit interrupt latency.

Can you actually measure this? Your CPU would have to be extremely slow
if you could see the impact of an extra function call on interrupt
latency. From a maintenance perspective, this isn't very nice (it is
effectively code duplication).

I suggest you start with the simplest version of the code, and provide
an optimisation once you can measurably demonstrate that mask_ack is
better.

[...]

>>> + irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
>>> +     else
>>> + irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);
>>
>> Since you do not express the trigger in DT, you need to tell the core
>> about it by calling irqd_set_trigger_type() with the right setting.
>>
>
> Since the comments say so, I will add ativic32_set_type() for irq_set_type()
> in the next version patch.
>
> /*
>  * Must only be called inside irq_chip.irq_set_type() functions.
>  */
> static inline void irqd_set_trigger_type(struct irq_data *d, u32 type)
> {
>         __irqd_to_state(d) &= ~IRQD_TRIGGER_MASK;
>         __irqd_to_state(d) |= type & IRQD_TRIGGER_MASK;
> }
>
> It will be like this.
> static int ativic32_set_type(struct irq_data *data, unsigned int flow_type)
> {
>         irqd_set_trigger_type(data, flow_type);
>         return IRQ_SET_MASK_OK;
> }

This feels wrong. Your interrupt controller doesn't seem to support the
trigger being changed, so irq_set_type would be a bit pointless. A
driver trying to set a level trigger on an edge interrupt would succeed,
and that is as bad as it gets. All you need is to expose the capability
of the HW when you register the flow handler instead of adding a rather
misleading callback.

Thanks,

	M.
Linus Walleij Dec. 1, 2017, 12:30 p.m. UTC | #44
On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:

> From: Rick Chen <rickchen36@gmail.com>
>
> ATCPIT100 is often used on the Andes architecture,
> This timer provide 4 PIT channels. Each PIT channel is a
> multi-function timer, can be configured as 32,16,8 bit timers
> or PWM as well.
>
> For system timer it will set channel 1 32-bit timer0 as clock
> source and count downwards until underflow and restart again.
>
> It also set channel 0 32-bit timer0 as clock event and count
> downwards until condition match. It will generate an interrupt
> for handling periodically.
>
> Signed-off-by: Rick Chen <rickchen36@gmail.com>
> Signed-off-by: Greentime Hu <green.hu@gmail.com>

The driver looks nice overall.

> +static struct timer_of to = {
> +       .flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
> +
> +       .clkevt = {
> +               .name = "atcpit100_tick",
> +               .rating = 300,
> +               .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> +               .set_state_shutdown = atcpit100_clkevt_shutdown,
> +               .set_state_periodic = atcpit100_clkevt_set_periodic,
> +               .set_state_oneshot = atcpit100_clkevt_set_oneshot,
> +               .tick_resume = atcpit100_clkevt_shutdown,
> +               .set_next_event = atcpit100_clkevt_next_event,
> +               .cpumask = cpu_all_mask,
> +       },
> +
> +       .of_irq = {
> +               .handler = atcpit100_timer_interrupt,
> +               .flags = IRQF_TIMER | IRQF_IRQPOLL,
> +       },

I would add:

.of_clk = {
    .name = "PCLK",
};

To be explicit on what we use.

(I hope I understand this OF timer right.)

Otherwise it looks good!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Dec. 2, 2017, 4:47 p.m. UTC | #45
2017-11-28 3:07 GMT+08:00 Rob Herring <robh+dt@kernel.org>:
> On Mon, Nov 27, 2017 at 6:28 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> This patch adds support for device tree.
>>
>> Signed-off-by: Vincent Chen <vincentc@andestech.com>
>> Signed-off-by: Greentime Hu <greentime@andestech.com>
>> ---
>>  arch/nds32/boot/dts/Makefile   |    8 ++++++
>>  arch/nds32/boot/dts/ae3xx.dts  |   55 ++++++++++++++++++++++++++++++++++++
>>  arch/nds32/boot/dts/ag101p.dts |   60 ++++++++++++++++++++++++++++++++++++++++
>>  arch/nds32/kernel/devtree.c    |   45 ++++++++++++++++++++++++++++++
>>  4 files changed, 168 insertions(+)
>>  create mode 100644 arch/nds32/boot/dts/Makefile
>>  create mode 100644 arch/nds32/boot/dts/ae3xx.dts
>>  create mode 100644 arch/nds32/boot/dts/ag101p.dts
>>  create mode 100644 arch/nds32/kernel/devtree.c
>>
>> diff --git a/arch/nds32/boot/dts/Makefile b/arch/nds32/boot/dts/Makefile
>> new file mode 100644
>> index 0000000..d31faa8
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/Makefile
>> @@ -0,0 +1,8 @@
>> +ifneq '$(CONFIG_NDS32_BUILTIN_DTB)' '""'
>
> Built-in dtb's are really for legacy bootloader cases where the
> bootloader doesn't understand dtbs. Do you have that here?
>
> Plus, I don't see any code here to handle the built-in dtb.

As you mentioned in the next thread, it is handled in head.S
We would like to keep it because we debug kernel through gdb without
bootloader very often.

>> +BUILTIN_DTB := $(patsubst "%",%,$(CONFIG_NDS32_BUILTIN_DTB)).dtb.o
>> +else
>> +BUILTIN_DTB :=
>> +endif
>> +obj-$(CONFIG_OF) += $(BUILTIN_DTB)
>> +
>> +clean-files := *.dtb *.dtb.S
>> diff --git a/arch/nds32/boot/dts/ae3xx.dts b/arch/nds32/boot/dts/ae3xx.dts
>> new file mode 100644
>> index 0000000..4181060
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/ae3xx.dts
>> @@ -0,0 +1,55 @@
>> +/dts-v1/;
>> +/ {
>> +       compatible = "nds32 ae3xx";
>
> This compatible needs to be documented and is not valid. Needs to be
> in the form "vendor,board-name" without spaces.

Sorry I forgot to check this.
I will provide a document in bindings like
"Documentation/devicetree/bindings/nds32/andestech-boards".

>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +       interrupt-parent = <&intc>;
>> +
>> +       chosen {
>> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
>> +               stdout-path = &serial0;
>> +       };
>> +
>> +       memory@0 {
>> +               device_type = "memory";
>> +               reg = <0x00000000 0x40000000>;
>> +       };
>> +
>> +       cpu {
>> +               device_type = "cpu";
>> +               compatible = "andestech,n13", "andestech,nds32v3";
>> +               clock-frequency = <60000000>;
>> +       };
>> +
>> +       intc: interrupt-controller {
>> +               compatible = "andestech,ativic32";
>> +               #interrupt-cells = <1>;
>> +               interrupt-controller;
>> +       };
>> +
>> +       serial0: serial@f0300000 {
>> +               compatible = "andestech,uart16550", "ns16550a";
>> +               reg = <0xf0300000 0x1000>;
>> +               interrupts = <8>;
>> +               clock-frequency = <14745600>;
>> +               reg-shift = <2>;
>> +               reg-offset = <32>;
>> +               no-loopback-test = <1>;
>> +       };
>> +
>> +       timer0: timer@f0400000 {
>> +               compatible = "andestech,atcpit100";
>> +               reg = <0xf0400000 0x1000>;
>> +               interrupts = <2>;
>> +               clock-frequency = <30000000>;
>> +               cycle-count-offset = <0x38>;
>> +               cycle-count-down;
>> +       };
>> +
>> +       mac0: mac@e0100000 {
>
> ethernet@...
>
>> +               compatible = "andestech,atmac100";
>> +               reg = <0xe0100000 0x1000>;
>> +               interrupts = <18>;
>> +       };
>> +
>> +};
>> diff --git a/arch/nds32/boot/dts/ag101p.dts b/arch/nds32/boot/dts/ag101p.dts
>> new file mode 100644
>> index 0000000..f1cb540
>> --- /dev/null
>> +++ b/arch/nds32/boot/dts/ag101p.dts
>> @@ -0,0 +1,60 @@
>> +/dts-v1/;
>> +/ {
>> +       compatible = "nds32 ag101p";
>
> Same here.

Sorry I forgot to check this.
I will provide a document in bindings like
"Documentation/devicetree/bindings/nds32/andestech-boards".

>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +       interrupt-parent = <&intc>;
>> +
>> +       chosen {
>> +               bootargs = "earlycon console=ttyS0,38400n8 debug loglevel=7";
>> +               stdout-path = &serial0;
>> +       };
>> +
>> +       memory@0 {
>> +               device_type = "memory";
>> +               reg = <0x00000000 0x40000000>;
>> +       };
>> +
>> +       cpu@0 {
>> +               device_type = "cpu";
>> +               compatible = "andestech,n13";
>> +               clock-frequency = <60000000>;
>> +               next-level-cache = <&L2>;
>> +       };
>> +
>> +       intc: interrupt-controller {
>> +               compatible = "andestech,ativic32";
>> +               #interrupt-cells = <2>;
>> +               interrupt-controller;
>> +       };
>> +
>> +       serial0: serial@99600000 {
>> +               compatible = "andestech,uart16550", "ns16550a";
>> +               reg = <0x99600000 0x1000>;
>> +               interrupts = <7 4>;
>> +               clock-frequency = <14745600>;
>> +               reg-shift = <2>;
>> +               no-loopback-test = <1>;
>> +       };
>> +
>> +       timer0: timer@98400000 {
>> +               compatible = "andestech,atftmr010";
>> +               reg = <0x98400000 0x1000>;
>> +               interrupts = <19 4>;
>> +               clock-frequency = <15000000>;
>> +               cycle-count-offset = <0x20>;
>> +       };
>> +
>> +       mac0: mac@90900000 {
>> +               compatible = "andestech,atmac100";
>> +               reg = <0x90900000 0x1000>;
>> +               interrupts = <25 4>;
>> +       };
>> +
>> +       L2: l2-cache {
>> +               compatible = "andestech,atl2c";
>> +               reg = <0x90f00000 0x1000>;
>> +               cache-unified;
>> +               cache-level = <2>;
>> +       };
>> +};
>> diff --git a/arch/nds32/kernel/devtree.c b/arch/nds32/kernel/devtree.c
>> new file mode 100644
>> index 0000000..2af0f1c
>> --- /dev/null
>> +++ b/arch/nds32/kernel/devtree.c
>> @@ -0,0 +1,45 @@
>> +/*
>> + * Copyright (C) 2005-2017 Andes Technology Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/memblock.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/bootmem.h>
>> +
>> +void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>> +{
>> +       size &= PAGE_MASK;
>> +       memblock_add_node(base, size, 0);
>> +}
>> +
>> +void *__init early_init_dt_alloc_memory_arch(u64 size, u64 align)
>> +{
>> +       return alloc_bootmem_align(size, align);
>> +}
>
> You should be able to use the default functions for these 2.

Thanks. I will remove these 2 functions to use default ones.

>> +
>> +void __init early_init_devtree(void *params)
>> +{
>> +       if (!params || !early_init_dt_scan(params)) {
>> +               pr_crit("\n"
>> +                       "Error: invalid device tree blob at (virtual address 0x%p)\n"
>> +                       "The dtb must be 8-byte aligned and must not exceed 8 KB in size\n"
>
> Why the size limit? That's pretty small for a DT.

Thanks. I will update it in the next version patch.

>> +                       "\nPlease check your bootloader.", params);
>> +
>> +               while (true)
>> +                       cpu_relax();
>
> Might as well use BUG_ON here if you're not going to continue. It's
> generally better to WARN and continue on otherwise the messages aren't
> visible until the console is up. However, if you have DT errors this
> early, there's not much you can really do here.

Yup. Maybe we shall hang in here for user to know he use a wrong DT.
I will change it to BUG_ON(1).

>> +       }
>> +
>> +       dump_stack_set_arch_desc("%s (DT)", of_flat_dt_get_machine_name());
>> +}
>> --
>> 1.7.9.5
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Lezcano Dec. 7, 2017, 8:39 a.m. UTC | #46
On 27/11/2017 13:28, Greentime Hu wrote:
> From: Rick Chen <rickchen36@gmail.com>
> 
> Add CLKSRC_ATCPIT100 for Andestech atcpit100 timer selection.
> It often be used in Andestech AE3XX platform.
> 
> Signed-off-by: Rick Chen <rickchen36@gmail.com>
> Signed-off-by: Greentime Hu <green.hu@gmail.com>
> ---
>  drivers/clocksource/Kconfig  |    6 ++++++
>  drivers/clocksource/Makefile |    1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cc60620..591362a 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -615,4 +615,10 @@ config CLKSRC_ST_LPC
>  	  Enable this option to use the Low Power controller timer
>  	  as clocksource.
>  
> +config CLKSRC_ATCPIT100
> +	bool "Clocksource for AE3XX platform" if COMPILE_TEST
> +	depends on GENERIC_CLOCKEVENTS && HAS_IOMEM

You can remove the GENERIC_CLOCKEVENTS as now it is factored out in the
higher menuconfig option.
Daniel Lezcano Dec. 7, 2017, 8:40 a.m. UTC | #47
On 28/11/2017 03:53, Greentime Hu wrote:
> 2017-11-27 22:11 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Mon, Nov 27, 2017 at 1:28 PM, Greentime Hu <green.hu@gmail.com> wrote:
>>> From: Rick Chen <rickchen36@gmail.com>
>>>
>>> Add CLKSRC_ATCPIT100 for Andestech atcpit100 timer selection.
>>> It often be used in Andestech AE3XX platform.
>>>
>>> Signed-off-by: Rick Chen <rickchen36@gmail.com>
>>> Signed-off-by: Greentime Hu <green.hu@gmail.com>
>>
>> No need to split out the Makefile patch from the actual driver, they
>> clearly belong together.
>> The binding change should be a separate patch, as you did.
>>
>> It's probably best to separate the driver patches from the
>> architecture submission
>> in the future, they can simply get merged by the respective subsystem
>> maintainers,
>> with a smaller Cc list.
>>
> 
> Hi, Arnd:
> 
> Thanks.
> We will merge these 2 patches together in the next version patch.
> We will sent to clocksource subsystem with a seperate patchset.

Sounds good.
Daniel Lezcano Dec. 7, 2017, 8:44 a.m. UTC | #48
On 27/11/2017 13:28, Greentime Hu wrote:
> From: Rick Chen <rickchen36@gmail.com>
> 
> ATCPIT100 is often used on the Andes architecture,
> This timer provide 4 PIT channels. Each PIT channel is a
> multi-function timer, can be configured as 32,16,8 bit timers
> or PWM as well.
> 
> For system timer it will set channel 1 32-bit timer0 as clock
> source and count downwards until underflow and restart again.
> 
> It also set channel 0 32-bit timer0 as clock event and count
> downwards until condition match. It will generate an interrupt
> for handling periodically.
> 
> Signed-off-by: Rick Chen <rickchen36@gmail.com>
> Signed-off-by: Greentime Hu <green.hu@gmail.com>
> ---

Looks good.

Please resend this patch folded with the Makefile change and the DT
binding (fixed) as suggested by Arnd. I will merge them.

Thanks

  -- Daniel
Al Viro Dec. 7, 2017, 4:40 p.m. UTC | #49
On Mon, Nov 27, 2017 at 08:27:53PM +0800, Greentime Hu wrote:
> +void do_page_fault(unsigned long entry, unsigned long addr,
> +		   unsigned int error_code, struct pt_regs *regs)
[snip]
> +	/*
> +	 * If we're in an interrupt or have no user
> +	 * context, we must not take the fault..
> +	 */
> +	if (unlikely(in_atomic() || !mm))

Broken.  in_atomic() is wrong here - it should be faulthandler_disabled().
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Dec. 7, 2017, 4:45 p.m. UTC | #50
On Mon, Nov 27, 2017 at 08:27:55PM +0800, Greentime Hu wrote:

> +#define start_thread(regs,pc,stack)			\
> +({							\
> +	set_fs(USER_DS);				\

Not the job of start_thread() - its users (->load_binary() methods of
assorted binfmt) must (and do) call flush_old_exec() first.  And
that will switch to USER_DS just fine.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Dec. 8, 2017, 5:26 a.m. UTC | #51
2017-12-08 0:40 GMT+08:00 Al Viro <viro@zeniv.linux.org.uk>:
> On Mon, Nov 27, 2017 at 08:27:53PM +0800, Greentime Hu wrote:
>> +void do_page_fault(unsigned long entry, unsigned long addr,
>> +                unsigned int error_code, struct pt_regs *regs)
> [snip]
>> +     /*
>> +      * If we're in an interrupt or have no user
>> +      * context, we must not take the fault..
>> +      */
>> +     if (unlikely(in_atomic() || !mm))
>
> Broken.  in_atomic() is wrong here - it should be faulthandler_disabled().

Thanks.
I will include <linux/uaccess.h> and replace in_atomic() with
faulthandler_disabled()
I will fix it in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greentime Hu Dec. 8, 2017, 5:27 a.m. UTC | #52
2017-12-08 0:45 GMT+08:00 Al Viro <viro@zeniv.linux.org.uk>:
> On Mon, Nov 27, 2017 at 08:27:55PM +0800, Greentime Hu wrote:
>
>> +#define start_thread(regs,pc,stack)                  \
>> +({                                                   \
>> +     set_fs(USER_DS);                                \
>
> Not the job of start_thread() - its users (->load_binary() methods of
> assorted binfmt) must (and do) call flush_old_exec() first.  And
> that will switch to USER_DS just fine.

Thanks. I will remove this setting in the next version patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html