[U-Boot] [PATCH v2] arm: mxs: add support for I2SE's Duckbill boards

[Marek Vasut <marex@denx.de>]

On Sunday, December 13, 2015 at 10:53:30 PM, Michael Heimpold wrote:
> Hi Marek,

Hi!

> thanks for your review. My comments inline below.

np :)

> Am Sunday 13 December 2015, 16:40:14 schrieb Marek Vasut:
> > On Sunday, December 13, 2015 at 12:09:58 PM, Michael Heimpold wrote:
> > 
> > Commit message describing the board would be real nice.
> 
> Ok, I'll add it in v3.
> 
> [...]
> 
> > >  #define MACH_TYPE_COLIBRI_T30          4493
> > >  #define MACH_TYPE_APALIS_T30           4513
> > >  #define MACH_TYPE_OMAPL138_LCDK        2495
> > > 
> > > +#define MACH_TYPE_DUCKBILL             4754
> > 
> > This board is still using mach id to boot kernel ? Wow ...
> 
> Not really. But I looked at other iMX28 boards and all still define such
> a MACH_TYPE. For historic reasons, only?

Yes, they are (probably) capable of booting some obscure pre-DT kernel versions.

> However, I think it does not hurt,
> so some customers who think they still need to go with old Freescale kernel
> could use this...

OK

> [...]
> 
> > > +
> > > +#ifdef	CONFIG_CMD_MMC
> > 
> > #ifdef[SPACE] please, not [TAB]
> 
> I heavily used m28evk and mx28evk as reference, so I thought
> that tab is the preferred style, but I can change, have no strong opinion
> on this...

Space please ;-)

> > [...]
> > 
> > > +void mx28_adjust_mac(int dev_id, unsigned char *mac)
> > > +{
> > > +	mac[0] = 0x00;
> > > +	mac[1] = 0x01;
> > > +	mac[2] = 0x87;

HERE

> > > +}
> > > +#endif
> > > +
> > > +#ifdef CONFIG_OF_BOARD_SETUP
> > > +int ft_board_setup(void *blob, bd_t *bd)
> > > +{
> > > +	uint8_t enetaddr[6];
> > > +	u32 mac = 0;
> > > +
> > > +	enetaddr[0] = 0x00;
> > > +	enetaddr[1] = 0x01;
> > > +	enetaddr[2] = 0x87;

HERE

> > Looks like there are two copies of the same OUI ?
> 
> I'm not sure, whether I understand the question correctly?

See the "HERE" markers above, you have two copies of the same thing in
the code for whatever reason.

> All Duckbill devices only have one Ethernet interface. For this
> interface the MAC is programmed into the first OTP fuse register
> as this seems to be best practise on this platform. Since this register
> is only 24 bit, the first two byte must be adjusted by board code.
> The "Duckbill SPI" is an evaluation platform for QCA7000 powerline
> chip. On this devices, the second OTP register is programmed with
> a second MAC adress, but this address must be passed via DT to kernel.
> Both addresses usually have the same OUI, thats correct.

See above, it's just a nitpick.

> > > +#ifdef CONFIG_MXS_OCOTP
> > > +	/* only Duckbill SPI has a MAC for the QCA7k */
> > > +	fuse_read(0, 1, &mac);
> > > +#endif
> > > +
> > > +	if (mac != 0) {
> > > +		enetaddr[3] = (mac >> 16) & 0xff;
> > > +		enetaddr[4] = (mac >>  8) & 0xff;
> > > +		enetaddr[5] =  mac        & 0xff;
> > > +
> > > +		fdt_find_and_setprop(blob,
> > > +
> > > "/apb at 80000000/apbh at 80000000/ssp at 80014000/ethernet at 0", +
> > > 
> > >     "local-mac-address", enetaddr, 6, 1);
> > 
> > You can use aliases {} to locate the ethernet node here.
> 
> "can" == "should" ? :-)

Yes, it's a good idea. It's also how U-Boot patches $ethaddr into the
DT, it uses the aliases to do that.

> I'll have a look, but AFAIR in linux' imx28.dtsi, ethernet1 alias is
> already used for mac1. I'm not sure, if I can redefine it in board DT
> to point to SPI node...

You can always define ethernet2 .

> [...]
> 
> > > +
> > > +	/* MX28_PAD_LCD_D17__GPIO_1_17: v1 = pull-down, v2 = pull-up */
> > > +	system_rev =
> > > +		gpio_get_value(MX28_PAD_LCD_D17__GPIO_1_17);
> > 
> > Does gpio_get_value() always return 0/1 value ? I don't think so.
> 
> Hm, on mxs this seems to be the case, and include/asm-generic/gpio.h
> tells this, too - but also tells, that this interface described there is
> deprecated? It seems that I do not see the forest for the trees looking
> for the API docu... Anyway, I would simply add !! to do the trick...

I think that's a good idea, I wouldn't depend on it returning 0/1 .

> [...]
> 
> > > +/* FEC Ethernet on SoC */
> > > +#ifdef	CONFIG_CMD_NET
> > > +#define CONFIG_FEC_MXC
> > > +#define CONFIG_NET_MULTI
> > > +#define CONFIG_MX28_FEC_MAC_IN_OCOTP
> > > +#define CONFIG_FEC_MXC_PHYADDR	1
> > > +#define IMX_FEC_BASE		MXS_ENET0_BASE
> > 
> > This IMX_FEC_BASE is definitelly unused on MXS.
> 
> I use fecmxc_initialize in the board setup, not fecmxc_initialize_multi,
> and thus this define is required. However, I could switch to
> fecmxc_initialize_multi and the I could drop CONFIG_FEC_MXC_PHYADDR
> and IMX_FEC_BASE...

Oh I see. Whichever you prefer then.

> > > +#endif
> > > +
> > > +#define CONFIG_IPADDR		192.168.1.10
> > > +#define CONFIG_SERVERIP		192.168.1.1
> > > +#define CONFIG_NETMASK		255.255.255.0
> > > +#define CONFIG_GATEWAYIP	192.168.1.254
> > 
> > Definitelly remove these, you should never ever hard-code these settings
> > into U-Boot.
> 
> I don't understand. This are only default settings in case, U-Boot starts
> with an empty environment. Other boards do the same, so what's wrong here?

Other boards should not do that, that's the agreement AFAICT.

> > > +/* BOOTP options */
> > > +#define CONFIG_BOOTP_SUBNETMASK
> > > +#define CONFIG_BOOTP_GATEWAY
> > > +#define CONFIG_BOOTP_HOSTNAME
> > 
> > DTTO
> 
> These defines affect what is requested in the DHCP/BOOTP packet. I do not
> think, it can drop it.

Ah ok, you need that on your board. OK.

> > > +/* SPI */
> > > +#ifdef CONFIG_CMD_SPI
> > > +#define CONFIG_DEFAULT_SPI_BUS		2
> > 
> > Add default CS please
> 
> I think I'll prefer to drop the SPI block completely, no real usage.

Don't you use SPI for your ethernet device ?

> > > +#define CONFIG_DEFAULT_SPI_MODE		SPI_MODE_0
> > > +#endif
> > > +
> > > +/* Boot Linux */
> > > +#define CONFIG_BOOTDELAY	1
> > > +#define CONFIG_BOOTFILE		"zImage"
> > 
> > Why don't you switch to fitImage ?
> 
> I don't see a huge gain: zImage and DT are loaded from /boot on ext4
> filesystem, so customers can easily change both files without fiddling
> to create fit image...

On the other hand, there is no real integrity protection in zImage. That's
the real gain. Also, adding cryptographic support into the system is then
easy.

> > > +#define CONFIG_LOADADDR		0x42000000
> > > +#define CONFIG_SYS_LOAD_ADDR	CONFIG_LOADADDR
> > > +#define CONFIG_REVISION_TAG
> > > +#define CONFIG_SERIAL_TAG
> > > +#define CONFIG_OF_BOARD_SETUP
> > > +#define CONFIG_BOOT_RETRY_TIME		120	/* retry autoboot after
> > 
> > 120 seconds */
> > 
> > > +#define CONFIG_BOOT_RETRY_TIME_MIN	1	/* can go down to 1 
second */
> > > +#define CONFIG_AUTOBOOT_KEYED
> > > +#define CONFIG_AUTOBOOT_PROMPT		"Autobooting in %d seconds, " \
> > > +					"press <c> to stop\n"
> > > +#define CONFIG_AUTOBOOT_DELAY_STR	"\x63"	/* allows retry after 
retry
> 
> time
> 
> > How does this work ?
> 
> Hardware misses a pull-up on debug uart Rx. Using autoboot feature prevents
> U-Boot to not stop the boot process because of a "ghost input" on serial
> line.

Eep.

> > > */ +#define CONFIG_AUTOBOOT_STOP_STR	" "	/* stop autoboot with 
<Space>
> > > */ +#define CONFIG_RESET_TO_RETRY			/* reset board to 
retry
> > 
> > booting */ +
> > 
> > > [...]
> 
> Thanks,
> Michael