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

login
register
mail settings
Submitter Amar
Date Jan. 4, 2013, 9:34 a.m.
Message ID <1357292050-12137-5-git-send-email-amarendra.xt@samsung.com>
Download mbox | patch
Permalink /patch/209413/
State Superseded
Delegated to: Minkyu Kang
Headers show

Comments

Amar - Jan. 4, 2013, 9:34 a.m.
This patch adds FDT support for DWMMC, by reading the DWMMC node data
from the device tree and initialising DWMMC channels as per data
obtained from the node.

Changes from V1:
        1)Updated code to have same signature for the function
        exynos_dwmci_init() for both FDT and non-FDT versions.
        2)Updated code to pass device_id parameter to the function
        exynos5_mmc_set_clk_div() instead of index.
        3)Updated code to decode the value of "samsung,width" from FDT.
        4)Channel index is computed instead of getting from FDT.

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

Changes from V3:
	1)Replaced the new function exynos5_mmc_set_clk_div() with the
	existing function set_mmc_clk(). set_mmc_clk() will do the purpose.
	2)Computation of FSYS block clock divisor (pre-ratio) is added.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
Signed-off-by: Amar <amarendra.xt@samsung.com>
---
 arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
 drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
 include/dwmmc.h                          |   4 +
 3 files changed, 130 insertions(+), 7 deletions(-)
Simon Glass - Jan. 10, 2013, 3:33 p.m.
Hi Amar,

On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
> This patch adds FDT support for DWMMC, by reading the DWMMC node data
> from the device tree and initialising DWMMC channels as per data
> obtained from the node.
>
> Changes from V1:
>         1)Updated code to have same signature for the function
>         exynos_dwmci_init() for both FDT and non-FDT versions.
>         2)Updated code to pass device_id parameter to the function
>         exynos5_mmc_set_clk_div() instead of index.
>         3)Updated code to decode the value of "samsung,width" from FDT.
>         4)Channel index is computed instead of getting from FDT.
>
> Changes from V2:
>         1)Updation of commit message and resubmition of proper patch set.
>
> Changes from V3:
>         1)Replaced the new function exynos5_mmc_set_clk_div() with the
>         existing function set_mmc_clk(). set_mmc_clk() will do the purpose.
>         2)Computation of FSYS block clock divisor (pre-ratio) is added.
>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> Signed-off-by: Amar <amarendra.xt@samsung.com>
> ---
>  arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
>  drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
>  include/dwmmc.h                          |   4 +
>  3 files changed, 130 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h b/arch/arm/include/asm/arch-exynos/dwmmc.h
> index 8acdf9b..40dcc7b 100644
> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> @@ -29,8 +29,12 @@
>
>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>
> +#ifdef CONFIG_OF_CONTROL
> +unsigned int exynos_dwmmc_init(const void *blob);
> +#else
>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)

Why unsigned?

I'm really not that keen on functions which change their signature
based on an #ifdef. Can we perhaps have

int exynos_dwmmc_init(const void *blob);

which will pass NULL when there is no FDT, and

int exynos_dwmmc_add_port(int index, int bus_width)

for use by non-FDT boards?

>  {
>         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..d7ca7d0 100644
> --- a/drivers/mmc/exynos_dw_mmc.c
> +++ b/drivers/mmc/exynos_dw_mmc.c
> @@ -19,39 +19,154 @@
>   */
>
>  #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
> +#define        ONE_MEGA_HZ                     1000000
> +#define        SCALED_VAL_FOUR_HUNDRED         400

I don't think you need these last two - you can just write the number
in the code

>
>  static char *EXYNOS_NAME = "EXYNOS DWMMC";

Same with this I think

> +u32 timing[3];
>
> +/*
> + * Function used as callback function to initialise the
> + * CLKSEL register for every mmc channel.
> + */
>  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);
>  }
>
>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
>  {
>         struct dwmci_host *host = NULL;
> +       unsigned int clock, div;
>         host = malloc(sizeof(struct dwmci_host));
>         if (!host) {
>                 printf("dwmci_host malloc fail!\n");
>                 return 1;
>         }
>
> +       /*
> +        * The max operating freq of FSYS block is 400MHz.
> +        * Scale down the 400MHz to number 400.
> +        * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
> +        * Arrive at the divisor value taking 400 as the reference.
> +        */
> +
> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
> +
> +       /* Arrive at the divisor value. */
> +       for (div = 0; div <= 0xf; div++) {
> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
> +                       break;
> +       }

What if you don't find the right divisor?
> +
> +       /* set the clock divisor for mmc */
> +       set_mmc_clk(index, div);
> +
>         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]));

timing should be a parameter, not a global. For the non-FDT case
perhaps you can accept NULL, meaning default? Then:

if (timing)
   do the code above
else
   do the code below

> +#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;
> -
> -       add_dwmci(host, 52000000, 400000);
> +       host->mmc_clk = exynos_dwmci_get_clk;
> +       /* Add the mmc chennel to be registered with mmc core */
> +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);

Is error checking needed here?

>
>         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;
> +       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;
> +
> +               /* Extract device id for each mmc channel */
> +               dev_id = pinmux_decode_periph_id(blob, node);
> +
> +               /* Get the bus width from the device node */
> +               bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
> +               if (bus_width < 0) {

<= 0? The function will return 0 if there is no property.

> +                       debug("DWMMC: Can't get bus-width\n");
> +                       return -1;
> +               }
> +               if (8 == bus_width)
> +                       flag = PINMUX_FLAG_8BIT_MODE;
> +               else
> +                       flag = PINMUX_FLAG_NONE;
> +
> +               /* config pinmux for each mmc channel */
> +               err = exynos_pinmux_config(dev_id, flag);
> +               if (err) {
> +                       debug("DWMMC not configured\n");
> +                       return err;
> +               }
> +
> +               index = dev_id - PERIPH_ID_SDMMC0;
> +
> +               /* 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;
> +               }
> +               /* Extract the timing info from the node */
> +               err = fdtdec_get_int_array(blob, node, "samsung,timing",
> +                                       timing, 3);
> +               if (err) {
> +                       debug("Can't get sdr-timings for divider\n");
> +                       return -1;
> +               }
> +               /* Initialise each mmc channel */
> +               err =  exynos_dwmci_init(base, bus_width, index);
> +               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)
> +

Remove this extra line?

>
>  #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.8.0
>
Regards,
Simon
Jaehoon Chung - Jan. 11, 2013, 4:12 a.m.
On 01/11/2013 12:33 AM, Simon Glass wrote:
> Hi Amar,
> 
> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
>> This patch adds FDT support for DWMMC, by reading the DWMMC node data
>> from the device tree and initialising DWMMC channels as per data
>> obtained from the node.
>>
>> Changes from V1:
>>         1)Updated code to have same signature for the function
>>         exynos_dwmci_init() for both FDT and non-FDT versions.
>>         2)Updated code to pass device_id parameter to the function
>>         exynos5_mmc_set_clk_div() instead of index.
>>         3)Updated code to decode the value of "samsung,width" from FDT.
>>         4)Channel index is computed instead of getting from FDT.
>>
>> Changes from V2:
>>         1)Updation of commit message and resubmition of proper patch set.
>>
>> Changes from V3:
>>         1)Replaced the new function exynos5_mmc_set_clk_div() with the
>>         existing function set_mmc_clk(). set_mmc_clk() will do the purpose.
>>         2)Computation of FSYS block clock divisor (pre-ratio) is added.
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> ---
>>  arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
>>  drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
>>  include/dwmmc.h                          |   4 +
>>  3 files changed, 130 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h b/arch/arm/include/asm/arch-exynos/dwmmc.h
>> index 8acdf9b..40dcc7b 100644
>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
>> @@ -29,8 +29,12 @@
>>
>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>>
>> +#ifdef CONFIG_OF_CONTROL
>> +unsigned int exynos_dwmmc_init(const void *blob);
>> +#else
>>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> 
> Why unsigned?
> 
> I'm really not that keen on functions which change their signature
> based on an #ifdef. Can we perhaps have
> 
> int exynos_dwmmc_init(const void *blob);
> 
> which will pass NULL when there is no FDT, and
> 
> int exynos_dwmmc_add_port(int index, int bus_width)
> 
> for use by non-FDT boards?
> 
>>  {
>>         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..d7ca7d0 100644
>> --- a/drivers/mmc/exynos_dw_mmc.c
>> +++ b/drivers/mmc/exynos_dw_mmc.c
>> @@ -19,39 +19,154 @@
>>   */
>>
>>  #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
>> +#define        ONE_MEGA_HZ                     1000000
>> +#define        SCALED_VAL_FOUR_HUNDRED         400
> 
> I don't think you need these last two - you can just write the number
> in the code
Why didn't add into the dwmmc.h?
> 
>>
>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
> 
> Same with this I think
Sorry..What means? Also need not?
> 
>> +u32 timing[3];
>>
>> +/*
>> + * Function used as callback function to initialise the
>> + * CLKSEL register for every mmc channel.
>> + */
>>  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);
>>  }
>>
>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
>>  {
>>         struct dwmci_host *host = NULL;
>> +       unsigned int clock, div;
>>         host = malloc(sizeof(struct dwmci_host));
>>         if (!host) {
>>                 printf("dwmci_host malloc fail!\n");
>>                 return 1;
>>         }
>>
>> +       /*
>> +        * The max operating freq of FSYS block is 400MHz.
>> +        * Scale down the 400MHz to number 400.
>> +        * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
>> +        * Arrive at the divisor value taking 400 as the reference.
>> +        */
>> +
>> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
>> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
>> +
>> +       /* Arrive at the divisor value. */
>> +       for (div = 0; div <= 0xf; div++) {
>> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
>> +                       break;
>> +       }
> 
> What if you don't find the right divisor?
i want to use like this.

sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value
div = DIV_ROUND_UP(sclk, freq); => freq is request clock value.
mmc_set_clk(index, div);

Then we can set to div value at clock register.
It didn't need to add this code...
>> +
>> +       /* set the clock divisor for mmc */
>> +       set_mmc_clk(index, div);
>> +
>>         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]));
> 
> timing should be a parameter, not a global. For the non-FDT case
> perhaps you can accept NULL, meaning default? Then:
> 
> if (timing)
>    do the code above
> else
>    do the code below
> 
>> +#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;
>> -
>> -       add_dwmci(host, 52000000, 400000);
>> +       host->mmc_clk = exynos_dwmci_get_clk;
>> +       /* Add the mmc chennel to be registered with mmc core */
>> +       add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
> 
> Is error checking needed here?
> 
>>
>>         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;
>> +       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;
>> +
>> +               /* Extract device id for each mmc channel */
>> +               dev_id = pinmux_decode_periph_id(blob, node);
>> +
>> +               /* Get the bus width from the device node */
>> +               bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
>> +               if (bus_width < 0) {
> 
> <= 0? The function will return 0 if there is no property.
> 
>> +                       debug("DWMMC: Can't get bus-width\n");
>> +                       return -1;
>> +               }
>> +               if (8 == bus_width)
>> +                       flag = PINMUX_FLAG_8BIT_MODE;
>> +               else
>> +                       flag = PINMUX_FLAG_NONE;
>> +
>> +               /* config pinmux for each mmc channel */
>> +               err = exynos_pinmux_config(dev_id, flag);
>> +               if (err) {
>> +                       debug("DWMMC not configured\n");
>> +                       return err;
>> +               }
>> +
>> +               index = dev_id - PERIPH_ID_SDMMC0;
>> +
>> +               /* 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;
>> +               }
>> +               /* Extract the timing info from the node */
>> +               err = fdtdec_get_int_array(blob, node, "samsung,timing",
>> +                                       timing, 3);
>> +               if (err) {
>> +                       debug("Can't get sdr-timings for divider\n");
>> +                       return -1;
>> +               }
>> +               /* Initialise each mmc channel */
>> +               err =  exynos_dwmci_init(base, bus_width, index);
>> +               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)
>> +
> 
> Remove this extra line?
> 
>>
>>  #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.8.0
>>
> Regards,
> Simon
>
Simon Glass - Jan. 11, 2013, 5:44 a.m.
Hi Jaehoon,

On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 01/11/2013 12:33 AM, Simon Glass wrote:
>> Hi Amar,
>>
>> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
>>> This patch adds FDT support for DWMMC, by reading the DWMMC node data
>>> from the device tree and initialising DWMMC channels as per data
>>> obtained from the node.
>>>
>>> Changes from V1:
>>>         1)Updated code to have same signature for the function
>>>         exynos_dwmci_init() for both FDT and non-FDT versions.
>>>         2)Updated code to pass device_id parameter to the function
>>>         exynos5_mmc_set_clk_div() instead of index.
>>>         3)Updated code to decode the value of "samsung,width" from FDT.
>>>         4)Channel index is computed instead of getting from FDT.
>>>
>>> Changes from V2:
>>>         1)Updation of commit message and resubmition of proper patch set.
>>>
>>> Changes from V3:
>>>         1)Replaced the new function exynos5_mmc_set_clk_div() with the
>>>         existing function set_mmc_clk(). set_mmc_clk() will do the purpose.
>>>         2)Computation of FSYS block clock divisor (pre-ratio) is added.
>>>
>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>>> ---
>>>  arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
>>>  drivers/mmc/exynos_dw_mmc.c              | 129 +++++++++++++++++++++++++++++--
>>>  include/dwmmc.h                          |   4 +
>>>  3 files changed, 130 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h b/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> index 8acdf9b..40dcc7b 100644
>>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
>>> @@ -29,8 +29,12 @@
>>>
>>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>>>
>>> +#ifdef CONFIG_OF_CONTROL
>>> +unsigned int exynos_dwmmc_init(const void *blob);
>>> +#else
>>>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
>>
>> Why unsigned?
>>
>> I'm really not that keen on functions which change their signature
>> based on an #ifdef. Can we perhaps have
>>
>> int exynos_dwmmc_init(const void *blob);
>>
>> which will pass NULL when there is no FDT, and
>>
>> int exynos_dwmmc_add_port(int index, int bus_width)
>>
>> for use by non-FDT boards?
>>
>>>  {
>>>         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..d7ca7d0 100644
>>> --- a/drivers/mmc/exynos_dw_mmc.c
>>> +++ b/drivers/mmc/exynos_dw_mmc.c
>>> @@ -19,39 +19,154 @@
>>>   */
>>>
>>>  #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
>>> +#define        ONE_MEGA_HZ                     1000000
>>> +#define        SCALED_VAL_FOUR_HUNDRED         400
>>
>> I don't think you need these last two - you can just write the number
>> in the code
> Why didn't add into the dwmmc.h?
>>
>>>
>>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
>>
>> Same with this I think
> Sorry..What means? Also need not?

Yes I mean that you probably don't need this - just put the string in the code.

>>
>>> +u32 timing[3];
>>>
>>> +/*
>>> + * Function used as callback function to initialise the
>>> + * CLKSEL register for every mmc channel.
>>> + */
>>>  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);
>>>  }
>>>
>>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
>>>  {
>>>         struct dwmci_host *host = NULL;
>>> +       unsigned int clock, div;
>>>         host = malloc(sizeof(struct dwmci_host));
>>>         if (!host) {
>>>                 printf("dwmci_host malloc fail!\n");
>>>                 return 1;
>>>         }
>>>
>>> +       /*
>>> +        * The max operating freq of FSYS block is 400MHz.
>>> +        * Scale down the 400MHz to number 400.
>>> +        * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
>>> +        * Arrive at the divisor value taking 400 as the reference.
>>> +        */
>>> +
>>> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
>>> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
>>> +
>>> +       /* Arrive at the divisor value. */
>>> +       for (div = 0; div <= 0xf; div++) {
>>> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
>>> +                       break;
>>> +       }
>>
>> What if you don't find the right divisor?
> i want to use like this.
>
> sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value
> div = DIV_ROUND_UP(sclk, freq); => freq is request clock value.
> mmc_set_clk(index, div);
>
> Then we can set to div value at clock register.
> It didn't need to add this code...

OK, sounds good.

Regards,
Simon
Amarendra Reddy - Jan. 11, 2013, 1:06 p.m.
Hi Simon / Jaehoon,

Thanks for review comments.
Please find the responses below.

Thanks & Regards
Amarendra Reddy

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

> Hi Jaehoon,
>
> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung@samsung.com>
> wrote:
> > On 01/11/2013 12:33 AM, Simon Glass wrote:
> >> Hi Amar,
> >>
> >> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
> >>> This patch adds FDT support for DWMMC, by reading the DWMMC node data
> >>> from the device tree and initialising DWMMC channels as per data
> >>> obtained from the node.
> >>>
> >>> Changes from V1:
> >>>         1)Updated code to have same signature for the function
> >>>         exynos_dwmci_init() for both FDT and non-FDT versions.
> >>>         2)Updated code to pass device_id parameter to the function
> >>>         exynos5_mmc_set_clk_div() instead of index.
> >>>         3)Updated code to decode the value of "samsung,width" from FDT.
> >>>         4)Channel index is computed instead of getting from FDT.
> >>>
> >>> Changes from V2:
> >>>         1)Updation of commit message and resubmition of proper patch
> set.
> >>>
> >>> Changes from V3:
> >>>         1)Replaced the new function exynos5_mmc_set_clk_div() with the
> >>>         existing function set_mmc_clk(). set_mmc_clk() will do the
> purpose.
> >>>         2)Computation of FSYS block clock divisor (pre-ratio) is added.
> >>>
> >>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >>> Signed-off-by: Amar <amarendra.xt@samsung.com>
> >>> ---
> >>>  arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
> >>>  drivers/mmc/exynos_dw_mmc.c              | 129
> +++++++++++++++++++++++++++++--
> >>>  include/dwmmc.h                          |   4 +
> >>>  3 files changed, 130 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> index 8acdf9b..40dcc7b 100644
> >>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> @@ -29,8 +29,12 @@
> >>>
> >>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index);
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +unsigned int exynos_dwmmc_init(const void *blob);
> >>> +#else
> >>>  static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> >>
> >> Why unsigned?
>
Ok, shall replace "unsigned int exynos_dwmmc_init(int index, int
bus_width)" with
int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32
clksel).
 Regarding the parameter *'clksel':*
i) "timing" value shall be passed in case of FDT, to be written into CLKSEL
register.
ii) NULL will be passed in case of non-FDT.


>  >>
> >> I'm really not that keen on functions which change their signature
> >> based on an #ifdef. Can we perhaps have
> >>
> >> int exynos_dwmmc_init(const void *blob);
> >>
> >> which will pass NULL when there is no FDT, and
> >>
> >> int exynos_dwmmc_add_port(int index, int bus_width)
> >>
> >> for use by non-FDT boards?
>

Ok. I will call the function int exynos_dwmmc_init(NULL) for non-FDT and
int exynos_dwmmc_init(const void *blob) for FDT.
And use "int exynos_dwmci_add_port(int index, u32 regbase, int bus_width,
u32 clksel)".


>  >>
> >>>  {
> >>>         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..d7ca7d0 100644
> >>> --- a/drivers/mmc/exynos_dw_mmc.c
> >>> +++ b/drivers/mmc/exynos_dw_mmc.c
> >>> @@ -19,39 +19,154 @@
> >>>   */
> >>>
> >>>  #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
> >>> +#define        ONE_MEGA_HZ                     1000000
> >>> +#define        SCALED_VAL_FOUR_HUNDRED         400
> >>
> >> I don't think you need these last two - you can just write the number
> >> in the code
> > Why didn't add into the dwmmc.h?
>

Ok, will just write the number in the code.

>  >>
> >>>
> >>>  static char *EXYNOS_NAME = "EXYNOS DWMMC";
> >>
> >> Same with this I think
> > Sorry..What means? Also need not?
>
> Yes I mean that you probably don't need this - just put the string in the
> code.
>
Ok.

>
> >>
> >>> +u32 timing[3];
> >>>
> >>> +/*
> >>> + * Function used as callback function to initialise the
> >>> + * CLKSEL register for every mmc channel.
> >>> + */
> >>>  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);
> >>>  }
> >>>
> >>>  int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> >>>  {
> >>>         struct dwmci_host *host = NULL;
> >>> +       unsigned int clock, div;
> >>>         host = malloc(sizeof(struct dwmci_host));
> >>>         if (!host) {
> >>>                 printf("dwmci_host malloc fail!\n");
> >>>                 return 1;
> >>>         }
> >>>
> >>> +       /*
> >>> +        * The max operating freq of FSYS block is 400MHz.
> >>> +        * Scale down the 400MHz to number 400.
> >>> +        * Scale down the MPLL clock by dividing MPLL_CLK with
> ONE_MEGA_HZ.
> >>> +        * Arrive at the divisor value taking 400 as the reference.
> >>> +        */
> >>> +
> >>> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
> >>> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
> >>> +
> >>> +       /* Arrive at the divisor value. */
> >>> +       for (div = 0; div <= 0xf; div++) {
> >>> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
> >>> +                       break;
> >>> +       }
> >>
> >> What if you don't find the right divisor?
> > i want to use like this.
> >
> > sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value
> > div = DIV_ROUND_UP(sclk, freq); => freq is request clock value.
> > mmc_set_clk(index, div);
> >
> > Then we can set to div value at clock register.
> > It didn't need to add this code...
>
> OK, sounds good.
>
> Ok, shall implement as suggested by Jaehoon.


>  Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Joonyoung Shim - Jan. 22, 2013, 5:23 a.m.
On 01/11/2013 10:06 PM, Amarendra Reddy wrote:
> Hi Simon / Jaehoon,
>
> Thanks for review comments.
> Please find the responses below.
>
> Thanks & Regards
> Amarendra Reddy
>
> On 11 January 2013 11:14, Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Jaehoon,
>>
>> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung@samsung.com>
>> wrote:
>>> On 01/11/2013 12:33 AM, Simon Glass wrote:
>>>> Hi Amar,
>>>>
>>>> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
>>>>> This patch adds FDT support for DWMMC, by reading the DWMMC node data
>>>>> from the device tree and initialising DWMMC channels as per data
>>>>> obtained from the node.
>>>>>
>>>>> Changes from V1:
>>>>>          1)Updated code to have same signature for the function
>>>>>          exynos_dwmci_init() for both FDT and non-FDT versions.
>>>>>          2)Updated code to pass device_id parameter to the function
>>>>>          exynos5_mmc_set_clk_div() instead of index.
>>>>>          3)Updated code to decode the value of "samsung,width" from FDT.
>>>>>          4)Channel index is computed instead of getting from FDT.
>>>>>
>>>>> Changes from V2:
>>>>>          1)Updation of commit message and resubmition of proper patch
>> set.
>>>>> Changes from V3:
>>>>>          1)Replaced the new function exynos5_mmc_set_clk_div() with the
>>>>>          existing function set_mmc_clk(). set_mmc_clk() will do the
>> purpose.
>>>>>          2)Computation of FSYS block clock divisor (pre-ratio) is added.
>>>>>
>>>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>>>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>>>>> ---
>>>>>   arch/arm/include/asm/arch-exynos/dwmmc.h |   4 +
>>>>>   drivers/mmc/exynos_dw_mmc.c              | 129
>> +++++++++++++++++++++++++++++--
>>>>>   include/dwmmc.h                          |   4 +
>>>>>   3 files changed, 130 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
>> b/arch/arm/include/asm/arch-exynos/dwmmc.h
>>>>> index 8acdf9b..40dcc7b 100644
>>>>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
>>>>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
>>>>> @@ -29,8 +29,12 @@
>>>>>
>>>>>   int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>>>>>
>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>> +unsigned int exynos_dwmmc_init(const void *blob);
>>>>> +#else
>>>>>   static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
>>>> Why unsigned?
> Ok, shall replace "unsigned int exynos_dwmmc_init(int index, int
> bus_width)" with
> int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32
> clksel).
>   Regarding the parameter *'clksel':*
> i) "timing" value shall be passed in case of FDT, to be written into CLKSEL
> register.
> ii) NULL will be passed in case of non-FDT.
>
>
>>   >>
>>>> I'm really not that keen on functions which change their signature
>>>> based on an #ifdef. Can we perhaps have
>>>>
>>>> int exynos_dwmmc_init(const void *blob);
>>>>
>>>> which will pass NULL when there is no FDT, and
>>>>
>>>> int exynos_dwmmc_add_port(int index, int bus_width)
>>>>
>>>> for use by non-FDT boards?
> Ok. I will call the function int exynos_dwmmc_init(NULL) for non-FDT and
> int exynos_dwmmc_init(const void *blob) for FDT.
> And use "int exynos_dwmci_add_port(int index, u32 regbase, int bus_width,
> u32 clksel)".
>

But patch v5 doesn't use exynos_dwmmc_init(NULL) and it uses 
exynos_dwmci_add_port directly in board file.

Why we need blob argument in exynos_dwmmc_init? Already spi_init() just 
uses gd->fdt_blob directly of drivers/spi/exynos_spi.c
I think exynos_dwmmc_init function needs a struct argument for int index 
and int bus_width such follows.

struct exynos_dwmmc_config {
     int index;
     int bus_width;
};

exynos_dwmmc_init(struct exynos_dwmmc_config *config)
{
...
}

If config is NULL and gd->fdt_blob isn't NULL, we can get index and 
bus_width from blob.

Thanks.


>>   >>
>>>>>   {
>>>>>          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..d7ca7d0 100644
>>>>> --- a/drivers/mmc/exynos_dw_mmc.c
>>>>> +++ b/drivers/mmc/exynos_dw_mmc.c
>>>>> @@ -19,39 +19,154 @@
>>>>>    */
>>>>>
>>>>>   #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
>>>>> +#define        ONE_MEGA_HZ                     1000000
>>>>> +#define        SCALED_VAL_FOUR_HUNDRED         400
>>>> I don't think you need these last two - you can just write the number
>>>> in the code
>>> Why didn't add into the dwmmc.h?
> Ok, will just write the number in the code.
>
>>   >>
>>>>>   static char *EXYNOS_NAME = "EXYNOS DWMMC";
>>>> Same with this I think
>>> Sorry..What means? Also need not?
>> Yes I mean that you probably don't need this - just put the string in the
>> code.
>>
> Ok.
>
>>>>> +u32 timing[3];
>>>>>
>>>>> +/*
>>>>> + * Function used as callback function to initialise the
>>>>> + * CLKSEL register for every mmc channel.
>>>>> + */
>>>>>   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);
>>>>>   }
>>>>>
>>>>>   int exynos_dwmci_init(u32 regbase, int bus_width, int index)
>>>>>   {
>>>>>          struct dwmci_host *host = NULL;
>>>>> +       unsigned int clock, div;
>>>>>          host = malloc(sizeof(struct dwmci_host));
>>>>>          if (!host) {
>>>>>                  printf("dwmci_host malloc fail!\n");
>>>>>                  return 1;
>>>>>          }
>>>>>
>>>>> +       /*
>>>>> +        * The max operating freq of FSYS block is 400MHz.
>>>>> +        * Scale down the 400MHz to number 400.
>>>>> +        * Scale down the MPLL clock by dividing MPLL_CLK with
>> ONE_MEGA_HZ.
>>>>> +        * Arrive at the divisor value taking 400 as the reference.
>>>>> +        */
>>>>> +
>>>>> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
>>>>> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
>>>>> +
>>>>> +       /* Arrive at the divisor value. */
>>>>> +       for (div = 0; div <= 0xf; div++) {
>>>>> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
>>>>> +                       break;
>>>>> +       }
>>>> What if you don't find the right divisor?
>>> i want to use like this.
>>>
>>> sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value
>>> div = DIV_ROUND_UP(sclk, freq); => freq is request clock value.
>>> mmc_set_clk(index, div);
>>>
>>> Then we can set to div value at clock register.
>>> It didn't need to add this code...
>> OK, sounds good.
>>
>> Ok, shall implement as suggested by Jaehoon.
>
>>   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 - Jan. 22, 2013, 6:41 a.m.
Hi Joonyoung,
Thanks for the comments.
Please find my response below.

Thanks & Regards
Amarendra

On 22 January 2013 10:53, Joonyoung Shim <jy0922.shim@samsung.com> wrote:

>  On 01/11/2013 10:06 PM, Amarendra Reddy wrote:
>
>>  Hi Simon / Jaehoon,
>>
>> Thanks for review comments.
>> Please find the responses below.
>>
>> Thanks & Regards
>> Amarendra Reddy
>>
>> On 11 January 2013 11:14, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Jaehoon,
>>>
>>> On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung@samsung.com>
>>> wrote:
>>>
>>>> On 01/11/2013 12:33 AM, Simon Glass wrote:
>>>>
>>>>> Hi Amar,
>>>>>
>>>>> On Fri, Jan 4, 2013 at 1:34 AM, Amar <amarendra.xt@samsung.com> wrote:
>>>>>
>>>>>> This patch adds FDT support for DWMMC, by reading the DWMMC node data
>>>>>> from the device tree and initialising DWMMC channels as per data
>>>>>> obtained from the node.
>>>>>>
>>>>>> Changes from V1:
>>>>>>          1)Updated code to have same signature for the function
>>>>>>          exynos_dwmci_init() for both FDT and non-FDT versions.
>>>>>>          2)Updated code to pass device_id parameter to the function
>>>>>>          exynos5_mmc_set_clk_div() instead of index.
>>>>>>          3)Updated code to decode the value of "samsung,width" from
>>>>>> FDT.
>>>>>>          4)Channel index is computed instead of getting from FDT.
>>>>>>
>>>>>> Changes from V2:
>>>>>>          1)Updation of commit message and resubmition of proper patch
>>>>>>
>>>>> set.
>>>
>>>>  Changes from V3:
>>>>>>          1)Replaced the new function exynos5_mmc_set_clk_div() with
>>>>>> the
>>>>>>          existing function set_mmc_clk(). set_mmc_clk() will do the
>>>>>>
>>>>> purpose.
>>>
>>>>           2)Computation of FSYS block clock divisor (pre-ratio) is
>>>>>> added.
>>>>>>
>>>>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>>>>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>>>>>> ---
>>>>>>   arch/arm/include/asm/arch-**exynos/dwmmc.h |   4 +
>>>>>>   drivers/mmc/exynos_dw_mmc.c              | 129
>>>>>>
>>>>> +++++++++++++++++++++++++++++-**-
>>>
>>>>    include/dwmmc.h                          |   4 +
>>>>>>   3 files changed, 130 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/include/asm/arch-**exynos/dwmmc.h
>>>>>>
>>>>> b/arch/arm/include/asm/arch-**exynos/dwmmc.h
>>>
>>>>  index 8acdf9b..40dcc7b 100644
>>>>>> --- a/arch/arm/include/asm/arch-**exynos/dwmmc.h
>>>>>> +++ b/arch/arm/include/asm/arch-**exynos/dwmmc.h
>>>>>> @@ -29,8 +29,12 @@
>>>>>>
>>>>>>   int exynos_dwmci_init(u32 regbase, int bus_width, int index);
>>>>>>
>>>>>> +#ifdef CONFIG_OF_CONTROL
>>>>>> +unsigned int exynos_dwmmc_init(const void *blob);
>>>>>> +#else
>>>>>>   static inline unsigned int exynos_dwmmc_init(int index, int
>>>>>> bus_width)
>>>>>>
>>>>> Why unsigned?
>>>>>
>>>> Ok, shall replace "unsigned int exynos_dwmmc_init(int index, int
>> bus_width)" with
>> int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32
>> clksel).
>>   Regarding the parameter *'clksel':*
>>
>> i) "timing" value shall be passed in case of FDT, to be written into
>> CLKSEL
>> register.
>> ii) NULL will be passed in case of non-FDT.
>>
>>
>>   >>
>>>
>>>> I'm really not that keen on functions which change their signature
>>>>> based on an #ifdef. Can we perhaps have
>>>>>
>>>>> int exynos_dwmmc_init(const void *blob);
>>>>>
>>>>> which will pass NULL when there is no FDT, and
>>>>>
>>>>> int exynos_dwmmc_add_port(int index, int bus_width)
>>>>>
>>>>> for use by non-FDT boards?
>>>>>
>>>> Ok. I will call the function int exynos_dwmmc_init(NULL) for non-FDT and
>> int exynos_dwmmc_init(const void *blob) for FDT.
>> And use "int exynos_dwmci_add_port(int index, u32 regbase, int bus_width,
>> u32 clksel)".
>>
>>
> But patch v5 doesn't use exynos_dwmmc_init(NULL) and it uses
> exynos_dwmci_add_port directly in board file.
>
> The initial thought was to use
a) exynos_dwmmc_init(const void *blob) for FDT  and
b) exynos_dwmmc_init(NULL) for non-FDT

But there were review comments from Simon, stating that
"exynos_dwmmc_add_port() should go in the board file, since without an FDT
the driver can't know what ports to init".
Only the board file knows the port numbers.
Hence exynos_dwmmc_init(NULL) is not used in non-FDT case.

Please find below comments from Simon, regarding the same
***********************************************************************
>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.
**********************************************************************


> Why we need blob argument in exynos_dwmmc_init? Already spi_init() just
> uses gd->fdt_blob directly of drivers/spi/exynos_spi.c
>
Yes, in case of spi_init(), there is no explicit mention (hard code) of
port numbers, bus_width etc.
But for dwmmc, in case of non-FDT, we need to hard code port number and
bus_width and this is done in board file.


> I think exynos_dwmmc_init function needs a struct argument for int index
> and int bus_width such follows.
>
> struct exynos_dwmmc_config {
>     int index;
>     int bus_width;
> };
>
> exynos_dwmmc_init(struct exynos_dwmmc_config *config)
> {
> ...
> }
>
> If config is NULL and gd->fdt_blob isn't NULL, we can get index and
> bus_width from blob.
>
> Yes, this is a good approach.
Also in near future the non-FDT part may be removed, retaining only the FDT
part.
Please comment whether to add this new approach by using 'struct
exynos_dwmmc_config', I can add in next patch.


> Thanks.
>
>
>
>    >>
>>>
>>>>    {
>>>>>>          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..d7ca7d0 100644
>>>>>> --- a/drivers/mmc/exynos_dw_mmc.c
>>>>>> +++ b/drivers/mmc/exynos_dw_mmc.c
>>>>>> @@ -19,39 +19,154 @@
>>>>>>    */
>>>>>>
>>>>>>   #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
>>>>>> +#define        ONE_MEGA_HZ                     1000000
>>>>>> +#define        SCALED_VAL_FOUR_HUNDRED         400
>>>>>>
>>>>> I don't think you need these last two - you can just write the number
>>>>> in the code
>>>>>
>>>> Why didn't add into the dwmmc.h?
>>>>
>>> Ok, will just write the number in the code.
>>
>>   >>
>>>
>>>>    static char *EXYNOS_NAME = "EXYNOS DWMMC";
>>>>>>
>>>>> Same with this I think
>>>>>
>>>> Sorry..What means? Also need not?
>>>>
>>> Yes I mean that you probably don't need this - just put the string in the
>>> code.
>>>
>>> Ok.
>>
>>   +u32 timing[3];
>>>>>>
>>>>>> +/*
>>>>>> + * Function used as callback function to initialise the
>>>>>> + * CLKSEL register for every mmc channel.
>>>>>> + */
>>>>>>   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);
>>>>>>   }
>>>>>>
>>>>>>   int exynos_dwmci_init(u32 regbase, int bus_width, int index)
>>>>>>   {
>>>>>>          struct dwmci_host *host = NULL;
>>>>>> +       unsigned int clock, div;
>>>>>>          host = malloc(sizeof(struct dwmci_host));
>>>>>>          if (!host) {
>>>>>>                  printf("dwmci_host malloc fail!\n");
>>>>>>                  return 1;
>>>>>>          }
>>>>>>
>>>>>> +       /*
>>>>>> +        * The max operating freq of FSYS block is 400MHz.
>>>>>> +        * Scale down the 400MHz to number 400.
>>>>>> +        * Scale down the MPLL clock by dividing MPLL_CLK with
>>>>>>
>>>>> ONE_MEGA_HZ.
>>>
>>>>  +        * Arrive at the divisor value taking 400 as the reference.
>>>>>> +        */
>>>>>> +
>>>>>> +       /* get mpll clock and divide it by ONE_MEGA_HZ */
>>>>>> +       clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
>>>>>> +
>>>>>> +       /* Arrive at the divisor value. */
>>>>>> +       for (div = 0; div <= 0xf; div++) {
>>>>>> +               if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
>>>>>> +                       break;
>>>>>> +       }
>>>>>>
>>>>> What if you don't find the right divisor?
>>>>>
>>>> i want to use like this.
>>>>
>>>> sclk = mmc_get_clk(); -> then we can get the FSYS1 clock value
>>>> div = DIV_ROUND_UP(sclk, freq); => freq is request clock value.
>>>> mmc_set_clk(index, div);
>>>>
>>>> Then we can set to div value at clock register.
>>>> It didn't need to add this code...
>>>>
>>> OK, sounds good.
>>>
>>> Ok, shall implement as suggested by Jaehoon.
>>>
>>
>>   Regards,
>>> Simon
>>> ______________________________**_________________
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> http://lists.denx.de/mailman/**listinfo/u-boot<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<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..40dcc7b 100644
--- a/arch/arm/include/asm/arch-exynos/dwmmc.h
+++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
@@ -29,8 +29,12 @@ 
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index);
 
+#ifdef CONFIG_OF_CONTROL
+unsigned int exynos_dwmmc_init(const void *blob);
+#else
 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..d7ca7d0 100644
--- a/drivers/mmc/exynos_dw_mmc.c
+++ b/drivers/mmc/exynos_dw_mmc.c
@@ -19,39 +19,154 @@ 
  */
 
 #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
+#define	ONE_MEGA_HZ			1000000
+#define	SCALED_VAL_FOUR_HUNDRED		400
 
 static char *EXYNOS_NAME = "EXYNOS DWMMC";
+u32 timing[3];
 
+/*
+ * Function used as callback function to initialise the
+ * CLKSEL register for every mmc channel.
+ */
 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);
 }
 
 int exynos_dwmci_init(u32 regbase, int bus_width, int index)
 {
 	struct dwmci_host *host = NULL;
+	unsigned int clock, div;
 	host = malloc(sizeof(struct dwmci_host));
 	if (!host) {
 		printf("dwmci_host malloc fail!\n");
 		return 1;
 	}
 
+	/*
+	 * The max operating freq of FSYS block is 400MHz.
+	 * Scale down the 400MHz to number 400.
+	 * Scale down the MPLL clock by dividing MPLL_CLK with ONE_MEGA_HZ.
+	 * Arrive at the divisor value taking 400 as the reference.
+	 */
+
+	/* get mpll clock and divide it by ONE_MEGA_HZ */
+	clock = get_pll_clk(MPLL) / ONE_MEGA_HZ;
+
+	/* Arrive at the divisor value. */
+	for (div = 0; div <= 0xf; div++) {
+		if ((clock / (div + 1)) <= SCALED_VAL_FOUR_HUNDRED)
+			break;
+	}
+
+	/* set the clock divisor for mmc */
+	set_mmc_clk(index, div);
+
 	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;
-
-	add_dwmci(host, 52000000, 400000);
+	host->mmc_clk = exynos_dwmci_get_clk;
+	/* Add the mmc chennel to be registered with mmc core */
+	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;
+	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;
+
+		/* Extract device id for each mmc channel */
+		dev_id = pinmux_decode_periph_id(blob, node);
+
+		/* Get the bus width from the device node */
+		bus_width = fdtdec_get_int(blob, node, "samsung,bus-width", 0);
+		if (bus_width < 0) {
+			debug("DWMMC: Can't get bus-width\n");
+			return -1;
+		}
+		if (8 == bus_width)
+			flag = PINMUX_FLAG_8BIT_MODE;
+		else
+			flag = PINMUX_FLAG_NONE;
+
+		/* config pinmux for each mmc channel */
+		err = exynos_pinmux_config(dev_id, flag);
+		if (err) {
+			debug("DWMMC not configured\n");
+			return err;
+		}
+
+		index = dev_id - PERIPH_ID_SDMMC0;
+
+		/* 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;
+		}
+		/* Extract the timing info from the node */
+		err = fdtdec_get_int_array(blob, node, "samsung,timing",
+					timing, 3);
+		if (err) {
+			debug("Can't get sdr-timings for divider\n");
+			return -1;
+		}
+		/* Initialise each mmc channel */
+		err =  exynos_dwmci_init(base, bus_width, index);
+		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;