[U-Boot] [PATCH] powerpc/T1040EMU: Add T1040 emulator support

[Wolfgang Denk]

Dear Priyanka Jain,

In message <1382936067-30488-1-git-send-email-Priyanka.Jain at freescale.com> you wrote:
> Add emulator support for T1040. Emulator has limited peripherals
> and interfaces.
> Difference between T1040QDS and emulator includes:
> -ECC for DDR is disabled due to procedure to load images
> -Depends on raw timing for DDR initialization
> -No board FPGA (Qixis)
> -No PCI
> -No SPI
> -No flash support
...
> diff --git a/include/configs/T1040EMU.h b/include/configs/T1040EMU.h
> new file mode 100644
> index 0000000..7005cb1
> --- /dev/null
> +++ b/include/configs/T1040EMU.h
> @@ -0,0 +1,408 @@
> +/*
> + * Copyright 2013 Freescale Semiconductor, Inc.
> + *
> + * 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
> + */

Please convert to SPDX ID.


> +#define CONFIG_SYS_MEMTEST_START	0x00200000	/* memtest works on */
> +#define CONFIG_SYS_MEMTEST_END		0x00400000

Does this really make sense?

> +/* Serial Port - controlled on board with jumper J8
> + * open - index 2
> + * shorted - index 1
> + */

Incorrect multiline comment style.

> +/* new uImage format support */
> +#define CONFIG_FIT
> +#define CONFIG_FIT_VERBOSE	/* enable fit_format_{error,warning}() */

Strictly speaking, FIT image and uImage are completely different
things.  FIT is _not_ "new uImage".


> +#define CONFIG_SYS_PROMPT	"=> "		/* Monitor Command Prompt */

No need to redefine the default.

> +#define	CONFIG_EXTRA_ENV_SETTINGS				\
> +	"hwconfig=fsl_ddr:ctlr_intlv=cacheline,"		\
> +	"bank_intlv=cs0_cs1;"					\

It appears hwconfig is not NUL terminated?

> +	"netdev=eth0\0"						\
> +	"uboot=" __stringify(CONFIG_UBOOTPATH) "\0"		\
> +	"ubootaddr=" __stringify(CONFIG_SYS_TEXT_BASE) "\0"	\
> +	"tftpflash=tftpboot $loadaddr $uboot && "		\
> +	"protect off $ubootaddr +$filesize && "			\
> +	"erase $ubootaddr +$filesize && "			\
> +	"cp.b $loadaddr $ubootaddr $filesize && "		\
> +	"protect on $ubootaddr +$filesize && "			\
> +	"cmp.b $loadaddr $ubootaddr $filesize\0"		\

I'd recommend to use indentation to show where the next command
starts.

> +	"c=ffe\0"

I sthis intentional?  What's the sense?

> +#define CONFIG_PROOF_POINTS			\
> +	"setenv bootargs root=/dev/$bdev rw "		\
> +	"console=$consoledev,$baudrate $othbootargs;"	\
> +	"cpu 1 release 0x29000000 - - -;"              \
> +	"cpu 2 release 0x29000000 - - -;"              \
> +	"cpu 3 release 0x29000000 - - -;"              \
> +	"cpu 4 release 0x29000000 - - -;"              \
> +	"cpu 5 release 0x29000000 - - -;"              \
> +	"cpu 6 release 0x29000000 - - -;"              \
> +	"cpu 7 release 0x29000000 - - -;"              \
> +	"go 0x29000000"					\

Not NUL terminated?

> +	"setenv bootargs root=/dev/$bdev rw "		\
> +	"console=$consoledev,$baudrate $othbootargs;"	\
> +	"cpu 1 release 0x29000000 - - -;"		\
> +	"cpu 2 release 0x29000000 - - -;"		\
> +	"cpu 3 release 0x29000000 - - -;"		\
> +	"cpu 4 release 0x29000000 - - -;"		\
> +	"cpu 5 release 0x29000000 - - -;"		\
> +	"cpu 6 release 0x29000000 - - -;"		\
> +	"cpu 7 release 0x29000000 - - -;"		\
> +	"go 0x29000000"

Intentionally duplicated?

> +#define CONFIG_LINUX                       \
> +	"setenv bootargs root=/dev/ram rw "            \
> +	"console=$consoledev,$baudrate $othbootargs;"  \
> +	"setenv ramdiskaddr 0x02000000;"               \
> +	"setenv fdtaddr 0x00c00000;"		       \
> +	"setenv loadaddr 0x1000000;"		       \
> +	"bootm $loadaddr $ramdiskaddr $fdtaddr"

Not NUL terminated?

> +#define CONFIG_HDBOOT					\
> +	"setenv bootargs root=/dev/$bdev rw "		\
> +	"console=$consoledev,$baudrate $othbootargs;"	\
> +	"tftp $loadaddr $bootfile;"			\
> +	"tftp $fdtaddr $fdtfile;"			\
> +	"bootm $loadaddr - $fdtaddr"

Not NUL terminated?

> +#define CONFIG_NFSBOOTCOMMAND			\
> +	"setenv bootargs root=/dev/nfs rw "	\
> +	"nfsroot=$serverip:$rootpath "		\
> +	"ip=$ipaddr:$serverip:$gatewayip:$netmask:$hostname:$netdev:off " \
> +	"console=$consoledev,$baudrate $othbootargs;"	\
> +	"tftp $loadaddr $bootfile;"		\
> +	"tftp $fdtaddr $fdtfile;"		\
> +	"bootm $loadaddr - $fdtaddr"

Not NUL terminated?

> +#define CONFIG_RAMBOOTCOMMAND				\
> +	"setenv bootargs root=/dev/ram rw "		\
> +	"console=$consoledev,$baudrate $othbootargs;"	\
> +	"tftp $ramdiskaddr $ramdiskfile;"		\
> +	"tftp $loadaddr $bootfile;"			\
> +	"tftp $fdtaddr $fdtfile;"			\
> +	"bootm $loadaddr $ramdiskaddr $fdtaddr"

Not NUL terminated?


In general, this is a pretty mess of env settings, that could greatly
be simplified if you'd restructure things, like factoring out stuff
that repeats again and again.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The shortest unit of time in the multiverse is the News York  Second,
defined  as  the  period  of  time between the traffic lights turning
green and the cab behind you honking.
                                - Terry Pratchett, _Lords and Ladies_