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

[Marek Vasut]

On Monday, October 26, 2015 at 09:17:41 AM, Stefan Roese wrote:
> Hi Marek,

Hi,

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

Maybe we should place those boards under board/johndoe/ or something then ?
There is way too many boards in boards/ I think. But either way, I won't
oppose to keeping it just under boards/ either.

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

Sure, thanks!

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

OK

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

Argh, I absolutelly don't like this. Also, I don't like the reasoning, it looks 
vaguely similar to certain other "system", where a certain group got a certain 
feature widely used and then "forced" patches mainline just because that feature
is in use by many by now.

Maybe you can at least pull these hacks into some compat.c file with a big 
warning somewhere in there that this is certainly NOT how things were supposed
to be done ?

[...]

> >> +/*
> >> + * 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.

OK, no problem with this. Just keep the ECC part in mind.