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

Sun Dec 13 22:53:30 CET 2015

Hi Marek,

thanks for your review. My comments inline below.

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? However, I think it does not hurt,
so some customers who think they still need to go with old Freescale kernel
could use this...

[...]

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

> [...]
> 
> > +void mx28_adjust_mac(int dev_id, unsigned char *mac)
> > +{
> > +	mac[0] = 0x00;
> > +	mac[1] = 0x01;
> > +	mac[2] = 0x87;
> > +}
> > +#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;
> 
> Looks like there are two copies of the same OUI ?

I'm not sure, whether I understand the question correctly?
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.

> 
> > +#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" ? :-)
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...

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

[...]

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

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

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

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

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

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

Thanks,
Michael