On 14/12/2011 20:52, Fabio Estevam wrote:
> Add initial support for Freescale MX28EVK board.
> 
> Tested boot via SD card and by loading a kernel via TFTP through
> the FEC interface.
> 
> Signed-off-by: Fabio Estevam <fabio.este...@freescale.com>
> ---

Hi Fabio,

> - Currently MAC addresses are being taken from environment variables:
> set ethaddr xx:xx:xx:xx:xx:xx
> set eth1addr yy:yy:yy:yy:yy:yy
> 
> - For correct operation of saving environment variables into the SD card,
> the following patch is needed:
> http://lists.denx.de/pipermail/u-boot/2011-November/111448.html
> 
>  MAINTAINERS                       |    1 +
>  board/freescale/mx28evk.c         |  190 
> +++++++++++++++++++++++++++++++++++++

Slipped in wrong directory, I presume

> +
> +#define      HW_DIGCTRL_SCRATCH0     0x8001c280
> +#define      HW_DIGCTRL_SCRATCH1     0x8001c290
> +int dram_init(void)
> +{
> +     uint32_t sz[2];
> +
> +     sz[0] = readl(HW_DIGCTRL_SCRATCH0);
> +     sz[1] = readl(HW_DIGCTRL_SCRATCH1);
> +
> +     if (sz[0] != sz[1]) {
> +             printf("MX28:\n"
> +                     "Error, the RAM size in HW_DIGCTRL_SCRATCH0 and\n"
> +                     "HW_DIGCTRL_SCRATCH1 is not the same. Please\n"
> +                     "verify these two registers contain valid RAM size!\n");

Here we should use puts instead of printf.

> +             hang();
> +     }
> +
> +     gd->ram_size = sz[0];
> +     return 0;
> +}

This function is copied from m28evk.c. Can we factorize it ? Then the
board dram_init() can call the common function if a special
implementation is still needed.

> +#ifdef       CONFIG_CMD_MMC
> +static int mx28evk_mmc_wp(int id)
> +{
> +     if (id != 0) {
> +             printf("MXS MMC: Invalid card selected (card id = %d)\n", id);
> +             return 1;
> +     }
> +
> +     return gpio_get_value(MX28_PAD_SSP1_SCK__GPIO_2_12);
> +}
> +
> +int board_mmc_init(bd_t *bis)
> +{
> +     /* Configure WP as input */
> +     gpio_direction_input(MX28_PAD_SSP1_SCK__GPIO_2_12);
> +
> +     /* Configure MMC0 Power Enable */
> +     gpio_direction_output(MX28_PAD_PWM3__GPIO_3_28, 0);
> +
> +     return mxsmmc_initialize(bis, 0, mx28evk_mmc_wp);
> +}
> +#endif
> +
> +#ifdef       CONFIG_CMD_NET
> +
> +#define      MII_OPMODE_STRAP_OVERRIDE       0x16
> +#define      MII_PHY_CTRL1                   0x1e
> +#define      MII_PHY_CTRL2                   0x1f
> +
> +int fecmxc_mii_postcall(int phy)
> +{
> +     miiphy_write("FEC1", phy, MII_BMCR, 0x9000);
> +     miiphy_write("FEC1", phy, MII_OPMODE_STRAP_OVERRIDE, 0x0202);
> +     if (phy == 3)
> +             miiphy_write("FEC1", 3, MII_PHY_CTRL2, 0x8180);
> +     return 0;
> +}
> +
> +int board_eth_init(bd_t *bis)
> +{
> +     struct mx28_clkctrl_regs *clkctrl_regs =
> +             (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> +     struct eth_device *dev;
> +     int ret;
> +
> +     ret = cpu_eth_init(bis);
> +
> +     /* MX28EVK uses ENET_CLK PAD to drive FEC clock */
> +     writel(CLKCTRL_ENET_TIME_SEL_RMII_CLK | CLKCTRL_ENET_CLK_OUT_EN,
> +                                     &clkctrl_regs->hw_clkctrl_enet);

Right, I know this issue.

> +     /* Power-on FECs */
> +     gpio_direction_output(MX28_PAD_SSP1_DATA3__GPIO_2_15, 0);
> +
> +     /* Reset FEC PHYs */
> +     gpio_direction_output(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 0);
> +     udelay(200);
> +     gpio_set_value(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 1);
> +
> +     ret = fecmxc_initialize_multi(bis, 0, 0, MXS_ENET0_BASE);
> +     if (ret) {
> +             printf("FEC MXS: Unable to init FEC0\n");

Use puts, check globally

> +     ret = fecmxc_initialize_multi(bis, 1, 3, MXS_ENET1_BASE);
> +     if (ret) {
> +             printf("FEC MXS: Unable to init FEC1\n");

Ditto

> +void imx_get_mac_from_fuse(char *mac)
> +{
> +     memset(mac, 0, 6);
> +}
> +
> +#endif

Sure imx_get_mac_from_fuse() belong to the processor and not to the
board - it must be moved. If it is not desired to get the MAC address
from the fuses (have Freescale's SOC always a valid MAC in fuses or are
they also delivered with empty MAC ?), you can add a CONFIG_ define to
fall back to the implementation in this patch clearing the MAC.

> diff --git a/include/configs/mx28evk.h b/include/configs/mx28evk.h
> new file mode 100644
> index 0000000..2c8d06b
> --- /dev/null
> +++ b/include/configs/mx28evk.h
> @@ -0,0 +1,164 @@
> +/*
> + * Copyright (C) 2011 Marek Vasut <marek.va...@gmail.com>
> + * on behalf of DENX Software Engineering GmbH
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.       See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __MX28_H__
> +#define __MX28_H__
> +
> +#include <asm/arch/regs-base.h>
> +
> +/*
> + * SoC configurations
> + */
> +#define      CONFIG_MX28                             /* i.MX28 SoC */
> +#define      CONFIG_MXS_GPIO                         /* GPIO control */
> +#define      CONFIG_SYS_HZ           1000            /* Ticks per second */
> +
> +#define CONFIG_MACH_TYPE     MACH_TYPE_MX28EVK
> +
> +#define      CONFIG_SYS_NO_FLASH
> +#define      CONFIG_SYS_ICACHE_OFF
> +#define      CONFIG_SYS_DCACHE_OFF
> +#define      CONFIG_BOARD_EARLY_INIT_F
> +#define      CONFIG_ARCH_CPU_INIT
> +#define      CONFIG_ARCH_MISC_INIT
> +
> +/*
> + * SPL
> + */
> +#define CONFIG_SPL
> +#define CONFIG_SPL_NO_CPU_SUPPORT_CODE
> +#define CONFIG_SPL_START_S_PATH      "arch/arm/cpu/arm926ejs/mx28"
> +#define CONFIG_SPL_LDSCRIPT  "arch/arm/cpu/arm926ejs/mx28/u-boot-spl.lds"
> +#define CONFIG_SPL_LIBCOMMON_SUPPORT
> +#define CONFIG_SPL_LIBGENERIC_SUPPORT
> +
> +/*
> + * U-Boot Commands
> + */
> +#include <config_cmd_default.h>
> +#define      CONFIG_DISPLAY_CPUINFO
> +#define      CONFIG_DOS_PARTITION
> +#define CONFIG_CMD_FAT
> +
> +#define      CONFIG_CMD_CACHE
> +#define      CONFIG_CMD_DHCP
> +#define      CONFIG_CMD_GPIO
> +#define      CONFIG_CMD_MII
> +#define      CONFIG_CMD_MMC
> +#define      CONFIG_CMD_NET
> +#define      CONFIG_CMD_NFS
> +#define      CONFIG_CMD_PING
> +
> +/*
> + * Memory configurations
> + */
> +#define      CONFIG_NR_DRAM_BANKS            1               /* 1 bank of 
> DRAM */
> +#define      PHYS_SDRAM_1                    0x40000000      /* Base address 
> */
> +#define      PHYS_SDRAM_1_SIZE               0x40000000      /* Max 1 GB RAM 
> */
> +#define      CONFIG_STACKSIZE                0x00010000      /* 128 KB stack 
> */
                                                ^-- I read 64KB


> +#define      CONFIG_SYS_MALLOC_LEN           0x00400000      /* 4 MB for 
> malloc */
> +#define      CONFIG_SYS_GBL_DATA_SIZE        128             /* Initial data 
> */
                
Why is not used GENERATED_GBL_DATA_SIZE ? The same question I had to
post for m28evk, I know, but it seems I have not seen there during review.

> +
> +/*
> + * MMC Driver
> + */
> +#define CONFIG_ENV_IS_IN_MMC
> +#define CONFIG_ENV_OFFSET    (256 * 1024)
> +#define CONFIG_ENV_SIZE              (16 * 1024)
> +#define CONFIG_SYS_MMC_ENV_DEV 0
> +#define CONFIG_CMD_SAVEENV
> +#ifdef       CONFIG_CMD_MMC

Should be not CONFIG_ENV_IS_IN_MMC put inside the #ifdef ? We cannot use
CONFIG_ENV_IS_IN_MMC if CONFIG_MMC is not set.

> +/*
> + * Extra Environments
> + */
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +     "console_fsl=console=ttyAM0" \
> +     "console_mainline=console=ttyAMA0" \

This depends on the tty driver in kernel - as far as I know, it is
ttyAMA. Which is the reason for ttyAM0 ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to