[U-Boot] [PATCH] arm: socfpga: Add SoCFPGA SR1500 board

[Marek Vasut <marex@denx.de>]

On Friday, October 23, 2015 at 09:26:53 AM, Stefan Roese wrote:
> The SR1500

Does SR mean Stefan Roese ? :-)

Anyway, shouldn't you place this device under board/vendorname/boardname
instead of plain board/boardname/ ?

And one more thing, would it be possible for you to do a short README on
adding a new board? That'd be real cool. Obviously, it's not something I
demand or that'd block this patch series, it'd be nice though.

> board is a CycloneV based board, similar to the EBV
> SoCrates, equipped with the following devices:
> 
> - SPI NOR
> - eMMC
> - Ethernet
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>

[...]

> +int board_early_init_f(void)
> +{
> +	int ret;
> +
> +	/* Reset the Marvell PHY 88E1510 */
> +	ret = gpio_request(63, "PHY reset");
> +	if (ret)
> +		return ret;
> +
> +	gpio_direction_output(63, 0);
> +	mdelay(20);
> +	gpio_set_value(63, 1);

Does the PHY come out of reset immediatelly after you deassert the nRESET
GPIO or not ? You might want to add a small delay here to bullet-proof the
code a bit more.

> +	return 0;
> +}
> +
> +#define CONFIG_SYS_IDT_CLK_ADDR		0x6a
> +
> +static int do_clksave(cmd_tbl_t *cmdtp, int flag, int argc, char *const
> argv[]) +{
> +	u8 buf[1];
> +
> +	buf[0] = 0x01;
> +	i2c_write(CONFIG_SYS_IDT_CLK_ADDR, 0, 0, buf, 1);
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(clksave, 1, 0, do_clksave,
> +	   "IDT 5V49EE702 Progsave command", "");

I am not convinced I should let this slide. Wouldn't it be better to just
have an environment script which sends 0x1 to this IDT Versaclock using the
i2c command ?

> +#define NET_DEV_NAME			"ethernet at ff702000"
> +#define MII_MARVELL_PHY_PAGE		22
> +#define PHY_DIAG_START			(1 << 15)
> +#define PHY_DIAG_BUSY			(1 << 11)
> +
> +static char str[16];

Please move this static var into do_phytest() and pass it into pair_state()
as an argument.

> +static char *pair_state(int val)
> +{
> +	switch (val) {
> +	case 0x00:
> +		strcpy(str, "Invalid");
> +		break;
> +	case 0x01:
> +		strcpy(str, "Pair Ok");
> +		break;
> +	case 0x02:
> +		strcpy(str, "Pair Open");
> +		break;
> +	case 0x03:
> +		strcpy(str, "Same Pair Short");
> +		break;
> +	case 0x04:
> +		strcpy(str, "Cross Pair Short");

Do I count correctly that you do strcpy() here on a string which is 16 byte
long + one trailing '\0' (total 17 bytes) and you strcpy() it into 16 byte
long buffer ? Well that's not good, this will overwrite one byte past the
$str buffer with '\0' :-)

> +		break;
> +	case 0x09:
> +		strcpy(str, "Pair Busy");
> +		break;
> +	default:
> +		strcpy(str, "Reserved");
> +		break;
> +	};
> +
> +	return str;
> +}
> +
> +static int do_phytest(cmd_tbl_t *cmdtp, int flag, int argc, char *const
> argv[]) +{
> +	char devname[] = NET_DEV_NAME;

const char * here ?

> +	int addr = 0;

Looks like unsigned value to me, please make it so.

> +	u16 data;
> +	u16 status;
> +	u16 oldpage;
> +	int i;
> +
> +	/* Save current page register */
> +	miiphy_read(devname, addr, MII_MARVELL_PHY_PAGE, &oldpage);
> +
> +	/*
> +	 * Run cable disgnostics
> +	 */
> +	printf("Running cable diagnostic test...");
> +	miiphy_write(devname, addr, MII_MARVELL_PHY_PAGE, 7);
> +	miiphy_write(devname, addr, 21, PHY_DIAG_START);
> +	miiphy_read(devname, addr, 21, &data);
> +	while ((data & PHY_DIAG_BUSY) == PHY_DIAG_BUSY) {
> +		miiphy_read(devname, addr, 21, &data);
> +		mdelay(1);

Unbounded loop, do I need to say more ? ;-)

> +	}
> +	printf("done!\n");

[...]

> +/* Booting Linux */
> +#define CONFIG_BOOTDELAY	3
> +#define CONFIG_BOOTFILE		"uImage"
> +#define CONFIG_BOOTARGS		"console=ttyS0" 
__stringify(CONFIG_BAUDRATE)
> +#ifdef CONFIG_SOCFPGA_VIRTUAL_TARGET

Do you really need socfpga_vt on your quite certainly physical hardware ?

> +#define CONFIG_BOOTCOMMAND	"run ramboot"
> +#else
> +#define CONFIG_BOOTCOMMAND	"run mmcload; run mmcboot"
> +#endif
> +#define CONFIG_LOADADDR		0x8000
> +#define CONFIG_SYS_LOAD_ADDR	CONFIG_LOADADDR
> +#define CONFIG_SYS_CONSOLE_INFO_QUIET	/* don't print console @ startup 
*/
> +
> +/* Ethernet on SoC (EMAC) */
> +#if defined(CONFIG_CMD_NET)
> +#define CONFIG_EMAC_BASE		SOCFPGA_EMAC1_ADDRESS

This EMAC address is certainly not needed now, it should come from OF.

> +#define CONFIG_PHY_INTERFACE_MODE	PHY_INTERFACE_MODE_RGMII
> +/* The PHY is autodetected, so no MII PHY address is needed here */
> +#define CONFIG_PHY_MARVELL
> +#define PHY_ANEG_TIMEOUT	8000
> +#endif
> +
> +/* Extra Environment */
> +#define CONFIG_HOSTNAME		sr1500
> +
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +	"verify=n\0" \
> +	"loadaddr= " __stringify(CONFIG_SYS_LOAD_ADDR) "\0" \
> +	"ramboot=setenv bootargs " CONFIG_BOOTARGS ";" \
> +		"bootm ${loadaddr} - ${fdt_addr}\0" \
> +	"bootimage=zImage\0" \
> +	"fdt_addr=100\0" \
> +	"fdtimage=socfpga.dtb\0" \
> +		"fsloadcmd=ext2load\0" \
> +	"bootm ${loadaddr} - ${fdt_addr}\0" \
> +	"mmcroot=/dev/mmcblk0p2\0" \
> +	"mmcboot=setenv bootargs " CONFIG_BOOTARGS \
> +		" root=${mmcroot} rw rootwait;" \
> +		"bootz ${loadaddr} - ${fdt_addr}\0" \
> +	"mmcload=mmc rescan;" \
> +		"load mmc 0:1 ${loadaddr} ${bootimage};" \
> +		"load mmc 0:1 ${fdt_addr} ${fdtimage}\0" \
> +	"qspiroot=/dev/mtdblock0\0" \
> +	"qspirootfstype=jffs2\0" \
> +	"qspiboot=setenv bootargs " CONFIG_BOOTARGS \
> +		" root=${qspiroot} rw rootfstype=${qspirootfstype};"\
> +		"bootm ${loadaddr} - ${fdt_addr}\0"
> +
> +/* Environment */
> +#define CONFIG_ENV_IS_IN_SPI_FLASH
> +
> +/* Enable SPI NOR flash reset, needed for SPI booting */
> +#define CONFIG_SPI_N25Q256A_RESET
> +
> +/* Environment setting for SPI flash */
> +#define CONFIG_SYS_REDUNDAND_ENVIRONMENT
> +#define CONFIG_ENV_SECT_SIZE	(64 * 1024)
> +#define CONFIG_ENV_SIZE		(16 * 1024)
> +#define CONFIG_ENV_OFFSET	(0x00040000)

Parenthesis not needed.

> +#define CONFIG_ENV_OFFSET_REDUND (CONFIG_ENV_OFFSET +
> CONFIG_ENV_SECT_SIZE) +#define CONFIG_ENV_SPI_BUS	0
> +#define CONFIG_ENV_SPI_CS	0
> +#define CONFIG_ENV_SPI_MODE	SPI_MODE_3
> +#define CONFIG_ENV_SPI_MAX_HZ	CONFIG_SF_DEFAULT_SPEED
> +
> +/* U-Boot payload is stored at offset 0x60000 */
> +#define CONFIG_SYS_SPI_U_BOOT_OFFS	0x60000
> +
> +/*
> + * Bootcounter
> + */
> +#define CONFIG_BOOTCOUNT_LIMIT
> +/* last 2 lwords in OCRAM */
> +#define CONFIG_SYS_BOOTCOUNT_ADDR	0xfffffff8
> +#define CONFIG_SYS_BOOTCOUNT_BE

It might be better to use some scratch registers for this, no ?
Especially since you can get SRAM corruption if you fiddle with
SRAM ECC configuration.

> +/* The rest of the configuration is shared */
> +#include <configs/socfpga_common.h>
> +
> +#endif	/* __CONFIG_SOCFPGA_SR1500_H__ */