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

[Stefan Roese]

Hi Marek,

On 23.10.2015 20:40, Marek Vasut wrote:
> On Friday, October 23, 2015 at 09:26:53 AM, Stefan Roese wrote:
>> The SR1500
>
> Does SR mean Stefan Roese ? :-)

Not really. I had no influence on this board naming. But I
really like it! ;)

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

Placing the board directly in the "board" directory is common practise
for boards without a vendor (or where the vendor doesn't want to be
listed).

> 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.

Let me see, if I can cook something up here quickly.

>> 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 at denx.de>
>> Cc: Marek Vasut <marex at denx.de>
>> Cc: Pavel Machek <pavel at denx.de>
>> Cc: Dinh Nguyen <dinguyen at 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.

Yes, makes sense. I'll re-tune this in v2.

>> +	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 ?

This code is already pretty old. And was written in some board bringup
session quite hastily. This explains the quite poor quality. Sorry,
I should I have spent a bit time on the cleanup here - totally
forgot about that.

But this command is now part of the manufacturing process. So I
would really like to keep it.

>> +#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' :-)

Good catch. Thx.

>> +		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 ? ;-)

No.

>> +	}
>> +	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.

While adding this bootcounter support for this board a few months
ago I could not come up with any scratch registers for this quickly.
Internal SRAM is often used for this feature, so I used it. And it
works just fine. I would really like to keep it this way,
especially since the board is already in production and also using
this location in the Linux version of this bootcounter driver.

Thanks for the review.

Thanks,
Stefan