[U-Boot] [PATCH] mpc83xx: New board support SIMPC8313

[Kim Phillips]

On Tue,  4 Nov 2008 16:37:45 -0800
Ron Madrid <ron_madrid at sbcglobal.net> wrote:

> This patch will create a new board, SIMPC8313, from Sheldon Instruments.  This
> board boots from NAND devices and is configurable for either a large page or
> small page device.
> 
> Signed-off-by: Ron Madrid <ron_madrid at sbcglobal.net>

Hi Ron, thanks for this; can you please describe how memory is
connected (soldered vs. non-), and give us some brief info regarding
the board's other features/interfaces, e.g. number & nature of ethernet
ports, pci interfaces, PHYs...

> ---
>  MAINTAINERS                                 |    4 +
>  MAKEALL                                     |    1 +
>  Makefile                                    |   20 +
>  board/sheldon/simpc8313/Makefile            |   50 +++
>  board/sheldon/simpc8313/config.mk           |   13 +
>  board/sheldon/simpc8313/sdram.c             |  206 +++++++++++
>  board/sheldon/simpc8313/simpc8313.c         |  136 +++++++
>  doc/README.simpc8313                        |   71 ++++
>  include/configs/SIMPC8313.h                 |  534 +++++++++++++++++++++++++++
>  nand_spl/board/sheldon/simpc8313/Makefile   |  101 +++++
>  nand_spl/board/sheldon/simpc8313/u-boot.lds |   52 +++
>  11 files changed, 1188 insertions(+), 0 deletions(-)

although this patch applies, it doesn't build due to the CFG_ ->
CONFIG_SYS_ change; please s/CFG_/CONFIG_SYS_/g to avoid getting this
(and a slew of other errors):

...NAND...Configuring for SIMPC8313 board...
In file included from simpc8313.c:32:
/home/kim/git/u-boot/include/ns16550.h:108:2: error: #error "Please define NS16550 registers size."

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 750e374..f844ef4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -259,6 +259,10 @@ Jon Loeliger <jdl at freescale.com>
>  
>  	MPC8641HPCN	MPC8641D
>  
> +Ron Madrid <info at sheldoninst.com>

hmm..I'm not sure if this email address is acceptable.

> diff --git a/MAKEALL b/MAKEALL
> index 9ccb9ac..635bd0d 100755
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -339,6 +339,7 @@ LIST_83xx="		\
>  	MPC837XEMDS	\
>  	MPC837XERDB	\
>  	MVBLM7		\
> +	SIMPC8313_LP	\
>  	sbc8349		\
>  	TQM834x		\

please move SIMPC8313 below sbc8349 to maintain alpha order

>  "
> diff --git a/Makefile b/Makefile
> index 7c13ce8..20c6d9c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2248,6 +2248,26 @@ MPC837XERDB_config:	unconfig
>  MVBLM7_config: unconfig
>  	@$(MKCONFIG) $(@:_config=) ppc mpc83xx mvblm7 matrix_vision
>  
> +SIMPC8313_LP_config \
> +SIMPC8313_SP_config: unconfig
> +	@mkdir -p $(obj)include
> +	@mkdir -p $(obj)board/sheldon/simpc8313
> +	@mkdir -p $(obj)nand_spl/board/sheldon/simpc8313

what happens if you leave out the line above?

> +	@if [ "$(findstring _LP_,$@)" ] ; then \
> +		$(XECHO) -n "...NAND..." ; \
> +		echo "TEXT_BASE = 0x00100000" > $(obj)board/sheldon/simpc8313/config.tmp ; \
> +		echo "#define CONFIG_NAND_U_BOOT" >> $(obj)include/config.h ; \
> +		echo "#define CONFIG_NAND_LP" >> $(obj)include/config.h ; \
> +	fi ; \
> +	if [ "$(findstring _SP_,$@)" ] ; then \
> +		$(XECHO) -n "...NAND..." ; \

might want to inform the user whether they are building a small vs.
large NAND page u-boot.

> +		echo "TEXT_BASE = 0x00100000" > $(obj)board/sheldon/simpc8313/config.tmp ; \

TEXT_BASE doesn't differ between the two - can we rm these lines from
here and change the value of the one in the board's config.mk file?  If
the board can't boot from anything but nand, might want to put
CONFIG_NAND_U_BOOT elsewhere, too.

> +		echo "#define CONFIG_NAND_U_BOOT" >> $(obj)include/config.h ; \
> +		echo "#define CONFIG_NAND_SP" >> $(obj)include/config.h ; \
> +	fi ;
> +	@$(MKCONFIG) -a SIMPC8313 ppc mpc83xx simpc8313 sheldon
> +	@echo "CONFIG_NAND_U_BOOT = y" >> $(obj)include/config.mk
> +
>  sbc8349_config:		unconfig
>  	@$(MKCONFIG) $(@:_config=) ppc mpc83xx sbc8349

the new board target code belongs after the sbc8349 (alpha order again).

> diff --git a/board/sheldon/simpc8313/Makefile b/board/sheldon/simpc8313/Makefile

> +include $(TOPDIR)/config.mk
> +
> +LIB	= $(obj)lib$(BOARD).a
> +
> +COBJS	:= $(BOARD).o sdram.o
> +
> +SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS))
> +SOBJS	:= $(addprefix $(obj),$(SOBJS))

please clean up the code alignment here.

> diff --git a/board/sheldon/simpc8313/config.mk b/board/sheldon/simpc8313/config.mk
> new file mode 100644
> index 0000000..95f6481
> --- /dev/null
> +++ b/board/sheldon/simpc8313/config.mk
> @@ -0,0 +1,13 @@
> +ifndef NAND_SPL
> +sinclude $(OBJTREE)/board/$(BOARDDIR)/config.tmp
> +endif
> +
> +ifndef TEXT_BASE
> +TEXT_BASE = 0xFE000000

this is the place I was referring to in the Makefile comment above.

> diff --git a/board/sheldon/simpc8313/sdram.c b/board/sheldon/simpc8313/sdram.c
> new file mode 100644
> index 0000000..4af29e2
> --- /dev/null
> +++ b/board/sheldon/simpc8313/sdram.c
> @@ -0,0 +1,206 @@
> +/*
> + * Copyright (C) Sheldon Instruments, Inc. 2008
> + *
> + * Author: Ron Madrid <info at sheldoninst.com>
> + * Adapted from ../freescale/mpc8313erdb/sdram.c

if that's true, you've neglected to carry the Freescale copyright.

> + *
> + * (C) Copyright 2006
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> + *
> + * 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,
> + */

the last part of the address of the FSF is missing.

> +static long fixed_sdram(void);

can you reorder the functions to avoid having to have these types of
declarations?

> +#if defined(CONFIG_NAND_SPL)
> +#define puts(v) {}

why is this necessary? do you mean to delete the puts() calls
themselves?

> +void si_read_i2c(int, int, u8*);
> +void si_wait_i2c(void);

please reorder the functions to eliminate these declarations, thanks.

> +#endif
> +
> +#define DDRLAWAR_SIZE	   0x0000003F

please use LAWAR_SIZE instead.

> +phys_size_t initdram(int board_type)
> +{
> +	volatile immap_t *im = (immap_t *) CFG_IMMR;
> +	volatile lbus83xx_t *lbc= &im->lbus;
> +
> +	u32 msize = 0;

initialization not needed.

> +	if ((im->sysconf.immrbar & IMMRBAR_BASE_ADDR) != (u32) im)
> +		return -1;
> +
> +	puts("Initializing\n");
> +
> +	/* DDR SDRAM - Main SODIMM */
> +	im->sysconf.ddrlaw[0].bar = CFG_DDR_BASE & LAWBAR_BAR;
> +
> +	msize = fixed_sdram();
> +
> +	/* Local Bus setup lbcr and mrtpr */
> +	lbc->lbcr = CFG_LBC_LBCR;
> +	lbc->mrtpr = CFG_LBC_MRTPR;
> +	sync();
> +
> +	puts("   DDR RAM: ");
> +	/* return total bus SDRAM size(bytes)  -- DDR */
> +	return (msize * 1024 * 1024);
> +}
> +
> +/*************************************************************************
> + *  fixed sdram init -- reads values from boot sequencer I2C
> + ************************************************************************/
> +static long fixed_sdram(void)
> +{
> +	volatile immap_t *im = (immap_t *) CFG_IMMR;

s/CFG_/CONFIG_SYS_/g

> +	u32 msizelog2, msize = 1;
> +#if defined(CONFIG_NAND_SPL)
> +	u32 i;
> +	u8 buffer[135];
> +	u32 lbyte = 0, count = 135;

lbyte can be factored out (it's only used once), count can have a more
descriptive name, made a const int, and moved above buffer and be used
in buffer's definition.

> +	u32 addr = 0, data = 0;

these don't need to be initialized.

> +
> +	si_read_i2c(lbyte, count, buffer);
> +
> +	for (i = 18; i < count; i+=7){

s/i+=7/i += 7/

> +		*((u32 *)(CFG_IMMR + addr)) = data;

use the out_be32 i/o memory accessor and friends please (here and
elsewhere in the patch).

> +#if defined(CONFIG_NAND_SPL)
> +void si_read_i2c(int lbyte, int count, u8 *buffer)
> +{
> +	u8 chip = 0x50; /* boot sequencer I2C */
> +	u8 ubyte;
> +	u8 dummy;
> +	u32 i;
> +	volatile immap_t *im = (immap_t *) CFG_IMMR;
> +
> +	chip <<= 1;
> +	ubyte = (lbyte&0xff00)>>8;

u-boot codingstyle makes this:

ubyte = (lbyte & 0xff00) >> 8;

> diff --git a/board/sheldon/simpc8313/simpc8313.c b/board/sheldon/simpc8313/simpc8313.c
> new file mode 100644
> index 0000000..8875973
> --- /dev/null
> +++ b/board/sheldon/simpc8313/simpc8313.c
> @@ -0,0 +1,136 @@
> +/*
> + * Copyright (C) Sheldon Instruments, Inc. 2008
> + *
> + * Author: Ron Madrid <info at sheldoninst.com>
> + * Adapted from board/freescale/mpc8313erdb/mpc8313erdb.c

again, please maintain copyrights of others.

> +#include <common.h>
> +#if defined(CONFIG_OF_LIBFDT)
> +#include <libfdt.h>
> +#endif

protecting this include isn't necessary anymore.

> diff --git a/doc/README.simpc8313 b/doc/README.simpc8313

> +4.2	Downloading and Booting Linux Kernel
> +
> +	TODO:

are you not able to boot an OS?

> +#define CONFIG_ROOTPATH		/tftpboot/10.196.31.85

this doesn't look like a path to an nfs server.

> +#define CONFIG_BOOTCOMMAND	"nand read 500000 kernel 600000;bootm 500000 - ae0000"

instead of hardcoding, please use $loadaddr, $fdtaddr, etc.

Kim