[U-Boot] [PATCHv1] ARM: Add Altera SOCFPGA Cyclone5

[Marek Vasut <marex@denx.de>]

Dear Pavel Machek,

Minor ramblings below :)

> On Wed 2012-08-29 11:26:45, Tom Rini wrote:
> > On Wed, Aug 29, 2012 at 03:41:54PM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > > diff --git a/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
> > > > > b/arch/arm/cpu/armv7/socfpga/u-boot-spl.lds
> > > > 
> > > > You should setup MEMORY declarations like the other u-boot-spl linker
> > > > scripts do so we get build-time confirmation that we haven't exceeded
> > > > our size limitations.
> > > 
> > > Hmm, I tried, but I don't know socfpga memory layout by heart.
> > > 
> > > Dinh, can you help here?
> > 
> > I think once you get the answers you should be able to re-post the
> > series cleanly and depend on my v5 (or v6) branch.  Thanks!
> 
> Porting it to your v5 was easy :-). Newer patch for review is attached.
> 
> I took oportunity to cleanup whitespace in
> arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. Perhaps someone can
> merge that...

Argh ... what about using git send-email for the patch submission please?

NOTE: I really have a great deal of respect for Pavel, so i do have trouble 
stepping on him properly during the patch review ;-)

> Thanks,
> 								Pavel
> 
> commit a7ecaa40d9a02e84ac81da8f49d48595d7f342ad
> Author: Pavel <pavel@ucw.cz>
> Date:   Thu Aug 30 01:28:00 2012 +0200
> 
>     Add changes for socfpga
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c5a6f2f..df48dea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -765,6 +765,11 @@ Nagendra T S  <nagendra@mistralsolutions.com>
> 
>     am3517_crane    ARM ARMV7 (AM35x SoC)
> 
> +Dinh Nguyen <dinguyen@altera.com>
> +Chin Liang See <clsee@altera.com>
> +
> +	socfpga		socfpga_cyclone5
> +
>  Kyungmin Park <kyungmin.park@samsung.com>
> 
>  	apollon		ARM1136EJS
> diff --git a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
> b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds index 8867e06..1d8efb2
> 100644
> --- a/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
> +++ b/arch/arm/cpu/armv7/omap-common/u-boot-spl.lds
> @@ -37,9 +37,9 @@ SECTIONS
>  {
>  	.text      :
>  	{
> -	__start = .;
> -	  arch/arm/cpu/armv7/start.o	(.text)
> -	  *(.text*)
> +		__start = .;
> +		arch/arm/cpu/armv7/start.o	(.text)
> +		*(.text*)
>  	} >.sram

Maybe the cleanup should simply be split into separate patch?

>  	. = ALIGN(4);
> diff --git a/arch/arm/cpu/armv7/socfpga/Makefile
> b/arch/arm/cpu/armv7/socfpga/Makefile new file mode 100644
> index 0000000..376a4bd
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/socfpga/Makefile
> @@ -0,0 +1,51 @@
> +#
> +# (C) Copyright 2000-2003
> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> +#
> +# Copyright (C) 2012 Altera Corporation <www.altera.com>
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# 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, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB	=  $(obj)lib$(SOC).o
> +
> +SOBJS	:= lowlevel_init.o
> +COBJS-y	:= misc.o timer.o
> +COBJS-$(CONFIG_SPL_BUILD) += spl.o
> +
> +COBJS	:= $(COBJS-y)
> +SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS) $(SOBJS))
> +
> +all:	 $(obj).depend $(LIB)
> +
> +$(LIB):	$(OBJS)
> +	$(call cmd_link_o_target, $(OBJS))
> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/arch/arm/cpu/armv7/socfpga/config.mk
> b/arch/arm/cpu/armv7/socfpga/config.mk new file mode 100644
> index 0000000..b72ed1e
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/socfpga/config.mk
> @@ -0,0 +1,16 @@
> +#
> +# Copyright (C) 2011, Texas Instruments, Incorporated - http://www.ti.com/
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed "as is" WITHOUT ANY WARRANTY of any
> +# kind, whether express or implied; without even the implied warranty
> +# of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +ifndef CONFIG_SPL_BUILD
> +ALL-y	+= $(obj)u-boot.img
> +endif
> diff --git a/arch/arm/cpu/armv7/socfpga/lowlevel_init.S
> b/arch/arm/cpu/armv7/socfpga/lowlevel_init.S new file mode 100644
> index 0000000..815073e
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/socfpga/lowlevel_init.S
> @@ -0,0 +1,79 @@
> +/*
> + *  Copyright (C) 2012 Altera Corporation <www.altera.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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 <config.h>
> +#include <version.h>
> +
> +/* Save the parameter pass in by previous boot loader */
> +.global save_boot_params
> +save_boot_params:
> +	/* save the parameter here */
> +
> +	/*
> +	 * Setup stack for exception, which is located
> +	 * at the end of on-chip RAM. We don't expect exception prior to
> +	 * relocation and if that happens, we won't worry -- it will overide
> +	 * global data region as the code will goto reset. After relocation,
> +	 * this region won't be used by other part of program.
> +	 * Hence it is safe.
> +	 */
> +	ldr	r0, =CONFIG_SYS_INIT_RAM_ADDR
> +	ldr	r1, =CONFIG_SYS_INIT_RAM_SIZE
> +	add	r0, r0, r1

Won't the preprocessor compute (CONFIG_SYS_INIT_RAM_ADDR + 
CONFIG_SYS_INIT_RAM_SIZE) for you? Then you'd only have to do one LDR and no ADD 
instruction, saving two ticks of CPU :-)

> +	ldr	r1, =IRQ_STACK_START_IN
> +	str	r0, [r1]
> +
> +	bx	lr
> +
> +
> +/* Set up the platform, once the cpu has been initialized */
> +.globl lowlevel_init
> +lowlevel_init:
> +
> +	/* Remap */
> +#ifdef CONFIG_SPL_BUILD
> +	/*
> +	 * SPL : configure the remap (L3 NIC-301 GPV)
> +	 * so the on-chip RAM at lower memory instead ROM.
> +	 */
> +	ldr	r0, =SOCFPGA_L3REGS_ADDRESS
> +	mov	r1, #0x19
> +	str	r1, [r0]
> +#else
> +	/*
> +	 * U-Boot : configure the remap (L3 NIC-301 GPV)
> +	 * so the SDRAM at lower memory instead on-chip RAM.
> +	 */
> +	ldr	r0, =SOCFPGA_L3REGS_ADDRESS
> +	mov	r1, #0x2
> +	str	r1, [r0]
> +
> +	/* Private components security */
> +
> +	/*
> +	 * U-Boot : configure private timer, global timer and cpu
> +	 * component access as non secure for kernel stage (as required
> +	 * by kernel)
> +	 */
> +	mrc	p15,4,r0,c15,c0,0
> +	add	r1, r0, #0x54
> +	ldr	r2, [r1]
> +	orr	r2, r2, #0xff
> +	orr	r2, r2, #0xf00
> +	str	r2, [r1]
> +#endif	/* #ifdef CONFIG_SPL_BUILD */
> +	mov	pc, lr
> diff --git a/arch/arm/cpu/armv7/socfpga/misc.c
> b/arch/arm/cpu/armv7/socfpga/misc.c new file mode 100644
> index 0000000..b906701
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/socfpga/misc.c
> @@ -0,0 +1,54 @@
> +/*
> + *  Copyright (C) 2012 Altera Corporation <www.altera.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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 <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/reset_manager.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static const struct socfpga_reset_manager *reset_manager_base =
> +		(void *)SOCFPGA_RSTMGR_ADDRESS;
> +
> +/*
> + * Write the reset manager register to cause reset
> + */

You might want to add __attribute__((noreturn)) to this function :-)

> +void reset_cpu(ulong addr)
> +{
> +	/* request a warm reset */
> +	writel(RSTMGR_CTRL_SWWARMRSTREQ_LSB, &reset_manager_base->ctrl);
> +	/*
> +	 * infinite loop here as watchdog will trigger and reset
> +	 * the processor
> +	 */
> +	while (1)
> +		;
> +}

[...]

> diff --git a/arch/arm/cpu/armv7/socfpga/timer.c
> b/arch/arm/cpu/armv7/socfpga/timer.c new file mode 100644
> index 0000000..28da4d0
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/socfpga/timer.c
> @@ -0,0 +1,149 @@
> +/*
> + *  Copyright (C) 2012 Altera Corporation <www.altera.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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 <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/timer.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static const struct socfpga_timer *timer_base = (void
> *)CONFIG_SYS_TIMERBASE; +
> +/*
> + * Timer initialization
> + */
> +int timer_init(void)
> +{
> +	writel(TIMER_LOAD_VAL, &timer_base->load_val);
> +	writel(TIMER_LOAD_VAL, &timer_base->curr_val);
> +	writel((readl((&timer_base->ctrl)) | 0x3),

I think you should stick to programming in C here, not ((((LISP)))), so try 
cutting down on the ((braces)) :-)

btw. what's this 0x3 magic constant ?

> +		(&timer_base->ctrl));
> +	return 0;
> +}

[...]

> +#include <common.h>
> +#include <asm/arch/reset_manager.h>
> +#include <asm/io.h>
> +
> +#include <netdev.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +void show_boot_progress(int progress)
> +{
> +	debug("Boot reached stage %d\n", progress);
> +}
> +
> +static inline void delay(unsigned long loops)
> +{
> +	__asm__ volatile ("1:\n"
> +		"subs %0, %1, #1\n"
> +		"bne 1b" : "=r" (loops) : "0" (loops));
> +}

Am I flat blind or is this not used here?
[...]

All in all, the code is nice.