[V4,6/9] SMDK5250: Initialise and Enable DWMMC, support FDT and non-FDT

This patch enables and initialises DWMMC for SMDK5250.
Supports both FDT and non-FDT. This patch creates a new file
'exynos5-dt.c' meant for FDT support.
        exynos5-dt.c:   This file shall contain all code which supports FDT.
                        Any addition of FDT support for any module needs to be
                        added in this file.
        smdk5250.c:     This file shall contain the code which supports non-FDT.
                        version. Any addition of non-FDT support for any module
                        needs to be added in this file.
                        May be, the file smdk5250.c can be removed in near future
                        when non-FDT is not required.

The Makefile is updated to compile only one of the files
exynos5-dt.c / smdk5250.c based on FDT configuration.

NOTE:
Please note that all additions corresponding to FDT need to be added into the
file exynos5-dt.c.
At same time if non-FDT support is required then add the corresponding
updations into smdk5250.c.

Changes from V1:
        1)A new file 'exynos5-dt.c' is created meant for FDT support
        2)Makefile is updated to compile only one of the files
        exynos5-dt.c / smdk5250.c based on FDT configuration

Changes from V2:
        1)Updation of commit message and resubmition of proper patch set.

Changes from V3:
        No change.

Signed-off-by: Amar <amarendra.xt@samsung.com>
---
 board/samsung/smdk5250/Makefile     |   4 +
 board/samsung/smdk5250/exynos5-dt.c | 242 ++++++++++++++++++++++++++++++++++++
 board/samsung/smdk5250/smdk5250.c   |  97 +++++++--------
 include/configs/exynos5250-dt.h     |   2 +
 include/i2c.h                       |   2 +
 5 files changed, 292 insertions(+), 55 deletions(-)
 create mode 100644 board/samsung/smdk5250/exynos5-dt.c

Hi Amar,

On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
> This patch enables and initialises DWMMC for SMDK5250.
> Supports both FDT and non-FDT. This patch creates a new file
> 'exynos5-dt.c' meant for FDT support.
>         exynos5-dt.c:   This file shall contain all code which supports FDT.
>                         Any addition of FDT support for any module needs to be
>                         added in this file.
>         smdk5250.c:     This file shall contain the code which supports non-FDT.
>                         version. Any addition of non-FDT support for any module
>                         needs to be added in this file.
>                         May be, the file smdk5250.c can be removed in near future
>                         when non-FDT is not required.
>
> The Makefile is updated to compile only one of the files
> exynos5-dt.c / smdk5250.c based on FDT configuration.
>
> NOTE:
> Please note that all additions corresponding to FDT need to be added into the
> file exynos5-dt.c.
> At same time if non-FDT support is required then add the corresponding
> updations into smdk5250.c.
>
> Changes from V1:
>         1)A new file 'exynos5-dt.c' is created meant for FDT support
>         2)Makefile is updated to compile only one of the files
>         exynos5-dt.c / smdk5250.c based on FDT configuration
>
> Changes from V2:
>         1)Updation of commit message and resubmition of proper patch set.
>
> Changes from V3:
>         No change.
>
> Signed-off-by: Amar <amarendra.xt@samsung.com>
> ---
>  board/samsung/smdk5250/Makefile     |   4 +
>  board/samsung/smdk5250/exynos5-dt.c | 242 ++++++++++++++++++++++++++++++++++++
>  board/samsung/smdk5250/smdk5250.c   |  97 +++++++--------
>  include/configs/exynos5250-dt.h     |   2 +
>  include/i2c.h                       |   2 +
>  5 files changed, 292 insertions(+), 55 deletions(-)
>  create mode 100644 board/samsung/smdk5250/exynos5-dt.c
>
> diff --git a/board/samsung/smdk5250/Makefile b/board/samsung/smdk5250/Makefile
> index 47c6a5a..ecca9f3 100644
> --- a/board/samsung/smdk5250/Makefile
> +++ b/board/samsung/smdk5250/Makefile
> @@ -32,8 +32,12 @@ COBJS        += tzpc_init.o
>  COBJS  += smdk5250_spl.o
>
>  ifndef CONFIG_SPL_BUILD
> +ifdef CONFIG_OF_CONTROL
> +COBJS  += exynos5-dt.o
> +else
>  COBJS  += smdk5250.o
>  endif
> +endif
>
>  ifdef CONFIG_SPL_BUILD
>  COBJS  += spl_boot.o
> diff --git a/board/samsung/smdk5250/exynos5-dt.c b/board/samsung/smdk5250/exynos5-dt.c
> new file mode 100644
> index 0000000..da539ca
> --- /dev/null
> +++ b/board/samsung/smdk5250/exynos5-dt.c
> @@ -0,0 +1,242 @@
> +/*
> + * Copyright (C) 2012 Samsung Electronics
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <asm/io.h>
> +#include <i2c.h>
> +#include <netdev.h>
> +#include <spi.h>
> +#include <asm/arch/cpu.h>
> +#include <asm/arch/dwmmc.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/arch/mmc.h>
> +#include <asm/arch/pinmux.h>
> +#include <asm/arch/sromc.h>
> +#include <power/pmic.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int board_init(void)
> +{
> +       gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL);
> +#ifdef CONFIG_EXYNOS_SPI
> +       spi_init();
> +#endif
> +       return 0;
> +}
> +
> +int dram_init(void)
> +{
> +       gd->ram_size    = get_ram_size((long *)PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_2, PHYS_SDRAM_2_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_3, PHYS_SDRAM_3_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_4, PHYS_SDRAM_4_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_5, PHYS_SDRAM_7_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_6, PHYS_SDRAM_7_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_7, PHYS_SDRAM_7_SIZE)
> +                       + get_ram_size((long *)PHYS_SDRAM_8, PHYS_SDRAM_8_SIZE);

This looks ugly - is there any other way of doing this? Also 7 appears
in more than one line.

Since the banks are all SDRAM_BANK_SIZE apart, perhaps you could just
use a for loop with a single base address?

If this function is common with the other file then perhaps it should
go in a common file?

> +       return 0;
> +}
> +
> +#if defined(CONFIG_POWER)
> +int power_init_board(void)
> +{
> +       if (pmic_init(I2C_PMIC))

debug()

> +               return -1;
> +       else
> +               return 0;
> +}
> +#endif
> +
> +void dram_init_banksize(void)
> +{
> +       gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> +       gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1,
> +                                                       PHYS_SDRAM_1_SIZE);
> +       gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
> +       gd->bd->bi_dram[1].size = get_ram_size((long *)PHYS_SDRAM_2,
> +                                                       PHYS_SDRAM_2_SIZE);
> +       gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
> +       gd->bd->bi_dram[2].size = get_ram_size((long *)PHYS_SDRAM_3,
> +                                                       PHYS_SDRAM_3_SIZE);
> +       gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
> +       gd->bd->bi_dram[3].size = get_ram_size((long *)PHYS_SDRAM_4,
> +                                                       PHYS_SDRAM_4_SIZE);
> +       gd->bd->bi_dram[4].start = PHYS_SDRAM_5;
> +       gd->bd->bi_dram[4].size = get_ram_size((long *)PHYS_SDRAM_5,
> +                                                       PHYS_SDRAM_5_SIZE);
> +       gd->bd->bi_dram[5].start = PHYS_SDRAM_6;
> +       gd->bd->bi_dram[5].size = get_ram_size((long *)PHYS_SDRAM_6,
> +                                                       PHYS_SDRAM_6_SIZE);
> +       gd->bd->bi_dram[6].start = PHYS_SDRAM_7;
> +       gd->bd->bi_dram[6].size = get_ram_size((long *)PHYS_SDRAM_7,
> +                                                       PHYS_SDRAM_7_SIZE);
> +       gd->bd->bi_dram[7].start = PHYS_SDRAM_8;
> +       gd->bd->bi_dram[7].size = get_ram_size((long *)PHYS_SDRAM_8,
> +                                                       PHYS_SDRAM_8_SIZE);

and here

> +}
> +
> +static int decode_sromc(const void *blob, struct fdt_sromc *config)
> +{
> +       int err;
> +       int node;
> +
> +       node = fdtdec_next_compatible(blob, 0, COMPAT_SAMSUNG_EXYNOS5_SROMC);
> +       if (node < 0) {
> +               debug("Could not find SROMC node\n");
> +               return node;
> +       }
> +
> +       config->bank = fdtdec_get_int(blob, node, "bank", 0);
> +       config->width = fdtdec_get_int(blob, node, "width", 2);
> +
> +       err = fdtdec_get_int_array(blob, node, "srom-timing", config->timing,
> +                       FDT_SROM_TIMING_COUNT);
> +       if (err < 0) {
> +               debug("Could not decode SROMC configuration\n");

Suggest:

debug("Could not decode SROMC configuration: %s\n", fdt_strerror(err));

> +               return -FDT_ERR_NOTFOUND;

return err? Or the caller might just want -1

> +       }
> +
> +       return 0;
> +}
> +
> +int board_eth_init(bd_t *bis)
> +{
> +#ifdef CONFIG_SMC911X
> +       u32 smc_bw_conf, smc_bc_conf;
> +       struct fdt_sromc config;
> +       fdt_addr_t base_addr;
> +       int node;
> +
> +       node = decode_sromc(gd->fdt_blob, &config);
> +       if (node < 0) {
> +               debug("%s: Could not find sromc configuration\n", __func__);
> +               return 0;
> +       }
> +       node = fdtdec_next_compatible(gd->fdt_blob, node, COMPAT_SMSC_LAN9215);
> +       if (node < 0) {
> +               debug("%s: Could not find lan9215 configuration\n", __func__);
> +               return 0;
> +       }
> +
> +       /* We now have a node, so any problems from now on are errors */
> +       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
> +       if (base_addr == FDT_ADDR_T_NONE) {
> +               debug("%s: Could not find lan9215 address\n", __func__);
> +               return -1;
> +       }
> +
> +       /* Ethernet needs data bus width of 16 bits */
> +       if (config.width != 2) {
> +               debug("%s: Unsupported bus width %d\n", __func__,
> +                       config.width);
> +               return -1;
> +       }
> +       smc_bw_conf = SROMC_DATA16_WIDTH(config.bank)
> +                       | SROMC_BYTE_ENABLE(config.bank);
> +
> +       smc_bc_conf = SROMC_BC_TACS(config.timing[FDT_SROM_TACS])   |\

Can you remove the \ from each line?

> +                       SROMC_BC_TCOS(config.timing[FDT_SROM_TCOS]) |\
> +                       SROMC_BC_TACC(config.timing[FDT_SROM_TACC]) |\
> +                       SROMC_BC_TCOH(config.timing[FDT_SROM_TCOH]) |\
> +                       SROMC_BC_TAH(config.timing[FDT_SROM_TAH])   |\
> +                       SROMC_BC_TACP(config.timing[FDT_SROM_TACP]) |\
> +                       SROMC_BC_PMC(config.timing[FDT_SROM_PMC]);
> +
> +       /* Select and configure the SROMC bank */
> +       exynos_pinmux_config(PERIPH_ID_SROMC, config.bank);
> +       s5p_config_sromc(config.bank, smc_bw_conf, smc_bc_conf);
> +       return smc911x_initialize(0, base_addr);
> +#endif
> +       return 0;
> +}
> +
> +#ifdef CONFIG_DISPLAY_BOARDINFO
> +int checkboard(void)
> +{
> +       printf("\nBoard: SMDK5250\n");
> +
> +       return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_GENERIC_MMC
> +int board_mmc_init(bd_t *bis)
> +{
> +       int ret = 0;

Remove =0

> +
> +       /* dwmmc initializattion for available channels */
> +       ret = exynos_dwmmc_init(gd->fdt_blob);
> +       if (ret)
> +               debug("dwmmc init failed\n");
> +
> +       return ret;
> +}
> +#endif
> +
> +static int board_uart_init(void)
> +{
> +       int err;
> +
> +       err = exynos_pinmux_config(PERIPH_ID_UART0, PINMUX_FLAG_NONE);
> +       if (err) {
> +               debug("UART0 not configured\n");
> +               return err;
> +       }
> +
> +       err = exynos_pinmux_config(PERIPH_ID_UART1, PINMUX_FLAG_NONE);
> +       if (err) {
> +               debug("UART1 not configured\n");
> +               return err;
> +       }
> +
> +       err = exynos_pinmux_config(PERIPH_ID_UART2, PINMUX_FLAG_NONE);
> +       if (err) {
> +               debug("UART2 not configured\n");
> +               return err;
> +       }
> +
> +       err = exynos_pinmux_config(PERIPH_ID_UART3, PINMUX_FLAG_NONE);
> +       if (err) {
> +               debug("UART3 not configured\n");
> +               return err;
> +       }

Loop for this?

> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_BOARD_EARLY_INIT_F
> +int board_early_init_f(void)
> +{
> +       int err;

blank line

> +       err = board_uart_init();
> +       if (err) {
> +               debug("UART init failed\n");
> +               return err;
> +       }
> +#ifdef CONFIG_SYS_I2C_INIT_BOARD
> +       board_i2c_init(gd->fdt_blob);
> +#endif
> +       return err;
> +}
> +#endif
> diff --git a/board/samsung/smdk5250/smdk5250.c b/board/samsung/smdk5250/smdk5250.c
> index 73c3ec0..e0fec11 100644
> --- a/board/samsung/smdk5250/smdk5250.c
> +++ b/board/samsung/smdk5250/smdk5250.c
> @@ -27,6 +27,7 @@
>  #include <netdev.h>
>  #include <spi.h>
>  #include <asm/arch/cpu.h>
> +#include <asm/arch/dwmmc.h>
>  #include <asm/arch/gpio.h>
>  #include <asm/arch/mmc.h>
>  #include <asm/arch/pinmux.h>
> @@ -95,59 +96,13 @@ void dram_init_banksize(void)
>                                                         PHYS_SDRAM_8_SIZE);
>  }
>
> -#ifdef CONFIG_OF_CONTROL
> -static int decode_sromc(const void *blob, struct fdt_sromc *config)
> -{
> -       int err;
> -       int node;
> -
> -       node = fdtdec_next_compatible(blob, 0, COMPAT_SAMSUNG_EXYNOS5_SROMC);
> -       if (node < 0) {
> -               debug("Could not find SROMC node\n");
> -               return node;
> -       }
> -
> -       config->bank = fdtdec_get_int(blob, node, "bank", 0);
> -       config->width = fdtdec_get_int(blob, node, "width", 2);
> -
> -       err = fdtdec_get_int_array(blob, node, "srom-timing", config->timing,
> -                       FDT_SROM_TIMING_COUNT);
> -       if (err < 0) {
> -               debug("Could not decode SROMC configuration\n");
> -               return -FDT_ERR_NOTFOUND;
> -       }
> -
> -       return 0;
> -}
> -#endif
> -
>  int board_eth_init(bd_t *bis)
>  {
>  #ifdef CONFIG_SMC911X
>         u32 smc_bw_conf, smc_bc_conf;
>         struct fdt_sromc config;
>         fdt_addr_t base_addr;
> -       int node;
> -
> -#ifdef CONFIG_OF_CONTROL
> -       node = decode_sromc(gd->fdt_blob, &config);
> -       if (node < 0) {
> -               debug("%s: Could not find sromc configuration\n", __func__);
> -               return 0;
> -       }
> -       node = fdtdec_next_compatible(gd->fdt_blob, node, COMPAT_SMSC_LAN9215);
> -       if (node < 0) {
> -               debug("%s: Could not find lan9215 configuration\n", __func__);
> -               return 0;
> -       }
>
> -       /* We now have a node, so any problems from now on are errors */
> -       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
> -       if (base_addr == FDT_ADDR_T_NONE) {
> -               debug("%s: Could not find lan9215 address\n", __func__);
> -               return -1;
> -       }
> -#else
>         /* Non-FDT configuration - bank number and timing parameters*/
>         config.bank = CONFIG_ENV_SROM_BANK;
>         config.width = 2;
> @@ -160,7 +115,6 @@ int board_eth_init(bd_t *bis)
>         config.timing[FDT_SROM_TACP] = 0x09;
>         config.timing[FDT_SROM_PMC] = 0x01;
>         base_addr = CONFIG_SMC911X_BASE;
> -#endif
>
>         /* Ethernet needs data bus width of 16 bits */
>         if (config.width != 2) {
> @@ -199,16 +153,31 @@ int checkboard(void)
>  #ifdef CONFIG_GENERIC_MMC
>  int board_mmc_init(bd_t *bis)
>  {
> -       int err;
> +       int err = 0, ret = 0;
>
>         err = exynos_pinmux_config(PERIPH_ID_SDMMC0, PINMUX_FLAG_8BIT_MODE);
> -       if (err) {
> +       if (err)
>                 debug("SDMMC0 not configured\n");
> -               return err;
> -       }
> -
> -       err = s5p_mmc_init(0, 8);
> -       return err;
> +       ret |= err;
> +
> +       /*EMMC: dwmmc Channel-0 with 8 bit bus width */
> +       err = exynos_dwmmc_init(0, 8);

This is not really init  of the whole dwmmc, only a port - suggest
exynos_dwmmc_add_port() or similar

> +       if (err)
> +               debug("dwmmc Channel-0 init failed\n");
> +       ret |= err;
> +
> +       err = exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
> +       if (err)
> +               debug("SDMMC2 not configured\n");
> +       ret |= err;
> +
> +       /*SD: dwmmc Channel-2 with 4 bit bus width */
> +       err = exynos_dwmmc_init(2, 4);
> +       if (err)
> +               debug("dwmmc Channel-2 init failed\n");
> +       ret |= err;
> +
> +       return ret;
>  }
>  #endif
>
> @@ -243,6 +212,24 @@ static int board_uart_init(void)
>         return 0;
>  }
>
> +#ifdef CONFIG_SYS_I2C_INIT_BOARD
> +static int board_i2c_init(void)
> +{
> +       int i, err;
> +
> +       for (i = 0; i < CONFIG_MAX_I2C_NUM; i++) {
> +               err = exynos_pinmux_config((PERIPH_ID_I2C0 + i),
> +                                               PINMUX_FLAG_NONE);
> +               if (err) {
> +                       debug("I2C%d not configured\n", (PERIPH_ID_I2C0 + i));
> +                       return err;
> +               }
> +       }
> +       i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> +       return 0;
> +}
> +#endif
> +
>  #ifdef CONFIG_BOARD_EARLY_INIT_F
>  int board_early_init_f(void)
>  {
> @@ -253,7 +240,7 @@ int board_early_init_f(void)
>                 return err;
>         }
>  #ifdef CONFIG_SYS_I2C_INIT_BOARD
> -       board_i2c_init(gd->fdt_blob);
> +       board_i2c_init();
>  #endif
>         return err;
>  }
> diff --git a/include/configs/exynos5250-dt.h b/include/configs/exynos5250-dt.h
> index 59182f4..6ce73dc 100644
> --- a/include/configs/exynos5250-dt.h
> +++ b/include/configs/exynos5250-dt.h
> @@ -84,6 +84,8 @@
>  #define CONFIG_MMC
>  #define CONFIG_SDHCI
>  #define CONFIG_S5P_SDHCI
> +#define CONFIG_DWMMC
> +#define CONFIG_EXYNOS_DWMMC
>
>  #define CONFIG_BOARD_EARLY_INIT_F
>
> diff --git a/include/i2c.h b/include/i2c.h
> index c60d075..0944141 100644
> --- a/include/i2c.h
> +++ b/include/i2c.h
> @@ -263,6 +263,7 @@ extern int get_multi_sda_pin(void);
>  extern int multi_i2c_init(void);
>  #endif
>
> +#ifdef CONFIG_OF_CONTROL
>  /**
>   * Get FDT values for i2c bus.
>   *
> @@ -270,6 +271,7 @@ extern int multi_i2c_init(void);
>   * @return the number of I2C bus
>   */
>  void board_i2c_init(const void *blob);
> +#endif

Do you need this #ifdef? It would be better to avoid having the same
function with a different signature.

>
>  /**
>   * Find the I2C bus number by given a FDT I2C node.
> --
> 1.8.0
>
Regards,
Simon

Hi Simon,

Thanks for review comments.
Please find my responses below.

Thanks & Regards
Amarendra Reddy

On 10 January 2013 22:27, Simon Glass <sjg@chromium.org> wrote:

> Hi Amar,
>
> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
> > This patch enables and initialises DWMMC for SMDK5250.
> > Supports both FDT and non-FDT. This patch creates a new file
> > 'exynos5-dt.c' meant for FDT support.
> >         exynos5-dt.c:   This file shall contain all code which supports
> FDT.
> >                         Any addition of FDT support for any module needs
> to be
> >                         added in this file.
> >         smdk5250.c:     This file shall contain the code which supports
> non-FDT.
> >                         version. Any addition of non-FDT support for any
> module
> >                         needs to be added in this file.
> >                         May be, the file smdk5250.c can be removed in
> near future
> >                         when non-FDT is not required.
> >
> > The Makefile is updated to compile only one of the files
> > exynos5-dt.c / smdk5250.c based on FDT configuration.
> >
> > NOTE:
> > Please note that all additions corresponding to FDT need to be added
> into the
> > file exynos5-dt.c.
> > At same time if non-FDT support is required then add the corresponding
> > updations into smdk5250.c.
> >
> > Changes from V1:
> >         1)A new file 'exynos5-dt.c' is created meant for FDT support
> >         2)Makefile is updated to compile only one of the files
> >         exynos5-dt.c / smdk5250.c based on FDT configuration
> >
> > Changes from V2:
> >         1)Updation of commit message and resubmition of proper patch set.
> >
> > Changes from V3:
> >         No change.
> >
> > Signed-off-by: Amar <amarendra.xt@samsung.com>
> > ---
> >  board/samsung/smdk5250/Makefile     |   4 +
> >  board/samsung/smdk5250/exynos5-dt.c | 242
> ++++++++++++++++++++++++++++++++++++
> >  board/samsung/smdk5250/smdk5250.c   |  97 +++++++--------
> >  include/configs/exynos5250-dt.h     |   2 +
> >  include/i2c.h                       |   2 +
> >  5 files changed, 292 insertions(+), 55 deletions(-)
> >  create mode 100644 board/samsung/smdk5250/exynos5-dt.c
> >
> > diff --git a/board/samsung/smdk5250/Makefile
> b/board/samsung/smdk5250/Makefile
> > index 47c6a5a..ecca9f3 100644
> > --- a/board/samsung/smdk5250/Makefile
> > +++ b/board/samsung/smdk5250/Makefile
> > @@ -32,8 +32,12 @@ COBJS        += tzpc_init.o
> >  COBJS  += smdk5250_spl.o
> >
> >  ifndef CONFIG_SPL_BUILD
> > +ifdef CONFIG_OF_CONTROL
> > +COBJS  += exynos5-dt.o
> > +else
> >  COBJS  += smdk5250.o
> >  endif
> > +endif
> >
> >  ifdef CONFIG_SPL_BUILD
> >  COBJS  += spl_boot.o
> > diff --git a/board/samsung/smdk5250/exynos5-dt.c
> b/board/samsung/smdk5250/exynos5-dt.c
> > new file mode 100644
> > index 0000000..da539ca
> > --- /dev/null
> > +++ b/board/samsung/smdk5250/exynos5-dt.c
> > @@ -0,0 +1,242 @@
> > +/*
> > + * Copyright (C) 2012 Samsung Electronics
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * 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.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#include <common.h>
> > +#include <fdtdec.h>
> > +#include <asm/io.h>
> > +#include <i2c.h>
> > +#include <netdev.h>
> > +#include <spi.h>
> > +#include <asm/arch/cpu.h>
> > +#include <asm/arch/dwmmc.h>
> > +#include <asm/arch/gpio.h>
> > +#include <asm/arch/mmc.h>
> > +#include <asm/arch/pinmux.h>
> > +#include <asm/arch/sromc.h>
> > +#include <power/pmic.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +int board_init(void)
> > +{
> > +       gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL);
> > +#ifdef CONFIG_EXYNOS_SPI
> > +       spi_init();
> > +#endif
> > +       return 0;
> > +}
> > +
> > +int dram_init(void)
> > +{
> > +       gd->ram_size    = get_ram_size((long *)PHYS_SDRAM_1,
> PHYS_SDRAM_1_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_2,
> PHYS_SDRAM_2_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_3,
> PHYS_SDRAM_3_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_4,
> PHYS_SDRAM_4_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_5,
> PHYS_SDRAM_7_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_6,
> PHYS_SDRAM_7_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_7,
> PHYS_SDRAM_7_SIZE)
> > +                       + get_ram_size((long *)PHYS_SDRAM_8,
> PHYS_SDRAM_8_SIZE);
>
> This looks ugly - is there any other way of doing this? Also 7 appears
> in more than one line.
>
> Since the banks are all SDRAM_BANK_SIZE apart, perhaps you could just
> use a for loop with a single base address?
>
> If this function is common with the other file then perhaps it should
> go in a common file?
>
>
In fact, this file "exynos5-dt.c" has been created for FDT support.
Existing code from "smdk5250.c" has been copied into "exynos5-dt.c".
The above piece of code computing 'gd->ram_size = ' is also copied from
smdk5250.c.

So, Is it required to do changes for existing code as well?
Please comment.

> +       return 0;
> > +}
> > +
> > +#if defined(CONFIG_POWER)
> > +int power_init_board(void)
> > +{
> > +       if (pmic_init(I2C_PMIC))
>
> debug()
>
> > +               return -1;
> > +       else
> > +               return 0;
> > +}
> > +#endif
> > +
> > +void dram_init_banksize(void)
> > +{
> > +       gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> > +       gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1,
> > +
> PHYS_SDRAM_1_SIZE);
> > +       gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
> > +       gd->bd->bi_dram[1].size = get_ram_size((long *)PHYS_SDRAM_2,
> > +
> PHYS_SDRAM_2_SIZE);
> > +       gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
> > +       gd->bd->bi_dram[2].size = get_ram_size((long *)PHYS_SDRAM_3,
> > +
> PHYS_SDRAM_3_SIZE);
> > +       gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
> > +       gd->bd->bi_dram[3].size = get_ram_size((long *)PHYS_SDRAM_4,
> > +
> PHYS_SDRAM_4_SIZE);
> > +       gd->bd->bi_dram[4].start = PHYS_SDRAM_5;
> > +       gd->bd->bi_dram[4].size = get_ram_size((long *)PHYS_SDRAM_5,
> > +
> PHYS_SDRAM_5_SIZE);
> > +       gd->bd->bi_dram[5].start = PHYS_SDRAM_6;
> > +       gd->bd->bi_dram[5].size = get_ram_size((long *)PHYS_SDRAM_6,
> > +
> PHYS_SDRAM_6_SIZE);
> > +       gd->bd->bi_dram[6].start = PHYS_SDRAM_7;
> > +       gd->bd->bi_dram[6].size = get_ram_size((long *)PHYS_SDRAM_7,
> > +
> PHYS_SDRAM_7_SIZE);
> > +       gd->bd->bi_dram[7].start = PHYS_SDRAM_8;
> > +       gd->bd->bi_dram[7].size = get_ram_size((long *)PHYS_SDRAM_8,
> > +
> PHYS_SDRAM_8_SIZE);
>
> and here
>
> > +}
> > +
> > +static int decode_sromc(const void *blob, struct fdt_sromc *config)
> > +{
> > +       int err;
> > +       int node;
> > +
> > +       node = fdtdec_next_compatible(blob, 0,
> COMPAT_SAMSUNG_EXYNOS5_SROMC);
> > +       if (node < 0) {
> > +               debug("Could not find SROMC node\n");
> > +               return node;
> > +       }
> > +
> > +       config->bank = fdtdec_get_int(blob, node, "bank", 0);
> > +       config->width = fdtdec_get_int(blob, node, "width", 2);
> > +
> > +       err = fdtdec_get_int_array(blob, node, "srom-timing",
> config->timing,
> > +                       FDT_SROM_TIMING_COUNT);
> > +       if (err < 0) {
> > +               debug("Could not decode SROMC configuration\n");
>
> Suggest:
>
> debug("Could not decode SROMC configuration: %s\n", fdt_strerror(err));
>
> > +               return -FDT_ERR_NOTFOUND;
>
> return err? Or the caller might just want -1
>
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +int board_eth_init(bd_t *bis)
> > +{
> > +#ifdef CONFIG_SMC911X
> > +       u32 smc_bw_conf, smc_bc_conf;
> > +       struct fdt_sromc config;
> > +       fdt_addr_t base_addr;
> > +       int node;
> > +
> > +       node = decode_sromc(gd->fdt_blob, &config);
> > +       if (node < 0) {
> > +               debug("%s: Could not find sromc configuration\n",
> __func__);
> > +               return 0;
> > +       }
> > +       node = fdtdec_next_compatible(gd->fdt_blob, node,
> COMPAT_SMSC_LAN9215);
> > +       if (node < 0) {
> > +               debug("%s: Could not find lan9215 configuration\n",
> __func__);
> > +               return 0;
> > +       }
> > +
> > +       /* We now have a node, so any problems from now on are errors */
> > +       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
> > +       if (base_addr == FDT_ADDR_T_NONE) {
> > +               debug("%s: Could not find lan9215 address\n", __func__);
> > +               return -1;
> > +       }
> > +
> > +       /* Ethernet needs data bus width of 16 bits */
> > +       if (config.width != 2) {
> > +               debug("%s: Unsupported bus width %d\n", __func__,
> > +                       config.width);
> > +               return -1;
> > +       }
> > +       smc_bw_conf = SROMC_DATA16_WIDTH(config.bank)
> > +                       | SROMC_BYTE_ENABLE(config.bank);
> > +
> > +       smc_bc_conf = SROMC_BC_TACS(config.timing[FDT_SROM_TACS])   |\
>
> Can you remove the \ from each line?
>
> > +                       SROMC_BC_TCOS(config.timing[FDT_SROM_TCOS]) |\
> > +                       SROMC_BC_TACC(config.timing[FDT_SROM_TACC]) |\
> > +                       SROMC_BC_TCOH(config.timing[FDT_SROM_TCOH]) |\
> > +                       SROMC_BC_TAH(config.timing[FDT_SROM_TAH])   |\
> > +                       SROMC_BC_TACP(config.timing[FDT_SROM_TACP]) |\
> > +                       SROMC_BC_PMC(config.timing[FDT_SROM_PMC]);
> > +
> > +       /* Select and configure the SROMC bank */
> > +       exynos_pinmux_config(PERIPH_ID_SROMC, config.bank);
> > +       s5p_config_sromc(config.bank, smc_bw_conf, smc_bc_conf);
> > +       return smc911x_initialize(0, base_addr);
> > +#endif
> > +       return 0;
> > +}
> > +
> > +#ifdef CONFIG_DISPLAY_BOARDINFO
> > +int checkboard(void)
> > +{
> > +       printf("\nBoard: SMDK5250\n");
> > +
> > +       return 0;
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_GENERIC_MMC
> > +int board_mmc_init(bd_t *bis)
> > +{
> > +       int ret = 0;
>
> Remove =0
>
> > +
> > +       /* dwmmc initializattion for available channels */
> > +       ret = exynos_dwmmc_init(gd->fdt_blob);
> > +       if (ret)
> > +               debug("dwmmc init failed\n");
> > +
> > +       return ret;
> > +}
> > +#endif
> > +
> > +static int board_uart_init(void)
> > +{
> > +       int err;
> > +
> > +       err = exynos_pinmux_config(PERIPH_ID_UART0, PINMUX_FLAG_NONE);
> > +       if (err) {
> > +               debug("UART0 not configured\n");
> > +               return err;
> > +       }
> > +
> > +       err = exynos_pinmux_config(PERIPH_ID_UART1, PINMUX_FLAG_NONE);
> > +       if (err) {
> > +               debug("UART1 not configured\n");
> > +               return err;
> > +       }
> > +
> > +       err = exynos_pinmux_config(PERIPH_ID_UART2, PINMUX_FLAG_NONE);
> > +       if (err) {
> > +               debug("UART2 not configured\n");
> > +               return err;
> > +       }
> > +
> > +       err = exynos_pinmux_config(PERIPH_ID_UART3, PINMUX_FLAG_NONE);
> > +       if (err) {
> > +               debug("UART3 not configured\n");
> > +               return err;
> > +       }
>
> Loop for this?
>
> > +
> > +       return 0;
> > +}
> > +
> > +#ifdef CONFIG_BOARD_EARLY_INIT_F
> > +int board_early_init_f(void)
> > +{
> > +       int err;
>
> blank line
>
> > +       err = board_uart_init();
> > +       if (err) {
> > +               debug("UART init failed\n");
> > +               return err;
> > +       }
> > +#ifdef CONFIG_SYS_I2C_INIT_BOARD
> > +       board_i2c_init(gd->fdt_blob);
> > +#endif
> > +       return err;
> > +}
> > +#endif
> > diff --git a/board/samsung/smdk5250/smdk5250.c
> b/board/samsung/smdk5250/smdk5250.c
> > index 73c3ec0..e0fec11 100644
> > --- a/board/samsung/smdk5250/smdk5250.c
> > +++ b/board/samsung/smdk5250/smdk5250.c
> > @@ -27,6 +27,7 @@
> >  #include <netdev.h>
> >  #include <spi.h>
> >  #include <asm/arch/cpu.h>
> > +#include <asm/arch/dwmmc.h>
> >  #include <asm/arch/gpio.h>
> >  #include <asm/arch/mmc.h>
> >  #include <asm/arch/pinmux.h>
> > @@ -95,59 +96,13 @@ void dram_init_banksize(void)
> >
> PHYS_SDRAM_8_SIZE);
> >  }
> >
> > -#ifdef CONFIG_OF_CONTROL
> > -static int decode_sromc(const void *blob, struct fdt_sromc *config)
> > -{
> > -       int err;
> > -       int node;
> > -
> > -       node = fdtdec_next_compatible(blob, 0,
> COMPAT_SAMSUNG_EXYNOS5_SROMC);
> > -       if (node < 0) {
> > -               debug("Could not find SROMC node\n");
> > -               return node;
> > -       }
> > -
> > -       config->bank = fdtdec_get_int(blob, node, "bank", 0);
> > -       config->width = fdtdec_get_int(blob, node, "width", 2);
> > -
> > -       err = fdtdec_get_int_array(blob, node, "srom-timing",
> config->timing,
> > -                       FDT_SROM_TIMING_COUNT);
> > -       if (err < 0) {
> > -               debug("Could not decode SROMC configuration\n");
> > -               return -FDT_ERR_NOTFOUND;
> > -       }
> > -
> > -       return 0;
> > -}
> > -#endif
> > -
> >  int board_eth_init(bd_t *bis)
> >  {
> >  #ifdef CONFIG_SMC911X
> >         u32 smc_bw_conf, smc_bc_conf;
> >         struct fdt_sromc config;
> >         fdt_addr_t base_addr;
> > -       int node;
> > -
> > -#ifdef CONFIG_OF_CONTROL
> > -       node = decode_sromc(gd->fdt_blob, &config);
> > -       if (node < 0) {
> > -               debug("%s: Could not find sromc configuration\n",
> __func__);
> > -               return 0;
> > -       }
> > -       node = fdtdec_next_compatible(gd->fdt_blob, node,
> COMPAT_SMSC_LAN9215);
> > -       if (node < 0) {
> > -               debug("%s: Could not find lan9215 configuration\n",
> __func__);
> > -               return 0;
> > -       }
> >
> > -       /* We now have a node, so any problems from now on are errors */
> > -       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
> > -       if (base_addr == FDT_ADDR_T_NONE) {
> > -               debug("%s: Could not find lan9215 address\n", __func__);
> > -               return -1;
> > -       }
> > -#else
> >         /* Non-FDT configuration - bank number and timing parameters*/
> >         config.bank = CONFIG_ENV_SROM_BANK;
> >         config.width = 2;
> > @@ -160,7 +115,6 @@ int board_eth_init(bd_t *bis)
> >         config.timing[FDT_SROM_TACP] = 0x09;
> >         config.timing[FDT_SROM_PMC] = 0x01;
> >         base_addr = CONFIG_SMC911X_BASE;
> > -#endif
> >
> >         /* Ethernet needs data bus width of 16 bits */
> >         if (config.width != 2) {
> > @@ -199,16 +153,31 @@ int checkboard(void)
> >  #ifdef CONFIG_GENERIC_MMC
> >  int board_mmc_init(bd_t *bis)
> >  {
> > -       int err;
> > +       int err = 0, ret = 0;
> >
> >         err = exynos_pinmux_config(PERIPH_ID_SDMMC0,
> PINMUX_FLAG_8BIT_MODE);
> > -       if (err) {
> > +       if (err)
> >                 debug("SDMMC0 not configured\n");
> > -               return err;
> > -       }
> > -
> > -       err = s5p_mmc_init(0, 8);
> > -       return err;
> > +       ret |= err;
> > +
> > +       /*EMMC: dwmmc Channel-0 with 8 bit bus width */
> > +       err = exynos_dwmmc_init(0, 8);
>
> This is not really init  of the whole dwmmc, only a port - suggest
> exynos_dwmmc_add_port() or similar
>

Instead of calling exynos_dwmmc_add_port() here, I shall call
exynos_dwmmc_init(*NULL*) here, as this is a non-FDT case. Inside the
function
exynos_dwmmc_init( * blob)
{
     #ifdef CONFIG_OF_CONTROL

            /* Read data from FDT */

            exynos_dwmmc_add_port(index, bus_width, ...)

     #else

            exynos_dwmmc_add_port(0,8...)

            exynos_dwmmc_add_port(2,4...)

     #endif
}

Please comment on the above.

>
> > +       if (err)
> > +               debug("dwmmc Channel-0 init failed\n");
> > +       ret |= err;
> > +
> > +       err = exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
> > +       if (err)
> > +               debug("SDMMC2 not configured\n");
> > +       ret |= err;
> > +
> > +       /*SD: dwmmc Channel-2 with 4 bit bus width */
> > +       err = exynos_dwmmc_init(2, 4);
> > +       if (err)
> > +               debug("dwmmc Channel-2 init failed\n");
> > +       ret |= err;
> > +
> > +       return ret;
> >  }
> >  #endif
> >
> > @@ -243,6 +212,24 @@ static int board_uart_init(void)
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_SYS_I2C_INIT_BOARD
> > +static int board_i2c_init(void)
> > +{
> > +       int i, err;
> > +
> > +       for (i = 0; i < CONFIG_MAX_I2C_NUM; i++) {
> > +               err = exynos_pinmux_config((PERIPH_ID_I2C0 + i),
> > +                                               PINMUX_FLAG_NONE);
> > +               if (err) {
> > +                       debug("I2C%d not configured\n", (PERIPH_ID_I2C0
> + i));
> > +                       return err;
> > +               }
> > +       }
> > +       i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> > +       return 0;
> > +}
> > +#endif
> > +
> >  #ifdef CONFIG_BOARD_EARLY_INIT_F
> >  int board_early_init_f(void)
> >  {
> > @@ -253,7 +240,7 @@ int board_early_init_f(void)
> >                 return err;
> >         }
> >  #ifdef CONFIG_SYS_I2C_INIT_BOARD
> > -       board_i2c_init(gd->fdt_blob);
> > +       board_i2c_init();
> >  #endif
> >         return err;
> >  }
> > diff --git a/include/configs/exynos5250-dt.h
> b/include/configs/exynos5250-dt.h
> > index 59182f4..6ce73dc 100644
> > --- a/include/configs/exynos5250-dt.h
> > +++ b/include/configs/exynos5250-dt.h
> > @@ -84,6 +84,8 @@
> >  #define CONFIG_MMC
> >  #define CONFIG_SDHCI
> >  #define CONFIG_S5P_SDHCI
> > +#define CONFIG_DWMMC
> > +#define CONFIG_EXYNOS_DWMMC
> >
> >  #define CONFIG_BOARD_EARLY_INIT_F
> >
> > diff --git a/include/i2c.h b/include/i2c.h
> > index c60d075..0944141 100644
> > --- a/include/i2c.h
> > +++ b/include/i2c.h
> > @@ -263,6 +263,7 @@ extern int get_multi_sda_pin(void);
> >  extern int multi_i2c_init(void);
> >  #endif
> >
> > +#ifdef CONFIG_OF_CONTROL
> >  /**
> >   * Get FDT values for i2c bus.
> >   *
> > @@ -270,6 +271,7 @@ extern int multi_i2c_init(void);
> >   * @return the number of I2C bus
> >   */
> >  void board_i2c_init(const void *blob);
> > +#endif
>
> Do you need this #ifdef? It would be better to avoid having the same
> function with a different signature.
>
> OK. Shall take care in next patch set.
i) call board_i2c_init(NULL) in case of non-FDT.
ii) call board_i2c_init(const void *blob) in case of FDT.

> >
> >  /**
> >   * Find the I2C bus number by given a FDT I2C node.
> > --
> > 1.8.0
> >
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

Hi Amar,

On Fri, Jan 11, 2013 at 9:58 AM, Amarendra Reddy
<amar.lavanuru@gmail.com> wrote:
> Hi Simon,
>
> Thanks for review comments.
> Please find my responses below.
>
> Thanks & Regards
> Amarendra Reddy
>
> On 10 January 2013 22:27, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Amar,
>>
>> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
>> > This patch enables and initialises DWMMC for SMDK5250.
>> > Supports both FDT and non-FDT. This patch creates a new file
>> > 'exynos5-dt.c' meant for FDT support.
>> >         exynos5-dt.c:   This file shall contain all code which supports
>> > FDT.
>> >                         Any addition of FDT support for any module needs
>> > to be
>> >                         added in this file.
>> >         smdk5250.c:     This file shall contain the code which supports
>> > non-FDT.
>> >                         version. Any addition of non-FDT support for any
>> > module
>> >                         needs to be added in this file.
>> >                         May be, the file smdk5250.c can be removed in
>> > near future
>> >                         when non-FDT is not required.
>> >
>> > The Makefile is updated to compile only one of the files
>> > exynos5-dt.c / smdk5250.c based on FDT configuration.
>> >
>> > NOTE:
>> > Please note that all additions corresponding to FDT need to be added
>> > into the
>> > file exynos5-dt.c.
>> > At same time if non-FDT support is required then add the corresponding
>> > updations into smdk5250.c.
>> >
>> > Changes from V1:
>> >         1)A new file 'exynos5-dt.c' is created meant for FDT support
>> >         2)Makefile is updated to compile only one of the files
>> >         exynos5-dt.c / smdk5250.c based on FDT configuration
>> >
>> > Changes from V2:
>> >         1)Updation of commit message and resubmition of proper patch
>> > set.
>> >
>> > Changes from V3:
>> >         No change.
>> >
>> > Signed-off-by: Amar <amarendra.xt@samsung.com>
>> > ---
>> >  board/samsung/smdk5250/Makefile     |   4 +
>> >  board/samsung/smdk5250/exynos5-dt.c | 242
>> > ++++++++++++++++++++++++++++++++++++
>> >  board/samsung/smdk5250/smdk5250.c   |  97 +++++++--------
>> >  include/configs/exynos5250-dt.h     |   2 +
>> >  include/i2c.h                       |   2 +
>> >  5 files changed, 292 insertions(+), 55 deletions(-)
>> >  create mode 100644 board/samsung/smdk5250/exynos5-dt.c
>> >
>> > diff --git a/board/samsung/smdk5250/Makefile
>> > b/board/samsung/smdk5250/Makefile
>> > index 47c6a5a..ecca9f3 100644
>> > --- a/board/samsung/smdk5250/Makefile
>> > +++ b/board/samsung/smdk5250/Makefile
>> > @@ -32,8 +32,12 @@ COBJS        += tzpc_init.o
>> >  COBJS  += smdk5250_spl.o
>> >
>> >  ifndef CONFIG_SPL_BUILD
>> > +ifdef CONFIG_OF_CONTROL
>> > +COBJS  += exynos5-dt.o
>> > +else
>> >  COBJS  += smdk5250.o
>> >  endif
>> > +endif
>> >
>> >  ifdef CONFIG_SPL_BUILD
>> >  COBJS  += spl_boot.o
>> > diff --git a/board/samsung/smdk5250/exynos5-dt.c
>> > b/board/samsung/smdk5250/exynos5-dt.c
>> > new file mode 100644
>> > index 0000000..da539ca
>> > --- /dev/null
>> > +++ b/board/samsung/smdk5250/exynos5-dt.c
>> > @@ -0,0 +1,242 @@
>> > +/*
>> > + * Copyright (C) 2012 Samsung Electronics
>> > + *
>> > + * See file CREDITS for list of people who contributed to this
>> > + * project.
>> > + *
>> > + * 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.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > + * along with this program; if not, write to the Free Software
>> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> > + * MA 02111-1307 USA
>> > + */
>> > +
>> > +#include <common.h>
>> > +#include <fdtdec.h>
>> > +#include <asm/io.h>
>> > +#include <i2c.h>
>> > +#include <netdev.h>
>> > +#include <spi.h>
>> > +#include <asm/arch/cpu.h>
>> > +#include <asm/arch/dwmmc.h>
>> > +#include <asm/arch/gpio.h>
>> > +#include <asm/arch/mmc.h>
>> > +#include <asm/arch/pinmux.h>
>> > +#include <asm/arch/sromc.h>
>> > +#include <power/pmic.h>
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +int board_init(void)
>> > +{
>> > +       gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL);
>> > +#ifdef CONFIG_EXYNOS_SPI
>> > +       spi_init();
>> > +#endif
>> > +       return 0;
>> > +}
>> > +
>> > +int dram_init(void)
>> > +{
>> > +       gd->ram_size    = get_ram_size((long *)PHYS_SDRAM_1,
>> > PHYS_SDRAM_1_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_2,
>> > PHYS_SDRAM_2_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_3,
>> > PHYS_SDRAM_3_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_4,
>> > PHYS_SDRAM_4_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_5,
>> > PHYS_SDRAM_7_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_6,
>> > PHYS_SDRAM_7_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_7,
>> > PHYS_SDRAM_7_SIZE)
>> > +                       + get_ram_size((long *)PHYS_SDRAM_8,
>> > PHYS_SDRAM_8_SIZE);
>>
>> This looks ugly - is there any other way of doing this? Also 7 appears
>> in more than one line.
>>
>> Since the banks are all SDRAM_BANK_SIZE apart, perhaps you could just
>> use a for loop with a single base address?
>>
>> If this function is common with the other file then perhaps it should
>> go in a common file?
>>
>
> In fact, this file "exynos5-dt.c" has been created for FDT support.
> Existing code from "smdk5250.c" has been copied into "exynos5-dt.c".
> The above piece of code computing 'gd->ram_size = ' is also copied from
> smdk5250.c.
>
> So, Is it required to do changes for existing code as well?
> Please comment.

I suppose I am responding to a patch to add a copy of this code into a
new file. Yes I think it would be better to create a common file that
both include, and then add a cleaned-up version of that function
(assuming it can be cleaned up as I suggested) to that common file,
and call the function from both places.

Copying code can cause bad problems when people want to refactor later.

>
>> > +       return 0;
>> > +}
>> > +
>> > +#if defined(CONFIG_POWER)
>> > +int power_init_board(void)
>> > +{
>> > +       if (pmic_init(I2C_PMIC))
>>
>> debug()
>>
>> > +               return -1;
>> > +       else
>> > +               return 0;
>> > +}
>> > +#endif
>> > +
>> > +void dram_init_banksize(void)
>> > +{
>> > +       gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
>> > +       gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1,
>> > +
>> > PHYS_SDRAM_1_SIZE);
>> > +       gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
>> > +       gd->bd->bi_dram[1].size = get_ram_size((long *)PHYS_SDRAM_2,
>> > +
>> > PHYS_SDRAM_2_SIZE);
>> > +       gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
>> > +       gd->bd->bi_dram[2].size = get_ram_size((long *)PHYS_SDRAM_3,
>> > +
>> > PHYS_SDRAM_3_SIZE);
>> > +       gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
>> > +       gd->bd->bi_dram[3].size = get_ram_size((long *)PHYS_SDRAM_4,
>> > +
>> > PHYS_SDRAM_4_SIZE);
>> > +       gd->bd->bi_dram[4].start = PHYS_SDRAM_5;
>> > +       gd->bd->bi_dram[4].size = get_ram_size((long *)PHYS_SDRAM_5,
>> > +
>> > PHYS_SDRAM_5_SIZE);
>> > +       gd->bd->bi_dram[5].start = PHYS_SDRAM_6;
>> > +       gd->bd->bi_dram[5].size = get_ram_size((long *)PHYS_SDRAM_6,
>> > +
>> > PHYS_SDRAM_6_SIZE);
>> > +       gd->bd->bi_dram[6].start = PHYS_SDRAM_7;
>> > +       gd->bd->bi_dram[6].size = get_ram_size((long *)PHYS_SDRAM_7,
>> > +
>> > PHYS_SDRAM_7_SIZE);
>> > +       gd->bd->bi_dram[7].start = PHYS_SDRAM_8;
>> > +       gd->bd->bi_dram[7].size = get_ram_size((long *)PHYS_SDRAM_8,
>> > +
>> > PHYS_SDRAM_8_SIZE);
>>
>> and here
>>
>> > +}
>> > +
>> > +static int decode_sromc(const void *blob, struct fdt_sromc *config)
>> > +{
>> > +       int err;
>> > +       int node;
>> > +
>> > +       node = fdtdec_next_compatible(blob, 0,
>> > COMPAT_SAMSUNG_EXYNOS5_SROMC);
>> > +       if (node < 0) {
>> > +               debug("Could not find SROMC node\n");
>> > +               return node;
>> > +       }
>> > +
>> > +       config->bank = fdtdec_get_int(blob, node, "bank", 0);
>> > +       config->width = fdtdec_get_int(blob, node, "width", 2);
>> > +
>> > +       err = fdtdec_get_int_array(blob, node, "srom-timing",
>> > config->timing,
>> > +                       FDT_SROM_TIMING_COUNT);
>> > +       if (err < 0) {
>> > +               debug("Could not decode SROMC configuration\n");
>>
>> Suggest:
>>
>> debug("Could not decode SROMC configuration: %s\n", fdt_strerror(err));
>>
>> > +               return -FDT_ERR_NOTFOUND;
>>
>> return err? Or the caller might just want -1
>>
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +int board_eth_init(bd_t *bis)
>> > +{
>> > +#ifdef CONFIG_SMC911X
>> > +       u32 smc_bw_conf, smc_bc_conf;
>> > +       struct fdt_sromc config;
>> > +       fdt_addr_t base_addr;
>> > +       int node;
>> > +
>> > +       node = decode_sromc(gd->fdt_blob, &config);
>> > +       if (node < 0) {
>> > +               debug("%s: Could not find sromc configuration\n",
>> > __func__);
>> > +               return 0;
>> > +       }
>> > +       node = fdtdec_next_compatible(gd->fdt_blob, node,
>> > COMPAT_SMSC_LAN9215);
>> > +       if (node < 0) {
>> > +               debug("%s: Could not find lan9215 configuration\n",
>> > __func__);
>> > +               return 0;
>> > +       }
>> > +
>> > +       /* We now have a node, so any problems from now on are errors */
>> > +       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
>> > +       if (base_addr == FDT_ADDR_T_NONE) {
>> > +               debug("%s: Could not find lan9215 address\n", __func__);
>> > +               return -1;
>> > +       }
>> > +
>> > +       /* Ethernet needs data bus width of 16 bits */
>> > +       if (config.width != 2) {
>> > +               debug("%s: Unsupported bus width %d\n", __func__,
>> > +                       config.width);
>> > +               return -1;
>> > +       }
>> > +       smc_bw_conf = SROMC_DATA16_WIDTH(config.bank)
>> > +                       | SROMC_BYTE_ENABLE(config.bank);
>> > +
>> > +       smc_bc_conf = SROMC_BC_TACS(config.timing[FDT_SROM_TACS])   |\
>>
>> Can you remove the \ from each line?
>>
>> > +                       SROMC_BC_TCOS(config.timing[FDT_SROM_TCOS]) |\
>> > +                       SROMC_BC_TACC(config.timing[FDT_SROM_TACC]) |\
>> > +                       SROMC_BC_TCOH(config.timing[FDT_SROM_TCOH]) |\
>> > +                       SROMC_BC_TAH(config.timing[FDT_SROM_TAH])   |\
>> > +                       SROMC_BC_TACP(config.timing[FDT_SROM_TACP]) |\
>> > +                       SROMC_BC_PMC(config.timing[FDT_SROM_PMC]);
>> > +
>> > +       /* Select and configure the SROMC bank */
>> > +       exynos_pinmux_config(PERIPH_ID_SROMC, config.bank);
>> > +       s5p_config_sromc(config.bank, smc_bw_conf, smc_bc_conf);
>> > +       return smc911x_initialize(0, base_addr);
>> > +#endif
>> > +       return 0;
>> > +}
>> > +
>> > +#ifdef CONFIG_DISPLAY_BOARDINFO
>> > +int checkboard(void)
>> > +{
>> > +       printf("\nBoard: SMDK5250\n");
>> > +
>> > +       return 0;
>> > +}
>> > +#endif
>> > +
>> > +#ifdef CONFIG_GENERIC_MMC
>> > +int board_mmc_init(bd_t *bis)
>> > +{
>> > +       int ret = 0;
>>
>> Remove =0
>>
>> > +
>> > +       /* dwmmc initializattion for available channels */
>> > +       ret = exynos_dwmmc_init(gd->fdt_blob);
>> > +       if (ret)
>> > +               debug("dwmmc init failed\n");
>> > +
>> > +       return ret;
>> > +}
>> > +#endif
>> > +
>> > +static int board_uart_init(void)
>> > +{
>> > +       int err;
>> > +
>> > +       err = exynos_pinmux_config(PERIPH_ID_UART0, PINMUX_FLAG_NONE);
>> > +       if (err) {
>> > +               debug("UART0 not configured\n");
>> > +               return err;
>> > +       }
>> > +
>> > +       err = exynos_pinmux_config(PERIPH_ID_UART1, PINMUX_FLAG_NONE);
>> > +       if (err) {
>> > +               debug("UART1 not configured\n");
>> > +               return err;
>> > +       }
>> > +
>> > +       err = exynos_pinmux_config(PERIPH_ID_UART2, PINMUX_FLAG_NONE);
>> > +       if (err) {
>> > +               debug("UART2 not configured\n");
>> > +               return err;
>> > +       }
>> > +
>> > +       err = exynos_pinmux_config(PERIPH_ID_UART3, PINMUX_FLAG_NONE);
>> > +       if (err) {
>> > +               debug("UART3 not configured\n");
>> > +               return err;
>> > +       }
>>
>> Loop for this?
>>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +#ifdef CONFIG_BOARD_EARLY_INIT_F
>> > +int board_early_init_f(void)
>> > +{
>> > +       int err;
>>
>> blank line
>>
>> > +       err = board_uart_init();
>> > +       if (err) {
>> > +               debug("UART init failed\n");
>> > +               return err;
>> > +       }
>> > +#ifdef CONFIG_SYS_I2C_INIT_BOARD
>> > +       board_i2c_init(gd->fdt_blob);
>> > +#endif
>> > +       return err;
>> > +}
>> > +#endif
>> > diff --git a/board/samsung/smdk5250/smdk5250.c
>> > b/board/samsung/smdk5250/smdk5250.c
>> > index 73c3ec0..e0fec11 100644
>> > --- a/board/samsung/smdk5250/smdk5250.c
>> > +++ b/board/samsung/smdk5250/smdk5250.c
>> > @@ -27,6 +27,7 @@
>> >  #include <netdev.h>
>> >  #include <spi.h>
>> >  #include <asm/arch/cpu.h>
>> > +#include <asm/arch/dwmmc.h>
>> >  #include <asm/arch/gpio.h>
>> >  #include <asm/arch/mmc.h>
>> >  #include <asm/arch/pinmux.h>
>> > @@ -95,59 +96,13 @@ void dram_init_banksize(void)
>> >
>> > PHYS_SDRAM_8_SIZE);
>> >  }
>> >
>> > -#ifdef CONFIG_OF_CONTROL
>> > -static int decode_sromc(const void *blob, struct fdt_sromc *config)
>> > -{
>> > -       int err;
>> > -       int node;
>> > -
>> > -       node = fdtdec_next_compatible(blob, 0,
>> > COMPAT_SAMSUNG_EXYNOS5_SROMC);
>> > -       if (node < 0) {
>> > -               debug("Could not find SROMC node\n");
>> > -               return node;
>> > -       }
>> > -
>> > -       config->bank = fdtdec_get_int(blob, node, "bank", 0);
>> > -       config->width = fdtdec_get_int(blob, node, "width", 2);
>> > -
>> > -       err = fdtdec_get_int_array(blob, node, "srom-timing",
>> > config->timing,
>> > -                       FDT_SROM_TIMING_COUNT);
>> > -       if (err < 0) {
>> > -               debug("Could not decode SROMC configuration\n");
>> > -               return -FDT_ERR_NOTFOUND;
>> > -       }
>> > -
>> > -       return 0;
>> > -}
>> > -#endif
>> > -
>> >  int board_eth_init(bd_t *bis)
>> >  {
>> >  #ifdef CONFIG_SMC911X
>> >         u32 smc_bw_conf, smc_bc_conf;
>> >         struct fdt_sromc config;
>> >         fdt_addr_t base_addr;
>> > -       int node;
>> > -
>> > -#ifdef CONFIG_OF_CONTROL
>> > -       node = decode_sromc(gd->fdt_blob, &config);
>> > -       if (node < 0) {
>> > -               debug("%s: Could not find sromc configuration\n",
>> > __func__);
>> > -               return 0;
>> > -       }
>> > -       node = fdtdec_next_compatible(gd->fdt_blob, node,
>> > COMPAT_SMSC_LAN9215);
>> > -       if (node < 0) {
>> > -               debug("%s: Could not find lan9215 configuration\n",
>> > __func__);
>> > -               return 0;
>> > -       }
>> >
>> > -       /* We now have a node, so any problems from now on are errors */
>> > -       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
>> > -       if (base_addr == FDT_ADDR_T_NONE) {
>> > -               debug("%s: Could not find lan9215 address\n", __func__);
>> > -               return -1;
>> > -       }
>> > -#else
>> >         /* Non-FDT configuration - bank number and timing parameters*/
>> >         config.bank = CONFIG_ENV_SROM_BANK;
>> >         config.width = 2;
>> > @@ -160,7 +115,6 @@ int board_eth_init(bd_t *bis)
>> >         config.timing[FDT_SROM_TACP] = 0x09;
>> >         config.timing[FDT_SROM_PMC] = 0x01;
>> >         base_addr = CONFIG_SMC911X_BASE;
>> > -#endif
>> >
>> >         /* Ethernet needs data bus width of 16 bits */
>> >         if (config.width != 2) {
>> > @@ -199,16 +153,31 @@ int checkboard(void)
>> >  #ifdef CONFIG_GENERIC_MMC
>> >  int board_mmc_init(bd_t *bis)
>> >  {
>> > -       int err;
>> > +       int err = 0, ret = 0;
>> >
>> >         err = exynos_pinmux_config(PERIPH_ID_SDMMC0,
>> > PINMUX_FLAG_8BIT_MODE);
>> > -       if (err) {
>> > +       if (err)
>> >                 debug("SDMMC0 not configured\n");
>> > -               return err;
>> > -       }
>> > -
>> > -       err = s5p_mmc_init(0, 8);
>> > -       return err;
>> > +       ret |= err;
>> > +
>> > +       /*EMMC: dwmmc Channel-0 with 8 bit bus width */
>> > +       err = exynos_dwmmc_init(0, 8);
>>
>> This is not really init  of the whole dwmmc, only a port - suggest
>> exynos_dwmmc_add_port() or similar
>
>
> Instead of calling exynos_dwmmc_add_port() here, I shall call
> exynos_dwmmc_init(NULL) here, as this is a non-FDT case. Inside the function
> exynos_dwmmc_init( * blob)

That's fine. Don't forget that gd->fdt_blob is NULL when there is no
fdt, so you can use

exynos_dwmmc_init(gd->fdt_blob)

in both cases. However if it just one line of code then that's fine.

Note that in the absence of an FDT it is supposed to be the board file
which knows which MMC ports are active.

> {
>      #ifdef CONFIG_OF_CONTROL
>
>             /* Read data from FDT */
>
>             exynos_dwmmc_add_port(index, bus_width, ...)

This code should go in the mmc driver. One of the ideas behind FDT is
that the drivers can figure out by themselves what ports to set up.
Also only the driver knows about its particular fields.

>
>      #else
>
>             exynos_dwmmc_add_port(0,8...)
>
>             exynos_dwmmc_add_port(2,4...)

This code should go in the board file, since without an FDT the driver
can't know what ports to init.

>
>      #endif
> }
>
> Please comment on the above.
>>
>>
>> > +       if (err)
>> > +               debug("dwmmc Channel-0 init failed\n");
>> > +       ret |= err;
>> > +
>> > +       err = exynos_pinmux_config(PERIPH_ID_SDMMC2, PINMUX_FLAG_NONE);
>> > +       if (err)
>> > +               debug("SDMMC2 not configured\n");
>> > +       ret |= err;
>> > +
>> > +       /*SD: dwmmc Channel-2 with 4 bit bus width */
>> > +       err = exynos_dwmmc_init(2, 4);
>> > +       if (err)
>> > +               debug("dwmmc Channel-2 init failed\n");
>> > +       ret |= err;
>> > +
>> > +       return ret;
>> >  }
>> >  #endif
>> >
>> > @@ -243,6 +212,24 @@ static int board_uart_init(void)
>> >         return 0;
>> >  }
>> >
>> > +#ifdef CONFIG_SYS_I2C_INIT_BOARD
>> > +static int board_i2c_init(void)
>> > +{
>> > +       int i, err;
>> > +
>> > +       for (i = 0; i < CONFIG_MAX_I2C_NUM; i++) {
>> > +               err = exynos_pinmux_config((PERIPH_ID_I2C0 + i),
>> > +                                               PINMUX_FLAG_NONE);
>> > +               if (err) {
>> > +                       debug("I2C%d not configured\n", (PERIPH_ID_I2C0
>> > + i));
>> > +                       return err;
>> > +               }
>> > +       }
>> > +       i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>> > +       return 0;
>> > +}
>> > +#endif
>> > +
>> >  #ifdef CONFIG_BOARD_EARLY_INIT_F
>> >  int board_early_init_f(void)
>> >  {
>> > @@ -253,7 +240,7 @@ int board_early_init_f(void)
>> >                 return err;
>> >         }
>> >  #ifdef CONFIG_SYS_I2C_INIT_BOARD
>> > -       board_i2c_init(gd->fdt_blob);
>> > +       board_i2c_init();
>> >  #endif
>> >         return err;
>> >  }
>> > diff --git a/include/configs/exynos5250-dt.h
>> > b/include/configs/exynos5250-dt.h
>> > index 59182f4..6ce73dc 100644
>> > --- a/include/configs/exynos5250-dt.h
>> > +++ b/include/configs/exynos5250-dt.h
>> > @@ -84,6 +84,8 @@
>> >  #define CONFIG_MMC
>> >  #define CONFIG_SDHCI
>> >  #define CONFIG_S5P_SDHCI
>> > +#define CONFIG_DWMMC
>> > +#define CONFIG_EXYNOS_DWMMC
>> >
>> >  #define CONFIG_BOARD_EARLY_INIT_F
>> >
>> > diff --git a/include/i2c.h b/include/i2c.h
>> > index c60d075..0944141 100644
>> > --- a/include/i2c.h
>> > +++ b/include/i2c.h
>> > @@ -263,6 +263,7 @@ extern int get_multi_sda_pin(void);
>> >  extern int multi_i2c_init(void);
>> >  #endif
>> >
>> > +#ifdef CONFIG_OF_CONTROL
>> >  /**
>> >   * Get FDT values for i2c bus.
>> >   *
>> > @@ -270,6 +271,7 @@ extern int multi_i2c_init(void);
>> >   * @return the number of I2C bus
>> >   */
>> >  void board_i2c_init(const void *blob);
>> > +#endif
>>
>> Do you need this #ifdef? It would be better to avoid having the same
>> function with a different signature.
>>
> OK. Shall take care in next patch set.
> i) call board_i2c_init(NULL) in case of non-FDT.
> ii) call board_i2c_init(const void *blob) in case of FDT.
>>
>> >
>> >  /**
>> >   * Find the I2C bus number by given a FDT I2C node.
>> > --
>> > 1.8.0
>> >
>> Regards,
>> Simon
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>

Regards,
Simon

Hi Simon,

Thanks for the review comments.
Please find my responses below.

Thanks & Regards
Amarendra Reddy

On 12 January 2013 22:11, Simon Glass <sjg@chromium.org> wrote:

> Hi Amar,
>
> On Fri, Jan 11, 2013 at 9:58 AM, Amarendra Reddy
> <amar.lavanuru@gmail.com> wrote:
> > Hi Simon,
> >
> > Thanks for review comments.
> > Please find my responses below.
> >
> > Thanks & Regards
> > Amarendra Reddy
> >
> > On 10 January 2013 22:27, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Amar,
> >>
> >> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
> >> > This patch enables and initialises DWMMC for SMDK5250.
> >> > Supports both FDT and non-FDT. This patch creates a new file
> >> > 'exynos5-dt.c' meant for FDT support.
> >> >         exynos5-dt.c:   This file shall contain all code which
> supports
> >> > FDT.
> >> >                         Any addition of FDT support for any module
> needs
> >> > to be
> >> >                         added in this file.
> >> >         smdk5250.c:     This file shall contain the code which
> supports
> >> > non-FDT.
> >> >                         version. Any addition of non-FDT support for
> any
> >> > module
> >> >                         needs to be added in this file.
> >> >                         May be, the file smdk5250.c can be removed in
> >> > near future
> >> >                         when non-FDT is not required.
> >> >
> >> > The Makefile is updated to compile only one of the files
> >> > exynos5-dt.c / smdk5250.c based on FDT configuration.
> >> >
> >> > NOTE:
> >> > Please note that all additions corresponding to FDT need to be added
> >> > into the
> >> > file exynos5-dt.c.
> >> > At same time if non-FDT support is required then add the corresponding
> >> > updations into smdk5250.c.
> >> >
> >> > Changes from V1:
> >> >         1)A new file 'exynos5-dt.c' is created meant for FDT support
> >> >         2)Makefile is updated to compile only one of the files
> >> >         exynos5-dt.c / smdk5250.c based on FDT configuration
> >> >
> >> > Changes from V2:
> >> >         1)Updation of commit message and resubmition of proper patch
> >> > set.
> >> >
> >> > Changes from V3:
> >> >         No change.
> >> >
> >> > Signed-off-by: Amar <amarendra.xt@samsung.com>
> >> > ---
> >> >  board/samsung/smdk5250/Makefile     |   4 +
> >> >  board/samsung/smdk5250/exynos5-dt.c | 242
> >> > ++++++++++++++++++++++++++++++++++++
> >> >  board/samsung/smdk5250/smdk5250.c   |  97 +++++++--------
> >> >  include/configs/exynos5250-dt.h     |   2 +
> >> >  include/i2c.h                       |   2 +
> >> >  5 files changed, 292 insertions(+), 55 deletions(-)
> >> >  create mode 100644 board/samsung/smdk5250/exynos5-dt.c
> >> >
> >> > diff --git a/board/samsung/smdk5250/Makefile
> >> > b/board/samsung/smdk5250/Makefile
> >> > index 47c6a5a..ecca9f3 100644
> >> > --- a/board/samsung/smdk5250/Makefile
> >> > +++ b/board/samsung/smdk5250/Makefile
> >> > @@ -32,8 +32,12 @@ COBJS        += tzpc_init.o
> >> >  COBJS  += smdk5250_spl.o
> >> >
> >> >  ifndef CONFIG_SPL_BUILD
> >> > +ifdef CONFIG_OF_CONTROL
> >> > +COBJS  += exynos5-dt.o
> >> > +else
> >> >  COBJS  += smdk5250.o
> >> >  endif
> >> > +endif
> >> >
> >> >  ifdef CONFIG_SPL_BUILD
> >> >  COBJS  += spl_boot.o
> >> > diff --git a/board/samsung/smdk5250/exynos5-dt.c
> >> > b/board/samsung/smdk5250/exynos5-dt.c
> >> > new file mode 100644
> >> > index 0000000..da539ca
> >> > --- /dev/null
> >> > +++ b/board/samsung/smdk5250/exynos5-dt.c
> >> > @@ -0,0 +1,242 @@
> >> > +/*
> >> > + * Copyright (C) 2012 Samsung Electronics
> >> > + *
> >> > + * See file CREDITS for list of people who contributed to this
> >> > + * project.
> >> > + *
> >> > + * 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.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License
> >> > + * along with this program; if not, write to the Free Software
> >> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> >> > + * MA 02111-1307 USA
> >> > + */
> >> > +
> >> > +#include <common.h>
> >> > +#include <fdtdec.h>
> >> > +#include <asm/io.h>
> >> > +#include <i2c.h>
> >> > +#include <netdev.h>
> >> > +#include <spi.h>
> >> > +#include <asm/arch/cpu.h>
> >> > +#include <asm/arch/dwmmc.h>
> >> > +#include <asm/arch/gpio.h>
> >> > +#include <asm/arch/mmc.h>
> >> > +#include <asm/arch/pinmux.h>
> >> > +#include <asm/arch/sromc.h>
> >> > +#include <power/pmic.h>
> >> > +
> >> > +DECLARE_GLOBAL_DATA_PTR;
> >> > +
> >> > +int board_init(void)
> >> > +{
> >> > +       gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL);
> >> > +#ifdef CONFIG_EXYNOS_SPI
> >> > +       spi_init();
> >> > +#endif
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +int dram_init(void)
> >> > +{
> >> > +       gd->ram_size    = get_ram_size((long *)PHYS_SDRAM_1,
> >> > PHYS_SDRAM_1_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_2,
> >> > PHYS_SDRAM_2_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_3,
> >> > PHYS_SDRAM_3_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_4,
> >> > PHYS_SDRAM_4_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_5,
> >> > PHYS_SDRAM_7_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_6,
> >> > PHYS_SDRAM_7_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_7,
> >> > PHYS_SDRAM_7_SIZE)
> >> > +                       + get_ram_size((long *)PHYS_SDRAM_8,
> >> > PHYS_SDRAM_8_SIZE);
> >>
> >> This looks ugly - is there any other way of doing this? Also 7 appears
> >> in more than one line.
> >>
> >> Since the banks are all SDRAM_BANK_SIZE apart, perhaps you could just
> >> use a for loop with a single base address?
> >>
> >> If this function is common with the other file then perhaps it should
> >> go in a common file?
> >>
> >
> > In fact, this file "exynos5-dt.c" has been created for FDT support.
> > Existing code from "smdk5250.c" has been copied into "exynos5-dt.c".
> > The above piece of code computing 'gd->ram_size = ' is also copied from
> > smdk5250.c.
> >
> > So, Is it required to do changes for existing code as well?
> > Please comment.
>
> I suppose I am responding to a patch to add a copy of this code into a
> new file. Yes I think it would be better to create a common file that
> both include, and then add a cleaned-up version of that function
> (assuming it can be cleaned up as I suggested) to that common file,
> and call the function from both places.
>
> Copying code can cause bad problems when people want to refactor later.
>
> Ok. Shall update the file exynos5-dt.c in response to your review
comments.

>  >
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +#if defined(CONFIG_POWER)
> >> > +int power_init_board(void)
> >> > +{
> >> > +       if (pmic_init(I2C_PMIC))
> >>
> >> debug()
> >>
> >> > +               return -1;
> >> > +       else
> >> > +               return 0;
> >> > +}
> >> > +#endif
> >> > +
> >> > +void dram_init_banksize(void)
> >> > +{
> >> > +       gd->bd->bi_dram[0].start = PHYS_SDRAM_1;
> >> > +       gd->bd->bi_dram[0].size = get_ram_size((long *)PHYS_SDRAM_1,
> >> > +
> >> > PHYS_SDRAM_1_SIZE);
> >> > +       gd->bd->bi_dram[1].start = PHYS_SDRAM_2;
> >> > +       gd->bd->bi_dram[1].size = get_ram_size((long *)PHYS_SDRAM_2,
> >> > +
> >> > PHYS_SDRAM_2_SIZE);
> >> > +       gd->bd->bi_dram[2].start = PHYS_SDRAM_3;
> >> > +       gd->bd->bi_dram[2].size = get_ram_size((long *)PHYS_SDRAM_3,
> >> > +
> >> > PHYS_SDRAM_3_SIZE);
> >> > +       gd->bd->bi_dram[3].start = PHYS_SDRAM_4;
> >> > +       gd->bd->bi_dram[3].size = get_ram_size((long *)PHYS_SDRAM_4,
> >> > +
> >> > PHYS_SDRAM_4_SIZE);
> >> > +       gd->bd->bi_dram[4].start = PHYS_SDRAM_5;
> >> > +       gd->bd->bi_dram[4].size = get_ram_size((long *)PHYS_SDRAM_5,
> >> > +
> >> > PHYS_SDRAM_5_SIZE);
> >> > +       gd->bd->bi_dram[5].start = PHYS_SDRAM_6;
> >> > +       gd->bd->bi_dram[5].size = get_ram_size((long *)PHYS_SDRAM_6,
> >> > +
> >> > PHYS_SDRAM_6_SIZE);
> >> > +       gd->bd->bi_dram[6].start = PHYS_SDRAM_7;
> >> > +       gd->bd->bi_dram[6].size = get_ram_size((long *)PHYS_SDRAM_7,
> >> > +
> >> > PHYS_SDRAM_7_SIZE);
> >> > +       gd->bd->bi_dram[7].start = PHYS_SDRAM_8;
> >> > +       gd->bd->bi_dram[7].size = get_ram_size((long *)PHYS_SDRAM_8,
> >> > +
> >> > PHYS_SDRAM_8_SIZE);
> >>
> >> and here
> >>
> >> > +}
> >> > +
> >> > +static int decode_sromc(const void *blob, struct fdt_sromc *config)
> >> > +{
> >> > +       int err;
> >> > +       int node;
> >> > +
> >> > +       node = fdtdec_next_compatible(blob, 0,
> >> > COMPAT_SAMSUNG_EXYNOS5_SROMC);
> >> > +       if (node < 0) {
> >> > +               debug("Could not find SROMC node\n");
> >> > +               return node;
> >> > +       }
> >> > +
> >> > +       config->bank = fdtdec_get_int(blob, node, "bank", 0);
> >> > +       config->width = fdtdec_get_int(blob, node, "width", 2);
> >> > +
> >> > +       err = fdtdec_get_int_array(blob, node, "srom-timing",
> >> > config->timing,
> >> > +                       FDT_SROM_TIMING_COUNT);
> >> > +       if (err < 0) {
> >> > +               debug("Could not decode SROMC configuration\n");
> >>
> >> Suggest:
> >>
> >> debug("Could not decode SROMC configuration: %s\n", fdt_strerror(err));
> >>
> >> > +               return -FDT_ERR_NOTFOUND;
> >>
> >> return err? Or the caller might just want -1
> >>
> >> > +       }
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +int board_eth_init(bd_t *bis)
> >> > +{
> >> > +#ifdef CONFIG_SMC911X
> >> > +       u32 smc_bw_conf, smc_bc_conf;
> >> > +       struct fdt_sromc config;
> >> > +       fdt_addr_t base_addr;
> >> > +       int node;
> >> > +
> >> > +       node = decode_sromc(gd->fdt_blob, &config);
> >> > +       if (node < 0) {
> >> > +               debug("%s: Could not find sromc configuration\n",
> >> > __func__);
> >> > +               return 0;
> >> > +       }
> >> > +       node = fdtdec_next_compatible(gd->fdt_blob, node,
> >> > COMPAT_SMSC_LAN9215);
> >> > +       if (node < 0) {
> >> > +               debug("%s: Could not find lan9215 configuration\n",
> >> > __func__);
> >> > +               return 0;
> >> > +       }
> >> > +
> >> > +       /* We now have a node, so any problems from now on are errors
> */
> >> > +       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
> >> > +       if (base_addr == FDT_ADDR_T_NONE) {
> >> > +               debug("%s: Could not find lan9215 address\n",
> __func__);
> >> > +               return -1;
> >> > +       }
> >> > +
> >> > +       /* Ethernet needs data bus width of 16 bits */
> >> > +       if (config.width != 2) {
> >> > +               debug("%s: Unsupported bus width %d\n", __func__,
> >> > +                       config.width);
> >> > +               return -1;
> >> > +       }
> >> > +       smc_bw_conf = SROMC_DATA16_WIDTH(config.bank)
> >> > +                       | SROMC_BYTE_ENABLE(config.bank);
> >> > +
> >> > +       smc_bc_conf = SROMC_BC_TACS(config.timing[FDT_SROM_TACS])   |\
> >>
> >> Can you remove the \ from each line?
> >>
> >> > +                       SROMC_BC_TCOS(config.timing[FDT_SROM_TCOS]) |\
> >> > +                       SROMC_BC_TACC(config.timing[FDT_SROM_TACC]) |\
> >> > +                       SROMC_BC_TCOH(config.timing[FDT_SROM_TCOH]) |\
> >> > +                       SROMC_BC_TAH(config.timing[FDT_SROM_TAH])   |\
> >> > +                       SROMC_BC_TACP(config.timing[FDT_SROM_TACP]) |\
> >> > +                       SROMC_BC_PMC(config.timing[FDT_SROM_PMC]);
> >> > +
> >> > +       /* Select and configure the SROMC bank */
> >> > +       exynos_pinmux_config(PERIPH_ID_SROMC, config.bank);
> >> > +       s5p_config_sromc(config.bank, smc_bw_conf, smc_bc_conf);
> >> > +       return smc911x_initialize(0, base_addr);
> >> > +#endif
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +#ifdef CONFIG_DISPLAY_BOARDINFO
> >> > +int checkboard(void)
> >> > +{
> >> > +       printf("\nBoard: SMDK5250\n");
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +#endif
> >> > +
> >> > +#ifdef CONFIG_GENERIC_MMC
> >> > +int board_mmc_init(bd_t *bis)
> >> > +{
> >> > +       int ret = 0;
> >>
> >> Remove =0
> >>
> >> > +
> >> > +       /* dwmmc initializattion for available channels */
> >> > +       ret = exynos_dwmmc_init(gd->fdt_blob);
> >> > +       if (ret)
> >> > +               debug("dwmmc init failed\n");
> >> > +
> >> > +       return ret;
> >> > +}
> >> > +#endif
> >> > +
> >> > +static int board_uart_init(void)
> >> > +{
> >> > +       int err;
> >> > +
> >> > +       err = exynos_pinmux_config(PERIPH_ID_UART0, PINMUX_FLAG_NONE);
> >> > +       if (err) {
> >> > +               debug("UART0 not configured\n");
> >> > +               return err;
> >> > +       }
> >> > +
> >> > +       err = exynos_pinmux_config(PERIPH_ID_UART1, PINMUX_FLAG_NONE);
> >> > +       if (err) {
> >> > +               debug("UART1 not configured\n");
> >> > +               return err;
> >> > +       }
> >> > +
> >> > +       err = exynos_pinmux_config(PERIPH_ID_UART2, PINMUX_FLAG_NONE);
> >> > +       if (err) {
> >> > +               debug("UART2 not configured\n");
> >> > +               return err;
> >> > +       }
> >> > +
> >> > +       err = exynos_pinmux_config(PERIPH_ID_UART3, PINMUX_FLAG_NONE);
> >> > +       if (err) {
> >> > +               debug("UART3 not configured\n");
> >> > +               return err;
> >> > +       }
> >>
> >> Loop for this?
> >>
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +#ifdef CONFIG_BOARD_EARLY_INIT_F
> >> > +int board_early_init_f(void)
> >> > +{
> >> > +       int err;
> >>
> >> blank line
> >>
> >> > +       err = board_uart_init();
> >> > +       if (err) {
> >> > +               debug("UART init failed\n");
> >> > +               return err;
> >> > +       }
> >> > +#ifdef CONFIG_SYS_I2C_INIT_BOARD
> >> > +       board_i2c_init(gd->fdt_blob);
> >> > +#endif
> >> > +       return err;
> >> > +}
> >> > +#endif
> >> > diff --git a/board/samsung/smdk5250/smdk5250.c
> >> > b/board/samsung/smdk5250/smdk5250.c
> >> > index 73c3ec0..e0fec11 100644
> >> > --- a/board/samsung/smdk5250/smdk5250.c
> >> > +++ b/board/samsung/smdk5250/smdk5250.c
> >> > @@ -27,6 +27,7 @@
> >> >  #include <netdev.h>
> >> >  #include <spi.h>
> >> >  #include <asm/arch/cpu.h>
> >> > +#include <asm/arch/dwmmc.h>
> >> >  #include <asm/arch/gpio.h>
> >> >  #include <asm/arch/mmc.h>
> >> >  #include <asm/arch/pinmux.h>
> >> > @@ -95,59 +96,13 @@ void dram_init_banksize(void)
> >> >
> >> > PHYS_SDRAM_8_SIZE);
> >> >  }
> >> >
> >> > -#ifdef CONFIG_OF_CONTROL
> >> > -static int decode_sromc(const void *blob, struct fdt_sromc *config)
> >> > -{
> >> > -       int err;
> >> > -       int node;
> >> > -
> >> > -       node = fdtdec_next_compatible(blob, 0,
> >> > COMPAT_SAMSUNG_EXYNOS5_SROMC);
> >> > -       if (node < 0) {
> >> > -               debug("Could not find SROMC node\n");
> >> > -               return node;
> >> > -       }
> >> > -
> >> > -       config->bank = fdtdec_get_int(blob, node, "bank", 0);
> >> > -       config->width = fdtdec_get_int(blob, node, "width", 2);
> >> > -
> >> > -       err = fdtdec_get_int_array(blob, node, "srom-timing",
> >> > config->timing,
> >> > -                       FDT_SROM_TIMING_COUNT);
> >> > -       if (err < 0) {
> >> > -               debug("Could not decode SROMC configuration\n");
> >> > -               return -FDT_ERR_NOTFOUND;
> >> > -       }
> >> > -
> >> > -       return 0;
> >> > -}
> >> > -#endif
> >> > -
> >> >  int board_eth_init(bd_t *bis)
> >> >  {
> >> >  #ifdef CONFIG_SMC911X
> >> >         u32 smc_bw_conf, smc_bc_conf;
> >> >         struct fdt_sromc config;
> >> >         fdt_addr_t base_addr;
> >> > -       int node;
> >> > -
> >> > -#ifdef CONFIG_OF_CONTROL
> >> > -       node = decode_sromc(gd->fdt_blob, &config);
> >> > -       if (node < 0) {
> >> > -               debug("%s: Could not find sromc configuration\n",
> >> > __func__);
> >> > -               return 0;
> >> > -       }
> >> > -       node = fdtdec_next_compatible(gd->fdt_blob, node,
> >> > COMPAT_SMSC_LAN9215);
> >> > -       if (node < 0) {
> >> > -               debug("%s: Could not find lan9215 configuration\n",
> >> > __func__);
> >> > -               return 0;
> >> > -       }
> >> >
> >> > -       /* We now have a node, so any problems from now on are errors
> */
> >> > -       base_addr = fdtdec_get_addr(gd->fdt_blob, node, "reg");
> >> > -       if (base_addr == FDT_ADDR_T_NONE) {
> >> > -               debug("%s: Could not find lan9215 address\n",
> __func__);
> >> > -               return -1;
> >> > -       }
> >> > -#else
> >> >         /* Non-FDT configuration - bank number and timing parameters*/
> >> >         config.bank = CONFIG_ENV_SROM_BANK;
> >> >         config.width = 2;
> >> > @@ -160,7 +115,6 @@ int board_eth_init(bd_t *bis)
> >> >         config.timing[FDT_SROM_TACP] = 0x09;
> >> >         config.timing[FDT_SROM_PMC] = 0x01;
> >> >         base_addr = CONFIG_SMC911X_BASE;
> >> > -#endif
> >> >
> >> >         /* Ethernet needs data bus width of 16 bits */
> >> >         if (config.width != 2) {
> >> > @@ -199,16 +153,31 @@ int checkboard(void)
> >> >  #ifdef CONFIG_GENERIC_MMC
> >> >  int board_mmc_init(bd_t *bis)
> >> >  {
> >> > -       int err;
> >> > +       int err = 0, ret = 0;
> >> >
> >> >         err = exynos_pinmux_config(PERIPH_ID_SDMMC0,
> >> > PINMUX_FLAG_8BIT_MODE);
> >> > -       if (err) {
> >> > +       if (err)
> >> >                 debug("SDMMC0 not configured\n");
> >> > -               return err;
> >> > -       }
> >> > -
> >> > -       err = s5p_mmc_init(0, 8);
> >> > -       return err;
> >> > +       ret |= err;
> >> > +
> >> > +       /*EMMC: dwmmc Channel-0 with 8 bit bus width */
> >> > +       err = exynos_dwmmc_init(0, 8);
> >>
> >> This is not really init  of the whole dwmmc, only a port - suggest
> >> exynos_dwmmc_add_port() or similar
> >
> >
> > Instead of calling exynos_dwmmc_add_port() here, I shall call
> > exynos_dwmmc_init(NULL) here, as this is a non-FDT case. Inside the
> function
> > exynos_dwmmc_init( * blob)
>
> That's fine. Don't forget that gd->fdt_blob is NULL when there is no
> fdt, so you can use
>
> exynos_dwmmc_init(gd->fdt_blob)
>
> in both cases. However if it just one line of code then that's fine.
>
> Note that in the absence of an FDT it is supposed to be the board file
> which knows which MMC ports are active.
>
> > {
> >      #ifdef CONFIG_OF_CONTROL
> >
> >             /* Read data from FDT */
> >
> >             exynos_dwmmc_add_port(index, bus_width, ...)
>
> This code should go in the mmc driver. One of the ideas behind FDT is
> that the drivers can figure out by themselves what ports to set up.
> Also only the driver knows about its particular fields.
>

Ok.

>
> >
> >      #else
> >
> >             exynos_dwmmc_add_port(0,8...)
> >
> >             exynos_dwmmc_add_port(2,4...)
>
> This code should go in the board file, since without an FDT the driver
> can't know what ports to init.
>

Ok.

>
> >
> >      #endif
> > }
> >
> > Please comment on the above.
> >>
> >>
> >> > +       if (err)
> >> > +               debug("dwmmc Channel-0 init failed\n");
> >> > +       ret |= err;
> >> > +
> >> > +       err = exynos_pinmux_config(PERIPH_ID_SDMMC2,
> PINMUX_FLAG_NONE);
> >> > +       if (err)
> >> > +               debug("SDMMC2 not configured\n");
> >> > +       ret |= err;
> >> > +
> >> > +       /*SD: dwmmc Channel-2 with 4 bit bus width */
> >> > +       err = exynos_dwmmc_init(2, 4);
> >> > +       if (err)
> >> > +               debug("dwmmc Channel-2 init failed\n");
> >> > +       ret |= err;
> >> > +
> >> > +       return ret;
> >> >  }
> >> >  #endif
> >> >
> >> > @@ -243,6 +212,24 @@ static int board_uart_init(void)
> >> >         return 0;
> >> >  }
> >> >
> >> > +#ifdef CONFIG_SYS_I2C_INIT_BOARD
> >> > +static int board_i2c_init(void)
> >> > +{
> >> > +       int i, err;
> >> > +
> >> > +       for (i = 0; i < CONFIG_MAX_I2C_NUM; i++) {
> >> > +               err = exynos_pinmux_config((PERIPH_ID_I2C0 + i),
> >> > +                                               PINMUX_FLAG_NONE);
> >> > +               if (err) {
> >> > +                       debug("I2C%d not configured\n",
> (PERIPH_ID_I2C0
> >> > + i));
> >> > +                       return err;
> >> > +               }
> >> > +       }
> >> > +       i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> >> > +       return 0;
> >> > +}
> >> > +#endif
> >> > +
> >> >  #ifdef CONFIG_BOARD_EARLY_INIT_F
> >> >  int board_early_init_f(void)
> >> >  {
> >> > @@ -253,7 +240,7 @@ int board_early_init_f(void)
> >> >                 return err;
> >> >         }
> >> >  #ifdef CONFIG_SYS_I2C_INIT_BOARD
> >> > -       board_i2c_init(gd->fdt_blob);
> >> > +       board_i2c_init();
> >> >  #endif
> >> >         return err;
> >> >  }
> >> > diff --git a/include/configs/exynos5250-dt.h
> >> > b/include/configs/exynos5250-dt.h
> >> > index 59182f4..6ce73dc 100644
> >> > --- a/include/configs/exynos5250-dt.h
> >> > +++ b/include/configs/exynos5250-dt.h
> >> > @@ -84,6 +84,8 @@
> >> >  #define CONFIG_MMC
> >> >  #define CONFIG_SDHCI
> >> >  #define CONFIG_S5P_SDHCI
> >> > +#define CONFIG_DWMMC
> >> > +#define CONFIG_EXYNOS_DWMMC
> >> >
> >> >  #define CONFIG_BOARD_EARLY_INIT_F
> >> >
> >> > diff --git a/include/i2c.h b/include/i2c.h
> >> > index c60d075..0944141 100644
> >> > --- a/include/i2c.h
> >> > +++ b/include/i2c.h
> >> > @@ -263,6 +263,7 @@ extern int get_multi_sda_pin(void);
> >> >  extern int multi_i2c_init(void);
> >> >  #endif
> >> >
> >> > +#ifdef CONFIG_OF_CONTROL
> >> >  /**
> >> >   * Get FDT values for i2c bus.
> >> >   *
> >> > @@ -270,6 +271,7 @@ extern int multi_i2c_init(void);
> >> >   * @return the number of I2C bus
> >> >   */
> >> >  void board_i2c_init(const void *blob);
> >> > +#endif
> >>
> >> Do you need this #ifdef? It would be better to avoid having the same
> >> function with a different signature.
> >>
> > OK. Shall take care in next patch set.
> > i) call board_i2c_init(NULL) in case of non-FDT.
> > ii) call board_i2c_init(const void *blob) in case of FDT.
> >>
> >> >
> >> >  /**
> >> >   * Find the I2C bus number by given a FDT I2C node.
> >> > --
> >> > 1.8.0
> >> >
> >> Regards,
> >> Simon
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot@lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> >
> >
>
> Regards,
> Simon
>