Patchwork [U-Boot,4/9] EXYNOS5: DWMMC: Added dt support for DWMMC

login
register
mail settings
Submitter Amar
Date Dec. 17, 2012, 11:19 a.m.
Message ID <1355743176-12305-5-git-send-email-amarendra.xt@samsung.com>
Download mbox | patch
Permalink /patch/206832/
State Superseded
Delegated to: Minkyu Kang
Headers show

Comments

Amar - Dec. 17, 2012, 11:19 a.m.
Signed-off-by: Amar <amarendra.xt@samsung.com>
---
 arch/arm/include/asm/arch-exynos/dwmmc.h |    4 +
 drivers/mmc/exynos_dw_mmc.c              |  117 ++++++++++++++++++++++++++++--
 include/dwmmc.h                          |    4 +
 3 files changed, 119 insertions(+), 6 deletions(-)
Simon Glass - Dec. 20, 2012, 2:23 a.m.
Hi Amar,

On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt@samsung.com> wrote:

> Signed-off-by: Amar <amarendra.xt@samsung.com>
>

Good to see this patch! Please can you add a short commit message?


> ---

 arch/arm/include/asm/arch-exynos/dwmmc.h |    4 +
>  drivers/mmc/exynos_dw_mmc.c              |  117
> ++++++++++++++++++++++++++++--
>  include/dwmmc.h                          |    4 +
>  3 files changed, 119 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> b/arch/arm/include/asm/arch-exynos/dwmmc.h
> index 8acdf9b..92352df 100644
> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> @@ -27,6 +27,9 @@
>  #define DWMCI_SET_DRV_CLK(x)   ((x) << 16)
>  #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
>
> +#ifdef CONFIG_OF_CONTROL
> +unsigned int exynos_dwmmc_init(const void *blob);
> +#else
>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>
>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> @@ -34,3 +37,4 @@ static inline unsigned int exynos_dwmmc_init(int index,
> int bus_width)
>         unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
>         return exynos_dwmci_init(base, bus_width, index);
>  }
> +#endif
> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> index 72a31b7..3b3e3e5 100644
> --- a/drivers/mmc/exynos_dw_mmc.c
> +++ b/drivers/mmc/exynos_dw_mmc.c
> @@ -19,23 +19,38 @@
>   */
>
>  #include <common.h>
> -#include <malloc.h>
>  #include <dwmmc.h>
> +#include <fdtdec.h>
> +#include <libfdt.h>
> +#include <malloc.h>
>  #include <asm/arch/dwmmc.h>
>  #include <asm/arch/clk.h>
> +#include <asm/arch/pinmux.h>
> +
> +#define        DWMMC_MAX_CH_NUM                4
> +#define        DWMMC_MAX_FREQ                  52000000
> +#define        DWMMC_MIN_FREQ                  400000
> +#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
> +#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
>

What do these last two values mean? I think clock setting should be done in
clock.c, not here.


>
>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
>
>  static void exynos_dwmci_clksel(struct dwmci_host *host)
>  {
> -       u32 val;
> -       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> -               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(0);
> +       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
> +}
>
> -       dwmci_writel(host, DWMCI_CLKSEL, val);
> +unsigned int exynos_dwmci_get_clk(int dev_index)
> +{
> +       return get_mmc_clk(dev_index);
>  }
>
> +#ifdef CONFIG_OF_CONTROL
> +static int exynos_dwmci_init(u32 regbase, int bus_width,
> +                                       int index, u32 *timing)
> +#else
>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> +#endif
>

I'm really not keen on having the same function with different signatures.
My first question is whether we need to support operation without
CONFIG_OF_CONTROL. If so, I suggest having an init routine that takes an
FDT blob as a parameter, and a separate add_port function which can be
called by non-FDT-enabled board files. The init routine will call the
add_port function for each port it finds in the FDT.

Also please can you briefly comment non-trivial functions?


>  {
>         struct dwmci_host *host = NULL;
>         host = malloc(sizeof(struct dwmci_host));
> @@ -44,14 +59,104 @@ int exynos_dwmci_init(u32 regbase, int bus_width, int
> index)
>                 return 1;
>         }
>
> +       /* set the clock divisor - clk_div_fsys for mmc */
> +       if (exynos5_mmc_set_clk_div(index)) {
> +               debug("mmc clock div set failed\n");
> +               return -1;
> +       }
> +
>         host->name = EXYNOS_NAME;
>         host->ioaddr = (void *)regbase;
>         host->buswidth = bus_width;
> +#ifdef CONFIG_OF_CONTROL
> +       host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
> +                               DWMCI_SET_DRV_CLK(timing[1]) |
> +                               DWMCI_SET_DIV_RATIO(timing[2]));
> +#else
> +       if (0 == index)
> +               host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
> +       if (2 == index)
> +               host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
> +#endif
>         host->clksel = exynos_dwmci_clksel;
>         host->dev_index = index;
> +       host->mmc_clk = exynos_dwmci_get_clk;
>
> -       add_dwmci(host, 52000000, 400000);
> +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
>
>         return 0;
>  }
>
> +#ifdef CONFIG_OF_CONTROL
> +unsigned int exynos_dwmmc_init(const void *blob)
> +{
> +       u32 base;
> +       int index, bus_width;
> +       int node_list[DWMMC_MAX_CH_NUM];
> +       int err = 0;
> +       int dev_id, flag;
> +       u32 timing[3];
> +       int count, i;
> +
> +       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
> +                               COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
> +                               DWMMC_MAX_CH_NUM);
> +
> +       for (i = 0; i < count; i++) {
> +               int node = node_list[i];
> +
> +               if (node <= 0)
> +                       continue;
> +
> +               /* config pinmux for each mmc channel */
> +               dev_id = pinmux_decode_periph_id(blob, node);
> +               if (dev_id == PERIPH_ID_SDMMC0)
> +                       flag = PINMUX_FLAG_8BIT_MODE;
> +               else
> +                       flag = PINMUX_FLAG_NONE;
>

This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node,
"samsung,width") == 8.


> +
> +               err = exynos_pinmux_config(dev_id, flag);
> +               if (err) {
> +                       debug("DWMMC not configured\n");
> +                       return err;
> +               }
> +
> +               /* Get the base address from the device node */
> +               base = fdtdec_get_addr(blob, node, "reg");
> +               if (!base) {
> +                       debug("DWMMC: Can't get base address\n");
> +                       return -1;
> +               }
> +
> +               /* Get the channel index from the device node */
> +               index = fdtdec_get_int(blob, node, "index", 0);
> +               if (index < 0) {
> +                       debug("DWMMC: Can't get channel index\n");
> +                       return -1;
> +               }
>

What is this needed for? Doesn't the reg address tell you everything you
need here?


> +
> +               /* Get the bus width from the device node */
> +               bus_width = fdtdec_get_int(blob, node, "bus-width", 0);
> +               if (bus_width < 0) {
> +                       debug("DWMMC: Can't get bus-width\n");
> +                       return -1;
> +               }

+
> +               err = fdtdec_get_int_array(blob, node, "timing",
> +                                       timing, 3);
> +               if (err) {
> +                       debug("Can't get sdr-timings for divider\n");
> +                       return -1;
> +               }
> +
> +               err =  exynos_dwmci_init(base, bus_width,
> +                                               index, timing);
> +               if (err) {
> +                       debug("Can't do dwmci init\n");
> +                       return -1;
> +               }
> +       }
> +
> +       return 0;
> +}
> +#endif
> diff --git a/include/dwmmc.h b/include/dwmmc.h
> index c8b1d40..4a42849 100644
> --- a/include/dwmmc.h
> +++ b/include/dwmmc.h
> @@ -123,6 +123,9 @@
>  #define MSIZE(x)               ((x) << 28)
>  #define RX_WMARK(x)            ((x) << 16)
>  #define TX_WMARK(x)            (x)
> +#define RX_WMARK_SHIFT         16
> +#define RX_WMARK_MASK          (0xfff << RX_WMARK_SHIFT)
> +
>

Extra blank line here?


>
>  #define DWMCI_IDMAC_OWN                (1 << 31)
>  #define DWMCI_IDMAC_CH         (1 << 4)
> @@ -144,6 +147,7 @@ struct dwmci_host {
>         unsigned int bus_hz;
>         int dev_index;
>         int buswidth;
> +       u32 clksel_val;
>         u32 fifoth_val;
>         struct mmc *mmc;
>
> --
> 1.7.0.4
>
>
Regards,
Simon
Amarendra Reddy - Dec. 21, 2012, 5:16 a.m.
Hi Simon,

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

Thanks & Regards
Amarendra Reddy

On 20 December 2012 07:53, Simon Glass <sjg@chromium.org> wrote:

> Hi Amar,
>
> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt@samsung.com> wrote:
>
> > Signed-off-by: Amar <amarendra.xt@samsung.com>
> >
>
> Good to see this patch! Please can you add a short commit message?
>  Oops.. sorry. I will put the commit msg.
>
> > ---
>
>  arch/arm/include/asm/arch-exynos/dwmmc.h |    4 +
> >  drivers/mmc/exynos_dw_mmc.c              |  117
> > ++++++++++++++++++++++++++++--
> >  include/dwmmc.h                          |    4 +
> >  3 files changed, 119 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> > b/arch/arm/include/asm/arch-exynos/dwmmc.h
> > index 8acdf9b..92352df 100644
> > --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> > +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> > @@ -27,6 +27,9 @@
> >  #define DWMCI_SET_DRV_CLK(x)   ((x) << 16)
> >  #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
> >
> > +#ifdef CONFIG_OF_CONTROL
> > +unsigned int exynos_dwmmc_init(const void *blob);
> > +#else
> >  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
> >
> >  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> > @@ -34,3 +37,4 @@ static inline unsigned int exynos_dwmmc_init(int index,
> > int bus_width)
> >         unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
> >         return exynos_dwmci_init(base, bus_width, index);
> >  }
> > +#endif
> > diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> > index 72a31b7..3b3e3e5 100644
> > --- a/drivers/mmc/exynos_dw_mmc.c
> > +++ b/drivers/mmc/exynos_dw_mmc.c
> > @@ -19,23 +19,38 @@
> >   */
> >
> >  #include <common.h>
> > -#include <malloc.h>
> >  #include <dwmmc.h>
> > +#include <fdtdec.h>
> > +#include <libfdt.h>
> > +#include <malloc.h>
> >  #include <asm/arch/dwmmc.h>
> >  #include <asm/arch/clk.h>
> > +#include <asm/arch/pinmux.h>
> > +
> > +#define        DWMMC_MAX_CH_NUM                4
> > +#define        DWMMC_MAX_FREQ                  52000000
> > +#define        DWMMC_MIN_FREQ                  400000
> > +#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
> > +#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
> >
>
> What do these last two values mean? I think clock setting should be done in
> clock.c, not here.
>
> In case of non-dt support, these two values are written into respective
clock selection register-CLKSEL, to select the clock for MMC-channel0 and
MMC-channel2.
Will move clock setting into clock.c and the #defines will be moved into
include/asm/arch/clk.h.

>
> >
> >  static char *EXYNOS_NAME = "EXYNOS DWMMC";
> >
> >  static void exynos_dwmci_clksel(struct dwmci_host *host)
> >  {
> > -       u32 val;
> > -       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> > -               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |
> DWMCI_SET_DIV_RATIO(0);
> > +       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
> > +}
> >
> > -       dwmci_writel(host, DWMCI_CLKSEL, val);
> > +unsigned int exynos_dwmci_get_clk(int dev_index)
> > +{
> > +       return get_mmc_clk(dev_index);
> >  }
> >
> > +#ifdef CONFIG_OF_CONTROL
> > +static int exynos_dwmci_init(u32 regbase, int bus_width,
> > +                                       int index, u32 *timing)
> > +#else
> >  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> > +#endif
> >
>
> I'm really not keen on having the same function with different signatures.
> My first question is whether we need to support operation without
> CONFIG_OF_CONTROL. If so, I suggest having an init routine that takes an
> FDT blob as a parameter, and a separate add_port function which can be
> called by non-FDT-enabled board files. The init routine will call the
> add_port function for each port it finds in the FDT.
>
> I think we need support operation without CONFIG_OF_CONTROL atleast untill
the entire FDT is formed.
Hence I will implement it as per your suggestion.


> Also please can you briefly comment non-trivial functions?
>  OK.
>
> >  {
> >         struct dwmci_host *host = NULL;
> >         host = malloc(sizeof(struct dwmci_host));
> > @@ -44,14 +59,104 @@ int exynos_dwmci_init(u32 regbase, int bus_width,
> int
> > index)
> >                 return 1;
> >         }
> >
> > +       /* set the clock divisor - clk_div_fsys for mmc */
> > +       if (exynos5_mmc_set_clk_div(index)) {
> > +               debug("mmc clock div set failed\n");
> > +               return -1;
> > +       }
> > +
> >         host->name = EXYNOS_NAME;
> >         host->ioaddr = (void *)regbase;
> >         host->buswidth = bus_width;
> > +#ifdef CONFIG_OF_CONTROL
> > +       host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
> > +                               DWMCI_SET_DRV_CLK(timing[1]) |
> > +                               DWMCI_SET_DIV_RATIO(timing[2]));
> > +#else
> > +       if (0 == index)
> > +               host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
> > +       if (2 == index)
> > +               host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
> > +#endif
> >         host->clksel = exynos_dwmci_clksel;
> >         host->dev_index = index;
> > +       host->mmc_clk = exynos_dwmci_get_clk;
> >
> > -       add_dwmci(host, 52000000, 400000);
> > +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
> >
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_OF_CONTROL
> > +unsigned int exynos_dwmmc_init(const void *blob)
> > +{
> > +       u32 base;
> > +       int index, bus_width;
> > +       int node_list[DWMMC_MAX_CH_NUM];
> > +       int err = 0;
> > +       int dev_id, flag;
> > +       u32 timing[3];
> > +       int count, i;
> > +
> > +       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
> > +                               COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
> > +                               DWMMC_MAX_CH_NUM);
> > +
> > +       for (i = 0; i < count; i++) {
> > +               int node = node_list[i];
> > +
> > +               if (node <= 0)
> > +                       continue;
> > +
> > +               /* config pinmux for each mmc channel */
> > +               dev_id = pinmux_decode_periph_id(blob, node);
> > +               if (dev_id == PERIPH_ID_SDMMC0)
> > +                       flag = PINMUX_FLAG_8BIT_MODE;
> > +               else
> > +                       flag = PINMUX_FLAG_NONE;
> >
>
> This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node,
> "samsung,width") == 8.
>
> OK.
>


> > +
> > +               err = exynos_pinmux_config(dev_id, flag);
> > +               if (err) {
> > +                       debug("DWMMC not configured\n");
> > +                       return err;
> > +               }
> > +
> > +               /* Get the base address from the device node */
> > +               base = fdtdec_get_addr(blob, node, "reg");
> > +               if (!base) {
> > +                       debug("DWMMC: Can't get base address\n");
> > +                       return -1;
> > +               }
> > +
> > +               /* Get the channel index from the device node */
> > +               index = fdtdec_get_int(blob, node, "index", 0);
> > +               if (index < 0) {
> > +                       debug("DWMMC: Can't get channel index\n");
> > +                       return -1;
> > +               }
> >
>
> What is this needed for? Doesn't the reg address tell you everything you
> need here?
>  Ok will change accordingly.
>
> > +
> > +               /* Get the bus width from the device node */
> > +               bus_width = fdtdec_get_int(blob, node, "bus-width", 0);
> > +               if (bus_width < 0) {
> > +                       debug("DWMMC: Can't get bus-width\n");
> > +                       return -1;
> > +               }
>
> +
> > +               err = fdtdec_get_int_array(blob, node, "timing",
> > +                                       timing, 3);
> > +               if (err) {
> > +                       debug("Can't get sdr-timings for divider\n");
> > +                       return -1;
> > +               }
> > +
> > +               err =  exynos_dwmci_init(base, bus_width,
> > +                                               index, timing);
> > +               if (err) {
> > +                       debug("Can't do dwmci init\n");
> > +                       return -1;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +#endif
> > diff --git a/include/dwmmc.h b/include/dwmmc.h
> > index c8b1d40..4a42849 100644
> > --- a/include/dwmmc.h
> > +++ b/include/dwmmc.h
> > @@ -123,6 +123,9 @@
> >  #define MSIZE(x)               ((x) << 28)
> >  #define RX_WMARK(x)            ((x) << 16)
> >  #define TX_WMARK(x)            (x)
> > +#define RX_WMARK_SHIFT         16
> > +#define RX_WMARK_MASK          (0xfff << RX_WMARK_SHIFT)
> > +
> >
>
> Extra blank line here?
> Will remove it.
>
> >
> >  #define DWMCI_IDMAC_OWN                (1 << 31)
> >  #define DWMCI_IDMAC_CH         (1 << 4)
> > @@ -144,6 +147,7 @@ struct dwmci_host {
> >         unsigned int bus_hz;
> >         int dev_index;
> >         int buswidth;
> > +       u32 clksel_val;
> >         u32 fifoth_val;
> >         struct mmc *mmc;
> >
> > --
> > 1.7.0.4
> >
> >
> Regards,
> Simon
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>
Jaehoon Chung - Dec. 21, 2012, 9:18 a.m.
On 12/21/2012 02:16 PM, Amarendra Reddy wrote:
> Hi Simon,
> 
> Thanks for your review comments.
> Please find the responses below.
> 
> Thanks & Regards
> Amarendra Reddy
> 
> On 20 December 2012 07:53, Simon Glass <sjg@chromium.org> wrote:
> 
>> Hi Amar,
>>
>> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt@samsung.com> wrote:
>>
>>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>>>
>>
>> Good to see this patch! Please can you add a short commit message?
>>  Oops.. sorry. I will put the commit msg.
>>
>>> ---
>>
>>  arch/arm/include/asm/arch-exynos/dwmmc.h |    4 +
>>>  drivers/mmc/exynos_dw_mmc.c              |  117
>>> ++++++++++++++++++++++++++++--
>>>  include/dwmmc.h                          |    4 +
>>>  3 files changed, 119 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> b/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> index 8acdf9b..92352df 100644
>>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> @@ -27,6 +27,9 @@
>>>  #define DWMCI_SET_DRV_CLK(x)   ((x) << 16)
>>>  #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
>>>
>>> +#ifdef CONFIG_OF_CONTROL
>>> +unsigned int exynos_dwmmc_init(const void *blob);
>>> +#else
>>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>>>
>>>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
>>> @@ -34,3 +37,4 @@ static inline unsigned int exynos_dwmmc_init(int index,
>>> int bus_width)
>>>         unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
>>>         return exynos_dwmci_init(base, bus_width, index);
>>>  }
>>> +#endif
>>> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
>>> index 72a31b7..3b3e3e5 100644
>>> --- a/drivers/mmc/exynos_dw_mmc.c
>>> +++ b/drivers/mmc/exynos_dw_mmc.c
>>> @@ -19,23 +19,38 @@
>>>   */
>>>
>>>  #include <common.h>
>>> -#include <malloc.h>
>>>  #include <dwmmc.h>
>>> +#include <fdtdec.h>
>>> +#include <libfdt.h>
>>> +#include <malloc.h>
>>>  #include <asm/arch/dwmmc.h>
>>>  #include <asm/arch/clk.h>
>>> +#include <asm/arch/pinmux.h>
>>> +
>>> +#define        DWMMC_MAX_CH_NUM                4
>>> +#define        DWMMC_MAX_FREQ                  52000000
>>> +#define        DWMMC_MIN_FREQ                  400000
>>> +#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
>>> +#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
>>>
>>
>> What do these last two values mean? I think clock setting should be done in
>> clock.c, not here.
>>
>> In case of non-dt support, these two values are written into respective
> clock selection register-CLKSEL, to select the clock for MMC-channel0 and
> MMC-channel2.
> Will move clock setting into clock.c and the #defines will be moved into
> include/asm/arch/clk.h.
I didn't want to move into clk.h. Why do you move there?
It is related with CLKSEL register into DesignWare IP.

> 
>>
>>>
>>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
>>>
>>>  static void exynos_dwmci_clksel(struct dwmci_host *host)
>>>  {
>>> -       u32 val;
>>> -       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
>>> -               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |
>> DWMCI_SET_DIV_RATIO(0);
>>> +       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
>>> +}
>>>
>>> -       dwmci_writel(host, DWMCI_CLKSEL, val);
>>> +unsigned int exynos_dwmci_get_clk(int dev_index)
>>> +{
>>> +       return get_mmc_clk(dev_index);
>>>  }
>>>
>>> +#ifdef CONFIG_OF_CONTROL
>>> +static int exynos_dwmci_init(u32 regbase, int bus_width,
>>> +                                       int index, u32 *timing)
>>> +#else
>>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
>>> +#endif
>>>
>>
>> I'm really not keen on having the same function with different signatures.
>> My first question is whether we need to support operation without
>> CONFIG_OF_CONTROL. If so, I suggest having an init routine that takes an
>> FDT blob as a parameter, and a separate add_port function which can be
>> called by non-FDT-enabled board files. The init routine will call the
>> add_port function for each port it finds in the FDT.
>>
>> I think we need support operation without CONFIG_OF_CONTROL atleast untill
> the entire FDT is formed.
> Hence I will implement it as per your suggestion.
> 
> 
>> Also please can you briefly comment non-trivial functions?
>>  OK.
>>
>>>  {
>>>         struct dwmci_host *host = NULL;
>>>         host = malloc(sizeof(struct dwmci_host));
>>> @@ -44,14 +59,104 @@ int exynos_dwmci_init(u32 regbase, int bus_width,
>> int
>>> index)
>>>                 return 1;
>>>         }
>>>
>>> +       /* set the clock divisor - clk_div_fsys for mmc */
>>> +       if (exynos5_mmc_set_clk_div(index)) {
>>> +               debug("mmc clock div set failed\n");
>>> +               return -1;
>>> +       }
>>> +
>>>         host->name = EXYNOS_NAME;
>>>         host->ioaddr = (void *)regbase;
>>>         host->buswidth = bus_width;
>>> +#ifdef CONFIG_OF_CONTROL
>>> +       host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
>>> +                               DWMCI_SET_DRV_CLK(timing[1]) |
>>> +                               DWMCI_SET_DIV_RATIO(timing[2]));
>>> +#else
>>> +       if (0 == index)
>>> +               host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
>>> +       if (2 == index)
>>> +               host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
>>> +#endif
>>>         host->clksel = exynos_dwmci_clksel;
>>>         host->dev_index = index;
>>> +       host->mmc_clk = exynos_dwmci_get_clk;
>>>
>>> -       add_dwmci(host, 52000000, 400000);
>>> +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
>>>
>>>         return 0;
>>>  }
>>>
>>> +#ifdef CONFIG_OF_CONTROL
>>> +unsigned int exynos_dwmmc_init(const void *blob)
>>> +{
>>> +       u32 base;
>>> +       int index, bus_width;
>>> +       int node_list[DWMMC_MAX_CH_NUM];
>>> +       int err = 0;
>>> +       int dev_id, flag;
>>> +       u32 timing[3];
>>> +       int count, i;
>>> +
>>> +       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
>>> +                               COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
>>> +                               DWMMC_MAX_CH_NUM);
>>> +
>>> +       for (i = 0; i < count; i++) {
>>> +               int node = node_list[i];
>>> +
>>> +               if (node <= 0)
>>> +                       continue;
>>> +
>>> +               /* config pinmux for each mmc channel */
>>> +               dev_id = pinmux_decode_periph_id(blob, node);
>>> +               if (dev_id == PERIPH_ID_SDMMC0)
>>> +                       flag = PINMUX_FLAG_8BIT_MODE;
>>> +               else
>>> +                       flag = PINMUX_FLAG_NONE;
>>>
>>
>> This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node,
>> "samsung,width") == 8.
>>
>> OK.
>>
> 
> 
>>> +
>>> +               err = exynos_pinmux_config(dev_id, flag);
>>> +               if (err) {
>>> +                       debug("DWMMC not configured\n");
>>> +                       return err;
>>> +               }
>>> +
>>> +               /* Get the base address from the device node */
>>> +               base = fdtdec_get_addr(blob, node, "reg");
>>> +               if (!base) {
>>> +                       debug("DWMMC: Can't get base address\n");
>>> +                       return -1;
>>> +               }
>>> +
>>> +               /* Get the channel index from the device node */
>>> +               index = fdtdec_get_int(blob, node, "index", 0);
>>> +               if (index < 0) {
>>> +                       debug("DWMMC: Can't get channel index\n");
>>> +                       return -1;
>>> +               }
>>>
>>
>> What is this needed for? Doesn't the reg address tell you everything you
>> need here?
>>  Ok will change accordingly.
>>
>>> +
>>> +               /* Get the bus width from the device node */
>>> +               bus_width = fdtdec_get_int(blob, node, "bus-width", 0);
>>> +               if (bus_width < 0) {
>>> +                       debug("DWMMC: Can't get bus-width\n");
>>> +                       return -1;
>>> +               }
>>
>> +
>>> +               err = fdtdec_get_int_array(blob, node, "timing",
>>> +                                       timing, 3);
>>> +               if (err) {
>>> +                       debug("Can't get sdr-timings for divider\n");
>>> +                       return -1;
>>> +               }
>>> +
>>> +               err =  exynos_dwmci_init(base, bus_width,
>>> +                                               index, timing);
>>> +               if (err) {
>>> +                       debug("Can't do dwmci init\n");
>>> +                       return -1;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +#endif
>>> diff --git a/include/dwmmc.h b/include/dwmmc.h
>>> index c8b1d40..4a42849 100644
>>> --- a/include/dwmmc.h
>>> +++ b/include/dwmmc.h
>>> @@ -123,6 +123,9 @@
>>>  #define MSIZE(x)               ((x) << 28)
>>>  #define RX_WMARK(x)            ((x) << 16)
>>>  #define TX_WMARK(x)            (x)
>>> +#define RX_WMARK_SHIFT         16
>>> +#define RX_WMARK_MASK          (0xfff << RX_WMARK_SHIFT)
>>> +
>>>
>>
>> Extra blank line here?
>> Will remove it.
>>
>>>
>>>  #define DWMCI_IDMAC_OWN                (1 << 31)
>>>  #define DWMCI_IDMAC_CH         (1 << 4)
>>> @@ -144,6 +147,7 @@ struct dwmci_host {
>>>         unsigned int bus_hz;
>>>         int dev_index;
>>>         int buswidth;
>>> +       u32 clksel_val;
>>>         u32 fifoth_val;
>>>         struct mmc *mmc;
>>>
>>> --
>>> 1.7.0.4
>>>
>>>
>> 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
>
Amarendra Reddy - Dec. 24, 2012, 7:20 a.m.
Hi Jaehoon,
Thanks for your comments.
Please find the response below.

Thanks & Regards
Amarendra

On 21 December 2012 14:48, Jaehoon Chung <jh80.chung@samsung.com> wrote:

>  On 12/21/2012 02:16 PM, Amarendra Reddy wrote:
> > Hi Simon,
> >
> > Thanks for your review comments.
> > Please find the responses below.
> >
> > Thanks & Regards
> > Amarendra Reddy
> >
> > On 20 December 2012 07:53, Simon Glass <sjg@chromium.org> wrote:
> >
> >> Hi Amar,
> >>
> >> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra.xt@samsung.com> wrote:
> >>
> >>> Signed-off-by: Amar <amarendra.xt@samsung.com>
> >>>
> >>
> >> Good to see this patch! Please can you add a short commit message?
> >>  Oops.. sorry. I will put the commit msg.
> >>
> >>> ---
> >>
> >>  arch/arm/include/asm/arch-exynos/dwmmc.h |    4 +
> >>>  drivers/mmc/exynos_dw_mmc.c              |  117
> >>> ++++++++++++++++++++++++++++--
> >>>  include/dwmmc.h                          |    4 +
> >>>  3 files changed, 119 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> index 8acdf9b..92352df 100644
> >>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> @@ -27,6 +27,9 @@
> >>>  #define DWMCI_SET_DRV_CLK(x)   ((x) << 16)
> >>>  #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +unsigned int exynos_dwmmc_init(const void *blob);
> >>> +#else
> >>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
> >>>
> >>>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> >>> @@ -34,3 +37,4 @@ static inline unsigned int exynos_dwmmc_init(int
> index,
> >>> int bus_width)
> >>>         unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
> >>>         return exynos_dwmci_init(base, bus_width, index);
> >>>  }
> >>> +#endif
> >>> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> >>> index 72a31b7..3b3e3e5 100644
> >>> --- a/drivers/mmc/exynos_dw_mmc.c
> >>> +++ b/drivers/mmc/exynos_dw_mmc.c
> >>> @@ -19,23 +19,38 @@
> >>>   */
> >>>
> >>>  #include <common.h>
> >>> -#include <malloc.h>
> >>>  #include <dwmmc.h>
> >>> +#include <fdtdec.h>
> >>> +#include <libfdt.h>
> >>> +#include <malloc.h>
> >>>  #include <asm/arch/dwmmc.h>
> >>>  #include <asm/arch/clk.h>
> >>> +#include <asm/arch/pinmux.h>
> >>> +
> >>> +#define        DWMMC_MAX_CH_NUM                4
> >>> +#define        DWMMC_MAX_FREQ                  52000000
> >>> +#define        DWMMC_MIN_FREQ                  400000
> >>> +#define        DWMMC_MMC0_CLKSEL_VAL           0x03030001
> >>> +#define        DWMMC_MMC2_CLKSEL_VAL           0x03020001
> >>>
> >>
> >> What do these last two values mean? I think clock setting should be
> done in
> >> clock.c, not here.
> >>
> >> In case of non-dt support, these two values are written into respective
> > clock selection register-CLKSEL, to select the clock for MMC-channel0 and
> > MMC-channel2.
> > Will move clock setting into clock.c and the #defines will be moved into
> > include/asm/arch/clk.h.
> I didn't want to move into clk.h. Why do you move there?
> It is related with CLKSEL register into DesignWare IP.
>
> Ok, now I understand that all register settings corresponding to
DesignWare IP should be present in exynos_dw_mmc.c / dw_mmc.c.
Fine, I will not move CLKSEL register setting into clock.c / clk.h.


>  >
> >>
> >>>
> >>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
> >>>
> >>>  static void exynos_dwmci_clksel(struct dwmci_host *host)
> >>>  {
> >>> -       u32 val;
> >>> -       val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> >>> -               DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |
> >> DWMCI_SET_DIV_RATIO(0);
> >>> +       dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
> >>> +}
> >>>
> >>> -       dwmci_writel(host, DWMCI_CLKSEL, val);
> >>> +unsigned int exynos_dwmci_get_clk(int dev_index)
> >>> +{
> >>> +       return get_mmc_clk(dev_index);
> >>>  }
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +static int exynos_dwmci_init(u32 regbase, int bus_width,
> >>> +                                       int index, u32 *timing)
> >>> +#else
> >>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> >>> +#endif
> >>>
> >>
> >> I'm really not keen on having the same function with different
> signatures.
> >> My first question is whether we need to support operation without
> >> CONFIG_OF_CONTROL. If so, I suggest having an init routine that takes an
> >> FDT blob as a parameter, and a separate add_port function which can be
> >> called by non-FDT-enabled board files. The init routine will call the
> >> add_port function for each port it finds in the FDT.
> >>
> >> I think we need support operation without CONFIG_OF_CONTROL atleast
> untill
> > the entire FDT is formed.
> > Hence I will implement it as per your suggestion.
> >
> >
> >> Also please can you briefly comment non-trivial functions?
> >>  OK.
> >>
> >>>  {
> >>>         struct dwmci_host *host = NULL;
> >>>         host = malloc(sizeof(struct dwmci_host));
> >>> @@ -44,14 +59,104 @@ int exynos_dwmci_init(u32 regbase, int bus_width,
> >> int
> >>> index)
> >>>                 return 1;
> >>>         }
> >>>
> >>> +       /* set the clock divisor - clk_div_fsys for mmc */
> >>> +       if (exynos5_mmc_set_clk_div(index)) {
> >>> +               debug("mmc clock div set failed\n");
> >>> +               return -1;
> >>> +       }
> >>> +
> >>>         host->name = EXYNOS_NAME;
> >>>         host->ioaddr = (void *)regbase;
> >>>         host->buswidth = bus_width;
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +       host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
> >>> +                               DWMCI_SET_DRV_CLK(timing[1]) |
> >>> +                               DWMCI_SET_DIV_RATIO(timing[2]));
> >>> +#else
> >>> +       if (0 == index)
> >>> +               host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
> >>> +       if (2 == index)
> >>> +               host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
> >>> +#endif
> >>>         host->clksel = exynos_dwmci_clksel;
> >>>         host->dev_index = index;
> >>> +       host->mmc_clk = exynos_dwmci_get_clk;
> >>>
> >>> -       add_dwmci(host, 52000000, 400000);
> >>> +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
> >>>
> >>>         return 0;
> >>>  }
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +unsigned int exynos_dwmmc_init(const void *blob)
> >>> +{
> >>> +       u32 base;
> >>> +       int index, bus_width;
> >>> +       int node_list[DWMMC_MAX_CH_NUM];
> >>> +       int err = 0;
> >>> +       int dev_id, flag;
> >>> +       u32 timing[3];
> >>> +       int count, i;
> >>> +
> >>> +       count = fdtdec_find_aliases_for_id(blob, "dwmmc",
> >>> +                               COMPAT_SAMSUNG_EXYNOS5_DWMMC,
> node_list,
> >>> +                               DWMMC_MAX_CH_NUM);
> >>> +
> >>> +       for (i = 0; i < count; i++) {
> >>> +               int node = node_list[i];
> >>> +
> >>> +               if (node <= 0)
> >>> +                       continue;
> >>> +
> >>> +               /* config pinmux for each mmc channel */
> >>> +               dev_id = pinmux_decode_periph_id(blob, node);
> >>> +               if (dev_id == PERIPH_ID_SDMMC0)
> >>> +                       flag = PINMUX_FLAG_8BIT_MODE;
> >>> +               else
> >>> +                       flag = PINMUX_FLAG_NONE;
> >>>
> >>
> >> This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node,
> >> "samsung,width") == 8.
> >>
> >> OK.
> >>
> >
> >
> >>> +
> >>> +               err = exynos_pinmux_config(dev_id, flag);
> >>> +               if (err) {
> >>> +                       debug("DWMMC not configured\n");
> >>> +                       return err;
> >>> +               }
> >>> +
> >>> +               /* Get the base address from the device node */
> >>> +               base = fdtdec_get_addr(blob, node, "reg");
> >>> +               if (!base) {
> >>> +                       debug("DWMMC: Can't get base address\n");
> >>> +                       return -1;
> >>> +               }
> >>> +
> >>> +               /* Get the channel index from the device node */
> >>> +               index = fdtdec_get_int(blob, node, "index", 0);
> >>> +               if (index < 0) {
> >>> +                       debug("DWMMC: Can't get channel index\n");
> >>> +                       return -1;
> >>> +               }
> >>>
> >>
> >> What is this needed for? Doesn't the reg address tell you everything you
> >> need here?
> >>  Ok will change accordingly.
> >>
> >>> +
> >>> +               /* Get the bus width from the device node */
> >>> +               bus_width = fdtdec_get_int(blob, node, "bus-width", 0);
> >>> +               if (bus_width < 0) {
> >>> +                       debug("DWMMC: Can't get bus-width\n");
> >>> +                       return -1;
> >>> +               }
> >>
> >> +
> >>> +               err = fdtdec_get_int_array(blob, node, "timing",
> >>> +                                       timing, 3);
> >>> +               if (err) {
> >>> +                       debug("Can't get sdr-timings for divider\n");
> >>> +                       return -1;
> >>> +               }
> >>> +
> >>> +               err =  exynos_dwmci_init(base, bus_width,
> >>> +                                               index, timing);
> >>> +               if (err) {
> >>> +                       debug("Can't do dwmci init\n");
> >>> +                       return -1;
> >>> +               }
> >>> +       }
> >>> +
> >>> +       return 0;
> >>> +}
> >>> +#endif
> >>> diff --git a/include/dwmmc.h b/include/dwmmc.h
> >>> index c8b1d40..4a42849 100644
> >>> --- a/include/dwmmc.h
> >>> +++ b/include/dwmmc.h
> >>> @@ -123,6 +123,9 @@
> >>>  #define MSIZE(x)               ((x) << 28)
> >>>  #define RX_WMARK(x)            ((x) << 16)
> >>>  #define TX_WMARK(x)            (x)
> >>> +#define RX_WMARK_SHIFT         16
> >>> +#define RX_WMARK_MASK          (0xfff << RX_WMARK_SHIFT)
> >>> +
> >>>
> >>
> >> Extra blank line here?
> >> Will remove it.
> >>
> >>>
> >>>  #define DWMCI_IDMAC_OWN                (1 << 31)
> >>>  #define DWMCI_IDMAC_CH         (1 << 4)
> >>> @@ -144,6 +147,7 @@ struct dwmci_host {
> >>>         unsigned int bus_hz;
> >>>         int dev_index;
> >>>         int buswidth;
> >>> +       u32 clksel_val;
> >>>         u32 fifoth_val;
> >>>         struct mmc *mmc;
> >>>
> >>> --
> >>> 1.7.0.4
> >>>
> >>>
> >> 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
> >
>
>

Patch

diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h b/arch/arm/include/asm/arch-exynos/dwmmc.h
index 8acdf9b..92352df 100644
--- a/arch/arm/include/asm/arch-exynos/dwmmc.h
+++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
@@ -27,6 +27,9 @@ 
 #define DWMCI_SET_DRV_CLK(x)	((x) << 16)
 #define DWMCI_SET_DIV_RATIO(x)	((x) << 24)
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
 int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
 static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
@@ -34,3 +37,4 @@  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
 	unsigned int base = samsung_get_base_mmc() + (0x10000 * index);
 	return exynos_dwmci_init(base, bus_width, index);
 }
+#endif
diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
index 72a31b7..3b3e3e5 100644
--- a/drivers/mmc/exynos_dw_mmc.c
+++ b/drivers/mmc/exynos_dw_mmc.c
@@ -19,23 +19,38 @@ 
  */
 
 #include <common.h>
-#include <malloc.h>
 #include <dwmmc.h>
+#include <fdtdec.h>
+#include <libfdt.h>
+#include <malloc.h>
 #include <asm/arch/dwmmc.h>
 #include <asm/arch/clk.h>
+#include <asm/arch/pinmux.h>
+
+#define	DWMMC_MAX_CH_NUM		4
+#define	DWMMC_MAX_FREQ			52000000
+#define	DWMMC_MIN_FREQ			400000
+#define	DWMMC_MMC0_CLKSEL_VAL		0x03030001
+#define	DWMMC_MMC2_CLKSEL_VAL		0x03020001
 
 static char *EXYNOS_NAME = "EXYNOS DWMMC";
 
 static void exynos_dwmci_clksel(struct dwmci_host *host)
 {
-	u32 val;
-	val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
-		DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) | DWMCI_SET_DIV_RATIO(0);
+	dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
+}
 
-	dwmci_writel(host, DWMCI_CLKSEL, val);
+unsigned int exynos_dwmci_get_clk(int dev_index)
+{
+	return get_mmc_clk(dev_index);
 }
 
+#ifdef CONFIG_OF_CONTROL
+static int exynos_dwmci_init(u32 regbase, int bus_width,
+					int index, u32 *timing)
+#else
 int exynos_dwmci_init(u32 regbase, int bus_width, int index)
+#endif
 {
 	struct dwmci_host *host = NULL;
 	host = malloc(sizeof(struct dwmci_host));
@@ -44,14 +59,104 @@  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
 		return 1;
 	}
 
+	/* set the clock divisor - clk_div_fsys for mmc */
+	if (exynos5_mmc_set_clk_div(index)) {
+		debug("mmc clock div set failed\n");
+		return -1;
+	}
+
 	host->name = EXYNOS_NAME;
 	host->ioaddr = (void *)regbase;
 	host->buswidth = bus_width;
+#ifdef CONFIG_OF_CONTROL
+	host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
+				DWMCI_SET_DRV_CLK(timing[1]) |
+				DWMCI_SET_DIV_RATIO(timing[2]));
+#else
+	if (0 == index)
+		host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
+	if (2 == index)
+		host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
+#endif
 	host->clksel = exynos_dwmci_clksel;
 	host->dev_index = index;
+	host->mmc_clk = exynos_dwmci_get_clk;
 
-	add_dwmci(host, 52000000, 400000);
+	add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
 
 	return 0;
 }
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob)
+{
+	u32 base;
+	int index, bus_width;
+	int node_list[DWMMC_MAX_CH_NUM];
+	int err = 0;
+	int dev_id, flag;
+	u32 timing[3];
+	int count, i;
+
+	count = fdtdec_find_aliases_for_id(blob, "dwmmc",
+				COMPAT_SAMSUNG_EXYNOS5_DWMMC, node_list,
+				DWMMC_MAX_CH_NUM);
+
+	for (i = 0; i < count; i++) {
+		int node = node_list[i];
+
+		if (node <= 0)
+			continue;
+
+		/* config pinmux for each mmc channel */
+		dev_id = pinmux_decode_periph_id(blob, node);
+		if (dev_id == PERIPH_ID_SDMMC0)
+			flag = PINMUX_FLAG_8BIT_MODE;
+		else
+			flag = PINMUX_FLAG_NONE;
+
+		err = exynos_pinmux_config(dev_id, flag);
+		if (err) {
+			debug("DWMMC not configured\n");
+			return err;
+		}
+
+		/* Get the base address from the device node */
+		base = fdtdec_get_addr(blob, node, "reg");
+		if (!base) {
+			debug("DWMMC: Can't get base address\n");
+			return -1;
+		}
+
+		/* Get the channel index from the device node */
+		index = fdtdec_get_int(blob, node, "index", 0);
+		if (index < 0) {
+			debug("DWMMC: Can't get channel index\n");
+			return -1;
+		}
+
+		/* Get the bus width from the device node */
+		bus_width = fdtdec_get_int(blob, node, "bus-width", 0);
+		if (bus_width < 0) {
+			debug("DWMMC: Can't get bus-width\n");
+			return -1;
+		}
+
+		err = fdtdec_get_int_array(blob, node, "timing",
+					timing, 3);
+		if (err) {
+			debug("Can't get sdr-timings for divider\n");
+			return -1;
+		}
+
+		err =  exynos_dwmci_init(base, bus_width,
+						index, timing);
+		if (err) {
+			debug("Can't do dwmci init\n");
+			return -1;
+		}
+	}
+
+	return 0;
+}
+#endif
diff --git a/include/dwmmc.h b/include/dwmmc.h
index c8b1d40..4a42849 100644
--- a/include/dwmmc.h
+++ b/include/dwmmc.h
@@ -123,6 +123,9 @@ 
 #define MSIZE(x)		((x) << 28)
 #define RX_WMARK(x)		((x) << 16)
 #define TX_WMARK(x)		(x)
+#define RX_WMARK_SHIFT		16
+#define RX_WMARK_MASK		(0xfff << RX_WMARK_SHIFT)
+
 
 #define DWMCI_IDMAC_OWN		(1 << 31)
 #define DWMCI_IDMAC_CH		(1 << 4)
@@ -144,6 +147,7 @@  struct dwmci_host {
 	unsigned int bus_hz;
 	int dev_index;
 	int buswidth;
+	u32 clksel_val;
 	u32 fifoth_val;
 	struct mmc *mmc;