Re: [U-Boot] [PATCH 2/8] xtensa: add support for the xtensa processor architecture [2/2]

Hi Simon,

On Tue, Jul 12, 2016 at 03:56:05PM -0600, Simon Glass wrote:

[...]

Reviewed-by: Simon Glass sjg@chromium.org

Some comments below.

...

diff --git a/arch/Kconfig b/arch/Kconfig index 566f044..a36ac2f 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -83,6 +83,13 @@ config X86 select DM_SPI select DM_SPI_FLASH

+config XTENSA

•   bool "Xtensa architecture"
  

•   select CREATE_ARCH_SYMLINK
  

•   select HAVE_GENERIC_BOARD
  

This doesn't exist anymore as the migration is complete.

...

•   select SUPPORT_OF_CONTROL
  

•   select SYS_GENERIC_BOARD
  

Same here.

Ok, will remove both.

...

diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig new file mode 100644 index 0000000..e4e3625 --- /dev/null +++ b/arch/xtensa/Kconfig @@ -0,0 +1,26 @@ +menu "Xtensa architecture"

•   depends on XTENSA
  

+config SYS_ARCH

•   string
  

•   default "xtensa"
  

+config SYS_CPU

•   string "Xtensa Core Variant"
  

+choice

•   prompt "Target select"
  

+endchoice

+config HAVE_SYS_ASCDISP

•   bool
  

+config SYS_ASCDISP

•   bool "System LCD Display"
  

•   default y
  

•   depends on HAVE_SYS_ASCDISP
  

Needs a help message. Should SYS_ASCDISP go in drivers/video?

Ok, will move it there and write help message.

[...]

...

diff --git a/arch/xtensa/cpu/cpu.c b/arch/xtensa/cpu/cpu.c new file mode 100644 index 0000000..fb7f810 --- /dev/null +++ b/arch/xtensa/cpu/cpu.c @@ -0,0 +1,92 @@ +/*

• • (C) Copyright 2008 - 2013 Tensilica Inc.
• • (C) Copyright 2014 - 2016 Cadence Design Systems Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+/*

• • CPU specific code
• */

+#include <common.h> +#include <command.h> +#include <linux/stringify.h> +#include <asm/global_data.h> +#include <asm/cache.h> +#include <asm/string.h> +#include <asm/misc.h>

+DECLARE_GLOBAL_DATA_PTR; +gd_t *gd;

+#if defined(CONFIG_DISPLAY_CPUINFO) +/*

• • Print information about the CPU.
• */

+int print_cpuinfo(void) +{

•   char buf[120], mhz[8];
  

•   uint32_t id0, id1;
  

•   asm volatile ("rsr %0, 176\n"
  

•                 "rsr %1, 208\n"
  

•                 : "=r"(id0), "=r"(id1));
  

•   sprintf(buf, "CPU:   Xtensa %s (id: %08x:%08x) at %s MHz\n",
  

•           XCHAL_CORE_ID, id0, id1, strmhz(mhz, gd->cpu_clk));
  

•   puts(buf);
  

•   return 0;
  

+} +#endif

+/*

• • Implement the "reset" command.
• • We need support from the board, though.
• */

+int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{

•   board_reset();
  

•   /* Shouldn't come back! */
  

•   printf("****** RESET FAILED ******\n");
  

All of this can be replaced by a call to sysreset_walk_halt() I think, if you have a suitable UCLASS_RESET driver.

Ok, let me look at that.

...

•   return 0;
  

+}

+/*

• • We currently run always with caches enabled when running for mmemory.

Do you mean 'from memory'?

Yes, will fix.

[...]

...

diff --git a/arch/xtensa/cpu/exceptions.c b/arch/xtensa/cpu/exceptions.c new file mode 100644 index 0000000..412ca5f --- /dev/null +++ b/arch/xtensa/cpu/exceptions.c @@ -0,0 +1,64 @@ +/*

• • (C) Copyright 2008 - 2013 Tensilica Inc.
• • (C) Copyright 2014 Cadence Design Systems Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+/*

• • Exception handling.
• • We currently don't handle any exception and force a reset.
• • (Note that alloca is a special case and handled in start.S)
• */

+#include <common.h> +#include <command.h> +#include <asm/xtensa.h> +#include <asm/string.h> +#include <asm/regs.h>

+typedef void (*handler_t)(struct pt_regs *);

+void xtensa_mem_exc_dummy(struct pt_regs *);

+void unhandled_exception(struct pt_regs *regs) +{

•   display_printf("!! EXCCAUSE = %2ld", regs->exccause);
  

•   printf("Unhandled Exception: EXCCAUSE = %ld (pc %lx)\n",
  

•          regs->exccause, regs->pc);
  

•   udelay(10000000);       /* 10s to read display message */
  

Probably not needed? Up to you though.

I'd either drop it together with ASCII display, or leave them both.

[...]

...

diff --git a/arch/xtensa/cpu/start.S b/arch/xtensa/cpu/start.S new file mode 100644 index 0000000..ac32efb --- /dev/null +++ b/arch/xtensa/cpu/start.S

[...]

...

+3: /* All code and initalized data segments have been copied. */

•   /* Setup PS, PS.WOE = 1, PS.EXCM = 0, PS.INTLEVEL = EXCM level. */
  

+#if __XTENSA_CALL0_ABI__

•   movi    a2, XCHAL_EXCM_LEVEL
  

+#else

•   movi    a2, (1<<PS_WOE_BIT) | XCHAL_EXCM_LEVEL
  

+#endif

•   wsr     a2, PS
  

•   rsync
  

•   /* Clear BSS */
  

•   movi    a2, _bss_start
  

•   movi    a3, _bss_end
  

Can you please use board_init_f_init_reserve(), etc.? You can copy how ARM does things perhaps. This should not be in assembly.

We use them, one screen below. What exactly should not be in assembly? BSS zeroing?

...

•   __loopt a2, a3, a4, 2
  

•   s32i    a0, a2, 0
  

•   __endla a2, a3, 4
  

•   /* Writeback */
  

•   ___flush_dcache_all a2, a3
  

+#ifdef __XTENSA_WINDOWED_ABI__

•   /*
  

•    * In windowed ABI caller and call target need to be within the same
  

•    * gigabyte. Put the rest of the code into the text segment and jump
  

•    * there.
  

•    */
  

•   movi    a4, .Lboard_init_code
  

•   jx      a4
  

•   .text
  

•   .align  4
  

+.Lboard_init_code: +#endif

•   movi    a0, 0
  

•   movi    sp, (CONFIG_SYS_TEXT_ADDR - 16) & 0xfffffff0
  

+#ifdef CONFIG_DEBUG_UART

•   movi    a4, debug_uart_init
  

+#ifdef __XTENSA_CALL0_ABI__

•   callx0  a4
  

+#else

•   callx4  a4
  

+#endif +#endif

•   movi    a4, board_init_f_alloc_reserve
  

+#ifdef __XTENSA_CALL0_ABI__

•   mov     a2, sp
  

•   callx0  a4
  

•   mov     sp, a2
  

+#else

•   mov     a6, sp
  

•   callx4  a4
  

•   movsp   sp, a6
  

+#endif

•   movi    a4, board_init_f_init_reserve
  

+#ifdef __XTENSA_CALL0_ABI__

•   callx0  a4
  

+#else

•   callx4  a4
  

+#endif

•    /*
  

•    * Call board initialization routine (never returns).
  

•    */
  

•   movi    a4, board_init_f
  

As above. This is using the old approach.

I see the same sequence in the ARM's _main in arch/arm/lib/crt0.S:

mov r0, sp bl board_init_f_alloc_reserve mov sp, r0 /* set up gd here, outside any C code */ mov r9, r0 bl board_init_f_init_reserve

mov r0, #0 bl board_init_f

What is 'the new approach' and how does it differ from the old?

[...]

...

•   /* Does not return here. */
  

nit: drop periods before */

Ok.

[...]

...

+fast_alloca_exception: /* must be at _WindowUnderflow4 + 16

*/ ?

Oops, right.

[...]

...

diff --git a/arch/xtensa/cpu/u-boot.lds b/arch/xtensa/cpu/u-boot.lds new file mode 100644 index 0000000..853ae5a --- /dev/null +++ b/arch/xtensa/cpu/u-boot.lds @@ -0,0 +1,116 @@ +/*

• • (C) Copyright 2008 - 2013 Tensilica, Inc.
• • (C) Copyright 2014 - 2016 Cadence Design Systems Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <config.h> +#include <asm/ldscript.h> +#include <asm/arch/core.h> +#include <asm/addrspace.h> +#include <asm-offsets.h>

+OUTPUT_ARCH(xtensa) +ENTRY(_start)

+/*

• • U-Boot resets from SYSROM and unpacks itself from a ROM store to RAM.
• • The reset vector is usually near the base of SYSROM and has room
• • above it for the ROM store into which the rest of U-Boot is packed.
• • The ROM store also needs to be above any other vectors that are in ROM.
• • If a core has its vectors near the top of ROM, this must be edited.
• • Note that to run C code out of ROM, the processor would have to support
• • 'relocatable' exception vectors and provide a scratch memory for the
• • initial stack. Not all Xtensa processor configurations support that, so
• • we can simplify the boot process and unpack U-Boot to RAM immediately.
• • This, however, requires that memory have been initialized throug some
• • other means (serial ROM, for example) or are initialized early (requiring
• • an assembler function. See start.S for more details)
• */

+SECTIONS +{

• . = + SIZEOF_HEADERS;
• SECTION_ResetVector(XCHAL_RESET_VECTOR_VADDR, LMA_EQ_VMA)
• .reloc_table ALIGN(4) : FOLLOWING(.ResetVector.text)
• {
• __reloc_table_start = ABSOLUTE(.);

+#if XCHAL_HAVE_WINDOWED

• RELOCATE2(WindowVectors,text);

+#endif

• RELOCATE2(KernelExceptionVector,literal);
• RELOCATE2(KernelExceptionVector,text);
• RELOCATE2(UserExceptionVector,literal);
• RELOCATE2(UserExceptionVector,text);
• RELOCATE2(DoubleExceptionVector,literal);
• RELOCATE2(DoubleExceptionVector,text);
• RELOCATE1(text);
• RELOCATE1(rodata);
• RELOCATE1(data);
• RELOCATE1(u_boot_list);
• __reloc_table_end = ABSOLUTE(.);
• }

+#if XCHAL_HAVE_WINDOWED

• SECTION_VECTOR(WindowVectors,text,XCHAL_WINDOW_VECTORS_VADDR,

•            FOLLOWING(.reloc_table))
  

• SECTION_VECTOR(KernelExceptionVector,literal,XCHAL_KERNEL_VECTOR_VADDR-8,

•            FOLLOWING(.WindowVectors.text))
  

+#else

• SECTION_VECTOR(KernelExceptionVector,literal,XCHAL_KERNEL_VECTOR_VADDR-8,

•            FOLLOWING(.reloc_table))
  

+#endif

• SECTION_VECTOR(KernelExceptionVector,text,XCHAL_KERNEL_VECTOR_VADDR,

•            FOLLOWING(.KernelExceptionVector.literal))
  

• SECTION_VECTOR(UserExceptionVector,literal,XCHAL_USER_VECTOR_VADDR-8,

•            FOLLOWING(.KernelExceptionVector.text))
  

• SECTION_VECTOR(UserExceptionVector,text,XCHAL_USER_VECTOR_VADDR,

•            FOLLOWING(.UserExceptionVector.literal))
  

• SECTION_VECTOR(DoubleExceptionVector,literal,XCHAL_DOUBLEEXC_VECTOR_VADDR-8,

•            FOLLOWING(.UserExceptionVector.text))
  

• SECTION_VECTOR(DoubleExceptionVector,text,XCHAL_DOUBLEEXC_VECTOR_VADDR,

•            FOLLOWING(.DoubleExceptionVector.literal))
  

• __monitor_start = CONFIG_SYS_TEXT_ADDR;
• SECTION_text(CONFIG_SYS_TEXT_ADDR, FOLLOWING(.DoubleExceptionVector.text))
• SECTION_rodata(ALIGN(16), FOLLOWING(.text))
• SECTION_u_boot_list(ALIGN(16), FOLLOWING(.rodata))
• SECTION_data(ALIGN(16), FOLLOWING(.u_boot_list))
• __reloc_end = .;
• __init_end = .;
• SECTION_bss(__init_end (OVERLAY),)
• __monitor_end = .;
• /*
• • On many Xtensa boards a region of RAM may be mapped to the ROM address
• • space to facilitate on-chip-debug, and U-Boot must fit with that region.
• • The config variables CONFIG_SYS_MONITOR_* define the region.
• • If U-Boot extends beyond this region it will appear discontiguous in the
• • address space and is in danger of overwriting itself during unpacking
• • ("relocation").
• • This causes U-Boot to crash in a way that is difficult to debug. On some
• • boards (such as xtav60) the region is small enough that U-Boot will not
• • fit if compiled entirely with -O0 (a common scenario). To avoid a lengthy
• • debugging session when this happens, ensure a link-time error occurs.
• */
• ASSERT(__monitor_end - __monitor_start <= CONFIG_SYS_MONITOR_LEN,

•      "U-Boot ROM image is too large. Check optimization level.")
  

• SECTION_xtensa
• SECTION_debug
• /DISCARD/ : { *(.dynstr*) }
• /DISCARD/ : { *(.hash*) }
• /DISCARD/ : { *(.interp) }
• /DISCARD/ : { *(.got*) }
• /DISCARD/ : { *(.dynsym) }

Are you including the list regions, etc.? u_boot_list is what it is in most .lds files.

Yes, it's in the SECTION_u_boot_list a few lines above.

[...]

...

diff --git a/arch/xtensa/include/asm/byteorder.h b/arch/xtensa/include/asm/byteorder.h new file mode 100644 index 0000000..485bc4b --- /dev/null +++ b/arch/xtensa/include/asm/byteorder.h @@ -0,0 +1,81 @@ +/*

• • Based on Linux/Xtensa kernel version
• • Copyright (C) 2001 - 2007 Tensilica Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _XTENSA_BYTEORDER_H +#define _XTENSA_BYTEORDER_H

+#include <asm/types.h>

+static inline __attribute__((const)) __u32 ___arch__swab32(__u32 x) +{

•   __u32 res;
  

•   /* instruction sequence from Xtensa ISA release 2/2000 */
  

•   __asm__("ssai     8\n\t"
  

•           "srli     %0, %1, 16\n\t"
  

•           "src      %0, %0, %1\n\t"
  

•           "src      %0, %0, %0\n\t"
  

•           "src      %0, %1, %0\n"
  

•           : "=&a" (res)
  

•           : "a" (x)
  

•           );
  

•   return res;
  

+}

+static inline __attribute__((const)) __u16 ___arch__swab16(__u16 x) +{

•   /* Given that 'short' values are signed (i.e., can be negative),
  

/*

• Given

Ok.

[...]

...

diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c new file mode 100644 index 0000000..8c89d2c --- /dev/null +++ b/arch/xtensa/lib/bootm.c

[...]

...

+/*

• • Boot Linux.
• */

+int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) +{

•   struct bp_tag *params, *params_start;
  

•   ulong initrd_start, initrd_end;
  

•   char *commandline = getenv("bootargs");
  

•   if (!(flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)))
  

•           return 0;
  

•   show_boot_progress(15);
  

•   if (images->rd_start) {
  

•           initrd_start = images->rd_start;
  

•           initrd_end = images->rd_end;
  

•   } else {
  

•           initrd_start = 0;
  

•           initrd_end = 0;
  

•   }
  

•   params_start = (struct bp_tag *)gd->bd->bi_boot_params;
  

Do you not use device tree with Linux? It looks like you need to support both that and tags?

We can use device tree with linux, but that's not mandatory. When it's used we pass DTB in one of the tags.