Hi Simon, Thanks for review comments. Please find my responses below.
Thanks & Regards Amarendra Reddy On 10 January 2013 22:27, Simon Glass <s...@chromium.org> wrote: > Hi Amar, > > On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra...@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...@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 >
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot