Hi Simon,
On Tue, Jul 12, 2016 at 03:56:05PM -0600, Simon Glass wrote:
[...]
> Reviewed-by: Simon Glass <s...@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.
--
Thanks.
-- Max
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
