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

[Simon Glass]

Hi Jaehoon,

On Thu, Jan 10, 2013 at 8:12 PM, Jaehoon Chung <jh80.chung at 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 at 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 at samsung.com>
>>> Signed-off-by: Amar <amarendra.xt at 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