Hi Stefano,
On Mon, 02 Mar 2015 11:17:42 +0100
Stefano Babic <sba...@denx.de> wrote:
> Hi Boris,
>
> On 16/02/2015 14:27, Boris Brezillon wrote:
> > Add basic SECO MX6Q/uQ7 board support (Ethernet, UART, SD are supported).
> > It also adds a Kconfig skeleton to later add more SECO board (supporting
> > SoC and board variants).
> >
> > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> > ---
> > arch/arm/cpu/armv7/mx6/Kconfig | 11 +++
> > board/seco/Kconfig | 63 ++++++++++++++
> > board/seco/common/Makefile | 2 +
> > board/seco/common/mx6.c | 137 ++++++++++++++++++++++++++++++
> > board/seco/common/mx6.h | 9 ++
> > board/seco/mx6quq7/Makefile | 7 ++
> > board/seco/mx6quq7/mx6quq7-2g.cfg | 173
> > ++++++++++++++++++++++++++++++++++++++
> > board/seco/mx6quq7/mx6quq7.c | 165
> > ++++++++++++++++++++++++++++++++++++
> > configs/secomx6quq7_defconfig | 7 ++
> > include/configs/secomx6quq7.h | 162 +++++++++++++++++++++++++++++++++++
>
> MAINTAINERS file is missing.
>
> I have tried your patches, I think there is something not coherent with
> names and the board cannot be built clean with buildman. Board is
> recognized as secomx6quq7, but then there is a mismatch in the board
> object library.
>
> You set:
>
> obj-y := mx6q-uq7.o
>
>
> but the source is mx6quq7.c and building breaks with
>
> make[1]: *** No rule to make target `board/seco/mx6quq7/mx6q-uq7.o',
> needed by `board/seco/mx6quq7/built-in.o'. Stop.
Indeed.
I renamed all the files to match what's done for other boards (no
dashes or underscores in names), but apparently forgot to change it in
the Makefile.
I didn't notice it when compiling because I still add an mx6q-uq7.o file
in my directory.
Anyway, I'll fix that.
[...]
> > +void seco_mx6_setup_usdhc_iomux(int id)
> > +{
> > + switch (id) {
> > + case 3:
> > + imx_iomux_v3_setup_multiple_pads(usdhc3_pads,
> > + ARRAY_SIZE(usdhc3_pads));
> > + break;
> > +
> > + case 4:
> > + imx_iomux_v3_setup_multiple_pads(usdhc4_pads,
> > + ARRAY_SIZE(usdhc4_pads));
> > + break;
> > +
> > + default:
>
> Maybe it is better to print an error due to wrong id.
Sure, I'll add an error message.
[...]
> > +int board_mmc_init(bd_t *bis)
> > +{
> > + int status = 0;
> > + u32 index = 0;
> > +
> > + /*
> > + * Following map is done:
> > + * (U-boot device node) (Physical Port)
> > + * mmc0 eMMC on Board
> > + * mmc1 Ext SD
> > + */
> > + for (index = 0; index < CONFIG_SYS_FSL_USDHC_NUM; ++index) {
> > + switch (index) {
> > + case 0:
> > + seco_mx6_setup_usdhc_iomux(3);
> > + usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK);
> > + usdhc_cfg[0].max_bus_width = 4;
> > + break;
> > + case 1:
> > + seco_mx6_setup_usdhc_iomux(4);
> > + usdhc_cfg[1].sdhc_clk = mxc_get_clock(MXC_ESDHC4_CLK);
> > + usdhc_cfg[1].max_bus_width = 4;
> > + break;
> > +
> > + default:
> > + printf("Warning: %d exceed maximum number of SD ports
> > %d\n",
> > + index + 1, CONFIG_SYS_FSL_USDHC_NUM);
> > + return status;
> > + }
> > +
> > + status |= fsl_esdhc_initialize(bis, &usdhc_cfg[index]);
>
> Fabio cleaned up all for all boards these statement, check for example
> the sabresd. Instead of cumulating the error, the loop stops at the
> first failure. Do the same to be coherent.
Okay, I'll have a look.
>
> > + }
> > +
> > + return status;
> > +}
> > +
> > +int board_init(void)
> > +{
> > + /* address of boot parameters */
> > + gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
> > +
> > + imx_iomux_v3_setup_pad(MX6_PAD_NANDF_D4__GPIO2_IO04 |
> > + MUX_PAD_CTRL(NO_PAD_CTRL));
> > +
> > + gpio_direction_output(IMX_GPIO_NR(2, 4), 0);
> > +
>
> Is it reset for phy ? Can you add a comment to explain what it is ?
I don't know what this pin is controlling, but if I don't do the
following toggle sequence, the board hangs.
Note that I don't have the schematics of this board, and all my work is
based on SECO's BSP.
>
> > + /* Set Low */
> > + gpio_set_value(IMX_GPIO_NR(2, 4), 0);
> > + udelay(1000);
> > +
> > + /* Set High */
> > + gpio_set_value(IMX_GPIO_NR(2, 4), 1);
> > +
> > + return 0;
> > +}
> > +
> > +int board_late_init(void)
> > +{
> > + return 0;
> > +}
>
> Drop it, as well as CONFIG_BOARD_LATE_INIT
Ok.
[...]
> > +#define CONFIG_EXTRA_ENV_SETTINGS \
> > + "netdev=eth0\0" \
> > + "ethprime=FEC0\0" \
> > + "netdev=eth0\0" \
> > + "ethprime=FEC0\0" \
> > + "uboot=u-boot.bin\0" \
> > + "kernel=uImage\0" \
> > + "nfsroot=/opt/eldk/arm\0" \
> > + "ip_local=10.0.0.5::10.0.0.1:255.255.255.0::eth0:off\0" \
> > + "ip_server=10.0.0.1\0" \
> > + "nfs_path=/targetfs \0" \
> > + "memory=mem=1024M\0" \
> > + "bootdev=mmc dev 0; ext2load mmc 0:1\0" \
> > + "root=root=/dev/mmcblk0p1\0" \
> > + "option=rootwait rw fixrtc rootflags=barrier=1\0" \
> > + "cpu_freq=arm_freq=996\0" \
> > + "setbootargs=setenv bootargs console=ttymxc1,115200 ${root} ${option}
> > ${memory} ${cpu_freq}\0" \
> > + "setbootargs_nfs=setenv bootargs console=ttymxc1,115200 root=/dev/nfs
> > nfsroot=${ip_server}:${nfs_path} nolock,wsize=4096,rsize=4096
> > ip=:::::eth0:dhcp ${memory} ${cpu_freq}\0" \
>
> checkpatch is not very happy with these very long lines, can you fix
> them, please ?
Yes I know, I just wasn't sure if there was a rule like 'do not split
bootargs definitions in multi-line strings'.
I'll split the lines appropriately.
>
> > + "setbootdev=setenv boot_dev ${bootdev} 10800000 /boot/uImage\0" \
> > + "bootcmd=run setbootargs; run setbootdev; run boot_dev; bootm
> > 0x10800000\0" \
> > + "stdin=serial\0" \
> > + "stdout=serial\0" \
> > + "stderr=serial\0"
> > +
> > +
> > +/* Miscellaneous configurable options */
> > +#define CONFIG_SYS_LONGHELP
> > +#define CONFIG_SYS_HUSH_PARSER
> > +#define CONFIG_SYS_PROMPT "SECO MX6Q uQ7 U-Boot > "
> > +
> > +#define CONFIG_AUTO_COMPLETE
> > +#define CONFIG_SYS_CBSIZE 256
> > +
> > +/* Print Buffer Size */
> > +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + \
> > + sizeof(CONFIG_SYS_PROMPT) + 16)
> > +#define CONFIG_SYS_MAXARGS 16
> > +#define CONFIG_SYS_BARGSIZE CONFIG_SYS_CBSIZE
> > +
> > +#define CONFIG_SYS_LOAD_ADDR CONFIG_LOADADDR
> > +#define CONFIG_SYS_HZ 1000
> > +
> > +#define CONFIG_CMDLINE_EDITING
> > +
> > +
> > +/* Physical Memory Map */
> > +#define CONFIG_NR_DRAM_BANKS 1
> > +#define PHYS_SDRAM MMDC0_ARB_BASE_ADDR
> > +#define PHYS_SDRAM_SIZE (2u * 1024 * 1024 * 1024)
> > +
> > +#define CONFIG_SYS_SDRAM_BASE PHYS_SDRAM
> > +#define CONFIG_SYS_INIT_RAM_ADDR IRAM_BASE_ADDR
> > +#define CONFIG_SYS_INIT_RAM_SIZE IRAM_SIZE
> > +
> > +#define CONFIG_SYS_INIT_SP_OFFSET \
> > + (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE)
> > +#define CONFIG_SYS_INIT_SP_ADDR \
> > + (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET)
> > +
> > +/* FLASH and environment organization */
> > +#define CONFIG_SYS_NO_FLASH
> > +
> > +#define CONFIG_ENV_SIZE (8 * 1024)
> > +
> > +#if defined(CONFIG_ENV_IS_IN_MMC)
> > + #define CONFIG_ENV_OFFSET (6 * 128 * 1024)
> > + #define CONFIG_SYS_MMC_ENV_DEV 0
> > + #define CONFIG_DYNAMIC_MMC_DEVNO
> > +#endif
> > +
> > +#define CONFIG_OF_LIBFDT
> > +#define CONFIG_CMD_BOOTZ
> > +
> > +#ifndef CONFIG_SYS_DCACHE_OFF
> > +#define CONFIG_CMD_CACHE
> > +#endif
> > +
> > +#endif /* __CONFIG_H */
> >
>
Thanks for your review.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
