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@80000000/apbh@80000000/ssp@80014000/ethernet@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
