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 <s...@denx.de>
Cc: Marek Vasut <ma...@denx.de>
Cc: Pavel Machek <pa...@denx.de>
Cc: Dinh Nguyen <dingu...@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@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
